From 8aa187b39328494eeb24e7608163693551b43faf Mon Sep 17 00:00:00 2001 From: Andrey Pshenkin Date: Tue, 12 Aug 2025 13:12:10 +0300 Subject: [PATCH] [V2] add url validation for BrowserOpenURL (#4484) * add url validation for BrowserOpenURL * update changelog * don't process invalid urls * address AI issues * added more validations and sanitization --- .../frontend/desktop/darwin/browser.go | 10 +- v2/internal/frontend/desktop/linux/browser.go | 13 +- .../frontend/desktop/windows/browser.go | 12 +- v2/internal/frontend/utils/urlValidator.go | 58 +++++ .../frontend/utils/urlValidator_test.go | 207 ++++++++++++++++++ website/src/pages/changelog.mdx | 1 + 6 files changed, 296 insertions(+), 5 deletions(-) create mode 100644 v2/internal/frontend/utils/urlValidator.go create mode 100644 v2/internal/frontend/utils/urlValidator_test.go diff --git a/v2/internal/frontend/desktop/darwin/browser.go b/v2/internal/frontend/desktop/darwin/browser.go index 12b2bc8fc..c865ab6d9 100644 --- a/v2/internal/frontend/desktop/darwin/browser.go +++ b/v2/internal/frontend/desktop/darwin/browser.go @@ -4,11 +4,19 @@ package darwin import ( + "fmt" "github.com/pkg/browser" + "github.com/wailsapp/wails/v2/internal/frontend/utils" ) // BrowserOpenURL Use the default browser to open the url -func (f *Frontend) BrowserOpenURL(url string) { +func (f *Frontend) BrowserOpenURL(rawURL string) { + url, err := utils.ValidateAndSanitizeURL(rawURL) + if err != nil { + f.logger.Error(fmt.Sprintf("Invalid URL %s", err.Error())) + return + } + // Specific method implementation if err := browser.OpenURL(url); err != nil { f.logger.Error("Unable to open default system browser") diff --git a/v2/internal/frontend/desktop/linux/browser.go b/v2/internal/frontend/desktop/linux/browser.go index 30ca9620c..962e3b28a 100644 --- a/v2/internal/frontend/desktop/linux/browser.go +++ b/v2/internal/frontend/desktop/linux/browser.go @@ -3,10 +3,19 @@ package linux -import "github.com/pkg/browser" +import ( + "fmt" + "github.com/pkg/browser" + "github.com/wailsapp/wails/v2/internal/frontend/utils" +) // BrowserOpenURL Use the default browser to open the url -func (f *Frontend) BrowserOpenURL(url string) { +func (f *Frontend) BrowserOpenURL(rawURL string) { + url, err := utils.ValidateAndSanitizeURL(rawURL) + if err != nil { + f.logger.Error(fmt.Sprintf("Invalid URL %s", err.Error())) + return + } // Specific method implementation if err := browser.OpenURL(url); err != nil { f.logger.Error("Unable to open default system browser") diff --git a/v2/internal/frontend/desktop/windows/browser.go b/v2/internal/frontend/desktop/windows/browser.go index 2b058feda..13d037b14 100644 --- a/v2/internal/frontend/desktop/windows/browser.go +++ b/v2/internal/frontend/desktop/windows/browser.go @@ -4,7 +4,9 @@ package windows import ( + "fmt" "github.com/pkg/browser" + "github.com/wailsapp/wails/v2/internal/frontend/utils" "golang.org/x/sys/windows" ) @@ -16,9 +18,15 @@ var fallbackBrowserPaths = []string{ } // BrowserOpenURL Use the default browser to open the url -func (f *Frontend) BrowserOpenURL(url string) { +func (f *Frontend) BrowserOpenURL(rawURL string) { + url, err := utils.ValidateAndSanitizeURL(rawURL) + if err != nil { + f.logger.Error(fmt.Sprintf("Invalid URL %s", err.Error())) + return + } + // Specific method implementation - err := browser.OpenURL(url) + err = browser.OpenURL(url) if err == nil { return } diff --git a/v2/internal/frontend/utils/urlValidator.go b/v2/internal/frontend/utils/urlValidator.go new file mode 100644 index 000000000..76ba216ce --- /dev/null +++ b/v2/internal/frontend/utils/urlValidator.go @@ -0,0 +1,58 @@ +package utils + +import ( + "errors" + "fmt" + "net/url" + "regexp" + "strings" +) + +func ValidateAndSanitizeURL(rawURL string) (string, error) { + // Check for null bytes (can cause truncation issues in some systems) + if strings.Contains(rawURL, "\x00") { + return "", errors.New("null bytes not allowed in URL") + } + + // Parse URL first - this handles most malformed URLs + parsedURL, err := url.Parse(rawURL) + if err != nil { + return "", fmt.Errorf("invalid URL format: %v", err) + } + + scheme := strings.ToLower(parsedURL.Scheme) + + if scheme == "javascript" || scheme == "data" || scheme == "file" || scheme == "ftp" || scheme == "" { + return "", errors.New("scheme not allowed") + } + + // Ensure there's actually a host for http/https URLs + if (scheme == "http" || scheme == "https") && parsedURL.Host == "" { + return "", fmt.Errorf("missing host for %s URL", scheme) + } + + sanitizedURL := parsedURL.String() + + // Check for control characters that might cause issues + // (but allow legitimate URL characters like &, ;, etc.) + for i, r := range sanitizedURL { + // Block control characters except tab, but allow other printable chars + if r < 32 && r != 9 { // 9 is tab, which might be legitimate + return "", fmt.Errorf("control character at position %d not allowed", i) + } + } + + // Shell metacharacter check + shellDangerous := `[;\|` + "`" + `$\\<>*{}\[\]()~! \t\n\r]` + if matched, _ := regexp.MatchString(shellDangerous, sanitizedURL); matched { + return "", errors.New("shell metacharacters not allowed") + } + + // Unicode danger check + unicodeDangerous := "[\u0000-\u001F\u007F\u00A0\u1680\u2000-\u200F\u2028-\u202F\u205F\u2060\u3000\uFEFF]" + if matched, _ := regexp.MatchString(unicodeDangerous, sanitizedURL); matched { + return "", errors.New("unicode dangerous characters not allowed") + } + + return sanitizedURL, nil +} diff --git a/v2/internal/frontend/utils/urlValidator_test.go b/v2/internal/frontend/utils/urlValidator_test.go new file mode 100644 index 000000000..b385ccec1 --- /dev/null +++ b/v2/internal/frontend/utils/urlValidator_test.go @@ -0,0 +1,207 @@ +package utils_test + +import ( + "strings" + "testing" + + "github.com/wailsapp/wails/v2/internal/frontend/utils" +) + +// Test cases for ValidateAndOpenURL +func TestValidateURL(t *testing.T) { + testCases := []struct { + name string + url string + shouldErr bool + errMsg string + expected string + }{ + // Valid URLs + { + name: "valid https URL", + url: "https://www.example.com", + shouldErr: false, + expected: "https://www.example.com", + }, + { + name: "valid http URL", + url: "http://example.com", + shouldErr: false, + expected: "http://example.com", + }, + { + name: "URL with query parameters", + url: "https://example.com/search?q=cats&dogs", + shouldErr: false, + expected: "https://example.com/search?q=cats&dogs", + }, + { + name: "URL with port", + url: "https://example.com:8080/path", + shouldErr: false, + expected: "https://example.com:8080/path", + }, + { + name: "URL with fragment", + url: "https://example.com/page#section", + shouldErr: false, + expected: "https://example.com/page#section", + }, + { + name: "urlencode params", + url: "http://google.com/ ----browser-subprocess-path=C:\\\\Users\\\\Public\\\\test.bat", + shouldErr: false, + expected: "http://google.com/%20----browser-subprocess-path=C:%5C%5CUsers%5C%5CPublic%5C%5Ctest.bat", + }, + + // Invalid schemes + { + name: "javascript scheme", + url: "javascript:alert('xss')", + shouldErr: true, + errMsg: "scheme not allowed", + }, + { + name: "data scheme", + url: "data:text/html,", + shouldErr: true, + errMsg: "scheme not allowed", + }, + { + name: "file scheme", + url: "file:///etc/passwd", + shouldErr: true, + errMsg: "scheme not allowed", + }, + { + name: "ftp scheme", + url: "ftp://files.example.com/file.txt", + shouldErr: true, + errMsg: "scheme not allowed", + }, + + // Malformed URLs + { + name: "not a URL", + url: "not-a-url", + shouldErr: true, + errMsg: "scheme not allowed", // will have empty scheme + }, + { + name: "missing scheme", + url: "example.com", + shouldErr: true, + errMsg: "scheme not allowed", + }, + { + name: "malformed URL", + url: "https://", + shouldErr: true, + errMsg: "missing host", + }, + { + name: "empty host", + url: "http:///path", + shouldErr: true, + errMsg: "missing host", + }, + + // Security issues + { + name: "null byte in URL", + url: "https://example.com\x00/hidden", + shouldErr: true, + errMsg: "null bytes not allowed", + }, + { + name: "control characters", + url: "https://example.com\n/path", + shouldErr: true, + errMsg: "control character", + }, + { + name: "carriage return", + url: "https://example.com\r/path", + shouldErr: true, + errMsg: "control character", + }, + { + name: "URL with tab character", + url: "https://example.com/path?q=hello\tworld", + shouldErr: true, + errMsg: "control character", + }, + { + name: "URL with path parameters", + url: "https://example.com/path;param=value", + shouldErr: true, + errMsg: "shell metacharacters not allowed", + }, + { + name: "URL with special characters in query", + url: "https://example.com/search?q=hello world&filter=price>100", + shouldErr: true, + errMsg: "shell metacharacters not allowed", + }, + { + name: "URL with special characters in query and params", + url: "https://example.com/search?q=hello ----browser-subprocess-path=C:\\\\Users\\\\Public\\\\test.bat", + shouldErr: true, + errMsg: "shell metacharacters not allowed", + }, + { + name: "URL with dollar sign in query", + url: "https://example.com/search?price=$100", + shouldErr: true, + errMsg: "shell metacharacters not allowed", + }, + { + name: "URL with parentheses", + url: "https://example.com/file(1).html", + shouldErr: true, + errMsg: "shell metacharacters not allowed", + }, + { + name: "URL with unicode", + url: "https://example.com/search?q=hello\u2001foo", + shouldErr: true, + errMsg: "unicode dangerous characters not allowed", + }, + + // Edge cases + { + name: "international domain", + url: "https://例え.テスト/path", + shouldErr: false, + expected: "https://%E4%BE%8B%E3%81%88.%E3%83%86%E3%82%B9%E3%83%88/path", + }, + { + name: "URL with pipe character", + url: "https://example.com/user/123|admin", + shouldErr: false, + expected: "https://example.com/user/123%7Cadmin", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // We'll test only the validation part to avoid actually opening URLs + sanitized, err := utils.ValidateAndSanitizeURL(tc.url) + + if tc.shouldErr { + if err == nil { + t.Errorf("expected error for URL %q, but got none", tc.url) + } else if tc.errMsg != "" && !strings.Contains(err.Error(), tc.errMsg) { + t.Errorf("expected error containing %q, got %q", tc.errMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("expected no error for URL %q, but got: %v", tc.url, err) + } + if sanitized != tc.expected { + t.Errorf("unexpected sanitized URL for %q: expected %q, got %q", tc.url, tc.expected, sanitized) + } + } + }) + } +} diff --git a/website/src/pages/changelog.mdx b/website/src/pages/changelog.mdx index fa1ef4024..6e3dbc7ef 100644 --- a/website/src/pages/changelog.mdx +++ b/website/src/pages/changelog.mdx @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added `build:tags` to project specification for automatically adding compilation tags by @symball in [PR](https://github.com/wailsapp/wails/pull/4439) ### Fixed +- Added url validation for BrowserOpenURL by @APshenkin in [PR](https://github.com/wailsapp/wails/pull/4484) - Fixed C compilation error in onWayland on Linux due to declaration after label [#4446](https://github.com/wailsapp/wails/pull/4446) by [@jaesung9507](https://github.com/jaesung9507) - Use computed style when adding 'wails-drop-target-active' [PR](https://github.com/wailsapp/wails/pull/4420) by [@riannucci](https://github.com/riannucci)