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

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
 	}

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)

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") {

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