Properly validate auth during refresh (#3847)

* Ensure that the credentials used during a refresh correspond to those used to create the OAuth tokens.

* tidy

* Bind the OAuth session to the kid that was used to authenticate the client (private_key_jwt)

* Store the whole authentication method in the client session store rather than the kid only

* tidy

* Improve error reporting in case an invalid `token_endpoint_auth_method` is used in the client metadata document.

* tidy

* tidy

* Improve JAR checks

* tidy

* changeset

* tidy

* Remove schema's `.optional()` modifier when a `.default()` is defined

* tidy

* verify client auth during code exchange

* tidy

* Minor naming improvement

* tidy

* Update .changeset/quiet-pans-fix.md

Co-authored-by: devin ivy <devinivy@gmail.com>

* Update packages/oauth/oauth-client/src/oauth-client-auth.ts

* Use `private_key_jwt` instead of incorrect `client_secret_jwt` as authentication method for confidential clients

* style

* code split

* dead code removal

* Represent missing client auth with a `null` instead of "none" when storing request data.

* Allow storing `null` in authorization_request's `clientAuth` json column

* document

* tidy

* Remove non-standard behavior that allowed client to authenticate through JAR

* Improved error messages

* Parse JSON encoded Authorization Request Parameters

* Use `application/x-www-form-urlencoded` content instead of JSON for OAuth requests

Fixes: #3723

* tidy

* tidy

* tidy

* tidy

* code style

* remove un-necessary checks

* tidy

* Pre-process number too

* improved type checking

* add missing exports

* fix merge conflict

* tidy

* Remove invalid default for `code_challenge_method` authorization request parameter

* tidy

* Delete inaccurate changeset

* PR comment

* tidy

* Update OAuth client credentials factory to return headers and payload separately.

* tidy

* Renamed `clientAuthCheck` to `validateClientAuth`

* Validate presence of DPoP proofs sooner when processing token requests.

Fixes: #3859

* Protect against concurrent use of request code

* tidy

* tidy

* Update packages/oauth/oauth-provider/src/client/client.ts

Co-authored-by: devin ivy <devinivy@gmail.com>

* Review comments

* Add missing `exp` claim in client attestation JWT

* fixup! Review comments

* Review comments

* Refactor: explicit optionality of unsigned JAR issuer & audience

* Use client attestation's `exp` claim to determine the life time of JWT's `jti` nonce.

* Fix PDS: consumeRequestCode should delete request data

* tidy

* tidy

* Unused code removal

* Restore "Native clients must authenticate using "none" method" check

* tidy

* tidy

* cleanup

* comment

* Allow missing DPoP header during PAR request if `dpop_jkt` is provided

* tidy

---------

Co-authored-by: devin ivy <devinivy@gmail.com>

.changeset/chilly-eagles-try.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": minor
+---
+
+Improve error reporting in case an invalid `token_endpoint_auth_method` is used in the client metadata document.

.changeset/gentle-months-walk.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Fix a flawed logic preventing the proper error from being propagated upon failed code grant

.changeset/heavy-steaks-destroy.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": minor
+---
+
+Change the default client authentication method in the client document metadata to "client_secret_basic" (as per spec)

.changeset/lazy-roses-switch.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Verify the "aud" claim of JAR requests

.changeset/perfect-boxes-kneel.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": minor
+---
+
+OAuthProvider `requestStore` option is now required

.changeset/quick-sloths-marry.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client-node": patch
+---
+
+Minor typing change

.changeset/quiet-pans-fix.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Verify the presence of a "kid" in signed JAR headers

.changeset/rare-pillows-search.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": minor
+---
+
+Ensure that the credentials used during a refresh correspond to those used to create the OAuth tokens.

.changeset/shy-adults-pay.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Properly authenticate revoke requests by requiring a DPoP proof to better comply with OAuth 2.0 Token Revocation (RFC7009)

.changeset/six-beers-notice.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client": minor
+---
+
+Bind the OAuth session to the kid that was used to authenticate the client (private_key_jwt)

.changeset/sixty-nails-knock.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": minor
+---
+
+Represent missing client auth with a `null` instead of "none" when storing request data.

.changeset/slow-boats-hide.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client": patch
+---
+
+Add missing `exp` claim in client attestation JWT

.changeset/smart-kiwis-unite.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Remove invalid default for `code_challenge_method` authorization request parameter

.changeset/smooth-laws-explain.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Remove schema's `.optional()` modifier when a `.default()` is defined

.changeset/strong-buckets-fetch.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Allow missing DPoP header during PAR request if `dpop_jkt` is provided

.changeset/sweet-bottles-confess.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Protect against concurrent use of request code

.changeset/tender-berries-melt.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Prevent using `none` as `token_endpoint_auth_signing_alg_values_supported` in authorization server metadata documents.

.changeset/thin-waves-decide.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Validate presence of DPoP proofs sooner when processing token requests

.changeset/wet-kangaroos-tease.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Use client attestation's `exp` claim to determine the life time of JWT's `jti` nonce.

.changeset/wicked-coats-repeat.md

@@ -0,0 +1,5 @@
+---
+"@atproto/jwk": minor
+---
+
+Rename `findKey` to `findPrivateKey` to better reflect the method's behavior

.changeset/wild-rules-repeat.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Silently ignore "Refresh token replayed" errors when revoking a refresh token that has already been revoked.

packages/oauth/jwk/src/keyset.ts

@@ -153,11 +153,11 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
     }
   }
 
-  findKey({ kid, alg, use }: KeySearch): [key: Key, alg: string] {
+  findPrivateKey({ kid, alg, use }: KeySearch): [key: Key, alg: string] {
     const matchingKeys: Key[] = []
 
     for (const key of this.list({ kid, alg, use })) {
-      // Not a signing key
+      // Not a private key
       if (!key.isPrivate) continue
 
       // Skip negotiation if a specific "alg" was provided
@@ -186,7 +186,7 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
     }
 
     throw new JwkError(
-      `No signing key found for ${kid || alg || use || '<unknown>'}`,
+      `No private key found for ${kid || alg || use || '<unknown>'}`,
       ERR_JWK_NOT_FOUND,
     )
   }
@@ -200,7 +200,11 @@ export class Keyset<K extends Key = Key> implements Iterable<K> {
     payload: JwtPayload | JwtPayloadGetter,
   ): Promise<SignedJwt> {
     try {
-      const [key, alg] = this.findKey({ alg: sAlg, kid: sKid, use: 'sig' })
+      const [key, alg] = this.findPrivateKey({
+        alg: sAlg,
+        kid: sKid,
+        use: 'sig',
+      })
       const protectedHeader = { ...header, alg, kid: key.kid }
 
       if (typeof payload === 'function') {

packages/oauth/oauth-client-node/src/node-dpop-store.ts

@@ -11,9 +11,10 @@ type ToDpopJwkValue<V extends { dpopKey: Key }> = Omit<V, 'dpopKey'> & {
  * Utility function that allows to simplify the store interface by exposing a
  * JWK (JSON) instead of a Key instance.
  */
-export function toDpopKeyStore<K extends string, V extends { dpopKey: Key }>(
-  store: SimpleStore<K, ToDpopJwkValue<V>>,
-): SimpleStore<K, V> {
+export function toDpopKeyStore<
+  K extends string,
+  V extends { dpopKey: Key; dpopJwk?: never },
+>(store: SimpleStore<K, ToDpopJwkValue<V>>): SimpleStore<K, V> {
   return {
     async set(sub: K, { dpopKey, ...data }: V) {
       const dpopJwk = dpopKey.privateJwk

packages/oauth/oauth-client/src/errors/auth-method-unsatisfiable-error.ts

@@ -0,0 +1 @@
+export class AuthMethodUnsatisfiableError extends Error {}

packages/oauth/oauth-client/src/oauth-client-auth.ts

@@ -0,0 +1,182 @@
+import { Keyset } from '@atproto/jwk'
+import {
+  CLIENT_ASSERTION_TYPE_JWT_BEARER,
+  OAuthAuthorizationServerMetadata,
+  OAuthClientCredentials,
+} from '@atproto/oauth-types'
+import { FALLBACK_ALG } from './constants.js'
+import { AuthMethodUnsatisfiableError } from './errors/auth-method-unsatisfiable-error.js'
+import { Runtime } from './runtime.js'
+import { ClientMetadata } from './types.js'
+import { Awaitable } from './util.js'
+
+export type ClientAuthMethod =
+  | { method: 'none' }
+  | { method: 'private_key_jwt'; kid: string }
+
+export function negotiateClientAuthMethod(
+  serverMetadata: OAuthAuthorizationServerMetadata,
+  clientMetadata: ClientMetadata,
+  keyset?: Keyset,
+): ClientAuthMethod {
+  const method = clientMetadata.token_endpoint_auth_method
+
+  // @NOTE ATproto spec requires that AS support both "none" and
+  // "private_key_jwt", and that clients use one of the other. The following
+  // check ensures that the AS is indeed compliant with this client's
+  // configuration.
+  const methods = supportedMethods(serverMetadata)
+  if (!methods.includes(method)) {
+    throw new Error(
+      `The server does not support "${method}" authentication. Supported methods are: ${methods.join(
+        ', ',
+      )}.`,
+    )
+  }
+
+  if (method === 'private_key_jwt') {
+    // Invalid client configuration. This should not happen as
+    // "validateClientMetadata" already check this.
+    if (!keyset) throw new Error('A keyset is required for private_key_jwt')
+
+    const alg = supportedAlgs(serverMetadata)
+
+    // @NOTE we can't use `keyset.findPrivateKey` here because we can't enforce
+    // that the returned key contains a "kid". The following implementation is
+    // more robust against keysets containing keys without a "kid" property.
+    for (const key of keyset.list({ use: 'sig', alg })) {
+      // Return the first key from the key set that matches the server's
+      // supported algorithms.
+      if (key.isPrivate && key.kid) {
+        return { method: 'private_key_jwt', kid: key.kid }
+      }
+    }
+
+    throw new Error(
+      alg.includes(FALLBACK_ALG)
+        ? `Client authentication method "${method}" requires at least one "${FALLBACK_ALG}" signing key with a "kid" property`
+        : // AS is not compliant with the ATproto OAuth spec.
+          `Authorization server requires "${method}" authentication method, but does not support "${FALLBACK_ALG}" algorithm.`,
+    )
+  }
+
+  if (method === 'none') {
+    return { method: 'none' }
+  }
+
+  throw new Error(
+    `The ATProto OAuth spec requires that client use either "none" or "private_key_jwt" authentication method.` +
+      (method === 'client_secret_basic'
+        ? ' You might want to explicitly set "token_endpoint_auth_method" to one of those values in the client metadata document.'
+        : ` You set "${method}" which is not allowed.`),
+  )
+}
+
+export type ClientCredentialsFactory = () => Awaitable<{
+  headers?: Record<string, string>
+  payload?: OAuthClientCredentials
+}>
+
+/**
+ * @throws {AuthMethodUnsatisfiableError} if the authentication method is no
+ * long usable (either because the AS changed, of because the key is no longer
+ * available in the keyset).
+ */
+export function createClientCredentialsFactory(
+  authMethod: ClientAuthMethod,
+  serverMetadata: OAuthAuthorizationServerMetadata,
+  clientMetadata: ClientMetadata,
+  runtime: Runtime,
+  keyset?: Keyset,
+): ClientCredentialsFactory {
+  // Ensure the AS still supports the auth method.
+  if (!supportedMethods(serverMetadata).includes(authMethod.method)) {
+    throw new AuthMethodUnsatisfiableError(
+      `Client authentication method "${authMethod.method}" no longer supported`,
+    )
+  }
+
+  if (authMethod.method === 'none') {
+    return () => ({
+      payload: {
+        client_id: clientMetadata.client_id,
+      },
+    })
+  }
+
+  if (authMethod.method === 'private_key_jwt') {
+    try {
+      // The client used to be a confidential client but no longer has a keyset.
+      if (!keyset) throw new Error('A keyset is required for private_key_jwt')
+
+      // @NOTE throws if no matching key can be found
+      const [key, alg] = keyset.findPrivateKey({
+        use: 'sig',
+        kid: authMethod.kid,
+        alg: supportedAlgs(serverMetadata),
+      })
+
+      // https://www.rfc-editor.org/rfc/rfc7523.html#section-3
+      return async () => ({
+        payload: {
+          client_id: clientMetadata.client_id,
+          client_assertion_type: CLIENT_ASSERTION_TYPE_JWT_BEARER,
+          client_assertion: await key.createJwt(
+            { alg },
+            {
+              // > The JWT MUST contain an "iss" (issuer) claim that contains a
+              // > unique identifier for the entity that issued the JWT.
+              iss: clientMetadata.client_id,
+              // > For client authentication, the subject MUST be the
+              // > "client_id" of the OAuth client.
+              sub: clientMetadata.client_id,
+              // > The JWT MUST contain an "aud" (audience) claim containing a value
+              // > that identifies the authorization server as an intended audience.
+              // > The token endpoint URL of the authorization server MAY be used as a
+              // > value for an "aud" element to identify the authorization server as an
+              // > intended audience of the JWT.
+              aud: serverMetadata.issuer,
+              // > The JWT MAY contain a "jti" (JWT ID) claim that provides a
+              // > unique identifier for the token.
+              jti: await runtime.generateNonce(),
+              // > The JWT MAY contain an "iat" (issued at) claim that
+              // > identifies the time at which the JWT was issued.
+              iat: Math.floor(Date.now() / 1000),
+              // > The JWT MUST contain an "exp" (expiration time) claim that
+              // > limits the time window during which the JWT can be used.
+              exp: Math.floor(Date.now() / 1000) + 60, // 1 minute
+            },
+          ),
+        },
+      })
+    } catch (cause) {
+      throw new AuthMethodUnsatisfiableError('Failed to load private key', {
+        cause,
+      })
+    }
+  }
+
+  throw new AuthMethodUnsatisfiableError(
+    // @ts-expect-error
+    `Unsupported auth method ${authMethod.method}`,
+  )
+}
+
+function supportedMethods(serverMetadata: OAuthAuthorizationServerMetadata) {
+  return serverMetadata['token_endpoint_auth_methods_supported']
+}
+
+function supportedAlgs(serverMetadata: OAuthAuthorizationServerMetadata) {
+  return (
+    serverMetadata['token_endpoint_auth_signing_alg_values_supported'] ?? [
+      // @NOTE If not specified, assume that the server supports the ES256
+      // algorithm, as prescribed by the spec:
+      //
+      // > Clients and Authorization Servers currently must support the ES256
+      // > cryptographic system [for client authentication].
+      //
+      // https://atproto.com/specs/oauth#confidential-client-authentication
+      FALLBACK_ALG,
+    ]
+  )
+}

packages/oauth/oauth-client/src/oauth-client.ts

@@ -26,12 +26,14 @@ import {
 import { IdentityResolver } from '@atproto-labs/identity-resolver'
 import { SimpleStoreMemory } from '@atproto-labs/simple-store-memory'
 import { FALLBACK_ALG } from './constants.js'
+import { AuthMethodUnsatisfiableError } from './errors/auth-method-unsatisfiable-error.js'
 import { TokenRevokedError } from './errors/token-revoked-error.js'
 import {
   AuthorizationServerMetadataCache,
   OAuthAuthorizationServerMetadataResolver,
 } from './oauth-authorization-server-metadata-resolver.js'
 import { OAuthCallbackError } from './oauth-callback-error.js'
+import { negotiateClientAuthMethod } from './oauth-client-auth.js'
 import {
   OAuthProtectedResourceMetadataResolver,
   ProtectedResourceMetadataCache,
@@ -290,11 +292,17 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
       metadata.dpop_signing_alg_values_supported || [FALLBACK_ALG],
     )
 
+    const authMethod = negotiateClientAuthMethod(
+      metadata,
+      this.clientMetadata,
+      this.keyset,
+    )
     const state = await this.runtime.generateNonce()
 
     await this.stateStore.set(state, {
       iss: metadata.issuer,
       dpopKey,
+      authMethod,
       verifier: pkce.verifier,
       appState: options?.state,
     })
@@ -327,7 +335,11 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
     }
 
     if (metadata.pushed_authorization_request_endpoint) {
-      const server = await this.serverFactory.fromMetadata(metadata, dpopKey)
+      const server = await this.serverFactory.fromMetadata(
+        metadata,
+        authMethod,
+        dpopKey,
+      )
       const parResponse = await server.request(
         'pushed_authorization_request',
         parameters,
@@ -423,6 +435,8 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
 
       const server = await this.serverFactory.fromIssuer(
         stateData.iss,
+        // Using the literal 'legacy' if the authMethod is not defined (because stateData was created through an old version of this lib)
+        stateData.authMethod ?? 'legacy',
         stateData.dpopKey,
       )
 
@@ -455,6 +469,7 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
       try {
         await this.sessionGetter.setStored(tokenSet.sub, {
           dpopKey: stateData.dpopKey,
+          authMethod: server.authMethod,
           tokenSet,
         })
 
@@ -486,24 +501,45 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
     // sub arg is lightly typed for convenience of library user
     assertAtprotoDid(sub)
 
-    const { dpopKey, tokenSet } = await this.sessionGetter.get(sub, {
+    const {
+      dpopKey,
+      authMethod = 'legacy',
+      tokenSet,
+    } = await this.sessionGetter.get(sub, {
       noCache: refresh === true,
       allowStale: refresh === false,
     })
 
-    const server = await this.serverFactory.fromIssuer(tokenSet.iss, dpopKey, {
-      noCache: refresh === true,
-      allowStale: refresh === false,
-    })
+    try {
+      const server = await this.serverFactory.fromIssuer(
+        tokenSet.iss,
+        authMethod,
+        dpopKey,
+        {
+          noCache: refresh === true,
+          allowStale: refresh === false,
+        },
+      )
 
-    return this.createSession(server, sub)
+      return this.createSession(server, sub)
+    } catch (err) {
+      if (err instanceof AuthMethodUnsatisfiableError) {
+        await this.sessionGetter.delStored(sub, err)
+      }
+
+      throw err
+    }
   }
 
   async revoke(sub: string) {
     // sub arg is lightly typed for convenience of library user
     assertAtprotoDid(sub)
 
-    const { dpopKey, tokenSet } = await this.sessionGetter.get(sub, {
+    const {
+      dpopKey,
+      authMethod = 'legacy',
+      tokenSet,
+    } = await this.sessionGetter.get(sub, {
       allowStale: true,
     })
 
@@ -511,7 +547,11 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
     // the tokens to be deleted even if it was not possible to fetch the issuer
     // data.
     try {
-      const server = await this.serverFactory.fromIssuer(tokenSet.iss, dpopKey)
+      const server = await this.serverFactory.fromIssuer(
+        tokenSet.iss,
+        authMethod,
+        dpopKey,
+      )
       await server.revoke(tokenSet.access_token)
     } finally {
       await this.sessionGetter.delStored(sub, new TokenRevokedError(sub))

packages/oauth/oauth-client/src/oauth-server-agent.ts

@@ -1,10 +1,8 @@
 import { AtprotoDid } from '@atproto/did'
 import { Key, Keyset } from '@atproto/jwk'
 import {
-  CLIENT_ASSERTION_TYPE_JWT_BEARER,
   OAuthAuthorizationRequestPar,
   OAuthAuthorizationServerMetadata,
-  OAuthClientCredentials,
   OAuthEndpointName,
   OAuthParResponse,
   OAuthTokenRequest,
@@ -17,9 +15,13 @@ import {
   AtprotoTokenResponse,
   atprotoTokenResponseSchema,
 } from './atproto-token-response.js'
-import { FALLBACK_ALG } from './constants.js'
 import { TokenRefreshError } from './errors/token-refresh-error.js'
 import { dpopFetchWrapper } from './fetch-dpop.js'
+import {
+  ClientAuthMethod,
+  ClientCredentialsFactory,
+  createClientCredentialsFactory,
+} from './oauth-client-auth.js'
 import { OAuthResolver } from './oauth-resolver.js'
 import { OAuthResponseError } from './oauth-response-error.js'
 import { Runtime } from './runtime.js'
@@ -43,8 +45,13 @@ export type DpopNonceCache = SimpleStore<string, string>
 
 export class OAuthServerAgent {
   protected dpopFetch: Fetch<unknown>
+  protected clientCredentialsFactory: ClientCredentialsFactory
 
+  /**
+   * @throws see {@link createClientCredentialsFactory}
+   */
   constructor(
+    readonly authMethod: ClientAuthMethod,
     readonly dpopKey: Key,
     readonly serverMetadata: OAuthAuthorizationServerMetadata,
     readonly clientMetadata: ClientMetadata,
@@ -54,6 +61,14 @@ export class OAuthServerAgent {
     readonly keyset?: Keyset,
     fetch?: Fetch,
   ) {
+    this.clientCredentialsFactory = createClientCredentialsFactory(
+      authMethod,
+      serverMetadata,
+      clientMetadata,
+      runtime,
+      keyset,
+    )
+
     this.dpopFetch = dpopFetchWrapper<void>({
       fetch: bindFetch(fetch),
       key: dpopKey,
@@ -204,7 +219,7 @@ export class OAuthServerAgent {
     const url = this.serverMetadata[`${endpoint}_endpoint`]
     if (!url) throw new Error(`No ${endpoint} endpoint available`)
 
-    const auth = await this.buildClientAuth(endpoint)
+    const auth = await this.clientCredentialsFactory()
 
     // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-13#section-3.2.2
     // https://datatracker.ietf.org/doc/html/rfc7009#section-2.1
@@ -232,73 +247,6 @@ export class OAuthServerAgent {
       throw new OAuthResponseError(response, json)
     }
   }
-
-  async buildClientAuth(endpoint: OAuthEndpointName): Promise<{
-    headers?: Record<string, string>
-    payload: OAuthClientCredentials
-  }> {
-    const methodSupported =
-      this.serverMetadata[`token_endpoint_auth_methods_supported`]
-
-    const method = this.clientMetadata[`token_endpoint_auth_method`]
-
-    if (
-      method === 'private_key_jwt' ||
-      (this.keyset &&
-        !method &&
-        (methodSupported?.includes('private_key_jwt') ?? false))
-    ) {
-      if (!this.keyset) throw new Error('No keyset available')
-
-      try {
-        const alg =
-          this.serverMetadata[
-            `token_endpoint_auth_signing_alg_values_supported`
-          ] ?? FALLBACK_ALG
-
-        // If jwks is defined, make sure to only sign using a key that exists in
-        // the jwks. If jwks_uri is defined, we can't be sure that the key we're
-        // looking for is in there so we will just assume it is.
-        const kid = this.clientMetadata.jwks?.keys
-          .map(({ kid }) => kid)
-          .filter((v): v is string => typeof v === 'string')
-
-        return {
-          payload: {
-            client_id: this.clientMetadata.client_id,
-            client_assertion_type: CLIENT_ASSERTION_TYPE_JWT_BEARER,
-            client_assertion: await this.keyset.createJwt(
-              { alg, kid },
-              {
-                iss: this.clientMetadata.client_id,
-                sub: this.clientMetadata.client_id,
-                aud: this.serverMetadata.issuer,
-                jti: await this.runtime.generateNonce(),
-                iat: Math.floor(Date.now() / 1000),
-              },
-            ),
-          },
-        }
-      } catch (err) {
-        if (method === 'private_key_jwt') throw err
-
-        // Else try next method
-      }
-    }
-
-    if (
-      method === 'none' ||
-      (!method && (methodSupported?.includes('none') ?? true))
-    ) {
-      return {
-        payload: {
-          client_id: this.clientMetadata.client_id,
-        },
-      }
-    }
-
-    throw new Error(`Unsupported ${endpoint} authentication method`)
-  }
 }
 
 function wwwFormUrlEncode(payload: Record<string, undefined | unknown>) {

packages/oauth/oauth-client/src/oauth-server-factory.ts

@@ -2,6 +2,10 @@ import { Key, Keyset } from '@atproto/jwk'
 import { OAuthAuthorizationServerMetadata } from '@atproto/oauth-types'
 import { Fetch } from '@atproto-labs/fetch'
 import { GetCachedOptions } from './oauth-authorization-server-metadata-resolver.js'
+import {
+  ClientAuthMethod,
+  negotiateClientAuthMethod,
+} from './oauth-client-auth.js'
 import { OAuthResolver } from './oauth-resolver.js'
 import { DpopNonceCache, OAuthServerAgent } from './oauth-server-agent.js'
 import { Runtime } from './runtime.js'
@@ -17,19 +21,50 @@ export class OAuthServerFactory {
     readonly dpopNonceCache: DpopNonceCache,
   ) {}
 
-  async fromIssuer(issuer: string, dpopKey: Key, options?: GetCachedOptions) {
+  /**
+   * @param authMethod `undefined` means that we are restoring a session that
+   * was created before we started storing the `authMethod` in the session. In
+   * that case, we will use the first key from the keyset.
+   *
+   * Support for this might be removed in the future.
+   *
+   * @throws see {@link OAuthServerFactory.fromMetadata}
+   */
+  async fromIssuer(
+    issuer: string,
+    authMethod: 'legacy' | ClientAuthMethod,
+    dpopKey: Key,
+    options?: GetCachedOptions,
+  ) {
     const serverMetadata = await this.resolver.getAuthorizationServerMetadata(
       issuer,
       options,
     )
-    return this.fromMetadata(serverMetadata, dpopKey)
+
+    if (authMethod === 'legacy') {
+      // @NOTE Because we were previously not storing the authMethod in the
+      // session data, we provide a backwards compatible implementation by
+      // computing it here.
+      authMethod = negotiateClientAuthMethod(
+        serverMetadata,
+        this.clientMetadata,
+        this.keyset,
+      )
+    }
+
+    return this.fromMetadata(serverMetadata, authMethod, dpopKey)
   }
 
+  /**
+   * @throws see {@link OAuthServerAgent}
+   */
   async fromMetadata(
     serverMetadata: OAuthAuthorizationServerMetadata,
+    authMethod: ClientAuthMethod,
     dpopKey: Key,
   ) {
     return new OAuthServerAgent(
+      authMethod,
       dpopKey,
       serverMetadata,
       this.clientMetadata,

packages/oauth/oauth-client/src/session-getter.ts

@@ -5,9 +5,11 @@ import {
   GetCachedOptions,
   SimpleStore,
 } from '@atproto-labs/simple-store'
+import { AuthMethodUnsatisfiableError } from './errors/auth-method-unsatisfiable-error.js'
 import { TokenInvalidError } from './errors/token-invalid-error.js'
 import { TokenRefreshError } from './errors/token-refresh-error.js'
 import { TokenRevokedError } from './errors/token-revoked-error.js'
+import { ClientAuthMethod } from './oauth-client-auth.js'
 import { OAuthResponseError } from './oauth-response-error.js'
 import { TokenSet } from './oauth-server-agent.js'
 import { OAuthServerFactory } from './oauth-server-factory.js'
@@ -16,6 +18,10 @@ import { CustomEventTarget, combineSignals, timeoutSignal } from './util.js'
 
 export type Session = {
   dpopKey: Key
+  /**
+   * Previous implementation of this lib did not define an `authMethod`
+   */
+  authMethod?: ClientAuthMethod
   tokenSet: TokenSet
 }
 
@@ -51,7 +57,7 @@ export class SessionGetter extends CachedGetter<AtprotoDid, Session> {
     private readonly runtime: Runtime,
   ) {
     super(
-      async (sub, options, storedSession): Promise<Session> => {
+      async (sub, options, storedSession) => {
         // There needs to be a previous session to be able to refresh. If
         // storedSession is undefined, it means that the store does not contain
         // a session for the given sub.
@@ -73,7 +79,7 @@ export class SessionGetter extends CachedGetter<AtprotoDid, Session> {
         // concurrent access (which, normally, should not happen if a proper
         // runtime lock was provided).
 
-        const { dpopKey, tokenSet } = storedSession
+        const { dpopKey, authMethod = 'legacy', tokenSet } = storedSession
 
         if (sub !== tokenSet.sub) {
           // Fool-proofing (e.g. against invalid session storage)
@@ -94,7 +100,11 @@ export class SessionGetter extends CachedGetter<AtprotoDid, Session> {
         // always possible. If no lock implementation is provided, we will use
         // the store to check if a concurrent refresh occurred.
 
-        const server = await serverFactory.fromIssuer(tokenSet.iss, dpopKey)
+        const server = await serverFactory.fromIssuer(
+          tokenSet.iss,
+          authMethod,
+          dpopKey,
+        )
 
         // Because refresh tokens can only be used once, we must not use the
         // "signal" to abort the refresh, or throw any abort error beyond this
@@ -111,7 +121,11 @@ export class SessionGetter extends CachedGetter<AtprotoDid, Session> {
             throw new TokenRefreshError(sub, 'Token set sub mismatch')
           }
 
-          return { dpopKey, tokenSet: newTokenSet }
+          return {
+            dpopKey,
+            tokenSet: newTokenSet,
+            authMethod: server.authMethod,
+          }
         } catch (cause) {
           // If the refresh token is invalid, let's try to recover from
           // concurrency issues, or make sure the session is deleted by throwing
@@ -173,17 +187,36 @@ export class SessionGetter extends CachedGetter<AtprotoDid, Session> {
                 30e3 * Math.random()
           )
         },
-        onStoreError: async (err, sub, { tokenSet, dpopKey }) => {
-          // If the token data cannot be stored, let's revoke it
-          const server = await serverFactory.fromIssuer(tokenSet.iss, dpopKey)
-          await server.revoke(tokenSet.refresh_token ?? tokenSet.access_token)
+        onStoreError: async (
+          err,
+          sub,
+          { tokenSet, dpopKey, authMethod = 'legacy' as const },
+        ) => {
+          if (!(err instanceof AuthMethodUnsatisfiableError)) {
+            // If the error was an AuthMethodUnsatisfiableError, there is no
+            // point in trying to call `fromIssuer`.
+            try {
+              // If the token data cannot be stored, let's revoke it
+              const server = await serverFactory.fromIssuer(
+                tokenSet.iss,
+                authMethod,
+                dpopKey,
+              )
+              await server.revoke(
+                tokenSet.refresh_token ?? tokenSet.access_token,
+              )
+            } catch {
+              // Let the original error propagate
+            }
+          }
+
           throw err
         },
         deleteOnError: async (err) =>
-          // Optimization: More likely to happen first
           err instanceof TokenRefreshError ||
           err instanceof TokenRevokedError ||
-          err instanceof TokenInvalidError,
+          err instanceof TokenInvalidError ||
+          err instanceof AuthMethodUnsatisfiableError,
       },
     )
   }

packages/oauth/oauth-client/src/state-store.ts

@@ -1,9 +1,12 @@
 import { Key } from '@atproto/jwk'
 import { SimpleStore } from '@atproto-labs/simple-store'
+import { ClientAuthMethod } from './oauth-client-auth.js'
 
 export type InternalStateData = {
   iss: string
   dpopKey: Key
+  /** @note optional for legacy reasons */
+  authMethod?: ClientAuthMethod
   verifier?: string
   appState?: string
 }

packages/oauth/oauth-client/src/validate-client-metadata.ts

@@ -4,28 +4,13 @@ import {
   assertOAuthDiscoverableClientId,
   assertOAuthLoopbackClientId,
 } from '@atproto/oauth-types'
+import { FALLBACK_ALG } from './constants.js'
 import { ClientMetadata, clientMetadataSchema } from './types.js'
 
-const TOKEN_ENDPOINT_AUTH_METHOD = `token_endpoint_auth_method`
-const TOKEN_ENDPOINT_AUTH_SIGNING_ALG = `token_endpoint_auth_signing_alg`
-
 export function validateClientMetadata(
   input: OAuthClientMetadataInput,
   keyset?: Keyset,
 ): ClientMetadata {
-  if (input.jwks) {
-    if (!keyset) {
-      throw new TypeError(`Keyset must not be provided when jwks is provided`)
-    }
-    for (const key of input.jwks.keys) {
-      if (!key.kid) {
-        throw new TypeError(`Key must have a "kid" property`)
-      } else if (!keyset.has(key.kid)) {
-        throw new TypeError(`Key with kid "${key.kid}" not found in keyset`)
-      }
-    }
-  }
-
   // Allow to pass a keyset and omit the jwks/jwks_uri properties
   if (!input.jwks && !input.jwks_uri && keyset?.size) {
     input = { ...input, jwks: keyset.toJSON() }
@@ -53,32 +38,60 @@ export function validateClientMetadata(
     throw new TypeError(`"grant_types" must include "authorization_code"`)
   }
 
-  const method = metadata[TOKEN_ENDPOINT_AUTH_METHOD]
+  const method = metadata.token_endpoint_auth_method
+  const methodAlg = metadata.token_endpoint_auth_signing_alg
   switch (method) {
-    case undefined:
-      throw new TypeError(`${TOKEN_ENDPOINT_AUTH_METHOD} must be provided`)
     case 'none':
-      if (metadata[TOKEN_ENDPOINT_AUTH_SIGNING_ALG]) {
+      if (methodAlg) {
         throw new TypeError(
-          `${TOKEN_ENDPOINT_AUTH_SIGNING_ALG} must not be provided when ${TOKEN_ENDPOINT_AUTH_METHOD} is "${method}"`,
+          `"token_endpoint_auth_signing_alg" must not be provided when "token_endpoint_auth_method" is "${method}"`,
         )
       }
       break
-    case 'private_key_jwt':
-      if (!keyset?.size) {
+
+    case 'private_key_jwt': {
+      if (!methodAlg) {
         throw new TypeError(
-          `A non-empty keyset must be provided when ${TOKEN_ENDPOINT_AUTH_METHOD} is "${method}"`,
+          `"token_endpoint_auth_signing_alg" must be provided when "token_endpoint_auth_method" is "${method}"`,
         )
       }
-      if (!metadata[TOKEN_ENDPOINT_AUTH_SIGNING_ALG]) {
+
+      const signingKeys = keyset
+        ? Array.from(keyset.list({ use: 'sig' })).filter(
+            (key) => key.isPrivate && key.kid,
+          )
+        : null
+
+      if (!signingKeys?.some((key) => key.algorithms.includes(FALLBACK_ALG))) {
         throw new TypeError(
-          `${TOKEN_ENDPOINT_AUTH_SIGNING_ALG} must be provided when ${TOKEN_ENDPOINT_AUTH_METHOD} is "${method}"`,
+          `Client authentication method "${method}" requires at least one "${FALLBACK_ALG}" signing key with a "kid" property`,
         )
       }
+
+      if (metadata.jwks) {
+        // Ensure that all the signing keys that could end-up being used are
+        // advertised in the JWKS.
+        for (const key of signingKeys) {
+          if (!metadata.jwks.keys.some((k) => k.kid === key.kid)) {
+            throw new TypeError(`Key with kid "${key.kid}" not found in jwks`)
+          }
+        }
+      } else if (metadata.jwks_uri) {
+        // @NOTE we only ensure that all the signing keys are referenced in JWKS
+        // when it is available (see previous "if") as we don't want to download
+        // that file here (for efficiency reasons).
+      } else {
+        throw new TypeError(
+          `Client authentication method "${method}" requires a JWKS`,
+        )
+      }
+
       break
+    }
+
     default:
       throw new TypeError(
-        `Invalid "token_endpoint_auth_method" value: ${method}`,
+        `Unsupported "token_endpoint_auth_method" value: ${method}`,
       )
   }
 

packages/oauth/oauth-provider/src/client/client-auth.ts

@@ -1,45 +1,64 @@
-import { KeyLike, calculateJwkThumbprint, errors, exportJWK } from 'jose'
 import { CLIENT_ASSERTION_TYPE_JWT_BEARER } from '@atproto/oauth-types'
-import { InvalidClientError } from '../errors/invalid-client-error.js'
-
-const { JOSEError } = errors
 
 export type ClientAuth =
   | { method: 'none' }
   | {
-      method: typeof CLIENT_ASSERTION_TYPE_JWT_BEARER
+      method: 'private_key_jwt'
+
+      /**
+       * Algorithm used for client authentication.
+       *
+       * @note We could allow clients to use a different algorithm over time
+       * (e.g. because new safer algorithms become available). For now, we
+       * require that the algorithm remains the same, as it is a bad practice to
+       * use the same key for different purposes.
+       */
       alg: string
+
+      /**
+       * ID of the key that was used for client authentication.
+       *
+       * @note The most important thing to validate is that the actual key didn't change (which is )
+       */
       kid: string
+
+      /**
+       * Thumbprint of the key used for client authentication. This value must
+       * be the same during token refreshes as the thumbprint of the key used
+       * during initial token issuance.
+       *
+       * @note This value is computed by the AS to ensure that the key used for
+       * client auth does not change
+       */
       jkt: string
+
+      /**
+       * Nonce used to prevent replay attacks. This value is generated by the
+       * client when generating it's assertion JWT and must be unique for each
+       * request.
+       *
+       * @see {@link https://www.rfc-editor.org/rfc/rfc7523.html#section-3}
+       */
+      jti: string
+
+      /**
+       * "exp" (expiration time) claim that limits the time window during which
+       * the JWT can be used.
+       *
+       * @note This field is optional for legacy reasons.
+       */
+      exp?: number
     }
 
-export function compareClientAuth(a: ClientAuth, b: ClientAuth): boolean {
-  if (a.method === 'none') {
-    if (b.method !== a.method) return false
-
-    return true
-  }
-
-  if (a.method === CLIENT_ASSERTION_TYPE_JWT_BEARER) {
-    if (b.method !== a.method) return false
-
-    return true
-  }
-
-  // Fool-proof
-  throw new TypeError('Invalid ClientAuth method')
-}
-
-export async function authJwkThumbprint(
-  key: Uint8Array | KeyLike,
-): Promise<string> {
-  try {
-    return await calculateJwkThumbprint(await exportJWK(key), 'sha512')
-  } catch (err) {
-    const message =
-      err instanceof JOSEError
-        ? err.message
-        : 'Failed to compute JWK thumbprint'
-    throw new InvalidClientError(message, err)
-  }
+/**
+ * @note In its previous version, the code was storing the
+ * "client_assertion_type" instead of the authentication method, which was
+ * confusing and prevented proper comparison with the client's
+ * "token_endpoint_auth_method" metadata.
+ */
+export type ClientAuthLegacy = {
+  method: typeof CLIENT_ASSERTION_TYPE_JWT_BEARER
+  alg: string
+  kid: string
+  jkt: string
 }

packages/oauth/oauth-provider/src/client/client-manager.ts

@@ -311,13 +311,7 @@ export class ClientManager {
       )
     }
 
-    const method = metadata[`token_endpoint_auth_method`]
-    switch (method) {
-      case undefined:
-        throw new InvalidClientMetadataError(
-          'Missing token_endpoint_auth_method client metadata',
-        )
-
+    switch (metadata.token_endpoint_auth_method) {
       case 'none':
         if (metadata.token_endpoint_auth_signing_alg) {
           throw new InvalidClientMetadataError(
@@ -346,7 +340,7 @@ export class ClientManager {
 
       default:
         throw new InvalidClientMetadataError(
-          `${method} is not a supported "token_endpoint_auth_method". Use "private_key_jwt" or "none".`,
+          `Unsupported client authentication method "${metadata.token_endpoint_auth_method}". Make sure "token_endpoint_auth_method" is set to one of: "${Client.AUTH_METHODS_SUPPORTED.join('", "')}"`,
         )
     }
 
@@ -421,6 +415,29 @@ export class ClientManager {
       )
     }
 
+    if (
+      metadata.application_type === 'native' &&
+      metadata.token_endpoint_auth_method !== 'none'
+    ) {
+      // https://datatracker.ietf.org/doc/html/rfc8252#section-8.4
+      //
+      // > Except when using a mechanism like Dynamic Client Registration
+      // > [RFC7591] to provision per-instance secrets, native apps are
+      // > classified as public clients, as defined by Section 2.1 of OAuth 2.0
+      // > [RFC6749]; they MUST be registered with the authorization server as
+      // > such. Authorization servers MUST record the client type in the client
+      // > registration details in order to identify and process requests
+      // > accordingly.
+
+      // @NOTE We may want to remove this restriction in the future, for example
+      // if https://github.com/bluesky-social/proposals/tree/main/0010-client-assertion-backend
+      // gets adopted
+
+      throw new InvalidClientMetadataError(
+        'Native clients must authenticate using "none" method',
+      )
+    }
+
     if (
       metadata.application_type === 'web' &&
       metadata.grant_types.includes('implicit')
@@ -670,7 +687,7 @@ export class ClientManager {
       )
     }
 
-    const method = metadata[`token_endpoint_auth_method`]
+    const method = metadata.token_endpoint_auth_method
     if (method !== 'none') {
       throw new InvalidClientMetadataError(
         `Loopback clients are not allowed to use "token_endpoint_auth_method" ${method}`,
@@ -738,24 +755,6 @@ export class ClientManager {
       }
     }
 
-    const method = metadata[`token_endpoint_auth_method`]
-    switch (method) {
-      case 'none':
-      case 'private_key_jwt':
-      case undefined:
-        break
-      case 'client_secret_post':
-      case 'client_secret_basic':
-      case 'client_secret_jwt':
-        throw new InvalidClientMetadataError(
-          `Client authentication method "${method}" is not allowed for discoverable clients`,
-        )
-      default:
-        throw new InvalidClientMetadataError(
-          `Unsupported client authentication method "${method}"`,
-        )
-    }
-
     for (const redirectUri of metadata.redirect_uris) {
       const url = parseRedirectUri(redirectUri)
 

packages/oauth/oauth-provider/src/client/client.ts

@@ -1,18 +1,21 @@
 import {
+  JWTClaimVerificationOptions,
+  type JWTHeaderParameters,
   type JWTPayload,
-  type JWTVerifyGetKey,
   type JWTVerifyOptions,
   type JWTVerifyResult,
   type KeyLike,
   type ResolvedKey,
   UnsecuredJWT,
   type UnsecuredResult,
+  calculateJwkThumbprint,
   createLocalJWKSet,
   createRemoteJWKSet,
   errors,
+  exportJWK,
   jwtVerify,
 } from 'jose'
-import { Jwks } from '@atproto/jwk'
+import { Jwks, SignedJwt, UnsignedJwt } from '@atproto/jwk'
 import {
   CLIENT_ASSERTION_TYPE_JWT_BEARER,
   OAuthAuthorizationRequestParameters,
@@ -27,8 +30,10 @@ import { InvalidClientMetadataError } from '../errors/invalid-client-metadata-er
 import { InvalidParametersError } from '../errors/invalid-parameters-error.js'
 import { InvalidRequestError } from '../errors/invalid-request-error.js'
 import { InvalidScopeError } from '../errors/invalid-scope-error.js'
+import { asArray } from '../lib/util/cast.js'
 import { compareRedirectUri } from '../lib/util/redirect-uri.js'
-import { ClientAuth, authJwkThumbprint } from './client-auth.js'
+import { Awaitable } from '../lib/util/type.js'
+import { ClientAuth } from './client-auth.js'
 import { ClientId } from './client-id.js'
 import { ClientInfo } from './client-info.js'
 
@@ -40,7 +45,9 @@ export class Client {
    */
   static readonly AUTH_METHODS_SUPPORTED = ['none', 'private_key_jwt'] as const
 
-  private readonly keyGetter: JWTVerifyGetKey
+  private readonly keyGetter: (
+    protectedHeader: JWTHeaderParameters,
+  ) => Awaitable<KeyLike | Uint8Array>
 
   constructor(
     public readonly id: ClientId,
@@ -55,26 +62,42 @@ export class Client {
         : createRemoteJWKSet(new URL(metadata.jwks_uri), {})
   }
 
-  public async decodeRequestObject(jar: string) {
+  /**
+   * @see {@link https://www.rfc-editor.org/rfc/rfc9101.html#name-request-object-2}
+   */
+  public async decodeRequestObject(
+    jar: SignedJwt | UnsignedJwt,
+    audience: string,
+  ) {
+    // https://www.rfc-editor.org/rfc/rfc9101.html#name-request-object-2
+    // > If signed, the Authorization Request Object SHOULD contain the Claims
+    // > iss (issuer) and aud (audience) as members with their semantics being
+    // > the same as defined in the JWT [RFC7519] specification. The value of
+    // > aud should be the value of the authorization server (AS) issuer, as
+    // > defined in RFC 8414 [RFC8414].
     try {
-      switch (this.metadata.request_object_signing_alg) {
-        case 'none':
-          return await this.jwtVerifyUnsecured(jar, {
-            maxTokenAge: JAR_MAX_AGE / 1000,
-          })
-        case undefined:
-          // https://openid.net/specs/openid-connect-registration-1_0.html#rfc.section.2
-          // > The default, if omitted, is that any algorithm supported by the OP
-          // > and the RP MAY be used.
-          return await this.jwtVerify(jar, {
-            maxTokenAge: JAR_MAX_AGE / 1000,
-          })
-        default:
-          return await this.jwtVerify(jar, {
-            maxTokenAge: JAR_MAX_AGE / 1000,
-            algorithms: [this.metadata.request_object_signing_alg],
-          })
+      // We need to special case the "none" algorithm, as the validation method
+      // is different for signed and unsigned JWTs.
+      if (this.metadata.request_object_signing_alg === 'none') {
+        return await this.jwtVerifyUnsecured(jar, {
+          audience,
+          maxTokenAge: JAR_MAX_AGE / 1e3,
+          allowMissingAudience: true,
+          allowMissingIssuer: true,
+        })
       }
+
+      return await this.jwtVerify(jar, {
+        audience,
+        maxTokenAge: JAR_MAX_AGE / 1e3,
+        algorithms: this.metadata.request_object_signing_alg
+          ? [this.metadata.request_object_signing_alg]
+          : // https://openid.net/specs/openid-connect-registration-1_0.html#rfc.section.2
+            //
+            // > The default, if omitted, is that any algorithm supported by the OP
+            // > and the RP MAY be used.
+            undefined,
+      })
     } catch (err) {
       const message =
         err instanceof JOSEError
@@ -87,12 +110,38 @@ export class Client {
 
   protected async jwtVerifyUnsecured<PayloadType = JWTPayload>(
     token: string,
-    options?: Omit<JWTVerifyOptions, 'issuer'>,
+    {
+      audience,
+      allowMissingAudience = false,
+      allowMissingIssuer = false,
+      ...options
+    }: Omit<JWTClaimVerificationOptions, 'issuer'> & {
+      allowMissingIssuer?: boolean
+      allowMissingAudience?: boolean
+    } = {},
   ): Promise<UnsecuredResult<PayloadType>> {
-    return UnsecuredJWT.decode(token, {
-      ...options,
-      issuer: this.id,
-    })
+    // jose does not support `allowMissingAudience` and `allowMissingIssuer`
+    // options, so we need to handle audience and issuer checks manually (see
+    // bellow).
+
+    const result = UnsecuredJWT.decode<PayloadType>(token, options)
+
+    if (!allowMissingIssuer || result.payload.iss != null) {
+      if (result.payload.iss !== this.id) {
+        throw new JOSEError(`Invalid "iss" claim "${result.payload.iss}"`)
+      }
+    }
+
+    if (!allowMissingAudience || result.payload.aud != null) {
+      if (audience != null) {
+        const payloadAud = asArray(result.payload.aud)
+        if (!asArray(audience).some((aud) => payloadAud.includes(aud))) {
+          throw new JOSEError(`Invalid "aud" claim "${result.payload.aud}"`)
+        }
+      }
+    }
+
+    return result
   }
 
   protected async jwtVerify<PayloadType = JWTPayload>(
@@ -110,63 +159,103 @@ export class Client {
    * @see {@link https://datatracker.ietf.org/doc/html/rfc7523#section-3}
    * @see {@link https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#token-endpoint-auth-method}
    */
-  public async verifyCredentials(
+  public async authenticate(
     input: OAuthClientCredentials,
     checks: {
-      audience: string
+      authorizationServerIdentifier: string
     },
-  ): Promise<{
-    clientAuth: ClientAuth
-    // for replay protection
-    nonce?: string
-  }> {
-    const method = this.metadata[`token_endpoint_auth_method`]
+  ): Promise<ClientAuth> {
+    const method = this.metadata.token_endpoint_auth_method
 
     if (method === 'none') {
-      const clientAuth: ClientAuth = { method: 'none' }
-      return { clientAuth }
+      return { method: 'none' }
     }
 
     if (method === 'private_key_jwt') {
-      if (!('client_assertion_type' in input) || !input.client_assertion_type) {
+      if (!('client_assertion' in input)) {
         throw new InvalidRequestError(
-          `client_assertion_type required for "${method}"`,
-        )
-      } else if (!input.client_assertion) {
-        throw new InvalidRequestError(
-          `client_assertion required for "${method}"`,
+          `client authentication method "${method}" required a "client_assertion"`,
         )
       }
 
       if (input.client_assertion_type === CLIENT_ASSERTION_TYPE_JWT_BEARER) {
+        // https://www.rfc-editor.org/rfc/rfc7523.html#section-3
+
         const result = await this.jwtVerify<{
           jti: string
+          exp?: number
         }>(input.client_assertion, {
-          audience: checks.audience,
-          subject: this.id,
-          maxTokenAge: CLIENT_ASSERTION_MAX_AGE / 1000,
-          requiredClaims: ['jti'],
-        }).catch((err) => {
-          if (err instanceof JOSEError) {
-            const msg = `Validation of "client_assertion" failed: ${err.message}`
-            throw new InvalidClientError(msg, err)
-          }
+          // > 1. The JWT MUST contain an "iss" (issuer) claim that contains a
+          // >    unique identifier for the entity that issued the JWT.
+          //
+          // The "issuer" is already checked by jwtVerify()
 
-          throw err
+          // > 2. The JWT MUST contain a "sub" (subject) claim identifying the
+          // >    principal that is the subject of the JWT. Two cases need to be
+          // >    differentiated: [...] For client authentication, the subject
+          // >    MUST be the "client_id" of the OAuth client.
+          subject: this.id,
+
+          // > 3. The JWT MUST contain an "aud" (audience) claim containing a
+          // >    value that identifies the authorization server as an intended
+          // >    audience. The token endpoint URL of the authorization server
+          // >    MAY be used as a value for an "aud" element to identify the
+          // >    authorization server as an intended audience of the JWT.
+          audience: checks.authorizationServerIdentifier,
+
+          requiredClaims: [
+            // > 4. The JWT MUST contain an "exp" (expiration time) claim that
+            // >    limits the time window during which the JWT can be used.
+            //
+            // @TODO The presence of "exp" didn't use to be enforced by this
+            // implementation (or provided by the oauth-client). This is mostly
+            // fine because "iat" *is* required, but this makes this
+            // implementation non compliant with RFC7523. We can't just make it
+            // required as it might break existing clients.
+
+            // 'exp',
+
+            // > 7. The JWT MAY contain a "jti" (JWT ID) claim that provides a
+            // >    unique identifier for the token. The authorization server
+            // >    MAY ensure that JWTs are not replayed by maintaining the set
+            // >    of used "jti" values for the length of time for which the
+            // >    JWT would be considered valid based on the applicable "exp"
+            // >    instant.
+            'jti',
+          ],
+
+          // > 5. The JWT MAY contain an "nbf" (not before) claim that
+          // >    identifies the time before which the token MUST NOT be
+          // >    accepted for processing.
+          //
+          // This is already enforced by jose
+
+          // > 6. The JWT MAY contain an "iat" (issued at) claim that identifies
+          // >    the time at which the JWT was issued.  Note that the
+          // >    authorization server may reject JWTs with an "iat" claim value
+          // >    that is unreasonably far in the past.
+          maxTokenAge: CLIENT_ASSERTION_MAX_AGE / 1000,
+        }).catch((err) => {
+          const msg =
+            err instanceof JOSEError
+              ? `Validation of "client_assertion" failed: ${err.message}`
+              : `Unable to verify "client_assertion" JWT`
+
+          throw new InvalidClientError(msg, err)
         })
 
         if (!result.protectedHeader.kid) {
           throw new InvalidClientError(`"kid" required in client_assertion`)
         }
 
-        const clientAuth: ClientAuth = {
-          method: CLIENT_ASSERTION_TYPE_JWT_BEARER,
+        return {
+          method: 'private_key_jwt',
+          jti: result.payload.jti,
+          exp: result.payload.exp,
           jkt: await authJwkThumbprint(result.key),
           alg: result.protectedHeader.alg,
           kid: result.protectedHeader.kid,
         }
-
-        return { clientAuth, nonce: result.payload.jti }
       }
 
       throw new InvalidClientError(
@@ -189,41 +278,6 @@ export class Client {
     )
   }
 
-  /**
-   * Ensures that a {@link ClientAuth} generated in the past is still valid wrt
-   * the current client metadata & jwks. This is used to invalidate tokens when
-   * the client stops advertising the key that it used to authenticate itself
-   * during the initial token request.
-   */
-  public async validateClientAuth(clientAuth: ClientAuth): Promise<boolean> {
-    if (clientAuth.method === 'none') {
-      return this.metadata[`token_endpoint_auth_method`] === 'none'
-    }
-
-    if (clientAuth.method === CLIENT_ASSERTION_TYPE_JWT_BEARER) {
-      if (this.metadata[`token_endpoint_auth_method`] !== 'private_key_jwt') {
-        return false
-      }
-      try {
-        const key = await this.keyGetter(
-          {
-            kid: clientAuth.kid,
-            alg: clientAuth.alg,
-          },
-          { payload: '', signature: '' },
-        )
-        const jtk = await authJwkThumbprint(key)
-
-        return jtk === clientAuth.jkt
-      } catch (e) {
-        return false
-      }
-    }
-
-    // @ts-expect-error
-    throw new Error(`Invalid method "${clientAuth.method}"`)
-  }
-
   /**
    * Validates the request parameters against the client metadata.
    */
@@ -328,3 +382,13 @@ export class Client {
     return redirect_uris.length === 1 ? redirect_uris[0] : undefined
   }
 }
+
+export async function authJwkThumbprint(
+  key: Uint8Array | KeyLike,
+): Promise<string> {
+  try {
+    return await calculateJwkThumbprint(await exportJWK(key), 'sha512')
+  } catch (err) {
+    throw new InvalidClientError('Unable to compute JWK thumbprint', err)
+  }
+}

packages/oauth/oauth-provider/src/constants.ts

@@ -38,17 +38,17 @@ export const TOKEN_MAX_AGE = 60 * MINUTE
 /** 5 minutes */
 export const AUTHORIZATION_INACTIVITY_TIMEOUT = 5 * MINUTE
 
-/** 1 months */
-export const AUTHENTICATED_REFRESH_INACTIVITY_TIMEOUT = 1 * MONTH
+/** 1 week */
+export const PUBLIC_CLIENT_SESSION_LIFETIME = 1 * WEEK
 
 /** 2 days */
-export const UNAUTHENTICATED_REFRESH_INACTIVITY_TIMEOUT = 2 * DAY
-
-/** 1 week */
-export const UNAUTHENTICATED_REFRESH_LIFETIME = 1 * WEEK
+export const PUBLIC_CLIENT_REFRESH_LIFETIME = 2 * DAY
 
 /** 1 year */
-export const AUTHENTICATED_REFRESH_LIFETIME = 1 * YEAR
+export const CONFIDENTIAL_CLIENT_SESSION_LIFETIME = 1 * YEAR
+
+/** 1 months */
+export const CONFIDENTIAL_CLIENT_REFRESH_LIFETIME = 1 * MONTH
 
 /** 5 minutes */
 export const PAR_EXPIRES_IN = 5 * MINUTE
@@ -73,3 +73,5 @@ export const SESSION_FIXATION_MAX_AGE = 5 * SECOND
 
 /** 1 day */
 export const CODE_CHALLENGE_REPLAY_TIMEFRAME = 1 * DAY
+
+export const NODE_ENV = process.env.NODE_ENV || 'production'

packages/oauth/oauth-provider/src/metadata/build-metadata.ts

@@ -2,7 +2,7 @@ import { Keyset } from '@atproto/jwk'
 import {
   OAuthAuthorizationServerMetadata,
   OAuthIssuerIdentifier,
-  oauthAuthorizationServerMetadataSchema,
+  oauthAuthorizationServerMetadataValidator,
 } from '@atproto/oauth-types'
 import { Client } from '../client/client.js'
 import { VERIFY_ALGOS } from '../lib/util/crypto.js'
@@ -22,7 +22,7 @@ export function buildMetadata(
   keyset: Keyset,
   customMetadata?: CustomMetadata,
 ): OAuthAuthorizationServerMetadata {
-  return oauthAuthorizationServerMetadataSchema.parse({
+  return oauthAuthorizationServerMetadataValidator.parse({
     issuer,
 
     scopes_supported: [

packages/oauth/oauth-provider/src/oauth-provider.ts

@@ -1,4 +1,6 @@
+import { createHash } from 'node:crypto'
 import type { Redis, RedisOptions } from 'ioredis'
+import { ZodError } from 'zod'
 import { Jwks, Keyset } from '@atproto/jwk'
 import type { Account } from '@atproto/oauth-provider-api'
 import {
@@ -33,7 +35,7 @@ import {
   DeviceAccount,
   asAccountStore,
 } from './account/account-store.js'
-import { ClientAuth, authJwkThumbprint } from './client/client-auth.js'
+import { ClientAuth, ClientAuthLegacy } from './client/client-auth.js'
 import { ClientId } from './client/client-id.js'
 import {
   ClientManager,
@@ -41,7 +43,14 @@ import {
 } from './client/client-manager.js'
 import { ClientStore, ifClientStore } from './client/client-store.js'
 import { Client } from './client/client.js'
-import { AUTHENTICATION_MAX_AGE, TOKEN_MAX_AGE } from './constants.js'
+import {
+  AUTHENTICATION_MAX_AGE,
+  CONFIDENTIAL_CLIENT_REFRESH_LIFETIME,
+  CONFIDENTIAL_CLIENT_SESSION_LIFETIME,
+  PUBLIC_CLIENT_REFRESH_LIFETIME,
+  PUBLIC_CLIENT_SESSION_LIFETIME,
+  TOKEN_MAX_AGE,
+} from './constants.js'
 import { Branding, BrandingInput } from './customization/branding.js'
 import {
   Customization,
@@ -58,8 +67,9 @@ import { DeviceStore, asDeviceStore } from './device/device-store.js'
 import { AccessDeniedError } from './errors/access-denied-error.js'
 import { AccountSelectionRequiredError } from './errors/account-selection-required-error.js'
 import { ConsentRequiredError } from './errors/consent-required-error.js'
+import { InvalidDpopKeyBindingError } from './errors/invalid-dpop-key-binding-error.js'
+import { InvalidDpopProofError } from './errors/invalid-dpop-proof-error.js'
 import { InvalidGrantError } from './errors/invalid-grant-error.js'
-import { InvalidParametersError } from './errors/invalid-parameters-error.js'
 import { InvalidRequestError } from './errors/invalid-request-error.js'
 import { LoginRequiredError } from './errors/login-required-error.js'
 import { HcaptchaConfig } from './lib/hcaptcha.js'
@@ -76,18 +86,20 @@ import {
 } from './oauth-verifier.js'
 import { ReplayStore, ifReplayStore } from './replay/replay-store.js'
 import { codeSchema } from './request/code.js'
-import { RequestInfo } from './request/request-info.js'
 import { RequestManager } from './request/request-manager.js'
-import { RequestStoreMemory } from './request/request-store-memory.js'
-import { RequestStoreRedis } from './request/request-store-redis.js'
-import { RequestStore, ifRequestStore } from './request/request-store.js'
+import { RequestStore, asRequestStore } from './request/request-store.js'
 import { requestUriSchema } from './request/request-uri.js'
 import { AuthorizationRedirectParameters } from './result/authorization-redirect-parameters.js'
 import { AuthorizationResultAuthorizePage } from './result/authorization-result-authorize-page.js'
 import { AuthorizationResultRedirect } from './result/authorization-result-redirect.js'
 import { ErrorHandler } from './router/error-handler.js'
+import { TokenData } from './token/token-data.js'
 import { TokenManager } from './token/token-manager.js'
-import { TokenStore, asTokenStore } from './token/token-store.js'
+import {
+  TokenStore,
+  asTokenStore,
+  refreshTokenSchema,
+} from './token/token-store.js'
 import {
   VerifyTokenClaimsOptions,
   VerifyTokenClaimsResult,
@@ -242,18 +254,17 @@ export class OAuthProvider extends OAuthVerifier {
     metadata,
 
     safeFetch = safeFetchWrap(),
-    redis,
     store, // compound store implementation
 
     // Requires stores
     accountStore = asAccountStore(store),
     deviceStore = asDeviceStore(store),
     tokenStore = asTokenStore(store),
+    requestStore = asRequestStore(store),
 
     // These are optional
     clientStore = ifClientStore(store),
     replayStore = ifReplayStore(store),
-    requestStore = ifRequestStore(store),
 
     clientJwksCache = new SimpleStoreMemory({
       maxSize: 50_000_000,
@@ -288,11 +299,7 @@ export class OAuthProvider extends OAuthVerifier {
     // be the responsibility of the super class.
     const superOptions: OAuthVerifierOptions = rest
 
-    super({ replayStore, redis, ...superOptions })
-
-    requestStore ??= redis
-      ? new RequestStoreRedis({ redis })
-      : new RequestStoreMemory()
+    super({ replayStore, ...superOptions })
 
     this.accessTokenMode = accessTokenMode
     this.authenticationMaxAge = authenticationMaxAge
@@ -363,97 +370,92 @@ export class OAuthProvider extends OAuthVerifier {
   }
 
   protected async authenticateClient(
-    credentials: OAuthClientCredentials,
-  ): Promise<[Client, ClientAuth]> {
-    const client = await this.clientManager.getClient(credentials.client_id)
-    const { clientAuth, nonce } = await client.verifyCredentials(credentials, {
-      audience: this.issuer,
-    })
+    clientCredentials: OAuthClientCredentials,
+    dpopProof: null | DpopProof,
+    options?: {
+      allowMissingDpopProof?: boolean
+    },
+  ): Promise<{
+    client: Client
+    clientAuth: ClientAuth
+  }> {
+    const client = await this.clientManager.getClient(
+      clientCredentials.client_id,
+    )
 
     if (
-      client.metadata.application_type === 'native' &&
-      clientAuth.method !== 'none'
+      client.metadata.dpop_bound_access_tokens &&
+      !dpopProof &&
+      !options?.allowMissingDpopProof
     ) {
-      // https://datatracker.ietf.org/doc/html/rfc8252#section-8.4
-      //
-      // > Except when using a mechanism like Dynamic Client Registration
-      // > [RFC7591] to provision per-instance secrets, native apps are
-      // > classified as public clients, as defined by Section 2.1 of OAuth 2.0
-      // > [RFC6749]; they MUST be registered with the authorization server as
-      // > such. Authorization servers MUST record the client type in the client
-      // > registration details in order to identify and process requests
-      // > accordingly.
-
-      throw new InvalidGrantError(
-        'Native clients must authenticate using "none" method',
-      )
+      throw new InvalidDpopProofError('DPoP proof required')
     }
 
-    if (nonce != null) {
-      const unique = await this.replayManager.uniqueAuth(nonce, client.id)
+    if (dpopProof && !client.metadata.dpop_bound_access_tokens) {
+      throw new InvalidDpopProofError('DPoP proof not allowed for this client')
+    }
+
+    const clientAuth = await client.authenticate(clientCredentials, {
+      authorizationServerIdentifier: this.issuer,
+    })
+
+    if (clientAuth.method === 'private_key_jwt') {
+      // Clients MUST NOT use their client assertion key to sign DPoP proofs
+      if (dpopProof && clientAuth.jkt === dpopProof.jkt) {
+        throw new InvalidRequestError(
+          'The DPoP proof must be signed with a different key than the client assertion',
+        )
+      }
+
+      // https://www.rfc-editor.org/rfc/rfc7523.html#section-3
+      // > 7.  [...] The authorization server MAY ensure that JWTs are not
+      // >     replayed by maintaining the set of used "jti" values for the
+      // >     length of time for which the JWT would be considered valid based
+      // >     on the applicable "exp" instant.
+
+      const unique = await this.replayManager.uniqueAuth(
+        clientAuth.jti,
+        client.id,
+        clientAuth.exp,
+      )
       if (!unique) {
         throw new InvalidGrantError(`${clientAuth.method} jti reused`)
       }
     }
 
-    return [client, clientAuth]
+    return { client, clientAuth }
   }
 
   protected async decodeJAR(
     client: Client,
     input: OAuthAuthorizationRequestJar,
-  ): Promise<
-    | {
-        payload: OAuthAuthorizationRequestParameters
-      }
-    | {
-        payload: OAuthAuthorizationRequestParameters
-        protectedHeader: { kid: string; alg: string }
-        jkt: string
-      }
-  > {
-    const result = await client.decodeRequestObject(input.request)
-    const payload = oauthAuthorizationRequestParametersSchema.parse(
-      result.payload,
+  ): Promise<OAuthAuthorizationRequestParameters> {
+    const { payload } = await client.decodeRequestObject(
+      input.request,
+      this.issuer,
     )
 
-    if (!result.payload.jti) {
-      throw new InvalidParametersError(
-        payload,
-        'Request object must contain a jti claim',
+    const { jti } = payload
+    if (!jti) {
+      throw new InvalidRequestError(
+        'Request object payload must contain a "jti" claim',
       )
     }
-
-    if (!(await this.replayManager.uniqueJar(result.payload.jti, client.id))) {
-      throw new InvalidParametersError(
-        payload,
-        'Request object jti is not unique',
-      )
+    if (!(await this.replayManager.uniqueJar(jti, client.id))) {
+      throw new InvalidRequestError('Request object was replayed')
     }
 
-    if ('protectedHeader' in result) {
-      if (!result.protectedHeader.kid) {
-        throw new InvalidParametersError(payload, 'Missing "kid" in header')
-      }
+    const parameters = await oauthAuthorizationRequestParametersSchema
+      .parseAsync(payload)
+      .catch((err) => {
+        const message =
+          err instanceof ZodError
+            ? `Invalid request parameters: ${err.message}`
+            : `Invalid "request" object`
+        throw InvalidRequestError.from(err, message)
+      })
 
-      return {
-        jkt: await authJwkThumbprint(result.key),
-        payload,
-        protectedHeader: result.protectedHeader as {
-          alg: string
-          kid: string
-        },
-      }
-    }
-
-    if ('header' in result) {
-      return {
-        payload,
-      }
-    }
-
-    // Should never happen
-    throw new Error('Invalid request object')
+    return parameters
   }
 
   /**
@@ -465,12 +467,43 @@ export class OAuthProvider extends OAuthVerifier {
     dpopProof: null | DpopProof,
   ): Promise<OAuthParResponse> {
     try {
-      const [client, clientAuth] = await this.authenticateClient(credentials)
+      const { client, clientAuth } = await this.authenticateClient(
+        credentials,
+        dpopProof,
+        // Allow missing DPoP header for PAR requests as rfc9449 allows it
+        // (though the dpop_jkt parameter must be present in that case, see
+        // check bellow).
+        { allowMissingDpopProof: true },
+      )
 
-      const { payload: parameters } =
+      const parameters =
         'request' in authorizationRequest // Handle JAR
           ? await this.decodeJAR(client, authorizationRequest)
-          : { payload: authorizationRequest }
+          : authorizationRequest
+
+      if (!parameters.dpop_jkt) {
+        if (client.metadata.dpop_bound_access_tokens) {
+          if (dpopProof) parameters.dpop_jkt = dpopProof.jkt
+          else {
+            // @NOTE When both PAR and DPoP are used, either the DPoP header, or
+            // the dpop_jkt parameter must be present. We do not enforce this
+            // for legacy reasons.
+            // https://datatracker.ietf.org/doc/html/rfc9449#section-10.1
+          }
+        }
+      } else {
+        if (!client.metadata.dpop_bound_access_tokens) {
+          throw new InvalidRequestError(
+            'DPoP bound access tokens are not enabled for this client',
+          )
+        }
+
+        // Proof is optional if the dpop_jkt is provided, but if it is provided,
+        // it must match the DPoP proof JKT.
+        if (dpopProof && dpopProof.jkt !== parameters.dpop_jkt) {
+          throw new InvalidDpopKeyBindingError()
+        }
+      }
 
       const { uri, expiresAt } =
         await this.requestManager.createAuthorizationRequest(
@@ -478,7 +511,6 @@ export class OAuthProvider extends OAuthVerifier {
           clientAuth,
           parameters,
           null,
-          dpopProof,
         )
 
       return {
@@ -501,7 +533,8 @@ export class OAuthProvider extends OAuthVerifier {
     client: Client,
     deviceId: DeviceId,
     query: OAuthAuthorizationRequestQuery,
-  ): Promise<RequestInfo> {
+  ) {
+    // PAR
     if ('request_uri' in query) {
       const requestUri = await requestUriSchema
         .parseAsync(query.request_uri, { path: ['query', 'request_uri'] })
@@ -515,43 +548,35 @@ export class OAuthProvider extends OAuthVerifier {
       return this.requestManager.get(requestUri, deviceId, client.id)
     }
 
+    // JAR
     if ('request' in query) {
-      const requestObject = await this.decodeJAR(client, query)
-
-      if ('protectedHeader' in requestObject && requestObject.protectedHeader) {
-        // Allow using signed JAR during "/authorize" as client authentication.
-        // This allows clients to skip PAR to initiate trusted sessions.
-        const clientAuth: ClientAuth = {
-          method: CLIENT_ASSERTION_TYPE_JWT_BEARER,
-          kid: requestObject.protectedHeader.kid,
-          alg: requestObject.protectedHeader.alg,
-          jkt: requestObject.jkt,
-        }
-
-        return this.requestManager.createAuthorizationRequest(
-          client,
-          clientAuth,
-          requestObject.payload,
-          deviceId,
-          null,
-        )
-      }
+      // @NOTE Since JAR are signed with the client's private key, a JAR *could*
+      // technically be used to authenticate the client when requests are
+      // created without PAR (i.e. created on the fly by the authorize
+      // endpoint). This implementation actually used to support this
+      // (un-spec'd) behavior. That support was removed:
+      // - Because it was not actually used
+      // - Because it was not part of any standard
+      // - Because it makes extending the client authentication mechanism more
+      //   complex since any extension would not only need to affect the
+      //   "private_key_jwt" auth method but also the JAR "request" object.
+      const parameters = await this.decodeJAR(client, query)
 
       return this.requestManager.createAuthorizationRequest(
         client,
-        { method: 'none' },
-        requestObject.payload,
-        deviceId,
         null,
+        parameters,
+        deviceId,
       )
     }
 
+    // "Regular" authorization request (created on the fly by directing the user
+    // to the authorization endpoint with all the parameters in the url).
     return this.requestManager.createAuthorizationRequest(
       client,
-      { method: 'none' },
+      null,
       query,
       deviceId,
-      null,
     )
   }
 
@@ -723,8 +748,10 @@ export class OAuthProvider extends OAuthVerifier {
     request: OAuthTokenRequest,
     dpopProof: null | DpopProof,
   ): Promise<OAuthTokenResponse> {
-    const [client, clientAuth] =
-      await this.authenticateClient(clientCredentials)
+    const { client, clientAuth } = await this.authenticateClient(
+      clientCredentials,
+      dpopProof,
+    )
 
     if (!this.metadata.grant_types_supported?.includes(request.grant_type)) {
       throw new InvalidGrantError(
@@ -739,7 +766,7 @@ export class OAuthProvider extends OAuthVerifier {
     }
 
     if (request.grant_type === 'authorization_code') {
-      return this.codeGrant(
+      return this.authorizationCodeGrant(
         client,
         clientAuth,
         clientMetadata,
@@ -763,116 +790,289 @@ export class OAuthProvider extends OAuthVerifier {
     )
   }
 
-  protected async codeGrant(
+  protected async compareClientAuth(
+    client: Client,
+    clientAuth: ClientAuth,
+    dpopProof: null | DpopProof,
+    initial: {
+      parameters: OAuthAuthorizationRequestParameters
+      clientId: ClientId
+      clientAuth: null | ClientAuth | ClientAuthLegacy
+    },
+  ): Promise<void> {
+    // Fool proofing, ensure that the client is authenticating using the right method
+    if (clientAuth.method !== client.metadata.token_endpoint_auth_method) {
+      throw new InvalidGrantError(
+        `Client authentication method mismatch (expected ${client.metadata.token_endpoint_auth_method}, got ${clientAuth.method})`,
+      )
+    }
+
+    if (initial.clientId !== client.id) {
+      throw new InvalidGrantError(`Token was not issued to this client`)
+    }
+
+    const { parameters } = initial
+    if (parameters.dpop_jkt) {
+      if (!dpopProof) {
+        throw new InvalidGrantError(`DPoP proof is required for this request`)
+      } else if (parameters.dpop_jkt !== dpopProof.jkt) {
+        throw new InvalidGrantError(
+          `DPoP proof does not match the expected JKT`,
+        )
+      }
+    }
+
+    if (!initial.clientAuth) {
+      // If the client did not use PAR, it was not authenticated when the request
+      // was initially created (see authorize() method in OAuthProvider). Since
+      // PAR is not mandatory, and since the token exchange currently taking place
+      // *is* authenticated (`clientAuth`), we allow "upgrading" the
+      // authentication method (the token created will be bound to the current
+      // clientAuth).
+      return
+    }
+
+    switch (initial.clientAuth.method) {
+      case CLIENT_ASSERTION_TYPE_JWT_BEARER: // LEGACY
+      case 'private_key_jwt':
+        if (clientAuth.method !== 'private_key_jwt') {
+          throw new InvalidGrantError(
+            `Client authentication method mismatch (expected ${initial.clientAuth.method})`,
+          )
+        }
+        if (
+          clientAuth.kid !== initial.clientAuth.kid ||
+          clientAuth.alg !== initial.clientAuth.alg ||
+          clientAuth.jkt !== initial.clientAuth.jkt
+        ) {
+          throw new InvalidGrantError(
+            `The session was initiated with a different key than the client assertion currently used`,
+          )
+        }
+        break
+      case 'none':
+        // @NOTE We allow the client to "upgrade" to a confidential client if
+        // the session was initially created without client authentication.
+        break
+      default:
+        throw new InvalidGrantError(
+          // @ts-expect-error (future proof, backwards compatibility)
+          `Invalid method "${initial.clientAuth.method}"`,
+        )
+    }
+  }
+
+  protected async authorizationCodeGrant(
     client: Client,
     clientAuth: ClientAuth,
     clientMetadata: RequestMetadata,
     input: OAuthAuthorizationCodeGrantTokenRequest,
     dpopProof: null | DpopProof,
   ): Promise<OAuthTokenResponse> {
-    const code = codeSchema.parse(input.code)
-    try {
-      const { sub, deviceId, parameters } = await this.requestManager.findCode(
-        client,
-        clientAuth,
-        code,
-      )
-
-      // the following check prevents re-use of PKCE challenges, enforcing the
-      // clients to generate a new challenge for each authorization request. The
-      // replay manager typically prevents replay over a certain time frame,
-      // which might not cover the entire lifetime of the token (depending on
-      // the implementation of the replay store). For this reason, we should
-      // ideally ensure that the code_challenge was not already used by any
-      // existing token or any other pending request.
-      //
-      // The current implementation will cause client devs not issuing a new
-      // code challenge for each authorization request to fail, which should be
-      // a good enough incentive to follow the best practices, until we have a
-      // better implementation.
-      //
-      // @TODO Use tokenManager to ensure uniqueness of code_challenge
-      if (parameters.code_challenge) {
-        const unique = await this.replayManager.uniqueCodeChallenge(
-          parameters.code_challenge,
+    const code = await codeSchema
+      .parseAsync(input.code, { path: ['code'] })
+      .catch((err) => {
+        throw InvalidGrantError.from(
+          err,
+          err instanceof ZodError
+            ? `Invalid code: ${err.message}`
+            : `Invalid code`,
         )
-        if (!unique) {
-          throw new InvalidGrantError('Code challenge already used')
+      })
+
+    const data = await this.requestManager
+      .consumeCode(code)
+      .catch(async (err) => {
+        // Code not found in request manager: check for replays
+        const tokenInfo = await this.tokenManager.findByCode(code)
+        if (tokenInfo) {
+          // try/finally to ensure that both code path get executed (sequentially)
+          try {
+            // "code" was replayed, delete existing session
+            await this.tokenManager.deleteToken(tokenInfo.id)
+          } finally {
+            // As an additional security measure, we also sign the device out,
+            // so that the device cannot be used to access the account anymore
+            // without a new authentication.
+            const { deviceId, sub } = tokenInfo.data
+            if (deviceId) {
+              await this.accountManager.removeDeviceAccount(deviceId, sub)
+            }
+          }
         }
-      }
 
-      const { account } = await this.accountManager.getAccount(sub)
+        throw InvalidGrantError.from(err, `Invalid code`)
+      })
 
-      return await this.tokenManager.create(
-        client,
-        clientAuth,
-        clientMetadata,
-        account,
-        deviceId,
-        parameters,
-        input,
-        dpopProof,
+    // @NOTE at this point, the request data was removed from the store and only
+    // exists in memory here (in the "data" variable). Because of this, any
+    // error thrown after this point will permanently cause the request data to
+    // be lost.
+
+    await this.compareClientAuth(client, clientAuth, dpopProof, data)
+
+    // If the DPoP proof was not provided earlier (PAR / authorize), let's add
+    // it now.
+    const parameters =
+      dpopProof &&
+      client.metadata.dpop_bound_access_tokens &&
+      !data.parameters.dpop_jkt
+        ? { ...data.parameters, dpop_jkt: dpopProof.jkt }
+        : data.parameters
+
+    await this.validateCodeGrant(parameters, input)
+
+    const { account } = await this.accountManager.getAccount(data.sub)
+
+    return this.tokenManager.createToken(
+      client,
+      clientAuth,
+      clientMetadata,
+      account,
+      data.deviceId,
+      parameters,
+      code,
+    )
+  }
+
+  protected async validateCodeGrant(
+    parameters: OAuthAuthorizationRequestParameters,
+    input: OAuthAuthorizationCodeGrantTokenRequest,
+  ): Promise<void> {
+    if (parameters.redirect_uri !== input.redirect_uri) {
+      throw new InvalidGrantError(
+        'The redirect_uri parameter must match the one used in the authorization request',
       )
-    } catch (err) {
-      // If a token is replayed, requestManager.findCode will throw. In that
-      // case, we need to revoke any token that was issued for this code.
+    }
 
-      const tokenInfo = await this.tokenManager.findByCode(code)
-      if (tokenInfo) {
-        await this.tokenManager.deleteToken(tokenInfo.id)
-
-        //  As an additional security measure, we also sign the device out, so
-        // that the device cannot be used to access the account anymore without
-        // a new authentication.
-        const { deviceId, sub } = tokenInfo.data
-        if (deviceId) {
-          await this.accountManager.removeDeviceAccount(deviceId, sub)
-        }
+    if (parameters.code_challenge) {
+      if (!input.code_verifier) {
+        throw new InvalidGrantError('code_verifier is required')
       }
+      if (input.code_verifier.length < 43) {
+        throw new InvalidGrantError('code_verifier too short')
+      }
+      switch (parameters.code_challenge_method) {
+        case undefined: // default is "plain"
+        case 'plain':
+          if (parameters.code_challenge !== input.code_verifier) {
+            throw new InvalidGrantError('Invalid code_verifier')
+          }
+          break
 
-      throw err
+        case 'S256': {
+          const inputChallenge = Buffer.from(
+            parameters.code_challenge,
+            'base64',
+          )
+          const computedChallenge = createHash('sha256')
+            .update(input.code_verifier)
+            .digest()
+          if (inputChallenge.compare(computedChallenge) !== 0) {
+            throw new InvalidGrantError('Invalid code_verifier')
+          }
+          break
+        }
+
+        default:
+          // Should never happen (because request validation should catch this)
+          throw new Error(`Unsupported code_challenge_method`)
+      }
+      const unique = await this.replayManager.uniqueCodeChallenge(
+        parameters.code_challenge,
+      )
+      if (!unique) {
+        throw new InvalidGrantError('Code challenge already used')
+      }
+    } else if (input.code_verifier !== undefined) {
+      throw new InvalidRequestError("code_challenge parameter wasn't provided")
     }
   }
 
-  async refreshTokenGrant(
+  protected async refreshTokenGrant(
     client: Client,
     clientAuth: ClientAuth,
     clientMetadata: RequestMetadata,
     input: OAuthRefreshTokenGrantTokenRequest,
     dpopProof: null | DpopProof,
   ): Promise<OAuthTokenResponse> {
-    return this.tokenManager.refresh(
-      client,
-      clientAuth,
-      clientMetadata,
-      input,
-      dpopProof,
-    )
+    const refreshToken = await refreshTokenSchema
+      .parseAsync(input.refresh_token, { path: ['refresh_token'] })
+      .catch((err) => {
+        throw InvalidGrantError.from(err, `Invalid refresh token`)
+      })
+
+    const tokenInfo = await this.tokenManager.consumeRefreshToken(refreshToken)
+
+    try {
+      const { data } = tokenInfo
+      await this.compareClientAuth(client, clientAuth, dpopProof, data)
+      await this.validateRefreshGrant(client, clientAuth, data)
+
+      return await this.tokenManager.rotateToken(
+        client,
+        clientAuth,
+        clientMetadata,
+        tokenInfo,
+      )
+    } catch (err) {
+      await this.tokenManager.deleteToken(tokenInfo.id)
+
+      throw err
+    }
+  }
+
+  protected async validateRefreshGrant(
+    client: Client,
+    clientAuth: ClientAuth,
+    data: TokenData,
+  ): Promise<void> {
+    const [sessionLifetime, refreshLifetime] =
+      clientAuth.method !== 'none' || client.info.isFirstParty
+        ? [
+            CONFIDENTIAL_CLIENT_SESSION_LIFETIME,
+            CONFIDENTIAL_CLIENT_REFRESH_LIFETIME,
+          ]
+        : [PUBLIC_CLIENT_SESSION_LIFETIME, PUBLIC_CLIENT_REFRESH_LIFETIME]
+
+    const sessionAge = Date.now() - data.createdAt.getTime()
+    if (sessionAge > sessionLifetime) {
+      throw new InvalidGrantError(`Session expired`)
+    }
+
+    const refreshAge = Date.now() - data.updatedAt.getTime()
+    if (refreshAge > refreshLifetime) {
+      throw new InvalidGrantError(`Refresh token expired`)
+    }
   }
 
   /**
    * @see {@link https://datatracker.ietf.org/doc/html/rfc7009#section-2.1 rfc7009}
    */
   public async revoke(
-    credentials: OAuthClientCredentials,
+    clientCredentials: OAuthClientCredentials,
     { token }: OAuthTokenIdentification,
+    dpopProof: null | DpopProof,
   ) {
     // > The authorization server first validates the client credentials (in
     // > case of a confidential client)
-    const [client, clientAuth] = await this.authenticateClient(credentials)
+    const { client, clientAuth } = await this.authenticateClient(
+      clientCredentials,
+      dpopProof,
+    )
 
     const tokenInfo = await this.tokenManager.findToken(token)
+    if (tokenInfo) {
+      // > [...] and then verifies whether the token was issued to the client
+      // > making the revocation request.
+      const { data } = tokenInfo
+      await this.compareClientAuth(client, clientAuth, dpopProof, data)
 
-    // > [...] and then verifies whether the token was issued to the client
-    // > making the revocation request. If this validation fails, the request is
-    // > refused and the client is informed of the error by the authorization
-    // > server as described below.
-    await this.tokenManager.validateAccess(client, clientAuth, tokenInfo)
-
-    // > In the next step, the authorization server invalidates the token. The
-    // > invalidation takes place immediately, and the token cannot be used
-    // > again after the revocation.
-    await this.tokenManager.deleteToken(tokenInfo.id)
+      // > In the next step, the authorization server invalidates the token. The
+      // > invalidation takes place immediately, and the token cannot be used
+      // > again after the revocation.
+      await this.tokenManager.deleteToken(tokenInfo.id)
+    }
   }
 
   protected override async verifyToken(

packages/oauth/oauth-provider/src/replay/replay-manager.ts

@@ -13,12 +13,16 @@ const asTimeFrame = (timeFrame: number) => Math.ceil(timeFrame * SECURITY_RATIO)
 export class ReplayManager {
   constructor(protected readonly replayStore: ReplayStore) {}
 
-  async uniqueAuth(jti: string, clientId: ClientId): Promise<boolean> {
-    return this.replayStore.unique(
-      `Auth@${clientId}`,
-      jti,
-      asTimeFrame(CLIENT_ASSERTION_MAX_AGE),
-    )
+  async uniqueAuth(
+    jti: string,
+    clientId: ClientId,
+    exp?: number,
+  ): Promise<boolean> {
+    const timeFrame =
+      exp == null
+        ? asTimeFrame(CLIENT_ASSERTION_MAX_AGE)
+        : exp * 1000 - Date.now()
+    return this.replayStore.unique(`Auth@${clientId}`, jti, timeFrame)
   }
 
   async uniqueJar(jti: string, clientId: ClientId): Promise<boolean> {

packages/oauth/oauth-provider/src/request/request-data.ts

@@ -1,14 +1,24 @@
 import { OAuthAuthorizationRequestParameters } from '@atproto/oauth-types'
-import { ClientAuth } from '../client/client-auth.js'
+import { ClientAuth, ClientAuthLegacy } from '../client/client-auth.js'
 import { ClientId } from '../client/client-id.js'
 import { DeviceId } from '../device/device-id.js'
 import { NonNullableKeys } from '../lib/util/type.js'
 import { Sub } from '../oidc/sub.js'
 import { Code } from './code.js'
 
+export type {
+  ClientAuth,
+  ClientAuthLegacy,
+  ClientId,
+  Code,
+  DeviceId,
+  OAuthAuthorizationRequestParameters,
+  Sub,
+}
+
 export type RequestData = {
   clientId: ClientId
-  clientAuth: ClientAuth
+  clientAuth: null | ClientAuth | ClientAuthLegacy
   parameters: Readonly<OAuthAuthorizationRequestParameters>
   expiresAt: Date
   deviceId: DeviceId | null

packages/oauth/oauth-provider/src/request/request-info.ts

@@ -10,5 +10,5 @@ export type RequestInfo = {
   parameters: Readonly<OAuthAuthorizationRequestParameters>
   expiresAt: Date
   clientId: ClientId
-  clientAuth: ClientAuth
+  clientAuth: null | ClientAuth
 }

packages/oauth/oauth-provider/src/request/request-manager.ts

@@ -1,7 +1,6 @@
 import { isAtprotoDid } from '@atproto/did'
 import type { Account } from '@atproto/oauth-provider-api'
 import {
-  CLIENT_ASSERTION_TYPE_JWT_BEARER,
   OAuthAuthorizationRequestParameters,
   OAuthAuthorizationServerMetadata,
 } from '@atproto/oauth-types'
@@ -11,6 +10,7 @@ import { ClientId } from '../client/client-id.js'
 import { Client } from '../client/client.js'
 import {
   AUTHORIZATION_INACTIVITY_TIMEOUT,
+  NODE_ENV,
   PAR_EXPIRES_IN,
   TOKEN_MAX_AGE,
 } from '../constants.js'
@@ -18,8 +18,6 @@ import { DeviceId } from '../device/device-id.js'
 import { AccessDeniedError } from '../errors/access-denied-error.js'
 import { ConsentRequiredError } from '../errors/consent-required-error.js'
 import { InvalidAuthorizationDetailsError } from '../errors/invalid-authorization-details-error.js'
-import { InvalidDpopKeyBindingError } from '../errors/invalid-dpop-key-binding-error.js'
-import { InvalidDpopProofError } from '../errors/invalid-dpop-proof-error.js'
 import { InvalidGrantError } from '../errors/invalid-grant-error.js'
 import { InvalidParametersError } from '../errors/invalid-parameters-error.js'
 import { InvalidRequestError } from '../errors/invalid-request-error.js'
@@ -27,7 +25,6 @@ import { InvalidScopeError } from '../errors/invalid-scope-error.js'
 import { RequestMetadata } from '../lib/http/request.js'
 import { callAsync } from '../lib/util/function.js'
 import { OAuthHooks } from '../oauth-hooks.js'
-import { DpopProof } from '../oauth-verifier.js'
 import { Signer } from '../signer/signer.js'
 import { Code, generateCode } from './code.js'
 import {
@@ -35,7 +32,6 @@ import {
   isRequestDataAuthorized,
 } from './request-data.js'
 import { generateRequestId } from './request-id.js'
-import { RequestInfo } from './request-info.js'
 import { RequestStore, UpdateRequestData } from './request-store.js'
 import {
   RequestUri,
@@ -58,21 +54,12 @@ export class RequestManager {
 
   async createAuthorizationRequest(
     client: Client,
-    clientAuth: ClientAuth,
+    clientAuth: null | ClientAuth,
     input: Readonly<OAuthAuthorizationRequestParameters>,
     deviceId: null | DeviceId,
-    dpopProof: null | DpopProof,
-  ): Promise<RequestInfo> {
-    const parameters = await this.validate(client, clientAuth, input, dpopProof)
-    return this.create(client, clientAuth, parameters, deviceId)
-  }
+  ) {
+    const parameters = await this.validate(client, clientAuth, input)
 
-  protected async create(
-    client: Client,
-    clientAuth: ClientAuth,
-    parameters: Readonly<OAuthAuthorizationRequestParameters>,
-    deviceId: null | DeviceId = null,
-  ): Promise<RequestInfo> {
     const expiresAt = new Date(Date.now() + PAR_EXPIRES_IN)
     const id = await generateRequestId()
 
@@ -87,14 +74,13 @@ export class RequestManager {
     })
 
     const uri = encodeRequestUri(id)
-    return { id, uri, expiresAt, parameters, clientId: client.id, clientAuth }
+    return { uri, expiresAt, parameters }
   }
 
   protected async validate(
     client: Client,
-    clientAuth: ClientAuth,
+    clientAuth: null | ClientAuth,
     parameters: Readonly<OAuthAuthorizationRequestParameters>,
-    dpopProof: null | DpopProof,
   ): Promise<Readonly<OAuthAuthorizationRequestParameters>> {
     // -------------------------------
     // Validate unsupported parameters
@@ -199,24 +185,6 @@ export class RequestManager {
 
     parameters = { ...parameters, scope: [...scopes].join(' ') || undefined }
 
-    // https://datatracker.ietf.org/doc/html/rfc9449#section-10
-    if (!parameters.dpop_jkt) {
-      if (dpopProof) parameters = { ...parameters, dpop_jkt: dpopProof.jkt }
-    } else if (!dpopProof) {
-      throw new InvalidDpopProofError('DPoP proof required')
-    } else if (parameters.dpop_jkt !== dpopProof.jkt) {
-      throw new InvalidDpopKeyBindingError()
-    }
-
-    if (clientAuth.method === CLIENT_ASSERTION_TYPE_JWT_BEARER) {
-      if (parameters.dpop_jkt && clientAuth.jkt === parameters.dpop_jkt) {
-        throw new InvalidParametersError(
-          parameters,
-          'The DPoP proof must be signed with a different key than the client assertion',
-        )
-      }
-    }
-
     if (parameters.code_challenge) {
       switch (parameters.code_challenge_method) {
         case undefined:
@@ -294,7 +262,7 @@ export class RequestManager {
     if (
       !client.info.isTrusted &&
       !client.info.isFirstParty &&
-      clientAuth.method === 'none'
+      client.metadata.token_endpoint_auth_method === 'none'
     ) {
       if (parameters.prompt === 'none') {
         throw new ConsentRequiredError(
@@ -328,11 +296,7 @@ export class RequestManager {
     return parameters
   }
 
-  async get(
-    uri: RequestUri,
-    deviceId: DeviceId,
-    clientId?: ClientId,
-  ): Promise<RequestInfo> {
+  async get(uri: RequestUri, deviceId: DeviceId, clientId?: ClientId) {
     const id = decodeRequestUri(uri)
 
     const data = await this.store.readRequest(id)
@@ -383,12 +347,10 @@ export class RequestManager {
     }
 
     return {
-      id,
       uri,
       expiresAt: updates.expiresAt || data.expiresAt,
       parameters: data.parameters,
       clientId: data.clientId,
-      clientAuth: data.clientAuth,
     }
   }
 
@@ -458,53 +420,31 @@ export class RequestManager {
    * @note If this method throws an error, any token previously generated from
    * the same `code` **must** me revoked.
    */
-  public async findCode(
-    client: Client,
-    clientAuth: ClientAuth,
-    code: Code,
-  ): Promise<RequestDataAuthorized & { requestUri: RequestUri }> {
-    const result = await this.store.findRequestByCode(code)
+  public async consumeCode(code: Code): Promise<RequestDataAuthorized> {
+    const result = await this.store.consumeRequestCode(code)
     if (!result) throw new InvalidGrantError('Invalid code')
 
     const { id, data } = result
-    try {
-      if (!isRequestDataAuthorized(data)) {
-        // Should never happen: maybe the store implementation is faulty ?
-        throw new Error('Unexpected request state')
+
+    // Fool-proofing the store implementation against code replay attacks (in
+    // case consumeRequestCode() does not delete the request).
+    if (NODE_ENV !== 'production') {
+      const result = await this.store.readRequest(id)
+      if (result) {
+        throw new Error('Invalid store implementation: request not deleted')
       }
-
-      if (data.clientId !== client.id) {
-        // Note: do not reveal the original client ID to the client using an invalid id
-        throw new InvalidGrantError(
-          `The code was not issued to client "${client.id}"`,
-        )
-      }
-
-      if (data.expiresAt < new Date()) {
-        throw new InvalidGrantError('This code has expired')
-      }
-
-      if (data.clientAuth.method === 'none') {
-        // If the client did not use PAR, it was not authenticated when the
-        // request was created (see authorize() method above). Since PAR is not
-        // mandatory, and since the token exchange currently taking place *is*
-        // authenticated (`clientAuth`), we allow "upgrading" the authentication
-        // method (the token created will be bound to the current clientAuth).
-      } else {
-        if (clientAuth.method !== data.clientAuth.method) {
-          throw new InvalidGrantError('Invalid client authentication')
-        }
-
-        if (!(await client.validateClientAuth(data.clientAuth))) {
-          throw new InvalidGrantError('Invalid client authentication')
-        }
-      }
-
-      return { ...data, requestUri: encodeRequestUri(id) }
-    } finally {
-      // A "code" can only be used once
-      await this.store.deleteRequest(id)
     }
+
+    if (!isRequestDataAuthorized(data) || data.code !== code) {
+      // Should never happen: maybe the store implementation is faulty ?
+      throw new Error('Unexpected request state')
+    }
+
+    if (data.expiresAt < new Date()) {
+      throw new InvalidGrantError('This code has expired')
+    }
+
+    return data
   }
 
   async delete(uri: RequestUri): Promise<void> {

packages/oauth/oauth-provider/src/request/request-store-memory.ts

@@ -1,39 +0,0 @@
-import { Code } from './code.js'
-import { RequestData } from './request-data.js'
-import { RequestId } from './request-id.js'
-import { RequestStore } from './request-store.js'
-
-export class RequestStoreMemory implements RequestStore {
-  #requests = new Map<RequestId, RequestData>()
-
-  async readRequest(id: RequestId): Promise<RequestData | null> {
-    return this.#requests.get(id) ?? null
-  }
-
-  async createRequest(id: RequestId, data: RequestData): Promise<void> {
-    this.#requests.set(id, data)
-  }
-
-  async updateRequest(
-    id: RequestId,
-    data: Partial<RequestData>,
-  ): Promise<void> {
-    const current = this.#requests.get(id)
-    if (!current) throw new Error('Request not found')
-    const newData = { ...current, ...data }
-    this.#requests.set(id, newData)
-  }
-
-  async deleteRequest(id: RequestId): Promise<void> {
-    this.#requests.delete(id)
-  }
-
-  async findRequestByCode(
-    code: Code,
-  ): Promise<{ id: RequestId; data: RequestData } | null> {
-    for (const [id, data] of this.#requests) {
-      if (data.code === code) return { id, data }
-    }
-    return null
-  }
-}

packages/oauth/oauth-provider/src/request/request-store-redis.ts

@@ -1,71 +0,0 @@
-import type { Redis } from 'ioredis'
-import { CreateRedisOptions, createRedis } from '../lib/redis.js'
-import { Code } from './code.js'
-import { RequestData } from './request-data.js'
-import { RequestId, requestIdSchema } from './request-id.js'
-import { RequestStore } from './request-store.js'
-
-export type { CreateRedisOptions, Redis }
-
-export type ReplayStoreRedisOptions = {
-  redis: CreateRedisOptions
-}
-
-export class RequestStoreRedis implements RequestStore {
-  private readonly redis: Redis
-
-  constructor(options: ReplayStoreRedisOptions) {
-    this.redis = createRedis(options.redis)
-  }
-
-  async readRequest(id: RequestId): Promise<RequestData | null> {
-    const data = await this.redis.get(id)
-    return data ? JSON.parse(data) : null
-  }
-
-  async createRequest(id: RequestId, data: RequestData): Promise<void> {
-    const timeFrame = data.expiresAt.getTime() - Date.now()
-    await this.redis.set(id, JSON.stringify(data), 'PX', timeFrame)
-    if (data.code) await this.redis.set(data.code, id, 'PX', timeFrame)
-  }
-
-  async updateRequest(
-    id: RequestId,
-    data: Partial<RequestData>,
-  ): Promise<void> {
-    const current = await this.readRequest(id)
-    if (!current) throw new Error('Request not found')
-    if (current.code) await this.redis.del(current.code)
-    const newData = { ...current, ...data }
-    await this.createRequest(id, newData)
-  }
-
-  async deleteRequest(id: RequestId): Promise<void> {
-    const data = await this.readRequest(id)
-    if (!data) return
-    if (data.code) await this.redis.del(data.code)
-    await this.redis.del(id)
-  }
-
-  private async findRequestIdByCode(code: Code): Promise<RequestId | null> {
-    const value = await this.redis.get(code)
-    if (!value) return null
-
-    const parsed = requestIdSchema.safeParse(value)
-    if (!parsed.success) return null
-
-    return parsed.data
-  }
-
-  async findRequestByCode(
-    code: Code,
-  ): Promise<{ id: RequestId; data: RequestData } | null> {
-    const id = await this.findRequestIdByCode(code)
-    if (!id) return null
-
-    const data = await this.readRequest(id)
-    if (!data) return null
-
-    return { id, data }
-  }
-}

packages/oauth/oauth-provider/src/request/request-store.ts

@@ -32,10 +32,14 @@ export interface RequestStore {
   updateRequest(id: RequestId, data: UpdateRequestData): Awaitable<void>
   deleteRequest(id: RequestId): void | Awaitable<void>
   /**
+   * @note it is **IMPORTANT** that this method prevents concurrent retrieval of
+   * the same code. If two requests are made with the same code, only one of
+   * them should succeed and return the request data.
+   *
    * @throws {InvalidGrantError} - When the request is not found or has expired
    * (allows to provide an error message instead of returning `null`).
    */
-  findRequestByCode(code: Code): Awaitable<FoundRequestResult | null>
+  consumeRequestCode(code: Code): Awaitable<FoundRequestResult | null>
 }
 
 export const isRequestStore = buildInterfaceChecker<RequestStore>([
@@ -43,15 +47,14 @@ export const isRequestStore = buildInterfaceChecker<RequestStore>([
   'readRequest',
   'updateRequest',
   'deleteRequest',
-  'findRequestByCode',
+  'consumeRequestCode',
 ])
 
-export function ifRequestStore<V extends Partial<RequestStore>>(
+export function asRequestStore<V extends Partial<RequestStore>>(
   implementation?: V,
-): (V & RequestStore) | undefined {
-  if (implementation && isRequestStore(implementation)) {
-    return implementation
+): V & RequestStore {
+  if (!implementation || !isRequestStore(implementation)) {
+    throw new Error('Invalid RequestStore implementation')
   }
-
-  return undefined
+  return implementation
 }

packages/oauth/oauth-provider/src/router/create-api-middleware.ts

@@ -367,7 +367,7 @@ export function createApiMiddleware<
           this.input.tokenId,
         )
 
-        if (tokenInfo.account.sub !== account.sub) {
+        if (!tokenInfo || tokenInfo.account.sub !== account.sub) {
           // report this as though the token was not found
           throw new InvalidRequestError(`Invalid token`)
         }

packages/oauth/oauth-provider/src/router/create-oauth-middleware.ts

@@ -168,8 +168,14 @@ export function createOAuthMiddleware<
         .parseAsync(payload, { path: ['body'] })
         .catch(throwInvalidRequest)
 
+      const dpopProof = await server.checkDpopProof(
+        req.method!,
+        this.url,
+        req.headers,
+      )
+
       try {
-        await server.revoke(credentials, tokenIdentification)
+        await server.revoke(credentials, tokenIdentification, dpopProof)
       } catch (err) {
         // > Note: invalid tokens do not cause an error response since the
         // > client cannot handle such an error in a reasonable way.  Moreover,

packages/oauth/oauth-provider/src/token/token-data.ts

@@ -2,7 +2,7 @@ import {
   OAuthAuthorizationDetails,
   OAuthAuthorizationRequestParameters,
 } from '@atproto/oauth-types'
-import { ClientAuth } from '../client/client-auth.js'
+import { ClientAuth, ClientAuthLegacy } from '../client/client-auth.js'
 import { ClientId } from '../client/client-id.js'
 import { DeviceId } from '../device/device-id.js'
 import { Sub } from '../oidc/sub.js'
@@ -23,7 +23,7 @@ export type TokenData = {
   updatedAt: Date
   expiresAt: Date
   clientId: ClientId
-  clientAuth: ClientAuth
+  clientAuth: ClientAuth | ClientAuthLegacy
   deviceId: DeviceId | null
   sub: Sub
   parameters: OAuthAuthorizationRequestParameters

packages/oauth/oauth-provider/src/token/token-manager.ts

@@ -1,30 +1,16 @@
-import { createHash } from 'node:crypto'
 import { SignedJwt, isSignedJwt } from '@atproto/jwk'
 import type { Account } from '@atproto/oauth-provider-api'
 import {
-  CLIENT_ASSERTION_TYPE_JWT_BEARER,
   OAuthAccessToken,
-  OAuthAuthorizationCodeGrantTokenRequest,
   OAuthAuthorizationRequestParameters,
-  OAuthClientCredentialsGrantTokenRequest,
-  OAuthPasswordGrantTokenRequest,
-  OAuthRefreshTokenGrantTokenRequest,
   OAuthTokenResponse,
   OAuthTokenType,
 } from '@atproto/oauth-types'
 import { AccessTokenMode } from '../access-token/access-token-mode.js'
 import { ClientAuth } from '../client/client-auth.js'
 import { Client } from '../client/client.js'
-import {
-  AUTHENTICATED_REFRESH_INACTIVITY_TIMEOUT,
-  AUTHENTICATED_REFRESH_LIFETIME,
-  TOKEN_MAX_AGE,
-  UNAUTHENTICATED_REFRESH_INACTIVITY_TIMEOUT,
-  UNAUTHENTICATED_REFRESH_LIFETIME,
-} from '../constants.js'
+import { TOKEN_MAX_AGE } from '../constants.js'
 import { DeviceId } from '../device/device-id.js'
-import { InvalidDpopKeyBindingError } from '../errors/invalid-dpop-key-binding-error.js'
-import { InvalidDpopProofError } from '../errors/invalid-dpop-proof-error.js'
 import { InvalidGrantError } from '../errors/invalid-grant-error.js'
 import { InvalidRequestError } from '../errors/invalid-request-error.js'
 import { InvalidTokenError } from '../errors/invalid-token-error.js'
@@ -41,7 +27,6 @@ import {
   RefreshToken,
   generateRefreshToken,
   isRefreshToken,
-  refreshTokenSchema,
 } from './refresh-token.js'
 import { TokenData } from './token-data.js'
 import { TokenId, generateTokenId, isTokenId } from './token-id.js'
@@ -94,125 +79,16 @@ export class TokenManager {
     })
   }
 
-  async create(
+  async createToken(
     client: Client,
     clientAuth: ClientAuth,
     clientMetadata: RequestMetadata,
     account: Account,
     deviceId: null | DeviceId,
     parameters: OAuthAuthorizationRequestParameters,
-    input:
-      | OAuthAuthorizationCodeGrantTokenRequest
-      | OAuthClientCredentialsGrantTokenRequest
-      | OAuthPasswordGrantTokenRequest,
-    dpopProof: null | DpopProof,
+    code: Code,
   ): Promise<OAuthTokenResponse> {
-    // @NOTE the atproto specific DPoP requirement is enforced though the
-    // "dpop_bound_access_tokens" metadata, which is enforced by the
-    // ClientManager class.
-    if (client.metadata.dpop_bound_access_tokens && !dpopProof) {
-      throw new InvalidDpopProofError('DPoP proof required')
-    }
-
-    if (!parameters.dpop_jkt) {
-      // Allow clients to bind their access tokens to a DPoP key during
-      // token request if they didn't provide a "dpop_jkt" during the
-      // authorization request.
-      if (dpopProof) parameters = { ...parameters, dpop_jkt: dpopProof.jkt }
-    } else if (!dpopProof) {
-      throw new InvalidDpopProofError('DPoP proof required')
-    } else if (parameters.dpop_jkt !== dpopProof.jkt) {
-      throw new InvalidDpopKeyBindingError()
-    }
-
-    if (clientAuth.method === CLIENT_ASSERTION_TYPE_JWT_BEARER) {
-      // Clients **must not** use their private key to sign DPoP proofs.
-      if (parameters.dpop_jkt && clientAuth.jkt === parameters.dpop_jkt) {
-        throw new InvalidRequestError(
-          'The DPoP proof must be signed with a different key than the client assertion',
-        )
-      }
-    }
-
-    if (!client.metadata.grant_types.includes(input.grant_type)) {
-      throw new InvalidGrantError(
-        `This client is not allowed to use the "${input.grant_type}" grant type`,
-      )
-    }
-
-    let code: Code | null = null
-
-    switch (input.grant_type) {
-      case 'authorization_code': {
-        if (!isCode(input.code)) {
-          throw new InvalidGrantError('Invalid code')
-        }
-
-        // @NOTE not using `this.findByCode` because we want to delete the token
-        // if it still exists (rather than throwing if the code is invalid).
-        const tokenInfo = await this.store.findTokenByCode(input.code)
-        if (tokenInfo) {
-          await this.deleteToken(tokenInfo.id)
-          throw new InvalidGrantError(`Code replayed`)
-        }
-
-        code = input.code
-
-        if (parameters.redirect_uri !== input.redirect_uri) {
-          throw new InvalidGrantError(
-            'The redirect_uri parameter must match the one used in the authorization request',
-          )
-        }
-
-        if (parameters.code_challenge) {
-          if (!input.code_verifier) {
-            throw new InvalidGrantError('code_verifier is required')
-          }
-          if (input.code_verifier.length < 43) {
-            throw new InvalidGrantError('code_verifier too short')
-          }
-          switch (parameters.code_challenge_method ?? 'plain') {
-            case 'plain': {
-              if (parameters.code_challenge !== input.code_verifier) {
-                throw new InvalidGrantError('Invalid code_verifier')
-              }
-              break
-            }
-            case 'S256': {
-              const inputChallenge = Buffer.from(
-                parameters.code_challenge,
-                'base64',
-              )
-              const computedChallenge = createHash('sha256')
-                .update(input.code_verifier)
-                .digest()
-              if (inputChallenge.compare(computedChallenge) !== 0) {
-                throw new InvalidGrantError('Invalid code_verifier')
-              }
-              break
-            }
-            default: {
-              // Should never happen (because request validation should catch this)
-              throw new Error(`Unsupported code_challenge_method`)
-            }
-          }
-        } else if (input.code_verifier !== undefined) {
-          throw new InvalidRequestError(
-            "code_challenge parameter wasn't provided",
-          )
-        }
-
-        break
-      }
-
-      default: {
-        // Other grants (e.g "password", "client_credentials") could be added
-        // here in the future...
-        throw new InvalidRequestError(
-          `Unsupported grant type "${input.grant_type}"`,
-        )
-      }
-    }
+    await this.validateTokenParams(client, clientAuth, parameters)
 
     const tokenId = await generateTokenId()
     const refreshToken = client.metadata.grant_types.includes('refresh_token')
@@ -235,26 +111,26 @@ export class TokenManager {
       code,
     }
 
+    const accessToken = await this.buildAccessToken(
+      tokenId,
+      account,
+      client,
+      parameters,
+      { now, expiresAt },
+    )
+
+    const response = await this.buildTokenResponse(
+      client,
+      accessToken,
+      refreshToken,
+      expiresAt,
+      parameters,
+      account.sub,
+    )
+
     await this.store.createToken(tokenId, tokenData, refreshToken)
 
     try {
-      const accessToken = await this.buildAccessToken(
-        tokenId,
-        account,
-        client,
-        parameters,
-        { now, expiresAt },
-      )
-
-      const response = await this.buildTokenResponse(
-        client,
-        accessToken,
-        refreshToken,
-        expiresAt,
-        parameters,
-        account.sub,
-      )
-
       await callAsync(this.hooks.onTokenCreated, {
         client,
         clientAuth,
@@ -265,13 +141,25 @@ export class TokenManager {
 
       return response
     } catch (err) {
-      // Just in case the token could not be issued, we delete it from the store
+      // If the hook fails, we delete the token to avoid leaving a dangling
+      // token in the store.
       await this.deleteToken(tokenId)
-
       throw err
     }
   }
 
+  protected async validateTokenParams(
+    client: Client,
+    clientAuth: ClientAuth,
+    parameters: OAuthAuthorizationRequestParameters,
+  ): Promise<void> {
+    if (client.metadata.dpop_bound_access_tokens && !parameters.dpop_jkt) {
+      throw new InvalidGrantError(
+        `DPoP JKT is required for DPoP bound access tokens`,
+      )
+    }
+  }
+
   protected buildTokenResponse(
     client: Client,
     accessToken: OAuthAccessToken,
@@ -299,166 +187,68 @@ export class TokenManager {
     }
   }
 
-  public async validateAccess(
-    client: Client,
-    clientAuth: ClientAuth,
-    tokenInfo: TokenInfo,
-  ) {
-    if (tokenInfo.data.clientId !== client.id) {
-      throw new InvalidGrantError(`Token was not issued to this client`)
-    }
-
-    if (tokenInfo.data.clientAuth.method !== clientAuth.method) {
-      throw new InvalidGrantError(`Client authentication method mismatch`)
-    }
-
-    if (!(await client.validateClientAuth(tokenInfo.data.clientAuth))) {
-      throw new InvalidGrantError(`Client authentication mismatch`)
-    }
-  }
-
-  public async validateRefresh(
-    client: Client,
-    clientAuth: ClientAuth,
-    { data }: TokenInfo,
-  ): Promise<void> {
-    // @TODO This value should be computable even if we don't have the "client"
-    // (because fetching client info could be flaky). Instead, all the info
-    // needed should be stored in the token info.
-    const allowLongerLifespan =
-      client.info.isFirstParty || data.clientAuth.method !== 'none'
-
-    const lifetime = allowLongerLifespan
-      ? AUTHENTICATED_REFRESH_LIFETIME
-      : UNAUTHENTICATED_REFRESH_LIFETIME
-
-    if (data.createdAt.getTime() + lifetime < Date.now()) {
-      throw new InvalidGrantError(`Refresh token expired`)
-    }
-
-    const inactivityTimeout = allowLongerLifespan
-      ? AUTHENTICATED_REFRESH_INACTIVITY_TIMEOUT
-      : UNAUTHENTICATED_REFRESH_INACTIVITY_TIMEOUT
-
-    if (data.updatedAt.getTime() + inactivityTimeout < Date.now()) {
-      throw new InvalidGrantError(`Refresh token exceeded inactivity timeout`)
-    }
-  }
-
-  async refresh(
+  async rotateToken(
     client: Client,
     clientAuth: ClientAuth,
     clientMetadata: RequestMetadata,
-    input: OAuthRefreshTokenGrantTokenRequest,
-    dpopProof: null | DpopProof,
+    tokenInfo: TokenInfo,
   ): Promise<OAuthTokenResponse> {
-    const refreshTokenParsed = refreshTokenSchema.safeParse(input.refresh_token)
-    if (!refreshTokenParsed.success) {
-      throw new InvalidRequestError('Invalid refresh token')
-    }
-    const refreshToken = refreshTokenParsed.data
-
-    const tokenInfo = await this.findByRefreshToken(refreshToken).catch(
-      (err) => {
-        throw InvalidGrantError.from(
-          err,
-          err instanceof InvalidRequestError
-            ? err.error_description
-            : 'Invalid refresh token',
-        )
-      },
-    )
-
     const { account, data } = tokenInfo
     const { parameters } = data
 
-    try {
-      await this.validateAccess(client, clientAuth, tokenInfo)
-      await this.validateRefresh(client, clientAuth, tokenInfo)
+    await this.validateTokenParams(client, clientAuth, parameters)
 
-      if (!client.metadata.grant_types.includes(input.grant_type)) {
-        // In case the client metadata was updated after the token was issued
-        throw new InvalidGrantError(
-          `This client is not allowed to use the "${input.grant_type}" grant type`,
-        )
-      }
+    const nextTokenId = await generateTokenId()
+    const nextRefreshToken = await generateRefreshToken()
 
-      if (parameters.dpop_jkt) {
-        if (!dpopProof) {
-          throw new InvalidDpopProofError('DPoP proof required')
-        } else if (parameters.dpop_jkt !== dpopProof.jkt) {
-          throw new InvalidDpopKeyBindingError()
-        }
-      }
+    const now = new Date()
+    const expiresAt = this.createTokenExpiry(now)
 
-      const nextTokenId = await generateTokenId()
-      const nextRefreshToken = await generateRefreshToken()
+    await this.store.rotateToken(tokenInfo.id, nextTokenId, nextRefreshToken, {
+      updatedAt: now,
+      expiresAt,
+      // @NOTE Normally, the clientAuth not change over time. There are two
+      // exceptions:
+      // - Upgrade from a legacy representation of client authentication to
+      //   a modern one.
+      // - Allow clients to become "confidential" if they were previously
+      //   "public"
+      clientAuth,
+    })
 
-      const now = new Date()
-      const expiresAt = this.createTokenExpiry(now)
+    const accessToken = await this.buildAccessToken(
+      nextTokenId,
+      account,
+      client,
+      parameters,
+      { now, expiresAt },
+    )
 
-      await this.store.rotateToken(
-        tokenInfo.id,
-        nextTokenId,
-        nextRefreshToken,
-        {
-          updatedAt: now,
-          expiresAt,
-          // When clients rotate their public keys, we store the key that was
-          // used by the client to authenticate itself while requesting new
-          // tokens. The validateAccess() method will ensure that the client
-          // still advertises the key that was used to issue the previous
-          // refresh token. If a client stops advertising a key, all tokens
-          // bound to that key will no longer be be refreshable. This allows
-          // clients to proactively invalidate tokens when a key is compromised.
-          // Note that the original DPoP key cannot be rotated. This protects
-          // users in case the ownership of the client id changes. In the latter
-          // case, a malicious actor could still advertises the public keys of
-          // the previous owner, but the new owner would not be able to present
-          // a valid DPoP proof.
-          clientAuth,
-        },
-      )
+    const response = await this.buildTokenResponse(
+      client,
+      accessToken,
+      nextRefreshToken,
+      expiresAt,
+      parameters,
+      account.sub,
+    )
 
-      const accessToken = await this.buildAccessToken(
-        nextTokenId,
-        account,
-        client,
-        parameters,
-        { now, expiresAt },
-      )
+    await callAsync(this.hooks.onTokenRefreshed, {
+      client,
+      clientAuth,
+      clientMetadata,
+      account,
+      parameters,
+    })
 
-      const response = await this.buildTokenResponse(
-        client,
-        accessToken,
-        nextRefreshToken,
-        expiresAt,
-        parameters,
-        account.sub,
-      )
-
-      await callAsync(this.hooks.onTokenRefreshed, {
-        client,
-        clientAuth,
-        clientMetadata,
-        account,
-        parameters,
-      })
-
-      return response
-    } catch (err) {
-      // Just in case the token could not be refreshed, we delete it from the store
-      await this.deleteToken(tokenInfo.id)
-
-      throw err
-    }
+    return response
   }
 
   /**
    * @note The token validity is not guaranteed. The caller must ensure that the
    * token is valid before using the returned token info.
    */
-  public async findToken(token: string): Promise<TokenInfo> {
+  public async findToken(token: string): Promise<null | TokenInfo> {
     if (isTokenId(token)) {
       return this.getTokenInfo(token)
     } else if (isCode(token)) {
@@ -466,18 +256,19 @@ export class TokenManager {
     } else if (isRefreshToken(token)) {
       return this.findByRefreshToken(token)
     } else if (isSignedJwt(token)) {
-      return this.findBySignedJwt(token)
+      return this.findByAccessToken(token)
     } else {
       throw new InvalidRequestError(`Invalid token`)
     }
   }
 
-  public async findBySignedJwt(token: SignedJwt): Promise<TokenInfo> {
+  public async findByAccessToken(token: SignedJwt): Promise<null | TokenInfo> {
     const { payload } = await this.signer.verifyAccessToken(token, {
       clockTolerance: Infinity,
     })
 
     const tokenInfo = await this.getTokenInfo(payload.jti)
+    if (!tokenInfo) return null
 
     // Fool-proof: Invalid store implementation ?
     if (payload.sub !== tokenInfo.account.sub) {
@@ -490,44 +281,49 @@ export class TokenManager {
     return tokenInfo
   }
 
-  public async findByRefreshToken(token: RefreshToken): Promise<TokenInfo> {
-    const tokenInfo = await this.store.findTokenByRefreshToken(token)
+  protected async findByRefreshToken(
+    token: RefreshToken,
+  ): Promise<null | TokenInfo> {
+    return this.store.findTokenByRefreshToken(token)
+  }
+
+  public async consumeRefreshToken(token: RefreshToken): Promise<TokenInfo> {
+    // @NOTE concurrent refreshes of the same refresh token could theoretically
+    // lead to two new tokens (access & refresh) being created. This is deemed
+    // acceptable for now (as the mechanism can only be used once since only one
+    // of the two refresh token created will be valid, and any future refresh
+    // attempts from outdated tokens will cause the entire session to be
+    // invalidated). Ideally, the store should be able to handle this case by
+    // atomically consuming the refresh token and returning the token info.
+
+    // @TODO Add another store method that atomically consumes the refresh token
+    // with a lock.
+    const tokenInfo = await this.findByRefreshToken(token).catch((err) => {
+      throw InvalidTokenError.from(err, `Invalid refresh token`)
+    })
 
     if (!tokenInfo) {
-      throw new InvalidRequestError(`Invalid refresh token`)
+      throw new InvalidGrantError(`Invalid refresh token`)
     }
 
     if (tokenInfo.currentRefreshToken !== token) {
       await this.deleteToken(tokenInfo.id)
-
-      throw new InvalidRequestError(`Refresh token replayed`)
+      throw new InvalidGrantError(`Refresh token replayed`)
     }
 
     return tokenInfo
   }
 
-  public async findByCode(code: Code): Promise<TokenInfo> {
-    const tokenInfo = await this.store.findTokenByCode(code)
-
-    if (!tokenInfo) {
-      throw new InvalidRequestError(`Invalid code`)
-    }
-
-    return tokenInfo
+  public async findByCode(code: Code): Promise<null | TokenInfo> {
+    return this.store.findTokenByCode(code)
   }
 
   public async deleteToken(tokenId: TokenId): Promise<void> {
     return this.store.deleteToken(tokenId)
   }
 
-  async getTokenInfo(tokenId: TokenId): Promise<TokenInfo> {
-    const tokenInfo = await this.store.readToken(tokenId)
-
-    if (!tokenInfo) {
-      throw new InvalidRequestError(`Invalid token`)
-    }
-
-    return tokenInfo
+  async getTokenInfo(tokenId: TokenId): Promise<null | TokenInfo> {
+    return this.store.readToken(tokenId)
   }
 
   async verifyToken(
@@ -541,6 +337,10 @@ export class TokenManager {
       throw InvalidTokenError.from(err, tokenType)
     })
 
+    if (!tokenInfo) {
+      throw new InvalidTokenError(tokenType, `Invalid token`)
+    }
+
     if (isCurrentTokenExpired(tokenInfo)) {
       await this.deleteToken(tokenId)
       throw new InvalidTokenError(tokenType, `Token expired`)

packages/oauth/oauth-types/src/oauth-authorization-request-parameters.ts

@@ -27,10 +27,9 @@ export const oauthAuthorizationRequestParametersSchema = z.object({
 
   // PKCE
 
+  // https://datatracker.ietf.org/doc/html/rfc7636#section-4.3
   code_challenge: z.string().optional(),
-  code_challenge_method: oauthCodeChallengeMethodSchema
-    .default('S256')
-    .optional(),
+  code_challenge_method: oauthCodeChallengeMethodSchema.optional(),
 
   // DPOP
 

packages/oauth/oauth-types/src/oauth-authorization-server-metadata.ts

@@ -45,7 +45,11 @@ export const oauthAuthorizationServerMetadataSchema = z.object({
   authorization_endpoint: webUriSchema, // .optional(),
 
   token_endpoint: webUriSchema, // .optional(),
-  token_endpoint_auth_methods_supported: z.array(z.string()).optional(),
+  // https://www.rfc-editor.org/rfc/rfc8414.html#section-2
+  token_endpoint_auth_methods_supported: z
+    .array(z.string())
+    // > If omitted, the default is "client_secret_basic" [...].
+    .default(['client_secret_basic']),
   token_endpoint_auth_signing_alg_values_supported: z
     .array(z.string())
     .optional(),
@@ -98,3 +102,15 @@ export const oauthAuthorizationServerMetadataValidator =
         }
       }
     })
+    .superRefine((data, ctx) => {
+      if (
+        data.token_endpoint_auth_signing_alg_values_supported?.includes('none')
+      ) {
+        // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3
+        // > The value `none` MUST NOT be used.
+        ctx.addIssue({
+          code: z.ZodIssueCode.custom,
+          message: 'Client authentication method "none" is not allowed',
+        })
+      }
+    })

packages/oauth/oauth-types/src/oauth-client-metadata.ts

@@ -19,6 +19,7 @@ export const oauthClientMetadataSchema = z.object({
   /**
    * @note redirect_uris require additional validation
    */
+  // https://www.rfc-editor.org/rfc/rfc7591.html#section-2
   redirect_uris: z.array(oauthRedirectUriSchema).nonempty(),
   response_types: z
     .array(oauthResponseTypeSchema)
@@ -33,19 +34,20 @@ export const oauthClientMetadataSchema = z.object({
     // > "authorization_code" Grant Type.
     .default(['authorization_code']),
   scope: oauthScopeSchema.optional(),
+  // https://www.rfc-editor.org/rfc/rfc7591.html#section-2
   token_endpoint_auth_method: oauthEndpointAuthMethod
-    .default('none')
-    .optional(),
+    // > If unspecified or omitted, the default is "client_secret_basic" [...].
+    .default('client_secret_basic'),
   token_endpoint_auth_signing_alg: z.string().optional(),
   userinfo_signed_response_alg: z.string().optional(),
   userinfo_encrypted_response_alg: z.string().optional(),
   jwks_uri: webUriSchema.optional(),
   jwks: jwksPubSchema.optional(),
-  application_type: z.enum(['web', 'native']).default('web').optional(), // default, per spec, is "web"
-  subject_type: z.enum(['public', 'pairwise']).default('public').optional(),
+  application_type: z.enum(['web', 'native']).default('web'), // default, per spec, is "web"
+  subject_type: z.enum(['public', 'pairwise']).default('public'),
   request_object_signing_alg: z.string().optional(),
   id_token_signed_response_alg: z.string().optional(),
-  authorization_signed_response_alg: z.string().default('RS256').optional(),
+  authorization_signed_response_alg: z.string().default('RS256'),
   authorization_encrypted_response_enc: z.enum(['A128CBC-HS256']).optional(),
   authorization_encrypted_response_alg: z.string().optional(),
   client_id: oauthClientIdSchema.optional(),

packages/pds/src/account-manager/db/schema/authorization-request.ts

@@ -1,6 +1,7 @@
 import { Selectable } from 'kysely'
 import {
   ClientAuth,
+  ClientAuthLegacy,
   Code,
   DeviceId,
   OAuthAuthorizationRequestParameters,
@@ -15,7 +16,7 @@ export interface AuthorizationRequest {
   deviceId: DeviceId | null
 
   clientId: OAuthClientId
-  clientAuth: JsonEncoded<ClientAuth>
+  clientAuth: JsonEncoded<null | ClientAuth | ClientAuthLegacy>
   parameters: JsonEncoded<OAuthAuthorizationRequestParameters>
   expiresAt: DateISO
   code: Code | null

packages/pds/src/account-manager/db/schema/token.ts

@@ -1,6 +1,7 @@
 import { Generated, Selectable } from 'kysely'
 import {
   ClientAuth,
+  ClientAuthLegacy,
   Code,
   DeviceId,
   OAuthAuthorizationDetails,
@@ -21,7 +22,7 @@ export interface Token {
   updatedAt: DateISO
   expiresAt: DateISO
   clientId: OAuthClientId
-  clientAuth: JsonEncoded<ClientAuth>
+  clientAuth: JsonEncoded<ClientAuth | ClientAuthLegacy>
   deviceId: DeviceId | null
   parameters: JsonEncoded<OAuthAuthorizationRequestParameters>
   details: JsonEncoded<OAuthAuthorizationDetails> | null

packages/pds/src/account-manager/helpers/authorization-request.ts

@@ -73,10 +73,10 @@ export const removeOldExpiredQB = (db: AccountDb, delay = 600e3) =>
 export const removeByIdQB = (db: AccountDb, id: RequestId) =>
   db.db.deleteFrom('authorization_request').where('id', '=', id)
 
-export const findByCodeQB = (db: AccountDb, code: Code) =>
+export const consumeByCodeQB = (db: AccountDb, code: Code) =>
   db.db
-    .selectFrom('authorization_request')
+    .deleteFrom('authorization_request')
     // uses "authorization_request_code_idx" partial index (hence the null check)
     .where('code', '=', code)
     .where('code', 'is not', null)
-    .selectAll()
+    .returningAll()

packages/pds/src/account-manager/oauth-store.ts

@@ -428,9 +428,9 @@ export class OAuthStore
     await this.db.executeWithRetry(authRequestHelper.removeByIdQB(this.db, id))
   }
 
-  async findRequestByCode(code: Code): Promise<FoundRequestResult | null> {
+  async consumeRequestCode(code: Code): Promise<FoundRequestResult | null> {
     const row = await authRequestHelper
-      .findByCodeQB(this.db, code)
+      .consumeByCodeQB(this.db, code)
       .executeTakeFirst()
     return row ? authRequestHelper.rowToFoundRequestResult(row) : null
   }