Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for PKCE #245

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
34ab848
Add a test watch script to package json for easier working with tests
tanettrimas Jun 29, 2023
ada2b55
fix: add .idea to gitignore
Jul 2, 2024
5a9e6c6
Add tests for verification of pkce verifiers
Jul 2, 2024
c514b2d
Make sure the regex is according to the RFC spec
tanettrimas Jun 29, 2023
465d4aa
Add generator for a valid pkce verifier
tanettrimas Jun 29, 2023
e5c7c9f
Add generator for a valid code challenge
tanettrimas Jun 29, 2023
63d44b1
Add code_verifier to the tokenrequest
Jul 2, 2024
02578e8
Add an asserter for code_challenge_method
tanettrimas Jun 29, 2023
7b514e0
Use string or undefined assertion on code_challenge and code_challeng…
tanettrimas Jun 29, 2023
a4f949e
Add tests for verification of challenge and verifier in combination
tanettrimas Jun 29, 2023
8631506
Add the actual implementation of PKCE
tanettrimas Jun 29, 2023
62b99ac
Added metadata information about pkce
Jul 2, 2024
28428a0
Add corresponding oauth2-service tests for the functionality
tanettrimas Jun 29, 2023
86cd8c3
Fix formatting
Jul 2, 2024
c8cf8db
Remove .idea
tanettrimas Jun 29, 2023
71a6478
Format
Jul 2, 2024
b5b6441
format
Jul 2, 2024
96c9c3f
format
tanettrimas Jun 30, 2023
c27f31b
format
Jul 2, 2024
05720c9
format
tanettrimas Jun 30, 2023
79bb5ec
format
tanettrimas Jun 30, 2023
350f928
format
Jul 2, 2024
204d972
format
tanettrimas Jun 30, 2023
9ad92eb
Inline function
tanettrimas Jun 30, 2023
16e3da5
Remove assertion functions
tanettrimas Jun 30, 2023
f8bdb2a
Remove helper functions
Jul 2, 2024
a0794a0
fix: remove unused helper methods
Jul 2, 2024
2904885
refactor: use explicit assertion instead of "tobetruthy/falsy"
Jul 2, 2024
404a83b
fix: lint issues
Jul 2, 2024
472cae4
Update src/lib/oauth2-service.ts
tanettrimas Jul 3, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/node_modules/
/coverage/
/.vscode/
/.idea
/.cache/
/dist/
.idea
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
"lint": "eslint --cache --cache-location .cache/ --ext=.ts src test --max-warnings 0",
"prepack": "yarn build --tsBuildInfoFile null --incremental false",
"pretest": "yarn lint",
"test": "yarn vitest --run --coverage"
"test": "yarn vitest --run --coverage",
"test:watch": "yarn vitest --watch"
},
"dependencies": {
"basic-auth": "^2.0.1",
Expand Down
49 changes: 48 additions & 1 deletion src/lib/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import { readFileSync } from 'fs';

import { isPlainObject } from 'is-plain-object';

import type { TokenRequest } from './types';
import type { CodeChallenge, PKCEAlgorithm, TokenRequest } from './types';
import { webcrypto as crypto } from 'crypto';

export const defaultTokenTtl = 3600;

Expand Down Expand Up @@ -60,6 +61,17 @@ export function assertIsPlainObject(
}
}

export async function pkceVerifierMatchesChallenge(
verifier: string,
challenge: CodeChallenge,
) {
const generatedChallenge = await createPKCECodeChallenge(
verifier,
challenge.method,
);
return generatedChallenge === challenge.challenge;
}

export function assertIsValidTokenRequest(
body: unknown,
): asserts body is TokenRequest {
Expand Down Expand Up @@ -111,3 +123,38 @@ export const readJsonFromFile = (filepath: string): Record<string, unknown> => {

return maybeJson;
};

export const isValidPkceCodeVerifier = (verifier: string) => {
const PKCE_CHALLENGE_REGEX = /^[A-Za-z0-9\-._~]{43,128}$/;
return PKCE_CHALLENGE_REGEX.test(verifier);
};

export const createPKCEVerifier = () => {
const randomBytes = crypto.getRandomValues(new Uint8Array(32));
return Buffer.from(randomBytes).toString('base64url');
};

export const supportedPkceAlgorithms = ['plain', 'S256'] as const;

export const createPKCECodeChallenge = async (
verifier: string = createPKCEVerifier(),
algorithm: PKCEAlgorithm = 'plain',
) => {
let challenge: string;

switch (algorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tanettrimas How about adding a default: clause for this switch and make it throw?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do I need a default? it is only used with constant values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good coding practice to leave no execution branches unchecked, no matter how unlikely you think their execution might be.

Every public method (in this case, "public" means "an exported function in a module that someone else can consume") should validate its input, and throw if it receives an input it's not expected to handle.

I can see that you are already guarding against invalid values and returning an HTTP 400 error response if this happens, and today nothing in the rest of the code will pass an invalid value to this method. The thing is that tomorrow some other developer might decide to reuse this method somewhere else and not guard against invalid values, thus producing an unexpected behaviour and unintentionally introducing a bug.

case 'plain': {
challenge = verifier;
break;
}
case 'S256': {
const buffer = await crypto.subtle.digest(
'SHA-256',
new TextEncoder().encode(verifier),
);
challenge = Buffer.from(buffer).toString('base64url');
break;
}
}
return challenge;
};
72 changes: 72 additions & 0 deletions src/lib/oauth2-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,25 @@ import {
assertIsStringOrUndefined,
assertIsValidTokenRequest,
defaultTokenTtl,
isValidPkceCodeVerifier,
pkceVerifierMatchesChallenge,
supportedPkceAlgorithms,
} from './helpers';
import type {
CodeChallenge,
JwtTransform,
MutableRedirectUri,
MutableResponse,
MutableToken,
OAuth2Endpoints,
OAuth2EndpointsInput,
PKCEAlgorithm,
ScopesOrTransform,
StatusCodeMutableResponse,
} from './types';
import { Events } from './types';
import { InternalEvents } from './types-internals';
import { AssertionError } from 'assert';

const DEFAULT_ENDPOINTS: OAuth2Endpoints = Object.freeze({
wellKnownDocument: '/.well-known/openid-configuration',
Expand All @@ -71,6 +77,7 @@ export class OAuth2Service extends EventEmitter {
#issuer: OAuth2Issuer;
#requestHandler: RequestListener;
#nonce: Record<string, string>;
#codeChallenges: Map<string, CodeChallenge>;
#endpoints: OAuth2Endpoints;

constructor(oauth2Issuer: OAuth2Issuer, endpoints?: OAuth2EndpointsInput) {
Expand All @@ -80,6 +87,7 @@ export class OAuth2Service extends EventEmitter {
this.#endpoints = { ...DEFAULT_ENDPOINTS, ...endpoints };
this.#requestHandler = this.buildRequestHandler();
this.#nonce = {};
this.#codeChallenges = new Map();
}

/**
Expand Down Expand Up @@ -169,6 +177,7 @@ export class OAuth2Service extends EventEmitter {
subject_types_supported: ['public'],
end_session_endpoint: `${this.issuer.url}${this.#endpoints.endSession}`,
introspection_endpoint: `${this.issuer.url}${this.#endpoints.introspect}`,
code_challenge_methods_supported: supportedPkceAlgorithms,
};

return res.json(openidConfig);
Expand All @@ -190,6 +199,39 @@ export class OAuth2Service extends EventEmitter {
let xfn: ScopesOrTransform | undefined;

assertIsValidTokenRequest(req.body);

if ('code_verifier' in req.body && 'code' in req.body) {
try {
const code = req.body.code;
const verifier = req.body['code_verifier'];
const savedCodeChallenge = this.#codeChallenges.get(code);
if (savedCodeChallenge === undefined) {
throw new AssertionError({
message: 'code_challenge required',
});
}
this.#codeChallenges.delete(code);
if (!isValidPkceCodeVerifier(verifier)) {
throw new AssertionError({
message:
"Invalid 'code_verifier'. The verifier does not conform with the RFC7636 spec. Ref: https://datatracker.ietf.org/doc/html/rfc7636#section-4.1",
});
}
const doesVerifierMatchCodeChallenge =
await pkceVerifierMatchesChallenge(verifier, savedCodeChallenge);
if (!doesVerifierMatchCodeChallenge) {
throw new AssertionError({
message: 'code_verifier provided does not match code_challenge',
});
}
} catch (e) {
return res.status(400).json({
error: 'invalid_request',
error_description: (e as AssertionError).message,
});
}
}

const reqBody = req.body;

let { scope } = reqBody;
Expand Down Expand Up @@ -294,16 +336,46 @@ export class OAuth2Service extends EventEmitter {
redirect_uri: redirectUri,
response_type: responseType,
state,
code_challenge,
code_challenge_method,
} = req.query;

assertIsString(redirectUri, 'Invalid redirectUri type');
assertIsStringOrUndefined(nonce, 'Invalid nonce type');
assertIsStringOrUndefined(scope, 'Invalid scope type');
assertIsStringOrUndefined(state, 'Invalid state type');
assertIsStringOrUndefined(code_challenge, 'Invalid code_challenge type');
assertIsStringOrUndefined(
code_challenge_method,
'Invalid code_challenge_method type',
);

const url = new URL(redirectUri);

if (responseType === 'code') {
if (code_challenge) {
const codeChallengeMethod = code_challenge_method ?? 'plain';
assertIsString(
codeChallengeMethod,
"Invalid 'code_challenge_method' type",
);
if (
!supportedPkceAlgorithms.includes(
codeChallengeMethod as PKCEAlgorithm,
)
) {
return res.status(400).json({
error: 'invalid_request',
error_description: `Unsupported code_challenge method ${codeChallengeMethod}. The one of the following code_challenge_method are supported: ${supportedPkceAlgorithms.join(
', ',
)}`,
});
}
this.#codeChallenges.set(code, {
challenge: code_challenge,
method: codeChallengeMethod as PKCEAlgorithm,
});
}
if (nonce !== undefined) {
this.#nonce[code] = nonce;
}
Expand Down
11 changes: 11 additions & 0 deletions src/lib/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ServerOptions } from 'https';
import { JWKWithKid } from './types-internals';
import { supportedPkceAlgorithms } from './helpers';

export interface TokenRequest {
scope?: string;
Expand All @@ -8,6 +9,7 @@ export interface TokenRequest {
client_id?: unknown;
code?: string;
aud?: string[] | string;
code_verifier?: string;
}

export interface Options {
Expand All @@ -33,6 +35,7 @@ export interface MutableToken {

export interface Header {
kid: string;

[key: string]: unknown;
}

Expand All @@ -41,6 +44,7 @@ export interface Payload {
iat: number;
exp: number;
nbf: number;

[key: string]: unknown;
}

Expand Down Expand Up @@ -106,3 +110,10 @@ export type OAuth2EndpointsInput = Partial<OAuth2Endpoints>;
export interface OAuth2Options {
endpoints?: OAuth2EndpointsInput;
}

export type PKCEAlgorithm = (typeof supportedPkceAlgorithms)[number];
tanettrimas marked this conversation as resolved.
Show resolved Hide resolved

export interface CodeChallenge {
challenge: string;
method: PKCEAlgorithm;
}
60 changes: 57 additions & 3 deletions test/helpers.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, it, expect } from 'vitest';
import { describe, expect, it } from 'vitest';
import type { AddressInfo } from 'net';

import {
Expand All @@ -7,8 +7,13 @@ import {
assertIsString,
assertIsStringOrUndefined,
assertIsValidTokenRequest,
createPKCECodeChallenge,
createPKCEVerifier,
isValidPkceCodeVerifier,
pkceVerifierMatchesChallenge,
shift,
} from '../src/lib/helpers';
import { CodeChallenge } from '../src';

describe('helpers', () => {
describe('assertIsString', () => {
Expand Down Expand Up @@ -97,7 +102,7 @@ describe('helpers', () => {
{ grant_type: "g", scope: "s", code: 1 },
{ grant_type: "g", scope: 1, code: "c" },
{ grant_type: "g", scope: "1", code: "c", aud: 1 },
{ grant_type: "g", scope: "1", code: "c", aud: [1] },
{ grant_type: "g", scope: "1", code: "c", aud: [1] }
])('throws on wrong values (%s)', (input) => {
expect(() => assertIsValidTokenRequest(input)).toThrow();
});
Expand All @@ -108,7 +113,7 @@ describe('helpers', () => {
{ grant_type: "g", scope: "s" },
{ grant_type: "g", scope: "s", code: "c" },
{ grant_type: "g", scope: "s", code: "c", aud: "a" },
{ grant_type: "g", scope: "s", code: "c", aud: ["a", "b"] },
{ grant_type: "g", scope: "s", code: "c", aud: ["a", "b"] }
])('does not throw on valid input (%s)', (input) => {
expect(() => assertIsValidTokenRequest(input)).not.toThrow();
});
Expand All @@ -127,4 +132,53 @@ describe('helpers', () => {
expect(() => shift(["a"])).not.toThrow();
});
});

describe('pkce', () => {
describe('code_verifier', () => {
it('should accept a valid PKCE code_verifier', () => {
const verifier128 =
'PXa7p8YHHUAJGrcG2eW0x7FY_EBtRTlaUHnyz1jKWnNp0G-2HZt9KjA0UOp87DmuIqoV4Y_owVsM-QICvrSa5dWxOndVEhSsFMMgy68AYkw4PGHkGaN_aIRIHJ8mQ4EZ';
const verifier42 = 'xyo94uhy3zKvgB0NJwLms86SwcjtWviEOpkBnGgaLlo';
expect(isValidPkceCodeVerifier(verifier128)).toBe(true);
expect(isValidPkceCodeVerifier(verifier42)).toBe(true);

const verifierWith129chars = `${verifier128}a`;
expect(isValidPkceCodeVerifier(verifierWith129chars)).toBe(false);
expect(
isValidPkceCodeVerifier(verifier42.slice(0, verifier42.length - 1))
).toBe(false);
});

it('should create a valid code_verifier', () => {
expect(isValidPkceCodeVerifier(createPKCEVerifier())).toBe(true);
});

it('should create a valid code_challenge', async () => {
const verifier = 'xyo94uhy3zKvgB0NJwLms86SwcjtWviEOpkBnGgaLlo';
const expectedChallenge = 'b7elB7ZyxIXgFyvBznKvxl7wOB-H17Pz0a3B62NIMFI';
const generatedCodeChallenge = await createPKCECodeChallenge(
verifier,
'S256'
);
expect(generatedCodeChallenge).toBe(expectedChallenge);
const expectedCodeLength = 43; // BASE64-urlencoded sha256 hashes should always be 43 characters in length.
expect(
await createPKCECodeChallenge(createPKCEVerifier(), 'S256')
).toHaveLength(expectedCodeLength);
});

it('should match code_verifier and code_challenge', async () => {
const verifier = createPKCEVerifier();
const codeChallengeMethod = 'S256';
const challenge: CodeChallenge = {
challenge: await createPKCECodeChallenge(
verifier,
codeChallengeMethod
),
method: codeChallengeMethod,
};
expect(await pkceVerifierMatchesChallenge(verifier, challenge)).toBe(true);
});
});
});
});
Loading