oauth/cimd: post-merge review nits (F1, F2, F6, F7)#117
Merged
Conversation
F1 (high) — /authorize and /callback emitted plain text/plain errors via http.Error. Now all error paths in those two handlers route through writeOAuthTokenError, producing the RFC 6749 §5.2 JSON shape that the rest of /oauth/* already used. Error codes mapped: - missing client_id/redirect_uri/response_type → invalid_request - resolver failure → invalid_client - redirect_uri not registered → invalid_request - missing/wrong PKCE → invalid_request - resource indicator mismatch → invalid_target (RFC 8707) - PKCE-verifier generation failure → server_error (500) - pending-auth JWE encode failure → server_error (500) - upstream-AS URL resolve failure → server_error (502) - missing state/code at /callback → invalid_request - unknown/expired pending-auth at /callback → invalid_request - auth-code JWE encode failure → server_error (500) - unparseable pending.RedirectURI at /callback → server_error (502) Method-not-allowed at /authorize stays 405 but as JSON. F2 (medium) — writeOAuthTokenError now sets `WWW-Authenticate: Bearer error="...", error_description="..."` when status == 401. RFC 7235 §3.1 mandates the header on 401s; RFC 6750 §3 specifies the Bearer challenge shape. The two existing 401 callers on /oauth/token (`invalid_client` for missing CIMD client + unsupported client auth) now carry the challenge for free. F6 (nit) — pkg/config/config.go:105 said "secrets like GatingSecretKey can be injected" — the field is SigningSecret; comment updated. While in the file, also rewrote the SigningSecret desc tag (was referring to "refresh tokens, DCR client_secrets" — both gone post-#115) to accurately describe what the secret HKDF-derives keys for in v1: pending-auth state JWE + downstream auth-code JWE. F7 (nit) — docs/oauth_authorization.md listed `refresh_revokes_tracking` in the gating-mode forbidden list at three places. The field never existed under that name post-DCR-removal — validateOAuthRuntimeConfig forbids client_id / client_secret / token_url / auth_url / userinfo_url / public_auth_server_url, and that's it. Stripped from the doc. Tests added: - TestWriteOAuthTokenError gains a 401-Bearer-challenge subtest pinning the new WWW-Authenticate behaviour. - TestOAuthAuthorizeErrorsAreJSON pins F1: four error paths through /authorize and /callback verified to return application/json bodies with RFC 6749 error/error_description. No behaviour change beyond the four fixes; all existing tests stay green; `go vet ./...` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four findings from the post-merge review on #116, all addressed on one branch.
Test plan
🤖 Generated with Claude Code