diff --git a/cmd/mecated/mcplogin.go b/cmd/mecated/mcplogin.go index 32bd456a1e..753b3ebd0c 100644 --- a/cmd/mecated/mcplogin.go +++ b/cmd/mecated/mcplogin.go @@ -267,7 +267,25 @@ 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} + // 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 d5a9e2578d..485db4bc5b 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.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