diff --git a/.changeset/oauth-client-auth-method.md b/.changeset/oauth-client-auth-method.md new file mode 100644 index 000000000..e68d769f9 --- /dev/null +++ b/.changeset/oauth-client-auth-method.md @@ -0,0 +1,5 @@ +--- +"executor": minor +--- + +**OAuth clients can now persist `clientAuth: "basic"` to use `client_secret_basic` for authorization-code exchange, refresh, and client-credentials token requests; existing clients continue to use `client_secret_post`** diff --git a/packages/core/sdk/src/core-schema.ts b/packages/core/sdk/src/core-schema.ts index b03adf5df..101c04ba7 100644 --- a/packages/core/sdk/src/core-schema.ts +++ b/packages/core/sdk/src/core-schema.ts @@ -256,6 +256,9 @@ export const coreTables = defineTables({ token_url: textColumn("token_url"), grant: textColumn("grant"), client_id: textColumn("client_id"), + // Token-endpoint authentication for confidential clients. Null in old + // databases means the historical default, client_secret_post ("body"). + client_auth: nullableTextColumn("client_auth"), // The client secret is NOT stored inline — it's a provider `item_id` that // resolves to the value via the default writable credential provider // (WorkOS Vault on cloud, the local store on desktop). Null for public / diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index fb831260b..875fc6803 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -161,9 +161,11 @@ import { type Tool, type ToolAnnotations, type ToolDef, type ToolListFilter } fr import { buildToolTypeScriptPreview } from "./schema-types"; import { collectReferencedDefinitions } from "./schema-refs"; import { + DEFAULT_CLIENT_AUTH_METHOD, refreshAccessToken, exchangeClientCredentials, shouldRefreshToken, + type ClientAuthMethod, type OAuthEndpointUrlPolicy, } from "./oauth-helpers"; import { connectionIdentifier } from "./connection-name-identifier"; @@ -1839,6 +1841,15 @@ export const createExecutor = { grant: "client_credentials", clientId: "test-client", clientSecret: "test-secret", + clientAuth: "basic", }); const started = yield* executor.oauth.start({ @@ -626,6 +627,13 @@ describe("oauth.start / oauth.complete", () => { template: TEMPLATE, }); expect(started.status).toBe("connected"); + const tokenRequest = (yield* server.requests).find( + (request) => request.path === "/token" && request.body.includes("client_credentials"), + ); + expect(tokenRequest?.headers.authorization).toBe( + `Basic ${Buffer.from("test%2Dclient:test%2Dsecret").toString("base64")}`, + ); + expect(tokenRequest?.body).not.toContain("client_secret="); }), ), ); @@ -765,6 +773,7 @@ describe("oauth token refresh in resolveConnectionValue", () => { grant: "authorization_code", clientId: "test-client", clientSecret: "test-secret", + clientAuth: "basic", resource: server.mcpResourceUrl, }); @@ -811,9 +820,17 @@ describe("oauth token refresh in resolveConnectionValue", () => { expect(refreshedToken.token).not.toBe(firstToken.token); expect(yield* server.acceptsAccessToken(refreshedToken.token)).toBe(true); const requests = yield* server.requests; + const expectedAuthorization = `Basic ${Buffer.from("test%2Dclient:test%2Dsecret").toString("base64")}`; + const exchangeRequest = requests.find( + (r) => r.path === "/token" && r.body.includes("grant_type=authorization_code"), + ); const refreshRequest = requests.find( (r) => r.path === "/token" && r.method === "POST" && r.body.includes("refresh_token"), ); + expect(exchangeRequest?.headers.authorization).toBe(expectedAuthorization); + expect(exchangeRequest?.body).not.toContain("client_secret="); + expect(refreshRequest?.headers.authorization).toBe(expectedAuthorization); + expect(refreshRequest?.body).not.toContain("client_secret="); expect(refreshRequest?.body).toContain( `resource=${encodeURIComponent(server.mcpResourceUrl)}`, ); diff --git a/packages/core/sdk/src/oauth-list-clients.test.ts b/packages/core/sdk/src/oauth-list-clients.test.ts index 086513ce3..5e7e090b5 100644 --- a/packages/core/sdk/src/oauth-list-clients.test.ts +++ b/packages/core/sdk/src/oauth-list-clients.test.ts @@ -50,7 +50,7 @@ describe("oauth.listClients", () => { it.effect("returns owner-visible clients as summaries without the secret", () => Effect.scoped( Effect.gen(function* () { - const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + const { config, executor } = yield* makeTestWorkspaceHarness({ plugins }); yield* executor.oauth.createClient({ owner: "org", @@ -60,6 +60,7 @@ describe("oauth.listClients", () => { grant: "authorization_code", clientId: "org-client-id", clientSecret: "org-super-secret", + clientAuth: "basic", }); yield* executor.oauth.createClient({ owner: "user", @@ -70,6 +71,14 @@ describe("oauth.listClients", () => { clientId: "user-client-id", clientSecret: "user-super-secret", }); + // Rows written before client_auth existed read as the historical body + // default rather than becoming unusable after an upgrade. + yield* Effect.promise(() => + config.db.updateMany("oauth_client", { + where: (b) => b("slug", "=", String(USER_CLIENT)), + set: { client_auth: null }, + }), + ); const clients = yield* executor.oauth.listClients(); @@ -91,6 +100,7 @@ describe("oauth.listClients", () => { tokenUrl: "https://acme.test/token", resource: null, clientId: "org-client-id", + clientAuth: "basic", // Manual apps carry a nullable recorded-intent integration; a client // created outside any integration dialog stamps null. origin: { kind: "manual", integration: null }, @@ -98,6 +108,7 @@ describe("oauth.listClients", () => { expect(user!.owner).toBe("user"); expect(user!.grant).toBe("client_credentials"); expect(user!.clientId).toBe("user-client-id"); + expect(user!.clientAuth).toBe("body"); // The secret is NEVER projected onto a summary. for (const client of clients) { diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 4e4f32aa6..6680ff4c2 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -67,6 +67,8 @@ import { exchangeClientCredentials, isLoopbackHttpUrl, rebindTokenEndpointHostToCallbackDomain, + DEFAULT_CLIENT_AUTH_METHOD, + type ClientAuthMethod, type OAuth2TokenResponse, type OAuthEndpointUrlPolicy, } from "./oauth-helpers"; @@ -300,6 +302,9 @@ const clientOwnerFromPayload = (payload: unknown): Owner | null => { const parseGrant = (grant: unknown): OAuthGrant | null => grant === "client_credentials" || grant === "authorization_code" ? grant : null; +const parseClientAuth = (clientAuth: unknown): ClientAuthMethod | null => + clientAuth == null || clientAuth === "body" ? "body" : clientAuth === "basic" ? "basic" : null; + const canonicalDcrIssuer = ( issuer: string | null | undefined, registrationEndpoint: string, @@ -398,6 +403,7 @@ interface LoadedOAuthClient { readonly tokenUrl: string; readonly grant: OAuthGrant; readonly clientId: string; + readonly clientAuth: ClientAuthMethod; /** Resolved literal secret (read from the provider via the stored item id). */ readonly clientSecret: string; readonly resource: string | null; @@ -642,6 +648,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { token_url: input.tokenUrl, grant: input.grant, client_id: input.clientId, + client_auth: input.clientAuth ?? DEFAULT_CLIENT_AUTH_METHOD, client_secret_item_id: clientSecretItemIdValue, resource: input.resource ?? null, origin_kind: input.origin?.kind ?? "manual", @@ -959,6 +966,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { Effect.flatMap((rows) => Effect.forEach(rows, (row) => { const grant = parseGrant(row.grant); + const clientAuth = parseClientAuth(row.client_auth); // EXPLICIT — a row with an unknown grant is corrupt; surface it // loudly rather than silently displaying it as authorization_code. if (grant === null) { @@ -969,6 +977,14 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }), ); } + if (clientAuth === null) { + return Effect.fail( + new StorageError({ + message: `oauth_client ${String(row.slug)} has an unknown client auth method: ${String(row.client_auth)}`, + cause: undefined, + }), + ); + } return Effect.succeed({ owner: String(row.owner) as Owner, slug: OAuthClientSlug.make(String(row.slug)), @@ -977,6 +993,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { tokenUrl: String(row.token_url), resource: row.resource == null ? null : String(row.resource), clientId: String(row.client_id), + clientAuth, origin: parseOAuthClientOrigin(row), } satisfies OAuthClientSummary); }), @@ -1000,6 +1017,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { Effect.flatMap((row) => { if (!row) return Effect.succeed(null); const grant = parseGrant(row.grant); + const clientAuth = parseClientAuth(row.client_auth); // EXPLICIT — this row drives the token exchange. An unknown grant is a // corrupt row; fail loudly rather than guessing authorization_code and // running the wrong flow. @@ -1011,6 +1029,14 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }), ); } + if (clientAuth === null) { + return Effect.fail( + new StorageError({ + message: `oauth_client ${String(slug)} has an unknown client auth method: ${String(row.client_auth)}`, + cause: undefined, + }), + ); + } // `client_secret_item_id` is null for DCR-minted / public PKCE clients; // the token exchange treats a missing secret as "public client, omit // client_secret" (see pickClientAuth). A confidential client persisted @@ -1031,6 +1057,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { tokenUrl: String(row.token_url), grant, clientId: String(row.client_id), + clientAuth, clientSecret, resource: row.resource == null ? null : String(row.resource), } satisfies LoadedOAuthClient; @@ -1131,6 +1158,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { tokenUrl: client.tokenUrl, clientId: client.clientId, clientSecret: client.clientSecret, + clientAuth: client.clientAuth, scopes: requestedScopes, resource: client.resource ?? undefined, endpointUrlPolicy: deps.endpointUrlPolicy, @@ -1332,6 +1360,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { tokenUrl, clientId: client.clientId, clientSecret: client.clientSecret, + clientAuth: client.clientAuth, redirectUrl: session.redirectUrl, codeVerifier: session.pkceVerifier, code: input.code, diff --git a/packages/core/sdk/src/testing/oauth-test-server.ts b/packages/core/sdk/src/testing/oauth-test-server.ts index 95ef31ec0..97b385a77 100644 --- a/packages/core/sdk/src/testing/oauth-test-server.ts +++ b/packages/core/sdk/src/testing/oauth-test-server.ts @@ -183,8 +183,10 @@ const decodeBasicAuthorization = ( const separator = decoded.indexOf(":"); if (separator < 0) return null; return { - username: decoded.slice(0, separator), - password: decoded.slice(separator + 1), + // RFC 6749 §2.3.1 applies application/x-www-form-urlencoded encoding to + // each credential before constructing the Basic value. + username: new URLSearchParams(`value=${decoded.slice(0, separator)}`).get("value") ?? "", + password: new URLSearchParams(`value=${decoded.slice(separator + 1)}`).get("value") ?? "", }; };