From 2a3ee548cba550fd7c2a594232ba36d36ca8474f Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Thu, 10 Sep 2026 17:26:36 -0400 Subject: [PATCH 1/2] fix(cmd/mecated): pin mcp login's OAuth callback to the fixed redirect Every server reachable through `mecated mcp login` already commits to a client identity that must be registered ahead of time: selectMCPLoginServer only returns servers whose OAuth was populated by loadOAuthClient, which accepts exactly a preregistered confidential client or a CIMD client, never DCR. Leaving oauthlogin.Options.RedirectURL empty put every such login on the runtime's random-path, ephemeral-port default (ADR 0112) instead, so the presented redirect_uri never matched what either client kind had registered -- the login could never actually succeed against a standards-compliant authorization server enforcing RFC 8252 loopback matching (which tolerates only a varying port, never a varying path). Set RedirectURL to the existing ExactRedirectURL constant instead, the same fixed callback cmd/mecatui/login.go's remote-login path already uses. Renamed and updated the one test that explicitly locked in the old random-path behavior. Co-Authored-By: Claude Sonnet 5 --- cmd/mecated/mcplogin.go | 9 ++++++++- cmd/mecated/mcplogin_test.go | 6 +++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/cmd/mecated/mcplogin.go b/cmd/mecated/mcplogin.go index 352c694c1..843971558 100644 --- a/cmd/mecated/mcplogin.go +++ b/cmd/mecated/mcplogin.go @@ -149,7 +149,14 @@ func runMCPLogin(args []string, stdout io.Writer) error { return err } - opts := oauthlogin.Options{NoBrowser: parsed.noBrowser} + // RedirectURL is pinned to the fixed callback (ADR 0112's ExactRedirectURL), not left + // to the runtime's random-path/ephemeral-port default: selectMCPLoginServer only ever + // returns a server whose OAuth was populated by loadOAuthClient, which accepts exactly + // a preregistered confidential client or a CIMD client — never DCR (mcpprofile.go) — so + // every server reachable here already commits to a client identity (a preregistered + // client_id, or a CIMD document's declared redirect_uris) that must be registered + // ahead of time. A random callback path can never match a value fixed in advance. + opts := oauthlogin.Options{NoBrowser: parsed.noBrowser, RedirectURL: oauthlogin.ExactRedirectURL} if parsed.noBrowser { opts.URLWriter = stdout } diff --git a/cmd/mecated/mcplogin_test.go b/cmd/mecated/mcplogin_test.go index 851e4058b..7a218c5d8 100644 --- a/cmd/mecated/mcplogin_test.go +++ b/cmd/mecated/mcplogin_test.go @@ -190,7 +190,7 @@ func TestMCPLoginUsesExplicitOperatorPrecedence(t *testing.T) { } } -func TestRunMCPLoginExecutionPathUsesRandomCallback(t *testing.T) { +func TestRunMCPLoginExecutionPathUsesFixedCallback(t *testing.T) { key := base64.StdEncoding.EncodeToString(make([]byte, 32)) t.Setenv("MECATL_LOGIN_KEY", key) t.Setenv("MECATL_LOGIN_CREDENTIAL", base64.StdEncoding.EncodeToString([]byte("opaque"))) @@ -207,8 +207,8 @@ func TestRunMCPLoginExecutionPathUsesRandomCallback(t *testing.T) { if server.Name != "GitHub" || server.OAuth == nil || server.OAuth.CredentialStore == nil || server.OAuth.CredentialReader != nil { t.Fatalf("selected server = %#v", server) } - if !opts.NoBrowser || opts.URLWriter == nil || opts.RedirectURL != "" { - t.Fatalf("runtime options = %#v; no-browser or random-path default was not forwarded", opts) + if !opts.NoBrowser || opts.URLWriter == nil || opts.RedirectURL != oauthlogin.ExactRedirectURL { + t.Fatalf("runtime options = %#v; no-browser or fixed-callback redirect was not forwarded", opts) } return nil } From dbe04357ca779ea58bda9d8eeff7961aea5026d1 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Thu, 10 Sep 2026 18:27:02 -0400 Subject: [PATCH 2/2] fix(mcp/oauthlogin): pin only the callback path, not the port The previous commit reused ExactRedirectURL for mecated mcp login, which fixes BOTH the callback path and the port. That reintroduces exactly the local port-squatting exposure issue #522's original design required avoiding ("use an unguessable callback path"): a well-known port is predictable and can be pre-bound by another local process 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 SS7.3 loopback matching -- which every authorization server this login talks to already implements, since neither client kind is reachable through DCR -- ignores the port on both sides of the comparison. So the port can and should stay ephemeral, exactly as the random-path default already does; only the path needs to stop being randomly generated per invocation. Add Options.PinCallbackPath: fixes the callback to fixedCallbackPath while still binding "127.0.0.1:0" (OS-assigned port), extracted alongside ExactRedirectURL's fully-fixed mode into resolveCallbackMode to keep Authorize's branch count under the gocyclo limit. ExactRedirectURL itself is untouched and stays available for a target that genuinely needs an exact string match (e.g. cmd/mecatui/login.go's remote OIDC login, which cannot assume its target implements RFC 8252 dynamic-port matching). mecated mcp login now uses PinCallbackPath instead. This also removes the port-contention regression the previous commit introduced: concurrent `mecated mcp login` calls to different servers each get their own ephemeral port again, same as before either commit. Co-Authored-By: Claude Sonnet 5 --- cmd/mecated/mcplogin.go | 21 +++--- cmd/mecated/mcplogin_test.go | 4 +- mcp/oauthlogin/runtime.go | 90 ++++++++++++++++++++------ mcp/oauthlogin/runtime_test.go | 114 +++++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 31 deletions(-) diff --git a/cmd/mecated/mcplogin.go b/cmd/mecated/mcplogin.go index 843971558..f467f98cf 100644 --- a/cmd/mecated/mcplogin.go +++ b/cmd/mecated/mcplogin.go @@ -149,14 +149,19 @@ func runMCPLogin(args []string, stdout io.Writer) error { return err } - // RedirectURL is pinned to the fixed callback (ADR 0112's ExactRedirectURL), not left - // to the runtime's random-path/ephemeral-port default: selectMCPLoginServer only ever - // returns a server whose OAuth was populated by loadOAuthClient, which accepts exactly - // a preregistered confidential client or a CIMD client — never DCR (mcpprofile.go) — so - // every server reachable here already commits to a client identity (a preregistered - // client_id, or a CIMD document's declared redirect_uris) that must be registered - // ahead of time. A random callback path can never match a value fixed in advance. - opts := oauthlogin.Options{NoBrowser: parsed.noBrowser, RedirectURL: oauthlogin.ExactRedirectURL} + // PinCallbackPath fixes the callback's path (not its port): selectMCPLoginServer + // only ever returns a server whose OAuth was populated by loadOAuthClient, which + // accepts exactly a preregistered confidential client or a CIMD client — never + // DCR (mcpprofile.go) — so every server reachable here already commits to a + // client identity that must be registered ahead of time, and a random callback + // path can never match a value fixed in advance. Both client kinds only ever + // target MCP-shaped, RFC 8252-aware authorization servers, which accept any port + // for a registered loopback redirect_uri as long as the path matches — so unlike + // oauthlogin.ExactRedirectURL (a fully fixed callback, port included, for a + // general-purpose OIDC target that cannot be assumed to implement RFC 8252 + // dynamic-port matching), this login still gets an unpredictable port every run, + // preserving the squatting resistance a foreseeable port would give up. + opts := oauthlogin.Options{NoBrowser: parsed.noBrowser, PinCallbackPath: true} if parsed.noBrowser { opts.URLWriter = stdout } diff --git a/cmd/mecated/mcplogin_test.go b/cmd/mecated/mcplogin_test.go index 7a218c5d8..60ec1d261 100644 --- a/cmd/mecated/mcplogin_test.go +++ b/cmd/mecated/mcplogin_test.go @@ -207,8 +207,8 @@ func TestRunMCPLoginExecutionPathUsesFixedCallback(t *testing.T) { if server.Name != "GitHub" || server.OAuth == nil || server.OAuth.CredentialStore == nil || server.OAuth.CredentialReader != nil { t.Fatalf("selected server = %#v", server) } - if !opts.NoBrowser || opts.URLWriter == nil || opts.RedirectURL != oauthlogin.ExactRedirectURL { - t.Fatalf("runtime options = %#v; no-browser or fixed-callback redirect was not forwarded", opts) + if !opts.NoBrowser || opts.URLWriter == nil || !opts.PinCallbackPath || opts.RedirectURL != "" { + t.Fatalf("runtime options = %#v; no-browser or pinned-callback-path was not forwarded", opts) } return nil } diff --git a/mcp/oauthlogin/runtime.go b/mcp/oauthlogin/runtime.go index 28d2d4628..dd05227ed 100644 --- a/mcp/oauthlogin/runtime.go +++ b/mcp/oauthlogin/runtime.go @@ -21,10 +21,18 @@ const ( callbackBytes = 32 shutdownTimeout = time.Second + // fixedCallbackPath is the well-known, pre-registerable callback path shared by + // ExactRedirectURL (fixed path + fixed port) and Options.PinCallbackPath (fixed + // path + ephemeral port) — see PinCallbackPath's doc comment for why a target's + // own capabilities decide which of the two a caller should use. + fixedCallbackPath = "/oauth/callback" + // ExactRedirectURL is the fixed callback URI used by clients that have a - // pre-registered redirect. It is deliberately IPv4-literal and must not be + // pre-registered redirect requiring an EXACT string match, port included (a + // general-purpose OIDC target with no obligation to implement RFC 8252 loopback + // dynamic-port matching). It is deliberately IPv4-literal and must not be // changed to localhost or a wildcard address. - ExactRedirectURL = "http://127.0.0.1:18473/oauth/callback" + ExactRedirectURL = "http://127.0.0.1:18473" + fixedCallbackPath ) var ( @@ -69,10 +77,25 @@ type Options struct { URLWriter io.Writer Launcher BrowserLauncher - // RedirectURL enables an explicitly configured callback. The only accepted - // value is ExactRedirectURL; empty preserves the random-path, ephemeral-port - // behavior used by existing callers. + // RedirectURL enables an explicitly configured callback with a FIXED PORT as + // well as a fixed path. The only accepted value is ExactRedirectURL. Use this + // only for a target that requires an exact redirect_uri string match (a + // general-purpose OIDC target with no obligation to implement RFC 8252 loopback + // dynamic-port matching) — it reintroduces local port-squatting exposure that + // PinCallbackPath does not (see its doc comment). Mutually exclusive with + // PinCallbackPath. Empty preserves the random-path, ephemeral-port default. RedirectURL string + + // PinCallbackPath fixes the callback's PATH to the same well-known value + // ExactRedirectURL uses, while still binding an EPHEMERAL port. Use this for a + // target whose authorization server implements RFC 8252 §7.3 loopback dynamic- + // port matching — the AS accepts any port for a registered loopback redirect_uri + // as long as the path matches — which lets a client register one fixed + // redirect_uri while every login still gets its own unpredictable port, + // preserving the squatting resistance ExactRedirectURL's fixed port gives up. + // Mutually exclusive with RedirectURL: New rejects setting both rather than + // silently picking one. + PinCallbackPath bool } // Result is the validated loopback authorization response. @@ -104,6 +127,9 @@ func New(opts Options) (*Runtime, error) { if opts.RedirectURL != "" && !isExactRedirectURL(opts.RedirectURL) { return nil, errors.New("OAuth redirect URL is invalid") } + if opts.RedirectURL != "" && opts.PinCallbackPath { + return nil, errors.New("OAuth redirect URL and pinned callback path are mutually exclusive") + } launcher := opts.Launcher if launcher == nil { launcher = systemBrowserLauncher{} @@ -142,22 +168,9 @@ func (r *Runtime) Authorize(ctx context.Context, expectedIssuer string, authoriz if err := ctx.Err(); err != nil { return err } - path := "" - address := "127.0.0.1:0" - callbackHost := "" - redirectURL := "" - attemptPolicy := attemptMatchingRoute - if r.opts.RedirectURL == "" { - path, err = randomCallbackPath(r.random) - if err != nil { - return errors.New("generate OAuth callback path: failed") - } - } else { - path = "/oauth/callback" - address = "127.0.0.1:18473" - callbackHost = "127.0.0.1:18473" - redirectURL = ExactRedirectURL - attemptPolicy = attemptFixedRoute + path, address, callbackHost, redirectURL, attemptPolicy, err := resolveCallbackMode(r.opts, r.random) + if err != nil { + return err } ln, err := r.listen(ctx, "tcp4", address) if err != nil { @@ -286,6 +299,41 @@ func randomCallbackPath(reader io.Reader) (string, error) { return callbackPrefix + base64.RawURLEncoding.EncodeToString(raw[:]), nil } +// resolveCallbackMode picks the callback path, bind address, Host-header match, any +// pre-computed redirect URL, and attempt policy for one Authorize call. Extracted from +// Authorize purely to keep that function's branch count under the gocyclo limit; the +// three cases mirror Options.RedirectURL/PinCallbackPath's doc comments exactly. +func resolveCallbackMode(opts Options, random io.Reader) ( + path, address, callbackHost, redirectURL string, attemptPolicy callbackAttemptPolicy, err error, +) { + address = "127.0.0.1:0" + switch { + case opts.RedirectURL != "": + // Fixed path AND fixed port: an exact redirect_uri string match, for a + // target with no obligation to implement RFC 8252 dynamic-port matching. + path = fixedCallbackPath + address = "127.0.0.1:18473" + callbackHost = "127.0.0.1:18473" + redirectURL = ExactRedirectURL + attemptPolicy = attemptFixedRoute + case opts.PinCallbackPath: + // Fixed path, ephemeral port: address/callbackHost/redirectURL stay at their + // random-port defaults — computed by the caller from the OS-assigned + // listener address, exactly like the random-path case below — so only the + // path is pinned; an RFC 8252-compliant AS's dynamic-port matching accepts + // whichever port this run happens to get. + path = fixedCallbackPath + attemptPolicy = attemptFixedRoute + default: + attemptPolicy = attemptMatchingRoute + path, err = randomCallbackPath(random) + if err != nil { + return "", "", "", "", 0, errors.New("generate OAuth callback path: failed") + } + } + return path, address, callbackHost, redirectURL, attemptPolicy, nil +} + func isExactRedirectURL(raw string) bool { return raw == ExactRedirectURL } diff --git a/mcp/oauthlogin/runtime_test.go b/mcp/oauthlogin/runtime_test.go index cfa250239..19eae45d9 100644 --- a/mcp/oauthlogin/runtime_test.go +++ b/mcp/oauthlogin/runtime_test.go @@ -719,6 +719,120 @@ func TestExactRedirectValidationIsStrict(t *testing.T) { } } +// TestPinCallbackPathUsesFixedPathEphemeralPort proves PinCallbackPath fixes only the +// callback PATH: two successive Authorize calls both land on fixedCallbackPath, but get +// different ports, since RFC 8252 dynamic-port matching (which the client this option is +// for already relies on) never checks the port. +func TestPinCallbackPathUsesFixedPathEphemeralPort(t *testing.T) { + var redirect string + runtime, err := New(Options{ + PinCallbackPath: true, + Launcher: launcherFunc(func(_ context.Context, _ string) error { + valid, _ := http.NewRequest(http.MethodGet, callbackURL(redirect, "good", "s", testIssuer), nil) + if got := request(t, valid).status; got != http.StatusOK { + t.Fatalf("valid status = %d", got) + } + return nil + }), + }) + if err != nil { + t.Fatal(err) + } + + authorizeOnce := func() string { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err := runtime.Authorize(ctx, testIssuer, func(ctx context.Context, got string, present func(context.Context, string) (Result, error)) error { + redirect = got + result, err := present(ctx, "https://as.example.test/authorize?state=s") + if err != nil { + return err + } + if result.Code != "good" { + t.Fatalf("result = %#v", result) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + return redirect + } + + first := authorizeOnce() + second := authorizeOnce() + + for _, redirect := range []string{first, second} { + if !strings.HasSuffix(redirect, fixedCallbackPath) { + t.Fatalf("redirect = %q, want suffix %q", redirect, fixedCallbackPath) + } + } + firstPort := strings.TrimSuffix(strings.TrimPrefix(first, "http://127.0.0.1:"), fixedCallbackPath) + secondPort := strings.TrimSuffix(strings.TrimPrefix(second, "http://127.0.0.1:"), fixedCallbackPath) + if firstPort == "" || secondPort == "" { + t.Fatalf("could not extract ports from %q, %q", first, second) + } + if firstPort == secondPort { + t.Fatalf("both authorizations bound the same port %q; PinCallbackPath must not fix the port", firstPort) + } +} + +// TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts mirrors +// TestExactRedirectUnauthenticatedFloodDoesNotSpendAttempts: a pinned path is public and +// pre-registered exactly like ExactRedirectURL, so it must get the same attemptFixedRoute +// policy (ambient probes never exhaust the attempt budget). +func TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts(t *testing.T) { + var redirect string + runtime, err := New(Options{ + PinCallbackPath: true, + Launcher: launcherFunc(func(_ context.Context, _ string) error { + for i := range 2 * maxRequestAttempts { + var req *http.Request + switch i % 3 { + case 0: + req, _ = http.NewRequest(http.MethodGet, callbackURL(redirect, "probe", "wrong-state", testIssuer), nil) + case 1: + req, _ = http.NewRequest(http.MethodPost, redirect+"?state=wrong-state", nil) + default: + req, _ = http.NewRequest(http.MethodGet, redirect+"?state=%zz", nil) + } + if got := request(t, req).status; got < 400 { + t.Fatalf("probe %d status = %d", i, got) + } + } + valid, _ := http.NewRequest(http.MethodGet, callbackURL(redirect, "good", "s", testIssuer), nil) + if got := request(t, valid).status; got != http.StatusOK { + t.Fatalf("valid status after pinned-path flood = %d", got) + } + return nil + }), + }) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err = runtime.Authorize(ctx, testIssuer, func(ctx context.Context, got string, present func(context.Context, string) (Result, error)) error { + redirect = got + result, err := present(ctx, "https://as.example.test/authorize?state=s") + if err == nil && result.Code != "good" { + t.Fatalf("result = %#v", result) + } + return err + }) + if err != nil { + t.Fatal(err) + } +} + +// TestPinCallbackPathAndRedirectURLAreMutuallyExclusive proves New refuses the +// nonsensical combination rather than silently preferring one or the other. +func TestPinCallbackPathAndRedirectURLAreMutuallyExclusive(t *testing.T) { + if _, err := New(Options{RedirectURL: ExactRedirectURL, PinCallbackPath: true}); err == nil { + t.Fatal("accepted RedirectURL and PinCallbackPath set together") + } +} + func TestCancellationWhileWaitingForCallback(t *testing.T) { started := make(chan struct{}) var redirect string