Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 0 additions & 97 deletions cmd/goose/security.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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=<action> 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=<value> 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
}
6 changes: 2 additions & 4 deletions cmd/goose/security_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
18 changes: 11 additions & 7 deletions pkg/safebrowse/safebrowse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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")
}
Expand All @@ -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] {
Expand Down Expand Up @@ -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 {
Expand Down
Loading