diff --git a/src/main/routes/oauth2redirect.ts b/src/main/routes/oauth2redirect.ts index 7de8888a4..e156fa771 100644 --- a/src/main/routes/oauth2redirect.ts +++ b/src/main/routes/oauth2redirect.ts @@ -6,7 +6,28 @@ export const COOKIE_ACCESS_TOKEN = "accessToken"; const router = express.Router(); export const oauth2redirect = (req, res, next) => { - if (req.query.code) { + const expectedState = req.session && req.session.oauthState; + const rawState = req.query && req.query.state; + const receivedState = Array.isArray(rawState) ? rawState[0] : rawState; + + if (!expectedState || !receivedState || expectedState !== receivedState) { + if (req.session) { + delete req.session.oauthState; + } + return next({ + message: "Invalid state parameter - possible CSRF attack", + status: 400, + }); + } + // Single-use nonce: consume immediately after successful validation. + delete req.session.oauthState; + + if (!req.query.code) { + return next({ + message: "Unable to obtain access token - no OAuth2 code provided", + status: 400, + }); + } else { // On successfully obtaining a token, the redirect should go back to ourselves. // Note: This *must not* include any query string. req.query.redirect_uri = `${req.protocol}://${req.get("host")}${req.originalUrl}` @@ -23,8 +44,6 @@ export const oauth2redirect = (req, res, next) => { res.redirect(302, "/"); }) .catch((err) => next(err)); - } else { - throw new Error("Unable to obtain access token - no OAuth2 code provided"); } }; diff --git a/src/main/user/auth-checker-user-only-filter.ts b/src/main/user/auth-checker-user-only-filter.ts index bea0cd7bd..b12673752 100644 --- a/src/main/user/auth-checker-user-only-filter.ts +++ b/src/main/user/auth-checker-user-only-filter.ts @@ -1,6 +1,7 @@ import { authorize } from "./user-request-authorizer"; import { get } from "config"; import { Logger } from "@hmcts/nodejs-logging"; +import { randomBytes } from "crypto"; export const authCheckerUserOnlyFilter = (req, res, next) => { @@ -19,8 +20,9 @@ export const authCheckerUserOnlyFilter = (req, res, next) => { if (error.status === 403) { next(error); } else { + req.session.oauthState = randomBytes(32).toString("hex"); res.redirect(302, `${get("adminWeb.login_url")}?response_type=code&client_id=` + - `${get("idam.oauth2.client_id")}&redirect_uri=${REDIRECT_URI}`); + `${get("idam.oauth2.client_id")}&redirect_uri=${REDIRECT_URI}&state=${encodeURIComponent(req.session.oauthState)}`); } }); }; diff --git a/src/test/routes/oauth2redirect.spec.ts b/src/test/routes/oauth2redirect.spec.ts index 41609d9bb..779dc7192 100644 --- a/src/test/routes/oauth2redirect.spec.ts +++ b/src/test/routes/oauth2redirect.spec.ts @@ -16,31 +16,70 @@ describe("oauth2redirect", () => { const token = "ey123.ey456"; + const createOauthSession = () => { + return request(app) + .get("/") + .redirects(0) + .then((res) => { + const redirectLocation = res.headers.location; + const stateMatch = /[?&]state=([^&]+)/.exec(redirectLocation); + return { + sessionCookies: res.get("Set-Cookie"), + state: stateMatch && decodeURIComponent(stateMatch[1]), + }; + }); + }; + describe("when OAuth2 code is present", () => { it("should set an accessToken cookie and redirect to /", () => { idamServiceMock.resolveExchangeCode(token); - return request(app) - .get("/oauth2redirect?code=abc123") - .then((res) => { - const cookies = res.get("Set-Cookie").map((_) => cookie.parse(_)); - expect(cookies.some((c) => c[`${COOKIE_ACCESS_TOKEN}`] === token)).to.be.true; - expect(res.headers.location).to.equal("/"); + return createOauthSession() + .then(({ sessionCookies, state }) => { + return request(app) + .get(`/oauth2redirect?code=abc123&state=${encodeURIComponent(state)}`) + .set("Cookie", sessionCookies) + .then((res) => { + const cookies = res.get("Set-Cookie").map((_) => cookie.parse(_)); + expect(cookies.some((c) => c[`${COOKIE_ACCESS_TOKEN}`] === token)).to.be.true; + expect(res.headers.location).to.equal("/"); + }); }); }); }); - describe("when OAuth2 code is not present", () => { - it("should not set an accessToken cookie", () => { + describe("when OAuth2 state is not present", () => { + it("should reject the callback and not set an accessToken cookie", () => { idamServiceMock.resolveExchangeCode(token); return request(app) .get("/oauth2redirect") .then((res) => { - const cookies = res.get("Set-Cookie").map((_) => cookie.parse(_)); + const setCookie = res.get("Set-Cookie") || []; + const cookies = setCookie.map((_) => cookie.parse(_)); expect(cookies.some((c) => c[`${COOKIE_ACCESS_TOKEN}`] === token)).to.be.false; - expect(res.status).to.equal(500); - expect(res.text).includes("Error: Unable to obtain access token - no OAuth2 code provided"); + expect(res.status).to.equal(400); + expect(res.text).includes("Invalid state parameter - possible CSRF attack"); + }); + }); + }); + + describe("when OAuth2 code is not present but state is valid", () => { + it("should return 400 and not set an accessToken cookie", () => { + idamServiceMock.resolveExchangeCode(token); + + return createOauthSession() + .then(({ sessionCookies, state }) => { + return request(app) + .get(`/oauth2redirect?state=${encodeURIComponent(state)}`) + .set("Cookie", sessionCookies) + .then((res) => { + const setCookie = res.get("Set-Cookie") || []; + const cookies = setCookie.map((_) => cookie.parse(_)); + expect(cookies.some((c) => c[`${COOKIE_ACCESS_TOKEN}`] === token)).to.be.false; + expect(res.status).to.equal(400); + expect(res.text).includes("Unable to obtain access token - no OAuth2 code provided"); + }); }); }); }); @@ -65,7 +104,8 @@ describe("oauth2redirect", () => { }; req = sinonExpressMock.mockReq(); - req.query = {code: "code", redirect_uri: "https://localhost:5000"}; + req.query = {code: "code", redirect_uri: "https://localhost:5000", state: "oauth-state"}; + req.session = {oauthState: "oauth-state"}; res = sinonExpressMock.mockRes(); next = sinon.stub(); accessTokenRequest = sinon.stub(); diff --git a/src/test/user/auth-checker-user-only-filter.spec.ts b/src/test/user/auth-checker-user-only-filter.spec.ts index 0eca941a5..57b145952 100644 --- a/src/test/user/auth-checker-user-only-filter.spec.ts +++ b/src/test/user/auth-checker-user-only-filter.spec.ts @@ -26,6 +26,7 @@ describe("authCheckerUserOnlyFilter", () => { req = { get: sinon.stub(), protocol: "http", + session: {}, }; req.get.withArgs("host").returns("localhost"); res = {}; @@ -88,16 +89,21 @@ describe("authCheckerUserOnlyFilter", () => { it("should redirect to the IdAM login URL", (done) => { res = { redirect: (code, url) => { - assert.equal(code, 302); - assert.equal(url, completeUrl); - done(); + try { + assert.equal(code, 302); + expect(url.startsWith(`${completeUrl}&state=`)).to.equal(true); + expect(req.session.oauthState).to.match(/^[a-f0-9]{64}$/); + expect(url).to.contain(`state=${encodeURIComponent(req.session.oauthState)}`); + done(); + } catch (e) { + done(e); + } }, }; filter(req, res, (err) => { try { - expect(err).to.equal(error); - expect(res.redirect).to.be.calledWith(302, completeUrl); + expect(err).to.be.undefined; done(); } catch (e) { done(e);