diff --git a/.changeset/oauth-provider-trusted-client-status.md b/.changeset/oauth-provider-trusted-client-status.md new file mode 100644 index 0000000000..1eee35640d --- /dev/null +++ b/.changeset/oauth-provider-trusted-client-status.md @@ -0,0 +1,5 @@ +--- +"@better-auth/oauth-provider": patch +--- + +If you configure `cachedTrustedClients`, attempts to update, delete, or rotate the secret of one of those clients through the client management endpoints now fail with HTTP 400 instead of 500. The response body is unchanged (`error: "invalid_client"`), so only code that branches on the status code is affected. A 500 previously made a deliberate refusal look like a server failure. diff --git a/packages/oauth-provider/src/client-key-rotation.ts b/packages/oauth-provider/src/client-key-rotation.ts index f3328159c8..4506dacfe6 100644 --- a/packages/oauth-provider/src/client-key-rotation.ts +++ b/packages/oauth-provider/src/client-key-rotation.ts @@ -2,6 +2,7 @@ import type { GenericEndpointContext } from "@better-auth/core"; import { APIError, NO_STORE_HEADERS } from "better-auth/api"; import { assertValidClientJwks } from "./register"; import type { OAuthClient, OAuthOptions, SchemaClient, Scope } from "./types"; +import { assertNotTrustedClient } from "./utils"; import { verifyClientAssertion } from "./utils/client-assertion"; type ClientJwks = NonNullable; @@ -85,12 +86,7 @@ function assertRotatable( opts: OAuthOptions, client: SchemaClient, ) { - if (opts.cachedTrustedClients?.has(client.clientId)) { - throw new APIError("BAD_REQUEST", { - error: "invalid_client", - error_description: "trusted clients must be updated manually", - }); - } + assertNotTrustedClient(opts, client.clientId); if (client.clientDiscoveryId || client.jwksUri || !client.jwks) { throw new APIError("BAD_REQUEST", { error: "invalid_client", diff --git a/packages/oauth-provider/src/oauthClient/endpoints.test.ts b/packages/oauth-provider/src/oauthClient/endpoints.test.ts index 729cd7b60d..bb93d35214 100644 --- a/packages/oauth-provider/src/oauthClient/endpoints.test.ts +++ b/packages/oauth-provider/src/oauthClient/endpoints.test.ts @@ -541,3 +541,51 @@ describe("oauthClient private_key_jwt clients", async () => { expect(result.error?.status).toBeLessThan(500); }); }); + +describe("oauthClient - trusted clients", async () => { + const baseURL = "http://localhost:3000"; + const trustedClients = new Set(); + const { auth, signInWithTestUser, customFetchImpl } = await getTestInstance({ + baseURL, + plugins: [ + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + cachedTrustedClients: trustedClients, + }), + jwt(), + ], + }); + const { headers } = await signInWithTestUser(); + + it("refuses to change a trusted client as a client error, not a server failure", async () => { + const created = await auth.api.createOAuthClient({ + headers, + body: { redirect_uris: ["https://rp.example.com/callback"] }, + }); + const clientId = created.client_id; + trustedClients.add(clientId); + + for (const [path, body] of [ + [ + "/oauth2/update-client", + { client_id: clientId, update: { client_name: "x" } }, + ], + ["/oauth2/client/rotate-secret", { client_id: clientId }], + ["/oauth2/delete-client", { client_id: clientId }], + ] as const) { + const response = await customFetchImpl(`${baseURL}/api/auth${path}`, { + method: "POST", + headers: { + ...Object.fromEntries(headers), + "content-type": "application/json", + }, + body: JSON.stringify(body), + }); + expect(response.status, path).toBe(400); + expect(await response.json(), path).toMatchObject({ + error: "invalid_client", + }); + } + }); +}); diff --git a/packages/oauth-provider/src/oauthClient/endpoints.ts b/packages/oauth-provider/src/oauthClient/endpoints.ts index 505b97cf82..2f86e999d4 100644 --- a/packages/oauth-provider/src/oauthClient/endpoints.ts +++ b/packages/oauth-provider/src/oauthClient/endpoints.ts @@ -11,7 +11,7 @@ import type { Scope, } from "../types"; import type { GrantType } from "../types/oauth"; -import { getClient, storeClientSecret } from "../utils"; +import { assertNotTrustedClient, getClient, storeClientSecret } from "../utils"; import { normalizeClientCredentialsScopes, validateClientCredentialsScopes, @@ -153,13 +153,7 @@ export async function deleteClientEndpoint( if (!session) throw new APIError("UNAUTHORIZED"); const clientId = ctx.body.client_id; - const trustedClient = opts.cachedTrustedClients?.has(clientId); - if (trustedClient) { - throw new APIError("INTERNAL_SERVER_ERROR", { - error_description: "trusted clients must be updated manually", - error: "invalid_client", - }); - } + assertNotTrustedClient(opts, clientId); const client = await getClient(ctx, opts, clientId); if (!client) { @@ -232,13 +226,7 @@ export async function updateClientEndpoint( if (!session) throw new APIError("UNAUTHORIZED"); const clientId = ctx.body.client_id; - const trustedClient = opts.cachedTrustedClients?.has(clientId); - if (trustedClient) { - throw new APIError("INTERNAL_SERVER_ERROR", { - error_description: "trusted clients must be updated manually", - error: "invalid_client", - }); - } + assertNotTrustedClient(opts, clientId); const client = await getClient(ctx, opts, clientId); if (!client) { @@ -410,13 +398,7 @@ export async function rotateClientSecretEndpoint( if (!session) throw new APIError("UNAUTHORIZED"); const clientId = ctx.body.client_id; - const trustedClient = opts.cachedTrustedClients?.has(clientId); - if (trustedClient) { - throw new APIError("INTERNAL_SERVER_ERROR", { - error_description: "trusted clients must be updated manually", - error: "invalid_client", - }); - } + assertNotTrustedClient(opts, clientId); const client = await getClient(ctx, opts, clientId); if (!client) { diff --git a/packages/oauth-provider/src/utils/index.ts b/packages/oauth-provider/src/utils/index.ts index c2f1390cee..09e4271c38 100644 --- a/packages/oauth-provider/src/utils/index.ts +++ b/packages/oauth-provider/src/utils/index.ts @@ -247,6 +247,22 @@ export async function verifyOAuthQueryParams( ); } +/** + * Rejects changes to a client configured in code through `cachedTrustedClients`; + * those clients are updated in code, not through client management endpoints. + */ +export function assertNotTrustedClient( + opts: OAuthOptions, + clientId: string, +) { + if (opts.cachedTrustedClients?.has(clientId)) { + throw new APIError("BAD_REQUEST", { + error: "invalid_client", + error_description: "trusted clients must be updated manually", + }); + } +} + /** * Get a client by ID, checking trusted clients first, then database */