History

fix(oauth): require exact explicit DCR redirect match (#2032)

Treat a caller-supplied DCR redirectUri as authoritative: only a client registered with that exact callback is reused. A legacy client with no recorded redirect is kept for existing connections while a new uniquely named client is registered for the explicit callback. Callers that rely on the configured default redirect keep the previous reuse behavior.

Swedish Chef committed 1 month ago

Commit 38a77251 parent 67db4f63 files · +163 −72Browse files

Unified · Split

3 files
  1. .changeset/strict-explicit-dcr-redirect.md +5 −0
  2. packages/core/sdk/src/oauth-register-dynamic.test.ts +145 −52
  3. packages/core/sdk/src/oauth-service.ts +13 −20
.changeset/strict-explicit-dcr-redirect.md+5−0
@@ -0,0 +1,5 @@
1+---
2+"@executor-js/sdk": patch
3+---
4+
5+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+145−52
@@ -6,6 +6,7 @@ import { · 5 unmodified lines
66 ConnectionName,
77 IntegrationSlug,
88 OAuthClientSlug,
9+ ToolAddress,
910 ToolName,
1011 } from "./ids";
1112 import { OAuthRegisterDynamicError } from "./oauth-client";
@@ -406,61 +407,153 @@ describe("oauth.registerDynamicClient", () => { · 394 unmodified lines
406407 ),
407408 );
408409
409− it.effect("reuses a legacy DCR row once its origin_issuer is backfilled", () =>
410− Effect.scoped(
411− Effect.gen(function* () {
412− // The post-backfill counterpart: after the GC migration stamps a legacy
413− // row's origin_issuer, the reuse lookup keys on it and mints no
414− // duplicate. This is the steady state the migration establishes.
415− const server = yield* serveOAuthTestServer({ scopes: ["read"] });
416− const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });
417− yield* executor.acme.seed();
418− const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
419− const legacySlug = OAuthClientSlug.make("cloudflare-mcp");
410+ it.effect(
411+ "reuses a legacy DCR row without an explicit redirect once its issuer is backfilled",
412+ () =>
413+ Effect.scoped(
414+ Effect.gen(function* () {
415+ // The post-backfill counterpart: after the GC migration stamps a legacy
416+ // row's origin_issuer, the reuse lookup keys on it and mints no
417+ // duplicate. This is the steady state the migration establishes.
418+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
419+ const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });
420+ yield* executor.acme.seed();
421+ const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
422+ const legacySlug = OAuthClientSlug.make("cloudflare-mcp");
420423
421− yield* executor.oauth.createClient({
422− owner: "org",
423− slug: legacySlug,
424− authorizationUrl: probe.authorizationUrl,
425− tokenUrl: probe.tokenUrl,
426− resource: server.mcpResourceUrl,
427− grant: "authorization_code",
428− clientId: "legacy-dcr-client",
429− clientSecret: "",
430− });
431− // Simulate the migration's backfill: legacy DCR stamp + issuer set.
432− yield* Effect.promise(() =>
433− config.db.updateMany("oauth_client", {
434− where: (b) => b("slug", "=", String(legacySlug)),
435− set: {
436− origin_kind: "dynamic_client_registration",
437− origin_integration: null,
438− origin_issuer: probe.issuer,
439− },
440− }),
441− );
442− yield* server.clearRequests;
424+ yield* executor.oauth.createClient({
425+ owner: "org",
426+ slug: legacySlug,
427+ authorizationUrl: probe.authorizationUrl,
428+ tokenUrl: probe.tokenUrl,
429+ resource: server.mcpResourceUrl,
430+ grant: "authorization_code",
431+ clientId: "legacy-dcr-client",
432+ clientSecret: "",
433+ });
434+ // Simulate the migration's backfill: legacy DCR stamp + issuer set.
435+ yield* Effect.promise(() =>
436+ config.db.updateMany("oauth_client", {
437+ where: (b) => b("slug", "=", String(legacySlug)),
438+ set: {
439+ origin_kind: "dynamic_client_registration",
440+ origin_integration: null,
441+ origin_issuer: probe.issuer,
442+ },
443+ }),
444+ );
445+ yield* server.clearRequests;
443446
444− const reused = yield* executor.oauth.registerDynamicClient({
445− owner: "org",
446− slug: OAuthClientSlug.make("new-attempt"),
447− issuer: probe.issuer,
448− registrationEndpoint: probe.registrationEndpoint!,
449− authorizationUrl: probe.authorizationUrl,
450− tokenUrl: probe.tokenUrl,
451− resource: server.mcpResourceUrl,
452− scopes: ["read"],
453− tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
454− clientName: "Acme DCR",
455− redirectUri: FLOW_REDIRECT_URI,
456− originIntegration: INTEG,
457− });
447+ const reused = yield* executor.oauth.registerDynamicClient({
448+ owner: "org",
449+ slug: OAuthClientSlug.make("new-attempt"),
450+ issuer: probe.issuer,
451+ registrationEndpoint: probe.registrationEndpoint!,
452+ authorizationUrl: probe.authorizationUrl,
453+ tokenUrl: probe.tokenUrl,
454+ resource: server.mcpResourceUrl,
455+ scopes: ["read"],
456+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
457+ clientName: "Acme DCR",
458+ originIntegration: INTEG,
459+ });
458460
459− expect(reused).toBe(legacySlug);
460− const requests = yield* server.requests;
461− expect(registerRequestCount(requests)).toBe(0);
462− }),
463− ),
461+ expect(reused).toBe(legacySlug);
462+ const requests = yield* server.requests;
463+ expect(registerRequestCount(requests)).toBe(0);
464+ }),
465+ ),
466+ );
467+
468+ it.effect(
469+ "does not reuse a legacy null-redirect client when the caller supplies an explicit redirect",
470+ () =>
471+ Effect.scoped(
472+ Effect.gen(function* () {
473+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
474+ const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });
475+ yield* executor.acme.seed();
476+ const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
477+
478+ const legacySlug = yield* executor.oauth.registerDynamicClient({
479+ owner: "org",
480+ slug: OAuthClientSlug.make("legacy-client"),
481+ issuer: probe.issuer,
482+ registrationEndpoint: probe.registrationEndpoint!,
483+ authorizationUrl: probe.authorizationUrl,
484+ tokenUrl: probe.tokenUrl,
485+ resource: probe.resource,
486+ scopes: ["read"],
487+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
488+ clientName: "Legacy DCR",
489+ redirectUri: FLOW_REDIRECT_URI,
490+ originIntegration: INTEG,
491+ });
492+
493+ const started = yield* executor.oauth.start({
494+ owner: "org",
495+ client: legacySlug,
496+ clientOwner: "org",
497+ name: ConnectionName.make("legacy"),
498+ integration: INTEG,
499+ template: TEMPLATE,
500+ redirectUri: FLOW_REDIRECT_URI,
501+ });
502+ expect(started.status).toBe("redirect");
503+ if (started.status !== "redirect") return;
504+ const callback = yield* server.completeAuthorizationCodeFlow({
505+ authorizationUrl: started.authorizationUrl,
506+ });
507+ yield* executor.oauth.complete({ state: started.state, code: callback.code });
508+
509+ // Simulate a client written before origin_redirect_uri was persisted.
510+ yield* Effect.promise(() =>
511+ config.db.updateMany("oauth_client", {
512+ where: (b) => b("slug", "=", String(legacySlug)),
513+ set: { origin_redirect_uri: null },
514+ }),
515+ );
516+ yield* server.clearRequests;
517+
518+ const stableRedirectUri = "https://agent.example.test/executor/oauth/callback";
519+ const replacementSlug = yield* executor.oauth.registerDynamicClient({
520+ owner: "org",
521+ slug: OAuthClientSlug.make("stable-callback"),
522+ issuer: probe.issuer,
523+ registrationEndpoint: probe.registrationEndpoint!,
524+ authorizationUrl: probe.authorizationUrl,
525+ tokenUrl: probe.tokenUrl,
526+ resource: probe.resource,
527+ scopes: ["read"],
528+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
529+ clientName: "Stable callback DCR",
530+ redirectUri: stableRedirectUri,
531+ originIntegration: INTEG,
532+ });
533+
534+ expect(String(replacementSlug)).not.toBe(String(legacySlug));
535+ expect(registerRequestCount(yield* server.requests)).toBe(1);
536+ const clientSlugs = yield* Effect.map(executor.oauth.listClients(), (clients) =>
537+ clients.map((client) => String(client.slug)),
538+ );
539+ expect(clientSlugs).toContain(String(legacySlug));
540+ expect(clientSlugs).toContain(String(replacementSlug));
541+
542+ // The legacy row may still back live connections. Keeping it allows
543+ // those grants to refresh while new flows use the stable callback.
544+ yield* Effect.promise(() =>
545+ config.db.updateMany("connection", {
546+ where: (b) => b("name", "=", "legacy"),
547+ set: { expires_at: Date.now() - 60_000 },
548+ }),
549+ );
550+ const refreshed = (yield* executor.execute(
551+ ToolAddress.make("tools.acme.org.legacy.whoami"),
552+ {},
553+ )) as { token: string };
554+ expect(refreshed.token).toMatch(/^at_/);
555+ }),
556+ ),
464557 );
465558
466559 it.effect("uses resource to distinguish DCR clients only after an issuer already differs", () =>
packages/core/sdk/src/oauth-service.ts+13−20
@@ -1266,7 +1266,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 1265 unmodified lines
12661266 readonly slug: OAuthClientSlug;
12671267 readonly resource: string | null;
12681268 /** Redirect URI the candidate registered with the AS; null for rows
1269− * predating the column (treated as matching any flow callback). */
1269+ * predating the column. */
12701270 readonly redirectUri: string | null;
12711271 };
12721272
@@ -1357,20 +1357,17 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 84 unmodified lines
13571357 Effect.gen(function* () {
13581358 const candidates = yield* dcrCandidatesForIssuer(input.owner, issuer);
13591359 const resource = input.resource ?? null;
1360− // A candidate is reusable only when the callback it registered with the
1361− // AS still matches the current flow's callback — strict servers reject an
1362− // authorize request whose redirect_uri differs from the registration
1363− // (e.g. the callback origin changed after a sandbox was recreated while
1364− // the persisted client survived). A null stored redirect is a legacy row
1365− // predating the column: treated as matching so an upgrade doesn't
1366− // re-register every client whose callback never changed. A null FLOW
1367− // redirect has nothing to compare against, so it also reuses — the only
1368− // alternative is a fresh registration, which the missing-redirectUri
1369− // guard would fail.
1370− const redirectMatches = (candidate: DcrReuseCandidate): boolean =>
1371− candidate.redirectUri === null ||
1372− flowRedirectUri === null ||
1373− candidate.redirectUri === flowRedirectUri;
1360+ // A caller-supplied redirect is authoritative: only a client registered
1361+ // with that exact callback can be reused. In particular, a legacy row
1362+ // with no recorded redirect is not proof of a match. When the caller
1363+ // relies on the executor's configured default, retain the legacy-null
1364+ // compatibility behavior so upgrades do not re-register every client.
1365+ const hasExplicitRedirectUri = input.redirectUri != null;
1366+ const redirectMatches = (candidate: DcrReuseCandidate): boolean => {
1367+ if (candidate.redirectUri === flowRedirectUri) return true;
1368+ if (hasExplicitRedirectUri) return false;
1369+ return candidate.redirectUri === null || flowRedirectUri === null;
1370+ };
13741371 // A fresh registration must never take a slug an existing candidate
13751372 // holds: `createClient` deletes any colliding (owner, slug) row first,
13761373 // which would clobber a client that live connections still refresh
@@ -1384,11 +1381,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 7 unmodified lines
13841381 // resource row is the STRANDED one — but the first drift recovery
13851382 // already minted a client bound to the CURRENT callback, and later
13861383 // reconnects must reuse that instead of registering another duplicate
1387− // each time. Known limitation: the legacy null-redirect rule in
1388− // `redirectMatches` (a legacy row with no stored redirect matches any
1389− // flow redirect) still lets such a row win over a later, exactly-
1390− // matching one; kept deliberately so upgrades don't re-register every
1391− // client whose callback never changed.
1384+ // each time.
13921385 const reusable = candidates.find(
13931386 (client) => client.resource === resource && redirectMatches(client),
13941387 );