diff --git a/ghost/core/core/frontend/web/middleware/error-handler.js b/ghost/core/core/frontend/web/middleware/error-handler.js index 1022619ee6f..b5158683439 100644 --- a/ghost/core/core/frontend/web/middleware/error-handler.js +++ b/ghost/core/core/frontend/web/middleware/error-handler.js @@ -1,5 +1,4 @@ const hbs = require('express-hbs'); -const _ = require('lodash'); const path = require('path'); const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); @@ -7,6 +6,7 @@ const sentry = require('../../../shared/sentry'); const config = require('../../../shared/config'); const renderer = require('../../services/rendering'); +const themeEngine = require('../../services/theme-engine'); // @TODO: make this properly shared code const {prepareError, prepareErrorCacheControl, prepareStack} = require('@tryghost/mw-error-handler'); @@ -93,14 +93,29 @@ const themeErrorRenderer = function themeErrorRenderer(err, req, res, next) { // @TODO: very dirty !!!!!! renderer.templates.setTemplate(req, res); - // It can be that something went wrong with the theme or otherwise loading handlebars - // This ensures that no matter what res.render will work here + // If the active theme isn't mounted into Express we cannot render a theme error template. + // This happens when the error was thrown before the theme middleware ran (e.g. early in + // the request pipeline, or on the first requests after a boot): the theme's engine and + // views directory aren't set up, and neither is the per-request/global template data + // (@site, @custom, member, ...) that theme error pages such as error-404.hbs depend on. + // Fall back to Ghost's self-contained built-in error template instead. + // + // The built-in template is referenced by absolute path rather than by repointing the + // app-wide `views` directory, because mutating `views` here persists on the Express app + // and would break theme template lookups (e.g. error-404) on subsequent requests. // @TODO: split the error handler for assets, admin & theme to refactor this away - if (_.isEmpty(req.app.engines)) { - res._template = 'error'; - req.app.engine('hbs', createHbsEngine()); - req.app.set('view engine', 'hbs'); - req.app.set('views', config.get('paths').defaultViews); + const activeTheme = themeEngine.getActive(); + if (!activeTheme || !activeTheme.mounted) { + res._template = path.resolve(config.get('paths').defaultViews, 'error.hbs'); + + // Register the bare-minimum hbs engine if it isn't already available. We check for the + // `hbs` engine specifically rather than testing whether req.app.engines is empty, so the + // fallback still works if some other engine happens to be registered. Express keys + // engines by file extension with a leading dot, hence `.hbs`. + if (!req.app.engines || !req.app.engines['.hbs']) { + req.app.engine('hbs', createHbsEngine()); + req.app.set('view engine', 'hbs'); + } } // @TODO use renderer here?! diff --git a/ghost/core/test/unit/frontend/web/middleware/error-handler.test.js b/ghost/core/test/unit/frontend/web/middleware/error-handler.test.js index 234cdb65dc3..eb47736c595 100644 --- a/ghost/core/test/unit/frontend/web/middleware/error-handler.test.js +++ b/ghost/core/test/unit/frontend/web/middleware/error-handler.test.js @@ -1,9 +1,14 @@ const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); const express = require('express'); const request = require('supertest'); +const sinon = require('sinon'); const errors = require('@tryghost/errors'); const {handleThemeResponse} = require('../../../../../core/frontend/web/middleware/error-handler'); +const themeEngine = require('../../../../../core/frontend/services/theme-engine'); describe('Frontend Error Handler', function () { // The themeErrorRenderer is the last middleware in the handleThemeResponse array @@ -209,4 +214,140 @@ describe('Frontend Error Handler', function () { assert(response.text.includes('Template rendering failed')); }); }); + + // Regression tests for: an error thrown before the theme middleware mounts the theme + // (e.g. early in the request pipeline / first requests after boot) reaching the error + // renderer with an unmounted theme. Previously this resolved a theme-specific error + // template name (e.g. `error-404`) against Ghost's *default* views directory, producing + // `Failed to lookup view "error-404"` -> HTTP 500 (and permanently repointing the app's + // views dir). The renderer now renders Ghost's self-contained built-in error template + // whenever the theme isn't mounted (its engine/views and template data aren't set up), + // and only renders theme error templates when the theme is genuinely mounted. + describe('built-in error fallback when the active theme is not mounted', function () { + let themeDir; + + beforeEach(function () { + // a throwaway "theme" dir with a real, self-contained error-404 template + themeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ghost-theme-err-')); + fs.writeFileSync( + path.join(themeDir, 'error-404.hbs'), + '
{{statusCode}} — {{message}}
' + ); + }); + + afterEach(function () { + sinon.restore(); + fs.rmSync(themeDir, {recursive: true, force: true}); + }); + + const errorTemplates = ['error-404', 'error-4xx', 'error']; + + // One app instance whose theme is NOT mounted (no hbs engine / views configured), + // funnelling a fresh 404 into the theme error renderer on each request — mirrors the + // post-boot, pre-theme-middleware state. + function createUnmountedSiteApp() { + const app = express(); + app.use((req, res, next) => { + const err = {statusCode: 404, message: 'Page not found'}; + res.statusCode = 404; + req.err = err; + next(err); + }); + app.use(themeErrorRenderer); + return app; + } + + it('renders the built-in error template (not the theme template) when the theme is not mounted', async function () { + // theme HAS error-404 but is NOT mounted (error thrown before theme middleware ran) + sinon.stub(themeEngine, 'getActive').returns({ + mounted: false, + hasTemplate: name => errorTemplates.includes(name) + }); + + const res = await request(createUnmountedSiteApp()) + .get('/missing-page') + .expect(404); + + assert.doesNotMatch(res.text, /Failed to lookup view/); + assert.doesNotMatch(res.text, /themed-error/); // not the theme template + assert.match(res.text, /error-code/); // built-in template markup + }); + + it('does not 500 or repoint the global views dir across repeated early errors (regression)', async function () { + sinon.stub(themeEngine, 'getActive').returns({ + mounted: false, + hasTemplate: name => errorTemplates.includes(name) + }); + + const app = createUnmountedSiteApp(); + + // First request previously armed the fallback (registering an engine + repointing + // views), which then made the second request resolve `error-404` against the wrong + // directory: `Failed to lookup view "error-404"` -> HTTP 500. + await request(app).get('/missing-one').expect(404); + const res = await request(app).get('/missing-two').expect(404); + + assert.doesNotMatch(res.text, /Failed to lookup view/); + // the fallback must not repoint the app-wide views dir to Ghost's default views + assert.doesNotMatch(app.get('views') || '', /core[/\\]server[/\\]views/); + }); + + it('falls back to the built-in error template without repointing the global views dir when there is no active theme', async function () { + sinon.stub(themeEngine, 'getActive').returns(undefined); + + const app = createUnmountedSiteApp(); + const res = await request(app).get('/missing-page').expect(404); + + assert.doesNotMatch(res.text, /Failed to lookup view/); + assert.doesNotMatch(app.get('views') || '', /core[/\\]server[/\\]views/); + }); + + it('registers the hbs engine for the built-in fallback even if another engine is already registered', async function () { + sinon.stub(themeEngine, 'getActive').returns(undefined); + + const app = express(); + // a non-hbs engine is registered, but hbs is not — the fallback must still register + // hbs (checking req.app.engines emptiness instead would skip it and 500) + app.engine('ejs', (filePath, options, cb) => cb(null, 'ejs output')); + app.use((req, res, next) => { + const err = {statusCode: 404, message: 'Page not found'}; + res.statusCode = 404; + req.err = err; + next(err); + }); + app.use(themeErrorRenderer); + + const res = await request(app).get('/missing-page').expect(404); + + assert.doesNotMatch(res.text, /Failed to lookup view/); + assert.match(res.text, /error-code/); // built-in template rendered via the hbs engine + }); + + it('still renders the theme error template when the theme IS mounted', async function () { + sinon.stub(themeEngine, 'getActive').returns({ + mounted: true, + hasTemplate: name => errorTemplates.includes(name) + }); + + // mimic a mounted theme: real hbs engine + views pointed at the theme dir + const expressHbs = require('express-hbs'); + const app = express(); + app.engine('hbs', expressHbs.create().express4()); + app.set('view engine', 'hbs'); + app.set('views', themeDir); + app.use((req, res, next) => { + const err = {statusCode: 404, message: 'Page not found'}; + res.statusCode = 404; + req.err = err; + next(err); + }); + app.use(themeErrorRenderer); + + const res = await request(app).get('/missing-page').expect(404); + + assert.match(res.text, /themed-error/); + assert.match(res.text, /404 — Page not found/); + assert.doesNotMatch(res.text, /Failed to lookup view/); + }); + }); });