mirror of
https://github.com/better-auth/better-auth.git
synced 2026-10-20 20:26:55 -05:00
fix(oauth-provider): refuse trusted-client changes with 400, not 500
Updating, deleting, or rotating the secret of a client configured in code through `cachedTrustedClients` is a deliberate refusal, but the three management endpoints reported it as a 500 server failure. They now share one guard with client key rotation and return 400 `invalid_client`.
This commit is contained in:
@@ -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.
|
||||
@@ -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<OAuthClient["jwks"]>;
|
||||
@@ -85,12 +86,7 @@ function assertRotatable(
|
||||
opts: OAuthOptions<Scope[]>,
|
||||
client: SchemaClient<Scope[]>,
|
||||
) {
|
||||
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",
|
||||
|
||||
@@ -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<string>();
|
||||
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",
|
||||
});
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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<Scope[]>,
|
||||
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
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user