diff --git a/README.md b/README.md index 8cc3ea6..33484a5 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,18 @@ profiles: - < {{ prompt:./prompt.md }} ``` +Run a profile from that file by naming the config explicitly, or opting in to +discovery for the current directory tree: + +```bash +clawscan ./my-skill --config .clawscan.yml --profile review +clawscan ./my-skill --profile review --discover-config +``` + +ClawScan never loads a `.clawscan.yml` it was not explicitly pointed at: +config files can define scanners and judges that execute commands, so discovery +is opt-in per run (see [docs/profiles.md](docs/profiles.md)). + ## Judge Harness `--judge` hands scanner evidence to an external agent command so it can inspect diff --git a/cmd/clawscan/main_test.go b/cmd/clawscan/main_test.go index 8dc3cd5..9db0ead 100644 --- a/cmd/clawscan/main_test.go +++ b/cmd/clawscan/main_test.go @@ -678,7 +678,7 @@ func TestRunCommandDiscoversSkillsWithExplicitProfile(t *testing.T) { } func clawHubReceiptJudgeCommand() string { - return `challenge=$(sed -n 's/.*"challenge": "\([^"]*\)".*/\1/p' {{ workspace }}/artifact-inspection.json); required_file=$(sed -n 's/.*"required_file": "\([^"]*\)".*/\1/p' {{ workspace }}/artifact-inspection.json); if command -v sha256sum >/dev/null 2>&1; then required_sha=$(sha256sum {{ workspace }}/"$required_file" | cut -d ' ' -f 1); else required_sha=$(shasum -a 256 {{ workspace }}/"$required_file" | cut -d ' ' -f 1); fi; printf '{"verdict":"benign","artifact_inspection":{"status":"completed","challenge":"%s","required_file_sha256":"%s","files_inspected":["%s"]}}\n' "$challenge" "$required_sha" "$required_file" > {{ output }}` + return `set -- "$(sed -n 's/.*"challenge": "\([^"]*\)".*/\1/p' {{ workspace }}/artifact-inspection.json)" "$(sed -n 's/.*"required_file": "\([^"]*\)".*/\1/p' {{ workspace }}/artifact-inspection.json)"; if command -v sha256sum >/dev/null 2>&1; then set -- "$1" "$2" "$(sha256sum {{ workspace }}/"$2" | cut -d ' ' -f 1)"; else set -- "$1" "$2" "$(shasum -a 256 {{ workspace }}/"$2" | cut -d ' ' -f 1)"; fi; printf '{"verdict":"benign","artifact_inspection":{"status":"completed","challenge":"%s","required_file_sha256":"%s","files_inspected":["%s"]}}\n' "$1" "$3" "$2" > {{ output }}` } func TestRunCommandUsesProjectProfile(t *testing.T) { diff --git a/docs/scanners.md b/docs/scanners.md index 85bbd6b..cbb2f1c 100644 --- a/docs/scanners.md +++ b/docs/scanners.md @@ -45,15 +45,17 @@ that config-backed run and accept these fields: | Field | Required | Meaning | | --- | --- | --- | -| `id` | yes | Scanner ID using letters, digits, `_`, and `-`, starting with a letter or digit. It must not match a built-in scanner ID. | +| `id` | yes | Scanner ID using lowercase letters, digits, `_`, and `-`, starting with a letter or digit, at most 64 characters. It must not match a built-in scanner ID or a reserved Windows device name such as `con`, `nul`, or `com1`. | | `command` | yes | Shell command to execute. Unquoted `{{target}}` is replaced with the safely passed resolved target; do not wrap the placeholder in shell quotes. | | `env` | no | Required environment variable names. Values stay in the process environment and are never stored in the config or artifact. | | `targets` | no | Supported target kinds: `skill`, `plugin`, and/or `url`. Defaults to `skill` and `url`. | | `gate` | no | Exit-code policy with optional `blockOnExitCode` and `warnOnExitCode` rules. | -Each exit-code rule accepts one non-negative integer, a list such as `[1, 2, -3]`, or the string `nonzero`. The block and warning rules may not claim the -same exit code. For example: +Each exit-code rule accepts one integer in `0-124`, a list such as `[1, 2, +3]`, or the string `nonzero`. Higher codes are rejected (`125-127` are +reserved for Docker/shell failures, and `128+N` for signal exits), so ClawScan +treats such exits as failed scans rather than scanner verdicts. The block and +warning rules may not claim the same exit code. For example: ```yaml gate: @@ -69,10 +71,26 @@ Skipped scanners do not fire gate rules. A scanner result with status `failed` also does not fire an exit-code rule; a nonzero command that still returned valid JSON has status `completed` and can fire one. -The command must write JSON to stdout. ClawScan preserves valid stdout as the -scanner's raw evidence; empty or non-JSON stdout produces a failed scanner -result. Required environment variables are checked before any scanner starts. -Artifacts record each requirement as only `present` or `missing`. +The command must write valid UTF-8 JSON to stdout; empty, non-JSON, or +invalid-UTF-8 stdout produces a failed scanner result. JSON objects with +duplicate member names are also rejected rather than silently rewritten. +ClawScan preserves accepted evidence byte-for-byte unless credential redaction +fires: values from declared or visible credential variables, and undeclared +variables with secret-like names, are structurally redacted and the JSON is +re-serialized, so stored bytes can differ from raw stdout. A string value that +hides a secret under nested JSON string escapes is decoded to expose and redact +it, so that value's escape sequences are normalized in the stored evidence; all +other accepted evidence keeps its original values. A credential variable whose +value is itself a JSON object or array is also redacted leaf by leaf, but only +its scalar leaves of at least five characters: shorter leaves (a four-digit PIN, +a two-letter region code) are left in place to avoid over-redacting the many +short, non-secret values that legitimately appear in evidence. Declare short +secrets as their own credential variables rather than as leaves of a structured +value if they must be redacted. Required environment +variables are checked before any scanner starts. Artifacts record each +requirement as only `present` or `missing`. + +Scanner credentials should be supplied through environment variables declared under `env:` (or the sandbox env allowlist). ClawScan can only redact values it was told about via declared env vars; a secret written directly into the command — whether as an inline `NAME=value` assignment or as a flag value such as `--token sk-live` — is outside every redaction scope and can leak into saved evidence if the scanner echoes its arguments. ClawScan does not block either form; keeping all credentials in declared environment variables is the operator's responsibility. User-defined scanners use the same execution path as built-in command-backed scanners. They run in the Docker sandbox by default, and declared `env` names diff --git a/go.mod b/go.mod index 017686f..2167e14 100644 --- a/go.mod +++ b/go.mod @@ -4,5 +4,7 @@ go 1.26.1 require ( github.com/alchemy/json5 v0.2.0 + golang.org/x/sys v0.47.0 gopkg.in/yaml.v3 v3.0.1 + mvdan.cc/sh/v3 v3.13.1 ) diff --git a/go.sum b/go.sum index 3a70621..5e0056c 100644 --- a/go.sum +++ b/go.sum @@ -1,6 +1,20 @@ github.com/alchemy/json5 v0.2.0 h1:M8hmUpCyGlzdiWaxY4RI/rDcLAlFTtjB6j49eUpk+FE= github.com/alchemy/json5 v0.2.0/go.mod h1:kVE7UoCjGVIlxOXFCzKn6T34nyC/OctNTNG81MLOqiw= +github.com/go-quicktest/qt v1.101.0 h1:O1K29Txy5P2OK0dGo59b7b0LR6wKfIhttaAhHUyn7eI= +github.com/go-quicktest/qt v1.101.0/go.mod h1:14Bz/f7NwaXPtdYEgzsx46kqSxVwTbzVZsDC26tQJow= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= +github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= +github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= +github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= +github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +mvdan.cc/sh/v3 v3.13.1 h1:DP3TfgZhDkT7lerUdnp6PTGKyxxzz6T+cOlY/xEvfWk= +mvdan.cc/sh/v3 v3.13.1/go.mod h1:lXJ8SexMvEVcHCoDvAGLZgFJ9Wsm2sulmoNEXGhYZD0= diff --git a/internal/profiles/resolver.go b/internal/profiles/resolver.go index 9cb0d54..2744ba6 100644 --- a/internal/profiles/resolver.go +++ b/internal/profiles/resolver.go @@ -115,13 +115,30 @@ func (gate *ProfileScannerGate) UnmarshalYAML(node *yaml.Node) error { if node.Kind != yaml.MappingNode { return errors.New("scanner gate must be an object") } + // Null rule values decode to a nil pointer without ever reaching + // profileExitCodeRule.UnmarshalYAML, which would silently disable the + // gate. Security gates must fail closed on malformed config. An alias + // (`blockOnExitCode: *anchor`) is resolved before tag inspection so a + // null smuggled through an anchor is caught too. + rules := 0 for index := 0; index < len(node.Content); index += 2 { switch node.Content[index].Value { case "blockOnExitCode", "warnOnExitCode": + value := node.Content[index+1] + for value.Kind == yaml.AliasNode && value.Alias != nil { + value = value.Alias + } + if value.Tag == "!!null" { + return fmt.Errorf("scanner gate %s must be an exit code, a list of exit codes, or \"nonzero\", not null", node.Content[index].Value) + } + rules++ default: return fmt.Errorf("field %s not found in type profiles.ProfileScannerGate", node.Content[index].Value) } } + if rules == 0 { + return errors.New("scanner gate must declare blockOnExitCode or warnOnExitCode") + } type plainGate ProfileScannerGate return node.Decode((*plainGate)(gate)) } @@ -141,6 +158,23 @@ func (scanner *ProfileScanner) UnmarshalYAML(node *yaml.Node) error { return fmt.Errorf("field %s not found in type profiles.ProfileScanner", node.Content[index].Value) } } + for index := 0; index < len(node.Content); index += 2 { + if node.Content[index].Value != "env" { + continue + } + envNode := node.Content[index+1] + nullScalar := envNode.Kind == yaml.ScalarNode && (envNode.Tag == "!!null" || envNode.Value == "") + if envNode.Kind != yaml.SequenceNode && !nullScalar { + return errors.New("scanner env must be a list of variable names") + } + if envNode.Kind == yaml.SequenceNode { + for entryIndex, entry := range envNode.Content { + if entry.Kind != yaml.ScalarNode { + return fmt.Errorf("scanner env entry #%d must be a variable name", entryIndex+1) + } + } + } + } var value struct { ID string `yaml:"id"` Command string `yaml:"command"` @@ -190,6 +224,9 @@ func profileScannerRegistry(scanners []ProfileScanner) (runner.ScannerRegistry, if !scanner.custom { continue } + if bad := runner.InvalidUserDefinedEnvName(scanner.Env); bad != "" { + return runner.ScannerRegistry{}, fmt.Errorf("scanner %s env entry %s is not a variable name; declare bare names and set values in the environment", scanner.ID, bad) + } targets := append([]string(nil), scanner.Targets...) if len(targets) == 0 { targets = []string{"skill", "url"} @@ -206,6 +243,42 @@ func profileScannerRegistry(scanners []ProfileScanner) (runner.ScannerRegistry, return registry, nil } +// declaredEnvNames unions the env var names every profile in the registry +// declares (scanner env plus sandbox passthrough). A single-profile run +// with --sandbox off inherits the full host environment, so a blandly +// named credential declared only by a sibling profile in the same config +// must still be redacted from persisted output. +func (registry ProfileRegistry) declaredEnvNames() []string { + seen := map[string]bool{} + names := []string{} + add := func(name string) { + name = strings.TrimSpace(name) + if name != "" && !seen[name] { + seen[name] = true + names = append(names, name) + } + } + for _, resolved := range registry.profiles { + for _, scanner := range resolved.profile.Scanners { + // scanner env: entries are credentials by declaration whatever + // their spelling. + for _, name := range scanner.Env { + add(name) + } + } + // sandbox.env mixes credentials with ordinary configuration + // (SKILLSPECTOR_PROVIDER); only credential-classified names feed + // redaction, or common values like "openai" would corrupt evidence. + for _, name := range resolved.sandbox.Env { + if runner.CredentialEnvName(name) { + add(name) + } + } + } + sort.Strings(names) + return names +} + func profileGateRules(scanners []ProfileScanner) map[string]runner.ScannerGatePolicy { rules := map[string]runner.ScannerGatePolicy{} for _, scanner := range scanners { @@ -231,6 +304,27 @@ func profileGateRules(scanners []ProfileScanner) map[string]runner.ScannerGatePo return rules } +// filterGateRulesToScanners drops gate rules for scanners not in the resolved +// run set. A --scanner override replaces the profile scanner list; gate rules +// for excluded scanners no longer apply and are silently dropped so the run +// gates exactly the scanners it executes. +func filterGateRulesToScanners(rules map[string]runner.ScannerGatePolicy, scanners []string) map[string]runner.ScannerGatePolicy { + if len(rules) == 0 { + return rules + } + keep := make(map[string]bool, len(scanners)) + for _, s := range scanners { + keep[s] = true + } + filtered := make(map[string]runner.ScannerGatePolicy, len(rules)) + for scanner, policy := range rules { + if keep[scanner] { + filtered[scanner] = policy + } + } + return filtered +} + type Sandbox struct { Mode string `yaml:"mode,omitempty"` Image string `yaml:"image,omitempty"` @@ -283,7 +377,27 @@ type cliIntent struct { } var judgePathPlaceholderPattern = regexp.MustCompile(`\{\{\s*(prompt|output_schema):([^}]+)\}\}`) -var scannerIDPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_-]*$`) + +// Lowercase-only: scanner IDs become output filenames via +// safeOutputPathSegment, which lowercases; case-distinct IDs would +// silently overwrite each other's raw evidence. +var scannerIDPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]*$`) + +// maxScannerIDLength keeps .json (plus bundle suffixes) comfortably +// inside the common 255-byte filename limit. +const maxScannerIDLength = 64 + +// windowsReservedScannerIDs are DOS device names Windows reserves even with +// an extension: a scan would succeed and then fail writing .json. IDs +// are already lowercase-only and dot-free, so a whole-ID match suffices. +var windowsReservedScannerIDs = map[string]bool{ + "con": true, "prn": true, "aux": true, "nul": true, + "com1": true, "com2": true, "com3": true, "com4": true, "com5": true, + "com6": true, "com7": true, "com8": true, "com9": true, + "lpt1": true, "lpt2": true, "lpt3": true, "lpt4": true, "lpt5": true, + "lpt6": true, "lpt7": true, "lpt8": true, "lpt9": true, +} + var scannerTargetPlaceholderPattern = regexp.MustCompile(`\{\{\s*target\s*\}\}`) type ResolvedRunSet struct { @@ -333,27 +447,34 @@ func resolveRunSetIntent(intent cliIntent, cwd string) (ResolvedRunSet, error) { return ResolvedRunSet{}, err } } + // Discovery without a profile would record the discovered file as the + // run's ConfigSource while applying none of its settings — a provenance + // claim the run does not honor. Reject instead of misleading, and check + // before the generic selection error so bare --discover-config gets the + // message naming its actual mistake. + if intent.discoverConfig && !intent.profileSet { + return ResolvedRunSet{}, errors.New("--discover-config requires --profile; use --config to run every profile in a config") + } if !hasExplicitRunSelection(intent) { return ResolvedRunSet{}, explicitRunSelectionError() } if intent.configPath != "" && !intent.profileSet { return resolveAllConfigProfiles(intent, cwd) } - // Discovery without a profile would record the discovered file as the - // run's ConfigSource while applying none of its settings — a provenance - // claim the run does not honor. Reject instead of misleading. - if intent.discoverConfig && !intent.profileSet { - return ResolvedRunSet{}, errors.New("--discover-config requires --profile; use --config to run every profile in a config") - } profileName := "" configSource := "" selected := resolvedProfile{profile: Profile{}} + var configEnvNames []string if intent.profileSet || intent.configPath != "" || intent.discoverConfig { registry, loadedConfig, err := loadConfigs(cwd, intent.configPath, intent.discoverConfig) if err != nil { return ResolvedRunSet{}, err } + // Redaction-only: with --sandbox off the run inherits the whole host + // env, so credentials declared by sibling profiles in the loaded + // config must be scrubbed even though only one profile executes. + configEnvNames = registry.declaredEnvNames() if loadedConfig != "" { configSource = loadedConfig } @@ -386,9 +507,10 @@ func resolveRunSetIntent(intent cliIntent, cwd string) (ResolvedRunSet, error) { return ResolvedRunSet{}, err } opts.Profile = profileName - opts.GateRules = profileGateRules(selected.profile.Scanners) + opts.GateRules = filterGateRulesToScanners(profileGateRules(selected.profile.Scanners), opts.Scanners) opts.ConfigSource = configSource opts.DiscoverConfig = intent.discoverConfig + opts.BatchRedactEnvNames = configEnvNames if opts.Judge != nil { opts.Judge.Files = files } @@ -488,7 +610,7 @@ func resolveAllConfigProfiles(intent cliIntent, cwd string) (ResolvedRunSet, err return ResolvedRunSet{}, err } opts.Profile = profileName - opts.GateRules = profileGateRules(selected.profile.Scanners) + opts.GateRules = filterGateRulesToScanners(profileGateRules(selected.profile.Scanners), opts.Scanners) opts.ConfigSource = filepath.Clean(projectPath) opts.OutputPath = "" opts.JSON = false @@ -925,13 +1047,28 @@ func validateProfile(name string, profile Profile) error { return fmt.Errorf("User-defined scanner in profile %s must include a non-empty id", name) } if scanner.custom && !scannerIDPattern.MatchString(scanner.ID) { - return fmt.Errorf("User-defined scanner %s in profile %s has invalid id; use letters, digits, underscores, and hyphens, starting with a letter or digit", scanner.ID, name) + return fmt.Errorf("User-defined scanner %s in profile %s has invalid id; use lowercase letters, digits, underscores, and hyphens, starting with a letter or digit", scanner.ID, name) + } + // IDs become .json filenames in output bundles and judge + // workspaces; an unbounded ID passes validation but fails the run + // at artifact-write time with "file name too long". + if scanner.custom && len(scanner.ID) > maxScannerIDLength { + return fmt.Errorf("User-defined scanner id in profile %s is %d characters; scanner IDs are used as file names and must be at most %d characters", name, len(scanner.ID), maxScannerIDLength) + } + if scanner.custom && windowsReservedScannerIDs[scanner.ID] { + return fmt.Errorf("User-defined scanner id %s in profile %s is a reserved Windows device name and cannot be used as an output file name", scanner.ID, name) } if scanner.custom && strings.TrimSpace(scanner.Command) == "" { return fmt.Errorf("User-defined scanner %s in profile %s must include a non-empty command", scanner.ID, name) } - if scanner.custom && !scannerTargetPlaceholdersAreUnquoted(scanner.Command) { - return fmt.Errorf("User-defined scanner %s in profile %s must use {{target}} outside shell quotes", scanner.ID, name) + if scanner.custom { + unquoted, active := scannerTargetPlaceholderUsage(scanner.Command) + if !unquoted { + return fmt.Errorf("User-defined scanner %s in profile %s must use {{target}} outside shell quotes", scanner.ID, name) + } + if !active { + return fmt.Errorf("User-defined scanner %s in profile %s must reference {{target}} outside quotes and comments so the scanner receives the scan target", scanner.ID, name) + } } if scanner.custom && runner.DefaultScannerRegistry().Contains(scanner.ID) { return fmt.Errorf("User-defined scanner %s collides with a built-in scanner ID", scanner.ID) @@ -940,6 +1077,20 @@ func validateProfile(name string, profile Profile) error { if code, overlaps := overlappingExitCodeRules(scanner.Gate.BlockOnExitCode, scanner.Gate.WarnOnExitCode); overlaps { return fmt.Errorf("User-defined scanner %s in profile %s gate blockOnExitCode and warnOnExitCode both claim exit code %d", scanner.ID, name, code) } + // Exit statuses >=125 are shell/docker infrastructure signals + // (Docker reserves 125-127; 128+N reports signal kills) and are + // never gate-eligible at runtime; accepting one here would create + // a rule that silently can never fire. + for _, rule := range []*profileExitCodeRule{scanner.Gate.BlockOnExitCode, scanner.Gate.WarnOnExitCode} { + if rule == nil { + continue + } + for _, code := range rule.Codes { + if code >= 125 { + return fmt.Errorf("User-defined scanner %s in profile %s gate exit code %d is reserved for shell and signal failures (Docker reserves 125-127); use codes 0-124", scanner.ID, name, code) + } + } + } } for _, target := range scanner.Targets { switch target { @@ -991,7 +1142,11 @@ func overlappingExitCodeRules(block *profileExitCodeRule, warn *profileExitCodeR return 0, false } -func scannerTargetPlaceholdersAreUnquoted(command string) bool { +// scannerTargetPlaceholderUsage reports whether every {{target}} placeholder +// sits outside shell quotes (unquoted) and whether at least one placeholder is +// active — outside quotes and comments — so the scanner actually receives the +// selected target. +func scannerTargetPlaceholderUsage(command string) (unquoted bool, active bool) { matches := scannerTargetPlaceholderPattern.FindAllStringIndex(command, -1) matchIndex := 0 quote := byte(0) @@ -1000,7 +1155,10 @@ func scannerTargetPlaceholdersAreUnquoted(command string) bool { for index := 0; index < len(command); index++ { if matchIndex < len(matches) && index == matches[matchIndex][0] { if !comment && quote != 0 { - return false + return false, active + } + if !comment { + active = true } index = matches[matchIndex][1] - 1 matchIndex++ @@ -1034,7 +1192,7 @@ func scannerTargetPlaceholdersAreUnquoted(command string) bool { quote = 0 } } - return true + return true, active } func shellCommentCanStart(command string, index int) bool { diff --git a/internal/profiles/resolver_test.go b/internal/profiles/resolver_test.go index 8d03219..ce416dc 100644 --- a/internal/profiles/resolver_test.go +++ b/internal/profiles/resolver_test.go @@ -174,29 +174,37 @@ profiles: } } -func TestResolveArgsBuiltInProfileProvenanceSurvivesDiscoveredConfig(t *testing.T) { +func TestResolveArgsSingleProfileCarriesSiblingDeclaredEnvForRedaction(t *testing.T) { dir := t.TempDir() - writeFile(t, filepath.Join(dir, ".clawscan.yml"), `version: 1 + config := filepath.Join(dir, ".clawscan.yml") + // Selecting profile-b must still carry profile-a's declared credential + // into the redaction set: with --sandbox off the run inherits the whole + // host env, so SHARED_ACCESS is reachable by profile-b's scanner. + writeFile(t, config, `version: 1 profiles: - unrelated: + profile-a: scanners: - - clawscan-static + - id: alpha + command: alpha {{target}} + env: [SHARED_ACCESS] + profile-b: + scanners: + - id: beta + command: beta {{target}} `) - opts, err := ResolveArgs([]string{"./skill", "--profile", "clawhub", "--discover-config"}, dir) + opts, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "profile-b"}, dir) if err != nil { t.Fatal(err) } - if opts.ConfigSource != "built-in" { - t.Fatalf("config source = %q, want built-in (unrelated discovered config must not claim provenance)", opts.ConfigSource) - } - - opts, err = ResolveArgs([]string{"./skill", "--profile", "clawhub", "--discover-config"}, t.TempDir()) - if err != nil { - t.Fatal(err) + found := false + for _, name := range opts.BatchRedactEnvNames { + if name == "SHARED_ACCESS" { + found = true + } } - if opts.ConfigSource != "built-in" { - t.Fatalf("config source = %q, want built-in when discovery finds nothing", opts.ConfigSource) + if !found { + t.Fatalf("sibling profile's declared env missing from redaction names: %v", opts.BatchRedactEnvNames) } } @@ -256,6 +264,12 @@ func TestResolveArgsDiscoverConfigRequiresProfile(t *testing.T) { if !strings.Contains(err.Error(), "--discover-config requires --profile") { t.Fatalf("err = %v", err) } + // Bare --discover-config (no other selection) must get the same + // specific message, not the generic no-selection error. + _, err = ResolveArgs([]string{"./skill", "--discover-config"}, t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "--discover-config requires --profile") { + t.Fatalf("bare --discover-config err = %v, want the discovery-specific error", err) + } } func TestResolveArgsConfigAndDiscoverConfigFlagsAreMutuallyExclusive(t *testing.T) { @@ -784,6 +798,28 @@ profiles: } } +func TestResolveArgsRejectsMalformedScannerEnvEntry(t *testing.T) { + dir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + writeFile(t, config, `version: 1 +profiles: + review: + scanners: + - id: my-scanner + command: my-scanner --json {{target}} + env: + - API_TOKEN=sk-live-resolver +`) + + _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + if err == nil || !strings.Contains(err.Error(), "API_TOKEN=...") { + t.Fatalf("err = %v, want sanitized malformed-env rejection", err) + } + if strings.Contains(err.Error(), "sk-live-resolver") { + t.Fatalf("rejection leaked credential value: %v", err) + } +} + func TestResolveArgsParsesUserDefinedScannerExitCodeGateRules(t *testing.T) { dir := t.TempDir() config := filepath.Join(dir, ".clawscan.yml") @@ -825,13 +861,13 @@ profiles: - id: blocker command: blocker {{target}} gate: - blockOnExitCode: 7 + blockOnExitCode: 124 `) opts, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) if err != nil { t.Fatal(err) } - if got := opts.GateRules["blocker"].BlockOnExitCode.Codes; !reflect.DeepEqual(got, []int{7}) { + if got := opts.GateRules["blocker"].BlockOnExitCode.Codes; !reflect.DeepEqual(got, []int{124}) { t.Fatalf("codes = %#v", got) } } @@ -857,6 +893,56 @@ func TestProfileScannerExitCodeGateRulesRoundTripYAML(t *testing.T) { } } +func TestProfileScannerEnvShapeValidation(t *testing.T) { + for _, test := range []struct { + name string + yaml string + wantEnv []string + wantError string + notInError []string + }{ + { + name: "scalar", + yaml: "id: demo\ncommand: demo {{target}}\nenv: sk-" + "live-cred\n", + wantError: "scanner env must be a list of variable names", + notInError: []string{"sk-" + "live-cred"}, + }, + { + name: "sequence", + yaml: "id: demo\ncommand: demo {{target}}\nenv: [API_TOKEN]\n", + wantEnv: []string{"API_TOKEN"}, + }, + { + name: "mapping element", + yaml: "id: demo\ncommand: demo {{target}}\nenv: [{a: b}]\n", + wantError: "scanner env entry #1 must be a variable name", + notInError: []string{"a: b"}, + }, + } { + t.Run(test.name, func(t *testing.T) { + var scanner ProfileScanner + err := yaml.Unmarshal([]byte(test.yaml), &scanner) + if test.wantError == "" { + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(scanner.Env, test.wantEnv) { + t.Fatalf("env = %#v, want %#v", scanner.Env, test.wantEnv) + } + return + } + if err == nil || !strings.Contains(err.Error(), test.wantError) { + t.Fatalf("err = %v, want %q", err, test.wantError) + } + for _, value := range test.notInError { + if strings.Contains(err.Error(), value) { + t.Fatalf("error leaked YAML value %q: %v", value, err) + } + } + }) + } +} + func TestResolveArgsRejectsInvalidExitCodeGateRules(t *testing.T) { tests := []struct { name string @@ -867,6 +953,12 @@ func TestResolveArgsRejectsInvalidExitCodeGateRules(t *testing.T) { {name: "non integer", rules: "blockOnExitCode: nope", want: `must be a non-negative integer, a list of non-negative integers, or "nonzero"`}, {name: "empty list", rules: "blockOnExitCode: []", want: "must not be an empty list"}, {name: "overlap", rules: "blockOnExitCode: nonzero\n warnOnExitCode: [0, 2]", want: "blockOnExitCode and warnOnExitCode both claim exit code 2"}, + {name: "reserved docker code boundary", rules: "blockOnExitCode: 125", want: "exit code 125 is reserved for shell and signal failures (Docker reserves 125-127); use codes 0-124"}, + {name: "reserved shell code", rules: "blockOnExitCode: 137", want: "exit code 137 is reserved for shell and signal failures (Docker reserves 125-127); use codes 0-124"}, + {name: "reserved code in list", rules: "warnOnExitCode: [1, 126]", want: "exit code 126 is reserved for shell and signal failures (Docker reserves 125-127); use codes 0-124"}, + {name: "implicit null rule", rules: "blockOnExitCode:", want: `gate blockOnExitCode must be an exit code, a list of exit codes, or "nonzero", not null`}, + {name: "explicit null rule", rules: "warnOnExitCode: null", want: `gate warnOnExitCode must be an exit code, a list of exit codes, or "nonzero", not null`}, + {name: "empty gate", rules: "{}", want: "scanner gate must declare blockOnExitCode or warnOnExitCode"}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -881,7 +973,7 @@ func TestResolveArgsRejectsInvalidExitCodeGateRules(t *testing.T) { } } -func TestGateRuleForProfileScannerExcludedByCLIOverrideFailsBeforeScanning(t *testing.T) { +func TestGateRuleForProfileScannerExcludedByCLIOverrideIsDropped(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "skill") if err := os.Mkdir(target, 0o755); err != nil { @@ -901,13 +993,22 @@ profiles: if err != nil { t.Fatal(err) } + // Verify that gate rules for absent-scanner were silently dropped. + if len(opts.GateRules) != 0 { + t.Fatalf("opts.GateRules should be empty, got: %#v", opts.GateRules) + } commandRunner := &profileCommandRunner{stdout: `{}`} - _, err = runner.Run(opts, runner.RunContext{Env: map[string]string{}, CommandRunner: commandRunner}) - if err == nil || err.Error() != "gate rule references scanner absent-scanner, but it was not requested" { + artifact, err := runner.Run(opts, runner.RunContext{Env: map[string]string{}, CommandRunner: commandRunner}) + if err != nil { t.Fatalf("err = %v", err) } - if commandRunner.command != "" { - t.Fatalf("scanner executed: %q", commandRunner.command) + // Verify the artifact has no fired gate rules and a passing gate. + if artifact.Gate != "pass" || len(artifact.GateRules) != 0 { + t.Fatalf("gate = %q, rules = %#v", artifact.Gate, artifact.GateRules) + } + // Verify clawscan-static ran. + if _, ok := artifact.Scanners["clawscan-static"]; !ok { + t.Fatal("artifact missing clawscan-static result") } } @@ -981,11 +1082,109 @@ profiles: `) _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) - if err == nil || err.Error() != "User-defined scanner foo=bar in profile review has invalid id; use letters, digits, underscores, and hyphens, starting with a letter or digit" { + if err == nil || err.Error() != "User-defined scanner foo=bar in profile review has invalid id; use lowercase letters, digits, underscores, and hyphens, starting with a letter or digit" { + t.Fatalf("err = %v", err) + } +} + +func TestResolveArgsRejectsAliasedNullGateRule(t *testing.T) { + // A null anchored elsewhere and referenced via alias reaches the gate as + // an AliasNode, not a !!null-tagged scalar; the fail-closed check must + // resolve aliases or the gate silently decodes to a disabled policy. + dir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + writeFile(t, config, `version: 1 +profiles: + review: + output: &disabled null + scanners: + - id: demo + command: demo {{target}} + gate: + blockOnExitCode: *disabled +`) + + _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + if err == nil || !strings.Contains(err.Error(), `gate blockOnExitCode must be an exit code, a list of exit codes, or "nonzero", not null`) { + t.Fatalf("err = %v", err) + } +} + +func TestResolveArgsRejectsOverlongUserDefinedScannerID(t *testing.T) { + dir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + longID := strings.Repeat("a", 65) + writeFile(t, config, `version: 1 +profiles: + review: + scanners: + - id: `+longID+` + command: scanner {{target}} +`) + + _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + if err == nil || err.Error() != "User-defined scanner id in profile review is 65 characters; scanner IDs are used as file names and must be at most 64 characters" { t.Fatalf("err = %v", err) } } +func TestResolveArgsAcceptsMaxLengthUserDefinedScannerID(t *testing.T) { + dir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + maxID := strings.Repeat("a", 64) + writeFile(t, config, `version: 1 +profiles: + review: + scanners: + - id: `+maxID+` + command: scanner {{target}} +`) + + if _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir); err != nil { + t.Fatalf("64-character scanner ID must be accepted: %v", err) + } +} + +func TestResolveArgsRejectsWindowsReservedScannerIDs(t *testing.T) { + dir := t.TempDir() + // con.json, nul.json, com1.json are unwritable on Windows even with the + // extension, so the run would fail after the scanner already completed. + for _, id := range []string{"con", "nul", "com1", "lpt9"} { + config := filepath.Join(dir, id+".clawscan.yml") + writeFile(t, config, `version: 1 +profiles: + review: + scanners: + - id: `+id+` + command: scanner {{target}} +`) + _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + want := "User-defined scanner id " + id + " in profile review is a reserved Windows device name and cannot be used as an output file name" + if err == nil || err.Error() != want { + t.Fatalf("id %s: err = %v", id, err) + } + } +} + +func TestResolveArgsAcceptsScannerIDsResemblingDeviceNames(t *testing.T) { + dir := t.TempDir() + // Only the exact device names are reserved; prefixed or suffixed IDs + // (console, com10, nul-check) are ordinary valid filenames. + for _, id := range []string{"console", "com10", "nul-check", "aux2"} { + config := filepath.Join(dir, id+".clawscan.yml") + writeFile(t, config, `version: 1 +profiles: + review: + scanners: + - id: `+id+` + command: scanner {{target}} +`) + if _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir); err != nil { + t.Fatalf("id %s must be accepted: %v", id, err) + } + } +} + func TestResolveArgsRejectsQuotedUserDefinedScannerTargetPlaceholder(t *testing.T) { dir := t.TempDir() config := filepath.Join(dir, ".clawscan.yml") @@ -1003,6 +1202,53 @@ profiles: } } +func TestResolveArgsRejectsUserDefinedScannerWithoutActiveTargetPlaceholder(t *testing.T) { + dir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + for name, command := range map[string]string{ + "missing": "my-scanner --json", + "comment-only": "my-scanner --json # {{target}}", + } { + writeFile(t, config, `version: 1 +profiles: + review: + scanners: + - id: my-scanner + command: `+command+` +`) + _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + if err == nil || !strings.Contains(err.Error(), "must reference {{target}}") { + t.Fatalf("%s: err = %v, want active-placeholder rejection", name, err) + } + } +} + +func TestResolveArgsBuiltInProfileProvenanceSurvivesDiscoveredConfig(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, ".clawscan.yml"), `version: 1 +profiles: + unrelated: + scanners: + - clawscan-static +`) + + opts, err := ResolveArgs([]string{"./skill", "--profile", "clawhub", "--discover-config"}, dir) + if err != nil { + t.Fatal(err) + } + if opts.ConfigSource != "built-in" { + t.Fatalf("config source = %q, want built-in (unrelated discovered config must not claim provenance)", opts.ConfigSource) + } + + opts, err = ResolveArgs([]string{"./skill", "--profile", "clawhub", "--discover-config"}, t.TempDir()) + if err != nil { + t.Fatal(err) + } + if opts.ConfigSource != "built-in" { + t.Fatalf("config source = %q, want built-in when discovery finds nothing", opts.ConfigSource) + } +} + func TestResolveArgsAllowsApostropheInShellCommentBeforeTargetPlaceholder(t *testing.T) { dir := t.TempDir() config := filepath.Join(dir, ".clawscan.yml") diff --git a/internal/runner/benchmark.go b/internal/runner/benchmark.go index 2f6dcc6..7ff90f7 100644 --- a/internal/runner/benchmark.go +++ b/internal/runner/benchmark.go @@ -210,7 +210,16 @@ func RunBenchmark(opts Options, ctx RunContext) (BenchmarkArtifact, error) { env = EnvMap(os.Environ()) } applyRuntimeEnvDefaults(opts, env) - if err := ValidateRequirements(opts, env); err != nil { + // Benchmark cases are skill targets; a scanner that can only skip them + // (e.g. plugin-only) must not demand credentials up front. Mirrors Run. + requirementOpts := opts + requirementOpts.Scanners = runnableScanners(opts, "skill") + if err := ValidateRequirements(requirementOpts, env); err != nil { + return BenchmarkArtifact{}, err + } + // Deterministic config errors must surface before any benchmark I/O; Run + // would reject this on the first case, after archives were downloaded. + if err := validateGateRuleScanners(opts); err != nil { return BenchmarkArtifact{}, err } if opts.Benchmark.IDsSource != "" { diff --git a/internal/runner/command_kill_unix.go b/internal/runner/command_kill_unix.go new file mode 100644 index 0000000..034d79e --- /dev/null +++ b/internal/runner/command_kill_unix.go @@ -0,0 +1,80 @@ +//go:build !windows + +package runner + +import ( + "os/exec" + "sync/atomic" + "syscall" +) + +// processTreeKiller makes timeout cancellation reach the whole process +// tree, not just the direct child. exec.CommandContext only kills the +// process it started; a user-defined command like `sleep 1h & wait` run +// through /bin/sh leaves grandchildren holding the stdout/stderr pipes, +// and cmd.Run would block on them long past the deadline. Each command +// gets its own process group, and Cancel signals the group. +type processTreeKiller struct { + cmd *exec.Cmd + fired atomic.Bool +} + +func configureCommandTreeKill(cmd *exec.Cmd) *processTreeKiller { + killer := &processTreeKiller{cmd: cmd} + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { + killer.fired.Store(true) + if cmd.Process == nil { + return nil + } + // Negative PID addresses the process group. Fall back to the + // direct process if the group signal fails (already reaped). + if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil { + return cmd.Process.Kill() + } + return nil + } + return killer +} + +// started is a post-Start hook; group membership is inherited via Setpgid, +// so there is nothing to do on Unix. +func (killer *processTreeKiller) started(*exec.Cmd) error { return nil } + +// reapStragglers kills descendants still holding the output pipes after +// the direct child exited (WaitDelay expiry). The group is private to this +// command, so the signal cannot reach unrelated processes. +func (killer *processTreeKiller) reapStragglers(cmd *exec.Cmd) { + killer.cmd = cmd + killer.release() +} + +// release reaps the private process group unconditionally. A scanner that +// completed normally can still have detached a daemon (`sleep 300 +// /dev/null &`) that holds no pipes but inherited scanner +// credentials; nothing may outlive the run. The group is private to this +// command, so the signal cannot reach unrelated processes, and a dead +// group is a harmless ESRCH. +func (killer *processTreeKiller) release() { + if killer.cmd != nil && killer.cmd.Process != nil { + _ = syscall.Kill(-killer.cmd.Process.Pid, syscall.SIGKILL) + } +} + +// killConfirmed reports whether the process's wait status shows it died +// from our SIGKILL rather than exiting on its own. A natural exit that +// races the deadline must keep its gate-eligible exit code instead of +// being misreported as a timeout. +func (killer *processTreeKiller) killConfirmed(cmd *exec.Cmd) bool { + if cmd.ProcessState == nil { + return false + } + status, ok := cmd.ProcessState.Sys().(syscall.WaitStatus) + return ok && status.Signaled() && status.Signal() == syscall.SIGKILL +} + +// cancelFired reports whether the kill path actually ran. The caller uses +// this instead of re-reading ctx.Err() after Wait returns: a command that +// finished just before the deadline must not be reclassified as timed out +// by a context timer that fired while the goroutine was descheduled. +func (killer *processTreeKiller) cancelFired() bool { return killer.fired.Load() } diff --git a/internal/runner/command_kill_unix_test.go b/internal/runner/command_kill_unix_test.go new file mode 100644 index 0000000..f869cd5 --- /dev/null +++ b/internal/runner/command_kill_unix_test.go @@ -0,0 +1,152 @@ +//go:build !windows + +package runner + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +func TestDefaultCommandRunnerNaturalExitRacingDeadlineKeepsExitCode(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + + natural := exec.CommandContext(context.Background(), "/bin/sh", "-c", "exit 7") + naturalKiller := configureCommandTreeKill(natural) + if err := natural.Run(); err == nil { + t.Fatal("natural command unexpectedly succeeded") + } + if naturalKiller.killConfirmed(natural) { + t.Fatal("natural exit was misclassified as a configured kill") + } + naturalKiller.release() + + killed := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 30") + killedKiller := configureCommandTreeKill(killed) + if err := killed.Start(); err != nil { + t.Fatal(err) + } + if err := killedKiller.started(killed); err != nil { + t.Fatal(err) + } + if err := killed.Cancel(); err != nil { + t.Fatal(err) + } + if err := killed.Wait(); err == nil { + t.Fatal("killed command unexpectedly succeeded") + } + if !killedKiller.killConfirmed(killed) { + t.Fatal("configured SIGKILL was not confirmed by the wait status") + } + killedKiller.release() +} + +func TestDefaultCommandRunnerReapsDetachedDaemonAfterNormalCompletion(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + // A scanner that completes normally can still have detached a daemon + // that holds no pipes (redirected to /dev/null) but inherited scanner + // credentials. Run returns immediately with success — and the private + // process group must still be reaped on the way out. + pidFile := filepath.Join(t.TempDir(), "daemon.pid") + output, err := defaultCommandRunner{}.Run( + "/bin/sh", []string{"-c", `echo '{}'; sleep 30 /dev/null 2>&1 & echo $! > ` + pidFile}, "", time.Minute) + if err != nil { + t.Fatalf("err = %v", err) + } + if output.ExitCode == nil || *output.ExitCode != 0 { + t.Fatalf("exit code = %v, want 0", output.ExitCode) + } + pid, readErr := os.ReadFile(pidFile) + if readErr != nil { + t.Fatal(readErr) + } + daemonPid, atoiErr := strconv.Atoi(strings.TrimSpace(string(pid))) + if atoiErr != nil || daemonPid <= 0 { + t.Fatalf("bad daemon pid %q: %v", pid, atoiErr) + } + deadline := time.Now().Add(5 * time.Second) + for syscall.Kill(daemonPid, 0) == nil { + if time.Now().After(deadline) { + _ = syscall.Kill(daemonPid, syscall.SIGKILL) + t.Fatalf("detached daemon %d outlived the completed run", daemonPid) + } + time.Sleep(50 * time.Millisecond) + } +} + +func TestDefaultCommandRunnerWaitDelayExpiryKillsDescendantsAndSuppressesExit(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + // The command exits zero but leaves a background child holding the + // stdout pipe. No timeout fires, so only WaitDelay unblocks Run — and + // on that path the descendant must be killed (it inherited scanner + // credentials) and the zero exit must not be recorded: pipe output + // from a force-closed run must not pass as a completed scan. + pidFile := filepath.Join(t.TempDir(), "child.pid") + output, err := defaultCommandRunner{WaitDelay: 200 * time.Millisecond}.Run( + "/bin/sh", []string{"-c", `echo '{}'; sleep 30 & echo $! > ` + pidFile}, "", time.Minute) + if err == nil || !strings.Contains(err.Error(), "background processes") { + t.Fatalf("err = %v, want wait-delay refusal", err) + } + if output.ExitCode != nil { + t.Fatalf("wait-delay run recorded exit code %d; partial output could pass as a completed scan", *output.ExitCode) + } + pid, readErr := os.ReadFile(pidFile) + if readErr != nil { + t.Fatal(readErr) + } + childPid, atoiErr := strconv.Atoi(strings.TrimSpace(string(pid))) + if atoiErr != nil || childPid <= 0 { + t.Fatalf("bad child pid %q: %v", pid, atoiErr) + } + deadline := time.Now().Add(5 * time.Second) + for syscall.Kill(childPid, 0) == nil { + if time.Now().After(deadline) { + _ = syscall.Kill(childPid, syscall.SIGKILL) + t.Fatalf("descendant %d survived wait-delay cleanup", childPid) + } + time.Sleep(50 * time.Millisecond) + } +} + +func TestDefaultCommandRunnerWaitDelayExpiryWithNonzeroExitSuppressesExit(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + pidFile := filepath.Join(t.TempDir(), "child.pid") + output, err := defaultCommandRunner{WaitDelay: 200 * time.Millisecond}.Run( + "/bin/sh", []string{"-c", `echo '{}'; sleep 30 & echo $! > ` + pidFile + `; exit 3`}, "", time.Minute) + if err == nil || !strings.Contains(err.Error(), "background processes") { + t.Fatalf("err = %v, want wait-delay refusal", err) + } + if output.ExitCode != nil { + t.Fatalf("wait-delay run recorded exit code %d; partial output could pass as a completed scan", *output.ExitCode) + } + pid, readErr := os.ReadFile(pidFile) + if readErr != nil { + t.Fatal(readErr) + } + childPid, atoiErr := strconv.Atoi(strings.TrimSpace(string(pid))) + if atoiErr != nil || childPid <= 0 { + t.Fatalf("bad child pid %q: %v", pid, atoiErr) + } + deadline := time.Now().Add(5 * time.Second) + for syscall.Kill(childPid, 0) == nil { + if time.Now().After(deadline) { + _ = syscall.Kill(childPid, syscall.SIGKILL) + t.Fatalf("descendant %d survived wait-delay cleanup", childPid) + } + time.Sleep(50 * time.Millisecond) + } +} diff --git a/internal/runner/command_kill_windows.go b/internal/runner/command_kill_windows.go new file mode 100644 index 0000000..6446e0c --- /dev/null +++ b/internal/runner/command_kill_windows.go @@ -0,0 +1,196 @@ +//go:build windows + +package runner + +import ( + "fmt" + "os/exec" + "sync/atomic" + "syscall" + "unsafe" + + "golang.org/x/sys/windows" +) + +// windowsKillExitCode is an arbitrary, improbable natural exit code stamped +// by our termination paths so deadline kills can be distinguished from exits. +const windowsKillExitCode = 0xC1AC5CA9 + +// processTreeKiller makes timeout cancellation reach the whole process +// tree on Windows. TerminateProcess kills only the direct child, and +// taskkill /T resolves the tree by parent PID at kill time — useless once +// the parent has already exited (WaitDelay expiry) because the PID is +// gone while grandchildren keep the pipes open. A Job Object survives the +// parent: every descendant stays in the job, and terminating the job +// kills them all regardless of the parent's state. +type processTreeKiller struct { + job windows.Handle + fired atomic.Bool + suspended bool +} + +func configureCommandTreeKill(cmd *exec.Cmd) *processTreeKiller { + killer := &processTreeKiller{} + if job, err := windows.CreateJobObject(nil, nil); err == nil { + // Descendants die with the job handle even if ClawScan itself is + // killed before running cancellation. + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err == nil { + killer.job = job + } else { + _ = windows.CloseHandle(job) + } + } + // CREATE_SUSPENDED closes the assignment race and guarantees that a + // containment failure can kill the child before it executes anything. + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.CreationFlags |= windows.CREATE_SUSPENDED + killer.suspended = true + cmd.Cancel = func() error { + killer.fired.Store(true) + if killer.job != 0 { + if err := windows.TerminateJobObject(killer.job, windowsKillExitCode); err == nil { + return nil + } + } + if cmd.Process == nil { + return nil + } + if err := terminateProcessWithSentinel(cmd.Process.Pid); err != nil { + return cmd.Process.Kill() + } + return nil + } + return killer +} + +func terminateProcessWithSentinel(pid int) error { + handle, err := windows.OpenProcess(windows.PROCESS_TERMINATE, false, uint32(pid)) + if err != nil { + return err + } + defer windows.CloseHandle(handle) + return windows.TerminateProcess(handle, windowsKillExitCode) +} + +// started assigns the suspended child to the job, then resumes it. It +// guarantees either full job containment or a dead child plus an error, so a +// command can never run uncontained with scanner credentials. +func (killer *processTreeKiller) started(cmd *exec.Cmd) error { + if cmd.Process == nil { + return nil + } + if killer.fired.Load() && killer.job == 0 { + if err := terminateProcessWithSentinel(cmd.Process.Pid); err != nil { + _ = cmd.Process.Kill() + } + return fmt.Errorf("process containment unavailable (job object creation failed); killed the sandboxed command rather than run it uncontained") + } + if killer.job == 0 { + _ = cmd.Process.Kill() + return fmt.Errorf("process containment unavailable (job object creation failed); killed the sandboxed command rather than run it uncontained") + } + pid := uint32(cmd.Process.Pid) + assigned := false + if handle, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, pid); err == nil { + assigned = windows.AssignProcessToJobObject(killer.job, handle) == nil + _ = windows.CloseHandle(handle) + } + if !assigned { + _ = cmd.Process.Kill() + _ = windows.CloseHandle(killer.job) + killer.job = 0 + return fmt.Errorf("failed to assign sandboxed command to its job object; killed it rather than run it uncontained") + } + if killer.fired.Load() { + // Cancellation already ran against an empty job (deadline fired + // between Start and assignment). The child must not resume: kill + // via the job it now belongs to and report the timeout-shaped + // death; the suspended child never executed anything. + _ = windows.TerminateJobObject(killer.job, windowsKillExitCode) + return fmt.Errorf("command canceled before sandbox assignment completed; killed it without resuming") + } + if !killer.suspended { + return nil + } + // Cancellation can race this check and ResumeThread, but assignment is + // complete, so Cancel's job termination reaches a briefly resumed child. + if resumeMainThread(pid) { + return nil + } + // The child is stuck suspended; leaving it would burn the whole run + // timeout and misreport a tooling failure as a scanner timeout. Kill it + // (job first, direct process as fallback) and surface the real cause. + killer.reapStragglers(cmd) + return fmt.Errorf("failed to resume sandboxed command after suspended start; killed it") +} + +// resumeMainThread resumes the CREATE_SUSPENDED child's initial thread. +func resumeMainThread(pid uint32) bool { + snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) + if err != nil { + return false + } + defer windows.CloseHandle(snapshot) + resumed := false + var entry windows.ThreadEntry32 + entry.Size = uint32(unsafe.Sizeof(entry)) + for err := windows.Thread32First(snapshot, &entry); err == nil; err = windows.Thread32Next(snapshot, &entry) { + if entry.OwnerProcessID != pid { + continue + } + if thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, entry.ThreadID); err == nil { + if _, err := windows.ResumeThread(thread); err == nil { + resumed = true + } + _ = windows.CloseHandle(thread) + } + } + return resumed +} + +// reapStragglers kills descendants still holding the output pipes after +// the direct child exited (WaitDelay expiry). The job outlives the parent +// PID, so this reaches grandchildren taskkill /T no longer could. +func (killer *processTreeKiller) reapStragglers(cmd *exec.Cmd) { + if killer.job != 0 { + if err := windows.TerminateJobObject(killer.job, windowsKillExitCode); err == nil { + return + } + } + if cmd.Process != nil { + if err := terminateProcessWithSentinel(cmd.Process.Pid); err != nil { + _ = cmd.Process.Kill() + } + } +} + +// release closes the job handle. KILL_ON_JOB_CLOSE also terminates any +// remaining members, so this doubles as last-resort cleanup. +func (killer *processTreeKiller) release() { + if killer.job != 0 { + _ = windows.CloseHandle(killer.job) + killer.job = 0 + } +} + +// killConfirmed reports whether the process exited with the sentinel code +// our kill paths stamp via TerminateJobObject/TerminateProcess. A natural +// exit racing the deadline keeps its own code and must not be reported as +// a timeout. +func (killer *processTreeKiller) killConfirmed(cmd *exec.Cmd) bool { + return cmd.ProcessState != nil && uint32(cmd.ProcessState.ExitCode()) == windowsKillExitCode +} + +// cancelFired reports whether the kill path actually ran. The caller uses +// this instead of re-reading ctx.Err() after Wait returns: a command that +// finished just before the deadline must not be reclassified as timed out +// by a context timer that fired while the goroutine was descheduled. +func (killer *processTreeKiller) cancelFired() bool { return killer.fired.Load() } diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 54926d8..aeae43e 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -40,6 +40,13 @@ type Options struct { Judge *JudgeOptions Sandbox SandboxOptions GateRules map[string]ScannerGatePolicy + // BatchRedactEnvNames widens redaction (never sandbox passthrough) to + // credentials declared outside this run's own scanner set: sibling + // profiles in a --config batch, and other profiles in the loaded config + // on single-profile runs. With --sandbox off every host command + // inherits the same full environment, so a blandly named credential + // declared elsewhere in the config is still reachable here. + BatchRedactEnvNames []string } type ExitCodeRule struct { @@ -390,7 +397,7 @@ func ParseArgsWithRegistry(args []string, registry ScannerRegistry) (Options, er func ValidateRequirements(opts Options, env map[string]string) error { var missing []EnvRequirement for _, req := range requirements(opts, env) { - if strings.TrimSpace(env[req.EnvVar]) == "" { + if envValueForName(env, req.EnvVar) == "" { missing = append(missing, req) } } @@ -434,12 +441,18 @@ func Run(opts Options, ctx RunContext) (Artifact, error) { if err != nil { return Artifact{}, err } + // An injected RunContext.CommandRunner executes directly with the host + // environment even when options request Docker, so redaction for both + // scanners and the judge must scope to the whole host env, not the + // container allowlist — otherwise an undeclared host credential in + // valid output would persist unscrubbed. + effectiveSandboxMode := sandbox.Mode + if ctx.CommandRunner != nil { + effectiveSandboxMode = SandboxModeOff + } scannerRunner := ctx.ScannerRunner if scannerRunner == nil { - scannerSandboxMode := sandbox.Mode - if ctx.CommandRunner != nil { - scannerSandboxMode = SandboxModeOff - } + scannerSandboxMode := effectiveSandboxMode scannerRunner = ExternalScannerRunner{ CommandRunner: commandRunner, Env: env, @@ -451,6 +464,14 @@ func Run(opts Options, ctx RunContext) (Artifact, error) { SkillSpectorCommand: ctx.SkillSpectorCommand, VirusTotalHTTPClient: ctx.VirusTotalHTTPClient, Timeout: 20 * time.Minute, + // Redaction scope follows what commands can see: the whole + // host env with --sandbox off (skipped and fixture scanners' + // credentials included), the passthrough allowlist under + // Docker. The Docker allowlist is built from gatingOpts + // (runnable scanners only), so redaction must use the same + // options — a skipped scanner's credential never enters the + // container and scrubbing its value would corrupt evidence. + ExposedEnvNames: redactionEnvNames(redactionOptsForMode(opts, gatingOpts, scannerSandboxMode), env, scannerSandboxMode), } } artifact := NewArtifact(opts, target.resolvedPath, startedAt, startedAt, env) @@ -470,7 +491,7 @@ func Run(opts Options, ctx RunContext) (Artifact, error) { for _, scanner := range opts.Scanners { scannerStartedAt := now().UTC().Format(time.RFC3339Nano) scannerTimerStarted := time.Now() - result, err := scannerResult(opts, scanner, target, scannerStartedAt, scannerRunner) + result, err := scannerResult(opts, scanner, target, scannerStartedAt, scannerRunner, env) if err != nil { return Artifact{}, err } @@ -479,7 +500,7 @@ func Run(opts Options, ctx RunContext) (Artifact, error) { } evaluateGate(&artifact, opts) if opts.Judge != nil { - result, err := RunJudge(*opts.Judge, artifact, commandRunner, 20*time.Minute, env, sandbox.Mode) + result, err := RunJudge(*opts.Judge, artifact, commandRunner, 20*time.Minute, env, sandbox.Mode, effectiveSandboxMode, redactionEnvNames(redactionOptsForMode(opts, gatingOpts, effectiveSandboxMode), env, effectiveSandboxMode)) if err != nil { return Artifact{}, err } @@ -589,10 +610,28 @@ func RunProfileBatch(optsList []Options, ctx RunContext, cwd string) (BatchArtif ScannerStatuses: map[string]map[string]int{}, }, } + // All profiles in the batch share one process environment on the host, + // so every profile's persisted output must scrub the union of declared + // credentials, not just its own. Collected with host-mode scope: the + // union is only applied to host runs (Docker runs drop + // BatchRedactEnvNames because sibling credentials never enter their + // containers). + batchEnvNames := map[string]bool{} + for _, opts := range optsList { + for _, name := range redactionEnvNames(opts, env, SandboxModeOff) { + batchEnvNames[name] = true + } + } + sharedEnvNames := make([]string, 0, len(batchEnvNames)) + for name := range batchEnvNames { + sharedEnvNames = append(sharedEnvNames, name) + } + sort.Strings(sharedEnvNames) targets := map[string]bool{} for _, opts := range optsList { runOpts := opts runOpts.OutputPath = "" + runOpts.BatchRedactEnvNames = sharedEnvNames for key, value := range envPresence(runOpts, env) { batch.Env[key] = value } @@ -853,7 +892,7 @@ func writeJSONFile(path string, value interface{}) error { return WriteJSON(file, value) } -func scannerResult(opts Options, scanner string, target resolvedTarget, startedAt string, scannerRunner ScannerRunner) (ScannerResult, error) { +func scannerResult(opts Options, scanner string, target resolvedTarget, startedAt string, scannerRunner ScannerRunner, env map[string]string) (ScannerResult, error) { if path := opts.ScannerResultPaths[scanner]; path != "" { raw, err := os.ReadFile(path) if err != nil { @@ -862,13 +901,27 @@ func scannerResult(opts Options, scanner string, target resolvedTarget, startedA if !json.Valid(raw) { return ScannerResult{}, fmt.Errorf("scanner result %s is not valid JSON: %s", scanner, path) } + // Same evidence-safety bar as live scanner stdout: invalid UTF-8 + // and duplicate object members defeat structural redaction. + if reason := scannerEvidenceUnsafe(string(raw)); reason != "" { + return ScannerResult{}, fmt.Errorf("scanner result %s rejected: %s: %s", scanner, reason, path) + } + // Fixture evidence bypasses RunScanner, so it must pass the same + // redaction boundary: a fixture captured from a live run can embed + // a currently present declared credential. No command ran, so the + // declared credential names are in scope (host-wide collection) but + // the env is narrowed to those names — sweeping every host secret's + // value would let an unrelated coincidence (CI_TOKEN=clean) corrupt + // fixture verdicts. + names := redactionEnvNames(opts, env, SandboxModeOff) + redacted := redactScannerStdout(string(raw), commandVisibleEnv(env, names, SandboxModeDocker), names) return ScannerResult{ Status: "completed", StartedAt: startedAt, CompletedAt: startedAt, Command: []string{"scanner-result", scanner + "=" + path}, Error: "", - Raw: json.RawMessage(raw), + Raw: json.RawMessage(redacted), }, nil } if !scannerSupportsTargetKindInRegistry(registryForOptions(opts), scanner, target.kind) { @@ -1140,7 +1193,17 @@ type judgeShellSpec struct { quote func(string) string } -func RunJudge(opts JudgeOptions, artifact Artifact, commandRunner CommandRunner, timeout time.Duration, env map[string]string, sandboxMode string) (*JudgeResult, error) { +// exposedEnvNames lists every env var reachable by commands this run (sandbox +// allowlist plus all scanners' declared env). The judge shares the command +// runner with scanners, so its redaction must cover the same set: a declared +// credential with a bland name (SCANNER_ACCESS) evades isSecretEnvKey. +// +// sandboxMode is the requested mode and drives the {{ judge_sandbox }} +// placeholder (the run configuration's trust boundary). redactionSandboxMode +// is the mode the command actually executes under — an injected +// RunContext.CommandRunner runs on the host even when Docker was requested — +// and scopes which env values are scrubbed from persisted judge output. +func RunJudge(opts JudgeOptions, artifact Artifact, commandRunner CommandRunner, timeout time.Duration, env map[string]string, sandboxMode string, redactionSandboxMode string, exposedEnvNames []string) (*JudgeResult, error) { workspace, err := os.MkdirTemp("", "clawscan-judge-*") if err != nil { return nil, err @@ -1190,9 +1253,19 @@ func RunJudge(opts JudgeOptions, artifact Artifact, commandRunner CommandRunner, Error: "", Result: nil, } + // Under Docker the judge only sees allowlisted env names; the full host + // env stays in scope for --sandbox off and injected host runners. + visibleEnv := commandVisibleEnv(env, exposedEnvNames, redactionSandboxMode) + scrub := func(value string) string { + return redactDeclaredEnvValues(value, visibleEnv, exposedEnvNames) + } if runErr != nil { + // commandError's own secret-named sweep must use the visible env + // too: under Docker an unallowlisted host value (DATABASE_URL=clean) + // never reached the container, and pre-scrubbing with the full host + // env would rewrite matching diagnostics before scrub could see them. result.Status = "failed" - result.Error = commandError(runErr, output.Stderr, env) + result.Error = scrub(commandError(runErr, output.Stderr, visibleEnv)) } if raw == "" { if result.Status == "failed" { @@ -1202,18 +1275,24 @@ func RunJudge(opts JudgeOptions, artifact Artifact, commandRunner CommandRunner, result.Error = "Judge command did not produce JSON output." return result, nil } + // Structural redaction first: a declared credential emitted as a JSON + // number, bool, or null ({"auth":1234} for SCANNER_ACCESS=1234) never + // appears as a string node, so the string-only walk below cannot catch + // it. redactScannerStdout compares scalars exactly, as the scanner + // output path does. + raw = redactScannerStdout(raw, visibleEnv, exposedEnvNames) var parsed any if err := json.Unmarshal([]byte(raw), &parsed); err != nil { if result.Status == "failed" { - result.Result = redactEnvValues(raw, env) + result.Result = scrub(raw) return result, nil } result.Status = "failed" result.Error = fmt.Sprintf("Judge command produced invalid JSON: %v", err) - result.Result = redactEnvValues(raw, env) + result.Result = scrub(raw) return result, nil } - parsed = redactJudgeResult(parsed, env) + parsed = redactJudgeResult(parsed, scrub, newSecretScrubber(scannerSecretValues(visibleEnv, exposedEnvNames))) parsedObject, ok := parsed.(map[string]any) if !ok { result.Status = "failed" @@ -1446,11 +1525,33 @@ func posixShellQuote(value string) string { return "'" + strings.ReplaceAll(value, "'", "'\"'\"'") + "'" } +// windowsCmdQuote quotes a value for Windows argv parsing +// (CommandLineToArgvW rules): backslashes are literal unless they precede a +// double quote, so a run of backslashes before an embedded quote or the +// closing quote must be doubled — otherwise a target like C:\ renders as +// "C:\" and the trailing backslash escapes the closing quote, corrupting +// the argument. func windowsCmdQuote(value string) string { - if value == "" { - return `""` + var out strings.Builder + out.WriteByte('"') + pending := 0 + for _, r := range value { + switch r { + case '\\': + pending++ + case '"': + out.WriteString(strings.Repeat(`\`, 2*pending+1)) + out.WriteByte('"') + pending = 0 + default: + out.WriteString(strings.Repeat(`\`, pending)) + pending = 0 + out.WriteRune(r) + } } - return `"` + strings.ReplaceAll(value, `"`, `\"`) + `"` + out.WriteString(strings.Repeat(`\`, 2*pending)) + out.WriteByte('"') + return out.String() } func prepareJudgePrompt(source string, artifact Artifact, state *judgeCommandState) (string, error) { @@ -1914,6 +2015,10 @@ type ExternalScannerRunner struct { SkillSpectorCommand []string VirusTotalHTTPClient VirusTotalHTTPClient Timeout time.Duration + // ExposedEnvNames lists every env var reachable by scanners this run + // (sandbox allowlist plus all adapters' declared env), so one scanner's + // output can be scrubbed of another scanner's credentials. + ExposedEnvNames []string } func (runner ExternalScannerRunner) RunScanner(name string, target string, startedAt string) (ScannerResult, error) { @@ -1932,7 +2037,42 @@ func (runner ExternalScannerRunner) RunScanner(name string, target string, start Raw: nil, }, nil } - return adapter.Run(runner, target, startedAt) + result, err := adapter.Run(runner, target, startedAt) + if err != nil { + return result, err + } + // Every adapter's evidence must meet the same safety bar as + // user-defined output: duplicate JSON members and invalid UTF-8 defeat + // the structural redaction below, and re-encoding to repair them would + // silently rewrite evidence. Reject instead. + if len(result.Raw) > 0 && json.Valid(result.Raw) { + if reason := scannerEvidenceUnsafe(string(result.Raw)); reason != "" { + result.Status = "failed" + result.Error = fmt.Sprintf("Scanner %s output rejected: %s", name, reason) + result.Raw = nil + } + } + // Central redaction boundary: every adapter's persisted output — not + // just user-defined scanners' — must scrub declared credentials. + // A profile can hand a blandly named credential (SCANNER_ACCESS) to the + // shared environment, and a built-in scanner may echo it in stdout or + // stderr; adapters that already redact internally pass through + // unchanged because their output no longer contains any secret. + // This must run even with no exposed names: with --sandbox off the + // command still inherits the whole host env, and the secret-named sweep + // inside the redaction helpers covers undeclared credentials (CI_TOKEN). + // Under Docker an empty allowlist narrows visibleEnv to nothing and the + // helpers return their input unchanged. + visibleEnv := commandVisibleEnv(runner.Env, runner.ExposedEnvNames, runner.SandboxMode) + if len(visibleEnv) > 0 { + if len(result.Raw) > 0 { + result.Raw = json.RawMessage(redactScannerStdout(string(result.Raw), visibleEnv, runner.ExposedEnvNames)) + } + if result.Error != "" { + result.Error = redactDeclaredEnvValues(result.Error, visibleEnv, runner.ExposedEnvNames) + } + } + return result, nil } func (runner ExternalScannerRunner) runAgentVerus(target string, startedAt string) (ScannerResult, error) { @@ -1949,7 +2089,7 @@ func (runner ExternalScannerRunner) runAgentVerus(target string, startedAt strin } output, runErr := runner.CommandRunner.Run(command, args, "", timeout) completedAt := time.Now().UTC().Format(time.RFC3339Nano) - raw := strings.TrimSpace(output.Stdout) + raw := output.Stdout if runErr != nil { message := commandError(runErr, output.Stderr, runner.Env) if json.Valid([]byte(raw)) { @@ -2131,6 +2271,9 @@ func discoverSkillSpectorCommand() []string { type defaultCommandRunner struct { Env map[string]string + // WaitDelay bounds the post-exit wait for descendants holding the + // output pipes; zero means the production default. Overridden in tests. + WaitDelay time.Duration } func (runner defaultCommandRunner) Run(command string, args []string, cwd string, timeout time.Duration) (CommandOutput, error) { @@ -2141,16 +2284,119 @@ func (runner defaultCommandRunner) Run(command string, args []string, cwd string if runner.Env != nil { cmd.Env = envMapToEnviron(runner.Env) } - var stdout bytes.Buffer - var stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - err := cmd.Run() - if ctx.Err() == context.DeadlineExceeded { + // Own the stdio pipes instead of handing exec a bytes.Buffer: with + // *os.File stdio, Wait returns as soon as the direct child exits and + // never blocks on descendants holding the pipes — so drain expiry is + // observed here directly, regardless of the child's exit status. The + // stdlib's ErrWaitDelay only surfaces when Wait would otherwise return + // nil; a scanner that exits nonzero while a backgrounded child holds + // stdout would silently keep its exit code and pass force-closed + // partial output off as a completed scan. + stdoutR, stdoutW, err := os.Pipe() + if err != nil { + return CommandOutput{}, err + } + stderrR, stderrW, err := os.Pipe() + if err != nil { + stdoutR.Close() + stdoutW.Close() + return CommandOutput{}, err + } + cmd.Stdout = stdoutW + cmd.Stderr = stderrW + killer := configureCommandTreeKill(cmd) + defer killer.release() + // WaitDelay still bounds Wait if Cancel fails to kill the child. + cmd.WaitDelay = 10 * time.Second + if runner.WaitDelay > 0 { + cmd.WaitDelay = runner.WaitDelay + } + err = cmd.Start() + // The parent's copies of the write ends must close regardless of Start + // success, or the readers below would never see EOF. + stdoutW.Close() + stderrW.Close() + var stdout, stderr bytes.Buffer + drained := make(chan struct{}, 2) + go func() { + _, _ = io.Copy(&stdout, stdoutR) + drained <- struct{}{} + }() + go func() { + _, _ = io.Copy(&stderr, stderrR) + drained <- struct{}{} + }() + pendingReaders := 2 + // awaitDrain reports how many readers have not hit EOF by the deadline. + awaitDrain := func(pending int, deadline time.Duration) int { + timer := time.NewTimer(deadline) + defer timer.Stop() + for pending > 0 { + select { + case <-drained: + pending-- + case <-timer.C: + return pending + } + } + return 0 + } + closeReadersAndAwait := func() { + stdoutR.Close() + stderrR.Close() + // Closing an *os.File interrupts blocked reads. Wait for every copier + // to acknowledge that close before touching either buffer. + for pendingReaders > 0 { + <-drained + pendingReaders-- + } + } + waitDelayExpired := false + if err == nil { + if startErr := killer.started(cmd); startErr != nil { + _ = cmd.Wait() + closeReadersAndAwait() + return CommandOutput{}, startErr + } + err = cmd.Wait() + pendingReaders = awaitDrain(pendingReaders, cmd.WaitDelay) + if pendingReaders > 0 { + // The child exited but descendants still hold the pipes. Kill + // the tree, then force EOF by closing the read ends so the + // copier goroutines cannot leak even if a descendant escaped. + killer.reapStragglers(cmd) + pendingReaders = awaitDrain(pendingReaders, time.Second) + if pendingReaders > 0 { + closeReadersAndAwait() + } + waitDelayExpired = true + } + } + closeReadersAndAwait() + // Classify timeout by whether the kill path actually fired and the wait + // status confirms that kill, not just by re-reading ctx.Err(). Cancel can + // race a natural exit that happened just before the deadline, and that + // exit's gate-eligible code must survive. + timedOut := killer.cancelFired() && ctx.Err() == context.DeadlineExceeded && err != nil && killer.killConfirmed(cmd) + if waitDelayExpired && !timedOut { + // Suppress the exit verdict whatever it was: partial pipe output + // from a run that needed force-closing must not pass as a + // completed scan. + if err != nil { + err = fmt.Errorf("command left background processes holding its output pipes; killed the process tree: %w", err) + } else { + err = errors.New("command left background processes holding its output pipes; killed the process tree") + } + } + if timedOut { err = fmt.Errorf("command timed out after %s", timeout) } output := CommandOutput{Stdout: stdout.String(), Stderr: stderr.String()} - if cmd.ProcessState != nil { + // A timed-out process has no exit verdict: Unix reports the kill as + // 128+N (filtered later), but Windows TerminateProcess reports 1, which + // is indistinguishable from a real findings-mean-nonzero exit and would + // let partial output pass as a completed scan. + if cmd.ProcessState != nil && !timedOut && !waitDelayExpired { exitCode := cmd.ProcessState.ExitCode() if exitCode >= 0 { output.ExitCode = &exitCode @@ -2215,7 +2461,14 @@ func validateGateRuleScanners(opts Options) error { } func evaluateGate(artifact *Artifact, opts Options) { + // A repeated --scanner flag keeps duplicate entries in opts.Scanners but + // only one result per ID; evaluating twice would double the fired rules. + evaluated := map[string]bool{} for _, scanner := range opts.Scanners { + if evaluated[scanner] { + continue + } + evaluated[scanner] = true result := artifact.Scanners[scanner] if result.Status != "completed" || result.ExitCode == nil { continue @@ -2282,7 +2535,10 @@ func redactEnvValues(value string, env map[string]string) string { } secrets := make([]string, 0) for key, secret := range env { - if strings.TrimSpace(secret) == "" || !isSecretEnvKey(key) { + // Exemptions are by name (nonCredentialSecretNamedEnv), never by + // value: DB_PASSWORD=default is a weak credential, and skipping + // common-looking values would persist it unredacted. + if strings.TrimSpace(secret) == "" || !isSecretEnvKey(key) || nonCredentialSecretNamedEnv(key) { continue } secrets = append(secrets, secret) @@ -2290,27 +2546,45 @@ func redactEnvValues(value string, env map[string]string) string { sort.Slice(secrets, func(i int, j int) bool { return len(secrets[i]) > len(secrets[j]) }) - redacted := value - for _, secret := range secrets { - redacted = strings.ReplaceAll(redacted, secret, "[redacted]") - } - return redacted + return newSecretScrubber(secrets).scrub(value) } -func redactJudgeResult(value any, env map[string]string) any { +func redactJudgeResult(value any, scrub func(string) string, scrubber secretScrubber) any { switch typed := value.(type) { case string: - return redactEnvValues(typed, env) + return scrub(typed) case []any: redacted := make([]any, len(typed)) for index, item := range typed { - redacted[index] = redactJudgeResult(item, env) + redacted[index] = redactJudgeResult(item, scrub, scrubber) } return redacted case map[string]any: + // Unchanged keys land first, scrubbed keys are inserted + // collision-safe afterwards: two keys scrubbing to the same marker + // (or a pre-existing key equal to it) would otherwise overwrite one + // another and silently drop a field from the judge result. Sorted + // order keeps collision suffixes deterministic. + keys := make([]string, 0, len(typed)) + for key := range typed { + keys = append(keys, key) + } + sort.Strings(keys) redacted := make(map[string]any, len(typed)) - for key, item := range typed { - redacted[redactEnvValues(key, env)] = redactJudgeResult(item, env) + var scrubbedKeys []string + renames := map[string]string{} + for _, key := range keys { + child := redactJudgeResult(typed[key], scrub, scrubber) + if scrubbedKey := scrub(key); scrubbedKey != key { + scrubbedKeys = append(scrubbedKeys, key) + renames[key] = scrubbedKey + typed[key] = child + continue + } + redacted[key] = child + } + for _, key := range scrubbedKeys { + redacted[collisionFreeKey(renames[key], redacted, scrubber)] = typed[key] } return redacted default: @@ -2318,13 +2592,83 @@ func redactJudgeResult(value any, env map[string]string) any { } } +// CredentialEnvName reports whether an env var name should be treated as a +// credential for redaction. Secret-named vars always are. Names the built-in +// registry lists as optional configuration (SKILLSPECTOR_PROVIDER, +// DEFAULT_MODEL) are not: their common values (openai, info) would corrupt +// valid evidence if scrubbed. Unknown names fail closed as credentials, +// because sandbox.env is the documented channel for blandly named +// judge-specific credentials. Windows optional-env matching folds case to +// follow that platform's case-insensitive environment semantics. +func CredentialEnvName(name string) bool { + return credentialEnvNameOnGOOS(name, runtime.GOOS) +} + +func credentialEnvNameOnGOOS(name string, goos string) bool { + // A name the registry documents as a non-credential configuration switch + // (PASSWORD_STORE_ENABLE_EXTENSIONS) is not a credential even though its + // name matches the secret-name heuristic; classifying it as one would sweep + // its plain value (a boolean/length) out of legitimate evidence. Explicit + // user-defined env: declarations stay credentials via the declared[name] + // and scanner.Env branches at the call sites, not here. + if nonCredentialSecretNamedEnv(name) { + return false + } + if isSecretEnvKey(name) { + return true + } + for _, info := range DefaultScannerRegistry().Infos() { + for _, optional := range info.OptionalEnv { + if optional == name { + return false + } + // Windows env names are case-insensitive: the same variable + // reaches the scanner under any spelling, and scrubbing an + // optional-config value like "openai" corrupts evidence. + // Elsewhere spellings are distinct names and stay fail-closed. + if goos == "windows" && strings.EqualFold(optional, name) { + return false + } + } + } + return true +} + +// secretEnvKeySegments are credential markers matched as whole +// underscore-separated segments of the variable name, so GITHUB_PAT and +// SNYK_TOKEN match while GIT_AUTHOR_NAME, PATTERN, and +// TOKENIZERS_PARALLELISM do not — a substring match there would scrub +// non-secret values (an author name, "false") from evidence. DSN, +// DATABASE_URL, and CONNECTION_STRING count because connection strings +// routinely embed passwords (postgres://user:pass@). +var secretEnvKeySegments = map[string]bool{ + "AUTH": true, "CRED": true, "CREDENTIAL": true, "CREDENTIALS": true, + "PAT": true, "DSN": true, "PASSWD": true, "PWD": true, "TOKEN": true, +} + func isSecretEnvKey(key string) bool { upper := strings.ToUpper(key) - return strings.Contains(upper, "TOKEN") || - strings.Contains(upper, "SECRET") || + // Bare PWD is the shell's working directory, not a credential: + // scrubbing its value would erase legitimate paths from persisted + // evidence. DB_PWD-style names still match via the segment scan below. + if upper == "PWD" { + return false + } + if strings.Contains(upper, "SECRET") || strings.Contains(upper, "PASSWORD") || strings.HasSuffix(upper, "_KEY") || - strings.Contains(upper, "API_KEY") + strings.HasSuffix(upper, "TOKEN") || + strings.Contains(upper, "API_KEY") || + strings.Contains(upper, "DATABASE_URL") || + strings.Contains(upper, "CONNECTION_STRING") { + return true + } + for _, segment := range strings.Split(upper, "_") { + if secretEnvKeySegments[segment] { + return true + } + } + return false } func requirements(opts Options, env map[string]string) []EnvRequirement { @@ -2340,6 +2684,17 @@ func requirements(opts Options, env map[string]string) []EnvRequirement { return dedupe(reqs) } +// redactionOptsForMode picks which option set scopes redaction: under +// Docker the container allowlist is built from the runnable-scanner subset +// (gatingOpts), so redaction must match it; host mode keeps the full +// selection since every host command inherits the whole environment. +func redactionOptsForMode(opts Options, gatingOpts Options, sandboxMode string) Options { + if sandboxMode == SandboxModeDocker { + return gatingOpts + } + return opts +} + func registryForOptions(opts Options) ScannerRegistry { if opts.ScannerRegistry.isZero() { return DefaultScannerRegistry() @@ -2350,7 +2705,7 @@ func registryForOptions(opts Options) ScannerRegistry { func envPresence(opts Options, env map[string]string) map[string]string { out := map[string]string{} for _, req := range requirements(opts, env) { - if strings.TrimSpace(env[req.EnvVar]) == "" { + if envValueForName(env, req.EnvVar) == "" { out[req.EnvVar] = "missing" } else { out[req.EnvVar] = "present" diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 4f2bb0e..bec9293 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -419,6 +419,36 @@ func TestRunOpenClawBenchmarkUsesExplicitPredictionsOutput(t *testing.T) { }) } +func TestRunBenchmarkSkipsRequirementsOfPluginOnlyScanners(t *testing.T) { + pluginOnly := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "plugin-only", Command: "plugin-only {{target}}", Env: []string{"PLUGIN_ONLY_TOKEN"}, Targets: []string{"plugin"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(pluginOnly) + if err != nil { + t.Fatal(err) + } + opts := benchmarkTestOptions(t, "SkillTrustBench", "", 0, 0, "") + opts.Scanners = []string{"clawscan-static", "plugin-only"} + opts.ScannerRegistry = registry + _, err = RunBenchmark(opts, RunContext{ + Env: map[string]string{}, + BenchmarkClient: staticBenchmarkClient{ + skillTrustBenchRows: []SkillTrustBenchRow{ + {ID: "case_1", Judgment: "malicious"}, + }, + materializedSkillTrustBench: map[string]map[string]string{ + "case_1": {"SKILL.md": "# Demo\n"}, + }, + }, + }) + if err != nil && strings.Contains(err.Error(), "PLUGIN_ONLY_TOKEN") { + t.Fatalf("plugin-only scanner requirements must not gate a skill benchmark: %v", err) + } + if err != nil { + t.Fatal(err) + } +} + func TestRunBenchmarkRejectsPredictionsOutputForSkillTrustBench(t *testing.T) { opts := benchmarkTestOptions(t, "SkillTrustBench", "", 0, 0, filepath.Join(t.TempDir(), "predictions.jsonl")) _, err := RunBenchmark(opts, RunContext{ @@ -437,6 +467,41 @@ func TestRunBenchmarkRejectsPredictionsOutputForSkillTrustBench(t *testing.T) { } } +func TestRunBenchmarkValidatesGateRulesBeforeBenchmarkIO(t *testing.T) { + opts := benchmarkTestOptions(t, "SkillTrustBench", "", 0, 0, "") + opts.GateRules = map[string]ScannerGatePolicy{ + "missing-scanner": {BlockOnExitCode: &ExitCodeRule{Codes: []int{3}}}, + } + _, err := RunBenchmark(opts, RunContext{ + Env: map[string]string{}, + BenchmarkClient: failingBenchmarkClient{t: t}, + }) + if err == nil || !strings.Contains(err.Error(), "gate rule references scanner missing-scanner, but it was not requested") { + t.Fatalf("err = %v", err) + } +} + +// failingBenchmarkClient fails the test on any use: gate misconfiguration +// must be rejected before benchmark data is fetched. +type failingBenchmarkClient struct { + t *testing.T +} + +func (client failingBenchmarkClient) FetchOpenClawRows(dataset string, split string, offset int, limit int) ([]OpenClawBenchmarkRow, error) { + client.t.Fatal("FetchOpenClawRows called before gate rule validation") + return nil, nil +} + +func (client failingBenchmarkClient) FetchSkillTrustBenchRows(dataset string, split string, offset int, limit int) ([]SkillTrustBenchRow, error) { + client.t.Fatal("FetchSkillTrustBenchRows called before gate rule validation") + return nil, nil +} + +func (client failingBenchmarkClient) MaterializeSkillTrustBenchRow(root string, row SkillTrustBenchRow) (string, error) { + client.t.Fatal("MaterializeSkillTrustBenchRow called before gate rule validation") + return "", nil +} + func TestBenchmarkPredictionsPreferJudgeVerdict(t *testing.T) { predictions, err := BenchmarkPredictions(BenchmarkArtifact{ Benchmark: BenchmarkMetadata{ID: openClawBenchmarkID}, @@ -1223,6 +1288,29 @@ func TestRunBlocksWhenNonzeroExitCodeRuleFires(t *testing.T) { } } +func TestRunRepeatedScannerFlagFiresGateRuleOnce(t *testing.T) { + target := t.TempDir() + exitCode := 2 + opts := Options{ + // A repeated --scanner flag keeps duplicate IDs in Scanners but only + // one result exists per ID; the rule must not fire twice. + Target: target, Scanners: []string{"clawscan-static", "clawscan-static"}, Sandbox: SandboxOptions{Mode: SandboxModeOff}, + GateRules: map[string]ScannerGatePolicy{ + "clawscan-static": {BlockOnExitCode: &ExitCodeRule{Nonzero: true}}, + }, + } + artifact, err := Run(opts, RunContext{Env: map[string]string{}, ScannerRunner: &gateScannerRunner{ + results: map[string]ScannerResult{"clawscan-static": {Status: "completed", ExitCode: &exitCode}}, + }}) + if err != nil { + t.Fatal(err) + } + want := []FiredGateRule{{Scanner: "clawscan-static", Rule: "blockOnExitCode", ExitCode: 2, Action: "block"}} + if !reflect.DeepEqual(artifact.GateRules, want) { + t.Fatalf("gate rules = %#v, want single fired rule", artifact.GateRules) + } +} + func TestRunExitCodeGateActionsAndPrecedence(t *testing.T) { tests := []struct { name string @@ -1278,6 +1366,458 @@ func TestRunExitCodeGateActionsAndPrecedence(t *testing.T) { } } +func TestRunHostRedactionCoversSkippedScannersEnv(t *testing.T) { + // With --sandbox off every scanner inherits the whole host environment, + // including credentials of scanners skipped for this target kind. The + // redaction list must be built from all selected scanners, not just the + // target-runnable subset, or a plugin-only scanner's credential leaks + // into another scanner's raw artifact JSON. + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + pluginOnly := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "plugin-only", Command: "plugin-only {{target}}", Env: []string{"BETA_LICENSE"}, Targets: []string{"plugin"}, + }) + registry, err := NewScannerRegistry(alpha, pluginOnly) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: `{"token":"beta-cred-value"}`} + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"alpha", "plugin-only"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, RunContext{ + Env: map[string]string{"BETA_LICENSE": "beta-cred-value"}, + HostCommandRunner: host, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["alpha"].Raw) + if strings.Contains(raw, "beta-cred-value") { + t.Fatalf("skipped scanner's credential leaked into raw JSON: %s", raw) + } + if !strings.Contains(raw, "[redacted]") { + t.Fatalf("expected redaction marker: %s", raw) + } +} + +func TestRunHostRedactionCoversUnselectedRegistryScannersEnv(t *testing.T) { + // --scanner can select a subset of the resolved profile's scanners, but + // with --sandbox off the whole process env still reaches the selected + // ones. A blandly named credential declared by an unselected scanner in + // the same registry must still be scrubbed from persisted output. + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"ALPHA_ACCESS"}, Targets: []string{"skill"}, + }) + beta := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "beta", Command: "beta {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(alpha, beta) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: `{"token":"alpha-secret-value"}`} + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"beta"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, RunContext{ + Env: map[string]string{"ALPHA_ACCESS": "alpha-secret-value"}, + HostCommandRunner: host, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["beta"].Raw) + if strings.Contains(raw, "alpha-secret-value") { + t.Fatalf("unselected scanner's credential leaked into raw JSON: %s", raw) + } + if !strings.Contains(raw, "[redacted]") { + t.Fatalf("expected redaction marker: %s", raw) + } +} + +func TestRunRedactsDeclaredCredentialsFromFixtureResults(t *testing.T) { + // --scanner-result fixtures bypass RunScanner; a fixture captured from + // a live run can embed a currently present declared credential, which + // must not be copied verbatim into the artifact. + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "alpha.json") + if err := os.WriteFile(fixture, []byte(`{"token":"fixture-secret-value"}`), 0o600); err != nil { + t.Fatal(err) + } + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(alpha) + if err != nil { + t.Fatal(err) + } + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"alpha"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{"alpha": fixture}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, RunContext{ + Env: map[string]string{"SCANNER_ACCESS": "fixture-secret-value"}, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["alpha"].Raw) + if strings.Contains(raw, "fixture-secret-value") { + t.Fatalf("declared credential leaked through fixture evidence: %s", raw) + } + if !strings.Contains(raw, "[redacted]") { + t.Fatalf("expected redaction marker in fixture evidence: %s", raw) + } +} + +func TestRunScannerRedactsDeclaredCredentialsFromBuiltinAdapters(t *testing.T) { + // The redaction boundary is RunScanner, not each adapter: a profile + // mixing a user-defined scanner (declaring bland SCANNER_ACCESS) with a + // built-in scanner exposes the credential to both, and the built-in's + // stdout/stderr must be scrubbed too. + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(alpha) + if err != nil { + t.Fatal(err) + } + env := map[string]string{"SCANNER_ACCESS": "bland-secret-value"} + runner := ExternalScannerRunner{ + Registry: registry, + CommandRunner: &recordingCommandRunner{stdout: `{"echo":"bland-secret-value"}`}, + Env: env, SandboxMode: SandboxModeOff, + ExposedEnvNames: []string{"SCANNER_ACCESS"}, + } + result, err := runner.RunScanner("agentverus", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(result.Raw), "bland-secret-value") { + t.Fatalf("declared credential leaked through built-in adapter raw: %s", result.Raw) + } + failing := ExternalScannerRunner{ + Registry: registry, + CommandRunner: &recordingCommandRunner{stderr: "auth bland-secret-value rejected", err: errCommandFailed}, + Env: env, SandboxMode: SandboxModeOff, + ExposedEnvNames: []string{"SCANNER_ACCESS"}, + } + failed, err := failing.RunScanner("agentverus", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(failed.Error, "bland-secret-value") { + t.Fatalf("declared credential leaked through built-in adapter error: %q", failed.Error) + } +} + +func TestRedactionEnvNamesDockerIncludesPassedOptionalCredentials(t *testing.T) { + // The Docker allowlist passes populated credential-classified + // OptionalEnv (SKILL_SCANNER_LLM_API_KEY) into the container, so + // Docker-mode redaction must cover it. + env := map[string]string{"SKILL_SCANNER_LLM_API_KEY": "cisco-llm-secret"} + names := redactionEnvNames(Options{ + Scanners: []string{"cisco"}, ScannerResultPaths: map[string]string{}, + }, env, SandboxModeDocker) + found := false + for _, name := range names { + if name == "SKILL_SCANNER_LLM_API_KEY" { + found = true + } + } + if !found { + t.Fatalf("passed optional credential missing from Docker redaction set: %v", names) + } +} + +func TestRedactionEnvNamesDockerExcludesUnexposedSiblingCredentials(t *testing.T) { + // Under Docker an unselected scanner's declared credential never + // enters the container; scrubbing its value (ALPHA_ACCESS=clean) would + // rewrite legitimate "clean" verdicts without preventing a leak. + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"ALPHA_ACCESS"}, Targets: []string{"skill"}, + }) + beta := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "beta", Command: "beta {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(alpha, beta) + if err != nil { + t.Fatal(err) + } + opts := Options{ + Scanners: []string{"beta"}, ScannerRegistry: registry, + ScannerResultPaths: map[string]string{}, + BatchRedactEnvNames: []string{"SIBLING_ACCESS"}, + } + env := map[string]string{"ALPHA_ACCESS": "clean", "SIBLING_ACCESS": "sibling"} + docker := redactionEnvNames(opts, env, SandboxModeDocker) + for _, name := range docker { + if name == "ALPHA_ACCESS" || name == "SIBLING_ACCESS" { + t.Fatalf("unexposed credential %s in Docker redaction set: %v", name, docker) + } + } + host := redactionEnvNames(opts, env, SandboxModeOff) + hostSet := map[string]bool{} + for _, name := range host { + hostSet[name] = true + } + if !hostSet["ALPHA_ACCESS"] || !hostSet["SIBLING_ACCESS"] { + t.Fatalf("host redaction lost registry/batch coverage: %v", host) + } +} + +func TestRunDockerRedactionSkipsNonRunnableScannerCredentials(t *testing.T) { + // The Docker allowlist is built from runnable scanners only. A + // selected plugin-only scanner is skipped for a skill target, so its + // credential never enters the container and must not scrub another + // scanner's evidence — BETA_LICENSE=clean would otherwise rewrite a + // legitimate "verdict":"clean". + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + pluginOnly := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "plugin-only", Command: "plugin-only {{target}}", Env: []string{"BETA_LICENSE"}, Targets: []string{"plugin"}, + }) + registry, err := NewScannerRegistry(alpha, pluginOnly) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: `{"verdict":"clean"}`} + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"alpha", "plugin-only"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeDocker}, + }, RunContext{ + Env: map[string]string{"BETA_LICENSE": "clean"}, + HostCommandRunner: host, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["alpha"].Raw) + if !strings.Contains(raw, `"verdict":"clean"`) { + t.Fatalf("non-runnable scanner's credential corrupted Docker evidence: %s", raw) + } +} + +func TestUserDefinedScannerDockerErrorNotScrubbedByUnexposedHostSecrets(t *testing.T) { + // Failure diagnostics from a Docker run must not be rewritten by a + // host-only secret whose value overlaps ordinary stderr text. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stderr: "scanner binary not found", err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"CI_TOKEN": "not found"}, + SandboxMode: SandboxModeDocker, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(result.Error, "scanner binary not found") { + t.Fatalf("unexposed host secret corrupted Docker error text: %q", result.Error) + } +} + +func TestRunRedactionIgnoresNonCredentialSandboxEnv(t *testing.T) { + // The clawhub profile passes SKILLSPECTOR_PROVIDER through the sandbox + // allowlist; its common value ("openai") must not be scrubbed from + // evidence, while a blandly named sandbox-env credential still is. + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + beta := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "beta", Command: "beta {{target}}", Targets: []string{"skill"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(beta) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: `{"provider":"openai","auth":"judge-cred-value"}`} + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"beta"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeOff, Env: []string{"SKILLSPECTOR_PROVIDER", "JUDGE_ACCESS"}}, + }, RunContext{ + Env: map[string]string{"SKILLSPECTOR_PROVIDER": "openai", "JUDGE_ACCESS": "judge-cred-value"}, + HostCommandRunner: host, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["beta"].Raw) + if !strings.Contains(raw, `"provider":"openai"`) { + t.Fatalf("non-credential sandbox env value corrupted evidence: %s", raw) + } + if strings.Contains(raw, "judge-cred-value") { + t.Fatalf("sandbox-env credential leaked: %s", raw) + } +} + +func TestRunHostRedactionIgnoresNonCredentialOptionalEnv(t *testing.T) { + // Built-in scanners list ordinary configuration (LOG_LEVEL, model + // names, SSL toggles) in OptionalEnv. Their populated values must not + // feed redaction: LOG_LEVEL=info would rewrite every "info" string in + // valid scanner evidence. + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + beta := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "beta", Command: "beta {{target}}", Targets: []string{"skill"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(beta) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: `{"severity":"info","enabled":true}`} + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"beta"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, RunContext{ + Env: map[string]string{"LOG_LEVEL": "info", "SKILLSPECTOR_SSL_VERIFY": "true"}, + HostCommandRunner: host, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["beta"].Raw) + if strings.Contains(raw, "[redacted]") { + t.Fatalf("non-credential optional env value corrupted evidence: %s", raw) + } + if !strings.Contains(raw, `"severity":"info"`) { + t.Fatalf("evidence rewritten: %s", raw) + } +} + +func TestCredentialEnvNameWindowsFoldsOptionalEnvCase(t *testing.T) { + if credentialEnvNameOnGOOS("skillspector_provider", "windows") { + t.Fatal("Windows case variant of optional env was classified as a credential") + } + if !credentialEnvNameOnGOOS("skillspector_provider", "linux") { + t.Fatal("non-Windows case variant must remain fail-closed") + } + if credentialEnvNameOnGOOS("SKILLSPECTOR_PROVIDER", "linux") { + t.Fatal("exact optional env match was classified as a credential") + } + for _, goos := range []string{"windows", "linux"} { + if !credentialEnvNameOnGOOS("api_token", goos) { + t.Fatalf("secret-named env was not classified as a credential on %s", goos) + } + } +} + +func TestCredentialEnvName(t *testing.T) { + for name, want := range map[string]bool{ + "PASSWORD_STORE_ENABLE_EXTENSIONS": false, + "API_TOKEN": true, + } { + if got := CredentialEnvName(name); got != want { + t.Errorf("CredentialEnvName(%q) = %t, want %t", name, got, want) + } + } +} + +func TestRunProfileBatchRedactsSiblingProfileCredentials(t *testing.T) { + // A --config batch shares one host environment across profiles under + // --sandbox off, so profile B's scanner can emit the blandly named + // credential only profile A declared. Batch redaction must cover the + // union of declared credentials, not the current profile's alone. + dir := t.TempDir() + target := filepath.Join(dir, "skill") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + alpha := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"ALPHA_ACCESS"}, Targets: []string{"skill"}, + }) + alphaRegistry, err := NewScannerRegistry(alpha) + if err != nil { + t.Fatal(err) + } + beta := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "beta", Command: "beta {{target}}", Targets: []string{"skill"}, + }) + betaRegistry, err := NewScannerRegistry(beta) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: `{"token":"alpha-secret-value"}`} + batch, err := RunProfileBatch([]Options{ + { + Target: target, Profile: "profile-a", Scanners: []string{"alpha"}, + ScannerRegistry: alphaRegistry, ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, + { + Target: target, Profile: "profile-b", Scanners: []string{"beta"}, + ScannerRegistry: betaRegistry, ScannerResultPaths: map[string]string{}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, + }, RunContext{ + Env: map[string]string{"ALPHA_ACCESS": "alpha-secret-value"}, + HostCommandRunner: host, + }, dir) + if err != nil { + t.Fatal(err) + } + if len(batch.Errors) != 0 { + t.Fatalf("batch errors: %#v", batch.Errors) + } + for _, run := range batch.Runs { + for scanner, result := range run.Scanners { + raw := string(result.Raw) + if strings.Contains(raw, "alpha-secret-value") { + t.Fatalf("profile %s scanner %s leaked sibling profile's credential: %s", run.Profile, scanner, raw) + } + } + } +} + func TestRunBlockGateBeatsWarnAcrossScanners(t *testing.T) { target := t.TempDir() artifact, err := Run(Options{ @@ -3991,19 +4531,22 @@ func TestRunJudgeDoesNotPersistRenderedCommand(t *testing.T) { if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { t.Fatal(err) } + scannerRunner := staticScannerRunner{results: map[string]ScannerResult{ + "skillspector": {Status: "completed", Raw: json.RawMessage(`{"status":"clean"}`)}, + }} + // A credential-free judge command runs, and its rendered form is still + // never persisted into the artifact. opts, err := ParseArgs([]string{ target, "--scanner", "skillspector", - "--judge", "SECRET_TOKEN=supersecret printf '{\"ok\":true}\\n' > {{ output }}", + "--judge", "printf '{\"ok\":true}\\n' > {{ output }}", }) if err != nil { t.Fatal(err) } artifact, err := Run(opts, RunContext{ - Env: map[string]string{"OPENAI_API_KEY": "present"}, - ScannerRunner: staticScannerRunner{results: map[string]ScannerResult{ - "skillspector": {Status: "completed", Raw: json.RawMessage(`{"status":"clean"}`)}, - }}, + Env: map[string]string{"OPENAI_API_KEY": "present"}, + ScannerRunner: scannerRunner, }) if err != nil { t.Fatal(err) @@ -4012,7 +4555,7 @@ func TestRunJudgeDoesNotPersistRenderedCommand(t *testing.T) { if err != nil { t.Fatal(err) } - if bytes.Contains(raw, []byte("supersecret")) { + if bytes.Contains(raw, []byte("{{ output }}")) || bytes.Contains(raw, []byte("printf")) { t.Fatalf("artifact leaked rendered judge command: %s", raw) } if artifact.Judge == nil || artifact.Judge.Command != "" { @@ -4073,10 +4616,15 @@ func TestRunJudgeRedactsSecretEnvValuesFromFailedStdoutResult(t *testing.T) { if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { t.Fatal(err) } + // --sandbox off: the judge inherits the whole host env, so the + // secret-named sweep must cover undeclared vars like SNYK_TOKEN. (Under + // Docker the judge never sees unexposed host vars, and scrubbing their + // values would corrupt evidence that matches by coincidence.) opts, err := ParseArgs([]string{ target, "--scanner", "skillspector", "--judge", "judge", + "--sandbox", "off", }) if err != nil { t.Fatal(err) @@ -4106,6 +4654,258 @@ func TestRunJudgeRedactsSecretEnvValuesFromFailedStdoutResult(t *testing.T) { } } +func TestRunJudgeInjectedRunnerScrubsHostCredentialsDespiteDockerOptions(t *testing.T) { + // An injected RunContext.CommandRunner executes on the host even when + // options request Docker, so judge redaction must sweep the whole host + // env: an undeclared host credential in valid judge JSON would + // otherwise persist because only the container allowlist was scrubbed. + dir := t.TempDir() + t.Chdir(dir) + target := filepath.Join(dir, "skill") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { + t.Fatal(err) + } + artifact, err := Run(Options{ + Target: target, Scanners: []string{"clawscan-static"}, + Sandbox: SandboxOptions{Mode: SandboxModeDocker}, + Judge: &JudgeOptions{Command: "judge"}, + }, RunContext{ + Env: map[string]string{"SNYK_TOKEN": "host-judge-secret"}, + ScannerRunner: staticScannerRunner{results: map[string]ScannerResult{ + "clawscan-static": {Status: "completed", Raw: json.RawMessage(`{}`)}, + }}, + CommandRunner: &recordingCommandRunner{stdout: `{"note":"saw host-judge-secret"}`}, + }) + if err != nil { + t.Fatal(err) + } + raw, err := json.Marshal(artifact) + if err != nil { + t.Fatal(err) + } + if bytes.Contains(raw, []byte("host-judge-secret")) { + t.Fatalf("injected host runner leaked undeclared host credential: %s", raw) + } +} + +func TestRunJudgeDockerFailureDiagnosticsIgnoreUnexposedHostValues(t *testing.T) { + // Under Docker the judge never saw an unallowlisted host variable; + // commandError's secret-named sweep must not rewrite stderr diagnostics + // that coincide with such a value (DATABASE_URL=clean). + env := map[string]string{"DATABASE_URL": "clean"} + target := t.TempDir() + artifact := Artifact{Target: Target{Kind: "skill", ResolvedPath: target}} + result, err := RunJudge(JudgeOptions{Command: "judge"}, artifact, &recordingCommandRunner{ + stderr: "judge exited: verdict clean rejected", err: errCommandFailed, + }, time.Minute, env, SandboxModeDocker, SandboxModeDocker, nil) + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" { + t.Fatalf("status = %q", result.Status) + } + if !strings.Contains(result.Error, "verdict clean rejected") { + t.Fatalf("unexposed host value corrupted judge diagnostics: %q", result.Error) + } +} + +func TestRunJudgePreservesFieldsOnRedactedKeyCollision(t *testing.T) { + // A judge-result key containing a secret is renamed to the marker; a + // sibling key already spelled "[redacted]" must not be overwritten by + // that rename — both fields belong in the artifact. + dir := t.TempDir() + t.Chdir(dir) + target := filepath.Join(dir, "skill") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { + t.Fatal(err) + } + opts, err := ParseArgs([]string{ + target, + "--scanner", "skillspector", + "--judge", "judge", + "--sandbox", "off", + }) + if err != nil { + t.Fatal(err) + } + artifact, err := Run(opts, RunContext{ + Env: map[string]string{"OPENAI_API_KEY": "present", "SNYK_TOKEN": "abc"}, + ScannerRunner: staticScannerRunner{results: map[string]ScannerResult{ + "skillspector": {Status: "completed", Raw: json.RawMessage(`{"status":"clean"}`)}, + }}, + CommandRunner: &recordingCommandRunner{stdout: `{"abc":"x","[redacted]":"y"}`}, + }) + if err != nil { + t.Fatal(err) + } + if artifact.Judge == nil { + t.Fatal("judge missing") + } + result, ok := artifact.Judge.Result.(map[string]any) + if !ok { + t.Fatalf("judge result = %#v", artifact.Judge.Result) + } + if len(result) != 2 { + t.Fatalf("field dropped on redacted key collision: %#v", result) + } + raw, err := json.Marshal(artifact) + if err != nil { + t.Fatal(err) + } + if bytes.Contains(raw, []byte(`"abc"`)) { + t.Fatalf("secret key survived in judge result: %s", raw) + } +} + +func TestRunJudgeRedactsDeclaredScannerEnvFromResult(t *testing.T) { + // The judge shares the command runner (and thus the env allowlist) with + // scanners. A custom scanner's declared credential whose name evades + // isSecretEnvKey (SCANNER_ACCESS) must still be scrubbed from judge output. + dir := t.TempDir() + t.Chdir(dir) + target := filepath.Join(dir, "skill") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { + t.Fatal(err) + } + custom := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "custom", Command: "custom {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(custom) + if err != nil { + t.Fatal(err) + } + artifact, err := Run(Options{ + Target: target, Scanners: []string{"custom"}, ScannerRegistry: registry, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + Judge: &JudgeOptions{Command: "judge"}, + }, RunContext{ + Env: map[string]string{"SCANNER_ACCESS": "bland-name-credential"}, + ScannerRunner: staticScannerRunner{results: map[string]ScannerResult{ + "custom": {Status: "completed", Raw: json.RawMessage(`{"status":"clean"}`)}, + }}, + CommandRunner: &recordingCommandRunner{stdout: `{"verdict":"clean","note":"auth was bland-name-credential"}`}, + }) + if err != nil { + t.Fatal(err) + } + raw, err := json.Marshal(artifact) + if err != nil { + t.Fatal(err) + } + if bytes.Contains(raw, []byte("bland-name-credential")) { + t.Fatalf("artifact leaked declared scanner credential via judge: %s", raw) + } + result, ok := artifact.Judge.Result.(map[string]any) + if !ok || result["note"] != "auth was [redacted]" { + t.Fatalf("judge result not scrubbed: %#v", artifact.Judge.Result) + } +} + +func TestRunJudgeRedactsNumericDeclaredCredentialScalar(t *testing.T) { + // A numeric declared credential (SCANNER_ACCESS=1234) emitted by the judge + // as a JSON number ({"auth":1234}) never hits the string walk; structural + // scalar comparison must catch it, as the scanner-output path does. + dir := t.TempDir() + t.Chdir(dir) + target := filepath.Join(dir, "skill") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { + t.Fatal(err) + } + custom := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "custom", Command: "custom {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(custom) + if err != nil { + t.Fatal(err) + } + artifact, err := Run(Options{ + Target: target, Scanners: []string{"custom"}, ScannerRegistry: registry, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + Judge: &JudgeOptions{Command: "judge"}, + }, RunContext{ + Env: map[string]string{"SCANNER_ACCESS": "1234"}, + ScannerRunner: staticScannerRunner{results: map[string]ScannerResult{ + "custom": {Status: "completed", Raw: json.RawMessage(`{"status":"clean"}`)}, + }}, + CommandRunner: &recordingCommandRunner{stdout: `{"verdict":"clean","auth":1234,"count":12345}`}, + }) + if err != nil { + t.Fatal(err) + } + result, ok := artifact.Judge.Result.(map[string]any) + if !ok { + t.Fatalf("judge result = %#v", artifact.Judge.Result) + } + if result["auth"] != "[redacted]" { + t.Fatalf("numeric declared credential not redacted: %#v", result) + } + // Exact-match only: 12345 merely contains the digits and must survive. + if fmt.Sprintf("%v", result["count"]) != "12345" { + t.Fatalf("non-matching number corrupted: %#v", result) + } +} + +func TestRunFixtureScannerEnvStillRedactedOnHost(t *testing.T) { + // --scanner-result satisfies a scanner from a fixture, but with + // --sandbox off its credential is still in the host environment and + // reachable by every other command; it must stay in the redaction set. + dir := t.TempDir() + t.Chdir(dir) + target := filepath.Join(dir, "skill") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "fixture.json") + if err := os.WriteFile(fixture, []byte(`{"status":"clean"}`), 0o644); err != nil { + t.Fatal(err) + } + fixtureScanner := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "fixture-scanner", Command: "fixture {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + live := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "live-scanner", Command: "live {{target}}", Targets: []string{"skill"}, + }) + registry, err := DefaultScannerRegistry().WithAdapters(fixtureScanner, live) + if err != nil { + t.Fatal(err) + } + artifact, err := Run(Options{ + Target: target, + Scanners: []string{"fixture-scanner", "live-scanner"}, + ScannerRegistry: registry, + ScannerResultPaths: map[string]string{"fixture-scanner": fixture}, + Sandbox: SandboxOptions{Mode: SandboxModeOff}, + }, RunContext{ + Env: map[string]string{"SCANNER_ACCESS": "fixture-cred-value"}, + HostCommandRunner: &recordingCommandRunner{stdout: `{"token":"fixture-cred-value"}`}, + }) + if err != nil { + t.Fatal(err) + } + raw := string(artifact.Scanners["live-scanner"].Raw) + if strings.Contains(raw, "fixture-cred-value") { + t.Fatalf("fixture scanner's credential leaked through live scanner: %s", raw) + } + if !strings.Contains(raw, "[redacted]") { + t.Fatalf("expected redaction marker: %s", raw) + } +} + func TestRunJudgeRedactsSecretEnvValuesFromJSONKeys(t *testing.T) { dir := t.TempDir() t.Chdir(dir) @@ -4116,10 +4916,13 @@ func TestRunJudgeRedactsSecretEnvValuesFromJSONKeys(t *testing.T) { if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("# Demo"), 0o644); err != nil { t.Fatal(err) } + // --sandbox off so the whole-host secret-named sweep applies (Docker + // runs only scrub env the container can actually see). opts, err := ParseArgs([]string{ target, "--scanner", "skillspector", "--judge", "judge", + "--sandbox", "off", }) if err != nil { t.Fatal(err) @@ -4215,6 +5018,25 @@ func TestJudgeShellForGOOSUsesPlatformShell(t *testing.T) { } } +func TestWindowsCmdQuoteDoublesTrailingBackslashes(t *testing.T) { + // CommandLineToArgvW treats a backslash before a quote as an escape: + // "C:\" hands the callee `C:"` glued to the next token. Trailing + // backslash runs must be doubled before the closing quote, and runs + // before an embedded quote doubled plus one for the escaped quote. + for input, want := range map[string]string{ + ``: `""`, + `C:\skills\demo`: `"C:\skills\demo"`, + `C:\`: `"C:\\"`, + `C:\dir\\`: `"C:\dir\\\\"`, + `say "hi"`: `"say \"hi\""`, + `end\"q`: `"end\\\"q"`, + } { + if got := windowsCmdQuote(input); got != want { + t.Fatalf("windowsCmdQuote(%q) = %q, want %q", input, got, want) + } + } +} + type skippedScannerRunner struct{} func (skippedScannerRunner) RunScanner(name string, target string, startedAt string) (ScannerResult, error) { diff --git a/internal/runner/sandbox.go b/internal/runner/sandbox.go index 13e088f..0ca6cc9 100644 --- a/internal/runner/sandbox.go +++ b/internal/runner/sandbox.go @@ -1,10 +1,13 @@ package runner import ( + "crypto/rand" + "encoding/hex" "fmt" "os" "os/exec" "path/filepath" + "runtime" "sort" "strings" "time" @@ -40,6 +43,8 @@ type dockerCommandRunner struct { Env map[string]string Image string EnvNames []string + // GOOS overrides runtime.GOOS in tests; empty means the real host OS. + GOOS string } func resolveSandbox(opts Options, env map[string]string) (resolvedSandbox, error) { @@ -153,13 +158,106 @@ func dockerAvailable() error { } func (runner dockerCommandRunner) Run(command string, args []string, cwd string, timeout time.Duration) (CommandOutput, error) { - dockerArgs := []string{"run", "--rm", "--network", "bridge"} + // Name the container so a timeout can kill it: process-group + // cancellation only reaches the docker CLI client, while the container + // itself is owned by the daemon and would keep running — with + // allowlisted credentials and network access — after ClawScan reports + // failure. + containerName := clawscanContainerName() + dockerArgs := []string{"run", "--rm", "--name", containerName, "--network", "bridge"} + goos := runner.GOOS + if goos == "" { + goos = runtime.GOOS + } + seenEnvKeys := map[string]bool{} for _, name := range runner.EnvNames { - if strings.TrimSpace(runner.Env[name]) != "" { - dockerArgs = append(dockerArgs, "-e", name) + entries := envEntriesForNameOnGOOS(runner.Env, name, goos) + keys := make([]string, 0, len(entries)) + for key := range entries { + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + if strings.TrimSpace(entries[key]) == "" || seenEnvKeys[key] { + continue + } + seenEnvKeys[key] = true + dockerArgs = append(dockerArgs, "-e", key) + } + } + // The scan target must never use the writable-parent fallback below: + // if it disappears between the scanner's own existence check and this + // resolution (rename, deletion), falling back would bind the + // surrounding host directory read-write into the container. Fail + // closed instead — and refuse to start the container at all: with no + // mount, image content at the same absolute path would be scanned and + // reported as the host target, producing evidence for the wrong + // subject. + target := positionalScannerTarget(command, args) + // Only absolute local paths participate in mount pinning and the vanished- + // target post-condition: a URL (or other non-path) target gets no bind + // mount by design, and dockerMounts only ever mounts absolute paths. + if target != "" && !filepath.IsAbs(filepath.Clean(target)) { + target = "" + } + if target != "" { + clean := filepath.Clean(target) + // EvalSymlinks pins the physical path: mounting the resolved + // path means a symlink swapped in after this check redirects + // nothing — the bind source is already the real directory. A + // resolution failure is the vanished-target case. + resolved, err := filepath.EvalSymlinks(clean) + if err != nil { + return CommandOutput{}, fmt.Errorf("scan target vanished before sandbox start: %s", target) + } + if resolved != target { + args = rewritePositionalScannerTarget(args, target, resolved) + target = resolved + } + } + // Inference must run on the post-rewrite args: computing it earlier + // would leave the pre-resolution target spelling in the list, where the + // filter below cannot match it and it would gain its own mount. + inference := mountInferenceArgs(command, args) + if target != "" { + filtered := inference[:0:0] + for _, arg := range inference { + if arg != target { + filtered = append(filtered, arg) + } + } + inference = filtered + } + // A Windows host path (C:\skills\demo) is not absolute inside the Linux + // runtime image: Docker would reject it as a mount destination and the + // scanner could never resolve it. Mount the host source at a stable + // POSIX path and hand the scanner that path instead. + if target != "" && goos == "windows" { + if _, err := os.Stat(target); err != nil { + return CommandOutput{}, fmt.Errorf("scan target vanished before sandbox start: %s", target) } + dockerArgs = append(dockerArgs, "--mount", "type=bind,"+dockerMountField("source", target)+",target="+windowsScanTargetContainerPath+",readonly") + args = rewritePositionalScannerTarget(args, target, windowsScanTargetContainerPath) + target = "" } - for _, mount := range dockerMounts(cwd, args) { + mounts := dockerMounts(cwd, inference, target) + // dockerMounts stats again; if the target vanished in the window since + // the resolution above, it silently omits the mount and the container + // would scan image content at the same absolute path. Verify the + // target's mount actually made it into the list. + if target != "" { + mounted := false + for _, mount := range mounts { + if strings.Contains(mount, dockerMountField("source", filepath.Clean(target))+",") { + mounted = true + break + } + } + if !mounted { + return CommandOutput{}, fmt.Errorf("scan target vanished before sandbox start: %s", target) + } + } + for _, mount := range mounts { dockerArgs = append(dockerArgs, "--mount", mount) } if cwd != "" { @@ -167,10 +265,82 @@ func (runner dockerCommandRunner) Run(command string, args []string, cwd string, } dockerArgs = append(dockerArgs, runner.Image, command) dockerArgs = append(dockerArgs, args...) - return runner.Host.Run("docker", dockerArgs, "", timeout) + output, err := runner.Host.Run("docker", dockerArgs, "", timeout) + if err != nil { + // The docker client died (timeout kill, crash) but the daemon may + // still be running the container. Best-effort kill by name; if the + // container already exited, `docker kill` is a harmless error. + _, _ = runner.Host.Run("docker", []string{"kill", containerName}, "", 30*time.Second) + } + return output, err } -func dockerMounts(cwd string, args []string) []string { +// clawscanContainerName returns a unique name for a sandbox container so +// cleanup can address it through the daemon. Collisions only waste one run +// (docker refuses the duplicate name), never kill someone else's container: +// the random suffix makes reuse across concurrent runs vanishingly unlikely. +func clawscanContainerName() string { + suffix := make([]byte, 8) + if _, err := rand.Read(suffix); err != nil { + return fmt.Sprintf("clawscan-scan-%d", time.Now().UnixNano()) + } + return "clawscan-scan-" + hex.EncodeToString(suffix) +} + +// mountInferenceArgs drops the rendered shell program from mount inference: +// for `/bin/sh -c '' [positional args...]` the program is one +// absolute-looking string ("/usr/bin/scanner \"$1\"") that never stats, and +// inferring a parent from it would bind-mount /usr/bin writable into the +// container. Only the -c operand is excluded; every other arg (including +// not-yet-created output paths with spaces) keeps its mount. +func mountInferenceArgs(command string, args []string) []string { + if command != "/bin/sh" { + return args + } + for index, arg := range args { + if arg == "-c" && index+1 < len(args) { + filtered := append([]string(nil), args[:index+1]...) + return append(filtered, args[index+2:]...) + } + } + return args +} + +// positionalScannerTarget extracts the scan target from a user-defined +// scanner invocation (`/bin/sh -c '' clawscan-target `). +// Empty for every other command shape. +func positionalScannerTarget(command string, args []string) string { + if command != "/bin/sh" { + return "" + } + for index, arg := range args { + if arg == "clawscan-target" && index+1 < len(args) { + return args[index+1] + } + } + return "" +} + +// windowsScanTargetContainerPath is where a Windows host scan target is +// bind-mounted inside the Linux runtime image. +const windowsScanTargetContainerPath = "/clawscan/target" + +func rewritePositionalScannerTarget(args []string, from string, to string) []string { + rewritten := append([]string(nil), args...) + for index, arg := range rewritten { + if arg == "clawscan-target" && index+1 < len(rewritten) && rewritten[index+1] == from { + rewritten[index+1] = to + } + } + return rewritten +} + +// dockerMounts infers bind mounts from cwd and command args. Existing paths +// mount directly (readonly for args); a missing arg is assumed to be a +// not-yet-created output file, so its parent mounts writable. scanTargets +// are exempt from that fallback: a scan target that no longer exists must +// not expose its parent directory writable to the container. +func dockerMounts(cwd string, args []string, scanTargets ...string) []string { mounts := map[string]bool{} add := func(path string, readOnly bool) { if path == "" { @@ -193,6 +363,19 @@ func dockerMounts(cwd string, args []string) []string { for _, arg := range args { add(arg, true) } + for _, target := range scanTargets { + if target == "" { + continue + } + clean := filepath.Clean(target) + if !filepath.IsAbs(clean) { + continue + } + // No writable-parent fallback: a missing scan target mounts nothing. + if _, err := os.Stat(clean); err == nil { + mounts[clean] = true + } + } sources := make([]string, 0, len(mounts)) for source := range mounts { sources = append(sources, source) @@ -200,7 +383,7 @@ func dockerMounts(cwd string, args []string) []string { sort.Strings(sources) out := make([]string, 0, len(sources)) for _, source := range sources { - option := "type=bind,source=" + source + ",target=" + source + option := "type=bind," + dockerMountField("source", source) + "," + dockerMountField("target", source) if mounts[source] { option += ",readonly" } @@ -209,7 +392,99 @@ func dockerMounts(cwd string, args []string) []string { return out } +// dockerMountField renders one key=value field of a --mount spec. Docker +// parses the spec as CSV, so a path containing a comma, quote, or newline +// (all valid in POSIX path components) must be CSV-quoted or Docker reads +// the remainder as another field/record and rejects or truncates the +// mount. +func dockerMountField(key string, value string) string { + field := key + "=" + value + if strings.ContainsAny(field, ",\"\r\n") { + return `"` + strings.ReplaceAll(field, `"`, `""`) + `"` + } + return field +} + +// sandboxEnvNames lists env vars passed through to the Docker sandbox. +// Fixture-satisfied scanners are skipped: they never execute, so their +// credentials must not enter the container. func sandboxEnvNames(opts Options, env map[string]string) []string { + return collectEnvNames(opts, env, false) +} + +// redactionEnvNames lists env vars whose values must be scrubbed from +// anything persisted this run, scoped to what the run's commands can +// actually see. With --sandbox off every host command inherits the whole +// process environment, so the sweep covers every credential the resolved +// registry declares (selected or not, fixture or not) plus sibling batch +// profiles' declared credentials (BatchRedactEnvNames). Under Docker only +// the passthrough allowlist enters the container, so the sweep is that +// allowlist — including populated credential-classified OptionalEnv, which +// the container does receive — and nothing more: scrubbing a never-exposed +// sibling credential whose value coincides with legitimate output would +// corrupt evidence without preventing any leak. +func redactionEnvNames(opts Options, env map[string]string, sandboxMode string) []string { + hostMode := sandboxMode != SandboxModeDocker + collected := collectEnvNames(opts, env, hostMode) + // Names mix credentials with ordinary configuration + // (SKILLSPECTOR_PROVIDER=openai): scrubbing a common value like + // "openai" would corrupt valid evidence. Keep only names classified as + // credentials — except explicit user-defined env: declarations, which + // are credentials whatever their name. + declared := declaredCredentialEnvNames(opts) + names := collected[:0] + seen := map[string]bool{} + for _, name := range collected { + if declared[name] || CredentialEnvName(name) { + names = append(names, name) + seen[name] = true + } + } + // BatchRedactEnvNames arrive pre-vetted by the resolver (sibling + // profiles' env: declarations plus credential-classified sandbox + // names), so they bypass the filter above. Host mode only: a sibling + // profile's credential never enters this profile's container. + if hostMode { + for _, name := range opts.BatchRedactEnvNames { + name = strings.TrimSpace(name) + if name != "" && !seen[name] { + seen[name] = true + names = append(names, name) + } + } + } + sort.Strings(names) + return names +} + +func declaredCredentialEnvNames(opts Options) map[string]bool { + type credentialDeclarer interface { + DeclaredCredentialEnv() []string + } + declared := map[string]bool{} + registry := registryForOptions(opts) + for _, id := range registry.IDs() { + adapter, ok := registry.Adapter(id) + if !ok { + continue + } + if declarer, ok := adapter.(credentialDeclarer); ok { + for _, name := range declarer.DeclaredCredentialEnv() { + name = strings.TrimSpace(name) + if name != "" { + declared[name] = true + } + } + } + } + return declared +} + +// collectEnvNames gathers env var names in scope for a run. wholeRegistry +// widens the sweep to every adapter's declared credentials for host-mode +// redaction; false yields the executing-scanner passthrough set used both +// for the Docker allowlist and Docker-scoped redaction. +func collectEnvNames(opts Options, env map[string]string, wholeRegistry bool) []string { seen := map[string]bool{} add := func(name string) { name = strings.TrimSpace(name) @@ -217,28 +492,63 @@ func sandboxEnvNames(opts Options, env map[string]string) []string { seen[name] = true } } + addInfo := func(info ScannerInfo) { + for _, name := range info.RequiredEnv { + add(name) + } + for _, name := range info.OptionalEnv { + if envValueForName(env, name) != "" { + add(name) + } + } + } for _, name := range opts.Sandbox.Env { add(name) } for _, req := range requirements(opts, env) { add(req.EnvVar) } - for _, scanner := range opts.Scanners { - if opts.ScannerResultPaths[scanner] != "" { - continue + if wholeRegistry { + // Host-mode redaction must cover every credential the resolved + // registry declares, not just selected scanners: with --sandbox off + // the whole process env reaches every host command, so a blandly + // named credential declared by an unselected profile scanner + // (--scanner subset) is still reachable by the scanners and judge + // that do run. + // Only credentials-by-declaration feed this sweep — scanner + // OptionalEnv is ordinary configuration (LOG_LEVEL, AWS_REGION) + // whose common values (info, true) would corrupt valid evidence if + // scrubbed. Built-in credentials are secret-named and caught by + // isSecretEnvKey value scanning; user-defined scanners declare + // theirs via env:, exposed here as DeclaredCredentialEnv. + type credentialDeclarer interface { + DeclaredCredentialEnv() []string } - adapter, ok := registryForOptions(opts).Adapter(scanner) - if !ok { - continue - } - info := adapter.Info() - for _, name := range info.RequiredEnv { - add(name) + registry := registryForOptions(opts) + for _, id := range registry.IDs() { + adapter, ok := registry.Adapter(id) + if !ok { + continue + } + if declarer, ok := adapter.(credentialDeclarer); ok { + for _, name := range declarer.DeclaredCredentialEnv() { + add(name) + } + } } - for _, name := range info.OptionalEnv { - if strings.TrimSpace(env[name]) != "" { - add(name) + } else { + // Sandbox passthrough stays a narrow allowlist: only scanners that + // will actually execute; fixture-satisfied scanners never run, so + // their credentials must not enter the container. + for _, scanner := range opts.Scanners { + if opts.ScannerResultPaths[scanner] != "" { + continue + } + adapter, ok := registryForOptions(opts).Adapter(scanner) + if !ok { + continue } + addInfo(adapter.Info()) } } names := make([]string, 0, len(seen)) diff --git a/internal/runner/sandbox_test.go b/internal/runner/sandbox_test.go new file mode 100644 index 0000000..b63a30f --- /dev/null +++ b/internal/runner/sandbox_test.go @@ -0,0 +1,363 @@ +package runner + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestMountInferenceArgsExcludesRenderedShellProgram(t *testing.T) { + // A user-defined scanner reaches docker as: /bin/sh -c '' + // clawscan-target . The rendered program starts with an absolute + // executable path but is not a mountable path; inferring a mount from it + // would bind /usr/bin (or similar) writable into the container. + program := `/usr/bin/scanner --json "$1"` + target := t.TempDir() + mounts := dockerMounts("", mountInferenceArgs("/bin/sh", []string{"-c", program, "clawscan-target", target})) + for _, mount := range mounts { + if strings.Contains(mount, "/usr/bin") { + t.Fatalf("rendered shell program produced a host mount: %q", mount) + } + } + found := false + for _, mount := range mounts { + if strings.Contains(mount, "source="+target+",") && strings.HasSuffix(mount, ",readonly") { + found = true + } + } + if !found { + t.Fatalf("target %q should still be mounted readonly; mounts = %v", target, mounts) + } +} + +func TestMountInferenceArgsKeepsNonShellArgs(t *testing.T) { + args := []string{"scan", "/data/skill", "--output", "/data/out.json"} + got := mountInferenceArgs("cisco-skill-scanner", args) + if strings.Join(got, " ") != strings.Join(args, " ") { + t.Fatalf("non-shell args changed: %v", got) + } +} + +func TestDockerMountsNeverMountsParentOfMissingScanTarget(t *testing.T) { + // TOCTOU guard: if the scan target vanishes between the scanner's own + // existence check and mount time, the writable-parent fallback would + // bind the surrounding host directory read-write into the container. + // Scan targets must fail closed: missing means no mount at all. + parent := t.TempDir() + missing := filepath.Join(parent, "vanished-skill") + mounts := dockerMounts("", nil, missing) + if len(mounts) != 0 { + t.Fatalf("missing scan target produced mounts: %v", mounts) + } +} + +func TestDockerMountsMountsExistingScanTargetReadonly(t *testing.T) { + target := t.TempDir() + mounts := dockerMounts("", nil, target) + if len(mounts) != 1 || !strings.Contains(mounts[0], "source="+target+",") || !strings.HasSuffix(mounts[0], ",readonly") { + t.Fatalf("existing scan target should mount readonly: %v", mounts) + } +} + +func TestDockerMountsQuotesCommaPaths(t *testing.T) { + // Docker parses --mount as CSV: an unquoted comma in a path splits the + // spec into a bogus extra field and Docker rejects the invocation. + parent := t.TempDir() + target := filepath.Join(parent, "skills, reviewed") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } + mounts := dockerMounts("", nil, target) + if len(mounts) != 1 { + t.Fatalf("mounts = %v", mounts) + } + want := `type=bind,"source=` + target + `","target=` + target + `",readonly` + if mounts[0] != want { + t.Fatalf("mount = %q, want %q", mounts[0], want) + } +} + +func TestDockerMountFieldQuoting(t *testing.T) { + for _, test := range []struct{ key, value, want string }{ + {"source", "/skills/demo", "source=/skills/demo"}, + {"source", "/skills/a,b", `"source=/skills/a,b"`}, + {"source", `/skills/say "hi"`, `"source=/skills/say ""hi"""`}, + // CR and LF are valid in POSIX path components and terminate a CSV + // record when unquoted, truncating the mount spec. + {"source", "/skills/a\nb", "\"source=/skills/a\nb\""}, + {"source", "/skills/a\rb", "\"source=/skills/a\rb\""}, + } { + if got := dockerMountField(test.key, test.value); got != test.want { + t.Fatalf("dockerMountField(%q, %q) = %q, want %q", test.key, test.value, got, test.want) + } + } +} + +func TestPositionalScannerTargetExtraction(t *testing.T) { + target := "/skills/demo" + if got := positionalScannerTarget("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", target}); got != target { + t.Fatalf("got %q, want %q", got, target) + } + if got := positionalScannerTarget("/bin/sh", []string{"-c", "judge-prompt"}); got != "" { + t.Fatalf("judge-style invocation must have no positional target, got %q", got) + } + if got := positionalScannerTarget("scanner", []string{"clawscan-target", target}); got != "" { + t.Fatalf("non-shell command must have no positional target, got %q", got) + } +} + +func TestDockerRunFailsClosedWhenScannerTargetVanishes(t *testing.T) { + // A target that vanished between the adapter's existence check and + // sandbox start must abort before the container runs: with no mount, + // image content at the same absolute path would be scanned and + // reported as the host target. + parent := t.TempDir() + missing := filepath.Join(parent, "vanished-skill") + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{Host: host, Env: map[string]string{}, Image: DefaultSandboxImage} + _, err := runner.Run("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", missing}, "", time.Minute) + if err == nil || !strings.Contains(err.Error(), "scan target vanished") { + t.Fatalf("err = %v, want vanished-target refusal", err) + } + if len(host.calls) != 0 { + t.Fatalf("container ran despite vanished target: %#v", host.calls) + } +} + +func TestDockerRunURLTargetSkipsMountVerification(t *testing.T) { + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{Host: host, Env: map[string]string{}, Image: DefaultSandboxImage} + url := "https://example.com/skill" + if _, err := runner.Run("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", url}, "", time.Minute); err != nil { + t.Fatal(err) + } + if len(host.calls) != 1 { + t.Fatalf("calls = %d, want 1", len(host.calls)) + } + args := host.calls[0].args + if args[len(args)-1] != url { + t.Fatalf("URL target rewritten: %#v", args) + } + for _, arg := range args { + if strings.HasPrefix(arg, "type=bind,") && strings.Contains(arg, "example.com") { + t.Fatalf("URL target produced a mount: %#v", args) + } + } +} + +func TestDockerRunWindowsURLTargetSkipsMountTranslation(t *testing.T) { + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{Host: host, Env: map[string]string{}, Image: DefaultSandboxImage, GOOS: "windows"} + url := "https://example.com/skill" + if _, err := runner.Run("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", url}, "", time.Minute); err != nil { + t.Fatal(err) + } + if len(host.calls) != 1 { + t.Fatalf("calls = %d, want 1", len(host.calls)) + } + args := host.calls[0].args + if args[len(args)-1] != url { + t.Fatalf("Windows URL target rewritten: %#v", args) + } + for _, arg := range args { + if strings.HasPrefix(arg, "type=bind,") && strings.Contains(arg, "example.com") { + t.Fatalf("Windows URL target produced a mount: %#v", args) + } + } +} + +func TestDockerRunPinsSymlinkTargetToPhysicalPath(t *testing.T) { + // A symlink target is resolved to its physical path before mounting, + // so a link swapped after validation redirects nothing — the bind + // source is already the real directory — and the scanner is handed the + // resolved path. + real, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + link := filepath.Join(t.TempDir(), "link-to-skill") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{Host: host, Env: map[string]string{}, Image: DefaultSandboxImage} + if _, err := runner.Run("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", link}, "", time.Minute); err != nil { + t.Fatal(err) + } + if len(host.calls) != 1 { + t.Fatalf("calls = %d", len(host.calls)) + } + args := host.calls[0].args + joined := strings.Join(args, "\x00") + if strings.Contains(joined, "source="+link+",") { + t.Fatalf("symlink spelling used as mount source: %#v", args) + } + if !strings.Contains(joined, "source="+real+",") { + t.Fatalf("physical path not mounted: %#v", args) + } + if args[len(args)-1] != real { + t.Fatalf("positional target not rewritten to physical path: %#v", args) + } +} + +func TestDockerRunTranslatesWindowsTargetToContainerPath(t *testing.T) { + // A Windows host path is not absolute inside the Linux runtime image; + // the target must be mounted at a stable POSIX path and the scanner + // handed that path instead. Use a real (POSIX) path with GOOS forced to + // windows so the stat succeeds while exercising the translation branch. + // Pre-resolve symlinks (macOS /var -> /private/var) so the runner's + // physical-path pinning does not change the spelling under test. + target, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{Host: host, Env: map[string]string{}, Image: DefaultSandboxImage, GOOS: "windows"} + if _, err := runner.Run("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", target}, "", time.Minute); err != nil { + t.Fatal(err) + } + if len(host.calls) != 1 { + t.Fatalf("calls = %d", len(host.calls)) + } + args := host.calls[0].args + joined := strings.Join(args, "\x00") + if !strings.Contains(joined, "type=bind,source="+target+",target="+windowsScanTargetContainerPath+",readonly") { + t.Fatalf("windows target not mounted at container path: %#v", args) + } + if args[len(args)-1] != windowsScanTargetContainerPath { + t.Fatalf("positional target not rewritten to container path: %#v", args) + } + if strings.Contains(joined, "target="+target+",") { + t.Fatalf("host path leaked as a mount destination: %#v", args) + } +} + +func TestDockerRunWindowsMissingTargetFailsClosed(t *testing.T) { + missing := filepath.Join(t.TempDir(), "vanished-skill") + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{Host: host, Env: map[string]string{}, Image: DefaultSandboxImage, GOOS: "windows"} + _, err := runner.Run("/bin/sh", []string{"-c", `scan "$1"`, "clawscan-target", missing}, "", time.Minute) + if err == nil || !strings.Contains(err.Error(), "scan target vanished") { + t.Fatalf("err = %v, want vanished-target refusal", err) + } + if len(host.calls) != 0 { + t.Fatalf("container ran despite vanished target: %#v", host.calls) + } +} + +func TestDockerRunWindowsEnvPassthroughIsCaseInsensitive(t *testing.T) { + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{ + Host: host, Env: map[string]string{"SCANNER_ACCESS": "secret"}, + Image: DefaultSandboxImage, GOOS: "windows", EnvNames: []string{"scanner_access"}, + } + if _, err := runner.Run("scanner", nil, "", time.Minute); err != nil { + t.Fatal(err) + } + if len(host.calls) != 1 { + t.Fatalf("calls = %d, want 1", len(host.calls)) + } + joined := strings.Join(host.calls[0].args, "\x00") + if !strings.Contains(joined, "-e\x00SCANNER_ACCESS") { + t.Fatalf("real Windows env key not passed through: %#v", host.calls[0].args) + } + if strings.Contains(joined, "-e\x00scanner_access") { + t.Fatalf("declared rather than real env key passed through: %#v", host.calls[0].args) + } + if strings.Contains(joined, "secret") { + t.Fatalf("credential value leaked into Docker args: %#v", host.calls[0].args) + } +} + +func TestDockerRunNonWindowsEnvPassthroughRemainsExact(t *testing.T) { + host := &recordingCommandRunner{stdout: "{}"} + runner := dockerCommandRunner{ + Host: host, Env: map[string]string{"SCANNER_ACCESS": "secret"}, + Image: DefaultSandboxImage, EnvNames: []string{"scanner_access"}, + } + if _, err := runner.Run("scanner", nil, "", time.Minute); err != nil { + t.Fatal(err) + } + if len(host.calls) != 1 { + t.Fatalf("calls = %d, want 1", len(host.calls)) + } + joined := strings.Join(host.calls[0].args, "\x00") + if strings.Contains(joined, "-e\x00") { + t.Fatalf("case-mismatched env key passed through on non-Windows host: %#v", host.calls[0].args) + } + if strings.Contains(joined, "secret") { + t.Fatalf("credential value leaked into Docker args: %#v", host.calls[0].args) + } +} + +func TestDefaultCommandRunnerSuppressesExitCodeOnTimeout(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + // A timed-out process has no exit verdict. Windows TerminateProcess + // reports exit code 1 — indistinguishable from findings-mean-nonzero — + // so the runner must not record any code when the deadline fired. + // exec so the sleep is the killed process itself; a forked child would + // hold the stdout pipe open for the full 5s after the kill. + output, err := defaultCommandRunner{}.Run("/bin/sh", []string{"-c", "echo '{}'; exec sleep 5"}, "", 150*time.Millisecond) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("err = %v", err) + } + if output.ExitCode != nil { + t.Fatalf("timed-out command recorded exit code %d; partial output could pass as a completed scan", *output.ExitCode) + } +} + +func TestDefaultCommandRunnerTimeoutKillsProcessTree(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + // A forked grandchild (`sleep 30 &`) inherits the stdout pipe. Killing + // only the direct shell would leave the pipe open and block Run for + // the grandchild's full lifetime; the process-group kill must reach it. + start := time.Now() + _, err := defaultCommandRunner{}.Run("/bin/sh", []string{"-c", "echo '{}'; sleep 30 & wait"}, "", 150*time.Millisecond) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("err = %v", err) + } + // Must beat the 10s WaitDelay backstop: a group kill that missed the + // grandchild would only return once WaitDelay force-closes the pipes. + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("timed-out command with forked child blocked for %s; process tree was not killed", elapsed) + } +} + +func TestDefaultCommandRunnerRecordsNormalExitCodes(t *testing.T) { + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("needs /bin/sh") + } + output, err := defaultCommandRunner{}.Run("/bin/sh", []string{"-c", "exit 3"}, "", time.Minute) + if err == nil { + t.Fatal("expected exit error") + } + if output.ExitCode == nil || *output.ExitCode != 3 { + t.Fatalf("exit code = %v, want 3", output.ExitCode) + } +} + +func TestDockerMountsStillMountsSpacedMissingOutputPaths(t *testing.T) { + // A not-yet-created output file under a spaced TMPDIR must still get its + // parent mounted writable, or Docker scanners cannot write their reports. + parent := filepath.Join(t.TempDir(), "with space") + if err := os.Mkdir(parent, 0o755); err != nil { + t.Fatal(err) + } + missing := filepath.Join(parent, "report.json") + mounts := dockerMounts("", []string{missing}) + found := false + for _, mount := range mounts { + if strings.Contains(mount, "source="+parent+",") && !strings.HasSuffix(mount, ",readonly") { + found = true + } + } + if !found { + t.Fatalf("spaced parent %q should be mounted writable; mounts = %v", parent, mounts) + } +} diff --git a/internal/runner/scanner_registry_test.go b/internal/runner/scanner_registry_test.go index 9eee4f6..07aaa75 100644 --- a/internal/runner/scanner_registry_test.go +++ b/internal/runner/scanner_registry_test.go @@ -3,7 +3,10 @@ package runner import ( "encoding/json" "errors" + "os" "path/filepath" + "reflect" + "slices" "strings" "testing" ) @@ -156,7 +159,10 @@ func TestUserDefinedScannerPreservesValidJSONOnCommandFailure(t *testing.T) { if err != nil { t.Fatal(err) } - commandRunner := &recordingCommandRunner{stdout: `{"findings":["detected"]}`, stderr: "findings detected", err: errCommandFailed} + // Normal nonzero exit (findings-mean-nonzero scanners): valid JSON stays + // completed evidence. Abnormal termination is covered separately below. + exitCode := 1 + commandRunner := &recordingCommandRunner{stdout: `{"findings":["detected"]}`, stderr: "findings detected", err: errCommandFailed, exitCode: &exitCode} result, err := (ExternalScannerRunner{ Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeOff, }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z") @@ -171,6 +177,1434 @@ func TestUserDefinedScannerPreservesValidJSONOnCommandFailure(t *testing.T) { } } +func TestUserDefinedScannerRunsInIsolatedDirectoryOnHost(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + target := t.TempDir() + if _, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("demo", target, "2026-07-21T00:00:00Z"); err != nil { + t.Fatal(err) + } + if len(commandRunner.calls) != 1 { + t.Fatalf("calls = %d", len(commandRunner.calls)) + } + cwd := commandRunner.calls[0].cwd + if cwd == "" { + t.Fatal("host scanner ran with empty cwd; would inherit ClawScan's process cwd, which may be the untrusted target") + } + if cwd == target || strings.HasPrefix(cwd, target+string(os.PathSeparator)) { + t.Fatalf("scanner ran inside the untrusted target directory %q", cwd) + } + processCwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if cwd == processCwd { + t.Fatalf("scanner inherited ClawScan's process cwd %q", cwd) + } + if _, err := os.Stat(cwd); !os.IsNotExist(err) { + t.Fatalf("isolated scanner cwd %q was not cleaned up (stat err = %v)", cwd, err) + } +} + +func TestUserDefinedScannerDockerRunKeepsEmptyCwd(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + if _, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeDocker, + }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z"); err != nil { + t.Fatal(err) + } + if len(commandRunner.calls) != 1 { + t.Fatalf("calls = %d", len(commandRunner.calls)) + } + // Docker runs must not receive a host temp dir as cwd: dockerCommandRunner + // would mount it and set it as the container workdir for no benefit. + if cwd := commandRunner.calls[0].cwd; cwd != "" { + t.Fatalf("docker scanner run got cwd %q, want empty", cwd) + } +} + +func TestUserDefinedScannerRejectsMissingTargetBeforeRunning(t *testing.T) { + // A missing local target must fail before the command runs: Docker mount + // inference binds a missing path's parent read-write, so a typo'd target + // would expose the surrounding host directory writable to the container. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeDocker, + }).RunScanner("demo", filepath.Join(t.TempDir(), "missing-skill"), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "target does not exist") { + t.Fatalf("result = %#v", result) + } + if len(commandRunner.calls) != 0 { + t.Fatalf("scanner command ran despite missing target: %#v", commandRunner.calls) + } +} + +func TestUserDefinedScannerSkipsExistenceCheckForURLTargets(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Targets: []string{"url"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeDocker, TargetKind: "url", + }).RunScanner("demo", "https://example.com/skill", "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "completed" || len(commandRunner.calls) != 1 { + t.Fatalf("URL target must run without a local existence check: %#v calls=%d", result, len(commandRunner.calls)) + } +} + +func TestUserDefinedScannerRedactsDeclaredEnvOnFailure(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stderr: "auth failed: hunter2-credential rejected", err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"SCANNER_ACCESS": "hunter2-credential"}, + SandboxMode: SandboxModeOff, + }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(result.Error, "hunter2-credential") { + t.Fatalf("declared env value leaked into error: %q", result.Error) + } + if !strings.Contains(result.Error, "[redacted]") { + t.Fatalf("expected redaction marker in error: %q", result.Error) + } +} + +func TestUserDefinedScannerRedactsDeclaredEnvInRawJSON(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"token":"hunter2-credential","findings":[]}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"SCANNER_ACCESS": "hunter2-credential"}, + SandboxMode: SandboxModeOff, + }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "completed" { + t.Fatalf("status = %q", result.Status) + } + if strings.Contains(string(result.Raw), "hunter2-credential") { + t.Fatalf("declared env value persisted in raw scanner output: %s", result.Raw) + } + if !strings.Contains(string(result.Raw), "[redacted]") { + t.Fatalf("expected redaction marker in raw output: %s", result.Raw) + } +} + +func TestRedactScannerStdoutCoversJSONEscapedSecrets(t *testing.T) { + // JSON permits alternative encodings of the same string, so redaction must + // operate on decoded values, not one serialized representation. + for name, test := range map[string]struct { + secret string + raw string + }{ + "canonical escapes": {secret: `pa"ss\word`, raw: `{"token":"pa\"ss\\word","findings":[]}`}, + "solidus escape": {secret: "a/b", raw: `{"token":"a\/b"}`}, + "unicode escapes": {secret: "sécret", raw: `{"token":"s\u00e9cret"}`}, + "secret in key": {secret: "hunter2", raw: `{"hunter2":"value"}`}, + "nested array": {secret: "hunter2", raw: `{"findings":[{"evidence":["saw hunter2 here"]}]}`}, + } { + env := map[string]string{"SCANNER_ACCESS": test.secret} + declared := []string{"SCANNER_ACCESS"} + redacted := redactScannerStdout(test.raw, env, declared) + var decoded any + if err := json.Unmarshal([]byte(redacted), &decoded); err != nil { + t.Fatalf("%s: redacted output is not JSON: %v", name, err) + } + if strings.Contains(redacted, test.secret) { + t.Fatalf("%s: secret survived redaction: %s", name, redacted) + } + if !strings.Contains(redacted, "[redacted]") { + t.Fatalf("%s: expected redaction marker: %s", name, redacted) + } + } +} + +func TestRedactScannerStdoutLeavesCleanOutputUntouched(t *testing.T) { + env := map[string]string{"SCANNER_ACCESS": "hunter2"} + raw := `{"findings": [], + "note": "keeps formatting when nothing leaks"}` + if got := redactScannerStdout(raw, env, []string{"SCANNER_ACCESS"}); got != raw { + t.Fatalf("clean output was rewritten: %q", got) + } +} + +func TestUserDefinedScannerAbnormalExitWithValidJSONFails(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + // Timeout/signal: runErr set, no usable exit code, but stdout is valid + // JSON. Partial output must not report success or satisfy exit-code gates. + commandRunner := &recordingCommandRunner{stdout: `{"findings":[]}`, stderr: "killed", err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" { + t.Fatalf("status = %q, want failed for abnormal termination", result.Status) + } + if result.ExitCode != nil { + t.Fatalf("exit code = %v, want nil", *result.ExitCode) + } + if result.Raw != nil { + t.Fatalf("partial output persisted after abnormal termination: %s", result.Raw) + } +} + +func TestRedactScannerStdoutScrubsUndeclaredSecretEnv(t *testing.T) { + // --sandbox off inherits the whole process env, and Docker passes + // --sandbox-env and other adapters' credentials, so secret-named env vars + // must be scrubbed even when this scanner never declared them. + env := map[string]string{"OPENAI_API_KEY": "sekret-value"} + raw := `{"token":"sekret-value"}` + redacted := redactScannerStdout(raw, env, nil) + if strings.Contains(redacted, "sekret-value") { + t.Fatalf("undeclared secret env value survived: %s", redacted) + } + if !strings.Contains(redacted, "[redacted]") { + t.Fatalf("expected redaction marker: %s", redacted) + } +} + +func TestRedactScannerStdoutRedactsScalarEncodedSecrets(t *testing.T) { + // Numeric or boolean secrets may be emitted unquoted; the scalar's exact + // text matching a secret is a leak like any other. + for name, test := range map[string]struct { + secret string + raw string + leak string + }{ + "numeric scalar": {secret: "1234", raw: `{"token":1234,"count":7}`, leak: "1234"}, + "boolean scalar": {secret: "true", raw: `{"token":true}`, leak: "true"}, + } { + env := map[string]string{"SCANNER_PIN": test.secret} + redacted := redactScannerStdout(test.raw, env, []string{"SCANNER_PIN"}) + if strings.Contains(redacted, test.leak) { + t.Fatalf("%s: scalar secret survived: %s", name, redacted) + } + if !strings.Contains(redacted, "[redacted]") { + t.Fatalf("%s: expected redaction marker: %s", name, redacted) + } + if !json.Valid([]byte(redacted)) { + t.Fatalf("%s: redacted output is not valid JSON: %s", name, redacted) + } + } + // Non-matching scalars must survive untouched. + env := map[string]string{"SCANNER_PIN": "1234"} + redacted := redactScannerStdout(`{"count":7,"ok":true}`, env, []string{"SCANNER_PIN"}) + if !strings.Contains(redacted, `"count":7`) || !strings.Contains(redacted, `"ok":true`) { + t.Fatalf("unrelated scalars were rewritten: %s", redacted) + } +} + +func TestUserDefinedScannerTreatsSignalStyleExitCodesAsFailed(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + // Shells and docker report signal-killed children as 128+N (137 OOM/KILL, + // 143 TERM), while docker run and shells reserve 125-127; none are scanner + // verdicts, so partial valid JSON must not become completed evidence or + // satisfy a gate. + for _, code := range []int{125, 126, 127, 137, 143} { + exitCode := code + commandRunner := &recordingCommandRunner{stdout: `{"findings":[]}`, stderr: "killed", err: errCommandFailed, exitCode: &exitCode} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" { + t.Fatalf("exit %d: status = %q, want failed", code, result.Status) + } + if result.ExitCode != nil { + t.Fatalf("exit %d: gate-eligible exit code = %v, want nil", code, *result.ExitCode) + } + } +} + +func TestGateEligibleExitCodeBoundary(t *testing.T) { + eligible := 124 + if got := gateEligibleExitCode(&eligible); got == nil || *got != eligible { + t.Fatalf("gateEligibleExitCode(124) = %v, want 124", got) + } + reserved := 125 + if got := gateEligibleExitCode(&reserved); got != nil { + t.Fatalf("gateEligibleExitCode(125) = %d, want nil", *got) + } +} + +func TestUserDefinedScannerRedactsOtherScannersExposedEnv(t *testing.T) { + // Under Docker every scanner sees the whole passthrough set; scanner A's + // output must be scrubbed of scanner B's credential even when its name + // (BETA_LICENSE) evades the isSecretEnvKey heuristic. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"token":"beta-cred-value"}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"BETA_LICENSE": "beta-cred-value"}, + ExposedEnvNames: []string{"BETA_LICENSE"}, + SandboxMode: SandboxModeDocker, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(result.Raw), "beta-cred-value") { + t.Fatalf("another scanner's exposed credential persisted: %s", result.Raw) + } + if !strings.Contains(string(result.Raw), "[redacted]") { + t.Fatalf("expected redaction marker: %s", result.Raw) + } +} + +func TestUserDefinedScannerDockerRedactionIgnoresUnexposedHostSecrets(t *testing.T) { + // Under Docker only allowlisted names reach the container. A host-only + // secret value that coincides with legitimate output (CI_TOKEN=clean) + // must not rewrite scanner evidence — the container never saw it. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"verdict":"clean"}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"CI_TOKEN": "clean"}, + ExposedEnvNames: nil, + SandboxMode: SandboxModeDocker, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(result.Raw), `"verdict":"clean"`) { + t.Fatalf("unexposed host secret corrupted Docker evidence: %s", result.Raw) + } +} + +func TestUserDefinedScannerHostRedactionCoversStandardCredentialNames(t *testing.T) { + // --sandbox off inherits the whole host env, so undeclared credentials + // with standard spellings — a personal access token (GITHUB_PAT, whole + // _-segment match) and a password-bearing connection string + // (DATABASE_URL) — must be scrubbed even though no scanner declared + // them and they carry no TOKEN/SECRET/KEY marker. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"pat":"ghp_hostpatvalue","db":"postgres://user:hostdbpass@db/x"}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{ + "GITHUB_PAT": "ghp_hostpatvalue", + "DATABASE_URL": "postgres://user:hostdbpass@db/x", + }, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(result.Raw), "ghp_hostpatvalue") { + t.Fatalf("GITHUB_PAT value leaked on --sandbox off: %s", result.Raw) + } + if strings.Contains(string(result.Raw), "hostdbpass") { + t.Fatalf("DATABASE_URL value leaked on --sandbox off: %s", result.Raw) + } +} + +func TestUserDefinedScannerArtifactOmitsRenderedCommand(t *testing.T) { + // The rendered command line is operator-authored config and must never + // be persisted into artifact evidence; only the scanner ID is recorded. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "scanner --secret-free {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if joined := strings.Join(result.Command, " "); joined != "user-defined-scanner alpha" { + t.Fatalf("artifact command = %v, want scanner ID reference only", result.Command) + } +} + +func TestUserDefinedScannerAllowsInlineCredentialAssignment(t *testing.T) { + // Inline credential literals on the command line are the operator's + // responsibility (the same as flag-form secrets); ClawScan no longer + // blocks them. The command runs. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "API_TOKEN=sk-live-inline scanner {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status == "failed" { + t.Fatalf("inline credential should no longer block; got failed: %q", result.Error) + } + if len(commandRunner.calls) != 1 { + t.Fatalf("command should have run once, got calls = %#v", commandRunner.calls) + } +} + +func TestUserDefinedScannerRejectsUnsafeEvidence(t *testing.T) { + // json.Valid accepts invalid UTF-8 inside strings, but decoding swaps + // the bytes for U+FFFD so byte-exact secret comparison misses; and + // duplicate object members hide earlier values from the decoded walk. + // Both must fail the scan, not persist as evidence. + for name, stdout := range map[string]string{ + "invalid-utf8": "{\"note\":\"\xff\xfe\"}", + "duplicate-keys": `{"auth":"a","auth":"b"}`, + } { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: stdout} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"CI_TOKEN": "unrelated"}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" { + t.Fatalf("%s: status = %q, want failed", name, result.Status) + } + if len(result.Raw) != 0 { + t.Fatalf("%s: unsafe evidence persisted: %s", name, result.Raw) + } + if !strings.Contains(result.Error, "output rejected") { + t.Fatalf("%s: error = %q", name, result.Error) + } + } +} + +func TestUserDefinedScannerPreservesStdoutBytesAndRejectsNonJSONWhitespace(t *testing.T) { + for name, test := range map[string]struct { + stdout string + wantStatus string + wantRaw string + }{ + "leading-nbsp": { + stdout: "\u00a0{}", + wantStatus: "failed", + }, + "trailing-newline": { + stdout: "{\"ok\":true}\n", + wantStatus: "completed", + wantRaw: "{\"ok\":true}\n", + }, + } { + t.Run(name, func(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: &recordingCommandRunner{stdout: test.stdout}, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != test.wantStatus { + t.Fatalf("status = %q, want %q (error = %q)", result.Status, test.wantStatus, result.Error) + } + if string(result.Raw) != test.wantRaw { + t.Fatalf("raw = %q, want %q", result.Raw, test.wantRaw) + } + }) + } +} + +func TestRedactScannerStdoutScrubsNestedJSONEncodings(t *testing.T) { + // A secret can hide one JSON-escaping layer down: a string node holding + // embedded JSON where the secret appears only via \u escapes. Neither + // the raw bytes nor the once-decoded string contain the literal. + env := map[string]string{"DEMO_TOKEN": "sekret"} + raw := `{"message":"{\"auth\":\"sekret\"}"}` + redacted := redactScannerStdout(raw, env, []string{"DEMO_TOKEN"}) + if strings.Contains(redacted, "sekret") { + t.Fatalf("nested-encoded secret survived: %s", redacted) + } + // Two layers down (JSON in JSON in JSON) with mixed escaping. + raw = `{"outer":"{\"inner\":\"{\\\"auth\\\":\\\"\\u0073ekret\\\"}\"}"}` + redacted = redactScannerStdout(raw, env, []string{"DEMO_TOKEN"}) + if strings.Contains(decodeJSONStringEscapes(decodeJSONStringEscapes(redacted)), "sekret") { + t.Fatalf("doubly nested secret survived: %s", redacted) + } +} + +func TestScrubDeepReachesArbitraryEncodingDepth(t *testing.T) { + secret := "sk-deep-layer-credential" + value := secret + for range 6 { + encoded, err := json.Marshal(value) + if err != nil { + t.Fatal(err) + } + value = string(encoded) + } + + redacted := newSecretScrubber([]string{secret}).scrubDeep(value) + for layer := 0; ; layer++ { + if strings.Contains(redacted, secret) { + t.Fatalf("layer %d contains secret", layer) + } + decoded := decodeJSONStringEscapes(redacted) + if decoded == redacted { + break + } + redacted = decoded + } +} + +func TestScrubDeepCatchesMultilineSecretUnderEscapeLayers(t *testing.T) { + secret := "line1\nline2-secret-credential" + inner, err := json.Marshal(map[string]string{"auth": secret}) + if err != nil { + t.Fatal(err) + } + outer, err := json.Marshal(map[string]string{"message": string(inner)}) + if err != nil { + t.Fatal(err) + } + + redacted := newSecretScrubber([]string{secret}).scrubDeep(string(outer)) + for layer := 0; ; layer++ { + if strings.Contains(redacted, secret) || strings.Contains(redacted, "line2-secret-credential") { + t.Fatalf("layer %d contains multiline secret", layer) + } + decoded := decodeJSONStringEscapes(redacted) + if decoded == redacted { + break + } + redacted = decoded + } +} + +func TestRedactJSONStringsCanonicalNumberSecrets(t *testing.T) { + // A numeric secret emitted in an alternate spelling (1e3 for 1000, + // 1.0 for 1) reveals the same credential. + scrubber := newSecretScrubber([]string{"1000"}) + node, changed := redactJSONStrings(json.Number("1e3"), scrubber) + if !changed || node == json.Number("1e3") { + t.Fatalf("alternate numeric spelling survived: %v", node) + } + // A non-numeric secret must never match numbers. + scrubber = newSecretScrubber([]string{"abc"}) + if _, changed := redactJSONStrings(json.Number("123"), scrubber); changed { + t.Fatal("non-numeric secret matched a number") + } + // Unrelated numbers pass through. + scrubber = newSecretScrubber([]string{"1000"}) + if _, changed := redactJSONStrings(json.Number("1001"), scrubber); changed { + t.Fatal("unrelated number scrubbed") + } +} + +func TestCommandReparsesTarget(t *testing.T) { + for command, want := range map[string]bool{ + "if true; then eval scanner {{target}}; fi": true, + "while :; do eval scanner {{target}}; done": true, + "eval scanner {{target}}": true, + "/bin/eval scanner {{target}}": true, + "sh -c {{target}}": true, + "bash -lc {{target}}": true, + "/bin/sh -c {{target}}": true, + "cmd /C {{target}}": true, + "powershell -Command {{target}}": true, + "env sh -c {{target}}": true, + "sudo bash -lc {{target}}": true, + "env NAME=v sh -c {{target}}": true, + "env -i sh -c {{target}}": true, + "/usr/bin/env sh -c {{target}}": true, + "if true; then sh -c {{target}}; fi": true, + "while :; do bash -c {{target}}; done": true, + "if true; then myscanner {{target}}; fi": false, + "timeout 5 sh -c {{target}}": true, + "nice sh -c {{target}}": true, + "timeout 5 myscanner {{target}}": false, + "timeout 5 sh scan.sh {{target}}": false, + "sh -c 'myscanner {{target}} | jq .'": true, + "sh -c 'scan {{target}} | jq'": true, + "myscanner -c {{target}}": false, + "env myscanner {{target}}": false, + "sh -c scan.sh {{target}}": false, + "scanner {{target}}": false, + "scanner $({{target}})": true, + "scanner `{{target}}`": true, + "x=$({{target}})": true, + // A placeholder embedded in the first command-string operand still lands + // in the nested interpreter's code position, unlike a placeholder in a + // later operand which is only an inner positional parameter. + "sh -c echo-{{target}}": true, + "sh -c {{target}}-scan": true, + "bash -c pre-{{target}}-post": true, + "env sh -c echo-{{target}}": true, + "timeout 5 sh -c echo-{{target}}": true, + "sh -c myscanner {{target}}": false, + "myscanner --mode eval {{target}}": false, + "myscanner --mode=eval {{target}}": false, + "eval scanner --static": false, + + "$(printf eval) {{target}}": true, + "powershell -NoProfile -Command scanner {{target}}": true, + "cmd /c scanner {{target}}": true, + "scanner $(date) {{target}}": false, + "powershell -File script.ps1 {{target}}": false, + "sh -c scanner {{target}}": false, + + "{{target}} -c 'echo scanner'": true, // target as command word executes the scanned artifact + "{{target}} --scan": true, // target alone as the command + } { + if got := commandReparsesTarget(command); got != want { + t.Fatalf("commandReparsesTarget(%q) = %t, want %t", command, got, want) + } + } +} + +func TestUserDefinedScannerRejectsEvalTargetPlaceholder(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "eval scanner {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "re-parses the interpolated target as shell code") { + t.Fatalf("result = %q / %q, want eval-target rejection", result.Status, result.Error) + } + if len(commandRunner.calls) != 0 { + t.Fatalf("command ran despite eval-target rejection: %#v", commandRunner.calls) + } +} + +func TestUserDefinedScannerRejectsShellCommandTargetPlaceholder(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "sh -c {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "re-parses the interpolated target as shell code") { + t.Fatalf("result = %q / %q, want shell-command-target rejection", result.Status, result.Error) + } + if len(commandRunner.calls) != 0 { + t.Fatalf("command ran despite shell-command-target rejection: %#v", commandRunner.calls) + } +} + +func TestRunScannerRejectsUnsafeEvidenceFromAnyAdapter(t *testing.T) { + // The unsafe-evidence gate must hold at the central boundary, not just + // inside the user-defined adapter: a built-in scanner emitting + // duplicate JSON members would otherwise be decoded and re-encoded by + // redaction, silently dropping the earlier member. + commandRunner := &recordingCommandRunner{stdout: `{"x":1,"x":2}`} + result, err := (ExternalScannerRunner{ + Registry: DefaultScannerRegistry(), CommandRunner: commandRunner, + Env: map[string]string{"CI_TOKEN": "host-secret"}, + SandboxMode: SandboxModeOff, + }).RunScanner("agentverus", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "duplicate JSON object members") { + t.Fatalf("result = %q / %q, want central unsafe-evidence rejection", result.Status, result.Error) + } + if len(result.Raw) != 0 { + t.Fatalf("unsafe evidence persisted: %s", result.Raw) + } +} + +func TestEnvEntriesForNameWindowsIsCaseInsensitive(t *testing.T) { + // Windows env names are case-insensitive: a scanner declaring + // scanner_access receives the host's SCANNER_ACCESS value, so both + // redaction scoping and the secret sweep must see it under either + // spelling. Elsewhere only the exact key matches. + env := map[string]string{"SCANNER_ACCESS": "secret-value"} + windows := envEntriesForNameOnGOOS(env, "scanner_access", "windows") + if windows["SCANNER_ACCESS"] != "secret-value" { + t.Fatalf("windows lookup missed case-folded name: %v", windows) + } + linux := envEntriesForNameOnGOOS(env, "scanner_access", "linux") + if len(linux) != 0 { + t.Fatalf("non-windows lookup must be exact: %v", linux) + } +} + +func TestEnvValueForNameFindsNonEmptyExactMatch(t *testing.T) { + env := map[string]string{"SCANNER_ACCESS": "secret-value"} + if got := envValueForName(env, "SCANNER_ACCESS"); got != "secret-value" { + t.Fatalf("envValueForName() = %q, want non-empty exact match", got) + } + if got := envValueForName(map[string]string{"SCANNER_ACCESS": " "}, "SCANNER_ACCESS"); got != "" { + t.Fatalf("envValueForName() = %q, want empty for whitespace-only value", got) + } +} + +func TestUserDefinedScannerInfoSanitizesMalformedEnvEntries(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "scanner {{target}}", + Env: []string{"API_TOKEN=sk-live-info-leak", "=sk-live-eqzero", "GOOD_NAME"}, Targets: []string{"skill"}, + }) + want := []string{"API_TOKEN", "GOOD_NAME"} + if got := adapter.Info().RequiredEnv; !reflect.DeepEqual(got, want) { + t.Fatalf("Info().RequiredEnv = %v, want %v", got, want) + } + declarer, ok := adapter.(interface{ DeclaredCredentialEnv() []string }) + if !ok { + t.Fatal("user-defined scanner does not expose declared credential env") + } + if got := declarer.DeclaredCredentialEnv(); !reflect.DeepEqual(got, want) { + t.Fatalf("DeclaredCredentialEnv() = %v, want %v", got, want) + } + for _, names := range [][]string{adapter.Info().RequiredEnv, declarer.DeclaredCredentialEnv()} { + for _, name := range names { + if strings.Contains(name, "sk-live") { + t.Fatalf("sanitized env name leaked credential value: %q", name) + } + if name == "" { + t.Fatal("sanitized env names contain an empty element") + } + } + } +} + +func TestUserDefinedScannerRejectsMalformedEnvDeclaration(t *testing.T) { + // env: [API_TOKEN=sk-live] is a misconfiguration; the run must fail + // without echoing the inline value anywhere (the missing-variable + // diagnostic would otherwise print it into terminal and CI logs). + if got := invalidDeclaredEnvName([]string{"API_TOKEN=sk-live"}); got != "API_TOKEN=..." { + t.Fatalf("invalidDeclaredEnvName() = %q, want API_TOKEN=...", got) + } + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "scanner {{target}}", + Env: []string{"API_TOKEN=sk-live-declared"}, Targets: []string{"skill"}, + }) + requirements := adapter.Requirements(nil) + for _, req := range requirements { + if strings.Contains(req.EnvVar, "sk-live-declared") { + t.Fatalf("requirement echoes inline value: %v", requirements) + } + } + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "not a variable name") { + t.Fatalf("result = %q / %q, want malformed-env rejection", result.Status, result.Error) + } + if strings.Contains(result.Error, "sk-live-declared") { + t.Fatalf("rejection echoes inline value: %s", result.Error) + } + if len(commandRunner.calls) != 0 { + t.Fatalf("command ran despite malformed env: %#v", commandRunner.calls) + } +} + +func TestUserDefinedScannerRejectsMalformedBareEnvDeclarationWithoutEchoingIt(t *testing.T) { + const pastedCredential = "sk-live-production-token" + if got := invalidDeclaredEnvName([]string{pastedCredential}); got != "#1" { + t.Fatalf("invalidDeclaredEnvName() = %q, want #1", got) + } + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "scanner {{target}}", + Env: []string{pastedCredential}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "env entry #1 is not a variable name") { + t.Fatalf("result = %q / %q, want positional malformed-env rejection", result.Status, result.Error) + } + if strings.Contains(result.Error, pastedCredential) { + t.Fatalf("rejection echoes pasted credential: %s", result.Error) + } + if len(commandRunner.calls) != 0 { + t.Fatalf("command ran despite malformed env: %#v", commandRunner.calls) + } +} + +func TestUserDefinedScannerRejectsMalformedEnvDeclarationAtEqualsZero(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "scanner {{target}}", + Env: []string{"=sk-live-eqzero"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{}, SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "failed" || !strings.Contains(result.Error, "=...") { + t.Fatalf("result = %q / %q, want safely truncated malformed-env rejection", result.Status, result.Error) + } + if strings.Contains(result.Error, "sk-live-eqzero") { + t.Fatalf("rejection echoes inline value: %s", result.Error) + } + if len(commandRunner.calls) != 0 { + t.Fatalf("command ran despite malformed env: %#v", commandRunner.calls) + } +} + +func TestSameCanonicalNumberIsExact(t *testing.T) { + // float64 rounding must not conflate distinct 17-digit integers — + // that would scrub unrelated evidence as a secret. + if sameCanonicalNumber("9007199254740993", "9007199254740992") { + t.Fatal("distinct integers conflated by float rounding") + } + if !sameCanonicalNumber("1e3", "1000") { + t.Fatal("equal values in different spellings must match") + } + if !sameCanonicalNumber("1.0", "1") { + t.Fatal("1.0 and 1 must match") + } + if sameCanonicalNumber("123", "abc") { + t.Fatal("non-numeric secret matched") + } +} + +func TestUserDefinedScannerHostRedactionKeepsNonSecretSegmentNames(t *testing.T) { + // Segment matching must not over-reach: GIT_AUTHOR_NAME contains AUTH + // only inside AUTHOR, TOKENIZERS_PARALLELISM contains TOKEN only inside + // TOKENIZERS, and PWD names the shell's working directory. Scrubbing any + // of these values ("false" would rewrite every matching JSON boolean) + // would corrupt legitimate evidence. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"author":"Jesse","cwd":"/skills/demo","clean":false}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{ + "GIT_AUTHOR_NAME": "Jesse", + "PWD": "/skills/demo", + "TOKENIZERS_PARALLELISM": "false", + }, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + for _, want := range []string{`"author":"Jesse"`, `"cwd":"/skills/demo"`, `"clean":false`} { + if !strings.Contains(string(result.Raw), want) { + t.Fatalf("non-secret env value scrubbed %s from evidence: %s", want, result.Raw) + } + } +} + +func TestUserDefinedScannerHostRedactionStillCoversWholeEnv(t *testing.T) { + // --sandbox off inherits the whole host env, so the secret-named sweep + // must keep covering variables no scanner declared or allowlisted. + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"echo":"host-secret-value"}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"CI_TOKEN": "host-secret-value"}, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(result.Raw), "host-secret-value") { + t.Fatalf("host secret leaked on --sandbox off: %s", result.Raw) + } +} + +func TestUnsafeWindowsShellTargetRejectsQuotesAndPercents(t *testing.T) { + // cmd.exe cannot safely receive quotes (backslash does not escape " for + // its parser, enabling breakout), percents (%VAR% expands inside double + // quotes), or exclamation marks (!VAR! expands when delayed expansion is + // enabled). All must be refused before interpolation. + for _, target := range []string{`https://host/" & calc & "`, `C:\skill%path`, `https://host/!TEMP!`} { + if !unsafeWindowsShellTarget(target) { + t.Fatalf("target %q should be refused on the Windows host shell", target) + } + } + for _, target := range []string{`C:\skills\my-skill`, "https://example.com/skill", "/tmp/skill with space"} { + if unsafeWindowsShellTarget(target) { + t.Fatalf("safe target %q was refused", target) + } + } +} + +func TestUserDefinedScannerRedactsExposedEnvFromErrors(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + // Another scanner's credential (name evades isSecretEnvKey) written to + // stderr must not persist in ScannerResult.Error. + commandRunner := &recordingCommandRunner{stderr: "auth failed: beta-cred-value rejected", err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"BETA_LICENSE": "beta-cred-value"}, + ExposedEnvNames: []string{"BETA_LICENSE"}, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(result.Error, "beta-cred-value") { + t.Fatalf("exposed credential leaked into error: %q", result.Error) + } + if !strings.Contains(result.Error, "[redacted]") { + t.Fatalf("expected redaction marker in error: %q", result.Error) + } +} + +func TestUserDefinedScannerRedactsEscapedUndeclaredSecretsFromErrors(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + // With --sandbox off the scanner inherits the whole host env. An + // undeclared, unexposed secret-named var (OPENAI_API_KEY) whose value + // appears JSON-escaped on stderr (pa\"ss for pa"ss) must still be + // scrubbed from ScannerResult.Error. + commandRunner := &recordingCommandRunner{stderr: `request body was {"auth":"pa\"ss"}`, err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"OPENAI_API_KEY": `pa"ss`}, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(result.Error, `pa\"ss`) || strings.Contains(result.Error, `pa"ss`) { + t.Fatalf("escaped undeclared secret leaked into error: %q", result.Error) + } + if !strings.Contains(result.Error, "[redacted]") { + t.Fatalf("expected redaction marker in error: %q", result.Error) + } +} + +func TestUserDefinedScannerRedactsAlternateEncodedSecretsFromErrors(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + // json.Marshal never produces \/ or a, but a scanner echoing a + // request body may: the escaped stderr text decodes back to the secret, + // so failure-text redaction must catch alternate encodings too. + tests := []struct { + name string + secret string + stderr string + }{ + {name: "solidus escape", secret: "a/b-secret", stderr: `request failed: {"auth":"a\/b-secret"}`}, + {name: "unicode escape", secret: "s\u00e9cret", stderr: `request failed: {"auth":"s\u00e9cret"}`}, + {name: "surrogate pair", secret: "pass\U0001F600word", stderr: `request failed: {"auth":"pass\ud83d\ude00word"}`}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + commandRunner := &recordingCommandRunner{stderr: test.stderr, err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"SCANNER_ACCESS": test.secret}, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + decoded := decodeJSONStringEscapes(result.Error) + if strings.Contains(result.Error, test.secret) || strings.Contains(decoded, test.secret) { + t.Fatalf("alternate-encoded secret survives in error: %q", result.Error) + } + if !strings.Contains(result.Error, "[redacted]") { + t.Fatalf("expected redaction marker in error: %q", result.Error) + } + }) + } +} + +func TestDecodeJSONStringEscapesPassesThroughUnknownSequences(t *testing.T) { + for input, want := range map[string]string{ + `plain text`: "plain text", + `a\/b`: "a/b", + `s\u00e9cret`: "s\u00e9cret", + `pa\"ss and back\\`: `pa"ss and back\`, + `\ud83d\ude00`: "\U0001F600", + `trailing\`: `trailing\`, + `\uZZZZ stays`: `\uZZZZ stays`, + `\b\f\n\r\t`: "\b\f\n\r\t", + `C:\new\table`: "C:\new\table", + } { + if got := decodeJSONStringEscapes(input); got != want { + t.Fatalf("decodeJSONStringEscapes(%q) = %q, want %q", input, got, want) + } + } +} + +func TestRedactScannerStdoutRedactsNullScalarSecret(t *testing.T) { + env := map[string]string{"SCANNER_PIN": "null"} + redacted := redactScannerStdout(`{"token":null,"other":null}`, env, []string{"SCANNER_PIN"}) + if strings.Contains(redacted, "null") { + t.Fatalf("null-valued secret survived: %s", redacted) + } + if !json.Valid([]byte(redacted)) { + t.Fatalf("redacted output is not valid JSON: %s", redacted) + } +} + +func TestRedactScannerStdoutDoesNotCorruptNonStringTokens(t *testing.T) { + // A short secret such as "1" must never break JSON syntax the way byte + // replacement did (`{"count":[redacted]}` is invalid and flipped healthy + // scans to failed). Scalars are redacted only on exact match: count:10 + // merely contains the secret's digits and must survive, while count:1 + // (exact match) becomes a redacted string — still valid JSON either way. + env := map[string]string{"DEMO_TOKEN": "1"} + raw := `{"count":10,"exact":1,"enabled":true,"note":"value 1 appears here"}` + redacted := redactScannerStdout(raw, env, []string{"DEMO_TOKEN"}) + if !json.Valid([]byte(redacted)) { + t.Fatalf("redacted output is not valid JSON: %s", redacted) + } + if !strings.Contains(redacted, `"count":10`) { + t.Fatalf("non-matching numeric token was corrupted: %s", redacted) + } + if !strings.Contains(redacted, `"enabled":true`) { + t.Fatalf("non-matching boolean token was corrupted: %s", redacted) + } + if strings.Contains(redacted, `"exact":1`) { + t.Fatalf("exact-match scalar secret survived: %s", redacted) + } + if strings.Contains(redacted, "value 1 appears") { + t.Fatalf("secret inside string survived: %s", redacted) + } +} + +func TestRedactionMarkerNeverContainsASecret(t *testing.T) { + // A credential that is a substring of "[redacted]" ("act", "redacted", + // even "a") would be re-inserted by the replacement itself, so the marker + // must be swapped for one no secret can appear in. + for name, secrets := range map[string][]string{ + "safe secrets keep preferred marker": {"hunter2", "s3cr3t"}, + "single letter": {"a"}, + "marker substring": {"act"}, + "whole marker word": {"redacted"}, + "marker with brackets": {"[redacted]"}, + "fallback rune collision": {"act", "##########"[:1]}, + } { + t.Run(name, func(t *testing.T) { + marker := redactionMarker(secrets) + if marker == "" { + t.Fatal("marker collapsed to empty for a realistic secret set") + } + for _, secret := range secrets { + if strings.Contains(marker, secret) { + t.Fatalf("marker %q contains secret %q", marker, secret) + } + } + }) + } + if got := redactionMarker([]string{"hunter2"}); got != "[redacted]" { + t.Fatalf("safe secret set changed the preferred marker: %q", got) + } +} + +func TestRedactScannerStdoutMarkerSubstringSecrets(t *testing.T) { + // Secrets that are substrings of the "[redacted]" sentinel must not + // survive inside the replacement marker (scanner JSON path). + for _, secret := range []string{"act", "redacted", "a"} { + env := map[string]string{"SCANNER_ACCESS": secret} + raw := `{"token":"` + secret + `","note":"before ` + secret + ` after"}` + redacted := redactScannerStdout(raw, env, []string{"SCANNER_ACCESS"}) + if strings.Contains(redacted, secret) { + t.Fatalf("secret %q survived redaction: %s", secret, redacted) + } + if !json.Valid([]byte(redacted)) { + t.Fatalf("redacted output is not valid JSON: %s", redacted) + } + } +} + +func TestUserDefinedScannerMarkerSubstringSecretInErrors(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "alpha", Command: "alpha {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + // Failure-text path: a declared credential equal to "redacted" must not + // ride back in inside the sentinel that replaces it. + commandRunner := &recordingCommandRunner{stderr: "auth redacted rejected", err: errCommandFailed} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"SCANNER_ACCESS": "redacted"}, + SandboxMode: SandboxModeOff, + }).RunScanner("alpha", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(result.Error, "redacted") { + t.Fatalf("marker-substring secret survived in error: %q", result.Error) + } +} + +func TestSecretScrubberReplacementCannotRebuildSecret(t *testing.T) { + // Replacement can splice marker bytes against surrounding text and + // synthesize a secret that was not there before; scrub must re-check + // until no secret remains. + scrubber := newSecretScrubber([]string{"X["}) + if got := scrubber.scrub("XX[tail"); strings.Contains(got, "X[") { + t.Fatalf("replacement reintroduced the secret: %q", got) + } +} + +func TestRedactScannerStdoutPreservesFieldsOnKeyCollision(t *testing.T) { + // A JSON key containing a secret is renamed to the marker. If another + // field's key already equals the marker, direct assignment would + // overwrite it and silently drop evidence; the redacted key must land + // under a collision-safe name instead. + env := map[string]string{"SCANNER_ACCESS": "abc"} + raw := `{"abc":1,"[redacted]":2}` + redacted := redactScannerStdout(raw, env, []string{"SCANNER_ACCESS"}) + var parsed map[string]any + if err := json.Unmarshal([]byte(redacted), &parsed); err != nil { + t.Fatal(err) + } + if len(parsed) != 2 { + t.Fatalf("field dropped on key collision: %s", redacted) + } + if strings.Contains(redacted, "abc") { + t.Fatalf("secret key survived redaction: %s", redacted) + } + // Two secret keys redacting to the same marker must also both survive. + raw = `{"abc-primary":1,"abc-fallback":2}` + env = map[string]string{"SCANNER_ACCESS": "abc-primary", "SCANNER_ACCESS_2": "abc-fallback"} + redacted = redactScannerStdout(raw, env, []string{"SCANNER_ACCESS", "SCANNER_ACCESS_2"}) + parsed = nil + if err := json.Unmarshal([]byte(redacted), &parsed); err != nil { + t.Fatal(err) + } + if len(parsed) != 2 { + t.Fatalf("field dropped when two keys redact identically: %s", redacted) + } + for _, secret := range []string{"abc-primary", "abc-fallback"} { + if strings.Contains(redacted, secret) { + t.Fatalf("secret %q survived redaction: %s", secret, redacted) + } + } +} + +func TestRedactScannerStdoutDropsSecretsInDuplicateJSONKeys(t *testing.T) { + // Go's decoder keeps only an object's last duplicate member, so a + // secret hidden in the earlier duplicate is invisible to the node walk + // but still present in the raw bytes. The original text must not be + // returned verbatim in that case. + env := map[string]string{"DEMO_TOKEN": "sekret"} + raw := `{"auth":"sekret","auth":"safe"}` + redacted := redactScannerStdout(raw, env, []string{"DEMO_TOKEN"}) + if strings.Contains(redacted, "sekret") { + t.Fatalf("duplicate-key secret survived redaction: %s", redacted) + } + if !json.Valid([]byte(redacted)) { + t.Fatalf("redacted output is not valid JSON: %s", redacted) + } + // A duplicate secret value inside the duplicate key's value node too. + raw = `{"a":{"tok":"sekret","tok":"x"},"b":1}` + redacted = redactScannerStdout(raw, env, []string{"DEMO_TOKEN"}) + if strings.Contains(redacted, "sekret") { + t.Fatalf("nested duplicate-key secret survived: %s", redacted) + } + // No duplicates and no secrets: input must pass through untouched, + // preserving original formatting. + clean := `{"findings": [], + "note": "kept"}` + if got := redactScannerStdout(clean, env, []string{"DEMO_TOKEN"}); got != clean { + t.Fatalf("clean output was rewritten: %q", got) + } +} + +func TestHasDuplicateJSONKeys(t *testing.T) { + for raw, want := range map[string]bool{ + `{"a":1,"a":2}`: true, + `{"a":{"b":1,"b":2}}`: true, + `[{"x":1},{"x":1,"x":2}]`: true, + `{"a":1,"b":{"a":2}}`: false, + `{"a":"a"}`: false, + `[1,2,3]`: false, + `{"a":["a","a"]}`: false, + `{"a":{"b":1},"c":{"b":1}}`: false, + } { + if got := hasDuplicateJSONKeys(raw); got != want { + t.Fatalf("hasDuplicateJSONKeys(%s) = %v, want %v", raw, got, want) + } + } +} + +func TestScannerSecretValuesExemptsByNameNeverByValue(t *testing.T) { + // PASSWORD_STORE_ENABLE_EXTENSIONS is a known pass(1) toggle, exempted + // by NAME. DB_PASSWORD=default must still be swept — a weak credential + // holding a common-looking value is a credential all the same. + env := map[string]string{ + "PASSWORD_STORE_ENABLE_EXTENSIONS": "true", + "DB_PASSWORD": "default", + "CI_TOKEN": "real-secret", + } + secrets := scannerSecretValues(env, nil) + got := strings.Join(secrets, ",") + if strings.Contains(got, "true") { + t.Fatalf("known config toggle swept as secret: %v", secrets) + } + if !strings.Contains(got, "default") { + t.Fatalf("weak credential exempted by value: %v", secrets) + } + if !strings.Contains(got, "real-secret") { + t.Fatalf("real secret missing from sweep: %v", secrets) + } +} + +func TestScannerSecretValuesIncludesJSONCredentialLeaves(t *testing.T) { + credential := "sk-" + "live-cred" + jsonCredential := `{"to` + `ken":"` + credential + `"}` + env := map[string]string{"SCANNER_ACCESS": jsonCredential} + secrets := scannerSecretValues(env, []string{"SCANNER_ACCESS"}) + got := strings.Join(secrets, "\n") + for _, want := range []string{jsonCredential, credential} { + if !strings.Contains(got, want) { + t.Fatalf("secret %q missing from %v", want, secrets) + } + } +} + +func TestJSONSecretLeavesIncludesNumericCredential(t *testing.T) { + leaves := jsonSecretLeaves(`{"pin":123456}`) + if !slices.Contains(leaves, "123456") { + t.Fatalf("numeric JSON credential leaf missing from %v", leaves) + } +} + +func TestUserDefinedScannerRedactsNumericJSONCredentialLeaf(t *testing.T) { + adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ + ID: "demo", Command: "demo {{target}}", Env: []string{"SCANNER_ACCESS"}, Targets: []string{"skill"}, + }) + registry, err := NewScannerRegistry(adapter) + if err != nil { + t.Fatal(err) + } + commandRunner := &recordingCommandRunner{stdout: `{"pin":123456}`} + result, err := (ExternalScannerRunner{ + Registry: registry, CommandRunner: commandRunner, + Env: map[string]string{"SCANNER_ACCESS": `{"pin":123456}`}, + SandboxMode: SandboxModeOff, + }).RunScanner("demo", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "completed" { + t.Fatalf("status = %q, want completed", result.Status) + } + if strings.Contains(string(result.Raw), "123456") { + t.Fatalf("numeric JSON credential leaf survived redaction: %s", result.Raw) + } + if !strings.Contains(string(result.Raw), "[redacted]") { + t.Fatalf("expected redaction marker: %s", result.Raw) + } +} + +func TestRedactScannerStdoutScrubsReemittedJSONCredentialLeaves(t *testing.T) { + credential := "sk-" + "live-cred" + env := map[string]string{"SCANNER_ACCESS": `{"to` + `ken":"` + credential + `"}`} + redacted := redactScannerStdout(`{"auth":{"to`+`ken":"`+credential+`"}}`, env, []string{"SCANNER_ACCESS"}) + if strings.Contains(redacted, credential) { + t.Fatalf("JSON credential leaf survived redaction: %s", redacted) + } + if !strings.Contains(redacted, "[redacted]") { + t.Fatalf("expected redaction marker: %s", redacted) + } +} + +func TestScannerSecretValuesExcludesShortJSONCredentialLeaves(t *testing.T) { + env := map[string]string{"X": `{"v":"ab"}`} + secrets := scannerSecretValues(env, []string{"X"}) + for _, secret := range secrets { + if secret == "ab" { + t.Fatalf("short JSON credential leaf was included: %v", secrets) + } + } +} + +func TestJSONSecretLeavesExcludesShortNumericCredential(t *testing.T) { + leaves := jsonSecretLeaves(`{"count":42}`) + if slices.Contains(leaves, "42") { + t.Fatalf("short numeric JSON credential leaf was included: %v", leaves) + } + raw := `{"count":42}` + env := map[string]string{"SCANNER_ACCESS": raw} + if got := redactScannerStdout(raw, env, []string{"SCANNER_ACCESS"}); got != raw { + t.Fatalf("short numeric evidence was changed: got %s, want %s", got, raw) + } +} + +func TestRunScannerRedactsUndeclaredHostSecretsWithNoExposedNames(t *testing.T) { + // A built-in command scanner with no declared or passthrough env still + // inherits the whole host env with --sandbox off; the central redaction + // boundary must run despite ExposedEnvNames being empty. + commandRunner := &recordingCommandRunner{stdout: `{"echo":"host-secret-value"}`} + result, err := (ExternalScannerRunner{ + Registry: DefaultScannerRegistry(), CommandRunner: commandRunner, + Env: map[string]string{"CI_TOKEN": "host-secret-value"}, + ExposedEnvNames: nil, + SandboxMode: SandboxModeOff, + }).RunScanner("agentverus", t.TempDir(), "2026-07-21T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(result.Raw), "host-secret-value") { + t.Fatalf("built-in scanner leaked undeclared host secret with empty exposed names: %s", result.Raw) + } +} + func TestUserDefinedScannerRecordsExitCode(t *testing.T) { exitCode := 2 adapter := NewUserDefinedScanner(UserDefinedScannerConfig{ @@ -223,6 +1657,9 @@ func TestUserDefinedScannerInterpolatesDollarTargetLiterally(t *testing.T) { } commandRunner := &recordingCommandRunner{stdout: `{}`} target := filepath.Join(t.TempDir(), "skill-$USER") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatal(err) + } result, err := (ExternalScannerRunner{ Registry: registry, CommandRunner: commandRunner, Env: map[string]string{}, SandboxMode: SandboxModeOff, }).RunScanner("demo", target, "2026-07-21T00:00:00Z") diff --git a/internal/runner/snyk_scanner.go b/internal/runner/snyk_scanner.go index 9ee966a..1a69b7a 100644 --- a/internal/runner/snyk_scanner.go +++ b/internal/runner/snyk_scanner.go @@ -3,7 +3,6 @@ package runner import ( "encoding/json" "errors" - "strings" "time" ) @@ -23,7 +22,7 @@ func (runner ExternalScannerRunner) runSnyk(target string, startedAt string) (Sc } output, runErr := runner.CommandRunner.Run(command, args, "", timeout) completedAt := time.Now().UTC().Format(time.RFC3339Nano) - raw := strings.TrimSpace(output.Stdout) + raw := output.Stdout if runErr != nil { message := scannerCommandError(runErr, output.Stderr, runner.Env) if json.Valid([]byte(raw)) { diff --git a/internal/runner/socket_scanner.go b/internal/runner/socket_scanner.go index 3f37e8e..66492bc 100644 --- a/internal/runner/socket_scanner.go +++ b/internal/runner/socket_scanner.go @@ -3,7 +3,6 @@ package runner import ( "encoding/json" "errors" - "strings" "time" ) @@ -23,7 +22,7 @@ func (runner ExternalScannerRunner) runSocket(target string, startedAt string) ( } output, runErr := runner.CommandRunner.Run(command, args, "", timeout) completedAt := time.Now().UTC().Format(time.RFC3339Nano) - raw := strings.TrimSpace(output.Stdout) + raw := output.Stdout if runErr != nil { message := scannerCommandError(runErr, output.Stderr, runner.Env) if json.Valid([]byte(raw)) { diff --git a/internal/runner/user_defined_scanner.go b/internal/runner/user_defined_scanner.go index ceef300..727cad0 100644 --- a/internal/runner/user_defined_scanner.go +++ b/internal/runner/user_defined_scanner.go @@ -3,13 +3,19 @@ package runner import ( "encoding/json" "fmt" - "net/url" + "math/big" "os" - "path/filepath" + "path" "regexp" "runtime" + "sort" + "strconv" "strings" "time" + "unicode/utf16" + "unicode/utf8" + + "mvdan.cc/sh/v3/syntax" ) type UserDefinedScannerConfig struct { @@ -36,14 +42,14 @@ func (adapter userDefinedScannerAdapter) ID() string { return adapter.config.ID func (adapter userDefinedScannerAdapter) Requirements(_ map[string]string) []EnvRequirement { requirements := make([]EnvRequirement, 0, len(adapter.config.Env)) - for _, name := range adapter.config.Env { + for _, name := range sanitizedDeclaredEnvNames(adapter.config.Env) { requirements = append(requirements, EnvRequirement{EnvVar: name, Reason: adapter.config.ID + " scanner"}) } return requirements } func (adapter userDefinedScannerAdapter) Info() ScannerInfo { - return ScannerInfo{ID: adapter.config.ID, DisplayName: adapter.config.ID, RequiredEnv: append([]string(nil), adapter.config.Env...)} + return ScannerInfo{ID: adapter.config.ID, DisplayName: adapter.config.ID, RequiredEnv: sanitizedDeclaredEnvNames(adapter.config.Env)} } func (adapter userDefinedScannerAdapter) InstallPlan() InstallPlan { @@ -56,12 +62,52 @@ func (adapter userDefinedScannerAdapter) SupportsTargetKind(kind string) bool { func (adapter userDefinedScannerAdapter) CommandBacked() bool { return true } +// DeclaredCredentialEnv lists env vars that are credentials by declaration: +// whatever their spelling, a user-defined scanner's env: entries exist to +// hand the command secrets, so their values must always be redacted from +// persisted output. +func (adapter userDefinedScannerAdapter) DeclaredCredentialEnv() []string { + return sanitizedDeclaredEnvNames(adapter.config.Env) +} + func (adapter userDefinedScannerAdapter) Run(runner ExternalScannerRunner, target string, startedAt string) (ScannerResult, error) { + // A missing local target must fail here, before mount inference: Docker + // mount fallback binds a missing path's parent read-write (so scanners + // can create output files), and a typo'd target would hand the container + // writable access to the surrounding host directory. + if runner.TargetKind != targetKindURL { + if _, err := os.Stat(target); err != nil { + return ScannerResult{ + Status: "failed", StartedAt: startedAt, CompletedAt: time.Now().UTC().Format(time.RFC3339Nano), + Error: fmt.Sprintf("User-defined scanner %s target does not exist: %s", adapter.config.ID, target), + }, nil + } + } + // env: entries must be bare variable names: `env: [API_TOKEN=sk-live]` + // would otherwise flow into the missing-variable diagnostic verbatim, + // leaking the inline value into terminal and CI logs. + if bad := invalidDeclaredEnvName(adapter.config.Env); bad != "" { + return ScannerResult{ + Status: "failed", StartedAt: startedAt, CompletedAt: time.Now().UTC().Format(time.RFC3339Nano), + Error: fmt.Sprintf("User-defined scanner %s env entry %s is not a variable name; declare bare names and set values in the environment", adapter.config.ID, bad), + }, nil + } + if commandReparsesTarget(adapter.config.Command) { + return ScannerResult{ + Status: "failed", StartedAt: startedAt, CompletedAt: time.Now().UTC().Format(time.RFC3339Nano), + Error: fmt.Sprintf("User-defined scanner %s re-parses the interpolated target as shell code (eval, or a shell interpreter's -c/-Command operand); a target containing shell metacharacters would execute — remove the reparsing construct or drop the {{target}} placeholder", adapter.config.ID), + }, nil + } shell := userDefinedScannerShell(runtime.GOOS, runner.SandboxMode) targetReplacement := shell.quote(target) usePositionalTarget := shell.command == "/bin/sh" if usePositionalTarget { targetReplacement = `"$1"` + } else if unsafeWindowsShellTarget(target) { + return ScannerResult{ + Status: "failed", StartedAt: startedAt, CompletedAt: time.Now().UTC().Format(time.RFC3339Nano), + Error: fmt.Sprintf(`User-defined scanner %s cannot receive targets containing %%, !, or " on the Windows host shell; use the Docker sandbox or a target without those characters`, adapter.config.ID), + }, nil } rendered := targetPlaceholderPattern.ReplaceAllStringFunc(adapter.config.Command, func(string) string { return targetReplacement @@ -70,18 +116,80 @@ func (adapter userDefinedScannerAdapter) Run(runner ExternalScannerRunner, targe if usePositionalTarget { args = append(args, "clawscan-target", target) } - fullCommand := append([]string{shell.command}, args...) + // The artifact records only the scanner ID, never the rendered command + // line: a user-defined command is operator-authored config and may embed + // inline credentials (API_TOKEN=sk-live scanner {{target}}), which must + // not be persisted into evidence. + fullCommand := []string{"user-defined-scanner", adapter.config.ID} timeout := runner.Timeout if timeout == 0 { timeout = 20 * time.Minute } - output, runErr := runner.CommandRunner.Run(shell.command, args, userDefinedScannerCWD(target), timeout) + // Never run with an untrusted working directory: CWD-sensitive commands + // (python -m, npx) would resolve target-supplied code with scanner + // credentials in env. An empty cwd inherits ClawScan's own process cwd, + // which may itself be the untrusted target (clawscan .), so host runs get + // a fresh empty directory. Docker runs keep "" — with no cwd nothing is + // mounted and the container's own workdir is already isolated. + cwd := "" + if runner.SandboxMode != SandboxModeDocker { + isolated, err := os.MkdirTemp("", "clawscan-scanner-") + if err != nil { + return ScannerResult{ + Status: "failed", StartedAt: startedAt, CompletedAt: time.Now().UTC().Format(time.RFC3339Nano), + Error: fmt.Sprintf("User-defined scanner %s could not create an isolated working directory: %v", adapter.config.ID, err), + }, nil + } + defer os.RemoveAll(isolated) + cwd = isolated + } + output, runErr := runner.CommandRunner.Run(shell.command, args, cwd, timeout) exitCode := gateEligibleExitCode(output.ExitCode) completedAt := time.Now().UTC().Format(time.RFC3339Nano) - raw := strings.TrimSpace(output.Stdout) + // Raw stdout is persisted into the artifact and per-scanner output files, + // so credentials must be scrubbed from it, not just from failure text. + // Only valid JSON is ever persisted (both paths below gate on json.Valid), + // so structural redaction of decoded strings suffices — and byte-level + // replacement must not run first, or a short secret like "1" would corrupt + // non-string JSON tokens and flip a healthy scan to failed. + // Scrub this adapter's declared env plus everything else exposed to + // scanners this run (sandbox allowlist, other adapters' credentials): + // under Docker every scanner sees the whole passthrough set, and a name + // like BETA_LICENSE evades the isSecretEnvKey heuristic. + scrubNames := append(append([]string(nil), adapter.config.Env...), runner.ExposedEnvNames...) + // Under Docker only allowlisted names reach the container; scrubbing an + // unrelated host secret's value (CI_TOKEN=clean) would corrupt evidence + // without preventing any leak. --sandbox off keeps the full host env. + visibleEnv := commandVisibleEnv(runner.Env, scrubNames, runner.SandboxMode) + stdout := output.Stdout + // Evidence is rejected outright — not repaired — when structural + // redaction cannot see everything the raw bytes hold: invalid UTF-8 + // defeats byte-exact secret comparison after decoding, and duplicate + // object members hide earlier values from the decoded walk while + // re-encoding would silently rewrite the evidence. + evidenceUnsafe := "" + if json.Valid([]byte(stdout)) { + evidenceUnsafe = scannerEvidenceUnsafe(stdout) + } + raw := redactScannerStdout(stdout, visibleEnv, scrubNames) + if evidenceUnsafe != "" { + return ScannerResult{ + Status: "failed", StartedAt: startedAt, CompletedAt: completedAt, Command: fullCommand, + Error: fmt.Sprintf("User-defined scanner %s output rejected: %s", adapter.config.ID, evidenceUnsafe), ExitCode: exitCode, + }, nil + } if runErr != nil { - message := commandError(runErr, output.Stderr, runner.Env) - if json.Valid([]byte(raw)) { + // Failure text needs the same coverage as stdout: declared env plus + // everything exposed to scanners this run, whatever the spelling. + // commandError's own secret-named sweep must also use the visible + // env: pre-scrubbing with the whole host env would corrupt Docker + // diagnostics matching an unexposed host value by coincidence. + message := redactDeclaredEnvValues(commandError(runErr, output.Stderr, visibleEnv), visibleEnv, scrubNames) + // Valid JSON is completed evidence only for a normal nonzero exit + // (findings-mean-nonzero scanners). A nil gate-eligible exit code + // means timeout or signal: partial output must not report success + // or let exit-code gates pass. + if exitCode != nil && json.Valid([]byte(raw)) { return ScannerResult{ Status: "completed", StartedAt: startedAt, CompletedAt: completedAt, Command: fullCommand, Error: message, ExitCode: exitCode, Raw: json.RawMessage(raw), @@ -104,13 +212,892 @@ func (adapter userDefinedScannerAdapter) Run(runner ExternalScannerRunner, targe }, nil } +// unsafeWindowsShellTarget reports whether a target cannot be interpolated +// into a cmd.exe command line safely: %VAR% expands even inside double +// quotes, !VAR! does too when delayed expansion is enabled (a system-wide +// or shell-level setting ClawScan cannot detect), and backslash does not +// escape " for cmd.exe's parser, so a quote in a URL target could terminate +// the argument and inject host commands. +func unsafeWindowsShellTarget(target string) bool { + return strings.ContainsAny(target, `%"!`) +} + +const redactionMarkerPreferred = "[redacted]" + +var redactionMarkerRunes = []rune("#*~^@+=%&|:;") + +// secretScrubber replaces secret values with a redaction marker and verifies +// the result no longer contains any secret. Naive replacement leaks twice +// over: a credential that is a substring of the marker (such as "act" or +// "redacted") survives inside the sentinel itself, and replacement can +// synthesize a secret from marker bytes joined with surrounding text. +type secretScrubber struct { + secrets []string + marker string +} + +func newSecretScrubber(secrets []string) secretScrubber { + return secretScrubber{secrets: secrets, marker: redactionMarker(secrets)} +} + +func (scrubber secretScrubber) containsSecret(value string) bool { + for _, secret := range scrubber.secrets { + if secret != "" && strings.Contains(value, secret) { + return true + } + } + return false +} + +// scrub replaces every secret occurrence, then re-checks the result because +// a replacement can reintroduce a secret at marker/text boundaries. If +// bounded marker passes do not converge, it deletes the secret bytes +// outright, which always terminates because each pass strictly shrinks the +// text. +func (scrubber secretScrubber) scrub(value string) string { + const maxMarkerPasses = 4 + redacted := value + for pass := 0; pass < maxMarkerPasses; pass++ { + if !scrubber.containsSecret(redacted) { + return redacted + } + for _, secret := range scrubber.secrets { + if secret == "" { + continue + } + redacted = strings.ReplaceAll(redacted, secret, scrubber.marker) + } + } + for scrubber.containsSecret(redacted) { + for _, secret := range scrubber.secrets { + if secret == "" { + continue + } + redacted = strings.ReplaceAll(redacted, secret, "") + } + } + return redacted +} + +// scrubDeep scrubs value and then decodes it to a fixed point: a secret can +// hide one JSON-escaping layer down ({"message":"{\"auth\":\"\\u0073ekret\"}"}) +// where neither the raw bytes nor the once-decoded string contain the +// literal. Each decode layer that surfaces a secret is scrubbed in place; +// losing the original escaping is acceptable, leaking is not. +func (scrubber secretScrubber) scrubDeep(value string) string { + result := scrubber.scrub(value) + current := result + // Decode to a fixed point. Termination is guaranteed: every escape + // decodeJSONStringEscapes rewrites (\", \\, \/, \b, \f, \n, \r, \t, + // \uXXXX) strictly + // shrinks the string, and unknown escapes pass through unchanged, so + // decoded != current implies len(decoded) < len(current). + for { + decoded := decodeJSONStringEscapes(current) + if decoded == current { + return result + } + if scrubber.containsSecret(decoded) { + decoded = scrubber.scrub(decoded) + result = decoded + } + current = decoded + } +} + +// redactionMarker picks the replacement sentinel. The preferred marker is +// only used when no secret is a substring of it. Otherwise the marker is a +// repeated rune absent from every secret, so the sentinel can neither +// contain a credential nor combine with neighboring text to rebuild one. +func redactionMarker(secrets []string) string { + preferredSafe := true + for _, secret := range secrets { + if secret != "" && strings.Contains(redactionMarkerPreferred, secret) { + preferredSafe = false + break + } + } + if preferredSafe { + return redactionMarkerPreferred + } + if safe := secretFreeRune(secrets); safe != "" { + return strings.Repeat(safe, 10) + } + // Unreachable for realistic secret sets (secrets would need to cover + // every candidate rune); scrub then deletes the bytes as a last resort. + return "" +} + +// secretFreeRune returns a one-rune string absent from every secret, so text +// built from it can neither contain a credential nor combine with adjacent +// bytes to rebuild one. Empty only when the secrets cover every candidate +// rune, which no realistic secret set does. +func secretFreeRune(secrets []string) string { + runeSafe := func(r rune) bool { + for _, secret := range secrets { + if strings.ContainsRune(secret, r) { + return false + } + } + return true + } + for _, r := range redactionMarkerRunes { + if runeSafe(r) { + return string(r) + } + } + for r := rune(0x2580); r <= 0x25ff; r++ { + if runeSafe(r) { + return string(r) + } + } + return "" +} + func gateEligibleExitCode(exitCode *int) *int { - if exitCode == nil || *exitCode < 0 { + // Shells and docker report a signal-killed child as 128+N (137 for + // SIGKILL/OOM, 143 for SIGTERM) with a normal ProcessState. Docker reserves + // 125 for docker-run failure, and shells reserve 126/127 for + // not-executable/not-found. None of these are scanner verdicts: treating + // them as gate-eligible would let partial output from a failed scan pass an + // exit-code gate. + if exitCode == nil || *exitCode < 0 || *exitCode >= 125 { return nil } return exitCode } +func redactDeclaredEnvValues(value string, env map[string]string, declared []string) string { + if value == "" || len(env) == 0 { + return value + } + // Failure text needs the same secret set as stdout — declared env plus + // every secret-named var the scanner may see undeclared (--sandbox off + // inherits the whole host environment) — and each secret's JSON-escaped + // form too: a value like pa"ss emitted inside JSON on stderr appears as + // pa\"ss, so the literal never occurs in the bytes. + base := scannerSecretValues(env, declared) + secrets := make([]string, 0, len(base)*2) + for _, secret := range base { + secrets = append(secrets, secret) + if encoded, err := json.Marshal(secret); err == nil { + if escaped := strings.Trim(string(encoded), `"`); escaped != secret { + secrets = append(secrets, escaped) + } + } + } + sort.Slice(secrets, func(i int, j int) bool { + return len(secrets[i]) > len(secrets[j]) + }) + // JSON permits alternate encodings of the same string (a\/b for a/b, + // \u escapes) that byte replacement cannot enumerate, and a secret can + // hide multiple escaping layers down (JSON embedded in a JSON string). + // scrubDeep decodes to a fixed point and scrubs whatever surfaces — + // losing the original escaping is acceptable, leaking is not. + return newSecretScrubber(secrets).scrubDeep(value) +} + +// decodeJSONStringEscapes interprets JSON string escape sequences (\", \\, +// \/, \b, \f, \n, \r, \t, and \uXXXX including surrogate pairs) embedded in +// free-form text. Unknown or truncated sequences pass through unchanged. +func decodeJSONStringEscapes(value string) string { + if !strings.Contains(value, `\`) { + return value + } + var out strings.Builder + out.Grow(len(value)) + for index := 0; index < len(value); { + if value[index] != '\\' || index+1 >= len(value) { + out.WriteByte(value[index]) + index++ + continue + } + switch value[index+1] { + case '"', '\\', '/': + out.WriteByte(value[index+1]) + index += 2 + case 'b': + out.WriteByte('\b') + index += 2 + case 'f': + out.WriteByte('\f') + index += 2 + case 'n': + out.WriteByte('\n') + index += 2 + case 'r': + out.WriteByte('\r') + index += 2 + case 't': + out.WriteByte('\t') + index += 2 + case 'u': + if index+6 > len(value) { + out.WriteByte(value[index]) + index++ + continue + } + first, err := strconv.ParseUint(value[index+2:index+6], 16, 32) + if err != nil { + out.WriteByte(value[index]) + index++ + continue + } + decoded := rune(first) + width := 6 + if utf16.IsSurrogate(decoded) && index+12 <= len(value) && value[index+6] == '\\' && value[index+7] == 'u' { + if second, err := strconv.ParseUint(value[index+8:index+12], 16, 32); err == nil { + if combined := utf16.DecodeRune(decoded, rune(second)); combined != utf8.RuneError { + decoded = combined + width = 12 + } + } + } + out.WriteRune(decoded) + index += width + default: + out.WriteByte(value[index]) + index++ + } + } + return out.String() +} + +// redactScannerStdout scrubs credentials from scanner stdout before it is +// persisted. Only valid JSON ever reaches the artifact, and JSON permits +// alternative encodings of the same string ("a\/b", \u escapes), so redaction +// is structural: decode, scrub every string node, re-serialize. Byte-level +// replacement is deliberately not used here — a short secret such as "1" +// would corrupt non-string tokens ({"count":1}) and flip a healthy scan to +// failed. Non-JSON input is returned unchanged; the failure-text path redacts +// it separately. +// +// The scanner sees more than its declared env (--sandbox off inherits the +// process environment; Docker passes --sandbox-env and other adapters' +// credentials), so every secret-named env value is scrubbed too, not just +// declared ones. +func redactScannerStdout(value string, env map[string]string, declared []string) string { + if value == "" || len(env) == 0 || !json.Valid([]byte(value)) { + return value + } + secrets := scannerSecretValues(env, declared) + if len(secrets) == 0 { + return value + } + var document any + decoder := json.NewDecoder(strings.NewReader(value)) + decoder.UseNumber() + if err := decoder.Decode(&document); err != nil { + return value + } + document, changed := redactJSONStrings(document, newSecretScrubber(secrets)) + // Returning the original verbatim is only safe when the decoded walk saw + // everything the raw bytes hold. Two cases defeat it: Go's decoder keeps + // only an object's last duplicate member, hiding a secret in an earlier + // duplicate ({"auth":"sekret","auth":"x"}), and invalid UTF-8 decodes to + // U+FFFD so a byte-exact secret never matches. Re-encoding the decoded + // document destroys both hazards the same way any JSON consumer would + // read the text. Paths that persist scanner evidence reject these inputs + // outright (scannerEvidenceUnsafe) before evidence acceptance; this + // re-encode is the fail-safe for diagnostic and judge-output paths. + if !changed && !hasDuplicateJSONKeys(value) && utf8.ValidString(value) { + return value + } + encoded, err := json.Marshal(document) + if err != nil { + return value + } + return string(encoded) +} + +// scannerEvidenceUnsafe reports why valid-JSON evidence cannot be safely +// persisted through structural redaction, or "" when it can. Invalid UTF-8 +// survives json.Valid but decoding swaps the bytes for U+FFFD, so a +// byte-exact secret comparison misses and the original bytes would persist. +// Duplicate object members are invisible to the decoded walk (only the last +// survives), and re-encoding to drop them would silently rewrite evidence. +// Both cases fail closed: the callers reject the evidence outright. +func scannerEvidenceUnsafe(value string) string { + if !utf8.ValidString(value) { + return "contains invalid UTF-8" + } + if hasDuplicateJSONKeys(value) { + return "contains duplicate JSON object members" + } + return "" +} + +// hasDuplicateJSONKeys reports whether any object in the JSON text declares +// the same member name twice. Only valid JSON reaches this scan, so a token +// error just reports false and the caller keeps the original text. +func hasDuplicateJSONKeys(value string) bool { + decoder := json.NewDecoder(strings.NewReader(value)) + decoder.UseNumber() + type frame struct { + object bool + keys map[string]bool + expectKey bool + } + var stack []*frame + for { + token, err := decoder.Token() + if err != nil { + return false + } + top := func() *frame { + if len(stack) == 0 { + return nil + } + return stack[len(stack)-1] + } + switch typed := token.(type) { + case json.Delim: + switch typed { + case '{': + stack = append(stack, &frame{object: true, keys: map[string]bool{}, expectKey: true}) + case '[': + stack = append(stack, &frame{}) + case '}', ']': + stack = stack[:len(stack)-1] + if parent := top(); parent != nil && parent.object { + parent.expectKey = true + } + } + case string: + if current := top(); current != nil && current.object && current.expectKey { + if current.keys[typed] { + return true + } + current.keys[typed] = true + current.expectKey = false + continue + } + if current := top(); current != nil && current.object { + current.expectKey = true + } + default: + if current := top(); current != nil && current.object { + current.expectKey = true + } + } + if len(stack) == 0 && decoder.More() == false { + return false + } + } +} + +// commandVisibleEnv returns the env whose values feed redaction for a +// command that ran under the given sandbox mode. With --sandbox off the +// command inherits the whole host environment, so every secret-named var is +// in scope. Under Docker only the allowlisted names reach the container: +// scrubbing an unrelated host value (CI_TOKEN=clean) would rewrite +// legitimate evidence like "verdict":"clean" without preventing any leak. +func commandVisibleEnv(env map[string]string, names []string, sandboxMode string) map[string]string { + if sandboxMode != SandboxModeDocker { + return env + } + visible := make(map[string]string, len(names)) + for _, name := range names { + for key, value := range envEntriesForName(env, name) { + visible[key] = value + } + } + return visible +} + +// envEntriesForName returns the env entries a declared name addresses, +// keyed by their real spelling. On Windows environment names are +// case-insensitive: a scanner declaring scanner_access receives the host's +// SCANNER_ACCESS=secret, so redaction must see that value under either +// spelling or the credential persists in evidence. Elsewhere names are +// distinct and only the exact key matches. +func envEntriesForName(env map[string]string, name string) map[string]string { + return envEntriesForNameOnGOOS(env, name, runtime.GOOS) +} + +func envEntriesForNameOnGOOS(env map[string]string, name string, goos string) map[string]string { + entries := map[string]string{} + if value, ok := env[name]; ok { + entries[name] = value + } + if goos != "windows" { + return entries + } + for key, value := range env { + if strings.EqualFold(key, name) { + entries[key] = value + } + } + return entries +} + +// envValueForName returns a non-empty value for name under the platform's +// name-equality rules (case-insensitive on Windows), or "". +func envValueForName(env map[string]string, name string) string { + for _, value := range envEntriesForName(env, name) { + if strings.TrimSpace(value) != "" { + return value + } + } + return "" +} + +// scannerSecretValues collects the env values to scrub from scanner output: +// declared env vars are credentials by declaration whatever their spelling, +// and secret-named vars (isSecretEnvKey) are included because the scanner may +// see them without declaring them. Longest-first so overlapping secrets +// redact fully. +func scannerSecretValues(env map[string]string, declared []string) []string { + seen := map[string]bool{} + secrets := make([]string, 0, len(declared)) + add := func(secret string) { + if strings.TrimSpace(secret) == "" || seen[secret] { + return + } + seen[secret] = true + secrets = append(secrets, secret) + } + addWithJSONLeaves := func(secret string) { + add(secret) + for _, leaf := range jsonSecretLeaves(secret) { + add(leaf) + } + } + for _, name := range declared { + // Windows env names are case-insensitive: a declared + // scanner_access must also sweep the host's SCANNER_ACCESS value. + for _, value := range envEntriesForName(env, name) { + addWithJSONLeaves(value) + } + } + for name, secret := range env { + // Exemptions are by name, never by value: DB_PASSWORD=default is a + // weak credential, not configuration, and skipping "default" would + // persist it unredacted. Names isSecretEnvKey over-matches are + // listed in nonCredentialSecretNamedEnv instead. + if isSecretEnvKey(name) && !nonCredentialSecretNamedEnv(name) { + addWithJSONLeaves(secret) + } + } + sort.Slice(secrets, func(i int, j int) bool { + return len(secrets[i]) > len(secrets[j]) + }) + return secrets +} + +// jsonSecretLeaves returns string and numeric scalar leaf values of secret when +// secret is a JSON object or array, so a structurally re-emitted JSON credential +// ({"token":"sk-live"} surfacing as {"auth":{"token":"sk-live"}}) is redacted +// leaf by leaf. Only scalars of length >= 5 are returned; booleans, nulls, and +// short scalars are excluded to avoid over-redacting legitimate evidence. +// Duplicate object members inside a credential value remain an exotic residual: +// JSON decoding keeps only the last member. +func jsonSecretLeaves(secret string) []string { + trimmed := strings.TrimSpace(secret) + if len(trimmed) == 0 || (trimmed[0] != '{' && trimmed[0] != '[') || !json.Valid([]byte(trimmed)) { + return nil + } + var doc any + decoder := json.NewDecoder(strings.NewReader(trimmed)) + decoder.UseNumber() + if err := decoder.Decode(&doc); err != nil { + return nil + } + var leaves []string + var walk func(any) + walk = func(node any) { + switch value := node.(type) { + case map[string]any: + for _, child := range value { + walk(child) + } + case []any: + for _, child := range value { + walk(child) + } + case string: + if len(value) >= 5 { + leaves = append(leaves, value) + } + case json.Number: + if len(string(value)) >= 5 { + leaves = append(leaves, string(value)) + } + } + } + walk(doc) + return leaves +} + +var envAssignmentNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + +// commandReparsesTarget reports whether a target placeholder is evaluated by +// eval, a shell interpreter's command-string operand, or command substitution. +func commandReparsesTarget(command string) bool { + if !targetPlaceholderPattern.MatchString(command) { + return false + } + file, err := syntax.NewParser().Parse(strings.NewReader(command), "") + if err != nil { + // Unparseable as bash means the default-path /bin/sh rejects it too, so + // the reparsing construct never executes; the target's "$1" quoting is + // the primary defense regardless. Treat as no reparse. + return false + } + + reparses := false + syntax.Walk(file, func(node syntax.Node) bool { + if reparses { + return false + } + switch node := node.(type) { + case *syntax.CmdSubst: + reparses = nodeContainsTarget(command, node) + return !reparses + case *syntax.CallExpr: + if len(node.Args) > 0 { + // The command word is what actually executes. Fail closed if it + // IS the interpolated target (running the scanned artifact as a + // program) or is a dynamic word that could resolve to an + // interpreter while the target appears elsewhere in the call. + cmdWord := node.Args[0] + _, static := staticWord(cmdWord) + if nodeContainsTarget(command, cmdWord) || (!static && nodeContainsTarget(command, node)) { + reparses = true + return false + } + } + interpreter, args, eval := reparsingCommand(node) + if eval { + reparses = true + return false + } + if interpreter != "" { + reparses = interpreterReparsesTarget(command, interpreter, args) + } + } + return !reparses + }) + return reparses +} + +func interpreterReparsesTarget(command string, interpreter string, args []*syntax.Word) bool { + for i := 1; i < len(args); i++ { + word, ok := staticWord(args[i]) + if !ok || !isCommandStringFlag(interpreter, word) || i+1 >= len(args) { + continue + } + switch interpreter { + case "cmd", "cmd.exe", "powershell", "powershell.exe", "pwsh": + for _, arg := range args[i+1:] { + if nodeContainsTarget(command, arg) { + return true + } + } + return false + default: + return nodeContainsTarget(command, args[i+1]) + } + } + return false +} + +func reparsingCommand(call *syntax.CallExpr) (interpreter string, args []*syntax.Word, eval bool) { + if len(call.Args) == 0 { + return "", nil, false + } + commandWord, ok := staticWord(call.Args[0]) + if !ok { + return "", nil, false + } + base := strings.ToLower(path.Base(commandWord)) + if base == "eval" { + return "", nil, true + } + if reparsingInterpreterWords[base] { + return base, call.Args, false + } + if !launcherWords[base] { + return "", nil, false + } + for i, arg := range call.Args[1:] { + word, ok := staticWord(arg) + if !ok { + continue + } + base = strings.ToLower(path.Base(word)) + if base == "eval" { + return "", nil, true + } + if reparsingInterpreterWords[base] { + return base, call.Args[i+1:], false + } + } + return "", nil, false +} + +func nodeContainsTarget(command string, node syntax.Node) bool { + start, end := int(node.Pos().Offset()), int(node.End().Offset()) + return start >= 0 && end >= start && end <= len(command) && targetPlaceholderPattern.MatchString(command[start:end]) +} + +// launcherWords exec their trailing arguments as a command, so the real +// interpreter (sh -c ...) can hide behind one. Skip them plus their own +// options and NAME=value assignments when locating the command word. +var launcherWords = map[string]bool{ + "env": true, "command": true, "exec": true, "sudo": true, + "nohup": true, "nice": true, "timeout": true, "stdbuf": true, +} + +var reparsingInterpreterWords = map[string]bool{ + "sh": true, "bash": true, "zsh": true, "dash": true, "ksh": true, "ash": true, + "cmd": true, "cmd.exe": true, + "powershell": true, "powershell.exe": true, "pwsh": true, +} + +func isCommandStringFlag(interpreter string, token string) bool { + switch interpreter { + case "cmd", "cmd.exe": + t := strings.ToLower(token) + return t == "/c" || t == "/k" + case "powershell", "powershell.exe", "pwsh": + t := strings.ToLower(token) + return t == "-c" || t == "-command" || t == "-encodedcommand" + default: + return !strings.HasPrefix(token, "--") && strings.HasPrefix(token, "-") && strings.ContainsRune(token[1:], 'c') + } +} + +func staticWord(word *syntax.Word) (string, bool) { + var text strings.Builder + if !appendStaticWordParts(&text, word.Parts, false) { + return "", false + } + return text.String(), true +} + +func appendStaticWordParts(text *strings.Builder, parts []syntax.WordPart, quoted bool) bool { + for _, part := range parts { + switch part := part.(type) { + case *syntax.Lit: + for i := 0; i < len(part.Value); i++ { + if part.Value[i] == '\\' && !quoted && i+1 < len(part.Value) { + i++ + } + text.WriteByte(part.Value[i]) + } + case *syntax.SglQuoted: + text.WriteString(part.Value) + case *syntax.DblQuoted: + if !appendStaticWordParts(text, part.Parts, true) { + return false + } + default: + return false + } + } + return true +} + +// sanitizedDeclaredEnvNames returns the adapter's env: declarations with any +// malformed assignment entry truncated to its name part. The text after = in +// a misconfigured entry (API_TOKEN=sk-live) may be a live credential and must +// never flow into artifact metadata, requirements diagnostics, or redaction +// name lists. +func sanitizedDeclaredEnvNames(declared []string) []string { + sanitized := make([]string, 0, len(declared)) + for _, name := range declared { + if !envAssignmentNamePattern.MatchString(name) { + if eq := strings.IndexByte(name, '='); eq >= 0 { + name = name[:eq] + } + } + if name == "" { + continue + } + sanitized = append(sanitized, name) + } + return sanitized +} + +// invalidDeclaredEnvName reports the name portion of a scanner env: +// declaration that is not a bare variable name, or "". A misconfiguration +// like `env: [API_TOKEN=sk-live]` must be rejected without ever echoing +// the full entry — the value after = may be a live credential, and the +// missing-variable diagnostic would otherwise print it verbatim. +func invalidDeclaredEnvName(declared []string) string { + for i, name := range declared { + if envAssignmentNamePattern.MatchString(name) { + continue + } + if eq := strings.IndexByte(name, '='); eq >= 0 { + return name[:eq] + "=..." + } + // A malformed entry with no '=' may itself be a pasted credential + // (env: [sk-live-...]); never echo it. Identify it by position. + return fmt.Sprintf("#%d", i+1) + } + return "" +} + +// InvalidUserDefinedEnvName reports a safe descriptor for the offending entry +// of a user-defined scanner env: list that is not a bare variable name, or "" +// when all entries are valid. +func InvalidUserDefinedEnvName(declared []string) string { + return invalidDeclaredEnvName(declared) +} + +// nonCredentialSecretNamedEnvNames lists names isSecretEnvKey matches that +// are well-known configuration switches, not credentials. Exempting by NAME +// is deliberate: exempting by value (skipping "true" or "default") would +// persist a weak real credential like DB_PASSWORD=default unredacted, while +// sweeping a toggle's value would corrupt every matching scalar in +// legitimate scanner evidence. Unknown secret-named vars stay fail-closed +// as credentials. +var nonCredentialSecretNamedEnvNames = map[string]bool{ + // pass(1) configuration (PASSWORD_STORE_* settings hold directories, + // booleans, lengths, and seconds — never the store's secrets). Every + // entry here must actually match isSecretEnvKey; names it already + // ignores (GIT_ASKPASS, TOKENIZERS_PARALLELISM) do not belong. + "PASSWORD_STORE_DIR": true, + "PASSWORD_STORE_ENABLE_EXTENSIONS": true, + "PASSWORD_STORE_EXTENSIONS_DIR": true, + "PASSWORD_STORE_GENERATED_LENGTH": true, + "PASSWORD_STORE_CHARACTER_SET": true, + "PASSWORD_STORE_CLIP_TIME": true, + "PASSWORD_STORE_UMASK": true, + "PASSWORD_STORE_X_SELECTION": true, +} + +func nonCredentialSecretNamedEnv(name string) bool { + return nonCredentialSecretNamedEnvNames[strings.ToUpper(name)] +} + +func redactJSONStrings(node any, scrubber secretScrubber) (any, bool) { + switch typed := node.(type) { + case string: + // scrubDeep also catches secrets one JSON-escaping layer down: a + // string node holding embedded JSON ("{\"auth\":\"\\u0073ekret\"}") + // never contains the secret's literal bytes. + redacted := scrubber.scrubDeep(typed) + return redacted, redacted != typed + case json.Number: + // A numeric-looking secret (PIN=1234) may be emitted unquoted; the + // scalar's exact text matching a secret is a leak like any other, + // and so is an alternate spelling of the same number (1e3 for a + // secret of 1000) — compare canonical values too. + for _, secret := range scrubber.secrets { + if string(typed) == secret || sameCanonicalNumber(string(typed), secret) { + return scrubber.marker, true + } + } + return typed, false + case bool: + for _, secret := range scrubber.secrets { + if strconv.FormatBool(typed) == secret { + return scrubber.marker, true + } + } + return typed, false + case nil: + for _, secret := range scrubber.secrets { + if secret == "null" { + return scrubber.marker, true + } + } + return typed, false + case map[string]any: + // Two-phase rebuild: unchanged keys keep their entries first, then + // redacted keys are inserted collision-safe. Assigning a redacted key + // directly could overwrite an unrelated field whose key already + // equals the marker (or a second secret key redacting to the same + // marker), silently dropping evidence. Keys are processed in sorted + // order so collision suffixes are deterministic. + changed := false + keys := make([]string, 0, len(typed)) + for key := range typed { + keys = append(keys, key) + } + sort.Strings(keys) + out := make(map[string]any, len(typed)) + var renamedKeys []string + renamedChildren := map[string]any{} + renames := map[string]string{} + for _, key := range keys { + redactedChild, childChanged := redactJSONStrings(typed[key], scrubber) + redactedKey, keyChanged := redactJSONStrings(key, scrubber) + if keyChanged { + renamedKeys = append(renamedKeys, key) + renamedChildren[key] = redactedChild + renames[key] = redactedKey.(string) + changed = true + continue + } + out[key] = redactedChild + if childChanged { + changed = true + } + } + for _, key := range renamedKeys { + out[collisionFreeKey(renames[key], out, scrubber)] = renamedChildren[key] + } + return out, changed + case []any: + changed := false + for index, child := range typed { + redactedChild, childChanged := redactJSONStrings(child, scrubber) + if childChanged { + typed[index] = redactedChild + changed = true + } + } + return typed, changed + default: + return node, false + } +} + +// sameCanonicalNumber reports whether a JSON number token and a secret are +// the same numeric value in different spellings (1e3 vs 1000, 1.0 vs 1). +// Comparison is exact (arbitrary-precision rationals), not float64: two +// distinct 17-digit integers that round to the same float must not be +// conflated, or unrelated evidence would be scrubbed as a secret. Both +// sides must parse as numbers; a non-numeric secret never matches. +func sameCanonicalNumber(token string, secret string) bool { + tokenValue, ok := new(big.Rat).SetString(token) + if !ok { + return false + } + secretValue, ok := new(big.Rat).SetString(secret) + if !ok { + return false + } + return tokenValue.Cmp(secretValue) == 0 +} + +// collisionFreeKey returns candidate if no map entry holds it, otherwise +// candidate extended with repetitions of a rune absent from every secret: +// any substring spanning the appended boundary contains that rune, so the +// suffix can neither be nor complete a credential. Terminates because taken +// is finite and each repetition count yields a distinct key. +func collisionFreeKey(candidate string, taken map[string]any, scrubber secretScrubber) string { + if _, exists := taken[candidate]; !exists { + return candidate + } + safe := secretFreeRune(scrubber.secrets) + if safe == "" { + // Unreachable for realistic secret sets (they would need to cover + // every candidate rune); matches redactionMarker's last resort. + safe = "␀" + } + for count := 1; ; count++ { + next := candidate + strings.Repeat(safe, count) + if _, exists := taken[next]; !exists { + return next + } + } +} + func userDefinedScannerShell(goos string, sandboxMode string) judgeShellSpec { if sandboxMode == SandboxModeDocker { return judgeShellForGOOS("linux") @@ -119,15 +1106,3 @@ func userDefinedScannerShell(goos string, sandboxMode string) judgeShellSpec { } var targetPlaceholderPattern = regexp.MustCompile(`\{\{\s*target\s*\}\}`) - -func userDefinedScannerCWD(target string) string { - parsed, err := url.Parse(target) - if err == nil && parsed.Scheme != "" && parsed.Host != "" { - return "" - } - info, err := os.Stat(target) - if err == nil && info.IsDir() { - return target - } - return filepath.Dir(target) -} diff --git a/skills/clawscan-cli/SKILL.md b/skills/clawscan-cli/SKILL.md index c8a742e..66b6cd3 100644 --- a/skills/clawscan-cli/SKILL.md +++ b/skills/clawscan-cli/SKILL.md @@ -85,8 +85,8 @@ Built-in profiles: Profiles are loaded from embedded built-ins. Project `.clawscan.yml` / `.clawscan.yaml` files are NOT loaded automatically: pass `--config ` to -load a specific config, or `--discover-config` to load the nearest one found -upward from the current directory. A project profile with the same name shadows +load a specific config, or `--discover-config` (with `--profile`) to load the +nearest one found upward from the current directory. A project profile with the same name shadows the built-in whole profile. `--config ` without `--profile` runs every profile in that config and emits a `clawscan-batch-v1` artifact. Discovery is off by default because project configs can define scanner commands that execute