From 5fa831ce551eda0fb14ac4d05a372b8450048e07 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Tue, 6 Sep 2022 13:12:50 +0200 Subject: [PATCH 1/4] Add optional SameSite attribute to CookieConfigurer and return from defaultCookieConfigurer for secure contexts. Signed-off-by: Marcus Eide --- .changeset/kind-penguins-report.md | 5 ++++ plugins/auth-backend/api-report.md | 5 ++-- .../src/lib/oauth/OAuthAdapter.test.ts | 15 ++++++++--- .../src/lib/oauth/OAuthAdapter.ts | 16 ++++-------- .../src/lib/oauth/helpers.test.ts | 25 +++++++++++++++++++ plugins/auth-backend/src/lib/oauth/helpers.ts | 3 ++- plugins/auth-backend/src/providers/types.ts | 7 +++++- 7 files changed, 57 insertions(+), 19 deletions(-) create mode 100644 .changeset/kind-penguins-report.md diff --git a/.changeset/kind-penguins-report.md b/.changeset/kind-penguins-report.md new file mode 100644 index 0000000000..6596f727d2 --- /dev/null +++ b/.changeset/kind-penguins-report.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend': patch +--- + +Allow CookieConfigurer to optionally return the SameSite cookie attribute. Return `SameSite=None` in `defaultCookieConfigurer` for secure contexts to allow cookies to be included in third-party requests. diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index d47abeee83..ed3561d37c 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -187,6 +187,7 @@ export type CookieConfigurer = (ctx: { domain: string; path: string; secure: boolean; + sameSite?: 'none' | 'lax' | 'strict'; }; // @public @@ -280,11 +281,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { // @public (undocumented) export type OAuthAdapterOptions = { providerId: string; - secure: boolean; persistScopes?: boolean; - cookieDomain: string; - cookiePath: string; appOrigin: string; + cookieConfig: ReturnType; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; }; diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index 9dcd76e92e..a61554b600 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -18,6 +18,7 @@ import express from 'express'; import { THOUSAND_DAYS_MS, TEN_MINUTES_MS, OAuthAdapter } from './OAuthAdapter'; import { encodeState } from './helpers'; import { OAuthHandlers, OAuthState } from './types'; +import { CookieConfigurer } from '../../providers/types'; const mockResponseData = { providerInfo: { @@ -57,12 +58,17 @@ describe('OAuthAdapter', () => { } } const providerInstance = new MyAuthProvider(); + const cookieConfig = { + domain: 'example.com', + path: '/auth/test-provider', + secure: false, + sameSite: 'lax', + } as ReturnType; + const oAuthProviderOptions = { providerId: 'test-provider', - secure: false, appOrigin: 'http://localhost:3000', - cookieDomain: 'example.com', - cookiePath: '/auth/test-provider', + cookieConfig, tokenIssuer: { issueToken: async () => 'my-id-token', listPublicKeys: async () => ({ keys: [] }), @@ -136,6 +142,8 @@ describe('OAuthAdapter', () => { expect.objectContaining({ path: '/auth/test-provider', maxAge: THOUSAND_DAYS_MS, + secure: false, + sameSite: 'lax', }), ); }); @@ -331,6 +339,7 @@ describe('OAuthAdapter', () => { domain: 'authdomain.org', path: '/auth/test-provider/handler', secure: true, + sameSite: 'none', }), ); }); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 6caa9fed12..c021104010 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -24,6 +24,7 @@ import { import { AuthProviderRouteHandlers, AuthProviderConfig, + CookieConfigurer, } from '../../providers/types'; import { AuthenticationError, @@ -47,11 +48,9 @@ export const TEN_MINUTES_MS = 600 * 1000; /** @public */ export type OAuthAdapterOptions = { providerId: string; - secure: boolean; persistScopes?: boolean; - cookieDomain: string; - cookiePath: string; appOrigin: string; + cookieConfig: ReturnType; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; }; @@ -78,9 +77,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { return new OAuthAdapter(handlers, { ...options, appOrigin, - cookieDomain: cookieConfig.domain, - cookiePath: cookieConfig.path, - secure: cookieConfig.secure, + cookieConfig, isOriginAllowed: config.isOriginAllowed, }); } @@ -93,10 +90,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { ) { this.baseCookieOptions = { httpOnly: true, - sameSite: 'lax', - secure: this.options.secure, - path: this.options.cookiePath, - domain: this.options.cookieDomain, + ...this.options.cookieConfig, }; } @@ -265,7 +259,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { res.cookie(`${this.options.providerId}-nonce`, nonce, { maxAge: TEN_MINUTES_MS, ...this.baseCookieOptions, - path: `${this.options.cookiePath}/handler`, + path: `${this.options.cookieConfig.path}/handler`, }); }; diff --git a/plugins/auth-backend/src/lib/oauth/helpers.test.ts b/plugins/auth-backend/src/lib/oauth/helpers.test.ts index b3052c93e1..2202fa60ea 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.test.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.test.ts @@ -150,5 +150,30 @@ describe('OAuthProvider Utils', () => { secure: true, }); }); + + it('should set sameSite to none for https', () => { + expect( + defaultCookieConfigurer({ + baseUrl: '', + providerId: 'test-provider', + callbackUrl: 'https://domain.org/auth', + }), + ).toMatchObject({ + sameSite: 'none', + secure: true, + }); + }); + it('should set sameSite to lax for http', () => { + expect( + defaultCookieConfigurer({ + baseUrl: '', + providerId: 'test-provider', + callbackUrl: 'http://domain.org/auth', + }), + ).toMatchObject({ + sameSite: 'lax', + secure: false, + }); + }); }); }); diff --git a/plugins/auth-backend/src/lib/oauth/helpers.ts b/plugins/auth-backend/src/lib/oauth/helpers.ts index 63d5d62ef9..b18db59ca7 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.ts @@ -68,6 +68,7 @@ export const defaultCookieConfigurer: CookieConfigurer = ({ }) => { const { hostname: domain, pathname, protocol } = new URL(callbackUrl); const secure = protocol === 'https:'; + const sameSite = secure ? 'none' : 'lax'; // If the provider supports callbackUrls, the pathname will // contain the complete path to the frame handler so we need @@ -76,5 +77,5 @@ export const defaultCookieConfigurer: CookieConfigurer = ({ ? pathname.slice(0, -'/handler/frame'.length) : `${pathname}/${providerId}`; - return { domain, path, secure }; + return { domain, path, secure, sameSite }; }; diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts index 574afade79..f22e6b0492 100644 --- a/plugins/auth-backend/src/providers/types.ts +++ b/plugins/auth-backend/src/providers/types.ts @@ -100,7 +100,12 @@ export type CookieConfigurer = (ctx: { baseUrl: string; /** The configured callback URL of the auth provider */ callbackUrl: string; -}) => { domain: string; path: string; secure: boolean }; +}) => { + domain: string; + path: string; + secure: boolean; + sameSite?: 'none' | 'lax' | 'strict'; +}; /** @public */ export type AuthProviderConfig = { From 12943d1ade5a7658a05148d7a8a676ce98a43c40 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Fri, 9 Sep 2022 11:16:04 +0200 Subject: [PATCH 2/4] Calculate SameSite attribute more carefully, defaulting to lax in most cases Signed-off-by: Marcus Eide --- .changeset/kind-penguins-report.md | 7 +- plugins/auth-backend/api-report.md | 3 + .../src/lib/oauth/OAuthAdapter.test.ts | 121 +++++++++++++++++- .../src/lib/oauth/OAuthAdapter.ts | 25 +++- .../src/lib/oauth/helpers.test.ts | 40 +++++- plugins/auth-backend/src/lib/oauth/helpers.ts | 11 +- plugins/auth-backend/src/providers/types.ts | 2 + 7 files changed, 193 insertions(+), 16 deletions(-) diff --git a/.changeset/kind-penguins-report.md b/.changeset/kind-penguins-report.md index 6596f727d2..9f2401491e 100644 --- a/.changeset/kind-penguins-report.md +++ b/.changeset/kind-penguins-report.md @@ -1,5 +1,8 @@ --- -'@backstage/plugin-auth-backend': patch +'@backstage/plugin-auth-backend': minor --- -Allow CookieConfigurer to optionally return the SameSite cookie attribute. Return `SameSite=None` in `defaultCookieConfigurer` for secure contexts to allow cookies to be included in third-party requests. +CookieConfigurer can optionally return the `SameSite` cookie attribute. +CookieConfigurer now requires an additional argument `appOrigin` - the origin URL of the app - which is used to calculate the `SameSite` attribute. +defaultCookieConfigurer returns the `SameSite` attribute which defaults to `Lax`. In cases where an auth-backend is running on a different domain than the App, `SameSite=None` is used - but only for secure contexts. This is so that cookies can be included in third-party requests. +OAuthAdapterOptions has been modified to require additional arguments, `baseUrl`, `cookieConfigurer` and `cookieConfig`. diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index ed3561d37c..8cb7e35e9b 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -183,6 +183,7 @@ export type CookieConfigurer = (ctx: { providerId: string; baseUrl: string; callbackUrl: string; + appOrigin: string; }) => { domain: string; path: string; @@ -283,7 +284,9 @@ export type OAuthAdapterOptions = { providerId: string; persistScopes?: boolean; appOrigin: string; + baseUrl: string; cookieConfig: ReturnType; + cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; }; diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index a61554b600..d59a712205 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -37,6 +37,10 @@ const mockResponseData = { }; describe('OAuthAdapter', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + class MyAuthProvider implements OAuthHandlers { async start() { return { @@ -58,17 +62,19 @@ describe('OAuthAdapter', () => { } } const providerInstance = new MyAuthProvider(); - const cookieConfig = { + const mockCookieConfig: ReturnType = { domain: 'example.com', path: '/auth/test-provider', secure: false, - sameSite: 'lax', - } as ReturnType; + }; + const mockCookieConfigurer = jest.fn().mockReturnValue(mockCookieConfig); const oAuthProviderOptions = { providerId: 'test-provider', appOrigin: 'http://localhost:3000', - cookieConfig, + cookieConfig: mockCookieConfig, + cookieConfigurer: mockCookieConfigurer, + baseUrl: 'http://example.com:7007', tokenIssuer: { issueToken: async () => 'my-id-token', listPublicKeys: async () => ({ keys: [] }), @@ -135,11 +141,13 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.frameHandler(mockRequest, mockResponse); + expect(mockCookieConfigurer).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledWith( expect.stringContaining('test-provider-refresh-token'), expect.stringContaining('token'), expect.objectContaining({ + httpOnly: true, path: '/auth/test-provider', maxAge: THOUSAND_DAYS_MS, secure: false, @@ -210,6 +218,7 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.frameHandler(mockHandleReq, mockHandleRes); + expect(mockCookieConfigurer).toHaveBeenCalledTimes(1); expect(mockHandleRes.cookie).toHaveBeenCalledTimes(1); expect(mockHandleRes.cookie).toHaveBeenCalledWith( 'test-provider-granted-scope', @@ -303,7 +312,7 @@ describe('OAuthAdapter', () => { }); }); - it('sets the correct cookie configuration using a callbackUrl', async () => { + it('sets the correct cookie configuration using an unsecure callbackUrl', async () => { const config = { baseUrl: 'http://domain.org/auth', appUrl: 'http://domain.org', @@ -312,7 +321,7 @@ describe('OAuthAdapter', () => { const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { ...oAuthProviderOptions, - callbackUrl: 'https://authdomain.org/auth/test-provider/handler/frame', + callbackUrl: 'http://authdomain.org/auth/test-provider/handler/frame', }); const mockRequest = { @@ -330,14 +339,112 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.start(mockRequest, mockResponse); - + expect(mockCookieConfigurer).not.toHaveBeenCalled(); expect(mockResponse.cookie).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledWith( `${oAuthProviderOptions.providerId}-nonce`, expect.any(String), expect.objectContaining({ + httpOnly: true, domain: 'authdomain.org', path: '/auth/test-provider/handler', + secure: false, + sameSite: 'lax', + }), + ); + }); + + it('sets the correct cookie configuration using an secure callbackUrl', async () => { + const config = { + baseUrl: 'https://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://authdomain.org/auth/test-provider/handler/frame', + }); + + const state = { + nonce: 'nonce', + env: 'development', + }; + + const mockRequest = { + cookies: { + 'test-provider-nonce': 'nonce', + }, + query: { + state: encodeState(state), + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.frameHandler(mockRequest, mockResponse); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + expect.stringContaining('test-provider-refresh-token'), + expect.stringContaining('token'), + expect.objectContaining({ + httpOnly: true, + domain: 'authdomain.org', + path: '/auth/test-provider', + secure: true, + sameSite: 'none', + }), + ); + }); + + it('sets the correct cookie configuration using state', async () => { + const config = { + baseUrl: 'https://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => true, + }; + + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://domain.org/auth/test-provider/handler/frame', + }); + + const state = { + nonce: 'nonce', + env: 'development', + origin: 'http://other.domain', + }; + + const mockRequest = { + cookies: { + 'test-provider-nonce': 'nonce', + }, + query: { + state: encodeState(state), + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.frameHandler(mockRequest, mockResponse); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + expect.stringContaining('test-provider-refresh-token'), + expect.stringContaining('token'), + expect.objectContaining({ + httpOnly: true, + domain: 'domain.org', + path: '/auth/test-provider', secure: true, sameSite: 'none', }), diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index c021104010..64dc4f70e3 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -50,7 +50,9 @@ export type OAuthAdapterOptions = { providerId: string; persistScopes?: boolean; appOrigin: string; + baseUrl: string; cookieConfig: ReturnType; + cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; }; @@ -65,24 +67,28 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { 'providerId' | 'persistScopes' | 'callbackUrl' >, ): OAuthAdapter { - const { origin: appOrigin } = new URL(config.appUrl); + const { appUrl, baseUrl, isOriginAllowed } = config; + const { origin: appOrigin } = new URL(appUrl); const cookieConfigurer = config.cookieConfigurer ?? defaultCookieConfigurer; const cookieConfig = cookieConfigurer({ providerId: options.providerId, baseUrl: config.baseUrl, callbackUrl: options.callbackUrl, + appOrigin, }); return new OAuthAdapter(handlers, { ...options, appOrigin, + baseUrl, cookieConfig, - isOriginAllowed: config.isOriginAllowed, + cookieConfigurer, + isOriginAllowed, }); } - private readonly baseCookieOptions: CookieOptions; + private baseCookieOptions: CookieOptions; constructor( private readonly handlers: OAuthHandlers, @@ -90,6 +96,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { ) { this.baseCookieOptions = { httpOnly: true, + sameSite: 'lax', ...this.options.cookieConfig, }; } @@ -147,6 +154,18 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { } } + // Update cookie options to reflect any changes for the appOrigin + const updatedCookieOptions = this.options.cookieConfigurer({ + providerId: this.options.providerId, + baseUrl: this.options.baseUrl, + callbackUrl: this.options.callbackUrl, + appOrigin, + }); + this.baseCookieOptions = { + ...this.baseCookieOptions, + ...updatedCookieOptions, + }; + // verify nonce cookie and state cookie on callback verifyNonce(req, this.options.providerId); diff --git a/plugins/auth-backend/src/lib/oauth/helpers.test.ts b/plugins/auth-backend/src/lib/oauth/helpers.test.ts index 2202fa60ea..58a3dcab05 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.test.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.test.ts @@ -117,6 +117,7 @@ describe('OAuthProvider Utils', () => { baseUrl: '', providerId: 'test-provider', callbackUrl: 'http://domain.org/auth', + appOrigin: 'http://domain.org', }), ).toMatchObject({ domain: 'domain.org', @@ -131,6 +132,7 @@ describe('OAuthProvider Utils', () => { baseUrl: '', providerId: 'test-provider', callbackUrl: 'http://domain.org/auth/test-provider/handler/frame', + appOrigin: 'http://domain.org', }), ).toMatchObject({ domain: 'domain.org', @@ -145,35 +147,67 @@ describe('OAuthProvider Utils', () => { baseUrl: '', providerId: 'test-provider', callbackUrl: 'https://domain.org/auth', + appOrigin: 'http://domain.org', }), ).toMatchObject({ secure: true, }); }); - it('should set sameSite to none for https', () => { + it('should set sameSite to lax for https on the same domain', () => { expect( defaultCookieConfigurer({ baseUrl: '', providerId: 'test-provider', callbackUrl: 'https://domain.org/auth', + appOrigin: 'http://domain.org', }), ).toMatchObject({ - sameSite: 'none', + sameSite: 'lax', secure: true, }); }); - it('should set sameSite to lax for http', () => { + + it('should set sameSite to lax for http on the same domain', () => { expect( defaultCookieConfigurer({ baseUrl: '', providerId: 'test-provider', callbackUrl: 'http://domain.org/auth', + appOrigin: 'http://domain.org', }), ).toMatchObject({ sameSite: 'lax', secure: false, }); }); + + it('should set sameSite to lax if not secure and on different domains', () => { + expect( + defaultCookieConfigurer({ + baseUrl: '', + providerId: 'test-provider', + callbackUrl: 'http://authdomain.org/auth', + appOrigin: 'http://domain.org', + }), + ).toMatchObject({ + sameSite: 'lax', + secure: false, + }); + }); + + it('should set sameSite to none if secure and on different domains', () => { + expect( + defaultCookieConfigurer({ + baseUrl: '', + providerId: 'test-provider', + callbackUrl: 'https://authdomain.org/auth', + appOrigin: 'http://domain.org', + }), + ).toMatchObject({ + sameSite: 'none', + secure: true, + }); + }); }); }); diff --git a/plugins/auth-backend/src/lib/oauth/helpers.ts b/plugins/auth-backend/src/lib/oauth/helpers.ts index b18db59ca7..2b4e50b477 100644 --- a/plugins/auth-backend/src/lib/oauth/helpers.ts +++ b/plugins/auth-backend/src/lib/oauth/helpers.ts @@ -65,10 +65,19 @@ export const verifyNonce = (req: express.Request, providerId: string) => { export const defaultCookieConfigurer: CookieConfigurer = ({ callbackUrl, providerId, + appOrigin, }) => { const { hostname: domain, pathname, protocol } = new URL(callbackUrl); const secure = protocol === 'https:'; - const sameSite = secure ? 'none' : 'lax'; + + // For situations where the auth-backend is running on a + // different domain than the app, we set the SameSite attribute + // to 'none' to allow third-party access to the cookie, but + // only if it's in a secure context (https). + let sameSite: ReturnType['sameSite'] = 'lax'; + if (new URL(appOrigin).hostname !== domain && secure) { + sameSite = 'none'; + } // If the provider supports callbackUrls, the pathname will // contain the complete path to the frame handler so we need diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts index f22e6b0492..1459cff484 100644 --- a/plugins/auth-backend/src/providers/types.ts +++ b/plugins/auth-backend/src/providers/types.ts @@ -100,6 +100,8 @@ export type CookieConfigurer = (ctx: { baseUrl: string; /** The configured callback URL of the auth provider */ callbackUrl: string; + /** The origin URL of the app */ + appOrigin: string; }) => { domain: string; path: string; From 04c50e297d8a0e4fb0ad78fe9f5912ce1794f543 Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Fri, 16 Sep 2022 09:57:44 +0200 Subject: [PATCH 3/4] Resolve cookie configuration per request Signed-off-by: Marcus Eide --- .changeset/kind-penguins-report.md | 4 +- plugins/auth-backend/api-report.md | 1 - .../src/lib/oauth/OAuthAdapter.test.ts | 164 ++++++++++++++++-- .../src/lib/oauth/OAuthAdapter.ts | 74 ++++---- 4 files changed, 200 insertions(+), 43 deletions(-) diff --git a/.changeset/kind-penguins-report.md b/.changeset/kind-penguins-report.md index 9f2401491e..4023ea5976 100644 --- a/.changeset/kind-penguins-report.md +++ b/.changeset/kind-penguins-report.md @@ -5,4 +5,6 @@ CookieConfigurer can optionally return the `SameSite` cookie attribute. CookieConfigurer now requires an additional argument `appOrigin` - the origin URL of the app - which is used to calculate the `SameSite` attribute. defaultCookieConfigurer returns the `SameSite` attribute which defaults to `Lax`. In cases where an auth-backend is running on a different domain than the App, `SameSite=None` is used - but only for secure contexts. This is so that cookies can be included in third-party requests. -OAuthAdapterOptions has been modified to require additional arguments, `baseUrl`, `cookieConfigurer` and `cookieConfig`. + +OAuthAdapterOptions has been modified to require additional arguments, `baseUrl`, and `cookieConfigurer`. +OAuthAdapter now resolves cookie configuration using its supplied CookieConfigurer for each request to make sure that the proper attributes always are set. diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index 8cb7e35e9b..f4e8415266 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -285,7 +285,6 @@ export type OAuthAdapterOptions = { persistScopes?: boolean; appOrigin: string; baseUrl: string; - cookieConfig: ReturnType; cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index d59a712205..6521519808 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -63,7 +63,7 @@ describe('OAuthAdapter', () => { } const providerInstance = new MyAuthProvider(); const mockCookieConfig: ReturnType = { - domain: 'example.com', + domain: 'domain.org', path: '/auth/test-provider', secure: false, }; @@ -72,15 +72,14 @@ describe('OAuthAdapter', () => { const oAuthProviderOptions = { providerId: 'test-provider', appOrigin: 'http://localhost:3000', - cookieConfig: mockCookieConfig, + baseUrl: 'http://domain.org/auth', cookieConfigurer: mockCookieConfigurer, - baseUrl: 'http://example.com:7007', tokenIssuer: { issueToken: async () => 'my-id-token', listPublicKeys: async () => ({ keys: [] }), }, isOriginAllowed: () => false, - callbackUrl: 'http://example.com:7007/auth/test-provider/frame/handler', + callbackUrl: 'http://domain.org/auth/test-provider/handler/frame', }; it('sets the correct headers in start', async () => { @@ -108,7 +107,14 @@ describe('OAuthAdapter', () => { expect(mockResponse.cookie).toHaveBeenCalledWith( `${oAuthProviderOptions.providerId}-nonce`, expect.any(String), - expect.objectContaining({ maxAge: TEN_MINUTES_MS }), + expect.objectContaining({ + httpOnly: true, + path: '/auth/test-provider/handler', + maxAge: TEN_MINUTES_MS, + domain: 'domain.org', + sameSite: 'lax', + secure: false, + }), ); // redirect checks expect(mockResponse.setHeader).toHaveBeenCalledTimes(2); @@ -150,6 +156,7 @@ describe('OAuthAdapter', () => { httpOnly: true, path: '/auth/test-provider', maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', secure: false, sameSite: 'lax', }), @@ -218,14 +225,17 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.frameHandler(mockHandleReq, mockHandleRes); - expect(mockCookieConfigurer).toHaveBeenCalledTimes(1); expect(mockHandleRes.cookie).toHaveBeenCalledTimes(1); expect(mockHandleRes.cookie).toHaveBeenCalledWith( 'test-provider-granted-scope', 'user', expect.objectContaining({ + httpOnly: true, path: '/auth/test-provider', maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', + secure: false, + sameSite: 'lax', }), ); @@ -260,6 +270,7 @@ describe('OAuthAdapter', () => { const mockRequest = { header: () => 'XMLHttpRequest', + get: jest.fn(), } as unknown as express.Request; const mockResponse = { @@ -269,6 +280,7 @@ describe('OAuthAdapter', () => { } as unknown as express.Response; await oauthProvider.logout(mockRequest, mockResponse); + expect(mockRequest.get).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledWith( expect.stringContaining('test-provider-refresh-token'), @@ -312,7 +324,46 @@ describe('OAuthAdapter', () => { }); }); - it('sets the correct cookie configuration using an unsecure callbackUrl', async () => { + it('sets new access-token when old cookie exists', async () => { + const oauthProvider = new OAuthAdapter(providerInstance, { + ...oAuthProviderOptions, + isOriginAllowed: () => false, + }); + + const mockRequest = { + header: () => 'XMLHttpRequest', + cookies: { + 'test-provider-refresh-token': 'old-token', + }, + query: {}, + get: jest.fn(), + } as unknown as express.Request; + + const mockResponse = { + json: jest.fn().mockReturnThis(), + status: jest.fn().mockReturnThis(), + cookie: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.refresh(mockRequest, mockResponse); + expect(mockRequest.get).toHaveBeenCalledTimes(1); + expect(mockCookieConfigurer).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + 'test-provider-refresh-token', + 'token', + expect.objectContaining({ + httpOnly: true, + path: '/auth/test-provider', + maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', + secure: false, + sameSite: 'lax', + }), + ); + }); + + it('sets the correct nonce cookie configuration', async () => { const config = { baseUrl: 'http://domain.org/auth', appUrl: 'http://domain.org', @@ -321,7 +372,6 @@ describe('OAuthAdapter', () => { const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { ...oAuthProviderOptions, - callbackUrl: 'http://authdomain.org/auth/test-provider/handler/frame', }); const mockRequest = { @@ -346,7 +396,8 @@ describe('OAuthAdapter', () => { expect.any(String), expect.objectContaining({ httpOnly: true, - domain: 'authdomain.org', + domain: 'domain.org', + maxAge: TEN_MINUTES_MS, path: '/auth/test-provider/handler', secure: false, sameSite: 'lax', @@ -361,6 +412,54 @@ describe('OAuthAdapter', () => { isOriginAllowed: () => false, }; + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://domain.org/auth/test-provider/handler/frame', + }); + + const state = { + nonce: 'nonce', + env: 'development', + }; + + const mockRequest = { + cookies: { + 'test-provider-nonce': 'nonce', + }, + query: { + state: encodeState(state), + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.frameHandler(mockRequest, mockResponse); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + expect.stringContaining('test-provider-refresh-token'), + expect.stringContaining('token'), + expect.objectContaining({ + httpOnly: true, + domain: 'domain.org', + path: '/auth/test-provider', + secure: true, + sameSite: 'lax', + }), + ); + }); + + it('sets the correct cookie configuration when on different domains and secure', async () => { + const config = { + baseUrl: 'https://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { ...oAuthProviderOptions, callbackUrl: 'https://authdomain.org/auth/test-provider/handler/frame', @@ -402,7 +501,7 @@ describe('OAuthAdapter', () => { ); }); - it('sets the correct cookie configuration using state', async () => { + it('sets the correct cookie configuration using origin from state', async () => { const config = { baseUrl: 'https://domain.org/auth', appUrl: 'http://domain.org', @@ -450,4 +549,49 @@ describe('OAuthAdapter', () => { }), ); }); + + it('sets the correct cookie configuration using origin from header', async () => { + const config = { + baseUrl: 'https://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://domain.org/auth/test-provider/handler/frame', + }); + + const mockRequest = { + header: () => 'XMLHttpRequest', + cookies: { + 'test-provider-refresh-token': 'old-token', + }, + query: {}, + get: jest.fn().mockReturnValue('http://other.domain'), + } as unknown as express.Request; + + const mockResponse = { + json: jest.fn().mockReturnThis(), + status: jest.fn().mockReturnThis(), + cookie: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.refresh(mockRequest, mockResponse); + expect(mockRequest.get).toHaveBeenCalledTimes(1); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + 'test-provider-refresh-token', + 'token', + expect.objectContaining({ + httpOnly: true, + path: '/auth/test-provider', + maxAge: THOUSAND_DAYS_MS, + domain: 'domain.org', + secure: true, + sameSite: 'none', + }), + ); + }); }); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 64dc4f70e3..43fcc5e20b 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -51,7 +51,6 @@ export type OAuthAdapterOptions = { persistScopes?: boolean; appOrigin: string; baseUrl: string; - cookieConfig: ReturnType; cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; @@ -71,24 +70,17 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { const { origin: appOrigin } = new URL(appUrl); const cookieConfigurer = config.cookieConfigurer ?? defaultCookieConfigurer; - const cookieConfig = cookieConfigurer({ - providerId: options.providerId, - baseUrl: config.baseUrl, - callbackUrl: options.callbackUrl, - appOrigin, - }); return new OAuthAdapter(handlers, { ...options, appOrigin, baseUrl, - cookieConfig, cookieConfigurer, isOriginAllowed, }); } - private baseCookieOptions: CookieOptions; + private readonly baseCookieOptions: CookieOptions; constructor( private readonly handlers: OAuthHandlers, @@ -97,7 +89,6 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { this.baseCookieOptions = { httpOnly: true, sameSite: 'lax', - ...this.options.cookieConfig, }; } @@ -111,9 +102,11 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { throw new InputError('No env provided in request query parameters'); } + const cookieConfig = this.getCookieConfig(); + const nonce = crypto.randomBytes(16).toString('base64'); // set a nonce cookie before redirecting to oauth provider - this.setNonceCookie(res, nonce); + this.setNonceCookie(res, nonce, cookieConfig); const state: OAuthState = { nonce, env, origin }; @@ -154,33 +147,23 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { } } - // Update cookie options to reflect any changes for the appOrigin - const updatedCookieOptions = this.options.cookieConfigurer({ - providerId: this.options.providerId, - baseUrl: this.options.baseUrl, - callbackUrl: this.options.callbackUrl, - appOrigin, - }); - this.baseCookieOptions = { - ...this.baseCookieOptions, - ...updatedCookieOptions, - }; - // verify nonce cookie and state cookie on callback verifyNonce(req, this.options.providerId); const { response, refreshToken } = await this.handlers.handler(req); + const cookieConfig = this.getCookieConfig(appOrigin); + // Store the scope that we have been granted for this session. This is useful if // the provider does not return granted scopes on refresh or if they are normalized. if (this.options.persistScopes && state.scope) { - this.setGrantedScopeCookie(res, state.scope); + this.setGrantedScopeCookie(res, state.scope, cookieConfig); response.providerInfo.scope = state.scope; } if (refreshToken) { // set new refresh token - this.setRefreshTokenCookie(res, refreshToken); + this.setRefreshTokenCookie(res, refreshToken, cookieConfig); } const identity = await this.populateIdentity(response.backstageIdentity); @@ -208,7 +191,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { } // remove refresh token cookie if it is set - this.removeRefreshTokenCookie(res); + const origin = req.get('origin'); + const cookieConfig = this.getCookieConfig(origin); + this.removeRefreshTokenCookie(res, cookieConfig); res.status(200).end(); } @@ -248,7 +233,9 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { ); if (newRefreshToken && newRefreshToken !== refreshToken) { - this.setRefreshTokenCookie(res, newRefreshToken); + const origin = req.get('origin'); + const cookieConfig = this.getCookieConfig(origin); + this.setRefreshTokenCookie(res, newRefreshToken, cookieConfig); } res.status(200).json({ ...response, backstageIdentity }); @@ -274,18 +261,28 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { return prepareBackstageIdentityResponse(identity); } - private setNonceCookie = (res: express.Response, nonce: string) => { + private setNonceCookie = ( + res: express.Response, + nonce: string, + cookieConfig: ReturnType, + ) => { res.cookie(`${this.options.providerId}-nonce`, nonce, { maxAge: TEN_MINUTES_MS, ...this.baseCookieOptions, - path: `${this.options.cookieConfig.path}/handler`, + ...cookieConfig, + path: `${cookieConfig.path}/handler`, }); }; - private setGrantedScopeCookie = (res: express.Response, scope: string) => { + private setGrantedScopeCookie = ( + res: express.Response, + scope: string, + cookieConfig: ReturnType, + ) => { res.cookie(`${this.options.providerId}-granted-scope`, scope, { maxAge: THOUSAND_DAYS_MS, ...this.baseCookieOptions, + ...cookieConfig, }); }; @@ -296,17 +293,32 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { private setRefreshTokenCookie = ( res: express.Response, refreshToken: string, + cookieConfig: ReturnType, ) => { res.cookie(`${this.options.providerId}-refresh-token`, refreshToken, { maxAge: THOUSAND_DAYS_MS, ...this.baseCookieOptions, + ...cookieConfig, }); }; - private removeRefreshTokenCookie = (res: express.Response) => { + private removeRefreshTokenCookie = ( + res: express.Response, + cookieConfig: ReturnType, + ) => { res.cookie(`${this.options.providerId}-refresh-token`, '', { maxAge: 0, ...this.baseCookieOptions, + ...cookieConfig, + }); + }; + + private getCookieConfig = (origin?: string) => { + return this.options.cookieConfigurer({ + providerId: this.options.providerId, + baseUrl: this.options.baseUrl, + callbackUrl: this.options.callbackUrl, + appOrigin: origin ?? this.options.appOrigin, }); }; } From d0f7bb029175b487b476f403cd088f626822ea4d Mon Sep 17 00:00:00 2001 From: Marcus Eide Date: Tue, 20 Sep 2022 15:16:26 +0200 Subject: [PATCH 4/4] Use origin from request when calculating cookie config in start Signed-off-by: Marcus Eide --- .../src/lib/oauth/OAuthAdapter.test.ts | 45 +++++++++++++++++++ .../src/lib/oauth/OAuthAdapter.ts | 2 +- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index 6521519808..526b05d0ab 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -378,6 +378,7 @@ describe('OAuthAdapter', () => { query: { scope: 'user', env: 'development', + origin: 'http://domain.org', }, } as unknown as express.Request; @@ -405,6 +406,50 @@ describe('OAuthAdapter', () => { ); }); + it('sets the correct nonce cookie configuration using origin from request', async () => { + const config = { + baseUrl: 'http://domain.org/auth', + appUrl: 'http://domain.org', + isOriginAllowed: () => false, + }; + + const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { + ...oAuthProviderOptions, + callbackUrl: 'https://domain.org/auth/test-provider/handler/frame', + }); + + const mockRequest = { + query: { + scope: 'user', + env: 'development', + origin: 'http://other.domain', + }, + } as unknown as express.Request; + + const mockResponse = { + cookie: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + setHeader: jest.fn().mockReturnThis(), + statusCode: jest.fn().mockReturnThis(), + } as unknown as express.Response; + + await oauthProvider.start(mockRequest, mockResponse); + expect(mockCookieConfigurer).not.toHaveBeenCalled(); + expect(mockResponse.cookie).toHaveBeenCalledTimes(1); + expect(mockResponse.cookie).toHaveBeenCalledWith( + `${oAuthProviderOptions.providerId}-nonce`, + expect.any(String), + expect.objectContaining({ + httpOnly: true, + domain: 'domain.org', + maxAge: TEN_MINUTES_MS, + path: '/auth/test-provider/handler', + secure: true, + sameSite: 'none', + }), + ); + }); + it('sets the correct cookie configuration using an secure callbackUrl', async () => { const config = { baseUrl: 'https://domain.org/auth', diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 43fcc5e20b..8f08cfbbd0 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -102,7 +102,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { throw new InputError('No env provided in request query parameters'); } - const cookieConfig = this.getCookieConfig(); + const cookieConfig = this.getCookieConfig(origin); const nonce = crypto.randomBytes(16).toString('base64'); // set a nonce cookie before redirecting to oauth provider