diff --git a/.changeset/sour-bats-press.md b/.changeset/sour-bats-press.md new file mode 100644 index 0000000000..bfcf3985e6 --- /dev/null +++ b/.changeset/sour-bats-press.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-node': patch +--- + +fix flawed cookie removal logic with chunked tokens diff --git a/plugins/auth-node/src/oauth/OAuthCookieManager.ts b/plugins/auth-node/src/oauth/OAuthCookieManager.ts index 49cb188880..afbedd4dba 100644 --- a/plugins/auth-node/src/oauth/OAuthCookieManager.ts +++ b/plugins/auth-node/src/oauth/OAuthCookieManager.ts @@ -152,6 +152,36 @@ export class OAuthCookieManager { }; const req = res.req; let output = res; + + const chunkedFormatExists = OAuthCookieManager.chunkedCookieExists( + req, + name, + ); + + // If using the default cookieConfigurer, delete old cookie with domain + // explicitly set to the callbackUrl's domain (legacy behavior) + if (this.cookieConfigurer === defaultCookieConfigurer) { + const { hostname: domain } = new URL(this.options.callbackUrl); + output = output.cookie(name, '', { + ...this.getRemoveCookieOptions(), + domain: domain, + }); + + if (chunkedFormatExists) { + for (let chunkNumber = 0; ; chunkNumber++) { + const key = OAuthCookieManager.getCookieChunkName(name, chunkNumber); + const exists = !!req.cookies[key]; + if (!exists) { + break; + } + output = output.cookie(key, '', { + ...this.getRemoveCookieOptions(), + domain: domain, + }); + } + } + } + if (val.length > MAX_COOKIE_SIZE_CHARACTERS) { const nonChunkedFormatExists = !!req.cookies[name]; if (nonChunkedFormatExists) { @@ -169,10 +199,6 @@ export class OAuthCookieManager { return output; } - const chunkedFormatExists = OAuthCookieManager.chunkedCookieExists( - req, - name, - ); if (chunkedFormatExists) { for (let chunkNumber = 0; ; chunkNumber++) { const key = OAuthCookieManager.getCookieChunkName(name, chunkNumber); @@ -184,15 +210,6 @@ export class OAuthCookieManager { } } - // If using the default cookieConfigurer, delete old cookie with domain set to the callbackUrl's domain (legacy behavior) - if (this.cookieConfigurer === defaultCookieConfigurer) { - const { hostname: domain } = new URL(this.options.callbackUrl); - output = output.cookie(name, '', { - ...this.getRemoveCookieOptions(), - domain: domain, - }); - } - return output.cookie(name, val, options); } diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts index b5b4b7db2c..df012bcff1 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts @@ -337,6 +337,178 @@ describe('createOAuthRouteHandlers', () => { expect(getGrantedScopesCookie(agent)).toBeUndefined(); }); + it('should clean up old cookies (non-chunked) with domain attribute during migration', async () => { + const agent = request.agent( + wrapInApp(createOAuthRouteHandlers(baseConfig)), + ); + + agent.jar.setCookie( + 'my-provider-nonce=123', + '127.0.0.1', + '/my-provider/handler', + ); + + agent.jar.setCookie( + 'my-provider-refresh-token=old-refresh-token; Domain=127.0.0.1', + '127.0.0.1', + '/my-provider', + ); + + mockAuthenticator.authenticate.mockResolvedValue({ + fullProfile: { id: 'id' } as PassportProfile, + session: mockSession, + }); + + const res = await agent.get('/my-provider/handler/frame').query({ + state: encodeOAuthState({ + env: 'development', + nonce: '123', + } as OAuthState), + }); + + expect(res.status).toBe(200); + + const setCookieHeaders = [res.get('Set-Cookie') ?? []].flat(); + const hasDeleteWithDomain = setCookieHeaders.some( + cookie => + cookie.includes('my-provider-refresh-token=;') && + cookie.includes('Max-Age=0') && + cookie.includes('Domain=127.0.0.1'), + ); + expect(hasDeleteWithDomain).toBe(true); + + expect(getRefreshTokenCookie(agent).value).toBe('refresh-token'); + }); + + it('should clean up old chunked cookies with domain attribute during migration', async () => { + const agent = request.agent( + wrapInApp(createOAuthRouteHandlers(baseConfig)), + ); + + agent.jar.setCookie( + 'my-provider-nonce=123', + '127.0.0.1', + '/my-provider/handler', + ); + + // Simulate old chunked cookies with domain attribute (legacy format) + agent.jar.setCookie( + `my-provider-refresh-token-0=${fiveKilobyteRefreshToken.slice( + 0, + 4000, + )}; Domain=127.0.0.1`, + '/my-provider', + ); + agent.jar.setCookie( + `my-provider-refresh-token-1=${fiveKilobyteRefreshToken.slice( + 4000, + )}; Domain=127.0.0.1`, + '/my-provider', + ); + + mockAuthenticator.authenticate.mockResolvedValue({ + fullProfile: { id: 'id' } as PassportProfile, + session: { + ...mockSession, + refreshToken: fiveKilobyteRefreshToken, + }, + }); + + const res = await agent.get('/my-provider/handler/frame').query({ + state: encodeOAuthState({ + env: 'development', + nonce: '123', + } as OAuthState), + }); + + expect(res.status).toBe(200); + + const setCookieHeaders = [res.get('Set-Cookie') ?? []].flat(); + const hasChunk0DeleteWithDomain = setCookieHeaders.some( + cookie => + cookie.includes('my-provider-refresh-token-0=') && + cookie.includes('Max-Age=0') && + cookie.includes('Domain=127.0.0.1'), + ); + const hasChunk1DeleteWithDomain = setCookieHeaders.some( + cookie => + cookie.includes('my-provider-refresh-token-1=') && + cookie.includes('Max-Age=0') && + cookie.includes('Domain=127.0.0.1'), + ); + + expect(hasChunk0DeleteWithDomain).toBe(true); + expect(hasChunk1DeleteWithDomain).toBe(true); + + expect(getRefreshTokenCookie(agent, 0).value).toBe( + fiveKilobyteRefreshToken.slice(0, 4000), + ); + expect(getRefreshTokenCookie(agent, 1).value).toBe( + fiveKilobyteRefreshToken.slice(4000), + ); + }); + + it('should clean up old chunked cookies with domain when migrating to non-chunked', async () => { + const agent = request.agent( + wrapInApp(createOAuthRouteHandlers(baseConfig)), + ); + + agent.jar.setCookie( + 'my-provider-nonce=123', + '127.0.0.1', + '/my-provider/handler', + ); + + agent.jar.setCookie( + `my-provider-refresh-token-0=${fiveKilobyteRefreshToken.slice( + 0, + 4000, + )}; Domain=127.0.0.1`, + '/my-provider', + ); + agent.jar.setCookie( + `my-provider-refresh-token-1=${fiveKilobyteRefreshToken.slice( + 4000, + )}; Domain=127.0.0.1`, + '/my-provider', + ); + + mockAuthenticator.authenticate.mockResolvedValue({ + fullProfile: { id: 'id' } as PassportProfile, + session: mockSession, + }); + + const res = await agent.get('/my-provider/handler/frame').query({ + state: encodeOAuthState({ + env: 'development', + nonce: '123', + } as OAuthState), + }); + + expect(res.status).toBe(200); + + const setCookieHeaders = [res.get('Set-Cookie') ?? []].flat(); + const hasChunk0DeleteWithDomain = setCookieHeaders.some( + cookie => + cookie.includes('my-provider-refresh-token-0=;') && + cookie.includes('Max-Age=0') && + cookie.includes('Domain=127.0.0.1'), + ); + const hasChunk1DeleteWithDomain = setCookieHeaders.some( + cookie => + cookie.includes('my-provider-refresh-token-1=;') && + cookie.includes('Max-Age=0') && + cookie.includes('Domain=127.0.0.1'), + ); + + expect(hasChunk0DeleteWithDomain).toBe(true); + expect(hasChunk1DeleteWithDomain).toBe(true); + + expect(getRefreshTokenCookie(agent).value).toBe('refresh-token'); + expect(getRefreshTokenCookie(agent, 0)).toBeUndefined(); + expect(getRefreshTokenCookie(agent, 1)).toBeUndefined(); + }); + it('should authenticate with sign-in, profile transform, and persisted scopes', async () => { const agent = request.agent( wrapInApp(