OAuth url scheme validation (#3066)

* Improve message of OAuthResolverError in case of metadata validation error * Use named export from zod * docs * Enforce use of http and https url where applicable * Verify authorization_endpoint URL protocol * fix pds tests for new oauth resource metadata check * Allow non-https urls as resource metadata url * Strong validation or redirect_uri * Ensure that client-id is a web url * explicit use of "url" schema as potentially dangerous * changeset * tidy * simplify type * prevent loopback hostname for https: redirect uris * Forbid use of non https internet uris * allow "localhost" for web uris * tidy * tidy * tidy --------- Co-authored-by: Devin Ivy <devinivy@gmail.com>
bluesky-social · Nov 25, 2024 · 5ddd512 · 5ddd512
1 parent b5c6bce
commit 5ddd512
Show file tree

Hide file tree

Showing 28 changed files with 503 additions and 153 deletions.
diff --git a/.changeset/angry-spiders-hunt.md b/.changeset/angry-spiders-hunt.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Add oauthClientIdLoopbackSchema and oauthClientIdDiscoverableSchema schemas
diff --git a/.changeset/cyan-parrots-obey.md b/.changeset/cyan-parrots-obey.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client": patch
+---
+
+Verify authorization_endpoint URL protocol
diff --git a/.changeset/green-cameras-grin.md b/.changeset/green-cameras-grin.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client": patch
+---
+
+Ensure that client-id is a web url
diff --git a/.changeset/little-pets-sit.md b/.changeset/little-pets-sit.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client": patch
+---
+
+Improve message of OAuthResolverError in case of metadata validation error
diff --git a/.changeset/new-badgers-cross.md b/.changeset/new-badgers-cross.md
@@ -0,0 +1,5 @@
+---
+"@atproto/pds": patch
+---
+
+Prevent use of non https: resource uri in production environments
diff --git a/.changeset/small-apes-retire.md b/.changeset/small-apes-retire.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Enforce use of http and https url where applicable
diff --git a/.changeset/spotty-owls-drum.md b/.changeset/spotty-owls-drum.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Strong validation or redirect_uri
diff --git a/packages/oauth/oauth-client/src/atproto-token-response.ts b/packages/oauth/oauth-client/src/atproto-token-response.ts
@@ -1,6 +1,6 @@
 import { atprotoDidSchema } from '@atproto/did'
 import { oauthTokenResponseSchema } from '@atproto/oauth-types'
-import { z } from 'zod'
+import { z, TypeOf } from 'zod'
 
 import { includesSpaceSeparatedValue, SpaceSeparatedValue } from './util'
 
@@ -19,4 +19,4 @@ export const atprotoTokenResponseSchema = oauthTokenResponseSchema.extend({
   id_token: z.never().optional(),
 })
 
-export type AtprotoTokenResponse = z.infer<typeof atprotoTokenResponseSchema>
+export type AtprotoTokenResponse = TypeOf<typeof atprotoTokenResponseSchema>
diff --git a/packages/oauth/oauth-client/src/oauth-client.ts b/packages/oauth/oauth-client/src/oauth-client.ts
@@ -316,14 +316,26 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
       scope: options?.scope ?? this.clientMetadata.scope,
     }
 
+    const authorizationUrl = new URL(metadata.authorization_endpoint)
+
+    // Since the user will be redirected to the authorization_endpoint url using
+    // a browser, we need to make sure that the url is valid.
+    if (
+      authorizationUrl.protocol !== 'https:' &&
+      authorizationUrl.protocol !== 'http:'
+    ) {
+      throw new TypeError(
+        `Invalid authorization endpoint protocol: ${authorizationUrl.protocol}`,
+      )
+    }
+
     if (metadata.pushed_authorization_request_endpoint) {
       const server = await this.serverFactory.fromMetadata(metadata, dpopKey)
       const parResponse = await server.request(
         'pushed_authorization_request',
         parameters,
       )
 
-      const authorizationUrl = new URL(metadata.authorization_endpoint)
       authorizationUrl.searchParams.set(
         'client_id',
         this.clientMetadata.client_id,
@@ -335,7 +347,6 @@ export class OAuthClient extends CustomEventTarget<OAuthClientEventMap> {
         'Server requires pushed authorization requests (PAR) but no PAR endpoint is available',
       )
     } else {
-      const authorizationUrl = new URL(metadata.authorization_endpoint)
       for (const [key, value] of Object.entries(parameters)) {
         if (value) authorizationUrl.searchParams.set(key, String(value))
       }

diff --git a/packages/oauth/oauth-client/src/oauth-resolver-error.ts b/packages/oauth/oauth-client/src/oauth-resolver-error.ts
@@ -1,11 +1,20 @@
+import { ZodError } from 'zod'
+
 export class OAuthResolverError extends Error {
   constructor(message: string, options?: { cause?: unknown }) {
     super(message, options)
   }
 
   static from(cause: unknown, message?: string): OAuthResolverError {
     if (cause instanceof OAuthResolverError) return cause
-    return new OAuthResolverError(message ?? `Unable to resolve identity`, {
+    const validationReason =
+      cause instanceof ZodError
+        ? `${cause.errors[0].path} ${cause.errors[0].message}`
+        : null
+    const fullMessage =
+      (message ?? `Unable to resolve identity`) +
+      (validationReason ? ` (${validationReason})` : '')
+    return new OAuthResolverError(fullMessage, {
       cause,
     })
   }

diff --git a/packages/oauth/oauth-client/src/types.ts b/packages/oauth/oauth-client/src/types.ts
@@ -1,9 +1,10 @@
 import {
+  oauthClientIdLoopbackSchema,
   OAuthAuthorizationRequestParameters,
-  oauthClientIdSchema,
+  oauthClientIdDiscoverableSchema,
   oauthClientMetadataSchema,
 } from '@atproto/oauth-types'
-import z from 'zod'
+import { TypeOf, z } from 'zod'
 
 import { Simplify } from './util.js'
 
@@ -26,7 +27,10 @@ export type AuthorizeOptions = Simplify<
 >
 
 export const clientMetadataSchema = oauthClientMetadataSchema.extend({
-  client_id: oauthClientIdSchema.url(),
+  client_id: z.union([
+    oauthClientIdDiscoverableSchema,
+    oauthClientIdLoopbackSchema,
+  ]),
 })
 
-export type ClientMetadata = z.infer<typeof clientMetadataSchema>
+export type ClientMetadata = TypeOf<typeof clientMetadataSchema>
diff --git a/packages/oauth/oauth-provider/src/client/client.ts b/packages/oauth/oauth-provider/src/client/client.ts
@@ -4,6 +4,7 @@ import {
   OAuthAuthorizationRequestParameters,
   OAuthClientCredentials,
   OAuthClientMetadata,
+  OAuthRedirectUri,
 } from '@atproto/oauth-types'
 import {
   UnsecuredJWT,
@@ -323,7 +324,7 @@ export class Client {
     return parameters
   }
 
-  get defaultRedirectUri(): string | undefined {
+  get defaultRedirectUri(): OAuthRedirectUri | undefined {
     const { redirect_uris } = this.metadata
     return redirect_uris.length === 1 ? redirect_uris[0] : undefined
   }

diff --git a/packages/oauth/oauth-provider/src/oauth-verifier.ts b/packages/oauth/oauth-provider/src/oauth-verifier.ts
@@ -1,6 +1,7 @@
 import { Key, Keyset, isSignedJwt } from '@atproto/jwk'
 import {
   OAuthAccessToken,
+  OAuthIssuerIdentifier,
   OAuthTokenType,
   oauthIssuerIdentifierSchema,
 } from '@atproto/oauth-types'
@@ -77,7 +78,7 @@ export {
 }
 
 export class OAuthVerifier {
-  public readonly issuer: string
+  public readonly issuer: OAuthIssuerIdentifier
   public readonly keyset: Keyset
 
   protected readonly accessTokenType: AccessTokenType

diff --git a/packages/oauth/oauth-provider/src/request/request-uri.ts b/packages/oauth/oauth-provider/src/request/request-uri.ts
@@ -6,7 +6,6 @@ export const REQUEST_URI_PREFIX = 'urn:ietf:params:oauth:request_uri:'
 
 export const requestUriSchema = z
   .string()
-  .url()
   .refinement(
     (data): data is `${typeof REQUEST_URI_PREFIX}${RequestId}` =>
       data.startsWith(REQUEST_URI_PREFIX) &&

diff --git a/packages/oauth/oauth-types/src/atproto-loopback-client-metadata.ts b/packages/oauth/oauth-types/src/atproto-loopback-client-metadata.ts
@@ -1,16 +1,21 @@
-import { parseOAuthLoopbackClientId } from './oauth-client-id-loopback.js'
+import {
+  OAuthClientIdLoopback,
+  parseOAuthLoopbackClientId,
+} from './oauth-client-id-loopback.js'
 import { OAuthClientMetadataInput } from './oauth-client-metadata.js'
 
 export function atprotoLoopbackClientMetadata(
   clientId: string,
-): OAuthClientMetadataInput {
+): OAuthClientMetadataInput & {
+  client_id: OAuthClientIdLoopback
+} {
   const {
     scope = 'atproto',
     redirect_uris = [`http://127.0.0.1/`, `http://[::1]/`],
   } = parseOAuthLoopbackClientId(clientId)
 
   return {
-    client_id: clientId,
+    client_id: clientId as OAuthClientIdLoopback,
     scope,
     redirect_uris,
     client_name: 'Loopback client',

diff --git a/packages/oauth/oauth-types/src/index.ts b/packages/oauth/oauth-types/src/index.ts
@@ -1,4 +1,5 @@
 export * from './constants.js'
+export * from './uri.js'
 export * from './util.js'
 
 export * from './atproto-loopback-client-metadata.js'
@@ -25,6 +26,7 @@ export * from './oauth-issuer-identifier.js'
 export * from './oauth-par-response.js'
 export * from './oauth-password-grant-token-request.js'
 export * from './oauth-protected-resource-metadata.js'
+export * from './oauth-redirect-uri.js'
 export * from './oauth-refresh-token-grant-token-request.js'
 export * from './oauth-refresh-token.js'
 export * from './oauth-request-uri.js'

diff --git a/packages/oauth/oauth-types/src/oauth-authorization-code-grant-token-request.ts b/packages/oauth/oauth-types/src/oauth-authorization-code-grant-token-request.ts
@@ -1,9 +1,10 @@
 import { z } from 'zod'
+import { oauthRedirectUriSchema } from './oauth-redirect-uri.js'
 
 export const oauthAuthorizationCodeGrantTokenRequestSchema = z.object({
   grant_type: z.literal('authorization_code'),
   code: z.string().min(1),
-  redirect_uri: z.string().url(),
+  redirect_uri: oauthRedirectUriSchema,
   /** @see {@link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1} */
   code_verifier: z
     .string()

diff --git a/packages/oauth/oauth-types/src/oauth-authorization-details.ts b/packages/oauth/oauth-types/src/oauth-authorization-details.ts
@@ -1,14 +1,34 @@
 import { z } from 'zod'
+import { dangerousUriSchema } from './uri.js'
 
 /**
  * @see {@link https://datatracker.ietf.org/doc/html/rfc9396#section-2 | RFC 9396, Section 2}
  */
 export const oauthAuthorizationDetailSchema = z.object({
   type: z.string(),
-  locations: z.array(z.string().url()).optional(),
+  /**
+   * An array of strings representing the location of the resource or RS. These
+   * strings are typically URIs identifying the location of the RS.
+   */
+  locations: z.array(dangerousUriSchema).optional(),
+  /**
+   * An array of strings representing the kinds of actions to be taken at the
+   * resource.
+   */
   actions: z.array(z.string()).optional(),
+  /**
+   * An array of strings representing the kinds of data being requested from the
+   * resource.
+   */
   datatypes: z.array(z.string()).optional(),
+  /**
+   * A string identifier indicating a specific resource available at the API.
+   */
   identifier: z.string().optional(),
+  /**
+   * An array of strings representing the types or levels of privilege being
+   * requested at the resource.
+   */
   privileges: z.array(z.string()).optional(),
 })
 

diff --git a/packages/oauth/oauth-types/src/oauth-authorization-request-parameters.ts b/packages/oauth/oauth-types/src/oauth-authorization-request-parameters.ts
@@ -4,6 +4,7 @@ import { z } from 'zod'
 import { oauthAuthorizationDetailsSchema } from './oauth-authorization-details.js'
 import { oauthClientIdSchema } from './oauth-client-id.js'
 import { oauthCodeChallengeMethodSchema } from './oauth-code-challenge-method.js'
+import { oauthRedirectUriSchema } from './oauth-redirect-uri.js'
 import { oauthResponseTypeSchema } from './oauth-response-type.js'
 import { oauthScopeSchema } from './oauth-scope.js'
 import { oidcClaimsParameterSchema } from './oidc-claims-parameter.js'
@@ -16,7 +17,7 @@ import { oidcEntityTypeSchema } from './oidc-entity-type.js'
 export const oauthAuthorizationRequestParametersSchema = z.object({
   client_id: oauthClientIdSchema,
   state: z.string().optional(),
-  redirect_uri: z.string().url().optional(),
+  redirect_uri: oauthRedirectUriSchema.optional(),
   scope: oauthScopeSchema.optional(),
   response_type: oauthResponseTypeSchema,
 

diff --git a/packages/oauth/oauth-types/src/oauth-authorization-server-metadata.ts b/packages/oauth/oauth-types/src/oauth-authorization-server-metadata.ts
@@ -2,9 +2,13 @@ import { z } from 'zod'
 
 import { oauthCodeChallengeMethodSchema } from './oauth-code-challenge-method.js'
 import { oauthIssuerIdentifierSchema } from './oauth-issuer-identifier.js'
+import { webUriSchema } from './uri.js'
 
 /**
  * @see {@link https://datatracker.ietf.org/doc/html/rfc8414}
+ * @note we do not enforce https: scheme in URIs to support development
+ * environments. Make sure to validate the URIs before using it in a production
+ * environment.
  */
 export const oauthAuthorizationServerMetadataSchema = z.object({
   issuer: oauthIssuerIdentifierSchema,
@@ -37,31 +41,31 @@ export const oauthAuthorizationServerMetadataSchema = z.object({
     .array(z.string())
     .optional(),
 
-  jwks_uri: z.string().url().optional(),
+  jwks_uri: webUriSchema.optional(),
 
-  authorization_endpoint: z.string().url(), // .optional(),
+  authorization_endpoint: webUriSchema, // .optional(),
 
-  token_endpoint: z.string().url(), // .optional(),
+  token_endpoint: webUriSchema, // .optional(),
   token_endpoint_auth_methods_supported: z.array(z.string()).optional(),
   token_endpoint_auth_signing_alg_values_supported: z
     .array(z.string())
     .optional(),
 
-  revocation_endpoint: z.string().url().optional(),
-  introspection_endpoint: z.string().url().optional(),
-  pushed_authorization_request_endpoint: z.string().url().optional(),
+  revocation_endpoint: webUriSchema.optional(),
+  introspection_endpoint: webUriSchema.optional(),
+  pushed_authorization_request_endpoint: webUriSchema.optional(),
 
   require_pushed_authorization_requests: z.boolean().optional(),
 
-  userinfo_endpoint: z.string().url().optional(),
-  end_session_endpoint: z.string().url().optional(),
-  registration_endpoint: z.string().url().optional(),
+  userinfo_endpoint: webUriSchema.optional(),
+  end_session_endpoint: webUriSchema.optional(),
+  registration_endpoint: webUriSchema.optional(),
 
   // https://datatracker.ietf.org/doc/html/rfc9449#section-5.1
   dpop_signing_alg_values_supported: z.array(z.string()).optional(),
 
   // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-resource-metadata-05#section-4
-  protected_resources: z.array(z.string().url()).optional(),
+  protected_resources: z.array(webUriSchema).optional(),
 
   // https://drafts.aaronpk.com/draft-parecki-oauth-client-id-metadata-document/draft-parecki-oauth-client-id-metadata-document.html
   client_id_metadata_document_supported: z.boolean().optional(),