sec (6/9): run user-defined scanners in an isolated working directory#30
Conversation
82956cd to
5ecd550
Compare
494f412 to
1774bc0
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed July 26, 2026, 12:35 PM ET / 16:35 UTC. ClawSweeper reviewWhat this changesThis PR runs profile-defined scanners from a disposable host directory and stops deriving Docker’s working directory from the scan target, while preserving a read-only Docker mount for the target argument. Merge readinessKeep this PR open for maintainer review. The latest head appears to resolve the earlier Docker-read regression: it leaves Docker’s working directory unset while the shared runner mounts the absolute target read-only, and the new focused test checks that contract. The author is a repository member, so this cleanup workflow should not auto-close it. Priority: P1 Review scores
Verification
How this fits togetherProfile-defined scanners are configured as shell commands and executed by ClawScan’s user-defined scanner adapter. That adapter sends a target to the shared host-or-Docker command runner, whose output becomes raw scanner evidence in the run artifact. flowchart LR
A[Profile scanner command] --> B[User-defined scanner adapter]
C[Skill or URL target] --> B
B --> D[Shared command runner]
D --> E[Host temporary workspace]
D --> F[Docker read-only target mount]
E --> G[Raw scanner JSON]
F --> G
G --> H[Run artifact]
Decision needed
Why: The source and new test indicate that this is the intended safe boundary, but accepting the sandbox contract for all supported target forms is a maintainer-owned compatibility decision. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Retain the isolated host workspace and empty Docker working directory, with the target supplied only through the shared read-only Docker mount; merge after a maintainer confirms the focused coverage reflects the supported target forms. Do we have a high-confidence way to reproduce the issue? Yes, at source level: a profile-defined scanner can attempt to read the passed target and write beside it under both host and Docker execution. The focused tests and the contributor’s described Alpine CLI run cover the expected read-success/write-denial outcome, although this read-only review environment could not execute the commands independently. Is this the best way to solve the issue? Yes, with maintainer confirmation of the shared Docker mount contract. Avoiding a target-derived working directory removes ambient write access while retaining explicit read access through the runner’s read-only mount. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 316cd87b69d1. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
The scanner used to run with a working directory derived from the target: the target's parent directory, or the target itself when it was a directory. That handed the scanner writable access to files sitting next to the target, and under Docker that parent became a writable bind mount. Run host scanners in a fresh os.MkdirTemp directory (removed after the run) and keep the Docker cwd empty so nothing target-derived is mounted; the container workdir is already isolated. Removes the now-unused userDefinedScannerCWD helper.
1774bc0 to
5f06f1e
Compare
clawsweeper
Bot
added
the
merge-risk: 🚨 security-boundary
|
The current P1 appears to overlook the shared Docker runner behavior. Passing an empty cwd removes only the writable working-directory mount; dockerCommandRunner.Run still sends the absolute target argument through dockerMounts, producing a read-only bind mount at the identical container path. I also verified the complete behavior with a real Alpine container through the ClawScan CLI: the user-defined scanner successfully read SKILL.md, was denied a write inside the target, returned valid JSON, and completed with exit code 0. The focused unit test additionally verifies that the generated Docker arguments contain the read-only target mount and no -w argument. Please trace user_defined_scanner.go -> dockerCommandRunner.Run -> dockerMounts when reassessing the finding. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
What changes
A user-defined scanner no longer runs with its working directory set to the
target's folder. On a host run it now runs in a fresh isolated temp directory
(
clawscan-scanner-*, removed after the run); under Docker the workdir staysempty so nothing target-adjacent is mounted.
Why it matters
Running with cwd = the target's directory hands the scanner writable access
to every file sitting next to the target. A malicious or buggy scanner could
modify or drop files beside the skill being scanned (and under Docker that
directory would be a writable bind mount). Giving the scanner a private throwaway
cwd removes that ambient write access; the target is still passed explicitly as
an argument.
Before / after (runnable)
Scanner that records its cwd and tries to write a neighbor file:
NEIGHBOR_LEAKlands next to the skillVerify
TestUserDefinedScannerUsesIsolatedCwdasserts the host run's cwd is a freshtemp dir (not the target dir) and that Docker runs use an empty workdir.