fix(mcp/oauthlogin): pin mcp login's callback path, keep the port ephemeral

[reyortiz3]

Summary

mecated mcp login <server> could never actually complete against a standards-compliant authorization server for its preregistered-confidential or CIMD profiles. Both of those client kinds commit to a redirect_uri that must be registered ahead of time. But runMCPLogin left the login runtime on its random-callback-path, ephemeral-port default (ADR 0112) — a fresh path and port on every single invocation. RFC 8252 §7.3 loopback matching (which a standards-compliant AS implements) only tolerates a varying port; the path must match exactly. So the presented redirect_uri could never match anything registered in advance, for either of those client kinds.

Update after rebase (2026-09-17): when this PR was opened, loadOAuthClient (internal/cliconfig/mcpprofile.go) accepted only preregistered/CIMD clients, never DCR — the sentence above and the original in-code comment said so accurately at the time. Direct MCP OAuth DCR (ADR 0325, Client.DCR, oauthlogin.AuthorizeWithCallbackPath) landed on main via #1413 while this PR sat open, so mecated mcp login now has a third client kind with its own, already-correct, registration-bound callback-path mechanism. The bug this PR fixes is unchanged and still live for the preregistered/CIMD case; see "Interaction with DCR" below for how the fix coexists with it.

This surfaced while wiring Connector Gateway's embedded auth server for CIMD interop with mecatl (stacklok/stacklok-enterprise-platform#3843): the connector-gateway side is ready, but a real mecated mcp login against it would still fail on this.

Fix (revised after review discussion)

An earlier version of this PR reused oauthlogin.ExactRedirectURL (mecatui's remote-login constant), which fixes both the callback path and the port (18473). On reflection that reintroduces exactly what issue #522's original design required avoiding: "use an unguessable callback path". A well-known, fixed port is squattable — another local process can pre-bind it before the legitimate login starts, hijacking or denying the real callback.

Only the path actually needs to be fixed for a preregistered/CIMD client's redirect_uri to be statically registerable. RFC 8252 §7.3 loopback matching — which every AS this command talks to already implements, since neither reachable client kind goes through DCR — ignores the port on both sides of the comparison. So this PR instead adds oauthlogin.Options.PinCallbackPath: it fixes the callback to a well-known path but keeps binding an ephemeral port (127.0.0.1:0), exactly like the existing random-path default already does. mecated mcp login now uses this instead of ExactRedirectURL.

Net effect:

• The redirect_uri path is now stable enough to pre-register in a CIMD document or a preregistered client.
• The port stays unpredictable per run, preserving the squatting resistance the original design wanted — an attacker still can't know which port to pre-bind.
• No port-contention regression either: concurrent mecated mcp login calls to different servers each get their own ephemeral port, same as before this PR.

oauthlogin.ExactRedirectURL itself is untouched and still available — cmd/mecatui/login.go's remote-login flow keeps using it, since a general-purpose OIDC target can't be assumed to implement RFC 8252 dynamic-port matching the way an MCP-shaped AS does.

Interaction with DCR (added on rebase)

cmd/mecated/mcplogin.go builds one shared oauthlogin.Runtime with PinCallbackPath: true before it knows which client kind selectMCPLoginServer returned, and a DCR login is routed through the same runtime's AuthorizeWithCallbackPath(ctx, issuer, callbackPath, ...) with its own registration-bound callbackPath. resolveCallbackMode's case order gives that explicit per-call callbackPath priority over PinCallbackPath, so a DCR login always presents the redirect_uri it actually registered, never the well-known /oauth/callback path — PinCallbackPath only ever takes effect for the preregistered/CIMD path (a plain Authorize call with no explicit callbackPath). Pinned by TestPinCallbackPathDoesNotOverrideRegistrationBoundPath.

Development stage

• Implementation — narrow, mechanism-level fix; no separate plan PR
• Human waiver of spine: Yes — directing human (this session) authorized skipping the Plan/Interface split, given the scope (one new Options field + a small Authorize refactor + a new test matrix, no new subsystem).

Contract linkage

• Work classification: Bounded (a targeted addition to an existing, accepted mechanism — not a new architectural decision)
• Classification rationale: oauthlogin.Options/ExactRedirectURL/the fixed-vs-random callback split already exist; this adds a third, narrower mode alongside them (PinCallbackPath) and one call site's choice of which mode to use. No new abstractions beyond that.
• Decision record: ADR 0112 (accepted) — its Consequences section already names "profiles compatible with random loopback ports and paths" as the runtime's known limitation. This PR extends the runtime to also support a profile that needs a fixed path while preserving the ephemeral port property ADR 0112's own security posture (and issue Runtime-owned browser and loopback login for MCP OAuth #522's "unguessable callback path" requirement) relied on. It doesn't reverse anything ADR 0112 decided; ExactRedirectURL's fully-fixed mode (introduced later, for mecatui) is also untouched.

Issue relationship

Relates to stacklok/stacklok-enterprise-platform#3843 (Connector Gateway ↔ mecatl CIMD interop). No mecatl-side issue was filed for this specific gap before this PR.

Type of change

• Bug fix

Test plan

• Rebased onto current main (post-feat(mcp): add direct MCP OAuth dynamic client registration #1413 DCR landing); git diff still scoped to the same 4 files
• Linting (task lint / golangci-lint run ./mcp/oauthlogin/... ./cmd/mecated/...) — 0 issues
• Offline test suite — targeted: go test ./cmd/mecated/... ./mcp/oauthlogin/... ./internal/app/... ./internal/cliconfig/... all pass
• Offline demo (go run ./cmd/mecademo) — not applicable
• Markdown changed — no
• User docs/user-facing behavior changed — no docs updated in this PR; flagging as a possible follow-up if mecated mcp login's docs mention the callback shape
• Guarded engine API affected — no, cmd/ and mcp/oauthlogin only
• Landed plan strict acceptance trace — no plan for this bug fix
• Panel review received and addressed (see PR comment) — the ship-blocker finding (PinCallbackPath vs. DCR's registration-bound path) was real post-rebase and is fixed via case-order priority, not by rejecting the combination

Changes

File	Change
mcp/oauthlogin/runtime.go	Extract fixedCallbackPath; add Options.PinCallbackPath (fixed path, ephemeral port); extract resolveCallbackMode to keep authorize's branch count under the gocyclo limit, with explicit per-call callbackPath (DCR) prioritized over PinCallbackPath; validate RedirectURL/PinCallbackPath are mutually exclusive
mcp/oauthlogin/runtime_test.go	Add TestPinCallbackPathUsesFixedPathEphemeralPort (proves path fixed, port varies across two runs; asserts against the literal path, not just the constant), TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts (mirrors the equivalent ExactRedirect* test), TestPinCallbackPathAndRedirectURLAreMutuallyExclusive, TestPinCallbackPathDoesNotOverrideRegistrationBoundPath (proves DCR's explicit callback path wins over PinCallbackPath)
cmd/mecated/mcplogin.go	Use PinCallbackPath: true instead of RedirectURL: ExactRedirectURL; updated the call-site comment for the now-three-way (preregistered/CIMD/DCR) client split
cmd/mecated/mcplogin_test.go	Update the one test that asserted the old (random-path) behavior to assert PinCallbackPath is forwarded instead

User-facing change

mecated mcp login <server> now binds its OAuth callback listener on a fixed, well-known path (/oauth/callback) instead of a random one, but the port is still chosen by the OS at bind time, same as before this change — no port pinning, no port-contention risk. Operators registering a preregistered client or publishing a CIMD document for a mecatl-facing server should use http://127.0.0.1/oauth/callback (no port, or any port — matching ignores it) as the redirect_uri.

Special notes for reviewers

• This PR went through two iterations in review: the first reused ExactRedirectURL (fixed path + fixed port), which a security-minded read flagged as reintroducing the port-squatting exposure issue Runtime-owned browser and loopback login for MCP OAuth #522 deliberately avoided. This version fixes only the path, keeping the port ephemeral, which should fully reconcile both concerns — please double check that reasoning holds.
• No new ADR was written; I read this as adding a narrower variant to ADR 0112's already-accepted mechanism, not a new architectural decision. Happy to write one if reviewers disagree with that framing.

🤖 Generated with Claude Code

[jhrozek]

Panel review — combined findings

Fixed point: origin/main (post-rebase)
Diff: 4 files, 205 insertions, 32 deletions, 2 commits

This combines an in-session four-axis panel review (Spec / Standards / Test adequacy / Domain) with an independently run second panel review (review-openai.md) that used a different spec source (issue #522) and an additional oauth-expert reviewer. Both reviews converged independently on the same root-cause defect.

---

Spec — does this implement what was asked?

Sources: PR description; Issue #522 (independently fetched by the other review)

• [blocker · Wrong, cross-confirmed by two independent spec reviews] PinCallbackPath silently overrides the registration-bound callback path that direct DCR relies on. mecated mcp login does support DCR (internal/cliconfig/mcpprofile.go's loadOAuthClient populates opts.Client.DCR) — the new comment in mcplogin.go claiming otherwise is incorrect. DCR calls AuthorizeWithCallbackPath with its registered path; resolveCallbackMode picks PinCallbackPath's /oauth/callback instead, violating the unguessable/registration-bound path requirement.
  cmd/mecated/mcplogin.go:191, mcp/oauthlogin/runtime.go (AuthorizeWithCallbackPath, resolveCallbackMode), internal/app/mcplogin.go:99
• [important · Missing] No end-to-end test proves the real mecated mcp login flow succeeds through actual redirect-URI matching — command tests stub executeMCPLogin, runtime tests invoke the callback directly; neither would have caught this regression.
• [advisory] The current case order makes the gap unreachable today only because the one PinCallbackPath caller never calls AuthorizeWithCallbackPath — nothing enforces that invariant going forward.

---

Standards — does this follow project conventions?

Sources read: AGENTS.md, ADR 0112, ADR 0325 (verified — Decision point 5: "Bind a stable random callback path to the registration and a fresh ephemeral IPv4 loopback port to each authorization")

• [blocker · Violation] The blanket PinCallbackPath: true wiring in mcplogin.go directly contradicts ADR 0325 §5's accepted decision for direct-DCR profiles — a documented, superseding-record-required violation, not just an implicit-convention gap.
  docs/adr/0325-direct-mcp-dcr.md:44-56; cmd/mecated/mcplogin.go:191, mcp/oauthlogin/runtime.go:365
• [important · Violation] For preregistered/CIMD profiles, this further diverges from ADR 0112's stated scope (random path and port) without a superseding ADR or an updated living-design note.
• [important] Independent of the ADR question: the file's own established convention is fail-closed, explicit mutual exclusion (as done for RedirectURL+PinCallbackPath) — leaving PinCallbackPath+callbackPath to silent case-order precedence breaks that internal consistency too.

---

Test adequacy — do the tests independently prove the contract?

• [blocker] TestPinCallbackPathUsesFixedPathEphemeralPort asserts strings.HasSuffix(redirect, fixedCallbackPath) — comparing against the same internal constant under test, not the external contract literal "/oauth/callback". A coordinated wrong-value change to the constant would still pass. (runtime_test.go:827)
• [important · cross-confirmed] No test combines PinCallbackPath with AuthorizeWithCallbackPath — exactly the gap that let the ship-blocker regression through untested. Found independently by both reviews.
• [important] The sequential-authorization test doesn't prove two logins can coexist concurrently, though it does already extract and diff two distinct ports across sequential calls.
• [advisory] No test guards that fixedCallbackPath ("/oauth/callback") and ToolHiveCompatibleRedirectURL's path ("/callback") stay distinct.

---

Domain — what do the specialists say?

Panel (union of both runs): secure-code-reviewer, software-architect, library-reuse-reviewer, code-duplication-reviewer, oauth-expert

Ship-blockers (1)

• [blocker · High, cross-confirmed across two independent panel runs] Same DCR-override defect, from the security/architecture lens: resolveCallbackMode's case order lets PinCallbackPath win over an explicit, registration-bound callbackPath, and AuthorizeWithCallbackPath only guards against RedirectURL, not PinCallbackPath. RFC 6749 §3.1.2.3 / RFC 7591 exact-match enforcement then rejects the login (fails closed), or on a lenient AS silently breaks the DCR contract.
  Suggested fix: reject PinCallbackPath in AuthorizeWithCallbackPath (mirroring its existing RedirectURL guard), and/or give an explicit callbackPath priority over PinCallbackPath in the switch; only pin the callback path at the mecplogin.go call site for non-DCR (server.OAuth.Client.DCR == nil) servers. Correct the inaccurate "never DCR" comment.
  This exact ordering was introduced by the manual rebase-conflict resolution — it existed in neither original commit.

Judgement calls (1)

• [important · Medium] RFC 8252's variable-port loopback exception is a native-app pattern, not guaranteed for every MCP authorization server; CIMD metadata may in some cases require exact redirect-string matching. Consider constraining which profiles/servers PinCallbackPath applies to, backed by a real-authorization-server test. (oauth-expert)

Advisory / Polish (5)

• Stale doc comment: resolveCallbackMode claims its cases "mirror Options.RedirectURL/PinCallbackPath's doc comments exactly" — no longer true with the third, non-Options-driven case.
• The resolveCallbackMode extraction is justified only by a gocyclo comment — if that's the only reason, inlining may be more readable than a 6-return-value function.
• RedirectURL and PinCallbackPath cases both set attemptPolicy = attemptFixedRoute — minor shrink candidate, not worth blocking on.
• The RFC 8252 rationale is repeated three times (struct doc, switch-case comment, mcplogin.go call-site comment); the 13-line call-site comment could shrink to 2–3 lines pointing at the type doc.
• TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts is near-byte-identical to the existing TestExactRedirect... flood test — Rule of Three not yet met (2 sites); extract a shared helper only if a third variant appears.

---

Summary

• Spec axis: 3 findings (1 blocker, 1 important, 1 advisory)
• Standards axis: 3 findings (1 blocker, 2 important)
• Test adequacy axis: 4 findings (1 blocker, 2 important, 1 advisory)
• Domain axis: 1 blocker (cross-confirmed across two independent panel runs), 1 important (judgement call), 5 advisory

Most important single issue: the branch currently breaks direct MCP DCR login by having PinCallbackPath silently override its ADR-0325-mandated, registration-bound callback path — independently found by every axis in both review runs. This is a real correctness/security regression, introduced specifically by the manual git-rebase conflict resolution (not present in either original commit against the pre-rebase base), and should be fixed before merge.

PANEL: ship_blockers=4 important=6 advisory=7 reviewer_failures=0

🤖 Generated with Claude Code

[reyortiz3]

Rebased onto current main and re-verified the premise still holds (task lint + targeted task test green; full diff now 4 files as before, no scope creep).

On the panel review's ship-blocker

The review's core finding — that PinCallbackPath could silently override a DCR client's registration-bound callback path — turned out to be real, but only after the rebase, not in the branch as originally opened. When this PR was opened, direct MCP DCR (internal/cliconfig/mcpprofile.go's Client.DCR arm, oauthlogin.AuthorizeWithCallbackPath, ADR 0325) didn't exist on main yet; it landed via #1413 while this PR sat open. So the original PR body's claim "loadOAuthClient accepts exactly a preregistered confidential client or a CIMD client, never DCR" was accurate at the time and is now stale — I've corrected the comment in cmd/mecated/mcplogin.go to describe the current three-way split instead of asserting DCR is unreachable.

Rather than rejecting PinCallbackPath whenever a call is DCR-routed (the review's first suggested fix), which would break the very DCR interop this was written to unblock — cmd/mecated/mcplogin.go builds one shared oauthlogin.Runtime with PinCallbackPath: true up front, before it knows which client kind selectMCPLoginServer returned, and DCR logins always go through LoginMCPWithOptions → runtime.AuthorizeWithCallbackPath on that same runtime — I gave the explicit per-call callbackPath (DCR's own registration-bound path) priority over PinCallbackPath in resolveCallbackMode's case order:

RedirectURL (exact port+path)  >  explicit callbackPath (DCR)  >  PinCallbackPath  >  random default

This is exactly the review's second suggested fix ("give an explicit callbackPath priority over PinCallbackPath"), and it's the one that doesn't regress DCR. Added TestPinCallbackPathDoesNotOverrideRegistrationBoundPath in mcp/oauthlogin/runtime_test.go to pin it: constructs a Runtime with PinCallbackPath: true, drives it through AuthorizeWithCallbackPath with a registration-bound path, and asserts the actual redirect uses that path, not /oauth/callback.

Other findings addressed

• Test asserting against the internal constant, not the external literal (TestPinCallbackPathUsesFixedPathEphemeralPort): now asserts strings.HasSuffix(redirect, "/oauth/callback") directly.
• Stale resolveCallbackMode doc comment ("the three cases mirror..."): rewritten to describe the actual (now four-way) case order and why DCR's explicit path wins.
• Left as advisory / not worth a change, per the review's own severity: the near-duplicate flood tests (Rule of Three not met — 2 sites, not 3), and the shared attemptFixedRoute assignment across two cases (real but trivial).

Still relevant?

Yes. Confirmed on rebased main: cmd/mecated/mcplogin.go's runMCPLogin still builds oauthlogin.Options{NoBrowser: parsed.noBrowser} with no path pinning for the non-DCR (preregistered/CIMD) path, so the original bug — a random callback path can never match a redirect_uri fixed in advance at either of those client kinds — is still live on main today. DCR landing separately didn't fix it; it only added a third client kind with its own (already-correct) callback-path mechanism.