From 6658e28f0e8e6314fe77361ce0fd247d813fbfb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 31 May 2026 03:05:44 +0200 Subject: [PATCH 1/6] fix(cloud): cap harness output during the run instead of buffering unbounded execCLI buffered the full stdout/stderr in memory and truncated only after the process returned, so a command emitting a very large response could consume unbounded memory despite defaultOutputLimit. Capture stdout/stderr through a bounded limitedWriter that retains at most limit bytes each and records overflow, so the cap is effective during the run. Every existing guarantee is preserved: no shell, explicit minimal env, closed stdin, Truncated set on overflow, stderr captured and capped, non-zero exit as a normal CLIResult, real start/exec failure as a Go error. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/harness.go | 68 +++++++++++++++++++++++------------ pkg/mcp/cloud/harness_test.go | 16 +++++++++ 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/pkg/mcp/cloud/harness.go b/pkg/mcp/cloud/harness.go index 8d11dc06..8c2b11d3 100644 --- a/pkg/mcp/cloud/harness.go +++ b/pkg/mcp/cloud/harness.go @@ -1,7 +1,6 @@ package cloud import ( - "bytes" "context" "errors" "os/exec" @@ -11,40 +10,63 @@ import ( // the agent's context budget. Output beyond it is dropped and flagged. const defaultOutputLimit = 64 * 1024 +// limitedWriter retains at most limit bytes of everything written to it and +// records whether any write pushed it past that cap. It never grows past limit, +// so a command emitting an arbitrarily large response cannot consume unbounded +// memory: bytes past the cap are counted for the overflow flag and discarded. +type limitedWriter struct { + buf []byte + limit int + overflow bool +} + +// Write retains up to the remaining capacity in the buffer and discards the +// rest, flagging overflow whenever a write carries more bytes than the buffer +// can still hold. It always reports the full length written so the child +// process is never blocked on a short write. +func (w *limitedWriter) Write(p []byte) (int, error) { + room := w.limit - len(w.buf) + if len(p) > room { + w.overflow = true + } + if room > 0 { + take := len(p) + if take > room { + take = room + } + w.buf = append(w.buf, p[:take]...) + } + return len(p), nil +} + // execCLI runs binPath with argv via execve — no shell, ever. The argv tokens // reach the binary as literal arguments, so shell metacharacters are inert. The // subprocess runs with exactly the supplied env (never the parent environment, // so a poisoned PATH cannot redirect the binary and ambient secrets do not -// leak), closed stdin (no interactive prompt), and stdout capped at limit. A -// non-zero exit is a normal result carried in ExitCode, not a Go error; a Go -// error means the process could not be run at all. Stderr — where gcloud/aws -// write their error context — is captured alongside stdout and capped at the -// same limit, so a non-zero exit carries an explanation instead of an empty -// result. +// leak), closed stdin (no interactive prompt), and stdout/stderr captured +// through bounded writers that retain at most limit bytes each — the cap is +// effective during the run, so a command emitting a very large response can +// never buffer it all in memory. A non-zero exit is a normal result carried in +// ExitCode, not a Go error; a Go error means the process could not be run at +// all. Stderr — where gcloud/aws write their error context — is captured +// alongside stdout and capped at the same limit, so a non-zero exit carries an +// explanation instead of an empty result. func execCLI(ctx context.Context, binPath string, argv []string, env []string, limit int) (CLIResult, error) { cmd := exec.CommandContext(ctx, binPath, argv...) cmd.Env = env cmd.Stdin = nil - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr + stdout := &limitedWriter{limit: limit} + stderr := &limitedWriter{limit: limit} + cmd.Stdout = stdout + cmd.Stderr = stderr err := cmd.Run() - res := CLIResult{} - out := stdout.Bytes() - if len(out) > limit { - out = out[:limit] - res.Truncated = true - } - res.Stdout = string(out) - - errOut := stderr.Bytes() - if len(errOut) > limit { - errOut = errOut[:limit] - res.Truncated = true + res := CLIResult{ + Stdout: string(stdout.buf), + Stderr: string(stderr.buf), + Truncated: stdout.overflow || stderr.overflow, } - res.Stderr = string(errOut) if err != nil { var exitErr *exec.ExitError diff --git a/pkg/mcp/cloud/harness_test.go b/pkg/mcp/cloud/harness_test.go index 0d4861f1..79a0f5b5 100644 --- a/pkg/mcp/cloud/harness_test.go +++ b/pkg/mcp/cloud/harness_test.go @@ -41,3 +41,19 @@ func TestExecCLITruncatesStderr(t *testing.T) { require.NoError(t, err) assert.LessOrEqual(t, len(r.Stderr), 10, "stderr exceeded limit") } + +// TestExecCLICapsLargeOutputWithoutBuffering drives a payload orders of +// magnitude past the limit through a shell-free command (head reading 8MB from +// /dev/zero) and asserts the captured stdout is capped at the limit with +// Truncated set, so a command emitting a very large response cannot retain +// unbounded bytes in memory. The cap is effective during the run, not a +// post-hoc slice of a fully buffered output. +func TestExecCLICapsLargeOutputWithoutBuffering(t *testing.T) { + t.Parallel() + const limit = 1024 + r, err := execCLI(context.Background(), "/usr/bin/head", + []string{"-c", "8388608", "/dev/zero"}, nil, limit) + require.NoError(t, err) + assert.True(t, r.Truncated, "an output far larger than limit must be flagged truncated") + assert.LessOrEqual(t, len(r.Stdout), limit, "captured stdout must be capped at limit, not the full 8MB payload") +} From b603f12d813b6d1f801c52a7e0d03a194edfb4c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 31 May 2026 03:06:29 +0200 Subject: [PATCH 2/6] fix(cloud): report the pinned identity on a degraded probe When the provider failed to resolve an identity, Probe returned Valid:false with an empty AssumedIdentity even though the caller passed the pinned identity in expected, so session_status no longer named which pinned identity was degraded. Fall back to expected whenever the resulting status has an empty AssumedIdentity, on both the degraded and valid paths, so the displayed identity is always the pinned one. Degrade-never-error semantics are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/probe.go | 12 ++++++++---- pkg/mcp/cloud/probe_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/pkg/mcp/cloud/probe.go b/pkg/mcp/cloud/probe.go index 6a90fd39..38f65a85 100644 --- a/pkg/mcp/cloud/probe.go +++ b/pkg/mcp/cloud/probe.go @@ -30,9 +30,10 @@ func Probe(ctx context.Context, p Provider, expected string, env []string) (Iden st, err := p.Identity(ctx, run, expected) if err != nil { return IdentityStatus{ - Provider: p.Name(), - Valid: false, - Hint: err.Error(), + Provider: p.Name(), + AssumedIdentity: expected, + Valid: false, + Hint: err.Error(), }, nil } if st.Provider == "" { @@ -40,8 +41,11 @@ func Probe(ctx context.Context, p Provider, expected string, env []string) (Iden } if st.AssumedIdentity == "" { // A whoami that resolved no identity is not a valid session, whatever - // the provider reported. + // the provider reported. Report the pinned identity so the degraded + // session names which credential the operator must fix instead of an + // empty one. st.Valid = false + st.AssumedIdentity = expected } return st, nil } diff --git a/pkg/mcp/cloud/probe_test.go b/pkg/mcp/cloud/probe_test.go index a2cf8aea..4925233b 100644 --- a/pkg/mcp/cloud/probe_test.go +++ b/pkg/mcp/cloud/probe_test.go @@ -111,3 +111,31 @@ func TestProbeInvalidWhenIdentityEmpty(t *testing.T) { require.NoError(t, err) assert.False(t, st.Valid, "an empty resolved identity must not be reported valid") } + +// TestProbeDegradedReportsPinnedIdentity proves a degraded probe still names +// WHICH pinned identity is degraded: when the provider errors and resolves no +// identity, Probe falls back to the expected identity the caller pinned, so +// session_status stays actionable instead of showing an empty identity. +func TestProbeDegradedReportsPinnedIdentity(t *testing.T) { + t.Parallel() + const pinned = "ro-sa@proj.iam.gserviceaccount.com" + p := &fakeProvider{name: "gcp", identityErr: errors.New("token expired")} + st, err := Probe(context.Background(), p, pinned, nil) + require.NoError(t, err, "Probe should degrade, not error") + assert.False(t, st.Valid) + assert.Equal(t, pinned, st.AssumedIdentity, + "a degraded probe must report the pinned identity so the operator knows what to fix") +} + +// TestProbeFallsBackToExpectedWhenProviderOmitsIdentity covers the valid path: +// a provider that resolves to valid but reports no identity (an unusual but +// possible projection gap) still shows the pinned identity rather than empty. +func TestProbeFallsBackToExpectedWhenProviderOmitsIdentity(t *testing.T) { + t.Parallel() + const pinned = "arn:aws:iam::111122223333:role/triage-ro" + p := &fakeProvider{name: "aws", identity: IdentityStatus{Provider: "aws", Valid: true}} + st, err := Probe(context.Background(), p, pinned, nil) + require.NoError(t, err) + assert.Equal(t, pinned, st.AssumedIdentity, + "an empty resolved identity must fall back to the pinned identity") +} From f04a62431ad50e401eff68cd1d626a926ec241b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 31 May 2026 03:07:14 +0200 Subject: [PATCH 3/6] fix(cloud): keep the pinned identity when provider construction fails A provider construction failure (e.g. a missing gcloud/aws binary) returned IdentityStatus{Provider, Valid:false, Hint} with no AssumedIdentity, so preflight and connections reported the degraded source without the identity the operator must fix. Carry src.AssumedIdentity through the construction-error status, mirroring the probe-path fallback so both ProbeSource exits name the pinned identity. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/providers/probe.go | 7 ++++++- pkg/mcp/cloud/providers/probe_test.go | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/pkg/mcp/cloud/providers/probe.go b/pkg/mcp/cloud/providers/probe.go index 8106b69d..2273a520 100644 --- a/pkg/mcp/cloud/providers/probe.go +++ b/pkg/mcp/cloud/providers/probe.go @@ -45,7 +45,12 @@ type Source struct { func ProbeSource(ctx context.Context, src Source) cloud.IdentityStatus { p, err := New(src.Provider) if err != nil { - return cloud.IdentityStatus{Provider: src.Provider, Valid: false, Hint: err.Error()} + return cloud.IdentityStatus{ + Provider: src.Provider, + AssumedIdentity: src.AssumedIdentity, + Valid: false, + Hint: err.Error(), + } } return probeProvider(ctx, p, src.AssumedIdentity, sourceEnvFor(p, src)) } diff --git a/pkg/mcp/cloud/providers/probe_test.go b/pkg/mcp/cloud/providers/probe_test.go index 4c484ac1..8d54a8b8 100644 --- a/pkg/mcp/cloud/providers/probe_test.go +++ b/pkg/mcp/cloud/providers/probe_test.go @@ -84,6 +84,21 @@ func TestProbeSourceUnknownProviderDegrades(t *testing.T) { assert.NotEmpty(t, st.Hint) } +// TestProbeSourceConstructionFailureKeepsPinnedIdentity proves a provider +// construction failure (here an unknown provider, which never reaches New's CLI +// lookup but exercises the same construction-error path) still reports the +// pinned identity, so preflight and connections name the degraded source's +// identity the operator must fix instead of an empty one. +func TestProbeSourceConstructionFailureKeepsPinnedIdentity(t *testing.T) { + const pinned = "arn:aws:iam::111122223333:role/triage-ro" + st := ProbeSource(context.Background(), Source{Provider: "azure", AssumedIdentity: pinned}) + assert.False(t, st.Valid) + assert.Equal(t, "azure", st.Provider) + assert.Equal(t, pinned, st.AssumedIdentity, + "a construction failure must still carry the pinned identity") + assert.NotEmpty(t, st.Hint) +} + // fakePassthroughProvider exposes a fixed EnvPassthrough so sourceEnv's // carry-and-overlay behaviour can be asserted without a real cloud CLI. type fakePassthroughProvider struct{ passthrough []string } From dfc3a106fec62d04925896f6f18cd967b79521db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 31 May 2026 03:09:16 +0200 Subject: [PATCH 4/6] fix(cloud): resolve the provider CLI to an absolute path The harness relies on a fixed absolute binary path so a later subprocess env/PATH change cannot redirect what executes, but exec.LookPath returns a relative path (flagged with exec.ErrDot) when PATH carries relative entries. Pass the LookPath result through filepath.Abs in each provider's New(), recovering the relative path on ErrDot and erroring if it still cannot be made absolute. Applied identically to gcp and aws. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/providers/aws/provider.go | 15 +++++++++--- pkg/mcp/cloud/providers/aws/provider_test.go | 25 ++++++++++++++++++++ pkg/mcp/cloud/providers/gcp/provider.go | 15 +++++++++--- pkg/mcp/cloud/providers/gcp/provider_test.go | 25 ++++++++++++++++++++ 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/pkg/mcp/cloud/providers/aws/provider.go b/pkg/mcp/cloud/providers/aws/provider.go index d954adff..85da7afc 100644 --- a/pkg/mcp/cloud/providers/aws/provider.go +++ b/pkg/mcp/cloud/providers/aws/provider.go @@ -13,8 +13,10 @@ package aws import ( _ "embed" "encoding/json" + "errors" "fmt" "os/exec" + "path/filepath" "github.com/sourcehawk/triagent/pkg/mcp/cloud" ) @@ -50,13 +52,20 @@ type Provider struct { } // New constructs the AWS provider, resolving aws to an absolute path once via -// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. +// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. A +// PATH with relative entries makes LookPath return a relative path (flagged with +// exec.ErrDot); the path is made absolute so a later subprocess env/PATH change +// cannot reinterpret it against a different working directory. func New() (*Provider, error) { bin, err := exec.LookPath("aws") - if err != nil { + if err != nil && !errors.Is(err, exec.ErrDot) { return nil, fmt.Errorf("aws: resolve aws binary: %w", err) } - return newWithBinary(bin) + abs, err := filepath.Abs(bin) + if err != nil { + return nil, fmt.Errorf("aws: resolve aws binary to absolute path: %w", err) + } + return newWithBinary(abs) } // newWithBinary builds the provider against an already-resolved binary path. It diff --git a/pkg/mcp/cloud/providers/aws/provider_test.go b/pkg/mcp/cloud/providers/aws/provider_test.go index 0df882a5..b3efd361 100644 --- a/pkg/mcp/cloud/providers/aws/provider_test.go +++ b/pkg/mcp/cloud/providers/aws/provider_test.go @@ -23,6 +23,31 @@ func TestNewResolvesProvider(t *testing.T) { assert.Equal(t, "/usr/bin/aws", p.Binary()) } +// TestNewResolvesBinaryToAbsolutePath proves New stores an absolute binary path +// even when PATH resolution would yield a relative one, so a later subprocess +// env/PATH change cannot redirect what executes. The CLI is dropped into a temp +// dir reachable through a relative PATH entry; the resolved binary must come +// back absolute. +func TestNewResolvesBinaryToAbsolutePath(t *testing.T) { + dir := t.TempDir() + bin := filepath.Join(dir, "aws") + require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o755)) + + cwd, err := os.Getwd() + require.NoError(t, err) + t.Cleanup(func() { _ = os.Chdir(cwd) }) + require.NoError(t, os.Chdir(dir)) + + // "." is a relative PATH entry; exec.LookPath("aws") resolves to "aws" + // (relative) under it. + t.Setenv("PATH", ".") + + p, err := New() + require.NoError(t, err) + assert.True(t, filepath.IsAbs(p.Binary()), + "New must store an absolute binary path, got %q", p.Binary()) +} + func TestDefaultAllowlistCoversReadOnlyAxes(t *testing.T) { p, err := newWithBinary("/usr/bin/aws") require.NoError(t, err) diff --git a/pkg/mcp/cloud/providers/gcp/provider.go b/pkg/mcp/cloud/providers/gcp/provider.go index 8f13cfdf..3b819ead 100644 --- a/pkg/mcp/cloud/providers/gcp/provider.go +++ b/pkg/mcp/cloud/providers/gcp/provider.go @@ -8,8 +8,10 @@ package gcp import ( _ "embed" "encoding/json" + "errors" "fmt" "os/exec" + "path/filepath" "github.com/sourcehawk/triagent/pkg/mcp/cloud" ) @@ -36,13 +38,20 @@ type Provider struct { } // New constructs the gcp provider, resolving gcloud to an absolute path once via -// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. +// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. A +// PATH with relative entries makes LookPath return a relative path (flagged with +// exec.ErrDot); the path is made absolute so a later subprocess env/PATH change +// cannot reinterpret it against a different working directory. func New() (*Provider, error) { bin, err := exec.LookPath("gcloud") - if err != nil { + if err != nil && !errors.Is(err, exec.ErrDot) { return nil, fmt.Errorf("gcp: resolve gcloud binary: %w", err) } - return newWithBinary(bin) + abs, err := filepath.Abs(bin) + if err != nil { + return nil, fmt.Errorf("gcp: resolve gcloud binary to absolute path: %w", err) + } + return newWithBinary(abs) } // newWithBinary builds the provider against an already-resolved binary path. It diff --git a/pkg/mcp/cloud/providers/gcp/provider_test.go b/pkg/mcp/cloud/providers/gcp/provider_test.go index 5c1f5445..2544384b 100644 --- a/pkg/mcp/cloud/providers/gcp/provider_test.go +++ b/pkg/mcp/cloud/providers/gcp/provider_test.go @@ -20,6 +20,31 @@ func TestNewResolvesBinaryAndName(t *testing.T) { assert.Equal(t, "/usr/bin/gcloud", p.Binary()) } +// TestNewResolvesBinaryToAbsolutePath proves New stores an absolute binary path +// even when PATH resolution would yield a relative one, so a later subprocess +// env/PATH change cannot redirect what executes. The provider's CLI is dropped +// into a temp dir reachable through a relative PATH entry; the resolved binary +// must come back absolute. +func TestNewResolvesBinaryToAbsolutePath(t *testing.T) { + dir := t.TempDir() + bin := filepath.Join(dir, "gcloud") + require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o755)) + + cwd, err := os.Getwd() + require.NoError(t, err) + t.Cleanup(func() { _ = os.Chdir(cwd) }) + require.NoError(t, os.Chdir(dir)) + + // "." is a relative PATH entry; exec.LookPath("gcloud") resolves to "gcloud" + // (relative) under it. + t.Setenv("PATH", ".") + + p, err := New() + require.NoError(t, err) + assert.True(t, filepath.IsAbs(p.Binary()), + "New must store an absolute binary path, got %q", p.Binary()) +} + func TestDefaultAllowlistLoadsEmbeddedJSON(t *testing.T) { t.Parallel() p, err := newWithBinary("/usr/bin/gcloud") From 4442c512213b711f91a6ec3e242cda782ae64017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 31 May 2026 03:09:34 +0200 Subject: [PATCH 5/6] docs(cloud): correct CLIResult doc to say raw truncated output The comment claimed output was shaped/redacted, but run_cli returns the provider CLI's raw stdout/stderr, only truncated. State that CLIResult carries the raw CLI stdout (and stderr), capped at the output limit with Truncated set when exceeded, so callers do not assume shaping or redaction beyond truncation. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/provider.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/mcp/cloud/provider.go b/pkg/mcp/cloud/provider.go index 111ebad1..32d1bee1 100644 --- a/pkg/mcp/cloud/provider.go +++ b/pkg/mcp/cloud/provider.go @@ -69,8 +69,10 @@ type IdentityStatus struct { Hint string `json:"hint,omitempty"` } -// CLIResult is the shaped result of one run_cli invocation. Raw provider JSON -// is never surfaced; the harness caps output and reports truncation. +// CLIResult is the result of one run_cli invocation. It carries the provider +// CLI's raw stdout (and stderr), each capped at the output limit with Truncated +// set when the output exceeded it. The bytes are not otherwise shaped or +// redacted; callers must not assume any projection beyond truncation. type CLIResult struct { Stdout string `json:"stdout"` Stderr string `json:"stderr,omitempty"` From 03eff540f5fd4c369f3a47752be6819a844ea94b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 31 May 2026 03:10:03 +0200 Subject: [PATCH 6/6] refactor(cloud): drop the unused Command.Redact field Command.Redact was advertised in the allowlist schema and documented as marking output for secret-scrubbing, but nothing read it before returning run_cli output, so it promised protection that did not exist. No shipped default_commands.json sets it. Remove the field and its doc; run_cli is the gated escape hatch returning raw (truncated) output by design, and typed tools are where projection lives. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/allowlist.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/mcp/cloud/allowlist.go b/pkg/mcp/cloud/allowlist.go index f9f025c0..ac5444c8 100644 --- a/pkg/mcp/cloud/allowlist.go +++ b/pkg/mcp/cloud/allowlist.go @@ -19,11 +19,10 @@ var defaultCommandsJSON []byte // Command is one entry in the command allowlist. Path is the normalized // subcommand path the allowlist matches against (for example "projects list" or // "compute firewall-rules list"). Description carries the investigative axis the -// command serves (prose only). Redact marks output that needs secret-scrubbing. +// command serves (prose only). type Command struct { Path string `json:"path"` Description string `json:"description,omitempty"` - Redact bool `json:"redact,omitempty"` } // CommandAllowlist is the decoded allowlist document: the positive set of