feat(backend): validate JWT issuer (iss) claim when an issuer option is provided#8772
feat(backend): validate JWT issuer (iss) claim when an issuer option is provided#8772jacekradko wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: b0232f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an opt-in issuer option (string or string[]) to verifyJwt()/verifyToken() that validates the JWT iss claim for session tokens; removes predicate-based issuer resolvers, introduces TokenInvalidIssuer, wires assertIssuerClaim into verifyJwt, scopes machine-token calls to avoid issuer checks, and adds tests and a changeset. ChangesJWT Issuer Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/backend/src/jwt/__tests__/assertions.test.ts`:
- Around line 237-242: Update the tests in
packages/backend/src/jwt/__tests__/assertions.test.ts to stop treating an
explicitly configured-but-empty issuer as "opt-in": change the expectation for
assertIssuerClaim(iss, '') to expect it to throw (i.e., configured-empty should
fail), and add a new predicate-edge-case test that calls assertIssuerClaim with
a non-string or missing iss (e.g., an object with iss:number or undefined)
alongside a predicate function to verify it fails in a controlled way; reference
the assertIssuerClaim function and adjust the related assertions in both the
block around the current opt-in test and the second block noted (lines ~254-267)
to cover these failure paths.
In `@packages/backend/src/jwt/assertions.ts`:
- Around line 99-115: assertIssuerClaim currently treats an empty-string issuer
as "not configured" and calls predicate resolvers with iss cast to string, which
can skip validation or throw non-TokenVerificationError exceptions; change the
guard to check for issuer === undefined (so empty string is treated as
configured) and ensure the predicate is only invoked when typeof iss ===
'string' (otherwise throw TokenVerificationError with
TokenVerificationErrorAction.EnsureClerkJWT and
TokenVerificationErrorReason.TokenInvalidIssuer); update the isValid computation
in assertIssuerClaim to perform the typeof iss === 'string' check before calling
a function issuer(iss) and produce the stable token-invalid-issuer error when
iss is missing or malformed.
🪄 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: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 217d0731-ea9e-47a4-a02c-b7fb4e77af3a
📒 Files selected for processing (7)
.changeset/backend-issuer-validation.mdpackages/backend/src/errors.tspackages/backend/src/jwt/__tests__/assertions.test.tspackages/backend/src/jwt/__tests__/verifyJwt.test.tspackages/backend/src/jwt/assertions.tspackages/backend/src/jwt/verifyJwt.tspackages/backend/src/tokens/__tests__/verify.test.ts
| it('does not throw if no issuer is provided (opt-in)', () => { | ||
| expect(() => assertIssuerClaim(iss)).not.toThrow(); | ||
| expect(() => assertIssuerClaim(iss, undefined)).not.toThrow(); | ||
| expect(() => assertIssuerClaim(undefined)).not.toThrow(); | ||
| expect(() => assertIssuerClaim(iss, '')).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Adjust issuer tests to avoid codifying validation bypass and cover predicate edge-case errors.
The current case at Line 241 (issuer: '') asserts skip behavior even though issuer is supplied. Add/adjust tests so configured-but-empty issuer fails, and include a predicate case with non-string/missing iss to verify controlled failure behavior.
Suggested test updates
it('does not throw if no issuer is provided (opt-in)', () => {
expect(() => assertIssuerClaim(iss)).not.toThrow();
expect(() => assertIssuerClaim(iss, undefined)).not.toThrow();
expect(() => assertIssuerClaim(undefined)).not.toThrow();
- expect(() => assertIssuerClaim(iss, '')).not.toThrow();
+ expect(() => assertIssuerClaim(iss, '')).toThrow(`Invalid JWT issuer claim (iss) "https://clerk.example.com".`);
});
@@
it('throws if iss is missing or not a string when an issuer string is required', () => {
expect(() => assertIssuerClaim(undefined, iss)).toThrow(`Invalid JWT issuer claim (iss) undefined.`);
expect(() => assertIssuerClaim(42, iss)).toThrow(`Invalid JWT issuer claim (iss) 42.`);
});
+
+ it('throws if iss is missing when issuer resolver is a predicate', () => {
+ expect(() => assertIssuerClaim(undefined, i => i.startsWith('https://clerk.'))).toThrow(
+ `Invalid JWT issuer claim (iss) undefined.`,
+ );
+ });As per coding guidelines, **/*.{test,spec}.{ts,tsx} should verify proper error handling and edge cases for new functionality.
Also applies to: 254-267
🤖 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/backend/src/jwt/__tests__/assertions.test.ts` around lines 237 -
242, Update the tests in packages/backend/src/jwt/__tests__/assertions.test.ts
to stop treating an explicitly configured-but-empty issuer as "opt-in": change
the expectation for assertIssuerClaim(iss, '') to expect it to throw (i.e.,
configured-empty should fail), and add a new predicate-edge-case test that calls
assertIssuerClaim with a non-string or missing iss (e.g., an object with
iss:number or undefined) alongside a predicate function to verify it fails in a
controlled way; reference the assertIssuerClaim function and adjust the related
assertions in both the block around the current opt-in test and the second block
noted (lines ~254-267) to cover these failure paths.
Source: Coding guidelines
| export const assertIssuerClaim = (iss: unknown, issuer?: IssuerResolver) => { | ||
| // No issuer configured, skip validation. Preserves the default behavior, matching how | ||
| // the audience and authorized parties claims are only checked when an option is provided. | ||
| if (!issuer) { | ||
| return; | ||
| } | ||
|
|
||
| const isValid = typeof issuer === 'function' ? issuer(iss as string) : typeof iss === 'string' && iss === issuer; | ||
|
|
||
| if (!isValid) { | ||
| throw new TokenVerificationError({ | ||
| action: TokenVerificationErrorAction.EnsureClerkJWT, | ||
| reason: TokenVerificationErrorReason.TokenInvalidIssuer, | ||
| message: `Invalid JWT issuer claim (iss) ${JSON.stringify(iss)}.`, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fail closed for configured issuer and guard predicate execution.
Line 102 currently treats issuer: '' as “not configured”, which silently skips validation. Line 106 also calls predicate resolvers with iss as string, so malformed tokens can surface non-TokenVerificationError exceptions instead of a stable token-invalid-issuer failure.
Suggested fix
-export const assertIssuerClaim = (iss: unknown, issuer?: IssuerResolver) => {
+export const assertIssuerClaim = (iss: unknown, issuer?: IssuerResolver): void => {
// No issuer configured, skip validation. Preserves the default behavior, matching how
// the audience and authorized parties claims are only checked when an option is provided.
- if (!issuer) {
+ if (typeof issuer === 'undefined') {
return;
}
- const isValid = typeof issuer === 'function' ? issuer(iss as string) : typeof iss === 'string' && iss === issuer;
+ const issValue = typeof iss === 'string' ? iss : undefined;
+ let isValid = false;
+
+ if (typeof issuer === 'function') {
+ try {
+ isValid = typeof issValue === 'string' && issuer(issValue);
+ } catch {
+ isValid = false;
+ }
+ } else {
+ isValid = typeof issValue === 'string' && issValue === issuer;
+ }
if (!isValid) {
throw new TokenVerificationError({
action: TokenVerificationErrorAction.EnsureClerkJWT,
reason: TokenVerificationErrorReason.TokenInvalidIssuer,
message: `Invalid JWT issuer claim (iss) ${JSON.stringify(iss)}.`,
});
}
};🤖 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/backend/src/jwt/assertions.ts` around lines 99 - 115,
assertIssuerClaim currently treats an empty-string issuer as "not configured"
and calls predicate resolvers with iss cast to string, which can skip validation
or throw non-TokenVerificationError exceptions; change the guard to check for
issuer === undefined (so empty string is treated as configured) and ensure the
predicate is only invoked when typeof iss === 'string' (otherwise throw
TokenVerificationError with TokenVerificationErrorAction.EnsureClerkJWT and
TokenVerificationErrorReason.TokenInvalidIssuer); update the isValid computation
in assertIssuerClaim to perform the typeof iss === 'string' check before calling
a function issuer(iss) and produce the stable token-invalid-issuer error when
iss is missing or malformed.
API Changes Report
Summary
@clerk/backendCurrent version: 3.6.1 Subpath
|
jacekradko
changed the title
jacekradko
force-pushed
the
jacek/backend-issuer-validation
branch
from
73cdabf to
b0232f2
Compare
1 participant
verifyJwtnever validated theissclaim.VerifyJwtOptionshad noissuerfield, theIssuerResolvertype injwt/assertions.tswas unused, and eleven tests inverifyJwt.test.tspassed anissuer:that nothing read, so they passed regardless. This wires the option up for real: pass a string or a list of strings, the token'sissmust match one exactly, and mismatches surface as a newtoken-invalid-issuererror. Omitting the option skips the check, the same opt-in shape asaudienceandazp.Two decisions worth scrutiny. The option is
string | string[]rather than a predicate, because middleware options cross a JSON serialization boundary in Next.js (encryptClerkRequestData) where a function value would be silently dropped. And machine token verification now passes an explicit options object toverifyJwtinstead of spreading the caller's options through, so anissuer(oraudience/authorizedParties) meant for session tokens can't reject M2M and OAuth tokens, whoseissdiffers. Both paths have regression tests, and the previously inert tests now exercise the assertion.Summary by CodeRabbit
Release Notes
verifyToken()andverifyJwt()