Handle unauthorized errors plus return safe codes

app/services/plugins/error-pages.service.js

@@ -21,27 +21,31 @@
  * decide how to direct the response
  */
 function go (request) {
-  const result = {
-    stopResponse: _stopResponse(request),
-    statusCode: _statusCode(request)
-  }
+  const stopResponse = _stopResponse(request)
 
-  _logError(result.statusCode, request)
+  let statusCode = _extractStatusCode(request)
 
-  return result
-}
+  _logError(statusCode, request)
 
-function _logError (statusCode, request) {
-  const { response } = request
+  // If we're stopping the response i.e. redirecting to an error page we need to return a 'safe' status code.
+  if (stopResponse) {
+    statusCode = _determineSafeStatusCode(statusCode)
+  }
 
-  if (statusCode === 404) {
-    global.GlobalNotifier.omg('Page not found', { path: request.path })
-  } else if (response.isBoom) {
-    global.GlobalNotifier.omfg(response.message, {}, response)
+  return {
+    statusCode,
+    stopResponse
   }
 }
 
-function _statusCode (request) {
+/**
+ * Extract the status code from the request
+ *
+ * If the request object reflects a 2xx or 3xx response then the status code will be a property of the request. But if
+ * it's because an error is thrown, `request` is actually a Boom error instance which means the status code is
+ * somewhere else. So, we need this bit of logic to figure out what status code we're dealing with!
+ */
+function _extractStatusCode (request) {
   const { response } = request
 
   if (response.isBoom) {
@@ -51,6 +55,18 @@ function _statusCode (request) {
   return response.statusCode
 }
 
+function _logError (statusCode, request) {
+  const { response } = request
+
+  if (statusCode === 404) {
+    global.GlobalNotifier.omg('Page not found', { path: request.path })
+  } else if (statusCode === 403) {
+    global.GlobalNotifier.omg('Not authorised', { path: request.path })
+  } else if (response.isBoom) {
+    global.GlobalNotifier.omfg(response.message, {}, response)
+  }
+}
+
 /**
  * Determine if the response should be stopped and redirected to an error page
  *
@@ -83,6 +99,33 @@ function _stopResponse (request) {
   return isBoom && !plainOutput
 }
 
+/**
+ * Determine the safe error code to use
+ *
+ * This will only be called when _stopResponse() has determined we need to redirect to an error page. In this case
+ * we need to ensure the code we return is secure and will not get the response blocked by the WAF we have in our AWS
+ * environments.
+ */
+function _determineSafeStatusCode (statusCode) {
+  // The status code will be a 2xx or 3xx so safe to return as is
+  if (statusCode < 400) {
+    return statusCode
+  }
+
+  // If it was an unauthorised (403) request we pretend the page doesn't exist for security reasons. Returning anything
+  // other than 404 could be seen as confirmation the page exists and used in an enumeration attack.
+  // Returning 404 is the accepted response when you want to keep the targetted resource hidden
+  // https://www.rfc-editor.org/rfc/rfc9110.html#name-403-forbidden
+  if ([404, 403].includes(statusCode)) {
+    return 404
+  }
+
+  // If it's any other status code it will reflect an error has occurred. Our F5 Silverline Managed Web Application
+  // Firewall (WAF) will block the response and serve its own error page. We don't want this so we have to return a
+  // 'safe' 200.
+  return 200
+}
+
 module.exports = {
   go
 }

test/plugins/error-pages.plugin.test.js

@@ -8,9 +8,6 @@ const Sinon = require('sinon')
 const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script()
 const { expect } = Code
 
-// Things we need to stub
-const ErrorPagesService = require('../../app/services/plugins/error-pages.service.js')
-
 // Test helpers
 const Boom = require('@hapi/boom')
 
@@ -23,104 +20,162 @@ describe('Error Pages plugin', () => {
     url: '/error-pages'
   }
 
-  let testRoute
+  let notifierStub
   let server
+  let testRoute
 
   beforeEach(async () => {
+    testRoute = {
+      method: 'GET',
+      path: '/error-pages',
+      options: {
+        auth: false
+      }
+    }
+
     // Create server before each test
     server = await init()
+
+    notifierStub = { omg: Sinon.stub(), omfg: Sinon.stub() }
+    global.GlobalNotifier = notifierStub
   })
 
   afterEach(() => {
     Sinon.restore()
   })
 
   describe('When the response is a Boom error', () => {
-    beforeEach(() => {
-      testRoute = {
-        method: 'GET',
-        path: '/error-pages',
-        handler: function (_request, _h) {
-          return Boom.badRequest('Things go boom')
-        },
-        options: {
-          auth: false
+    describe('because the request was not found', () => {
+      beforeEach(() => {
+        testRoute.handler = (_request, _h) => {
+          return Boom.notFound('where has my boom gone?')
         }
-      }
-    })
+      })
 
-    describe('and it is not a 404', () => {
       describe('and the route is not configured for plain output (redirect to error page)', () => {
         beforeEach(async () => {
           server.route(testRoute)
-
-          Sinon.stub(ErrorPagesService, 'go').returns({ stopResponse: true, statusCode: 400 })
         })
 
-        it('returns our general error HTML page', async () => {
+        it('returns our 404 error HTML page', async () => {
           const response = await server.inject(request)
 
-          expect(response.statusCode).to.equal(400)
-          expect(response.statusMessage).to.equal('Bad Request')
+          expect(response.statusCode).to.equal(404)
+          expect(response.statusMessage).to.equal('Not Found')
           expect(response.payload).startsWith('<!DOCTYPE html>')
-          expect(response.payload).contains('Sorry, there is a problem with the service')
+          expect(response.payload).contains('Page not found')
         })
       })
 
       describe('and the route is configured for plain output (do not redirect to error page)', () => {
         beforeEach(async () => {
           testRoute.options.app = { plainOutput: true }
+
           server.route(testRoute)
+        })
+
+        it('returns a plain response', async () => {
+          const response = await server.inject(request)
 
-          Sinon.stub(ErrorPagesService, 'go').returns({ stopResponse: false, statusCode: 400 })
+          expect(response.statusCode).to.equal(404)
+          expect(response.statusMessage).to.equal('Not Found')
+          expect(response.payload).not.to.startWith('<!DOCTYPE html>')
+          expect(response.payload).contains('where has my boom gone?')
+        })
+      })
+    })
+
+    describe('because the request was forbidden', () => {
+      beforeEach(() => {
+        testRoute.handler = (_request, _h) => {
+          return Boom.forbidden("can't touch this")
+        }
+      })
+
+      describe('and the route is not configured for plain output (redirect to error page)', () => {
+        beforeEach(async () => {
+          server.route(testRoute)
+        })
+
+        it('returns our 404 error HTML page', async () => {
+          const response = await server.inject(request)
+
+          expect(response.statusCode).to.equal(404)
+          expect(response.statusMessage).to.equal('Not Found')
+          expect(response.payload).startsWith('<!DOCTYPE html>')
+          expect(response.payload).contains('Page not found')
+        })
+      })
+
+      describe('and the route is configured for plain output (do not redirect to error page)', () => {
+        beforeEach(async () => {
+          testRoute.options.app = { plainOutput: true }
+
+          server.route(testRoute)
         })
 
         it('returns a plain response', async () => {
           const response = await server.inject(request)
 
-          expect(response.statusCode).to.equal(400)
-          expect(response.statusMessage).to.equal('Bad Request')
+          expect(response.statusCode).to.equal(403)
+          expect(response.statusMessage).to.equal('Forbidden')
           expect(response.payload).not.to.startWith('<!DOCTYPE html>')
-          expect(response.payload).contains('Things go boom')
+          expect(response.payload).contains("can't touch this")
         })
       })
     })
 
-    describe('and it is a 404', () => {
+    describe('because the request was bad (any other error)', () => {
       beforeEach(() => {
-        Sinon.stub(ErrorPagesService, 'go').returns({ stopResponse: true, statusCode: 404 })
+        testRoute.handler = (_request, _h) => {
+          return Boom.badRequest('computer says no')
+        }
       })
 
-      it('returns our 404 error HTML page', async () => {
-        const response = await server.inject(request)
+      describe('and the route is not configured for plain output (redirect to error page)', () => {
+        beforeEach(async () => {
+          server.route(testRoute)
+        })
+
+        it('returns our 500 (there is a problem) error HTML page', async () => {
+          const response = await server.inject(request)
+
+          expect(response.statusCode).to.equal(200)
+          expect(response.statusMessage).to.equal('OK')
+          expect(response.payload).startsWith('<!DOCTYPE html>')
+          expect(response.payload).contains('Sorry, there is a problem with the service')
+        })
+      })
+
+      describe('and the route is configured for plain output (do not redirect to error page)', () => {
+        beforeEach(async () => {
+          testRoute.options.app = { plainOutput: true }
 
-        expect(response.statusCode).to.equal(404)
-        expect(response.statusMessage).to.equal('Not Found')
-        expect(response.payload).startsWith('<!DOCTYPE html>')
-        expect(response.payload).contains('Page not found')
+          server.route(testRoute)
+        })
+
+        it('returns a plain response', async () => {
+          const response = await server.inject(request)
+
+          expect(response.statusCode).to.equal(400)
+          expect(response.statusMessage).to.equal('Bad Request')
+          expect(response.payload).not.to.startWith('<!DOCTYPE html>')
+          expect(response.payload).contains('computer says no')
+        })
       })
     })
   })
 
   describe('When the response is not a Boom error', () => {
     beforeEach(() => {
-      testRoute = {
-        method: 'GET',
-        path: '/error-pages',
-        handler: function (_request, h) {
-          return h.response({ hello: 'world' }).code(200)
-        },
-        options: {
-          auth: false
-        }
+      testRoute.handler = (_request, h) => {
+        return h.response({ hello: 'world' }).code(200)
       }
     })
 
     describe('and the route is not configured for plain output (redirect to error page)', () => {
       beforeEach(async () => {
         server.route(testRoute)
-
-        Sinon.stub(ErrorPagesService, 'go').returns({ stopResponse: false, statusCode: 200 })
       })
 
       it('lets the response continue without change', async () => {
@@ -135,8 +190,6 @@ describe('Error Pages plugin', () => {
       beforeEach(async () => {
         testRoute.options.app = { plainOutput: true }
         server.route(testRoute)
-
-        Sinon.stub(ErrorPagesService, 'go').returns({ stopResponse: false, statusCode: 200 })
       })
 
       it('lets the response continue without change', async () => {