Conversation
Adds CRUD checks to certain tRPC routes to make sure the current user is able to do the action.
perkinsjr
requested review from
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds ctx-aware validation to org- and workspace-scoped TRPC resolvers, rejecting requests when input IDs don’t match the current context. Updates org routes to destructure ctx, enforce orgId equality, and keep existing provider calls and error mapping. Disables Next.js X-Powered-By header via next.config.js. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as TRPC Router (Org*)
participant Ctx as ctx.workspace
participant AP as authProvider
C->>R: Call org resolver (input.orgId, ...)
R->>Ctx: Read orgId
alt orgId mismatch
R-->>C: TRPCError BAD_REQUEST ("Invalid organization ID")
else orgId matches
R->>AP: invoke (e.g., getInvitationList / invite / revoke / update / remove)
AP-->>R: result or error
alt provider error
R-->>C: TRPCError INTERNAL_SERVER_ERROR (cause=provider error)
else success
R-->>C: result
end
end
note over R,AP: Applies to getInvitationList, inviteMember, revokeInvitation,<br/>updateMembership, removeMembership
sequenceDiagram
autonumber
participant C as Client
participant R as TRPC Router (changeWorkspaceName)
participant Ctx as ctx.workspace
participant DB as DB Tx / Audit / OrgName Sync
C->>R: changeWorkspaceName(input.workspaceId, name)
R->>Ctx: Read workspaceId
alt workspaceId mismatch
R-->>C: TRPCError BAD_REQUEST ("Invalid workspace ID")
else matches
R->>DB: run transaction + audit + org-name update
DB-->>R: success or error
alt error
R-->>C: TRPCError INTERNAL_SERVER_ERROR (cause=error)
else success
R-->>C: updated workspace
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
| .query(async ({ input: orgId }) => { | ||
| .query(async ({ ctx, input: orgId }) => { | ||
| try { | ||
| if (orgId !== ctx.workspace?.orgId) { |
There was a problem hiding this comment.
this should've never been passed from the frontend and still shouldn't, why don't we use ctx.workspace.orgId and remove the input altogether?
| .mutation(async ({ input }) => { | ||
| .mutation(async ({ ctx, input }) => { | ||
| try { | ||
| if (input.orgId !== ctx.workspace?.orgId) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/dashboard/lib/trpc/routers/org/inviteMember.ts (2)
16-28: Harden authorization: return 403 and pass orgId from ctx (not user input)Avoid 400s for auth failures and prefer ctx-derived IDs when calling providers.
- .mutation(async ({ ctx, input }) => { + .mutation(async ({ ctx, input }) => { try { - if (input.orgId !== ctx.workspace?.orgId) { + if (input.orgId !== ctx.workspace?.orgId) { throw new TRPCError({ - code: "BAD_REQUEST", - message: "Invalid organization ID", + code: "FORBIDDEN", + message: "Forbidden", }); } return await authProvider.inviteMember({ email: input.email, role: input.role, - orgId: input.orgId, + orgId: ctx.workspace!.orgId, });
10-14: Validate email formatUse Zod’s .email() to reject malformed addresses early.
- email: z.string(), + email: z.string().email(),apps/dashboard/lib/trpc/routers/org/revokeInvitation.ts (1)
15-30: Guarded TRPCError is rewritten to 500; move guard out of try and rethrow TRPCError in catch.The authorization guard throws inside the try; the catch then converts it to INTERNAL_SERVER_ERROR, losing the intended status. Also prefer FORBIDDEN over BAD_REQUEST to avoid ID enumeration and better reflect semantics.
- .mutation(async ({ ctx, input }) => { - try { - if (input.orgId !== ctx.workspace?.orgId) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Invalid organization ID", - }); - } - return await authProvider.revokeOrgInvitation(input.invitationId); - } catch (error) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to revoke invitation", - cause: error, - }); - } - }); + .mutation(async ({ ctx, input }) => { + // Ensure this does not get caught and rewritten as 500 + if (input.orgId !== ctx.workspace?.orgId) { + throw new TRPCError({ code: "FORBIDDEN", message: "Forbidden" }); + } + try { + return await authProvider.revokeOrgInvitation(input.invitationId); + } catch (error) { + if (error instanceof TRPCError) throw error; + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to revoke invitation", + cause: error, + }); + } + });apps/dashboard/lib/trpc/routers/org/updateMembership.ts (3)
16-34: Same catch-and-rewrite bug; use early guard and preserve TRPCError.Move the orgId check out of the try and rethrow TRPCError instances. Use FORBIDDEN for mismatches.
- .mutation(async ({ ctx, input }) => { - try { - if (input.orgId !== ctx.workspace?.orgId) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Invalid organization ID", - }); - } - return await authProvider.updateMembership({ - membershipId: input.membershipId, - role: input.role, - }); - } catch (error) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to update membership", - cause: error, - }); - } - }); + .mutation(async ({ ctx, input }) => { + if (input.orgId !== ctx.workspace?.orgId) { + throw new TRPCError({ code: "FORBIDDEN", message: "Forbidden" }); + } + try { + return await authProvider.updateMembership({ + membershipId: input.membershipId, + role: input.role, + }); + } catch (error) { + if (error instanceof TRPCError) throw error; + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to update membership", + cause: error, + }); + } + });
10-14: Constrain role with Zod enum.role is a free-form string. Prefer z.enum([...]) or z.nativeEnum(Role) from shared types to prevent invalid role escalation.
24-27: Enforce org-scoped validation inupdateMembership
PassorgIdintoauthProvider.updateMembershipand, in each implementation (apps/dashboard/lib/auth/local.ts,…/workos.ts), verify the givenmembershipIdbelongs to that org before applying the role change to prevent cross-organization updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/dashboard/lib/trpc/routers/org/getInvitationList.ts(1 hunks)apps/dashboard/lib/trpc/routers/org/inviteMember.ts(1 hunks)apps/dashboard/lib/trpc/routers/org/removeMembership.ts(1 hunks)apps/dashboard/lib/trpc/routers/org/revokeInvitation.ts(1 hunks)apps/dashboard/lib/trpc/routers/org/updateMembership.ts(1 hunks)apps/dashboard/lib/trpc/routers/workspace/changeName.ts(1 hunks)apps/dashboard/next.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages
Files:
apps/dashboard/next.config.jsapps/dashboard/lib/trpc/routers/org/removeMembership.tsapps/dashboard/lib/trpc/routers/org/inviteMember.tsapps/dashboard/lib/trpc/routers/workspace/changeName.tsapps/dashboard/lib/trpc/routers/org/revokeInvitation.tsapps/dashboard/lib/trpc/routers/org/updateMembership.tsapps/dashboard/lib/trpc/routers/org/getInvitationList.ts
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
apps/dashboard/next.config.jsapps/dashboard/lib/trpc/routers/org/removeMembership.tsapps/dashboard/lib/trpc/routers/org/inviteMember.tsapps/dashboard/lib/trpc/routers/workspace/changeName.tsapps/dashboard/lib/trpc/routers/org/revokeInvitation.tsapps/dashboard/lib/trpc/routers/org/updateMembership.tsapps/dashboard/lib/trpc/routers/org/getInvitationList.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects
Files:
apps/dashboard/lib/trpc/routers/org/removeMembership.tsapps/dashboard/lib/trpc/routers/org/inviteMember.tsapps/dashboard/lib/trpc/routers/workspace/changeName.tsapps/dashboard/lib/trpc/routers/org/revokeInvitation.tsapps/dashboard/lib/trpc/routers/org/updateMembership.tsapps/dashboard/lib/trpc/routers/org/getInvitationList.ts
🧠 Learnings (2)
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
apps/dashboard/lib/trpc/routers/workspace/changeName.ts
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using `eq(table.workspaceId, ctx.workspace.id)` to prevent cross-workspace access.
Applied to files:
apps/dashboard/lib/trpc/routers/workspace/changeName.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/next.config.js (1)
16-16: Disable X-Powered-By: LGTMThis closes the pentest finding for the header disclosure.
apps/dashboard/lib/trpc/routers/org/getInvitationList.ts (1)
9-17: requireOrgAdmin middleware input shape is already correct
requireOrgAdmin treats rawInput as either a string (the orgId) or an object with anorgIdproperty—using.input(z.string())for getInvitationList is valid; no change needed.Likely an incorrect or invalid review comment.
apps/dashboard/lib/trpc/routers/org/removeMembership.ts (1)
15-23: removeMembership signature verified Only requires a singlemembershipIdstring; passinginput.membershipIdis correct.
| .input(z.string()) | ||
| .query(async ({ input: orgId }) => { | ||
| .query(async ({ ctx, input: orgId }) => { | ||
| try { | ||
| if (orgId !== ctx.workspace?.orgId) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid organization ID", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Input shape likely breaks requireOrgAdmin; also use FORBIDDEN (403)
requireOrgAdmin typically expects input.orgId. Current z.string() input won’t expose orgId to the middleware. Also, make the auth failure 403.
- .input(z.string())
- .query(async ({ ctx, input: orgId }) => {
+ .input(z.object({ orgId: z.string() }))
+ .query(async ({ ctx, input: { orgId } }) => {
try {
- if (orgId !== ctx.workspace?.orgId) {
+ if (orgId !== ctx.workspace?.orgId) {
throw new TRPCError({
- code: "BAD_REQUEST",
- message: "Invalid organization ID",
+ code: "FORBIDDEN",
+ message: "Forbidden",
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .input(z.string()) | |
| .query(async ({ input: orgId }) => { | |
| .query(async ({ ctx, input: orgId }) => { | |
| try { | |
| if (orgId !== ctx.workspace?.orgId) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid organization ID", | |
| }); | |
| } | |
| .input(z.object({ orgId: z.string() })) | |
| .query(async ({ ctx, input: { orgId } }) => { | |
| try { | |
| if (orgId !== ctx.workspace?.orgId) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "Forbidden", | |
| }); | |
| } | |
| // ...rest of your query logic | |
| } catch (error) { | |
| // ...existing error handling | |
| } | |
| }) |
🤖 Prompt for AI Agents
In apps/dashboard/lib/trpc/routers/org/getInvitationList.ts around lines 9 to
17, the route currently declares .input(z.string()) and compares the raw input
to ctx.workspace?.orgId which breaks middleware like requireOrgAdmin that
expects an object with orgId; change the input schema to an object (e.g.,
z.object({ orgId: z.string() })), use input.orgId in the comparison and anywhere
else, and when the orgId does not match throw a TRPCError with code "FORBIDDEN"
and an appropriate message.
| .mutation(async ({ ctx, input }) => { | ||
| try { | ||
| if (input.orgId !== ctx.workspace?.orgId) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid organization ID", | ||
| }); | ||
| } | ||
| return await authProvider.removeMembership(input.membershipId); |
There was a problem hiding this comment.
Ensure membershipId is scoped to the same org (potential auth bypass)
An admin of org A could supply orgId of A but a membershipId from org B if the provider doesn’t enforce scoping. Pass orgId to the provider or pre-validate membership ownership.
- .mutation(async ({ ctx, input }) => {
+ .mutation(async ({ ctx, input }) => {
try {
- if (input.orgId !== ctx.workspace?.orgId) {
+ if (input.orgId !== ctx.workspace?.orgId) {
throw new TRPCError({
- code: "BAD_REQUEST",
- message: "Invalid organization ID",
+ code: "FORBIDDEN",
+ message: "Forbidden",
});
}
- return await authProvider.removeMembership(input.membershipId);
+ // Option A: enforce scoping in provider call
+ return await authProvider.removeMembership({
+ membershipId: input.membershipId,
+ orgId: ctx.workspace!.orgId,
+ });If the provider doesn’t accept an object, pre-fetch the membership and verify its org matches ctx.workspace.orgId before removal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .mutation(async ({ ctx, input }) => { | |
| try { | |
| if (input.orgId !== ctx.workspace?.orgId) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid organization ID", | |
| }); | |
| } | |
| return await authProvider.removeMembership(input.membershipId); | |
| .mutation(async ({ ctx, input }) => { | |
| try { | |
| if (input.orgId !== ctx.workspace?.orgId) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "Forbidden", | |
| }); | |
| } | |
| return await authProvider.removeMembership({ | |
| membershipId: input.membershipId, | |
| orgId: ctx.workspace!.orgId, | |
| }); |
🤖 Prompt for AI Agents
In apps/dashboard/lib/trpc/routers/org/removeMembership.ts around lines 15 to
23, the current check only validates input.orgId against ctx.workspace.orgId but
does not ensure input.membershipId actually belongs to that org, allowing an
admin to remove a membership from another org; fix by either passing the orgId
into authProvider.removeMembership (e.g., removeMembership({ membershipId, orgId
})) so the provider enforces scoping, or if the provider API cannot be changed,
pre-fetch the membership by membershipId, verify its orgId equals
ctx.workspace.orgId and throw a TRPCError (BAD_REQUEST or FORBIDDEN) if it does
not, then proceed to call removeMembership.
| message: "Invalid organization ID", | ||
| }); | ||
| } | ||
| return await authProvider.revokeOrgInvitation(input.invitationId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enforce org scoping at the provider boundary.
Passing only invitationId relies on upstream checks; ensure the provider validates the invitation belongs to the same org to prevent cross-org revocations. Ideally pass orgId too or have the provider verify via membership/ownership.
🏁 Script executed:
#!/bin/bash
# Verify provider method enforces org scoping
rg -nP -C3 --type=ts --type=tsx '\brevokeOrgInvitation\s*\('
rg -nP -C3 --type=ts --type=tsx 'function\s+revokeOrgInvitation\b|revokeOrgInvitation:\s*\('Length of output: 213
🏁 Script executed:
#!/bin/bash
set -e
# Search for all occurrences of revokeOrgInvitation across the codebase (excluding node_modules)
rg -n --color never --glob '!node_modules/*' 'revokeOrgInvitation' .
# Search for authProvider definition to inspect its methods
rg -n --color never --glob '!node_modules/*' 'authProvider' .Length of output: 3501
🏁 Script executed:
#!/bin/bash
set -e
# Print base-provider revokeOrgInvitation signature
sed -n '240,260p' apps/dashboard/lib/auth/base-provider.ts
# Print local provider implementation
sed -n '320,360p' apps/dashboard/lib/auth/local.ts
# Print workos provider implementation
sed -n '520,560p' apps/dashboard/lib/auth/workos.tsLength of output: 3024
Enforce org scoping in revokeOrgInvitation: update the revokeOrgInvitation signature in apps/dashboard/lib/auth/base-provider.ts to accept an orgId, pass ctx.tenant.id from the TRPC router, and add validation in both local.ts and workos.ts implementations to ensure the invitation’s organizationId matches before revoking.
| if (input.workspaceId !== ctx.workspace.id) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid workspace ID", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return FORBIDDEN (403) instead of BAD_REQUEST (400) to avoid ID enumeration
This is an auth failure, not an input error. Use a generic message.
- if (input.workspaceId !== ctx.workspace.id) {
- throw new TRPCError({
- code: "BAD_REQUEST",
- message: "Invalid workspace ID",
- });
- }
+ if (input.workspaceId !== ctx.workspace.id) {
+ throw new TRPCError({
+ code: "FORBIDDEN",
+ message: "Forbidden",
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (input.workspaceId !== ctx.workspace.id) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid workspace ID", | |
| }); | |
| } | |
| if (input.workspaceId !== ctx.workspace.id) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "Forbidden", | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/dashboard/lib/trpc/routers/workspace/changeName.ts around lines 20 to
25, the handler currently throws a TRPCError with code "BAD_REQUEST" and a
specific "Invalid workspace ID" message; change this to throw a TRPCError with
code "FORBIDDEN" (HTTP 403) and a generic message such as "Access denied" or
"Forbidden" to reflect an authorization failure and avoid leaking whether the ID
exists.
2 participants
What does this PR do?
During a pen test, we found a couple items where if a user was smart enough to figure out the 1E18 combination for an organization they could potentially update a different org.
This addresses:
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit