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

cmd/mecated/mcplogin.go

@@ -149,7 +149,19 @@ func runMCPLogin(args []string, stdout io.Writer) error {
 		return err
 	}
 
-	opts := oauthlogin.Options{NoBrowser: parsed.noBrowser}
+	// 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
 	}

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.PinCallbackPath || opts.RedirectURL != "" {
+			t.Fatalf("runtime options = %#v; no-browser or pinned-callback-path was not forwarded", opts)
 		}
 		return nil
 	}

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
 }

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