fix(shared,clerk-js,nextjs): authorization bypass in combined-condition has() checks - #8372
Merged
Conversation
…n has() checks
`has()`, `auth.protect()`, and related predicates could return true for certain
calls that combined conditions from more than one dimension (role, permission,
feature, plan, reverification) when the result should have been false. The
combining logic in `createCheckAuthorization` treated any helper returning null
as "skip this dimension", so a hard-negative in one dimension could collapse
into a hard-positive whenever another dimension returned true.
The helpers now return an internal `pass | fail | skip` verdict and the combiner
enforces that every requested dimension must individually pass:
- Dimensions the caller did not ask about do not contribute to the result.
- Asked dimensions with missing, malformed, or invalid session data fail closed
(covers missing org claims, empty billing claims, `factorVerificationAge` being
null or malformed, invalid reverification configs, and `factor1Age === -1`).
- `has({})` continues to return false.
- Multi-key calls (e.g. `{ permission, feature }`) now require every requested
sub-check to pass (AND). The previous `billingAuthorization || orgAuthorization`
OR-coercion is removed.
- Non-string `role` / `permission` / `feature` / `plan` values fail closed
instead of throwing.
`session.checkAuthorization()` now uses the active organization's id rather than
the membership row id when constructing the authorization options. The
corresponding test fixture now gives memberships and organizations distinct
ids so regressions around this are caught. The global mock of
`createCheckAuthorization` in the React `useAuth` test suite has been removed so
the predicate is exercised against real session state.
Previously, `has({ reverification: 'strict_mfa' })` (and any `multi_factor`
level reverification) silently downgraded to a first-factor check when the
user had not enrolled a second factor (`factor2Age === -1`). This answered a
different question than the caller asked: strict_mfa means "prove a fresh
second factor", and if the user has no second factor the requirement cannot
be satisfied.
Fail closed in that case and direct apps to enroll a second factor before
gating behind strict_mfa. `second_factor` (strict/moderate/lax) is unchanged
and still falls back to a fresh first factor when no second factor is
enrolled - that is a separate product decision.
The existing Session.test assertions that pinned the downgrade behavior are
flipped to assert the fail-closed behavior and a corresponding regression
test is added to authorization.spec.ts.
…tect()
Previously `auth.protect()` silently skipped the authorization check whenever
the argument object contained any of `unauthenticatedUrl`, `unauthorizedUrl`,
or `token` alongside `role` / `permission` / `feature` / `plan` /
`reverification`. TypeScript's excess-property check caught this only when
the argument was an inline object literal; variable-assigned or spread
arguments (and JS callers) bypassed the check entirely, letting every
authenticated user through. Example:
const opts = { role: 'org:admin', unauthorizedUrl: '/denied' };
await auth.protect(opts); // used to authorize everyone
`getAuthorizationParams` now picks the known authorization keys from the
argument with an explicit allowlist (`role`, `permission`, `feature`,
`plan`, `reverification`). Mixed-shape calls enforce the authorization
check against the extracted params; options-only calls take the existing
fast path. Unknown extra keys are ignored rather than forwarded to `has()`,
so objects with stray keys from spread or variable construction do not
accidentally deny legitimate requests.
Regression tests in `clerkMiddleware.test.ts` cover:
- `{ role, unauthorizedUrl }` unauthorized user redirects to unauthorizedUrl
- `{ permission, token }` unauthorized user hits the rewrite path
- `{ role, unauthorizedUrl }` authorized user passes through
- options-only object with an extra unknown key still takes the fast path
…dback - Collapse combine() to a readable some/every expression; drop the local boolean flag. - Rename permAsked -> permissionAsked in checkOrgAuthorization for consistency with the other dimensions. - Trim redundant @returns JSDoc blocks (pass/fail/skip semantics are already documented on the CheckResult type). - Drop the 'separate product decision' hedge on the second_factor branch; the comment belonged in a tracker, not in code. - Shorten the AUTH_PARAM_KEYS comment in protect.ts to one line. - Delete three test-body comments in Session.test.ts that restated what the describe/it names already said. No behavior changes.
- Harden combine() to an allowlist of 'pass' | 'skip'. If any helper ever returns an off-type or unexpected value, treat it as a denial rather than silently authorizing. Defense in depth around the one remaining foot-gun flagged by review. - Add @clerk/backend to the changeset. authObject.has is built on createCheckAuthorization at request time, so backend's CHANGELOG should carry the fix explicitly even though the package patch-bumps transitively. - Clarify the single-condition claim in the changeset so it does not contradict the strict_mfa behavior change. - Delete narrative 'Regression: previously...' comments from the new tests; the ticket story belongs in commit messages, not in test bodies.
…, add e2e tests
Fredrik pointed out that strict_mfa / multi_factor graceful downgrade is
explicitly documented policy at clerk.com/docs/guides/secure/reverification,
not a bug. Reverting the F2 behavior change and the factor1Age===-1 early
return so every documented downgrade case continues to pass.
The reverification helper still gets its refactor: it returns crisp
'pass' | 'fail' | 'skip' instead of null so the combiner cannot silently
bypass via another true dimension. Every pre-existing Session.test assertion
remains true - the factor state x level matrix was walked against the
existing cases.
Tests:
- authorization.spec.ts: dropped the two F2 / factor1=-1 regression tests,
added three positive multi-requirement tests (permission+reverification,
role+feature, the all-three triple) per Fredrik's suggestion.
- Session.test.ts: reverted three test flips that pinned the non-graceful
variant.
- integration/tests/protect.test.ts: added e2e coverage for SDK-67 (role
AND permission) and SDK-68 (mixed auth params with unauthorizedUrl, mixed
permission with token). Three new template pages plus a /settings/denied
redirect target in the existing next-app-router template.
Changeset:
- Shrunk to @clerk/shared + @clerk/clerk-js + @clerk/nextjs (packages with
direct code changes). Others patch-bump transitively via shared.
- Reworded the 'combined checks' bullet to describe the indeterminate
collapse rather than a universal OR break.
- Dropped the has({}) and strict_mfa bullets per Fredrik.
- Simplified the Session.ts membership-id note.
Covers two combiner scenarios flagged in review: - permission + strict_mfa with [0, -1] -> pass via graceful downgrade - permission + reverification with [-1, -1] -> fail (no factors enrolled) Also qualifies the reverification-only claim in the changeset: single-condition reverification calls with malformed factorVerificationAge payloads now deny (previously returned indeterminate).
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/two-factor-authentication |
@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: |
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 (2)
📝 WalkthroughWalkthroughImplements fail-closed authorization by changing internal checks to explicit verdicts ('pass' | 'fail' | 'skip') for organization, billing, and reverification dimensions and combining them so at least one dimension must pass and any non-skipped fail causes denial. Fixes Session.checkAuthorization to use the organization id from the membership object. Updates Next.js protect() to extract only known authorization keys and to enforce authorization when those keys are present even if the same options object also contains unauthenticatedUrl, unauthorizedUrl, or token. Adds tests and Next.js example pages exercising mixed-shape protect calls. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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. 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 the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/authorization.ts`:
- Around line 112-126: The code currently assumes session claims (orgRole,
orgPermissions, features, plans) have the expected shapes and directly calls
prefixWithOrg(...).replace/.includes/.split which throws on
malformed/truthy-but-invalid values; update the guard logic in the
functions/blocks that use roleAsked, permissionAsked, feature/plan checks (the
branch using prefixWithOrg(params.role),
orgPermissions.includes(prefixWithOrg(params.permission)), and the similar block
at the later 178-192 range) to first validate the claim shape (e.g., ensure
orgRole is a string before calling prefixWithOrg, ensure orgPermissions is an
Array or string that can be split/iterated, ensure features/plans are
arrays/strings) and swallow parsing errors by returning 'fail' on validation
failure or inside a try/catch around parsing; this ensures any malformed claim
yields a safe 'fail' result instead of throwing.
🪄 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: cfeb5eb2-6d66-4091-81bf-fd56d7838ab2
📒 Files selected for processing (13)
.changeset/authorization-bypass-combined-conditions.mdintegration/templates/next-app-router/src/app/settings/auth-protect-mixed-args/page.tsxintegration/templates/next-app-router/src/app/settings/auth-protect-mixed-token/page.tsxintegration/templates/next-app-router/src/app/settings/auth-protect-role-and-permission/page.tsxintegration/templates/next-app-router/src/app/settings/denied/page.tsxintegration/tests/protect.test.tspackages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/test/core-fixtures.tspackages/nextjs/src/server/__tests__/clerkMiddleware.test.tspackages/nextjs/src/server/protect.tspackages/react/src/hooks/__tests__/useAuth.test.tsxpackages/shared/src/__tests__/authorization.spec.tspackages/shared/src/authorization.ts
5 tasks
- Wrap single-line if bodies in braces to satisfy the curly eslint rule in packages/shared/src/authorization.ts. - Cast mixed-shape auth.protect literals as any in two new regression tests and two new integration template pages so TypeScript does not block the build on excess-property checks. The fixture shape is intentionally off-type because the regression guards target runtime behavior for variable-assigned / JS callers.
🦋 Changeset detectedLatest commit: 9750ecb The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 withCustomRoles integration instance provisions custom permissions like org:posts:manage (used by existing /settings/auth-has). Our two new routes referenced org:sys_profile:delete / org:sys_memberships:read which the admin user does not hold on that instance, so the admin-path assertions failed. Swap both routes to org:posts:manage so admin passes and viewer still fails (viewer lacks the permission per existing /settings/auth-has coverage).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Ephem
approved these changes
Apr 22, 2026
Ephem
left a comment
Ephem
left a comment
Member
There was a problem hiding this comment.
I think the Rabbit had a good NIT to be extra defensive, but other than that I think this is gtg.
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.
Why
has(),auth.protect(), and related authorization predicates can returntruefor certain calls that combined more than one condition (role, permission, feature, plan, reverification) when the result should have beenfalse. When a requested dimension could not be satisfied because the underlying session data was missing or indeterminate, the combining logic ignored that dimension instead of denying. If any remaining requested dimension passed, the overall result wastrue.Separately,
auth.protect()in@clerk/nextjssilently discarded authorization params (role,permission,feature,plan,reverification) whenever the same argument object also containedunauthenticatedUrl,unauthorizedUrl, ortoken. TypeScript's excess-property check caught this only for inline object literals; variable-assigned or spread arguments and any JavaScript caller let every authenticated user through.What changed
packages/shared/src/authorization.ts'pass' | 'fail' | 'skip'tri-state. The combiner treats any asked dimension returning'fail'as a denial and requires at least one'pass'. Missing, malformed, or invalid session data now denies instead of being treated as indeterminate.billingAuthorization || orgAuthorizationOR-coercion; each asked dimension is evaluated independently.role/permission/feature/planvalues that are not strings (e.g.nullcast throughas any) now fail closed instead of throwing.factorVerificationAgetuple shape validation (length, numeric,-1 | >= 0) before comparisons.strict/strict_mfaper the existing docs: when no second factor is enrolled, these levels still evaluate against the first factor.multi_factorstill requires both factors when both are enrolled.packages/clerk-js/src/core/resources/Session.tssession.checkAuthorization()now uses the active organization's id (activeMembership.organization.id) when building authorization options, not the membership row id.packages/clerk-js/src/test/core-fixtures.tspackages/nextjs/src/server/protect.tsgetAuthorizationParamsnow picks the known authorization keys from the argument via an explicit allowlist. Mixed shapes like{ role, unauthorizedUrl }or{ permission, token }enforce the authorization check; options-only objects take the fast path; unknown extra keys are ignored.packages/react/src/hooks/__tests__/useAuth.test.tsxvi.mockofcreateCheckAuthorizationso the React hook tests exercise the real predicate.Tests
packages/shared/src/__tests__/authorization.spec.tscovering every "asked but unsatisfiable" cell, AND semantics within and across dimensions, malformed payloads, null / non-string params, invalid reverification configs, and positive multi-requirement cases.packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts.integration/tests/protect.test.tsfor three new/settings/*routes in the existingnext-app-routertemplate:auth-protect-mixed-args,auth-protect-mixed-token,auth-protect-role-and-permission. Admin / signed-out / viewer assertions added.Changeset
@clerk/shared,@clerk/clerk-js,@clerk/nextjsat patch. Other packages pick up the fix transitively via the shared patch.Test plan
pnpm --filter @clerk/shared test- 25/25 on the new matrixpnpm --filter @clerk/clerk-js test- Session.test.ts greenpnpm --filter @clerk/clerk-react test- useAuth.test.tsx green after removing the mockpnpm --filter @clerk/backend test- greenpnpm --filter @clerk/nextjs test- middleware tests green (pre-existing type-only failures unrelated to this PR)pnpm --filter @clerk/vue test/pnpm --filter @clerk/astro test- greenintegration/tests/protect.test.ts- new admin / viewer / signed-out assertions run in CI against thewithCustomRoles+withBillingJwtV2instances.Backport
A Core 2 backport is prepared on the advisory's private fork and will be merged once this lands.