From dbd1648bf407a3996f35128d48a3c95cbaca6ef5 Mon Sep 17 00:00:00 2001 From: Dimitris Klouvas Date: Fri, 31 Mar 2023 18:32:13 +0300 Subject: [PATCH] chore(backend): Extract jwt audience assertion & add unit tests Also there was some changes in the code: - aud claim with empty values does NOT throw verification error --- .../backend/src/tokens/jwt/assertions.test.ts | 108 ++++++++++++++++++ packages/backend/src/tokens/jwt/assertions.ts | 45 ++++++++ .../src/tokens/jwt/hasValidSignature.ts | 45 ++++++++ packages/backend/src/tokens/jwt/verifyJwt.ts | 41 +------ packages/backend/tests/suites.ts | 2 + 5 files changed, 202 insertions(+), 39 deletions(-) create mode 100644 packages/backend/src/tokens/jwt/assertions.test.ts create mode 100644 packages/backend/src/tokens/jwt/assertions.ts create mode 100644 packages/backend/src/tokens/jwt/hasValidSignature.ts diff --git a/packages/backend/src/tokens/jwt/assertions.test.ts b/packages/backend/src/tokens/jwt/assertions.test.ts new file mode 100644 index 0000000000..7d3d58e7b7 --- /dev/null +++ b/packages/backend/src/tokens/jwt/assertions.test.ts @@ -0,0 +1,108 @@ +import type QUnit from 'qunit'; + +import { assertAudienceClaim } from './assertions'; + +export default (QUnit: QUnit) => { + const { module, test } = QUnit; + + module('assertAudienceClaim(audience?, aud?)', () => { + const audience = 'http://audience.example'; + const otherAudience = 'http://audience-other.example'; + const invalidAudience = 'http://invalid-audience.example'; + + test('does not throw error if aud is missing', assert => { + assert.equal(undefined, assertAudienceClaim()); + assert.equal(undefined, assertAudienceClaim(undefined)); + assert.equal(undefined, assertAudienceClaim(undefined, audience)); + assert.equal(undefined, assertAudienceClaim(undefined, [audience])); + assert.equal(undefined, assertAudienceClaim('')); + assert.equal(undefined, assertAudienceClaim('', audience)); + assert.equal(undefined, assertAudienceClaim('', [audience])); + assert.equal(undefined, assertAudienceClaim('', [audience, otherAudience])); + }); + + test('does not throw error if audience is missing', assert => { + assert.equal(undefined, assertAudienceClaim(undefined, undefined)); + assert.equal(undefined, assertAudienceClaim(audience, undefined)); + assert.equal(undefined, assertAudienceClaim([audience], undefined)); + + assert.equal(undefined, assertAudienceClaim(undefined, '')); + assert.equal(undefined, assertAudienceClaim(audience, '')); + assert.equal(undefined, assertAudienceClaim([audience], '')); + assert.equal(undefined, assertAudienceClaim([audience, otherAudience], '')); + }); + + test('does not throw error if aud contains empty values', assert => { + assert.equal(undefined, assertAudienceClaim([], audience)); + assert.equal(undefined, assertAudienceClaim([undefined, undefined], audience)); + assert.equal(undefined, assertAudienceClaim([null, null], audience)); + assert.equal(undefined, assertAudienceClaim([false, false], audience)); + assert.equal(undefined, assertAudienceClaim(['', ''], [audience])); + assert.equal(undefined, assertAudienceClaim(['', ''], [audience, otherAudience])); + }); + + test('does not throw error if audience is empty or contains empty values', assert => { + assert.equal(undefined, assertAudienceClaim(audience, [])); + assert.equal(undefined, assertAudienceClaim(audience, [undefined, undefined])); + assert.equal(undefined, assertAudienceClaim(audience, [null, null])); + assert.equal(undefined, assertAudienceClaim(audience, [false, false])); + assert.equal(undefined, assertAudienceClaim(audience, ['', ''])); + assert.equal(undefined, assertAudienceClaim([audience], ['', ''])); + assert.equal(undefined, assertAudienceClaim([audience, otherAudience], ['', ''])); + }); + + test('does not throw error when audience matches aud', assert => { + assert.equal(undefined, assertAudienceClaim(audience, audience)); + }); + + test('does not throw error when audience list contains aud', assert => { + assert.equal(undefined, assertAudienceClaim(audience, [audience, otherAudience])); + }); + + test('does not throw error when audience string[] has intersection with aud string[]', assert => { + assert.equal(undefined, assertAudienceClaim([audience], [audience, otherAudience])); + assert.equal(undefined, assertAudienceClaim([audience, otherAudience], [audience])); + }); + + test('throws error when audience does not match aud', assert => { + assert.raises( + () => assertAudienceClaim(audience, invalidAudience), + `Invalid JWT audience claim (aud) ${audience}. Is not included in "[${invalidAudience}]".`, + ); + }); + + test('throws error when audience is substring of aud', assert => { + assert.raises( + () => assertAudienceClaim(audience, audience.slice(0, -2)), + `Invalid JWT audience claim (aud) ${audience}. Is not included in "${audience.slice(0, -2)}".`, + ); + }); + + test('throws error when audience is substring of an aud when aud is a string[]', assert => { + assert.raises( + () => assertAudienceClaim([audience, otherAudience], audience.slice(0, -2)), + `Invalid JWT audience claim (aud) ${[audience, otherAudience]}. Is not included in "[${audience.slice( + 0, + -2, + )}]".`, + ); + }); + + test('throws error when aud is a substring of audience', assert => { + assert.raises( + () => assertAudienceClaim(audience.slice(0, -2), audience), + `Invalid JWT audience claim (aud) ${audience.slice(0, -2)}. Is not included in "${audience}".`, + ); + }); + + test('throws error when aud is substring of an audience when audience is a string[]', assert => { + assert.raises( + () => assertAudienceClaim(audience.slice(0, -2), [audience, otherAudience]), + `Invalid JWT audience claim (aud) ${audience.slice(0, -2)}. Is not included in "[${[ + audience, + otherAudience, + ]}]".`, + ); + }); + }); +}; diff --git a/packages/backend/src/tokens/jwt/assertions.ts b/packages/backend/src/tokens/jwt/assertions.ts new file mode 100644 index 0000000000..6f0bdfc3fd --- /dev/null +++ b/packages/backend/src/tokens/jwt/assertions.ts @@ -0,0 +1,45 @@ +import { TokenVerificationError, TokenVerificationErrorAction, TokenVerificationErrorReason } from '../errors'; + +const isArrayString = (s: unknown): s is string[] => { + return Array.isArray(s) && s.length > 0 && s.every(a => typeof a === 'string'); +}; + +export const assertAudienceClaim = (aud?: unknown, audience?: unknown) => { + const audienceList = [audience].flat().filter(a => !!a); + const audList = [aud].flat().filter(a => !!a); + const shouldVerifyAudience = audienceList.length > 0 && audList.length > 0; + + if (!shouldVerifyAudience) { + // Notice: Clerk JWTs use AZP claim instead of Audience + // + // return { + // valid: false, + // reason: `Invalid JWT audience claim (aud) ${JSON.stringify( + // aud, + // )}. Expected a string or a non-empty array of strings.`, + // }; + return; + } + + if (typeof aud === 'string') { + if (!audienceList.includes(aud)) { + throw new TokenVerificationError({ + action: TokenVerificationErrorAction.EnsureClerkJWT, + reason: TokenVerificationErrorReason.TokenVerificationFailed, + message: `Invalid JWT audience claim (aud) ${JSON.stringify(aud)}. Is not included in "${JSON.stringify( + audienceList, + )}".`, + }); + } + } else if (isArrayString(aud)) { + if (!aud.some(a => audienceList.includes(a))) { + throw new TokenVerificationError({ + action: TokenVerificationErrorAction.EnsureClerkJWT, + reason: TokenVerificationErrorReason.TokenVerificationFailed, + message: `Invalid JWT audience claim array (aud) ${JSON.stringify(aud)}. Is not included in "${JSON.stringify( + audienceList, + )}".`, + }); + } + } +}; diff --git a/packages/backend/src/tokens/jwt/hasValidSignature.ts b/packages/backend/src/tokens/jwt/hasValidSignature.ts new file mode 100644 index 0000000000..1daac53565 --- /dev/null +++ b/packages/backend/src/tokens/jwt/hasValidSignature.ts @@ -0,0 +1,45 @@ +import type { Jwt } from '@clerk/types'; + +// DO NOT CHANGE: Runtime needs to be imported as a default export so that we can stub its dependencies with Sinon.js +// For more information refer to https://sinonjs.org/how-to/stub-dependency/ +import runtime from '../../runtime'; + +const algToHash: Record = { + RS256: 'SHA-256', + RS384: 'SHA-384', + RS512: 'SHA-512', + ES256: 'SHA-256', + ES384: 'SHA-384', + ES512: 'SHA-512', +}; + +const RSA_ALGORITHM_NAME = 'RSASSA-PKCS1-v1_5'; +const EC_ALGORITHM_NAME = 'ECDSA'; + +const jwksAlgToCryptoAlg: Record = { + RS256: RSA_ALGORITHM_NAME, + RS384: RSA_ALGORITHM_NAME, + RS512: RSA_ALGORITHM_NAME, + ES256: EC_ALGORITHM_NAME, + ES384: EC_ALGORITHM_NAME, + ES512: EC_ALGORITHM_NAME, +}; + +export async function hasValidSignature(jwt: Jwt, jwk: JsonWebKey) { + const { header, signature, raw } = jwt; + const encoder = new TextEncoder(); + const data = encoder.encode([raw.header, raw.payload].join('.')); + + const cryptoKey = await runtime.crypto.subtle.importKey( + 'jwk', + jwk, + { + name: jwksAlgToCryptoAlg[header.alg], + hash: algToHash[header.alg], + }, + false, + ['verify'], + ); + + return runtime.crypto.subtle.verify('RSASSA-PKCS1-v1_5', cryptoKey, signature, data); +} diff --git a/packages/backend/src/tokens/jwt/verifyJwt.ts b/packages/backend/src/tokens/jwt/verifyJwt.ts index 1ebb2d0a7c..2e75dde17e 100644 --- a/packages/backend/src/tokens/jwt/verifyJwt.ts +++ b/packages/backend/src/tokens/jwt/verifyJwt.ts @@ -5,6 +5,7 @@ import type { Jwt, JwtPayload } from '@clerk/types'; import runtime from '../../runtime'; import { base64url } from '../../util/rfc4648'; import { TokenVerificationError, TokenVerificationErrorAction, TokenVerificationErrorReason } from '../errors'; +import { assertAudienceClaim } from './assertions'; type IssuerResolver = string | ((iss: string) => boolean); @@ -33,10 +34,6 @@ const jwksAlgToCryptoAlg: Record = { const algs = Object.keys(algToHash); -const isArrayString = (s: unknown): s is string[] => { - return Array.isArray(s) && s.length > 0 && s.every(a => typeof a === 'string'); -}; - export async function hasValidSignature(jwt: Jwt, jwk: JsonWebKey) { const { header, signature, raw } = jwt; const encoder = new TextEncoder(); @@ -151,41 +148,7 @@ export async function verifyJwt( }); } - // Verify audience claim (aud) - const audiences = [audience].flat().filter(a => !!a); - const shouldVerifyAudience = audiences.length > 0 && aud; - - if (!shouldVerifyAudience) { - // Avoid verifying aud claim & audience param - // Notice: Clerk JWTs use AZP claim instead of Audience - // - // return { - // valid: false, - // reason: `Invalid JWT audience claim (aud) ${JSON.stringify( - // aud, - // )}. Expected a string or a non-empty array of strings.`, - // }; - } else if (typeof aud === 'string') { - if (!audiences.includes(aud)) { - throw new TokenVerificationError({ - action: TokenVerificationErrorAction.EnsureClerkJWT, - reason: TokenVerificationErrorReason.TokenVerificationFailed, - message: `Invalid JWT audience claim (aud) ${JSON.stringify(aud)}. Is not included in "${JSON.stringify( - audiences, - )}".`, - }); - } - } else if (isArrayString(aud)) { - if (!aud.some(a => audiences.includes(a))) { - throw new TokenVerificationError({ - action: TokenVerificationErrorAction.EnsureClerkJWT, - reason: TokenVerificationErrorReason.TokenVerificationFailed, - message: `Invalid JWT audience claim array (aud) ${JSON.stringify(aud)}. Is not included in "${JSON.stringify( - audiences, - )}".`, - }); - } - } + assertAudienceClaim([aud], [audience]); // Verify authorized parties claim (azp) if (azp && authorizedParties && authorizedParties.length > 0 && !authorizedParties.includes(azp)) { diff --git a/packages/backend/tests/suites.ts b/packages/backend/tests/suites.ts index f875fafce7..db4cf04f7b 100644 --- a/packages/backend/tests/suites.ts +++ b/packages/backend/tests/suites.ts @@ -6,6 +6,7 @@ import keysTest from './dist/tokens/keys.test.js'; import pathTest from './dist/util/path.test.js'; import verifyTest from './dist/tokens/verify.test.js'; import verifyJwtTest from './dist/tokens/jwt/verifyJwt.test.js'; +import jwtAssertionsTest from './dist/tokens/jwt/assertions.test.js'; import utilRequestTest from './dist/util/request.test.js'; import factoryTest from './dist/api/factory.test.js'; @@ -18,6 +19,7 @@ import redirectTest from './dist/redirections.test.js'; const suites = [ apiTest, exportsTest, + jwtAssertionsTest, requestTest, utilRequestTest, keysTest,