fix(backend): harden machine-token verification and redact debug-logged tokens#8744
Merged
Conversation
🦋 Changeset detectedLatest commit: acf2352 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.
|
Contributor
|
Break Check: no API changes detected across the tracked packages. Last ran on |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a type guard in verifyMachineAuthToken to ensure decodedResult.payload.sub is a string before calling startsWith, preventing runtime errors and routing non-string/missing subs to OAuth verification. It extends auth debug redaction in the backend to truncate sessionToken, tokenInHeader, sessionTokenInCookie, refreshTokenInCookie, devBrowserToken, and handshakeToken, and updates the nextjs logFormatter to recursively truncate credential-like keys at any nesting depth. Both areas include unit/regression tests and Changeset entries. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tokens/authObjects.ts`:
- Around line 173-178: The debug output still includes long-lived bearer fields
(refreshTokenInCookie, devBrowserToken, handshakeToken) via AuthenticateContext
when signedInAuthObject builds debugData, so update the redaction logic to
truncate those fields the same way as sessionToken: locate the lines setting
res.sessionToken/res.tokenInHeader/res.sessionTokenInCookie and also redact
res.refreshTokenInCookie, res.devBrowserToken, and res.handshakeToken (e.g., set
each to (value||'').substring(0,7) or remove them from the debug object) and
ensure signedInAuthObject uses the redacted AuthenticateContext when creating
debugData so no full tokens are ever spread into logs.
🪄 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: edfaa399-5a58-4e32-a53e-148ec84b39d1
📒 Files selected for processing (6)
.changeset/machine-token-sub-guard.md.changeset/redact-tokens-debug-output.mdpackages/backend/src/tokens/__tests__/authObjects.test.tspackages/backend/src/tokens/__tests__/verify.test.tspackages/backend/src/tokens/authObjects.tspackages/backend/src/tokens/verify.ts
A malformed OAuth (at+jwt) bearer token with a missing or non-string sub caused verifyMachineAuthToken to throw a TypeError on decodedResult.payload.sub.startsWith() before token verification ran, surfacing as an unhandled error during request authentication. Mirror the typeof guard already used by isM2MJwt so such tokens are classified and rejected with a typed verification error instead.
The auth object's debug payload spread the full sessionToken, tokenInHeader and sessionTokenInCookie into its output, so SDKs that enable middleware debug logging wrote live bearer credentials to logs. Truncate them to a short, non-reconstructable prefix, matching the existing secretKey/jwtKey handling.
jacekradko
force-pushed
the
jacek/backend-token-hardening
branch
from
4637795 to
f9d1a12
Compare
@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: |
…tput The debug log formatter only masked top-level string values, so a bearer token nested in a debug object (for example under debug.sessionToken) was stringified in full. Recursively truncate known credential keys at any depth as a defense-in-depth backstop alongside the source-level redaction in @clerk/backend.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/nextjs/src/utils/logFormatter.ts (1)
42-52: ⚡ Quick winAdd an explicit return type to the exported formatter.
Please annotate
logFormatteras: stringso this public utility matches the repo's TypeScript API rule.As per coding guidelines,
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs.🤖 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/nextjs/src/utils/logFormatter.ts` around lines 42 - 52, Add an explicit return type annotation of string to the exported formatter function so the public API follows the repo rule; update the signature of logFormatter (which accepts LogEntry and uses maskSecretKey and redactSensitive internally) to declare it returns string (e.g., annotate the const/arrow function with : string) and keep the existing implementation unchanged.packages/nextjs/src/utils/__tests__/logFormatter.test.ts (2)
6-29: ⚡ Quick winCover
secretKeyandjwtKeyin the redaction matrix.
logFormatternow special-cases five sensitive keys, but this regression only asserts the three token fields. AddingsecretKeyandjwtKeyhere would better lock the security contract to the same key set used inpackages/backend/src/tokens/authObjects.ts:168-181.Suggested assertion expansion
debug: { + secretKey: 'sk_test_51H8P1h7v123456789', + jwtKey: 'eyJhbGciOiJSUzI1NiJ9.jwt-key-should-not-appear', sessionToken: 'eyJhbGciOiJSUzI1NiJ9.payload.full-session-segment-should-not-appear', tokenInHeader: 'eyJhbGciOiJSUzI1NiJ9.payload.header-segment-should-not-appear', sessionTokenInCookie: 'eyJhbGciOiJSUzI1NiJ9.payload.cookie-segment-should-not-appear', }, @@ + expect(output).not.toContain('jwt-key-should-not-appear'); + expect(output).toContain('"secretKey": "sk_test"'); + expect(output).toContain('"jwtKey": "eyJhbGc"'); expect(output).toContain('"sessionToken": "eyJhbGc"');🤖 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/nextjs/src/utils/__tests__/logFormatter.test.ts` around lines 6 - 29, The test 'truncates sensitive token keys nested in debug objects' currently asserts redaction for three token fields; extend it to also assert that any values under the additional sensitive keys secretKey and jwtKey are redacted by logFormatter and that only the short non-reconstructable prefix remains (e.g., expect output not toContain the full secret/jwt payload and toContain the short prefix form). Update the test entry object's debug payload to include secretKey and jwtKey sample long tokens and add corresponding expect(...) checks mirroring the existing sessionToken/tokenInHeader/sessionTokenInCookie assertions so the test covers the same key set used by the redaction logic.
1-3: ⚡ Quick winAvoid
as anyin these tests.These casts bypass the
LogEntrycontract the test is supposed to exercise. Prefer a typed fixture (const entry: LogEntry = ...orsatisfies LogEntry) and a type-only import from../debugLogger.As per coding guidelines, `**/*.{ts,tsx}`: Avoid `any` type - prefer `unknown` when type is uncertain, then narrow with type guards, and `Verify type-only imports where possible for better tree-shaking in code review`.Suggested cleanup
+import type { LogEntry } from '../debugLogger'; import { describe, expect, it } from 'vitest'; import { logFormatter } from '../logFormatter'; @@ - const entry = [ + const entry: LogEntry = [ @@ - const output = logFormatter(entry as any); + const output = logFormatter(entry); @@ - const entry = ['auth', { debug: { sessionToken: 'eyJhbGc' } }]; + const entry: LogEntry = ['auth', { debug: { sessionToken: 'eyJhbGc' } }]; @@ - const output = logFormatter(entry as any); + const output = logFormatter(entry);Also applies to: 7-19, 32-34
🤖 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/nextjs/src/utils/__tests__/logFormatter.test.ts` around lines 1 - 3, Tests use `as any` to cast fixtures which bypasses the LogEntry contract; replace those casts with proper typed fixtures and type-only imports. Define test entries with the LogEntry type (or `satisfies LogEntry`) instead of `as any`, import the LogEntry type from `../debugLogger` as a type-only import, and when a value is genuinely unknown use `unknown` then narrow with guards before passing to logFormatter; update occurrences around the test file where fixtures are cast (references: logFormatter and LogEntry) to remove `as any` and ensure test fixtures conform to the LogEntry shape.
🤖 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/nextjs/src/utils/__tests__/logFormatter.test.ts`:
- Around line 6-29: The test 'truncates sensitive token keys nested in debug
objects' currently asserts redaction for three token fields; extend it to also
assert that any values under the additional sensitive keys secretKey and jwtKey
are redacted by logFormatter and that only the short non-reconstructable prefix
remains (e.g., expect output not toContain the full secret/jwt payload and
toContain the short prefix form). Update the test entry object's debug payload
to include secretKey and jwtKey sample long tokens and add corresponding
expect(...) checks mirroring the existing
sessionToken/tokenInHeader/sessionTokenInCookie assertions so the test covers
the same key set used by the redaction logic.
- Around line 1-3: Tests use `as any` to cast fixtures which bypasses the
LogEntry contract; replace those casts with proper typed fixtures and type-only
imports. Define test entries with the LogEntry type (or `satisfies LogEntry`)
instead of `as any`, import the LogEntry type from `../debugLogger` as a
type-only import, and when a value is genuinely unknown use `unknown` then
narrow with guards before passing to logFormatter; update occurrences around the
test file where fixtures are cast (references: logFormatter and LogEntry) to
remove `as any` and ensure test fixtures conform to the LogEntry shape.
In `@packages/nextjs/src/utils/logFormatter.ts`:
- Around line 42-52: Add an explicit return type annotation of string to the
exported formatter function so the public API follows the repo rule; update the
signature of logFormatter (which accepts LogEntry and uses maskSecretKey and
redactSensitive internally) to declare it returns string (e.g., annotate the
const/arrow function with : string) and keep the existing implementation
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c5a8281e-87e6-4378-847f-c0d4df589458
📒 Files selected for processing (3)
.changeset/redact-nested-tokens-debug-formatter.mdpackages/nextjs/src/utils/__tests__/logFormatter.test.tspackages/nextjs/src/utils/logFormatter.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/redact-nested-tokens-debug-formatter.md
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
2 participants
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.
Two backend hardening fixes that came out of a security scan of the token-verification paths.
verifyMachineAuthTokencould throw on a crafted token: anat+jwtJWT with a missing or non-stringsubreacheddecodedResult.payload.sub.startsWith(...)before verification ran, and theTypeErrorescaped the{ errors }return contract therequest.tscallers depend on. Because the framework middlewares authenticate withacceptsToken: 'any', an unauthenticated request could trigger it through theAuthorizationheader. The fix is the sametypeof sub === 'string'guardisM2MJwtalready uses, so the token falls through to OAuth verification and comes back as a typed error.Separately, the
authdebug payload was logging live tokens.createDebugalready truncatessecretKey/jwtKey, butsignedInAuthObjectspreads the wholeauthenticateContextinto the payload, so every bearer field riding along on it leaked at full length once middleware debug logging was on. That's the session token plustokenInHeader,sessionTokenInCookie,refreshTokenInCookie,devBrowserTokenandhandshakeToken. The refresh token is the one that stings: it's long-lived. All of them are now truncated at the source, which covers every consumer rather than just the Next.js formatter where this first surfaced.Both paths gained regression tests that I confirmed fail without the fix.