Skip to content

Commit 336f1e7

Browse files
ErisDSErisDS
committed
🐛 Fixed rendering of 404s for theme assets
ref https://linear.app/ghost/issue/PROD-2214/return-404-from-frontends-assets-folder ref https://linear.app/ghost/issue/PROD-2322/improve-theme-static-asset-handling - Change theme assets to render an immediate 404 error if they're not found - This means we don't pass them through all of our other middleware like redirecting to a trailing slash - And ultimately means they'll render a plain error and not an HTML one - This will make so much more sense to users when a file doesn't exist - NOTE: we have an exception for robots.txt and sitemap*.xml files as we make use of fallthrough to allow the theme to attempt to serve an override and if not, fall back to our default
1 parent 984bca4 commit 336f1e7

File tree

2 files changed

+144
-1
lines changed

2 files changed

+144
-1
lines changed

‎ghost/core/core/frontend/web/middleware/static-theme.js

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,25 @@ function forwardToExpressStatic(req, res, next) {
5959
return next();
6060
}
6161

62+
// We allow robots.txt to fall through to the next middleware, so that we can return our default robots.txt
63+
// We also allow sitemap.xml and sitemap-:resource.xml to fall through so that we can serve our defaults if they're not found in the theme
64+
const fallthroughFiles = [
65+
'/robots.txt',
66+
'/sitemap.xml',
67+
'/sitemap-posts.xml',
68+
'/sitemap-pages.xml',
69+
'/sitemap-tags.xml',
70+
'/sitemap-authors.xml',
71+
'/sitemap-users.xml',
72+
'/sitemap.xsl'
73+
];
74+
const fallthrough = fallthroughFiles.includes(req.path) ? true : false;
75+
6276
express.static(themeEngine.getActive().path, {
6377
// @NOTE: the maxAge config passed below are in milliseconds and the config
6478
// is specified in seconds. See https://github.com/expressjs/serve-static/issues/150 for more context
65-
maxAge: config.get('caching:theme:maxAge') * 1000
79+
maxAge: config.get('caching:theme:maxAge') * 1000,
80+
fallthrough
6681
}
6782
)(req, res, next);
6883
}

‎ghost/core/test/unit/frontend/web/middleware/static-theme.test.js

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,4 +326,132 @@ describe('staticTheme', function () {
326326
});
327327
});
328328
});
329+
330+
describe('fallthrough behavior', function () {
331+
it('should set fallthrough to true for /robots.txt', function (done) {
332+
req.path = '/robots.txt';
333+
334+
staticTheme()(req, res, function next() {
335+
// Specifically gets called twice
336+
activeThemeStub.calledTwice.should.be.true();
337+
expressStaticStub.called.should.be.true();
338+
339+
// Check that express static gets called with correct options
340+
should.exist(expressStaticStub.firstCall.args);
341+
expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
342+
343+
const options = expressStaticStub.firstCall.args[1];
344+
options.should.be.an.Object();
345+
options.should.have.property('maxAge');
346+
options.should.have.property('fallthrough', true);
347+
348+
done();
349+
});
350+
});
351+
352+
it('should set fallthrough to true for /sitemap.xml', function (done) {
353+
req.path = '/sitemap.xml';
354+
355+
staticTheme()(req, res, function next() {
356+
// Specifically gets called twice
357+
activeThemeStub.calledTwice.should.be.true();
358+
expressStaticStub.called.should.be.true();
359+
360+
// Check that express static gets called with correct options
361+
should.exist(expressStaticStub.firstCall.args);
362+
expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
363+
364+
const options = expressStaticStub.firstCall.args[1];
365+
options.should.be.an.Object();
366+
options.should.have.property('maxAge');
367+
options.should.have.property('fallthrough', true);
368+
369+
done();
370+
});
371+
});
372+
373+
it('should set fallthrough to true for /sitemap-posts.xml', function (done) {
374+
req.path = '/sitemap-posts.xml';
375+
376+
staticTheme()(req, res, function next() {
377+
// Specifically gets called twice
378+
activeThemeStub.calledTwice.should.be.true();
379+
expressStaticStub.called.should.be.true();
380+
381+
// Check that express static gets called with correct options
382+
should.exist(expressStaticStub.firstCall.args);
383+
expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
384+
385+
const options = expressStaticStub.firstCall.args[1];
386+
options.should.be.an.Object();
387+
options.should.have.property('maxAge');
388+
options.should.have.property('fallthrough', true);
389+
390+
done();
391+
});
392+
});
393+
394+
it('should set fallthrough to false for other static files', function (done) {
395+
req.path = '/style.css';
396+
397+
staticTheme()(req, res, function next() {
398+
// Specifically gets called twice
399+
activeThemeStub.calledTwice.should.be.true();
400+
expressStaticStub.called.should.be.true();
401+
402+
// Check that express static gets called with correct options
403+
should.exist(expressStaticStub.firstCall.args);
404+
expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
405+
406+
const options = expressStaticStub.firstCall.args[1];
407+
options.should.be.an.Object();
408+
options.should.have.property('maxAge');
409+
options.should.have.property('fallthrough', false);
410+
411+
done();
412+
});
413+
});
414+
415+
it('should set fallthrough to false for nested files', function (done) {
416+
req.path = '/assets/style.css';
417+
418+
staticTheme()(req, res, function next() {
419+
// Specifically gets called twice
420+
activeThemeStub.calledTwice.should.be.true();
421+
expressStaticStub.called.should.be.true();
422+
423+
// Check that express static gets called with correct options
424+
should.exist(expressStaticStub.firstCall.args);
425+
expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
426+
427+
const options = expressStaticStub.firstCall.args[1];
428+
options.should.be.an.Object();
429+
options.should.have.property('maxAge');
430+
options.should.have.property('fallthrough', false);
431+
432+
done();
433+
});
434+
});
435+
436+
it('should set fallthrough to false for allowed special files like manifest.json', function (done) {
437+
req.path = '/manifest.json';
438+
439+
staticTheme()(req, res, function next() {
440+
// Specifically gets called twice
441+
activeThemeStub.calledTwice.should.be.true();
442+
expressStaticStub.called.should.be.true();
443+
444+
// Check that express static gets called with correct options
445+
should.exist(expressStaticStub.firstCall.args);
446+
expressStaticStub.firstCall.args[0].should.eql('my/fake/path');
447+
448+
const options = expressStaticStub.firstCall.args[1];
449+
options.should.be.an.Object();
450+
options.should.have.property('maxAge');
451+
options.should.have.property('fallthrough', false);
452+
453+
done();
454+
});
455+
});
456+
});
329457
});

0 commit comments

Comments
 (0)