diff --git a/cmd/goose/security.go b/cmd/goose/security.go index 7ec2bf3..cb25394 100644 --- a/cmd/goose/security.go +++ b/cmd/goose/security.go @@ -28,11 +28,6 @@ var ( // New tokens: ghp_ (personal), ghs_ (server), ghr_ (refresh), gho_ (OAuth), ghu_ (user-to-server) followed by base62 chars. // Fine-grained tokens: github_pat_ followed by base62 chars. githubTokenRegex = regexp.MustCompile(`^[a-f0-9]{40}$|^gh[psoru]_[A-Za-z0-9]{36,251}$|^github_pat_[A-Za-z0-9]{82}$`) - - // githubPRURLRegex validates strict GitHub PR URL format for auto-opening. - // Must match: https://github.com/{owner}/{repo}/pull/{number} - // Owner and repo follow GitHub naming rules, number is digits only. - githubPRURLRegex = regexp.MustCompile(`^https://github\.com/[a-zA-Z0-9][a-zA-Z0-9-]{0,38}/[a-zA-Z0-9][a-zA-Z0-9._-]{0,99}/pull/[1-9][0-9]{0,9}$`) ) // validateGitHubUsername validates a GitHub username. @@ -93,95 +88,3 @@ func sanitizeForLog(s string) string { return s } - -// validateURL performs strict validation on URLs. -func validateURL(rawURL string) error { - if rawURL == "" { - return errors.New("URL cannot be empty") - } - - // Check for null bytes or control characters - for _, r := range rawURL { - if r < minPrintableChar || r == deleteChar { - return errors.New("URL contains control characters") - } - } - - // Ensure URL starts with https:// - if !strings.HasPrefix(rawURL, "https://") { - return errors.New("URL must use HTTPS") - } - - // Check for URL length limits - if len(rawURL) > maxURLLength { - return errors.New("URL too long") - } - - return nil -} - -// validateGitHubPRURL performs strict validation for GitHub PR URLs used in auto-opening. -// This ensures the URL follows the exact pattern: https://github.com/{owner}/{repo}/pull/{number} -// with no additional path segments, fragments, or suspicious characters. -// The URL may optionally have ?goose= parameter which we add for tracking. -func validateGitHubPRURL(rawURL string) error { - // First do basic URL validation - if err := validateURL(rawURL); err != nil { - return err - } - - // Strip the ?goose parameter if present for pattern validation - urlToValidate := rawURL - if idx := strings.Index(rawURL, "?goose="); idx != -1 { - urlToValidate = rawURL[:idx] - } - - // Check against strict GitHub PR URL pattern - if !githubPRURLRegex.MatchString(urlToValidate) { - return fmt.Errorf("URL does not match GitHub PR pattern: %s", urlToValidate) - } - - // Additional security checks - // Reject URLs with @ (potential credential injection) - if strings.Contains(rawURL, "@") { - return errors.New("URL contains @ character") - } - - // Reject URLs with URL encoding (could hide malicious content) - // Exception: %3D which is = in URL encoding, only as part of ?goose parameter - if strings.Contains(rawURL, "%") { - // Allow URL encoding only in the goose parameter value - idx := strings.Index(rawURL, "?goose=") - if idx == -1 { - return errors.New("URL contains encoded characters") - } - // Check if encoding is only in the goose parameter - if strings.Contains(rawURL[:idx], "%") { - return errors.New("URL contains encoded characters outside goose parameter") - } - } - - // Reject URLs with fragments - if strings.Contains(rawURL, "#") { - return errors.New("URL contains fragments") - } - - // Allow only ?goose= query parameter, nothing else - if strings.Contains(rawURL, "?") { - // Check if it's the goose parameter - if idx := strings.Index(rawURL, "?goose="); idx == -1 { - return errors.New("URL contains unexpected query parameters") - } - // Ensure no additional parameters after goose - if strings.Contains(rawURL[strings.Index(rawURL, "?goose=")+7:], "&") { - return errors.New("URL contains additional query parameters") - } - } - - // Reject URLs with double slashes (except after https:) - if strings.Contains(rawURL[8:], "//") { - return errors.New("URL contains double slashes") - } - - return nil -} diff --git a/cmd/goose/security_test.go b/cmd/goose/security_test.go index 72d6dd4..fe21b26 100644 --- a/cmd/goose/security_test.go +++ b/cmd/goose/security_test.go @@ -261,11 +261,9 @@ func TestSanitizeForLog(t *testing.T) { if strings.Contains(result, "ghp_") || strings.Contains(result, "github_pat_") { t.Errorf("sanitizeForLog() = %v, still contains sensitive pattern", result) } - } else { + } else if result != tt.input { // Should be unchanged - if result != tt.input { - t.Errorf("sanitizeForLog() = %v, want %v", result, tt.input) - } + t.Errorf("sanitizeForLog() = %v, want %v", result, tt.input) } }) } diff --git a/go.mod b/go.mod index b8f658d..3f01220 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.24.0 require ( github.com/codeGROOVE-dev/retry v1.3.0 - github.com/codeGROOVE-dev/sprinkler v0.0.0-20251105232821-c5aeed50a046 + github.com/codeGROOVE-dev/sprinkler v0.0.0-20251113030909-5962af625370 github.com/codeGROOVE-dev/turnclient v0.0.0-20251107215141-ee43672b3dc7 github.com/energye/systray v1.0.2 github.com/gen2brain/beeep v0.11.1 diff --git a/go.sum b/go.sum index 176dee9..5899684 100644 --- a/go.sum +++ b/go.sum @@ -4,8 +4,8 @@ github.com/codeGROOVE-dev/prx v0.0.0-20251109164430-90488144076d h1:KKt93PVYR9Ug github.com/codeGROOVE-dev/prx v0.0.0-20251109164430-90488144076d/go.mod h1:FEy3gz9IYDXWnKWkoDSL+pWu6rujxbBSrF4w5A8QSK0= github.com/codeGROOVE-dev/retry v1.3.0 h1:/+ipAWRJLL6y1R1vprYo0FSjSBvH6fE5j9LKXjpD54g= github.com/codeGROOVE-dev/retry v1.3.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E= -github.com/codeGROOVE-dev/sprinkler v0.0.0-20251105232821-c5aeed50a046 h1:OsTP4XobXlDHrCD3ClX3W0Uhxay52wRutzqv5wEygdc= -github.com/codeGROOVE-dev/sprinkler v0.0.0-20251105232821-c5aeed50a046/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg= +github.com/codeGROOVE-dev/sprinkler v0.0.0-20251113030909-5962af625370 h1:uYXBDnaRRf4q6X/IWOm6O/cOj57tVkpjfVvwn+SfHp0= +github.com/codeGROOVE-dev/sprinkler v0.0.0-20251113030909-5962af625370/go.mod h1:sOvKRad1kRPAOIUm7spNR3aeVQjtk9VoS8uo6NL4kus= github.com/codeGROOVE-dev/turnclient v0.0.0-20251107215141-ee43672b3dc7 h1:183q0bj2y/9hh/K0HZvDXI6sG7liYSRcQVgFx0GY+UA= github.com/codeGROOVE-dev/turnclient v0.0.0-20251107215141-ee43672b3dc7/go.mod h1:dVS3MlJDgL6WkfurJAyS7I9Fe1yxxoxxarjVifY5bIo= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/pkg/safebrowse/safebrowse.go b/pkg/safebrowse/safebrowse.go index fdf5403..b388694 100644 --- a/pkg/safebrowse/safebrowse.go +++ b/pkg/safebrowse/safebrowse.go @@ -19,7 +19,7 @@ func Open(ctx context.Context, rawURL string) error { if err := validate(rawURL, false); err != nil { return err } - return open(ctx, rawURL) + return openBrowser(ctx, rawURL) } // OpenWithParams validates and opens a URL with query parameters. @@ -60,7 +60,7 @@ func OpenWithParams(ctx context.Context, rawURL string, params map[string]string return err } - return open(ctx, finalURL) + return openBrowser(ctx, finalURL) } // ValidateURL performs strict security validation on a URL. @@ -75,7 +75,11 @@ func ValidateGitHubPRURL(rawURL string) error { } // Additional GitHub PR-specific checks - u, _ := url.Parse(rawURL) // Already validated + u, err := url.Parse(rawURL) + if err != nil { + // Should never happen since we already validated, but check anyway + return fmt.Errorf("parse url: %w", err) + } if u.Host != "github.com" { return errors.New("must be github.com") } @@ -86,7 +90,7 @@ func ValidateGitHubPRURL(rawURL string) error { } // Validate PR number (must start with 1-9) - if len(parts[3]) == 0 || parts[3][0] < '1' || parts[3][0] > '9' { + if parts[3] == "" || parts[3][0] < '1' || parts[3][0] > '9' { return errors.New("PR number must start with 1-9") } for _, c := range parts[3] { @@ -200,15 +204,15 @@ func validateParamString(s string) error { return errors.New("cannot be empty") } for _, r := range s { - if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_') { + if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && (r < '0' || r > '9') && r != '-' && r != '_' { return fmt.Errorf("contains invalid character %q", r) } } return nil } -// open opens a URL in the system browser. -func open(ctx context.Context, rawURL string) error { +// openBrowser opens a URL in the system browser. +func openBrowser(ctx context.Context, rawURL string) error { var cmd *exec.Cmd switch runtime.GOOS { diff --git a/pkg/safebrowse/safebrowse_test.go b/pkg/safebrowse/safebrowse_test.go index 0e4f073..848a517 100644 --- a/pkg/safebrowse/safebrowse_test.go +++ b/pkg/safebrowse/safebrowse_test.go @@ -6,361 +6,224 @@ import ( "testing" ) -func TestValidateURL(t *testing.T) { +func TestValidateURL_ValidURLs(t *testing.T) { tests := []struct { - name string - url string - wantErr bool + name string + url string }{ - // Valid URLs - { - name: "valid GitHub PR URL", - url: "https://github.com/owner/repo/pull/123", - wantErr: false, - }, - { - name: "valid GitHub repo URL", - url: "https://github.com/owner/repo", - wantErr: false, - }, - { - name: "valid dashboard URL", - url: "https://dash.ready-to-review.dev", - wantErr: false, - }, - { - name: "valid URL with path", - url: "https://github.com/owner/repo/pulls", - wantErr: false, - }, - { - name: "valid URL with dots in domain", - url: "https://api.github.com/repos/owner/repo", - wantErr: false, - }, + {"valid GitHub PR URL", "https://github.com/owner/repo/pull/123"}, + {"valid GitHub repo URL", "https://github.com/owner/repo"}, + {"valid dashboard URL", "https://dash.ready-to-review.dev"}, + {"valid URL with path", "https://github.com/owner/repo/pulls"}, + {"valid URL with dots in domain", "https://api.github.com/repos/owner/repo"}, + {"uppercase domain", "https://GITHUB.COM/owner/repo"}, + {"mixed case domain", "https://GitHub.Com/owner/repo"}, + } - // Invalid - basic security - { - name: "empty string", - url: "", - wantErr: true, - }, - { - name: "HTTP instead of HTTPS", - url: "http://github.com/owner/repo", - wantErr: true, - }, - { - name: "FTP scheme", - url: "ftp://example.com/file", - wantErr: true, - }, - { - name: "no scheme", - url: "github.com/owner/repo", - wantErr: true, - }, - { - name: "URL too long", - url: "https://github.com/" + strings.Repeat("a", 3000), - wantErr: true, - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err != nil { + t.Errorf("ValidateURL() error = %v, want nil", err) + } + }) + } +} - // Invalid - percent encoding (blocks %00, %0A, %0D, etc.) - { - name: "percent-encoded null byte", - url: "https://github.com/owner/repo%00", - wantErr: true, - }, - { - name: "percent-encoded newline", - url: "https://github.com/owner/repo%0A", - wantErr: true, - }, - { - name: "percent-encoded carriage return", - url: "https://github.com/owner/repo%0D", - wantErr: true, - }, - { - name: "percent-encoded space", - url: "https://github.com/owner/repo%20", - wantErr: true, - }, - { - name: "percent-encoded slash", - url: "https://github.com/owner%2Frepo", - wantErr: true, - }, +func TestValidateURL_BasicSecurity(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"empty string", ""}, + {"HTTP instead of HTTPS", "http://github.com/owner/repo"}, + {"FTP scheme", "ftp://example.com/file"}, + {"no scheme", "github.com/owner/repo"}, + {"URL too long", "https://github.com/" + strings.Repeat("a", 3000)}, + } - // Invalid - control characters (Windows and Unix attacks) - { - name: "null byte", - url: "https://github.com/owner\x00/repo", - wantErr: true, - }, - { - name: "newline character", - url: "https://github.com/owner/repo\n", - wantErr: true, - }, - { - name: "carriage return", - url: "https://github.com/owner/repo\r", - wantErr: true, - }, - { - name: "tab character", - url: "https://github.com/owner/repo\t", - wantErr: true, - }, - { - name: "vertical tab", - url: "https://github.com/owner/repo\v", - wantErr: true, - }, - { - name: "form feed", - url: "https://github.com/owner/repo\f", - wantErr: true, - }, - { - name: "bell character", - url: "https://github.com/owner/repo\a", - wantErr: true, - }, - { - name: "backspace", - url: "https://github.com/owner/repo\b", - wantErr: true, - }, - { - name: "delete character", - url: "https://github.com/owner/repo\x7F", - wantErr: true, - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} - // Invalid - shell metacharacters (Unix/Windows command injection) - { - name: "semicolon", - url: "https://github.com/owner/repo;ls", - wantErr: true, - }, - { - name: "pipe character", - url: "https://github.com/owner/repo|cat", - wantErr: true, - }, - { - name: "ampersand", - url: "https://github.com/owner/repo&", - wantErr: true, - }, - { - name: "backtick", - url: "https://github.com/owner/repo`whoami`", - wantErr: true, - }, - { - name: "dollar sign", - url: "https://github.com/owner/repo$PATH", - wantErr: true, - }, - { - name: "command substitution", - url: "https://github.com/owner/repo$(whoami)", - wantErr: true, - }, - { - name: "parentheses", - url: "https://github.com/owner/repo()", - wantErr: true, - }, - { - name: "curly braces", - url: "https://github.com/owner/repo{}", - wantErr: true, - }, - { - name: "square brackets", - url: "https://github.com/owner/repo[]", - wantErr: true, - }, - { - name: "less than", - url: "https://github.com/owner/repofile", - wantErr: true, - }, +func TestValidateURL_PercentEncoding(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"percent-encoded null byte", "https://github.com/owner/repo%00"}, + {"percent-encoded newline", "https://github.com/owner/repo%0A"}, + {"percent-encoded carriage return", "https://github.com/owner/repo%0D"}, + {"percent-encoded space", "https://github.com/owner/repo%20"}, + {"percent-encoded slash", "https://github.com/owner%2Frepo"}, + {"double-encoded null", "https://github.com/owner/repo%2500"}, + } - // Invalid - Windows-specific attack vectors - { - name: "Windows path separator backslash", - url: "https://github.com/owner\\repo", - wantErr: true, - }, - { - name: "Windows command separator", - url: "https://github.com/owner/repo&&calc", - wantErr: true, - }, - { - name: "Windows batch variable", - url: "https://github.com/owner/%TEMP%", - wantErr: true, - }, - { - name: "caret character (Windows escape)", - url: "https://github.com/owner/repo^", - wantErr: true, - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} - // Invalid - URL components we don't allow - { - name: "user info in URL", - url: "https://user@github.com/owner/repo", - wantErr: true, - }, - { - name: "password in URL", - url: "https://user:pass@github.com/owner/repo", - wantErr: true, - }, - { - name: "fragment", - url: "https://github.com/owner/repo#section", - wantErr: true, - }, - { - name: "query parameters (must use OpenWithParams)", - url: "https://github.com/owner/repo?foo=bar", - wantErr: true, - }, +func TestValidateURL_ControlCharacters(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"null byte", "https://github.com/owner\x00/repo"}, + {"newline character", "https://github.com/owner/repo\n"}, + {"carriage return", "https://github.com/owner/repo\r"}, + {"tab character", "https://github.com/owner/repo\t"}, + {"vertical tab", "https://github.com/owner/repo\v"}, + {"form feed", "https://github.com/owner/repo\f"}, + {"bell character", "https://github.com/owner/repo\a"}, + {"backspace", "https://github.com/owner/repo\b"}, + {"delete character", "https://github.com/owner/repo\x7F"}, + } - // Invalid - Unicode/IDN attacks - { - name: "Unicode character", - url: "https://github.com/owner/repō", - wantErr: true, - }, - { - name: "IDN homograph attack (Cyrillic)", - url: "https://gіthub.com/owner/repo", // Cyrillic 'і' instead of 'i' - wantErr: true, - }, - { - name: "Right-to-left override", - url: "https://github.com/owner/repo\u202E", - wantErr: true, - }, - { - name: "Zero-width space", - url: "https://github.com/owner\u200B/repo", - wantErr: true, - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} - // Invalid - double encoding attacks - { - name: "double-encoded null", - url: "https://github.com/owner/repo%2500", - wantErr: true, - }, +func TestValidateURL_ShellMetacharacters(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"semicolon", "https://github.com/owner/repo;ls"}, + {"pipe character", "https://github.com/owner/repo|cat"}, + {"ampersand", "https://github.com/owner/repo&"}, + {"backtick", "https://github.com/owner/repo`whoami`"}, + {"dollar sign", "https://github.com/owner/repo$PATH"}, + {"command substitution", "https://github.com/owner/repo$(whoami)"}, + {"parentheses", "https://github.com/owner/repo()"}, + {"curly braces", "https://github.com/owner/repo{}"}, + {"square brackets", "https://github.com/owner/repo[]"}, + {"less than", "https://github.com/owner/repofile"}, + } - // Invalid - path traversal - { - name: "dot dot slash", - url: "https://github.com/../etc/passwd", - wantErr: true, - }, - { - name: "double slash in path", - url: "https://github.com//owner/repo", - wantErr: true, - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} - // Invalid - special characters - { - name: "single quote", - url: "https://github.com/owner'/repo", - wantErr: true, - }, - { - name: "double quote", - url: "https://github.com/owner\"/repo", - wantErr: true, - }, - { - name: "plus sign", - url: "https://github.com/owner+org/repo", - wantErr: true, - }, - { - name: "at sign", - url: "https://github.com/owner@org/repo", - wantErr: true, - }, - { - name: "asterisk", - url: "https://github.com/owner*/repo", - wantErr: true, - }, - { - name: "tilde", - url: "https://github.com/~owner/repo", - wantErr: true, - }, - { - name: "exclamation", - url: "https://github.com/owner!/repo", - wantErr: true, - }, +func TestValidateURL_WindowsAttacks(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"Windows path separator backslash", "https://github.com/owner\\repo"}, + {"Windows command separator", "https://github.com/owner/repo&&calc"}, + {"Windows batch variable", "https://github.com/owner/%TEMP%"}, + {"caret character (Windows escape)", "https://github.com/owner/repo^"}, + } - // Invalid - custom ports (new security fix) - { - name: "custom port 8080", - url: "https://github.com:8080/owner/repo", - wantErr: true, - }, - { - name: "SSH port 22", - url: "https://github.com:22/owner/repo", - wantErr: true, - }, - { - name: "explicit HTTPS port 443", - url: "https://github.com:443/owner/repo", - wantErr: true, - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} - // Invalid - colon in path (new security fix) - { - name: "colon in path", - url: "https://github.com/owner:repo/path", - wantErr: true, - }, +func TestValidateURL_URLComponents(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"user info in URL", "https://user@github.com/owner/repo"}, + {"password in URL", "https://user:pass@github.com/owner/repo"}, + {"fragment", "https://github.com/owner/repo#section"}, + {"query parameters (must use OpenWithParams)", "https://github.com/owner/repo?foo=bar"}, + {"custom port 8080", "https://github.com:8080/owner/repo"}, + {"SSH port 22", "https://github.com:22/owner/repo"}, + {"explicit HTTPS port 443", "https://github.com:443/owner/repo"}, + {"colon in path", "https://github.com/owner:repo/path"}, + } - // Valid - case normalization (should pass) - { - name: "uppercase domain", - url: "https://GITHUB.COM/owner/repo", - wantErr: false, // Now normalized to lowercase - }, - { - name: "mixed case domain", - url: "https://GitHub.Com/owner/repo", - wantErr: false, // Now normalized to lowercase - }, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} + +func TestValidateURL_UnicodeAttacks(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"Unicode character", "https://github.com/owner/repō"}, + {"IDN homograph attack (Cyrillic)", "https://gіthub.com/owner/repo"}, // Cyrillic 'і' instead of 'i' + {"Right-to-left override", "https://github.com/owner/repo\u202E"}, + {"Zero-width space", "https://github.com/owner\u200B/repo"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := ValidateURL(tt.url) - if (err != nil) != tt.wantErr { - t.Errorf("ValidateURL() error = %v, wantErr %v", err, tt.wantErr) + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} + +func TestValidateURL_PathTraversal(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"dot dot slash", "https://github.com/../etc/passwd"}, + {"double slash in path", "https://github.com//owner/repo"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") + } + }) + } +} + +func TestValidateURL_SpecialCharacters(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"single quote", "https://github.com/owner'/repo"}, + {"double quote", "https://github.com/owner\"/repo"}, + {"plus sign", "https://github.com/owner+org/repo"}, + {"at sign", "https://github.com/owner@org/repo"}, + {"asterisk", "https://github.com/owner*/repo"}, + {"tilde", "https://github.com/~owner/repo"}, + {"exclamation", "https://github.com/owner!/repo"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateURL(tt.url); err == nil { + t.Errorf("ValidateURL() error = nil, want error") } }) }