backend: OIDC: Return error on state generation failure#5643
backend: OIDC: Return error on state generation failure#5643harrshita123 wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harrshita123 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 |
0c97387 to
1adca74
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the backend OIDC login flow to avoid panicking when OAuth state generation fails, instead returning a controlled 500 Internal Server Error and logging the underlying failure. It also introduces a small helper to make state generation testable.
Changes:
- Added
generateOidcState()and a test seam (randRead) to return(state, error)instead of panicking on entropy read failures. - Updated the
/oidchandler to log and return HTTP 500 when state generation fails. - Added a regression test covering success/failure paths for state generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/cmd/headlamp.go | Replaces panic-based state generation with an error-returning helper and graceful HTTP 500 handling in the OIDC login route. |
| backend/cmd/headlamp_test.go | Adds a regression test for generateOidcState() using an overridable randomness function. |
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
the PR has a merge-main commit; please rebase against main to keep the history clean.
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
backend/cmd/headlamp.go:1129
generateOidcStateignores the byte count returned byrandRead. SinceReadis allowed to returnn < len(b)witherr == nil, this can produce a state value with partially zero-filled bytes. Treat short reads as an error (e.g., checkn != len(b)and returnio.ErrUnexpectedEOF) and add a test for the partial-read case.
func generateOidcState() (string, error) {
b := make([]byte, 32)
if _, err := randRead(b); err != nil {
return "", err
}
| Cluster string // cluster context name this is associated with | ||
| } | ||
|
|
||
| var randRead = rand.Read |
| func TestGenerateOidcState(t *testing.T) { | ||
| originalRandRead := randRead | ||
|
|
||
| t.Cleanup(func() { | ||
| randRead = originalRandRead | ||
| }) |
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
the PR has a merge-main commit; please rebase against main to keep the history clean.
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
Summary
This PR fixes the OIDC login path so Headlamp returns a normal error when OAuth state generation fails instead of panicking the backend handler.
Related Issue
Fixes #5630
Changes
backend/cmd/headlamp.goto log and return500 Internal Server Errorif state generation failsbackend/cmd/headlamp_test.goSteps to Test
TestGenerateOidcStateregression test passes.panic(err)for state generation failure.Screenshots
Not applicable.
Notes for the Reviewer
This change is intentionally small and isolated to the OIDC login path. It replaces the panic path with explicit error handling and keeps the rest of the login flow unchanged.