Skip to content

Commit 9747a66

Browse files
committed
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.
1 parent e966bb6 commit 9747a66

5 files changed

Lines changed: 76 additions & 12 deletions

File tree

src/app.constants.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ export const ERROR_MESSAGES = {
139139
PAGE_NOT_FOUND: "Request page not found",
140140
};
141141

142+
export const CSRF_MISSING_CODE = "EBADCSRFTOKEN";
143+
142144
export const ERROR_LOG_LEVEL = {
143145
ERROR: "Error",
144146
INFO: "Info",
Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,32 @@
11
import type { NextFunction, Request, Response } from "express";
2+
import { sessionIsValid } from "../middleware/session-middleware.js";
3+
import { CSRF_MISSING_CODE, HTTP_STATUS_CODES } from "../app.constants.js";
24

35
export function csrfMissingHandler(
46
err: any,
57
req: Request,
68
res: Response,
79
next: NextFunction
810
): void {
9-
const CSRF_MISSING_CODE = "EBADCSRFTOKEN";
1011
if (err.code === CSRF_MISSING_CODE) {
11-
// CSRF token missing or invalid
12-
res.status(403).send("Forbidden: Invalid or missing CSRF token");
13-
} else {
14-
next(err);
12+
res.status(HTTP_STATUS_CODES.FORBIDDEN);
13+
14+
if (sessionIsValid(req)) {
15+
req.log.error(
16+
"Invalid or missing CSRF token, but session is valid. Redirecting to 'internal server error' page."
17+
);
18+
19+
return res.render("common/errors/500.njk");
20+
} else {
21+
req.log.warn(
22+
"Session has expired - unable to validate CSRF token if present in request body. Redirecting to 'session expired' error page."
23+
);
24+
25+
return res.render("common/errors/session-expired.njk", {
26+
strategicAppChannel: res.locals.strategicAppChannel,
27+
});
28+
}
1529
}
30+
31+
next(err);
1632
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { NextFunction, Request, Response } from "express";
22
import { getAccountManagementUrl } from "../config.js";
33
import { ERROR_MESSAGES, HTTP_STATUS_CODES } from "../app.constants.js";
4+
45
export function serverErrorHandler(
56
err: any,
67
req: Request,
Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,25 @@
11
import { expect } from "chai";
22
import type { NextFunction, Request, Response } from "express";
33
import sinon from "sinon";
4-
import { HTTP_STATUS_CODES } from "../../app.constants.js";
4+
import { CSRF_MISSING_CODE, HTTP_STATUS_CODES } from "../../app.constants.js";
55
import { csrfMissingHandler } from "../csrf-missing-handler.js";
6+
import { createMockRequest } from "../../../test/helpers/mock-request-helper.js";
67
describe("csrfMissingHandler", () => {
78
let req: Request;
89
let res: Response;
910
let next: NextFunction;
1011

1112
beforeEach(() => {
12-
req = {} as Request;
13+
req = createMockRequest("/", {
14+
session: {
15+
id: "session-id",
16+
sessionRestored: true,
17+
},
18+
cookies: {
19+
gs: "gs-cookie",
20+
aps: "aps-cookie",
21+
},
22+
});
1323
res = {
1424
headersSent: false,
1525
statusCode: 200,
@@ -19,20 +29,44 @@ describe("csrfMissingHandler", () => {
1929
return this;
2030
},
2131
send: sinon.spy(),
32+
locals: {
33+
strategicAppChannel: false,
34+
},
2235
} as unknown as Response;
2336
next = sinon.spy();
2437
});
2538

26-
it("should return unauthorized if the error is a csrf missing error", () => {
39+
it("should render 500 template if the error is a csrf missing error and the session is valid", () => {
2740
const err = new Error("Invalid CSRF token") as any;
28-
err.code = "EBADCSRFTOKEN";
41+
err.code = CSRF_MISSING_CODE;
42+
43+
const renderSpy = sinon.spy(res, "render");
44+
const expectedTemplate = "common/errors/500.njk";
2945

3046
csrfMissingHandler(err, req, res, next);
3147

48+
expect(renderSpy).to.have.been.calledOnceWith(expectedTemplate);
3249
expect(res.statusCode).to.equal(HTTP_STATUS_CODES.FORBIDDEN);
33-
expect(res.send).to.have.been.calledOnceWith(
34-
"Forbidden: Invalid or missing CSRF token"
35-
);
50+
expect(req.log.error).to.have.been.calledOnce;
51+
52+
expect(next).not.to.have.been.called;
53+
});
54+
55+
it("should render session-expired template if the error is a csrf missing error and the session is invalid", () => {
56+
delete req.session;
57+
58+
const err = new Error("Invalid CSRF token") as any;
59+
err.code = CSRF_MISSING_CODE;
60+
61+
const renderSpy = sinon.spy(res, "render");
62+
const expectedTemplate = "common/errors/session-expired.njk";
63+
64+
csrfMissingHandler(err, req, res, next);
65+
66+
expect(renderSpy).to.have.been.calledOnceWith(expectedTemplate);
67+
expect(res.statusCode).to.equal(HTTP_STATUS_CODES.FORBIDDEN);
68+
expect(req.log.warn).to.have.been.calledOnce;
69+
3670
expect(next).not.to.have.been.called;
3771
});
3872

@@ -42,6 +76,9 @@ describe("csrfMissingHandler", () => {
4276

4377
csrfMissingHandler(err, req, res, next);
4478

79+
expect(req.log.error).not.to.have.been.called;
80+
expect(req.log.warn).not.to.have.been.called;
81+
4582
expect(next).to.have.been.called;
4683
});
4784
});

test/helpers/mock-request-helper.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { mockRequest } from "mock-req-res";
44

55
export interface MockRequestOptions {
66
headers?: any;
7+
session?: any;
8+
cookies?: any;
79
}
810

911
export function createMockRequest(
@@ -33,5 +35,11 @@ export function createMockRequest(
3335
if (request.headers["x-forwarded-for"]) {
3436
request.ip = options?.headers["x-forwarded-for"];
3537
}
38+
if (options?.session) {
39+
request.session = options.session;
40+
}
41+
if (options?.cookies) {
42+
request.cookies = options.cookies;
43+
}
3644
return request;
3745
}

0 commit comments

Comments
 (0)