From 40355c23a79b649d1399f25acea384e206ade2b1 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 32bd456a1e..c459d14760 100644 --- a/cmd/mecated/mcplogin.go +++ b/cmd/mecated/mcplogin.go @@ -267,7 +267,14 @@ func runMCPLoginContext(ctx context.Context, args []string, stdout io.Writer, st } _, _ = fmt.Fprintln(stdout, "MCP onboarding: preparing browser authorization") - 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 d5a9e2578d..683cd6541b 100644 --- a/cmd/mecated/mcplogin_test.go +++ b/cmd/mecated/mcplogin_test.go @@ -261,7 +261,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"))) @@ -278,8 +278,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) } if loginOpts.DCRAction != mcp.OAuthDCRLoginRetryRegistration { t.Fatalf("login options = %#v", loginOpts) From da145b02e927f318997bacf6949f7956f26eab81 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 | 27 ++++-- cmd/mecated/mcplogin_test.go | 4 +- mcp/oauthlogin/runtime.go | 102 +++++++++++++++------ mcp/oauthlogin/runtime_test.go | 159 +++++++++++++++++++++++++++++++++ 4 files changed, 256 insertions(+), 36 deletions(-) diff --git a/cmd/mecated/mcplogin.go b/cmd/mecated/mcplogin.go index c459d14760..753b3ebd0c 100644 --- a/cmd/mecated/mcplogin.go +++ b/cmd/mecated/mcplogin.go @@ -267,14 +267,25 @@ func runMCPLoginContext(ctx context.Context, args []string, stdout io.Writer, st } _, _ = fmt.Fprintln(stdout, "MCP onboarding: preparing browser authorization") - // 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) for the preregistered + // and CIMD client kinds loadOAuthClient (mcpprofile.go) populates directly onto + // opts.Client: both commit to a redirect_uri that must be registered ahead of + // time, and a random callback path can never match a value fixed in advance. + // Both 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. + // + // A DCR-configured server (opts.Client.DCR) never reaches this fixed path: it + // registers its own random redirect_uri at registration time and is driven + // through LoginMCPWithOptions's AuthorizeWithCallbackPath call instead, whose + // explicit registration-bound callback path always takes priority over + // PinCallbackPath (see oauthlogin.resolveCallbackMode's case order) — so setting + // PinCallbackPath here unconditionally is safe for every client kind + // selectMCPLoginServer can return. + 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 683cd6541b..485db4bc5b 100644 --- a/cmd/mecated/mcplogin_test.go +++ b/cmd/mecated/mcplogin_test.go @@ -278,8 +278,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) } if loginOpts.DCRAction != mcp.OAuthDCRLoginRetryRegistration { t.Fatalf("login options = %#v", loginOpts) diff --git a/mcp/oauthlogin/runtime.go b/mcp/oauthlogin/runtime.go index 2537f50e71..3e3b09c7d2 100644 --- a/mcp/oauthlogin/runtime.go +++ b/mcp/oauthlogin/runtime.go @@ -22,10 +22,16 @@ 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 remote mecatui login. // 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 // ToolHiveCompatibleRedirectURL is the fixed callback URI used by native // LLM login so an existing ToolHive-compatible client registration works. @@ -74,10 +80,30 @@ type Options struct { URLWriter io.Writer Launcher BrowserLauncher - // RedirectURL enables an explicitly configured callback. Only the package's - // fixed redirect constants are accepted; 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. Only the package's fixed redirect constants + // (ExactRedirectURL, ToolHiveCompatibleRedirectURL) are accepted. Use this only + // for a target that requires an exact redirect_uri string match (a + // general-purpose OIDC or ToolHive-compatible 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 a fixed port gives up. Mutually exclusive + // with RedirectURL: New rejects setting both rather than silently picking one. + // A per-call explicit callback path (AuthorizeWithCallbackPath, used for DCR's + // own registration-bound path) always takes priority over this option, since + // that path — not this well-known one — is what a DCR client actually + // registered; see resolveCallbackMode's case order. + PinCallbackPath bool } // Result is the validated loopback authorization response. @@ -111,6 +137,9 @@ func New(opts Options) (*Runtime, error) { 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{} @@ -140,7 +169,7 @@ func (r *Runtime) AuthorizeWithCallbackPath(ctx context.Context, expectedIssuer, return r.authorize(ctx, expectedIssuer, callbackPath, authorize) } -func (r *Runtime) authorize(ctx context.Context, expectedIssuer, callbackPath string, authorize AuthorizeFunc) error { //nolint:gocyclo // callback lifecycle and cleanup states stay explicit. +func (r *Runtime) authorize(ctx context.Context, expectedIssuer, callbackPath string, authorize AuthorizeFunc) error { if ctx == nil { return errors.New("OAuth authorization requires a context") } @@ -162,26 +191,9 @@ func (r *Runtime) authorize(ctx context.Context, expectedIssuer, callbackPath st if err := ctx.Err(); err != nil { return err } - path := "" - address := "127.0.0.1:0" - callbackHost := "" - redirectURL := "" - attemptPolicy := attemptMatchingRoute - if r.opts.RedirectURL == "" { - path = callbackPath - if path == "" { - path, err = randomCallbackPath(r.random) - if err != nil { - return errors.New("generate OAuth callback path: failed") - } - } - } else { - fixed, _ := fixedRedirect(r.opts.RedirectURL) - path = fixed.path - address = fixed.address - callbackHost = fixed.host - redirectURL = r.opts.RedirectURL - attemptPolicy = attemptFixedRoute + path, address, callbackHost, redirectURL, attemptPolicy, err := resolveCallbackMode(r.opts, callbackPath, r.random) + if err != nil { + return err } ln, err := r.listen(ctx, "tcp4", address) if err != nil { @@ -319,7 +331,7 @@ type fixedRedirectConfig struct { func fixedRedirect(raw string) (fixedRedirectConfig, bool) { switch raw { case ExactRedirectURL: - return fixedRedirectConfig{address: "127.0.0.1:18473", host: "127.0.0.1:18473", path: "/oauth/callback"}, true + return fixedRedirectConfig{address: "127.0.0.1:18473", host: "127.0.0.1:18473", path: fixedCallbackPath}, true case ToolHiveCompatibleRedirectURL: return fixedRedirectConfig{address: "localhost:8666", host: "localhost:8666", path: "/callback"}, true default: @@ -336,6 +348,44 @@ func validCallbackPath(path string) bool { return err == nil && len(raw) == callbackBytes && base64.RawURLEncoding.EncodeToString(raw) == encoded } +// resolveCallbackMode picks the callback path, bind address, Host-header match, any +// pre-computed redirect URL, and attempt policy for one authorize call. Extracted +// purely to keep authorize's branch count under the gocyclo limit. Priority order +// matters: a fixed Options.RedirectURL (exact port+path match, for a target with no +// RFC 8252 loopback support) wins first; an explicit per-call callbackPath (from +// AuthorizeWithCallbackPath — DCR's own registration-bound path) wins second, ahead +// of PinCallbackPath, because that path — not the well-known PinCallbackPath one — +// is what a DCR client actually registered with the authorization server; +// PinCallbackPath (fixed path, ephemeral port) is third; a fresh random path with an +// ephemeral port is the default when none of the above apply. +func resolveCallbackMode(opts Options, callbackPath string, random io.Reader) ( + path, address, callbackHost, redirectURL string, attemptPolicy callbackAttemptPolicy, err error, +) { + address = "127.0.0.1:0" + switch { + case opts.RedirectURL != "": + fixed, _ := fixedRedirect(opts.RedirectURL) + path = fixed.path + address = fixed.address + callbackHost = fixed.host + redirectURL = opts.RedirectURL + attemptPolicy = attemptFixedRoute + case callbackPath != "": + path = callbackPath + attemptPolicy = attemptFixedRoute + case opts.PinCallbackPath: + 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 canonicalIssuer(raw string) (string, error) { u, err := url.Parse(raw) if err != nil || u.Scheme == "" || u.Host == "" || u.User != nil || u.RawQuery != "" || u.Fragment != "" || (u.Scheme != "http" && u.Scheme != "https") { diff --git a/mcp/oauthlogin/runtime_test.go b/mcp/oauthlogin/runtime_test.go index 175a396d82..7de99b0f43 100644 --- a/mcp/oauthlogin/runtime_test.go +++ b/mcp/oauthlogin/runtime_test.go @@ -781,6 +781,165 @@ func TestFixedRedirectValidationIsStrict(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} { + // Assert against the literal, not just fixedCallbackPath: a coordinated wrong + // change to that constant must still fail this test. + if !strings.HasSuffix(redirect, "/oauth/callback") { + t.Fatalf("redirect = %q, want suffix %q", redirect, "/oauth/callback") + } + } + 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") + } +} + +// TestPinCallbackPathDoesNotOverrideRegistrationBoundPath proves a DCR client's +// own registration-bound callback path (AuthorizeWithCallbackPath) always wins over +// a Runtime-wide PinCallbackPath, since mecated shares one Runtime across every +// client kind selectMCPLoginServer can return: were PinCallbackPath to win, a DCR +// login would present a redirect_uri the authorization server never registered. +func TestPinCallbackPathDoesNotOverrideRegistrationBoundPath(t *testing.T) { + path := callbackPrefix + base64.RawURLEncoding.EncodeToString(bytes.Repeat([]byte{9}, callbackBytes)) + 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) + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err = runtime.AuthorizeWithCallbackPath(ctx, testIssuer, path, 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) + } + parsed, parseErr := url.Parse(redirect) + if parseErr != nil || parsed.Path != path { + t.Fatalf("redirect = %q, want registration-bound path %q despite PinCallbackPath", redirect, path) + } +} + func TestCancellationWhileWaitingForCallback(t *testing.T) { started := make(chan struct{}) var redirect string