fix(shared): decode JWT v2 permission masks exactly - #9381
Conversation
🦋 Changeset detectedLatest commit: 9dd9f76 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 (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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
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.Type of change