OAuth client validation improvements (#4289)

matthieusieben · web-flow · commit 8ff5ec4caa9a · 2025-10-24T12:53:03.000+02:00
* OAuth client validation improvements

* Remove `isLocalHostname` export

* tidy
diff --git a/.changeset/afraid-rings-tan.md b/.changeset/afraid-rings-tan.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-client-expo": patch
+---
+
+The package now re-exports everything from `@atproto/oauth-client`, avoiding the need to explicitly depend on it.
diff --git a/.changeset/honest-comics-search.md b/.changeset/honest-comics-search.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": patch
+---
+
+Enforce stronger validation of `privateUseUriSchema`
diff --git a/.changeset/lucky-lies-float.md b/.changeset/lucky-lies-float.md
@@ -0,0 +1,9 @@
+---
+"@atproto/oauth-types": minor
+---
+
+Remove exported `oauthHttpsRedirectURISchema` and `oauthPrivateUseRedirectURISchema` in favor of `oauthRedirectUriSchema` and `oauthLoopbackClientRedirectUriSchema`, which provide semantically meaningful groupings of redirect URIs based on OAuth client types.
+
+`oauthHttpsRedirectURISchema` can still be accessed using `httpsUriSchema`.
+`oauthPrivateUseRedirectURISchema` can still be accessed using `privateUseUriSchema`.
+
diff --git a/.changeset/soft-ghosts-try.md b/.changeset/soft-ghosts-try.md
@@ -0,0 +1,5 @@
+---
+"@atproto-labs/fetch-node": minor
+---
+
+Remove `isLocalHostname` export
diff --git a/.changeset/tiny-zebras-cough.md b/.changeset/tiny-zebras-cough.md
@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-types": minor
+---
+
+Remove unused `isLoopbackUrl` utility
diff --git a/packages/internal/fetch-node/src/unicast.ts b/packages/internal/fetch-node/src/unicast.ts
@@ -229,7 +229,7 @@ export function unicastLookup(
  * @returns whether the hostname is a name typically used for on locale area networks.
  * @note **DO NOT** use for security reasons. Only as heuristic.
  */
-export function isLocalHostname(hostname: string): boolean {
+function isLocalHostname(hostname: string): boolean {
   const parts = hostname.split('.')
   if (parts.length < 2) return true
 
diff --git a/packages/oauth/oauth-client-expo/src/index.ts b/packages/oauth/oauth-client-expo/src/index.ts
@@ -1,5 +1,7 @@
 import './polyfill'
 
+export * from '@atproto/oauth-client'
+
 export type { ExpoOAuthClientInterface } from './expo-oauth-client-interface'
 export type { ExpoOAuthClientOptions } from './expo-oauth-client-options'
 
diff --git a/packages/oauth/oauth-provider/src/client/client-manager.ts b/packages/oauth/oauth-provider/src/client/client-manager.ts
@@ -5,7 +5,7 @@ import {
   OAuthClientIdLoopback,
   OAuthClientMetadata,
   OAuthClientMetadataInput,
-  isLoopbackHost,
+  isLocalHostname,
   isOAuthClientIdDiscoverable,
   isOAuthClientIdLoopback,
   oauthClientMetadataSchema,
@@ -17,7 +17,6 @@ import {
   fetchJsonZodProcessor,
   fetchOkProcessor,
 } from '@atproto-labs/fetch'
-import { isLocalHostname } from '@atproto-labs/fetch-node'
 import { pipe } from '@atproto-labs/pipe'
 import {
   CachedGetter,
@@ -232,6 +231,10 @@ export class ClientManager {
     clientId: ClientId,
     metadata: OAuthClientMetadata,
   ): OAuthClientMetadata {
+    // @TODO This method should only check for rules that are specific to this
+    // implementation or the ATPROTO specification. All generic validation rules
+    // should be moved to the @atproto/oauth-types package.
+
     if (metadata.jwks && metadata.jwks_uri) {
       throw new InvalidClientMetadataError(
         'jwks_uri and jwks are mutually exclusive',
@@ -604,56 +607,12 @@ export class ClientManager {
         }
 
         case isPrivateUseUriScheme(url): {
-          // https://datatracker.ietf.org/doc/html/rfc8252#section-7.1
-          //
-          // > When choosing a URI scheme to associate with the app, apps MUST
-          // > use a URI scheme based on a domain name under their control,
-          // > expressed in reverse order, as recommended by Section 3.8 of
-          // > [RFC7595] for private-use URI schemes.
-
           if (metadata.application_type !== 'native') {
             throw new InvalidRedirectUriError(
               `Private-Use URI Scheme redirect URI are only allowed for native apps`,
             )
           }
 
-          // https://datatracker.ietf.org/doc/html/rfc8252#section-8.4
-          //
-          // > In addition to the collision-resistant properties, requiring a
-          // > URI scheme based on a domain name that is under the control of
-          // > the app can help to prove ownership in the event of a dispute
-          // > where two apps claim the same private-use URI scheme (where one
-          // > app is acting maliciously).
-          //
-          // We can't check for ownership here (as there is no concept of
-          // proven ownership in the generic client validation), but we can
-          // check that the domain is a valid domain name.
-
-          const urlDomain = reverseDomain(url.protocol.slice(0, -1))
-
-          if (isLocalHostname(urlDomain)) {
-            throw new InvalidRedirectUriError(
-              `Private-use URI Scheme redirect URI must not be a local hostname`,
-            )
-          }
-
-          // https://datatracker.ietf.org/doc/html/rfc8252#section-7.1
-          //
-          // > Following the requirements of Section 3.2 of [RFC3986], as there
-          // > is no naming authority for private-use URI scheme redirects, only
-          // > a single slash ("/") appears after the scheme component.
-          if (
-            url.href.startsWith(`${url.protocol}//`) ||
-            url.username ||
-            url.password ||
-            url.hostname ||
-            url.port
-          ) {
-            throw new InvalidRedirectUriError(
-              `Private-Use URI Scheme must be in the form ${url.protocol}/<path>`,
-            )
-          }
-
           break
         }
 
@@ -700,22 +659,6 @@ export class ClientManager {
       )
     }
 
-    for (const redirectUri of metadata.redirect_uris) {
-      const url = parseRedirectUri(redirectUri)
-
-      if (url.protocol !== 'http:') {
-        throw new InvalidRedirectUriError(
-          `Loopback clients must use HTTP redirect URIs`,
-        )
-      }
-
-      if (!isLoopbackHost(url.hostname)) {
-        throw new InvalidRedirectUriError(
-          `Loopback clients must use loopback redirect URIs`,
-        )
-      }
-    }
-
     return metadata
   }
 
@@ -762,9 +705,19 @@ export class ClientManager {
     }
 
     for (const redirectUri of metadata.redirect_uris) {
+      // @NOTE at this point, all redirect URIs have already been validated by
+      // oauthRedirectUriSchema
+
       const url = parseRedirectUri(redirectUri)
 
       if (isPrivateUseUriScheme(url)) {
+        // https://datatracker.ietf.org/doc/html/rfc8252#section-7.1
+        //
+        // > When choosing a URI scheme to associate with the app, apps MUST use
+        // > a URI scheme based on a domain name under their control, expressed
+        // > in reverse order, as recommended by Section 3.8 of [RFC7595] for
+        // > private-use URI schemes.
+
         // https://datatracker.ietf.org/doc/html/rfc8252#section-8.4
         //
         // > In addition to the collision-resistant properties, requiring a
@@ -773,11 +726,14 @@ export class ClientManager {
         // > where two apps claim the same private-use URI scheme (where one
         // > app is acting maliciously).
 
-        // https://www.ietf.org/archive/id/draft-ietf-oauth-client-id-metadata-document-00.html
+        // https://atproto.com/specs/oauth
         //
-        // Fully qualified domain name (FQDN) of the client_id, in reverse
-        // order. This could be relaxed to allow same apex domain names, or
-        // parent domains, but for now we require an exact match.
+        // > Any custom scheme must match the client_id hostname in
+        // > reverse-domain order. The URI scheme must be followed by a single
+        // > colon (:) then a single forward slash (/) and then a URI path
+        // > component. For example, an app with client_id
+        // > https://app.example.com/client-metadata.json could have a
+        // > redirect_uri of com.example.app:/callback.
         const protocol = `${reverseDomain(clientIdUrl.hostname)}:`
         if (url.protocol !== protocol) {
           throw new InvalidRedirectUriError(
diff --git a/packages/oauth/oauth-provider/src/client/client-utils.ts b/packages/oauth/oauth-provider/src/client/client-utils.ts
@@ -1,8 +1,8 @@
 import {
   OAuthClientIdDiscoverable,
+  isLocalHostname,
   parseOAuthDiscoverableClientId,
 } from '@atproto/oauth-types'
-import { isLocalHostname } from '@atproto-labs/fetch-node'
 import { InvalidClientIdError } from '../errors/invalid-client-id-error.js'
 import { InvalidRedirectUriError } from '../errors/invalid-redirect-uri-error.js'
 
diff --git a/packages/oauth/oauth-types/src/atproto-loopback-client-id.ts b/packages/oauth/oauth-types/src/atproto-loopback-client-id.ts
@@ -12,7 +12,7 @@ import {
 } from './oauth-client-id-loopback.js'
 import {
   OAuthLoopbackRedirectURI,
-  oauthLoopbackRedirectURISchema,
+  oauthLoopbackClientRedirectUriSchema,
 } from './oauth-redirect-uri.js'
 import { arrayEquivalent, asArray } from './util.js'
 
@@ -41,7 +41,10 @@ export function buildAtprotoLoopbackClientId(
         throw new TypeError(`Unexpected empty "redirect_uris" config`)
       }
       for (const uri of redirectUris) {
-        params.append('redirect_uri', oauthLoopbackRedirectURISchema.parse(uri))
+        params.append(
+          'redirect_uri',
+          oauthLoopbackClientRedirectUriSchema.parse(uri),
+        )
       }
     }
 
diff --git a/packages/oauth/oauth-types/src/oauth-client-id-loopback.ts b/packages/oauth/oauth-types/src/oauth-client-id-loopback.ts
@@ -1,7 +1,7 @@
 import { oauthClientIdSchema } from './oauth-client-id.js'
 import {
   OAuthLoopbackRedirectURI,
-  oauthLoopbackRedirectURISchema,
+  oauthLoopbackClientRedirectUriSchema,
 } from './oauth-redirect-uri.js'
 import { OAuthScope, oauthScopeSchema } from './oauth-scope.js'
 
@@ -138,7 +138,7 @@ export function safeParseOAuthLoopbackClientIdQueryString(
 
       params.scope = res.data
     } else if (key === 'redirect_uri') {
-      const res = oauthLoopbackRedirectURISchema.safeParse(value)
+      const res = oauthLoopbackClientRedirectUriSchema.safeParse(value)
       if (!res.success) {
         const reason = res.error.issues.map((i) => i.message).join(', ')
         return {
diff --git a/packages/oauth/oauth-types/src/oauth-redirect-uri.ts b/packages/oauth/oauth-types/src/oauth-redirect-uri.ts
@@ -6,19 +6,23 @@ import {
   privateUseUriSchema,
 } from './uri.js'
 
-export const oauthLoopbackRedirectURISchema = loopbackUriSchema.superRefine(
+/**
+ * This is a {@link loopbackUriSchema} with the additional restriction that
+ * the hostname `localhost` is not allowed.
+ *
+ * @see {@link https://datatracker.ietf.org/doc/html/rfc8252#section-8.3 Loopback Redirect Considerations} RFC8252
+ *
+ * > While redirect URIs using localhost (i.e.,
+ * > "http://localhost:{port}/{path}") function similarly to loopback IP
+ * > redirects described in Section 7.3, the use of localhost is NOT
+ * > RECOMMENDED. Specifying a redirect URI with the loopback IP literal rather
+ * > than localhost avoids inadvertently listening on network interfaces other
+ * > than the loopback interface.  It is also less susceptible to client-side
+ * > firewalls and misconfigured host name resolution on the user's device.
+ */
+export const loopbackRedirectURISchema = loopbackUriSchema.superRefine(
   (value, ctx): value is Exclude<LoopbackUri, `http://localhost${string}`> => {
     if (value.startsWith('http://localhost')) {
-      // https://datatracker.ietf.org/doc/html/rfc8252#section-8.3
-      //
-      // > While redirect URIs using localhost (i.e.,
-      // > "http://localhost:{port}/{path}") function similarly to loopback IP
-      // > redirects described in Section 7.3, the use of localhost is NOT
-      // > RECOMMENDED.  Specifying a redirect URI with the loopback IP literal
-      // > rather than localhost avoids inadvertently listening on network
-      // > interfaces other than the loopback interface.  It is also less
-      // > susceptible to client-side firewalls and misconfigured host name
-      // > resolution on the user's device.
       ctx.addIssue({
         code: ZodIssueCode.custom,
         message:
@@ -30,27 +34,17 @@ export const oauthLoopbackRedirectURISchema = loopbackUriSchema.superRefine(
     return true
   },
 )
-export type OAuthLoopbackRedirectURI = TypeOf<
-  typeof oauthLoopbackRedirectURISchema
->
-
-export const oauthHttpsRedirectURISchema = httpsUriSchema
-export type OAuthHttpsRedirectURI = TypeOf<typeof oauthHttpsRedirectURISchema>
+export type LoopbackRedirectURI = TypeOf<typeof loopbackRedirectURISchema>
 
-export const oauthPrivateUseRedirectURISchema = privateUseUriSchema
-export type OAuthPrivateUseRedirectURI = TypeOf<
-  typeof oauthPrivateUseRedirectURISchema
+export const oauthLoopbackClientRedirectUriSchema = loopbackRedirectURISchema
+export type OAuthLoopbackRedirectURI = TypeOf<
+  typeof oauthLoopbackClientRedirectUriSchema
 >
 
 export const oauthRedirectUriSchema = z.union(
-  [
-    oauthLoopbackRedirectURISchema,
-    oauthHttpsRedirectURISchema,
-    oauthPrivateUseRedirectURISchema,
-  ],
+  [loopbackRedirectURISchema, httpsUriSchema, privateUseUriSchema],
   {
     message: `URL must use the "https:" or "http:" protocol, or a private-use URI scheme (RFC 8252)`,
   },
 )
-
 export type OAuthRedirectUri = TypeOf<typeof oauthRedirectUriSchema>
diff --git a/packages/oauth/oauth-types/src/uri.ts b/packages/oauth/oauth-types/src/uri.ts
diff --git a/packages/oauth/oauth-types/src/util.ts b/packages/oauth/oauth-types/src/util.ts

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@atproto/oauth-client-expo": patch
 +---
++
 +The package now re-exports everything from `@atproto/oauth-client`, avoiding the need to explicitly depend on it.
Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ import {`
`12`	`12`	`} from './oauth-client-id-loopback.js'`
`13`	`13`	`import {`
`14`	`14`	`OAuthLoopbackRedirectURI,`
`15`		`- oauthLoopbackRedirectURISchema,`
	`15`	`+ oauthLoopbackClientRedirectUriSchema,`
`16`	`16`	`} from './oauth-redirect-uri.js'`
`17`	`17`	`import { arrayEquivalent, asArray } from './util.js'`
`18`	`18`
`@@ -41,7 +41,10 @@ export function buildAtprotoLoopbackClientId(`
`41`	`41`	throw new TypeError(`Unexpected empty "redirect_uris" config`)
`42`	`42`	`}`
`43`	`43`	`for (const uri of redirectUris) {`
`44`		`- params.append('redirect_uri', oauthLoopbackRedirectURISchema.parse(uri))`
	`44`	`+ params.append(`
	`45`	`+ 'redirect_uri',`
	`46`	`+ oauthLoopbackClientRedirectUriSchema.parse(uri),`
	`47`	`+ )`
`45`	`48`	`}`
`46`	`49`	`}`
`47`	`50`