From cd99b40177cc3eef706ab37d21f4351e86934cc6 Mon Sep 17 00:00:00 2001 From: Axel Meinhardt <26243798+ameinhardt@users.noreply.github.com> Date: Mon, 26 Aug 2024 06:37:10 +0800 Subject: [PATCH] feat(oidc-auth): restrict cookie paths (#709) * feat(oidc-auth): restrict cookie paths Signed-off-by: Axel Meinhardt <26243798+ameinhardt@users.noreply.github.com> * chore(oidc-auth): add documentation Signed-off-by: Axel Meinhardt <26243798+ameinhardt@users.noreply.github.com> * fix(oidc-auth): add tests Signed-off-by: Axel Meinhardt <26243798+ameinhardt@users.noreply.github.com> * refactor and format --------- Signed-off-by: Axel Meinhardt <26243798+ameinhardt@users.noreply.github.com> Co-authored-by: Yusuke Wada --- .changeset/kind-dolphins-itch.md | 5 +++ .changeset/odd-islands-act.md | 5 +++ packages/oidc-auth/README.md | 1 + packages/oidc-auth/package.json | 3 ++ packages/oidc-auth/src/index.ts | 47 ++++++++++++++---------- packages/oidc-auth/test/index.test.ts | 52 +++++++++++++++++++++++---- packages/oidc-auth/tsconfig.json | 1 + 7 files changed, 90 insertions(+), 24 deletions(-) create mode 100644 .changeset/kind-dolphins-itch.md create mode 100644 .changeset/odd-islands-act.md diff --git a/.changeset/kind-dolphins-itch.md b/.changeset/kind-dolphins-itch.md new file mode 100644 index 00000000..c3cd14d6 --- /dev/null +++ b/.changeset/kind-dolphins-itch.md @@ -0,0 +1,5 @@ +--- +'@hono/oidc-auth': minor +--- + +Optionally restrict cookie path with new envvar OIDC_COOKIE_PATH diff --git a/.changeset/odd-islands-act.md b/.changeset/odd-islands-act.md new file mode 100644 index 00000000..7197d541 --- /dev/null +++ b/.changeset/odd-islands-act.md @@ -0,0 +1,5 @@ +--- +'@hono/oidc-auth': minor +--- + +Restrict path of callback cookies to pathname of OIDC_REDIRECT_URI diff --git a/packages/oidc-auth/README.md b/packages/oidc-auth/README.md index b5205fab..63d6390e 100644 --- a/packages/oidc-auth/README.md +++ b/packages/oidc-auth/README.md @@ -52,6 +52,7 @@ The middleware requires the following environment variables to be set: | OIDC_CLIENT_ID | The OAuth 2.0 client ID assigned to your application. This ID is used to identify your application to the OIDC provider. | None, must be provided | | OIDC_CLIENT_SECRET | The OAuth 2.0 client secret assigned to your application. This secret is used to authenticate your application to the OIDC provider. | None, must be provided | | OIDC_REDIRECT_URI | The URL to which the OIDC provider should redirect the user after authentication. This URL must be registered as a redirect URI in the OIDC provider. | None, must be provided | +| OIDC_COOKIE_PATH | The path to which the `oidc-auth` cookie is set. Restrict to not send it with every request to your domain | / | ## How to Use diff --git a/packages/oidc-auth/package.json b/packages/oidc-auth/package.json index 9eba75b3..e8b424ef 100644 --- a/packages/oidc-auth/package.json +++ b/packages/oidc-auth/package.json @@ -47,5 +47,8 @@ }, "dependencies": { "oauth4webapi": "^2.6.0" + }, + "engines": { + "node": ">=18.0.0" } } diff --git a/packages/oidc-auth/src/index.ts b/packages/oidc-auth/src/index.ts index 2a330345..bcf67cdd 100644 --- a/packages/oidc-auth/src/index.ts +++ b/packages/oidc-auth/src/index.ts @@ -4,7 +4,7 @@ import type { Context, MiddlewareHandler } from 'hono' import { env } from 'hono/adapter' -import { getCookie, setCookie, deleteCookie } from 'hono/cookie' +import { deleteCookie, getCookie, setCookie } from 'hono/cookie' import { createMiddleware } from 'hono/factory' import { HTTPException } from 'hono/http-exception' import { sign, verify } from 'hono/jwt' @@ -40,6 +40,7 @@ type OidcAuthEnv = { OIDC_CLIENT_ID: string OIDC_CLIENT_SECRET: string OIDC_REDIRECT_URI: string + OIDC_COOKIE_PATH?: string } /** @@ -122,7 +123,7 @@ export const getAuth = async (c: Context): Promise => { try { auth = await verify(session_jwt, env.OIDC_AUTH_SECRET) } catch (e) { - deleteCookie(c, oidcAuthCookieName) + deleteCookie(c, oidcAuthCookieName, { path: env.OIDC_COOKIE_PATH ?? '/' }) return null } if (auth === null || auth.rtkexp === undefined || auth.ssnexp === undefined) { @@ -137,7 +138,7 @@ export const getAuth = async (c: Context): Promise => { if (auth.rtkexp < now) { // Refresh the token if it has expired if (auth.rtk === undefined || auth.rtk === '') { - deleteCookie(c, oidcAuthCookieName) + deleteCookie(c, oidcAuthCookieName, { path: env.OIDC_COOKIE_PATH ?? '/' }) return null } const as = await getAuthorizationServer(c) @@ -146,7 +147,7 @@ export const getAuth = async (c: Context): Promise => { const result = await oauth2.processRefreshTokenResponse(as, client, response) if (oauth2.isOAuth2Error(result)) { // The refresh_token might be expired or revoked - deleteCookie(c, oidcAuthCookieName) + deleteCookie(c, oidcAuthCookieName, { path: env.OIDC_COOKIE_PATH ?? '/' }) return null } auth = await updateAuth(c, auth, result) @@ -186,7 +187,11 @@ const updateAuth = async ( ssnexp: orig?.ssnexp || Math.floor(Date.now() / 1000) + authExpires, } const session_jwt = await sign(updated, env.OIDC_AUTH_SECRET) - setCookie(c, oidcAuthCookieName, session_jwt, { path: '/', httpOnly: true, secure: true }) + setCookie(c, oidcAuthCookieName, session_jwt, { + path: env.OIDC_COOKIE_PATH ?? '/', + httpOnly: true, + secure: true, + }) c.set('oidcAuthJwt', session_jwt) return updated } @@ -198,8 +203,8 @@ export const revokeSession = async (c: Context): Promise => { const session_jwt = getCookie(c, oidcAuthCookieName) if (session_jwt !== undefined) { const env = getOidcAuthEnv(c) - deleteCookie(c, oidcAuthCookieName) - const auth: OidcAuth = await verify(session_jwt, env.OIDC_AUTH_SECRET) + deleteCookie(c, oidcAuthCookieName, { path: env.OIDC_COOKIE_PATH ?? '/' }) + const auth = await verify(session_jwt, env.OIDC_AUTH_SECRET) if (auth.rtk !== undefined && auth.rtk !== '') { // revoke refresh token const as = await getAuthorizationServer(c) @@ -215,7 +220,7 @@ export const revokeSession = async (c: Context): Promise => { } } } - c.set('oidcAuth', undefined) + c.set('oidcAuth', null) } /** @@ -273,7 +278,8 @@ export const processOAuthCallback = async (c: Context) => { // Parses the authorization response and validates the state parameter const state = getCookie(c, 'state') - deleteCookie(c, 'state') + const path = new URL(env.OIDC_REDIRECT_URI).pathname + deleteCookie(c, 'state', { path }) const currentUrl: URL = new URL(c.req.url) const params = oauth2.validateAuthResponse(as, client, currentUrl, state) if (oauth2.isOAuth2Error(params)) { @@ -285,11 +291,11 @@ export const processOAuthCallback = async (c: Context) => { // Exchanges the authorization code for a refresh token const code = c.req.query('code') const nonce = getCookie(c, 'nonce') - deleteCookie(c, 'nonce') + deleteCookie(c, 'nonce', { path }) const code_verifier = getCookie(c, 'code_verifier') - deleteCookie(c, 'code_verifier') + deleteCookie(c, 'code_verifier', { path }) const continue_url = getCookie(c, 'continue') - deleteCookie(c, 'continue') + deleteCookie(c, 'continue', { path }) if (code === undefined || nonce === undefined || code_verifier === undefined) { throw new HTTPException(500, { message: 'Missing required parameters / cookies' }) } @@ -352,20 +358,21 @@ export const oidcAuthMiddleware = (): MiddlewareHandler => { try { const auth = await getAuth(c) if (auth === null) { + const path = new URL(env.OIDC_REDIRECT_URI).pathname // Redirect to IdP for login const state = oauth2.generateRandomState() const nonce = oauth2.generateRandomNonce() const code_verifier = oauth2.generateRandomCodeVerifier() const code_challenge = await oauth2.calculatePKCECodeChallenge(code_verifier) const url = await generateAuthorizationRequestUrl(c, state, nonce, code_challenge) - setCookie(c, 'state', state, { path: '/', httpOnly: true, secure: true }) - setCookie(c, 'nonce', nonce, { path: '/', httpOnly: true, secure: true }) - setCookie(c, 'code_verifier', code_verifier, { path: '/', httpOnly: true, secure: true }) - setCookie(c, 'continue', c.req.url, { path: '/', httpOnly: true, secure: true }) + setCookie(c, 'state', state, { path, httpOnly: true, secure: true }) + setCookie(c, 'nonce', nonce, { path, httpOnly: true, secure: true }) + setCookie(c, 'code_verifier', code_verifier, { path, httpOnly: true, secure: true }) + setCookie(c, 'continue', c.req.url, { path, httpOnly: true, secure: true }) return c.redirect(url) } } catch (e) { - deleteCookie(c, oidcAuthCookieName) + deleteCookie(c, oidcAuthCookieName, { path: env.OIDC_COOKIE_PATH ?? '/' }) throw new HTTPException(500, { message: 'Invalid session' }) } await next() @@ -373,7 +380,11 @@ export const oidcAuthMiddleware = (): MiddlewareHandler => { // Workaround to set the session cookie when the response is returned by the origin server const session_jwt = c.get('oidcAuthJwt') if (session_jwt !== undefined) { - setCookie(c, oidcAuthCookieName, session_jwt, { path: '/', httpOnly: true, secure: true }) + setCookie(c, oidcAuthCookieName, session_jwt, { + path: env.OIDC_COOKIE_PATH ?? '/', + httpOnly: true, + secure: true, + }) } }) } diff --git a/packages/oidc-auth/test/index.test.ts b/packages/oidc-auth/test/index.test.ts index f7ad4206..c77ab35d 100644 --- a/packages/oidc-auth/test/index.test.ts +++ b/packages/oidc-auth/test/index.test.ts @@ -168,11 +168,12 @@ app.all('/*', async (c) => { beforeEach(() => { process.env.OIDC_ISSUER = MOCK_ISSUER - ;(process.env.OIDC_CLIENT_ID = MOCK_CLIENT_ID), - (process.env.OIDC_CLIENT_SECRET = MOCK_CLIENT_SECRET), - (process.env.OIDC_REDIRECT_URI = MOCK_REDIRECT_URI) + process.env.OIDC_CLIENT_ID = MOCK_CLIENT_ID + process.env.OIDC_CLIENT_SECRET = MOCK_CLIENT_SECRET + process.env.OIDC_REDIRECT_URI = MOCK_REDIRECT_URI process.env.OIDC_AUTH_SECRET = MOCK_AUTH_SECRET process.env.OIDC_AUTH_EXPIRES = MOCK_AUTH_EXPIRES + delete process.env.OIDC_COOKIE_PATH }) describe('oidcAuthMiddleware()', () => { test('Should respond with 200 OK if session is active', async () => { @@ -228,7 +229,7 @@ describe('oidcAuthMiddleware()', () => { const res = await app.request(req, {}, {}) expect(res).not.toBeNull() expect(res.status).toBe(302) - expect(res.headers.get('set-cookie')).toMatch('oidc-auth=;') + expect(res.headers.get('set-cookie')).toMatch(new RegExp('oidc-auth=; Max-Age=0; Path=/($|,)')) }) test('Should delete session and redirect to authorization endpoint if the algorithm of the session JWT is invalid', async () => { const req = new Request('http://localhost/', { @@ -238,7 +239,7 @@ describe('oidcAuthMiddleware()', () => { const res = await app.request(req, {}, {}) expect(res).not.toBeNull() expect(res.status).toBe(302) - expect(res.headers.get('set-cookie')).toMatch('oidc-auth=;') + expect(res.headers.get('set-cookie')).toMatch(new RegExp('oidc-auth=; Max-Age=0; Path=/($|,)')) }) }) describe('processOAuthCallback()', () => { @@ -250,8 +251,35 @@ describe('processOAuthCallback()', () => { }, }) const res = await app.request(req, {}, {}) + const path = new URL(MOCK_REDIRECT_URI).pathname expect(res).not.toBeNull() expect(res.status).toBe(302) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`state=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`nonce=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`code_verifier=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`continue=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp('oidc-auth=[^;]+; Path=/; HttpOnly; Secure')) + expect(res.headers.get('location')).toBe('http://localhost/1234') + }) + test('Should restrict the auth cookie to a given path', async () => { + const MOCK_COOKIE_PATH = process.env.OIDC_COOKIE_PATH = '/some/subpath/for/authentication' + process.env.OIDC_REDIRECT_URI = `http://localhost${MOCK_COOKIE_PATH}/callback` + const parentApp = new Hono().route(MOCK_COOKIE_PATH, app) + const path = new URL(process.env.OIDC_REDIRECT_URI).pathname + const req = new Request(`${process.env.OIDC_REDIRECT_URI}?code=1234&state=${MOCK_STATE}`, { + method: 'GET', + headers: { + cookie: `state=${MOCK_STATE}; nonce=${MOCK_NONCE}; code_verifier=1234; continue=http%3A%2F%2Flocalhost%2F1234`, + }, + }) + const res = await parentApp.request(req, {}, {}) + expect(res).not.toBeNull() + expect(res.status).toBe(302) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`state=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`nonce=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`code_verifier=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`continue=; Max-Age=0; Path=${path}($|,)`)) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`oidc-auth=[^;]+; Path=${process.env.OIDC_COOKIE_PATH}; HttpOnly; Secure`)) expect(res.headers.get('location')).toBe('http://localhost/1234') }) test('Should return an error if the state parameter does not match', async () => { @@ -298,6 +326,18 @@ describe('RevokeSession()', () => { const res = await app.request(req, {}, {}) expect(res).not.toBeNull() expect(res.status).toBe(200) - expect(res.headers.get('set-cookie')).toMatch('oidc-auth=;') + expect(res.headers.get('set-cookie')).toMatch(new RegExp('oidc-auth=; Max-Age=0; Path=/($|,)')) + }) + test('Should revoke the session of the given path', async () => { + const MOCK_COOKIE_PATH = process.env.OIDC_COOKIE_PATH = '/some/subpath/for/authentication' + const parentApp = new Hono().route(MOCK_COOKIE_PATH, app) + const req = new Request(`http://localhost${MOCK_COOKIE_PATH}/logout`, { + method: 'GET', + headers: { cookie: `oidc-auth=${MOCK_JWT_ACTIVE_SESSION}` }, + }) + const res = await parentApp.request(req, {}, {}) + expect(res).not.toBeNull() + expect(res.status).toBe(200) + expect(res.headers.get('set-cookie')).toMatch(new RegExp(`oidc-auth=; Max-Age=0; Path=${MOCK_COOKIE_PATH}($|,)`)) }) }) diff --git a/packages/oidc-auth/tsconfig.json b/packages/oidc-auth/tsconfig.json index acfcd843..083746ff 100644 --- a/packages/oidc-auth/tsconfig.json +++ b/packages/oidc-auth/tsconfig.json @@ -1,6 +1,7 @@ { "extends": "../../tsconfig.json", "compilerOptions": { + "module": "ESNext", "rootDir": "./src", "outDir": "./dist", },