From 04c6ca87461e1985350885989ed8a1ec33f81514 Mon Sep 17 00:00:00 2001 From: Jesse Merhi <79823012+jesse-merhi@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:26:48 +1000 Subject: [PATCH] fix(runner): replace writable-parent mount guess with explicit sandbox mounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Docker sandbox auto-mounted the parent directory of every path-like scanner argument as writable, so command-backed scanners had somewhere to write result files. That handed each scanner writable host access to a directory it never named — a footgun, worst of all for user-defined scanners. Remove the writable-parent branch. Target args now mount read-only and nothing target-adjacent is writable by default. Operators opt in to extra writable mounts explicitly: - profile `sandbox.mounts` accepts a bare path (read-only) or `{path, write: true}` (writable); - `--sandbox-mount ` does the same on the CLI; - both require an absolute, existing path and are recorded in sandbox metadata. The Cisco scanner relied on the removed auto-mount to read back its result file; it now uses its result dir as the Docker working directory (mounted writable), matching how the AIG scanner already worked. --- cmd/clawscan/main.go | 1 + internal/profiles/registry.go | 2 +- internal/profiles/resolver.go | 103 ++++++++++++++++++++++++- internal/profiles/resolver_test.go | 59 +++++++++++++++ internal/runner/cisco_scanner.go | 6 +- internal/runner/cisco_scanner_test.go | 23 ++++++ internal/runner/runner.go | 29 +++++++ internal/runner/runner_test.go | 104 ++++++++++++++++++++++++++ internal/runner/sandbox.go | 55 ++++++++++---- 9 files changed, 362 insertions(+), 20 deletions(-) diff --git a/cmd/clawscan/main.go b/cmd/clawscan/main.go index cefb420..59bdc02 100644 --- a/cmd/clawscan/main.go +++ b/cmd/clawscan/main.go @@ -593,6 +593,7 @@ Core flags: --sandbox Command sandbox mode. Defaults to docker. --sandbox-image Docker runtime image. Defaults to %s or CLAWSCAN_SANDBOX_IMAGE. --sandbox-env Allow an env var through the Docker sandbox. Repeat for multiple vars. + --sandbox-mount Bind-mount a host dir into the Docker sandbox (read-only; append :rw for writable). Repeat for multiple. Benchmark command flags: --split Benchmark split. Defaults to benchmark for SkillTrustBench and eval_holdout for clawhub-security-signals. diff --git a/internal/profiles/registry.go b/internal/profiles/registry.go index 0bcaa56..650a396 100644 --- a/internal/profiles/registry.go +++ b/internal/profiles/registry.go @@ -124,5 +124,5 @@ func (registry ProfileRegistry) IDs() []string { } func sandboxIsZero(sandbox Sandbox) bool { - return sandbox.Mode == "" && sandbox.Image == "" && len(sandbox.Env) == 0 + return sandbox.Mode == "" && sandbox.Image == "" && len(sandbox.Env) == 0 && len(sandbox.Mounts) == 0 } diff --git a/internal/profiles/resolver.go b/internal/profiles/resolver.go index 23e74e1..6168799 100644 --- a/internal/profiles/resolver.go +++ b/internal/profiles/resolver.go @@ -232,9 +232,51 @@ func profileGateRules(scanners []ProfileScanner) map[string]runner.ScannerGatePo } type Sandbox struct { - Mode string `yaml:"mode,omitempty"` - Image string `yaml:"image,omitempty"` - Env []string `yaml:"env,omitempty"` + Mode string `yaml:"mode,omitempty"` + Image string `yaml:"image,omitempty"` + Env []string `yaml:"env,omitempty"` + Mounts []SandboxMount `yaml:"mounts,omitempty"` +} + +type SandboxMount struct { + Path string + Write bool +} + +func (m *SandboxMount) UnmarshalYAML(node *yaml.Node) error { + switch node.Kind { + case yaml.ScalarNode: + return node.Decode(&m.Path) + case yaml.MappingNode: + for i := 0; i < len(node.Content); i += 2 { + switch node.Content[i].Value { + case "path", "write": + default: + return fmt.Errorf("field %s not found in type profiles.SandboxMount", node.Content[i].Value) + } + } + var v struct { + Path string `yaml:"path"` + Write bool `yaml:"write"` + } + if err := node.Decode(&v); err != nil { + return err + } + m.Path, m.Write = v.Path, v.Write + return nil + default: + return fmt.Errorf("sandbox mount must be a string or object") + } +} + +func (m SandboxMount) MarshalYAML() (interface{}, error) { + if !m.Write { + return m.Path, nil + } + return struct { + Path string `yaml:"path"` + Write bool `yaml:"write"` + }{Path: m.Path, Write: m.Write}, nil } type Judge struct { @@ -268,6 +310,7 @@ type cliIntent struct { sandboxImage string sandboxImageSet bool sandboxEnv []string + sandboxMounts []SandboxMount benchmark string benchmarkSet bool split string @@ -606,6 +649,7 @@ func mergeSandbox(defaults *Sandbox, override *Sandbox) Sandbox { if defaults != nil { out = *defaults out.Env = append([]string(nil), defaults.Env...) + out.Mounts = append([]SandboxMount(nil), defaults.Mounts...) } if override != nil { if override.Mode != "" { @@ -617,6 +661,9 @@ func mergeSandbox(defaults *Sandbox, override *Sandbox) Sandbox { if len(override.Env) > 0 { out.Env = append(out.Env, override.Env...) } + if len(override.Mounts) > 0 { + out.Mounts = append(out.Mounts, override.Mounts...) + } } out.Env = dedupeStrings(out.Env) return out @@ -747,6 +794,17 @@ func parseCLIIntent(args []string) (cliIntent, error) { } intent.sandboxEnv = append(intent.sandboxEnv, value) i = next + case "--sandbox-mount": + value, next, err := readValue(args, i, arg) + if err != nil { + return cliIntent{}, err + } + mount, err := parseSandboxMountFlag(value) + if err != nil { + return cliIntent{}, err + } + intent.sandboxMounts = append(intent.sandboxMounts, mount) + i = next case "--split": value, next, err := readValue(args, i, arg) if err != nil { @@ -891,6 +949,10 @@ func buildRunnerArgs(intent cliIntent, selected resolvedProfile, profileName str for _, envVar := range selected.sandbox.Env { args = append(args, "--sandbox-env", envVar) } + args, err := appendSandboxMountArgs(args, selected.sandbox.Mounts) + if err != nil { + return nil, nil, err + } if intent.sandboxSet { args = append(args, "--sandbox", intent.sandbox) } @@ -900,9 +962,44 @@ func buildRunnerArgs(intent cliIntent, selected resolvedProfile, profileName str for _, envVar := range intent.sandboxEnv { args = append(args, "--sandbox-env", envVar) } + args, err = appendSandboxMountArgs(args, intent.sandboxMounts) + if err != nil { + return nil, nil, err + } return args, selected.files, nil } +func parseSandboxMountFlag(value string) (SandboxMount, error) { + if i := strings.LastIndexByte(value, ':'); i >= 0 { + suffix := value[i+1:] + if suffix == "rw" || suffix == "write" { + path := value[:i] + if strings.TrimSpace(path) == "" { + return SandboxMount{}, fmt.Errorf("--sandbox-mount requires a path before %q", ":"+suffix) + } + return SandboxMount{Path: path, Write: true}, nil + } + } + return SandboxMount{Path: value, Write: false}, nil +} + +func appendSandboxMountArgs(args []string, mounts []SandboxMount) ([]string, error) { + for _, mount := range mounts { + if !filepath.IsAbs(mount.Path) { + return nil, fmt.Errorf("sandbox mount path must be absolute: %q", mount.Path) + } + if _, err := os.Stat(mount.Path); err != nil { + return nil, fmt.Errorf("sandbox mount path does not exist: %q", mount.Path) + } + value := mount.Path + if mount.Write { + value += ":rw" + } + args = append(args, "--sandbox-mount", value) + } + return args, nil +} + func shouldUseProfileJudge(intent cliIntent) bool { if len(intent.scanners) == 0 { return true diff --git a/internal/profiles/resolver_test.go b/internal/profiles/resolver_test.go index e5c0b49..abb5294 100644 --- a/internal/profiles/resolver_test.go +++ b/internal/profiles/resolver_test.go @@ -678,6 +678,65 @@ profiles: } } +func TestResolveArgsSupportsSandboxMountConfig(t *testing.T) { + dir := t.TempDir() + readOnlyDir := t.TempDir() + writableDir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + writeFile(t, config, "version: 1\n"+ + "sandbox:\n"+ + " mounts:\n"+ + " - "+readOnlyDir+"\n"+ + " - path: "+writableDir+"\n"+ + " write: true\n"+ + "profiles:\n"+ + " review:\n"+ + " scanners:\n"+ + " - clawscan-static\n") + + opts, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + if err != nil { + t.Fatal(err) + } + want := []runner.SandboxMount{ + {Path: readOnlyDir}, + {Path: writableDir, Write: true}, + } + if !reflect.DeepEqual(opts.Sandbox.Mounts, want) { + t.Fatalf("sandbox mounts = %#v, want %#v", opts.Sandbox.Mounts, want) + } +} + +func TestResolveArgsValidatesSandboxMountConfig(t *testing.T) { + tests := []struct { + name string + path string + wantErr string + }{ + {name: "relative", path: "relative/rules", wantErr: "must be absolute"}, + {name: "missing", path: filepath.Join(t.TempDir(), "missing"), wantErr: "does not exist"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + dir := t.TempDir() + config := filepath.Join(dir, ".clawscan.yml") + writeFile(t, config, "version: 1\n"+ + "sandbox:\n"+ + " mounts:\n"+ + " - "+test.path+"\n"+ + "profiles:\n"+ + " review:\n"+ + " scanners:\n"+ + " - clawscan-static\n") + + _, err := ResolveArgs([]string{"./skill", "--config", config, "--profile", "review"}, dir) + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("err = %v, want substring %q", err, test.wantErr) + } + }) + } +} + func TestResolveArgsRejectsUnrequestedScannerResultAfterOverrides(t *testing.T) { _, err := ResolveArgs([]string{ "./skill", diff --git a/internal/runner/cisco_scanner.go b/internal/runner/cisco_scanner.go index b036da5..c337cb4 100644 --- a/internal/runner/cisco_scanner.go +++ b/internal/runner/cisco_scanner.go @@ -28,7 +28,11 @@ func (runner ExternalScannerRunner) runCisco(target string, startedAt string) (S timeout = 20 * time.Minute } - output, runErr := runner.CommandRunner.Run(command, args, "", timeout) + cwd := "" + if runner.SandboxMode == SandboxModeDocker { + cwd = resultDir + } + output, runErr := runner.CommandRunner.Run(command, args, cwd, timeout) raw, readErr := os.ReadFile(resultPath) completedAt := time.Now().UTC().Format(time.RFC3339Nano) if readErr != nil { diff --git a/internal/runner/cisco_scanner_test.go b/internal/runner/cisco_scanner_test.go index 77ef102..51cdb9b 100644 --- a/internal/runner/cisco_scanner_test.go +++ b/internal/runner/cisco_scanner_test.go @@ -58,6 +58,29 @@ func TestCiscoScannerCompletesWithJSONOutputFile(t *testing.T) { } } +func TestCiscoScannerUsesResultDirAsDockerSandboxCWD(t *testing.T) { + commandRunner := &ciscoRecordingCommandRunner{output: `{"scanner":"cisco","findings":[]}`} + result, err := (ExternalScannerRunner{ + CommandRunner: commandRunner, + Env: map[string]string{}, + SandboxMode: SandboxModeDocker, + }).runCisco(t.TempDir(), "2026-07-24T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "completed" { + t.Fatalf("result = %#v", result) + } + if len(commandRunner.calls) != 1 { + t.Fatalf("calls = %#v", commandRunner.calls) + } + call := commandRunner.calls[0] + outputPath := argValue(call.args, "--output") + if call.cwd == "" || call.cwd != filepath.Dir(outputPath) { + t.Fatalf("cwd = %q, output path = %q", call.cwd, outputPath) + } +} + func TestCiscoScannerEnablesUpstreamAnalyzersFromEnv(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "skill") diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 54926d8..0f5d9d9 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -365,6 +365,17 @@ func ParseArgsWithRegistry(args []string, registry ScannerRegistry) (Options, er } opts.Sandbox.Env = append(opts.Sandbox.Env, value) i = next + case "--sandbox-mount": + value, next, err := readValue(args, i, arg) + if err != nil { + return Options{}, err + } + mount, err := parseSandboxMountFlag(value) + if err != nil { + return Options{}, err + } + opts.Sandbox.Mounts = append(opts.Sandbox.Mounts, mount) + i = next default: return Options{}, fmt.Errorf("Unknown argument: %s", arg) } @@ -387,6 +398,22 @@ func ParseArgsWithRegistry(args []string, registry ScannerRegistry) (Options, er return opts, nil } +// parseSandboxMountFlag parses a --sandbox-mount value: "" is read-only, +// ":rw" or ":write" is writable. +func parseSandboxMountFlag(value string) (SandboxMount, error) { + if i := strings.LastIndexByte(value, ':'); i >= 0 { + suffix := value[i+1:] + if suffix == "rw" || suffix == "write" { + path := value[:i] + if strings.TrimSpace(path) == "" { + return SandboxMount{}, fmt.Errorf("--sandbox-mount requires a path before %q", ":"+suffix) + } + return SandboxMount{Path: path, Write: true}, nil + } + } + return SandboxMount{Path: value, Write: false}, nil +} + func ValidateRequirements(opts Options, env map[string]string) error { var missing []EnvRequirement for _, req := range requirements(opts, env) { @@ -2028,6 +2055,8 @@ func (runner ExternalScannerRunner) runSkillSpector(target string, startedAt str scanTarget = "artifact" cwd = resultDir resultName = "skillspector-report-0.json" + } else if runner.SandboxMode == SandboxModeDocker { + cwd = resultDir } resultPath := filepath.Join(resultDir, resultName) args = append(args, "scan", scanTarget, "--format", "json", "--output", resultPath) diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index d47003a..fabe27b 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -133,6 +133,8 @@ func TestParseArgsSupportsSandboxFlags(t *testing.T) { "--sandbox-image", "ghcr.io/acme/runtime:v1", "--sandbox-env", "OPENAI_API_KEY", "--sandbox-env", "ANTHROPIC_API_KEY", + "--sandbox-mount", "/opt/rules", + "--sandbox-mount", "/var/cache:rw", }) if err != nil { t.Fatal(err) @@ -146,6 +148,82 @@ func TestParseArgsSupportsSandboxFlags(t *testing.T) { if got := strings.Join(opts.Sandbox.Env, ","); got != "OPENAI_API_KEY,ANTHROPIC_API_KEY" { t.Fatalf("sandbox env = %q", got) } + wantMounts := []SandboxMount{{Path: "/opt/rules"}, {Path: "/var/cache", Write: true}} + if !reflect.DeepEqual(opts.Sandbox.Mounts, wantMounts) { + t.Fatalf("sandbox mounts = %#v, want %#v", opts.Sandbox.Mounts, wantMounts) + } +} + +func TestParseSandboxMountFlag(t *testing.T) { + tests := []struct { + value string + want SandboxMount + }{ + {value: "/opt/rules", want: SandboxMount{Path: "/opt/rules"}}, + {value: "/var/cache:rw", want: SandboxMount{Path: "/var/cache", Write: true}}, + {value: "/var/cache:write", want: SandboxMount{Path: "/var/cache", Write: true}}, + } + for _, test := range tests { + t.Run(test.value, func(t *testing.T) { + got, err := parseSandboxMountFlag(test.value) + if err != nil { + t.Fatal(err) + } + if got != test.want { + t.Fatalf("mount = %#v, want %#v", got, test.want) + } + }) + } +} + +func TestDockerMountsDropsWritableParentForMissingPath(t *testing.T) { + mounts := dockerMounts("", []string{"-c", "run", "clawscan-target", "/bin/definitely-missing-xyz"}, nil) + for _, mount := range mounts { + if strings.Contains(mount, "source=/bin,") || strings.Contains(mount, "source=/bin/definitely-missing-xyz,") { + t.Fatalf("unexpected mount for missing path or its parent: %q", mount) + } + } +} + +func TestDockerMountsMountsTargetReadOnlyWithoutWritableParent(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "SKILL.md") + if err := os.WriteFile(target, []byte("# demo"), 0o644); err != nil { + t.Fatal(err) + } + mounts := dockerMounts("", []string{"-c", "scan", "clawscan-target", target}, nil) + + sawTargetReadOnly := false + for _, mount := range mounts { + // The security property this replaces: an existing target must not make + // its parent directory a writable bind mount, and the target itself is + // read-only. + if strings.Contains(mount, "source="+dir+",") { + t.Fatalf("target parent dir must not be mounted, got %q", mount) + } + if strings.Contains(mount, "source="+target+",") { + if !strings.Contains(mount, ",readonly") { + t.Fatalf("target must be mounted read-only, got %q", mount) + } + sawTargetReadOnly = true + } + } + if !sawTargetReadOnly { + t.Fatalf("expected the target file to be mounted read-only, got %#v", mounts) + } +} + +func TestDockerMountsAddsExplicitMounts(t *testing.T) { + dir := t.TempDir() + readOnly := "type=bind,source=" + dir + ",target=" + dir + ",readonly" + writable := "type=bind,source=" + dir + ",target=" + dir + + if got := dockerMounts("", nil, []SandboxMount{{Path: dir}}); !reflect.DeepEqual(got, []string{readOnly}) { + t.Fatalf("read-only mounts = %#v, want %#v", got, []string{readOnly}) + } + if got := dockerMounts("", nil, []SandboxMount{{Path: dir, Write: true}}); !reflect.DeepEqual(got, []string{writable}) { + t.Fatalf("writable mounts = %#v, want %#v", got, []string{writable}) + } } func TestParseArgsRejectsUnsupportedSandboxMode(t *testing.T) { @@ -1512,6 +1590,32 @@ func TestRunExecutesSkillSpectorScanner(t *testing.T) { } } +func TestSkillSpectorUsesResultDirAsDockerSandboxCWD(t *testing.T) { + commandRunner := &recordingCommandRunner{ + writeOutput: `{"status":"clean","findings":[]}`, + } + result, err := (ExternalScannerRunner{ + CommandRunner: commandRunner, + Env: map[string]string{}, + SandboxMode: SandboxModeDocker, + SkillSpectorCommand: []string{"skillspector"}, + }).runSkillSpector(t.TempDir(), "2026-07-24T00:00:00Z") + if err != nil { + t.Fatal(err) + } + if result.Status != "completed" { + t.Fatalf("result = %#v", result) + } + if len(commandRunner.calls) != 1 { + t.Fatalf("calls = %#v", commandRunner.calls) + } + call := commandRunner.calls[0] + outputPath := argValue(call.args, "--output") + if call.cwd == "" || call.cwd != filepath.Dir(outputPath) { + t.Fatalf("cwd = %q, output path = %q", call.cwd, outputPath) + } +} + func TestRunClawHubProfileMatchesProductionSkillSpectorWorkspace(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "skill") diff --git a/internal/runner/sandbox.go b/internal/runner/sandbox.go index 13e088f..b40105f 100644 --- a/internal/runner/sandbox.go +++ b/internal/runner/sandbox.go @@ -18,16 +18,23 @@ const ( ) type SandboxOptions struct { - Mode string - Image string - Env []string + Mode string + Image string + Env []string + Mounts []SandboxMount +} + +type SandboxMount struct { + Path string `json:"path"` + Write bool `json:"write,omitempty"` } type SandboxMetadata struct { - Mode string `json:"mode"` - Image string `json:"image,omitempty"` - Network string `json:"network,omitempty"` - Env []string `json:"env,omitempty"` + Mode string `json:"mode"` + Image string `json:"image,omitempty"` + Network string `json:"network,omitempty"` + Env []string `json:"env,omitempty"` + Mounts []SandboxMount `json:"mounts,omitempty"` } type resolvedSandbox struct { @@ -40,6 +47,7 @@ type dockerCommandRunner struct { Env map[string]string Image string EnvNames []string + Mounts []SandboxMount } func resolveSandbox(opts Options, env map[string]string) (resolvedSandbox, error) { @@ -84,6 +92,7 @@ func sandboxMetadata(opts Options, env map[string]string) (SandboxMetadata, erro metadata.Image = sandbox.Image metadata.Network = "on" metadata.Env = sandboxEnvNames(opts, env) + metadata.Mounts = append([]SandboxMount(nil), opts.Sandbox.Mounts...) } return metadata, nil } @@ -106,13 +115,26 @@ func sandboxMetadataForOptionList(optsList []Options, env map[string]string) San if next.Mode != first.Mode || next.Image != first.Image || next.Network != first.Network || - strings.Join(next.Env, "\x00") != strings.Join(first.Env, "\x00") { + strings.Join(next.Env, "\x00") != strings.Join(first.Env, "\x00") || + !sandboxMountsEqual(next.Mounts, first.Mounts) { return SandboxMetadata{Mode: "mixed"} } } return first } +func sandboxMountsEqual(left []SandboxMount, right []SandboxMount) bool { + if len(left) != len(right) { + return false + } + for i := range left { + if left[i] != right[i] { + return false + } + } + return true +} + func commandRunnerForOptions(opts Options, ctx RunContext, env map[string]string) (CommandRunner, SandboxMetadata, error) { metadata, err := sandboxMetadata(opts, env) if err != nil { @@ -142,6 +164,7 @@ func commandRunnerForOptions(opts Options, ctx RunContext, env map[string]string Env: env, Image: metadata.Image, EnvNames: metadata.Env, + Mounts: opts.Sandbox.Mounts, }, metadata, nil } @@ -159,7 +182,7 @@ func (runner dockerCommandRunner) Run(command string, args []string, cwd string, dockerArgs = append(dockerArgs, "-e", name) } } - for _, mount := range dockerMounts(cwd, args) { + for _, mount := range dockerMounts(cwd, args, runner.Mounts) { dockerArgs = append(dockerArgs, "--mount", mount) } if cwd != "" { @@ -170,7 +193,7 @@ func (runner dockerCommandRunner) Run(command string, args []string, cwd string, return runner.Host.Run("docker", dockerArgs, "", timeout) } -func dockerMounts(cwd string, args []string) []string { +func dockerMounts(cwd string, args []string, extra []SandboxMount) []string { mounts := map[string]bool{} add := func(path string, readOnly bool) { if path == "" { @@ -182,17 +205,19 @@ func dockerMounts(cwd string, args []string) []string { } if existing, err := os.Stat(clean); err == nil { mounts[clean] = readOnly || !existing.IsDir() - return - } - parent := filepath.Dir(clean) - if parent != "." && parent != clean { - mounts[parent] = false } } add(cwd, false) for _, arg := range args { add(arg, true) } + for _, mount := range extra { + clean := filepath.Clean(mount.Path) + if !filepath.IsAbs(clean) { + continue + } + mounts[clean] = !mount.Write + } sources := make([]string, 0, len(mounts)) for source := range mounts { sources = append(sources, source)