Skip to content

Fix : use ?? [ ] for requestPath match fallback#697

Open
nikunjkumar05 wants to merge 1 commit intotekdi:mainfrom
nikunjkumar05:cleanup
Open

Fix : use ?? [ ] for requestPath match fallback#697
nikunjkumar05 wants to merge 1 commit intotekdi:mainfrom
nikunjkumar05:cleanup

Conversation

@nikunjkumar05
Copy link
Copy Markdown

@nikunjkumar05 nikunjkumar05 commented Mar 19, 2026

potential null reference by ensuring parts in requestPath

Summary by CodeRabbit

Bug Fixes

  • Improved reliability of permission validation by enhancing handling of edge cases during request path processing to prevent potential failures.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 19, 2026

Walkthrough

The checkPermissions() function now safely handles cases where the regex pattern match operation returns null by using the nullish coalescing operator to default parts to an empty array, ensuring downstream array operations do not fail.

Changes

Cohort / File(s) Summary
Null Safety Fix
src/middleware/permission.middleware.ts
Added null coalescing operator (?? []) to handle null regex match results, ensuring parts is always an array before being passed to getApiPaths().

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a nullish coalescing operator with empty array fallback to handle null returns from regex matching.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can enable review details to help with troubleshooting, context usage and more.

Enable the reviews.review_details setting to include review details such as the model used, the time taken for each step and more in the review comments.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🟠 Major

Handle potential error responses from fetchPermissions.

Based on the getPermissionForMiddleware implementation, 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 | 🟡 Minor

Add 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), apiPath remains 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

📥 Commits

Reviewing files that changed from the base of the PR and between 36a6488 and 41f0d82.

📒 Files selected for processing (1)
  • src/middleware/permission.middleware.ts

@nikunjkumar05
Copy link
Copy Markdown
Author

@Sourav-Tekdi @Shubham4026 sir can review my PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant