Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/exact-jwt-permission-masks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/shared': patch
---

Ensure organization permission checks remain accurate when JWT v2 permission masks exceed JavaScript's safe integer range.
80 changes: 79 additions & 1 deletion packages/shared/src/__tests__/jwtPayloadParser.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, test } from 'vitest';

import { splitByScope } from '../authorization';
import { createCheckAuthorization, splitByScope } from '../authorization';
import { __experimental_JWTPayloadToAuthObjectProperties as JWTPayloadToAuthObjectProperties } from '../jwtPayloadParser';

const baseClaims = {
Expand All @@ -14,6 +14,33 @@ const baseClaims = {
__raw: '',
};

const permissionNames = Array.from({ length: 54 }, (_, index) => `permission_${index}`);

const authFromFeaturePermissionMask = (fpm: string) => {
const authObject = JWTPayloadToAuthObjectProperties({
...baseClaims,
v: 2,
fea: 'o:feature',
o: {
id: 'org_id',
rol: 'admin',
per: permissionNames.join(','),
fpm,
},
});
const has = createCheckAuthorization({
userId: authObject.userId,
orgId: authObject.orgId,
orgRole: authObject.orgRole,
orgPermissions: authObject.orgPermissions,
factorVerificationAge: authObject.factorVerificationAge,
features: null,
plans: null,
});

return { authObject, has };
};

describe('JWTPayloadToAuthObjectProperties', () => {
test('auth object with JWT v2 does not produces anything org related if there is no org active', () => {
const { sessionClaims: v2Claims, ...signedInAuthObjectV2 } = JWTPayloadToAuthObjectProperties({
Expand Down Expand Up @@ -261,6 +288,57 @@ describe('JWTPayloadToAuthObjectProperties', () => {
].sort(),
);
});

test('preserves permissions above the safe integer boundary', () => {
const { authObject, has } = authFromFeaturePermissionMask('9007199254740993');

expect(authObject.orgPermissions).toEqual(['org:feature:permission_0', 'org:feature:permission_53']);
expect(has({ permission: 'org:feature:permission_0' })).toBe(true);
expect(has({ permission: 'org:feature:permission_53' })).toBe(true);
});

test('does not introduce permissions when decoding a mask above the safe integer boundary', () => {
const { authObject, has } = authFromFeaturePermissionMask('9007199254740995');

expect(authObject.orgPermissions).toEqual([
'org:feature:permission_0',
'org:feature:permission_1',
'org:feature:permission_53',
]);
expect(has({ permission: 'org:feature:permission_0' })).toBe(true);
expect(has({ permission: 'org:feature:permission_1' })).toBe(true);
expect(has({ permission: 'org:feature:permission_2' })).toBe(false);
expect(has({ permission: 'org:feature:permission_53' })).toBe(true);
});

test('discards mask bits outside the declared permission list', () => {
const { authObject, has } = authFromFeaturePermissionMask('18014398509481984');

expect(authObject.orgPermissions).toEqual([]);
expect(has({ permission: 'org:feature:undefined' })).toBe(false);
});

test.each([
['1', [0]],
['3', [0, 1]],
['7', [0, 1, 2]],
['21', [0, 2, 4]],
])('preserves permissions for the safe mask %s', (fpm, expectedPermissionIndexes) => {
const { authObject, has } = authFromFeaturePermissionMask(fpm);
const expectedPermissions = expectedPermissionIndexes.map(index => `org:feature:permission_${index}`);

expect(authObject.orgPermissions).toEqual(expectedPermissions);
for (const permission of expectedPermissions) {
expect(has({ permission })).toBe(true);
}
});

test.each(['1invalid', '-1', '1.5'])('fails closed for the malformed mask %s', fpm => {
const { authObject, has } = authFromFeaturePermissionMask(fpm);

expect(authObject.orgPermissions).toEqual([]);
expect(has({ permission: 'org:feature:permission_0' })).toBe(false);
});
});

describe('splitByScope ', () => {
Expand Down
50 changes: 38 additions & 12 deletions packages/shared/src/jwtPayloadParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,52 @@ import type {
SharedSignedInAuthObjectProperties,
} from './types';

const decimalToBinaryBits = (decimal: string, minimumLength: number): number[] | undefined => {
if (!/^\d+$/.test(decimal)) {
return undefined;
}

let remaining = decimal.replace(/^0+/, '') || '0';
const bits: number[] = [];

while (remaining !== '0') {
let quotient = '';
let remainder = 0;

for (let i = 0; i < remaining.length; i++) {
const value = remainder * 10 + remaining.charCodeAt(i) - 48;
const quotientDigit = Math.floor(value / 2);

if (quotient || quotientDigit !== 0) {
quotient += quotientDigit;
}
remainder = value % 2;
}

bits.push(remainder);
remaining = quotient || '0';
}

if (bits.length === 0) {
bits.push(0);
}
while (bits.length < minimumLength) {
bits.push(0);
}

return bits;
};

export const parsePermissions = ({ per, fpm }: { per?: string; fpm?: string }) => {
if (!per || !fpm) {
return { permissions: [], featurePermissionMap: [] };
}

const permissions = per.split(',').map(p => p.trim());

// TODO: make this more efficient
const featurePermissionMap = fpm
.split(',')
.map(permission => Number.parseInt(permission.trim(), 10))
.map((permission: number) =>
permission
.toString(2)
.padStart(permissions.length, '0')
.split('')
.map(bit => Number.parseInt(bit, 10))
.reverse(),
)
.filter(Boolean);
.map(permission => decimalToBinaryBits(permission.trim(), permissions.length) ?? []);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

return { permissions, featurePermissionMap };
};
Expand Down Expand Up @@ -62,7 +88,7 @@ function buildOrgPermissions({
continue;
}

for (let permIndex = 0; permIndex < permissionBits.length; permIndex++) {
for (let permIndex = 0; permIndex < permissionBits.length && permIndex < permissions.length; permIndex++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

if (permissionBits[permIndex] === 1) {
orgPermissions.push(`org:${feature}:${permissions[permIndex]}`);
}
Expand Down
Loading