From 3d41e08da84c648d0dbe9926cc5a4f2f6f432a28 Mon Sep 17 00:00:00 2001 From: "IceHe.xyz" Date: Mon, 15 Nov 2021 12:25:58 +0800 Subject: [PATCH] feat(js): add state to loginWithRedirect to prevent csrf attacking (#94) --- packages/client/src/index.test.ts | 83 +++++++++++--- packages/client/src/index.ts | 34 +++--- packages/client/src/parse-callback.test.ts | 2 +- packages/client/src/parse-callback.ts | 8 +- packages/client/src/request-login.test.ts | 113 ++++++++++++++++++-- packages/client/src/request-login.ts | 10 +- packages/client/src/session-manager.test.ts | 5 +- packages/client/src/session-manager.ts | 3 +- packages/client/src/utils.test.ts | 2 +- 9 files changed, 209 insertions(+), 51 deletions(-) diff --git a/packages/client/src/index.test.ts b/packages/client/src/index.test.ts index e76e6b5e2..b3c64f1b8 100644 --- a/packages/client/src/index.test.ts +++ b/packages/client/src/index.test.ts @@ -1,6 +1,6 @@ import { KeyObject } from 'crypto'; -import { SignJWT, generateKeyPair } from 'jose'; +import { generateKeyPair, SignJWT } from 'jose'; import nock from 'nock'; import LogtoClient from '.'; @@ -8,6 +8,13 @@ import { DEFAULT_SCOPE_STRING, SESSION_MANAGER_KEY } from './constants'; import { MemoryStorage } from './storage'; import { verifyIdToken } from './verify-id-token'; +const STATE = 'state1'; + +jest.mock('./generators', () => ({ + ...jest.requireActual('./generators'), + generateState: () => STATE, +})); + jest.mock('./verify-id-token'); const DOMAIN = 'logto.dev'; @@ -16,8 +23,7 @@ const ISSUER = `${BASE_URL}/oidc`; const CLIENT_ID = 'client1'; const SUBJECT = 'subject1'; const REDIRECT_URI = 'http://localhost:3000'; -const REDIRECT_CALLBACK = `${REDIRECT_URI}?code=authorization_code`; -const REDIRECT_CALLBACK_WITH_ERROR = `${REDIRECT_CALLBACK}&error=invalid_request&error_description=code_challenge%20must%20be%20a%20string%20with%20a%20minimum%20length%20of%2043%20characters`; +const CODE = 'code1'; const LOGTO_TOKEN_SET_CACHE_KEY = `LOGTO_TOKEN_SET_CACHE::${ISSUER}::${CLIENT_ID}::${DEFAULT_SCOPE_STRING}`; const discoverResponse = { @@ -146,13 +152,15 @@ describe('LogtoClient', () => { storage, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - expect(storage.getItem('LOGTO_SESSION_MANAGER')).toHaveProperty('redirectUri', REDIRECT_URI); - expect(storage.getItem('LOGTO_SESSION_MANAGER')).toHaveProperty('codeVerifier'); + const sessionPayload = storage.getItem('LOGTO_SESSION_MANAGER'); + expect(sessionPayload).toHaveProperty('redirectUri', REDIRECT_URI); + expect(sessionPayload).toHaveProperty('codeVerifier'); + expect(sessionPayload).toHaveProperty('state'); }); }); describe('isLoginRedirect', () => { - test('url contains code and starts with redirect_uri in session should be true', async () => { + test('url contains code and state and starts with redirect_uri in session should be true', async () => { const storage = new MemoryStorage(); const logto = await LogtoClient.create({ domain: DOMAIN, @@ -160,7 +168,7 @@ describe('LogtoClient', () => { storage, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - expect(logto.isLoginRedirect(REDIRECT_CALLBACK)).toBeTruthy(); + expect(logto.isLoginRedirect(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`)).toBeTruthy(); }); test('empty session should be false', async () => { @@ -170,7 +178,7 @@ describe('LogtoClient', () => { clientId: CLIENT_ID, storage, }); - expect(logto.isLoginRedirect(REDIRECT_CALLBACK)).toBeFalsy(); + expect(logto.isLoginRedirect(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`)).toBeFalsy(); }); test('no code in url should be false', async () => { @@ -181,7 +189,18 @@ describe('LogtoClient', () => { storage, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - expect(logto.isLoginRedirect(REDIRECT_URI)).toBeFalsy(); + expect(logto.isLoginRedirect(`${REDIRECT_URI}?state=${STATE}`)).toBeFalsy(); + }); + + test('no state in url should be false', async () => { + const storage = new MemoryStorage(); + const logto = await LogtoClient.create({ + domain: DOMAIN, + clientId: CLIENT_ID, + storage, + }); + await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); + expect(logto.isLoginRedirect(`${REDIRECT_URI}?code=${CODE}`)).toBeFalsy(); }); test('mismatch uri should be false', async () => { @@ -204,7 +223,9 @@ describe('LogtoClient', () => { clientId: CLIENT_ID, storage, }); - await expect(logto.handleCallback(REDIRECT_CALLBACK)).rejects.toThrowError(); + await expect( + logto.handleCallback(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`) + ).rejects.toThrowError(); }); test('no code in url should fail', async () => { @@ -214,7 +235,19 @@ describe('LogtoClient', () => { clientId: CLIENT_ID, storage, }); - await expect(logto.handleCallback('')).rejects.toThrowError(); + await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); + await expect(logto.handleCallback(`${REDIRECT_URI}?state=${STATE}`)).rejects.toThrowError(); + }); + + test('no state in url should fail', async () => { + const storage = new MemoryStorage(); + const logto = await LogtoClient.create({ + domain: DOMAIN, + clientId: CLIENT_ID, + storage, + }); + await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); + await expect(logto.handleCallback(`${REDIRECT_URI}?code=${CODE}`)).rejects.toThrowError(); }); test('should throw response error', async () => { @@ -224,7 +257,11 @@ describe('LogtoClient', () => { clientId: CLIENT_ID, storage, }); - await expect(logto.handleCallback(REDIRECT_CALLBACK_WITH_ERROR)).rejects.toThrowError(); + await expect( + logto.handleCallback( + `${REDIRECT_URI}?code=${CODE}&state=${STATE}&error=invalid_request&error_description=code_challenge%20must%20be%20a%20string%20with%20a%20minimum%20length%20of%2043%20characters` + ) + ).rejects.toThrowError(); }); test('verifyIdToken should be called', async () => { @@ -235,7 +272,7 @@ describe('LogtoClient', () => { storage, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - await logto.handleCallback(REDIRECT_CALLBACK); + await logto.handleCallback(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`); expect(verifyIdToken).toHaveBeenCalled(); }); @@ -247,10 +284,24 @@ describe('LogtoClient', () => { storage, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - await logto.handleCallback(REDIRECT_CALLBACK); + await logto.handleCallback(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`); expect(storage.getItem('LOGTO_SESSION_MANAGER')).toBeUndefined(); }); + test('unknown state in url should fail', async () => { + const storage = new MemoryStorage(); + const logto = await LogtoClient.create({ + domain: DOMAIN, + clientId: CLIENT_ID, + storage, + }); + await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); + const unknownState = `UNKNOWN_${STATE}`; + await expect( + logto.handleCallback(`${REDIRECT_URI}?code=${CODE}&state=${unknownState}`) + ).rejects.toThrowError(); + }); + test('should be authenticated', async () => { const storage = new MemoryStorage(); const logto = await LogtoClient.create({ @@ -259,7 +310,7 @@ describe('LogtoClient', () => { storage, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - await logto.handleCallback(REDIRECT_CALLBACK); + await logto.handleCallback(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`); expect(logto.isAuthenticated()).toBeTruthy(); }); }); @@ -395,7 +446,7 @@ describe('LogtoClient', () => { onAuthStateChange, }); await logto.loginWithRedirect(REDIRECT_URI, jest.fn()); - await logto.handleCallback(REDIRECT_CALLBACK); + await logto.handleCallback(`${REDIRECT_URI}?code=${CODE}&state=${STATE}`); expect(onAuthStateChange).toHaveBeenCalled(); }); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 54fdd66fa..35c640c5e 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -10,7 +10,7 @@ import { TokenSetParameters, } from './grant-token'; import { parseRedirectCallback } from './parse-callback'; -import { getLoginUrlAndCodeVerifier } from './request-login'; +import { getLoginUrlWithCodeVerifierAndState } from './request-login'; import { getLogoutUrl } from './request-logout'; import SessionManager from './session-manager'; import { ClientStorage, LocalStorage } from './storage'; @@ -69,29 +69,23 @@ export default class LogtoClient { redirectUri: string, onRedirect: (url: string) => void = createDefaultOnRedirect() ) { - const { url, codeVerifier } = await getLoginUrlAndCodeVerifier({ + const { url, codeVerifier, state } = await getLoginUrlWithCodeVerifierAndState({ baseUrl: this.oidcConfiguration.authorization_endpoint, clientId: this.clientId, scope: this.scope, redirectUri, }); - this.sessionManager.set({ redirectUri, codeVerifier }); + this.sessionManager.set({ redirectUri, codeVerifier, state }); onRedirect(url); } public isLoginRedirect(url: string) { - try { - const { code, error } = parseRedirectCallback(url); - - if (error || !code) { - return false; - } - } catch { + const { code, state, error } = parseRedirectCallback(url); + if (error || !code || !state) { return false; } const session = this.sessionManager.get(); - if (!session) { return false; } @@ -101,14 +95,18 @@ export default class LogtoClient { } public async handleCallback(url: string) { - const { code, error, error_description } = parseRedirectCallback(url); + const { code, state, error, error_description } = parseRedirectCallback(url); if (error) { throw new Error(error_description ?? error); } if (!code) { - throw new Error(`Can not found authorization_code in url: ${url}`); + throw new Error(`Authorization_code not found in url: ${url}`); + } + + if (!state) { + throw new Error(`State not found in url: ${url}`); } const session = this.sessionManager.get(); @@ -117,8 +115,13 @@ export default class LogtoClient { throw new Error('Session not found'); } - const { redirectUri, codeVerifier } = session; + const { redirectUri, codeVerifier, state: stateInStorage } = session; this.sessionManager.clear(); + + if (state !== stateInStorage) { + throw new Error('Unknown state'); + } + const tokenParameters = await grantTokenByAuthorizationCode( { endpoint: this.oidcConfiguration.token_endpoint, @@ -129,13 +132,16 @@ export default class LogtoClient { }, this.requester ); + await verifyIdToken( createJWKS(this.oidcConfiguration.jwks_uri), tokenParameters.id_token, this.clientId ); + this.storage.setItem(this.tokenSetCacheKey, tokenParameters); this.tokenSet = new TokenSet(tokenParameters); + if (this.onAuthStateChange) { this.onAuthStateChange(); } diff --git a/packages/client/src/parse-callback.test.ts b/packages/client/src/parse-callback.test.ts index 2e511d6b3..4a9bde87e 100644 --- a/packages/client/src/parse-callback.test.ts +++ b/packages/client/src/parse-callback.test.ts @@ -11,6 +11,6 @@ describe('parseRedirectCallback', () => { }); test('no query params should fail', () => { - expect(() => parseRedirectCallback('http://localhost:3000')).toThrow(); + expect(parseRedirectCallback('http://localhost:3000')).toHaveProperty('error'); }); }); diff --git a/packages/client/src/parse-callback.ts b/packages/client/src/parse-callback.ts index 065ed4a29..360e3eb6c 100644 --- a/packages/client/src/parse-callback.ts +++ b/packages/client/src/parse-callback.ts @@ -2,17 +2,17 @@ import qs from 'query-string'; export interface AuthenticationResult { code?: string; + state?: string; error?: string; error_description?: string; } -export const parseRedirectCallback = (url: string) => { +export const parseRedirectCallback = (url: string): AuthenticationResult => { const [, queryString] = url.split('?'); if (!queryString) { - throw new Error('There are no query params available for parsing.'); + return { error: 'There are no query params available for parsing.' }; } - const result = qs.parse(queryString) as AuthenticationResult; - + const result = qs.parse(queryString); return result; }; diff --git a/packages/client/src/request-login.test.ts b/packages/client/src/request-login.test.ts index 23466bd56..c5bf75d3b 100644 --- a/packages/client/src/request-login.test.ts +++ b/packages/client/src/request-login.test.ts @@ -1,11 +1,108 @@ -import { getLoginUrlAndCodeVerifier } from './request-login'; +import { DEFAULT_SCOPE_STRING } from './constants'; +import { getLoginUrlWithCodeVerifierAndState } from './request-login'; -test('getLoginUrlAndCodeVerifier', async () => { - const { url } = await getLoginUrlAndCodeVerifier({ - baseUrl: 'http://logto.dev/oidc/auth', - clientId: 'foo', - scope: 'openid offline_access', - redirectUri: 'http://localhost:3000', +const BASE_URL = 'http://logto.dev/oidc/auth'; +const CLIENT_ID = 'foo'; +const REDIRECT_URI = 'http://localhost:3000'; + +describe('getLoginUrlWithCodeVerifierAndState', () => { + test('url must start with baseUrl', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url.startsWith(BASE_URL)).toBeTruthy(); + }); + + test('url must contain expected client_id', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain(`client_id=${CLIENT_ID}`); + }); + + test('url must contains expected scope', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain(`scope=${encodeURI(DEFAULT_SCOPE_STRING)}`); + }); + + test('url must contain expected response_type', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain('response_type=code'); + }); + + test('url must contain expected redirect_uri', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain(`redirect_uri=${encodeURIComponent(REDIRECT_URI)}`); + }); + + test('url must contain expected prompt', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain('prompt=consent'); + }); + + test('url must contain expected redirect_uri', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain(`redirect_uri=${encodeURIComponent(REDIRECT_URI)}`); + }); + + test('url must contain expected code_challenge', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url.search('code_challenge=[^&=]+')).toBeGreaterThan(0); + }); + + test('url must contain expected code_challenge_method', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url).toContain('code_challenge_method=S256'); + }); + + test('url must contain expected state', async () => { + const { url } = await getLoginUrlWithCodeVerifierAndState({ + baseUrl: BASE_URL, + clientId: CLIENT_ID, + scope: DEFAULT_SCOPE_STRING, + redirectUri: REDIRECT_URI, + }); + expect(url.search('state=[^&=]+')).toBeGreaterThan(0); }); - expect(url).toContain('code_challenge_method=S256'); }); diff --git a/packages/client/src/request-login.ts b/packages/client/src/request-login.ts index 6a222594c..26c85012a 100644 --- a/packages/client/src/request-login.ts +++ b/packages/client/src/request-login.ts @@ -1,6 +1,6 @@ import qs from 'query-string'; -import { generateCodeChallenge, generateCodeVerifier } from './generators'; +import { generateCodeChallenge, generateCodeVerifier, generateState } from './generators'; export interface LoginPrepareParameters { baseUrl: string; @@ -9,12 +9,13 @@ export interface LoginPrepareParameters { redirectUri: string; } -export const getLoginUrlAndCodeVerifier = async ( +export const getLoginUrlWithCodeVerifierAndState = async ( parameters: LoginPrepareParameters -): Promise<{ url: string; codeVerifier: string }> => { +): Promise<{ url: string; codeVerifier: string; state: string }> => { const { baseUrl, clientId, scope, redirectUri } = parameters; const codeVerifier = generateCodeVerifier(); const codeChallenge = await generateCodeChallenge(codeVerifier); + const state = generateState(); const url = `${baseUrl}?${qs.stringify({ client_id: clientId, @@ -24,7 +25,8 @@ export const getLoginUrlAndCodeVerifier = async ( prompt: 'consent', code_challenge: codeChallenge, code_challenge_method: 'S256', + state, })}`; - return { url, codeVerifier }; + return { url, codeVerifier, state }; }; diff --git a/packages/client/src/session-manager.test.ts b/packages/client/src/session-manager.test.ts index d813d47b0..9085f0594 100644 --- a/packages/client/src/session-manager.test.ts +++ b/packages/client/src/session-manager.test.ts @@ -1,10 +1,11 @@ import { SESSION_MANAGER_KEY } from './constants'; -import SessionManager from './session-manager'; +import SessionManager, { Session } from './session-manager'; import { SessionStorage } from './storage'; -const transaction = { +const transaction: Session = { codeVerifier: 'codeVerifier', redirectUri: 'redirectUri', + state: 'state', }; describe('SessionManager', () => { diff --git a/packages/client/src/session-manager.ts b/packages/client/src/session-manager.ts index b39ea5e5b..58935d8f5 100644 --- a/packages/client/src/session-manager.ts +++ b/packages/client/src/session-manager.ts @@ -3,9 +3,10 @@ import { Optional } from '@silverhand/essentials'; import { SESSION_EXPIRES_MILLISECONDS, SESSION_MANAGER_KEY } from './constants'; import { ClientStorage } from './storage'; -interface Session { +export interface Session { codeVerifier: string; redirectUri: string; + state: string; } export default class SessionManager { diff --git a/packages/client/src/utils.test.ts b/packages/client/src/utils.test.ts index 8d3d297f5..0115d850c 100644 --- a/packages/client/src/utils.test.ts +++ b/packages/client/src/utils.test.ts @@ -1,4 +1,4 @@ -import { generateKeyPair, SignJWT } from 'jose'; +import { SignJWT, generateKeyPair } from 'jose'; import { StructError } from 'superstruct'; import { decodeToken } from './utils';