oauth/cimd: PR #119 post-merge review nits#120
Merged
Conversation
Five items from the post-merge review of #119: * `audienceMatches` doc/code mismatch — the verifyClientAssertion comment claimed we'd accept an AS-base-URL fallback for `aud`, but the function only does byte-equal. Drop the misleading comment; document the byte-equal contract explicitly with a forward note for when a real-world client ever needs origin-fallback. * `selectJWK` filters out `use: enc` keys. The kid path now rejects an enc-purpose key even on direct kid hit; the kid-empty fallback skips enc-only keys. RFC 7517 §4.2 distinguishes signing vs encryption use and our verifier must never use an enc key to verify a JWS. * Drop the dead `var _ = fmt.Sprintf` and the unused `fmt` import that was placeholder for stub helpers. `goimports` clean. * SECURITY: marker on the no-jti decision in client_assertion.go. Today's replay bound is the JWE auth-code single-use guarantee; if that invariant is ever weakened, the assertion's full exp window becomes the replay surface. Inline plan for adding the LRU keyed by jti+kid+iss. * 4 missing tests called out in review: - TestAudienceMatches: direct unit coverage (exact, one-of-many, origin rejected, trailing-slash rejected, empty, unrelated). - TestSelectJWK_EncKeyRejected: kid hit on use=enc returns nil; kid fallback skips enc-only and picks sig. - TestHandleOAuthTokenAuthCode_LenientPrivateKeyJWT: guards the parser accept of private_key_jwt against a future revert. - TestParseCIMDMetadata_ClientSecretRejectedForPrivateKeyJWT: doc declaring private_key_jwt MUST NOT also carry client_secret. - TestFetchJWKS_SSRFBlocked: link-local jwks_uri is allowed at parse but rejected at dial; confirms the resolver's blocklist actually fires for the JWKS fetch path. No behaviour change at the wire for claude.ai or ChatGPT. Co-Authored-By: Claude Opus 4.7 <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.
Follow-up to merged #119. Five items from the post-merge review, no wire-protocol changes.
Fixes
audienceMatchesdoc/code mismatch.verifyClientAssertioncomment said we'd accept AS-base-URL fallback foraud— the function only does byte-equal. Drop the misleading comment; document the byte-equal contract.selectJWKuse=sigfilter. Direct kid hit on ause: enckey now returns nil; kid-empty fallback skips enc-only keys. RFC 7517 §4.2 separates signing vs encryption use.var _ = fmt.Sprintfplaceholder and unusedfmtimport inclient_assertion_test.go.SECURITY:marker on no-jti decision. Inline note for future maintainers — replay window is bounded by the downstream JWE single-use today; if that invariant is ever weakened, we need the jti LRU.New tests
TestAudienceMatches— direct unit coverage (exact / one-of-many / origin rejected / trailing-slash rejected / empty / unrelated).TestSelectJWK_EncKeyRejected— kid hit onuse=encreturns nil; kid fallback picks theuse=sigkey.TestHandleOAuthTokenAuthCode_LenientPrivateKeyJWT— guards the parser-side accept ofprivate_key_jwtagainst a future revert to strict-only.TestParseCIMDMetadata_ClientSecretRejectedForPrivateKeyJWT— doc declaringprivate_key_jwtmust not also carryclient_secret.TestFetchJWKS_SSRFBlocked— link-localjwks_uriallowed at parse, rejected at dial; resolver's SSRF blocklist fires through the JWKS fetcher.Test plan
selectJWKuse=sigfilter is a runtime behaviour change — narrowing).otel-google-mcp,otel-google-gating-mcp,github-mcp,antalya-mcp.none+PKCE; ChatGPT still accepted because it didn't shipclient_assertionanyway).