Set Cache-Control: private for project files (#13750)

GitOrigin-RevId: b111c792a49a8a5e37734b5fcce1a69f4904c1ff

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)
           }
         )

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({

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()
       }
     )
   })

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)