From 2889c76995ce3c569f595ac3c678218e9ce659f0 Mon Sep 17 00:00:00 2001 From: Matthieu Sieben Date: Thu, 9 Jan 2025 14:26:07 +0100 Subject: [PATCH] Improve type safety and compatibility with Bun (#2879) * jwk: Improve type safety and compatibility with Bun * improve type safety of jwk keys * improve typing of verifyAccessToken * update @types/http-errors * Better report invalid content-encoding errors * Mark jwk key fields as readonly --- .changeset/early-tomatoes-cheer.md | 5 + .changeset/eight-eyes-float.md | 5 + .changeset/eighty-planes-sleep.md | 6 + .changeset/strong-mice-talk.md | 5 + .changeset/thin-mice-lick.md | 5 + packages/oauth/jwk-jose/src/jose-key.ts | 144 ++++++--- packages/oauth/jwk-webcrypto/package.json | 3 +- .../oauth/jwk-webcrypto/src/webcrypto-key.ts | 44 ++- packages/oauth/jwk/src/index.ts | 5 + packages/oauth/jwk/src/jwt-verify.ts | 8 +- packages/oauth/jwk/src/jwt.ts | 286 +++++++++--------- packages/oauth/jwk/src/key.ts | 41 +-- packages/oauth/jwk/src/keyset.ts | 9 +- packages/oauth/jwk/src/util.ts | 12 +- .../src/signer/signed-token-payload.ts | 3 +- .../oauth/oauth-provider/src/signer/signer.ts | 32 +- .../oauth-provider/src/token/token-manager.ts | 1 + packages/xrpc-server/package.json | 2 +- pnpm-lock.yaml | 15 +- 19 files changed, 368 insertions(+), 263 deletions(-) create mode 100644 .changeset/early-tomatoes-cheer.md create mode 100644 .changeset/eight-eyes-float.md create mode 100644 .changeset/eighty-planes-sleep.md create mode 100644 .changeset/strong-mice-talk.md create mode 100644 .changeset/thin-mice-lick.md diff --git a/.changeset/early-tomatoes-cheer.md b/.changeset/early-tomatoes-cheer.md new file mode 100644 index 00000000000..cd5269f0c25 --- /dev/null +++ b/.changeset/early-tomatoes-cheer.md @@ -0,0 +1,5 @@ +--- +"@atproto/jwk": patch +--- + +Mark jwk key fields as readonly diff --git a/.changeset/eight-eyes-float.md b/.changeset/eight-eyes-float.md new file mode 100644 index 00000000000..6ed8c304fff --- /dev/null +++ b/.changeset/eight-eyes-float.md @@ -0,0 +1,5 @@ +--- +"@atproto/jwk-jose": patch +--- + +Improve compatibility with runtimes relying on webcrypto (by explicit JOSE's importJWK() "alg" argument). diff --git a/.changeset/eighty-planes-sleep.md b/.changeset/eighty-planes-sleep.md new file mode 100644 index 00000000000..d03f9fc95c3 --- /dev/null +++ b/.changeset/eighty-planes-sleep.md @@ -0,0 +1,6 @@ +--- +"@atproto/jwk-jose": patch +"@atproto/jwk": patch +--- + +Remove unsafe type casting during JWT verification diff --git a/.changeset/strong-mice-talk.md b/.changeset/strong-mice-talk.md new file mode 100644 index 00000000000..2241147e6b5 --- /dev/null +++ b/.changeset/strong-mice-talk.md @@ -0,0 +1,5 @@ +--- +"@atproto/jwk": patch +--- + +Allow (passthrough) unknown properties in JWT payload & headers diff --git a/.changeset/thin-mice-lick.md b/.changeset/thin-mice-lick.md new file mode 100644 index 00000000000..385188002d5 --- /dev/null +++ b/.changeset/thin-mice-lick.md @@ -0,0 +1,5 @@ +--- +"@atproto/jwk": patch +--- + +Expose ValidationError to allow for proper error handling diff --git a/packages/oauth/jwk-jose/src/jose-key.ts b/packages/oauth/jwk-jose/src/jose-key.ts index 7bb0567ce21..e60687313dd 100644 --- a/packages/oauth/jwk-jose/src/jose-key.ts +++ b/packages/oauth/jwk-jose/src/jose-key.ts @@ -1,4 +1,19 @@ -import { JwtVerifyError } from '@atproto/jwk' +import { + Jwk, + JwkError, + JwtCreateError, + JwtHeader, + JwtPayload, + JwtVerifyError, + Key, + RequiredKey, + SignedJwt, + VerifyOptions, + VerifyResult, + jwkValidator, + jwtHeaderSchema, + jwtPayloadSchema, +} from '@atproto/jwk' import { SignJWT, errors, @@ -14,19 +29,6 @@ import { type KeyLike, } from 'jose' -import { - Jwk, - JwkError, - JwtCreateError, - JwtHeader, - JwtPayload, - Key, - SignedJwt, - VerifyOptions, - VerifyPayload, - VerifyResult, - jwkValidator, -} from '@atproto/jwk' import { either } from './util' const { JOSEError } = errors @@ -35,53 +37,97 @@ export type Importable = string | KeyLike | Jwk export type { GenerateKeyPairOptions, GenerateKeyPairResult } -export class JoseKey extends Key { - #keyObj?: KeyLike | Uint8Array - - protected async getKey() { +export class JoseKey extends Key { + /** + * Some runtimes (e.g. Bun) require an `alg` second argument to be set when + * invoking `importJWK`. In order to be compatible with these runtimes, we + * provide the following method to ensure the `alg` is always set. We also + * take the opportunity to ensure that the `alg` is compatible with this key. + */ + protected async getKeyObj(alg: string) { + if (!this.algorithms.includes(alg)) { + throw new JwkError(`Key cannot be used with algorithm "${alg}"`) + } try { - return (this.#keyObj ||= await importJWK(this.jwk as JWK)) + return await importJWK(this.jwk as JWK, alg) } catch (cause) { throw new JwkError('Failed to import JWK', undefined, { cause }) } } - async createJwt(header: JwtHeader, payload: JwtPayload) { - if (header.kid && header.kid !== this.kid) { - throw new JwtCreateError( - `Invalid "kid" (${header.kid}) used to sign with key "${this.kid}"`, - ) - } + async createJwt(header: JwtHeader, payload: JwtPayload): Promise { + try { + const { kid } = header + if (kid && kid !== this.kid) { + throw new JwtCreateError( + `Invalid "kid" (${kid}) used to sign with key "${this.kid}"`, + ) + } - if (!header.alg || !this.algorithms.includes(header.alg)) { - throw new JwtCreateError( - `Invalid "alg" (${header.alg}) used to sign with key "${this.kid}"`, - ) - } + const { alg } = header + if (!alg) { + throw new JwtCreateError('Missing "alg" in JWT header') + } + + const keyObj = await this.getKeyObj(alg) + const jwtBuilder = new SignJWT(payload).setProtectedHeader({ + ...header, + alg, + kid: this.kid, + }) + + const signedJwt = await jwtBuilder.sign(keyObj) - const keyObj = await this.getKey() - return new SignJWT(payload) - .setProtectedHeader({ ...header, kid: this.kid }) - .sign(keyObj) as Promise + return signedJwt as SignedJwt + } catch (cause) { + if (cause instanceof JOSEError) { + throw new JwtCreateError(cause.message, cause.code, { cause }) + } else { + throw JwtCreateError.from(cause) + } + } } - async verifyJwt< - P extends VerifyPayload = JwtPayload, - C extends string = string, - >(token: SignedJwt, options?: VerifyOptions): Promise> { + async verifyJwt( + token: SignedJwt, + options?: VerifyOptions, + ): Promise> { try { - const keyObj = await this.getKey() - const result = await jwtVerify(token, keyObj, { - ...options, - algorithms: this.algorithms, - } as JWTVerifyOptions) - - return result as VerifyResult - } catch (error) { - if (error instanceof JOSEError) { - throw new JwtVerifyError(error.message, error.code, { cause: error }) + const result = await jwtVerify( + token, + async ({ alg }) => this.getKeyObj(alg), + { ...options, algorithms: this.algorithms } as JWTVerifyOptions, + ) + + // @NOTE if all tokens are signed exclusively through createJwt(), then + // there should be no need to parse the payload and headers here. But + // since the JWT could have been signed with the same key from somewhere + // else, let's parse it to ensure the integrity (and type safety) of the + // data. + const headerParsed = jwtHeaderSchema.safeParse(result.protectedHeader) + if (!headerParsed.success) { + throw new JwtVerifyError('Invalid JWT header', undefined, { + cause: headerParsed.error, + }) + } + + const payloadParsed = jwtPayloadSchema.safeParse(result.payload) + if (!payloadParsed.success) { + throw new JwtVerifyError('Invalid JWT payload', undefined, { + cause: payloadParsed.error, + }) + } + + return { + protectedHeader: headerParsed.data, + // "requiredClaims" enforced by jwtVerify() + payload: payloadParsed.data as RequiredKey, + } + } catch (cause) { + if (cause instanceof JOSEError) { + throw new JwtVerifyError(cause.message, cause.code, { cause }) } else { - throw JwtVerifyError.from(error) + throw JwtVerifyError.from(cause) } } } diff --git a/packages/oauth/jwk-webcrypto/package.json b/packages/oauth/jwk-webcrypto/package.json index 628905a7489..bb3bf3bd172 100644 --- a/packages/oauth/jwk-webcrypto/package.json +++ b/packages/oauth/jwk-webcrypto/package.json @@ -25,7 +25,8 @@ }, "dependencies": { "@atproto/jwk": "workspace:*", - "@atproto/jwk-jose": "workspace:*" + "@atproto/jwk-jose": "workspace:*", + "zod": "^3.23.8" }, "devDependencies": { "typescript": "^5.6.3" diff --git a/packages/oauth/jwk-webcrypto/src/webcrypto-key.ts b/packages/oauth/jwk-webcrypto/src/webcrypto-key.ts index 32ba8757ac0..74dd24b379c 100644 --- a/packages/oauth/jwk-webcrypto/src/webcrypto-key.ts +++ b/packages/oauth/jwk-webcrypto/src/webcrypto-key.ts @@ -1,9 +1,20 @@ -import { Jwk, jwkSchema } from '@atproto/jwk' +import { JwkError, jwkSchema } from '@atproto/jwk' import { GenerateKeyPairOptions, JoseKey } from '@atproto/jwk-jose' +import z from 'zod' import { fromSubtleAlgorithm, isCryptoKeyPair } from './util.js' -export class WebcryptoKey extends JoseKey { +// Webcrypto keys are bound to a single algorithm +export const jwkWithAlgSchema = z.intersection( + jwkSchema, + z.object({ alg: z.string() }), +) + +export type JwkWithAlg = z.infer + +export class WebcryptoKey< + J extends JwkWithAlg = JwkWithAlg, +> extends JoseKey { // We need to override the static method generate from JoseKey because // the browser needs both the private and public keys static override async generate( @@ -26,29 +37,35 @@ export class WebcryptoKey extends JoseKey { // > The "use" and "key_ops" JWK members SHOULD NOT be used together; [...] // > Applications should specify which of these members they use. - const { key_ops: _, ...jwk } = await crypto.subtle.exportKey( + const { + key_ops, + use, + alg = fromSubtleAlgorithm(cryptoKeyPair.privateKey.algorithm), + ...jwk + } = await crypto.subtle.exportKey( 'jwk', cryptoKeyPair.privateKey.extractable ? cryptoKeyPair.privateKey : cryptoKeyPair.publicKey, ) - const use = jwk.use ?? 'sig' - const alg = - jwk.alg ?? fromSubtleAlgorithm(cryptoKeyPair.privateKey.algorithm) + if (use && use !== 'sig') { + throw new TypeError(`Unsupported JWK use "${use}"`) + } - if (use !== 'sig') { - throw new TypeError('Unsupported JWK use') + if (key_ops && !key_ops.some((o) => o === 'sign' || o === 'verify')) { + // Make sure that "key_ops", if present, is compatible with "use" + throw new TypeError(`Invalid key_ops "${key_ops}" for "sig" use`) } return new WebcryptoKey( - jwkSchema.parse({ ...jwk, use, kid, alg }), + jwkWithAlgSchema.parse({ ...jwk, kid, alg, use: 'sig' }), cryptoKeyPair, ) } constructor( - jwk: Jwk, + jwk: Readonly, readonly cryptoKeyPair: CryptoKeyPair, ) { super(jwk) @@ -58,12 +75,15 @@ export class WebcryptoKey extends JoseKey { return true } - get privateJwk(): Jwk | undefined { + get privateJwk(): Readonly | undefined { if (super.isPrivate) return this.jwk throw new Error('Private Webcrypto Key not exportable') } - protected override async getKey() { + protected override async getKeyObj(alg: string) { + if (this.jwk.alg !== alg) { + throw new JwkError(`Key cannot be used with algorithm "${alg}"`) + } return this.cryptoKeyPair.privateKey } } diff --git a/packages/oauth/jwk/src/index.ts b/packages/oauth/jwk/src/index.ts index e7296842cef..e640ce5c868 100644 --- a/packages/oauth/jwk/src/index.ts +++ b/packages/oauth/jwk/src/index.ts @@ -1,3 +1,8 @@ +// Since we expose zod schemas, let's expose ZodError (under a generic name) so +// that dependents can catch schema parsing errors without requiring an explicit +// dependency on zod, or risking a conflict in case of mismatching zob versions. +export { ZodError as ValidationError } from 'zod' + export * from './alg.js' export * from './errors.js' export * from './jwk.js' diff --git a/packages/oauth/jwk/src/jwt-verify.ts b/packages/oauth/jwk/src/jwt-verify.ts index 8c80c510d81..9b21a206042 100644 --- a/packages/oauth/jwk/src/jwt-verify.ts +++ b/packages/oauth/jwk/src/jwt-verify.ts @@ -1,7 +1,7 @@ import { JwtHeader, JwtPayload } from './jwt.js' import { RequiredKey } from './util.js' -export type VerifyOptions = { +export type VerifyOptions = { audience?: string | readonly string[] /** in seconds */ clockTolerance?: number @@ -14,9 +14,7 @@ export type VerifyOptions = { requiredClaims?: readonly C[] } -export type VerifyPayload = Record - -export type VerifyResult

= { - payload: RequiredKey

+export type VerifyResult = { + payload: RequiredKey protectedHeader: JwtHeader } diff --git a/packages/oauth/jwk/src/jwt.ts b/packages/oauth/jwk/src/jwt.ts index 76bfdd34084..69bab7a2e28 100644 --- a/packages/oauth/jwk/src/jwt.ts +++ b/packages/oauth/jwk/src/jwt.ts @@ -24,150 +24,154 @@ export const isUnsignedJwt = (data: unknown): data is UnsignedJwt => /** * @see {@link https://www.rfc-editor.org/rfc/rfc7515.html#section-4} */ -export const jwtHeaderSchema = z.object({ - /** "alg" (Algorithm) Header Parameter */ - alg: z.string(), - /** "jku" (JWK Set URL) Header Parameter */ - jku: z.string().url().optional(), - /** "jwk" (JSON Web Key) Header Parameter */ - jwk: z - .object({ - kty: z.string(), - crv: z.string().optional(), - x: z.string().optional(), - y: z.string().optional(), - e: z.string().optional(), - n: z.string().optional(), - }) - .optional(), - /** "kid" (Key ID) Header Parameter */ - kid: z.string().optional(), - /** "x5u" (X.509 URL) Header Parameter */ - x5u: z.string().optional(), - /** "x5c" (X.509 Certificate Chain) Header Parameter */ - x5c: z.array(z.string()).optional(), - /** "x5t" (X.509 Certificate SHA-1 Thumbprint) Header Parameter */ - x5t: z.string().optional(), - /** "x5t#S256" (X.509 Certificate SHA-256 Thumbprint) Header Parameter */ - 'x5t#S256': z.string().optional(), - /** "typ" (Type) Header Parameter */ - typ: z.string().optional(), - /** "cty" (Content Type) Header Parameter */ - cty: z.string().optional(), - /** "crit" (Critical) Header Parameter */ - crit: z.array(z.string()).optional(), -}) +export const jwtHeaderSchema = z + .object({ + /** "alg" (Algorithm) Header Parameter */ + alg: z.string(), + /** "jku" (JWK Set URL) Header Parameter */ + jku: z.string().url().optional(), + /** "jwk" (JSON Web Key) Header Parameter */ + jwk: z + .object({ + kty: z.string(), + crv: z.string().optional(), + x: z.string().optional(), + y: z.string().optional(), + e: z.string().optional(), + n: z.string().optional(), + }) + .optional(), + /** "kid" (Key ID) Header Parameter */ + kid: z.string().optional(), + /** "x5u" (X.509 URL) Header Parameter */ + x5u: z.string().optional(), + /** "x5c" (X.509 Certificate Chain) Header Parameter */ + x5c: z.array(z.string()).optional(), + /** "x5t" (X.509 Certificate SHA-1 Thumbprint) Header Parameter */ + x5t: z.string().optional(), + /** "x5t#S256" (X.509 Certificate SHA-256 Thumbprint) Header Parameter */ + 'x5t#S256': z.string().optional(), + /** "typ" (Type) Header Parameter */ + typ: z.string().optional(), + /** "cty" (Content Type) Header Parameter */ + cty: z.string().optional(), + /** "crit" (Critical) Header Parameter */ + crit: z.array(z.string()).optional(), + }) + .passthrough() export type JwtHeader = z.infer // https://www.iana.org/assignments/jwt/jwt.xhtml -export const jwtPayloadSchema = z.object({ - iss: z.string().optional(), - aud: z.union([z.string(), z.array(z.string()).nonempty()]).optional(), - sub: z.string().optional(), - exp: z.number().int().optional(), - nbf: z.number().int().optional(), - iat: z.number().int().optional(), - jti: z.string().optional(), - htm: z.string().optional(), - htu: z.string().optional(), - ath: z.string().optional(), - acr: z.string().optional(), - azp: z.string().optional(), - amr: z.array(z.string()).optional(), - // https://datatracker.ietf.org/doc/html/rfc7800 - cnf: z - .object({ - kid: z.string().optional(), // Key ID - jwk: jwkPubSchema.optional(), // JWK - jwe: z.string().optional(), // Encrypted key - jku: z.string().url().optional(), // JWK Set URI ("kid" should also be provided) - - // https://datatracker.ietf.org/doc/html/rfc9449#section-6.1 - jkt: z.string().optional(), - - // https://datatracker.ietf.org/doc/html/rfc8705 - 'x5t#S256': z.string().optional(), // X.509 Certificate SHA-256 Thumbprint - - // https://datatracker.ietf.org/doc/html/rfc9203 - osc: z.string().optional(), // OSCORE_Input_Material carrying the parameters for using OSCORE per-message security with implicit key confirmation - }) - .optional(), - - client_id: z.string().optional(), - - scope: z.string().optional(), - nonce: z.string().optional(), - - at_hash: z.string().optional(), - c_hash: z.string().optional(), - s_hash: z.string().optional(), - auth_time: z.number().int().optional(), - - // https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims - - // OpenID: "profile" scope - name: z.string().optional(), - family_name: z.string().optional(), - given_name: z.string().optional(), - middle_name: z.string().optional(), - nickname: z.string().optional(), - preferred_username: z.string().optional(), - gender: z.string().optional(), // OpenID only defines "male" and "female" without forbidding other values - picture: z.string().url().optional(), - profile: z.string().url().optional(), - website: z.string().url().optional(), - birthdate: z - .string() - .regex(/\d{4}-\d{2}-\d{2}/) // YYYY-MM-DD - .optional(), - zoneinfo: z - .string() - .regex(/^[A-Za-z0-9_/]+$/) - .optional(), - locale: z - .string() - .regex(/^[a-z]{2}(-[A-Z]{2})?$/) - .optional(), - updated_at: z.number().int().optional(), - - // OpenID: "email" scope - email: z.string().optional(), - email_verified: z.boolean().optional(), - - // OpenID: "phone" scope - phone_number: z.string().optional(), - phone_number_verified: z.boolean().optional(), - - // OpenID: "address" scope - // https://openid.net/specs/openid-connect-core-1_0.html#AddressClaim - address: z - .object({ - formatted: z.string().optional(), - street_address: z.string().optional(), - locality: z.string().optional(), - region: z.string().optional(), - postal_code: z.string().optional(), - country: z.string().optional(), - }) - .optional(), - - // https://datatracker.ietf.org/doc/html/rfc9396#section-14.2 - authorization_details: z - .array( - z - .object({ - type: z.string(), - // https://datatracker.ietf.org/doc/html/rfc9396#section-2.2 - locations: z.array(z.string()).optional(), - actions: z.array(z.string()).optional(), - datatypes: z.array(z.string()).optional(), - identifier: z.string().optional(), - privileges: z.array(z.string()).optional(), - }) - .passthrough(), - ) - .optional(), -}) +export const jwtPayloadSchema = z + .object({ + iss: z.string().optional(), + aud: z.union([z.string(), z.array(z.string()).nonempty()]).optional(), + sub: z.string().optional(), + exp: z.number().int().optional(), + nbf: z.number().int().optional(), + iat: z.number().int().optional(), + jti: z.string().optional(), + htm: z.string().optional(), + htu: z.string().optional(), + ath: z.string().optional(), + acr: z.string().optional(), + azp: z.string().optional(), + amr: z.array(z.string()).optional(), + // https://datatracker.ietf.org/doc/html/rfc7800 + cnf: z + .object({ + kid: z.string().optional(), // Key ID + jwk: jwkPubSchema.optional(), // JWK + jwe: z.string().optional(), // Encrypted key + jku: z.string().url().optional(), // JWK Set URI ("kid" should also be provided) + + // https://datatracker.ietf.org/doc/html/rfc9449#section-6.1 + jkt: z.string().optional(), + + // https://datatracker.ietf.org/doc/html/rfc8705 + 'x5t#S256': z.string().optional(), // X.509 Certificate SHA-256 Thumbprint + + // https://datatracker.ietf.org/doc/html/rfc9203 + osc: z.string().optional(), // OSCORE_Input_Material carrying the parameters for using OSCORE per-message security with implicit key confirmation + }) + .optional(), + + client_id: z.string().optional(), + + scope: z.string().optional(), + nonce: z.string().optional(), + + at_hash: z.string().optional(), + c_hash: z.string().optional(), + s_hash: z.string().optional(), + auth_time: z.number().int().optional(), + + // https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims + + // OpenID: "profile" scope + name: z.string().optional(), + family_name: z.string().optional(), + given_name: z.string().optional(), + middle_name: z.string().optional(), + nickname: z.string().optional(), + preferred_username: z.string().optional(), + gender: z.string().optional(), // OpenID only defines "male" and "female" without forbidding other values + picture: z.string().url().optional(), + profile: z.string().url().optional(), + website: z.string().url().optional(), + birthdate: z + .string() + .regex(/\d{4}-\d{2}-\d{2}/) // YYYY-MM-DD + .optional(), + zoneinfo: z + .string() + .regex(/^[A-Za-z0-9_/]+$/) + .optional(), + locale: z + .string() + .regex(/^[a-z]{2}(-[A-Z]{2})?$/) + .optional(), + updated_at: z.number().int().optional(), + + // OpenID: "email" scope + email: z.string().optional(), + email_verified: z.boolean().optional(), + + // OpenID: "phone" scope + phone_number: z.string().optional(), + phone_number_verified: z.boolean().optional(), + + // OpenID: "address" scope + // https://openid.net/specs/openid-connect-core-1_0.html#AddressClaim + address: z + .object({ + formatted: z.string().optional(), + street_address: z.string().optional(), + locality: z.string().optional(), + region: z.string().optional(), + postal_code: z.string().optional(), + country: z.string().optional(), + }) + .optional(), + + // https://datatracker.ietf.org/doc/html/rfc9396#section-14.2 + authorization_details: z + .array( + z + .object({ + type: z.string(), + // https://datatracker.ietf.org/doc/html/rfc9396#section-2.2 + locations: z.array(z.string()).optional(), + actions: z.array(z.string()).optional(), + datatypes: z.array(z.string()).optional(), + identifier: z.string().optional(), + privileges: z.array(z.string()).optional(), + }) + .passthrough(), + ) + .optional(), + }) + .passthrough() export type JwtPayload = z.infer diff --git a/packages/oauth/jwk/src/key.ts b/packages/oauth/jwk/src/key.ts index 519661615ed..fabd8e55872 100644 --- a/packages/oauth/jwk/src/key.ts +++ b/packages/oauth/jwk/src/key.ts @@ -1,12 +1,14 @@ import { jwkAlgorithms } from './alg.js' import { JwkError } from './errors.js' import { Jwk, jwkSchema } from './jwk.js' -import { VerifyOptions, VerifyPayload, VerifyResult } from './jwt-verify.js' +import { VerifyOptions, VerifyResult } from './jwt-verify.js' import { JwtHeader, JwtPayload, SignedJwt } from './jwt.js' import { cachedGetter } from './util.js' -export abstract class Key { - constructor(protected readonly jwk: Readonly) { +const jwkSchemaReadonly = jwkSchema.readonly() + +export abstract class Key { + constructor(protected readonly jwk: Readonly) { // A key should always be used either for signing or encryption. if (!jwk.use) throw new JwkError('Missing "use" Parameter value') } @@ -24,25 +26,28 @@ export abstract class Key { return false } - get privateJwk(): Jwk | undefined { + get privateJwk(): Readonly | undefined { return this.isPrivate ? this.jwk : undefined } @cachedGetter - get publicJwk(): Jwk | undefined { + get publicJwk(): + | Readonly & { d?: never }> + | undefined { if (this.isSymetric) return undefined - if (this.isPrivate) { - const { d: _, ...jwk } = this.jwk as any - return jwk - } - return this.jwk + + return jwkSchemaReadonly.parse({ + ...this.jwk, + d: undefined, + k: undefined, + }) as Exclude & { d?: never } } @cachedGetter - get bareJwk(): Jwk | undefined { + get bareJwk(): Readonly | undefined { if (this.isSymetric) return undefined const { kty, crv, e, n, x, y } = this.jwk as any - return jwkSchema.parse({ crv, e, kty, n, x, y }) + return jwkSchemaReadonly.parse({ crv, e, kty, n, x, y }) } get use() { @@ -64,7 +69,7 @@ export abstract class Key { } get crv() { - return (this.jwk as { crv: undefined } | Extract).crv + return (this.jwk as { crv: undefined } | Extract).crv } /** @@ -73,7 +78,7 @@ export abstract class Key { */ @cachedGetter get algorithms(): readonly string[] { - return Array.from(jwkAlgorithms(this.jwk)) + return Object.freeze(Array.from(jwkAlgorithms(this.jwk))) } /** @@ -86,8 +91,8 @@ export abstract class Key { * * @throws {JwtVerifyError} if the JWT is invalid */ - abstract verifyJwt< - P extends VerifyPayload = JwtPayload, - C extends string = string, - >(token: SignedJwt, options?: VerifyOptions): Promise> + abstract verifyJwt( + token: SignedJwt, + options?: VerifyOptions, + ): Promise> } diff --git a/packages/oauth/jwk/src/keyset.ts b/packages/oauth/jwk/src/keyset.ts index d151d3f2c36..ac76a3f32e8 100644 --- a/packages/oauth/jwk/src/keyset.ts +++ b/packages/oauth/jwk/src/keyset.ts @@ -213,13 +213,10 @@ export class Keyset implements Iterable { } } - async verifyJwt< - P extends Record = JwtPayload, - C extends string = string, - >( + async verifyJwt( token: SignedJwt, options?: VerifyOptions, - ): Promise & { key: K }> { + ): Promise & { key: K }> { const { header } = unsafeDecodeJwt(token) const { kid, alg } = header @@ -227,7 +224,7 @@ export class Keyset implements Iterable { for (const key of this.list({ kid, alg })) { try { - const result = await key.verifyJwt(token, options) + const result = await key.verifyJwt(token, options) return { ...result, key } } catch (err) { errors.push(err) diff --git a/packages/oauth/jwk/src/util.ts b/packages/oauth/jwk/src/util.ts index 0a67fc260e1..f3d01180728 100644 --- a/packages/oauth/jwk/src/util.ts +++ b/packages/oauth/jwk/src/util.ts @@ -5,12 +5,12 @@ import { RefinementCtx, ZodIssueCode } from 'zod' export type Simplify = { [K in keyof T]: T[K] } & {} export type Override = Simplify> -export type RequiredKey = Simplify< - string extends K - ? T - : { - [L in K]: Exclude - } & Omit +export type RequiredKey = Simplify< + T & { + [L in K]-?: unknown extends T[L] + ? NonNullable | null + : Exclude + } > // eslint-disable-next-line @typescript-eslint/ban-types diff --git a/packages/oauth/oauth-provider/src/signer/signed-token-payload.ts b/packages/oauth/oauth-provider/src/signer/signed-token-payload.ts index 3383ae8295c..1b366b7a74d 100644 --- a/packages/oauth/oauth-provider/src/signer/signed-token-payload.ts +++ b/packages/oauth/oauth-provider/src/signer/signed-token-payload.ts @@ -27,7 +27,8 @@ export const signedTokenPayloadSchema = z.intersection( jti: tokenIdSchema, sub: subSchema, client_id: clientIdSchema, - }), + }) + .passthrough(), ) export type SignedTokenPayload = Simplify< diff --git a/packages/oauth/oauth-provider/src/signer/signer.ts b/packages/oauth/oauth-provider/src/signer/signer.ts index 09090385e73..ce864807818 100644 --- a/packages/oauth/oauth-provider/src/signer/signer.ts +++ b/packages/oauth/oauth-provider/src/signer/signer.ts @@ -19,7 +19,7 @@ import { signedTokenPayloadSchema, } from './signed-token-payload.js' -export type SignPayload = Omit +export type SignPayload = JwtPayload & { iss?: never } export class Signer { constructor( @@ -27,11 +27,11 @@ export class Signer { public readonly keyset: Keyset, ) {} - async verify

= JwtPayload>( + async verify( token: SignedJwt, - options?: Omit, + options?: Omit, 'issuer'>, ) { - return this.keyset.verifyJwt

(token, { + return this.keyset.verifyJwt(token, { ...options, issuer: [this.issuer], }) @@ -84,18 +84,16 @@ export class Signer { ) } - async verifyAccessToken(token: SignedJwt) { - const result = await this.verify(token, { - typ: 'at+jwt', - }) - - // The result is already type casted as an AccessTokenPayload, but we need - // to actually verify this. That should already be covered by the fact that - // we don't sign 'at+jwt' tokens without a valid token ID. Let's double - // check in case another version/implementation was used to generate the - // token. - signedTokenPayloadSchema.parse(result.payload) - - return result + async verifyAccessToken( + token: SignedJwt, + options?: Omit, 'issuer' | 'typ'>, + ) { + const result = await this.verify(token, { ...options, typ: 'at+jwt' }) + type Payload = typeof result.payload // RequiredKey + return { + protectedHeader: result.protectedHeader, + payload: signedTokenPayloadSchema.parse(result.payload) as Payload & + SignedTokenPayload, + } } } diff --git a/packages/oauth/oauth-provider/src/token/token-manager.ts b/packages/oauth/oauth-provider/src/token/token-manager.ts index 881770d7eca..81d8b4bcb2a 100644 --- a/packages/oauth/oauth-provider/src/token/token-manager.ts +++ b/packages/oauth/oauth-provider/src/token/token-manager.ts @@ -462,6 +462,7 @@ export class TokenManager { case isSignedJwt(token): { const { payload } = await this.signer.verify(token, { clockTolerance: Infinity, + requiredClaims: ['jti'], }) const tokenId = tokenIdSchema.parse(payload.jti) await this.store.deleteToken(tokenId) diff --git a/packages/xrpc-server/package.json b/packages/xrpc-server/package.json index 94c480fa832..475f1357e7d 100644 --- a/packages/xrpc-server/package.json +++ b/packages/xrpc-server/package.json @@ -37,7 +37,7 @@ "@atproto/crypto": "workspace:^", "@types/express": "^4.17.13", "@types/express-serve-static-core": "^4.17.36", - "@types/http-errors": "^2.0.1", + "@types/http-errors": "^2.0.4", "@types/ws": "^8.5.4", "get-port": "^6.1.2", "jest": "^28.1.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b2eda360d72..88318f4e346 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -201,7 +201,7 @@ importers: version: 0.0.1 '@types/http-errors': specifier: ^2.0.1 - version: 2.0.1 + version: 2.0.4 compression: specifier: ^1.7.4 version: 1.7.4 @@ -812,6 +812,9 @@ importers: '@atproto/jwk-jose': specifier: workspace:* version: link:../jwk-jose + zod: + specifier: ^3.23.8 + version: 3.23.8 devDependencies: typescript: specifier: ^5.6.3 @@ -1566,8 +1569,8 @@ importers: specifier: ^4.17.36 version: 4.17.36 '@types/http-errors': - specifier: ^2.0.1 - version: 2.0.1 + specifier: ^2.0.4 + version: 2.0.4 '@types/ws': specifier: ^8.5.4 version: 8.5.4 @@ -6310,8 +6313,8 @@ packages: '@types/node': 18.19.67 dev: true - /@types/http-errors@2.0.1: - resolution: {integrity: sha512-/K3ds8TRAfBvi5vfjuz8y6+GiAYBZ0x4tXv1Av6CWBWn0IlADc+ZX9pMq7oU0fNQPnBwIZl3rmeLp6SBApbxSQ==} + /@types/http-errors@2.0.4: + resolution: {integrity: sha512-D0CFMMtydbJAegzOyHjtiKPLlvnm3iTZyZRSZoLq2mRhDdmLfIWOCYPfQJ4cu2erKghU++QvjcUjp/5h7hESpA==} /@types/is-ci@3.0.0: resolution: {integrity: sha512-Q0Op0hdWbYd1iahB+IFNQcWXFq4O0Q5MwQP7uN0souuQ4rPg1vEYcnIOfr1gY+M+6rc8FGoRaBO1mOOvL29sEQ==} @@ -6447,7 +6450,7 @@ packages: /@types/serve-static@1.15.2: resolution: {integrity: sha512-J2LqtvFYCzaj8pVYKw8klQXrLLk7TBZmQ4ShlcdkELFKGwGMfevMLneMMRkMgZxotOD9wg497LpC7O8PcvAmfw==} dependencies: - '@types/http-errors': 2.0.1 + '@types/http-errors': 2.0.4 '@types/mime': 3.0.1 '@types/node': 18.19.67