Updated error handler middleware to return plain text errors for static files #24666
Conversation
WalkthroughThe middleware error handler now imports path and @tryghost/errors, adds messages for notFound (“File not found”) and unexpectedError, and distinguishes static file requests using file extensions or STATIC_FILE_NOT_FOUND. For static files, it maps common HTTP status codes to Ghost error classes and returns plain text responses (e.g., 404 -> “File not found”), including overriding messages if needed. Non-static requests continue with HTML rendering. E2E tests are updated to assert text/plain 404s and add cases for missing assets. A new unit test suite exercises static vs HTML paths, status mapping, and render failure behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
| // Send a simple plain text error response | ||
| res.status(statusCode); | ||
| res.type('text/plain'); | ||
| return res.send(message); |
There was a problem hiding this comment.
We could alternatively call return next(err) here to fallback to express's default error handler, but this still returns HTML (although un-styled).
Note: this also changes the behavior for missing image files - see test/e2e-frontend/static-files.test.js below
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ghost/core/test/e2e-frontend/static-files.test.js (1)
27-34: Consider adding a HEAD request case for static filesHEAD requests for missing static assets should return the same headers without a body. A quick e2e case will guard against regressions.
Apply this diff to add the HEAD test:
@@ it('returns plain text 404 for non-existing asset files with extensions', async function () { const response = await frontendAgent .get('/assets/css/missing.css') .expect(404) .expect('Content-Type', 'text/plain; charset=utf-8'); assert.equal(response.text, 'File not found'); }); + + it('returns correct headers for HEAD on non-existing asset files', async function () { + await frontendAgent + .head('/assets/css/missing.css') + .expect(404) + .expect('Content-Type', 'text/plain; charset=utf-8'); + // Superagent doesn't expose body for HEAD; header checks are sufficient + });ghost/core/test/unit/frontend/web/middleware/error-handler.test.js (2)
61-72: Also assert next() is not called on static-file pathsYou assert next is not called for the HTML path; mirroring that for static paths increases confidence that plain-text handling short-circuits the chain.
Apply this diff in a couple of static-path tests, e.g. this one:
await themeErrorRenderer(err, req, res, next); assertPlainTextResponse(res, 404, 'File not found'); + next.called.should.be.false();
121-133: Optional: Add a unit test for unknown non-Ghost errors without statusCode (post-mapping change)This guards against leaking raw error messages when statusCode is absent.
Here’s a test you can add after strengthening the mapping:
it('should sanitize unknown non-Ghost errors without statusCode for static files', async function () { const err = new Error('Boom: internal stack detail'); req.path = '/assets/app.css'; await themeErrorRenderer(err, req, res, next); assertPlainTextResponse(res, 500, 'The server has encountered an error.'); next.called.should.be.false(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/frontend/web/middleware/error-handler.js(3 hunks)ghost/core/test/e2e-frontend/static-files.test.js(1 hunks)ghost/core/test/unit/frontend/web/middleware/error-handler.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
Learnt from: kevinansfield
PR: TryGhost/Ghost#24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
🧬 Code Graph Analysis (1)
ghost/core/core/frontend/web/middleware/error-handler.js (1)
ghost/core/test/unit/frontend/web/middleware/error-handler.test.js (19)
require(5-5)errors(3-3)themeErrorRenderer(59-59)err(62-66)err(75-78)err(87-89)err(98-101)err(110-113)err(122-124)err(136-140)err(149-152)err(161-164)err(173-176)err(185-188)err(203-206)err(221-224)req(8-8)res(9-9)next(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser tests
🔇 Additional comments (9)
ghost/core/test/e2e-frontend/static-files.test.js (3)
18-25: E2E assertion alignment looks correct for resized/original imagesValidates status and Content-Type and asserts the exact plain-text body. Matches middleware behavior and prevents recursion.
27-34: Good coverage for asset files under /assets/This covers a key path where themes commonly reference css/js. The expectations are precise and resilient.
36-43: Covers arbitrary extension paths under site rootEnsures extension-based detection works beyond canonical asset directories. Looks good.
ghost/core/core/frontend/web/middleware/error-handler.js (3)
41-49: Early plain-text handling for static files is a solid approachUsing path.extname(req.path) and STATIC_FILE_NOT_FOUND to short-circuit HTML rendering prevents recursion and boot-time failures. The rationale in comments is clear.
68-73: Correctly overrides message for static 404s already converted upstreamEnsures consistent messaging (“File not found”) and code tagging for static-file 404s, regardless of conversion source.
78-81: Plain-text response setup is minimal and correctSets status and content-type explicitly before sending the sanitized message. Matches e2e/unit expectations for Content-Type and avoids HTML rendering paths.
ghost/core/test/unit/frontend/web/middleware/error-handler.test.js (3)
41-56: Nice helper abstractions reduce assertion noiseThe plain text and HTML helpers keep tests expressive and focused on behavior rather than call plumbing.
97-107: Covers generic 500 for static files via InternalServerError mappingThis ensures a safe message. Once you harden unknown non-Ghost errors (no statusCode) to also map to InternalServerError, consider adding a unit test to lock that behavior.
I can contribute a focused test to cover this path after the middleware change. Would you like me to push a snippet?
220-241: Render failure fallback behavior is well assertedGood coverage for the error-template failure path with the generic “Oops…” HTML. This is important for resilience.
|
…ic files (#24666) ref https://linear.app/ghost/issue/ONC-1095/investigate-failed-to-lookup-view-error-404-in-views-directory When serving 404 errors for missing asset files, we should serve plain text error pages instead of full HTML. This prevents recursion issues if the full HTML 404 page references the missing asset, and avoids rendering errors if the theme engine hasn't been initialized yet (i.e. immediately after boot). This identifies static file errors in the frontend's error handler middleware based on whether the request has a file extension or if the error already has the `STATIC_FILE_NOT_FOUND` error code attached to it, and returns a plain text error before reaching the template rendering logic.
ref https://linear.app/ghost/issue/ONC-1095/investigate-failed-to-lookup-view-error-404-in-views-directory
When serving 404 errors for missing asset files, we should serve plain text error pages instead of full HTML. This prevents recursion issues if the full HTML 404 page references the missing asset, and avoids rendering errors if the theme engine hasn't been initialized yet (i.e. immediately after boot).
This identifies static file errors in the frontend's error handler middleware based on whether the request has a file extension or if the error already has the
STATIC_FILE_NOT_FOUNDerror code attached to it, and returns a plain text error before reaching the template rendering logic.