fix(shared): decode JWT v2 permission masks exactly - #9381
Conversation
🦋 Changeset detectedLatest commit: ccf4741 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: |
|
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 Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe JWT payload parser now converts decimal permission masks through custom decimal division. This avoids precision loss for values beyond JavaScript’s safe integer range. Invalid masks produce empty bit arrays. Tests cover large masks, valid safe masks, out-of-range bits, malformed masks, and fail-closed authorization checks. A patch changeset was added for Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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/shared/src/jwtPayloadParser.ts`:
- Around line 52-54: Update buildOrgPermissions and the featurePermissionMap
handling around decimalToBinaryBits so bit indexes greater than or equal to
permissions.length are discarded before permission names are resolved. Preserve
valid permission mappings and add a regression test covering the 54-item list
with mask 18014398509481984, ensuring no org:feature:undefined permission is
produced.
🪄 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: 1868a535-5352-4646-8435-325176767171
📒 Files selected for processing (3)
.changeset/exact-jwt-permission-masks.mdpackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/jwtPayloadParser.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)
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
| } | ||
|
|
||
| for (let permIndex = 0; permIndex < permissionBits.length; permIndex++) { | ||
| for (let permIndex = 0; permIndex < permissionBits.length && permIndex < permissions.length; permIndex++) { |
There was a problem hiding this comment.
[MEDIUM] fea/fpm index misalignment attaches a feature's permission mask to the wrong feature when feature targeting is active
buildOrgPermissions assumes featurePermissionMap[i] corresponds to features[i], but the encoder does not emit a mask per feature — pkg/auth/v2.go:223 skips any feature not in featuresInPermissions, so fpm is a compacted list while fea is the full one. The two only stay aligned when the permission-bearing features happen to form a prefix of fea. When params.Plan != nil (v2.go:69-95) featureSet is seeded from plan features and targeted features are appended after, breaking that prefix invariant — a plan feature the member has no permissions on then inherits the next mask in the list, so has({ permission: 'org:<wrong-feature>:manage' }) returns true for a permission the user was never granted.
This predates the diff, but it is in the function this PR rewrites and the PR's stated goal is decoding these masks exactly — the new permIndex < permissions.length clamp bounds the inner loop without fixing the outer index mapping. Suggest having the encoder emit a mask for every entry in fea (zero for features with no permissions), or emitting the feature name alongside each mask so the decoder can key on it rather than on position.
— Comment generated with Claude with @dominic-clerk's supervision
There was a problem hiding this comment.
Confirmed — this is a valid pre-existing encoder issue. I reproduced the false grant through the JWT v2 auth-object conversion and has() with a permission-bearing plan feature, a plan feature without permissions, and a permission-bearing targeted feature.
There isn't enough information in the current claims for ClerkJS to repair the mapping safely: once zero masks are omitted, the decoder cannot tell whether the next compacted mask belongs to the next fea entry or a later targeted feature. Failing closed whenever fea.length !== fpm.length would also deny valid permissions for existing plan tokens.
The correct fix is in the backend: emit one fpm entry for every fea entry, using 0 for features without permissions, and cover the plan-plus-targeting case there. I'm keeping this PR scoped to exact decimal mask decoding and undeclared-bit handling rather than adding an ambiguous decoder heuristic.
— Comment generated with Codex with @jeremy-clerk's supervision
There was a problem hiding this comment.
👍 I'll follow-up on this with a backend PR
There was a problem hiding this comment.
There was a problem hiding this comment.
Verified that the backend fix is merged. This PR is now updated to match the new positional contract: fixtures include 0 masks for features without permissions, and the regression covers fea = repositories,impersonation,billing with fpm = 6,0,3 through the v2 auth object and has().
— Comment generated with Codex with @jeremy-clerk's supervision
2 participants
Description
JWT v2 organization permission masks were parsed through JavaScript numbers, which could lose precision above the safe integer limit and cause
auth().has()to incorrectly grant or deny permissions.Decode permission masks directly from their decimal strings with
decimalToBinaryBits. This avoids relying onBigInt, which may be incompatible with ClerkJS's legacy browser bundle, while preserving the existing bit ordering for normal masks. Malformed and negative masks now fail closed.The backend now emits one permission mask per feature, using
0for features without permissions. The v2 authorization regressions cover this positional contract, including a targeted feature that follows a permission-less plan feature.Type of change