fix(policy): handle unmapped HTTP methods in methodToAction (HEAD, OPTIONS)#3158
fix(policy): handle unmapped HTTP methods in methodToAction (HEAD, OPTIONS)#3158PierreBrisorgueil merged 5 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMapped HEAD and OPTIONS to 'read' in the policy middleware; isAllowed now returns 405 for unmapped HTTP methods. Removed custom Zod string error option from the Task schema and updated docs/tests to reflect Zod v4's default "Invalid input: expected string, received number." message. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3158 +/- ##
=======================================
Coverage 89.85% 89.86%
=======================================
Files 52 52
Lines 1163 1164 +1
Branches 233 234 +1
=======================================
+ Hits 1045 1046 +1
Misses 107 107
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the authorization middleware to correctly handle HTTP methods that were previously unmapped (HEAD, OPTIONS), and adds unit tests to prevent regressions in method-to-action resolution.
Changes:
- Extend
methodToActionto mapHEADandOPTIONSto thereadCASL action. - Add an early-deny guard in
isAllowedfor methods that don’t map to a CASL action. - Add unit tests covering
isAllowedbehavior forHEAD,OPTIONS, and an unknown method (PROPFIND).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/middlewares/policy.js |
Adds HEAD/OPTIONS mappings and rejects unmapped methods to avoid ability.can(undefined, ...). |
modules/core/tests/core.unit.tests.js |
Adds unit coverage for isAllowed with HEAD, OPTIONS, and an unmapped method. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/middlewares/policy.js (1)
35-47: Missing@returnsin JSDoc for async function.Per coding guidelines, async functions must document the resolved value with
@returns. TheisAllowedJSDoc should include the return type.Suggested JSDoc update
/** * `@desc` MiddleWare to check if user is allowed * `@param` {Object} req - Express request object * `@param` {Object} res - Express response object * `@param` {Function} next - Express next middleware function + * `@returns` {Promise<void>} Calls next() or sends 403 response */ const isAllowed = async (req, res, next) => {As per coding guidelines: "Every new or modified function must have a JSDoc header: one-line description,
@paramfor each argument,@returnsfor any non-void return value (always include@returnsfor async functions to document the resolved value)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/middlewares/policy.js` around lines 35 - 47, The JSDoc for the async middleware is missing a `@returns` entry: update the JSDoc above the isAllowed function to include a `@returns` describing the resolved value (e.g., Promise<void> or Promise<express.Response|void> as appropriate) so callers/readers know what the async function resolves to; keep the existing `@param` tags for req, res, next and reference the isAllowed middleware and its behavior (it awaits defineAbilityFor, uses methodToAction and responses.error) in the brief description.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ERRORS.mdlib/middlewares/policy.jsmodules/core/tests/core.unit.tests.jsmodules/tasks/models/tasks.schema.jsmodules/tasks/tests/tasks.integration.tests.js
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/middlewares/policy.js`:
- Around line 35-47: The JSDoc for the async middleware is missing a `@returns`
entry: update the JSDoc above the isAllowed function to include a `@returns`
describing the resolved value (e.g., Promise<void> or
Promise<express.Response|void> as appropriate) so callers/readers know what the
async function resolves to; keep the existing `@param` tags for req, res, next and
reference the isAllowed middleware and its behavior (it awaits defineAbilityFor,
uses methodToAction and responses.error) in the brief description.
Add explicit `head: 'read'` and `options: 'read'` mappings so HEAD and OPTIONS requests are routed to the correct CASL action instead of resolving to `undefined`. Add a guard in `isAllowed` that returns 403 for any unmapped method to prevent incorrect authorization decisions. Closes #3137
In Zod v4, passing `error: 'string'` to `z.string()` does not override the `invalid_type` issue message — the actual output is 'Expected string, received number'. Remove the no-op `error` option from the Task schema and update the integration test expectation to match. Add a note to ERRORS.md to prevent the same mistake in future migrations.
- Return 405 Method Not Allowed (instead of 403) for unmapped HTTP methods with a descriptive message to distinguish from permission denials - Move the action lookup and early-return guard before defineAbilityFor to avoid unnecessary CASL import and rules evaluation for unsupported methods - Update unit test expectation from 403 to 405 accordingly
PierreBrisorgueil
force-pushed
the
worktree-httpmethodsfix
branch
from
9da99f2 to
32523c1
Compare
Local node_modules had Zod v3 (3.25.76) masking the real v4 behavior. Zod v4 (^4.3.6 from package.json) produces 'Invalid input: expected string, received number' — not the v3 message 'Expected string, received number'. Update the test expectation and the ERRORS.md entry to match the actual Zod v4 output.
Following Copilot review: add assertions on message and description fields in the 405 response to catch regressions if the error message format changes.
Summary
Closes #3137
Test plan
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests