feat(js): attach an optional server-configured session token to sign-in - #9299
Conversation
Mints an opaque, random correlation id and acquires a signed Protect session token once per browser session, shared across tabs under a lock. The token, the correlation id and an acquisition status travel in the form-encoded body of sign-in and sign-up POSTs. Acquisition never blocks a sign-in: on timeout, script-load failure or a non-2xx response a structured status travels in the token's place and the request proceeds. Instances whose loader config does not reference the new placeholders keep today's behaviour and store nothing in the browser.
🦋 Changeset detectedLatest commit: 8cace84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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: |
Follows the EDR-0020 amendment (clerk/protect#408): the session token now ships inline on the loader's own global rather than behind a second request, so the gate costs no extra round trip on the auth critical path. `tokenUrl` becomes an explicit opt-in for the upgrade mint, and is the only path that can report `fetch_error` or an HTTP status. That removes the derive-the-endpoint-from-an-attribute fallback, which resolved against `document.baseURI` (pointing polling at the app's own origin) and stripped a trailing `{cid}` via relative resolution. The session now owns the token loader: it injects it under the acquisition lock and reads the token when the element fires `load`. Every other loader is applied on every page load, rather than all loaders being suppressed whenever a token happened to be cached. Also fixed: - A malformed loader entry no longer escapes `Protect.load()` and fails `Clerk.load()` for every visitor. - `readStored` falls back to the in-memory store, so a token written there under an exhausted quota is no longer read back as absent. - `getRequestParams` is bounded by the acquisition deadline, and a server-supplied `tokenTimeoutMs` is capped, so a sign-in cannot be stalled before dispatch. It can no longer reject into the request either. - A settled, tokenless acquisition re-arms after a cooldown instead of being replayed for the life of the tab. - The token store and lock are namespaced per instance, so two instances on one origin no longer share a token. - A stored token is validated against a maximum lifetime and length, so a planted entry cannot suppress acquisition indefinitely. - The upgrade mint retries a transient 408/429/5xx within its deadline. - `{instance_id}` alone no longer mints and persists a client id. - `textContent` placeholders are detected and substituted. - `isMergeableBody` admits only plain objects, so a Blob or array body is no longer spread away.
Aligns with clerk/protect: the loader global carries `ready`, a promise
resolving to `{token, exp}` or `{status: "no_token"}`, rather than the
token directly. It is the loader's completion signal and the seam where
page-side probes and the EDR-0020 proof-of-possession challenge will
live, so the token is awaited (raced against the acquisition deadline)
instead of read synchronously.
`{sdkver}` joins the placeholder set, substituted with the build version.
Its presence is what tells the server this build interpolates at all, and
so can be served the current shape; `/v1/environment` is cached and
cannot vary by SDK version, so the negotiation has to happen per request.
A build that leaves it verbatim is served the base `{v, id}` shape.
`no_token` joins the status set, matching Warden's allowlist. It is what
the base shape, a load with no correlation id, and a failed mint all
report. Reporting `timeout` for those would mark every load between this
shipping and the server half deploying as a failure, making a normal
rollout look like an outage.
A `ready` that rejects is treated as nothing served rather than
propagated; the contract says it never rejects, but an unhandled
rejection on every page load is not worth taking on trust.
API Changes Report
Summary
🔴 Breaking changes index (1)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/sharedCurrent version: 4.28.1 Subpath
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 (11)
packages/clerk-js/bundlewatch.config.json (1)
3-7: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the Bundlewatch ceilings. Set them to
550KB,77KB,119KB,317KB, and77KBfor the five bundles. The current values add unnecessary headroom.🤖 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/clerk-js/bundlewatch.config.json` around lines 3 - 7, Update the Bundlewatch maxSize values for the five bundle entries in order: clerk.js to 550KB, clerk.browser.js to 77KB, clerk.legacy.browser.js to 119KB, clerk.no-rhc.js to 317KB, and clerk.native.js to 77KB.packages/shared/src/types/protectConfig.ts (1)
23-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the upper bound for
tokenTimeoutMs.
clampTimeoutinpackages/clerk-js/src/core/protectSession.tslimits this value to 10000 ms and falls back to 5000 for non-positive or non-finite values. The doc comment states only the default. State the ceiling so instance operators know that a larger configured value has no effect.📝 Proposed doc update
/** * How long to wait for the token before giving up and reporting a status instead. Defaults to - * 5000. + * 5000, and is capped at 10000 by the SDK. */ tokenTimeoutMs?: number;🤖 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/shared/src/types/protectConfig.ts` around lines 23 - 27, Update the documentation for tokenTimeoutMs in the protect configuration type to state that values are capped at 10000 ms, while retaining the existing 5000 ms default description.packages/clerk-js/src/core/protectSession.ts (2)
419-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to
ProtectSession.create.
createis a public static factory on an exported class. Its return type is inferred asProtectSession | undefined. Declare it so the public surface stays stable if the body changes.🔧 Proposed fix
- static create(loaders: ProtectLoader[], instanceId: string | undefined, applyLoader: ApplyLoader) { + static create( + loaders: ProtectLoader[], + instanceId: string | undefined, + applyLoader: ApplyLoader, + ): ProtectSession | undefined {As per coding guidelines: "Always define explicit return types for functions, especially public APIs". Based on learnings: enforce explicit return type annotations for exported functions and 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/clerk-js/src/core/protectSession.ts` around lines 419 - 425, Update the public static factory method ProtectSession.create with an explicit return type annotation of ProtectSession | undefined, preserving its existing undefined result for empty templated loaders and ProtectSession result otherwise.Sources: Coding guidelines, Learnings
5-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the volume of narrative comments.
The file carries many multi-line explanatory blocks. Several restate what the code already shows, for example lines 214-218 and 601-604. Keep the design rationale that is not obvious from the code, such as why
no_tokenis distinct fromtimeout, and shorten the rest to one line each. Consider moving the long module-level narrative to a design document.As per coding guidelines: "Keep code comments minimal. Add comments only when critical to explain why a non-obvious change was made; never restate code behavior, and keep warranted comments to one terse line rather than a verbose multi-line block."
Also applies to: 103-108, 153-156, 214-218, 245-248, 290-299, 506-509, 518-521, 601-604, 687-691
🤖 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/clerk-js/src/core/protectSession.ts` around lines 5 - 20, Reduce comments throughout protectSession.ts, especially the module-level block and the listed ranges, to terse single-line comments or remove them when they merely restate the surrounding code. Preserve only non-obvious design rationale, including why no_token differs from timeout, and retain concise comments only where they explain that rationale or another critical implementation decision.Source: Coding guidelines
packages/clerk-js/src/core/fapiClient.ts (2)
236-244: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the
getProtectParamsawait insidefapiClient.This
awaitsits on the sign-in and sign-up critical path. The current producer,Protect.getRequestParams, bounds itself with an acquisition deadline of at most 10 s and never rejects, so the request is not blocked indefinitely today.getProtectParamsis a public option onFapiClientOptions, so any other supplier can hang the request forever with no recovery.Add a local deadline so
fapiClientdoes not depend on the caller for that guarantee.🛡️ Proposed defensive bound
+const PROTECT_PARAMS_TIMEOUT_MS = 10_000; + +function withTimeout<T>(promise: Promise<T>, ms: number): Promise<T | undefined> { + return new Promise(resolve => { + const timer = setTimeout(() => resolve(undefined), ms); + const settle = (value: T | undefined) => { + clearTimeout(timer); + resolve(value); + }; + promise.then(settle, () => settle(undefined)); + }); +}- const protectParams = await options.getProtectParams().catch(() => undefined); + const protectParams = await withTimeout(options.getProtectParams(), PROTECT_PARAMS_TIMEOUT_MS);🤖 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/clerk-js/src/core/fapiClient.ts` around lines 236 - 244, Bound the getProtectParams() await in the fapiClient request flow with a local timeout so a hanging public option cannot block sign-in or sign-up indefinitely. Preserve the existing fallback behavior by treating timeout or supplier rejection as undefined and continuing without protect parameters.
240-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant assignment and cast.
isMergeableBodynarrowsbodytoRecord<string, unknown> | undefinedinside this block, so theas Record<string, unknown>cast adds nothing. Line 242 is also redundant, because lines 246-248 reassignrequestInit.bodyfor every plain-object body.♻️ Proposed simplification
const protectParams = await options.getProtectParams().catch(() => undefined); if (protectParams) { - body = { ...((body ?? {}) as Record<string, unknown>), ...protectParams } as unknown as BodyInit; - requestInit.body = body; + body = { ...(body ?? {}), ...protectParams } as unknown as BodyInit; }🤖 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/clerk-js/src/core/fapiClient.ts` around lines 240 - 243, In the protectParams handling near isMergeableBody, remove the redundant Record<string, unknown> cast and the immediate requestInit.body assignment; retain only the merged body assignment, since the later plain-object body flow already updates requestInit.body.packages/clerk-js/src/core/protect.ts (1)
38-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winType
#applyinput asunknown[].
config.loaderscomes from the server response and is not validated before this call. The tests passnull,'nope'and42in that array. The parameter typeProtectLoader[]states a guarantee that does not hold, andisLoaderis the guard that establishes it. Declare the input asunknown[]so the narrowing performed byisLoaderis visible in the types.🔧 Proposed change
- `#apply`(configured: ProtectLoader[], instanceId?: string): void { + `#apply`(configured: unknown[], instanceId?: string): void { // Rollout is decided before anything else, because the session is only meaningful for the // loaders we are actually going to apply. const loaders = configured.filter(loader => isLoader(loader) && inRollout(loader));
isLoaderalready returnsloader is ProtectLoader, soloadersstill narrows toProtectLoader[].🤖 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/clerk-js/src/core/protect.ts` around lines 38 - 41, Update the `#apply` method parameter from ProtectLoader[] to unknown[] to reflect that server-provided loader values are unvalidated. Preserve the existing configured.filter call so isLoader narrows valid entries to ProtectLoader[] before rollout processing.packages/clerk-js/src/core/__tests__/protectSession.test.ts (3)
370-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for a stored token that exceeds
MAX_TOKEN_LENGTH.
validateTokenrejects a token longer than 4096 characters. The suite covers the expiry bound and the lifetime bound, but not the length bound. Add a planted entry with an over-length token and assert thathasFreshToken()returnsfalse.As per coding guidelines: "Verify proper error handling and edge cases".
🤖 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/clerk-js/src/core/__tests__/protectSession.test.ts` around lines 370 - 384, Add a test case alongside the existing planted-token coverage that stores a token longer than MAX_TOKEN_LENGTH in localStorage, then initializes a session and asserts created?.hasFreshToken() is false. Reuse the existing session setup and token-storage key used in protectSession.test.ts, focusing only on the over-length validation path.Source: Coding guidelines
245-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the foreign correlation id.
'z'.repeat(26).replace(/z/g, 'a')produces'a'.repeat(26). Use the direct form so the intent stays readable.♻️ Proposed change
- serveInline(await injected(), { cid: buildCid('z'.repeat(26).replace(/z/g, 'a'), 'b'.repeat(26)) }); + serveInline(await injected(), { cid: buildCid('a'.repeat(26), 'b'.repeat(26)) });🤖 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/clerk-js/src/core/__tests__/protectSession.test.ts` around lines 245 - 255, In the test case “ignores a token minted for someone else’s run,” simplify the foreign correlation ID passed to buildCid by replacing the redundant 'z'.repeat(26).replace(/z/g, 'a') expression with the direct equivalent 'a'.repeat(26); leave the test behavior unchanged.
298-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese deadline tests depend on real wall-clock time.
tokenTimeoutMsvalues of 40, 50 and 60 ms are shorter than a slow CI tick. A stalled event loop can let the loader path settle after the deadline, or let an unrelated status win. The assertion at line 537 also measures elapsed real time.Raise the deadlines to a value that tolerates scheduler jitter, or drive them with
vi.useFakeTimers(). The test at lines 540-556 already mocksDate.now, so fake timers fit the existing style.Also applies to: 328-334, 531-538
🤖 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/clerk-js/src/core/__tests__/protectSession.test.ts` around lines 298 - 305, Stabilize the deadline tests in the protectSession suite by replacing the 40, 50, and 60 ms real-time tokenTimeoutMs values and elapsed-time assertion with scheduler-independent timing. Prefer vi.useFakeTimers() while preserving the existing mocked-Date.now style, and update the tests around the timeout cases and the assertion near the elapsed-time check so deadlines are advanced explicitly and timeout remains the winning status.packages/clerk-js/src/core/__tests__/protect.test.ts (1)
172-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for a token loader whose target does not resolve.
applyLoadernow returnsundefinedwhen the#idtarget element is missing, andProtectSession.#runTokenLoadermaps that toscript_error. No test drives that branch. Add a case withtarget: '#missing'on the token loader and assert__clerk_protect_status: 'script_error'.As per coding guidelines: "Verify proper error handling and edge cases".
🤖 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/clerk-js/src/core/__tests__/protect.test.ts` around lines 172 - 179, Add a test alongside the existing loader error case that configures the token loader with target: '`#missing`', invokes the Protect request flow, and asserts it resolves with __clerk_protect_status set to 'script_error'. Exercise the missing-target path in ProtectSession.#runTokenLoader rather than dispatching a script element error.Source: Coding guidelines
🤖 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 @.changeset/lucky-pandas-observe.md:
- Line 12: Update the release note describing ProtectLoader’s tokenUrl field so
it states that the loader token remains inline and tokenUrl is used only for
optional upgrade minting; retain tokenTimeoutMs and the existing optional-field
context.
---
Nitpick comments:
In `@packages/clerk-js/bundlewatch.config.json`:
- Around line 3-7: Update the Bundlewatch maxSize values for the five bundle
entries in order: clerk.js to 550KB, clerk.browser.js to 77KB,
clerk.legacy.browser.js to 119KB, clerk.no-rhc.js to 317KB, and clerk.native.js
to 77KB.
In `@packages/clerk-js/src/core/__tests__/protect.test.ts`:
- Around line 172-179: Add a test alongside the existing loader error case that
configures the token loader with target: '`#missing`', invokes the Protect request
flow, and asserts it resolves with __clerk_protect_status set to 'script_error'.
Exercise the missing-target path in ProtectSession.#runTokenLoader rather than
dispatching a script element error.
In `@packages/clerk-js/src/core/__tests__/protectSession.test.ts`:
- Around line 370-384: Add a test case alongside the existing planted-token
coverage that stores a token longer than MAX_TOKEN_LENGTH in localStorage, then
initializes a session and asserts created?.hasFreshToken() is false. Reuse the
existing session setup and token-storage key used in protectSession.test.ts,
focusing only on the over-length validation path.
- Around line 245-255: In the test case “ignores a token minted for someone
else’s run,” simplify the foreign correlation ID passed to buildCid by replacing
the redundant 'z'.repeat(26).replace(/z/g, 'a') expression with the direct
equivalent 'a'.repeat(26); leave the test behavior unchanged.
- Around line 298-305: Stabilize the deadline tests in the protectSession suite
by replacing the 40, 50, and 60 ms real-time tokenTimeoutMs values and
elapsed-time assertion with scheduler-independent timing. Prefer
vi.useFakeTimers() while preserving the existing mocked-Date.now style, and
update the tests around the timeout cases and the assertion near the
elapsed-time check so deadlines are advanced explicitly and timeout remains the
winning status.
In `@packages/clerk-js/src/core/fapiClient.ts`:
- Around line 236-244: Bound the getProtectParams() await in the fapiClient
request flow with a local timeout so a hanging public option cannot block
sign-in or sign-up indefinitely. Preserve the existing fallback behavior by
treating timeout or supplier rejection as undefined and continuing without
protect parameters.
- Around line 240-243: In the protectParams handling near isMergeableBody,
remove the redundant Record<string, unknown> cast and the immediate
requestInit.body assignment; retain only the merged body assignment, since the
later plain-object body flow already updates requestInit.body.
In `@packages/clerk-js/src/core/protect.ts`:
- Around line 38-41: Update the `#apply` method parameter from ProtectLoader[] to
unknown[] to reflect that server-provided loader values are unvalidated.
Preserve the existing configured.filter call so isLoader narrows valid entries
to ProtectLoader[] before rollout processing.
In `@packages/clerk-js/src/core/protectSession.ts`:
- Around line 419-425: Update the public static factory method
ProtectSession.create with an explicit return type annotation of ProtectSession
| undefined, preserving its existing undefined result for empty templated
loaders and ProtectSession result otherwise.
- Around line 5-20: Reduce comments throughout protectSession.ts, especially the
module-level block and the listed ranges, to terse single-line comments or
remove them when they merely restate the surrounding code. Preserve only
non-obvious design rationale, including why no_token differs from timeout, and
retain concise comments only where they explain that rationale or another
critical implementation decision.
In `@packages/shared/src/types/protectConfig.ts`:
- Around line 23-27: Update the documentation for tokenTimeoutMs in the protect
configuration type to state that values are capped at 10000 ms, while retaining
the existing 5000 ms default description.
🪄 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 Plus
Run ID: 5137fbb9-ce5a-4323-ab93-b45f8ad4be06
📒 Files selected for processing (10)
.changeset/lucky-pandas-observe.mdpackages/clerk-js/bundlewatch.config.jsonpackages/clerk-js/src/core/__tests__/fapiClient.test.tspackages/clerk-js/src/core/__tests__/protect.test.tspackages/clerk-js/src/core/__tests__/protectSession.test.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/fapiClient.tspackages/clerk-js/src/core/protect.tspackages/clerk-js/src/core/protectSession.tspackages/shared/src/types/protectConfig.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/__tests__/protectSession.test.ts (1)
336-384: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression coverage for malformed stored token entries. Test invalid JSON, missing or malformed
rid, and non-stringtoken;readStoredTokenmust reject each and start a fresh acquisition without constructing a malformed__clerk_protect_cid.🤖 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/clerk-js/src/core/__tests__/protectSession.test.ts` around lines 336 - 384, Extend the protectSession tests around stored-token reuse to cover invalid JSON, missing or malformed rid, and non-string token entries. Verify readStoredToken rejects each case, hasFreshToken() is false, and the session starts fresh acquisition through the loader without producing a malformed __clerk_protect_cid; reuse the existing session, loader, and request-parameter setup.Source: Linters/SAST tools
🤖 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/clerk-js/src/core/__tests__/protectSession.test.ts`:
- Around line 336-384: Extend the protectSession tests around stored-token reuse
to cover invalid JSON, missing or malformed rid, and non-string token entries.
Verify readStoredToken rejects each case, hasFreshToken() is false, and the
session starts fresh acquisition through the loader without producing a
malformed __clerk_protect_cid; reuse the existing session, loader, and
request-parameter setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 681154a7-1772-4ec0-9732-fd243673889d
📒 Files selected for processing (10)
.changeset/lucky-pandas-observe.mdpackages/clerk-js/bundlewatch.config.jsonpackages/clerk-js/src/core/__tests__/fapiClient.test.tspackages/clerk-js/src/core/__tests__/protect.test.tspackages/clerk-js/src/core/__tests__/protectSession.test.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/fapiClient.tspackages/clerk-js/src/core/protect.tspackages/clerk-js/src/core/protectSession.tspackages/shared/src/types/protectConfig.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/clerk-js/src/core/clerk.ts
- packages/clerk-js/src/core/fapiClient.ts
- packages/clerk-js/bundlewatch.config.json
- .changeset/lucky-pandas-observe.md
- packages/shared/src/types/protectConfig.ts
- packages/clerk-js/src/core/tests/fapiClient.test.ts
- packages/clerk-js/src/core/protect.ts
- packages/clerk-js/src/core/protectSession.ts
| if (this.#freshStoredToken()) { | ||
| // Step 1: valid and not near expiry. No lock, no loader, no network. | ||
| return 'ok'; | ||
| } |
There was a problem hiding this comment.
[MEDIUM] A page-writable localStorage entry suppresses the Protect loader entirely and makes the client report status: ok
Pre-PR every loader was injected on every page load. Now the {cid} loader is injected only when __clerk_protect_st lacks a fresh token, and #freshStoredToken() accepts an entry that merely satisfies the rid regex, typeof token === 'string' && length <= 4096 (no format check), and an exp within 24h. One localStorage.setItem therefore keeps the detection script out of the DOM — the new test asserts elements.length === 0 on this path — and makes getRequestParams() emit __clerk_protect_status: 'ok'.
Network-blocking the loader yields the distinguishable script_error; this path does not, so it is a strictly quieter suppression. The actionable half is server-side: __clerk_protect_status is client-asserted and unauthenticated, so ok accompanied by a token the fraud service cannot verify should read as adversarial rather than benign.
Suggest binding freshness to a token the client cannot mint (e.g. verify the v1.payload.mac shape before trusting the entry) and treating unverifiable ok as absent server-side.
— Comment generated with Claude with @dominic-clerk's supervision
There was a problem hiding this comment.
__clerk_protect_status is telemetry — it exists so we can diagnose why acquisition didn't produce a token (blocked, timed out, unsupported browser), and it's a debugging aid, not a security control. It's client-asserted and treated that way: nothing is granted on the strength of it.
The stored entry likewise gates a cache, not trust. The token is verified server-side, so an entry that doesn't verify reaches the same place as no entry at all — and writing one isn't cheaper than simply sending the same values directly, which was always possible. That's why binding freshness to an unmintable token can't be the mitigation: the client can't do the verification, and the route it would close isn't the cheap one.
Taking the shape check regardless, for a different reason: validateToken documents that intent and doesn't implement it, and a corrupt or truncated entry suppressing acquisition for a real user is worth closing on its own. It'll be version-agnostic so an older SDK doesn't reject a future mint.
Server-side handling of unverifiable tokens is already covered in the paired internal PR.
`validateToken` documented that a value which could not have come from a mint of ours is discarded, but only checked type, length and expiry — so a corrupt or truncated store entry counted as fresh and suppressed acquisition until it expired, up to the lifetime ceiling. This is hygiene, not a security boundary, and is deliberately not framed as one: only the backend can tell a real token from a well-formed forgery, and anything that can write the store can send the same values to the API directly. What it buys is that a broken entry starts a fresh run immediately. The shape is matched version-agnostically. Pinning it to the current version would mean an SDK rejecting a token the backend had minted ahead of it, and re-running the loader on every page load until the SDK caught up — a test guards against that tightening.
Conflict was bundlewatch.config.json only. main and this branch agreed on four of the five budgets; they diverged on clerk.native.js, which main raised 74->76 and this branch raised 74->77. Both increases are real and independent, so the resolution is additive: 79KB.
main's #9313 added the application-supplied Protect assertion through the same `getProtectParams` seam this branch added for the server-configured session token, so every conflict was on shared ground. * clerk.ts — both sides defined `getProtectParams`, each returning only its own feature's params. Taking either side compiles, type-checks and silently stops sending the other's. The two write disjoint params, so the hook now unions them via `#protectParams`: resolved concurrently, a failure on one side costs only that side's params, and `undefined` when neither contributes so a request from an instance using no Protect feature is unchanged. * fapiClient.ts — the merge plumbing had already landed identically on both sides; only the comments differed. Kept the wording that is feature-neutral, since the seam now carries two features rather than the session token alone. * fapiClient.test.ts — both sides wrote a describe block for the same plumbing from their own feature's angle. Merged into one, keeping every distinct assertion from each: the extra sign-up attempt path and the string-body and param-name-mangling cases from main, and the no-extra-headers, not-in-URL, hook-not-called and non-plain-body cases from here. The fixture now carries both features' params. * bundlewatch.config.json — neither side's budget is right after the merge: the two branches' size increases are additive, not alternative, so the per-file max still failed three bundles. Regenerated from a real build via `pnpm bundlewatch:fix`; bundlewatch passes. Verified: clerk-js 1036 tests pass, shared 1207 pass, eslint clean on the resolved files (6 pre-existing warnings elsewhere), prettier clean, bundlewatch PASS.
…tures
The assertion and the session token feed one hook from one expression, and
nothing covered that wiring — protectAssertion.test.ts and protect{,Session}
.test.ts each cover their own side, fapiClient.test.ts covers merging whatever
the hook returns into the body, but the gatherer between them was untested.
Dropping either source from it compiles, type-checks and passes every existing
test, while that feature's params silently stop being sent.
Checked by mutation: removing the session source fails 3 of these tests,
removing the assertion source fails 3, and dropping the `undefined` guard fails
1. Needs no environment harness — `#protect` is built in the constructor, so
mocking ../protect and capturing the options handed to createFapiClient is
enough.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/core/clerk.ts (1)
485-488: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument
setProtectAssertionas a public API.Line 485 adds a public Clerk method without JSDoc. Document the accepted assertion forms, the
undefinedclearing behavior, and its effect on later sign-in and sign-up requests.As per coding guidelines, “All public APIs must be documented with JSDoc.”
🤖 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/clerk-js/src/core/clerk.ts` around lines 485 - 488, Add JSDoc to the public setProtectAssertion method describing accepted ProtectAssertion forms, that passing undefined clears the assertion, and that the configured value affects subsequent sign-in and sign-up requests.Source: Coding guidelines
🤖 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/clerk-js/src/core/__tests__/clerk.protect-params.test.ts`:
- Around line 50-101: Add a test covering explicit assertion clearing:
initialize Clerk with protectAssertion, call setProtectAssertion(undefined),
then invoke the getProtectParams hook and assert the cleared assertion is
absent. Ensure the test distinguishes the cleared runtime value from any
fallback to this.#options.protectAssertion.
- Around line 23-31: Replace the `any` annotations in the hoisted
`capturedOptions` state and the mocked `createFapiClient` parameter with
`Parameters<typeof createFapiClient>[0]`, importing or referencing
`createFapiClient` as needed. Preserve the existing capture-and-delegate
behavior while ensuring the test tracks contract changes.
---
Outside diff comments:
In `@packages/clerk-js/src/core/clerk.ts`:
- Around line 485-488: Add JSDoc to the public setProtectAssertion method
describing accepted ProtectAssertion forms, that passing undefined clears the
assertion, and that the configured value affects subsequent sign-in and sign-up
requests.
🪄 Autofix
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 Plus
Run ID: b26c4276-4a48-43a2-af4a-dbededd678ea
📒 Files selected for processing (6)
packages/clerk-js/bundlewatch.config.jsonpackages/clerk-js/src/core/__tests__/clerk.protect-params.test.tspackages/clerk-js/src/core/__tests__/fapiClient.test.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/fapiClient.tspackages/shared/src/types/protectConfig.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/types/protectConfig.ts
- packages/clerk-js/src/core/tests/fapiClient.test.ts
- packages/clerk-js/bundlewatch.config.json
| const { capturedOptions } = vi.hoisted(() => ({ capturedOptions: { current: undefined as any } })); | ||
|
|
||
| vi.mock('../fapiClient', async importOriginal => { | ||
| const actual = await importOriginal<typeof import('../fapiClient')>(); | ||
| return { | ||
| ...actual, | ||
| createFapiClient: (options: any) => { | ||
| capturedOptions.current = options; | ||
| return actual.createFapiClient(options); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Type the FAPI mock options.
Lines 23 and 29 use any, so a createFapiClient contract change can bypass this integration test. Derive the captured value and mock parameter from Parameters<typeof createFapiClient>[0].
As per coding guidelines, “Avoid any type” and “No any types without justification in code review.”
🤖 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/clerk-js/src/core/__tests__/clerk.protect-params.test.ts` around
lines 23 - 31, Replace the `any` annotations in the hoisted `capturedOptions`
state and the mocked `createFapiClient` parameter with `Parameters<typeof
createFapiClient>[0]`, importing or referencing `createFapiClient` as needed.
Preserve the existing capture-and-delegate behavior while ensuring the test
tracks contract changes.
Source: Coding guidelines
| describe('Clerk getProtectParams', () => { | ||
| beforeEach(() => { | ||
| getRequestParams.mockReset(); | ||
| capturedOptions.current = undefined; | ||
| }); | ||
|
|
||
| it('is wired into the FAPI client', () => { | ||
| expect(hookFor()).toBeTypeOf('function'); | ||
| }); | ||
|
|
||
| it('unions the assertion and the session token', async () => { | ||
| getRequestParams.mockResolvedValue(sessionParams); | ||
|
|
||
| await expect(hookFor('token-abc')()).resolves.toEqual({ ...assertionParams, ...sessionParams }); | ||
| }); | ||
|
|
||
| it('sends the session token when no assertion is configured', async () => { | ||
| getRequestParams.mockResolvedValue(sessionParams); | ||
|
|
||
| await expect(hookFor()()).resolves.toEqual(sessionParams); | ||
| }); | ||
|
|
||
| it('sends the assertion when the session contributes nothing', async () => { | ||
| getRequestParams.mockResolvedValue(undefined); | ||
|
|
||
| await expect(hookFor('token-abc')()).resolves.toEqual(assertionParams); | ||
| }); | ||
|
|
||
| // Returning `{}` would make every sign-in body differ from what it was before the feature existed. | ||
| it('resolves to undefined when neither contributes anything', async () => { | ||
| getRequestParams.mockResolvedValue(undefined); | ||
|
|
||
| await expect(hookFor()()).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| // Neither feature may take the other down with it. | ||
| it('keeps the session token when the assertion resolver throws', async () => { | ||
| getRequestParams.mockResolvedValue(sessionParams); | ||
|
|
||
| await expect( | ||
| hookFor(() => { | ||
| throw new Error('boom'); | ||
| })(), | ||
| ).resolves.toEqual(sessionParams); | ||
| }); | ||
|
|
||
| it('keeps the assertion when acquiring the session token rejects', async () => { | ||
| getRequestParams.mockRejectedValue(new DOMException('storage is blocked', 'SecurityError')); | ||
|
|
||
| await expect(hookFor('token-abc')()).resolves.toEqual(assertionParams); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test explicit assertion clearing.
Add a test that loads Clerk with protectAssertion, then calls setProtectAssertion(undefined). Assert that getProtectParams no longer returns the option assertion. The current tests cannot detect a regression where clearing falls back to this.#options.protectAssertion.
As per coding guidelines, “Unit tests are required for all new functionality” and tests must verify edge cases.
🤖 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/clerk-js/src/core/__tests__/clerk.protect-params.test.ts` around
lines 50 - 101, Add a test covering explicit assertion clearing: initialize
Clerk with protectAssertion, call setProtectAssertion(undefined), then invoke
the getProtectParams hook and assert the cleared assertion is absent. Ensure the
test distinguishes the cleared runtime value from any fallback to
this.#options.protectAssertion.
Source: Coding guidelines
Ephem
left a comment
Ephem
left a comment
There was a problem hiding this comment.
I don't have a ton of context on the protect stuff and this is a lot to wrap ones head around, but I think I've managed to form a decent mental model.
I wont pretend I understand all the details here, but I've focused especially on understanding and verifying the mechanisms that makes sure existing clients aren't affected, and that seems 👌. Also tried it in our dashboard, no issues.
The approach seems good in general too, I see no obvious blockers. There's a bit of overlap with how we refresh session tokens which has been a thorny area historically, but this thankfully has a bit less complexity.
| Acquire an optional Protect session token and attach it to sign-in and sign-up requests. | ||
|
|
||
| On instances whose loader config references the new `{cid}` / `{pid}` / `{rid}` / `{instance_id}` / `{sdkver}` placeholders, Clerk mints an opaque, random correlation id and substitutes it into the loader's attributes and `textContent`. The loader is served with a signed session token, which is taken up once per browser session — shared across tabs under a lock rather than acquired once per tab — and travels alongside the correlation id and an acquisition status in the form-encoded body of sign-in and sign-up requests. | ||
|
|
||
| Acquisition can never block or fail a sign-in: it is bounded by a deadline, and when no token can be obtained a status (`no_token`, `timeout`, `script_error`, `fetch_error`, `unsupported`, `http_<n>`) travels in its place and the request proceeds unchanged. Only the loader carrying the correlation id is governed by the shared token; any other configured loader is still applied on every page load. Nothing is stored in the browser unless a loader references `{cid}`, `{pid}` or `{rid}`. | ||
|
|
||
| `ProtectLoader` gains two optional fields: `tokenTimeoutMs`, and `tokenUrl` for instances that opt into fetching the token from a dedicated endpoint instead of taking the one served with the loader. |
There was a problem hiding this comment.
This describes the technical implementation, but it doesn't really explain to an end user reading the release changelog what it does.
I'd favor a two sentence summary of the capabilities this gives instead, no need for this level of technical detail here.
There was a problem hiding this comment.
Agreed — rewritten as what the release does rather than how. Two sentences: what it improves, and that it's inert unless the instance opts in and can never block or delay a sign-in. The placeholder names and the status list are gone; they're implementation detail and this repo is public.
|
|
||
| /** | ||
| * Returns a session only when at least one loader references a placeholder — an instance not | ||
| * using any of them keeps today's behaviour exactly. |
There was a problem hiding this comment.
NIT: "Today's behavior" wont age well. 😄
There was a problem hiding this comment.
Ha, fair. Reworded to "an instance using none of them is unaffected", and did the same at the one other site that had it.
| clearTimeout(timer); | ||
| resolve(outcome); | ||
| }; | ||
| element.addEventListener('load', settle('loaded'), { once: true }); |
There was a problem hiding this comment.
Not sure it's something that can happen in practice, but wanted to point out that since load only fires if src is set, this wont ever trigger if it's a pure inline script via textContent, which would put things in a bad state.
There was a problem hiding this comment.
Good catch, and it's real. A classic inline script executes during appendChild and fires neither load nor error, so this would have waited out the whole deadline, reported timeout, and discarded a token the script body had already assigned.
Worth flagging one wrinkle, because the obvious fix is wrong: it isn't "no src ⇒ no event". An inline module does fire load — it evaluates off a module graph the browser still has to fetch — so short-circuiting on src alone would settle before the module body ran and report no token instead. The guard now resolves early only for an element that fetches nothing and isn't an inline module. Both paths have a test, and the fixtures moved to type=module to match what's actually served.
| /** Acquisition deadline when the instance does not configure one. */ | ||
| const DEFAULT_TOKEN_TIMEOUT_MS = 5 * 1_000; | ||
| /** Ceiling on the instance-configured deadline, so no server value can stall a sign-in. */ | ||
| const MAX_TOKEN_TIMEOUT_MS = 10 * 1_000; |
There was a problem hiding this comment.
These are per API request, not per sign-in/up flow right? So a combined sign in/up that transitions from sign-in to sign-up and fires multiple API requests might stall for twice the time in niche scenarios?
Feels like an edge case so not necessarily something we need to tackle, just wanted to highlight it.
There was a problem hiding this comment.
Correct, and worth having written down. Two things keep it off the common path: acquisition is prefetched at Clerk.load() rather than at sign-in, so a second request in the same flow reads the already-shared store rather than starting a run; and once an attempt settles without producing a token there's a cooldown before a fresh one is allowed, so back-to-back requests can't each pay a full deadline.
The worst case is still reachable — first load, acquisition failing, two requests far enough apart to clear the cooldown. Accepting it: the deadline is a ceiling on a request that would proceed anyway, and it never fails the sign-in.
| // The config is server-controlled and cached, so nothing it can contain may take `Clerk.load()` | ||
| // down with it. | ||
| try { | ||
| this.#apply(config.loaders, config.id || undefined); |
There was a problem hiding this comment.
id is typed as required, but treated as optional, all the way through to:
const suffix = instanceId ? `.${instanceId}` : '';
this.#tokenStorageKey = `${TOKEN_STORAGE_KEY}${suffix}`;
which made me start thinking about conflicting tokens stored under the same key etc before I realized it's probably expected to always be available?
If that's the case but we still want to be defensive at runtime (which I like), maybe we should validate its presence here and fail early instead (logging + early exit) to make the error predictable?
There was a problem hiding this comment.
You were right that the type and the code disagreed, and chasing it turned up more than expected: protect_config.id isn't sent at all. So instanceId was undefined on every load — the storage/lock key suffix never applied, and {instance_id} was never substituted, just left verbatim in the URL.
Resolved by removing it from the client rather than validating it: the instance id is the server's to place into the config it serves, not something the SDK interpolates. ProtectSession no longer takes it, {instance_id} is out of the placeholder set, and the store is explicitly one per origin. ProtectConfigResource also stops narrowing id to required — that narrowing was the mismatch you spotted. The JSON type keeps it, since ClerkResourceJSON requires an id of every resource (there's a TODO there asking the same question you did).
One consequence worth stating plainly: per-instance store namespacing is gone. It never actually applied, so nothing changes in practice, and the token names the instance that minted it and is verified server-side — two instances on one origin costs a rejected token, never one honoured for the wrong instance.
| /** | ||
| * The store is writable by anything running on the origin, so a value that could not have come | ||
| * from a mint of ours is discarded rather than trusted to suppress the loaders. | ||
| * | ||
| * The shape check is hygiene, not a security boundary: only the server can tell a real token from a | ||
| * well-formed forgery, and anything that can write the store can send the same values to the API | ||
| * directly. What it buys is that a corrupt or truncated entry starts a fresh run immediately | ||
| * instead of suppressing acquisition until it expires. | ||
| */ |
Review feedback from @Ephem on #9299. A `<script>` with no `src` executes during `appendChild` and fires neither `load` nor `error`, so `#runTokenLoader` waited on an event that could never arrive: it spent the whole deadline, reported `timeout`, and discarded a token the script body had already assigned. An inline module is the exception — it evaluates off a module graph the browser still fetches, so its `load` does arrive and settling early would read the global before it is written. Also from that review: - Drop `{instance_id}`. The instance id is the server's to place into the config it serves, not something the client interpolates, and the placeholder could never resolve anyway: `protect_config.id` is not sent, so it was left verbatim on every load. Removing it also removes the storage/lock key suffix it fed, which was likewise never applied. `ProtectConfigResource` stops narrowing `id` to required, which is what made the type disagree with the code. The wire type keeps it, since `ClerkResourceJSON` requires an id of every resource. - The store comment claimed a per-instance namespace that never held. It now states what is true: one store per origin, with the token scoped to its minting instance and verified server-side. - Rewrite the changeset as what the release does rather than how, and drop wording that dates itself. Test fixtures move to `type=module`, matching the served loader, so they stay on the event-driven path; the classic-inline and inline-module cases each get their own test.
`clerk/javascript` is public, so a backend service name in a comment or a test string is disclosure with no upside. The `__clerk_specter` global stays: it is the wire contract with the loader script the server serves, not prose.
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 @.changeset/lucky-pandas-observe.md:
- Around line 6-8: Update the changeset description to clarify that the
session-token behavior requires both Protect and the relevant loader
configuration, not Protect alone. Replace the broad claim that an instance
stores nothing in the browser with the narrower statement that existing
instances store no additional browser data, while preserving the claims about no
blocking, delaying, or extra network data.
🪄 Autofix
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 Plus
Run ID: b9ecb10e-18f3-4a0e-9c83-a8e71e81cd71
📒 Files selected for processing (6)
.changeset/lucky-pandas-observe.mdpackages/clerk-js/src/core/__tests__/protect.test.tspackages/clerk-js/src/core/__tests__/protectSession.test.tspackages/clerk-js/src/core/protect.tspackages/clerk-js/src/core/protectSession.tspackages/shared/src/types/protectConfig.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/clerk-js/src/core/tests/protect.test.ts
- packages/clerk-js/src/core/protectSession.ts
- packages/clerk-js/src/core/protect.ts
| Improve Clerk Protect's ability to distinguish real users from automated sign-in and sign-up attempts, on instances that have Protect enabled. | ||
|
|
||
| This is inert unless your instance opts in, it can never block or delay a sign-in or sign-up, and an instance that does not use it stores nothing in the browser and sends nothing extra. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the opt-in and storage claims.
The session-token behavior requires the loader configuration. Enabling Protect alone does not enable this feature. State that this feature stores no additional browser data instead of saying that the instance stores nothing in the browser.
The PR objective states that the feature is enabled through loader configuration and that existing instances store no additional browser data.
Proposed wording
-Improve Clerk Protect's ability to distinguish real users from automated sign-in and sign-up attempts, on instances that have Protect enabled.
+Add optional server-configured Protect session tokens to sign-in and sign-up requests for instances that opt in through loader configuration.
-This is inert unless your instance opts in, it can never block or delay a sign-in or sign-up, and an instance that does not use it stores nothing in the browser and sends nothing extra.
+This is inert unless your instance opts in through loader configuration. It does not block or delay authentication requests. Instances without this configuration retain current behavior, store no additional browser data, and send no additional parameters.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Improve Clerk Protect's ability to distinguish real users from automated sign-in and sign-up attempts, on instances that have Protect enabled. | |
| This is inert unless your instance opts in, it can never block or delay a sign-in or sign-up, and an instance that does not use it stores nothing in the browser and sends nothing extra. | |
| Add optional server-configured Protect session tokens to sign-in and sign-up requests for instances that opt in through loader configuration. | |
| This is inert unless your instance opts in through loader configuration. It does not block or delay authentication requests. Instances without this configuration retain current behavior, store no additional browser data, and send no additional parameters. |
🤖 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 @.changeset/lucky-pandas-observe.md around lines 6 - 8, Update the changeset
description to clarify that the session-token behavior requires both Protect and
the relevant loader configuration, not Protect alone. Replace the broad claim
that an instance stores nothing in the browser with the narrower statement that
existing instances store no additional browser data, while preserving the claims
about no blocking, delaying, or extra network data.
This repo is public, so the changelog cannot conceal the change — but it is what people skim, and a note naming what the feature detects works as an index entry pointing an adversary at the diff worth reading. The privacy-relevant detail belongs in the customer docs, which are tracked separately, not here.
…ed clock Both are outage behaviour rather than happy-path behaviour, so neither shows up in normal testing. The re-acquire cooldown was a flat 30s, so a Specter outage added the full acquisition deadline to a sign-in every 30 seconds for as long as it lasted -- the auth-critical-path cost landing exactly when things are already worst. It now doubles per consecutive tokenless attempt up to a 15m ceiling, low enough that a long-lived tab still notices a recovery. Success is judged on whether a token landed rather than on the status, since another tab may have won the lock and written one while this attempt reported `timeout`. A stored entry now carries `at`, the local time it was written. `exp` compares server truth against the browser's clock, so a clock running slow keeps sending lapsed tokens and one running fast discards good ones and re-runs the loader on every page load; elapsed local time is measured on a single clock, so a constant offset cancels. This bounds the damage rather than removing it -- a fully skew-proof check needs the server to send a relative lifetime instead of an absolute `exp`, which is a wire change. Also stops sending a token inside a 5s margin of expiry: it could only fail verification by the time it arrived, and reporting the acquisition status is the honest answer. `StoredToken` and `MintedToken` are now separate types so `at` is stamped by the write and a producer cannot forget it. Test fixtures that hand-write the store go through one helper, so each case overrides only the field under test -- several were about to start passing for the wrong reason.
…very floor `token_timeout_ms` has never been read. `ProtectConfig.fromJSON` assigns `data.loaders` verbatim -- there is no case conversion anywhere in clerk-js -- so a loader keeps the shape the server sent, while the SDK looked for `tokenTimeoutMs`. The wire name appears nowhere in clerk-js or shared, so `clampTimeout` always saw `undefined` and every instance silently ran the 5s default. The per-instance deadline that `JSLoaderConfig` documents did nothing. Every test passed throughout, because the fixtures build `ProtectLoader` as a TypeScript literal. A hand-built object typed by the same interface that describes the wire agrees with itself by construction and can never catch a name the server does not send. The new deadline test parses the wire shape instead, which is the seam the defect lives on. `ProtectLoader` is now typed in wire names throughout -- `text_content`, `token_url`, `token_timeout_ms` -- because that is what the object is. Both renamed fields are new in this PR and unreleased. Adds `tokens_invalid_before` (unix seconds) to `protect_config`. A stored token records the floor in force when it was acquired, and an entry below the configured floor is discarded and re-acquired. This is recovery, not revocation: an outstanding token stays cryptographically valid until it expires or its signing key is dropped, and dropping a key from the verifier's list is still what revokes. What this buys is that the fleet stops sending something the server will not accept within a page load, rather than for the token's full 12h lifetime. Absent on both sides means no floor, so introducing it does not make every browser re-acquire at once. The matching server field is a clerk_go change; until it lands this is inert, which is the safe direction.
3 participants
Description
Adds an optional, server-configured session token to
@clerk/clerk-js, attached to sign-in and sign-up requests.It is inert unless an instance's loader configuration opts into it. An instance not using it keeps today's behaviour exactly, stores nothing in the browser, and sends no additional parameters.
{cid},{pid},{rid},{instance_id},{sdkver}— substituted into a loader's attribute values andtextContentbefore the element is appended. An unrecognised{…}is left verbatim, so an older SDK loading the same configuration stays compatible.crypto.getRandomValuesas lowercase unpadded RFC 4648 base32. Nothing is derived from the device — no fingerprinting input, no clock, no user data.SafeLockprimitive (core/auth/safeLock.ts) thatSessionCookiePolleralready uses, with a re-check inside the lock so concurrent tabs do not each acquire in turn. A wedged leader delays nobody, and a leader tab closing mid-run releases its lock automatically.Clerk.load()rather than at sign-in, so acquisition stays off the latency-critical path. It is bounded by a deadline and can never block or fail a sign-in: when no token is obtained, a status travels in its place and the request proceeds unchanged._methodinto the query. No request header is added anywhere. The merge is an explicit step infapiClient.requeston a path allowlist, because theonBeforeRequestcallbacks fire after the body is stringified.ProtectLoadergains two optional fields,tokenUrlandtokenTimeoutMs, both inert unless set.rolloutmoved out ofapplyLoaderinto a module-level helper soload()can decide participation before it starts applying loaders. Same semantics, evaluated once.Bundle thresholds were bumped on the affected bundles to cover the addition.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change