🐛 Fixed themes with custom error templates returning HTTP 500

ghost/core/core/frontend/web/middleware/error-handler.js

@@ -1,12 +1,12 @@
 const hbs = require('express-hbs');
-const _ = require('lodash');
 const path = require('path');
 const tpl = require('@tryghost/tpl');
 const errors = require('@tryghost/errors');
 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?!

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'),
+                '<div class="themed-error">{{statusCode}} — {{message}}</div>'
+            );
+        });
+
+        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/);
+        });
+    });
 });