Skip to content
Merged
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/moody-sites-lead.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/shared': major
---

Adjust features parsing to throw errors on unknown scopes.
2 changes: 1 addition & 1 deletion packages/clerk-js/bundlewatch.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
{ "path": "./dist/clerk.browser.js", "maxSize": "66KB" },
{ "path": "./dist/clerk.chips.browser.js", "maxSize": "66KB" },
{ "path": "./dist/clerk.legacy.browser.js", "maxSize": "106KB" },
{ "path": "./dist/clerk.no-rhc.js", "maxSize": "305KB" },
{ "path": "./dist/clerk.no-rhc.js", "maxSize": "307KB" },
{ "path": "./dist/clerk.native.js", "maxSize": "65KB" },
{ "path": "./dist/vendors*.js", "maxSize": "7KB" },
{ "path": "./dist/coinbase*.js", "maxSize": "36KB" },
Expand Down
44 changes: 44 additions & 0 deletions packages/shared/src/__tests__/authorization.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { describe, expect, it } from 'vitest';

import { createCheckAuthorization, splitByScope } from '../authorization';

describe('createCheckAuthorization', () => {
it('correctly parses features', () => {
const checkAuthorization = createCheckAuthorization({
userId: 'user_123',
orgId: 'org_123',
orgRole: 'admin',
orgPermissions: ['org:read'],
features: 'o:reservations,u:dashboard',
plans: 'free_user,plus_user',
factorVerificationAge: [1000, 2000],
});
expect(checkAuthorization({ feature: 'o:reservations' })).toBe(true);
expect(checkAuthorization({ feature: 'org:reservations' })).toBe(true);
expect(checkAuthorization({ feature: 'organization:reservations' })).toBe(true);
expect(checkAuthorization({ feature: 'reservations' })).toBe(true);
expect(checkAuthorization({ feature: 'u:dashboard' })).toBe(true);
expect(checkAuthorization({ feature: 'user:dashboard' })).toBe(true);
expect(checkAuthorization({ feature: 'dashboard' })).toBe(true);

expect(() => checkAuthorization({ feature: 'lol:dashboard' })).toThrow('Invalid scope: lol');
});
});

describe('splitByScope', () => {
it('correctly splits features by scope', () => {
const { org, user } = splitByScope('o:reservations,u:dashboard');
expect(org).toEqual(['reservations']);
expect(user).toEqual(['dashboard']);
});

it('correctly splits features by scope with multiple scopes', () => {
const { org, user } = splitByScope('o:reservations,u:dashboard,ou:support-chat,uo:billing');
expect(org).toEqual(['reservations', 'support-chat', 'billing']);
expect(user).toEqual(['dashboard', 'support-chat', 'billing']);
});

it('throws an error if the claim element is missing a colon', () => {
expect(() => splitByScope('reservations,dashboard')).toThrow('Invalid claim element (missing colon): reservations');
});
});
65 changes: 49 additions & 16 deletions packages/shared/src/authorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ const ALLOWED_LEVELS = new Set<SessionVerificationLevel>(['first_factor', 'secon

const ALLOWED_TYPES = new Set<SessionVerificationTypes>(['strict_mfa', 'strict', 'moderate', 'lax']);

const ORG_SCOPES = new Set(['o', 'org', 'organization']);
const USER_SCOPES = new Set(['u', 'user']);

// Helper functions
const isValidMaxAge = (maxAge: any) => typeof maxAge === 'number' && maxAge > 0;
const isValidLevel = (level: any) => ALLOWED_LEVELS.has(level);
Expand Down Expand Up @@ -100,17 +103,26 @@ const checkOrgAuthorization: CheckOrgAuthorization = (params, options) => {

const checkForFeatureOrPlan = (claim: string, featureOrPlan: string) => {
const { org: orgFeatures, user: userFeatures } = splitByScope(claim);
const [scope, _id] = featureOrPlan.split(':');
const id = _id || scope;

if (scope === 'org') {
return orgFeatures.includes(id);
} else if (scope === 'user') {
return userFeatures.includes(id);
} else {
// Since org scoped features will not exist if there is not an active org, merging is safe.
return [...orgFeatures, ...userFeatures].includes(id);
const [rawScope, rawId] = featureOrPlan.split(':');
const hasExplicitScope = rawId !== undefined;
const scope = rawScope;
const id = rawId || rawScope;

if (hasExplicitScope && !ORG_SCOPES.has(scope) && !USER_SCOPES.has(scope)) {
throw new Error(`Invalid scope: ${scope}`);
}

if (hasExplicitScope) {
if (ORG_SCOPES.has(scope)) {
return orgFeatures.includes(id);
}
if (USER_SCOPES.has(scope)) {
return userFeatures.includes(id);
}
}

// Since org scoped features will not exist if there is not an active org, merging is safe.
return [...orgFeatures, ...userFeatures].includes(id);
};

const checkBillingAuthorization: CheckBillingAuthorization = (params, options) => {
Expand All @@ -127,13 +139,34 @@ const checkBillingAuthorization: CheckBillingAuthorization = (params, options) =
};

const splitByScope = (fea: string | null | undefined) => {
Copy link
Member

Choose a reason for hiding this comment

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

This got flagged by Codex

splitByScope edge case: missing :

In the new loop, it does:
• const colonIndex = part.indexOf(':')
• then slices using that index. 

If a segment ever lacks : (or is malformed), colonIndex becomes -1, and the slicing behavior gets weird (scope becomes part.slice(0, -1) and value becomes the entire string). Today the features claim may always be well-formed, but this helper is the kind of thing that gets reused and then surprises someone later.

Suggestion: guard early:
• if colonIndex <= 0 (or === -1) → continue (or treat as invalid and throw, depending on how strict you want claims parsing to be).
• also consider trimming out empty value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a check! These are from FAPI so they're likely always well-formed, but it doesn't hurt to at least throw an error.

const features = fea ? fea.split(',').map(f => f.trim()) : [];
const org: string[] = [];
const user: string[] = [];

// TODO: make this more efficient
return {
org: features.filter(f => f.split(':')[0].includes('o')).map(f => f.split(':')[1]),
user: features.filter(f => f.split(':')[0].includes('u')).map(f => f.split(':')[1]),
};
if (!fea) {
return { org, user };
}

const parts = fea.split(',');
for (let i = 0; i < parts.length; i++) {
const part = parts[i].trim();
const colonIndex = part.indexOf(':');
if (colonIndex === -1) {
throw new Error(`Invalid claim element (missing colon): ${part}`);
}
const scope = part.slice(0, colonIndex);
const value = part.slice(colonIndex + 1);

if (scope === 'o') {
org.push(value);
} else if (scope === 'u') {
user.push(value);
} else if (scope === 'ou' || scope === 'uo') {
org.push(value);
user.push(value);
}
}

return { org, user };
Comment on lines 141 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

splitByScope drops valid long scopes and still ignores unknown scopes

splitByScope only accepts o/u/ou/uo, so claim entries like org:feature or user:feature are silently discarded, and truly unknown scopes still pass without error. This contradicts the PR’s stated behavior and can lead to false negatives in authorization. Please accept long scopes and throw on unknown scopes here as well.

💡 Suggested fix
-    if (scope === 'o') {
-      org.push(value);
-    } else if (scope === 'u') {
-      user.push(value);
-    } else if (scope === 'ou' || scope === 'uo') {
-      org.push(value);
-      user.push(value);
-    }
+    if (ORG_SCOPES.has(scope)) {
+      org.push(value);
+    } else if (USER_SCOPES.has(scope)) {
+      user.push(value);
+    } else if (scope === 'ou' || scope === 'uo') {
+      org.push(value);
+      user.push(value);
+    } else {
+      throw new Error(`Invalid claim scope: ${scope}`);
+    }
🤖 Prompt for AI Agents
In `@packages/shared/src/authorization.ts` around lines 141 - 169, The
splitByScope function currently only handles short scope keys
('o','u','ou','uo') and silently ignores longer or unknown scopes; update
splitByScope to accept both short and long forms (map 'o' or 'org' to org, 'u'
or 'user' to user, and combined forms like 'ou','uo','org:user' equivalents) and
when a scope token isn't recognized throw an Error instead of ignoring it;
modify the scope detection logic inside splitByScope (the code that computes
scope and value from part and the subsequent if/else branches) to perform these
mappings and add a final else that throws an error for unknown scopes.

};

const validateReverificationConfig = (config: ReverificationConfig | undefined | null) => {
Expand Down
Loading