Rework the DCR OAuth client model: per-AS reuse, picker isolation, GC#1276
Open
RhysSullivan wants to merge 18 commits into
Open
Rework the DCR OAuth client model: per-AS reuse, picker isolation, GC#1276RhysSullivan wants to merge 18 commits into
RhysSullivan wants to merge 18 commits into
Conversation
A manual OAuth app registered from within an integration's connect dialog now stamps that integration on origin_integration (kind stays manual), mirroring how DCR clients already record the integration that requested them. Thread it through the createClient wire contract (HTTP API + core tool), persist it for both manual and DCR origins, and have parseOAuthClientOrigin surface it on the manual summary so the picker can match a bring-your-own app to its integration by recorded intent rather than guessing by root domain.
…n domain The Add-connection OAuth picker matched apps by registrable root domain only, so a DCR-minted MCP client (e.g. one registered against mcp.cloudflare.com) showed up when connecting the plain Cloudflare REST API. DCR clients are plumbing, reused per authorization server, not apps a user picks, so drop them from the picker entirely. For the manual apps that remain, grade into honest tiers: tier 1 is a match by recorded intent (registered from this integration's dialog) or exact endpoint host; tier 2 is a root-domain-only near-miss, kept separate so it is never silently presented as the integration's own app. A new registration from the dialog stamps its integration so future matches are exact, and the optimistic list mirrors the stamp immediately. selectDcrClientsForIntegration exposes the excluded DCR clients for a management surface.
Wire the modal to the new grouping: tier-1 exact/intent matches stay the default-selectable list, root-domain near-misses render in a visually separated, subdued "Other apps on this provider" group (selectable but never auto-selected), and the auto-registered DCR clients for this integration live in a collapsed "Auto-registered clients" section at the bottom, deletable but not pickable. Resolving the chosen client now spans every pickable tier so the near-miss and escape-hatch apps actually work. Also drop the DCR connect path's dependency on the picker's slug list: the slug is deterministic per authorization server since Part A, so runDcrConnect no longer needs existingSlugs threaded in.
Factor the DCR-vs-manual classification of an oauth_client row (explicit origin_kind or the legacy MCP-shaped heuristic) and the registrable-host helpers out of oauth-service into oauth-gc, so the runtime reuse lookup and the upcoming GC/backfill migrations share one source of truth for what counts as a DCR client. parseOAuthClientOrigin now routes through isDcrClassifiedRow. Also make the per-AS reuse pick deterministic: dcrCandidatesForIssuer now sorts matching candidates oldest-first (created_at, slug tiebreak) before start picks one, so which of several live duplicates sharing an (owner, issuer) gets reused is stable across boots and storage backends instead of raw findMany order. Add classifyOAuthClientGc (the conjunctive, fail-safe GC decision) and registrableOriginOfUrl (the issuer backfill value) with unit tests covering the decision matrix and the registrable-origin cases.
Add a libSQL boot data-migration (stamped through the existing data_migration ledger) that deletes DCR-classified oauth_client rows with zero referencing connections, and backfills origin_issuer for surviving DCR rows from the registrable origin of their token_url so the per-AS reuse lookup can key on them. The deletion predicate is conjunctive and fail-safe: a manual app, or a DCR app still referenced by a connection, is always kept. It reads the (tiny) oauth_client table and applies the shared classifyOAuthClientGc predicate in process rather than re-encoding the DCR heuristic as SQL, keeping one source of truth for a query that deletes user data. Idempotent: a second run is a no-op. Register it as the last entry in the local data-migration registry. Tests cover delete/keep/backfill, idempotency, the manual-never-deleted invariant, and the empty-table no-op against the real SQLite schema.
Add the cloud counterpart of the local DCR oauth_client cleanup as a stamped code migration run out-of-band by scripts/migrate.ts (with --dry-run and the advisory lock the runner already provides). It reuses the same shared classifyOAuthClientGc / registrableOriginOfUrl predicates the local path and the runtime reuse lookup use, so both hosts delete and backfill by an identical rule instead of a second Postgres-SQL encoding of the DCR heuristic and the eTLD+1 origin logic. Registered unconditionally in cloudCodeMigrations (no R2 dependency). Tests drive it over a fake sql handle: delete/keep/backfill, the dry-run plan, and the manual-never-deleted invariant.
Two integrations sharing a registrable root domain but differing by host (an MCP integration on mcp.cloudflare.com vs the REST integration on api.cloudflare.com) must not leak the MCP flow's auto-registered DCR client into the REST integration's app picker, and reconnecting the MCP integration must reuse the client rather than mint a duplicate. The e2e proves the server half over the wire: DCR mints the client against the MCP host, a second registration reuses it (no duplicate row), and listClients projects it as a DCR client keeping its MCP endpoints. The react integration test proves the picker half against the exact classifier the modal runs: the DCR client is offered in no tier of the REST integration's picker, while a same-root manual app still surfaces as a near-match and the DCR client stays visible in its own management list.
hostOf returns undefined for URLs new URL() can't parse (a scheme-less manifest-declared "oauth.cloudflare.com", or a DCR client built from discovery metadata that skipped endpoint validation). The exact-host comparison then read undefined === undefined as true, so an unrelated app with an unparseable URL landed in tier 1 and became the picker's default. Route every host/root comparison through a hostEq helper that only matches when both sides parsed. While here, memoize the picker grade so a fresh inline options object per keystroke no longer re-parses every client with tldts, and rename the client-side dcrClientSlug to optimisticDcrClientSlug: it is a host-only placeholder the UI shows before the server recomputes the authoritative, resource-aware slug, and sharing a name with the core function invited a grep-edit in one package to silently miss the other.
decideDcrClientReuse fell back to candidates[0] when the request had no resource and no resource-less candidate existed, so a resource-less flow could reuse a client minted for a specific RFC 8707 resource (whose tokens are bound to that resource). Only reuse a resource-less candidate now; otherwise register a fresh resource-less client. The base dcr-<host> slug can already be held by the first resource-scoped registration, and createClient deletes any colliding (owner, slug) before writing, so dedupe the fresh slug against existing candidates to avoid clobbering that client.
registrableHostname approximated eTLD+1 with a hardcoded 12-entry two-part suffix set, so hosts under suffixes it didn't list collapsed too far: accounts.example.co.in became co.in, colliding unrelated tenants in the GC deletion predicate and the issuer backfill. Depend on tldts (the version packages/react already pins) and use getDomain, falling back to the raw lowercase hostname when it returns null (localhost, IPs, single-label hosts). This matches how the react picker computes roots, so the reuse key and the picker agree on the same URL. Also refresh the module header: the predicates are one source of truth imported by three call sites, not three copies kept in lockstep.
Both GC migrations (cloud code migration and libSQL boot migration) ran a COUNT(*) against connection once per oauth_client row. Replace that N+1 with a single GROUP BY (tenant, oauth_client_owner, oauth_client) pass loaded into a Map before the loop. The cloud test fake now models the grouped query shape instead of the per-row COUNT.
Swap the hand-rolled useState + Button + "+/-" glyph toggle for the shared Collapsible/CollapsibleTrigger/CollapsibleContent with a rotating chevron, matching the Advanced disclosure in mcp-install-card. Keeps the subdued visual weight. Also drop the dead existingSlugs field from RunDcrConnectInput (runDcrConnect never read it) and the stale existingSlugs: [] test args, so readers stop thinking the connect path needs the picker's slug list.
Clarify that the column holds the discovered OIDC issuer for new DCR registrations (real AS metadata, can differ from a token_url derivation) and a derived registrable-origin for backfilled legacy rows (a cache of the pure registrableOriginOfUrl).
Contributor
Cloudflare preview
Sign-in is Cloudflare Access (one-time PIN to an allowed email). The preview has its own database and encryption key; it is destroyed when this PR closes. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-marketing | 3e72fa3 | Commit Preview URL Branch Preview URL |
Jul 03 2026, 04:58 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-cloud | 3e72fa3 | Jul 03 2026, 04:59 AM |
@executor-js/cli
@executor-js/config
@executor-js/execution
@executor-js/sdk
@executor-js/codemode-core
@executor-js/runtime-quickjs
@executor-js/plugin-file-secrets
@executor-js/plugin-graphql
@executor-js/plugin-keychain
@executor-js/plugin-mcp
@executor-js/plugin-onepassword
@executor-js/plugin-openapi
executor
commit: |
Greptile SummaryThis PR replaces the DCR OAuth client model with a cleaner per-AS reuse system, fixes picker contamination by completely hiding DCR clients from the app picker, and introduces one-time GC migrations to clean up legacy duplicate rows.
Confidence Score: 5/5Safe to merge. All existing connections resolve their client by stored (owner, slug) and are unaffected by the picker or GC changes. The GC only deletes orphaned rows with zero referencing connections, and the fail-safe predicate is conjunctive and well-tested. The core reuse logic, picker redesign, and GC decision matrix are solid and covered by thorough tests. The one nuance — that backfilled legacy rows on subdomain-based authorization servers may not be reused immediately after the migration — results in at most one bounded duplicate per affected AS rather than an ongoing accumulation, and does not break any existing connection or security boundary. The Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client as Connect Flow
participant Service as oauth-service.ts
participant DB as oauth_client table
participant AS as Authorization Server
note over DB: After GC migration: legacy rows have origin_issuer = registrableOrigin(token_url)
Client->>Service: registerDynamicClient(owner, issuer?, registrationEndpoint, resource)
Service->>AS: probe OIDC discovery
AS-->>Service: "issuer = https://oauth.cloudflare.com"
Service->>Service: canonicalDcrIssuer returns https://oauth.cloudflare.com
Service->>DB: findMany(owner) all DCR candidates
DB-->>Service: "slug=cloudflare-mcp, origin_issuer=https://cloudflare.com"
Service->>Service: dcrIssuerMatches(https://cloudflare.com, https://oauth.cloudflare.com)
note right of Service: rowIssuer non-null skips token-host fallback. Neither dcrIssuerMatches arm covers registrable-root vs subdomain.
alt issuer mismatch - subdomain AS with backfilled legacy row
Service->>Service: no candidate found
Service->>AS: POST /register new DCR client
AS-->>Service: client_id
Service->>DB: "INSERT dcr-oauth-cloudflare-com with origin_issuer=https://oauth.cloudflare.com"
note over DB: cloudflare-mcp AND dcr-oauth-cloudflare-com now co-exist
else issuer matches - localhost or exact origin
Service->>Service: existingSlug found return immediately
note over Service: No registration call, no duplicate
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client as Connect Flow
participant Service as oauth-service.ts
participant DB as oauth_client table
participant AS as Authorization Server
note over DB: After GC migration: legacy rows have origin_issuer = registrableOrigin(token_url)
Client->>Service: registerDynamicClient(owner, issuer?, registrationEndpoint, resource)
Service->>AS: probe OIDC discovery
AS-->>Service: "issuer = https://oauth.cloudflare.com"
Service->>Service: canonicalDcrIssuer returns https://oauth.cloudflare.com
Service->>DB: findMany(owner) all DCR candidates
DB-->>Service: "slug=cloudflare-mcp, origin_issuer=https://cloudflare.com"
Service->>Service: dcrIssuerMatches(https://cloudflare.com, https://oauth.cloudflare.com)
note right of Service: rowIssuer non-null skips token-host fallback. Neither dcrIssuerMatches arm covers registrable-root vs subdomain.
alt issuer mismatch - subdomain AS with backfilled legacy row
Service->>Service: no candidate found
Service->>AS: POST /register new DCR client
AS-->>Service: client_id
Service->>DB: "INSERT dcr-oauth-cloudflare-com with origin_issuer=https://oauth.cloudflare.com"
note over DB: cloudflare-mcp AND dcr-oauth-cloudflare-com now co-exist
else issuer matches - localhost or exact origin
Service->>Service: existingSlug found return immediately
note over Service: No registration call, no duplicate
end
Reviews (2): Last reviewed commit: "Escape the NUL separator so the cloud GC..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
307
to
309
| clientId: clientId.trim(), | ||
| clientSecret: clientSecret.trim(), | ||
| resource, |
There was a problem hiding this comment.
originIntegration: fixedSlug ? null : (integrationSlug ?? null) means that any edit operation (changing client ID, secret, name, etc.) replaces origin_integration with null, stripping the very intent stamp this PR relies on for tier-1 picker matching. An app registered from the Cloudflare MCP dialog with origin_integration = "cloudflare_mcp" would drop to tier-2 (root-domain near-match) after the user's first edit — even if no endpoint URLs changed. The OAuthClientFormPrefill interface doesn't thread originIntegration through, so the existing stamp is unrecoverable in the edit path.
Comment on lines
+132
to
+135
| yield* execute(client, "BEGIN"); | ||
| return yield* applyAll.pipe( | ||
| Effect.tapError(() => execute(client, "ROLLBACK").pipe(Effect.ignore)), | ||
| ); |
There was a problem hiding this comment.
Fiber interruption (process kill during boot) does not trigger tapError, leaving an open SQLite transaction that relies on connection-close for implicit rollback. Adding Effect.onInterrupt makes the boundary explicit.
Suggested change
| yield* execute(client, "BEGIN"); | |
| return yield* applyAll.pipe( | |
| Effect.tapError(() => execute(client, "ROLLBACK").pipe(Effect.ignore)), | |
| ); | |
| yield* execute(client, "BEGIN"); | |
| return yield* applyAll.pipe( | |
| Effect.tapError(() => execute(client, "ROLLBACK").pipe(Effect.ignore)), | |
| Effect.onInterrupt(() => execute(client, "ROLLBACK").pipe(Effect.ignore)), | |
| ); |
The reference-count map key used literal 0x00 bytes as its separator, which made git (and GitHub's diff view) treat the whole file as binary. Use the \u0000 escape instead — same runtime string, plain-text source. The local sqlite migration already used a printable separator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1120. Supersedes #1139 and #1142.
Users keep seeing DCR-minted MCP clients ("Cloudflare mcp", "Cloudflare mcp 2", …) in the Add-connection OAuth2 app picker for unrelated integrations on the same domain — e.g. the plain Cloudflare REST API offering clients registered against mcp.cloudflare.com. Root cause: the picker matched apps by registrable root domain only, and DCR re-registered a fresh client on every connect, so duplicates accumulated and then leaked across every integration sharing an eTLD+1.
This replaces the model instead of patching the filter. The principle: DCR clients are plumbing, not apps — they never appear in the connect picker; manual apps are matched by recorded intent first, domain heuristic second, and near-misses are grouped honestly instead of silently mixed in.
What changed
Per-AS DCR client reuse (no more duplicates)
oauth_clientgains a nullableorigin_issuercolumn (cloud0008_slow_mephisto, local0003_graceful_the_fallen). For new DCR registrations it stores the discovered AS issuer; for backfilled legacy rows, the registrable origin derived fromtoken_url.registerDynamicClientis now get-or-register: keyed on (owner, canonical AS issuer), with the resource differentiating only once a second resource shows up on the same AS. Legacy null-issuer rows are found via a token-host fallback; candidate ordering is deterministic (oldest first). A resource-less request never grabs a resource-scoped client.dcr-<host>(withuniqueDcrSlugfor the resource-differentiated case) instead of numeric suffixing.Picker redesign
origin.kind === "dynamic_client_registration"(including legacy rows classified by heuristic) are filtered out of the app picker entirely.origin_integrationor exact endpoint-host match) and a collapsed, subdued "Other apps on this provider" group for eTLD+1-only matches — never silently merged. Registering a manual app from within an integration's dialog now stampsorigin_integrationso future matching is exact.GC of accumulated duplicates
origin_issueron survivors so legacy clients are discoverable by the reuse lookup. Live duplicates sharing (owner, issuer) are left alone; deterministic ordering picks one at reuse time.isDcrClassifiedRow), and registrable-domain matching now uses the full public suffix list via tldts instead of a hand-rolled suffix set (fixes.co.in/.com.cn/.co.zastyle domains).Safety: connections resolve their client by stored (owner, slug) — refresh and reconnect never go through the picker, so hiding DCR clients cannot break existing connections. GC only deletes rows with zero referencing connections.
Tests
oauth-register-dynamic.test.ts— reuse hits/misses across owner/AS/resource, legacy fallback, slug determinismoauth-gc.test.ts— GC decision matrix (DCR+orphaned deleted, referenced kept, manual kept, heuristic-classified treated as DCR, backfill, idempotency), multi-part-TLD domainsuse-effective-oauth-client.test.ts+dcr-root-domain-isolation.test.ts(react) — DCR excluded, intent beats domain, domain-only lands tier 2, cross-integration leak regression, scheme-less endpoint regressione2e/cloud/dcr-root-domain-isolation.test.ts— over-the-wire DCR connect, no-duplicate reuse, API projectionNotes for review
git diff origin/main...HEAD -- apps/cloud/drizzle/meta/_journal.json apps/local/drizzle/meta/_journal.jsonand renumber.listClients(the mcp plugin currently re-implements the DCR check), and moving the legacy MCP-slug regex heuristic from the live deletion predicate to backfill-only.