fix(oauth): require exact explicit DCR redirect match

.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.

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", () =>

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),
         );