🐛 fix: Go security bugs — safe type assertions, race condition, dead code, hardcoded secret#3960
Conversation
…code, dev secret - Use safe two-value type assertion for issue/PR numbers in webhook handlers to prevent panics on malformed payloads (#3929) - Re-check PR cache after acquiring write lock to avoid redundant fetches from concurrent goroutines (#3930) - Use shared h.httpClient in fetchGitHubIssues instead of creating a local client; read response body into buffer before decoding to avoid reading an already-consumed stream (#3928) - Remove unused createGitHubIssue wrapper function (dead code) (#3932) - Add PullRequest field to GitHubIssue struct and use it to filter PRs instead of fragile URL substring matching (#3931) - Replace hardcoded dev JWT secret with crypto/rand generation (#3896) Fixes #3929 #3930 #3928 #3932 #3931 #3896 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/M
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
Hardens the backend’s GitHub integration by eliminating panic-prone webhook parsing, improving PR caching concurrency behavior, making GitHub issue/PR filtering more reliable, and removing insecure/dev-only secrets and dead code.
Changes:
- Generate a per-startup cryptographically random dev-mode JWT secret (replacing a hardcoded secret).
- Improve GitHub feedback handler robustness: safe webhook field extraction, reliable PR-vs-issue filtering via
pull_request, and safer GitHub response handling. - Reduce PR cache overwrite race by re-checking cache freshness after acquiring the write lock.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/api/server.go | Replaces hardcoded dev JWT secret with crypto/rand-backed generation. |
| pkg/api/handlers/feedback.go | Adds PullRequest field to issue model for robust PR filtering; improves PR cache locking; refactors GitHub issue fetch to reuse shared client and preserve response body for errors/decoding; makes webhook number parsing non-panicking; removes dead wrapper. |
| // crypto/rand.Read should never fail on supported platforms; | ||
| // if it does, fall back to a logged warning and a best-effort value. | ||
| log.Printf("WARNING: crypto/rand.Read failed: %v — using fallback", err) | ||
| return fmt.Sprintf("dev-fallback-%d", b) |
There was a problem hiding this comment.
In the crypto/rand failure branch, fmt.Sprintf("dev-fallback-%d", b) uses %d with a []byte, which produces a %!d([]uint8=...) formatting artifact and is not a reliable/intentional fallback secret. Use a byte-safe format (e.g., hex/base64) and ensure the fallback is not a constant value if b remains zeroed (or consider failing fast instead of generating an insecure secret).
| // crypto/rand.Read should never fail on supported platforms; | |
| // if it does, fall back to a logged warning and a best-effort value. | |
| log.Printf("WARNING: crypto/rand.Read failed: %v — using fallback", err) | |
| return fmt.Sprintf("dev-fallback-%d", b) | |
| // crypto/rand.Read should never fail on supported platforms; if it does, | |
| // fail fast rather than generating an insecure or deterministic secret. | |
| log.Fatalf("FATAL: crypto/rand.Read failed: %v — cannot generate dev secret", err) |
| var allPRs []GitHubPR | ||
| for _, state := range []string{"open", "closed"} { | ||
| prs := h.fetchPRPages(state) | ||
| allPRs = append(allPRs, prs...) | ||
| } | ||
|
|
||
| h.prCacheMu.Lock() | ||
| // Re-check: another goroutine may have populated the cache while we fetched. | ||
| if h.prCache != nil && time.Since(h.prCacheTime) < prCacheTTL { | ||
| cached := h.prCache | ||
| h.prCacheMu.Unlock() | ||
| return cached | ||
| } |
There was a problem hiding this comment.
This change prevents overwriting the cache with stale data, but it does not prevent redundant GitHub fetches: multiple goroutines can still observe a stale cache under the read lock, release it, and concurrently run fetchPRPages() before any goroutine acquires the write lock. If the goal is to avoid duplicated API calls under load, consider deduplicating in-flight fetches (e.g., singleflight.Group or an explicit "fetching" flag/condvar) so only one goroutine hits GitHub per refresh window.
| // Read the full body into a buffer so we can use it for both error | ||
| // reporting and JSON decoding (json.Decoder would consume the stream). | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read GitHub response body: %w", err) | ||
| } |
There was a problem hiding this comment.
io.ReadAll(resp.Body) reads the entire GitHub response into memory without any upper bound. Other handlers in this repo use io.LimitReader to cap body sizes; adding a reasonable limit here would avoid potential memory pressure/DoS if the upstream (or a proxy) returns an unexpectedly large payload.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 1 code suggestion(s) and 2 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
issue["number"].(float64)) that panic on missing/malformed fields. Now uses two-value form with error return.getCachedOrFetchPRsreleased the read lock, fetched data, then acquired the write lock without re-checking. Multiple goroutines could redundantly fetch and overwrite. Now re-checks cache freshness after acquiring the write lock.fetchGitHubIssuescreated a localhttp.Clientinstead of reusingh.httpClient, and the error path tried to readresp.Bodyafterjson.NewDecoderhad already consumed it. Now uses the shared client and reads body into a buffer first.createGitHubIssuewrapper (onlycreateGitHubIssueInRepois called)./pull/. Added aPullRequestfield toGitHubIssuestruct (matching GitHub's API response) and use it for reliable filtering.generateDevSecret()returned a static string. Now generates a cryptographically random 32-byte hex secret viacrypto/randon each server start.Test plan
go build ./...passesgo vet ./...passesnumberfield instead of panickingfetchGitHubIssuescorrectly filters PRs using thepull_requeststruct fieldFixes #3929 #3930 #3928 #3932 #3931 #3896