From c0ab5d498d092b4439efd8067525fd3bf6850aa9 Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Fri, 4 Aug 2023 08:39:30 +0100 Subject: [PATCH] Set `Cache-Control: private` for project files (#13750) GitOrigin-RevId: b111c792a49a8a5e37734b5fcce1a69f4904c1ff --- .../Features/FileStore/FileStoreController.js | 3 + services/web/app/src/infrastructure/Server.js | 27 +++++-- .../acceptance/src/SecurityHeadersTests.js | 72 +++++++++++++++++-- .../src/FileStore/FileStoreControllerTests.js | 14 +++- 4 files changed, 101 insertions(+), 15 deletions(-) diff --git a/services/web/app/src/Features/FileStore/FileStoreController.js b/services/web/app/src/Features/FileStore/FileStoreController.js index 77e3df9cac..71a281f9dc 100644 --- a/services/web/app/src/Features/FileStore/FileStoreController.js +++ b/services/web/app/src/Features/FileStore/FileStoreController.js @@ -38,6 +38,9 @@ module.exports = { preparePlainTextResponse(res) } res.setContentDisposition('attachment', { filename: file.name }) + // allow the browser to cache these immutable files + // note: both "private" and "max-age" appear to be required for caching + res.setHeader('Cache-Control', 'private, max-age=3600') stream.pipe(res) } ) diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 3eb53c03f2..5768113ed4 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -253,13 +253,28 @@ webRouter.use(AuthenticationController.validateAdmin) // add security headers using Helmet const noCacheMiddleware = require('nocache')() webRouter.use(function addNoCacheHeader(req, res, next) { - const isLoggedIn = SessionManager.isUserLoggedIn(req.session) - const isProjectPage = !!req.path.match('^/project/[a-f0-9]{24}$') - if (isLoggedIn || isProjectPage) { - noCacheMiddleware(req, res, next) - } else { - next() + const isProjectPage = /^\/project\/[a-f0-9]{24}$/.test(req.path) + if (isProjectPage) { + // always set no-cache headers on a project page, as it could be an anonymous token viewer + return noCacheMiddleware(req, res, next) } + + const isProjectFile = /^\/project\/[a-f0-9]{24}\/file\/[a-f0-9]{24}$/.test( + req.path + ) + if (isProjectFile) { + // don't set no-cache headers on a project file, as it's immutable and can be cached (privately) + return next() + } + + const isLoggedIn = SessionManager.isUserLoggedIn(req.session) + if (isLoggedIn) { + // always set no-cache headers for authenticated users (apart from project files, above) + return noCacheMiddleware(req, res, next) + } + + // allow other responses (anonymous users, except for project pages) to be cached + return next() }) webRouter.use( helmet({ diff --git a/services/web/test/acceptance/src/SecurityHeadersTests.js b/services/web/test/acceptance/src/SecurityHeadersTests.js index 9e0e1b608b..3a9e67bdb6 100644 --- a/services/web/test/acceptance/src/SecurityHeadersTests.js +++ b/services/web/test/acceptance/src/SecurityHeadersTests.js @@ -15,6 +15,7 @@ const { assert } = require('chai') const async = require('async') const User = require('./helpers/User') const request = require('./helpers/request') +const ProjectGetter = require('../../../app/src/Features/Project/ProjectGetter') const assertHasCommonHeaders = function (response) { const { headers } = response @@ -100,29 +101,86 @@ describe('SecurityHeaders', function () { ) }) - it('should have cache headers on project page', function (done) { + it('should have cache headers on project page when user is logged out', function (done) { return async.series( [ cb => this.user.login(cb), - cb => { - return this.user.createProject( + cb => + this.user.createProject('public-project', (error, projectId) => { + if (error != null) { + return done(error) + } + this.project_id = projectId + return this.user.makePublic(this.project_id, 'readAndWrite', cb) + }), + cb => this.user.logout(cb), + cb => request.get(`/project/${this.project_id}`, cb), + ], + (err, res) => { + const mainResponse = res[3][0] + assertHasCacheHeaders(mainResponse) + return done() + } + ) + }) + + it('should have private cache headers on project file', function (done) { + return async.series( + [ + cb => this.user.login(cb), + cb => + this.user.createProject( 'public-project', - (error, projectId) => { + (error, projectId, folderId) => { if (error != null) { return done(error) } this.project_id = projectId return this.user.makePublic(this.project_id, 'readAndWrite', cb) } + ), + cb => + ProjectGetter.getProject(this.project_id, (error, project) => { + if (error) { + return cb(error) + } + this.root_folder_id = project.rootFolder[0]._id.toString() + cb() + }), + cb => { + return this.user.uploadFileInProject( + this.project_id, + this.root_folder_id, + '2pixel.png', + '1pixel.png', + 'image/png', + (error, fileId) => { + if (error) { + return cb(error) + } + this.file_id = fileId + cb() + } ) }, + cb => + request.get(`/project/${this.project_id}/file/${this.file_id}`, cb), cb => this.user.logout(cb), ], (err, results) => { - return request.get(`/project/${this.project_id}`, (err, res, body) => { - assertHasCacheHeaders(res) - return done() + const res = results[4][0] + + assert.include(res.headers, { + 'cache-control': 'private, max-age=3600', }) + + assert.doesNotHaveAnyKeys(res.headers, [ + 'surrogate-control', + 'pragma', + 'expires', + ]) + + return done() } ) }) diff --git a/services/web/test/unit/src/FileStore/FileStoreControllerTests.js b/services/web/test/unit/src/FileStore/FileStoreControllerTests.js index 72c7a09321..3092061f9a 100644 --- a/services/web/test/unit/src/FileStore/FileStoreControllerTests.js +++ b/services/web/test/unit/src/FileStore/FileStoreControllerTests.js @@ -7,6 +7,10 @@ const MockResponse = require('../helpers/MockResponse') const MODULE_PATH = '../../../../app/src/Features/FileStore/FileStoreController.js' +const expectedFileHeaders = { + 'Cache-Control': 'private, max-age=3600', +} + describe('FileStoreController', function () { beforeEach(function () { this.FileStoreHandler = { @@ -104,7 +108,9 @@ describe('FileStoreController', function () { describe('from a non-ios browser', function () { it('should not set Content-Type', function (done) { this.stream.pipe = des => { - this.res.headers.should.deep.equal({}) + this.res.headers.should.deep.equal({ + ...expectedFileHeaders, + }) done() } this.controller.getFile(this.req, this.res) @@ -123,6 +129,7 @@ describe('FileStoreController', function () { it("should set Content-Type to 'text/plain'", function (done) { this.stream.pipe = des => { this.res.headers.should.deep.equal({ + ...expectedFileHeaders, 'Content-Type': 'text/plain; charset=utf-8', 'X-Content-Type-Options': 'nosniff', }) @@ -144,6 +151,7 @@ describe('FileStoreController', function () { it("should set Content-Type to 'text/plain'", function (done) { this.stream.pipe = des => { this.res.headers.should.deep.equal({ + ...expectedFileHeaders, 'Content-Type': 'text/plain; charset=utf-8', 'X-Content-Type-Options': 'nosniff', }) @@ -179,7 +187,9 @@ describe('FileStoreController', function () { it('Should not set the Content-type', function (done) { this.stream.pipe = des => { - this.res.headers.should.deep.equal({}) + this.res.headers.should.deep.equal({ + ...expectedFileHeaders, + }) done() } this.controller.getFile(this.req, this.res)