diff --git a/.changeset/strict-explicit-dcr-redirect.md b/.changeset/strict-explicit-dcr-redirect.md new file mode 100644 index 000000000..efd848b68 --- /dev/null +++ b/.changeset/strict-explicit-dcr-redirect.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Treat an explicit dynamic-client redirect URI as authoritative when selecting a reusable OAuth client. Legacy clients with no recorded redirect now remain available to existing connections while a new client is registered for the explicit callback; callers that rely on Executor's configured default retain the previous compatibility behavior. diff --git a/packages/core/sdk/src/oauth-register-dynamic.test.ts b/packages/core/sdk/src/oauth-register-dynamic.test.ts index bc13050de..d3a66b07a 100644 --- a/packages/core/sdk/src/oauth-register-dynamic.test.ts +++ b/packages/core/sdk/src/oauth-register-dynamic.test.ts @@ -6,6 +6,7 @@ import { ConnectionName, IntegrationSlug, OAuthClientSlug, + ToolAddress, ToolName, } from "./ids"; import { OAuthRegisterDynamicError } from "./oauth-client"; @@ -406,61 +407,153 @@ describe("oauth.registerDynamicClient", () => { ), ); - it.effect("reuses a legacy DCR row once its origin_issuer is backfilled", () => - Effect.scoped( - Effect.gen(function* () { - // The post-backfill counterpart: after the GC migration stamps a legacy - // row's origin_issuer, the reuse lookup keys on it and mints no - // duplicate. This is the steady state the migration establishes. - const server = yield* serveOAuthTestServer({ scopes: ["read"] }); - const { config, executor } = yield* makeTestWorkspaceHarness({ plugins }); - yield* executor.acme.seed(); - const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); - const legacySlug = OAuthClientSlug.make("cloudflare-mcp"); + it.effect( + "reuses a legacy DCR row without an explicit redirect once its issuer is backfilled", + () => + Effect.scoped( + Effect.gen(function* () { + // The post-backfill counterpart: after the GC migration stamps a legacy + // row's origin_issuer, the reuse lookup keys on it and mints no + // duplicate. This is the steady state the migration establishes. + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const { config, executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); + const legacySlug = OAuthClientSlug.make("cloudflare-mcp"); - yield* executor.oauth.createClient({ - owner: "org", - slug: legacySlug, - authorizationUrl: probe.authorizationUrl, - tokenUrl: probe.tokenUrl, - resource: server.mcpResourceUrl, - grant: "authorization_code", - clientId: "legacy-dcr-client", - clientSecret: "", - }); - // Simulate the migration's backfill: legacy DCR stamp + issuer set. - yield* Effect.promise(() => - config.db.updateMany("oauth_client", { - where: (b) => b("slug", "=", String(legacySlug)), - set: { - origin_kind: "dynamic_client_registration", - origin_integration: null, - origin_issuer: probe.issuer, - }, - }), - ); - yield* server.clearRequests; + yield* executor.oauth.createClient({ + owner: "org", + slug: legacySlug, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: server.mcpResourceUrl, + grant: "authorization_code", + clientId: "legacy-dcr-client", + clientSecret: "", + }); + // Simulate the migration's backfill: legacy DCR stamp + issuer set. + yield* Effect.promise(() => + config.db.updateMany("oauth_client", { + where: (b) => b("slug", "=", String(legacySlug)), + set: { + origin_kind: "dynamic_client_registration", + origin_integration: null, + origin_issuer: probe.issuer, + }, + }), + ); + yield* server.clearRequests; - const reused = yield* executor.oauth.registerDynamicClient({ - owner: "org", - slug: OAuthClientSlug.make("new-attempt"), - issuer: probe.issuer, - registrationEndpoint: probe.registrationEndpoint!, - authorizationUrl: probe.authorizationUrl, - tokenUrl: probe.tokenUrl, - resource: server.mcpResourceUrl, - scopes: ["read"], - tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, - clientName: "Acme DCR", - redirectUri: FLOW_REDIRECT_URI, - originIntegration: INTEG, - }); + const reused = yield* executor.oauth.registerDynamicClient({ + owner: "org", + slug: OAuthClientSlug.make("new-attempt"), + issuer: probe.issuer, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: server.mcpResourceUrl, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Acme DCR", + originIntegration: INTEG, + }); - expect(reused).toBe(legacySlug); - const requests = yield* server.requests; - expect(registerRequestCount(requests)).toBe(0); - }), - ), + expect(reused).toBe(legacySlug); + const requests = yield* server.requests; + expect(registerRequestCount(requests)).toBe(0); + }), + ), + ); + + it.effect( + "does not reuse a legacy null-redirect client when the caller supplies an explicit redirect", + () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const { config, executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); + + const legacySlug = yield* executor.oauth.registerDynamicClient({ + owner: "org", + slug: OAuthClientSlug.make("legacy-client"), + issuer: probe.issuer, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Legacy DCR", + redirectUri: FLOW_REDIRECT_URI, + originIntegration: INTEG, + }); + + const started = yield* executor.oauth.start({ + owner: "org", + client: legacySlug, + clientOwner: "org", + name: ConnectionName.make("legacy"), + integration: INTEG, + template: TEMPLATE, + redirectUri: FLOW_REDIRECT_URI, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* executor.oauth.complete({ state: started.state, code: callback.code }); + + // Simulate a client written before origin_redirect_uri was persisted. + yield* Effect.promise(() => + config.db.updateMany("oauth_client", { + where: (b) => b("slug", "=", String(legacySlug)), + set: { origin_redirect_uri: null }, + }), + ); + yield* server.clearRequests; + + const stableRedirectUri = "https://agent.example.test/executor/oauth/callback"; + const replacementSlug = yield* executor.oauth.registerDynamicClient({ + owner: "org", + slug: OAuthClientSlug.make("stable-callback"), + issuer: probe.issuer, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Stable callback DCR", + redirectUri: stableRedirectUri, + originIntegration: INTEG, + }); + + expect(String(replacementSlug)).not.toBe(String(legacySlug)); + expect(registerRequestCount(yield* server.requests)).toBe(1); + const clientSlugs = yield* Effect.map(executor.oauth.listClients(), (clients) => + clients.map((client) => String(client.slug)), + ); + expect(clientSlugs).toContain(String(legacySlug)); + expect(clientSlugs).toContain(String(replacementSlug)); + + // The legacy row may still back live connections. Keeping it allows + // those grants to refresh while new flows use the stable callback. + yield* Effect.promise(() => + config.db.updateMany("connection", { + where: (b) => b("name", "=", "legacy"), + set: { expires_at: Date.now() - 60_000 }, + }), + ); + const refreshed = (yield* executor.execute( + ToolAddress.make("tools.acme.org.legacy.whoami"), + {}, + )) as { token: string }; + expect(refreshed.token).toMatch(/^at_/); + }), + ), ); it.effect("uses resource to distinguish DCR clients only after an issuer already differs", () => diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index ba91260a5..a98eeda03 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -1266,7 +1266,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { readonly slug: OAuthClientSlug; readonly resource: string | null; /** Redirect URI the candidate registered with the AS; null for rows - * predating the column (treated as matching any flow callback). */ + * predating the column. */ readonly redirectUri: string | null; }; @@ -1357,20 +1357,17 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { Effect.gen(function* () { const candidates = yield* dcrCandidatesForIssuer(input.owner, issuer); const resource = input.resource ?? null; - // A candidate is reusable only when the callback it registered with the - // AS still matches the current flow's callback — strict servers reject an - // authorize request whose redirect_uri differs from the registration - // (e.g. the callback origin changed after a sandbox was recreated while - // the persisted client survived). A null stored redirect is a legacy row - // predating the column: treated as matching so an upgrade doesn't - // re-register every client whose callback never changed. A null FLOW - // redirect has nothing to compare against, so it also reuses — the only - // alternative is a fresh registration, which the missing-redirectUri - // guard would fail. - const redirectMatches = (candidate: DcrReuseCandidate): boolean => - candidate.redirectUri === null || - flowRedirectUri === null || - candidate.redirectUri === flowRedirectUri; + // A caller-supplied redirect is authoritative: only a client registered + // with that exact callback can be reused. In particular, a legacy row + // with no recorded redirect is not proof of a match. When the caller + // relies on the executor's configured default, retain the legacy-null + // compatibility behavior so upgrades do not re-register every client. + const hasExplicitRedirectUri = input.redirectUri != null; + const redirectMatches = (candidate: DcrReuseCandidate): boolean => { + if (candidate.redirectUri === flowRedirectUri) return true; + if (hasExplicitRedirectUri) return false; + return candidate.redirectUri === null || flowRedirectUri === null; + }; // A fresh registration must never take a slug an existing candidate // holds: `createClient` deletes any colliding (owner, slug) row first, // which would clobber a client that live connections still refresh @@ -1384,11 +1381,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // resource row is the STRANDED one — but the first drift recovery // already minted a client bound to the CURRENT callback, and later // reconnects must reuse that instead of registering another duplicate - // each time. Known limitation: the legacy null-redirect rule in - // `redirectMatches` (a legacy row with no stored redirect matches any - // flow redirect) still lets such a row win over a later, exactly- - // matching one; kept deliberately so upgrades don't re-register every - // client whose callback never changed. + // each time. const reusable = candidates.find( (client) => client.resource === resource && redirectMatches(client), );