From 793a4c1f20c0e9ba5d0e72602225706cd9c6300f Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sat, 3 Oct 2026 21:16:58 -0400 Subject: [PATCH 1/2] feat!: the MCP public URL is derived, and the host issuer is declared configuration (#48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ServeMCP refused to boot a deployed solution unless MCP_PUBLIC_URL and HOST_ISSUER_URL were set — fail-closed, which is right, naming two things no composition can supply. A Codefly render projects endpoints and declared configuration onto a service; it cannot set a bare environment variable on one. The wiki's README consequently told an operator to "set MCP_PUBLIC_URL in a deployment", which describes nothing that exists. The public MCP URL is now derived by construction from the resolved PUBLIC_URL and this solution's id — /solutions//mcp — so a composition declares nothing for it, and a deployment that already names the origin browsers reach the product at has addressed the MCP surface too. It is the one place the gateway's solution route is encoded: a client's token is audience-bound to that exact string (RFC 8707) and nothing downstream can resolve it later the way the host resolves a relative manifest URL. The override survives as mcp/public-url, for a host that routes differently. The issuer becomes the one value a composition supplies: mcp/issuer-url, resolved with codefly.For(ctx).WorkspaceConfiguration exactly as the solution-registration secret is, and refused by group/key — never by a variable name — in a deployed runtime context when absent. It stops being the composition's to supply once module-saas-starter#1003 settles what `iss` is. Both bare variables are gone, which is the break: a deployment that set HOST_ISSUER_URL is refused at boot until the value moves into the mcp group, and one that set MCP_PUBLIC_URL gets the derived identifier instead of its own. Exported surface only grows (MCPConfigurationGroup, MCPIssuerURLKey, MCPPublicURLKey); MCPEnvironment is unchanged. Tests drive the derivation, the two declared values and both deployed-context refusals through the SDK's own accessors, against the carrier a render projects — a fake in its place would only prove the runtime agrees with the fake about an encoding the SDK owns. README's configuration table, "three lines", and the resource-identifier section, and AGENTS.md's statement of the override contract, describe the new behaviour. Closes #48 Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 21 ++++- README.md | 91 ++++++++++++++------ mcp.go | 191 +++++++++++++++++++++++++++++++----------- mcp_test.go | 235 ++++++++++++++++++++++++++++++++++++++++++++++++++-- solution.go | 34 ++++---- 5 files changed, 472 insertions(+), 100 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7c7a8cc..0af8a53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,9 +32,17 @@ own: it builds on `Server.PassthroughHandler`, the handler `Serve` mounts. change a signature, a struct field or an exported constant; a change that breaks a consumer carries a `!` in the commit type and says so in the PR body. - Boot configuration is resolved in one place, `loadConfig`, and checked in one - place, `validate()`: resolved through the SDK, with an explicit env override, - refused loudly when unresolved. New configuration follows both rather than - reading the environment where it is used. + place, `validate()`: resolved through the SDK, refused loudly when unresolved. + New configuration follows both rather than reading the environment where it is + used. Most values also take an explicit env override, for an operator whose + deployment the resolver cannot see — but an override is not how a *composition* + supplies a value, because a Codefly render cannot set a bare environment + variable on a service. Anything the composition has to supply is a declared + workspace-configuration group read with `WorkspaceConfiguration` / + `WorkspaceSecret` (`solution-registration/SECRET`, `mcp/issuer-url`, + `mcp/public-url`), and a refusal names that group/key, never a variable name. + `MCP_PUBLIC_URL` and `HOST_ISSUER_URL` were bare variables for one release and + the boot refused with the name of something nobody could set (#48). - The exception is the consumed-API projection, read at serve time in `registerConsumedAPIs` and never seen by `validate()`. A malformed one is not refused: the runtime logs that the federation is disabled and serves on, so @@ -99,7 +107,12 @@ repository is the runtime that cost was measured against. port and refused by `validate()` in a deployed runtime context. Both token-exchange URLs are derived from their register URLs by swapping the path suffix, which is why an override that drops the documented suffix cannot be paired and is - refused at boot rather than guessed at. If you are typing an address, a port + refused at boot rather than guessed at. The MCP resource identifier is + derived the same way — from the resolved `PUBLIC_URL` and the solution id — + and is the one place the gateway's `/solutions/` route is encoded, because + a client's token is audience-bound to that exact string and nothing + downstream can resolve it later the way the host resolves a relative manifest + URL. README says so where it says the route layout is not encoded. If you are typing an address, a port or a credential, you are encoding something true only on your machine for the next ten minutes. 5. **Diagnose, do not pattern-match.** "It started working when I set X" is not diff --git a/README.md b/README.md index 798f271..4ddbe73 100644 --- a/README.md +++ b/README.md @@ -43,8 +43,16 @@ The runtime hardcodes nothing. On boot `Serve` calls `codefly.LoadEnvironmentVariables()` to load Codefly's injected carriers, then `loadConfig` resolves every address, port, and secret through the codefly Go SDK (`github.com/codefly-dev/sdk-go`), falling back to the local native workspace -map when not running under the runtime. Each value has an explicit env override; -the SDK-resolved value is the default. +map when not running under the runtime. Most values have an explicit env +override for an operator with a deployment the resolver cannot see; the +SDK-resolved value is the default. + +The two MCP values have none, because a Codefly composition cannot set a bare +environment variable on a service: every value a service receives is either an +endpoint the SDK resolves or a **declared configuration** the render projects. +Their override is therefore a value in the `mcp` workspace-configuration group, +resolved through the SDK the way the registration secret is — see [The resource +identifier and the issuer](#the-resource-identifier-and-the-issuer). | What | SDK resolution | Env override (default) | |---|---|---| @@ -63,8 +71,8 @@ the SDK-resolved value is the default. | Self upstream | the reachable self endpoint Codefly injects as `CODEFLY__SELF_ENDPOINT______HTTP__HTTP` (core ≥ v0.5.6); without it, the listen address `http://localhost:` | `SELF_UPSTREAM` | | Deployed or local | `CODEFLY__RUNTIME_CONTEXT`, injected by Codefly: `native`/`nix`/`container`/`free` (or unset) is a local run, anything else (a GitOps render's `kubernetes`, core ≥ v0.5.6) a deployment | — | | MF assets | `Manifest.Assets` when set (see below), else the `../fe-remote/dist` directory | `ASSETS_DIR` (directory only) | -| Host issuer (MCP) | the resolved host frontend origin — the host is the authorization server an MCP client authenticates against (see [Exposing an MCP server](#exposing-an-mcp-server)); checked at boot only when `ServeMCP` is declared | `HOST_ISSUER_URL` | -| Public MCP URL | none — the resource identifier is then reconstructed per request from `x-forwarded-proto` / `x-forwarded-host` / `x-forwarded-prefix` | `MCP_PUBLIC_URL` (must end in `/mcp`) | +| Host issuer (MCP) | `codefly.For(ctx).WorkspaceConfiguration("mcp", "issuer-url")` — the declared value the composition supplies; without one, the resolved host frontend origin (see [Exposing an MCP server](#exposing-an-mcp-server)). Checked at boot only when `ServeMCP` is declared | — (declared configuration, no env override) | +| Public MCP URL | derived by construction: `/solutions//mcp`. Overridden by `codefly.For(ctx).WorkspaceConfiguration("mcp", "public-url")` (must end in `/mcp`). With neither, the resource identifier is reconstructed per request from `x-forwarded-proto` / `x-forwarded-host` / `x-forwarded-prefix` | — (declared configuration, no env override) | The host it plugs into is named by Codefly-convention **service roles**, not by its workspace module name: the runtime discovers the single module that owns each @@ -89,7 +97,10 @@ callers, and neither is this process's listen address: on this backend, and the host resolves it against the route by which it reaches the solution — the runtime does not know, and does not encode, the host's route layout. `PUBLIC_URL` makes it absolute on that origin instead, - for an operator who exposes the solution's assets directly. + for an operator who exposes the solution's assets directly. (The one place the + runtime does encode that route is the MCP resource identifier, which has to be + byte-exact for a client and cannot be resolved by anyone else — see [The + resource identifier and the issuer](#the-resource-identifier-and-the-issuer).) - **`upstream`** (gateway registration) is dialled by the **gateway**. It is the address Codefly injects for reaching this service (`CODEFLY__SELF_ENDPOINT__…`, beside the `CODEFLY__ENDPOINT__…` carrier that @@ -564,6 +575,13 @@ solution.New(solution.Manifest{ID: "wiki", Title: "Wiki"}). Serve() ``` +A composition that renders this solution supplies **one** declared value for it, +and only in a deployment: `issuer-url` in the `mcp` workspace-configuration +group, declared as a workspace-configuration dependency of the solution's +backend — the origin MCP clients authenticate against. Everything else is +derived or resolved; see [The resource identifier and the +issuer](#the-resource-identifier-and-the-issuer). + `register` receives the official SDK's own `*mcp.Server` (`github.com/modelcontextprotocol/go-sdk`, pinned in this module's `go.mod`), so tools, prompts and resources are declared exactly as that SDK documents them and @@ -635,28 +653,51 @@ the document that challenge must point at is served here either way. The `resource` in the metadata document is what a client asks the authorization server for a token for (RFC 8707) and what its token is audience-bound to, so it must be the **public** MCP URL — not this process's listen address, not the -in-cluster address the gateway dials. The runtime cannot resolve it: the origin -is the host's, and the path is the host's route layout, which this runtime -deliberately does not encode (see -[Where the host reaches the solution](#where-the-host-reaches-the-solution)). - -So `MCP_PUBLIC_URL` is it when set, and must end in `/mcp` — the metadata +in-cluster address the gateway dials. A client that is handed any other +identifier rejects it, and unlike the manifest URL nobody downstream can resolve +it on the solution's behalf. + +So it is **derived by construction** from the two things the runtime already +has: the origin this product is reachable at and this solution's id — +`/solutions//mcp`. This is the one place the runtime encodes the +gateway's solution route (see [Where the host reaches the +solution](#where-the-host-reaches-the-solution)), and the reason it is worth +encoding is that it makes a composition declare nothing: a deployment that +already sets `PUBLIC_URL` to the origin browsers reach the product at has +addressed the MCP surface too. + +A host whose gateway fronts solutions under some other route declares +`public-url` in the `mcp` group instead. It must end in `/mcp` — the metadata document's own URL is derived from it by swapping that suffix, the same pairing the registration token URLs use, refused at boot for the same reason when it -cannot be made. Unset, the identifier is reconstructed per request from -`x-forwarded-proto`, `x-forwarded-host` and `x-forwarded-prefix`, and `ServeMCP` -logs at boot that it is doing so: a proxy that forwards no prefix yields an -identifier missing the path it stripped, which a conforming client rejects and a -tolerant one binds to the wrong resource. Set it in any deployment. - -`authorization_servers` is the host issuer resolved at boot — by role, like every -other host endpoint — and is never derived from a request, so a crafted `Host` -header cannot point a client at an authorization server of someone's choosing. -The resolved value is the address this composition reaches the host at: right for -a local run, and in a deployment an in-cluster address no public client could -reach, which is what `HOST_ISSUER_URL` is for. A solution that declares -`ServeMCP` and resolves no issuer is refused at boot; one that declares no MCP -server is unaffected. +cannot be made. With neither — no `PUBLIC_URL`, no declared override — the +identifier is reconstructed per request from `x-forwarded-proto`, +`x-forwarded-host` and `x-forwarded-prefix`, and `ServeMCP` logs at boot that it +is doing so: a proxy that forwards no prefix yields an identifier missing the +path it stripped, which a conforming client rejects and a tolerant one binds to +the wrong resource. In a deployed runtime context that state is refused at boot +rather than logged. + +`authorization_servers` is the host issuer resolved at boot, never derived from +a request, so a crafted `Host` header cannot point a client at an authorization +server of someone's choosing. It is `issuer-url` in the `mcp` group when the +composition declares one, and otherwise the host frontend's origin resolved by +role like every other host endpoint. That resolved value is the address this +composition reaches the host at: right for a local run, and in a deployment an +in-cluster address no public client could reach — so a deployed solution that +declares `ServeMCP` must supply `mcp`/`issuer-url`, and is refused at boot +naming it when it does not. It stops being the composition's to supply when +[module-saas-starter#1003](https://github.com/codefly-dev/module-saas-starter/issues/1003) +settles what `iss` is. A solution that declares `ServeMCP` and resolves no +issuer at all is refused at boot; one that declares no MCP server is unaffected. + +Both values are read with `codefly.For(ctx).WorkspaceConfiguration("mcp", …)`, +never from the environment. `MCP_PUBLIC_URL` and `HOST_ISSUER_URL` were bare +environment variables when `ServeMCP` landed, and a render cannot project one — +so the boot refused with the name of something no composition could set +(codefly-dev/solution-runtime-go#48). Provisioned locally with `codefly config +generate`, and from the cell's configuration when deployed, exactly as the +`solution-registration` group is. Three things about discovery belong to the host, not to this runtime, all of them tracked by diff --git a/mcp.go b/mcp.go index c253dd5..ab3bba9 100644 --- a/mcp.go +++ b/mcp.go @@ -8,6 +8,7 @@ import ( "net/url" "strings" + codefly "github.com/codefly-dev/sdk-go" "github.com/modelcontextprotocol/go-sdk/auth" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/modelcontextprotocol/go-sdk/oauthex" @@ -161,13 +162,14 @@ func (s *Server) mountMCP(mux *http.ServeMux) error { mux.Handle(ProtectedResourceMetadataPath, http.HandlerFunc(s.handleProtectedResourceMetadata)) if s.cfg.mcpPublicURL == "" { // The one signal that this is happening. The resource identifier a - // client binds its token to must be the URL the client dialled, and - // without MCP_PUBLIC_URL the runtime can only reconstruct it from what - // the proxy forwards: a gateway that forwards no prefix yields an - // identifier missing the path it stripped, which a conforming client - // rejects and a tolerant one silently binds to the wrong resource. - log.Printf("solution %q: MCP resource identifier derived per request from forwarded headers (%s unset): set it to the URL clients dial (%q) if this host's proxy does not forward %s", - s.manifest.ID, mcpPublicURLEnvironmentVariable, "https:///solutions/"+s.manifest.ID+MCPPath, forwardedPrefixHeader) + // client binds its token to must be the URL the client dialled; with + // no PUBLIC_URL to derive it from and no override declared, the runtime + // can only reconstruct it from what the proxy forwards, and a gateway + // that forwards no prefix yields an identifier missing the path it + // stripped — which a conforming client rejects and a tolerant one + // silently binds to the wrong resource. + log.Printf("solution %q: MCP resource identifier derived per request from forwarded headers (no PUBLIC_URL and no %s): set PUBLIC_URL to the origin clients reach this product at, so the identifier is %q", + s.manifest.ID, mcpConfigurationValue(MCPPublicURLKey), "https://"+gatewaySolutionsRoute+s.manifest.ID+MCPPath) } return nil } @@ -181,8 +183,9 @@ type MCPEnvironment struct { // IssuerURL is the host's OAuth issuer, published as the authorization // server in the metadata document. IssuerURL string - // PublicURL is the canonical public MCP URL, as Serve reads it from - // MCP_PUBLIC_URL. Empty derives it from each request's forwarded headers. + // PublicURL is the canonical public MCP URL, which Serve derives from the + // resolved PUBLIC_URL and this solution's id. Empty derives it from each + // request's forwarded headers, as it does when Serve has neither. PublicURL string } @@ -200,9 +203,11 @@ func (s *Server) MCPHandler(env MCPEnvironment) (http.Handler, error) { return nil, fmt.Errorf("solution %q declares no MCP server (ServeMCP)", s.manifest.ID) } s.cfg.gatewayURL, s.cfg.mcpIssuerURL, s.cfg.mcpPublicURL = env.GatewayURL, env.IssuerURL, env.PublicURL - // The caller named the issuer, so it is as explicit as the operator's - // override: what validate() refuses is an issuer nobody chose. + // The caller named both, so they are as explicit as the composition's + // declared values: what validate() refuses is an issuer nobody chose, and + // what a refusal has to name for a public URL is whoever set it. s.cfg.mcpIssuerExplicit = env.IssuerURL != "" + s.cfg.mcpPublicExplicit = env.PublicURL != "" s.cfg.mcp = true if err := s.cfg.validateMCP(); err != nil { return nil, fmt.Errorf("solution %q: %w", s.manifest.ID, err) @@ -402,10 +407,10 @@ func (s *Server) handleProtectedResourceMetadata(w http.ResponseWriter, r *http. // It is what the client asks the authorization server for a token for (RFC 8707) // and what the metadata document must name, so it has to be the public URL — not // this process's listen address and not the in-cluster address the gateway dials. -// The runtime cannot resolve it: the origin is the host's and the path prefix is -// the host's route layout, which this runtime deliberately does not encode (see -// frontendManifestURL). So it is the configured MCP_PUBLIC_URL, and otherwise -// reconstructed from what the proxy in front of this solution forwarded. +// So it is the public MCP URL resolved at boot: derived from PUBLIC_URL and this +// solution's id, or the override the composition declared (see +// resolveMCPPublicURL). With neither it is reconstructed from what the proxy in +// front of this solution forwarded, which validate() refuses in a deployment. func (s *Server) mcpResource(r *http.Request) string { if s.cfg.mcpPublicURL != "" { return s.cfg.mcpPublicURL @@ -465,8 +470,9 @@ func validHost(raw string) bool { // forwardedPrefix is the path the proxy stripped before this solution saw the // request — "/solutions/" at the host gateway. Empty when nothing forwarded -// one, which is the case MCP_PUBLIC_URL exists for: the alternative is to -// reconstruct the host's route layout here, and this runtime does not know it. +// one, which is why the identifier is derived at boot rather than per request +// wherever PUBLIC_URL resolves: a gateway that forwards no prefix leaves this +// the only input, and it has none. func forwardedPrefix(r *http.Request) string { prefix := strings.TrimRight(firstForwarded(r, forwardedPrefixHeader), "/") // A prefix that is not a plain absolute path is not a prefix: "//evil.test" @@ -508,22 +514,99 @@ func firstForwarded(r *http.Request, name string) string { // --- boot configuration --- +// MCPConfigurationGroup names the workspace-configuration group a composition +// declares for a solution that serves MCP, and MCPIssuerURLKey and +// MCPPublicURLKey the two values in it. They are resolved through the SDK the +// way the registration secret is (see SolutionRegistrationSecretGroup), because +// a Codefly composition has no way to set a bare environment variable on a +// service: every value a service receives is either an endpoint the SDK +// resolves or a declared configuration the render projects. Both were read as +// bare environment variables when ServeMCP landed, which named a provisioning +// path that does not exist. +// +// MCPIssuerURLKey is the origin MCP clients authenticate against — the host as +// an authorization server. A deployment must supply it: what the SDK resolves +// is the address this composition dials the host at, in-cluster and reachable +// by no client. It is the one value a composition has to declare for MCP, and +// it stops being one when module-saas-starter#1003 settles what `iss` is. +// +// MCPPublicURLKey overrides the public MCP URL, which the runtime otherwise +// derives by construction (see resolveMCPPublicURL). It exists for a host whose +// gateway fronts solutions under some other route; it must end in MCPPath, +// because the metadata document's own URL is derived from it by swapping that +// suffix — the same pairing the registration token URLs use, and refused at +// boot for the same reason when it cannot be made. const ( - // hostIssuerURLEnvironmentVariable overrides the host's OAuth issuer, which - // is otherwise the resolved host frontend origin. A deployment sets it - // because the resolved address is the one this composition dials the host - // at — in-cluster, reachable by no MCP client. - hostIssuerURLEnvironmentVariable = "HOST_ISSUER_URL" - - // mcpPublicURLEnvironmentVariable sets the canonical public MCP URL: the - // URL clients dial, which is the resource identifier their tokens are bound - // to. It must end in MCPPath, because the metadata document's own URL is - // derived from it by swapping that suffix — the same pairing the - // registration token URLs use, and refused at boot for the same reason when - // it cannot be made. - mcpPublicURLEnvironmentVariable = "MCP_PUBLIC_URL" + MCPConfigurationGroup = "mcp" + MCPIssuerURLKey = "issuer-url" + MCPPublicURLKey = "public-url" ) +// gatewaySolutionsRoute is the route prefix the host gateway fronts a solution +// under, "/solutions/" — the one piece of the host's route layout this +// runtime does encode, and only for the resource identifier. +// +// Everywhere else it deliberately does not: the manifest URL is registered +// root-relative and the host resolves it against the route it reached the +// solution by (see frontendManifestURL). An MCP client cannot do that. The +// resource identifier is what its token is audience-bound to (RFC 8707), so it +// has to be byte-exact with the URL the client dialled, and a client that is +// handed any other identifier rejects it. Deriving it is what makes a +// composition supply nothing; MCPPublicURLKey is the way out for a host that +// routes differently. +const gatewaySolutionsRoute = "/solutions/" + +// mcpConfigurationValue names one value in the group above, as a refusal and +// the README both name it: group/key, never an environment variable. +func mcpConfigurationValue(key string) string { + return MCPConfigurationGroup + "/" + key +} + +// resolveMCPPublicURL is the canonical public MCP URL: the URL an MCP client +// dials, which is the resource identifier its token is bound to. +// +// It is derived by construction from the origin this product is reachable at +// and this solution's id — PUBLIC_URL + gatewaySolutionsRoute + id + MCPPath — +// so a composition that renders a solution serving MCP declares nothing for +// it. Empty when PUBLIC_URL resolved nothing and no override was declared, +// which validate() refuses in a deployed runtime context: the identifier would +// then be reconstructed per request from the forwarded headers. +// +// The declared override wins, for a host whose gateway routes solutions +// elsewhere. It is read through the SDK rather than from the environment for +// the reason the group's doc comment gives. +func resolveMCPPublicURL(ctx context.Context, publicURL, id string) (string, bool) { + if declared, err := codefly.For(ctx).WorkspaceConfiguration(MCPConfigurationGroup, MCPPublicURLKey); err == nil { + if declared = strings.TrimRight(strings.TrimSpace(declared), "/"); declared != "" { + return declared, true + } + } + if publicURL == "" || id == "" { + return "", false + } + return strings.TrimRight(publicURL, "/") + gatewaySolutionsRoute + id + MCPPath, false +} + +// resolveMCPIssuer is the host's OAuth issuer, published as this resource's +// authorization server, and whether the composition declared it. The declared +// value wins over the resolved host frontend origin; the boolean is what tells +// a deployment that was told the origin clients authenticate against from one +// that resolved the host's in-cluster address, which validate() refuses. +// +// The SDK's error is dropped for the reason solutionRegistrationSecret drops +// it: it is the same "no workspace configuration value" whether the group was +// never declared or its carriers never loaded, and the one signal that +// distinguishes them is config.environmentLoadErr. +func resolveMCPIssuer(ctx context.Context, frontendURL string) (string, bool) { + declared, err := codefly.For(ctx).WorkspaceConfiguration(MCPConfigurationGroup, MCPIssuerURLKey) + if err == nil { + if declared = strings.TrimRight(strings.TrimSpace(declared), "/"); declared != "" { + return declared, true + } + } + return strings.TrimRight(frontendURL, "/"), false +} + // validateMCP refuses a configuration whose MCP surface would serve a metadata // document no client can act on. It runs only when the solution declared one: // a solution that serves no MCP has never needed the host's issuer, and must @@ -544,35 +627,49 @@ func (c config) validateMCP() error { return fmt.Errorf("unresolved host issuer %q, and loading Codefly's injected environment failed first: %w — nothing the SDK resolves can be trusted to be absent until that is fixed", c.mcpIssuerURL, c.environmentLoadErr) } - return fmt.Errorf("unresolved host issuer %q: ServeMCP publishes it as the authorization server of this MCP resource, so a client has nowhere to authenticate without it. Set %s to the origin MCP clients authenticate against, or ensure the SDK resolves the host frontend's http endpoint", - c.mcpIssuerURL, hostIssuerURLEnvironmentVariable) - } - // In a deployment both values below are the operator's to set, for the - // reason the loopback refusals above exist: the runtime cannot tell that - // what it resolved is unreachable, and serves a well-formed document naming - // it with a 200, from a solution that registered and looks healthy. A boot - // log line was the only signal, and a log line nobody reads is how a - // solution is absent while every gate is green. + return fmt.Errorf("unresolved host issuer %q: ServeMCP publishes it as the authorization server of this MCP resource, so a client has nowhere to authenticate without it. Provision the workspace configuration %s with the origin MCP clients authenticate against and declare that group as a workspace-configuration dependency of this backend, or ensure the SDK resolves the host frontend's http endpoint", + c.mcpIssuerURL, mcpConfigurationValue(MCPIssuerURLKey)) + } + // In a deployment neither value below can be resolved from inside the + // cluster, for the reason the loopback refusals above exist: the runtime + // cannot tell that what it resolved is unreachable, and serves a + // well-formed document naming it with a 200, from a solution that + // registered and looks healthy. A boot log line was the only signal, and a + // log line nobody reads is how a solution is absent while every gate is + // green. if deployedRuntimeContext(c.runtimeContext) { if c.mcpPublicURL == "" { - return fmt.Errorf("no %s set in the deployed runtime context %q: the gateway strips the route it proxies this solution under, so the identifier reconstructed from the forwarded headers is missing that prefix (and names the scheme of the hop, not the client's) — an MCP client binds its token to the identifier it dialled and rejects any other. Set it to the URL clients dial, ending in %s", - mcpPublicURLEnvironmentVariable, c.runtimeContext, MCPPath) + // Only reachable with PUBLIC_URL unresolved and no override + // declared: the identifier is otherwise derived by construction. + return fmt.Errorf("no public MCP URL in the deployed runtime context %q: it is derived as %s%s, and PUBLIC_URL resolved nothing — so the identifier would be reconstructed per request from the forwarded headers, missing the route the gateway strips (and naming the scheme of the hop, not the client's), and an MCP client binds its token to the identifier it dialled and rejects any other. Set PUBLIC_URL to the origin clients reach this product at, or provision the workspace configuration %s with the URL they dial, ending in %s", + c.runtimeContext, gatewaySolutionsRoute, MCPPath, mcpConfigurationValue(MCPPublicURLKey), MCPPath) } if !c.mcpIssuerExplicit { - return fmt.Errorf("no %s set in the deployed runtime context %q: the issuer resolved from the SDK (%q) is the address this composition dials the host at, which no MCP client can reach, and it would be published as this resource's authorization server. Set it to the origin clients authenticate against", - hostIssuerURLEnvironmentVariable, c.runtimeContext, c.mcpIssuerURL) + return fmt.Errorf("no %s provisioned in the deployed runtime context %q: the issuer resolved from the SDK (%q) is the address this composition dials the host at, which no MCP client can reach, and it would be published as this resource's authorization server. Provision that workspace configuration with the origin clients authenticate against and declare the %s group as a workspace-configuration dependency of this backend", + mcpConfigurationValue(MCPIssuerURLKey), c.runtimeContext, c.mcpIssuerURL, MCPConfigurationGroup) } } if c.mcpPublicURL == "" { return nil } if u, err := url.Parse(c.mcpPublicURL); err != nil || !u.IsAbs() || u.Host == "" { - return fmt.Errorf("unusable %s %q: it is the resource identifier an MCP client binds its token to, so it must be the absolute URL clients dial", - mcpPublicURLEnvironmentVariable, c.mcpPublicURL) + return fmt.Errorf("unusable %s (%q): it is the resource identifier an MCP client binds its token to, so it must be the absolute URL clients dial", + c.mcpPublicURLSource(), c.mcpPublicURL) } if siblingURL(c.mcpPublicURL, MCPPath, ProtectedResourceMetadataPath) == "" { - return fmt.Errorf("unpairable %s %q: it must end in %s, because the metadata document a 401 points a client to is derived from it by swapping that suffix", - mcpPublicURLEnvironmentVariable, c.mcpPublicURL, MCPPath) + return fmt.Errorf("unpairable %s (%q): it must end in %s, because the metadata document a 401 points a client to is derived from it by swapping that suffix", + c.mcpPublicURLSource(), c.mcpPublicURL, MCPPath) } return nil } + +// mcpPublicURLSource names where the public MCP URL came from, so a refusal +// sends an operator to the value they can change. A derived identifier is +// unusable because PUBLIC_URL is, and naming the override instead would send +// them to provision a value whose absence is not the problem. +func (c config) mcpPublicURLSource() string { + if c.mcpPublicExplicit { + return "workspace configuration " + mcpConfigurationValue(MCPPublicURLKey) + } + return "public MCP URL derived from PUBLIC_URL" +} diff --git a/mcp_test.go b/mcp_test.go index 96875f1..49452ea 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -13,6 +13,8 @@ import ( "sync" "testing" + "github.com/codefly-dev/core/resources" + codefly "github.com/codefly-dev/sdk-go" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -609,8 +611,8 @@ func TestValidateRefusesAnUnresolvedIssuerOnlyWhenMCPIsServed(t *testing.T) { if err == nil { t.Fatal("an unresolved issuer was accepted while serving MCP") } - if !strings.Contains(err.Error(), hostIssuerURLEnvironmentVariable) { - t.Errorf("refusal %q does not name %s, the variable that fixes it", err, hostIssuerURLEnvironmentVariable) + if !strings.Contains(err.Error(), mcpConfigurationValue(MCPIssuerURLKey)) { + t.Errorf("refusal %q does not name %s, the configuration that fixes it", err, mcpConfigurationValue(MCPIssuerURLKey)) } if err := (config{}).validateMCP(); err != nil { t.Errorf("a solution serving no MCP was refused: %v", err) @@ -622,11 +624,30 @@ func TestValidateRefusesAnUnresolvedIssuerOnlyWhenMCPIsServed(t *testing.T) { // registration token URLs are derived from their register URLs. An override that // drops the suffix cannot be paired, so it is refused at boot naming the suffix, // rather than leaving every 401 challenge without the one URL a client needs. +// +// The refusal names where the value came from, because the two sources are +// fixed in different places: a declared override is the composition's, and a +// derived identifier is unusable only because PUBLIC_URL is. func TestValidateRefusesAnUnpairablePublicURL(t *testing.T) { for _, public := range []string{"https://host.test/solutions/wiki", "https://host.test/mcp/", "/solutions/wiki/mcp"} { - cfg := config{mcp: true, mcpIssuerURL: testIssuer, mcpPublicURL: public} - if err := cfg.validateMCP(); err == nil { - t.Errorf("%s %q was accepted, want a refusal", mcpPublicURLEnvironmentVariable, public) + cfg := config{mcp: true, mcpIssuerURL: testIssuer, mcpPublicURL: public, mcpPublicExplicit: true} + err := cfg.validateMCP() + if err == nil { + t.Errorf("%s %q was accepted, want a refusal", mcpConfigurationValue(MCPPublicURLKey), public) + continue + } + if !strings.Contains(err.Error(), mcpConfigurationValue(MCPPublicURLKey)) { + t.Errorf("refusal %q for a declared override does not name %s", err, mcpConfigurationValue(MCPPublicURLKey)) + } + derived := cfg + derived.mcpPublicExplicit = false + err = derived.validateMCP() + if err == nil { + t.Errorf("derived public URL %q was accepted, want a refusal", public) + continue + } + if !strings.Contains(err.Error(), "PUBLIC_URL") || strings.Contains(err.Error(), mcpConfigurationValue(MCPPublicURLKey)) { + t.Errorf("refusal %q for a derived identifier does not send the operator to PUBLIC_URL", err) } } cfg := config{mcp: true, mcpIssuerURL: testIssuer, mcpPublicURL: "https://host.test/solutions/wiki" + MCPPath} @@ -697,8 +718,8 @@ func TestValidateRefusesADeployedMCPThatNobodyAddressed(t *testing.T) { if err == nil { t.Fatal("a deployed MCP surface with no public URL was accepted") } - if !strings.Contains(err.Error(), mcpPublicURLEnvironmentVariable) { - t.Errorf("refusal %q does not name %s", err, mcpPublicURLEnvironmentVariable) + if !strings.Contains(err.Error(), "PUBLIC_URL") || !strings.Contains(err.Error(), mcpConfigurationValue(MCPPublicURLKey)) { + t.Errorf("refusal %q names neither the origin it derives from nor %s", err, mcpConfigurationValue(MCPPublicURLKey)) } addressed := deployed @@ -707,8 +728,8 @@ func TestValidateRefusesADeployedMCPThatNobodyAddressed(t *testing.T) { if err == nil { t.Fatal("a deployed MCP surface with an issuer nobody chose was accepted") } - if !strings.Contains(err.Error(), hostIssuerURLEnvironmentVariable) { - t.Errorf("refusal %q does not name %s", err, hostIssuerURLEnvironmentVariable) + if !strings.Contains(err.Error(), mcpConfigurationValue(MCPIssuerURLKey)) { + t.Errorf("refusal %q does not name %s", err, mcpConfigurationValue(MCPIssuerURLKey)) } told := addressed @@ -740,3 +761,199 @@ func TestViewerFromContextOutsideAServedRequest(t *testing.T) { t.Errorf("refusal %q does not name ServeMCP", err) } } + +// --- the composition's half: a derived identifier and one declared value --- + +// mcpConfigurationCarrier is the environment variable Codefly's render projects +// one value of the `mcp` workspace-configuration group into. The runtime never +// reads it: it resolves the value through the SDK, exactly as it resolves the +// registration secret. The tests below write the carrier so the SDK's real +// accessor is what answers — a fake in its place would prove the runtime agrees +// with the fake about an encoding that is the SDK's to own. +func mcpConfigurationCarrier(key string) string { + return resources.WorkspaceConfigurationPrefix + "__" + + strings.ToUpper(MCPConfigurationGroup) + "__" + + strings.ToUpper(strings.ReplaceAll(key, "-", "_")) +} + +// declareMCPConfiguration projects one value of the group the way a composition +// does, and reloads the SDK's process-wide snapshot of the injected carriers. +func declareMCPConfiguration(t *testing.T, key, value string) { + t.Helper() + // Registered before Setenv so it runs after the environment is restored, + // leaving no declared value behind for the boot tests that follow. + t.Cleanup(func() { _ = codefly.LoadEnvironmentVariables() }) + t.Setenv(mcpConfigurationCarrier(key), value) + if err := codefly.LoadEnvironmentVariables(); err != nil { + t.Fatal(err) + } +} + +// TestMCPPublicURLIsDerivedFromTheProductOrigin is the whole point of issue #48: +// a composition has no way to set a bare environment variable on a service, so +// the URL an MCP client dials cannot be one an operator exports. It is built +// from the origin this product is reachable at and this solution's id — the two +// things the runtime already has — and a composition declares nothing for it. +func TestMCPPublicURLIsDerivedFromTheProductOrigin(t *testing.T) { + clearSelfEnvironment(t) + t.Setenv("PORT", "8080") + t.Setenv("PUBLIC_URL", "https://app.example.com/") + + cfg := loadConfig(context.Background(), "wiki") + if want := "https://app.example.com/solutions/wiki" + MCPPath; cfg.mcpPublicURL != want { + t.Fatalf("mcpPublicURL = %q, want the derived %q", cfg.mcpPublicURL, want) + } + if cfg.mcpPublicExplicit { + t.Error("a derived public URL reads as explicitly declared, so a refusal would name an override nobody set") + } + // Derived, it is pairable by construction: the metadata document a 401 + // points a client at is this URL with MCPPath swapped for the well-known + // path, so the derivation can never produce the shape validate() refuses. + paired := siblingURL(cfg.mcpPublicURL, MCPPath, ProtectedResourceMetadataPath) + if want := "https://app.example.com/solutions/wiki" + ProtectedResourceMetadataPath; paired != want { + t.Errorf("metadata URL = %q, want %q", paired, want) + } + // It is also what the served document names, with no request-derived part: + // mcpResource returns the configured identifier before it looks at any + // forwarded header. + s := New(Manifest{ID: "wiki"}) + s.cfg = cfg + request := httptest.NewRequest(http.MethodPost, MCPPath, nil) + request.Header.Set(forwardedHostHeader, "attacker.test") + if got := s.mcpResource(request); got != cfg.mcpPublicURL { + t.Errorf("mcpResource = %q, want the resolved %q", got, cfg.mcpPublicURL) + } + + // No origin to derive from: the identifier falls back to the forwarded + // headers, which validate() refuses in a deployment. + t.Setenv("PUBLIC_URL", "") + if got := loadConfig(context.Background(), "wiki").mcpPublicURL; got != "" { + t.Errorf("with no PUBLIC_URL, mcpPublicURL = %q, want empty", got) + } +} + +// TestMCPPublicURLOverrideIsADeclaredConfiguration: the override survives, for +// a host whose gateway fronts solutions under some other route — but only as a +// value the composition declares and the SDK resolves, never as a bare +// environment variable, which is what a render cannot project. +func TestMCPPublicURLOverrideIsADeclaredConfiguration(t *testing.T) { + clearSelfEnvironment(t) + t.Setenv("PORT", "8080") + t.Setenv("PUBLIC_URL", "https://app.example.com") + const declared = "https://mcp.example.com/wiki" + MCPPath + declareMCPConfiguration(t, MCPPublicURLKey, declared+"/") + + cfg := loadConfig(context.Background(), "wiki") + if cfg.mcpPublicURL != declared { + t.Fatalf("mcpPublicURL = %q, want the declared %q", cfg.mcpPublicURL, declared) + } + if !cfg.mcpPublicExplicit { + t.Error("a declared public URL does not read as explicit, so a refusal would send the operator to PUBLIC_URL instead of the value they set") + } + // The bare environment variable is gone: an operator who exports the name + // the runtime used to read gets the derived URL, not theirs. + t.Setenv("MCP_PUBLIC_URL", "https://exported.example.com/mcp") + if got := loadConfig(context.Background(), "wiki").mcpPublicURL; got != declared { + t.Errorf("mcpPublicURL = %q, want the declared %q — a bare MCP_PUBLIC_URL must have no effect", got, declared) + } +} + +// TestMCPIssuerIsADeclaredConfiguration: the issuer is the one value a +// composition must supply for MCP, and it supplies it as a declared workspace +// configuration. Resolved from the host frontend's endpoint otherwise, which is +// right for a local run — and refused in a deployment, where it is the +// in-cluster address this composition dials the host at. +func TestMCPIssuerIsADeclaredConfiguration(t *testing.T) { + clearSelfEnvironment(t) + t.Setenv("PORT", "8080") + t.Setenv("CODEFLY_HOST_FRONTEND", "frontend") + const declared = "https://login.example.com" + declareMCPConfiguration(t, MCPIssuerURLKey, declared+"/") + + cfg := loadConfig(context.Background(), "wiki") + if cfg.mcpIssuerURL != declared { + t.Fatalf("mcpIssuerURL = %q, want the declared %q", cfg.mcpIssuerURL, declared) + } + if !cfg.mcpIssuerExplicit { + t.Fatal("a declared issuer does not read as explicit, so a deployment that supplied one would still be refused") + } + // The bare environment variable is gone here too. + t.Setenv("HOST_ISSUER_URL", "https://exported.example.com") + if got := loadConfig(context.Background(), "wiki").mcpIssuerURL; got != declared { + t.Errorf("mcpIssuerURL = %q, want the declared %q — a bare HOST_ISSUER_URL must have no effect", got, declared) + } +} + +// TestDeployedMCPRefusalsNameTheConfigurationThroughTheSDK drives both F2 +// refusals the way a deployed cell reaches them: a rendered environment, the +// SDK's own accessors, and nothing else. It is the test that would have caught +// what #48 reports — the refusals naming variables a composition cannot set. +func TestDeployedMCPRefusalsNameTheConfigurationThroughTheSDK(t *testing.T) { + deployed := func(t *testing.T) config { + t.Helper() + clearSelfEnvironment(t) + t.Setenv("PORT", "8080") + t.Setenv("CODEFLY__RUNTIME_CONTEXT", "kubernetes") + t.Setenv("GATEWAY_URL", "http://auth-gateway.saas.svc.cluster.local:8080") + cfg := loadConfig(context.Background(), "wiki") + cfg.mcp = true + return cfg + } + + t.Run("no origin to derive the identifier from", func(t *testing.T) { + declareMCPConfiguration(t, MCPIssuerURLKey, "https://login.example.com") + cfg := deployed(t) + if cfg.mcpPublicURL != "" { + t.Fatalf("mcpPublicURL = %q, want empty with no PUBLIC_URL", cfg.mcpPublicURL) + } + err := cfg.validateMCP() + if err == nil { + t.Fatal("a deployed MCP surface with no identifier to publish was accepted") + } + if !strings.Contains(err.Error(), "PUBLIC_URL") { + t.Errorf("refusal %q does not name PUBLIC_URL, which the identifier is derived from", err) + } + if !strings.Contains(err.Error(), mcpConfigurationValue(MCPPublicURLKey)) { + t.Errorf("refusal %q does not name %s, the declared override", err, mcpConfigurationValue(MCPPublicURLKey)) + } + for _, gone := range []string{"MCP_PUBLIC_URL", "HOST_ISSUER_URL"} { + if strings.Contains(err.Error(), gone) { + t.Errorf("refusal %q names %s, an environment variable a composition cannot set", err, gone) + } + } + }) + + t.Run("no issuer the composition declared", func(t *testing.T) { + cfg := deployed(t) + t.Setenv("PUBLIC_URL", "https://app.example.com") + cfg = loadConfig(context.Background(), "wiki") + cfg.mcp = true + if cfg.mcpIssuerExplicit { + t.Fatal("an issuer nobody declared reads as explicit") + } + err := cfg.validateMCP() + if err == nil { + t.Fatal("a deployed MCP surface with an issuer nobody declared was accepted") + } + if !strings.Contains(err.Error(), mcpConfigurationValue(MCPIssuerURLKey)) { + t.Errorf("refusal %q does not name %s, the configuration that fixes it", err, mcpConfigurationValue(MCPIssuerURLKey)) + } + if strings.Contains(err.Error(), "HOST_ISSUER_URL") { + t.Errorf("refusal %q names HOST_ISSUER_URL, an environment variable a composition cannot set", err) + } + }) + + t.Run("the one declared value is enough", func(t *testing.T) { + declareMCPConfiguration(t, MCPIssuerURLKey, "https://login.example.com") + cfg := deployed(t) + t.Setenv("PUBLIC_URL", "https://app.example.com") + cfg = loadConfig(context.Background(), "wiki") + cfg.mcp = true + if err := cfg.validateMCP(); err != nil { + t.Fatalf("a deployed MCP surface with the issuer declared and PUBLIC_URL resolved was refused: %v", err) + } + if want := "https://app.example.com/solutions/wiki" + MCPPath; cfg.mcpPublicURL != want { + t.Errorf("mcpPublicURL = %q, want the derived %q", cfg.mcpPublicURL, want) + } + }) +} diff --git a/solution.go b/solution.go index 4c3e243..7d8c3d6 100644 --- a/solution.go +++ b/solution.go @@ -282,14 +282,18 @@ type config struct { // mcpIssuerURL is the host's OAuth issuer, published as the authorization // server of the MCP resource (RFC 9728). mcpIssuerURL string - // mcpPublicURL is the canonical public MCP URL an operator set, empty when - // none was (see mcpResource). + // mcpPublicURL is the canonical public MCP URL: derived from publicURL and + // this solution's id, or the override the composition declared. Empty when + // neither resolved (see mcpResource). mcpPublicURL string - // mcpIssuerExplicit says the issuer came from the operator rather than from - // the SDK. It is what tells a deployment that resolved the host's - // in-cluster address — which validate() refuses — from one that was told - // the origin clients authenticate against. + // mcpIssuerExplicit says the issuer came from the configuration the + // composition declared rather than from the SDK's endpoint resolution. It + // is what tells a deployment that resolved the host's in-cluster address — + // which validate() refuses — from one that was told the origin clients + // authenticate against. mcpPublicExplicit says the same of the public URL, + // which a refusal needs to name the value that fixes it. mcpIssuerExplicit bool + mcpPublicExplicit bool } func env(key, fallback string) string { @@ -497,16 +501,16 @@ func loadConfig(ctx context.Context, id string) config { moduleSecrets: parseModuleRegistrationSecrets(env(ModuleRegistrationSecretsEnvironmentVariable, "")), runtimeContext: strings.TrimSpace(env(resources.RuntimeContextPrefix, "")), apiConsumes: env(manifest.APIConsumesEnvironmentVariable, ""), - // The host is the authorization server an MCP client authenticates - // against, so its issuer is the host's own origin — resolved by role - // like every other host endpoint, never typed. The resolved address is - // the one this composition reaches the host at, which is right for a - // local run and is an in-cluster address in a deployment, where no - // public client could reach it: that is what the override is for. - mcpIssuerURL: strings.TrimRight(env(hostIssuerURLEnvironmentVariable, frontendURL), "/"), - mcpIssuerExplicit: env(hostIssuerURLEnvironmentVariable, "") != "", - mcpPublicURL: strings.TrimRight(env(mcpPublicURLEnvironmentVariable, ""), "/"), } + // The host is the authorization server an MCP client authenticates against, + // so its issuer is the host's own origin — resolved by role like every + // other host endpoint, never typed. The resolved address is the one this + // composition reaches the host at, which is right for a local run and is an + // in-cluster address in a deployment, where no public client could reach + // it: that is what the declared configuration is for. The public MCP URL + // needs no declaration at all, being derived from the two values above it. + cfg.mcpIssuerURL, cfg.mcpIssuerExplicit = resolveMCPIssuer(ctx, frontendURL) + cfg.mcpPublicURL, cfg.mcpPublicExplicit = resolveMCPPublicURL(ctx, public, id) cfg.registrationInterval, cfg.registrationIntervalErr = registrationIntervalFromEnv() return cfg } From a09143495f8b524428285f45cc13b9c525f9ff51 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sat, 3 Oct 2026 21:19:03 -0400 Subject: [PATCH 2/2] style: one shape for both MCP configuration resolvers Co-Authored-By: Claude Opus 5 (1M context) --- mcp.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mcp.go b/mcp.go index ab3bba9..e2a1501 100644 --- a/mcp.go +++ b/mcp.go @@ -598,8 +598,7 @@ func resolveMCPPublicURL(ctx context.Context, publicURL, id string) (string, boo // never declared or its carriers never loaded, and the one signal that // distinguishes them is config.environmentLoadErr. func resolveMCPIssuer(ctx context.Context, frontendURL string) (string, bool) { - declared, err := codefly.For(ctx).WorkspaceConfiguration(MCPConfigurationGroup, MCPIssuerURLKey) - if err == nil { + if declared, err := codefly.For(ctx).WorkspaceConfiguration(MCPConfigurationGroup, MCPIssuerURLKey); err == nil { if declared = strings.TrimRight(strings.TrimSpace(declared), "/"); declared != "" { return declared, true }