fix(backend): Clock skew of 0 should not fall back#8359
fix(backend): Clock skew of 0 should not fall back#8359dominic-clerk wants to merge 1 commit intomainfrom
Conversation
Because 0 is falsy the current code fell back to the default value when the clock skew was configured to 0. This changes the syntax to fall back on null-ish values which 0 is not.
🦋 Changeset detectedLatest commit: 0cdc524 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.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@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: |
📝 WalkthroughWalkthroughA patch update was made to the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/jwt/verifyJwt.ts`:
- Line 134: The resolved clockSkew value currently uses nullish coalescing but
can remain NaN; before calling assertExpirationClaim, assertActivationClaim, and
assertIssuedAtClaim validate that clockSkew (the const clockSkew variable) is a
finite number (e.g., Number.isFinite(clockSkew)), and if not replace it with
DEFAULT_CLOCK_SKEW_IN_MS or throw a descriptive error; update the code around
the clockSkew assignment so callers passing NaN cannot bypass claim checks by
ensuring a finite skew is passed into
assertExpirationClaim/assertActivationClaim/assertIssuedAtClaim.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 478f1c59-4092-4cbe-828b-319201b9cd77
📒 Files selected for processing (2)
.changeset/wacky-dryers-hammer.mdpackages/backend/src/jwt/verifyJwt.ts
| ): Promise<JwtReturnType<JwtPayload, TokenVerificationError>> { | ||
| const { audience, authorizedParties, clockSkewInMs, key, headerType } = options; | ||
| const clockSkew = clockSkewInMs || DEFAULT_CLOCK_SKEW_IN_MS; | ||
| const clockSkew = clockSkewInMs ?? DEFAULT_CLOCK_SKEW_IN_MS; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Demonstrate that `??` preserves NaN and time-claim comparisons become false.
node <<'NODE'
const DEFAULT_CLOCK_SKEW_IN_MS = 5000;
const clockSkewInMs = Number(undefined);
const clockSkew = clockSkewInMs ?? DEFAULT_CLOCK_SKEW_IN_MS;
const now = Date.now();
console.log({
clockSkew: String(clockSkew),
expirationRejected: now - 1000 <= now - clockSkew,
notBeforeRejected: now + 1000 > now + clockSkew,
issuedAtRejected: now + 1000 > now + clockSkew,
});
NODERepository: clerk/javascript
Length of output: 166
🏁 Script executed:
cat packages/backend/src/jwt/verifyJwt.ts | head -200 | tail -100Repository: clerk/javascript
Length of output: 3039
🏁 Script executed:
# Search for the function signature and clockSkewInMs type
rg "clockSkewInMs" packages/backend/src/jwt/verifyJwt.ts -B 10 -A 5 | head -100Repository: clerk/javascript
Length of output: 1052
🏁 Script executed:
# Check where clockSkew is used in comparisons
rg "clockSkew" packages/backend/src/jwt/verifyJwt.ts -A 2 -B 2Repository: clerk/javascript
Length of output: 734
🏁 Script executed:
# Check if there are existing input validations in the function
rg "Number.isFinite|isNaN|Infinity" packages/backend/src/jwt/ -A 1 -B 1Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Find and examine the assertion functions that use clockSkew
rg "assertExpirationClaim|assertActivationClaim|assertIssuedAtClaim" packages/backend/src/jwt/ -A 10Repository: clerk/javascript
Length of output: 17574
🏁 Script executed:
# Check the DEFAULT_CLOCK_SKEW_IN_MS value
rg "DEFAULT_CLOCK_SKEW_IN_MS" packages/backend/src/jwt/verifyJwt.ts -B 2 -A 2Repository: clerk/javascript
Length of output: 536
🏁 Script executed:
# Look for type imports and error handling patterns in the file
head -50 packages/backend/src/jwt/verifyJwt.tsRepository: clerk/javascript
Length of output: 1727
🏁 Script executed:
# Look at the full implementation of the assertion functions
rg "export const assertExpirationClaim|export const assertActivationClaim|export const assertIssuedAtClaim" packages/backend/src/jwt/assertions.ts -A 25 | head -150Repository: clerk/javascript
Length of output: 2790
🏁 Script executed:
# Check if there's any validation of clockSkew parameter anywhere in the codebase
rg "clockSkew" packages/backend/src/jwt/assertions.tsRepository: clerk/javascript
Length of output: 565
🏁 Script executed:
# Check if there are any existing tests for NaN clockSkew
rg "NaN|Infinity" packages/backend/src/jwt/__tests__/ -B 2 -A 2Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Verify how clockSkewInMs could realistically be passed as NaN
# Check how verifyJwt is called in the codebase
rg "verifyJwt\(" packages/backend/src/ -B 2 -A 2 | head -50Repository: clerk/javascript
Length of output: 3348
Add validation for finite clockSkew before timestamp comparisons.
Line 134 correctly preserves 0 with the nullish coalescing operator, but it also preserves NaN. If a caller passes NaN (e.g., from invalid config parsing), the timestamp comparisons in assertExpirationClaim, assertActivationClaim, and assertIssuedAtClaim all evaluate to false, silently bypassing exp, nbf, and iat claim validation. Validate that the resolved skew is finite before passing it to the assertions.
Proposed fix
const { audience, authorizedParties, clockSkewInMs, key, headerType } = options;
const clockSkew = clockSkewInMs ?? DEFAULT_CLOCK_SKEW_IN_MS;
+ if (!Number.isFinite(clockSkew)) {
+ return {
+ errors: [
+ new TokenVerificationError({
+ action: TokenVerificationErrorAction.EnsureClerkJWT,
+ reason: TokenVerificationErrorReason.TokenVerificationFailed,
+ message: '`clockSkewInMs` must be a finite number.',
+ }),
+ ],
+ };
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/jwt/verifyJwt.ts` at line 134, The resolved clockSkew
value currently uses nullish coalescing but can remain NaN; before calling
assertExpirationClaim, assertActivationClaim, and assertIssuedAtClaim validate
that clockSkew (the const clockSkew variable) is a finite number (e.g.,
Number.isFinite(clockSkew)), and if not replace it with DEFAULT_CLOCK_SKEW_IN_MS
or throw a descriptive error; update the code around the clockSkew assignment so
callers passing NaN cannot bypass claim checks by ensuring a finite skew is
passed into assertExpirationClaim/assertActivationClaim/assertIssuedAtClaim.
jacekradko
left a comment
jacekradko
left a comment
There was a problem hiding this comment.
Good enhancement, but we should add tests for this
2 participants
Description
Because 0 is falsy the current code fell back to the default value when the clock skew was configured to 0. This changes the syntax to fall back on null-ish values which 0 is not.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change