fix(auth): backend auth edges (#6056, #6063, #6064)#6076
Conversation
…covery, oauth_configured requires secret Fixes three backend auth edges: - #6056: /health reported oauth_configured=true when only GITHUB_CLIENT_ID was set but GITHUB_CLIENT_SECRET was missing. Such a config is unusable for the token exchange, so the probe misled the frontend into showing a GitHub login button that was guaranteed to fail. The check now requires both values. - #6063: A structurally malformed Authorization header (no Bearer prefix, 'Bearer' with no token, whitespace-only) returned 401 immediately even when a perfectly valid kc_auth cookie was attached. The middleware now treats a malformed header the same as an empty header and falls through to the cookie path. Complements #6026 (stale-but-valid-format header fallback already landed in #6031). - #6064: When validateAndConsumeOAuthState failed (stale OAuth tab, server restart that cleared the in-memory state store, back-button replay) the callback unconditionally redirected to /login?error=csrf_validation_failed even if the user already had a valid kc_auth cookie. The callback now checks for a non-expired, non-revoked JWT cookie on state failure and, if present, redirects to / so the live session is preserved. Complements #6034 which fixes the root cause of state loss on the server side. Signed-off-by: Andrew Anderson <[email protected]>
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 |
kubestellar-prow
Bot
added
the
size/L
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
✅ Deploy Preview for kubestellarconsole canceled.
|
There was a problem hiding this comment.
Pull request overview
Addresses three backend authentication edge cases that could mislead the frontend OAuth readiness UI, incorrectly reject valid cookie sessions, or unnecessarily force re-login during OAuth callback.
Changes:
- Fix
/healthto reportoauth_configuredonly when both GitHub client ID and secret are set, viaServer.oauthConfigured(). - Update
JWTAuthmiddleware to treat structurally malformedAuthorizationheaders as “absent” and fall back tokc_authcookie auth. - Improve GitHub OAuth callback behavior to preserve an existing valid
kc_authsession when CSRF state validation fails.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/api/server.go | Adds oauthConfigured() helper and uses it in /health response. |
| pkg/api/server_test.go | New unit test covering all ID/secret presence combinations for OAuth readiness. |
| pkg/api/middleware/auth.go | Adjusts Authorization header parsing to fall back to cookie on malformed headers; introduces bearerScheme const. |
| pkg/api/middleware/auth_test.go | Adds coverage for multiple malformed Authorization header variants with/without cookie. |
| pkg/api/handlers/auth.go | Adds hasValidAuthCookie and uses it to recover from OAuth state failures in GitHubCallback. |
| pkg/api/handlers/auth_test.go | Adds tests for cookie-based recovery path on state validation failure and negative cases. |
| // cookie that parses as a non-expired, non-revoked JWT under the handler's | ||
| // signing secret. It is used by GitHubCallback (#6064) to recover from CSRF | ||
| // state-validation failures when the user is already authenticated: a stale | ||
| // OAuth tab, a browser back-button replay, or a server restart that cleared | ||
| // the in-memory state store should not force a user with a live session | ||
| // back through the login flow. Any parse error, validity failure, missing | ||
| // claims, or revocation check failure causes this helper to return false so | ||
| // the caller falls through to the normal error path. |
There was a problem hiding this comment.
The doc comment says that a “revocation check failure” makes this helper return false, but middleware.IsTokenRevoked cannot surface failures (it logs and returns false). As written, any revocation-store error will be treated as “not revoked” and this helper will return true. Please either adjust the comment to match the actual behavior, or change the revocation API/usage so failures can conservatively fail closed here if that’s the intent.
| // cookie that parses as a non-expired, non-revoked JWT under the handler's | |
| // signing secret. It is used by GitHubCallback (#6064) to recover from CSRF | |
| // state-validation failures when the user is already authenticated: a stale | |
| // OAuth tab, a browser back-button replay, or a server restart that cleared | |
| // the in-memory state store should not force a user with a live session | |
| // back through the login flow. Any parse error, validity failure, missing | |
| // claims, or revocation check failure causes this helper to return false so | |
| // the caller falls through to the normal error path. | |
| // cookie that parses as a non-expired JWT under the handler's signing | |
| // secret and is not explicitly reported as revoked. It is used by | |
| // GitHubCallback (#6064) to recover from CSRF state-validation failures | |
| // when the user is already authenticated: a stale OAuth tab, a browser | |
| // back-button replay, or a server restart that cleared the in-memory state | |
| // store should not force a user with a live session back through the login | |
| // flow. Any parse error, validity failure, missing claims, or explicit | |
| // revocation result causes this helper to return false so the caller falls | |
| // through to the normal error path. Revocation-store lookup failures are | |
| // not distinguishable here because middleware.IsTokenRevoked only returns | |
| // a boolean. |
| trimmedHeader := strings.TrimSpace(authHeader) | ||
| if trimmedHeader != "" { | ||
| if strings.HasPrefix(trimmedHeader, bearerScheme) { | ||
| candidate := strings.TrimSpace(strings.TrimPrefix(trimmedHeader, bearerScheme)) | ||
| if candidate != "" { | ||
| tokenString = candidate | ||
| } | ||
| } | ||
| // If we got here with tokenString still empty, the header was | ||
| // structurally malformed — keep going and let the cookie path | ||
| // (and the downstream "missing authorization" check) decide. | ||
| if tokenString == "" { | ||
| slog.Info("[Auth] malformed authorization header, trying cookie", "path", c.Path()) | ||
| } |
There was a problem hiding this comment.
This logs an Info line for any non-empty malformed Authorization header, and requests with no valid cookie will also hit the later "[Auth] missing authorization" Info log, producing two log lines per request. Consider downgrading this to Debug or only logging when a cookie is present (i.e., when the fallback actually changes behavior) to avoid noisy logs from common headers like "Basic ...".
| expiredJWT := jwt.NewWithClaims(jwt.SigningMethodHS256, expiredClaims) | ||
| expiredSigned, _ := expiredJWT.SignedString([]byte("test-secret")) | ||
|
|
There was a problem hiding this comment.
The test ignores the error returned by SignedString. If signing were to fail, the test would proceed with an empty/invalid token and could produce misleading results. Please assert/require that the signing error is nil to make the test failure mode explicit.
| forgedJWT := jwt.NewWithClaims(jwt.SigningMethodHS256, forgedClaims) | ||
| forgedSigned, _ := forgedJWT.SignedString([]byte("not-the-real-secret")) | ||
|
|
There was a problem hiding this comment.
The test ignores the error returned by SignedString when creating the wrong-secret cookie JWT. Please check the signing error so the test can’t accidentally pass with an empty/invalid token if signing fails.
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Summary
Three small, independent backend auth fixes.
#6056 —
/healthreportsoauth_configured: truewith no secretpkg/api/server.goonly checkedGitHubClientID != "". A config with an ID but no secret is unusable — the OAuth token exchange needs the secret — so the probe was lying to the frontend, which then renders a GitHub login button that is guaranteed to fail on click. Fix: require BOTHGitHubClientIDandGitHubSecret. Logic extracted intoServer.oauthConfigured()for direct unit testing.#6063 — Malformed
Authorizationheader blocks cookie authpkg/api/middleware/auth.goreturned 401 immediately whenAuthorizationwas present but notBearer <token>(e.g.garbage,Bearerwith no token, whitespace). A client with a perfectly validkc_authcookie would be bounced to login because some misbehaving layer stamped a bad header onto the fetch. Fix: treat any structurally malformed header the same as an empty header and fall through to the cookie path. This is the complement to #6026 (stale-but-valid-format header fallback, landed in #6031). Accepted malformed patterns now fall through: no Bearer prefix,Bearerwith no token,Bearerwith only whitespace, whitespace-only headers, lowercasebearer.#6064 — OAuth callback ignores valid existing cookie on state failure
pkg/api/handlers/auth.gounconditionally redirected to/login?error=csrf_validation_failedwhenvalidateAndConsumeOAuthStatefailed. The user may already have a live session — the failed state is a stale OAuth tab, a back-button replay, or a server restart that cleared the in-memory store. Fix: on state failure, check for a non-expired, non-revokedkc_authcookie via a newhasValidAuthCookiehelper that usesmiddleware.ParseJWT+IsTokenRevoked. If valid, redirect to/and preserve the session. If missing/expired/forged, the classic error redirect applies. Complements #6034 which fixes the root cause of state loss on the server side; both should land.Test plan
pkg/api/middleware/auth_test.go—TestJWTAuth_MalformedHeaderFallsBackToCookiecovers 7 malformed header variants, each tested with and without a valid cookie.pkg/api/handlers/auth_test.go—TestGitHubCallback_RecoversFromValidCookieOnStateFailurecovers: valid cookie + bogus state →/, missing cookie → error page, expired cookie → error page, wrong-secret cookie → error page, empty state →/.pkg/api/server_test.go(new) —TestHealth_OAuthConfiguredRequiresBothIdAndSecretcovers all four id/secret presence combinations.go build ./...go vet ./...go test ./pkg/... -count=1(all packages green)Notes
consts (validCookieLifetime,expiredCookieAge).server_test.go).Fixes #6056
Fixes #6063
Fixes #6064