-
Notifications
You must be signed in to change notification settings - Fork 434
fix(shared): Throw error on unknown scopes #7754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0e684d3
2cf4786
128de6c
fde995e
708d7db
a371353
a9b2223
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| 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'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
|
@@ -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) => { | ||
|
|
@@ -127,13 +139,34 @@ const checkBillingAuthorization: CheckBillingAuthorization = (params, options) = | |
| }; | ||
|
|
||
| const splitByScope = (fea: string | null | undefined) => { | ||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. splitByScope drops valid long scopes and still ignores unknown scopes
💡 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 |
||
| }; | ||
|
|
||
| const validateReverificationConfig = (config: ReverificationConfig | undefined | null) => { | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.