diff --git a/internal/security/mask_test.go b/internal/security/mask_test.go index 40b22c88d..7c1683ae3 100644 --- a/internal/security/mask_test.go +++ b/internal/security/mask_test.go @@ -247,30 +247,78 @@ func TestMaskTextMasksBeyondTheDetectionCap(t *testing.T) { } } -// Removing the replacement cap makes "a payload stuffed with secrets" the -// worst case, so it must not be quadratic: a full-size activity response -// (64KB, the activity_max_response_size cap) of nothing but distinct tokens -// still has to mask in well under a second. -func TestMaskTextStaysCheapOnAPayloadFullOfSecrets(t *testing.T) { - d := NewDetector(nil) - +// secretsPayload builds text of at least targetLen bytes made entirely of +// distinct GitHub-PAT-shaped tokens ("key=ghp_"), +// the worst case for masking: every token is a separate match, none share a +// value, so no dedup/caching in the detector can shortcut the work. +func secretsPayload(targetLen int) string { var b strings.Builder - for i := 0; b.Len() < 64*1024; i++ { + for i := 0; b.Len() < targetLen; i++ { fmt.Fprintf(&b, "key%d=ghp_%036d\n", i, i) } - text := b.String() + return b.String() +} + +// A full-size activity response made entirely of distinct tokens exercises the +// worst-case masking shape without making correctness depend on host timing. +func TestMaskTextHandlesAPayloadFullOfSecrets(t *testing.T) { + d := NewDetector(nil) + + text := secretsPayload(64 * 1024) - start := time.Now() masked, _ := d.MaskText(text) - elapsed := time.Since(start) if strings.Contains(masked, "ghp_0000") { t.Fatal("tokens survived masking") } - if elapsed > 2*time.Second { - t.Fatalf("masking a %d-byte payload took %s", len(text), elapsed) +} + +// TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets guards against +// reintroducing the O(n^2) behavior TestMaskTextStaysCheapOnAPayloadFullOfSecrets +// used to catch (removed because an absolute wall-clock ceiling was flaky under +// CI load — see BenchmarkMaskTextPayloadFullOfSecrets below for the profiling +// counterpart). Instead of a fixed threshold, it compares masking a payload +// against one 4x the size: linear work predicts ~4x the time, so a generous +// growth ceiling still catches real quadratic blowups (~16x) while absorbing +// routine scheduler/GC noise from the comparison running in the same process. +func TestMaskTextScalesLinearlyOnAPayloadFullOfSecrets(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive; skipped under -short") + } + + measure := func(targetLen int) time.Duration { + d := NewDetector(nil) + text := secretsPayload(targetLen) + start := time.Now() + d.MaskText(text) + return time.Since(start) + } + + // Warm up (first run pays for allocator/CPU-cache warmup, not algorithmic cost). + measure(16 * 1024) + + small := measure(16 * 1024) + large := measure(64 * 1024) // 4x the payload + + // The absolute floor keeps this from firing on noise when both runs are + // too fast for the ratio to mean anything. + if large > 10*small && large > 50*time.Millisecond { + t.Fatalf("masking a payload 4x the size took %s vs %s for the baseline — looks like a complexity regression, not scheduler noise", large, small) + } +} + +func BenchmarkMaskTextPayloadFullOfSecrets(b *testing.B) { + d := NewDetector(nil) + text := secretsPayload(64 * 1024) + + b.SetBytes(int64(len(text))) + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if _, changed := d.MaskText(text); !changed { + b.Fatal("payload was not masked") + } } - t.Logf("masked %d bytes in %s", len(text), elapsed) } // Turning detection off later must not retroactively serve the credentials in diff --git a/internal/upstream/manager.go b/internal/upstream/manager.go index 7cc5f1147..ba1341da1 100644 --- a/internal/upstream/manager.go +++ b/internal/upstream/manager.go @@ -46,6 +46,16 @@ var dockerResolverFn = func(logger *zap.Logger) (string, error) { return shellwrap.ResolveDockerPath(logger) } +func runDockerInfo(ctx context.Context, dockerBin string) error { + return exec.CommandContext(ctx, dockerBin, "info", "--format", "{{json .ServerVersion}}").Run() +} + +// dockerInfoRunnerFn runs the resolved docker binary's `info` command. +// Overridable in tests, mirroring dockerResolverFn above so the two sibling +// fakes checkDockerAvailability needs (which binary, and running it) share one +// injection idiom instead of two. +var dockerInfoRunnerFn = runDockerInfo + // Docker recovery constants - internal implementation defaults const ( dockerCheckInterval = 30 * time.Second // How often to check Docker availability @@ -3035,8 +3045,7 @@ func (m *Manager) checkDockerAvailability(ctx context.Context) error { dockerBin = "docker" } - cmd := exec.CommandContext(checkCtx, dockerBin, "info", "--format", "{{json .ServerVersion}}") - if err := cmd.Run(); err != nil { + if err := dockerInfoRunnerFn(checkCtx, dockerBin); err != nil { return fmt.Errorf("docker unavailable: %w", err) } return nil diff --git a/internal/upstream/manager_docker_recovery_test.go b/internal/upstream/manager_docker_recovery_test.go index a3fc35473..f744922b5 100644 --- a/internal/upstream/manager_docker_recovery_test.go +++ b/internal/upstream/manager_docker_recovery_test.go @@ -15,40 +15,75 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) +// TestRunDockerInfo_ExecutesResolvedBinary exercises runDockerInfo's real +// exec.CommandContext(...).Run() invocation shape end-to-end against a real +// subprocess, so a regression in the argument/flag shape (wrong dockerBin +// position, broken --format string) is caught even though the tests below +// inject dockerInfoRunnerFn and never call this function. +func TestRunDockerInfo_ExecutesResolvedBinary(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("/bin/sh-style fake docker is POSIX only") + } + + dockerPath := filepath.Join(t.TempDir(), "docker") + // Requires the exact argv runDockerInfo passes (not just $1 == info), so a + // regression in argument count, the --format flag, or the Go template + // string fails this test instead of slipping through unnoticed. + script := `#!/bin/sh +if [ "$#" -eq 3 ] && [ "$1" = "info" ] && [ "$2" = "--format" ] && [ "$3" = '{{json .ServerVersion}}' ]; then + printf '"24.0.0"\n' + exit 0 +fi +exit 99 +` + if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + if err := runDockerInfo(ctx, dockerPath); err != nil { + t.Fatalf("expected fake docker info to succeed, got: %v", err) + } +} + +// TestRunDockerInfo_PropagatesNonZeroExit ensures a real non-zero process exit +// (not just a synthetic Go error) surfaces as an error from runDockerInfo. +func TestRunDockerInfo_PropagatesNonZeroExit(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("/bin/sh-style fake docker is POSIX only") + } + + dockerPath := filepath.Join(t.TempDir(), "docker") + if err := os.WriteFile(dockerPath, []byte("#!/bin/sh\nexit 7\n"), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + if err := runDockerInfo(ctx, dockerPath); err == nil { + t.Fatal("expected non-zero exit to surface as an error") + } +} + // TestCheckDockerAvailability_UsesShellwrapResolver verifies that a launchd-style // minimal PATH (the situation when mcpproxy is launched from /Applications/...app // or a LoginItem) does not break docker lookup, because checkDockerAvailability // now goes through dockerResolverFn (shellwrap-backed) instead of relying on // $PATH for the bare "docker" binary. // -// We exercise the real resolver indirectly: the test installs a fake docker -// shim into a temp dir, points dockerResolverFn at it, drops $PATH to the -// launchd minimum, and asserts that checkDockerAvailability still succeeds. -// Without the fix, exec.Command("docker") would error with `executable file -// not found in $PATH` regardless of how the resolver was wired. +// dockerInfoRunnerFn is faked so the test asserts the resolved executable +// directly without depending on process startup or a wall-clock deadline; the +// real exec.CommandContext(...).Run() shape is covered separately by +// TestRunDockerInfo_ExecutesResolvedBinary above. func TestCheckDockerAvailability_UsesShellwrapResolver(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("launchd PATH scenario is macOS/Linux only") } - tmpDir := t.TempDir() - dockerPath := filepath.Join(tmpDir, "docker") - - // Fake docker that prints a server version when invoked with `info`. - // Anything else exits non-zero so we know the call shape was correct. - script := `#!/bin/sh -case "$1" in - info) printf '"24.0.0"\n'; exit 0 ;; - *) exit 99 ;; -esac -` - if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { - t.Fatalf("write fake docker: %v", err) - } - - // Simulate launchd's minimal PATH — fake docker is NOT on it. If the - // production code resolved via os.Getenv("PATH") we would fail here. - t.Setenv("PATH", "/usr/bin:/bin:/usr/sbin:/sbin") + dockerPath := filepath.Join(t.TempDir(), "docker") original := dockerResolverFn t.Cleanup(func() { dockerResolverFn = original }) @@ -56,17 +91,26 @@ esac return dockerPath, nil } + var resolvedBin string + originalRunner := dockerInfoRunnerFn + t.Cleanup(func() { dockerInfoRunnerFn = originalRunner }) + dockerInfoRunnerFn = func(_ context.Context, dockerBin string) error { + resolvedBin = dockerBin + return nil + } + m := &Manager{logger: zap.NewNop()} - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - if err := m.checkDockerAvailability(ctx); err != nil { + if err := m.checkDockerAvailability(context.Background()); err != nil { t.Fatalf("expected docker to be reachable via resolver, got: %v", err) } + if resolvedBin != dockerPath { + t.Fatalf("docker executable = %q, want resolved path %q", resolvedBin, dockerPath) + } } // TestCheckDockerAvailability_FallbackOnResolverFailure ensures that even when -// the resolver itself errors out we still attempt a bare-name exec — preserving +// the resolver itself errors out we still select the bare executable name — preserving // the original behaviour for hosts where the shellwrap probes legitimately // cannot find docker but it IS on the parent's PATH. func TestCheckDockerAvailability_FallbackOnResolverFailure(t *testing.T) { @@ -74,31 +118,28 @@ func TestCheckDockerAvailability_FallbackOnResolverFailure(t *testing.T) { t.Skip("/bin/sh-style fake docker is POSIX only") } - tmpDir := t.TempDir() - dockerPath := filepath.Join(tmpDir, "docker") - script := `#!/bin/sh -exit 0 -` - if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { - t.Fatalf("write fake docker: %v", err) - } - - // Resolver fails — the fallback should still exec via bare PATH. - t.Setenv("PATH", tmpDir) - original := dockerResolverFn t.Cleanup(func() { dockerResolverFn = original }) dockerResolverFn = func(*zap.Logger) (string, error) { return "", errors.New("simulated resolver failure") } + var resolvedBin string + originalRunner := dockerInfoRunnerFn + t.Cleanup(func() { dockerInfoRunnerFn = originalRunner }) + dockerInfoRunnerFn = func(_ context.Context, dockerBin string) error { + resolvedBin = dockerBin + return nil + } + m := &Manager{logger: zap.NewNop()} - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - if err := m.checkDockerAvailability(ctx); err != nil { + if err := m.checkDockerAvailability(context.Background()); err != nil { t.Fatalf("expected fallback bare-name exec to succeed, got: %v", err) } + if resolvedBin != "docker" { + t.Fatalf("docker executable = %q, want bare-name fallback", resolvedBin) + } } // TestFreshenLoadedDockerRecoveryState verifies that a state loaded from the