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
4 changes: 4 additions & 0 deletions cmd/goose/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@
cacheHits := 0

for result := range results {
if result.err == nil && result.turnData != nil && result.turnData.Analysis.NextAction != nil {

Check failure on line 453 in cmd/goose/github.go

View workflow job for this annotation

GitHub Actions / golangci-lint

`if result.err == nil && result.turnData != nil && result.turnData.Analysis.NextAction != nil` has complex nested blocks (complexity: 12) (nestif)
turnSuccesses++
if result.wasFromCache {
cacheHits++
Expand All @@ -462,10 +462,12 @@
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)
Expand All @@ -475,10 +477,11 @@
// Update the PR in the slices directly
if result.isOwner {
for i := range *outgoing {
if (*outgoing)[i].URL == result.url {

Check failure on line 480 in cmd/goose/github.go

View workflow job for this annotation

GitHub Actions / golangci-lint

nestingReduce: invert if cond, replace body with `continue`, move old body after the statement (gocritic)
(*outgoing)[i].NeedsReview = needsReview
(*outgoing)[i].IsBlocked = isBlocked
(*outgoing)[i].ActionReason = actionReason
(*outgoing)[i].ActionKind = actionKind
break
}
}
Expand All @@ -487,6 +490,7 @@
if (*incoming)[i].URL == result.url {
(*incoming)[i].NeedsReview = needsReview
(*incoming)[i].ActionReason = actionReason
(*incoming)[i].ActionKind = actionKind
break
}
}
Expand Down
5 changes: 3 additions & 2 deletions cmd/goose/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
URL string
Repository string
ActionReason string
ActionKind string // The kind of action expected (review, merge, fix_tests, etc.)
Number int
IsBlocked bool
NeedsReview bool
Expand Down Expand Up @@ -219,7 +220,7 @@
logPath := filepath.Join(logDir, fmt.Sprintf("goose-%s.log", time.Now().Format("2006-01-02")))
logFile, err := os.OpenFile(logPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600)
if err != nil {
slog.Error("Failed to open log file", "error", err)

Check failure on line 223 in cmd/goose/main.go

View workflow job for this annotation

GitHub Actions / golangci-lint

add-constant: string literal "error" appears, at least, 6 times, create a named constant for it (revive)
} else {
// Update logger to write to both stderr and file
multiHandler := &MultiHandler{
Expand Down Expand Up @@ -675,12 +676,12 @@
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)

Check failure on line 680 in cmd/goose/main.go

View workflow job for this annotation

GitHub Actions / golangci-lint

add-constant: string literal "url" appears, at least, 6 times, create a named constant for it (revive)
} 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)
}
}
}
Expand Down
35 changes: 25 additions & 10 deletions cmd/goose/security.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,17 @@
// 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=<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=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
Expand All @@ -149,19 +149,34 @@
}

// 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 failure on line 155 in cmd/goose/security.go

View workflow job for this annotation

GitHub Actions / golangci-lint

early-return: if c { ... } else { ... return } can be simplified to if !c { ... return } ... (move short variable declaration to its own line if necessary) (revive)
// 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
if strings.Contains(rawURL, "#") {
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=<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:)
Expand Down
25 changes: 25 additions & 0 deletions cmd/goose/security_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
21 changes: 14 additions & 7 deletions cmd/goose/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
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 {
Expand All @@ -45,10 +46,15 @@
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()
}
Expand Down Expand Up @@ -237,7 +243,7 @@
} else {
title = fmt.Sprintf("🪿 %s", title)
slog.Debug("[MENU] Adding goose to incoming PR",
"url", sortedPRs[prIndex].URL,

Check failure on line 246 in cmd/goose/ui.go

View workflow job for this annotation

GitHub Actions / golangci-lint

add-constant: string literal "url" appears, at least, 6 times, create a named constant for it (revive)
"blocked_ago", timeSinceBlocked,
"remaining", blockedPRIconDuration-timeSinceBlocked)
}
Expand Down Expand Up @@ -284,10 +290,11 @@
// 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)
}
})
Expand Down Expand Up @@ -426,7 +433,7 @@
// 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)
}
})
Expand Down Expand Up @@ -536,7 +543,7 @@
// 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)
}
})
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
Loading