AUT-4272: render error pages upon CSRF error

Note that if the session is invalid (e.g., if it has expired) then we wish to render the standard "session expired" page. Previously in this case we would render the CSRF error text which is not user friendly.

This commit renders the "session expired" page if a CSRF error has occurred, but this is due to an invalid session. If the session is still active, then we render the "internal server error" page. The user can use the browsers back button to return to the form on the previous page, the token in the "hidden" input will be reset to a valid value automatically, allowing the user to resubmit the form successfully.

Author: sw-mattw

src/app.constants.ts

@@ -139,6 +139,8 @@ export const ERROR_MESSAGES = {
   PAGE_NOT_FOUND: "Request page not found",
 };
 
+export const CSRF_MISSING_CODE = "EBADCSRFTOKEN";
+
 export const ERROR_LOG_LEVEL = {
   ERROR: "Error",
   INFO: "Info",

src/handlers/csrf-missing-handler.ts

@@ -1,16 +1,28 @@
 import type { NextFunction, Request, Response } from "express";
+import { sessionIsValid } from "../middleware/session-middleware.js";
+import { CSRF_MISSING_CODE, HTTP_STATUS_CODES } from "../app.constants.js";
 
 export function csrfMissingHandler(
   err: any,
   req: Request,
   res: Response,
   next: NextFunction
 ): void {
-  const CSRF_MISSING_CODE = "EBADCSRFTOKEN";
   if (err.code === CSRF_MISSING_CODE) {
-    // CSRF token missing or invalid
-    res.status(403).send("Forbidden: Invalid or missing CSRF token");
-  } else {
-    next(err);
+    if (sessionIsValid(req)) {
+      req.log.error(
+        `Invalid or missing CSRF token, but session is valid. Returning ${HTTP_STATUS_CODES.FORBIDDEN} status.`
+      );
+
+      res.status(HTTP_STATUS_CODES.FORBIDDEN);
+    } else {
+      req.log.warn(
+        `Session has expired - unable to validate CSRF token if present in request body. Returning ${HTTP_STATUS_CODES.UNAUTHORIZED} status.`
+      );
+
+      res.status(HTTP_STATUS_CODES.UNAUTHORIZED);
+    }
   }
+
+  next(err);
 }

src/handlers/internal-server-error-handler.ts

@@ -1,6 +1,11 @@
 import type { NextFunction, Request, Response } from "express";
 import { getAccountManagementUrl } from "../config.js";
-import { ERROR_MESSAGES, HTTP_STATUS_CODES } from "../app.constants.js";
+import {
+  CSRF_MISSING_CODE,
+  ERROR_MESSAGES,
+  HTTP_STATUS_CODES,
+} from "../app.constants.js";
+
 export function serverErrorHandler(
   err: any,
   req: Request,
@@ -11,6 +16,13 @@ export function serverErrorHandler(
     return next(err);
   }
 
+  if (
+    res.statusCode == HTTP_STATUS_CODES.FORBIDDEN &&
+    err.code === CSRF_MISSING_CODE
+  ) {
+    return res.render("common/errors/500.njk");
+  }
+
   if (
     (res.statusCode == HTTP_STATUS_CODES.UNAUTHORIZED &&
       err.message ===

src/handlers/tests/csrf-missing-handler.test.ts

@@ -1,15 +1,25 @@
 import { expect } from "chai";
 import type { NextFunction, Request, Response } from "express";
 import sinon from "sinon";
-import { HTTP_STATUS_CODES } from "../../app.constants.js";
+import { CSRF_MISSING_CODE, HTTP_STATUS_CODES } from "../../app.constants.js";
 import { csrfMissingHandler } from "../csrf-missing-handler.js";
+import { createMockRequest } from "../../../test/helpers/mock-request-helper.js";
 describe("csrfMissingHandler", () => {
   let req: Request;
   let res: Response;
   let next: NextFunction;
 
   beforeEach(() => {
-    req = {} as Request;
+    req = createMockRequest("/", {
+      session: {
+        id: "session-id",
+        sessionRestored: true,
+      },
+      cookies: {
+        gs: "gs-cookie",
+        aps: "aps-cookie",
+      },
+    });
     res = {
       headersSent: false,
       statusCode: 200,
@@ -23,17 +33,30 @@ describe("csrfMissingHandler", () => {
     next = sinon.spy();
   });
 
-  it("should return unauthorized if the error is a csrf missing error", () => {
+  it("should return forbidden if the error is a csrf missing error and the session is valid", () => {
     const err = new Error("Invalid CSRF token") as any;
-    err.code = "EBADCSRFTOKEN";
+    err.code = CSRF_MISSING_CODE;
 
     csrfMissingHandler(err, req, res, next);
 
     expect(res.statusCode).to.equal(HTTP_STATUS_CODES.FORBIDDEN);
-    expect(res.send).to.have.been.calledOnceWith(
-      "Forbidden: Invalid or missing CSRF token"
-    );
-    expect(next).not.to.have.been.called;
+    expect(req.log.error).to.have.been.calledOnce;
+
+    expect(next).to.have.been.called;
+  });
+
+  it("should return unauthorized if the error is a csrf missing error and the session is invalid", () => {
+    delete req.session;
+
+    const err = new Error("Invalid CSRF token") as any;
+    err.code = CSRF_MISSING_CODE;
+
+    csrfMissingHandler(err, req, res, next);
+
+    expect(res.statusCode).to.equal(HTTP_STATUS_CODES.UNAUTHORIZED);
+    expect(req.log.warn).to.have.been.calledOnce;
+
+    expect(next).to.have.been.called;
   });
 
   it("should call next for any other error", () => {
@@ -42,6 +65,9 @@ describe("csrfMissingHandler", () => {
 
     csrfMissingHandler(err, req, res, next);
 
+    expect(req.log.error).not.to.have.been.called;
+    expect(req.log.warn).not.to.have.been.called;
+
     expect(next).to.have.been.called;
   });
 });

src/handlers/tests/internal-server-error-handler.test.ts

@@ -1,7 +1,11 @@
 import { expect } from "chai";
 import type { NextFunction, Request, Response } from "express";
 import sinon from "sinon";
-import { ERROR_MESSAGES, HTTP_STATUS_CODES } from "../../app.constants.js";
+import {
+  CSRF_MISSING_CODE,
+  ERROR_MESSAGES,
+  HTTP_STATUS_CODES,
+} from "../../app.constants.js";
 import { serverErrorHandler } from "../internal-server-error-handler.js";
 describe("serverErrorHandler", () => {
   let req: Request;
@@ -34,6 +38,21 @@ describe("serverErrorHandler", () => {
     expect(res.statusCode).to.equal(200);
   });
 
+  it("should render 500 template if error is forbidden and the error is a csrf missing error", () => {
+    const err = new Error("Invalid CSRF token") as any;
+    err.code = CSRF_MISSING_CODE;
+
+    res.statusCode = HTTP_STATUS_CODES.FORBIDDEN;
+
+    const renderSpy = sinon.spy(res, "render");
+    const expectedTemplate = "common/errors/500.njk";
+
+    serverErrorHandler(err, req, res, next);
+
+    expect(renderSpy).to.have.been.calledOnceWith(expectedTemplate);
+    expect(res.statusCode).to.equal(HTTP_STATUS_CODES.FORBIDDEN);
+  });
+
   it("should render mid-journey-direct-navigation template if session is invalid and not a GOV.UK request", () => {
     res.statusCode = HTTP_STATUS_CODES.UNAUTHORIZED;
     const err = new Error(

test/helpers/mock-request-helper.ts

@@ -4,6 +4,8 @@ import { mockRequest } from "mock-req-res";
 
 export interface MockRequestOptions {
   headers?: any;
+  session?: any;
+  cookies?: any;
 }
 
 export function createMockRequest(
@@ -33,5 +35,11 @@ export function createMockRequest(
   if (request.headers["x-forwarded-for"]) {
     request.ip = options?.headers["x-forwarded-for"];
   }
+  if (options?.session) {
+    request.session = options.session;
+  }
+  if (options?.cookies) {
+    request.cookies = options.cookies;
+  }
   return request;
 }