Fix : use ?? [ ] for requestPath match fallback#697
Fix : use ?? [ ] for requestPath match fallback#697nikunjkumar05 wants to merge 1 commit intotekdi:mainfrom
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/middleware/permission.middleware.ts (2)
52-55:⚠️ Potential issue | 🟠 MajorHandle potential error responses from
fetchPermissions.Based on the
getPermissionForMiddlewareimplementation, when a database error occurs, it returns the error object directly (return error). Calling.some()on a non-array will throw a TypeError.🐛 Proposed fix to validate response type
const allowedPermissions = await this.fetchPermissions(roleTitle, apiPath); + if (!Array.isArray(allowedPermissions) || allowedPermissions.length === 0) { + return false; + } return allowedPermissions.some((permission) => permission.requestType.includes(requestMethod) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/permission.middleware.ts` around lines 52 - 55, The call site using fetchPermissions (in permission.middleware.ts where allowedPermissions is assigned) assumes an array but fetchPermissions/getPermissionForMiddleware can return an error object; update the code to validate the response type before using .some — e.g., check Array.isArray(allowedPermissions) and if false, handle the error case by logging the error (or rethrowing) and returning false (denying permission) instead of calling allowedPermissions.some; reference the allowedPermissions variable and the fetchPermissions/getPermissionForMiddleware functions when making the guard.
71-76:⚠️ Potential issue | 🟡 MinorAdd error handling for malformed JWT tokens.
If the authorization header contains a malformed token (missing parts, invalid base64, or invalid JSON), this function will throw an unhandled exception. This causes the middleware to return a generic 500 error instead of a more appropriate 401/403.
🛡️ Proposed fix with error handling
getRole(token: string) { + try { + const parts = token.split("."); + if (parts.length < 2) { + LoggerUtil.warn("Invalid token format"); + return "public"; + } - const payloadBase64 = token.split(".")[1]; // Get the payload part + const payloadBase64 = parts[1]; // Get the payload part const payloadJson = Buffer.from(payloadBase64, "base64").toString("utf-8"); // Decode Base64 const payload = JSON.parse(payloadJson); // Convert to JSON - return payload.user_roles; + return payload.user_roles ?? "public"; + } catch (error) { + LoggerUtil.warn("Failed to parse token: " + error.message); + return "public"; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/permission.middleware.ts` around lines 71 - 76, The getRole(token: string) helper currently assumes a well-formed JWT and will throw on missing parts, invalid Base64, or bad JSON; update getRole to validate the token format (ensure token.split(".").length >= 2), wrap the Base64 decode and JSON.parse in a try/catch, and on error return a clear sentinel (e.g., null or empty array) or throw a specific error (e.g., InvalidTokenError) so the calling middleware can return a 401/403 instead of a 500; reference and update the getRole function in permission.middleware.ts and ensure the middleware that calls getRole checks for the sentinel/error and maps it to the appropriate 401/403 response.
🧹 Nitpick comments (1)
src/middleware/permission.middleware.ts (1)
57-64: Consider explicitly handling paths with fewer than 3 segments.When
parts.length < 3(e.g.,/api/endpoint),apiPathremains an empty string. This implicit behavior could be made explicit for clarity and maintainability.💡 Optional: Add explicit handling for short paths
getApiPaths(parts: string[]) { let apiPath = ""; - if (parts.length == 3) apiPath = `/${parts[0]}/${parts[1]}/*`; - if (parts.length > 3) apiPath = `/${parts[0]}/${parts[1]}/${parts[2]}/*`; + if (parts.length < 3) { + // Paths with fewer than 3 segments are not matched - access denied by default + apiPath = ""; + } else if (parts.length === 3) { + apiPath = `/${parts[0]}/${parts[1]}/*`; + } else { + apiPath = `/${parts[0]}/${parts[1]}/${parts[2]}/*`; + } LoggerUtil.log("apiPath: ", apiPath); return apiPath; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/permission.middleware.ts` around lines 57 - 64, The getApiPaths function can return an empty string when parts.length < 3; explicitly handle short paths by adding a branch for parts.length < 3 that constructs a sensible apiPath (for example set apiPath = `/${parts.join('/')}` or `/${parts.join('/')}/*` depending on intended matching semantics) so the function never returns an empty string; update getApiPaths to include this branch and keep the existing branches for length == 3 and > 3 unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/middleware/permission.middleware.ts`:
- Around line 52-55: The call site using fetchPermissions (in
permission.middleware.ts where allowedPermissions is assigned) assumes an array
but fetchPermissions/getPermissionForMiddleware can return an error object;
update the code to validate the response type before using .some — e.g., check
Array.isArray(allowedPermissions) and if false, handle the error case by logging
the error (or rethrowing) and returning false (denying permission) instead of
calling allowedPermissions.some; reference the allowedPermissions variable and
the fetchPermissions/getPermissionForMiddleware functions when making the guard.
- Around line 71-76: The getRole(token: string) helper currently assumes a
well-formed JWT and will throw on missing parts, invalid Base64, or bad JSON;
update getRole to validate the token format (ensure token.split(".").length >=
2), wrap the Base64 decode and JSON.parse in a try/catch, and on error return a
clear sentinel (e.g., null or empty array) or throw a specific error (e.g.,
InvalidTokenError) so the calling middleware can return a 401/403 instead of a
500; reference and update the getRole function in permission.middleware.ts and
ensure the middleware that calls getRole checks for the sentinel/error and maps
it to the appropriate 401/403 response.
---
Nitpick comments:
In `@src/middleware/permission.middleware.ts`:
- Around line 57-64: The getApiPaths function can return an empty string when
parts.length < 3; explicitly handle short paths by adding a branch for
parts.length < 3 that constructs a sensible apiPath (for example set apiPath =
`/${parts.join('/')}` or `/${parts.join('/')}/*` depending on intended matching
semantics) so the function never returns an empty string; update getApiPaths to
include this branch and keep the existing branches for length == 3 and > 3
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf13a3b8-91fe-4130-b507-deb9b0f52a8b
📒 Files selected for processing (1)
src/middleware/permission.middleware.ts
|
@Sourav-Tekdi @Shubham4026 sir can review my PR |



potential null reference by ensuring parts in requestPath
Summary by CodeRabbit
Bug Fixes