Merged
Conversation
Fixes issue Pi-Defi-world#24 - Replace unsafe 'as any' type cast with proper type inference. The permissions parameter is already typed as string[], and Prisma will handle the JSON serialization automatically without the need for type bypassing.
|
@DavisVT Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (5)
📝 WalkthroughWalkthroughReplaces unsafe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/middleware/auth.ts (2)
159-176: Consider validating input permissions againstPermissionScope.While removing the
as anycast is correct, there's no validation that incomingpermissionsstrings are validPermissionScopevalues. Invalid scope strings (e.g., typos like"p2p:raed") will be stored but will never match inrequireSegmentScope, causing silent permission failures.🛡️ Optional: Add input validation
+const VALID_SCOPES: readonly string[] = [ + "p2p:read", "p2p:write", "p2p:admin", + "sme:read", "sme:write", "sme:admin", + "gateway:read", "gateway:write", "gateway:admin", + "enterprise:read", "enterprise:write", "enterprise:admin", +] as const; + export async function generateApiKey( userId?: string, permissions: string[] = [], ): Promise<string> { + const invalidScopes = permissions.filter((p) => !VALID_SCOPES.includes(p)); + if (invalidScopes.length > 0) { + throw new AppError(`Invalid permission scopes: ${invalidScopes.join(", ")}`, 400); + } + const crypto = await import("crypto");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/auth.ts` around lines 159 - 176, The generateApiKey function currently accepts a permissions: string[] and persists them directly (via prisma.apiKey.create) without ensuring they are valid PermissionScope values; update generateApiKey to validate each entry in permissions against the PermissionScope enum/union (e.g., by checking membership in Object.values(PermissionScope) or a permissionSet) and either filter out invalid entries or throw a descriptive error, then persist only validated scopes so downstream checks like requireSegmentScope will behave correctly; reference generateApiKey, the permissions parameter, PermissionScope, and prisma.apiKey.create when making the change.
49-61: Consider logging when permissions data is malformed.If the database contains malformed permissions (non-array or mixed types), this function silently returns
[], which will causerequireSegmentScopeto deny access with a "Missing segment scope" error. Without logging, debugging such failures would be difficult.🔧 Proposed fix to add debug logging
function validatePermissions(permissions: unknown): string[] { if (!permissions) { return []; } if (Array.isArray(permissions)) { - return permissions.every((p) => typeof p === "string") - ? (permissions as string[]) - : []; + if (permissions.every((p) => typeof p === "string")) { + return permissions as string[]; + } + logger.warn("API key permissions array contains non-string elements", { + types: permissions.map((p) => typeof p), + }); + return []; } + logger.warn("API key permissions field is not an array", { + type: typeof permissions, + }); return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/auth.ts` around lines 49 - 61, The validatePermissions function currently swallows malformed permission values and returns an empty array; update validatePermissions to log a warning/debug message whenever the input is not an all-string array (e.g., non-array or array with non-string members) including the raw permissions value and its type so it's visible when requireSegmentScope denies access; locate validatePermissions in src/middleware/auth.ts and add a concise logger call (use the module's existing logger or console.warn/debug if none) that reports the malformed permissions before returning [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/middleware/auth.ts`:
- Around line 159-176: The generateApiKey function currently accepts a
permissions: string[] and persists them directly (via prisma.apiKey.create)
without ensuring they are valid PermissionScope values; update generateApiKey to
validate each entry in permissions against the PermissionScope enum/union (e.g.,
by checking membership in Object.values(PermissionScope) or a permissionSet) and
either filter out invalid entries or throw a descriptive error, then persist
only validated scopes so downstream checks like requireSegmentScope will behave
correctly; reference generateApiKey, the permissions parameter, PermissionScope,
and prisma.apiKey.create when making the change.
- Around line 49-61: The validatePermissions function currently swallows
malformed permission values and returns an empty array; update
validatePermissions to log a warning/debug message whenever the input is not an
all-string array (e.g., non-array or array with non-string members) including
the raw permissions value and its type so it's visible when requireSegmentScope
denies access; locate validatePermissions in src/middleware/auth.ts and add a
concise logger call (use the module's existing logger or console.warn/debug if
none) that reports the malformed permissions before returning [].
Member
|
@DavisVT cl failed |
1 similar comment
Member
|
@DavisVT cl failed |
Contributor
Author
|
done |
Member
|
@DavisVT fix please |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #24
Summary by CodeRabbit
Bug Fixes
Refactor