From 2b04ffb809b8446c2192aea0a47f52939035b1fa Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Thu, 15 Oct 2026 16:04:53 +0100 Subject: [PATCH] fix(oauth-provider): never hold discovered clients for approval A client ID metadata document identifies client software that every user and organization running it shares. Approving such a client made the first organization to consent its owner, which let that organization's members change its redirect URIs or delete it for everyone. Only open dynamic registration now creates pending clients. A metadata refresh still keeps a scope ceiling or a disable committed while it runs. --- .../oauth-provider-approve-pending-client.md | 15 +++-- docs/content/docs/plugins/cimd.mdx | 2 +- docs/content/docs/plugins/oauth-provider.mdx | 12 +++- packages/cimd/src/flows.test.ts | 64 ++----------------- .../src/client-approval.test.ts | 13 ++-- packages/oauth-provider/src/register.ts | 10 +-- packages/oauth-provider/src/types/index.ts | 13 ++-- 7 files changed, 44 insertions(+), 85 deletions(-) diff --git a/.changeset/oauth-provider-approve-pending-client.md b/.changeset/oauth-provider-approve-pending-client.md index 133c6c4173..52430299bd 100644 --- a/.changeset/oauth-provider-approve-pending-client.md +++ b/.changeset/oauth-provider-approve-pending-client.md @@ -2,10 +2,15 @@ "@better-auth/oauth-provider": minor --- -Apps with open dynamic client registration or CIMD discovery can now hold self-registered clients until a person approves them, by defining `approvePendingClient`. Apps that don't define it see no change. Every app needs to run its migration: this release adds an optional `pending` column to `oauthClient`. +Every app must run its migration: this release adds an optional `pending` column to `oauthClient`. -- A client registered without a session or initial access token, or discovered through CIMD, is stored as pending. It cannot obtain tokens until a user accepts consent and your callback names its owner (`clientReference`). -- If the callback returns `false`, the user is redirected to the client with `access_denied`, the same as a denied consent. -- A pending client must register the `authorization_code` grant. Open registration may also request `client_credentials`, but approval does not enable it: an administrator still assigns its scopes through `adminUpdateOAuthClient`. +Apps with `allowUnauthenticatedClientRegistration` can now give self-registered clients an owner, by defining `approvePendingClient`. Until now, such a client could obtain tokens as soon as any user consented, and it belonged to no user or organization, so nobody could see, update, or delete it through the client endpoints. With the option: + +- A client registered without a session or initial access token waits for approval and cannot obtain tokens. It must register the `authorization_code` grant. +- When a user accepts consent, your callback decides whether they may approve it and returns its owner (`clientReference`), such as the organization they administer. The owner's members can then manage the client. Returning `false` redirects to the client with `access_denied`, as a denied consent does. - Registration responses and the public client endpoints include `pending: true`, so your consent page can tell the user. -- For all apps, a CIMD metadata refresh no longer undoes a scope ceiling or a disable that was set while the refresh was running. +- Approval grants no machine access. Open registration may request `client_credentials`, but an administrator still assigns its scopes through `adminUpdateOAuthClient`. + +Apps that don't define the option see no change. Clients discovered through CIMD are never pending, since one discovered client serves every organization that uses the software. + +For all apps using CIMD, a metadata refresh no longer undoes a scope ceiling or a disable that an administrator set while the refresh was running. diff --git a/docs/content/docs/plugins/cimd.mdx b/docs/content/docs/plugins/cimd.mdx index 3c4476f543..dfe373de3f 100644 --- a/docs/content/docs/plugins/cimd.mdx +++ b/docs/content/docs/plugins/cimd.mdx @@ -202,7 +202,7 @@ Valid documents follow shared-cache semantics. `s-maxage` takes precedence over The metadata fetch governor is independent from HTTP freshness. Fresh-cache hits and concurrent callers joining the same client fetch consume no budget. Every new fetch consumes its concurrency and rolling-window budget when it starts, and excess work is rejected instead of queued. A successful fetch clears its per-client pacing record, so a response that cannot be cached or is immediately stale can be fetched again while origin and global budgets continue to apply. Failed fetches retain the `minimumFetchInterval` backoff. The metadata cache, client pacing records, and origin budget records are each bounded by `maxCacheEntries`. The governor evicts only inactive records; if every record still protects a live interval, rolling window, or fetch, a new client or origin fails closed. -A metadata document proves control of a URL, not of a business or account. When the OAuth Provider defines [`approvePendingClient`](/docs/plugins/oauth-provider#approving-self-registered-clients-at-consent), a newly discovered client is stored as pending and cannot obtain tokens until a user consents and your callback sets its owner. Its document must then list the `authorization_code` grant. Refresh never changes a client's pending state, owner, or scope ceiling, including an approval that commits while a refresh is in progress. +A metadata document proves control of a URL, not of a business or account, and one document serves every user and organization that runs the software. Discovered clients therefore have no owner and are never held by the OAuth Provider's [`approvePendingClient`](/docs/plugins/oauth-provider#approving-self-registered-clients-at-consent). Refresh never changes a client's `disabled` state or scope ceiling, including a change an administrator commits while a refresh is in progress. Refresh remains fail closed. A network, validation, or persistence failure preserves the previous database/cache state but rejects the current OAuth request. `onClientRefreshed` receives `previousClient`; operators can compare security-sensitive metadata and choose whether to revoke grants, tokens, or consent. diff --git a/docs/content/docs/plugins/oauth-provider.mdx b/docs/content/docs/plugins/oauth-provider.mdx index ff07258907..b846a26ced 100644 --- a/docs/content/docs/plugins/oauth-provider.mdx +++ b/docs/content/docs/plugins/oauth-provider.mdx @@ -1827,12 +1827,16 @@ Only disable PKCE for confidential clients when absolutely necessary for legacy #### Approving Self-Registered Clients at Consent -An open registration, or a client discovered through the [CIMD plugin](/docs/plugins/cimd), proves nothing about who controls the client. Define `approvePendingClient` to keep such clients pending until a signed-in user consents, and to decide then who owns the client. +With `allowUnauthenticatedClientRegistration`, a client such as a desktop or command-line AI assistant registers itself when someone installs it. Nobody vouches for that registration. The client can obtain tokens as soon as any user consents, and it belongs to no user or organization: it does not appear in anyone's [client list](#oauth-clients), nobody can update or delete it through the client endpoints, and [token claims](#client-specific-access-token-claims) cannot use its owner. + +Define `approvePendingClient` to hold these clients until a signed-in user consents, and to give each client an owner at that moment. For example, a member of an organization connects an assistant to your API. The assistant registers, the member signs in and accepts consent, and your callback checks that they administer the organization. The client then belongs to the organization: its members see and manage it, and its tokens can carry the organization. ```ts title="auth.ts" oauthProvider({ allowDynamicClientRegistration: true, allowUnauthenticatedClientRegistration: true, + clientReference: ({ session }) => + (session?.activeOrganizationId as string | undefined) ?? undefined, approvePendingClient: async ({ user, consentReferenceId }) => { // [!code highlight] // The organization the user picked on your post-login page // [!code highlight] if (!consentReferenceId) return false // [!code highlight] @@ -1842,9 +1846,11 @@ oauthProvider({ }) ``` +Approval fits a client that one organization uses, such as a client each installation registers for itself. An owner can change a client's redirect URIs or delete it, so if one registration serves users from several organizations, such as a hosted web app, the first organization to approve it would control it for everyone. Register those clients yourself with [`adminCreateOAuthClient`](#create-client) instead. For the same reason, clients discovered through the [CIMD plugin](/docs/plugins/cimd) are never pending: one metadata document identifies the software for every user and organization that runs it. + With the callback defined: -* A client created by unauthenticated registration, or discovered through CIMD, is stored as pending. Registrations made by a signed-in user or with an initial access token are not pending. Clients created before you define the callback are unaffected. +* A client created by unauthenticated registration is stored as pending. Registrations made by a signed-in user or with an initial access token are not pending. Clients created before you define the callback are unaffected. * A pending client must register the `authorization_code` grant, because consent is the only way to approve it. Registration without it fails with `invalid_client_metadata`. * Unauthenticated registration may also request the `client_credentials` grant. Approval does not enable it: the client cannot use it until an administrator assigns its scopes through [`adminUpdateOAuthClient`](#update-client), as for any other client. * A pending client cannot authenticate at the token, introspection, revocation, or device authorization endpoints. The authorization endpoint always sends its users to the consent page, even when `skipConsent` is set, and answers `prompt=none` with `consent_required`. @@ -2136,7 +2142,7 @@ oauthTokenExchange({ ### Organizations -OAuth Clients are tied to either a user or `reference_id` at registration and is immutable. If you are utilizing the [organization plugin](/docs/plugins/organization), you must ensure that the [`activeOrganizationId`](/docs/plugins/organization#active-organization) is set on your active session when you create new clients. +OAuth Clients are tied to either a user or `reference_id` at registration, or at approval for [pending clients](#approving-self-registered-clients-at-consent), and the owner is immutable. If you are utilizing the [organization plugin](/docs/plugins/organization), you must ensure that the [`activeOrganizationId`](/docs/plugins/organization#active-organization) is set on your active session when you create new clients. ```ts title="auth.ts" oauthProvider({ diff --git a/packages/cimd/src/flows.test.ts b/packages/cimd/src/flows.test.ts index d458dc85c0..f04f247ac4 100644 --- a/packages/cimd/src/flows.test.ts +++ b/packages/cimd/src/flows.test.ts @@ -1379,8 +1379,6 @@ describe("CIMD - pending client approval", async () => { clientReference: "business_1", })); - // metadataRevalidationInterval: 0 makes every request stale, so the second - // authorize hit refreshes the stored record. const { auth: authorizationServer, signInWithTestUser, @@ -1418,7 +1416,9 @@ describe("CIMD - pending client approval", async () => { vi.unstubAllGlobals(); }); - it("keeps a discovered client pending until consent, then keeps its owner across refreshes", async () => { + // One discovered client serves every user and organization of that + // software, so the first organization to consent must not become its owner. + it("never holds a discovered client for approval or gives it an owner", async () => { stubMetadataFetch(clientMetadataUrl, metadataDocument); const { headers } = await signInWithTestUser(); const authedClient = createAuthClient({ @@ -1438,61 +1438,11 @@ describe("CIMD - pending client approval", async () => { buildAuthorizeUrl(authServerBaseUrl, clientMetadataUrl, redirectUri), ); expect(code).toEqual(expect.any(String)); - expect(approvePendingClient).toHaveBeenCalledTimes(1); + expect(approvePendingClient).not.toHaveBeenCalled(); - const approved = await readClient(); - expect(approved).toMatchObject({ - pending: false, - referenceId: "business_1", - }); - expect(approved?.userId ?? null).toBeNull(); - - // A stale refresh rewrites the metadata but never the approval state. - await authedClient.$fetch( - buildAuthorizeUrl(authServerBaseUrl, clientMetadataUrl, redirectUri), - { method: "GET", onError() {} }, - ); - const refreshed = await readClient(); - expect(refreshed).toMatchObject({ - pending: false, - referenceId: "business_1", - }); - expect(refreshed?.userId ?? null).toBeNull(); - expect(approvePendingClient).toHaveBeenCalledTimes(1); - }); - - it("stores a newly discovered client as pending without a code", async () => { - const pendingUrl = - "https://mcp-client-pending-2.example.com/client-metadata.json"; - stubMetadataFetch(pendingUrl, { - ...metadataDocument, - client_id: pendingUrl, - }); - const { headers } = await signInWithTestUser(); - const authedClient = createAuthClient({ - plugins: [oauthProviderClient()], - baseURL: authServerBaseUrl, - fetchOptions: { customFetchImpl, headers }, - }); - let redirect = ""; - await authedClient.$fetch( - buildAuthorizeUrl(authServerBaseUrl, pendingUrl, redirectUri), - { - method: "GET", - onError(ctx) { - redirect = ctx.response.headers.get("Location") || ""; - }, - }, - ); - expect(redirect).toContain("/consent"); - expect(tryExtractCodeFromUrl(redirect)).toBeNull(); - const ctx = await authorizationServer.$context; - const stored = await ctx.adapter.findOne>({ - model: "oauthClient", - where: [{ field: "clientId", value: pendingUrl }], - }); - expect(stored?.pending).toBe(true); - expect(stored?.userId ?? null).toBeNull(); + const stored = await readClient(); + expect(stored?.pending ?? false).toBe(false); expect(stored?.referenceId ?? null).toBeNull(); + expect(stored?.userId ?? null).toBeNull(); }); }); diff --git a/packages/oauth-provider/src/client-approval.test.ts b/packages/oauth-provider/src/client-approval.test.ts index b8efdd70a8..4a1f492147 100644 --- a/packages/oauth-provider/src/client-approval.test.ts +++ b/packages/oauth-provider/src/client-approval.test.ts @@ -590,7 +590,7 @@ describe.each(adapters)("approvePendingClient on %s", (testWith) => { expect(((await response.json()) as OAuthClient).pending).toBe(true); }); - it("keeps an approval, a scope limit, and a disable that commit during a discovery refresh", async () => { + it("never holds a discovered client and keeps a scope limit and a disable set during its refresh", async () => { const t = await createInstance(testWith, { approvePendingClient: () => false, }); @@ -619,10 +619,12 @@ describe.each(adapters)("approvePendingClient on %s", (testWith) => { }); await discover(); const snapshot = (await t.row(clientId)) as SchemaClient; - expect(snapshot.pending).toBe(true); + // One discovered client serves every user of the software, so it never + // waits for an owner. + expect(snapshot.pending ?? false).toBe(false); + expect(snapshot.referenceId ?? null).toBeNull(); - // The approval commits while the refresh is fetching metadata. - await storeClientApproval(t.context.adapter, opts, clientId, "business_1"); + // An administrator writes while the refresh is fetching metadata. await t.context.adapter.update({ model: "oauthClient", where: [{ field: "clientId", value: clientId }], @@ -631,8 +633,7 @@ describe.each(adapters)("approvePendingClient on %s", (testWith) => { await discover(snapshot); const stored = await t.row(clientId); - expect(stored?.pending).toBe(false); - expect(stored?.referenceId).toBe("business_1"); + expect(stored?.pending ?? false).toBe(false); expect(stored?.clientCredentialsScopes).toEqual(["invoices:write"]); expect(stored?.disabled).toBe(true); }); diff --git a/packages/oauth-provider/src/register.ts b/packages/oauth-provider/src/register.ts index a323dc578e..5b71423a87 100644 --- a/packages/oauth-provider/src/register.ts +++ b/packages/oauth-provider/src/register.ts @@ -1059,14 +1059,10 @@ async function persistOAuthClientRegistration( clientId, body.redirect_uris, ); - // Only open registration and a first discovery create a pending client. - // Refreshing a discovered client never changes its approval state. + // Only open registration creates a pending client. A discovered client is + // shared by every user of the software, so no one organization owns it. const startsPending = - input.registrationSource === "dynamic" - ? Boolean(input.pending) - : input.registrationSource === "clientMetadataDocument" && - !input.existingClient && - Boolean(opts.approvePendingClient); + input.registrationSource === "dynamic" && Boolean(input.pending); // Approval happens at consent, which only the authorization code flow // reaches. Without it the client could never leave the pending state. if ( diff --git a/packages/oauth-provider/src/types/index.ts b/packages/oauth-provider/src/types/index.ts index 2f9eb593ff..640a1221b8 100644 --- a/packages/oauth-provider/src/types/index.ts +++ b/packages/oauth-provider/src/types/index.ts @@ -1187,12 +1187,13 @@ export interface OAuthOptions< * application decide who owns them. * * When defined, a client created by unauthenticated dynamic registration - * (RFC 7591) or discovered through Client ID Metadata Documents is stored as - * pending. Such a client must register the `authorization_code` grant, since - * consent is the only way to approve it. A pending client cannot - * authenticate at any token-related endpoint, and the authorization endpoint - * always sends its users to the consent page, even when they consented - * before. Open registration may also request the `client_credentials` grant, + * (RFC 7591) is stored as pending. Clients discovered through Client ID + * Metadata Documents are never pending: one discovered client serves every + * user of that software, so no single owner fits it. A pending client must + * register the `authorization_code` grant, since consent is the only way to + * approve it. A pending client cannot authenticate at any token-related + * endpoint, and the authorization endpoint always sends its users to the + * consent page, even when they consented before. Open registration may also request the `client_credentials` grant, * which stays unusable until an administrator assigns its scopes through * `adminUpdateOAuthClient`. *