fix(backend): validate JWT issuer (iss) claim when an issuer option is provided#8772
fix(backend): validate JWT issuer (iss) claim when an issuer option is provided#8772jacekradko wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 4602902 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.
|
📝 WalkthroughWalkthroughThis PR adds opt-in issuer claim validation to JWT verification in the Clerk backend. ChangesJWT Issuer Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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: |
API Changes Report
Summary
@clerk/backendCurrent version: 3.5.0 Subpath
|
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.
verifyJwtnever actually 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 it up.assertIssuerClaimis the actual change, and it skips when no issuer is configured:Nobody passes
issuertoday (including the networklessjwtKeypath and the machine/OAuth verifiers that shareverifyJwt), so current behavior doesn't change, andverifyToken/authenticateRequestpick the option up through the existing options composition. The eleven inert tests now exercise the assertion, and I added the negatives that were missing: a mismatched string, a rejecting predicate, and averifyToken({ issuer })case to confirm the option reaches the public API. This does not turn issuer validation on by default inauthenticateRequest; that's a separate change.Summary by CodeRabbit
New Features
issueroption. Provide a string for exact-match validation or a function for custom validation logic. When omitted, issuer validation is skipped entirely.Bug Fixes
issueroption is provided, rather than ignoring it.