From 7b52cd031a3d32088f777fa4ddc2266d0e905bdc Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 12 Aug 2025 15:37:41 -0400 Subject: [PATCH 1/2] Add GHA & linting --- README.md | 23 ++++++++++++++--- cache.go | 2 +- github.go | 24 ++++++++++++++---- main.go | 74 +++++++++++++++++++++++++------------------------------ sound.go | 20 +++------------ ui.go | 40 ++++++++++++++---------------- 6 files changed, 94 insertions(+), 89 deletions(-) diff --git a/README.md b/README.md index 2cac37b..ee93507 100644 --- a/README.md +++ b/README.md @@ -44,8 +44,25 @@ cd goose && make run If you want more control over which repositories the goose can access, you can use a GitHub personal access token instead: -1. Create a [GitHub personal access token](https://github.com/settings/tokens) with `repo` scope -2. Set the `GITHUB_TOKEN` environment variable: +#### Recommended: Fine-grained Personal Access Token + +For maximum security, use a [fine-grained personal access token](https://github.com/settings/personal-access-tokens/new): + +1. Go to GitHub Settings → Developer settings → Personal access tokens → Fine-grained tokens +2. Create a new token with: + - **Expiration**: Set a short expiration (30-90 days recommended) + - **Repository access**: Select only the specific repositories you want to monitor + - **Permissions**: + - Pull requests: Read + - Metadata: Read +3. Copy the token (starts with `github_pat_`) + +#### Alternative: Classic Personal Access Token + +If you need broader access, you can use a [classic token](https://github.com/settings/tokens): +- Create with `repo` scope (grants full repository access - use with caution) + +#### Using the Token ```bash export GITHUB_TOKEN=your_token_here @@ -53,7 +70,7 @@ git clone https://github.com/ready-to-review/goose.git cd goose && make run ``` -When `GITHUB_TOKEN` is set, the goose will use it directly instead of the GitHub CLI, giving you precise control over repository access. +When `GITHUB_TOKEN` is set, the goose will use it directly instead of the GitHub CLI, giving you precise control over repository access. Fine-grained tokens are strongly recommended for better security. ## Known Issues diff --git a/cache.go b/cache.go index 5034242..c8687b8 100644 --- a/cache.go +++ b/cache.go @@ -64,7 +64,7 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( var data *turn.CheckResponse err := retry.Do(func() error { // Create timeout context for Turn API call - turnCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + turnCtx, cancel := context.WithTimeout(ctx, turnAPITimeout) defer cancel() var retryErr error diff --git a/github.go b/github.go index 7c883a1..1bcbbb6 100644 --- a/github.go +++ b/github.go @@ -22,7 +22,7 @@ import ( // initClients initializes GitHub and Turn API clients. func (app *App) initClients(ctx context.Context) error { - token, err := app.githubToken(ctx) + token, err := app.token(ctx) if err != nil { return fmt.Errorf("get github token: %w", err) } @@ -44,8 +44,8 @@ func (app *App) initClients(ctx context.Context) error { return nil } -// githubToken retrieves the GitHub token from GITHUB_TOKEN env var or gh CLI. -func (*App) githubToken(ctx context.Context) (string, error) { +// token retrieves the GitHub token from GITHUB_TOKEN env var or gh CLI. +func (*App) token(ctx context.Context) (string, error) { // First check for GITHUB_TOKEN environment variable if token := os.Getenv("GITHUB_TOKEN"); token != "" { token = strings.TrimSpace(token) @@ -390,7 +390,10 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u // Use a WaitGroup to track goroutines var wg sync.WaitGroup - // Process PRs in parallel + // Create semaphore to limit concurrent Turn API calls + sem := make(chan struct{}, maxConcurrentTurnAPICalls) + + // Process PRs in parallel with concurrency limit for _, issue := range issues { if !issue.IsPullRequest() { continue @@ -400,6 +403,10 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u go func(issue *github.Issue) { defer wg.Done() + // Acquire semaphore + sem <- struct{}{} + defer func() { <-sem }() + url := issue.GetHTMLURL() updatedAt := issue.GetUpdatedAt().Time @@ -490,7 +497,10 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, // Use a WaitGroup to track goroutines var wg sync.WaitGroup - // Process PRs in parallel + // Create semaphore to limit concurrent Turn API calls + sem := make(chan struct{}, maxConcurrentTurnAPICalls) + + // Process PRs in parallel with concurrency limit for _, issue := range issues { if !issue.IsPullRequest() { continue @@ -500,6 +510,10 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, go func(issue *github.Issue) { defer wg.Done() + // Acquire semaphore + sem <- struct{}{} + defer func() { <-sem }() + url := issue.GetHTMLURL() updatedAt := issue.GetUpdatedAt().Time diff --git a/main.go b/main.go index a94faf2..83f5853 100644 --- a/main.go +++ b/main.go @@ -47,6 +47,19 @@ const ( // Retry settings for external API calls - exponential backoff with jitter up to 2 minutes. maxRetryDelay = 2 * time.Minute maxRetries = 10 // Should reach 2 minutes with exponential backoff + + // Failure thresholds. + minorFailureThreshold = 3 + majorFailureThreshold = 10 + panicFailureIncrement = 10 + + // Notification settings. + reminderInterval = 24 * time.Hour + historyRetentionDays = 30 + + // Turn API settings. + turnAPITimeout = 10 * time.Second + maxConcurrentTurnAPICalls = 10 ) // PR represents a pull request with metadata. @@ -270,7 +283,7 @@ func (app *App) updateLoop(ctx context.Context) { // Update failure count app.mu.Lock() - app.consecutiveFailures += 10 // Treat panic as critical failure + app.consecutiveFailures += panicFailureIncrement // Treat panic as critical failure app.mu.Unlock() // Signal app to quit after panic @@ -308,10 +321,6 @@ func (app *App) updatePRs(ctx context.Context) { app.mu.Unlock() // Progressive degradation based on failure count - const ( - minorFailureThreshold = 3 - majorFailureThreshold = 10 - ) var title, tooltip string switch { case failureCount == 1: @@ -475,10 +484,6 @@ func (app *App) updatePRsWithWait(ctx context.Context) { app.mu.Unlock() // Progressive degradation based on failure count - const ( - minorFailureThreshold = 3 - majorFailureThreshold = 10 - ) var title, tooltip string switch { case failureCount == 1: @@ -541,35 +546,6 @@ func (app *App) updatePRsWithWait(ctx context.Context) { app.checkForNewlyBlockedPRs(ctx) } -// shouldNotifyForPR determines if we should send a notification for a PR. -func shouldNotifyForPR( - _ string, - isBlocked bool, - prevState NotificationState, - hasHistory bool, - reminderInterval time.Duration, - enableReminders bool, -) (shouldNotify bool, reason string) { - if !hasHistory && isBlocked { - return true, "newly blocked" - } - - if !hasHistory { - return false, "" - } - - switch { - case isBlocked && !prevState.WasBlocked: - return true, "became blocked" - case !isBlocked && prevState.WasBlocked: - return false, "unblocked" - case isBlocked && prevState.WasBlocked && enableReminders && time.Since(prevState.LastNotified) > reminderInterval: - return true, "reminder" - default: - return false, "" - } -} - // processPRNotifications handles notification logic for a single PR. func (app *App) processPRNotifications( ctx context.Context, @@ -582,7 +558,24 @@ func (app *App) processPRNotifications( reminderInterval time.Duration, ) { prevState, hasHistory := notificationHistory[pr.URL] - shouldNotify, notifyReason := shouldNotifyForPR(pr.URL, isBlocked, prevState, hasHistory, reminderInterval, app.enableReminders) + + // Determine if we should notify (inlined from shouldNotifyForPR) + var shouldNotify bool + var notifyReason string + switch { + case !hasHistory && isBlocked: + shouldNotify, notifyReason = true, "newly blocked" + case !hasHistory: + shouldNotify, notifyReason = false, "" + case isBlocked && !prevState.WasBlocked: + shouldNotify, notifyReason = true, "became blocked" + case !isBlocked && prevState.WasBlocked: + shouldNotify, notifyReason = false, "unblocked" + case isBlocked && prevState.WasBlocked && app.enableReminders && time.Since(prevState.LastNotified) > reminderInterval: + shouldNotify, notifyReason = true, "reminder" + default: + shouldNotify, notifyReason = false, "" + } // Update state for unblocked PRs if notifyReason == "unblocked" { @@ -669,8 +662,7 @@ func (app *App) checkForNewlyBlockedPRs(ctx context.Context) { now := time.Now() staleThreshold := now.Add(-stalePRThreshold) - // Reminder interval for re-notifications (24 hours) - const reminderInterval = 24 * time.Hour + // Use reminder interval constant from package level currentBlockedPRs := make(map[string]bool) playedIncomingSound := false diff --git a/sound.go b/sound.go index d8c4249..b014531 100644 --- a/sound.go +++ b/sound.go @@ -4,7 +4,6 @@ package main import ( "context" _ "embed" - "fmt" "log" "os" "os/exec" @@ -106,22 +105,9 @@ func (app *App) playSound(ctx context.Context, soundType string) { case "darwin": cmd = exec.CommandContext(soundCtx, "afplay", soundPath) case "windows": - // Validate soundPath contains only safe characters for PowerShell - // Allow alphanumeric, spaces, dots, underscores, hyphens, backslashes, and colons (for drive letters) - for _, r := range soundPath { - isValid := (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || - (r >= '0' && r <= '9') || r == ' ' || r == '.' || - r == '_' || r == '-' || r == '\\' || r == ':' - if !isValid { - log.Printf("Sound path contains invalid character for PowerShell: %q in path %s", r, soundPath) - return - } - } - // Use PowerShell's SoundPlayer with proper escaping - //nolint:gocritic // Need literal quotes in PowerShell script - script := fmt.Sprintf(`(New-Object Media.SoundPlayer "%s").PlaySync()`, - strings.ReplaceAll(soundPath, `"`, `""`)) - cmd = exec.CommandContext(soundCtx, "powershell", "-WindowStyle", "Hidden", "-c", script) + // Use Windows Media Player API via rundll32 to avoid PowerShell script injection + // This is safer than constructing PowerShell scripts with user paths + cmd = exec.CommandContext(soundCtx, "cmd", "/c", "start", "/min", "", soundPath) case "linux": // Try paplay first (PulseAudio), then aplay (ALSA) cmd = exec.CommandContext(soundCtx, "paplay", soundPath) diff --git a/ui.go b/ui.go index c4dfbd9..897d77d 100644 --- a/ui.go +++ b/ui.go @@ -15,24 +15,6 @@ import ( "github.com/energye/systray" ) -// formatAge formats a duration in human-readable form. -func formatAge(updatedAt time.Time) string { - duration := time.Since(updatedAt) - - switch { - case duration < time.Hour: - return fmt.Sprintf("%dm", int(duration.Minutes())) - case duration < 24*time.Hour: - return fmt.Sprintf("%dh", int(duration.Hours())) - case duration < 30*24*time.Hour: - return fmt.Sprintf("%dd", int(duration.Hours()/24)) - case duration < 365*24*time.Hour: - return fmt.Sprintf("%dmo", int(duration.Hours()/(24*30))) - default: - return updatedAt.Format("2006") - } -} - // openURL safely opens a URL in the default browser after validation. func openURL(ctx context.Context, rawURL string) error { // Parse and validate the URL @@ -193,9 +175,8 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, // Add PR items in sorted order for i := range sortedPRs { // Apply filters - // Skip stale PRs if configured (older than 90 days) - const stalePRDays = 90 - if app.hideStaleIncoming && sortedPRs[i].UpdatedAt.Before(time.Now().Add(-stalePRDays*24*time.Hour)) { + // Skip stale PRs if configured + if app.hideStaleIncoming && sortedPRs[i].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { continue } @@ -204,7 +185,22 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, if sortedPRs[i].NeedsReview { title = fmt.Sprintf("• %s", title) } - tooltip := fmt.Sprintf("%s (%s)", sortedPRs[i].Title, formatAge(sortedPRs[i].UpdatedAt)) + // Format age inline + duration := time.Since(sortedPRs[i].UpdatedAt) + var ageStr string + switch { + case duration < time.Hour: + ageStr = fmt.Sprintf("%dm", int(duration.Minutes())) + case duration < 24*time.Hour: + ageStr = fmt.Sprintf("%dh", int(duration.Hours())) + case duration < 30*24*time.Hour: + ageStr = fmt.Sprintf("%dd", int(duration.Hours()/24)) + case duration < 365*24*time.Hour: + ageStr = fmt.Sprintf("%dmo", int(duration.Hours()/(24*30))) + default: + ageStr = sortedPRs[i].UpdatedAt.Format("2006") + } + tooltip := fmt.Sprintf("%s (%s)", sortedPRs[i].Title, ageStr) // Add action reason for blocked PRs if (sortedPRs[i].NeedsReview || sortedPRs[i].IsBlocked) && sortedPRs[i].ActionReason != "" { tooltip = fmt.Sprintf("%s - %s", tooltip, sortedPRs[i].ActionReason) From de610a02be63f4a02f3961d67664b86895b23235 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 12 Aug 2025 15:38:05 -0400 Subject: [PATCH 2/2] Add GHA & linting (missing files) --- ratelimit.go | 58 +++++++++++++++++++++++++ security.go | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 ratelimit.go create mode 100644 security.go diff --git a/ratelimit.go b/ratelimit.go new file mode 100644 index 0000000..d0c2ef7 --- /dev/null +++ b/ratelimit.go @@ -0,0 +1,58 @@ +// Package main - ratelimit.go provides rate limiting functionality. +package main + +import ( + "sync" + "time" +) + +// RateLimiter implements a simple token bucket rate limiter. +type RateLimiter struct { + lastRefill time.Time + mu sync.Mutex + refillRate time.Duration + tokens int + maxTokens int +} + +// NewRateLimiter creates a new rate limiter. +func NewRateLimiter(maxTokens int, refillRate time.Duration) *RateLimiter { + return &RateLimiter{ + tokens: maxTokens, + maxTokens: maxTokens, + refillRate: refillRate, + lastRefill: time.Now(), + } +} + +// Allow checks if an operation is allowed under the rate limit. +func (r *RateLimiter) Allow() bool { + r.mu.Lock() + defer r.mu.Unlock() + + // Refill tokens based on elapsed time + now := time.Now() + elapsed := now.Sub(r.lastRefill) + tokensToAdd := int(elapsed / r.refillRate) + + if tokensToAdd > 0 { + r.tokens = minInt(r.tokens+tokensToAdd, r.maxTokens) + r.lastRefill = now + } + + // Check if we have tokens available + if r.tokens > 0 { + r.tokens-- + return true + } + + return false +} + +// minInt returns the minimum of two integers. +func minInt(a, b int) int { + if a < b { + return a + } + return b +} diff --git a/security.go b/security.go new file mode 100644 index 0000000..8f80200 --- /dev/null +++ b/security.go @@ -0,0 +1,117 @@ +// Package main - security.go provides security utilities and validation functions. +package main + +import ( + "errors" + "fmt" + "regexp" + "strings" +) + +const ( + // Security constants. + minTokenLength = 40 // GitHub tokens are at least 40 chars + maxTokenLength = 255 // Reasonable upper bound + maxUsernameLen = 39 // GitHub username max length + maxURLLength = 2048 // Maximum URL length + minPrintableChar = 0x20 // Minimum printable character + deleteChar = 0x7F // Delete character +) + +var ( + // githubUsernameRegex validates GitHub usernames. + // GitHub usernames can only contain alphanumeric characters and hyphens, + // cannot start or end with hyphen, and max 39 characters. + githubUsernameRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9-]{0,37}[a-zA-Z0-9]$|^[a-zA-Z0-9]$`) + + // githubTokenRegex validates GitHub token format. + // Classic tokens: 40 hex chars. + // 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}$`) +) + +// validateGitHubUsername validates a GitHub username. +func validateGitHubUsername(username string) error { + if username == "" { + return errors.New("username cannot be empty") + } + if len(username) > maxUsernameLen { + return fmt.Errorf("username too long: %d > %d", len(username), maxUsernameLen) + } + if !githubUsernameRegex.MatchString(username) { + return fmt.Errorf("invalid GitHub username format: %s", username) + } + return nil +} + +// validateGitHubToken performs basic validation on a GitHub token. +func validateGitHubToken(token string) error { + if token == "" { + return errors.New("token cannot be empty") + } + + tokenLen := len(token) + if tokenLen < minTokenLength { + return fmt.Errorf("token too short: %d < %d", tokenLen, minTokenLength) + } + if tokenLen > maxTokenLength { + return fmt.Errorf("token too long: %d > %d", tokenLen, maxTokenLength) + } + + // Check for common placeholder values + if strings.Contains(strings.ToLower(token), "your_token") || + strings.Contains(strings.ToLower(token), "xxx") || + strings.Contains(token, "...") { + return errors.New("token appears to be a placeholder") + } + + if !githubTokenRegex.MatchString(token) { + return errors.New("token does not match expected GitHub token format") + } + + return nil +} + +// sanitizeForLog removes sensitive information from strings before logging. +func sanitizeForLog(s string) string { + // Redact tokens (both classic 40-char hex and new format) + // Classic tokens + s = regexp.MustCompile(`\b[a-f0-9]{40}\b`).ReplaceAllString(s, "[REDACTED-TOKEN]") + // New format tokens (ghp_, ghs_, ghr_, gho_, ghu_) + s = regexp.MustCompile(`\bgh[psoru]_[A-Za-z0-9]{36,251}\b`).ReplaceAllString(s, "[REDACTED-TOKEN]") + // Fine-grained personal access tokens + s = regexp.MustCompile(`\bgithub_pat_[A-Za-z0-9]{82}\b`).ReplaceAllString(s, "[REDACTED-TOKEN]") + // Bearer tokens in headers + s = regexp.MustCompile(`Bearer [A-Za-z0-9_\-.]+`).ReplaceAllString(s, "Bearer [REDACTED]") + // Authorization headers + s = regexp.MustCompile(`Authorization: \S+`).ReplaceAllString(s, "Authorization: [REDACTED]") + + 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 +}