sec (7/9): redact declared scanner env values from error text#31
Open
jesse-merhi wants to merge 1 commit into
Open
sec (7/9): redact declared scanner env values from error text#31jesse-merhi wants to merge 1 commit into
jesse-merhi wants to merge 1 commit into
Conversation
A user-defined scanner declares env vars precisely because they carry secrets. When the command fails, commandError composes an error string from stderr that can echo those secret values, and that string persists in ScannerResult.Error. Scrub the value of every declared env var from the error text, regardless of the variable name, so credentials never survive in the artifact even when the name evades the generic secret-name heuristic.
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: sec (7/9): redact declared scanner env values from error text This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Part 7 of 9 in the BYOS security-hardening stack. Stacked on sec (6/9) (#30). This is the last of the additive guardrails; parts 8 and 9 change existing behavior and get their own writeups.
A user-defined scanner declares the env vars it needs — and it declares them because they carry secrets (an API token, say). When the scanner command fails, ClawScan builds an error string from the command's stderr and stores it in the run artifact as
ScannerResult.Error. If the scanner echoed its token into stderr (many tools print the failing request, headers included), that secret would be written into the artifact.ClawScan already has a generic "does this look like a secret name" heuristic, but it can miss a value whose variable name doesn't match the pattern. The declared env list is ground truth: those names were explicitly registered as this scanner's secrets.
The fix
Two small helpers:
sanitizedDeclaredEnvNames— turns the declaredenv:entries into bare names (defensively dropping any=valuesuffix).redactDeclaredEnvValues— replaces every declared env value found in the error text with[redacted].Wired into the error path:
So whatever the variable is named, its value cannot survive in
ScannerResult.Error. This upholds the repo rule that artifacts record env presence, never secret values.Verify
New tests:
TestSanitizedDeclaredEnvNamesStripsValues—SECRET_KEY=value,TOKEN,EMPTY=→ namesSECRET_KEY,TOKEN,EMPTY.TestRedactDeclaredEnvValuesRemovesSecrets— the value is replaced by[redacted].TestUserDefinedScannerRedactsDeclaredEnvInError— end to end: a failing scanner whose stderr contains the secret produces anErrorwith[redacted]and without the secret value.The full
internal/runnersuite passes apart from two pre-existing macOS/var↔/private/varsymlink tests (TestResolveTargetClassifiesPlugin{Directory,ManifestFile}), which are unrelated to this change.