From 6e6a4822f29553c828e61de547784c49ccacb6b9 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Tue, 23 Nov 2021 12:16:32 +0000 Subject: [PATCH] Updated servePublicFile to honor v= cache keys - Currently it's assumed that public files are 100% static - With card assets, we're using it for files that are partially static, but can change between reboots and theme changes - We already have a system for managing cache busting across theme changes and restarts - the ?v= key that is added via the asset helper - This was already in place and used, but servePublicFile's internal cache didn't honor this key, and cached for the lifetime of boot - This small change means that if a ?v= query param is present on a request for a public file, that we pay attention to it. Else we cache as before --- .../web/middleware/serve-public-file.js | 33 +++++-- .../web/middleware/serve-public-file.test.js | 96 +++++++++++++------ 2 files changed, 94 insertions(+), 35 deletions(-) diff --git a/core/frontend/web/middleware/serve-public-file.js b/core/frontend/web/middleware/serve-public-file.js index 5f017d8ae8..3688192674 100644 --- a/core/frontend/web/middleware/serve-public-file.js +++ b/core/frontend/web/middleware/serve-public-file.js @@ -11,8 +11,23 @@ const messages = { fileNotFound: 'File not found' }; +/** + * If this request has a ?v= param, make sure the cache has the same key + * + * @param {Object} req + * @param {Object} cache + * @returns {boolean} + */ +function matchCacheKey(req, cache) { + if (req.query && req.query.v && cache && cache.key) { + return req.query.v === cache.key; + } + + return true; +} + function createPublicFileMiddleware(location, file, mime, maxAge) { - let content; + let cache; // These files are provided by Ghost, and therefore live inside of the core folder const staticFilePath = config.get('paths').publicFilePath; // These files are built on the fly, and must be saved in the content folder @@ -24,9 +39,9 @@ function createPublicFileMiddleware(location, file, mime, maxAge) { const blogRegex = /(\{\{blog-url\}\})/g; return function servePublicFileMiddleware(req, res, next) { - if (content) { - res.writeHead(200, content.headers); - return res.end(content.body); + if (cache && matchCacheKey(req, cache)) { + res.writeHead(200, cache.headers); + return res.end(cache.body); } // send image files directly and let express handle content-length, etag, etc @@ -67,17 +82,19 @@ function createPublicFileMiddleware(location, file, mime, maxAge) { str = str.replace(blogRegex, urlUtils.urlFor('home', true).replace(/\/$/, '')); } - content = { + cache = { headers: { 'Content-Type': mime, 'Content-Length': Buffer.from(str).length, ETag: `"${crypto.createHash('md5').update(str, 'utf8').digest('hex')}"`, 'Cache-Control': `public, max-age=${maxAge}` }, - body: str + body: str, + key: req.query && req.query.v ? req.query.v : null }; - res.writeHead(200, content.headers); - res.end(content.body); + + res.writeHead(200, cache.headers); + res.end(cache.body); }); }; } diff --git a/test/unit/frontend/web/middleware/serve-public-file.test.js b/test/unit/frontend/web/middleware/serve-public-file.test.js index c0ef33d4ea..bacbe04c5c 100644 --- a/test/unit/frontend/web/middleware/serve-public-file.test.js +++ b/test/unit/frontend/web/middleware/serve-public-file.test.js @@ -32,33 +32,7 @@ describe('servePublicFile', function () { next.called.should.be.true(); }); - it('should load the file and send it', function () { - const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); - const body = 'User-agent: * Disallow: /'; - req.path = '/robots.txt'; - - sinon.stub(fs, 'readFile').callsFake(function (file, cb) { - cb(null, body); - }); - - res = { - writeHead: sinon.spy(), - end: sinon.spy() - }; - - middleware(req, res, next); - next.called.should.be.false(); - res.writeHead.called.should.be.true(); - res.writeHead.args[0][0].should.equal(200); - res.writeHead.calledWith(200, sinon.match.has('Content-Type')).should.be.true(); - res.writeHead.calledWith(200, sinon.match.has('Content-Length')).should.be.true(); - res.writeHead.calledWith(200, sinon.match.has('ETag')).should.be.true(); - res.writeHead.calledWith(200, sinon.match.has('Cache-Control', 'public, max-age=3600')).should.be.true(); - - res.end.calledWith(body).should.be.true(); - }); - - it('should send the correct headers', function () { + it('should load the file and send it with the correct headers', function () { const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); const body = 'User-agent: * Disallow: /'; req.path = '/robots.txt'; @@ -84,6 +58,74 @@ describe('servePublicFile', function () { res.writeHead.calledWith(200, sinon.match.has('Cache-Control', 'public, max-age=3600')).should.be.true(); }); + it('should send the file from the cache the second time', function () { + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); + const body = 'User-agent: * Disallow: /'; + req.path = '/robots.txt'; + + let fileStub = sinon.stub(fs, 'readFile').callsFake(function (file, cb) { + cb(null, body); + }); + + res = { + writeHead: sinon.spy(), + end: sinon.spy() + }; + + middleware(req, res, next); + middleware(req, res, next); + + next.called.should.be.false(); + + // File only gets read onece + fileStub.calledOnce.should.be.true(); + fileStub.firstCall.args[0].should.endWith('core/frontend/public/robots.txt'); + + // File gets served twice + res.writeHead.calledTwice.should.be.true(); + res.writeHead.args[0][0].should.equal(200); + res.writeHead.calledWith(200, sinon.match.has('Content-Type')).should.be.true(); + res.writeHead.calledWith(200, sinon.match.has('Content-Length')).should.be.true(); + res.writeHead.calledWith(200, sinon.match.has('ETag')).should.be.true(); + res.writeHead.calledWith(200, sinon.match.has('Cache-Control', 'public, max-age=3600')).should.be.true(); + }); + + it('should not cache files requested with a different v tag', function () { + const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); + const body = 'User-agent: * Disallow: /'; + req.path = '/robots.txt'; + req.query = {v: 1}; + + let fileStub = sinon.stub(fs, 'readFile').callsFake(function (file, cb) { + cb(null, body); + }); + + res = { + writeHead: sinon.spy(), + end: sinon.spy() + }; + + middleware(req, res, next); + middleware(req, res, next); + + // Set a different cache key + req.query = {v: 2}; + middleware(req, res, next); + + fileStub.calledTwice.should.be.true(); + + next.called.should.be.false(); + fileStub.firstCall.args[0].should.endWith('core/frontend/public/robots.txt'); + fileStub.secondCall.args[0].should.endWith('core/frontend/public/robots.txt'); + + res.writeHead.calledThrice.should.be.true(); + res.writeHead.args[0][0].should.equal(200); + res.writeHead.calledWith(200, sinon.match.has('Content-Type')).should.be.true(); + res.writeHead.calledWith(200, sinon.match.has('Content-Length')).should.be.true(); + res.writeHead.calledWith(200, sinon.match.has('ETag')).should.be.true(); + res.writeHead.calledWith(200, sinon.match.has('Cache-Control', 'public, max-age=3600')).should.be.true(); + }); + it('should replace {{blog-url}} in text/plain', function () { const middleware = servePublicFile('static', 'robots.txt', 'text/plain', 3600); const body = 'User-agent: {{blog-url}}';