cme-969 update oauth2

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");
   }
 };
 

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)}`);
       }
     });
 };

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();

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);