fix(opencode): harden secret-read regex patterns#3295
Conversation
The secret-read blocked-command rule matched an allow-list of reader binaries (cat/less/more/head/tail/sed/awk), so equivalent credential reads via copy/encode/dump utilities (cp, xxd, base64, dd, strings, tee, grep, ...) and shell input redirection (`< .env`, `source .env`) fell through to the review path and executed. The env-dump pattern likewise missed `env >`, `declare -x`, and `export -p`, and the /proc environ path rule matched only numeric PIDs — so `/proc/self/environ` and `/proc/thread-self/environ` (a full dump of the agent's environment) escaped both the bash rule and the direct-resource path rule. Broaden the reader/redirect/env-dump patterns to key on the sensitive target rather than a reader allow-list, and extend the environ path pattern to self/thread-self. Precision is preserved for common legitimate commands: the `.env` template allow-list still applies, a copy's write destination is not treated as a read (`cp .env.example .env`), `declare -x VAR=val` (exporting a variable) is distinguished from a bare `declare -x` dump, and `env`/`set` are anchored to a command position so an incidental `.env>` in a string is not matched. Adds regression tests for both the closed bypasses and the preserved cases.
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your contribution! Great job on providing a detailed explanation of the changes and including comprehensive test coverage. Before merging, please address the following:
For guidance, refer to CONTRIBUTING.md. Let us know if you need any help! |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
LGTM. Anchoring the deny on the sensitive target rather than an allowlist of reader binaries is the right model, and the added coverage for encode/dump utilities, shell redirection, and /proc/self|thread-self/environ closes real gaps. The dropCopyDestinationToken handling, so cp .env.example .env scaffolding isn't misread as a sensitive-destination read, is a nice touch and the GNU -t case is tested. One framing note for future readers: a regex denylist is defense-in-depth, not a hard boundary, so let's keep treating it as best-effort.
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: test-generator — `agent-governance-opencode/lib/policy.mjs`
|
🤖 AI Agent: code-reviewer — Action Items:
TL;DR: 0 blockers, 1 warning. The PR improves security but requires additional documentation updates.
Action Items:
Warnings:
|
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
Agreed. Feels like a real issue having the regex become too bloated as well. The long-term direction could be a semantic command/resource classifier, perhaps an AST |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Anchoring the deny on the sensitive target instead of a reader allow-list is the right model, the bypasses are closed, and the copy-destination and env/declare edge cases are handled and tested. No ReDoS or fail-open introduced.
|
@MohammadHaroonAbuomar @liamcrumm gentle nudge: this PR has been awaiting a code-owner review for over 3 business days. Could one of you take a look when you have a moment? Thanks. |
2 participants
Description
The secret-read blocked-command rule matched an allow-list of reader binaries (cat/less/more/head/tail/sed/awk), so equivalent credential reads via copy/encode/dump utilities (cp, xxd, base64, dd, strings, tee, grep, ...) and shell input redirection (
< .env,source .env) fell through to the review path and executed. The env-dump pattern missedenv >,declare -x, andexport -p, and the /proc environ path rule matched only numeric PIDs, so e.g./proc/self/environand/proc/thread-self/environ(a full dump of the agent's environment) escaped both the bash rule and the direct-resource path rule.Broaden the reader/redirect/env-dump patterns to key on the sensitive target rather than a reader allow-list, and extend the environ path pattern to self/thread-self. Precision is preserved for common legitimate commands: the
.envtemplate allow-list still applies, a copy's write destination is not treated as a read (cp .env.example .env),declare -x VAR=val(exporting a variable) is distinguished from a baredeclare -xdump, andenv/setare anchored to a command position so an incidental.env>in a string is not matched. Adds regression tests for both the closed bypasses and the preserved cases.Type of Change
Package(s) Affected
Checklist
Attribution & Prior Art
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
Claude used for drafting the fix, and write the commit message. All changes reviewed.
IP, Patents, and Licensing
Related Issues