diff --git a/cmd/goose/github.go b/cmd/goose/github.go index d7e17c4..983aafc 100644 --- a/cmd/goose/github.go +++ b/cmd/goose/github.go @@ -462,10 +462,12 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u needsReview := false isBlocked := false actionReason := "" + actionKind := "" if action, exists := result.turnData.Analysis.NextAction[user]; exists { needsReview = true isBlocked = action.Critical // Only critical actions are blocking actionReason = action.Reason + actionKind = string(action.Kind) // Only log fresh API calls if !result.wasFromCache { slog.Debug("[TURN] NextAction", "url", result.url, "reason", action.Reason, "kind", action.Kind, "critical", action.Critical) @@ -479,6 +481,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u (*outgoing)[i].NeedsReview = needsReview (*outgoing)[i].IsBlocked = isBlocked (*outgoing)[i].ActionReason = actionReason + (*outgoing)[i].ActionKind = actionKind break } } @@ -487,6 +490,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u if (*incoming)[i].URL == result.url { (*incoming)[i].NeedsReview = needsReview (*incoming)[i].ActionReason = actionReason + (*incoming)[i].ActionKind = actionKind break } } diff --git a/cmd/goose/main.go b/cmd/goose/main.go index fb061b2..e7ec3d9 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -57,6 +57,7 @@ type PR struct { URL string Repository string ActionReason string + ActionKind string // The kind of action expected (review, merge, fix_tests, etc.) Number int IsBlocked bool NeedsReview bool @@ -675,12 +676,12 @@ func (app *App) tryAutoOpenPR(ctx context.Context, pr PR, autoBrowserEnabled boo slog.Warn("Auto-open strict validation failed", "url", sanitizeForLog(pr.URL), "error", err) return } - if err := openURL(ctx, pr.URL); err != nil { + if err := openURL(ctx, pr.URL, pr.ActionKind); err != nil { slog.Error("[BROWSER] Failed to auto-open PR", "url", pr.URL, "error", err) } else { app.browserRateLimiter.RecordOpen(pr.URL) slog.Info("[BROWSER] Successfully opened PR in browser", - "repo", pr.Repository, "number", pr.Number) + "repo", pr.Repository, "number", pr.Number, "action", pr.ActionKind) } } } diff --git a/cmd/goose/security.go b/cmd/goose/security.go index 04fc557..6b9f836 100644 --- a/cmd/goose/security.go +++ b/cmd/goose/security.go @@ -124,17 +124,17 @@ func validateURL(rawURL string) error { // 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=1 parameter which we add for tracking. +// 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=1 parameter if present for pattern validation + // Strip the ?goose parameter if present for pattern validation urlToValidate := rawURL - if strings.HasSuffix(rawURL, "?goose=1") { - urlToValidate = strings.TrimSuffix(rawURL, "?goose=1") + if idx := strings.Index(rawURL, "?goose="); idx != -1 { + urlToValidate = rawURL[:idx] } // Check against strict GitHub PR URL pattern @@ -149,9 +149,17 @@ func validateGitHubPRURL(rawURL string) error { } // Reject URLs with URL encoding (could hide malicious content) - // Exception: %3D which is = in URL encoding, only as part of ?goose=1 - if strings.Contains(rawURL, "%") && !strings.HasSuffix(rawURL, "?goose%3D1") { - return errors.New("URL contains encoded characters") + // 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 + if idx := strings.Index(rawURL, "?goose="); idx != -1 { + // Check if encoding is only in the goose parameter + if strings.Contains(rawURL[:idx], "%") { + return errors.New("URL contains encoded characters outside goose parameter") + } + } else { + return errors.New("URL contains encoded characters") + } } // Reject URLs with fragments @@ -159,9 +167,16 @@ func validateGitHubPRURL(rawURL string) error { return errors.New("URL contains fragments") } - // Allow only ?goose=1 query parameter, nothing else - if strings.Contains(rawURL, "?") && !strings.HasSuffix(rawURL, "?goose=1") && !strings.HasSuffix(rawURL, "?goose%3D1") { - return errors.New("URL contains unexpected query parameters") + // 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:) diff --git a/cmd/goose/security_test.go b/cmd/goose/security_test.go index 0840805..d5439c3 100644 --- a/cmd/goose/security_test.go +++ b/cmd/goose/security_test.go @@ -21,6 +21,26 @@ func TestValidateGitHubPRURL(t *testing.T) { url: "https://github.com/owner/repo/pull/123?goose=1", wantErr: false, }, + { + name: "valid PR URL with goose=review param", + url: "https://github.com/owner/repo/pull/123?goose=review", + wantErr: false, + }, + { + name: "valid PR URL with goose=merge param", + url: "https://github.com/owner/repo/pull/123?goose=merge", + wantErr: false, + }, + { + name: "valid PR URL with goose=fix_tests param", + url: "https://github.com/owner/repo/pull/123?goose=fix_tests", + wantErr: false, + }, + { + name: "valid PR URL with goose=resolve_comments param", + url: "https://github.com/owner/repo/pull/123?goose=resolve_comments", + wantErr: false, + }, { name: "valid PR URL with hyphen in owner", url: "https://github.com/owner-name/repo/pull/1", @@ -65,6 +85,11 @@ func TestValidateGitHubPRURL(t *testing.T) { }, // Invalid URLs - wrong format + { + name: "URL with multiple query parameters", + url: "https://github.com/owner/repo/pull/123?goose=review&other=param", + wantErr: true, + }, { name: "not a PR URL", url: "https://github.com/owner/repo", diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 4ec7d0f..2d82f46 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -20,7 +20,8 @@ import ( var _ *systray.MenuItem = nil // openURL safely opens a URL in the default browser after validation. -func openURL(ctx context.Context, rawURL string) error { +// action parameter is optional and specifies the expected action type for tracking. +func openURL(ctx context.Context, rawURL string, action string) error { // Parse and validate the URL u, err := url.Parse(rawURL) if err != nil { @@ -45,10 +46,15 @@ func openURL(ctx context.Context, rawURL string) error { return errors.New("URLs with user info are not allowed") } - // Add goose=1 parameter to track source for GitHub and dash URLs + // Add goose parameter to track source and action for GitHub and dash URLs if u.Host == "github.com" || u.Host == "www.github.com" || u.Host == "dash.ready-to-review.dev" { q := u.Query() - q.Set("goose", "1") + // Use action if provided, otherwise default to "1" for backward compatibility + if action != "" { + q.Set("goose", action) + } else { + q.Set("goose", "1") + } u.RawQuery = q.Encode() rawURL = u.String() } @@ -284,10 +290,11 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, // Create PR menu item item := app.systrayInterface.AddMenuItem(title, tooltip) - // Capture URL to avoid loop variable capture bug + // Capture URL and action to avoid loop variable capture bug prURL := sortedPRs[prIndex].URL + prAction := sortedPRs[prIndex].ActionKind item.Click(func() { - if err := openURL(ctx, prURL); err != nil { + if err := openURL(ctx, prURL, prAction); err != nil { slog.Error("failed to open url", "error", err) } }) @@ -426,7 +433,7 @@ func (app *App) rebuildMenu(ctx context.Context) { // Add error details errorMsg := app.systrayInterface.AddMenuItem(authError, "Click to see setup instructions") errorMsg.Click(func() { - if err := openURL(ctx, "https://cli.github.com/manual/gh_auth_login"); err != nil { + if err := openURL(ctx, "https://cli.github.com/manual/gh_auth_login", ""); err != nil { slog.Error("failed to open setup instructions", "error", err) } }) @@ -536,7 +543,7 @@ func (app *App) rebuildMenu(ctx context.Context) { // Add Web Dashboard link dashboardItem := app.systrayInterface.AddMenuItem("Web Dashboard", "") dashboardItem.Click(func() { - if err := openURL(ctx, "https://dash.ready-to-review.dev/"); err != nil { + if err := openURL(ctx, "https://dash.ready-to-review.dev/", ""); err != nil { slog.Error("failed to open dashboard", "error", err) } }) diff --git a/go.mod b/go.mod index 1f64138..4ca5221 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( require ( git.sr.ht/~jackmordaunt/go-toast v1.1.2 // indirect - github.com/codeGROOVE-dev/prx v0.0.0-20250908203157-0711b3ec5471 // indirect + github.com/codeGROOVE-dev/prx v0.0.0-20250923100916-d2b60be50274 // indirect github.com/esiqveland/notify v0.13.3 // indirect github.com/go-ole/go-ole v1.3.0 // indirect github.com/godbus/dbus/v5 v5.1.0 // indirect diff --git a/go.sum b/go.sum index 602cf07..94ffa72 100644 --- a/go.sum +++ b/go.sum @@ -2,6 +2,8 @@ git.sr.ht/~jackmordaunt/go-toast v1.1.2 h1:/yrfI55LRt1M7H1vkaw+NaH1+L1CDxrqDltwm git.sr.ht/~jackmordaunt/go-toast v1.1.2/go.mod h1:jA4OqHKTQ4AFBdwrSnwnskUIIS3HYzlJSgdzCKqfavo= github.com/codeGROOVE-dev/prx v0.0.0-20250908203157-0711b3ec5471 h1:CbUa70O+iNC9rPk5aoZGs/RZbpmPyfNydv5ncKLdOvk= github.com/codeGROOVE-dev/prx v0.0.0-20250908203157-0711b3ec5471/go.mod h1:7qLbi18baOyS8yO/6/64SBIqtyzSzLFdsDST15NPH3w= +github.com/codeGROOVE-dev/prx v0.0.0-20250923100916-d2b60be50274 h1:9eLzQdOaQEn30279ai3YjNdJOM/efbcYanWC9juAJ+M= +github.com/codeGROOVE-dev/prx v0.0.0-20250923100916-d2b60be50274/go.mod h1:7qLbi18baOyS8yO/6/64SBIqtyzSzLFdsDST15NPH3w= github.com/codeGROOVE-dev/retry v1.2.0 h1:xYpYPX2PQZmdHwuiQAGGzsBm392xIMl4nfMEFApQnu8= github.com/codeGROOVE-dev/retry v1.2.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E= github.com/codeGROOVE-dev/turnclient v0.0.0-20250922145707-664c2dcdf5b8 h1:3088TLJGgxzjM/bR1gafKQ609NMkBNlZe1Fd5SnRrrY=