Skip to content

sec (9/9): document that scanners must not print secrets into raw evidence#33

Merged
jesse-merhi merged 1 commit into
mainfrom
sec/09-doc-raw-evidence-secrets
Jul 27, 2026
Merged

sec (9/9): document that scanners must not print secrets into raw evidence#33
jesse-merhi merged 1 commit into
mainfrom
sec/09-doc-raw-evidence-secrets

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Jul 24, 2026

Copy link
Copy Markdown
Member

What this documents

A user-defined scanner must not print credentials into the JSON report it emits. ClawScan preserves valid scanner stdout as raw evidence, so a secret placed inside that JSON is intentionally not rewritten.

secretEnv protects scanner failure text:

stderr: authentication failed for credential-value
stored error: authentication failed for [redacted]

It does not transform scanner evidence:

{"scanner_output":"credential-value"}

That JSON remains raw. Avoiding secrets in it is the scanner author’s responsibility. The docs also correct the scanner ID contract to lowercase IDs of at most 64 characters.

Verification

  • go test -count=1 ./internal/runner -run '^TestUserDefinedScannerPreservesSecretEnvInRawEvidence$' -v — passed\n- make docs-site — passed\n\nDocumentation only; no runtime code changes.

Copilot AI review requested due to automatic review settings July 24, 2026 04:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jesse-merhi
jesse-merhi force-pushed the sec/08-explicit-sandbox-mounts branch from 04c6ca8 to e20d07d Compare July 26, 2026 02:38
@jesse-merhi
jesse-merhi force-pushed the sec/09-doc-raw-evidence-secrets branch from a5e0f41 to e0dc381 Compare July 26, 2026 02:38
@jesse-merhi
jesse-merhi changed the base branch from sec/08-explicit-sandbox-mounts to main July 26, 2026 02:38
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 26, 2026
@clawsweeper

clawsweeper Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 9:00 AM ET / 13:00 UTC.

ClawSweeper review

What this changes

This PR documents that user-defined scanners must keep credentials out of raw JSON evidence and corrects the documented custom scanner-ID constraints.

Merge readiness

Ready for maintainer review

Keep open. This is a narrow, correct documentation update, and the previous blocker is resolved: the declared-secretEnv error-text redaction it describes was merged through #31 before the current PR head was reviewed. The PR is clean, changes only one docs file, and is authored by a repository member, so it needs normal maintainer merge judgment rather than automated cleanup.

**Likely related people**

  • jesse-merhi — high confidence; introduced the user-defined-scanner workflow, carried the subsequent redaction hardening, and authored this documentation follow-up.

Priority: P2
Reviewed head: 75c8fbbdfaced0ff17db92447be76e7b7f7b800d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, correctly aligned documentation patch with passing supplied checks and no remaining correctness finding.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required because this PR only changes files under docs/.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required because this PR only changes files under docs/.
Evidence reviewed 4 items Narrow documentation diff: The current branch changes only the user-defined scanner documentation: it updates the custom-ID contract and adds a warning that raw JSON stdout is preserved without secret rewriting.
Prior review blocker resolved: The earlier review object identified error-text redaction as pending in the related PR; #31 is now merged at commit 1c247ac, matching the documentation's stderr-only redaction claim.
Feature provenance: User-defined scanners and their raw-JSON evidence contract entered the repository through the merged implementation PR #23; the behavior appears to date to its merge commit.
Findings None None.
Security None None.

How this fits together

Profile-defined user scanners run external commands through ClawScan's scanner runner. Their valid JSON stdout is retained as raw evidence, while declared secret environment values are redacted only from failure text before artifacts are written.

flowchart LR
  Profile[Profile scanner configuration] --> Runner[User-defined scanner runner]
  Secrets[Declared secret environment names] --> Runner
  Runner --> Sandbox[Sandboxed scanner command]
  Sandbox --> Split[Stdout and stderr handling]
  Split --> Evidence[Raw JSON evidence]
  Split --> Errors[Redacted failure text]
  Evidence --> Artifacts[Run artifacts]
  Errors --> Artifacts
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Documentation surface 1 file changed; 8 added, 1 removed The patch is tightly scoped to the user-defined scanner contract and does not alter runtime or release automation.

Technical review

Best possible solution:

Retain raw scanner JSON as evidence, keep declared secretEnv redaction limited to failure text, and merge this matching operator guidance after routine maintainer review.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR changes documentation only. Its central claims align with the merged redaction work and the existing raw-evidence contract described in the related implementation history.

Is this the best way to solve the issue?

Yes. Warning scanner authors not to emit credentials in raw JSON is the narrow maintainable solution because preserving scanner evidence verbatim is an intentional artifact contract; rewriting arbitrary third-party JSON could alter evidence.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a9b45ab69396.

Labels

Label justifications:

  • P2: This is a bounded security-contract documentation improvement for custom scanner authors, with limited blast radius and no runtime regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real behavior proof is not required because this PR only changes files under docs/.

Evidence

What I checked:

Likely related people:

  • jesse-merhi: Introduced the merged user-defined-scanner feature, authored the merged declared-secret redaction work, and authored this focused documentation follow-up. (role: feature author and recent area contributor; confidence: high; commits: 656e558b4ba5, 1c247ac9faae, 75c8fbbdface; files: internal/runner/user_defined_scanner.go, docs/scanners.md)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-07-26T02:42:06.282Z sha e0dc381 :: needs changes before merge. :: [P1] Do not claim redaction before it lands

…er evidence

ClawScan preserves a user-defined scanner's stdout verbatim as raw evidence, so
a secret the scanner prints into its own JSON output is persisted as-is. Unlike
declared env values in error text, ClawScan cannot redact inside raw evidence
without corrupting it. Document that a scanner must not echo credentials into
its report (finding 9, resolved as a documented author contract rather than
code). Also correct the scanner id rule to note the lowercase and 64-character
limits added in the hardening series.
@jesse-merhi
jesse-merhi force-pushed the sec/09-doc-raw-evidence-secrets branch from e0dc381 to 75c8fbb Compare July 27, 2026 12:57
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 27, 2026
@jesse-merhi
jesse-merhi merged commit b3bab6b into main Jul 27, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants