diff --git a/README.md b/README.md index 3fb320b..657ee53 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ Lives in your menubar like a tiny waterfowl of productivity shame, watching your - **✈️ Jet sounds** when your own PR is ready for the next stage - **🧠 Smart turn-based assignment** - knows who is blocking a PR, knows when tests are failing, etc. - **⭐ Auto-start** on login (macOS) +- **🔔 Auto-open** incoming PRs in your browser (off by default, rate-limited) You can also visit the web-based equivalent at https://dash.ready-to-review.dev/ diff --git a/cmd/goose/browser_rate_limiter.go b/cmd/goose/browser_rate_limiter.go new file mode 100644 index 0000000..56ae4af --- /dev/null +++ b/cmd/goose/browser_rate_limiter.go @@ -0,0 +1,119 @@ +// Package main implements browser rate limiting for PR auto-open feature. +package main + +import ( + "log" + "sync" + "time" +) + +// BrowserRateLimiter manages rate limiting for automatically opening browser windows. +type BrowserRateLimiter struct { + openedPRs map[string]bool + openedLastMinute []time.Time + openedToday []time.Time + startupDelay time.Duration + maxPerMinute int + maxPerDay int + mu sync.Mutex +} + +// NewBrowserRateLimiter creates a new browser rate limiter. +func NewBrowserRateLimiter(startupDelay time.Duration, maxPerMinute, maxPerDay int) *BrowserRateLimiter { + log.Printf("[BROWSER] Initializing rate limiter: startup_delay=%v, max_per_minute=%d, max_per_day=%d", + startupDelay, maxPerMinute, maxPerDay) + return &BrowserRateLimiter{ + openedLastMinute: make([]time.Time, 0), + openedToday: make([]time.Time, 0), + startupDelay: startupDelay, + maxPerMinute: maxPerMinute, + maxPerDay: maxPerDay, + openedPRs: make(map[string]bool), + } +} + +// CanOpen checks if we can open a browser window according to rate limits. +func (b *BrowserRateLimiter) CanOpen(startTime time.Time, prURL string) bool { + b.mu.Lock() + defer b.mu.Unlock() + + // Check if we've already opened this PR + if b.openedPRs[prURL] { + log.Printf("[BROWSER] Skipping auto-open: PR already opened - %s", prURL) + return false + } + + // Check startup delay + if time.Since(startTime) < b.startupDelay { + log.Printf("[BROWSER] Skipping auto-open: within startup delay period (%v remaining)", + b.startupDelay-time.Since(startTime)) + return false + } + + now := time.Now() + + // Clean old entries + b.cleanOldEntries(now) + + // Check per-minute limit + if len(b.openedLastMinute) >= b.maxPerMinute { + log.Printf("[BROWSER] Rate limit: already opened %d PRs in the last minute (max: %d)", + len(b.openedLastMinute), b.maxPerMinute) + return false + } + + // Check per-day limit + if len(b.openedToday) >= b.maxPerDay { + log.Printf("[BROWSER] Rate limit: already opened %d PRs today (max: %d)", + len(b.openedToday), b.maxPerDay) + return false + } + + return true +} + +// RecordOpen records that a browser window was opened. +func (b *BrowserRateLimiter) RecordOpen(prURL string) { + b.mu.Lock() + defer b.mu.Unlock() + + now := time.Now() + b.openedLastMinute = append(b.openedLastMinute, now) + b.openedToday = append(b.openedToday, now) + b.openedPRs[prURL] = true + + log.Printf("[BROWSER] Recorded browser open for %s (minute: %d/%d, today: %d/%d)", + prURL, len(b.openedLastMinute), b.maxPerMinute, len(b.openedToday), b.maxPerDay) +} + +// cleanOldEntries removes entries outside the time windows. +func (b *BrowserRateLimiter) cleanOldEntries(now time.Time) { + // Clean entries older than 1 minute + oneMinuteAgo := now.Add(-1 * time.Minute) + newLastMinute := make([]time.Time, 0, len(b.openedLastMinute)) + for _, t := range b.openedLastMinute { + if t.After(oneMinuteAgo) { + newLastMinute = append(newLastMinute, t) + } + } + b.openedLastMinute = newLastMinute + + // Clean entries older than 24 hours (1 day) + oneDayAgo := now.Add(-24 * time.Hour) + newToday := make([]time.Time, 0, len(b.openedToday)) + for _, t := range b.openedToday { + if t.After(oneDayAgo) { + newToday = append(newToday, t) + } + } + b.openedToday = newToday +} + +// Reset clears the opened PRs tracking - useful when toggling the feature. +func (b *BrowserRateLimiter) Reset() { + b.mu.Lock() + defer b.mu.Unlock() + previousCount := len(b.openedPRs) + b.openedPRs = make(map[string]bool) + log.Printf("[BROWSER] Rate limiter reset: cleared %d tracked PRs", previousCount) +} diff --git a/cmd/goose/main.go b/cmd/goose/main.go index 6cee8a7..71b8e32 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -44,6 +44,7 @@ const ( panicFailureIncrement = 10 turnAPITimeout = 10 * time.Second maxConcurrentTurnAPICalls = 10 + defaultMaxBrowserOpensDay = 20 ) // PR represents a pull request with metadata. @@ -90,8 +91,10 @@ type App struct { loadingTurnData bool menuInitialized bool initialLoadComplete bool - enableReminders bool enableAudioCues bool + enableAutoBrowser bool + browserRateLimiter *BrowserRateLimiter + startTime time.Time } func loadCurrentUser(ctx context.Context, app *App) { @@ -145,9 +148,15 @@ func main() { var targetUser string var noCache bool var updateInterval time.Duration + var browserOpenDelay time.Duration + var maxBrowserOpensMinute int + var maxBrowserOpensDay int flag.StringVar(&targetUser, "user", "", "GitHub user to query PRs for (defaults to authenticated user)") flag.BoolVar(&noCache, "no-cache", false, "Bypass cache for debugging") flag.DurationVar(&updateInterval, "interval", defaultUpdateInterval, "Update interval (e.g. 30s, 1m, 5m)") + flag.DurationVar(&browserOpenDelay, "browser-delay", 1*time.Minute, "Minimum delay before opening PRs in browser after startup") + flag.IntVar(&maxBrowserOpensMinute, "browser-max-per-minute", 2, "Maximum browser windows to open per minute") + flag.IntVar(&maxBrowserOpensDay, "browser-max-per-day", defaultMaxBrowserOpensDay, "Maximum browser windows to open per day") flag.Parse() // Validate target user if provided @@ -163,9 +172,25 @@ func main() { updateInterval = minUpdateInterval } + // Validate browser rate limit parameters + if maxBrowserOpensMinute < 0 { + log.Printf("Invalid browser-max-per-minute %d, using default of 2", maxBrowserOpensMinute) + maxBrowserOpensMinute = 2 + } + if maxBrowserOpensDay < 0 { + log.Printf("Invalid browser-max-per-day %d, using default of %d", maxBrowserOpensDay, defaultMaxBrowserOpensDay) + maxBrowserOpensDay = defaultMaxBrowserOpensDay + } + if browserOpenDelay < 0 { + log.Printf("Invalid browser-delay %v, using default of 1 minute", browserOpenDelay) + browserOpenDelay = 1 * time.Minute + } + log.SetFlags(log.LstdFlags | log.Lshortfile) log.Printf("Starting GitHub PR Monitor (version=%s, commit=%s, date=%s)", version, commit, date) log.Printf("Configuration: update_interval=%v, max_retries=%d, max_delay=%v", updateInterval, maxRetries, maxRetryDelay) + log.Printf("Browser auto-open: startup_delay=%v, max_per_minute=%d, max_per_day=%d", + browserOpenDelay, maxBrowserOpensMinute, maxBrowserOpensDay) ctx := context.Background() @@ -187,8 +212,10 @@ func main() { noCache: noCache, updateInterval: updateInterval, pendingTurnResults: make([]TurnResult, 0), - enableReminders: true, enableAudioCues: true, + enableAutoBrowser: false, // Default to false for safety + browserRateLimiter: NewBrowserRateLimiter(browserOpenDelay, maxBrowserOpensMinute, maxBrowserOpensDay), + startTime: time.Now(), } // Load saved settings @@ -492,6 +519,26 @@ func (app *App) updatePRsWithWait(ctx context.Context) { app.checkForNewlyBlockedPRs(ctx) } +// tryAutoOpenPR attempts to open a PR in the browser if enabled and rate limits allow. +func (app *App) tryAutoOpenPR(ctx context.Context, pr PR, autoBrowserEnabled bool, startTime time.Time) { + if !autoBrowserEnabled { + return + } + + if app.browserRateLimiter.CanOpen(startTime, pr.URL) { + log.Printf("[BROWSER] Auto-opening newly blocked PR: %s #%d - %s", + pr.Repository, pr.Number, pr.URL) + // Use strict validation for auto-opened URLs + if err := openURLAutoStrict(ctx, pr.URL); err != nil { + log.Printf("[BROWSER] Failed to auto-open PR %s: %v", pr.URL, err) + } else { + app.browserRateLimiter.RecordOpen(pr.URL) + log.Printf("[BROWSER] Successfully opened PR %s #%d in browser", + pr.Repository, pr.Number) + } + } +} + // notifyWithSound sends a notification and plays sound only once per cycle. func (app *App) notifyWithSound(ctx context.Context, pr PR, isIncoming bool, playedSound *bool) { var title, soundType string @@ -521,6 +568,8 @@ func (app *App) checkForNewlyBlockedPRs(ctx context.Context) { incoming := app.incoming outgoing := app.outgoing previousBlocked := app.previousBlockedPRs + autoBrowserEnabled := app.enableAutoBrowser + startTime := app.startTime app.mu.RUnlock() currentBlocked := make(map[string]bool) @@ -534,6 +583,7 @@ func (app *App) checkForNewlyBlockedPRs(ctx context.Context) { // Notify if newly blocked if !previousBlocked[incoming[i].URL] { app.notifyWithSound(ctx, incoming[i], true, &playedHonk) + app.tryAutoOpenPR(ctx, incoming[i], autoBrowserEnabled, startTime) } } } @@ -549,6 +599,7 @@ func (app *App) checkForNewlyBlockedPRs(ctx context.Context) { time.Sleep(2 * time.Second) } app.notifyWithSound(ctx, outgoing[i], false, &playedJet) + app.tryAutoOpenPR(ctx, outgoing[i], autoBrowserEnabled, startTime) } } } diff --git a/cmd/goose/security.go b/cmd/goose/security.go index 8f80200..058ce9a 100644 --- a/cmd/goose/security.go +++ b/cmd/goose/security.go @@ -29,6 +29,11 @@ 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. @@ -115,3 +120,54 @@ func validateURL(rawURL string) error { 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=1 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 + urlToValidate := rawURL + if strings.HasSuffix(rawURL, "?goose=1") { + urlToValidate = strings.TrimSuffix(rawURL, "?goose=1") + } + + // 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=1 + if strings.Contains(rawURL, "%") && !strings.HasSuffix(rawURL, "?goose%3D1") { + 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") + } + + // 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 new file mode 100644 index 0000000..321424a --- /dev/null +++ b/cmd/goose/security_test.go @@ -0,0 +1,108 @@ +package main + +import ( + "testing" +) + +func TestValidateGitHubPRURL(t *testing.T) { + tests := []struct { + name string + url string + wantErr bool + }{ + // Valid URLs + { + name: "valid PR URL", + url: "https://github.com/owner/repo/pull/123", + wantErr: false, + }, + { + name: "valid PR URL with goose param", + url: "https://github.com/owner/repo/pull/123?goose=1", + wantErr: false, + }, + { + name: "valid PR URL with hyphen in owner", + url: "https://github.com/owner-name/repo/pull/1", + wantErr: false, + }, + { + name: "valid PR URL with dots in repo", + url: "https://github.com/owner/repo.name/pull/999", + wantErr: false, + }, + + // Invalid URLs - security issues + { + name: "URL with credential injection", + url: "https://evil@github.com/owner/repo/pull/123", + wantErr: true, + }, + { + name: "URL with encoded characters", + url: "https://github.com/owner/repo/pull/123%2F../", + wantErr: true, + }, + { + name: "URL with double slashes", + url: "https://github.com//owner/repo/pull/123", + wantErr: true, + }, + { + name: "URL with fragment", + url: "https://github.com/owner/repo/pull/123#evil", + wantErr: true, + }, + { + name: "URL with extra query params", + url: "https://github.com/owner/repo/pull/123?foo=bar", + wantErr: true, + }, + { + name: "URL with extra path segments", + url: "https://github.com/owner/repo/pull/123/files", + wantErr: true, + }, + + // Invalid URLs - wrong format + { + name: "not a PR URL", + url: "https://github.com/owner/repo", + wantErr: true, + }, + { + name: "issue URL instead of PR", + url: "https://github.com/owner/repo/issues/123", + wantErr: true, + }, + { + name: "HTTP instead of HTTPS", + url: "http://github.com/owner/repo/pull/123", + wantErr: true, + }, + { + name: "wrong domain", + url: "https://gitlab.com/owner/repo/pull/123", + wantErr: true, + }, + { + name: "PR number with leading zero", + url: "https://github.com/owner/repo/pull/0123", + wantErr: true, + }, + { + name: "PR number zero", + url: "https://github.com/owner/repo/pull/0", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateGitHubPRURL(tt.url) + if (err != nil) != tt.wantErr { + t.Errorf("validateGitHubPRURL() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} \ No newline at end of file diff --git a/cmd/goose/settings.go b/cmd/goose/settings.go index ebf06cb..70eeb4c 100644 --- a/cmd/goose/settings.go +++ b/cmd/goose/settings.go @@ -10,13 +10,13 @@ import ( // Settings represents persistent user settings. type Settings struct { - EnableAudioCues bool `json:"enable_audio_cues"` - EnableReminders bool `json:"enable_reminders"` - HideStale bool `json:"hide_stale"` + EnableAudioCues bool `json:"enable_audio_cues"` + HideStale bool `json:"hide_stale"` + EnableAutoBrowser bool `json:"enable_auto_browser"` } -// getSettingsDir returns the configuration directory for settings. -func getSettingsDir() (string, error) { +// settingsDir returns the configuration directory for settings. +func settingsDir() (string, error) { configDir, err := os.UserConfigDir() if err != nil { return "", err @@ -26,13 +26,13 @@ func getSettingsDir() (string, error) { // loadSettings loads settings from disk or returns defaults. func (app *App) loadSettings() { - settingsDir, err := getSettingsDir() + settingsDir, err := settingsDir() if err != nil { log.Printf("Failed to get settings directory: %v", err) // Use defaults app.enableAudioCues = true - app.enableReminders = true app.hideStaleIncoming = true + app.enableAutoBrowser = false return } @@ -45,8 +45,8 @@ func (app *App) loadSettings() { } // Use defaults app.enableAudioCues = true - app.enableReminders = true app.hideStaleIncoming = true + app.enableAutoBrowser = false return } @@ -55,21 +55,21 @@ func (app *App) loadSettings() { log.Printf("Failed to parse settings: %v", err) // Use defaults app.enableAudioCues = true - app.enableReminders = true app.hideStaleIncoming = true + app.enableAutoBrowser = false return } app.enableAudioCues = settings.EnableAudioCues - app.enableReminders = settings.EnableReminders app.hideStaleIncoming = settings.HideStale - log.Printf("Loaded settings: audio_cues=%v, reminders=%v, hide_stale=%v", - app.enableAudioCues, app.enableReminders, app.hideStaleIncoming) + app.enableAutoBrowser = settings.EnableAutoBrowser + log.Printf("Loaded settings: audio_cues=%v, hide_stale=%v, auto_browser=%v", + app.enableAudioCues, app.hideStaleIncoming, app.enableAutoBrowser) } // saveSettings saves current settings to disk. func (app *App) saveSettings() { - settingsDir, err := getSettingsDir() + settingsDir, err := settingsDir() if err != nil { log.Printf("Failed to get settings directory: %v", err) return @@ -77,9 +77,9 @@ func (app *App) saveSettings() { app.mu.RLock() settings := Settings{ - EnableAudioCues: app.enableAudioCues, - EnableReminders: app.enableReminders, - HideStale: app.hideStaleIncoming, + EnableAudioCues: app.enableAudioCues, + HideStale: app.hideStaleIncoming, + EnableAutoBrowser: app.enableAutoBrowser, } app.mu.RUnlock() @@ -102,6 +102,6 @@ func (app *App) saveSettings() { return } - log.Printf("Saved settings: audio_cues=%v, reminders=%v, hide_stale=%v", - settings.EnableAudioCues, settings.EnableReminders, settings.HideStale) + log.Printf("Saved settings: audio_cues=%v, hide_stale=%v, auto_browser=%v", + settings.EnableAudioCues, settings.HideStale, settings.EnableAutoBrowser) } diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 71fee49..11741b0 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -15,6 +15,18 @@ import ( "github.com/energye/systray" ) +// openURLAutoStrict safely opens a URL in the default browser with strict validation for auto-opening. +// This function is used for auto-opening PRs and enforces stricter URL patterns. +func openURLAutoStrict(ctx context.Context, rawURL string) error { + // Validate against strict GitHub PR URL pattern + if err := validateGitHubPRURL(rawURL); err != nil { + return fmt.Errorf("strict validation failed: %w", err) + } + + // Use the regular openURL after strict validation passes + return openURL(ctx, rawURL) +} + // openURL safely opens a URL in the default browser after validation. func openURL(ctx context.Context, rawURL string) error { // Parse and validate the URL @@ -41,6 +53,14 @@ 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 + if u.Host == "github.com" || u.Host == "www.github.com" || u.Host == "dash.ready-to-review.dev" { + q := u.Query() + q.Set("goose", "1") + u.RawQuery = q.Encode() + rawURL = u.String() + } + // Execute the open command based on OS // Use safer methods that don't invoke shell interpretation var cmd *exec.Cmd @@ -350,32 +370,6 @@ func (app *App) addStaticMenuItems(ctx context.Context) { app.rebuildMenu(ctx) }) - // Daily reminders - // Add 'Daily reminders' option - reminderItem := systray.AddMenuItem("Daily reminders", "Send reminder notifications for blocked PRs every 24 hours") - app.mu.RLock() - if app.enableReminders { - reminderItem.Check() - } - app.mu.RUnlock() - reminderItem.Click(func() { - app.mu.Lock() - app.enableReminders = !app.enableReminders - enabled := app.enableReminders - app.mu.Unlock() - - if enabled { - reminderItem.Check() - log.Println("[SETTINGS] Daily reminders enabled") - } else { - reminderItem.Uncheck() - log.Println("[SETTINGS] Daily reminders disabled") - } - - // Save settings to disk - app.saveSettings() - }) - // Add login item option (macOS only) addLoginItemUI(ctx, app) @@ -405,6 +399,36 @@ func (app *App) addStaticMenuItems(ctx context.Context) { app.saveSettings() }) + // Auto-open blocked PRs in browser + // Add 'Auto-open PRs' option + autoOpenItem := systray.AddMenuItem("Auto-open incoming PRs", "Automatically open newly blocked PRs in browser (rate limited)") + app.mu.RLock() + if app.enableAutoBrowser { + autoOpenItem.Check() + } + app.mu.RUnlock() + autoOpenItem.Click(func() { + app.mu.Lock() + app.enableAutoBrowser = !app.enableAutoBrowser + enabled := app.enableAutoBrowser + // Reset rate limiter when toggling the feature + if !enabled { + app.browserRateLimiter.Reset() + } + app.mu.Unlock() + + if enabled { + autoOpenItem.Check() + log.Println("[SETTINGS] Auto-open blocked PRs enabled") + } else { + autoOpenItem.Uncheck() + log.Println("[SETTINGS] Auto-open blocked PRs disabled") + } + + // Save settings to disk + app.saveSettings() + }) + // Quit // Add 'Quit' option quitItem := systray.AddMenuItem("Quit", "")