feat(jwt)!: require aud when an audience is expected - #130
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesJWT audience verification
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jwt/src/verify.test.ts (1)
160-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an empty-string audience.
The contract includes
aud: "", but these tests cover onlyaud: []and a missing claim. Add a case that verifiesrequireAudience: truerejects the empty string branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jwt/src/verify.test.ts` around lines 160 - 193, Add a test alongside the existing empty-array case in the verifyJwt tests, using a mock verified payload with aud set to an empty string and requireAudience enabled. Assert that verifyJwt rejects with the existing “JWT audience is required but missing” error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/jwt/src/verify.test.ts`:
- Around line 149-156: Update the verifyJWT invocation assertions in this test
to inspect this test’s call rather than a prior suite call: clear the mock
history in beforeEach or reference the latest mock call before asserting
requireAudience is absent. Preserve the existing audience assertion.
---
Nitpick comments:
In `@packages/jwt/src/verify.test.ts`:
- Around line 160-193: Add a test alongside the existing empty-array case in the
verifyJwt tests, using a mock verified payload with aud set to an empty string
and requireAudience enabled. Assert that verifyJwt rejects with the existing
“JWT audience is required but missing” error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d341f587-2a22-4760-bf48-e95f555af5fe
📒 Files selected for processing (3)
.changeset/jwt-require-audience.mdpackages/jwt/src/verify.test.tspackages/jwt/src/verify.ts
b6875fa to
ea70b1f
Compare
|
@venables a review when convenient would be great. Companion to #129: an opt-in |
domleboss97
left a comment
domleboss97
left a comment
There was a problem hiding this comment.
Good catch here! I think, however, we shouldn't have a standalone requireAudience flag - whether or not audience was passed is sufficient signal. Couple reasons for this:
- it's how the JOSE and PyJWT libraries behave
- the
requireAudienceflag doesn't make sense if passed alone. PassingrequireAudience: truewith no audience fails everything.
I'd say let's do it so that, when audience is supplied, missing/empty aud fails verification.
I think the above would break verifyA2ASignedMessage, but I think we could do one of two things there: (1) just drop the audience: did, because the i think the messages we're producing don't even have aud on them so it'd be fine or (2) drop audience but, after verifyJwt returns, do the audience validation manually ourselves.
Per review: drop the standalone requireAudience flag. Supplying audience to verifyJwt now fails a token whose aud claim is missing or empty, matching jose and PyJWT semantics; did-jwt alone only matches aud when present. A2A call sites differ and are handled accordingly: - handshake verification keeps audience: did, since handshake JWTs have embedded aud (aud: params.recipient) since the original implementation - signed-message verification drops audience: did, since signed messages carry no aud (createSignedA2AMessage has no recipient parameter) and the option never provided a check there Changeset rewritten for the new semantics; tests updated on both packages.
EfeDurmaz16
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/jwt/src/verify.test.ts (1)
157-190: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover empty-string audience claims.
This only exercises
aud: []; add cases foraud: ""andaud: [""]to lock down bothhasAudiencebranches described by the release note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jwt/src/verify.test.ts` around lines 157 - 190, Extend the audience validation tests near the existing empty-array case to cover JWT payloads with aud set to an empty string and an array containing an empty string. Mock each verified result through verifyJWT and assert verifyJwt rejects with “JWT audience is required but missing,” covering both hasAudience branches while preserving the existing test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/jwt/src/verify.test.ts`:
- Around line 157-190: Extend the audience validation tests near the existing
empty-array case to cover JWT payloads with aud set to an empty string and an
array containing an empty string. Mock each verified result through verifyJWT
and assert verifyJwt rejects with “JWT audience is required but missing,”
covering both hasAudience branches while preserving the existing test setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd7026e2-78a2-43e1-a8c4-2db9a334ea10
📒 Files selected for processing (5)
.changeset/jwt-require-audience.mdpackages/ack-id/src/a2a/verify.test.tspackages/ack-id/src/a2a/verify.tspackages/jwt/src/verify.test.tspackages/jwt/src/verify.ts
|
Agreed, done in a5a6d0d: the flag is gone, supplying audience now fails on a missing or empty One thing I found while applying it to the A2A sites: the two paths differ.
so I went with your (1) and dropped audience from that call. No behavior change, did-jwt was skipping the check there anyway. Worth a follow-up maybe: the identity-a2a README documents Happy to do a small PR adding a recipient param to |
Lock down both hasAudience branches: aud as "", [] and [""] all reject when an audience is expected.
Added in 8a1ef67: the empty-aud test is now an it.each over [], "" and [""], so both hasAudience branches are covered. |
| }, | ||
| ) | ||
|
|
||
| it("throws when an audience is expected and the aud claim is missing", async () => { |
There was a problem hiding this comment.
nit - i think this could just fold into the above if the it.each(...) included the missing case
…dMessage The option stays in the type for callers; signed messages carry no aud claim to verify it against today.
2 participants
What
verifyJwtnow fails a token whoseaudclaim is missing or empty wheneverthe caller supplies an
audience. No flag: supplyingaudienceis thesignal, matching jose and PyJWT semantics (this PR originally added an
opt-in
requireAudienceboolean; reshaped per review).Why
did-jwt'sverifyJWTonly runs its audience match when the token carriesan
audclaim, so a token that omitsaudverifies even when the callersupplies an
audience, allowing cross-service replay. jose throwsERR_JWT_CLAIM_VALIDATION_FAILED(reason: "missing") and PyJWT raisesMissingRequiredClaimErrorin the same situation; did-jwt is the outlier.A quick code search also found an external consumer passing
audiencetoverifyJwtwhose doc comment assumesaudis enforced, so the strictdefault silently fixes real usage.
A2A call sites
The two
ack-idA2A verifiers turn out to differ:audience: did: handshake JWTs have embeddedaud: params.recipientsince the original implementation(
sign-message.ts), so the strict rule just works there.audience: did:createSignedA2AMessagehas norecipient parameter, its JWTs never carry
aud, and the option neverprovided a check on that path. No behavior change.
Possible follow-up (separate PR if wanted): give
createSignedA2AMessagearecipient parameter and verify
audstrictly on signed messages too. Theidentity-a2a README already documents
audon signed messages as MITMprotection, but the code never implemented it. It changes that function's
signature; a code search finds no external consumers of the a2a module.
Change
VerifyJwtOptions.requireAudienceremoved; presence check keys onoptions.audiencehasAudiencehelper unchanged (non-empty string or array with anon-empty entry)
verifyA2ASignedMessageno longer passesaudience@agentcommercekit/jwtminor (behavior changedocumented),
@agentcommercekit/ack-idpatchTests
packages/jwt: expected-audience-with-matching-aud passes; missing andempty-array
audreject; no-audience-expected accepts aud-less tokens.packages/ack-id: handshake still assertsaudience; signed-messageasserts no
audienceis passed.pnpm run check(build + lint + format +all tests) green: jwt 22, ack-id 39.
AI assistance disclosure
Per the repository AI policy: implemented and tested by gpt 5.6-sol and fable 5, then reviewed and understood by me. I can explain the implementation and its trade-offs without AI aid. Tests, typecheck, and formatting pass locally against the package suite.
Summary by CodeRabbit
audclaim is missing or empty.audclaim (handshake verification still validatesaud).