fix(oauth): validate discovered endpoints before merge/use (#511)

Discovery metadata was merged without the https/loopback endpoint rule that
configured endpoints already pass, letting a malicious issuer downgrade the
authorization/token/device/registration endpoint after config validation.

- Add exported ValidateEndpointURL as the single endpoint-safety rule;
  validateTokenEndpoint now delegates to it.
- Backstop in BuildAuthorizationURL (the choke point both the provider and MCP
  flows build their browser URL through).
- Validate each discovered endpoint before merge in the provider resolveEndpoints
  and validate resolved MCP metadata in both branches; fail closed on insecure.

AGENTS.md

@@ -1,71 +1,27 @@
-# Repository Guidelines for Zero
+---
+description: Project guidelines for the open-source CLI coding agent (zero) repository.
+globs: "*.go, *.js, *.md, *.json, *.toml, *.yaml, *.yml"
+alwaysApply: false
+---
 
-These instructions apply to all work in this repository. For the user-facing
-guide to extending Zero with specialists, hooks, plugins, MCP, and skills, see
-[docs/EXTENDING.md](docs/EXTENDING.md).
+# Repository Conventions for Zero
 
-## 1. Contribution and Pull Request Rules
+This file outlines the project conventions and repository guidelines for coding agents when working on the `zero` repository. For the general guide on how to extend Zero (write specialist sub-agents, hooks, plugins, MCP, skills), see [docs/EXTENDING.md](docs/EXTENDING.md).
 
-- Before opening any pull request, **all contributors**—including maintainers,
-  community contributors, and coding agents—must read and follow
-  [CONTRIBUTING.md](CONTRIBUTING.md).
-- Community pull requests require an existing parent issue with the
-  `issue-approved` label. Team members may open pull requests through the
-  internal development process described in `CONTRIBUTING.md`.
-- Keep each change focused on the approved or assigned scope. Do not include
-  unrelated fixes, refactors, formatting churn, generated output, or existing
-  local changes in the same commit or pull request.
-- Discuss new implementation languages, runtimes, major dependency changes,
-  and broad architectural rewrites with maintainers before implementation.
-- Pull request descriptions must explain what changed and why, link the parent
-  issue when required, and list the tests or verification performed. Include
-  screenshots or a short recording for user-visible UI changes when practical.
+## 1. Project Conventions
 
-## 2. Repository and Implementation Conventions
+- Build with `make`, not `go build` directly.
+- Tests live next to the source file (`foo_test.go` next to `foo.go`).
+- Run `make lint` before opening a PR.
+- Never edit files under `third_party/` — those are vendored.
+- Unify functions/methods where possible to prevent codebase inflation. Prefer a single cross-platform function with conditional checks over duplicating helpers per platform.
 
-- Use the Go version declared in `go.mod`. Do not hardcode a different local
-  toolchain version in scripts or documentation.
-- Use the repository build and release commands (`make` and
-  `go run ./cmd/zero-release ...`) instead of inventing parallel build flows.
-- Keep tests beside their source files (`foo_test.go` next to `foo.go`). Add a
-  regression test for behavior changes and run affected concurrent code under
-  the race detector.
-- Never edit files under `third_party/`; they are vendored.
-- Prefer one cross-platform function with small conditional checks over
-  duplicated platform-specific helpers when the behavior can remain unified.
-- Do not commit generated benchmark reports from
-  `internal/perfbench/reports/*.json`; reports are configuration-specific
-  evidence, not repository state.
-- Preserve the user's working tree. Do not overwrite, delete, stage, or commit
-  unrelated tracked or untracked files.
+## 2. Guidelines for Coding Agents
 
-## 3. Required Validation
+If you are an AI coding agent executing tasks in this repository, you **MUST** run all Go code quality and security checks before committing code or completing your task:
 
-Run validation from the repository root before committing, opening a pull
-request, or completing an implementation task:
+1. **Format & Vet**: Run `go fmt ./...` and `go vet ./...`.
+2. **Lint**: Run `go run github.com/golangci/golangci-lint/v2/cmd/[email protected] run --enable-only unused,ineffassign,staticcheck ./...`.
+3. **Security**: Run `go run golang.org/x/vuln/cmd/[email protected] ./...`.
 
-1. **Formatting check**: `make fmt-check`. If it fails, format with
-   `go fmt ./...` (or `make fmt`) and run the check again.
-2. **Vet**: `go vet ./...` (or `make vet`).
-3. **Tests**: `go test ./...`. Use `make test` for the full race-enabled suite,
-   or run focused tests with `-race`, when concurrency is affected.
-4. **Build**: `go run ./cmd/zero-release build`.
-5. **Smoke test**: `go run ./cmd/zero-release smoke`.
-6. **Advisory lint**:
-   `go run github.com/golangci/golangci-lint/v2/cmd/[email protected] run --enable-only unused,ineffassign,staticcheck ./...`.
-7. **Security**:
-   `go run golang.org/x/vuln/cmd/[email protected] ./...`.
-8. **Diff hygiene**: `git diff HEAD --check` (covers staged and unstaged
-   tracked changes).
-
-`make lint` currently runs the formatting check and `go vet`; it does **not**
-run golangci-lint. The pinned golangci-lint job is advisory in CI while the
-existing repository-wide backlog is cleaned up. Fix findings introduced by or
-related to the current change. Unrelated pre-existing advisory findings do not
-justify expanding a focused pull request; report them separately.
-
-Formatting, vet, tests, build, smoke, diff hygiene, and govulncheck are hard
-requirements. If a related check fails, fix it. If a required check cannot run
-because of the environment or fails for an unrelated external reason, report
-the exact failure and obtain maintainer direction rather than silently ignoring
-it.
+If any check fails or cannot be run, do not ignore it. Prompt the user for instructions or setup assistance before proceeding.

internal/mcp/oauth.go

@@ -133,6 +133,26 @@ func discoverAuthorizationServer(ctx context.Context, client *http.Client, baseU
 	}, nil
 }
 
+// validateResolvedEndpoints applies the shared https/loopback endpoint rule to
+// every non-empty endpoint in the resolved metadata — configured or discovered
+// alike — so discovery metadata can never downgrade an MCP login to an insecure
+// or attacker-controlled authorization, token, or registration endpoint.
+func validateResolvedEndpoints(metadata authServerMetadata) error {
+	for _, ep := range []struct{ kind, url string }{
+		{"authorization", metadata.AuthorizationEndpoint},
+		{"token", metadata.TokenEndpoint},
+		{"registration", metadata.RegistrationEndpoint},
+	} {
+		if strings.TrimSpace(ep.url) == "" {
+			continue
+		}
+		if err := oauth.ValidateEndpointURL(ep.url); err != nil {
+			return fmt.Errorf("mcp oauth: %s endpoint: %w", ep.kind, err)
+		}
+	}
+	return nil
+}
+
 // resolveAuthorizationServer discovers metadata and applies explicit config
 // overrides. Configured endpoints take precedence over discovered ones, and act
 // as a fallback when discovery fails or omits a value.
@@ -148,6 +168,9 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR
 			TokenEndpoint:         strings.TrimSpace(cfg.TokenEndpoint),
 			RegistrationEndpoint:  strings.TrimSpace(cfg.RegistrationEndpoint),
 		}
+		if err := validateResolvedEndpoints(metadata); err != nil {
+			return authServerMetadata{}, err
+		}
 		return metadata, nil
 	}
 
@@ -179,6 +202,9 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR
 	if strings.TrimSpace(metadata.TokenEndpoint) == "" {
 		return authServerMetadata{}, errors.New("no token endpoint discovered or configured")
 	}
+	if err := validateResolvedEndpoints(metadata); err != nil {
+		return authServerMetadata{}, err
+	}
 	return metadata, nil
 }
 

internal/mcp/oauth_test.go

@@ -73,6 +73,22 @@ func TestResolveEndpointsFallsBackToConfig(t *testing.T) {
 	}
 }
 
+// Discovered MCP metadata that serves an insecure endpoint must be rejected
+// before use, so discovery cannot downgrade the MCP login to an
+// attacker-controlled origin (issue #511).
+func TestResolveAuthorizationServerRejectsInsecureDiscovered(t *testing.T) {
+	mux := http.NewServeMux()
+	server := httptest.NewServer(mux)
+	defer server.Close()
+	mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(`{"issuer":"` + server.URL + `","token_endpoint":"` + server.URL + `/token","authorization_endpoint":"http://evil.example/authorize"}`))
+	})
+	_, err := resolveAuthorizationServer(context.Background(), server.Client(), server.URL, OAuthConfig{})
+	if err == nil || !strings.Contains(err.Error(), "authorization endpoint") {
+		t.Fatalf("err = %v, want insecure authorization endpoint rejection", err)
+	}
+}
+
 // roundTripperFunc adapts a function to http.RoundTripper so a test can
 // observe every outbound request.
 type roundTripperFunc func(*http.Request) (*http.Response, error)

internal/oauth/flow.go

@@ -39,6 +39,13 @@ func BuildAuthorizationURL(cfg Config, pkce PKCE, state, redirectURI string, ext
 	if endpoint == "" {
 		return "", errors.New("oauth: no authorization endpoint configured")
 	}
+	// Backstop: validate the authorization endpoint at the shared choke point
+	// both the provider and MCP flows build their browser URL through, so a
+	// discovery-downgraded endpoint can never open in the browser even if a
+	// merge site missed it.
+	if err := ValidateEndpointURL(endpoint); err != nil {
+		return "", err
+	}
 	parsed, err := url.Parse(endpoint)
 	if err != nil {
 		return "", fmt.Errorf("oauth: parse authorization endpoint: %w", err)
@@ -83,13 +90,15 @@ func isReservedAuthParam(key string) bool {
 	}
 }
 
-// validateTokenEndpoint refuses to send a credential to a token endpoint that is
-// not https, unless it is a loopback host (mirrors oauth2-client.js
-// validateTokenEndpointURL). This prevents leaking codes/tokens over cleartext.
-func validateTokenEndpoint(endpoint string) error {
+// ValidateEndpointURL refuses an OAuth endpoint that is not https, unless it is
+// a loopback host (mirrors oauth2-client.js validateTokenEndpointURL). Every
+// credential-bearing endpoint — configured OR learned from discovery — must
+// pass this single rule, so discovery metadata can never downgrade a login to
+// cleartext or redirect it to an attacker-controlled http origin.
+func ValidateEndpointURL(endpoint string) error {
 	parsed, err := url.Parse(trimmed(endpoint))
 	if err != nil || parsed.Host == "" {
-		return fmt.Errorf("oauth: invalid token endpoint %q", endpoint)
+		return fmt.Errorf("oauth: invalid endpoint %q", endpoint)
 	}
 	if parsed.Scheme == "https" {
 		return nil
@@ -100,6 +109,13 @@ func validateTokenEndpoint(endpoint string) error {
 	return fmt.Errorf("%w: %s", ErrInsecureTokenEndpoint, parsed.Scheme+"://"+parsed.Host)
 }
 
+// validateTokenEndpoint checks a token endpoint against the shared endpoint rule
+// (kept as a named helper for the token-exchange and device call sites). This
+// prevents leaking codes/tokens over cleartext.
+func validateTokenEndpoint(endpoint string) error {
+	return ValidateEndpointURL(endpoint)
+}
+
 func isLoopbackHost(host string) bool {
 	if host == "localhost" {
 		return true

internal/oauth/flow_test.go

@@ -112,6 +112,20 @@ func TestBuildAuthorizationURLRejectsPlain(t *testing.T) {
 	}
 }
 
+// The shared choke point both flows build their browser URL through must refuse
+// an insecure authorization endpoint, so a discovery-downgraded endpoint can
+// never open in the browser even if a merge site missed it (issue #511).
+func TestBuildAuthorizationURLRejectsInsecureEndpoint(t *testing.T) {
+	_, err := BuildAuthorizationURL(
+		Config{AuthorizationEndpoint: "http://evil.example/authorize", ClientID: "c"},
+		PKCE{Method: MethodS256, Challenge: "chal"},
+		"state", "http://127.0.0.1/cb", nil,
+	)
+	if !errors.Is(err, ErrInsecureTokenEndpoint) {
+		t.Fatalf("err = %v, want ErrInsecureTokenEndpoint", err)
+	}
+}
+
 func TestValidateTokenEndpoint(t *testing.T) {
 	cases := map[string]bool{
 		"https://auth.example.com/token": true,

internal/oauth/manager.go

@@ -218,13 +218,26 @@ func (m *Manager) resolveEndpoints(ctx context.Context, cfg Config) (Config, err
 	// may already be sufficient for the chosen flow. Only merge on success (and
 	// never return from the error branch, which would trip the nilerr linter).
 	if meta, err := m.discover(ctx, cfg.IssuerURL); err == nil {
-		if cfg.AuthorizationEndpoint == "" {
+		// A discovered endpoint must pass the same https/loopback rule as a
+		// configured one before it is merged, so discovery metadata can never
+		// silently downgrade the login to an insecure or attacker-controlled
+		// endpoint. Fail closed on a bad endpoint rather than merging it.
+		if cfg.AuthorizationEndpoint == "" && meta.AuthorizationEndpoint != "" {
+			if err := ValidateEndpointURL(meta.AuthorizationEndpoint); err != nil {
+				return cfg, fmt.Errorf("oauth: discovered authorization endpoint: %w", err)
+			}
 			cfg.AuthorizationEndpoint = meta.AuthorizationEndpoint
 		}
-		if cfg.TokenEndpoint == "" {
+		if cfg.TokenEndpoint == "" && meta.TokenEndpoint != "" {
+			if err := ValidateEndpointURL(meta.TokenEndpoint); err != nil {
+				return cfg, fmt.Errorf("oauth: discovered token endpoint: %w", err)
+			}
 			cfg.TokenEndpoint = meta.TokenEndpoint
 		}
-		if cfg.DeviceAuthorizationEndpoint == "" {
+		if cfg.DeviceAuthorizationEndpoint == "" && meta.DeviceAuthorizationEndpoint != "" {
+			if err := ValidateEndpointURL(meta.DeviceAuthorizationEndpoint); err != nil {
+				return cfg, fmt.Errorf("oauth: discovered device authorization endpoint: %w", err)
+			}
 			cfg.DeviceAuthorizationEndpoint = meta.DeviceAuthorizationEndpoint
 		}
 	}

internal/oauth/manager_test.go

@@ -2,6 +2,7 @@ package oauth
 
 import (
 	"context"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -200,6 +201,24 @@ func TestManagerGetFreshDiscoversIssuerEndpoints(t *testing.T) {
 	}
 }
 
+// A discovery document that serves an insecure (non-https, non-loopback)
+// endpoint must be rejected before it is merged, so discovery metadata cannot
+// silently downgrade the login to an attacker-controlled origin (issue #511).
+func TestResolveEndpointsRejectsInsecureDiscoveredEndpoint(t *testing.T) {
+	mux := http.NewServeMux()
+	server := httptest.NewServer(mux)
+	defer server.Close()
+	mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, _ *http.Request) {
+		// Loopback issuer, but an attacker-controlled http authorization endpoint.
+		_, _ = io.WriteString(w, `{"issuer":"`+server.URL+`","token_endpoint":"`+server.URL+`/token","authorization_endpoint":"http://evil.example/authorize"}`)
+	})
+	m := managerFor(t, map[string]string{}, nil)
+	_, err := m.resolveEndpoints(context.Background(), Config{IssuerURL: server.URL})
+	if !errors.Is(err, ErrInsecureTokenEndpoint) {
+		t.Fatalf("resolveEndpoints err = %v, want ErrInsecureTokenEndpoint", err)
+	}
+}
+
 func TestManagerGetFreshRefreshesExpired(t *testing.T) {
 	fp := newFakeProvider(t, `{"access_token":"fresh-at","expires_in":3600}`)
 	env := map[string]string{