Skip to content

Commit

Permalink
feat(js): add state to loginWithRedirect to prevent csrf attacking (#94)
Browse files Browse the repository at this point in the history
  • Loading branch information
@IceHe
IceHe authored Nov 15, 2021
1 parent 367ec03 commit 3d41e08
Show file tree
Hide file tree
Showing 9 changed files with 209 additions and 51 deletions.
83 changes: 67 additions & 16 deletions packages/client/src/index.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import { KeyObject } from 'crypto';

import { SignJWT, generateKeyPair } from 'jose';
import { generateKeyPair, SignJWT } from 'jose';
import nock from 'nock';

import LogtoClient from '.';
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';
Expand All @@ -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 = {
Expand Down Expand Up @@ -146,21 +152,23 @@ 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,
clientId: CLIENT_ID,
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 () => {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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();
});

Expand All @@ -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({
Expand All @@ -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();
});
});
Expand Down Expand Up @@ -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();
});

Expand Down
34 changes: 20 additions & 14 deletions packages/client/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
Expand All @@ -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();
Expand All @@ -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,
Expand All @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion packages/client/src/parse-callback.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
8 changes: 4 additions & 4 deletions packages/client/src/parse-callback.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Loading

0 comments on commit 3d41e08

Please sign in to comment.