From bec84be84f2999499c205c9972ece88845574d52 Mon Sep 17 00:00:00 2001 From: Jessica McInchak Date: Mon, 4 Nov 2024 11:31:13 +0100 Subject: [PATCH] Revert "[api] security hardening for microsoft auth (#3895)" This reverts commit 2a0918891d10d1d54406e43ef9cf5185904bd931. --- api.planx.uk/modules/auth/controller.ts | 1 - api.planx.uk/modules/auth/middleware.ts | 22 ++++-------------- .../modules/auth/strategy/microsoft-oidc.ts | 23 ++++--------------- api.planx.uk/server.ts | 9 ++++---- 4 files changed, 14 insertions(+), 41 deletions(-) diff --git a/api.planx.uk/modules/auth/controller.ts b/api.planx.uk/modules/auth/controller.ts index 694ccb34ec..a03fdc4b17 100644 --- a/api.planx.uk/modules/auth/controller.ts +++ b/api.planx.uk/modules/auth/controller.ts @@ -39,7 +39,6 @@ function setJWTCookie(returnTo: string, res: Response, req: Request) { maxAge: new Date( new Date().setFullYear(new Date().getFullYear() + 1), ).getTime(), - // pizzas rely on staging API for auth (due to static redirect URIs), so we have to allow cross-site sameSite: "none", secure: true, }; diff --git a/api.planx.uk/modules/auth/middleware.ts b/api.planx.uk/modules/auth/middleware.ts index b280b2b135..2443f0dace 100644 --- a/api.planx.uk/modules/auth/middleware.ts +++ b/api.planx.uk/modules/auth/middleware.ts @@ -8,7 +8,7 @@ import type { Authenticator } from "passport"; import type { RequestHandler } from "http-proxy-middleware"; import type { Role } from "@opensystemslab/planx-core/types"; import { AsyncLocalStorage } from "async_hooks"; -import type { CookieOptions, Request } from "express"; +import type { Request } from "express"; export const userContext = new AsyncLocalStorage<{ user: Express.User }>(); @@ -136,27 +136,15 @@ export const getMicrosoftAuthHandler = ( return (req, res, next) => { req.session!.returnTo = req.get("Referrer"); - // generate a nonce to enable us to validate the response from OP (mitigates against CSRF attacks) + // generate a nonce to enable us to validate the response from OP const nonce = generators.nonce(); console.debug(`Generated a nonce: %s`, nonce); + req.session!.nonce = nonce; - // we hash the nonce to avoid sending it plaintext over the wire in our auth request - const hash = crypto.createHash("sha256").update(nonce).digest("hex"); - console.debug(`Hashed nonce: %s`, hash); - - // we store the original nonce in a short-lived, httpOnly but cross-site cookie - const httpOnlyCookieOptions: CookieOptions = { - maxAge: 15 * 60 * 1000, // 15 mins - sameSite: "none", - httpOnly: true, - secure: true, - }; - res.cookie("ms-oidc-nonce", nonce, httpOnlyCookieOptions); - - // @ts-expect-error (method not typed to accept nonce, but it does include it in the request) + // @ts-expect-error (method not typed to accept nonce, but it does pass it to the strategy) return passport.authenticate("microsoft-oidc", { prompt: "select_account", - nonce: hash, + nonce, })(req, res, next); }; }; diff --git a/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts b/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts index 82fcc437b1..cc9c1dc015 100644 --- a/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts +++ b/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts @@ -1,4 +1,3 @@ -import crypto from "crypto"; import type { Client, ClientMetadata, @@ -42,30 +41,18 @@ export const getMicrosoftOidcStrategy = (client: Client): Strategy => { }; const verifyCallback: StrategyVerifyCallbackReq = async ( - req: Http.IncomingMessageWithCookies, + req: Http.IncomingMessageWithSession, tokenSet, done, ): Promise => { // TODO: use tokenSet.state to pass the redirectTo query param through the auth flow - // TODO: validate id_token sig with the public key from the jwks_uri (...v2.0/keys) const claims: IdTokenClaims = tokenSet.claims(); - - // ensure the response is authentic by comparing nonce + const email = claims.email; const returned_nonce = claims.nonce; - if (!req.cookies || !req.cookies["ms-oidc-nonce"]) { - return done(new Error("No nonce found in appropriate cookie")); - } - const original_nonce = req.cookies["ms-oidc-nonce"]; - const hash = crypto.createHash("sha256").update(original_nonce).digest("hex"); - if (returned_nonce != hash) { - return done( - new Error( - "Returned nonce does not match nonce sent with original request", - ), - ); - } - const email = claims.email; + if (returned_nonce != req.session?.nonce) { + return done(new Error("Returned nonce does not match session nonce")); + } if (!email) { return done(new Error("Unable to authenticate without email")); } diff --git a/api.planx.uk/server.ts b/api.planx.uk/server.ts index 39062dcafc..04730a5be0 100644 --- a/api.planx.uk/server.ts +++ b/api.planx.uk/server.ts @@ -112,8 +112,7 @@ assert(process.env.UNIFORM_SUBMISSION_URL); // needed for storing original URL to redirect to in login flow app.use( cookieSession({ - // we don't need session to persist for long - it's only required for auth flow - maxAge: 2 * 60 * 60 * 1000, // 2hrs + maxAge: 24 * 60 * 60 * 100, name: "session", secret: process.env.SESSION_SECRET, }), @@ -199,9 +198,9 @@ declare global { } namespace Http { - interface IncomingMessageWithCookies extends IncomingMessage { - cookies?: { - "ms-oidc-nonce": string; + interface IncomingMessageWithSession extends IncomingMessage { + session?: { + nonce: string; }; } }