diff --git a/CHANGELOG.md b/CHANGELOG.md index 8815997..524b4b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ Install via the ### Added +- Hosted formae support. A profile whose `cli.connection` is a `Hosted` connection now routes to its installation behind the shared endpoint and carries a credential, so every tool works against a hosted installation the way it does against a self-hosted agent. Requires formae 0.89.0 or newer. +- Hosted results say which installation answered, in a separate block alongside the payload. When a change fails after it was already sent, the result says so rather than implying nothing happened, so you know whether to go and check. - The MCP now warns when the connected formae agent is newer than your local `formae`, so you can tell when authoring may not reflect the agent's latest capabilities. The notice points at `/formae:upgrade`, which fetches the newer `formae` after you confirm (never silently in classic mode). ### Changed @@ -21,7 +23,10 @@ Install via the - Plugin renamed from `formae-mcp` to `formae`; added `/formae:setup` and `/formae:upgrade`. - The plugin now downloads its prebuilt `formae-mcp` and a matched `formae` into `~/.formae-ai/opt` on first run (no build-from-source; set `FORMAE_MCP_DEV=1` for local dev builds). - Commands issued through the MCP (apply, destroy, cancel, status, list) now identify with your CLI's client ID (`~/.pel/formae/cli_client_id`) instead of a fixed `formae-mcp` identity, so the agent attributes them to the same client as your own `formae` runs. When the ID file does not exist yet, the MCP runs `formae --version` once so formae creates it, and falls back to the old `formae-mcp` identity if it still cannot be read. -- Configuration now comes from the formae CLI (`formae profile show`) instead of a text scan of the profile file, so the MCP and your own `formae` runs always agree on where a profile points. Requires formae 0.89.0 or newer. +- Configuration and credentials now come from a single `formae connection resolve` per tool call, replacing `formae profile show`, so the MCP and your own `formae` runs always agree on where a profile points and a request can never combine one profile revision's endpoint with another's credential. Requires formae 0.89.0 or newer. +- An expired hosted credential is refreshed and the call retried once, but only for reads. A change that fails with an expired credential refreshes it for next time and reports the failure rather than being sent twice. +- On a hosted profile, "not found" from the shared endpoint is now explained rather than passed on. It answers that way for an installation it can no longer route to — one that was suspended or destroyed, or whose subscription lapsed — which can happen part-way through a long session. Every tool now says so, instead of reporting an empty result, an unhealthy agent, or a command that was never missing. +- When several profiles exist and none is named, a hosted call now lists the candidates and asks for the `profile` argument instead of guessing. - Every agent request is built by one internal executor, so cancellation and timeouts apply uniformly across every tool. - The plugin no longer installs a second `formae` alongside one you already have. On launch it looks for yours (`PATH`, then `/opt/pel/bin`, `/usr/local/bin`, `~/.local/bin`, `~/bin`) and uses it; it downloads one into `~/.formae-ai/opt` only when the machine has none. Previously it downloaded a copy on every launch and then ran whichever `formae` came first on `PATH`, so the downloaded one was usually dead weight — and `/formae:upgrade` could upgrade a copy the plugin was not running. Installs are compared by their resolved location, so a symlink pointing into the managed tree, or a home directory that is itself a symlink, is not mistaken for a second install that the plugin then declines to upgrade. - The version-skew notice now says which upgrade applies: `/formae:upgrade` for the copy the plugin installed, or the path of your own install, which the plugin will not change. diff --git a/internal/config/connection.go b/internal/config/connection.go index 866444b..08e409f 100644 --- a/internal/config/connection.go +++ b/internal/config/connection.go @@ -12,10 +12,18 @@ import ( // credentials may be sent is an escape hatch that outlives its reason. const HostedOrigin = "https://cloud.formae.ai" -// installationRE matches the canonical lowercase UUID text form. The constraint -// is syntactic and mirrors what the edge accepts as a routing key; it says -// nothing about UUID version or variant bits. -var installationRE = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) +// installationRE is the routing-key grammar: 27 base62 characters, case +// sensitive, which is the text form of a KSUID. It mirrors byte for byte what +// the edge accepts as a routing key. +// +// It is a shape check and deliberately not a decode. 27 base62 digits span a +// wider range than the 160 bits a KSUID encodes, so a few strings this accepts +// would fail a KSUID parser. Refusing them would make this client stricter than +// the edge that does the routing, so we would refuse an identifier the router +// would have accepted, and gain nothing: nothing mints one that cannot be +// decoded, and a well-formed identifier that is not routable comes back from +// the edge as a 404 that says so. +var installationRE = regexp.MustCompile(`^[0-9A-Za-z]{27}$`) // Connection is where the MCP sends agent requests. It has exactly two arms, so // a resolved configuration cannot be both classic and hosted, and a hosted one @@ -49,7 +57,7 @@ func ValidateHosted(h Hosted) error { return err } if !installationRE.MatchString(h.Installation) { - return fmt.Errorf("installation %q is not a canonical lowercase UUID", h.Installation) + return fmt.Errorf("installation %q is not a well-formed installation id", h.Installation) } return nil } diff --git a/internal/config/connection_test.go b/internal/config/connection_test.go index 2d523dd..90ef209 100644 --- a/internal/config/connection_test.go +++ b/internal/config/connection_test.go @@ -5,7 +5,7 @@ import "testing" func TestValidateHosted_AcceptsCanonicalEndpointAndInstallation(t *testing.T) { h := Hosted{ Endpoint: "https://cloud.formae.ai", - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } if err := ValidateHosted(h); err != nil { t.Fatalf("expected valid hosted connection, got error: %v", err) @@ -29,7 +29,7 @@ func TestValidateHosted_RejectsBadEndpoints(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateHosted(Hosted{ Endpoint: endpoint, - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", }) if err == nil { t.Fatalf("expected %s endpoint %q to be rejected", name, endpoint) @@ -40,12 +40,18 @@ func TestValidateHosted_RejectsBadEndpoints(t *testing.T) { func TestValidateHosted_RejectsBadInstallations(t *testing.T) { cases := map[string]string{ - "uppercase": "3F2B8C14-0000-4000-8000-000000000000", - "braces": "{3f2b8c14-0000-4000-8000-000000000000}", - "too short": "3f2b8c14-0000-4000-8000-00000000000", - "not a uuid": "default", - "empty": "", - "with spaces": "3f2b8c14-0000-4000-8000-000000000000 ", + // The format installations used to carry. Nothing mints one now, so a + // profile naming one addresses an installation that cannot exist. + "the retired uuid form": "3f2b8c14-0000-4000-8000-000000000000", + "braces": "{3HzFPXfPDGhwLJJVtaHbmFs6vLa}", + "one short": "3HzFPXfPDGhwLJJVtaHbmFs6vL", + "one long": "3HzFPXfPDGhwLJJVtaHbmFs6vLaa", + "a hyphen": "3HzFPXfPDGhwLJJVtaHbmFs6v-a", + "an underscore": "3HzFPXfPDGhwLJJVtaHbmFs6v_a", + "not an installation": "default", + "empty": "", + "trailing space": "3HzFPXfPDGhwLJJVtaHbmFs6vL ", + "a newline": "3HzFPXfPDGhwLJJVtaHbmFs6vLa\n", } for name, id := range cases { t.Run(name, func(t *testing.T) { @@ -56,3 +62,19 @@ func TestValidateHosted_RejectsBadInstallations(t *testing.T) { }) } } + +// The check is the routing key's grammar, not a decode. 27 base62 digits span a +// wider range than the 160 bits a KSUID encodes, so a few well-formed strings +// would fail a KSUID parser. Refusing them here would make this client stricter +// than the edge that does the routing, which validates the same grammar: we +// would refuse an identifier the router accepts and gain nothing, because +// nothing mints one that cannot be decoded. Pinned so the limit is a decision. +func TestValidateHosted_ChecksTheRoutingGrammarNotADecode(t *testing.T) { + err := ValidateHosted(Hosted{ + Endpoint: HostedOrigin, + Installation: "zzzzzzzzzzzzzzzzzzzzzzzzzzz", + }) + if err != nil { + t.Fatalf("a well-formed identifier must be accepted without decoding it: %v", err) + } +} diff --git a/internal/config/failure.go b/internal/config/failure.go new file mode 100644 index 0000000..d52fcaf --- /dev/null +++ b/internal/config/failure.go @@ -0,0 +1,165 @@ +package config + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "strings" + + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +// failureSchemaVersion is the envelope shape this build understands. It is +// checked before any other field, exactly as the success document's is. +const failureSchemaVersion = 1 + +// diagnose is the invitation attached to every declared failure. It is the same +// command, run by hand, and its human output is deliberately not redacted the +// way this consumer is: a person looking at their own profile in their own +// terminal is not the exposure we are closing. +const diagnose = "run `formae connection resolve` to see why" + +// declaredCodes is the closed namespace the producer promises. A code outside +// it is a protocol mismatch, not a message to pass along. +var declaredCodes = map[string]bool{ + "ambiguous_profile": true, + "auth_failed": true, + "untrusted_issuer": true, + "no_connection": true, + "internal": true, +} + +// declaredPluginCodes is what an auth plugin may report. +// +// This is validated rather than trusted, unlike the top-level code, because the +// producer does not close it: the plugin's error code is a bare string alias +// copied through without a membership check, so `details.pluginCode` is an open +// channel wearing a closed channel's name. A plugin that put a token there +// would otherwise route it straight into a tool result. +var declaredPluginCodes = map[string]bool{ + "unsupported": true, + "not_logged_in": true, + "session_expired": true, + "issuer_unreachable": true, +} + +// unrecognisedPluginCode replaces a plugin code we did not declare. It says a +// code was present and withheld, which is all a reader can safely be told. +const unrecognisedPluginCode = "unrecognised" + +// ResolveError is a failure the CLI declared. It carries the code so a caller +// can branch on it, and never the producer's free-text message. +type ResolveError struct { + Code string + // PluginCode is the auth plugin's own code for an auth_failed, validated + // against the declared set. Empty when absent or unrecognised. + PluginCode string +} + +func (e *ResolveError) Error() string { + switch e.Code { + case "auth_failed": + if e.PluginCode != "" { + return fmt.Sprintf("formae could not obtain a credential for this profile (%s); %s", + e.PluginCode, diagnose) + } + return fmt.Sprintf("formae could not obtain a credential for this profile; %s", diagnose) + case "untrusted_issuer": + return fmt.Sprintf( + "this profile's hosted connection names an issuer this build will not authenticate against; %s", + diagnose) + case "no_connection": + return fmt.Sprintf("this profile resolves no connection formae can use; %s", diagnose) + default: + return fmt.Sprintf("formae could not resolve the connection; %s", diagnose) + } +} + +// AmbiguousProfileError is the CLI refusing to guess which installation was +// meant. Its message is the instruction the caller acts on, so a model reading +// it can retry with the argument rather than needing to be told separately. +type AmbiguousProfileError struct { + Candidates []string + Active string +} + +func (e *AmbiguousProfileError) Error() string { + listed := make([]string, 0, len(e.Candidates)) + for _, c := range e.Candidates { + if c == e.Active { + c += " (active)" + } + listed = append(listed, c) + } + return fmt.Sprintf( + "more than one profile exists and none was named, so formae cannot tell which "+ + "installation you meant. Pass the profile argument on this call: %s", + strings.Join(listed, ", ")) +} + +// failureView is the envelope the producer emits on stdout when it fails. +type failureView struct { + SchemaVersion *int `json:"schemaVersion"` + Code string `json:"code"` + Details struct { + Candidates []string `json:"candidates"` + Active string `json:"active"` + PluginCode string `json:"pluginCode"` + } `json:"details"` + // Message is decoded so it is visibly accounted for, and deliberately never + // read: the producer builds it from an auth plugin's error string or an + // arbitrary err.Error(), and a Pkl failure quotes profile source lines, + // which for a classic profile can mean an inline password. + Message string `json:"message"` +} + +// decodeFailure turns a non-zero exit into a typed error. +// +// exitStatus names the failure when the envelope cannot be read at all, which +// is a supported path rather than a defensive one: argv the command cannot +// parse fails before the flags that say how to render a failure exist, so it +// exits non-zero with no envelope. The raw bytes never reach the error. +func decodeFailure(stdout []byte, exitStatus int) error { + unreadable := fmt.Errorf("formae could not resolve the connection (exit %d); %s", + exitStatus, diagnose) + + var v failureView + dec := json.NewDecoder(bytes.NewReader(stdout)) + if err := dec.Decode(&v); err != nil { + return unreadable + } + if _, err := dec.Token(); !errors.Is(err, io.EOF) { + return unreadable + } + if v.SchemaVersion == nil || *v.SchemaVersion != failureSchemaVersion { + return unreadable + } + if !declaredCodes[v.Code] { + return unreadable + } + + if v.Code == "ambiguous_profile" { + return &AmbiguousProfileError{ + Candidates: v.Details.Candidates, + Active: v.Details.Active, + } + } + + pluginCode := v.Details.PluginCode + if pluginCode != "" && !declaredPluginCodes[pluginCode] { + pluginCode = unrecognisedPluginCode + } + return &ResolveError{Code: v.Code, PluginCode: pluginCode} +} + +// Resolved is one profile evaluation: the effective profile name the CLI +// reported, the connection it resolved, and the credential that reaches it. +type Resolved struct { + Profile string + Conn Connection + // Credential is the zero value for classic: the MCP sends a self-hosted + // agent none, and that is a non-goal rather than an omission. + Credential secret.Value +} diff --git a/internal/config/oracle.go b/internal/config/oracle.go index e8af92f..2a4484b 100644 --- a/internal/config/oracle.go +++ b/internal/config/oracle.go @@ -12,6 +12,7 @@ import ( "time" "github.com/platform-engineering-labs/formae-mcp/internal/featuregate" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" ) // schemaVersion is the machine-view major version this build understands. It is @@ -28,32 +29,34 @@ const oracleTimeout = 10 * time.Second var errOutputTooLarge = errors.New("formae produced more output than expected") -// Resolved is one profile evaluation: the effective profile name the CLI -// reported, and the connection it resolved. -type Resolved struct { - Profile string - Conn Connection -} - // resolveVia runs the CLI as the configuration oracle and decodes its machine // view. Stdout and stderr are captured separately and drained concurrently: // combined capture would put subprocess output into errors, and reading two // pipes serially can deadlock when the child fills the one not being read. -func resolveVia(ctx context.Context, bin, profileName string) (Resolved, error) { +// +// One command produces both the connection and the credential. Two +// independently timed reads could not: between them the active pointer can +// move, the profile can be rewritten, or the auth block can change, and the +// request would carry an endpoint from one revision with a credential from +// another — for hosted, one installation's endpoint with another's credential. +func resolveVia(ctx context.Context, bin, profileName string, forceRefresh bool) (Resolved, error) { if _, ok := ctx.Deadline(); !ok { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, oracleTimeout) defer cancel() } - // `profile show []` takes the name positionally. There is no - // --profile flag on it, and none on the root command either, so a flag form - // would not parse. - args := []string{"profile", "show"} + // --profile is a local flag on `resolve`, registered by AddConfigFlags, so + // it follows the subcommand. This is the opposite of `profile show`, where + // the name is positional and no such flag exists. + args := []string{"connection", "resolve"} if profileName != "" { - args = append(args, profileName) + args = append(args, "--profile", profileName) } args = append(args, "--output-consumer", "machine", "--output-schema", "json") + if forceRefresh { + args = append(args, "--force-refresh") + } cmd := exec.CommandContext(ctx, bin, args...) // Own process group, so a deadline kills the CLI and anything it spawned. @@ -106,35 +109,43 @@ func resolveVia(ctx context.Context, bin, profileName string) (Resolved, error) return Resolved{}, fmt.Errorf("reading configuration from %s: %w", bin, out.err) } if waitErr != nil { - // Reports the failure and the exit status, never the bytes. + // A declared failure is an envelope on this same stdout, so a non-zero + // exit is read rather than reported blind. decodeFailure falls back to + // the exit status when there is no envelope to read, and the bytes + // never reach an error either way. var exitErr *exec.ExitError if errors.As(waitErr, &exitErr) { - return Resolved{}, fmt.Errorf("formae could not resolve the configuration (exit %d)", exitErr.ExitCode()) + return Resolved{}, decodeFailure(out.data, exitErr.ExitCode()) } return Resolved{}, fmt.Errorf("formae could not resolve the configuration: %w", waitErr) } - return decodeProfileShow(out.data) + return decodeResolved(out.data) } -// profileShowView is the subset of the machine view this code reads. Unknown -// fields are ignored so the producer can add fields without a break. -type profileShowView struct { +// resolvedView is the subset of the machine view this code reads. Unknown +// fields are ignored so the producer can add fields without a break — the +// hosted arm's `auth` object is one such field, reduced to a type discriminator +// this consumer has no use for. +// +// The connection is at the top level. `profile show` nests it under `cli`; +// `connection resolve` does not, and reading the wrong shape would silently +// yield no connection at all. +type resolvedView struct { SchemaVersion *int `json:"schemaVersion"` Profile string `json:"profile"` - Cli struct { - Connection *struct { - Mode string `json:"mode"` - URL string `json:"url"` - Port int `json:"port"` - Endpoint string `json:"endpoint"` - Installation string `json:"installation"` - } `json:"connection"` - } `json:"cli"` + Connection *struct { + Mode string `json:"mode"` + URL string `json:"url"` + Port int `json:"port"` + Endpoint string `json:"endpoint"` + Installation string `json:"installation"` + } `json:"connection"` + Credential string `json:"credential"` } -func decodeProfileShow(data []byte) (Resolved, error) { - var v profileShowView +func decodeResolved(data []byte) (Resolved, error) { + var v resolvedView dec := json.NewDecoder(bytes.NewReader(data)) if err := dec.Decode(&v); err != nil { return Resolved{}, errors.New("formae returned output this version cannot read") @@ -152,45 +163,66 @@ func decodeProfileShow(data []byte) (Resolved, error) { "formae output uses schema version %d; this build understands %d", *v.SchemaVersion, schemaVersion) } - if v.Cli.Connection == nil { - return Resolved{}, errors.New("formae output carries no cli.connection") + if v.Connection == nil { + return Resolved{}, errors.New("formae output carries no connection") } - switch v.Cli.Connection.Mode { + switch v.Connection.Mode { case "classic": // Classic gets a minimum check too. An empty URL would produce a // valid-looking connection and defer the failure to request // construction, which reports it far from its cause. - if v.Cli.Connection.URL == "" { + if v.Connection.URL == "" { return Resolved{}, errors.New("formae reported a classic connection with no url") } - if v.Cli.Connection.Port <= 0 { - return Resolved{}, fmt.Errorf("formae reported an unusable port %d", v.Cli.Connection.Port) + if v.Connection.Port <= 0 { + return Resolved{}, fmt.Errorf("formae reported an unusable port %d", v.Connection.Port) + } + // The MCP sends a self-hosted agent no credential. Refusing one here + // rather than dropping it keeps that non-goal out of reach of a + // producer bug, instead of leaving a token in a value that might later + // grow a path to a header. + if v.Credential != "" { + return Resolved{}, errors.New("formae reported a credential for a classic connection") } return Resolved{ Profile: v.Profile, - Conn: Classic{URL: v.Cli.Connection.URL, Port: v.Cli.Connection.Port}, + Conn: Classic{URL: v.Connection.URL, Port: v.Connection.Port}, }, nil case "hosted": h := Hosted{ - Endpoint: v.Cli.Connection.Endpoint, - Installation: v.Cli.Connection.Installation, + Endpoint: v.Connection.Endpoint, + Installation: v.Connection.Installation, } if err := ValidateHosted(h); err != nil { return Resolved{}, fmt.Errorf("hosted connection is not usable: %w", err) } - return Resolved{Profile: v.Profile, Conn: h}, nil + // A hosted connection that cannot be authenticated is not a usable + // connection. The producer refuses to emit one; this refuses to invent + // one rather than deferring the failure into a remote 401. + if v.Credential == "" { + return Resolved{}, errors.New("formae reported a hosted connection with no credential") + } + return Resolved{ + Profile: v.Profile, + Conn: h, + Credential: secret.New(v.Credential), + }, nil default: - return Resolved{}, fmt.Errorf("formae reported an unknown connection mode %q", v.Cli.Connection.Mode) + return Resolved{}, fmt.Errorf("formae reported an unknown connection mode %q", v.Connection.Mode) } } -// Resolve reads a profile's resolved configuration from the CLI. An empty -// profileName lets the CLI resolve the active profile and report which one it -// used, so this package never reasons about what "active" meant. -func Resolve(ctx context.Context, bin, profileName string) (Resolved, error) { +// Resolve reads a profile's resolved connection and credential from the CLI. An +// empty profileName lets the CLI resolve the active profile and report which one +// it used, so this package never reasons about what "active" meant. +// +// forceRefresh asks the auth plugin for a fresh credential rather than the +// stored one. It is for the 401 path, which re-resolves and then checks that +// the target did not move. +func Resolve(ctx context.Context, bin, profileName string, forceRefresh bool) (Resolved, error) { if err := featuregate.GuardFeatureContext(ctx, featuregate.FeatureConnectionOracle, bin); err != nil { return Resolved{}, err } - return resolveVia(ctx, bin, profileName) + return resolveVia(ctx, bin, profileName, forceRefresh) } diff --git a/internal/config/oracle_test.go b/internal/config/oracle_test.go index 8191760..9e6ad88 100644 --- a/internal/config/oracle_test.go +++ b/internal/config/oracle_test.go @@ -2,6 +2,7 @@ package config import ( "context" + "encoding/json" "errors" "os" "path/filepath" @@ -10,17 +11,23 @@ import ( "time" ) +// The resolve view is flat: the connection sits at the top level, not under a +// `cli` key the way `profile show` nests it. const classicView = `{"schemaVersion":1,"profile":"dev", - "cli":{"connection":{"mode":"classic","url":"http://localhost","port":49684}}}` + "connection":{"mode":"classic","url":"http://localhost","port":49684}}` const hostedView = `{"schemaVersion":1,"profile":"prod", - "cli":{"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai", - "installation":"3f2b8c14-0000-4000-8000-000000000000"}}}` + "connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai", + "installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa","auth":{"type":"oidc"}}, + "credential":"Bearer live-token"}` -func TestDecodeProfileShow_Classic(t *testing.T) { - got, err := decodeProfileShow([]byte(classicView)) +// token is recognisable, so a leak into an error shows up in a substring check. +const token = "Bearer sup3rs3cr3t" + +func TestDecodeResolved_Classic(t *testing.T) { + got, err := decodeResolved([]byte(classicView)) if err != nil { - t.Fatalf("decodeProfileShow: unexpected error: %v", err) + t.Fatalf("decodeResolved: unexpected error: %v", err) } if got.Profile != "dev" { t.Errorf("profile: want %q, got %q", "dev", got.Profile) @@ -29,59 +36,219 @@ func TestDecodeProfileShow_Classic(t *testing.T) { if got.Conn != Connection(want) { t.Errorf("connection: want %#v, got %#v", want, got.Conn) } + if !got.Credential.IsZero() { + t.Error("a classic connection must carry no credential") + } } -func TestDecodeProfileShow_Hosted(t *testing.T) { - got, err := decodeProfileShow([]byte(hostedView)) +func TestDecodeResolved_Hosted(t *testing.T) { + got, err := decodeResolved([]byte(hostedView)) if err != nil { - t.Fatalf("decodeProfileShow: unexpected error: %v", err) + t.Fatalf("decodeResolved: unexpected error: %v", err) } if got.Profile != "prod" { t.Errorf("profile: want %q, got %q", "prod", got.Profile) } want := Hosted{ Endpoint: "https://cloud.formae.ai", - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } if got.Conn != Connection(want) { t.Errorf("connection: want %#v, got %#v", want, got.Conn) } + if got.Credential.Reveal() != "Bearer live-token" { + t.Errorf("credential: got %q", got.Credential.Reveal()) + } } -// TestDecodeProfileShow_ToleratesUnknownFields pins that the producer can add -// fields without breaking this consumer. -func TestDecodeProfileShow_ToleratesUnknownFields(t *testing.T) { +// The producer can add fields without breaking this consumer. The hosted arm's +// `auth` object is one such field today: it is reduced to a type discriminator +// and this consumer has no use for it. +func TestDecodeResolved_ToleratesUnknownFields(t *testing.T) { view := `{"schemaVersion":1,"profile":"dev","futureField":{"a":1}, - "cli":{"connection":{"mode":"classic","url":"http://localhost","port":49684,"extra":true}}}` - got, err := decodeProfileShow([]byte(view)) + "connection":{"mode":"classic","url":"http://localhost","port":49684,"extra":true}}` + got, err := decodeResolved([]byte(view)) if err != nil { - t.Fatalf("decodeProfileShow: unexpected error: %v", err) + t.Fatalf("decodeResolved: unexpected error: %v", err) } if got.Conn != Connection(Classic{URL: "http://localhost", Port: 49684}) { t.Errorf("connection: got %#v", got.Conn) } } -func TestDecodeProfileShow_Rejects(t *testing.T) { +func TestDecodeResolved_Rejects(t *testing.T) { cases := []struct { name string view string }{ - {"unknown schema version", `{"schemaVersion":2,"cli":{"connection":{"mode":"classic","url":"http://x","port":1}}}`}, - {"missing schema version", `{"cli":{"connection":{"mode":"classic","url":"http://x","port":1}}}`}, - {"unknown mode", `{"schemaVersion":1,"cli":{"connection":{"mode":"orbital"}}}`}, - {"missing connection", `{"schemaVersion":1,"profile":"dev","cli":{}}`}, - {"hosted without installation", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai"}}}`}, - {"hosted with a foreign endpoint", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://evil.example.com","installation":"3f2b8c14-0000-4000-8000-000000000000"}}}`}, + {"unknown schema version", `{"schemaVersion":2,"connection":{"mode":"classic","url":"http://x","port":1}}`}, + {"missing schema version", `{"connection":{"mode":"classic","url":"http://x","port":1}}`}, + {"unknown mode", `{"schemaVersion":1,"connection":{"mode":"orbital"}}`}, + {"missing connection", `{"schemaVersion":1,"profile":"dev"}`}, + {"hosted without installation", `{"schemaVersion":1,"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai"},"credential":"Bearer x"}`}, + {"hosted with a foreign endpoint", `{"schemaVersion":1,"connection":{"mode":"hosted","endpoint":"https://evil.example.com","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"},"credential":"Bearer x"}`}, + // A hosted connection that cannot be authenticated is not a usable + // connection. The producer refuses to emit one; this is the consumer + // refusing to invent one. + {"hosted with no credential", `{"schemaVersion":1,"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"}}`}, + // The MCP sends a self-hosted agent no credential. Accepting one here + // would make that non-goal reachable by a producer bug. + {"classic with a credential", `{"schemaVersion":1,"connection":{"mode":"classic","url":"http://localhost","port":49684},"credential":"Bearer x"}`}, {"not json", `formae: command not found`}, - {"classic with an empty url", `{"schemaVersion":1,"cli":{"connection":{"mode":"classic","port":49684}}}`}, - {"classic with no port", `{"schemaVersion":1,"cli":{"connection":{"mode":"classic","url":"http://localhost"}}}`}, + {"classic with an empty url", `{"schemaVersion":1,"connection":{"mode":"classic","port":49684}}`}, + {"classic with no port", `{"schemaVersion":1,"connection":{"mode":"classic","url":"http://localhost"}}`}, {"trailing document", classicView + "\n" + classicView}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if _, err := decodeProfileShow([]byte(tc.view)); err == nil { - t.Fatalf("decodeProfileShow(%s): expected an error, got nil", tc.name) + if _, err := decodeResolved([]byte(tc.view)); err == nil { + t.Fatalf("decodeResolved(%s): expected an error, got nil", tc.name) + } + }) + } +} + +func envelope(t *testing.T, code, message string, details map[string]any) string { + t.Helper() + e := map[string]any{"schemaVersion": 1, "code": code, "message": message} + if details != nil { + e["details"] = details + } + b, err := json.Marshal(e) + if err != nil { + t.Fatalf("building an envelope: %v", err) + } + return string(b) +} + +func TestDecodeFailure_AmbiguousProfileCarriesTheCandidates(t *testing.T) { + env := envelope(t, "ambiguous_profile", "more than one profile exists", map[string]any{ + "candidates": []string{"prod", "staging"}, + "active": "prod", + }) + + err := decodeFailure([]byte(env), 1) + + var amb *AmbiguousProfileError + if !errors.As(err, &amb) { + t.Fatalf("want an AmbiguousProfileError, got %T: %v", err, err) + } + if strings.Join(amb.Candidates, ",") != "prod,staging" { + t.Errorf("candidates: got %v", amb.Candidates) + } + if amb.Active != "prod" { + t.Errorf("active: got %q", amb.Active) + } + for _, want := range []string{"prod", "staging", "profile"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the message must name %q so the caller can retry: %v", want, err) + } + } +} + +func TestDecodeFailure_DeclaredCodesCarryTheirCode(t *testing.T) { + for _, code := range []string{"auth_failed", "untrusted_issuer", "no_connection", "internal"} { + t.Run(code, func(t *testing.T) { + err := decodeFailure([]byte(envelope(t, code, "some producer prose", nil)), 1) + + var re *ResolveError + if !errors.As(err, &re) { + t.Fatalf("want a ResolveError, got %T: %v", err, err) + } + if re.Code != code { + t.Errorf("code: want %q, got %q", code, re.Code) + } + }) + } +} + +// The producer builds these messages out of an auth plugin's error string or an +// arbitrary err.Error(). A Pkl evaluation failure quotes source lines, and a +// classic profile can hold an inline password, so the free text is closed here. +func TestDecodeFailure_NeverSurfacesTheProducersMessage(t *testing.T) { + env := envelope(t, "internal", "cannot read profile: password = \""+token+"\"", nil) + + err := decodeFailure([]byte(env), 1) + + if strings.Contains(err.Error(), token) { + t.Fatalf("the producer's message reached the consumer: %v", err) + } + if strings.Contains(err.Error(), "password") { + t.Fatalf("the producer's message reached the consumer: %v", err) + } + if !strings.Contains(err.Error(), "formae connection resolve") { + t.Fatalf("the fixed text must point somewhere the reader can look: %v", err) + } +} + +// pkg/auth.ErrorCode is a bare string alias and the producer copies whatever the +// plugin returned into details.pluginCode without checking it, so this is an +// open channel wearing a closed channel's name. +func TestDecodeFailure_ValidatesThePluginCode(t *testing.T) { + t.Run("a declared code is kept", func(t *testing.T) { + env := envelope(t, "auth_failed", "session is gone", map[string]any{"pluginCode": "session_expired"}) + + err := decodeFailure([]byte(env), 1) + + var re *ResolveError + if !errors.As(err, &re) { + t.Fatalf("want a ResolveError, got %T", err) + } + if re.PluginCode != "session_expired" { + t.Errorf("pluginCode: got %q", re.PluginCode) + } + if !strings.Contains(err.Error(), "session_expired") { + t.Errorf("a declared plugin code carries diagnostic value and should be shown: %v", err) + } + }) + + t.Run("anything else is opaque", func(t *testing.T) { + env := envelope(t, "auth_failed", "refused", map[string]any{"pluginCode": token}) + + err := decodeFailure([]byte(env), 1) + + if strings.Contains(err.Error(), token) { + t.Fatalf("an unvalidated plugin code reached the consumer: %v", err) + } + var re *ResolveError + if !errors.As(err, &re) { + t.Fatalf("want a ResolveError, got %T", err) + } + if re.PluginCode == token { + t.Fatalf("an unvalidated plugin code was kept: %q", re.PluginCode) + } + }) +} + +func TestDecodeFailure_Rejects(t *testing.T) { + cases := []struct { + name string + body string + }{ + {"unknown envelope schema version", `{"schemaVersion":2,"code":"internal","message":"x"}`}, + {"missing envelope schema version", `{"code":"internal","message":"x"}`}, + {"an unregistered code", envelope(t, "teapot", "x", nil)}, + // Argv the command cannot parse fails before the output flags are + // established, so it exits non-zero with no envelope at all. The + // producer pins that, so this is a supported path, not a defensive one. + {"no envelope at all", "Error: unknown flag: --nope"}, + {"empty output", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := decodeFailure([]byte(tc.body), 7) + if err == nil { + t.Fatal("expected an error") + } + var amb *AmbiguousProfileError + if errors.As(err, &amb) { + t.Fatal("an unreadable envelope must not decode as ambiguity") + } + if !strings.Contains(err.Error(), "7") { + t.Errorf("an unreadable failure must name the exit status: %v", err) + } + if strings.Contains(err.Error(), "unknown flag") { + t.Errorf("an unreadable failure must not echo the bytes: %v", err) } }) } @@ -102,63 +269,93 @@ func stubFormae(t *testing.T, script string) string { return path } -// TestResolveVia_ReadsStdoutIgnoringStderr pins that warnings on stderr are -// never part of the parsed value. -func TestResolveVia_ReadsStdoutIgnoringStderr(t *testing.T) { - bin := stubFormae(t, "echo 'warning: cli.api is deprecated' >&2\ncat <<'EOF'\n"+classicView+"\nEOF\n") - - got, err := resolveVia(context.Background(), bin, "") - if err != nil { - t.Fatalf("resolveVia: unexpected error: %v", err) - } - if got.Conn != Connection(Classic{URL: "http://localhost", Port: 49684}) { - t.Fatalf("connection: got %#v", got.Conn) - } -} - -// TestResolveVia_PassesExactArgv pins the whole command line, so an -// implementation that invents a flag or drops the machine consumer fails here. -// `profile show` takes the name positionally; there is no --profile flag on it -// and none on the root command either, so a flag form would not even parse. +// TestResolveVia_PassesExactArgv pins the whole command line. --profile is a +// local flag on `resolve`, so it follows the subcommand — the opposite of +// `profile show`, where the name is positional. An implementation that carries +// the old shape over passes against a lenient stub and fails against the real +// binary, which is how slice 1's argv defect got in. func TestResolveVia_PassesExactArgv(t *testing.T) { cases := []struct { - name string - profile string - want string + name string + profile string + forceRefresh bool + want string }{ + { + "active profile", + "", false, + "connection resolve --output-consumer machine --output-schema json", + }, { "named profile", - "prod", - "profile show prod --output-consumer machine --output-schema json", + "prod", false, + "connection resolve --profile prod --output-consumer machine --output-schema json", }, { - "active profile", - "", - "profile show --output-consumer machine --output-schema json", + "named profile with a forced refresh", + "prod", true, + "connection resolve --profile prod --output-consumer machine --output-schema json --force-refresh", + }, + { + "active profile with a forced refresh", + "", true, + "connection resolve --output-consumer machine --output-schema json --force-refresh", }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { bin := stubFormae(t, "if [ \"$*\" != '"+tc.want+"' ]; then echo \"unexpected argv: $*\" >&2; exit 9; fi\ncat <<'EOF'\n"+classicView+"\nEOF\n") - if _, err := resolveVia(context.Background(), bin, tc.profile); err != nil { + if _, err := resolveVia(context.Background(), bin, tc.profile, tc.forceRefresh); err != nil { t.Fatalf("resolveVia: unexpected error: %v", err) } }) } } -// TestResolveVia_NonZeroExitDoesNotLeakOutput pins that a failure reports the -// exit status rather than the subprocess bytes, which may carry a credential. +// TestResolveVia_ReadsStdoutIgnoringStderr pins that warnings on stderr are +// never part of the parsed value. +func TestResolveVia_ReadsStdoutIgnoringStderr(t *testing.T) { + bin := stubFormae(t, "echo 'warning: cli.api is deprecated' >&2\ncat <<'EOF'\n"+classicView+"\nEOF\n") + + got, err := resolveVia(context.Background(), bin, "", false) + if err != nil { + t.Fatalf("resolveVia: unexpected error: %v", err) + } + if got.Conn != Connection(Classic{URL: "http://localhost", Port: 49684}) { + t.Fatalf("connection: got %#v", got.Conn) + } +} + +// A declared failure reaches the caller as its typed error, which means reading +// stdout on a non-zero exit rather than reporting the status and stopping. +func TestResolveVia_DecodesADeclaredFailure(t *testing.T) { + env := envelope(t, "ambiguous_profile", "pick one", map[string]any{ + "candidates": []string{"prod", "staging"}, + "active": "prod", + }) + bin := stubFormae(t, "cat <<'EOF'\n"+env+"\nEOF\nexit 1\n") + + _, err := resolveVia(context.Background(), bin, "", false) + + var amb *AmbiguousProfileError + if !errors.As(err, &amb) { + t.Fatalf("want an AmbiguousProfileError, got %T: %v", err, err) + } +} + +// TestResolveVia_NonZeroExitDoesNotLeakOutput pins that a failure the consumer +// cannot read reports the exit status rather than the subprocess bytes, which +// may carry a credential or a profile's contents. func TestResolveVia_NonZeroExitDoesNotLeakOutput(t *testing.T) { - const secret = "sensitive-subprocess-output" - bin := stubFormae(t, "echo '"+secret+"'\necho '"+secret+"' >&2\nexit 3\n") + const leaked = "sensitive-subprocess-output" + bin := stubFormae(t, "echo '"+leaked+"'\necho '"+leaked+"' >&2\nexit 3\n") - _, err := resolveVia(context.Background(), bin, "") + _, err := resolveVia(context.Background(), bin, "", false) if err == nil { t.Fatal("resolveVia: expected an error for a non-zero exit, got nil") } - if strings.Contains(err.Error(), secret) { + if strings.Contains(err.Error(), leaked) { t.Fatalf("error leaks subprocess output: %v", err) } if !strings.Contains(err.Error(), "3") { @@ -166,6 +363,41 @@ func TestResolveVia_NonZeroExitDoesNotLeakOutput(t *testing.T) { } } +// A credential must not reach an error on any path, nor a rendering of the +// value the resolver returns. +func TestResolveVia_ContainsTheCredential(t *testing.T) { + t.Run("a rendering of the resolved value", func(t *testing.T) { + bin := stubFormae(t, "cat <<'EOF'\n"+hostedView+"\nEOF\n") + + got, err := resolveVia(context.Background(), bin, "", false) + if err != nil { + t.Fatalf("resolveVia: %v", err) + } + out, err := json.Marshal(got) + if err != nil { + t.Fatalf("marshalling Resolved: %v", err) + } + if strings.Contains(string(out), "live-token") { + t.Fatalf("a JSON rendering of Resolved leaked the credential: %s", out) + } + }) + + t.Run("a success document that then fails to validate", func(t *testing.T) { + bad := `{"schemaVersion":1,"profile":"prod","connection":{"mode":"hosted",` + + `"endpoint":"https://evil.example.com","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"},` + + `"credential":"` + token + `"}` + bin := stubFormae(t, "cat <<'EOF'\n"+bad+"\nEOF\n") + + _, err := resolveVia(context.Background(), bin, "", false) + if err == nil { + t.Fatal("expected a foreign endpoint to be rejected") + } + if strings.Contains(err.Error(), token) { + t.Fatalf("the rejection leaked the credential: %v", err) + } + }) +} + func TestResolveVia_HonoursCancellation(t *testing.T) { bin := stubFormae(t, "sleep 30\n") @@ -174,7 +406,7 @@ func TestResolveVia_HonoursCancellation(t *testing.T) { done := make(chan error, 1) go func() { - _, err := resolveVia(ctx, bin, "") + _, err := resolveVia(ctx, bin, "", false) done <- err }() @@ -195,7 +427,7 @@ func TestResolveVia_BoundsOutputSizePromptly(t *testing.T) { done := make(chan error, 1) go func() { - _, err := resolveVia(context.Background(), bin, "") + _, err := resolveVia(context.Background(), bin, "", false) done <- err }() diff --git a/internal/execctx/execctx.go b/internal/execctx/execctx.go index 3b4b3a9..ab32584 100644 --- a/internal/execctx/execctx.go +++ b/internal/execctx/execctx.go @@ -9,6 +9,7 @@ import ( "github.com/platform-engineering-labs/formae-mcp/internal/config" "github.com/platform-engineering-labs/formae-mcp/internal/formaebin" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" ) // Context is the frozen decision set for one MCP call. @@ -18,6 +19,15 @@ type Context struct { ProfileName string // Conn is where this call sends agent requests. Conn config.Connection + // Credential authenticates this call, and is the zero value for classic. + // + // It sits beside the connection rather than inside the hosted arm on + // purpose: configuration gets logged, compared and rendered, and a secret + // inside it would ride along on all three. The combination this leaves + // representable — a classic connection with a credential — is ruled out + // where it would matter: the decoder refuses that shape, and the client + // holds a credential only in the hosted arm of its routing. + Credential secret.Value // FormaeBin is the formae binary this call shells out to. Resolved once; // callers take it from here rather than resolving again. FormaeBin string @@ -26,7 +36,7 @@ type Context struct { // Resolver builds Contexts. The resolve func is injectable so tests do not // shell out to a CLI. type Resolver struct { - resolve func(ctx context.Context, bin, profileName string) (config.Resolved, error) + resolve func(ctx context.Context, bin, profileName string, forceRefresh bool) (config.Resolved, error) bin formaebin.BinResolver } @@ -47,11 +57,19 @@ func (r *Resolver) Managed() bool { return r.bin.Managed() } // Resolve produces the context for an optional profile name. The CLI is the // configuration authority: it reports which profile it actually used, so this // package never reasons about what "active" meant at the time of the call. -func (r *Resolver) Resolve(ctx context.Context, profileName string) (Context, error) { +// +// forceRefresh is for the 401 path, which re-resolves with a fresh credential +// and then checks the target did not move. +func (r *Resolver) Resolve(ctx context.Context, profileName string, forceRefresh bool) (Context, error) { bin := r.bin.Resolve() - res, err := r.resolve(ctx, bin, profileName) + res, err := r.resolve(ctx, bin, profileName, forceRefresh) if err != nil { return Context{}, err } - return Context{ProfileName: res.Profile, Conn: res.Conn, FormaeBin: bin}, nil + return Context{ + ProfileName: res.Profile, + Conn: res.Conn, + Credential: res.Credential, + FormaeBin: bin, + }, nil } diff --git a/internal/execctx/execctx_test.go b/internal/execctx/execctx_test.go index a769a1b..e79ee15 100644 --- a/internal/execctx/execctx_test.go +++ b/internal/execctx/execctx_test.go @@ -2,10 +2,13 @@ package execctx import ( "context" + "encoding/json" + "strings" "testing" "github.com/platform-engineering-labs/formae-mcp/internal/config" "github.com/platform-engineering-labs/formae-mcp/internal/formaebin" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" ) func testBin() formaebin.BinResolver { @@ -20,13 +23,16 @@ func testBin() formaebin.BinResolver { // the one that was asked for. func TestResolveCarriesTheConnection(t *testing.T) { r := &Resolver{ - resolve: func(_ context.Context, bin, profileName string) (config.Resolved, error) { + resolve: func(_ context.Context, bin, profileName string, forceRefresh bool) (config.Resolved, error) { if bin != "/usr/bin/formae" { t.Errorf("resolve called with bin %q, want the resolved binary", bin) } if profileName != "dev" { t.Errorf("resolve called with profile %q, want %q", profileName, "dev") } + if forceRefresh { + t.Error("an ordinary resolution must not force a refresh") + } return config.Resolved{ Profile: "dev-effective", Conn: config.Classic{URL: "http://localhost", Port: 49684}, @@ -35,7 +41,7 @@ func TestResolveCarriesTheConnection(t *testing.T) { bin: testBin(), } - ec, err := r.Resolve(context.Background(), "dev") + ec, err := r.Resolve(context.Background(), "dev", false) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -48,25 +54,76 @@ func TestResolveCarriesTheConnection(t *testing.T) { if ec.Conn != config.Connection(config.Classic{URL: "http://localhost", Port: 49684}) { t.Errorf("Conn = %#v", ec.Conn) } + if !ec.Credential.IsZero() { + t.Error("a classic context must carry no credential") + } } func TestResolveHosted(t *testing.T) { hosted := config.Hosted{ Endpoint: config.HostedOrigin, - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } r := &Resolver{ - resolve: func(context.Context, string, string) (config.Resolved, error) { - return config.Resolved{Profile: "prod", Conn: hosted}, nil + resolve: func(context.Context, string, string, bool) (config.Resolved, error) { + return config.Resolved{ + Profile: "prod", + Conn: hosted, + Credential: secret.New("Bearer live-token"), + }, nil }, bin: testBin(), } - ec, err := r.Resolve(context.Background(), "") + ec, err := r.Resolve(context.Background(), "", false) if err != nil { t.Fatalf("Resolve: %v", err) } if ec.Conn != config.Connection(hosted) { t.Errorf("Conn = %#v, want the hosted arm", ec.Conn) } + if ec.Credential.Reveal() != "Bearer live-token" { + t.Errorf("Credential = %q", ec.Credential.Reveal()) + } +} + +// The 401 path re-resolves with the credential refreshed, so the flag has to +// reach the CLI rather than being dropped at this seam. +func TestResolveForwardsAForcedRefresh(t *testing.T) { + var saw bool + r := &Resolver{ + resolve: func(_ context.Context, _, _ string, forceRefresh bool) (config.Resolved, error) { + saw = forceRefresh + return config.Resolved{ + Profile: "dev", + Conn: config.Classic{URL: "http://localhost", Port: 49684}, + }, nil + }, + bin: testBin(), + } + + if _, err := r.Resolve(context.Background(), "dev", true); err != nil { + t.Fatalf("Resolve: %v", err) + } + if !saw { + t.Fatal("a forced refresh did not reach the resolver") + } +} + +// The context gets logged and serialised in the ordinary course of debugging. +func TestContextDoesNotRenderTheCredential(t *testing.T) { + ec := Context{ + ProfileName: "prod", + Conn: config.Hosted{Endpoint: config.HostedOrigin, Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa"}, + Credential: secret.New("Bearer sup3rs3cr3t"), + FormaeBin: "/usr/bin/formae", + } + + out, err := json.Marshal(ec) + if err != nil { + t.Fatalf("marshalling the context: %v", err) + } + if strings.Contains(string(out), "sup3rs3cr3t") { + t.Fatalf("a JSON rendering of the context leaked the credential: %s", out) + } } diff --git a/internal/secret/secret.go b/internal/secret/secret.go new file mode 100644 index 0000000..a21014a --- /dev/null +++ b/internal/secret/secret.go @@ -0,0 +1,80 @@ +// Package secret holds a credential in a type that cannot be printed by +// accident. +// +// The invariant this exists for — no code path writes a credential into a tool +// result, an error, or a log — is not something a test can establish, because +// the next handler someone adds is not covered by it. So it is a property of +// the type instead: every rendering path masks, and there is exactly one +// accessor, which is greppable. +package secret + +import ( + "fmt" + "log/slog" +) + +// Mask is what a credential renders as. It says a value is present and +// withheld, which is more useful than an empty string that reads as absent. +const Mask = "" + +// Value holds a credential. +// +// Methods take value receivers so a copy cannot lose the masking: a Value +// travels into structs, closures and interface values constantly, and a pointer +// receiver would leave every copy rendering its raw field. +// +// One limit, and it is not a small one. fmt reaches these methods through +// reflect.Value's Interface, which it cannot call on an *unexported* field, so +// a struct holding a Value in an unexported field is printed reflectively and +// the credential comes straight out under %v. Exported fields, encoding/json, +// and slog are all fine. A type that keeps a Value unexported must mask itself, +// which is a thing to check for rather than assume. +type Value struct { + // v is unexported, so encoding/json and any other reflection-based encoder + // sees a struct with no exported fields even before the marshallers below + // are consulted. + v string +} + +// New wraps a credential. +func New(s string) Value { return Value{v: s} } + +// String masks. Format below covers the verbs String does not. +func (val Value) String() string { return Mask } + +// Format masks every verb, including %#v and %q. +// +// String alone is not enough: fmt consults Stringer only for %v and %s, so %#v +// would otherwise print the struct with its field, which is precisely the +// rendering a developer reaches for when debugging the thing that holds a +// credential. +func (val Value) Format(f fmt.State, verb rune) { + switch verb { + case 'q': + _, _ = fmt.Fprintf(f, "%q", Mask) + default: + _, _ = fmt.Fprint(f, Mask) + } +} + +// MarshalJSON masks. Serialisation is the leak path that matters most, because +// it happens to a whole struct without anyone naming the field. +func (val Value) MarshalJSON() ([]byte, error) { + return []byte(`"` + Mask + `"`), nil +} + +// MarshalYAML masks. Nothing here marshals YAML today; the method costs one +// line and no dependency, and closes the path before something does. +func (val Value) MarshalYAML() (any, error) { return Mask, nil } + +// LogValue masks. slog resolves this instead of formatting the value, so a +// credential passed as a log attribute never reaches the handler. +func (val Value) LogValue() slog.Value { return slog.StringValue(Mask) } + +// Reveal returns the credential. This is the only way out, and the only place +// worth auditing: grep for it. +func (val Value) Reveal() string { return val.v } + +// IsZero reports whether no credential is held, which is what a classic +// connection carries. +func (val Value) IsZero() bool { return val.v == "" } diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go new file mode 100644 index 0000000..8f3fe8e --- /dev/null +++ b/internal/secret/secret_test.go @@ -0,0 +1,160 @@ +package secret + +import ( + "bytes" + "encoding/json" + "fmt" + "log/slog" + "strings" + "testing" +) + +// token is recognisable enough that any leak shows up in a substring check. +const token = "Bearer sup3rs3cr3t-do-not-print-me" + +// assertHidden fails when the rendering leaked the token, and also when it did +// not produce the mask: a rendering that silently produced nothing at all would +// pass a leak check while telling the reader nothing. +// +// The mask is matched in its escaped form too, because encoding/json escapes +// the angle brackets by default. That is the encoder being correct, not the +// mask failing, and a consumer decodes it straight back. +func assertHidden(t *testing.T, what, got string) { + t.Helper() + if strings.Contains(got, "sup3rs3cr3t") { + t.Fatalf("%s leaked the credential: %s", what, got) + } + escaped := strings.NewReplacer("<", `\u003c`, ">", `\u003e`).Replace(Mask) + if !strings.Contains(got, Mask) && !strings.Contains(got, escaped) { + t.Fatalf("%s did not render the mask, got: %s", what, got) + } +} + +func TestValueMasksEveryFormattingVerb(t *testing.T) { + v := New(token) + + assertHidden(t, "Sprint", fmt.Sprint(v)) + assertHidden(t, "%v", fmt.Sprintf("%v", v)) + assertHidden(t, "%s", fmt.Sprintf("%s", v)) + assertHidden(t, "%q", fmt.Sprintf("%q", v)) + assertHidden(t, "%#v", fmt.Sprintf("%#v", v)) + assertHidden(t, "%+v", fmt.Sprintf("%+v", v)) + assertHidden(t, "String", v.String()) +} + +// The realistic leak is a struct reaching an encoder, not a deliberate print. +// Masking String alone does not stop encoding/json walking a field. +func TestValueMasksInsideAStruct(t *testing.T) { + held := struct { + Profile string + Credential Value + }{Profile: "prod", Credential: New(token)} + + out, err := json.Marshal(held) + if err != nil { + t.Fatalf("marshalling a struct holding a credential: %v", err) + } + assertHidden(t, "json.Marshal of a containing struct", string(out)) + + pretty, err := json.MarshalIndent(held, "", " ") + if err != nil { + t.Fatalf("marshal indent: %v", err) + } + assertHidden(t, "json.MarshalIndent of a containing struct", string(pretty)) +} + +// holder keeps a credential unexported, the way a routing or a client does. +type holder struct { + name string + credential Value +} + +// The limit, pinned rather than left to be discovered. fmt reaches these +// methods through reflect.Value's Interface, which it cannot call on an +// unexported field, so a struct holding a Value unexported prints the +// credential under %v. A type in that shape has to mask itself, and this test +// exists so that requirement is a documented fact rather than folklore. +func TestAnUnexportedFieldIsNotProtectedByTheTypeAlone(t *testing.T) { + h := holder{name: "prod", credential: New(token)} + + if !strings.Contains(fmt.Sprintf("%v", h), "sup3rs3cr3t") { + t.Skip("fmt no longer prints unexported fields reflectively; " + + "the caveat on Value can be dropped and holders can stop masking themselves") + } + + // The remedy, and the only one: the holder masks. + if strings.Contains(fmt.Sprintf("%v", maskedHolder(h)), "sup3rs3cr3t") { + t.Fatal("a holder that masks itself must not leak") + } +} + +type maskedHolder holder + +func (h maskedHolder) String() string { + return fmt.Sprintf("holder{name:%s credential:%s}", h.name, h.credential) +} + +func TestValueMasksInJSONAndYAML(t *testing.T) { + v := New(token) + + out, err := json.Marshal(v) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + assertHidden(t, "json.Marshal", string(out)) + + y, err := v.MarshalYAML() + if err != nil { + t.Fatalf("MarshalYAML: %v", err) + } + assertHidden(t, "MarshalYAML", fmt.Sprint(y)) +} + +func TestValueMasksInStructuredLogs(t *testing.T) { + var buf bytes.Buffer + slog.New(slog.NewJSONHandler(&buf, nil)). + Info("resolved", "credential", New(token)) + + assertHidden(t, "slog", buf.String()) +} + +func TestValueMasksWhenWrappedInAnError(t *testing.T) { + err := fmt.Errorf("resolving the connection: %w", + fmt.Errorf("credential %v was refused", New(token))) + + assertHidden(t, "a wrapped error", err.Error()) +} + +func TestRevealIsTheOnlyWayOut(t *testing.T) { + if got := New(token).Reveal(); got != token { + t.Fatalf("Reveal must return the credential verbatim, got %q", got) + } +} + +// The zero value is what a classic connection carries, so it must be safe to +// hold, render, and ask about without anyone reaching for a nil check. +func TestZeroValueIsUsable(t *testing.T) { + var v Value + + if !v.IsZero() { + t.Fatal("the zero value must report IsZero") + } + if got := v.Reveal(); got != "" { + t.Fatalf("the zero value must reveal the empty string, got %q", got) + } + if New(token).IsZero() { + t.Fatal("a value holding a credential must not report IsZero") + } +} + +// A copy is the normal way a Value travels: into a struct, through a channel, +// into a closure. Masking must survive it, which is what value receivers buy. +func TestACopyStillMasks(t *testing.T) { + original := New(token) + copied := original + + assertHidden(t, "a copy", fmt.Sprintf("%v", copied)) + if copied.Reveal() != token { + t.Fatal("a copy must still carry the credential") + } +} diff --git a/internal/server/ambiguity_test.go b/internal/server/ambiguity_test.go new file mode 100644 index 0000000..962ed22 --- /dev/null +++ b/internal/server/ambiguity_test.go @@ -0,0 +1,64 @@ +package server + +import ( + "context" + "strings" + "testing" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/tools" +) + +// The CLI decides ambiguity, because it is the only side that can settle it +// before a credential is minted. The MCP's job is to turn that refusal into +// something the caller can act on rather than an opaque failure. +// +// Elicitation and a remembered per-session profile are deliberately not here. +// This message works in every MCP client, needs no session state and no +// capability negotiation, and is what an elicitation path would fall back to +// anyway. +func TestAmbiguityBecomesAnActionableInstruction(t *testing.T) { + s := New("") + s.ctxResolver = &stubResolver{ + ec: execctx.Context{FormaeBin: "/usr/bin/formae"}, + err: &config.AmbiguousProfileError{ + Candidates: []string{"acme-prod", "acme-staging"}, + Active: "acme-prod", + }, + } + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler returned a transport error: %v", err) + } + if !res.IsError { + t.Fatal("ambiguity must reach the caller as a failed call it can retry") + } + + text := textContent(t, res) + for _, want := range []string{"acme-prod", "acme-staging", "active", "profile"} { + if !strings.Contains(text, want) { + t.Errorf("the instruction must mention %q so the caller can retry: %s", want, text) + } + } +} + +// An explicit profile settles the choice, so the caller's retry has to reach +// the resolver as the named profile rather than being dropped. +func TestAnExplicitProfileIsPassedThroughOnTheRetry(t *testing.T) { + r := &stubResolver{ec: execctx.Context{ + ProfileName: "acme-staging", + Conn: config.Classic{URL: "http://localhost", Port: 49684}, + FormaeBin: "/usr/bin/formae", + }} + s := New("") + s.ctxResolver = r + + if _, err := s.clientFor(context.Background(), "acme-staging"); err != nil { + t.Fatalf("clientFor: %v", err) + } + if r.sawProfile != "acme-staging" { + t.Fatalf("the named profile did not reach the resolver, got %q", r.sawProfile) + } +} diff --git a/internal/server/attribution.go b/internal/server/attribution.go new file mode 100644 index 0000000..cd40a72 --- /dev/null +++ b/internal/server/attribution.go @@ -0,0 +1,100 @@ +package server + +import ( + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" +) + +// reach is how far one call got toward its destination. +// +// Only the request executor may advance it. Deriving it from IsError, or from +// the context having resolved, is how it stops being true: a forma file that +// fails to evaluate has resolved a destination and contacted nothing, and an +// agent 500 is a failure that was nonetheless answered. +type reach int + +const ( + // reachResolved: a destination is known and nothing was sent. + reachResolved reach = iota + // reachAttempted: bytes left, and the outcome is unknown. + reachAttempted + // reachAnswered: the agent responded, whatever the status. + reachAnswered +) + +// destination is what a result says about where the call went. The zero value +// means no destination was ever resolved, which is the honest answer for a +// failure that happened before resolution. +type destination struct { + ec execctx.Context + reach reach +} + +// resolved names a destination for a call that has not built a client yet. +func resolved(ec execctx.Context) destination { + return destination{ec: ec, reach: reachResolved} +} + +// reached names a destination for a call that has a client, taking how far it +// got from the client itself rather than guessing. +func reached(ec execctx.Context, c *FormaeClient) destination { + if c == nil { + return resolved(ec) + } + return destination{ec: ec, reach: c.reach} +} + +// note renders the attribution line, or "" when there is nothing true to say. +// +// Only hosted calls carry one. A classic connection addresses the agent the +// user pointed at, which they already know; a hosted one addresses a single +// installation behind an endpoint shared with every other installation, and a +// profile name can later be repointed, so the name alone is weak evidence. +// +// The wording differs by reach because the three states are different claims. +// Saying an installation was addressed when nothing was sent would send an +// operator to check for work that cannot exist, which is worse than saying +// nothing at all. +func (d destination) note() string { + hosted, ok := d.ec.Conn.(config.Hosted) + if !ok { + return "" + } + switch d.reach { + case reachAnswered: + // "The hosted endpoint answered", not "the installation answered". + // Every response arrives from the shared edge, and the edge answers for + // itself when it cannot route: a hosted 404 on a collection is reported + // two lines away as a routing miss, so claiming the installation had + // answered would contradict it in the same result. Telling the two + // apart needs a stable edge error envelope, which does not exist yet. + return fmt.Sprintf("The hosted endpoint answered for installation %s, via profile %q.", + hosted.Installation, d.ec.ProfileName) + case reachAttempted: + return fmt.Sprintf( + "This request was sent for installation %s via profile %q and no response came back, "+ + "so its outcome is unknown: it may already have taken effect.", + hosted.Installation, d.ec.ProfileName) + default: + return fmt.Sprintf("Profile %q resolves to installation %s; nothing was sent.", + d.ec.ProfileName, hosted.Installation) + } +} + +// attribute appends the attribution to a result as its own content block. +// +// A separate block, never a wrapper or a prefix: the first block is the agent's +// payload and a consumer parses it, so changing its content would change the +// result schema. The version-skew notice already works this way. +func attribute(d destination, res *mcp.CallToolResult) *mcp.CallToolResult { + note := d.note() + if note == "" { + return res + } + res.Content = append(res.Content, &mcp.TextContent{Text: note}) + return res +} diff --git a/internal/server/attribution_test.go b/internal/server/attribution_test.go new file mode 100644 index 0000000..a65648a --- /dev/null +++ b/internal/server/attribution_test.go @@ -0,0 +1,360 @@ +package server + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "os" + "regexp" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" + "github.com/platform-engineering-labs/formae-mcp/internal/tools" +) + +func blocks(t *testing.T, res *mcp.CallToolResult) []string { + t.Helper() + out := make([]string, 0, len(res.Content)) + for _, c := range res.Content { + tc, ok := c.(*mcp.TextContent) + if !ok { + t.Fatalf("expected TextContent, got %T", c) + } + out = append(out, tc.Text) + } + return out +} + +// hostedServerFor wires a server whose resolved context is hosted but whose +// transport lands on srv, so a handler can be driven end to end. +func hostedServerFor(t *testing.T, srv *httptest.Server) (*Server, execctx.Context) { + t.Helper() + ec := execctx.Context{ + ProfileName: "acme-prod", + Conn: config.Hosted{Endpoint: srv.URL, Installation: testInstallation}, + Credential: secret.New("Bearer live-token"), + FormaeBin: "/usr/bin/formae", + } + s := New("") + s.ctxResolver = &stubResolver{ec: ec} + // The hosted arm validates its endpoint against a compile-time origin, so a + // handler test has to build its client through the seam rather than through + // the guard. The guard itself is covered directly in routing_test.go. + s.newClient = func(ec execctx.Context) (*FormaeClient, error) { + c := newTestHostedClientAt(srv, "Bearer live-token", nil) + return c, nil + } + return s, ec +} + +func TestHostedResultCarriesAttributionWithoutTouchingThePayload(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[{"label":"default"}]`)) + })) + defer srv.Close() + + s, _ := hostedServerFor(t, srv) + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if res.IsError { + t.Fatalf("unexpected error result: %s", blocks(t, res)) + } + + got := blocks(t, res) + if len(got) != 2 { + t.Fatalf("want the payload and one attribution block, got %d: %v", len(got), got) + } + if got[0] != `[{"label":"default"}]` { + t.Errorf("the first block must still be exactly the agent's payload, got %q", got[0]) + } + if !strings.Contains(got[1], testInstallation) || !strings.Contains(got[1], "acme-prod") { + t.Errorf("attribution must name the installation and the profile: %q", got[1]) + } + if !strings.Contains(got[1], "answered") { + t.Errorf("a call the agent answered should say so: %q", got[1]) + } +} + +func TestClassicResultCarriesNoAttribution(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[{"label":"default"}]`)) + })) + defer srv.Close() + + s := New("") + s.ctxResolver = &stubResolver{ec: execctx.Context{ + ProfileName: "dev", + Conn: config.Classic{URL: srv.URL}, + FormaeBin: "/usr/bin/formae", + }} + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if got := blocks(t, res); len(got) != 1 { + t.Fatalf("a classic result must carry one block, got %d: %v", len(got), got) + } +} + +// An agent error is still an answer, and saying so is what tells an operator +// the installation is reachable and rejecting them rather than unreachable. +func TestAnAgentErrorStillCountsAsAnswered(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + s, _ := hostedServerFor(t, srv) + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if !res.IsError { + t.Fatal("a 500 must be an error result") + } + + got := blocks(t, res) + if len(got) != 2 || !strings.Contains(got[1], "answered") { + t.Fatalf("an answered failure must be attributed as answered: %v", got) + } +} + +// The case the attribution exists for. A mutation whose transport fails after +// the request went out does not establish that the agent did nothing. +func TestAMutationThatFailsAfterDispatchSaysItMayHaveActed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Answer nothing and drop the connection, so the failure lands after + // the request has already been sent. + hj, ok := w.(http.Hijacker) + if !ok { + t.Fatal("test server does not support hijacking") + } + conn, _, err := hj.Hijack() + if err != nil { + t.Fatalf("hijack: %v", err) + } + _ = conn.Close() + })) + defer srv.Close() + + s, _ := hostedServerFor(t, srv) + res, _, err := s.handleForceSync(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if !res.IsError { + t.Fatal("a dropped connection must be an error result") + } + + got := blocks(t, res) + if len(got) != 2 { + t.Fatalf("want the error and one attribution block, got %v", got) + } + if !strings.Contains(got[1], "may already have taken effect") { + t.Errorf("a post-dispatch failure must not claim the agent did nothing: %q", got[1]) + } + if strings.Contains(got[1], "answered") { + t.Errorf("nothing answered, so the attribution must not say so: %q", got[1]) + } + if strings.Contains(got[1], "was sent to installation") { + t.Errorf("the MCP cannot know the installation itself received it: %q", got[1]) + } +} + +// Resolved is not addressed. An apply whose forma file fails to evaluate has a +// destination and contacted nothing; claiming otherwise would send an operator +// to check an installation for work that cannot exist. +func TestAFailureBeforeDispatchSaysNothingWasSent(t *testing.T) { + s, _ := hostedServerFor(t, httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }))) + + res, _, err := s.handleApplyForma(context.Background(), nil, tools.ApplyFormaInput{ + FilePath: "/nonexistent/forma.pkl", + Mode: "reconcile", + }) + if err != nil { + t.Fatalf("handler: %v", err) + } + if !res.IsError { + t.Fatal("a missing forma file must be an error result") + } + + got := blocks(t, res) + if len(got) != 2 { + t.Fatalf("want the error and one attribution block, got %v", got) + } + if !strings.Contains(got[1], "nothing was sent") { + t.Errorf("a pre-dispatch failure must say nothing was sent: %q", got[1]) + } +} + +// Before a context resolves there is no destination to name. +func TestAFailureBeforeResolutionCarriesNoAttribution(t *testing.T) { + s := New("") + s.ctxResolver = &stubResolver{ + ec: execctx.Context{FormaeBin: "/usr/bin/formae"}, + err: &config.AmbiguousProfileError{Candidates: []string{"a", "b"}, Active: "a"}, + } + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if got := blocks(t, res); len(got) != 1 { + t.Fatalf("an unresolved call has no destination to name, got %v", got) + } +} + +func TestNoResultCarriesTheCredential(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + ec := execctx.Context{ + ProfileName: "acme-prod", + Conn: config.Hosted{Endpoint: srv.URL, Installation: testInstallation}, + Credential: secret.New("Bearer sup3rs3cr3t"), + FormaeBin: "/usr/bin/formae", + } + s := New("") + s.ctxResolver = &stubResolver{ec: ec} + s.newClient = func(execctx.Context) (*FormaeClient, error) { + return newTestHostedClientAt(srv, "Bearer sup3rs3cr3t", nil), nil + } + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + for _, b := range blocks(t, res) { + if strings.Contains(b, "sup3rs3cr3t") { + t.Fatalf("a result leaked the credential: %s", b) + } + } +} + +// A structural check, in the spirit of TestNoDirectHTTPConstruction. Every +// result an agent-backed handler returns must be attributed, and a per-handler +// audit is precisely how one of them ends up not being. +// +// It checks each return rather than merely whether the handler mentions +// attribute() anywhere. The weaker form passes on a handler whose client-build +// failure is attributed and whose success path is not, which is the mistake +// most likely to be made. +func TestEveryAgentBackedHandlerAttributes(t *testing.T) { + handler := regexp.MustCompile(`(?s)func \(s \*Server\) (handle\w+)\([^)]*\) \((?:res )?\*mcp\.CallToolResult[^{]*\{(.*?)\n\}`) + returnsResult := regexp.MustCompile(`return (?:attribute\([^,]+, )?(jsonResult|textResult|errorResult|withNotice)\(`) + + for _, file := range []string{ + "server.go", "policy.go", "policy_standalone_tools.go", "profile_tools.go", + } { + src, err := os.ReadFile(file) + if err != nil { + t.Fatalf("reading %s: %v", file, err) + } + for _, m := range handler.FindAllStringSubmatch(string(src), -1) { + name, body := m[1], m[2] + reachesAgent := strings.Contains(body, "s.clientFor(") || + strings.Contains(body, "s.newClient(") || + strings.Contains(body, "s.fetchPolicies(") || + strings.Contains(body, `"extract"`) + if !reachesAgent { + continue + } + // A handler may attribute every path at once with a deferred + // assignment to a named result, which the per-return check cannot + // see and does not need to. + if strings.Contains(body, "defer func() { res = attribute(") { + continue + } + // Only returns after the client exists are checked. Before that + // there is genuinely nothing to name — an input-validation failure + // or a resolution failure has no destination — and the one case in + // between, a forma file that fails to evaluate after resolution, is + // covered behaviourally rather than structurally. + var haveClient bool + for _, line := range strings.Split(body, "\n") { + if strings.Contains(line, "s.newClient(") || strings.Contains(line, "s.fetchPolicies(") { + haveClient = true + continue + } + if !haveClient || !returnsResult.MatchString(line) { + continue + } + if !strings.Contains(line, "attribute(") { + t.Errorf("%s in %s returns an unattributed result once a client exists; "+ + "a hosted caller would not learn which installation acted:\n\t%s", + name, file, strings.TrimSpace(line)) + } + } + } + } +} + +// Extract reaches the agent through the CLI, so its output never passes +// through the executor's scrub. Its diagnostics are the whole value of the +// failure and are kept, bounded, with the credential we handed the process +// removed. +func TestExtractFailureOutputIsBoundedAndScrubbed(t *testing.T) { + t.Run("the credential we passed is removed", func(t *testing.T) { + got := safeSubprocessOutput( + []byte("plugin refused: Authorization: Bearer sup3rs3cr3t"), + secret.New("Bearer sup3rs3cr3t")) + + if strings.Contains(got, "sup3rs3cr3t") { + t.Fatalf("extract output leaked the credential: %s", got) + } + if !strings.Contains(got, "plugin refused") { + t.Errorf("the diagnostics are the point and must survive: %s", got) + } + }) + + t.Run("output is bounded", func(t *testing.T) { + got := safeSubprocessOutput(bytes.Repeat([]byte("x"), maxSubprocessOutput*4), secret.Value{}) + + if len(got) > maxSubprocessOutput+len("\n… truncated") { + t.Fatalf("output was not bounded: %d bytes", len(got)) + } + if !strings.Contains(got, "truncated") { + t.Errorf("a truncated result should say so") + } + }) + + // The order matters: truncating first would leave a credential that + // straddles the cutoff partly intact, because the search string is no + // longer present in the truncated bytes. + t.Run("a credential straddling the cutoff is still removed", func(t *testing.T) { + const cred = "Bearer sup3rs3cr3t-and-then-some-more" + for offset := -len(cred); offset <= 1; offset++ { + start := maxSubprocessOutput + offset + if start < 0 { + continue + } + raw := append(bytes.Repeat([]byte("x"), start), []byte(cred)...) + raw = append(raw, bytes.Repeat([]byte("y"), 128)...) + + got := safeSubprocessOutput(raw, secret.New(cred)) + + if strings.Contains(got, "sup3rs3cr3t") { + t.Fatalf("a credential starting at offset %d survived truncation: %q", + start, got[max(0, len(got)-160):]) + } + } + }) + + t.Run("a classic call has no credential to remove", func(t *testing.T) { + if got := safeSubprocessOutput([]byte("plain diagnostics"), secret.Value{}); got != "plain diagnostics" { + t.Fatalf("got %q", got) + } + }) +} diff --git a/internal/server/client.go b/internal/server/client.go index e6ebe8a..ebae064 100644 --- a/internal/server/client.go +++ b/internal/server/client.go @@ -15,28 +15,39 @@ import ( ) // FormaeClient is a lightweight HTTP client for the formae agent REST API. +// +// There is no endpoint field: where a request goes and what authenticates it +// are one decision, held by the routing, so neither can be paired with the +// other arm's. type FormaeClient struct { - endpoint string + route routing httpClient *http.Client + // reach is the high-water mark for this call: how far its requests got. + // One client is built per tool call, so there is nothing to reset, and the + // executor is the only thing that advances it. + reach reach } +// NewFormaeClient builds a classic client for an endpoint that already carries +// its port. func NewFormaeClient(endpoint string) *FormaeClient { return &FormaeClient{ - endpoint: endpoint, - httpClient: &http.Client{ - Timeout: 30 * time.Second, - }, + route: classicRoute{base: endpoint}, + httpClient: &http.Client{Timeout: 30 * time.Second}, } } -// newClientFromCtx builds a client for a resolved connection. Hosted is -// recognised and refused: this build cannot authenticate, and shipping a -// routing header without a credential would turn an understandable -// "unsupported" into a remote 401. +// newClientFromCtx builds a client for a resolved connection. +// +// The hosted connection is re-validated here rather than trusted from the CLI. +// The MCP exposes tools that write profiles, so a model fed hostile input can +// author a hosted profile naming the real issuer with an attacker's endpoint; +// checking again at the last point before a credential reaches a header is what +// makes that a refusal rather than an exfiltration. // // A classic connection with no port of its own carries one in its URL (a forced // endpoint), so it is used as-is. -func newClientFromCtx(ec execctx.Context) (*FormaeClient, error) { +func newClientFromCtx(ec execctx.Context, refresh refresher) (*FormaeClient, error) { switch conn := ec.Conn.(type) { case config.Classic: endpoint := conn.URL @@ -44,26 +55,47 @@ func newClientFromCtx(ec execctx.Context) (*FormaeClient, error) { endpoint = fmt.Sprintf("%s:%d", conn.URL, conn.Port) } return NewFormaeClient(endpoint), nil + case config.Hosted: - return nil, fmt.Errorf( - "profile %q targets hosted formae, which this build does not support yet", - ec.ProfileName) + if err := config.ValidateHosted(conn); err != nil { + return nil, fmt.Errorf("profile %q resolved an unusable hosted connection: %w", + ec.ProfileName, err) + } + if ec.Credential.IsZero() { + return nil, fmt.Errorf( + "profile %q targets hosted formae but resolved no credential, so it cannot be authenticated", + ec.ProfileName) + } + return &FormaeClient{ + route: &hostedRoute{ + endpoint: conn.Endpoint, + installation: conn.Installation, + credential: ec.Credential, + refreshFn: refresh, + }, + httpClient: &http.Client{ + Timeout: 30 * time.Second, + // Refused rather than followed; see refuseRedirects. + CheckRedirect: refuseRedirects, + }, + }, nil + default: return nil, fmt.Errorf("profile %q resolved no usable connection", ec.ProfileName) } } -func (c *FormaeClient) get(ctx context.Context, path string, query url.Values) ([]byte, int, error) { - return c.do(ctx, request{Method: http.MethodGet, Path: path, Query: query}) +func (c *FormaeClient) get(ctx context.Context, path string, query url.Values, retry retryPolicy) ([]byte, int, error) { + return c.do(ctx, request{Method: http.MethodGet, Path: path, Query: query}, retry) } -func (c *FormaeClient) post(ctx context.Context, path string, query url.Values) ([]byte, int, error) { +func (c *FormaeClient) post(ctx context.Context, path string, query url.Values, retry retryPolicy) ([]byte, int, error) { return c.do(ctx, request{ Method: http.MethodPost, Path: path, Query: query, ContentType: "application/json", - }) + }, retry) } // ListResources queries the agent for resources matching the given query string. @@ -73,12 +105,15 @@ func (c *FormaeClient) ListResources(ctx context.Context, query string) (json.Ra q.Set("query", query) } - body, status, err := c.get(ctx, "/api/v1/resources", q) + body, status, err := c.get(ctx, "/api/v1/resources", q, retryOnce) if err != nil { return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) + } + if err := c.unroutedIf(status); err != nil { + return nil, err } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -89,12 +124,15 @@ func (c *FormaeClient) ListResources(ctx context.Context, query string) (json.Ra // ListStacks retrieves all stacks from the agent. func (c *FormaeClient) ListStacks(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.get(ctx, "/api/v1/stacks", nil) + body, status, err := c.get(ctx, "/api/v1/stacks", nil, retryOnce) if err != nil { return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) + } + if err := c.unroutedIf(status); err != nil { + return nil, err } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -105,12 +143,15 @@ func (c *FormaeClient) ListStacks(ctx context.Context) (json.RawMessage, error) // ListPolicies retrieves all standalone policies from the agent. func (c *FormaeClient) ListPolicies(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.get(ctx, "/api/v1/policies", nil) + body, status, err := c.get(ctx, "/api/v1/policies", nil, retryOnce) if err != nil { return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) + } + if err := c.unroutedIf(status); err != nil { + return nil, err } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -126,12 +167,15 @@ func (c *FormaeClient) ListTargets(ctx context.Context, query string) (json.RawM q.Set("query", query) } - body, status, err := c.get(ctx, "/api/v1/targets", q) + body, status, err := c.get(ctx, "/api/v1/targets", q, retryOnce) if err != nil { return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) + } + if err := c.unroutedIf(status); err != nil { + return nil, err } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -150,13 +194,24 @@ func (c *FormaeClient) GetCommandStatus(ctx context.Context, commandID string, c Path: "/api/v1/commands/status", Query: q, Headers: map[string]string{"Client-ID": clientID}, - }) + }, retryOnce) if err != nil { return nil, err } if status == http.StatusNotFound { + // Both readings, because the status alone cannot separate them: the + // agent answers 404 for a command it does not know, and the edge + // answers 404 for an installation it cannot route. Asserting the + // first sends the reader hunting for a command that was never the + // problem. + if unrouted := c.route.unrouted(); unrouted != nil { + return nil, fmt.Errorf("command %s was not found, or %w", commandID, unrouted) + } return nil, fmt.Errorf("command %s not found", commandID) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -179,12 +234,15 @@ func (c *FormaeClient) ListCommands(ctx context.Context, query string, maxResult Path: "/api/v1/commands/status", Query: q, Headers: map[string]string{"Client-ID": clientID}, - }) + }, retryOnce) if err != nil { return nil, err } if status == http.StatusNotFound { - return json.RawMessage(`{"Commands":[]}`), nil + return c.route.collectionMiss(json.RawMessage(`{"Commands":[]}`)) + } + if err := c.unroutedIf(status); err != nil { + return nil, err } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -195,10 +253,13 @@ func (c *FormaeClient) ListCommands(ctx context.Context, query string, maxResult // GetAgentStats retrieves agent statistics. func (c *FormaeClient) GetAgentStats(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.get(ctx, "/api/v1/stats", nil) + body, status, err := c.get(ctx, "/api/v1/stats", nil, retryOnce) if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -208,10 +269,13 @@ func (c *FormaeClient) GetAgentStats(ctx context.Context) (json.RawMessage, erro // CheckHealth checks if the agent is healthy. func (c *FormaeClient) CheckHealth(ctx context.Context) error { - _, status, err := c.get(ctx, "/api/v1/health", nil) + _, status, err := c.get(ctx, "/api/v1/health", nil, retryOnce) if err != nil { return fmt.Errorf("agent is not reachable: %w", err) } + if err := c.unroutedIf(status); err != nil { + return err + } if status != http.StatusOK { return fmt.Errorf("agent returned unhealthy status: %d", status) } @@ -244,6 +308,9 @@ func (c *FormaeClient) SubmitCommand(ctx context.Context, command string, mode s if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if !isCommandStatusOK(status, simulate) { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -263,6 +330,9 @@ func (c *FormaeClient) DestroyByQuery(ctx context.Context, query string, simulat if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if !isCommandStatusOK(status, simulate) { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -289,12 +359,12 @@ func (c *FormaeClient) CancelCommands(ctx context.Context, query string, clientI Path: "/api/v1/commands/cancel", Query: q, Headers: map[string]string{"Client-ID": clientID}, - }) + }, noRetry) if err != nil { return nil, err } if status == http.StatusNotFound { - return json.RawMessage(`{"CommandIds":[]}`), nil + return c.route.collectionMiss(json.RawMessage(`{"CommandIds":[]}`)) } if status != http.StatusAccepted { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -306,10 +376,13 @@ func (c *FormaeClient) CancelCommands(ctx context.Context, query string, clientI // ListChangesSinceLastReconcile retrieves modifications since last reconcile for a stack. func (c *FormaeClient) ListChangesSinceLastReconcile(ctx context.Context, stack string) (json.RawMessage, error) { path := fmt.Sprintf("/api/v1/stacks/%s/changes-since-last-reconcile", url.PathEscape(stack)) - body, status, err := c.get(ctx, path, nil) + body, status, err := c.get(ctx, path, nil, retryOnce) if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -319,10 +392,13 @@ func (c *FormaeClient) ListChangesSinceLastReconcile(ctx context.Context, stack // ForceSync triggers an immediate resource synchronization. func (c *FormaeClient) ForceSync(ctx context.Context) error { - _, status, err := c.post(ctx, "/api/v1/admin/synchronize", nil) + _, status, err := c.post(ctx, "/api/v1/admin/synchronize", nil, noRetry) if err != nil { return err } + if err := c.unroutedIf(status); err != nil { + return err + } if status != http.StatusOK { return fmt.Errorf("agent returned status %d", status) } @@ -332,10 +408,13 @@ func (c *FormaeClient) ForceSync(ctx context.Context) error { // ForceDiscover triggers an immediate resource discovery. func (c *FormaeClient) ForceDiscover(ctx context.Context) error { - _, status, err := c.post(ctx, "/api/v1/admin/discover", nil) + _, status, err := c.post(ctx, "/api/v1/admin/discover", nil, noRetry) if err != nil { return err } + if err := c.unroutedIf(status); err != nil { + return err + } if status != http.StatusOK { return fmt.Errorf("agent returned status %d", status) } @@ -345,10 +424,13 @@ func (c *FormaeClient) ForceDiscover(ctx context.Context) error { // ForceCheckTTL triggers an immediate TTL expiry sweep. func (c *FormaeClient) ForceCheckTTL(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.post(ctx, "/api/v1/admin/check-ttl", nil) + body, status, err := c.post(ctx, "/api/v1/admin/check-ttl", nil, noRetry) if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -360,10 +442,13 @@ func (c *FormaeClient) ForceCheckTTL(ctx context.Context) (json.RawMessage, erro // but body is also returned so callers can surface the agent's error JSON. func (c *FormaeClient) ForceReconcileStack(ctx context.Context, label string) (json.RawMessage, int, error) { path := fmt.Sprintf("/api/v1/stacks/%s/reconcile", url.PathEscape(label)) - body, status, err := c.post(ctx, path, nil) + body, status, err := c.post(ctx, path, nil, noRetry) if err != nil { return nil, 0, err } + if err := c.unroutedIf(status); err != nil { + return body, status, err + } if status != http.StatusOK && status != http.StatusAccepted { return body, status, fmt.Errorf("agent returned status %d", status) } @@ -401,5 +486,16 @@ func (c *FormaeClient) postMultipartWithHeaders(ctx context.Context, path string Headers: headers, Body: &buf, ContentType: w.FormDataContentType(), - }) + }, noRetry) +} + +// unroutedIf explains a 404 from an endpoint that has no "this object does not +// exist" reading. The agent answers those with 200 or an error, never 404, so +// under hosted a 404 came from the edge rather than from the installation. +// Classic reports nothing and the caller's own status handling stands. +func (c *FormaeClient) unroutedIf(status int) error { + if status != http.StatusNotFound { + return nil + } + return c.route.unrouted() } diff --git a/internal/server/collection_test.go b/internal/server/collection_test.go new file mode 100644 index 0000000..d222d53 --- /dev/null +++ b/internal/server/collection_test.go @@ -0,0 +1,195 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// The six endpoints that translate a 404 into an empty payload. Under hosted +// the shared edge answers 404 for an unknown or unrouted installation, so +// reporting "nothing here" would present a routing failure as a plausible +// answer — the one failure mode nobody investigates. +var collectionCalls = map[string]struct { + call func(context.Context, *FormaeClient) (json.RawMessage, error) + wantEmpty string +}{ + "ListResources": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListResources(ctx, "") }, + wantEmpty: "[]", + }, + "ListStacks": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListStacks(ctx) }, + wantEmpty: "[]", + }, + "ListPolicies": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListPolicies(ctx) }, + wantEmpty: "[]", + }, + "ListTargets": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListTargets(ctx, "") }, + wantEmpty: "[]", + }, + "ListCommands": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { + return c.ListCommands(ctx, "", "10", "cid") + }, + wantEmpty: `{"Commands":[]}`, + }, + "CancelCommands": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { + return c.CancelCommands(ctx, "", "cid") + }, + wantEmpty: `{"CommandIds":[]}`, + }, +} + +func notFoundServer(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestClassic404OnACollectionStaysAnEmptyList(t *testing.T) { + for name, tc := range collectionCalls { + t.Run(name, func(t *testing.T) { + c := newTestFormaeClient(notFoundServer(t)) + + got, err := tc.call(context.Background(), c) + if err != nil { + t.Fatalf("%s: a classic 404 must still read as empty: %v", name, err) + } + if string(got) != tc.wantEmpty { + t.Errorf("%s: got %s, want %s", name, got, tc.wantEmpty) + } + }) + } +} + +func TestHosted404OnACollectionIsARoutingError(t *testing.T) { + for name, tc := range collectionCalls { + t.Run(name, func(t *testing.T) { + c := newTestHostedClient(t, notFoundServer(t), "Bearer live-token", nil) + + got, err := tc.call(context.Background(), c) + if err == nil { + t.Fatalf("%s: a hosted 404 must not read as empty, got %s", name, got) + } + if !strings.Contains(err.Error(), "did not route") { + t.Errorf("%s: the error should say the request was not routed: %v", name, err) + } + if !strings.Contains(err.Error(), testInstallation) { + t.Errorf("%s: the error should name the installation it addressed: %v", name, err) + } + }) + } +} + +// GetCommandStatus is deliberately not in that set. Its 404 is an +// endpoint-specific "no such command" and keeps its meaning under both modes. +// Telling the two apart properly needs a stable edge error envelope, which does +// not exist yet, so the narrowing stops where the ambiguity starts. +func TestCommandStatus404KeepsItsMeaning(t *testing.T) { + t.Run("classic", func(t *testing.T) { + c := newTestFormaeClient(notFoundServer(t)) + + _, err := c.GetCommandStatus(context.Background(), "cmd-1", "cid") + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("want a command-not-found error, got %v", err) + } + }) + + // Under hosted the same 404 has two readings and the status cannot separate + // them, so it reports both rather than asserting the one that sends the + // reader hunting for a command that was never the problem. + t.Run("hosted", func(t *testing.T) { + c := newTestHostedClient(t, notFoundServer(t), "Bearer live-token", nil) + + _, err := c.GetCommandStatus(context.Background(), "cmd-1", "cid") + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "cmd-1 was not found") { + t.Errorf("the object reading must survive: %v", err) + } + if !strings.Contains(err.Error(), "did not route") { + t.Errorf("the routing reading must be offered too: %v", err) + } + }) +} + +// An installation can disappear underneath a live session: a trial ends, a +// subscription lapses, someone suspends or destroys it. Sessions stay open for +// days, so this is an ordinary event rather than an exotic one, and when it +// happens every call fails at once. Each of them has to say the same true +// thing rather than three different misleading ones. +func TestEveryCallExplainsAGoneInstallation(t *testing.T) { + srv := notFoundServer(t) + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + ctx := context.Background() + + calls := map[string]func() error{ + "ListResources": func() error { _, e := c.ListResources(ctx, ""); return e }, + "ListStacks": func() error { _, e := c.ListStacks(ctx); return e }, + "ListTargets": func() error { _, e := c.ListTargets(ctx, ""); return e }, + "ListPolicies": func() error { _, e := c.ListPolicies(ctx); return e }, + "ListCommands": func() error { _, e := c.ListCommands(ctx, "", "10", "cid"); return e }, + "CancelCommands": func() error { _, e := c.CancelCommands(ctx, "", "cid"); return e }, + "GetCommandStatus": func() error { + _, e := c.GetCommandStatus(ctx, "cmd-1", "cid") + return e + }, + "CheckHealth": func() error { return c.CheckHealth(ctx) }, + "GetAgentStats": func() error { _, e := c.GetAgentStats(ctx); return e }, + "ListChanges": func() error { _, e := c.ListChangesSinceLastReconcile(ctx, "default"); return e }, + "SubmitCommand": func() error { + _, e := c.SubmitCommand(ctx, "apply", "reconcile", false, false, []byte("{}"), "cid") + return e + }, + "DestroyByQuery": func() error { _, e := c.DestroyByQuery(ctx, "stack:x", false, "cid"); return e }, + "ForceSync": func() error { return c.ForceSync(ctx) }, + "ForceDiscover": func() error { return c.ForceDiscover(ctx) }, + "ForceCheckTTL": func() error { _, e := c.ForceCheckTTL(ctx); return e }, + "ForceReconcile": func() error { _, _, e := c.ForceReconcileStack(ctx, "default"); return e }, + } + for name, call := range calls { + t.Run(name, func(t *testing.T) { + err := call() + if err == nil { + t.Fatal("a gone installation must not read as success") + } + if !strings.Contains(err.Error(), testInstallation) { + t.Errorf("must name the installation that could not be reached: %v", err) + } + if !strings.Contains(err.Error(), "subscription") { + t.Errorf("must offer the reason a reader can act on: %v", err) + } + // The edge's body is not an explanation and must not be pasted in. + if strings.Contains(err.Error(), "404 page not found") { + t.Errorf("the endpoint's body reached the caller: %v", err) + } + }) + } +} + +// The same 404 against a self-hosted agent keeps meaning what the endpoint says +// it means. Only the shared edge is ambiguous. +func TestClassicKeepsItsOwn404Meanings(t *testing.T) { + c := newTestFormaeClient(notFoundServer(t)) + ctx := context.Background() + + if _, err := c.GetCommandStatus(ctx, "cmd-1", "cid"); err == nil || + !strings.Contains(err.Error(), "command cmd-1 not found") || + strings.Contains(err.Error(), "subscription") { + t.Errorf("a classic command lookup must report the command, nothing more: %v", err) + } + if err := c.CheckHealth(ctx); err == nil || !strings.Contains(err.Error(), "unhealthy status: 404") { + t.Errorf("classic health keeps its own wording: %v", err) + } +} diff --git a/internal/server/policy.go b/internal/server/policy.go index 2b526bb..572e934 100644 --- a/internal/server/policy.go +++ b/internal/server/policy.go @@ -23,7 +23,12 @@ func currentEvalFunc(bin string) EvalFunc { return makeFormaeEval(bin) } -func (s *Server) handleCreateInlinePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateInlinePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleCreateInlinePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateInlinePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if err := validateCreateInlinePolicyInput(input); err != nil { return errorResult(err), nil, nil } @@ -51,7 +56,9 @@ func (s *Server) handleCreateInlinePolicy(ctx context.Context, _ *mcp.CallToolRe // policies known from the agent" and the source check still runs. var inventory []policyInventoryItem if input.Operation == "set" { - if items, err := s.fetchPolicies(ctx); err == nil { + items, d, err := s.fetchPolicies(ctx) + dest = d + if err == nil { inventory = items } for _, item := range inventory { diff --git a/internal/server/policy_standalone_tools.go b/internal/server/policy_standalone_tools.go index 3407574..13661a8 100644 --- a/internal/server/policy_standalone_tools.go +++ b/internal/server/policy_standalone_tools.go @@ -63,20 +63,38 @@ func mcpPolicyType(agentType string) string { // fetchPolicies reads the agent's standalone policy inventory from the // active/default profile's agent (empty profile = active/default, matching the // server's per-call client resolution). -func (s *Server) fetchPolicies(ctx context.Context) ([]policyInventoryItem, error) { - c, err := s.clientFor(ctx, "") +// +// It reports the destination alongside the inventory because callers +// deliberately swallow its error: a planner's real work is local file +// planning, so an unreachable agent downgrades to "no policies known" rather +// than failing the tool. That makes "the agent answered" underivable from the +// planner's result, and a clean plan claiming an installation had answered +// would assert something false about a real installation. +// +// Under hosted this makes a second thing visible. These tools take no profile +// argument, so the inventory read resolves the active profile; with more than +// one profile that is ambiguous, and the ambiguity is swallowed like any other +// failure. The attribution is what says the installation was never reached. +// Giving the policy tools a profile argument is the actual fix, and belongs +// with whatever revisits that tool surface. +func (s *Server) fetchPolicies(ctx context.Context) ([]policyInventoryItem, destination, error) { + ec, err := s.resolveCtx(ctx, "") if err != nil { - return nil, err + return nil, destination{}, err + } + c, err := s.newClient(ec) + if err != nil { + return nil, resolved(ec), err } body, err := c.ListPolicies(ctx) if err != nil { - return nil, fmt.Errorf("list policies from agent: %w", err) + return nil, reached(ec, c), fmt.Errorf("list policies from agent: %w", err) } var items []policyInventoryItem if err := json.Unmarshal(body, &items); err != nil { - return nil, fmt.Errorf("parse policy inventory: %w", err) + return nil, reached(ec, c), fmt.Errorf("parse policy inventory: %w", err) } - return items, nil + return items, reached(ec, c), nil } // standaloneTypeOf resolves a standalone policy label to its MCP policy type, @@ -136,7 +154,12 @@ func validateStandalonePolicyFields(label, policyType string, ttlSeconds int64, return nil } -func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateStandalonePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateStandalonePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if err := validateStandalonePolicyFields(input.Label, input.PolicyType, input.TTLSeconds, input.OnDependents, input.IntervalSeconds); err != nil { return errorResult(err), nil, nil } @@ -153,7 +176,9 @@ func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallTo // state. The agent inventory is authoritative for what already exists, so // check it first: a policy the agent already knows must not be re-declared, // even if the current workspace source does not (yet) contain it. - if agentItems, err := s.fetchPolicies(ctx); err == nil { + agentItems, d, err := s.fetchPolicies(ctx) + dest = d + if err == nil { if _, known := findPolicyByLabel(agentItems, input.Label); known { out := tools.CreateStandalonePolicyOutput{ Operation: "noop", @@ -241,7 +266,12 @@ func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallTo return jsonResult(body), nil, nil } -func (s *Server) handleAttachStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.AttachStandalonePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleAttachStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.AttachStandalonePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if input.Stack == "" { return errorResult(fmt.Errorf("stack is required")), nil, nil } @@ -263,7 +293,8 @@ func (s *Server) handleAttachStandalonePolicy(ctx context.Context, _ *mcp.CallTo // before the first apply. Fall back to the workspace source in that case // rather than refusing a documented flow. var notes []string - items, fetchErr := s.fetchPolicies(ctx) + items, d, fetchErr := s.fetchPolicies(ctx) + dest = d if fetchErr != nil { items = nil } @@ -429,7 +460,12 @@ func specFromInventoryItem(item policyInventoryItem) (StandalonePolicySpec, erro }, nil } -func (s *Server) handleDeleteStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.DeleteStandalonePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleDeleteStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.DeleteStandalonePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if input.Label == "" { return errorResult(fmt.Errorf("label is required")), nil, nil } @@ -437,7 +473,8 @@ func (s *Server) handleDeleteStandalonePolicy(ctx context.Context, _ *mcp.CallTo return errorResult(err), nil, nil } - inventory, err := s.fetchPolicies(ctx) + inventory, d, err := s.fetchPolicies(ctx) + dest = d if err != nil { return errorResult(err), nil, nil } diff --git a/internal/server/requests.go b/internal/server/requests.go index f689f7a..c829401 100644 --- a/internal/server/requests.go +++ b/internal/server/requests.go @@ -2,9 +2,11 @@ package server import ( "context" + "errors" "fmt" "io" "net/http" + "net/http/httptrace" "net/url" ) @@ -20,14 +22,68 @@ type request struct { ContentType string } +// retryPolicy says whether a request may be sent a second time after its +// credential has been refreshed. +// +// It is an argument rather than a field on request, so a new call site cannot +// inherit an answer by leaving a zero value alone. Retryability is a property +// of the request and not of its verb: a simulated apply is a POST that may be +// safe, and a nominally read-only tool may issue a POST, so inferring it from +// the method or from a tool annotation would be guessing. +type retryPolicy int + +const ( + // noRetry returns the original failure. Mutations use this: nothing + // establishes that a 401 always precedes dispatch, and duplicating an + // infrastructure mutation to save one error is the wrong trade. + noRetry retryPolicy = iota + // retryOnce resends once, and only once, after a successful refresh. + retryOnce +) + +// maxResponseBytes bounds one agent response. +const maxResponseBytes = 32 << 20 + +var errResponseTooLarge = errors.New("the agent returned more data than this build will read") + +// errRetryableBody guards a combination that would corrupt a request silently. +var errRetryableBody = errors.New( + "a retryable request may not carry a body: the first attempt consumes the reader, " + + "so a retry would send an empty one") + // do executes a request against the agent and returns the body and status. -func (c *FormaeClient) do(ctx context.Context, r request) ([]byte, int, error) { - u := c.endpoint + r.Path - if len(r.Query) > 0 { - u += "?" + r.Query.Encode() +func (c *FormaeClient) do(ctx context.Context, r request, retry retryPolicy) ([]byte, int, error) { + if retry == retryOnce && r.Body != nil { + return nil, 0, errRetryableBody + } + + body, status, err := c.send(ctx, r) + if err != nil || status != http.StatusUnauthorized { + return body, status, err } - req, err := http.NewRequestWithContext(ctx, r.Method, u, r.Body) + // A 401 has two distinct sources, and only one of them is actionable here. + // A credential command that failed carries the plugin's own code and was + // reported at resolution. This 401 arrived after a successful resolution, + // so there is no plugin error behind it: it can mean expiry, wrong + // audience, wrong issuer, or a malformed credential, and nothing here can + // tell which. Refresh, and say no more than "unauthorized" if it recurs. + // + // 403 has no branch at all, deliberately: a denied installation or tenant + // is an authorization failure, not an expired session. + refreshed, rerr := c.route.refresh(ctx) + if rerr != nil { + return body, status, rerr + } + if !refreshed || retry != retryOnce { + return body, status, nil + } + return c.send(ctx, r) +} + +// send performs exactly one attempt. +func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) { + req, err := http.NewRequestWithContext(ctx, r.Method, c.route.url(r.Path, r.Query), r.Body) if err != nil { return nil, 0, fmt.Errorf("building request: %w", err) } @@ -37,16 +93,59 @@ func (c *FormaeClient) do(ctx context.Context, r request) ([]byte, int, error) { for k, v := range r.Headers { req.Header.Set(k, v) } + // The routing decorates last, so a caller cannot displace the routing + // header or the credential by naming one in Headers. + c.route.decorate(req.Header) + + // Attempted means bytes left this process, which is what "it may have + // acted" rests on. Taken from the trace rather than from "we are about to + // call Do", because a DNS or TLS failure sends nothing and telling an + // operator to go and check would be a false alarm in the costly direction. + req = req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{ + WroteRequest: func(httptrace.WroteRequestInfo) { + // Any invocation counts, including one carrying an error. The + // callback runs only after a connection exists and a write was + // attempted, and a write can fail having already put part of a + // multipart mutation on the wire. DNS, connection and TLS failures + // never reach here, which is the distinction this is for. + c.advance(reachAttempted) + }, + })) resp, err := c.httpClient.Do(req) if err != nil { return nil, 0, fmt.Errorf("request failed: %w", err) } + c.advance(reachAnswered) defer func() { _ = resp.Body.Close() }() - body, err := io.ReadAll(resp.Body) + // A 3xx only reaches here on a connection whose policy refuses to follow + // redirects. Reporting it as routing rather than passing the status up + // keeps a caller from treating a redirect body as an answer. + if resp.StatusCode >= 300 && resp.StatusCode < 400 { + return nil, resp.StatusCode, fmt.Errorf( + "the hosted endpoint answered %d with a redirect, which is refused: "+ + "this is a routing problem, not a response", resp.StatusCode) + } + + // Bounded, and scrubbed before it can reach a caller. The bound matches the + // one the configuration oracle already applies to its subprocess: an + // unbounded read from a peer is an unbounded allocation, and under hosted + // that peer is remote and shared rather than a process on this machine. + body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1)) if err != nil { return nil, resp.StatusCode, fmt.Errorf("failed to read response: %w", err) } - return body, resp.StatusCode, nil + if len(body) > maxResponseBytes { + return nil, resp.StatusCode, errResponseTooLarge + } + return c.route.scrub(body), resp.StatusCode, nil +} + +// advance raises the high-water mark, never lowers it: a call that answered +// once has answered, whatever a later attempt does. +func (c *FormaeClient) advance(r reach) { + if r > c.reach { + c.reach = r + } } diff --git a/internal/server/requests_test.go b/internal/server/requests_test.go index 656ebea..cc31578 100644 --- a/internal/server/requests_test.go +++ b/internal/server/requests_test.go @@ -25,7 +25,7 @@ func TestDo_SendsSuppliedHeaders(t *testing.T) { Method: "GET", Path: "/api/v1/health", Headers: map[string]string{"Client-ID": "abc123"}, - }); err != nil { + }, noRetry); err != nil { t.Fatalf("do: unexpected error: %v", err) } if got != "abc123" { @@ -45,7 +45,7 @@ func TestDo_HonoursCancellation(t *testing.T) { cancel() c := newTestFormaeClient(srv) - if _, _, err := c.do(ctx, request{Method: "GET", Path: "/api/v1/health"}); err == nil { + if _, _, err := c.do(ctx, request{Method: "GET", Path: "/api/v1/health"}, noRetry); err == nil { t.Fatal("do with a cancelled context: expected an error, got nil") } } diff --git a/internal/server/routing.go b/internal/server/routing.go new file mode 100644 index 0000000..e68f5ca --- /dev/null +++ b/internal/server/routing.go @@ -0,0 +1,220 @@ +package server + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "slices" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +// installationHeader routes a request to one hosted installation. The edge +// selects a backend on this alone, which is why it is set exactly once. +const installationHeader = "Formae-Installation" + +// refresher re-resolves this call's profile with the credential refreshed. It +// is injected rather than reached for directly, so the credential a retry uses +// comes from the same seam the first resolution did. +type refresher func(ctx context.Context) (execctx.Context, error) + +// routing is how one connection addresses and authenticates its requests. +// +// The address lives here with the credential rather than beside them on the +// client: a hosted route holding a classic endpoint, or a classic route holding +// the hosted one, is then not a thing that can be written down. +type routing interface { + // url builds the request URL for a path and query. + url(path string, q url.Values) string + + // decorate adds the headers this connection requires. + decorate(h http.Header) + + // collectionMiss answers a 404 from an endpoint that lists things. + collectionMiss(empty json.RawMessage) (json.RawMessage, error) + + // unrouted reports the error for a 404 this connection cannot explain as an + // object simply being absent, or nil when it can. + unrouted() error + + // scrub removes any credential this connection has sent from bytes the far + // end returned, before they can reach a result, an error, or a log. + scrub(body []byte) []byte + + // refresh re-resolves the credential and reports whether anything changed, + // so the caller knows whether a second attempt could differ from the first. + refresh(ctx context.Context) (bool, error) +} + +// classicRoute addresses a self-hosted agent. +type classicRoute struct { + base string +} + +func (r classicRoute) url(path string, q url.Values) string { return joinURL(r.base, path, q) } + +func (classicRoute) decorate(http.Header) {} + +func (classicRoute) collectionMiss(empty json.RawMessage) (json.RawMessage, error) { + return empty, nil +} + +// scrub does nothing: the MCP sends a self-hosted agent no credential, so a +// response cannot be quoting one. +func (classicRoute) scrub(body []byte) []byte { return body } + +// unrouted reports nothing. A self-hosted agent answers for itself, so its 404 +// means whatever the endpoint says a 404 means. +func (classicRoute) unrouted() error { return nil } + +// refresh does nothing. The MCP sends a self-hosted agent no credential, so +// there is nothing a second attempt would do differently. +func (classicRoute) refresh(context.Context) (bool, error) { return false, nil } + +// hostedRoute addresses one installation behind the shared edge. +type hostedRoute struct { + endpoint string + installation string + credential secret.Value + refreshFn refresher + // used is every credential this route has put on the wire, kept so a + // response quoting one can be scrubbed. It is bounded by one refresh. + used []string +} + +// String and GoString mask, and holding a secret.Value is not enough on its own +// to make that true. +// +// fmt reaches a field's Format or String method only through reflect.Value's +// Interface, which it cannot call on an *unexported* field. So a struct with an +// unexported secret.Value is printed reflectively, field by field, and the +// credential comes straight out — which is exactly what a developer sees when +// they print the routing while debugging the thing that holds it. Any type +// keeping a credential in an unexported field has to mask itself. +func (r *hostedRoute) String() string { + return fmt.Sprintf("hosted{endpoint:%s installation:%s credential:%s}", + r.endpoint, r.installation, r.credential) +} + +func (r *hostedRoute) GoString() string { return r.String() } + +func (r *hostedRoute) url(path string, q url.Values) string { return joinURL(r.endpoint, path, q) } + +// decorate sets the routing header with Set and never Add. The edge rejects a +// duplicated header after it has already selected a router, so a second value +// is not a warning, it is a failure that surfaces somewhere else entirely. +func (r *hostedRoute) decorate(h http.Header) { + h.Set(installationHeader, r.installation) + sent := r.credential.Reveal() + h.Set("Authorization", sent) + if !slices.Contains(r.used, sent) { + r.used = append(r.used, sent) + } +} + +// collectionMiss refuses to report a routing failure as an empty list. The +// shared edge answers 404 for an unknown or unrouted installation, and "no +// resources" would hide that behind a plausible answer. +func (r *hostedRoute) collectionMiss(json.RawMessage) (json.RawMessage, error) { + return nil, fmt.Errorf("%w; reporting this as an empty result would hide it", r.unrouted()) +} + +// unrouted reports that the shared endpoint may not have reached this +// installation at all. +// +// The edge answers 404 for an installation it cannot route — one that has been +// suspended, destroyed, or reaped when a trial or subscription ended — and that +// is indistinguishable from any other 404 without an edge error envelope, which +// does not exist. It happens on an ordinary day: a session left open for days +// outlives the installation it was working against. +// +// So this never asserts a cause. Callers that could legitimately receive a 404 +// report both possibilities; callers whose endpoint has no such reading report +// this alone. +func (r *hostedRoute) unrouted() error { + return fmt.Errorf( + "the hosted endpoint did not route this request to installation %s, which happens when an "+ + "installation is suspended, destroyed, or no longer covered by a subscription", r.installation) +} + +// refresh re-resolves with the credential refreshed and refuses to let the +// target move. +// +// The comparison is the point. Without it the refresh path silently +// re-introduces the skew that resolving configuration and credentials together +// exists to prevent, and a retry could read from a different installation than +// the one the call set out to address. +// +// The profile name is deliberately not compared: the target is the endpoint and +// the installation, and a pointer that moved while still resolving to the same +// installation has not moved the target. +func (r *hostedRoute) refresh(ctx context.Context) (bool, error) { + if r.refreshFn == nil { + return false, nil + } + next, err := r.refreshFn(ctx) + if err != nil { + return false, err + } + hosted, ok := next.Conn.(config.Hosted) + if !ok { + return false, errConnectionMoved + } + if hosted.Endpoint != r.endpoint || hosted.Installation != r.installation { + return false, errConnectionMoved + } + if next.Credential.IsZero() { + return false, errors.New("formae refreshed the connection but returned no credential") + } + r.credential = next.Credential + return true, nil +} + +// scrub removes any credential this route has used from bytes the far end +// sent back. +// +// The far end is trusted to route, not to be careful. An error page from an +// intermediary that echoes request headers would otherwise put the bearer +// token into a tool result and from there into a model's context and a +// transcript — a copy of the credential somewhere it can never be withdrawn +// from. Both the current and the previous credential are scrubbed, because a +// retry's response can still be quoting the request that failed. +func (r *hostedRoute) scrub(body []byte) []byte { + for _, used := range r.used { + if used == "" { + continue + } + body = bytes.ReplaceAll(body, []byte(used), []byte(secret.Mask)) + } + return body +} + +var errConnectionMoved = errors.New( + "the connection changed while this request was in flight, so it was abandoned rather than " + + "retried against a different installation") + +func joinURL(base, path string, q url.Values) string { + u := base + path + if len(q) > 0 { + u += "?" + q.Encode() + } + return u +} + +// refuseRedirects is the hosted redirect policy: hand the 3xx back rather than +// follow it, so do can report it as a routing error. +// +// Every redirect is refused, not only a cross-origin one. The agent API has no +// redirect worth following, and refusing outright avoids getting origin +// equivalence subtly right for no benefit. Returning ErrUseLastResponse rather +// than an error keeps the status available and leaves nothing wrapped in a +// *url.Error for a caller to unwrap incorrectly. +func refuseRedirects(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse +} diff --git a/internal/server/routing_test.go b/internal/server/routing_test.go new file mode 100644 index 0000000..2639608 --- /dev/null +++ b/internal/server/routing_test.go @@ -0,0 +1,328 @@ +package server + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +const testInstallation = "3HzFPXfPDGhwLJJVtaHbmFs6vLa" + +// hostedCtx is a resolved hosted context pointing at srv rather than the real +// edge, so the routing behaviour can be exercised against httptest. +func hostedCtx(credential string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: config.HostedOrigin, + Installation: testInstallation, + }, + Credential: secret.New(credential), + FormaeBin: "/usr/bin/formae", + } +} + +// newTestHostedClient builds a hosted client whose requests land on srv. The +// endpoint the routing validated is the real origin; only the transport is +// redirected, so the header and credential behaviour under test is the same +// behaviour that would reach the edge. +func newTestHostedClient(t *testing.T, srv *httptest.Server, credential string, refresh refresher) *FormaeClient { + t.Helper() + return newTestHostedClientAt(srv, credential, refresh) +} + +// newTestHostedClientAt builds the same client without a *testing.T, for the +// places that need one inside a seam closure. +func newTestHostedClientAt(srv *httptest.Server, credential string, refresh refresher) *FormaeClient { + return &FormaeClient{ + route: &hostedRoute{ + endpoint: srv.URL, + installation: testInstallation, + credential: secret.New(credential), + refreshFn: refresh, + }, + httpClient: &http.Client{ + Transport: srv.Client().Transport, + CheckRedirect: refuseRedirects, + }, + } +} + +func TestHostedRequestsCarryTheInstallationExactlyOnce(t *testing.T) { + var values []string + var auth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + values = r.Header.Values("Formae-Installation") + auth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + // The request names both headers itself. This is the case that separates + // Set from Add: on a fresh header the two are indistinguishable, and a + // second routing value is not a warning at the edge but a rejection that + // happens after a router has already been chosen, so it surfaces somewhere + // else entirely. It also pins that the routing decorates last and wins, + // rather than a caller being able to redirect a credentialled request. + if _, _, err := c.do(context.Background(), request{ + Method: "GET", + Path: "/api/v1/health", + Headers: map[string]string{ + "Formae-Installation": "2ZaBcDeFgHiJkLmNoPqRsTuVwXy", + "Authorization": "Bearer somebody-elses", + }, + }, noRetry); err != nil { + t.Fatalf("do: %v", err) + } + + if len(values) != 1 { + t.Fatalf("Formae-Installation must be set exactly once, got %d values: %v", len(values), values) + } + if values[0] != testInstallation { + t.Errorf("Formae-Installation = %q, want the routing's own", values[0]) + } + if auth != "Bearer live-token" { + t.Errorf("Authorization = %q, want the routing's own", auth) + } +} + +func TestClassicRequestsCarryNeitherHeader(t *testing.T) { + var installation, auth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + installation = r.Header.Get("Formae-Installation") + auth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + if _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry); err != nil { + t.Fatalf("do: %v", err) + } + + if installation != "" { + t.Errorf("a classic request must carry no routing header, got %q", installation) + } + if auth != "" { + t.Errorf("a classic request must carry no credential, got %q", auth) + } +} + +// Go strips Authorization across a cross-host redirect but forwards custom +// headers, so the routing header would follow one. Every 3xx is refused, and +// the target must never be contacted. +func TestHostedRefusesEveryRedirect(t *testing.T) { + for _, code := range []int{ + http.StatusMovedPermanently, http.StatusFound, http.StatusSeeOther, + http.StatusTemporaryRedirect, http.StatusPermanentRedirect, + } { + t.Run(fmt.Sprint(code), func(t *testing.T) { + var followed bool + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + followed = true + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+"/api/v1/health", code) + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + + if err == nil { + t.Fatal("a redirect on a hosted request must be an error") + } + if !strings.Contains(err.Error(), "routing") { + t.Errorf("the error should name routing as the cause: %v", err) + } + if followed { + t.Fatal("the redirect target was contacted") + } + }) + } +} + +// A classic connection keeps Go's default redirect handling: nothing about its +// behaviour changes in this slice. +func TestClassicStillFollowsRedirects(t *testing.T) { + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer target.Close() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+"/api/v1/health", http.StatusFound) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + body, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusOK || !strings.Contains(string(body), "ok") { + t.Fatalf("classic redirect handling changed: status %d body %s", status, body) + } +} + +// The MCP re-validates what the CLI resolved, because it exposes tools that +// write profiles: a model fed hostile input can author one. +func TestClientConstructionRevalidatesTheHostedConnection(t *testing.T) { + cases := map[string]config.Hosted{ + "a foreign endpoint": {Endpoint: "https://evil.example", Installation: testInstallation}, + "a malformed installation": {Endpoint: config.HostedOrigin, Installation: "not-an-installation"}, + "the retired uuid form": {Endpoint: config.HostedOrigin, Installation: "3f2b8c14-0000-4000-8000-000000000000"}, + "an http endpoint": {Endpoint: "http://cloud.formae.ai", Installation: testInstallation}, + "an endpoint with a path": {Endpoint: "https://cloud.formae.ai/api", Installation: testInstallation}, + } + for name, conn := range cases { + t.Run(name, func(t *testing.T) { + ec := execctx.Context{ProfileName: "prod", Conn: conn, Credential: secret.New("Bearer x")} + + if _, err := newClientFromCtx(ec, nil); err == nil { + t.Fatalf("%s must be refused before it can reach a header", name) + } + }) + } +} + +// A hosted connection with no credential cannot authenticate, so it is not a +// usable connection. The decoder refuses the shape; this refuses it again at +// the point where a request would otherwise go out unauthenticated. +func TestHostedClientRequiresACredential(t *testing.T) { + ec := hostedCtx("") + + if _, err := newClientFromCtx(ec, nil); err == nil { + t.Fatal("a hosted connection with no credential must be refused") + } +} + +// A credential must not reach a rendering of the client or its routing. +func TestClientDoesNotRenderTheCredential(t *testing.T) { + c, err := newClientFromCtx(hostedCtx("Bearer sup3rs3cr3t"), nil) + if err != nil { + t.Fatalf("newClientFromCtx: %v", err) + } + + for _, rendered := range []string{ + fmt.Sprintf("%v", c.route), + fmt.Sprintf("%+v", c.route), + fmt.Sprintf("%#v", c.route), + } { + if strings.Contains(rendered, "sup3rs3cr3t") { + t.Fatalf("a rendering of the routing leaked the credential: %s", rendered) + } + } + + out, err := json.Marshal(c.route) + if err != nil { + t.Fatalf("marshalling the routing: %v", err) + } + if strings.Contains(string(out), "sup3rs3cr3t") { + t.Fatalf("a JSON rendering of the routing leaked the credential: %s", out) + } +} + +// The far end is trusted to route, not to be careful. An error page from an +// intermediary that echoed the request headers would otherwise put the bearer +// token into a tool result, and from there into a model's context and a +// transcript, where it can never be withdrawn from. +func TestAResponseEchoingTheCredentialIsScrubbed(t *testing.T) { + for _, status := range []int{http.StatusOK, http.StatusBadGateway} { + t.Run(fmt.Sprint(status), func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(status) + _, _ = fmt.Fprintf(w, `{"error":"upstream rejected %s"}`, r.Header.Get("Authorization")) + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer sup3rs3cr3t", nil) + body, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + if err != nil { + t.Fatalf("do: %v", err) + } + if strings.Contains(string(body), "sup3rs3cr3t") { + t.Fatalf("the response body carried the credential back out: %s", body) + } + if !strings.Contains(string(body), secret.Mask) { + t.Errorf("the scrub should leave the mask behind: %s", body) + } + }) + } +} + +// A retry's response can still be quoting the request that failed, so the +// credential the first attempt used has to be scrubbed too. +func TestAScrubCoversTheCredentialARetryReplaced(t *testing.T) { + var attempts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts == 1 { + w.WriteHeader(http.StatusUnauthorized) + return + } + _, _ = fmt.Fprint(w, `{"note":"the earlier token Bearer stale-secret was rejected"}`) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer fresh-secret")} + c := newTestHostedClient(t, srv, "Bearer stale-secret", rr.refresh) + + body, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if strings.Contains(string(body), "stale-secret") { + t.Fatalf("the superseded credential survived the scrub: %s", body) + } +} + +// An unbounded read from a peer is an unbounded allocation, and under hosted +// that peer is remote and shared rather than a process on this machine. +func TestAnOversizedResponseIsRefused(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + chunk := make([]byte, 1<<20) + for i := 0; i <= maxResponseBytes>>20; i++ { + if _, err := w.Write(chunk); err != nil { + return + } + } + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + + if !errors.Is(err, errResponseTooLarge) { + t.Fatalf("want errResponseTooLarge, got %v", err) + } +} + +// Bytes that never left cannot have taken effect. A connection refused before +// the request is written must not tell an operator to go and check. +func TestATransportFailureBeforeTheRequestIsWrittenClaimsNothingWasSent(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + srv.Close() // nothing is listening now + + _, _, err := c.do(context.Background(), request{Method: "POST", Path: "/api/v1/commands"}, noRetry) + if err == nil { + t.Fatal("expected a transport failure") + } + if c.reach != reachResolved { + t.Fatalf("reach = %v, want resolved: nothing was written, so nothing can have acted", c.reach) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index cded0cd..f335d68 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -18,6 +18,7 @@ import ( "github.com/platform-engineering-labs/formae-mcp/internal/featuregate" "github.com/platform-engineering-labs/formae-mcp/internal/formaebin" "github.com/platform-engineering-labs/formae-mcp/internal/profile" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" "github.com/platform-engineering-labs/formae-mcp/internal/tools" "github.com/platform-engineering-labs/formae-mcp/internal/version" ) @@ -37,7 +38,10 @@ func implementation() *mcp.Implementation { // contextResolver is the seam server tests substitute. The concrete resolver // lives in execctx and its injection points are unexported there. type contextResolver interface { - Resolve(ctx context.Context, profileName string) (execctx.Context, error) + // Resolve produces the frozen context for a call. forceRefresh is for the + // 401 path, which re-resolves with a fresh credential and then checks that + // the target did not move. + Resolve(ctx context.Context, profileName string, forceRefresh bool) (execctx.Context, error) Bin() string // Managed reports whether the resolved formae is the copy we provisioned, // which decides whether an upgrade needs sudo. @@ -51,6 +55,11 @@ type Server struct { forcedEndpoint string // when set, empty-profile calls use this (tests / explicit) ctxResolver contextResolver // resolves per-call execution context clientID *clientid.Resolver // resolves the Client-ID header value + // newClient builds the agent client for a resolved context. It is a field + // for the same reason ctxResolver is: the hosted arm validates its endpoint + // against a compile-time origin, so without a seam no test can exercise a + // hosted handler at all. Production never replaces it. + newClient func(execctx.Context) (*FormaeClient, error) } // New creates a new formae MCP server connected to the given agent endpoint. @@ -69,6 +78,7 @@ func New(endpoint string) *Server { ctxResolver: execctx.NewResolver(formaebin.NewBinResolver()), clientID: clientid.NewResolver(), } + s.newClient = s.clientFrom s.registerTools() s.registerResources() @@ -83,7 +93,29 @@ func (s *Server) clientFor(ctx context.Context, profileName string) (*FormaeClie if err != nil { return nil, err } - return newClientFromCtx(ec) + return s.newClient(ec) +} + +// clientFrom builds the client for an already-resolved context. +// +// This is the one place that pairs a context with its refresher, so a 401 on +// any call re-resolves through the same seam the first resolution used rather +// than reaching past it. Handlers that resolve their own context call this +// instead of constructing a client directly. +func (s *Server) clientFrom(ec execctx.Context) (*FormaeClient, error) { + return newClientFromCtx(ec, s.refresherFor(ec)) +} + +// refresherFor re-resolves the same profile with the credential refreshed. +// +// It closes over the effective profile name from the original snapshot, not +// over whatever the active pointer says at refresh time: re-resolving "whatever +// is active now" is exactly the skew that resolving configuration and +// credentials together exists to prevent. +func (s *Server) refresherFor(ec execctx.Context) refresher { + return func(ctx context.Context) (execctx.Context, error) { + return s.ctxResolver.Resolve(ctx, ec.ProfileName, true) + } } // resolveCtx returns the immutable execution context for an optional profile. @@ -109,7 +141,7 @@ func (s *Server) resolveCtx(ctx context.Context, profileName string) (execctx.Co FormaeBin: s.ctxResolver.Bin(), }, nil } - ec, err := s.ctxResolver.Resolve(ctx, profileName) + ec, err := s.ctxResolver.Resolve(ctx, profileName, false) if err != nil { return execctx.Context{}, s.explainIfTooOld(err) } @@ -329,39 +361,51 @@ func (s *Server) registerTools() { // Tool handlers — read-only func (s *Server) handleListResources(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListResourcesInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListResources(ctx, input.Query) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListStacks(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListStacks(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListTargets(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListTargetsInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListTargets(ctx, input.Query) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleGetCommandStatus(ctx context.Context, _ *mcp.CallToolRequest, input tools.GetCommandStatusInput) (*mcp.CallToolResult, any, error) { @@ -372,15 +416,15 @@ func (s *Server) handleGetCommandStatus(ctx context.Context, _ *mcp.CallToolRequ if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.GetCommandStatus(ctx, input.CommandID, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListCommands(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListCommandsInput) (*mcp.CallToolResult, any, error) { @@ -392,27 +436,31 @@ func (s *Server) handleListCommands(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.ListCommands(ctx, input.Query, maxResults, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleGetAgentStats(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.GetAgentStats(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleCheckHealth(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { @@ -420,18 +468,18 @@ func (s *Server) handleCheckHealth(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } if err := c.CheckHealth(ctx); err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } msg := "Formae agent is healthy and reachable." if notice := s.buildSkewNotice(ctx, ec.FormaeBin, c); notice != "" { msg += "\n\n" + notice } - return textResult(msg), nil, nil + return attribute(reached(ec, c), textResult(msg)), nil, nil } // buildSkewNotice fetches the agent version and the local formae version and @@ -457,42 +505,50 @@ func (s *Server) buildSkewNotice(ctx context.Context, formaeBin string, c *Forma } func (s *Server) handleListPolicies(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListPolicies(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListChangesSinceLastReconcileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } if input.Stack != "" { result, err := c.ListChangesSinceLastReconcile(ctx, input.Stack) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } // No stack specified: fetch all stacks, then get drift for each stacksJSON, err := c.ListStacks(ctx) if err != nil { - return errorResult(fmt.Errorf("failed to list stacks: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to list stacks: %w", err))), nil, nil } var stacks []struct { Label string `json:"Label"` } if err := json.Unmarshal(stacksJSON, &stacks); err != nil { - return errorResult(fmt.Errorf("failed to parse stacks: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to parse stacks: %w", err))), nil, nil } type stackDrift struct { @@ -504,7 +560,7 @@ func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp for _, stack := range stacks { driftJSON, err := c.ListChangesSinceLastReconcile(ctx, stack.Label) if err != nil { - return errorResult(fmt.Errorf("failed to get drift for stack %s: %w", stack.Label, err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to get drift for stack %s: %w", stack.Label, err))), nil, nil } // Parse to check if there are modifications @@ -512,7 +568,7 @@ func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp ModifiedResources json.RawMessage `json:"ModifiedResources"` } if err := json.Unmarshal(driftJSON, &drift); err != nil { - return errorResult(fmt.Errorf("failed to parse drift for stack %s: %w", stack.Label, err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to parse drift for stack %s: %w", stack.Label, err))), nil, nil } results = append(results, stackDrift{ @@ -523,9 +579,9 @@ func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp aggregated, err := json.Marshal(results) if err != nil { - return errorResult(fmt.Errorf("failed to marshal results: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to marshal results: %w", err))), nil, nil } - return jsonResult(aggregated), nil, nil + return attribute(reached(ec, c), jsonResult(aggregated)), nil, nil } func (s *Server) handleExtractResources(ctx context.Context, _ *mcp.CallToolRequest, input tools.ExtractResourcesInput) (*mcp.CallToolResult, any, error) { @@ -551,21 +607,30 @@ func (s *Server) handleExtractResources(ctx context.Context, _ *mcp.CallToolRequ args = append(args, "--profile", ec.ProfileName) } args = append(args, outFile) + // Extract reaches the agent through the CLI, so do never runs and cannot + // advance reach. It is taken from the subprocess outcome instead, and that + // is sound here for a reason rather than by exemption: extract is a read. + // It creates, changes and destroys nothing, so "might this have acted?" + // has one answer however it fails, and neither wording can send an operator + // looking for work that cannot exist. Do not copy this to a mutation. cmd := commandWithContext(ctx, ec.FormaeBin, args...) if output, err := cmd.CombinedOutput(); err != nil { - return errorResult(fmt.Errorf("formae extract failed: %w\noutput: %s", err, string(output))), nil, nil + return attribute(resolved(ec), + errorResult(fmt.Errorf("formae extract failed: %w\noutput: %s", + err, safeSubprocessOutput(output, ec.Credential)))), nil, nil } + extracted := destination{ec: ec, reach: reachAnswered} content, err := os.ReadFile(outFile) if err != nil { - return errorResult(fmt.Errorf("failed to read extracted file: %w", err)), nil, nil + return attribute(extracted, errorResult(fmt.Errorf("failed to read extracted file: %w", err))), nil, nil } notice := "" - if c, cerr := newClientFromCtx(ec); cerr == nil { + if c, cerr := s.newClient(ec); cerr == nil { notice = s.buildSkewNotice(ctx, ec.FormaeBin, c) } - return withNotice(textResult(string(content)), notice), nil, nil + return attribute(extracted, withNotice(textResult(string(content)), notice)), nil, nil } func (s *Server) handleSearchHubPlugins(_ context.Context, _ *mcp.CallToolRequest, input tools.SearchHubPluginsInput) (*mcp.CallToolResult, any, error) { @@ -646,18 +711,18 @@ func (s *Server) handleApplyForma(ctx context.Context, _ *mcp.CallToolRequest, i } formaJSON, err := evalFormaFile(ctx, ec, input.FilePath) if err != nil { - return errorResult(fmt.Errorf("failed to evaluate forma file: %w", err)), nil, nil + return attribute(resolved(ec), errorResult(fmt.Errorf("failed to evaluate forma file: %w", err))), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.SubmitCommand(ctx, "apply", input.Mode, input.Simulate, input.Force, formaJSON, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return withNotice(jsonResult(result), s.buildSkewNotice(ctx, ec.FormaeBin, c)), nil, nil + return attribute(reached(ec, c), withNotice(jsonResult(result), s.buildSkewNotice(ctx, ec.FormaeBin, c))), nil, nil } func (s *Server) handleDestroyForma(ctx context.Context, _ *mcp.CallToolRequest, input tools.DestroyFormaInput) (*mcp.CallToolResult, any, error) { @@ -671,29 +736,29 @@ func (s *Server) handleDestroyForma(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } if input.Query != "" { result, err := c.DestroyByQuery(ctx, input.Query, input.Simulate, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } formaJSON, err := evalFormaFile(ctx, ec, input.FilePath) if err != nil { - return errorResult(fmt.Errorf("failed to evaluate forma file: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to evaluate forma file: %w", err))), nil, nil } result, err := c.SubmitCommand(ctx, "destroy", "", input.Simulate, false, formaJSON, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleCancelCommands(ctx context.Context, _ *mcp.CallToolRequest, input tools.CancelCommandsInput) (*mcp.CallToolResult, any, error) { @@ -701,67 +766,112 @@ func (s *Server) handleCancelCommands(ctx context.Context, _ *mcp.CallToolReques if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.CancelCommands(ctx, input.Query, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleForceSync(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } if err := c.ForceSync(ctx); err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return textResult("Resource synchronization triggered successfully."), nil, nil + return attribute(reached(ec, c), textResult("Resource synchronization triggered successfully.")), nil, nil } func (s *Server) handleForceDiscover(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } if err := c.ForceDiscover(ctx); err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return textResult("Resource discovery triggered successfully."), nil, nil + return attribute(reached(ec, c), textResult("Resource discovery triggered successfully.")), nil, nil } func (s *Server) handleForceCheckTTL(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ForceCheckTTL(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleForceReconcileStack(ctx context.Context, _ *mcp.CallToolRequest, input tools.ForceReconcileStackInput) (*mcp.CallToolResult, any, error) { if input.Stack == "" { return errorResult(fmt.Errorf("stack is required")), nil, nil } - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } body, _, err := c.ForceReconcileStack(ctx, input.Stack) if err != nil { if body != nil { - return errorResult(fmt.Errorf("%s: %s", err.Error(), string(body))), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("%s: %s", err.Error(), string(body)))), nil, nil } - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(body), nil, nil + return attribute(reached(ec, c), jsonResult(body)), nil, nil +} + +// maxSubprocessOutput bounds the diagnostics a failed CLI invocation may put +// into a tool result. Unlike the configuration oracle, which reports an exit +// status and never the bytes, extract's output is the user's own Pkl and +// plugin diagnostics and is the whole value of the failure, so it is kept — +// bounded, and with the credential we handed the process removed. +const maxSubprocessOutput = 8 << 10 + +// safeSubprocessOutput bounds subprocess output and removes the credential +// this call resolved. +// +// It covers the credential we gave the process. A credential the CLI minted +// for itself is not ours to know, and masking that is the CLI's own job on its +// own output — said plainly here because "output is sanitised" would be a +// wider claim than this makes good. +func safeSubprocessOutput(output []byte, credential secret.Value) string { + // Scrub first, then truncate. The other order leaves a credential that + // straddles the cutoff partly intact: the search string is no longer + // present in the truncated bytes, so nothing is replaced and all but the + // tail of the token survives. + scrubbed := string(output) + if !credential.IsZero() { + scrubbed = strings.ReplaceAll(scrubbed, credential.Reveal(), secret.Mask) + } + if len(scrubbed) > maxSubprocessOutput { + scrubbed = scrubbed[:maxSubprocessOutput] + "\n… truncated" + } + return scrubbed } // Helpers diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 12ab851..f5e86aa 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -794,10 +794,24 @@ type stubResolver struct { err error managed bool sawProfile string + // calls and refreshes count what the 401 path did, so a test can assert + // that a refresh happened, or that one did not. + calls int + refreshes int + // refreshed, when set, is what a forced refresh resolves to. Nil means a + // refresh returns the same context as the first resolution. + refreshed *execctx.Context } -func (r *stubResolver) Resolve(_ context.Context, profileName string) (execctx.Context, error) { +func (r *stubResolver) Resolve(_ context.Context, profileName string, forceRefresh bool) (execctx.Context, error) { r.sawProfile = profileName + r.calls++ + if forceRefresh { + r.refreshes++ + if r.refreshed != nil { + return *r.refreshed, nil + } + } return r.ec, r.err } @@ -822,8 +836,8 @@ func TestClientFor_ExplicitProfileReachesTheResolver(t *testing.T) { if r.sawProfile != "p" { t.Errorf("resolver saw profile %q, want %q", r.sawProfile, "p") } - if c.endpoint != "http://p-host:7000" { - t.Errorf("endpoint = %q, want the profile endpoint", c.endpoint) + if got := c.route.url("", nil); got != "http://p-host:7000" { + t.Errorf("endpoint = %q, want the profile endpoint", got) } } @@ -835,7 +849,7 @@ func TestClientFor_RefusesHosted(t *testing.T) { ProfileName: "acme-prod", Conn: config.Hosted{ Endpoint: config.HostedOrigin, - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", }, }} s := New("") @@ -861,8 +875,8 @@ func TestClientFor_ClassicBuildsURLPort(t *testing.T) { if err != nil { t.Fatal(err) } - if c.endpoint != "http://localhost:49684" { - t.Errorf("endpoint = %q", c.endpoint) + if got := c.route.url("", nil); got != "http://localhost:49684" { + t.Errorf("endpoint = %q", got) } } @@ -886,8 +900,8 @@ func TestClientFor_ForcedEndpointIsClassicWithoutAPort(t *testing.T) { if err != nil { t.Fatal(err) } - if c.endpoint != "http://forced:1" { - t.Errorf("endpoint = %q, want the forced endpoint", c.endpoint) + if got := c.route.url("", nil); got != "http://forced:1" { + t.Errorf("endpoint = %q, want the forced endpoint", got) } } diff --git a/internal/server/unauthorized_test.go b/internal/server/unauthorized_test.go new file mode 100644 index 0000000..baedbcb --- /dev/null +++ b/internal/server/unauthorized_test.go @@ -0,0 +1,335 @@ +package server + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +// recordingRefresher stands in for the resolver seam, counting what the 401 +// path asked for. Counting the calls is the point: an implementation that +// reaches past the seam, or that refreshes when it should not, is invisible to +// an assertion that only looks at the error. +type recordingRefresher struct { + calls int + next execctx.Context + err error +} + +func (r *recordingRefresher) refresh(context.Context) (execctx.Context, error) { + r.calls++ + return r.next, r.err +} + +// hostedAt is what a refresh resolves to: the same installation at the same +// endpoint the route already holds. The tests aim the transport at httptest, so +// the refreshed context has to name that endpoint too — otherwise every refresh +// would look like the target moving, and the binding check would pass these +// tests for the wrong reason. +func hostedAt(endpoint, credential string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: endpoint, + Installation: testInstallation, + }, + Credential: secret.New(credential), + FormaeBin: "/usr/bin/formae", + } +} + +// unauthorizedThenOK answers 401 until the request carries wantAuth, then 200. +// It records every Authorization it saw, so a retry can be checked to have used +// the refreshed credential rather than merely to have happened. +type unauthorizedThenOK struct { + wantAuth string + seen []string +} + +func (h *unauthorizedThenOK) ServeHTTP(w http.ResponseWriter, r *http.Request) { + auth := r.Header.Get("Authorization") + h.seen = append(h.seen, auth) + if auth != h.wantAuth { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"ok":true}`)) +} + +func TestRetryableRequestRefreshesAndRetriesWithTheNewCredential(t *testing.T) { + handler := &unauthorizedThenOK{wantAuth: "Bearer refreshed"} + srv := httptest.NewServer(handler) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer refreshed")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusOK { + t.Fatalf("status = %d, want the retry to succeed", status) + } + if rr.calls != 1 { + t.Errorf("resolver calls = %d, want exactly one refresh", rr.calls) + } + if len(handler.seen) != 2 { + t.Fatalf("requests = %d, want the original and one retry", len(handler.seen)) + } + if handler.seen[0] != "Bearer stale" || handler.seen[1] != "Bearer refreshed" { + t.Errorf("the retry did not carry the refreshed credential: %v", handler.seen) + } +} + +// A second 401 is reported as unauthorized without invention. It has no plugin +// error behind it and can mean expiry, wrong audience, wrong issuer or a +// malformed credential, so nothing here can say which. +func TestASecondUnauthorizedIsNotRefreshedAgain(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer also-rejected")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusUnauthorized { + t.Errorf("status = %d, want the unauthorized to be reported", status) + } + if rr.calls != 1 { + t.Errorf("resolver calls = %d, want exactly one refresh", rr.calls) + } + if requests != 2 { + t.Errorf("requests = %d, want exactly one retry", requests) + } +} + +// A mutation refreshes so the next call succeeds, and returns the original +// failure. Nothing establishes that a 401 always precedes dispatch, and +// duplicating an infrastructure mutation to save one error is the wrong trade. +func TestAMutationRefreshesButNeverRetries(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer refreshed")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "POST", Path: "/api/v1/commands"}, noRetry) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusUnauthorized { + t.Errorf("status = %d, want the original failure", status) + } + if rr.calls != 1 { + t.Errorf("resolver calls = %d, want the refresh to still happen", rr.calls) + } + if requests != 1 { + t.Errorf("requests = %d, want no second attempt", requests) + } +} + +// A denied installation or tenant is an authorization failure, not an expired +// session. Asserting the resolver count rather than the error text is what +// makes this a real check: an implementation that refreshed and then reported +// the 403 anyway would pass a text assertion. +func TestForbiddenNeverReauthenticates(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer refreshed")} + c := newTestHostedClient(t, srv, "Bearer live", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusForbidden { + t.Errorf("status = %d", status) + } + if rr.calls != 0 { + t.Fatalf("a 403 triggered %d resolver calls; it must trigger none", rr.calls) + } +} + +// A refresh that comes back pointing somewhere else is abandoned rather than +// retried. Without this the refresh path silently re-introduces the skew that +// resolving configuration and credentials together exists to prevent. +func TestARefreshThatMovesTheTargetAborts(t *testing.T) { + // Each case is built against the endpoint the route actually holds, so it + // fails the check it is named for. Hard-coding the real origin here would + // make every case fail the endpoint comparison first, and the credential + // case in particular would pass without ever exercising its own check. + moved := map[string]func(endpoint string) execctx.Context{ + "a different installation": func(endpoint string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: endpoint, + Installation: "2ZaBcDeFgHiJkLmNoPqRsTuVwXy", + }, + Credential: secret.New("Bearer refreshed"), + } + }, + "a different origin": func(string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: "https://other.formae.ai", + Installation: testInstallation, + }, + Credential: secret.New("Bearer refreshed"), + } + }, + "a mode that flipped to classic": func(string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Classic{URL: "http://localhost", Port: 49684}, + } + }, + "a refresh that returned no credential": func(endpoint string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: endpoint, + Installation: testInstallation, + }, + } + }, + } + for name, build := range moved { + t.Run(name, func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: build(srv.URL)} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if err == nil { + t.Fatal("a refresh that moved the target must abort") + } + if requests != 1 { + t.Errorf("requests = %d, want no attempt against the moved target", requests) + } + }) + } +} + +// The connection-moved abort names what happened, because "unauthorized" would +// send the reader looking at credentials rather than at their profile. +func TestTheConnectionMovedErrorSaysSo(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: config.HostedOrigin, + Installation: "2ZaBcDeFgHiJkLmNoPqRsTuVwXy", + }, + Credential: secret.New("Bearer refreshed"), + }} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if !errors.Is(err, errConnectionMoved) { + t.Fatalf("want errConnectionMoved, got %v", err) + } +} + +// A refresh that fails outright surfaces its own error rather than the 401. +func TestARefreshFailureSurfaces(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{err: errors.New("the auth plugin is not installed")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if err == nil || !strings.Contains(err.Error(), "auth plugin") { + t.Fatalf("want the refresh failure, got %v", err) + } +} + +// The MCP sends a self-hosted agent no credential, so a 401 from one is +// somebody else's problem and there is nothing a retry would change. +func TestAClassicUnauthorizedIsReturnedAsIs(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusUnauthorized { + t.Errorf("status = %d", status) + } + if requests != 1 { + t.Errorf("requests = %d, want no retry", requests) + } +} + +// A retryable request carrying a body would replay an already-consumed reader +// and silently send an empty one, which looks like a successful request rather +// than a corrupted one. Refused outright instead. +func TestARetryableRequestMayNotCarryABody(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + _, _, err := c.do(context.Background(), request{ + Method: "POST", + Path: "/api/v1/commands", + Body: strings.NewReader("payload"), + }, retryOnce) + + if !errors.Is(err, errRetryableBody) { + t.Fatalf("want errRetryableBody, got %v", err) + } + if requests != 0 { + t.Errorf("the request must be refused before it is sent, got %d requests", requests) + } +}