fix: keys setting vulns #2718
fix: keys setting vulns #2718
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Line range hint
52-56: Improve audit log description accuracyBased on previous feedback, the audit log description should focus on the actual change being made rather than including the workspaceId in the description.
- description: `Changed ${keyAuth.workspaceId} default prefix from ${keyAuth.defaultPrefix} to ${input.defaultPrefix}`, + description: `Updated API default prefix from ${keyAuth.defaultPrefix} to ${input.defaultPrefix}`,apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts (1)
Line range hint
57-61: Maintain consistency in audit log descriptionsFor consistency with setDefaultPrefix changes, the audit log description should be simplified.
- description: `Changed ${keyAuth.workspaceId} default byte size for keys from ${keyAuth.defaultBytes} to ${input.defaultBytes}`, + description: `Updated API default byte size from ${keyAuth.defaultBytes} to ${input.defaultBytes}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx(0 hunks)apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx(1 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts(1 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx
🧰 Additional context used
📓 Learnings (2)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts:80-80
Timestamp: 2024-11-10T16:45:16.994Z
Learning: Ensure that audit log descriptions accurately reflect the action being performed, such as updating the `defaultPrefix`, and avoid incorrect references like 'name' when not applicable.
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (1)
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx:74-75
Timestamp: 2024-11-10T16:45:16.994Z
Learning: In `apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx`, the hidden `<input>` elements for `workspaceId` and `keyAuthId` work correctly without being registered with React Hook Form.
🔇 Additional comments (4)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Line range hint 8-26: LGTM! Security improvement by removing workspaceId validation
The removal of workspaceId from input validation is a good security enhancement that prevents potential workspace-hopping attacks while maintaining proper authorization through keyAuth lookup.
apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts (1)
Line range hint 8-31: LGTM! Consistent security improvement
The changes align with setDefaultPrefix.ts, removing workspaceId from input validation while maintaining proper authorization and input validation for byte size.
apps/dashboard/app/(app)/apis/[apiId]/settings/default-prefix.tsx (2)
21-24: LGTM! Frontend changes align with security improvements
The removal of workspaceId from the form schema and Props type properly complements the backend security enhancements while maintaining all necessary functionality.
Also applies to: 26-31
Line range hint 1-1: Verify complete removal of workspaceId references
Let's ensure all workspaceId references have been properly removed from the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Line range hint
64-83: Improve audit log description clarity.While the audit logging is generally well-implemented, the description could be more explicit about what was changed.
Consider this minor improvement:
- description: `Changed ${keyAuth.id} default prefix from ${keyAuth.defaultPrefix} to ${input.defaultPrefix}`, + description: `Updated API key default prefix for ${keyAuth.name || keyAuth.id} from "${keyAuth.defaultPrefix}" to "${input.defaultPrefix}"`,This makes it clearer what was changed and uses quotes to make the prefix values more distinguishable, especially if they contain spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts(4 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts:80-80
Timestamp: 2024-11-10T16:45:16.994Z
Learning: Ensure that audit log descriptions accurately reflect the action being performed, such as updating the `defaultPrefix`, and avoid incorrect references like 'name' when not applicable.
🔇 Additional comments (2)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (2)
33-47: LGTM! Comprehensive error handling.
The error handling is thorough with appropriate error codes and user-friendly messages. The separation between workspace and keyAuth not found cases provides clear feedback.
Line range hint 48-64: LGTM! Well-structured transaction with proper error handling.
The transaction properly ensures atomicity between the update and audit logging. The use of the verified keyAuth.id from the workspace query maintains security.
| const workspace = await db.query.workspaces | ||
| .findFirst({ | ||
| where: (table, { eq }) => eq(table.id, input.keyAuthId), | ||
| where: (table, { eq }) => eq(table.tenantId, ctx.tenant.id), | ||
| with: { | ||
| keySpaces: { | ||
| where: (table, { eq }) => eq(table.id, input.keyAuthId), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Several endpoints still accept workspaceId directly in their input schemas, which is inconsistent with the security improvement implemented in setDefaultPrefix.ts. These endpoints should be updated to use tenantId from context instead:
apps/dashboard/lib/trpc/routers/workspace/changePlan.tsapps/dashboard/lib/trpc/routers/workspace/changeName.tsapps/dashboard/lib/trpc/routers/key/updateRatelimit.tsapps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.tsapps/dashboard/lib/trpc/routers/api/updateName.tsapps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts
🔗 Analysis chain
LGTM! Secure workspace retrieval implementation.
The change from using workspaceId from input to tenantId from context is a good security improvement. The query is well-structured with proper type safety.
Let's verify that no direct workspaceId usage remains in similar endpoints:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct workspaceId usage in API router input schemas
ast-grep --pattern 'input(
z.object({
$$$
workspaceId: z.string(),
$$$
})
)'
Length of output: 94
Script:
#!/bin/bash
# Let's try a broader search for workspaceId in router input schemas
rg -l "workspaceId.*z\.string" apps/dashboard/lib/trpc/routers/
# Also search for direct workspaceId usage in query conditions
rg -l "workspaceId.*input\." apps/dashboard/lib/trpc/routers/
# Search for any remaining direct workspaceId references in API routers
ast-grep --pattern 'workspaceId: z.string()'
Length of output: 515
Script:
#!/bin/bash
# Let's examine the content of these files to see how workspaceId is used
for file in apps/dashboard/lib/trpc/routers/workspace/changePlan.ts \
apps/dashboard/lib/trpc/routers/workspace/changeName.ts \
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts \
apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts \
apps/dashboard/lib/trpc/routers/api/updateName.ts \
apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts; do
echo "=== $file ==="
rg "workspaceId.*z\.string|where.*workspaceId" -A 5 "$file"
done
Length of output: 5709
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts (3)
23-33: Improve error message specificityThe error message is generic and doesn't provide context about what operation failed. Consider making it more specific to help with debugging.
- "We were unable to update the key auth. Please try again or contact support@unkey.dev", + "Failed to fetch key authentication details. Please try again or contact support@unkey.dev",
Line range hint
43-58: Consider adding defaultBytes null handlingWhile the input schema makes defaultBytes optional, the update logic might benefit from explicit null handling to clarify the intended behavior when defaultBytes is undefined.
.set({ - defaultBytes: input.defaultBytes, + defaultBytes: input.defaultBytes ?? null, // Explicit null handling })
Line range hint
59-76: Enhance audit log clarityThe audit log description could be more precise by handling undefined values and using a more structured format.
- description: `Changed ${keyAuth.id} default byte size for keys from ${keyAuth.defaultBytes} to ${input.defaultBytes}`, + description: `Updated key auth ${keyAuth.id} default byte size from ${keyAuth.defaultBytes ?? 'unset'} to ${input.defaultBytes ?? 'unset'}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts
🔇 Additional comments (3)
apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts (3)
Line range hint 8-15: Security improvement: Removed workspaceId from input schema
Good job removing the workspaceId from the input schema. This prevents potential security vulnerabilities by ensuring workspace identification comes from the authenticated context rather than user input.
36-42: LGTM: Proper tenant isolation implemented
Good implementation of tenant isolation by checking keyAuth.workspace.tenantId against ctx.tenant.id. This ensures that users can only modify resources within their tenant.
Line range hint 1-85: Verify complete removal of workspaceId from RPC calls
Let's ensure there are no remaining instances of workspaceId being passed through RPC calls across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
No security concerns with workspaceId usage in RPC calls
The codebase shows that workspaceId is consistently used in a secure manner:
- It's properly validated through Zod schemas in input validation
- Access control is enforced by checking tenant/workspace ownership
- It's primarily used for audit logging and internal database operations
- No instances found where workspaceId is exposed in unsafe ways through RPC calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential workspaceId usage in RPC calls
echo "Checking for workspaceId in tRPC procedures..."
ast-grep --pattern 'procedure.input(z.object({ $$$workspaceId: z.string()$$$}))'
echo "Checking for workspaceId in API route handlers..."
rg -l 'workspaceId.*req\.body' 'apps/'
Length of output: 333
Script:
#!/bin/bash
# Let's try a broader search for workspaceId in procedure inputs
echo "Checking for workspaceId in tRPC procedures with a more flexible pattern..."
ast-grep --pattern 'procedure$$$input($$$workspaceId$$$)'
echo "Checking for direct workspaceId references in router files..."
rg -A 3 'workspaceId' 'apps/dashboard/lib/trpc/routers/'
echo "Checking for workspaceId in schema validations..."
rg -A 3 'workspaceId.*z\.' 'apps/dashboard/'
Length of output: 58935
What does this PR do?
Fixes vulnerabilities in key settings, prevents the exposure of workspaceId to RPC.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
DefaultBytesandDefaultPrefixcomponents have been updated to remove theworkspaceIdfield, simplifying the form structure.Bug Fixes
setDefaultBytesandsetDefaultPrefixprocedures by streamlining the input validation process.Refactor
workspaceId, improving overall functionality.