[mashkovd]

What

Adds OAUTH_DCR_REDIRECT_URIS, a new, separate, byte-exact redirect URI allowlist for POST /oauth/register. It is empty by default, so nothing changes for any deployment that does not set it. With it set, the Cloudflare MCP portal can register itself, which lets tg move from manual to automatic (DCR) mode as org rule mctlhq/.github#137 requires.

Rules at /oauth/register:

Registration's redirect_uris	Result
all on OAUTH_DCR_REDIRECT_URIS	accepted, pinned client (below); the implicit-host list is not consulted
a listed URI plus anything else (even https://claude.ai/cb)	400 invalid_redirect_uri, new audit reason redirect_not_on_dcr_list
no listed URI	the unchanged OAUTH_ALLOWED_IMPLICIT_HOSTS path

A pinned registration:

• gets a client_id derived from its canonical (deduplicated, sorted) redirect set (tgdcr_ + a truncated SHA-256). Repeating the registration returns the same client and adds no row, so the rows are bounded by the subsets of the list. seerrsense#75 made the same idempotency choice.
• is exempt from the 24h ClientRegistrationTTL sweep and from the MaxRegisteredClients cap. This covers both the in-memory map and the Postgres queries. It is needed because the portal registers once and then uses that client_id for every user login. Without it, portal logins would break 24h after registration, since the portal hosts are not on the implicit list.
• is checked again at /oauth/authorize: each redirect_uri must still be on the current list, so removing an entry revokes it without a DB change.

Entries are validated at startup with the pre-registration rules: https, or http on loopback only; a host; no userinfo, backslash or fragment. A bad entry aborts the boot. The check is shared through a new validateExactRedirectURI, and validatePreregisteredClient now uses it too, with its error messages unchanged.

Scopes (rule 3): no code change was needed. narrowGrant already returns the full entitlement when /oauth/authorize names no scope. A new end-to-end test covers this for a pinned DCR client: register with no scope, authorize with no scope, and a client-tier principal gets telegram:dialogs:read telegram:messages:read telegram:messages:send telegram:messages:pin account:manage. A client that names scopes still gets exactly those. scope in the registration body is still only logged, as before.

Rate limit: the per-IP allowRegister limit (10/min by default) runs before anything else, pinned path included.

Why, and how it fits with #585

The #585 design says the portal callback must not go into OAUTH_ALLOWED_IMPLICIT_HOSTS or into any new additive implicit-host list. A host list is too broad and also covers every unregistered client_id. This PR follows that:

• the new list matches exact URIs, not hosts;
• it only affects /oauth/register, and never an unregistered client_id at /oauth/authorize;
• the implicit path, PKCE-S256, the public-client contract and scope behaviour are unchanged.

#585 made pre-registration the preferred portal path when DCR was compatibility-only. #137 since found that manual mode freezes the portal's tool catalogue, so the org rule now wants DCR restricted to the portal. This adds that path with the same exact-match boundary. OAUTH_PREREGISTERED_CLIENTS keeps working, so both paths can run side by side during the switch. docs/cloudflare-portal-compat.md has a new "Automatic (DCR) mode" section that explains this.

client_name of a pinned client is server-assigned. It is always the constant db.PinnedClientName ("Cloudflare MCP portal"), stored and returned; the caller's client_name is ignored. The reason is that a pinned client_id can be derived from public callbacks, and the name flows into oauth_refresh_tokens.client_name and broadcast audience selection. Neither a replay nor whoever registers first can choose it. The row is also written once (ON CONFLICT DO NOTHING) as a guard. Registering never evicts for a pinned client (it adds no row), on the DB path as well as in memory. A preregistered client_id using the reserved tgdcr_ prefix is refused at boot. Codes always go to the listed callbacks.

Tests

New in internal/oauth/dcr_allowlist_test.go. These fail on the old code; there, the portal registration is refused on redirect_host_not_allowed:

• TestDCRAllowlist_ExactSetAccepted: both callbacks accepted with AllowImplicitClient=false, and the client validates against each.
• TestDCRAllowlist_OneForeignRedirectRejected: a foreign URI in the set gives 400 invalid_redirect_uri, including claude.ai, and leaves no pinned row behind.
• TestDCRAllowlist_NearMissesRejected: prefix, trailing slash, suffix, query, fragment, port, host/path case, http, another server id, and leading whitespace are all rejected.
• TestDCRAllowlist_EmptyListKeepsPreviousBehaviour: with the list empty, the portal registration is refused with the same message as before and claude.ai still gets a random tgmcp_ id. With the list set, the implicit path is unchanged.
• TestDCRAllowlist_IdempotentAndBounded, TestDCRAllowlist_PinnedSurvivesSweepAndCap, TestDCRAllowlist_RemovedURIStopsAuthorizing, TestNew_RejectsBadDCRRedirectURIs.
• TestDCRAllowlist_NoScopeGetsFullNegotiableScopes: the full negotiable set when no scope is named, and exactly the named scopes otherwise.

Also added:

• TestDCRAllowlist_PostgresPinnedWiring (internal/oauth/dcr_allowlist_pg_test.go): drives /oauth/register on the useDB path in an isolated Postgres schema. A pinned replay at the cap evicts no dynamic row, an arbitrary client_name is stored and echoed as the server-assigned constant, and the client validates from the DB. It fails without either fix (checked by mutation).
• internal/db/client_reg_pinned_test.go: TestClientRegPinned_PostgresExemptFromSweepAndEviction, a Postgres-backed test of the SQL exemptions. It is picked up by the CI "must not skip" guard. It fails when the exemption is removed (checked by mutation).
• internal/config: TestLoadOAuthDCRRedirectURIs.

The tests use synthetic callbacks (*.example.test), following the repo rule not to bake production callbacks into it.

Local runs (TEST_DATABASE_URL pointed at a postgres:17.2 container):

go vet ./...                       -> clean
go test -race ./...                -> all packages ok (internal/oauth 79.9s, internal/db 82.4s, docs ok)
go test ./internal/db -run 'Postgres|TestOpenPool' -v -> all PASS, none skipped
golangci-lint run --new-from-rev origin/main ./... -> 0 issues
GOOS=windows go build ./cmd/local; GOOS=darwin GOARCH=arm64 go build ./... -> ok
go run ./cmd/productupdates gate   -> ok (no tool surface change)

Docs updated: .env.example, README.md (env table), SECURITY.md, docs/cloudflare-portal-compat.md, docs/runbook.md (new reason token, dcr_allowlisted field on accepted lines), docs/troubleshooting.md, and the config comments in internal/config and internal/oauth. CHANGELOG.md is maintained by release-please.

Deployment (not done here; mctl-gitops)

Once this is released, add this to platform-gitops/services/labs/mctl-telegram/values.yaml, next to OAUTH_PREREGISTERED_CLIENTS:

  # Exact-match DCR allowlist (mctlhq/.github#137): lets the Cloudflare MCP
  # portal register itself and run tg in automatic mode. Byte-exact; not a
  # host allowlist, so OAUTH_ALLOWED_IMPLICIT_HOSTS stays as it is.
  OAUTH_DCR_REDIRECT_URIS: "https://mcp.mctl.ai/servers-callback,https://dash.cloudflare.com/6a09f637d20e1f66a8e9d45ebe778058/one/access-controls/ai-controls/mcp-server/oauth-callback/tg"

Then switch tg to automatic mode in mcp-servers.tf (mctlhq/mctl-gitops#1363). Keep OAUTH_PREREGISTERED_CLIENTS until the automatic-mode login is confirmed, then it can be dropped.

Refs mctlhq/.github#137, mctlhq/mctl-gitops#1363

🤖 Generated with Claude Code

https://claude.ai/code/session_01T5nLoYSquzMsGsdS64wvry

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @mashkovd's task in 5m 3s —— View job

---

Review posted — changes requested (0 P1, 2 P2, 2 P3)

• Read the pinned diff (d1f22cd...d0bf000)
• Analyze internal/oauth/server.go changes
• Analyze internal/db/store_oauth.go and config changes
• Check test coverage
• Post inline findings and verdict

The exact-match boundary is sound. The pinned path never widens OAUTH_ALLOWED_IMPLICIT_HOSTS, never touches an unregistered client_id at /oauth/authorize, and checkPinnedRedirect correctly re-applies the current list to a registration that outlives the TTL sweep. Boot validation is shared and fail-closed. Coverage of the in-memory path and of the SQL exemptions is thorough. I also checked the $1/$2 placeholder reuse in the new SQL — oauth_client_registrations lives only in pgSchema() and useDB is Postgres-only, so it is fine.

P2 — internal/oauth/server.go:2712: cap eviction runs for pinned registrations on the DB path.
The in-memory branch is deliberately guarded (if s.cfg.MaxRegisteredClients > 0 && !pinned, server.go:2732) with the comment "a pinned re-registration overwrites its own entry, so it never needs room either". The useDB branch has no such guard; the new SQL only excludes pinned rows from being the victim, not from triggering an eviction. At the cap, each replayed pinned registration deletes a real dynamic registration while adding no row of its own — net-destructive, where a normal replay is net-neutral. Fix this →

P2 — internal/db/store_oauth.go:266: client_name last-write-wins is not display-only.
The PR body flags this as a cosmetic known limit, but the name propagates through clientDisplayName into oauth_refresh_tokens.client_name for every subsequent portal login, and internal/db/broadcast.go:182-184 uses that column to build the "connected via" fact for broadcast audience selection. A pinned client_id is derivable from the portal's public callbacks, so any unauthenticated caller can set it and — since the portal registers once and never again — the value persists. Make the name write-once for pinned rows. Fix this →

Test gap: nothing covers the pinned + useDB wiring — dcr_allowlist_test.go is in-memory only, client_reg_pinned_test.go is SQL only. That seam is exactly where the first P2 slipped through.

P3 (non-blocking), inline on the PR: unescaped _ wildcard in pinnedClientIDPattern; no boot-time guard against a PreregisteredClients entry using the reserved tgdcr_ prefix (it would be silently unusable at /oauth/authorize).

Four inline comments and a CHANGES_REQUESTED review are on the PR, bound to d0bf000. I could not run go vet or the test suite — those commands need approval in this environment, so the review is static only.
· branch feat/dcr-portal-callbacks

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit d0bf000.

No significant issues found.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 2 P2, 2 P3.

The exact-match boundary itself is right, and the security argument in the PR body holds up: the pinned path never widens AllowedImplicitHosts, never touches an unregistered client_id at /oauth/authorize, and checkPinnedRedirect correctly re-applies the current list to a registration that outlives the sweep. Validation is shared and fail-closed at boot. Test coverage of the in-memory path and of the SQL exemptions is thorough.

Two things to fix:

• P2 (internal/oauth/server.go:2712) — the useDB branch calls EvictOldestClientRegIfOver for pinned registrations, while the in-memory branch below deliberately skips it (&& !pinned). At the cap, each replayed pinned registration destroys a real dynamic registration while adding no row of its own.
• P2 (internal/db/store_oauth.go:266) — the last-write-wins client_name is not display-only: it propagates to oauth_refresh_tokens.client_name for every subsequent portal login, which internal/db/broadcast.go:182 uses for audience selection. Since a pinned client_id is derivable from public callbacks, any unauthenticated caller can set it. Make the name write-once for pinned rows.

Also worth noting: nothing covers the pinned + useDB wiring — dcr_allowlist_test.go is in-memory only and client_reg_pinned_test.go is SQL only, which is the seam the first P2 fell through.

P3s (non-blocking) are inline: unescaped _ in the LIKE pattern, and no boot-time guard against a preregistered client_id using the reserved tgdcr_ prefix.

[claude]

Claude finished @mashkovd's task in 3m 58s —— View job

---

Review of f783dad — approve (0 P1, 0 P2, 3 P3)

• Read the pinned diff (d1f22cd...f783dad)
• Verify the two P2 fixes (cap eviction on the DB path, write-once client_name)
• Re-check the P3s and scan for new issues
• Post inline findings and verdict

Both P2s from the d0bf000 round are fixed, and fixed more thoroughly than I suggested.

Cap eviction. The pinned + useDB path now branches to InsertPinnedClientReg and never calls EvictOldestClientRegIfOver at all (internal/oauth/server.go:2716). On top of that, EvictOldestClientRegIfOver excludes pinned rows from the count as well as from victim selection (internal/db/store_oauth.go:152-160), so a pinned row cannot push a real registration over the cap either — stronger than the if !pinned guard I proposed. When there are no non-pinned rows the victim subselect yields NULL and the DELETE is a no-op, which is correct. The in-memory branch matches (&& !pinned, plus isPinnedClientID in the dynamicCount scan and in sweep).

client_name. InsertPinnedClientReg is ON CONFLICT DO NOTHING and returns the stored row, and the in-memory path keeps existing.ClientName. The handler then echoes the stored name, so a replay does not even confirm to the caller that its name was discarded. That closes the oauth_refresh_tokens.client_name → internal/db/broadcast.go path. The prefix guard in InsertPinnedClientReg keeps the write-once semantics from being reachable for a non-pinned id by mistake.

Both P3s are addressed: starts_with replaces LIKE 'tgdcr_%' at all three sites (no _ wildcard left; the table exists only in pgSchema() and useDB is Postgres-only, so the function is always available), and validatePreregisteredClient now rejects the reserved prefix at boot with a clear message. The validateExactRedirectURI extraction keeps the pre-registration error strings byte-identical, so the pinned path inherits the full shape contract — which is what makes skipping validateImplicitRedirectURI safe: every accepted URI is byte-equal to a list entry already validated in New.

Test seam closed. TestDCRAllowlist_PostgresPinnedWiring drives POST /oauth/register through the real store in its own schema and asserts both new properties end to end — replay at the cap evicts nothing, and the name and echoed name stay at the first registration's value. That is exactly the handler/store wiring the first P2 fell through.

P3 (non-blocking, inline on the PR)

1. internal/oauth/dcr_allowlist_pg_test.go:24 — the new Postgres test is outside the CI "must not skip" guard, which runs only ./internal/db/. It runs today, but it would go quiet rather than red if the service container stopped reaching the test environment. Its name already matches the Postgres filter, so adding ./internal/oauth/ to that one command covers it. Fix this →
2. docs/cloudflare-portal-compat.md:137 — the write-once client_name is operator-visible and documented nowhere. A portal renaming itself is silently ignored, and correcting a name needs a manual row delete. One sentence here and in SECURITY.md. Fix this →
3. internal/db/store_oauth.go:72 — "bounded by the allowlist itself" is 2^n - 1 non-empty subsets, and those rows are reclaimed by neither the sweep nor the cap. Negligible at the planned two entries (3 rows); worth stating in the constant's comment so the next operator sizing a list sees the shape. Fix this →

Verdict

Approve. The three inline P3s are posted and bound to f783dad. The formal approving review could not be submitted: GitHub rejects a review bound to a commit that is no longer the PR head (422 — this pull request has been updated since you started reviewing), and this run is pinned to f783dad, so I will not resubmit it against whatever the head is now. The SHA-scoped run for the newer commit covers that.

This is a static review — go vet and the test suite need command approval in this environment, so I could not run them.
· branch feat/dcr-portal-callbacks

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit f783dad.

No significant issues found.

[claude]

Claude finished @mashkovd's task in 5m 35s —— View job

---

Review posted — approved (0 P1, 0 P2, 3 P3)

• Read the pinned diff (d1f22cd...0322b89)
• Verify the server-assigned client_name change and the earlier P2 fixes
• Re-check open P3s and scan for new issues
• Post inline findings and verdict

Both P2s from the previous round are fixed, and in the stronger form rather than the minimum:

• Cap eviction (internal/oauth/server.go:2718) — the pinned registration now takes its own useDB branch with no EvictOldestClientRegIfOver, matching the in-memory !pinned guard, and EvictOldestClientRegIfOver excludes pinned rows from the count too, not just from being the victim. TestDCRAllowlist_PostgresPinnedWiring covers the handler/store seam the finding fell through, against a schema-isolated Postgres.
• client_name (internal/db/store_oauth.go:289, server.go:2714) — server-assigned (db.PinnedClientName) rather than merely write-once, which closes the first-writer squat write-once alone left open. InsertPinnedClientReg keeps ON CONFLICT DO NOTHING for rows from an earlier build and refuses a non-pinned id; the in-memory path keeps the existing entry; the response echoes the stored value. I checked that no other call site can write a tgdcr_ row through the plain InsertClientReg upsert — both inserts have exactly one caller, inside handleClientRegistration.

Both P3s are handled as well: all three query sites use NOT starts_with(client_id, $2) (with tgdcrXnotpinned pinned in the store test), and validatePreregisteredClient refuses the reserved prefix at boot.

Re-verified on this commit: a registered pinned client with an unlisted redirect_uri fails closed in validateClient instead of falling through to the implicit path; removing an allowlist entry revokes it with no DB change; starts_with is reachable only on the Postgres path (sweepDB and useDB are both gated), so SQLite never sees it; the $1/$2 reuse binds correctly; boot validation is shared and fail-closed with the pre-registration messages unchanged.

P3s (non-blocking), two inline on the PR: pinned rows are exempt from both the sweep and the cap, so the permanent-row count grows with the subsets of OAUTH_DCR_REDIRECT_URIS (2^n-1) rather than its length — 3 rows as shipped, thousands if the list grows to one dashboard callback per portal-managed server; and the server-assigned client_name means the registration response carries metadata the client did not send (RFC 7591 §3.2.1 permits it), worth a line in docs/cloudflare-portal-compat.md.

Still open from last round: the CI "must not skip" guard at .github/workflows/build.yml:68 covers only ./internal/db/, so the new TestDCRAllowlist_PostgresPinnedWiring in internal/oauth can silently degrade to a skip. Adding ./internal/oauth/ to that go test line fixes it — I'm unable to edit workflow files (the GitHub App has no workflow write permission).

Approving review bound to 0322b89. I could not run go build, go vet or the test suite — those commands need approval in this environment — so this is a static review; the signatures the new Postgres test depends on (db.Open, db.Migrate, db.NewStore, newPostgresTestStore) were checked by hand.
· branch feat/dcr-portal-callbacks

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 0322b89.

No significant issues found.

[claude]

No P1/P2 findings (3 P3). Good to merge.

Both P2s from the previous round are properly fixed, and the fixes are the right shape rather than the minimum:

• Cap eviction (internal/oauth/server.go:2718) — the pinned registration now takes its own useDB branch with no EvictOldestClientRegIfOver call, matching the in-memory !pinned guard, and EvictOldestClientRegIfOver also excludes pinned rows from the count, not just from being the victim. TestDCRAllowlist_PostgresPinnedWiring covers the handler/store seam the finding fell through, on a real schema-isolated Postgres.
• client_name (internal/db/store_oauth.go:289, server.go:2714) — the name is now server-assigned (db.PinnedClientName) rather than merely write-once, which closes the first-writer squat that write-once alone left open. InsertPinnedClientReg keeps ON CONFLICT DO NOTHING as a guard for rows written by an earlier build and refuses a non-pinned client_id; the in-memory path keeps the existing entry; the response echoes the stored value. Checked that no other call site can write a tgdcr_ row through the plain InsertClientReg upsert — both inserts have exactly one caller, inside handleClientRegistration.

The P3s are also handled: all three query sites use NOT starts_with(client_id, $2) instead of a LIKE pattern with an unescaped _ (and the store test pins tgdcrXnotpinned being swept normally), and validatePreregisteredClient refuses the reserved prefix at boot.

Re-verified on this commit: the exact-match boundary still never widens AllowedImplicitHosts; a registered pinned client with an unlisted redirect_uri fails closed in validateClient rather than falling through to the implicit path; removing an allowlist entry revokes it with no DB change; starts_with is Postgres-only and sweepDB/useDB are both gated, so the SQLite path never reaches it; the $1/$2 reuse in the eviction query binds correctly; boot validation is shared through validateExactRedirectURI with the pre-registration messages unchanged.

Non-blocking P3s, two inline:

• pinned rows are exempt from both the sweep and the cap, so the permanent-row count grows with the subsets of OAUTH_DCR_REDIRECT_URIS (2^n-1), not its length — 3 rows for the two entries this ships with, thousands if the list grows to one dashboard callback per portal-managed server. Docs wording or a dedicated pinned cap.
• the server-assigned client_name means the registration response carries metadata the client did not send (RFC 7591 §3.2.1 permits it); worth a line in docs/cloudflare-portal-compat.md so a portal-side mismatch is not mistaken for an allowlist bug.
• still open from last round: the CI "must not skip" guard in .github/workflows/build.yml:68 covers only ./internal/db/, so the new TestDCRAllowlist_PostgresPinnedWiring in internal/oauth can silently degrade to a skip — adding ./internal/oauth/ to that go test line fixes it. I cannot edit workflow files myself.

I could not run go build, go vet or the tests in this environment (those commands need approval), so this is a static review; the signatures the new Postgres test depends on (db.Open, db.Migrate, db.NewStore, newPostgresTestStore) were checked by hand.