PP-11850 update csp middleware (#3798)

- support chrome reporting api for csp violations
alphagov · Feb 8, 2024 · 7ef4f74 · 7ef4f74
1 parent 5096c9c
commit 7ef4f74
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 45 deletions.
diff --git a/app/middleware/csp.js b/app/middleware/csp.js
@@ -117,34 +117,44 @@ const requestParseMiddleware = (maxPayloadBytes) => {
 const detectErrorsMiddleware = (err, req, res, next) => {
   if (err) {
     if (err.type === 'entity.too.large') logger.info('CSP violation request payload exceeds maximum size')
-    if (err.type === 'entity.parse.failed') logger.info('CSP violation request payload could not be parsed')
+    if (err.type === 'entity.parse.failed') logger.info('CSP violation request payload did not match expected content type')
     return res.status(400).end()
   }
   next()
 }
 
 const captureEventMiddleware = (ignoredStrings) => {
   return (req, res) => {
-    const cspReport = req.body['csp-report']
+    let reports = undefined
+    if (Array.isArray(req.body) && req.body.length > 0) {
+      reports = req.body.filter(report => report.type === 'csp-violation') // new style reporting-api
+    } else if (req.body['csp-report'] !== undefined) {
+      reports = [{ body: req.body['csp-report'] }] // old style report-uri
+    }
     const userAgent = req.headers['user-agent']
-    if (cspReport !== undefined) {
-      const blockedUri = cspReport['blocked-uri']
-      const violatedDirective = cspReport['violated-directive']
-      if (violatedDirective === undefined || blockedUri === undefined) {
-        return res.status(400).end()
-      } else {
-        if (hasSubstr(ignoredStrings, blockedUri)) return res.status(204).end()
-        Sentry.captureEvent({
-          message: `Blocked ${violatedDirective} from ${blockedUri}`,
-          level: 'warning',
-          extra: {
-            cspReport: cspReport,
-            userAgent: userAgent
-          }
-        })
-        return res.status(204).end()
-      }
+    if (reports !== undefined) {
+      reports.forEach(report => {
+        const body = report.body
+        const blockedUri = body['blocked-uri']
+        const violatedDirective = body['violated-directive']
+        if (violatedDirective === undefined || blockedUri === undefined) {
+          logger.info('CSP violation report is invalid')
+          return res.status(400).end()
+        } else {
+          if (hasSubstr(ignoredStrings, blockedUri)) return res.status(204).end()
+          Sentry.captureEvent({
+            message: `Blocked ${violatedDirective} from ${blockedUri}`,
+            level: 'warning',
+            extra: {
+              cspReport: body,
+              userAgent: userAgent
+            }
+          })
+          return res.status(204).end()
+        }
+      })
     } else {
+      logger.info('CSP violation report missing')
       return res.status(400).end()
     }
   }

diff --git a/app/routes.js b/app/routes.js
@@ -67,7 +67,7 @@ exports.bind = function (app) {
 
   const cspMiddlewareStack = [
     rateLimitMiddleware,
-    requestParseMiddleware(2000),
+    requestParseMiddleware(4000),
     detectErrorsMiddleware,
     captureEventMiddleware([
       'www.facebook.com',

diff --git a/test/middleware/csp.test.js b/test/middleware/csp.test.js
@@ -44,29 +44,69 @@ describe('CSP report endpoint', () => {
     sentrySpy.resetHistory()
   })
 
-  const validPayload = {
+  const validReportUriPayload = {
     'csp-report': {
-      'blocked-uri': 'https://www.example.com',
-      'violated-directive': 'connect-src'
+      'document-uri': 'https://example.com/page-with-violation',
+      'referrer': '',
+      'violated-directive': 'script-src \'self\'',
+      'effective-directive': 'script-src',
+      'original-policy': 'default-src \'none\'; script-src \'self\'; object-src \'self\'; report-to: default',
+      'disposition': 'enforce',
+      'blocked-uri': 'https://evil.example.com/malicious.js',
+      'line-number': 22,
+      'column-number': 13,
+      'source-file': 'https://example.com/script.js',
+      'status-code': 200,
+      'script-sample': ''
     }
   }
 
-  it('should return 204 and send to sentry if a valid csp report is present', async () => {
-    await request(underTest)
-      .post('/test')
-      .set('user-agent', 'supertest')
-      .send(validPayload)
-      .expect(204)
-
-    expect(sentrySpy.calledOnce).to.be.true
-    expect(sentrySpy.calledWith({
-      message: 'Blocked connect-src from https://www.example.com',
-      level: 'warning',
-      extra: {
-        'cspReport': validPayload['csp-report'],
-        'userAgent': 'supertest'
+  const validReportingAPIPayload = [
+    {
+      'type': 'csp-violation',
+      'age': 310,
+      'url': 'https://example.com/page-with-violation',
+      'user_agent': 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36',
+      'body': {
+        'document-uri': 'https://example.com/page-with-violation',
+        'referrer': '',
+        'violated-directive': 'script-src \'self\'',
+        'effective-directive': 'script-src',
+        'original-policy': 'default-src \'none\'; script-src \'self\'; object-src \'self\'; report-to: default',
+        'disposition': 'enforce',
+        'blocked-uri': 'https://evil.example.com/malicious.js',
+        'line-number': 22,
+        'column-number': 13,
+        'source-file': 'https://example.com/script.js',
+        'status-code': 200,
+        'script-sample': ''
       }
-    })).to.be.true
+    }
+  ]
+
+  const validPayloads = [
+    { arg: validReportUriPayload, expected: validReportUriPayload['csp-report'], type: 'report-uri' },
+    { arg: validReportingAPIPayload, expected: validReportingAPIPayload[0]['body'], type: 'reporting api' }
+  ]
+
+  validPayloads.forEach(test => {
+    it(`should return 204 and send to sentry if a valid ${test.type} csp report is present`, async () => {
+      await request(underTest)
+        .post('/test')
+        .set('user-agent', 'supertest')
+        .send(test.arg)
+        .expect(204)
+
+      expect(sentrySpy.calledOnce).to.be.true
+      expect(sentrySpy.calledWith({
+        message: 'Blocked script-src \'self\' from https://evil.example.com/malicious.js',
+        level: 'warning',
+        extra: {
+          'cspReport': test.expected,
+          'userAgent': 'supertest'
+        }
+      })).to.be.true
+    })
   })
 
   it('should return 204 and not send to sentry if a valid csp report is present but the blocked-uri is ignored', async () => {
@@ -88,14 +128,14 @@ describe('CSP report endpoint', () => {
     await request(underTest)
       .post('/test')
       .set('content-type', 'application/x-www-form-urlencoded')
-      .send(validPayload)
+      .send(validReportUriPayload)
       .expect(400)
   })
 
   it('should return 400 if payload is too large', async () => {
 
     const largePayload = {
-      ...validPayload,
+      ...validReportUriPayload,
       'large_value': 'some text here x1000'.repeat(1000)
     }
 
@@ -116,7 +156,7 @@ describe('CSP report endpoint', () => {
       .expect(400)
 
     expect(loggingSpy.calledOnce).to.be.true
-    expect(loggingSpy.calledWith('CSP violation request payload could not be parsed')).to.be.true
+    expect(loggingSpy.calledWith('CSP violation request payload did not match expected content type')).to.be.true
   })
 
   it('should return 400 if json does not included expected values', async () => {
@@ -135,26 +175,26 @@ describe('CSP report endpoint', () => {
       await request(underTest)
         .post('/test')
         .set('x-forwarded-for', '1.2.3.4, 2.2.2.2, 3.3.3.3')
-        .send(validPayload)
+        .send(validReportingAPIPayload)
         .expect(204)
 
       await request(underTest)
         .post('/test')
         .set('x-forwarded-for', '5.6.7.8, 2.2.2.2, 3.3.3.3')
-        .send(validPayload)
+        .send(validReportUriPayload)
         .expect(204)
     }
 
     await request(underTest)
       .post('/test')
       .set('x-forwarded-for', '1.2.3.4, 2.2.2.2, 3.3.3.3')
-      .send(validPayload)
+      .send(validReportUriPayload)
       .expect(429)
 
     await request(underTest)
       .post('/test')
       .set('x-forwarded-for', '5.6.7.8, 2.2.2.2, 3.3.3.3')
-      .send(validPayload)
+      .send(validReportingAPIPayload)
       .expect(429)
   })
 })