Skip to content
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

chore(backend,types): Prevent system permissions usage in server-side #4816

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
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
8 changes: 8 additions & 0 deletions .changeset/fair-bobcats-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@clerk/backend': patch
'@clerk/types': patch
---

Add type-level validation to prevent server-side usage of system permissions

System permissions (e.g., `org:sys_domains:manage`) are intentionally excluded from session claims to maintain reasonable JWT sizes. For more information, refer to our docs: https://clerk.com/docs/organizations/roles-permissions#system-permissions
6 changes: 3 additions & 3 deletions packages/backend/src/tokens/authObjects.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { createCheckAuthorization } from '@clerk/shared/authorization';
import type {
ActClaim,
CheckAuthorizationWithCustomPermissions,
CheckAuthorizationFromSessionClaims,
JwtPayload,
OrganizationCustomPermissionKey,
OrganizationCustomRoleKey,
Expand Down Expand Up @@ -42,7 +42,7 @@ export type SignedInAuthObject = {
*/
factorVerificationAge: [number, number] | null;
getToken: ServerGetToken;
has: CheckAuthorizationWithCustomPermissions;
has: CheckAuthorizationFromSessionClaims;
debug: AuthObjectDebug;
};

Expand All @@ -65,7 +65,7 @@ export type SignedOutAuthObject = {
*/
factorVerificationAge: null;
getToken: ServerGetToken;
has: CheckAuthorizationWithCustomPermissions;
has: CheckAuthorizationFromSessionClaims;
debug: AuthObjectDebug;
};

Expand Down
7 changes: 7 additions & 0 deletions packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ describe('ClerkMiddleware type tests', () => {
clerkMiddlewareMock();
});

it('prevents usage of system permissions with auth.has()', () => {
clerkMiddlewareMock(async (auth, _event, _request) => {
// @ts-expect-error - system permissions are not allowed
(await auth()).has({ permission: 'org:sys_foo' });
});
});

Comment on lines +116 to +122
Copy link
Member

Choose a reason for hiding this comment

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

do you think we should add a test case for useAuth() where the type says that sys permissions are allowed ?

In order to not break something in the future

describe('Multi domain', () => {
const defaultProps = { publishableKey: '', secretKey: '' };

Expand Down
2 changes: 1 addition & 1 deletion packages/types/src/jwtv2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export interface JwtPayload extends CustomJwtSessionClaims {
org_role?: OrganizationCustomRoleKey;

/**
* Active organization role
* Active organization permissions
*/
org_permissions?: OrganizationCustomPermissionKey[];

Expand Down
13 changes: 7 additions & 6 deletions packages/types/src/organizationMembership.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,14 @@ export type OrganizationCustomRoleKey = ClerkAuthorization extends Placeholder
: Base['role']
: Base['role'];

export type OrganizationSystemPermissionPrefix = 'org:sys_';
export type OrganizationSystemPermissionKey =
| 'org:sys_domains:manage'
| 'org:sys_profile:manage'
| 'org:sys_profile:delete'
| 'org:sys_memberships:read'
| 'org:sys_memberships:manage'
| 'org:sys_domains:read';
| `${OrganizationSystemPermissionPrefix}domains:manage`
| `${OrganizationSystemPermissionPrefix}profile:manage`
| `${OrganizationSystemPermissionPrefix}profile:delete`
| `${OrganizationSystemPermissionPrefix}memberships:read`
| `${OrganizationSystemPermissionPrefix}memberships:manage`
| `${OrganizationSystemPermissionPrefix}domains:read`;

/**
* OrganizationPermissionKey is a combination of system and custom permissions.
Expand Down
24 changes: 24 additions & 0 deletions packages/types/src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
OrganizationCustomPermissionKey,
OrganizationCustomRoleKey,
OrganizationPermissionKey,
OrganizationSystemPermissionPrefix,
} from './organizationMembership';
import type { ClerkResource } from './resource';
import type {
Expand All @@ -25,6 +26,29 @@ import type {
import type { TokenResource } from './token';
import type { UserResource } from './user';

type DisallowSystemPermissions<P extends string> = P extends `${OrganizationSystemPermissionPrefix}${string}`
? 'System permissions are not included in session claims and cannot be used on the server-side'
: P;
Comment on lines +29 to +31
Copy link
Member

Choose a reason for hiding this comment

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

This is really cool!


/**
* Type guard for server-side authorization checks using session claims.
* System permissions are not allowed since they are not included
* in session claims and cannot be verified on the server side.
*/
export type CheckAuthorizationFromSessionClaims = <P extends OrganizationCustomPermissionKey>(
isAuthorizedParams: WithReverification<
| {
role: OrganizationCustomRoleKey;
permission?: never;
}
| {
role?: never;
permission: DisallowSystemPermissions<P>;
}
| { role?: never; permission?: never }
>,
) => boolean;

export type CheckAuthorizationFn<Params> = (isAuthorizedParams: Params) => boolean;

export type CheckAuthorizationWithCustomPermissions =
Expand Down
Loading