From d7fbf94d911a160e8acd0f5c2290aeb65f5050c6 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Fri, 22 Oct 2021 14:25:58 +0100 Subject: [PATCH] Fixed ETag header for admin templates not changing between versions (#13680) refs https://github.com/TryGhost/Team/issues/1175 We found the ETag header sent when serving the Admin template for /ghost/ was not changing between versions which after an upgrade could result in out of date cached content being served containing links to JS/CSS files that no longer existed. The culprit is weak etags served by Node's `send` package, coupled with Admin template filesize not changing between versions and `npm pack` setting a fixed modification date for every file. See https://github.com/pillarjs/send/issues/176 for more details. - updated the Admin app's controller to read the template and generate an md5 hash of the contents so we can serve a strong ETag header value when serving the `/ghost/` html --- core/server/web/admin/controller.js | 11 ++++ test/e2e-server/admin.test.js | 55 +++++++++++++++++-- .../fixtures/admin-views/default-prod.html | 4 ++ test/utils/fixtures/admin-views/default.html | 4 ++ 4 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 test/utils/fixtures/admin-views/default-prod.html create mode 100644 test/utils/fixtures/admin-views/default.html diff --git a/core/server/web/admin/controller.js b/core/server/web/admin/controller.js index 4e4685abc4..272b70b943 100644 --- a/core/server/web/admin/controller.js +++ b/core/server/web/admin/controller.js @@ -1,5 +1,7 @@ const debug = require('@tryghost/debug')('web:admin:controller'); const path = require('path'); +const fs = require('fs'); +const crypto = require('crypto'); const config = require('../../../shared/config'); const updateCheck = require('../../update-check'); @@ -21,6 +23,15 @@ module.exports = function adminController(req, res) { const templatePath = path.resolve(config.get('paths').adminViews, defaultTemplate); const headers = {}; + // Generate our own ETag header + // `sendFile` by default uses filesize+lastmod date to generate an etag. + // That doesn't work for admin templates because the filesize doesn't change between versions + // and `npm pack` sets a fixed lastmod date for every file meaning the default etag never changes + const fileBuffer = fs.readFileSync(templatePath); + const hashSum = crypto.createHash('md5'); + hashSum.update(fileBuffer); + headers.ETag = hashSum.digest('hex'); + if (config.get('adminFrameProtection')) { headers['X-Frame-Options'] = 'sameorigin'; } diff --git a/test/e2e-server/admin.test.js b/test/e2e-server/admin.test.js index b19aa02ae3..4795dd4266 100644 --- a/test/e2e-server/admin.test.js +++ b/test/e2e-server/admin.test.js @@ -4,6 +4,8 @@ // But then again testing real code, rather than mock code, might be more useful... const should = require('should'); +const path = require('path'); +const fs = require('fs'); const supertest = require('supertest'); const testUtils = require('../utils'); @@ -28,14 +30,14 @@ describe('Admin Routing', function () { describe('Assets', function () { it('should return 404 for unknown assets', async function () { - request.get('/ghost/assets/not-found.js') + await request.get('/ghost/assets/not-found.js') .expect('Cache-Control', testUtils.cacheRules.private) .expect(404) .expect(assertCorrectHeaders); }); it('should retrieve built assets', async function () { - request.get('/ghost/assets/vendor.js') + await request.get('/ghost/assets/vendor.js') .expect('Cache-Control', testUtils.cacheRules.year) .expect(200) .expect(assertCorrectHeaders); @@ -44,7 +46,7 @@ describe('Admin Routing', function () { describe('Admin Redirects', function () { it('should redirect /GHOST/ to /ghost/', async function () { - request.get('/GHOST/') + await request.get('/GHOST/') .expect('Location', '/ghost/') .expect(301) .expect(assertCorrectHeaders); @@ -67,17 +69,60 @@ describe('Admin Routing', function () { }); it('should redirect admin access over non-HTTPS', async function () { - request.get('/ghost/') + await request.get('/ghost/') .expect('Location', /^https:\/\/localhost:2390\/ghost\//) .expect(301) .expect(assertCorrectHeaders); }); it('should allow admin access over HTTPS', async function () { - request.get('/ghost/') + await request.get('/ghost/') .set('X-Forwarded-Proto', 'https') .expect(200) .expect(assertCorrectHeaders); }); }); + + describe('built template', function () { + beforeEach(function () { + const configPaths = configUtils.config.get('paths'); + configPaths.adminViews = path.resolve('test/utils/fixtures/admin-views'); + configUtils.set('paths', configPaths); + }); + + afterEach(function () { + configUtils.restore(); + }); + + it('serves prod file in production', async function () { + configUtils.set('env', 'production'); + + const prodTemplate = fs.readFileSync(path.resolve('test/utils/fixtures/admin-views/default-prod.html')).toString(); + + const res = await request.get('/ghost/') + .set('X-Forwarded-Proto', 'https') + .expect(200); + + res.text.should.equal(prodTemplate); + }); + + it('serves dev file when not in production', async function () { + const devTemplate = fs.readFileSync(path.resolve('test/utils/fixtures/admin-views/default.html')).toString(); + + const res = await request.get('/ghost/') + .set('X-Forwarded-Proto', 'https') + .expect(200); + + res.text.should.equal(devTemplate); + }); + + it('generates it\'s own ETag header from file contents', async function () { + const res = await request.get('/ghost/') + .set('X-Forwarded-Proto', 'https') + .expect(200); + + should.exist(res.headers.etag); + res.headers.etag.should.equal('b448e5380dbfc46bc7c6da6045bf3043'); + }); + }); }); diff --git a/test/utils/fixtures/admin-views/default-prod.html b/test/utils/fixtures/admin-views/default-prod.html new file mode 100644 index 0000000000..dfd79f5dc7 --- /dev/null +++ b/test/utils/fixtures/admin-views/default-prod.html @@ -0,0 +1,4 @@ + + Prod template + Prod template + diff --git a/test/utils/fixtures/admin-views/default.html b/test/utils/fixtures/admin-views/default.html new file mode 100644 index 0000000000..936a758de2 --- /dev/null +++ b/test/utils/fixtures/admin-views/default.html @@ -0,0 +1,4 @@ + + Dev template + Dev template +