History

Drop the token-host fallback from the DCR reuse lookup

dcrCandidatesForIssuer previously matched a null-origin_issuer row by comparing registrable token hosts. Now a candidate matches only via dcrIssuerMatches on a non-null, canonicalized stored issuer. The GC migration backfills origin_issuer on every surviving DCR row, so post-migration the token-host fallback is dead code; in the brief deploy->migration window a missed lookup just mints one duplicate that the migration immediately GCs, which is safer than reusing on a fuzzy token-host guess. Drops the now-unused tokenUrl parameter and the registrableHostOfUrl import. register-dynamic tests: the old null-issuer-token-host reuse test becomes two cases: a null-issuer legacy row is NOT reused (mints fresh), and a backfilled row (origin_issuer set) IS reused with no duplicate.

Rhys Sullivan committed 4 months ago

Commit e9ee6661 parent 69b9bc63 files · +79 −20Browse files

Unified · Split

3 files
  1. packages/core/sdk/src/oauth-gc.ts +6 −4
  2. packages/core/sdk/src/oauth-register-dynamic.test.ts +64 −1
  3. packages/core/sdk/src/oauth-service.ts +9 −15
packages/core/sdk/src/oauth-gc.ts+6−4
@@ -104,10 +104,12 @@ export interface OAuthClientGcRow { · 103 unmodified lines
104 * is a DCR client. This was validated against the production database: of the104 * is a DCR client. This was validated against the production database: of the
105 * 394 rows that carry an explicit origin_kind stamp, `grant =105 * 394 rows that carry an explicit origin_kind stamp, `grant =
106 * 'authorization_code' AND resource IS NOT NULL` matches all 329 stamped DCR106 * 'authorization_code' AND resource IS NOT NULL` matches all 329 stamped DCR
107 * rows and zero stamped-manual rows (100% recall, no false positives). The107 * rows (100% recall). It also matches a handful of stamped-manual BYO rows
108 * earlier slug/resource `…mcp…` regex heuristic had bad recall (it missed108 * that point at MCP servers, but those never reach this arm: an explicit
109 * legacy rows whose slug was a bare provider name like `linear` / `notion` /109 * stamp always wins. The earlier slug/resource `…mcp…` regex heuristic had
110 * `cloudflare`), so it is gone; the resource presence check subsumes it.110 * bad recall (it missed legacy rows whose slug was a bare provider name like
111 * `linear` / `notion` / `cloudflare`), so it is gone; the resource presence
112 * check subsumes it.
111 *113 *
112 * Explicit stamps always win (arm 1): a stamped `manual` BYO row keeps its114 * Explicit stamps always win (arm 1): a stamped `manual` BYO row keeps its
113 * classification even when it carries a resource, and a stamped DCR row is DCR115 * classification even when it carries a resource, and a stamped DCR row is DCR
packages/core/sdk/src/oauth-register-dynamic.test.ts+64−1
@@ -252,9 +252,14 @@ describe("oauth.registerDynamicClient", () => { · 251 unmodified lines
252 ),252 ),
253 );253 );
254 254
255 it.effect("reuses legacy DCR-looking rows with no stored issuer by token host", () =>255 it.effect("does NOT reuse a legacy DCR row with no stored issuer (mints fresh)", () =>
256 Effect.scoped(256 Effect.scoped(
257 Effect.gen(function* () {257 Effect.gen(function* () {
258 // Post-migration, the reuse lookup keys strictly on a non-null
259 // origin_issuer: the fuzzy token-host fallback is gone. A legacy row that
260 // has not yet been backfilled (null origin_issuer) is therefore NOT
261 // reused, so a fresh DCR registration happens. The GC migration then
262 // backfills/GCs any duplicate this transient window mints.
258 const server = yield* serveOAuthTestServer({ scopes: ["read"] });263 const server = yield* serveOAuthTestServer({ scopes: ["read"] });
259 const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });264 const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });
260 yield* executor.acme.seed();265 yield* executor.acme.seed();
@@ -279,6 +284,64 @@ describe("oauth.registerDynamicClient", () => { · 18 unmodified lines
279 );284 );
280 yield* server.clearRequests;285 yield* server.clearRequests;
281 286
287 const registered = yield* executor.oauth.registerDynamicClient({
288 owner: "org",
289 slug: OAuthClientSlug.make("new-attempt"),
290 issuer: probe.issuer,
291 registrationEndpoint: probe.registrationEndpoint!,
292 authorizationUrl: probe.authorizationUrl,
293 tokenUrl: probe.tokenUrl,
294 resource: server.mcpResourceUrl,
295 scopes: ["read"],
296 tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
297 clientName: "Acme DCR",
298 redirectUri: FLOW_REDIRECT_URI,
299 originIntegration: INTEG,
300 });
301
302 // A fresh client is registered, not the null-issuer legacy row.
303 expect(registered).not.toBe(legacySlug);
304 const requests = yield* server.requests;
305 expect(registerRequestCount(requests)).toBe(1);
306 }),
307 ),
308 );
309
310 it.effect("reuses a legacy DCR row once its origin_issuer is backfilled", () =>
311 Effect.scoped(
312 Effect.gen(function* () {
313 // The post-backfill counterpart: after the GC migration stamps a legacy
314 // row's origin_issuer, the reuse lookup keys on it and mints no
315 // duplicate. This is the steady state the migration establishes.
316 const server = yield* serveOAuthTestServer({ scopes: ["read"] });
317 const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });
318 yield* executor.acme.seed();
319 const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
320 const legacySlug = OAuthClientSlug.make("cloudflare-mcp");
321
322 yield* executor.oauth.createClient({
323 owner: "org",
324 slug: legacySlug,
325 authorizationUrl: probe.authorizationUrl,
326 tokenUrl: probe.tokenUrl,
327 resource: server.mcpResourceUrl,
328 grant: "authorization_code",
329 clientId: "legacy-dcr-client",
330 clientSecret: "",
331 });
332 // Simulate the migration's backfill: legacy DCR stamp + issuer set.
333 yield* Effect.promise(() =>
334 config.db.updateMany("oauth_client", {
335 where: (b) => b("slug", "=", String(legacySlug)),
336 set: {
337 origin_kind: "dynamic_client_registration",
338 origin_integration: null,
339 origin_issuer: probe.issuer,
340 },
341 }),
342 );
343 yield* server.clearRequests;
344
282 const reused = yield* executor.oauth.registerDynamicClient({345 const reused = yield* executor.oauth.registerDynamicClient({
283 owner: "org",346 owner: "org",
284 slug: OAuthClientSlug.make("new-attempt"),347 slug: OAuthClientSlug.make("new-attempt"),
packages/core/sdk/src/oauth-service.ts+9−15
@@ -70,13 +70,7 @@ import { · 69 unmodified lines
70 type OAuthEndpointUrlPolicy,70 type OAuthEndpointUrlPolicy,
71} from "./oauth-helpers";71} from "./oauth-helpers";
72import { OAUTH2_SESSION_TTL_MS, encodeOAuthCallbackState } from "./oauth";72import { OAUTH2_SESSION_TTL_MS, encodeOAuthCallbackState } from "./oauth";
73import {73import { canonicalIssuerUrl, hostOfUrl, isDcrClassifiedRow, parseUrl } from "./oauth-gc";
74 canonicalIssuerUrl,
75 hostOfUrl,
76 isDcrClassifiedRow,
77 parseUrl,
78 registrableHostOfUrl,
79} from "./oauth-gc";
80 74
81/** Connection-minting input for the OAuth flow — extends a connection create75/** Connection-minting input for the OAuth flow — extends a connection create
82 * with the OAuth lifecycle fields (client slug, refresh material, expiry,76 * with the OAuth lifecycle fields (client slug, refresh material, expiry,
@@ -677,7 +671,6 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 594 unmodified lines
677 const dcrCandidatesForIssuer = (671 const dcrCandidatesForIssuer = (
678 owner: Owner,672 owner: Owner,
679 issuer: string | null,673 issuer: string | null,
680 tokenUrl: string,
681 ): Effect.Effect<readonly DcrReuseCandidate[], StorageFailure> =>674 ): Effect.Effect<readonly DcrReuseCandidate[], StorageFailure> =>
682 deps.fuma675 deps.fuma
683 .use("oauth_client.findMany", (db) =>676 .use("oauth_client.findMany", (db) =>
@@ -687,17 +680,18 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 3 unmodified lines
687 )680 )
688 .pipe(681 .pipe(
689 Effect.map((rows) => {682 Effect.map((rows) => {
690 const inputTokenHost = registrableHostOfUrl(tokenUrl);
691 const matches = rows.flatMap(683 const matches = rows.flatMap(
692 (row): readonly (DcrReuseCandidate & { readonly createdAt: number })[] => {684 (row): readonly (DcrReuseCandidate & { readonly createdAt: number })[] => {
693 if (parseOAuthClientOrigin(row).kind !== "dynamic_client_registration") return [];685 if (parseOAuthClientOrigin(row).kind !== "dynamic_client_registration") return [];
686 // A candidate matches only via a non-null, canonicalized stored
687 // issuer. The GC migration backfills origin_issuer on every
688 // surviving DCR row, so post-migration a null-issuer row is a
689 // transient (unmigrated) row; skipping it just mints one duplicate
690 // the migration then GCs, rather than reusing on a fuzzy token-host
691 // guess.
694 const rowIssuer =692 const rowIssuer =
695 row.origin_issuer == null ? null : canonicalIssuerUrl(String(row.origin_issuer));693 row.origin_issuer == null ? null : canonicalIssuerUrl(String(row.origin_issuer));
696 const issuerMatches =694 const issuerMatches = rowIssuer !== null && dcrIssuerMatches(rowIssuer, issuer);
697 rowIssuer !== null
698 ? dcrIssuerMatches(rowIssuer, issuer)
699 : inputTokenHost !== null &&
700 registrableHostOfUrl(String(row.token_url)) === inputTokenHost;
701 if (!issuerMatches) return [];695 if (!issuerMatches) return [];
702 return [696 return [
703 {697 {
@@ -733,7 +727,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 29 unmodified lines
733 StorageFailure727 StorageFailure
734 > =>728 > =>
735 Effect.gen(function* () {729 Effect.gen(function* () {
736 const candidates = yield* dcrCandidatesForIssuer(input.owner, issuer, input.tokenUrl);730 const candidates = yield* dcrCandidatesForIssuer(input.owner, issuer);
737 const resource = input.resource ?? null;731 const resource = input.resource ?? null;
738 if (resource !== null) {732 if (resource !== null) {
739 const matchingResource = candidates.find((client) => client.resource === resource);733 const matchingResource = candidates.find((client) => client.resource === resource);