diff --git a/internal/detector/configaudit/bunfig.go b/internal/detector/configaudit/bunfig.go index 79b54c2..83287ca 100644 --- a/internal/detector/configaudit/bunfig.go +++ b/internal/detector/configaudit/bunfig.go @@ -213,6 +213,14 @@ func (d *BunDetector) collectFile(ctx context.Context, path, scope string) model if target, err := os.Readlink(path); err == nil { f.SymlinkTo = target } + // See npmrc.go's collectFile: project-scope files are found by + // walking arbitrary (possibly attacker-controlled) directories, so a + // symlink there is not followed. + if scope == "project" { + f.Readable = false + f.ParseError = "refused: project-scoped symlink not followed" + return f + } } info, err := os.Stat(path) diff --git a/internal/detector/configaudit/bunfig_test.go b/internal/detector/configaudit/bunfig_test.go index a2cbe75..f13838f 100644 --- a/internal/detector/configaudit/bunfig_test.go +++ b/internal/detector/configaudit/bunfig_test.go @@ -2,6 +2,7 @@ package configaudit import ( "context" + "os" "os/user" "path/filepath" "strings" @@ -249,3 +250,44 @@ registry = "https://registry.npmjs.org/" } } } + +// TestBunDetector_ProjectSymlinkNotFollowed: see npmrc_test.go's identical +// test — bunfig.toml discovery shares the same collectFile pattern and the +// same project-scope symlink-following bug and fix. +func TestBunDetector_ProjectSymlinkNotFollowed(t *testing.T) { + tmp := t.TempDir() + + secretPath := filepath.Join(tmp, "outside", "secret-credential") + mustWriteFile(t, secretPath, "EXAMPLE_SENSITIVE_DATA\n") + + projectPath := filepath.Join(tmp, "repo", "bunfig.toml") + if err := os.MkdirAll(filepath.Dir(projectPath), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.Symlink(secretPath, projectPath); err != nil { + t.Fatalf("symlink: %v", err) + } + + d := NewBunDetector(executor.NewMock()) + d.ownerLookup = fixedOwner() + d.gitTracked = func(_ context.Context, _ string) bool { return false } + d.inGitRepo = func(_ string) bool { return false } + + f := d.collectFile(context.Background(), projectPath, "project") + + if !f.Exists { + t.Fatalf("expected the symlink itself to be recorded as existing") + } + if f.SymlinkTo != secretPath { + t.Errorf("SymlinkTo = %q, want %q", f.SymlinkTo, secretPath) + } + if f.Readable { + t.Errorf("project-scoped symlink should not be marked readable") + } + if len(f.Sections) != 0 { + t.Errorf("target file contents must not be parsed, got sections: %+v", f.Sections) + } + if f.SHA256 != "" { + t.Errorf("target file must not be hashed, got %q", f.SHA256) + } +} diff --git a/internal/detector/configaudit/npmrc.go b/internal/detector/configaudit/npmrc.go index d1298e5..342073c 100644 --- a/internal/detector/configaudit/npmrc.go +++ b/internal/detector/configaudit/npmrc.go @@ -245,6 +245,18 @@ func (d *NPMRCDetector) collectFile(ctx context.Context, path, scope string) mod if target, err := os.Readlink(path); err == nil { f.SymlinkTo = target } + // Project-scope files are discovered by walking arbitrary directories, + // including cloned git repos. A malicious repo can commit a .npmrc + // symlink pointing anywhere on disk; following it would read and + // upload whatever it points to. Record the link but refuse to read + // through it. user/global/builtin paths are resolved by the detector + // itself (npm's own config resolution, or the user's home), so a + // symlink there is the operator's own dotfile setup, not attacker input. + if scope == "project" { + f.Readable = false + f.ParseError = "refused: project-scoped symlink not followed" + return f + } } // Stat (follows symlinks) for size/mtime/mode. diff --git a/internal/detector/configaudit/npmrc_test.go b/internal/detector/configaudit/npmrc_test.go index 1145644..78f4f9f 100644 --- a/internal/detector/configaudit/npmrc_test.go +++ b/internal/detector/configaudit/npmrc_test.go @@ -344,6 +344,71 @@ func TestNPMRCDetector_RespectsEnvOverridesForUserAndGlobal(t *testing.T) { } } +// TestNPMRCDetector_ProjectSymlinkNotFollowed: a malicious repo can commit a +// .npmrc that is a symlink to a predictable file elsewhere on the developer's +// machine (e.g. a credential file). Project-scope discovery walks arbitrary, +// possibly attacker-controlled directories, so that symlink must not be +// followed — the link is recorded as metadata but its target is never read, +// hashed, or parsed into telemetry. +func TestNPMRCDetector_ProjectSymlinkNotFollowed(t *testing.T) { + tmp := t.TempDir() + + secretPath := filepath.Join(tmp, "outside", "secret-credential") + mustWriteFile(t, secretPath, "EXAMPLE_SENSITIVE_DATA\n") + + projectPath := filepath.Join(tmp, "repo", ".npmrc") + if err := os.MkdirAll(filepath.Dir(projectPath), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.Symlink(secretPath, projectPath); err != nil { + t.Fatalf("symlink: %v", err) + } + + d := NewNPMRCDetector(executor.NewMock()) + d.ownerLookup = fixedOwner() + d.gitTracked = func(_ context.Context, _ string) bool { return false } + d.inGitRepo = func(_ string) bool { return false } + + f := d.collectFile(context.Background(), projectPath, "project") + + if !f.Exists { + t.Fatalf("expected the symlink itself to be recorded as existing") + } + if f.SymlinkTo != secretPath { + t.Errorf("SymlinkTo = %q, want %q", f.SymlinkTo, secretPath) + } + if f.Readable { + t.Errorf("project-scoped symlink should not be marked readable") + } + if len(f.Entries) != 0 { + t.Errorf("target file contents must not be parsed, got entries: %+v", f.Entries) + } + if f.SHA256 != "" { + t.Errorf("target file must not be hashed, got %q", f.SHA256) + } + + // A user-scope symlink (e.g. dotfiles managed via a symlink) is a setup + // the machine's own user controls, not attacker input — only project-scope + // (untrusted tree walk) discovery refuses to follow the link. + userPath := filepath.Join(tmp, "home", ".npmrc") + realConfig := filepath.Join(tmp, "dotfiles", ".npmrc") + mustWriteFile(t, realConfig, "registry=https://registry.npmjs.org/\n") + if err := os.MkdirAll(filepath.Dir(userPath), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.Symlink(realConfig, userPath); err != nil { + t.Fatalf("symlink: %v", err) + } + + uf := d.collectFile(context.Background(), userPath, "user") + if !uf.Readable { + t.Errorf("user-scope symlink should still be followed, got ParseError=%q", uf.ParseError) + } + if len(uf.Entries) == 0 { + t.Errorf("expected user-scope symlink target to be parsed") + } +} + // mustWriteFile creates parent dirs as needed and writes the content. func mustWriteFile(t *testing.T, path, content string) { t.Helper() diff --git a/internal/detector/configaudit/pnpm.go b/internal/detector/configaudit/pnpm.go index f7b0fd8..5b2ff40 100644 --- a/internal/detector/configaudit/pnpm.go +++ b/internal/detector/configaudit/pnpm.go @@ -177,6 +177,14 @@ func (d *PnpmDetector) collectFile(ctx context.Context, path, scope string) mode if target, err := os.Readlink(path); err == nil { f.SymlinkTo = target } + // See npmrc.go's collectFile: project-scope files are found by + // walking arbitrary (possibly attacker-controlled) directories, so a + // symlink there is not followed. + if scope == "project" { + f.Readable = false + f.ParseError = "refused: project-scoped symlink not followed" + return f + } } info, err := os.Stat(path) diff --git a/internal/detector/configaudit/pnpm_test.go b/internal/detector/configaudit/pnpm_test.go index c9173e8..0c99fd7 100644 --- a/internal/detector/configaudit/pnpm_test.go +++ b/internal/detector/configaudit/pnpm_test.go @@ -2,6 +2,7 @@ package configaudit import ( "context" + "os" "os/user" "path/filepath" "strings" @@ -10,6 +11,47 @@ import ( "github.com/step-security/dev-machine-guard/internal/executor" ) +// TestPnpmDetector_ProjectSymlinkNotFollowed: see npmrc_test.go's identical +// test — pnpm reuses the .npmrc walker/collector, so it shares the same +// project-scope symlink-following bug and fix. +func TestPnpmDetector_ProjectSymlinkNotFollowed(t *testing.T) { + tmp := t.TempDir() + + secretPath := filepath.Join(tmp, "outside", "secret-credential") + mustWriteFile(t, secretPath, "EXAMPLE_SENSITIVE_DATA\n") + + projectPath := filepath.Join(tmp, "repo", ".npmrc") + if err := os.MkdirAll(filepath.Dir(projectPath), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.Symlink(secretPath, projectPath); err != nil { + t.Fatalf("symlink: %v", err) + } + + d := NewPnpmDetector(executor.NewMock()) + d.ownerLookup = fixedOwner() + d.gitTracked = func(_ context.Context, _ string) bool { return false } + d.inGitRepo = func(_ string) bool { return false } + + f := d.collectFile(context.Background(), projectPath, "project") + + if !f.Exists { + t.Fatalf("expected the symlink itself to be recorded as existing") + } + if f.SymlinkTo != secretPath { + t.Errorf("SymlinkTo = %q, want %q", f.SymlinkTo, secretPath) + } + if f.Readable { + t.Errorf("project-scoped symlink should not be marked readable") + } + if len(f.Entries) != 0 { + t.Errorf("target file contents must not be parsed, got entries: %+v", f.Entries) + } + if f.SHA256 != "" { + t.Errorf("target file must not be hashed, got %q", f.SHA256) + } +} + func TestPnpmDetector_Discovery_AllScopes(t *testing.T) { tmp := t.TempDir() diff --git a/internal/detector/configaudit/yarn.go b/internal/detector/configaudit/yarn.go index c9df18a..658b06e 100644 --- a/internal/detector/configaudit/yarn.go +++ b/internal/detector/configaudit/yarn.go @@ -214,6 +214,14 @@ func (d *YarnDetector) collectFile(ctx context.Context, path, scope, flavor stri if target, err := os.Readlink(path); err == nil { f.SymlinkTo = target } + // See npmrc.go's collectFile: project-scope files are found by + // walking arbitrary (possibly attacker-controlled) directories, so a + // symlink there is not followed. + if scope == "project" { + f.Readable = false + f.ParseError = "refused: project-scoped symlink not followed" + return f + } } info, err := os.Stat(path) diff --git a/internal/detector/configaudit/yarn_test.go b/internal/detector/configaudit/yarn_test.go index 6f7a89e..7595732 100644 --- a/internal/detector/configaudit/yarn_test.go +++ b/internal/detector/configaudit/yarn_test.go @@ -2,6 +2,7 @@ package configaudit import ( "context" + "os" "os/user" "path/filepath" "strings" @@ -218,6 +219,47 @@ func TestYarnDetector_MalformedBerryYAML(t *testing.T) { } } +// TestYarnDetector_ProjectSymlinkNotFollowed: see npmrc_test.go's identical +// test — a malicious repo's .yarnrc(.yml) can be a symlink to a predictable +// file elsewhere on disk; project-scope discovery must not follow it. +func TestYarnDetector_ProjectSymlinkNotFollowed(t *testing.T) { + tmp := t.TempDir() + + secretPath := filepath.Join(tmp, "outside", "secret-credential") + mustWriteFile(t, secretPath, "EXAMPLE_SENSITIVE_DATA\n") + + projectPath := filepath.Join(tmp, "repo", ".yarnrc.yml") + if err := os.MkdirAll(filepath.Dir(projectPath), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.Symlink(secretPath, projectPath); err != nil { + t.Fatalf("symlink: %v", err) + } + + d := NewYarnDetector(executor.NewMock()) + d.ownerLookup = fixedOwner() + d.gitTracked = func(_ context.Context, _ string) bool { return false } + d.inGitRepo = func(_ string) bool { return false } + + f := d.collectFile(context.Background(), projectPath, "project", "berry") + + if !f.Exists { + t.Fatalf("expected the symlink itself to be recorded as existing") + } + if f.SymlinkTo != secretPath { + t.Errorf("SymlinkTo = %q, want %q", f.SymlinkTo, secretPath) + } + if f.Readable { + t.Errorf("project-scoped symlink should not be marked readable") + } + if len(f.Entries) != 0 { + t.Errorf("target file contents must not be parsed, got entries: %+v", f.Entries) + } + if f.SHA256 != "" { + t.Errorf("target file must not be hashed, got %q", f.SHA256) + } +} + func TestYarnFlavorFromVersion(t *testing.T) { cases := []struct { in string diff --git a/internal/detector/mcp.go b/internal/detector/mcp.go index b4361f2..833c69c 100644 --- a/internal/detector/mcp.go +++ b/internal/detector/mcp.go @@ -9,6 +9,7 @@ import ( "github.com/tailscale/hujson" + "github.com/step-security/dev-machine-guard/internal/aiagents/redact" "github.com/step-security/dev-machine-guard/internal/executor" "github.com/step-security/dev-machine-guard/internal/model" "github.com/step-security/dev-machine-guard/internal/tcc" @@ -288,7 +289,12 @@ func filterProjectScopedMCPServers(projectsRaw json.RawMessage) map[string]any { return filtered } -// filterServerFields keeps only command, args, serverUrl, url from each server entry. +// filterServerFields keeps only command, args, serverUrl, url from each server +// entry. env and headers are dropped outright since those are the fields +// vendors document for credentials. But command/args/url/serverUrl are +// real-world credential locations too (a bearer token or API key in a query +// string, an --api-key flag baked into args), so the kept values still pass +// through redact.Value before being uploaded. func filterServerFields(serversRaw json.RawMessage) map[string]any { var servers map[string]map[string]any if err := json.Unmarshal(serversRaw, &servers); err != nil { @@ -302,7 +308,7 @@ func filterServerFields(serversRaw json.RawMessage) map[string]any { filtered := make(map[string]any) for k, v := range serverConfig { if allowedKeys[k] { - filtered[k] = v + filtered[k] = redact.Value(v) } } result[name] = filtered diff --git a/internal/detector/mcp_test.go b/internal/detector/mcp_test.go index 679f677..96860db 100644 --- a/internal/detector/mcp_test.go +++ b/internal/detector/mcp_test.go @@ -818,6 +818,32 @@ func TestFilterMCPContent_NonOpenCodeUnchanged(t *testing.T) { } } +// TestFilterServerFields_RedactsSecretsInKeptFields: command/args/url/serverUrl +// are the fields we keep (env/headers are dropped outright), but real MCP +// configs sometimes carry a bearer token or API key inside those kept fields +// too — e.g. a query-string token on the server URL, or an --api-key flag in +// args. Those values must still be redacted before upload, not passed through +// verbatim just because the field name isn't "env" or "headers". +func TestFilterServerFields_RedactsSecretsInKeptFields(t *testing.T) { + det := &MCPDetector{} + content := `{"mcpServers":{"pipeboard":{ + "url":"https://meta-ads.mcp.pipeboard.co/?token=abcdEFGH12345678opaqueTokenValue", + "command":"npx", + "args":["-y","server","--api-key=abcdEFGH12345678opaqueTokenValue"] + }}}` + + filtered, ok := det.filterMCPContent("cursor", "/Users/testuser/.cursor/mcp.json", []byte(content)) + if !ok { + t.Fatalf("expected filtering to succeed") + } + if strings.Contains(string(filtered), "abcdEFGH12345678opaqueTokenValue") { + t.Fatalf("secret leaked into filtered output: %s", filtered) + } + if !strings.Contains(string(filtered), "token=") || !strings.Contains(string(filtered), "REDACTED") { + t.Errorf("expected the url token to be replaced with a redaction placeholder, got: %s", filtered) + } +} + // TestMCPConfigDefinitions_OpenCodeIsPlatformAgnostic: the two OpenCode // definitions leave the Windows and Linux fields empty on purpose, so every // consumer that walks mcpConfigDefinitions — including the known-user-config