fix(express): catch-all 404 covers every HTTP verb, not just GET - #4010
Conversation
app.get('/{*path}') left unmatched POST/PUT/PATCH/DELETE falling
through to Express's default HTML finalhandler, contradicting the
content-negotiated JSON 404 introduced for GET in #3975. Register the
catch-all with app.all instead so every unmatched verb gets the same
negotiated 404.
Adds verb coverage (POST/PUT/PATCH/DELETE on API and non-API paths)
and two OPTIONS tests proving, by execution rather than assumption,
that a real preflight is still answered by cors and never reaches the
catch-all. Also adds the MIGRATIONS.md entry the original #3975 change
was missing.
Closes #3978
Merge getApp/getAppWithCors into one getApp({ withCors }) builder and
fold the OPTIONS-without-cors case into the existing verb matrix
instead of a near-duplicate standalone test. No behavior change —
same 19 assertions, less repetition.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe unmatched Express route now uses ChangesUnmatched route 404 handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4010 +/- ##
=======================================
Coverage 93.52% 93.52%
=======================================
Files 170 170
Lines 5744 5744
Branches 1843 1843
=======================================
Hits 5372 5372
Misses 302 302
Partials 70 70
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/services/express.js`:
- Around line 291-297: Add a JSDoc header immediately before the catch-all
app.all handler, describing its purpose, documenting the req and res parameters,
and specifying the response return value. Update the handler to return the HTML
response as well as the existing JSON response so its implementation matches the
documented return behavior.
In `@lib/services/tests/express.notfound.unit.tests.js`:
- Around line 21-27: Update the JSDoc headers for both helper functions,
including mockCommonDeps and the helper around the second referenced block, to
use a single concise description line. Move the existing detailed setup
narrative into regular comments above or within each function while preserving
the JSDoc requirement.
In `@MIGRATIONS.md`:
- Around line 9-11: Update the migration note to state that only unmatched GET
requests previously returned the implicit 200, while unmatched POST, PUT, PATCH,
and DELETE requests already returned Express’s 404. Limit the
readiness/health-check guidance to undeclared GET paths, replacing “any
unmatched path” and “regardless of HTTP verb” accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69bcdf8d-0b97-4618-8ee5-ebccd63e8aa8
📒 Files selected for processing (3)
MIGRATIONS.mdlib/services/express.jslib/services/tests/express.notfound.unit.tests.js
- Add the required JSDoc header to the catch-all 404 handler (documents req/res/return, and returns the HTML response too so both branches match the documented return type). - Trim mockCommonDeps/getApp JSDoc to one-line descriptions per the repo's JSDoc convention, moving narrative detail to plain comments. - Correct MIGRATIONS.md: the implicit-200-to-404 status flip only ever applied to unmatched GET (#3975); unmatched POST/PUT/PATCH/ DELETE already 404'd via Express's default handler before this PR — what changes here is the response shape (now content-negotiated), not the status code, for those verbs.
1 participant
Problem
The content-negotiated 404 catch-all introduced for unmatched routes was registered with
app.get('/{*path}', ...)only.initErrorRoutesregisters a 4-arity error handler, which only fires onnext(err), never on an unmatched route. So unmatched POST / PUT / PATCH / DELETE requests fell through to Express's default finalhandler and returned HTML (Cannot POST /api/...) instead of the intended JSON 404 — contradicting the whole point of the change (API consumers get a proper JSON error instead of a false-positive success/wrong-shaped error).Fix
lib/services/express.js— register the catch-all withapp.allinstead ofapp.get, so every unmatched verb gets the same content-negotiated 404. The explicitapp.get('/')root route is registered before it and is unaffected.app.allalso matchesOPTIONS. That's safe: thecorsmiddleware is mounted earlier in the middleware chain (initMiddleware, beforeinitModulesServerRoutes) and always answers a preflight request itself without callingnext(), so a real preflight never reaches the catch-all. Verified by two tests, not assumed (see below).MIGRATIONS.md— the entry the original catch-all change never got: unmatched paths no longer return an implicit 200, and any readiness/health check must target a real declared route (e.g.GET /api/health).Tests
lib/services/tests/express.notfound.unit.tests.js:POST/PUT/PATCH/DELETE/OPTIONS(no cors mounted) on both an API path and a non-API path — the 8 pre-existing tests were all GET, so this closes the verb-coverage gap directly.corsmiddleware (not a mock) ahead of the catch-all, in the same order and config production uses, and sends a real preflight (Origin+Access-Control-Request-Method) — asserting cors answers it (200,Access-Control-Allow-Methodsset) rather than the catch-all's JSON 404 body.Full unit suite: 167 suites / 2300 tests passing. Lint clean.
Closes #3978
Summary by CodeRabbit
Bug Fixes
OPTIONShandling.Documentation