From 8ed50bfd04b6bc6a2c074031315206037ea0119f Mon Sep 17 00:00:00 2001 From: MK Date: Thu, 16 Jul 2026 15:33:08 -0400 Subject: [PATCH 1/3] feat(mcp): OAuth discovery (RFC 9728/8414) + dynamic client registration (RFC 7591) (closes #316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remote-MCP OAuth no longer requires hand-configuring the client and endpoints. Given only `transport: http` + `url:`, Forge derives everything the way every other MCP client does: - RFC 9728 — protected-resource metadata (from the 401 WWW-Authenticate `resource_metadata` pointer, or the origin's `.well-known/oauth-protected-resource`) → the authorization server. - RFC 8414 — auth-server metadata (`oauth-authorization-server`, with the OpenID `openid-configuration` variant as fallback) → authorize/token/ registration endpoints + scopes_supported. - RFC 7591 — dynamic client registration mints a client_id at first `forge mcp login`; persisted (encrypted, alongside the token) and reused on refresh — never re-minted per run. Config: `client_id`/`authorize_url`/`token_url` are now OPTIONAL for `type: oauth`. Precedence: explicit config wins; else a persisted registration; else discovery+DCR (login only — the refresh path never mints). A partial endpoint pair (one set, one empty) is a validation error. Fail-closed with a clear message when a server advertises no metadata / no registration_endpoint and no client_id is set. Persistence: new generic `oauth.SaveRecord/LoadRecord/DeleteRecord` (encrypted-preferred, plaintext fallback, mirroring the token store) hold the discovered endpoints + minted client under `mcp_reg_`. Egress: the discovered authorization-server host isn't in forge.yaml to pre-seed the allowlist, so `mcp.RegisteredOAuthHosts` reads it back from the login-time registration record and the runtime merges it into the egress allowlist (store-path override applied first). Also: docs example now points at `/mcp` (Streamable HTTP) rather than the legacy `/sse`. Wired through `OAuthFlow.Login` + `BearerToken` (new `resolveOAuthConfig`); `NewServer`/`ValidateMCPConfig` guards relaxed to allow discovery while still rejecting partial endpoint configs. Tests: 9728/8414/7591 happy path + persistence-and-reuse (one DCR call), WWW-Authenticate fallback, explicit-override precedence, no-registration fail-closed, refresh-never-discovers, RegisteredOAuthHosts, and the WWW-Authenticate / well-known parse helpers. Docs: mcp/configuration.md discovery section; forge.md MCP note. --- .claude/skills/forge.md | 10 +- docs/mcp/configuration.md | 92 +++++- forge-cli/cmd/mcp_login.go | 1 + forge-cli/runtime/runner.go | 5 + forge-cli/runtime/runner_mcp.go | 24 ++ forge-core/llm/oauth/store.go | 105 +++++++ forge-core/mcp/oauth_discovery.go | 396 +++++++++++++++++++++++++ forge-core/mcp/oauth_discovery_test.go | 251 ++++++++++++++++ forge-core/mcp/oauth_flow.go | 24 +- forge-core/mcp/server.go | 15 +- forge-core/mcp/server_b6_test.go | 28 +- forge-core/types/config.go | 28 +- forge-core/validate/mcp_config.go | 14 +- forge-core/validate/mcp_config_test.go | 25 +- 14 files changed, 968 insertions(+), 50 deletions(-) create mode 100644 forge-core/mcp/oauth_discovery.go create mode 100644 forge-core/mcp/oauth_discovery_test.go diff --git a/.claude/skills/forge.md b/.claude/skills/forge.md index 8db3b228..6e98f08b 100644 --- a/.claude/skills/forge.md +++ b/.claude/skills/forge.md @@ -296,7 +296,9 @@ The agent loop calls tools the LLM asks for. The registry merges: `mcp.servers[]` block. Names are namespaced `__` (e.g. `linear__create_issue`). Phase 1 ships HTTP transport only; stdio is rejected at validate time with a roadmap pointer. OAuth 2.1 PKCE - supported via `forge mcp login`. + supported via `forge mcp login`; `client_id`/`authorize_url`/`token_url` + are optional — discovered from the server url via RFC 9728/8414 with + RFC 7591 dynamic client registration when omitted (#316). `cli_execute` ships 13 security layers — shell denylist, binary allowlist, `LookPath` resolution at startup, argument validation @@ -821,9 +823,9 @@ enterprise raw-capture path. `docs/security/audit-logging.md` § Trace cross-link, `docs/security/egress-control.md` § OTel collector auto-extension. -### 12.11 Governance framework R1–R9 (#216 umbrella) +### 12.11 Governance framework R1–R10 (#216 umbrella) -Six MUST + three SHOULD requirements from an agent-runtime governance framework. Complete on `main` after #245 / #246 / #247 / #248 land (R4c is the last piece). +Six MUST + three SHOULD requirements from an agent-runtime governance framework, complete on `main` after #245 / #246 / #247 / #248 land (R4c is the last piece). **R10 (delegated identity) is a proposed fourth SHOULD** — not yet implemented; see #317 / #318. | # | Requirement | Type | Where it lives | Related PR | |---|---|:-:|---|:-:| @@ -838,6 +840,7 @@ Six MUST + three SHOULD requirements from an agent-runtime governance framework. | R7 | Semantic distance | SHOULD | `forge-core/security/intent/drift.go`; `security.intent_drift` on top of R3. Rolling-window mean-below-threshold + monotone-decrease trip conditions. State-transition dedup (one `entered`, one `recovered` — no per-call flood). | #246 | | R8 | OpenTelemetry export | SHOULD | `observability.tracing` in yaml. Real tracer provider; OTLP HTTP/gRPC export. Audit events carry `trace_id` + `span_id` closing the loop between the SIEM channel and the APM channel. Baseline (#108). | — | | R9 | Least-privilege credentials | SHOULD | `forge-core/credentials/`; top-level `credentials:` in yaml. Providers: `static`, `sts_assume_role`. Fresh credentials per tool call; injected into subprocess env (`cli_execute`) or outbound headers (`http_request`). `credential_issued` / `credential_revoked` audit events. **No credential material in audit payloads.** | #236 | +| R10 | Delegated identity / on-behalf-of authorization | SHOULD (proposed) | Downstream tool calls (esp. remote MCP) execute under the **requesting user's** delegated identity, not a shared service grant — per-user, per-session, on-behalf-of. Where R9 scopes *what a tool may do*, R10 scopes *whose authority it acts under*. One `BearerToken(server, subject, session)` seam, **resolver behind it** (`design-tool-registry.md` §18): the **Forge-local resolver** (interactive OAuth, ephemeral per session; standalone-capable — #317) and the **managed broker resolver** (vaulted 3LO → ID-JAG; holds the IdP trust relationship — initializ, `initializ/aip/mcp-delegated-identity-broker.md`; #318 = the thin `id_jag` `method:` sliver in the Forge repo). Forge never learns which resolver answered. Delegation follows authorization (never mint speculatively); token injected at egress, never through the agent; writes still route through DEFER regardless of token validity (§18.5). Planned audit events: `mcp_auth_requested` / `mcp_auth_completed` / `mcp_auth_denied`. | #317 / #318 | **Config off by default.** Every governance block ships disabled — an absent block leaves the hook unregistered and the wire shape unchanged from a pre-governance Forge. Rollout: turn each on independently, warn-only first (`hard_threshold: -1`), gather the score distribution against your embedder + workload, then set `hard_threshold` a bit **below** the observed floor of your normal traffic (typically ~0.2–0.3; the default is `0.3`). `hard_threshold` is "score below → deny", so a high value like `0.85` would deny almost all legitimate calls — aligned actions cluster well above it (0.6–0.9 on OpenAI `text-embedding-3-small`). @@ -850,6 +853,7 @@ Six MUST + three SHOULD requirements from an agent-runtime governance framework. - `docs/security/audit-signing.md` (R6) - `docs/security/audit-tamper-evidence.md` (R5) - `docs/security/least-privilege-credentials.md` (R9) +- R10 (delegated identity) — proposed; authority `design-tool-registry.md` §18; #317 (Forge-local interactive resolver) / #318 (`id_jag` sliver) / `initializ/aip/mcp-delegated-identity-broker.md` (broker resolvers). - `docs/security/policy-decisions.md` (five-decision enum reference) --- diff --git a/docs/mcp/configuration.md b/docs/mcp/configuration.md index 859b00ac..848852f9 100644 --- a/docs/mcp/configuration.md +++ b/docs/mcp/configuration.md @@ -19,10 +19,10 @@ mcp: url: https://mcp.linear.app/sse # required for transport: http auth: # optional type: oauth # oauth | bearer | static - client_id: my-client-id # required if type=oauth + client_id: my-client-id # optional for oauth (see Discovery) scopes: [read, write] # optional - authorize_url: https://... # required if type=oauth - token_url: https://... # required if type=oauth + authorize_url: https://... # optional for oauth (discovered if omitted) + token_url: https://... # optional for oauth (discovered if omitted) token_env: NAME_OF_ENV_VAR # required if type=bearer|static tools: # default-deny — at least one of: allow: [create_issue, list_issues] # explicit names or ["*"] @@ -62,15 +62,62 @@ supported. Optional. Omit for unauthenticated servers (e.g., trusted in-cluster MCPs). -| `auth.type` | Required fields | When to use | -|-------------|------------------------------------------|-------------| -| `oauth` | `client_id`, `authorize_url`, `token_url` | Hosted MCPs (Linear, Notion, GitHub hosted) | -| `bearer` | `token_env` | In-cluster sidecars; CI machine-to-machine | -| `static` | `token_env` | Same as bearer; named for clarity | +| `auth.type` | Required fields | When to use | +|-------------|-----------------|-------------| +| `oauth` | *(none required — see Discovery)* | Hosted MCPs (Linear, Notion, GitHub hosted) | +| `bearer` | `token_env` | In-cluster sidecars; CI machine-to-machine | +| `static` | `token_env` | Same as bearer; named for clarity | `token_env` is the name of an environment variable; the variable's value is read at runtime — never stored in `forge.yaml`. +#### OAuth discovery & dynamic client registration (#316) + +For `type: oauth`, `client_id` / `authorize_url` / `token_url` are all +**optional**. When omitted, Forge discovers them from the server `url` +at `forge mcp login` time, using the MCP Authorization spec: + +1. **RFC 9728** — protected-resource metadata (from the server's `401` + `WWW-Authenticate` header, or `{origin}/.well-known/oauth-protected-resource`) + to find the authorization server. +2. **RFC 8414** — authorization-server metadata + (`/.well-known/oauth-authorization-server`, with the OpenID + `openid-configuration` variant as a fallback) to discover the + `authorize` / `token` / `registration` endpoints. +3. **RFC 7591** — dynamic client registration mints a `client_id` at + first login; it is persisted (encrypted, alongside the token) and + **reused** on refresh — never re-minted per run. + +So a fully zero-config OAuth server is just: + +```yaml +- name: linear + transport: http + url: https://mcp.linear.app/mcp + auth: + type: oauth + scopes: [read, write] # optional; discovery uses scopes_supported when omitted + tools: + allow: [create_issue, list_issues] +``` + +**Precedence & rules:** + +- **Explicit config always wins.** Set `client_id`/`authorize_url`/`token_url` + to override discovery (for servers that don't advertise metadata or + don't support DCR). +- `authorize_url` and `token_url` must be set **together** (or both + omitted); a partial pair is a validation error. +- **Fail-closed:** if a server advertises no metadata / no + `registration_endpoint` and no `client_id` is configured, login fails + with a clear message — supply the fields explicitly, or use a + discovery-capable server. +- **Egress:** the discovered authorization-server host isn't in + `forge.yaml` to pre-seed the allowlist, so it is learned from the + login-time registration record and merged into the egress allowlist at + runtime automatically. (Discovery itself runs at laptop-time + `forge mcp login`, off the egress-enforced path.) + ### `mcp.servers[].tools` **Default-deny.** Validation rejects entries where both `allow` and @@ -98,14 +145,39 @@ Per-RPC timeout. Default 60s. Minimum 1s. ## Worked examples -### Vendor-hosted MCP with OAuth +### Vendor-hosted MCP with OAuth (discovery — preferred) + +Point at the server's Streamable HTTP endpoint (`/mcp`, not the legacy +`/sse`) and let discovery resolve everything: + +```yaml +mcp: + servers: + - name: linear + transport: http + url: https://mcp.linear.app/mcp + auth: + type: oauth + scopes: [read, write] # client_id + endpoints discovered (#316) + tools: + allow: [create_issue, list_issues] + required: true +``` + +Then `forge mcp login linear` once — Forge discovers the endpoints and +registers a client automatically. + +### Vendor-hosted MCP with OAuth (explicit override) + +For a server that doesn't advertise metadata or doesn't support dynamic +client registration, pin the fields (this overrides discovery): ```yaml mcp: servers: - name: linear transport: http - url: https://mcp.linear.app/sse + url: https://mcp.linear.app/mcp auth: type: oauth client_id: ${LINEAR_OAUTH_CLIENT_ID} diff --git a/forge-cli/cmd/mcp_login.go b/forge-cli/cmd/mcp_login.go index bcbdebc5..57f62e55 100644 --- a/forge-cli/cmd/mcp_login.go +++ b/forge-cli/cmd/mcp_login.go @@ -64,6 +64,7 @@ func mcpLoginRun(cmd *cobra.Command, args []string) error { fmt.Printf("opening browser to authorize Forge against %s...\n", name) fmt.Println("(if a browser does not open, look for the URL on stdout below)") if err := flow.Login(ctx, name, mcp.OAuthServerConfig{ + ServerURL: spec.URL, // enables RFC 9728/8414/7591 discovery when endpoints are omitted (#316) ClientID: spec.Auth.ClientID, Scopes: spec.Auth.Scopes, AuthorizeURL: spec.Auth.AuthorizeURL, diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 9cad12f8..601a8714 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -594,6 +594,11 @@ func (r *Runner) Run(ctx context.Context) error { // Same for MCP servers — without this, every HTTPS MCP call would // be silently blocked. Mirror the AuthDomains pattern. egressDomains = append(egressDomains, security.MCPDomains(r.cfg.Config.MCP)...) + // #316: with OAuth discovery the authorization-server host is not in + // forge.yaml to pre-seed the allowlist — it is learned at login time + // and persisted in the registration record. mcpRegisteredOAuthHosts + // applies the store-path override and reads those hosts back. + egressDomains = append(egressDomains, mcpRegisteredOAuthHosts(r.cfg.Config.MCP)...) // Phase 6 (#107 / #108) — same for the OTel collector. Without // this, dev runs with `observability.tracing.enabled: true` and // `egress.mode: allowlist` would silently drop spans on shutdown. diff --git a/forge-cli/runtime/runner_mcp.go b/forge-cli/runtime/runner_mcp.go index 1b0bcf43..c8e31d38 100644 --- a/forge-cli/runtime/runner_mcp.go +++ b/forge-cli/runtime/runner_mcp.go @@ -8,8 +8,32 @@ import ( "github.com/initializ/forge/forge-core/llm/oauth" "github.com/initializ/forge/forge-core/mcp" coreruntime "github.com/initializ/forge/forge-core/runtime" + "github.com/initializ/forge/forge-core/types" ) +// oauthServerNames returns the names of the oauth-typed MCP servers. +func oauthServerNames(cfg types.MCPConfig) []string { + var names []string + for _, s := range cfg.Servers { + if s.Auth != nil && s.Auth.Type == "oauth" { + names = append(names, s.Name) + } + } + return names +} + +// mcpRegisteredOAuthHosts applies the MCP token-store override, then +// returns the authorize/token/registration hosts persisted at login +// time by OAuth discovery (#316) — merged into the egress allowlist so +// a discovered authorization-server host (absent from forge.yaml) is +// reachable at runtime. +func mcpRegisteredOAuthHosts(cfg types.MCPConfig) []string { + if sp := mcpTokenStorePath(cfg.TokenStorePath); sp != "" { + oauth.SetCredentialsDir(sp) + } + return mcp.RegisteredOAuthHosts(oauthServerNames(cfg)) +} + // mcpTokenStorePath returns the effective OAuth credentials // directory for MCP. Precedence (highest first): // diff --git a/forge-core/llm/oauth/store.go b/forge-core/llm/oauth/store.go index 4d3848dc..3e88c6e7 100644 --- a/forge-core/llm/oauth/store.go +++ b/forge-core/llm/oauth/store.go @@ -149,6 +149,111 @@ func MigrateToEncrypted(provider string) error { return nil } +// --- generic JSON records (beyond Token) --- +// +// SaveRecord / LoadRecord / DeleteRecord persist an arbitrary +// JSON-serializable value under a caller-supplied key, using the same +// encrypted-preferred, plaintext-fallback strategy as the token +// helpers. Added for the MCP OAuth discovery/registration record +// (#316): the minted client_id + discovered endpoints must survive +// across a refresh and a pod restart, exactly like the token itself. + +// SaveRecord persists v (marshaled to JSON) under key. +func SaveRecord(key string, v any) error { + data, err := json.Marshal(v) + if err != nil { + return fmt.Errorf("marshaling record: %w", err) + } + if ep := encryptedProvider(); ep != nil { + if err := ep.Set(key, string(data)); err != nil { + return fmt.Errorf("saving encrypted record: %w", err) + } + _ = removeRecordFile(key) // migration clean-up + return nil + } + return saveRecordPlaintext(key, data) +} + +// LoadRecord loads a record saved by SaveRecord into v. Returns +// found=false (nil error) when no record exists for key. +func LoadRecord(key string, v any) (found bool, err error) { + if ep := encryptedProvider(); ep != nil { + val, gErr := ep.Get(key) + if gErr == nil { + if jErr := json.Unmarshal([]byte(val), v); jErr != nil { + return false, fmt.Errorf("parsing encrypted record: %w", jErr) + } + return true, nil + } + if !secrets.IsNotFound(gErr) { + return false, fmt.Errorf("reading encrypted record: %w", gErr) + } + // Not found in encrypted store — fall through to plaintext. + } + return loadRecordPlaintext(key, v) +} + +// DeleteRecord removes a record from both stores. Idempotent. +func DeleteRecord(key string) error { + if ep := encryptedProvider(); ep != nil { + if err := ep.Delete(key); err != nil && !secrets.IsNotFound(err) { + return fmt.Errorf("deleting encrypted record: %w", err) + } + } + return removeRecordFile(key) +} + +func recordPath(key string) (string, error) { + dir, err := DefaultCredentialsDir() + if err != nil { + return "", err + } + return filepath.Join(dir, key+".json"), nil +} + +func saveRecordPlaintext(key string, data []byte) error { + dir, err := DefaultCredentialsDir() + if err != nil { + return err + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return fmt.Errorf("creating credentials directory: %w", err) + } + if err := os.WriteFile(filepath.Join(dir, key+".json"), data, 0o600); err != nil { + return fmt.Errorf("writing record: %w", err) + } + return nil +} + +func loadRecordPlaintext(key string, v any) (bool, error) { + path, err := recordPath(key) + if err != nil { + return false, err + } + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("reading record: %w", err) + } + if err := json.Unmarshal(data, v); err != nil { + return false, fmt.Errorf("parsing record: %w", err) + } + return true, nil +} + +func removeRecordFile(key string) error { + path, err := recordPath(key) + if err != nil { + return err + } + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("removing record: %w", err) + } + return nil +} + // --- plaintext helpers --- func savePlaintext(provider string, token *Token) error { diff --git a/forge-core/mcp/oauth_discovery.go b/forge-core/mcp/oauth_discovery.go new file mode 100644 index 00000000..84e03d44 --- /dev/null +++ b/forge-core/mcp/oauth_discovery.go @@ -0,0 +1,396 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" + + "github.com/initializ/forge/forge-core/llm/oauth" +) + +const ( + // defaultDiscoveryTimeout bounds a discovery/DCR request when the + // flow has no egress-controlled client injected (laptop login). + defaultDiscoveryTimeout = 15 * time.Second + // maxMetadataBytes caps a metadata / registration response so a + // hostile or misconfigured endpoint can't stream unbounded data. + maxMetadataBytes = 1 << 20 // 1 MiB +) + +// MCP authorization discovery + dynamic client registration (#316). +// +// Implements the MCP Authorization spec's zero-config path so that a +// server declared with only `transport: http` + `url:` can complete +// OAuth with NO client_id / authorize_url / token_url in forge.yaml: +// +// RFC 9728 — protected-resource metadata: which authorization +// server(s) govern this MCP resource. +// RFC 8414 — authorization-server metadata: discover the +// authorize / token / registration endpoints. +// RFC 7591 — dynamic client registration: mint a client_id at +// first login; persisted so it is reused, never re-minted. +// +// The resolved endpoints + minted client are persisted in the same +// encrypted store as the token (oauth.SaveRecord) under regStoreKey, +// so runtime refresh and a pod restart reuse them without re-running +// discovery. Explicit config always wins (§ resolveOAuthConfig). + +// regStoreKey returns the credential-store key for a server's OAuth +// registration record. Namespaced apart from the token key. +func regStoreKey(name string) string { return "mcp_reg_" + name } + +// oauthRegistration is the persisted result of discovery + DCR. It +// carries everything needed to run the authorize/refresh flow later +// without re-discovering: the resolved endpoints and the minted client. +type oauthRegistration struct { + AuthorizeURL string `json:"authorize_url"` + TokenURL string `json:"token_url"` + RegistrationURL string `json:"registration_url,omitempty"` + ClientID string `json:"client_id"` + ClientSecret string `json:"client_secret,omitempty"` // if the AS issued a confidential client + Scopes []string `json:"scopes,omitempty"` +} + +// loopbackRedirectURIs are what DCR registers for laptop-time login. +// RFC 8252 §7.3: an AS MUST allow a variable port for loopback +// redirect URIs, so we register the port-less loopback so the minted +// client is reusable across logins (each picks a fresh ephemeral port). +var loopbackRedirectURIs = []string{ + "http://127.0.0.1/callback", + "http://localhost/callback", +} + +// authServerMetadata is the subset of RFC 8414 / OpenID discovery we consume. +type authServerMetadata struct { + Issuer string `json:"issuer"` + AuthorizationEndpoint string `json:"authorization_endpoint"` + TokenEndpoint string `json:"token_endpoint"` + RegistrationEndpoint string `json:"registration_endpoint"` + ScopesSupported []string `json:"scopes_supported"` +} + +// protectedResourceMetadata is the subset of RFC 9728 we consume. +type protectedResourceMetadata struct { + Resource string `json:"resource"` + AuthorizationServers []string `json:"authorization_servers"` +} + +// resolveOAuthConfig fills in whatever the operator did not configure, +// in strict precedence order: +// +// 1. Fully explicit config (client_id + authorize_url + token_url) → used verbatim. +// 2. A persisted registration from a prior discovery → reused (no network). +// 3. allowDiscovery → run RFC 9728 → 8414 → 7591 and persist the result. +// +// allowDiscovery is true only for interactive Login. BearerToken +// (refresh) passes false: if steps 1–2 can't satisfy it, the operator +// must `forge mcp login` first, exactly as before — a refresh path +// never mints a client. +func (f *OAuthFlow) resolveOAuthConfig(ctx context.Context, name string, cfg OAuthServerConfig, allowDiscovery bool) (OAuthServerConfig, error) { + // 1. Fully explicit — static override always wins. + if cfg.ClientID != "" && cfg.AuthorizeURL != "" && cfg.TokenURL != "" { + return cfg, nil + } + + // 2. Persisted registration from a prior discovery/login. + var reg oauthRegistration + found, err := oauth.LoadRecord(regStoreKey(name), ®) + if err != nil { + return cfg, fmt.Errorf("reading oauth registration for %q: %w", name, err) + } + if found { + return mergeRegistration(cfg, reg), nil + } + + if !allowDiscovery { + return cfg, fmt.Errorf("%w: no oauth endpoints for %q and no stored registration — run 'forge mcp login %s'", ErrNoToken, name, name) + } + + // 3. Discover + register. + if cfg.ServerURL == "" { + return cfg, fmt.Errorf("%w: server %q has no oauth endpoints configured and no url to discover from", ErrProtocolError, name) + } + meta, err := f.discoverAuthServer(ctx, cfg.ServerURL) + if err != nil { + return cfg, err + } + + clientID, clientSecret := cfg.ClientID, cfg.ClientSecret + if clientID == "" { + if meta.RegistrationEndpoint == "" { + return cfg, fmt.Errorf("%w: server %q advertises no registration_endpoint and no client_id is configured — supply client_id/authorize_url/token_url, or use a server that supports dynamic client registration", ErrProtocolError, name) + } + scopes := cfg.Scopes + if len(scopes) == 0 { + scopes = meta.ScopesSupported + } + clientID, clientSecret, err = f.registerClient(ctx, meta.RegistrationEndpoint, scopes) + if err != nil { + return cfg, err + } + } + + reg = oauthRegistration{ + AuthorizeURL: firstNonEmpty(cfg.AuthorizeURL, meta.AuthorizationEndpoint), + TokenURL: firstNonEmpty(cfg.TokenURL, meta.TokenEndpoint), + RegistrationURL: meta.RegistrationEndpoint, + ClientID: clientID, + ClientSecret: clientSecret, + Scopes: cfg.Scopes, + } + if reg.AuthorizeURL == "" || reg.TokenURL == "" { + return cfg, fmt.Errorf("%w: discovery for %q did not yield both authorize and token endpoints", ErrProtocolError, name) + } + if err := oauth.SaveRecord(regStoreKey(name), ®); err != nil { + return cfg, fmt.Errorf("persisting oauth registration for %q: %w", name, err) + } + return mergeRegistration(cfg, reg), nil +} + +// mergeRegistration fills empty cfg fields from a persisted/discovered +// registration. Explicit cfg fields are never overwritten (precedence). +func mergeRegistration(cfg OAuthServerConfig, reg oauthRegistration) OAuthServerConfig { + cfg.ClientID = firstNonEmpty(cfg.ClientID, reg.ClientID) + cfg.ClientSecret = firstNonEmpty(cfg.ClientSecret, reg.ClientSecret) + cfg.AuthorizeURL = firstNonEmpty(cfg.AuthorizeURL, reg.AuthorizeURL) + cfg.TokenURL = firstNonEmpty(cfg.TokenURL, reg.TokenURL) + if len(cfg.Scopes) == 0 { + cfg.Scopes = reg.Scopes + } + return cfg +} + +// discoverAuthServer runs RFC 9728 → RFC 8414 from the MCP server URL +// and returns the authorization-server metadata. +func (f *OAuthFlow) discoverAuthServer(ctx context.Context, serverURL string) (authServerMetadata, error) { + var zero authServerMetadata + + asURL, err := f.discoverProtectedResource(ctx, serverURL) + if err != nil { + return zero, err + } + + // RFC 8414: {issuer}/.well-known/oauth-authorization-server, with + // the OpenID variant as a fallback for servers that only publish + // openid-configuration. + for _, wk := range []string{ + wellKnown(asURL, "oauth-authorization-server"), + wellKnown(asURL, "openid-configuration"), + } { + meta, err := fetchJSON[authServerMetadata](ctx, f.httpClient(), wk) + if err == nil && meta.TokenEndpoint != "" && meta.AuthorizationEndpoint != "" { + return meta, nil + } + } + return zero, fmt.Errorf("%w: no usable authorization-server metadata at %s", ErrProtocolError, asURL) +} + +// discoverProtectedResource resolves the authorization server for an +// MCP resource (RFC 9728). It first tries the well-known path derived +// from the server URL; if the server instead answers 401 with a +// WWW-Authenticate `resource_metadata` pointer, that is honored too. +func (f *OAuthFlow) discoverProtectedResource(ctx context.Context, serverURL string) (string, error) { + // Primary: the well-known path off the server origin. + prURL := wellKnown(serverURL, "oauth-protected-resource") + if pr, err := fetchJSON[protectedResourceMetadata](ctx, f.httpClient(), prURL); err == nil && len(pr.AuthorizationServers) > 0 { + return pr.AuthorizationServers[0], nil + } + + // Fallback: probe the server itself and read the 401 + // WWW-Authenticate `resource_metadata` param (RFC 9728 §5.1). + if rm := f.probeResourceMetadataURL(ctx, serverURL); rm != "" { + if pr, err := fetchJSON[protectedResourceMetadata](ctx, f.httpClient(), rm); err == nil && len(pr.AuthorizationServers) > 0 { + return pr.AuthorizationServers[0], nil + } + } + return "", fmt.Errorf("%w: could not discover an authorization server for %s (no RFC 9728 protected-resource metadata)", ErrProtocolError, serverURL) +} + +// probeResourceMetadataURL makes an unauthenticated request to the MCP +// server and extracts the `resource_metadata` URL from a 401's +// WWW-Authenticate header, if present. +func (f *OAuthFlow) probeResourceMetadataURL(ctx context.Context, serverURL string) string { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, serverURL, nil) + if err != nil { + return "" + } + resp, err := f.httpClient().Do(req) + if err != nil { + return "" + } + defer func() { _ = resp.Body.Close() }() + _, _ = io.Copy(io.Discard, resp.Body) + if resp.StatusCode != http.StatusUnauthorized { + return "" + } + return resourceMetadataParam(resp.Header.Get("WWW-Authenticate")) +} + +// resourceMetadataParam pulls the resource_metadata="..." value out of +// a WWW-Authenticate header. Tolerant of ordering/quoting/whitespace. +func resourceMetadataParam(header string) string { + for _, part := range strings.Split(header, ",") { + part = strings.TrimSpace(part) + // Drop the leading scheme token (e.g. "Bearer") if present. + if i := strings.IndexByte(part, ' '); i >= 0 && !strings.Contains(part[:i], "=") { + part = strings.TrimSpace(part[i+1:]) + } + k, v, ok := strings.Cut(part, "=") + if ok && strings.EqualFold(strings.TrimSpace(k), "resource_metadata") { + return strings.Trim(strings.TrimSpace(v), `"`) + } + } + return "" +} + +// registerClient performs RFC 7591 dynamic client registration and +// returns the minted client_id and (if issued) client_secret. +func (f *OAuthFlow) registerClient(ctx context.Context, registrationURL string, scopes []string) (clientID, clientSecret string, err error) { + body := map[string]any{ + "client_name": "Forge MCP", + "redirect_uris": loopbackRedirectURIs, + "grant_types": []string{"authorization_code", "refresh_token"}, + "response_types": []string{"code"}, + "token_endpoint_auth_method": "none", // public client (PKCE) + } + if len(scopes) > 0 { + body["scope"] = strings.Join(scopes, " ") + } + raw, err := json.Marshal(body) + if err != nil { + return "", "", err + } + req, err := http.NewRequestWithContext(ctx, http.MethodPost, registrationURL, strings.NewReader(string(raw))) + if err != nil { + return "", "", err + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + resp, err := f.httpClient().Do(req) + if err != nil { + return "", "", fmt.Errorf("%w: dynamic client registration request failed: %v", ErrTransportUnavailable, err) + } + defer func() { _ = resp.Body.Close() }() + data, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK { + return "", "", fmt.Errorf("%w: dynamic client registration returned %d: %s", ErrProtocolError, resp.StatusCode, strings.TrimSpace(string(data))) + } + var out struct { + ClientID string `json:"client_id"` + ClientSecret string `json:"client_secret"` + } + if err := json.Unmarshal(data, &out); err != nil { + return "", "", fmt.Errorf("%w: parsing registration response: %v", ErrProtocolError, err) + } + if out.ClientID == "" { + return "", "", fmt.Errorf("%w: registration response carried no client_id", ErrProtocolError) + } + return out.ClientID, out.ClientSecret, nil +} + +// RegisteredOAuthHosts returns the authorize/token/registration hosts +// from persisted OAuth registrations for the named oauth servers. The +// runtime merges these into the egress allowlist — with discovery the +// auth-server host is not in forge.yaml to pre-seed, so it is learned +// from the login-time registration record (#316). +// +// Best-effort: a server with no stored registration (explicit config, +// or not yet logged in) contributes nothing here; explicit hosts come +// from security.MCPDomains as before. Callers must apply any +// credentials-dir override (oauth.SetCredentialsDir) first, so the +// right store is read. +func RegisteredOAuthHosts(oauthServerNames []string) []string { + seen := map[string]struct{}{} + for _, name := range oauthServerNames { + var reg oauthRegistration + found, err := oauth.LoadRecord(regStoreKey(name), ®) + if err != nil || !found { + continue + } + for _, raw := range []string{reg.AuthorizeURL, reg.TokenURL, reg.RegistrationURL} { + if h := hostOf(raw); h != "" { + seen[h] = struct{}{} + } + } + } + if len(seen) == 0 { + return nil + } + out := make([]string, 0, len(seen)) + for h := range seen { + out = append(out, h) + } + return out +} + +// --- small helpers --- + +// httpClient returns the flow's egress-controlled client, or a +// defaulting one. Discovery/DCR ride the same client as /token so they +// obey the egress allowlist. +func (f *OAuthFlow) httpClient() *http.Client { + if f.HTTPClient != nil { + return f.HTTPClient + } + return &http.Client{Timeout: defaultDiscoveryTimeout} +} + +func fetchJSON[T any](ctx context.Context, client *http.Client, target string) (T, error) { + var out T + req, err := http.NewRequestWithContext(ctx, http.MethodGet, target, nil) + if err != nil { + return out, err + } + req.Header.Set("Accept", "application/json") + resp, err := client.Do(req) + if err != nil { + return out, err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + _, _ = io.Copy(io.Discard, resp.Body) + return out, fmt.Errorf("GET %s: status %d", target, resp.StatusCode) + } + data, err := io.ReadAll(io.LimitReader(resp.Body, maxMetadataBytes)) + if err != nil { + return out, err + } + if err := json.Unmarshal(data, &out); err != nil { + return out, fmt.Errorf("parsing %s: %w", target, err) + } + return out, nil +} + +// wellKnown builds a `.well-known/` URL off the ORIGIN of base +// (scheme+host), per RFC 8414 §3 / RFC 9728 §3 — the well-known path +// is rooted at the host, ignoring any path component of base. +func wellKnown(base, name string) string { + u, err := url.Parse(base) + if err != nil || u.Host == "" { + return "" + } + u.Path = "/.well-known/" + name + u.RawQuery = "" + u.Fragment = "" + return u.String() +} + +func hostOf(raw string) string { + u, err := url.Parse(raw) + if err != nil { + return "" + } + return u.Hostname() +} + +func firstNonEmpty(a, b string) string { + if a != "" { + return a + } + return b +} diff --git a/forge-core/mcp/oauth_discovery_test.go b/forge-core/mcp/oauth_discovery_test.go new file mode 100644 index 00000000..090238f2 --- /dev/null +++ b/forge-core/mcp/oauth_discovery_test.go @@ -0,0 +1,251 @@ +package mcp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + + "github.com/initializ/forge/forge-core/llm/oauth" +) + +// withTempCredsDir points the oauth store at a throwaway dir so the +// discovery/registration persistence tests don't touch ~/.forge. +func withTempCredsDir(t *testing.T) { + t.Helper() + oauth.SetCredentialsDir(t.TempDir()) + t.Cleanup(func() { oauth.SetCredentialsDir("") }) +} + +// discoveryServer stands in for an MCP server + its authorization +// server. It serves RFC 9728 protected-resource metadata, RFC 8414 +// auth-server metadata, and an RFC 7591 registration endpoint. Knobs +// let individual tests suppress pieces to exercise the fallbacks. +type discoveryServer struct { + srv *httptest.Server + regCalls atomic.Int32 + noPRWellKnown bool // suppress /.well-known/oauth-protected-resource (force WWW-Authenticate path) + noRegEndpoint bool // omit registration_endpoint from AS metadata +} + +func newDiscoveryServer(t *testing.T) *discoveryServer { + t.Helper() + d := &discoveryServer{} + mux := http.NewServeMux() + d.srv = httptest.NewServer(mux) + base := d.srv.URL + + prMeta := func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(protectedResourceMetadata{ + Resource: base + "/mcp", + AuthorizationServers: []string{base}, + }) + } + mux.HandleFunc("/.well-known/oauth-protected-resource", func(w http.ResponseWriter, r *http.Request) { + if d.noPRWellKnown { + http.NotFound(w, r) + return + } + prMeta(w, r) + }) + // An ALTERNATE protected-resource metadata URL the 401 points at — + // always served, so the WWW-Authenticate fallback works even when the + // default well-known path is suppressed (RFC 9728 allows a custom URL). + mux.HandleFunc("/alt-resource-metadata", prMeta) + + mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, _ *http.Request) { + m := authServerMetadata{ + Issuer: base, + AuthorizationEndpoint: base + "/authorize", + TokenEndpoint: base + "/token", + ScopesSupported: []string{"read", "write"}, + } + if !d.noRegEndpoint { + m.RegistrationEndpoint = base + "/register" + } + _ = json.NewEncoder(w).Encode(m) + }) + + mux.HandleFunc("/register", func(w http.ResponseWriter, _ *http.Request) { + d.regCalls.Add(1) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"client_id":"dyn-client-123"}`)) + }) + + // The MCP endpoint itself: 401 with a WWW-Authenticate pointer, so + // the WWW-Authenticate discovery path can be exercised. + mux.HandleFunc("/mcp", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("WWW-Authenticate", + `Bearer resource_metadata="`+base+`/alt-resource-metadata"`) + w.WriteHeader(http.StatusUnauthorized) + }) + + t.Cleanup(d.srv.Close) + return d +} + +func (d *discoveryServer) url() string { return d.srv.URL + "/mcp" } + +// TestResolveOAuthConfig_Discovery: with no endpoints and no client_id, +// resolve runs 9728 → 8414 → 7591, populates the config, and persists a +// registration record that a second call reuses (no re-registration). +func TestResolveOAuthConfig_Discovery(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + f := NewOAuthFlow() + + cfg := OAuthServerConfig{ServerURL: d.url(), Scopes: []string{"read"}} + got, err := f.resolveOAuthConfig(context.Background(), "srv", cfg, true) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got.ClientID != "dyn-client-123" { + t.Errorf("client_id = %q, want the DCR-minted id", got.ClientID) + } + if !strings.HasSuffix(got.AuthorizeURL, "/authorize") || !strings.HasSuffix(got.TokenURL, "/token") { + t.Errorf("endpoints not discovered: %+v", got) + } + + // Second resolve must reuse the persisted registration — no new DCR. + got2, err := f.resolveOAuthConfig(context.Background(), "srv", cfg, true) + if err != nil { + t.Fatalf("resolve #2: %v", err) + } + if got2.ClientID != "dyn-client-123" { + t.Errorf("reuse: client_id = %q", got2.ClientID) + } + if n := d.regCalls.Load(); n != 1 { + t.Errorf("registration endpoint hit %d times, want 1 (persisted + reused)", n) + } +} + +// TestResolveOAuthConfig_WWWAuthenticateFallback: when the well-known +// protected-resource path is absent, the 401 WWW-Authenticate pointer +// drives discovery instead. +func TestResolveOAuthConfig_WWWAuthenticateFallback(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + d.noPRWellKnown = true + f := NewOAuthFlow() + + got, err := f.resolveOAuthConfig(context.Background(), "srv", OAuthServerConfig{ServerURL: d.url()}, true) + if err != nil { + t.Fatalf("resolve via WWW-Authenticate: %v", err) + } + if got.ClientID == "" || got.TokenURL == "" { + t.Errorf("discovery via WWW-Authenticate did not populate config: %+v", got) + } +} + +// TestResolveOAuthConfig_ExplicitWins: a fully-explicit config never +// touches the network (discovery would fail against a dead URL). +func TestResolveOAuthConfig_ExplicitWins(t *testing.T) { + withTempCredsDir(t) + f := NewOAuthFlow() + cfg := OAuthServerConfig{ + ServerURL: "http://127.0.0.1:0/dead", + ClientID: "static", + AuthorizeURL: "https://as/authorize", + TokenURL: "https://as/token", + } + got, err := f.resolveOAuthConfig(context.Background(), "srv", cfg, true) + if err != nil { + t.Fatalf("explicit config must not require discovery: %v", err) + } + if got.ClientID != "static" { + t.Errorf("explicit client_id overridden: %+v", got) + } +} + +// TestResolveOAuthConfig_NoRegistrationFailsClosed: a server advertising +// no registration_endpoint and no configured client_id fails closed with +// a clear message. +func TestResolveOAuthConfig_NoRegistrationFailsClosed(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + d.noRegEndpoint = true + f := NewOAuthFlow() + + _, err := f.resolveOAuthConfig(context.Background(), "srv", OAuthServerConfig{ServerURL: d.url()}, true) + if err == nil || !strings.Contains(err.Error(), "registration_endpoint") { + t.Fatalf("want a fail-closed no-registration error, got: %v", err) + } +} + +// TestResolveOAuthConfig_RefreshNoDiscovery: the refresh path +// (allowDiscovery=false) never discovers — with no stored registration +// it errors, pointing at `forge mcp login`. +func TestResolveOAuthConfig_RefreshNoDiscovery(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + f := NewOAuthFlow() + + _, err := f.resolveOAuthConfig(context.Background(), "srv", OAuthServerConfig{ServerURL: d.url()}, false) + if err == nil || !strings.Contains(err.Error(), "forge mcp login") { + t.Fatalf("refresh path must not discover; want login hint, got: %v", err) + } + if n := d.regCalls.Load(); n != 0 { + t.Errorf("refresh path hit the registration endpoint %d times, want 0", n) + } +} + +// TestRegisteredOAuthHosts: after discovery persists a registration, the +// egress helper returns the authorize/token/registration hosts. +func TestRegisteredOAuthHosts(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + f := NewOAuthFlow() + if _, err := f.resolveOAuthConfig(context.Background(), "srv", OAuthServerConfig{ServerURL: d.url()}, true); err != nil { + t.Fatalf("resolve: %v", err) + } + + hosts := RegisteredOAuthHosts([]string{"srv"}) + wantHost := hostOf(d.srv.URL) + found := false + for _, h := range hosts { + if h == wantHost { + found = true + } + } + if !found { + t.Errorf("RegisteredOAuthHosts = %v, want it to include %q", hosts, wantHost) + } + // A server with no stored registration contributes nothing. + if h := RegisteredOAuthHosts([]string{"never-logged-in"}); h != nil { + t.Errorf("unregistered server yielded hosts: %v", h) + } +} + +// TestResourceMetadataParam covers the WWW-Authenticate parse edge cases. +func TestResourceMetadataParam(t *testing.T) { + cases := map[string]string{ + `Bearer resource_metadata="https://as/.well-known/x"`: "https://as/.well-known/x", + `Bearer realm="r", resource_metadata="https://as/m", error="x"`: "https://as/m", + `resource_metadata=https://as/m`: "https://as/m", + `Bearer realm="r"`: "", + ``: "", + } + for header, want := range cases { + if got := resourceMetadataParam(header); got != want { + t.Errorf("resourceMetadataParam(%q) = %q, want %q", header, got, want) + } + } +} + +// TestWellKnown pins the origin-rooted well-known path construction. +func TestWellKnown(t *testing.T) { + cases := map[string]string{ + "https://as.example.com": "https://as.example.com/.well-known/oauth-authorization-server", + "https://as.example.com/": "https://as.example.com/.well-known/oauth-authorization-server", + "https://mcp.example.com/mcp?x=1": "https://mcp.example.com/.well-known/oauth-authorization-server", + "https://as.example.com/tenant/abc": "https://as.example.com/.well-known/oauth-authorization-server", + } + for base, want := range cases { + if got := wellKnown(base, "oauth-authorization-server"); got != want { + t.Errorf("wellKnown(%q) = %q, want %q", base, got, want) + } + } +} diff --git a/forge-core/mcp/oauth_flow.go b/forge-core/mcp/oauth_flow.go index 413f09b2..d4cd8c39 100644 --- a/forge-core/mcp/oauth_flow.go +++ b/forge-core/mcp/oauth_flow.go @@ -91,7 +91,11 @@ func NewOAuthFlow() *OAuthFlow { // to keep this package importable from cmd/ without a dependency on // the types package shape changing. type OAuthServerConfig struct { + // ServerURL is the MCP server endpoint. Used for RFC 9728/8414 + // discovery (#316) when the endpoints below are not configured. + ServerURL string ClientID string + ClientSecret string // set only when DCR issued a confidential client Scopes []string AuthorizeURL string TokenURL string @@ -137,8 +141,16 @@ func newLoginServer(handler http.Handler) *http.Server { // `forge mcp login `. Blocks until the callback fires or ctx // is cancelled. func (f *OAuthFlow) Login(ctx context.Context, name string, cfg OAuthServerConfig) error { + // Resolve endpoints + client_id: explicit config wins, else a + // prior registration, else RFC 9728/8414/7591 discovery + DCR + // (#316). allowDiscovery=true — this is the interactive path. + resolved, err := f.resolveOAuthConfig(ctx, name, cfg, true) + if err != nil { + return err + } + cfg = resolved if cfg.ClientID == "" || cfg.AuthorizeURL == "" || cfg.TokenURL == "" { - return fmt.Errorf("%w: oauth Login requires client_id, authorize_url, token_url", ErrProtocolError) + return fmt.Errorf("%w: oauth Login could not resolve client_id, authorize_url, token_url (configure them, or use a discovery-capable server)", ErrProtocolError) } if f.BrowserOpener == nil { // Fail fast — see the BrowserOpener field docstring. The CLI @@ -275,6 +287,16 @@ func (f *OAuthFlow) BearerToken(ctx context.Context, name string, cfg OAuthServe return tok.AccessToken, nil } + // Refresh needed — resolve token_url/client_id. Explicit config or + // a persisted registration only; the refresh path never runs + // discovery/DCR (allowDiscovery=false) — a first-time mint requires + // interactive `forge mcp login`. #316 + resolved, rerr := f.resolveOAuthConfig(ctx, name, cfg, false) + if rerr != nil { + return "", rerr + } + cfg = resolved + // Singleflight: collapse concurrent refreshes. The leader spawns // the refresh goroutine and EVERY subsequent caller — including // the leader itself — waits on grp.done below. diff --git a/forge-core/mcp/server.go b/forge-core/mcp/server.go index 38d84da7..0d9cc7b4 100644 --- a/forge-core/mcp/server.go +++ b/forge-core/mcp/server.go @@ -136,11 +136,17 @@ func NewServer(spec types.MCPServer, deps ServerDeps) (*Server, error) { return nil, fmt.Errorf("%w: server %q: auth.token_env is required for type=%s", ErrProtocolError, spec.Name, spec.Auth.Type) } case "oauth": - if spec.Auth.ClientID == "" { - return nil, fmt.Errorf("%w: server %q: auth.client_id is required for oauth", ErrProtocolError, spec.Name) + // #316: the endpoints + client_id may be discovered + // (RFC 9728/8414/7591) from the server URL, so the trio is no + // longer required. Only reject a PARTIAL endpoint config — + // authorize_url and token_url must be set together or both + // omitted (both-omitted ⇒ discovery). A server with no URL + // and no endpoints has nothing to discover from. + if (spec.Auth.AuthorizeURL == "") != (spec.Auth.TokenURL == "") { + return nil, fmt.Errorf("%w: server %q: auth.authorize_url and auth.token_url must be set together (or both omitted for discovery)", ErrProtocolError, spec.Name) } - if spec.Auth.AuthorizeURL == "" || spec.Auth.TokenURL == "" { - return nil, fmt.Errorf("%w: server %q: auth.authorize_url and auth.token_url are required for oauth", ErrProtocolError, spec.Name) + if spec.Auth.AuthorizeURL == "" && spec.URL == "" { + return nil, fmt.Errorf("%w: server %q: oauth needs either explicit authorize_url/token_url or a url to discover them from", ErrProtocolError, spec.Name) } if deps.OAuth == nil { return nil, fmt.Errorf("%w: server %q requires oauth but no OAuthFlow supplied", ErrProtocolError, spec.Name) @@ -513,6 +519,7 @@ func buildAuthFn(spec types.MCPServer, flow *OAuthFlow) AuthTokenFunc { } case "oauth": cfg := OAuthServerConfig{ + ServerURL: spec.URL, // for #316 discovery / persisted-registration lookup ClientID: spec.Auth.ClientID, Scopes: spec.Auth.Scopes, AuthorizeURL: spec.Auth.AuthorizeURL, diff --git a/forge-core/mcp/server_b6_test.go b/forge-core/mcp/server_b6_test.go index d1baa555..57991e36 100644 --- a/forge-core/mcp/server_b6_test.go +++ b/forge-core/mcp/server_b6_test.go @@ -78,9 +78,11 @@ func TestB6_NewServer_RequiresTokenEnvForBearerStatic(t *testing.T) { } } -// TestB6_NewServer_RequiresOAuthFields covers the other adjacent -// failure modes — same shape of bug for oauth. -func TestB6_NewServer_RequiresOAuthFields(t *testing.T) { +// TestB6_NewServer_RejectsPartialOAuthEndpoints covers the adjacent +// failure mode after #316: the endpoints may be discovered, so the trio +// is no longer required — but a PARTIAL endpoint config (one of +// authorize_url/token_url set, the other empty) is still rejected. +func TestB6_NewServer_RejectsPartialOAuthEndpoints(t *testing.T) { t.Parallel() full := &types.MCPAuth{ Type: "oauth", ClientID: "c", @@ -91,9 +93,8 @@ func TestB6_NewServer_RequiresOAuthFields(t *testing.T) { mutate func(*types.MCPAuth) wantSub string }{ - {"missing client_id", func(a *types.MCPAuth) { a.ClientID = "" }, "client_id is required"}, - {"missing authorize_url", func(a *types.MCPAuth) { a.AuthorizeURL = "" }, "authorize_url and auth.token_url are required"}, - {"missing token_url", func(a *types.MCPAuth) { a.TokenURL = "" }, "authorize_url and auth.token_url are required"}, + {"only token_url", func(a *types.MCPAuth) { a.AuthorizeURL = "" }, "must be set together"}, + {"only authorize_url", func(a *types.MCPAuth) { a.TokenURL = "" }, "must be set together"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -115,6 +116,21 @@ func TestB6_NewServer_RequiresOAuthFields(t *testing.T) { } } +// TestB6_NewServer_AcceptsDiscoveryOAuth: with #316, an oauth server +// that omits client_id AND both endpoints (relying on discovery from +// the url) constructs successfully. +func TestB6_NewServer_AcceptsDiscoveryOAuth(t *testing.T) { + t.Parallel() + _, err := NewServer(types.MCPServer{ + Name: "oa", Transport: "http", URL: "https://mcp.example.com/mcp", + Auth: &types.MCPAuth{Type: "oauth", Scopes: []string{"read"}}, + Tools: types.MCPToolFilter{Allow: []string{"x"}}, + }, ServerDeps{HTTPClient: http.DefaultClient, OAuth: NewOAuthFlow()}) + if err != nil { + t.Fatalf("discovery-based oauth should construct, got: %v", err) + } +} + // TestB6_NewServer_AcceptsKnownTypes_SanityCheck pins the inverse // of the rejection tests: every legal Auth.Type still constructs. func TestB6_NewServer_AcceptsKnownTypes_SanityCheck(t *testing.T) { diff --git a/forge-core/types/config.go b/forge-core/types/config.go index 2ebff847..5706da7f 100644 --- a/forge-core/types/config.go +++ b/forge-core/types/config.go @@ -583,29 +583,33 @@ type MCPToolFilter struct { type MCPAuth struct { // Type is one of: // - "oauth" → OAuth 2.1 PKCE; tokens stored in MCPConfig.TokenStorePath. - // Requires ClientID, AuthorizeURL, TokenURL. - // Use `forge mcp login ` once at laptop time. + // ClientID / AuthorizeURL / TokenURL are OPTIONAL: when + // omitted, Forge discovers them from the server URL via + // RFC 9728 / RFC 8414 and dynamically registers a client + // (RFC 7591) at first `forge mcp login` (#316). Set them + // explicitly to override discovery. // - "bearer" → static Bearer token from env var TokenEnv. // - "static" → same as bearer; named separately for clarity in // forge.yaml. Type string `yaml:"type"` - // ClientID is the OAuth client identifier registered with the MCP - // server's authorization service. Required when Type == "oauth". + // ClientID is the OAuth client identifier. Optional for Type == + // "oauth": when empty and the authorization server supports RFC 7591, + // Forge mints one at login and persists it (encrypted) for reuse. ClientID string `yaml:"client_id,omitempty"` - // Scopes is the OAuth scope set requested at login. + // Scopes is the OAuth scope set requested at login. When empty, + // discovery uses the server's advertised scopes_supported. Scopes []string `yaml:"scopes,omitempty"` - // AuthorizeURL is the OAuth 2.1 authorization endpoint (where - // `forge mcp login` opens the browser). Required when Type == - // "oauth". Phase 1 requires this be explicit; Phase 1.5 will add - // RFC 9728 / RFC 8414 automated discovery via the MCP server URL. + // AuthorizeURL is the OAuth 2.1 authorization endpoint. Optional for + // Type == "oauth": discovered via RFC 8414 when omitted. Must be set + // together with TokenURL (or both omitted for discovery). AuthorizeURL string `yaml:"authorize_url,omitempty"` - // TokenURL is the OAuth 2.1 token endpoint (where authorization - // codes and refresh tokens are exchanged). Required when - // Type == "oauth". + // TokenURL is the OAuth 2.1 token endpoint. Optional for Type == + // "oauth": discovered via RFC 8414 when omitted. Must be set together + // with AuthorizeURL. TokenURL string `yaml:"token_url,omitempty"` // TokenEnv names the environment variable holding the bearer diff --git a/forge-core/validate/mcp_config.go b/forge-core/validate/mcp_config.go index f1fb928b..55634a67 100644 --- a/forge-core/validate/mcp_config.go +++ b/forge-core/validate/mcp_config.go @@ -127,14 +127,12 @@ func validateMCPAuth(prefix string, a types.MCPAuth, r *ValidationResult) { switch a.Type { case "oauth": - if a.ClientID == "" { - r.Errors = append(r.Errors, prefix+": client_id is required for oauth") - } - if a.AuthorizeURL == "" { - r.Errors = append(r.Errors, prefix+": authorize_url is required for oauth") - } - if a.TokenURL == "" { - r.Errors = append(r.Errors, prefix+": token_url is required for oauth") + // #316: client_id + endpoints may be discovered (RFC 9728/8414/7591) + // from the server url, so the trio is no longer required. Only a + // PARTIAL endpoint config is an error — authorize_url and token_url + // must be set together, or both omitted (⇒ discovery from url). + if (a.AuthorizeURL == "") != (a.TokenURL == "") { + r.Errors = append(r.Errors, prefix+": authorize_url and token_url must be set together (or both omitted to discover them from the server url)") } case "bearer", "static": if a.TokenEnv == "" { diff --git a/forge-core/validate/mcp_config_test.go b/forge-core/validate/mcp_config_test.go index 751a70df..20667578 100644 --- a/forge-core/validate/mcp_config_test.go +++ b/forge-core/validate/mcp_config_test.go @@ -97,7 +97,9 @@ func TestValidateMCPConfig(t *testing.T) { wantErrs: []string{"duplicate name"}, }, { - name: "auth oauth without client_id", + // #316: client_id may be omitted (dynamic client registration), + // with both endpoints explicit — valid, no error. + name: "auth oauth without client_id (DCR) is allowed", cfg: types.MCPConfig{Servers: []types.MCPServer{{ Name: "x", Transport: "http", URL: "http://x", Auth: &types.MCPAuth{ @@ -107,10 +109,21 @@ func TestValidateMCPConfig(t *testing.T) { }, Tools: types.MCPToolFilter{Allow: []string{"y"}}, }}}, - wantErrs: []string{"client_id is required"}, + wantNoErr: true, + }, + { + // #316: both endpoints omitted ⇒ discover from the server url. Valid. + name: "auth oauth full discovery (no endpoints, no client_id) is allowed", + cfg: types.MCPConfig{Servers: []types.MCPServer{{ + Name: "x", Transport: "http", URL: "https://mcp.example.com/mcp", + Auth: &types.MCPAuth{Type: "oauth", Scopes: []string{"read"}}, + Tools: types.MCPToolFilter{Allow: []string{"y"}}, + }}}, + wantNoErr: true, }, { - name: "auth oauth without authorize_url", + // Partial endpoint config is still an error — must be paired. + name: "auth oauth with only token_url (partial) is an error", cfg: types.MCPConfig{Servers: []types.MCPServer{{ Name: "x", Transport: "http", URL: "http://x", Auth: &types.MCPAuth{ @@ -120,10 +133,10 @@ func TestValidateMCPConfig(t *testing.T) { }, Tools: types.MCPToolFilter{Allow: []string{"y"}}, }}}, - wantErrs: []string{"authorize_url is required"}, + wantErrs: []string{"must be set together"}, }, { - name: "auth oauth without token_url", + name: "auth oauth with only authorize_url (partial) is an error", cfg: types.MCPConfig{Servers: []types.MCPServer{{ Name: "x", Transport: "http", URL: "http://x", Auth: &types.MCPAuth{ @@ -133,7 +146,7 @@ func TestValidateMCPConfig(t *testing.T) { }, Tools: types.MCPToolFilter{Allow: []string{"y"}}, }}}, - wantErrs: []string{"token_url is required"}, + wantErrs: []string{"must be set together"}, }, { name: "auth bearer without token_env", From 534b6a28330507bc11fcb9a2637d3bbae8c0a7ba Mon Sep 17 00:00:00 2001 From: MK Date: Thu, 16 Jul 2026 18:06:54 -0400 Subject: [PATCH 2/3] =?UTF-8?q?fix(mcp):=20address=20#320=20review=20?= =?UTF-8?q?=E2=80=94=20don't=20persist=20DCR=20secret,=20pin=20refresh=20f?= =?UTF-8?q?ail-closed,=20multi-AS?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (security, merge-gating): a DCR confidential-client secret was persisted (and, without a passphrase, in the plaintext-fallback store) yet never used — the token path is public-client PKCE. Stop persisting it entirely: drop `ClientSecret` from the registration record and from OAuthServerConfig, and **fail closed** when an AS issues a confidential client (we register `token_endpoint_auth_method: none`, so a secret means we can't authenticate it anyway) — pointing the operator at explicit config. No secret ever reaches the store. Finding 3 (test + comment): pin the refresh-path fail-closed invariant. A partially-materialized config (client_id present, endpoints empty) on the refresh path (allowDiscovery=false) now has a test asserting it returns ErrNoToken and mints nothing — and a comment documents the asymmetry (the login path with the same partial config would fall through to DCR; "partial ⇒ fail-closed" is a refresh-path property). Finding 2 (docs): reframe explicit config as the standalone default's override AND the platform-materialized path (the common case under managed provisioning), not only the exception. Minors: - Multi-AS: RFC 9728 advertises a list; discovery now tries every authorization_servers entry (was [0] only) so a dead primary still completes. - SSRF note: comment that the login-time discovery fetch of server-controlled URLs rides the plain client (laptop-side; the runtime/refresh path is egress-enforced). - Recovery: `Logout` now also deletes the registration record, so `forge mcp logout` + `login` is the re-discover/re-register path after a client is revoked; documented in configuration.md. Tests: confidential-client fail-closed (nothing persisted), refresh-partial-config fail-closed (no mint). Docs updated. --- docs/mcp/configuration.md | 19 ++++++- forge-core/mcp/oauth_discovery.go | 76 ++++++++++++++++++-------- forge-core/mcp/oauth_discovery_test.go | 46 ++++++++++++++++ forge-core/mcp/oauth_flow.go | 14 ++++- 4 files changed, 126 insertions(+), 29 deletions(-) diff --git a/docs/mcp/configuration.md b/docs/mcp/configuration.md index 848852f9..cd3e3544 100644 --- a/docs/mcp/configuration.md +++ b/docs/mcp/configuration.md @@ -103,9 +103,13 @@ So a fully zero-config OAuth server is just: **Precedence & rules:** -- **Explicit config always wins.** Set `client_id`/`authorize_url`/`token_url` - to override discovery (for servers that don't advertise metadata or - don't support DCR). +- **Discovery is the standalone default; explicit config always wins.** + Setting `client_id`/`authorize_url`/`token_url` is both the *static + override* (for servers that don't advertise metadata or don't support + DCR) **and the platform-materialized path** — the normal case when a + control plane materializes these fields from a registry entry. Explicit + config is not just the exception; under managed/admission-time + provisioning it is the common case. - `authorize_url` and `token_url` must be set **together** (or both omitted); a partial pair is a validation error. - **Fail-closed:** if a server advertises no metadata / no @@ -117,6 +121,15 @@ So a fully zero-config OAuth server is just: login-time registration record and merged into the egress allowlist at runtime automatically. (Discovery itself runs at laptop-time `forge mcp login`, off the egress-enforced path.) +- **Recovery (revoked/expired client):** a dynamically-registered client + is minted once and never re-minted. If the authorization server revokes + it (or `client_secret_expires_at` passes), run `forge mcp logout ` + — that clears both the token and the stored registration — then + `forge mcp login ` again to re-discover and re-register. +- **Confidential clients are not supported.** Forge registers a public + (PKCE) client and sends no `client_secret`. If a server insists on + issuing a confidential client, login fails closed — configure + `client_id`/`authorize_url`/`token_url` explicitly for that server. ### `mcp.servers[].tools` diff --git a/forge-core/mcp/oauth_discovery.go b/forge-core/mcp/oauth_discovery.go index 84e03d44..fcbe750e 100644 --- a/forge-core/mcp/oauth_discovery.go +++ b/forge-core/mcp/oauth_discovery.go @@ -52,8 +52,12 @@ type oauthRegistration struct { TokenURL string `json:"token_url"` RegistrationURL string `json:"registration_url,omitempty"` ClientID string `json:"client_id"` - ClientSecret string `json:"client_secret,omitempty"` // if the AS issued a confidential client Scopes []string `json:"scopes,omitempty"` + // NOTE: a DCR client_secret is deliberately NOT persisted (#320 + // review, finding 1). We register as a public PKCE client and the + // token path sends no secret, so a secret would be stored (possibly + // in the plaintext-fallback store) for no benefit — pure liability. + // A confidential client is refused at resolve time instead. } // loopbackRedirectURIs are what DCR registers for laptop-time login. @@ -107,6 +111,12 @@ func (f *OAuthFlow) resolveOAuthConfig(ctx context.Context, name string, cfg OAu return mergeRegistration(cfg, reg), nil } + // Fail-closed asymmetry (#320 review, finding 3): the refresh path + // (BearerToken → allowDiscovery=false) stops here. So a + // partially-materialized config in a fresh pod — e.g. a platform env + // var came through empty, so step 1 didn't match and no record + // exists — fails closed and NEVER mints a divergent client. Only the + // interactive login path (allowDiscovery=true) falls through to DCR. if !allowDiscovery { return cfg, fmt.Errorf("%w: no oauth endpoints for %q and no stored registration — run 'forge mcp login %s'", ErrNoToken, name, name) } @@ -120,7 +130,7 @@ func (f *OAuthFlow) resolveOAuthConfig(ctx context.Context, name string, cfg OAu return cfg, err } - clientID, clientSecret := cfg.ClientID, cfg.ClientSecret + clientID := cfg.ClientID if clientID == "" { if meta.RegistrationEndpoint == "" { return cfg, fmt.Errorf("%w: server %q advertises no registration_endpoint and no client_id is configured — supply client_id/authorize_url/token_url, or use a server that supports dynamic client registration", ErrProtocolError, name) @@ -129,10 +139,19 @@ func (f *OAuthFlow) resolveOAuthConfig(ctx context.Context, name string, cfg OAu if len(scopes) == 0 { scopes = meta.ScopesSupported } + var clientSecret string clientID, clientSecret, err = f.registerClient(ctx, meta.RegistrationEndpoint, scopes) if err != nil { return cfg, err } + if clientSecret != "" { + // We register as a public PKCE client (token_endpoint_auth_method + // = none) and the token path sends no client_secret. If the AS + // ignored that and issued a confidential client, we can't + // authenticate it — and we refuse to persist a secret (#320 + // finding 1). Fail closed rather than half-register. + return cfg, fmt.Errorf("%w: server %q issued a confidential client (client_secret); Forge supports only public PKCE clients for MCP OAuth — configure client_id/authorize_url/token_url explicitly for this server", ErrProtocolError, name) + } } reg = oauthRegistration{ @@ -140,7 +159,6 @@ func (f *OAuthFlow) resolveOAuthConfig(ctx context.Context, name string, cfg OAu TokenURL: firstNonEmpty(cfg.TokenURL, meta.TokenEndpoint), RegistrationURL: meta.RegistrationEndpoint, ClientID: clientID, - ClientSecret: clientSecret, Scopes: cfg.Scopes, } if reg.AuthorizeURL == "" || reg.TokenURL == "" { @@ -156,7 +174,6 @@ func (f *OAuthFlow) resolveOAuthConfig(ctx context.Context, name string, cfg OAu // registration. Explicit cfg fields are never overwritten (precedence). func mergeRegistration(cfg OAuthServerConfig, reg oauthRegistration) OAuthServerConfig { cfg.ClientID = firstNonEmpty(cfg.ClientID, reg.ClientID) - cfg.ClientSecret = firstNonEmpty(cfg.ClientSecret, reg.ClientSecret) cfg.AuthorizeURL = firstNonEmpty(cfg.AuthorizeURL, reg.AuthorizeURL) cfg.TokenURL = firstNonEmpty(cfg.TokenURL, reg.TokenURL) if len(cfg.Scopes) == 0 { @@ -167,48 +184,61 @@ func mergeRegistration(cfg OAuthServerConfig, reg oauthRegistration) OAuthServer // discoverAuthServer runs RFC 9728 → RFC 8414 from the MCP server URL // and returns the authorization-server metadata. +// +// SECURITY (#320 review, minor): the RFC 9728 result — the +// `resource_metadata` pointer and the authorization_servers list — is +// server-controlled, and these fetches run with the plain 15s client at +// laptop-time login (the runtime/refresh path IS egress-enforced). A +// hostile MCP url could thus steer the login-time fetch at an internal +// metadata endpoint. Low severity (operator-initiated, laptop-side, +// byte/timeout-bounded), noted so it isn't mistaken for egress-enforced. func (f *OAuthFlow) discoverAuthServer(ctx context.Context, serverURL string) (authServerMetadata, error) { var zero authServerMetadata - asURL, err := f.discoverProtectedResource(ctx, serverURL) + authServers, err := f.discoverProtectedResource(ctx, serverURL) if err != nil { return zero, err } - // RFC 8414: {issuer}/.well-known/oauth-authorization-server, with - // the OpenID variant as a fallback for servers that only publish + // RFC 9728 advertises a LIST of authorization servers; try each in + // order so a multi-AS resource with a dead primary still completes. + // RFC 8414: {issuer}/.well-known/oauth-authorization-server, with the + // OpenID variant as a fallback for servers that only publish // openid-configuration. - for _, wk := range []string{ - wellKnown(asURL, "oauth-authorization-server"), - wellKnown(asURL, "openid-configuration"), - } { - meta, err := fetchJSON[authServerMetadata](ctx, f.httpClient(), wk) - if err == nil && meta.TokenEndpoint != "" && meta.AuthorizationEndpoint != "" { - return meta, nil + for _, asURL := range authServers { + for _, wk := range []string{ + wellKnown(asURL, "oauth-authorization-server"), + wellKnown(asURL, "openid-configuration"), + } { + meta, err := fetchJSON[authServerMetadata](ctx, f.httpClient(), wk) + if err == nil && meta.TokenEndpoint != "" && meta.AuthorizationEndpoint != "" { + return meta, nil + } } } - return zero, fmt.Errorf("%w: no usable authorization-server metadata at %s", ErrProtocolError, asURL) + return zero, fmt.Errorf("%w: no usable authorization-server metadata for %s (tried %d server(s))", ErrProtocolError, serverURL, len(authServers)) } -// discoverProtectedResource resolves the authorization server for an -// MCP resource (RFC 9728). It first tries the well-known path derived -// from the server URL; if the server instead answers 401 with a -// WWW-Authenticate `resource_metadata` pointer, that is honored too. -func (f *OAuthFlow) discoverProtectedResource(ctx context.Context, serverURL string) (string, error) { +// discoverProtectedResource resolves the authorization server(s) for an +// MCP resource (RFC 9728), returning the full authorization_servers +// list. It first tries the well-known path derived from the server URL; +// if the server instead answers 401 with a WWW-Authenticate +// `resource_metadata` pointer, that is honored too. +func (f *OAuthFlow) discoverProtectedResource(ctx context.Context, serverURL string) ([]string, error) { // Primary: the well-known path off the server origin. prURL := wellKnown(serverURL, "oauth-protected-resource") if pr, err := fetchJSON[protectedResourceMetadata](ctx, f.httpClient(), prURL); err == nil && len(pr.AuthorizationServers) > 0 { - return pr.AuthorizationServers[0], nil + return pr.AuthorizationServers, nil } // Fallback: probe the server itself and read the 401 // WWW-Authenticate `resource_metadata` param (RFC 9728 §5.1). if rm := f.probeResourceMetadataURL(ctx, serverURL); rm != "" { if pr, err := fetchJSON[protectedResourceMetadata](ctx, f.httpClient(), rm); err == nil && len(pr.AuthorizationServers) > 0 { - return pr.AuthorizationServers[0], nil + return pr.AuthorizationServers, nil } } - return "", fmt.Errorf("%w: could not discover an authorization server for %s (no RFC 9728 protected-resource metadata)", ErrProtocolError, serverURL) + return nil, fmt.Errorf("%w: could not discover an authorization server for %s (no RFC 9728 protected-resource metadata)", ErrProtocolError, serverURL) } // probeResourceMetadataURL makes an unauthenticated request to the MCP diff --git a/forge-core/mcp/oauth_discovery_test.go b/forge-core/mcp/oauth_discovery_test.go index 090238f2..fd50cc1e 100644 --- a/forge-core/mcp/oauth_discovery_test.go +++ b/forge-core/mcp/oauth_discovery_test.go @@ -29,6 +29,7 @@ type discoveryServer struct { regCalls atomic.Int32 noPRWellKnown bool // suppress /.well-known/oauth-protected-resource (force WWW-Authenticate path) noRegEndpoint bool // omit registration_endpoint from AS metadata + confidential bool // /register returns a client_secret (confidential client) } func newDiscoveryServer(t *testing.T) *discoveryServer { @@ -72,6 +73,10 @@ func newDiscoveryServer(t *testing.T) *discoveryServer { mux.HandleFunc("/register", func(w http.ResponseWriter, _ *http.Request) { d.regCalls.Add(1) w.WriteHeader(http.StatusCreated) + if d.confidential { + _, _ = w.Write([]byte(`{"client_id":"dyn-client-123","client_secret":"sh-sh-secret"}`)) + return + } _, _ = w.Write([]byte(`{"client_id":"dyn-client-123"}`)) }) @@ -192,6 +197,47 @@ func TestResolveOAuthConfig_RefreshNoDiscovery(t *testing.T) { } } +// TestResolveOAuthConfig_RefreshPartialConfigFailsClosed pins the +// managed-drift invariant (#320 review, finding 3): on the refresh path, +// a partially-materialized config (e.g. client_id came through but the +// endpoint env vars were empty) must fail closed and NEVER mint a +// divergent client — even though the same partial config on the login +// path would fall through to DCR. +func TestResolveOAuthConfig_RefreshPartialConfigFailsClosed(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + f := NewOAuthFlow() + + partial := OAuthServerConfig{ServerURL: d.url(), ClientID: "materialized-but-endpoints-empty"} + _, err := f.resolveOAuthConfig(context.Background(), "srv", partial, false) // refresh path + if err == nil || !strings.Contains(err.Error(), "forge mcp login") { + t.Fatalf("partial config on the refresh path must fail closed; got: %v", err) + } + if n := d.regCalls.Load(); n != 0 { + t.Errorf("refresh path minted a client for a partial config (%d DCR calls, want 0)", n) + } +} + +// TestResolveOAuthConfig_ConfidentialClientFailsClosed: if the AS issues +// a confidential client (client_secret) despite our public-client +// registration, resolve fails closed and persists nothing (#320 review, +// finding 1 — no secret ever hits the store). +func TestResolveOAuthConfig_ConfidentialClientFailsClosed(t *testing.T) { + withTempCredsDir(t) + d := newDiscoveryServer(t) + d.confidential = true + f := NewOAuthFlow() + + _, err := f.resolveOAuthConfig(context.Background(), "srv", OAuthServerConfig{ServerURL: d.url()}, true) + if err == nil || !strings.Contains(err.Error(), "confidential client") { + t.Fatalf("want a fail-closed confidential-client error, got: %v", err) + } + // Nothing persisted → no host learned back either. + if h := RegisteredOAuthHosts([]string{"srv"}); h != nil { + t.Errorf("a confidential-client failure persisted a registration: %v", h) + } +} + // TestRegisteredOAuthHosts: after discovery persists a registration, the // egress helper returns the authorize/token/registration hosts. func TestRegisteredOAuthHosts(t *testing.T) { diff --git a/forge-core/mcp/oauth_flow.go b/forge-core/mcp/oauth_flow.go index d4cd8c39..3026450f 100644 --- a/forge-core/mcp/oauth_flow.go +++ b/forge-core/mcp/oauth_flow.go @@ -95,7 +95,6 @@ type OAuthServerConfig struct { // discovery (#316) when the endpoints below are not configured. ServerURL string ClientID string - ClientSecret string // set only when DCR issued a confidential client Scopes []string AuthorizeURL string TokenURL string @@ -402,9 +401,18 @@ func (f *OAuthFlow) refreshTimeout() time.Duration { return 30 * time.Second } -// Logout deletes the stored token for an MCP server. Idempotent. +// Logout deletes the stored token AND the discovery/registration record +// for an MCP server. Clearing the registration (#320 review) is the +// recovery path when an authorization server revokes the dynamically +// registered client: the next `forge mcp login` re-discovers and +// re-registers from scratch. Idempotent. func (f *OAuthFlow) Logout(name string) error { - return oauth.DeleteCredentials(storeKey(name)) + tokErr := oauth.DeleteCredentials(storeKey(name)) + regErr := oauth.DeleteRecord(regStoreKey(name)) + if tokErr != nil { + return tokErr + } + return regErr } func (f *OAuthFlow) emit(server string, ok bool, reason string) { From 63f0f06af53a187a9ca4ccbc243487d3e7522574 Mon Sep 17 00:00:00 2001 From: MK Date: Thu, 16 Jul 2026 19:34:53 -0400 Subject: [PATCH 3/3] fix(mcp): `mcp test` honors token_store_path like login/logout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mcp login` and `mcp logout` apply the MCP token-store-path override (forge.yaml `mcp.token_store_path` > env `MCP_TOKEN_STORE_PATH`) via oauth.SetCredentialsDir before touching the store, but `mcp test` did not — so `test` always read the default ~/.forge/credentials and reported "no stored token" for any server logged in under a custom store path. Apply the same override in mcp test. Found while manually testing the #316 discovery flow with an isolated token store. --- forge-cli/cmd/mcp_test_cmd.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/forge-cli/cmd/mcp_test_cmd.go b/forge-cli/cmd/mcp_test_cmd.go index ebde9891..8011a171 100644 --- a/forge-cli/cmd/mcp_test_cmd.go +++ b/forge-cli/cmd/mcp_test_cmd.go @@ -7,6 +7,7 @@ import ( "os" "time" + "github.com/initializ/forge/forge-core/llm/oauth" "github.com/initializ/forge/forge-core/mcp" "github.com/spf13/cobra" ) @@ -32,6 +33,15 @@ func mcpTestRun(cmd *cobra.Command, args []string) error { return err } + // Apply the token-store-path override (forge.yaml > env) so `mcp test` + // reads OAuth tokens from the same location `mcp login` wrote them — + // mirroring mcp_login.go / mcp_logout.go. Without this, `test` always + // looked in the default ~/.forge/credentials and reported "no stored + // token" for any server logged in under MCP_TOKEN_STORE_PATH. + if path := loginTokenStorePath(cfg); path != "" { + oauth.SetCredentialsDir(path) + } + ctx, cancel := context.WithTimeout(context.Background(), 2*timeout+5*time.Second) defer cancel()