feat(security): add OBSERVE + BASH_HIGH coverage for outbound curl writes (#1280) - #1302
Conversation
…ites (#1280) - safe-destruct-patterns.ts: add curl-write-method and curl-data-flag OBSERVE entries covering `curl -X POST/PUT/PATCH/DELETE`, `curl -d`/`--data`/ `-F`/`--form`, and `wget --post-data`/`--post-file` - risk-classifier.ts: add BASH_HIGH entries for the same curl/wget shapes so AFK-mode autonomous sessions gate outbound HTTP writes behind approval - Tests: 11 new assertions in risk-classifier.test.ts (curl -X POST/PUT/PATCH/ DELETE, curl --data, curl -d, wget --post-data) and 11 new assertions in safe-destruct-detect.test.ts (pattern matching + hook approve decisions) - All 187 tests pass; lint (tsc --noEmit) clean Closes #1280
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbcb56e2e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'curl -X POST', | ||
| 'curl -X PUT', | ||
| 'curl -X PATCH', | ||
| 'curl -X DELETE', | ||
| 'curl --data', | ||
| 'curl -d ', | ||
| 'wget --post-data', |
There was a problem hiding this comment.
In autonomous AFK sessions, these literal substrings only classify writes when the mutating flag immediately follows curl; common valid commands such as curl -H 'Content-Type: application/json' -X POST URL -d '{}' or curl URL -X DELETE remain medium, so afk-mode-gate.ts allows them without approval. The same gap affects curl -F and wget --post-file, which this commit explicitly recognizes as writes in the OBSERVE detector. Match curl/wget invocations and their write flags independently of option ordering rather than matching only these exact command prefixes.
Useful? React with 👍 / 👎.
| id: 'curl-write-method', | ||
| re: /\bcurl\b[^|;&]*\s-X\s+(POST|PUT|PATCH|DELETE)\b/i, | ||
| tier: 'observe', |
There was a problem hiding this comment.
Normal sessions do not record curl --request POST https://api.example.com because this regex accepts only the short -X spelling. This is not an exotic alternate syntax: curl --help all documents the option as -X, --request <method> Specify request method to use. Consequently an equivalent POST/PUT/PATCH/DELETE command produces no safe-destruct observation or audit event; include the long form, and preferably the other accepted argument spellings, in this pattern.
Useful? React with 👍 / 👎.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Review: PR #1302 — feat(security): add OBSERVE + BASH_HIGH coverage for outbound curl writes (#1280)
Target: afk/iso-issue-1280-5-m6l4xx → main · ref dbcb56e2 · +68/−0 · 4 files
Regime: light (hotfix, ~68 lines)
Stated intent: PR #1302 title+body — spec-compliance assessed.
Findings
1 · low · blocking:false · confidence:high · correctness · src/agent/safe-destruct-patterns.ts:210 + src/agent/risk-classifier.ts:75–81 · ref:dbcb56e2 · file-state (verified via live regex execution)
curl -XPOST (no-space short form) misses both the safe-destruct regex and the BASH_HIGH substring. curl -XPOST https://api.example.com is valid curl syntax. The curl-write-method regex requires \s-X\s+ (whitespace both before and after -X), so -XPOST (flag and method attached with no space) does not match. The BASH_HIGH substring 'curl -X POST' also requires the space. Live regex test: re.test('curl -XPOST https://api.example.com') → false. The companion readonly-bash.ts (curlSegmentMutating) explicitly handles this form via /^-X(POST|PUT|PATCH|DELETE)$/i.
// safe-destruct-patterns.ts:210 — requires whitespace between -X and method
re: /\bcurl\b[^|;&]*\s-X\s+(POST|PUT|PATCH|DELETE)\b/i
// 'curl -XPOST https://...' → false (no space between -X and POST)suggestion: Change \s-X\s+ to \s-X\s* in the regex. Add 'curl -XPOST', 'curl -XPUT', 'curl -XPATCH', 'curl -XDELETE' to BASH_HIGH (or change 'curl -X POST' to 'curl -X' + add the 4 no-space variants). The no-space form is uncommon in model-generated shell but valid in CI scripts and human-authored commands.
2 · low · blocking:false · confidence:high · correctness · src/agent/risk-classifier.ts:80 · ref:dbcb56e2 · file-state (verified via live substring test)
BASH_HIGH 'curl -d ' (trailing space) misses the attached-value form curl -d'data'. classifyBash uses cmd.includes(p) for matching. The trailing space in 'curl -d ' ensures the substring does not false-positive on curl -data-binary, but it also means curl -d'{"x":1}' https://api.example.com (no space between -d and the value) is classified medium instead of high. Live test: 'curl -d"data" https://api.example.com'.includes('curl -d ') → false. The safe-destruct regex layer still catches this form via -d\b, so the OBSERVE recording works — only the AFK-mode BASH_HIGH approval gate is bypassed.
// risk-classifier.ts:80 — trailing space required
'curl -d ',
// 'curl -d"data" https://...' → includes('curl -d ') → falsesuggestion: Change 'curl -d ' to 'curl -d' (remove trailing space). The false-positive risk (curl -data-binary) is negligible — -data-binary is not a valid curl flag (the correct flag is --data-binary), and even if it were, classifying it high is the conservative direction.
3 · low · blocking:false · confidence:high · correctness · src/agent/risk-classifier.ts:75–81 · ref:dbcb56e2 · file-state (verified via grep)
wget --post-file absent from BASH_HIGH — asymmetric with safe-destruct coverage. The curl-data-flag regex in safe-destruct-patterns.ts:215 correctly includes --post-file\b in the wget alternation, so the OBSERVE layer records wget --post-file=... commands. However, BASH_HIGH contains only 'wget --post-data' — wget --post-file bypasses the AFK-mode approval gate, classifying as medium instead of high.
// risk-classifier.ts:81 — only --post-data
'wget --post-data',
// safe-destruct-patterns.ts:215 — covers both
\bwget\b[^|;&]*\s(--post-data\b|--post-file\b)suggestion: Add 'wget --post-file' after 'wget --post-data' in BASH_HIGH.
4 · low · blocking:false · confidence:high · test-coverage · src/agent/safe-destruct-detect.test.ts:97–116 · ref:dbcb56e2 · file-state (verified via grep: zero matches for curl -X GET in test file)
No negative test for curl -X GET in the benign commands table. The curl-write-method regex correctly excludes GET via the exhaustive alternation (POST|PUT|PATCH|DELETE) — live test confirms re.test('curl -X GET https://...') → false. The benign-commands test section (lines 97–116) includes curl https://…/health and curl -s https://…/status but not curl -X GET, leaving this boundary undocumented.
suggestion: Add ['curl -X GET https://api.example.com/items'] to the does not flag benign table.
Spec-compliance
Stated intent: PR #1302 title+body — spec-compliance assessed.
| Requirement | Status |
|---|---|
curl-write-method OBSERVE pattern (-X POST/PUT/PATCH/DELETE) |
✅ Implemented (safe-destruct-patterns.ts:208–211) |
curl-data-flag OBSERVE pattern (-d/--data/-F/--form/wget --post-data/--post-file) |
✅ Implemented (safe-destruct-patterns.ts:213–216) |
| 7 BASH_HIGH entries for curl/wget outbound writes | ✅ 7 entries added (risk-classifier.ts:75–81) |
| OBSERVE, not BLOCK — per issue recommendation | ✅ Both patterns use tier: 'observe' |
| Tests: 7 BASH_HIGH assertions + 8 OBSERVE pattern rows + 3 hook-approve rows | ✅ All present |
| Companion to #1281 (prompt-layer) and #1279/#1298 | ✅ Referenced in body |
| Closes #1280 | ✅ Linked |
No unmet intent. No scope creep detected.
Dimensions with no issues
- Security — no issues found. The new patterns correctly extend the safe-destruct OBSERVE layer and the BASH_HIGH classification without introducing any new bypass paths. The two-layer design (OBSERVE records + BASH_HIGH gates in AFK-mode) is consistent with the existing architecture. Read:
safe-destruct-patterns.ts,risk-classifier.ts. - API-compat — no issues found.
DESTRUCTIVE_PATTERNSis areadonlyarray — new append-only entries are backward compatible. Pattern ids (curl-write-method,curl-data-flag) are new stable identifiers. No exported type or function signatures changed. Read:safe-destruct-patterns.ts,risk-classifier.ts. - Perf-observability — no issues found. 7 new substring entries in the BASH_HIGH table (linear
includesscan), 2 new regex patterns in the DESTRUCTIVE_PATTERNS array — negligible additions to existing linear scans.[^|;&]*character classes are bounded by the delimiter set and cannot catastrophically backtrack. Read:risk-classifier.ts,safe-destruct-patterns.ts.
What was not checked
- Citations verified inline against branch HEAD
dbcb56e2e5407d4ec70949905b9f163dd8083e91viagit showand live Node.js regex/substring execution. - Stated intent: PR #1302 title+body — spec-compliance assessed.
- Not checked: runtime test execution — static review only. CI shows all checks passing (Lint & Build ✅, Tests ✅ ubuntu + macos).
- Not checked: interaction between the new BASH_HIGH entries and the existing
readonly-bash.tscurl-segment classification (the two systems run in different contexts:readonly-bash.tsruns only for read-only skill forks;risk-classifier.tsruns in normal sessions). - Not checked: whether
-Fand--formpatterns in the safe-destruct regex overlap with any existing pattern that handles multipart form uploads differently.
Severity arithmetic: 0 critical, 0 high, 0 medium → MERGE; 4 low.
Decision: MERGE — 0 blocking (4 low).
Clean, well-scoped security hardening that correctly adds OBSERVE + BASH_HIGH coverage for outbound HTTP write commands. The four low findings (no-space -XPOST form, trailing-space -d form, missing wget --post-file in BASH_HIGH, missing curl -X GET negative test) are edge cases that leave narrow gaps in the BASH_HIGH gate while the safe-destruct OBSERVE layer still catches most of them — follow-up hardening opportunities, not regressions.
… review Address all 6 review findings (Codex P1/P2 + AFK /review 1-4): safe-destruct-patterns.ts: - curl-write-method regex: \s-X\s+ → \s(-X\s*|--request\s+) to catch both the no-space -XPOST form and the --request long form risk-classifier.ts: - BASH_HIGH: prefixless -X/-XPOST/--request variants so intervening flags (curl -H '...' -X POST) still match - BASH_HIGH: 'curl -d ' trailing space → 'curl -d' (catches -d"data") - BASH_HIGH: add 'curl -F' (multipart form upload) - BASH_HIGH: add 'wget --post-file' (symmetric with safe-destruct) Tests: - 8 new risk-classifier assertions (no-space, long form, attached -d, -F, --post-file, intervening flags) - 5 new safe-destruct OBSERVE rows + 3 benign-commands negative tests (curl GET, plain curl, silent curl) - 3 new hook-approve rows (no-space, long form, --post-file)
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Review: PR #1302 — feat(security): add OBSERVE + BASH_HIGH coverage for outbound curl writes (#1280)
Target: afk/iso-issue-1280-5-m6l4xx → main · ref 19c6d14 · +130/−0 · 4 files
Regime: light (hotfix — security hardening)
Stated intent: PR #1302 title+body — spec-compliance assessed.
Prior rounds
1 prior automated review on this PR (on commit dbcb56e2). Current commit 19c6d14 ("fix(security): close curl/wget BASH_HIGH and OBSERVE gaps from PR #1302 review") addresses all 4 findings from the prior round. This round confirms the fixes and reviews the final state.
Prior findings — resolution status
| Prior finding | Status at 19c6d14 |
|---|---|
curl -XPOST (no-space form) misses safe-destruct regex |
✅ Fixed — regex `(-X\s* |
curl -d trailing space misses -d"data" form |
✅ Fixed — BASH_HIGH entry is 'curl -d' (no trailing space) |
wget --post-file absent from BASH_HIGH |
✅ Fixed — 'wget --post-file' added |
curl -X GET benign negative test missing |
✅ Fixed — ['curl -X GET https://api.example.com/items'] added to benign table |
All four prior findings are resolved.
Findings
1 · nit · blocking:false · confidence:high · correctness · src/agent/safe-destruct-patterns.ts:210,215 · ref:19c6d14 · file-state (verified)
New curl patterns use [^|;&]* (no \n) vs existing convention [^|&;\n]*. All pre-existing patterns in the file (rm-recursive-force-long, git-push-force, find-delete, docker-destructive) use [^|&;\n]* to stop at both shell delimiters and newlines. The two new curl patterns use [^|;&]* — same set of delimiter characters but omitting \n. Functionally equivalent for single-command strings (which is what detectDestructiveCommands receives), but the convention divergence is visible to a reader.
// Existing style (e.g. line 82):
re: /\brm\s+[^|&;\n]*--recursive\b/i
// New patterns (lines 210, 215):
re: /\bcurl\b[^|;&]*\s(-X\s*|--request\s+)…/isuggestion: Normalize to [^|&;\n]* for consistency with the rest of the file.
Spec-compliance
Stated intent: PR #1302 title+body — spec-compliance assessed.
| Requirement | Status |
|---|---|
curl-write-method OBSERVE pattern (-X POST/PUT/PATCH/DELETE + --request + no-space) |
✅ (-X\s*|--request\s+)(POST|PUT|PATCH|DELETE) covers all forms |
curl-data-flag OBSERVE pattern (-d/--data/-F/--form/wget --post-data/--post-file) |
✅ Implemented with word-boundary anchors |
| BASH_HIGH entries for curl/wget outbound writes | ✅ 17 entries: 4 spaced -X, 4 no-space -X, 4 --request, curl --data, curl -d, curl -F, wget --post-data, wget --post-file |
| OBSERVE, not BLOCK — per issue recommendation | ✅ Both patterns use tier: 'observe' |
Prefixless -X POST / --request POST catch intervening flags |
✅ -X POST as a bare substring in BASH_HIGH matches regardless of preceding flags |
| Tests: 15 BASH_HIGH assertions | ✅ All present (7 original + 8 hardening) |
| Tests: 13 OBSERVE pattern rows | ✅ All present (8 original + 5 hardening) |
| Tests: 6 hook-approve rows (OBSERVE returns approve) | ✅ All present |
Tests: 3 benign-command negative tests (curl GET, curl -s, curl -X GET) |
✅ All present |
| Prior review findings (4 lows) all addressed | ✅ All 4 resolved in commit 19c6d14 |
| Companion to #1281 (prompt-layer), #1279/#1298 (service-registration) | ✅ Referenced in body |
| Closes #1280 | ✅ Linked |
No unmet intent. No scope creep detected.
Dimensions with no issues
- Security — no issues found. The new patterns correctly extend the safe-destruct OBSERVE layer and the BASH_HIGH classification. Both layers fire independently: OBSERVE records the event in all agent contexts; BASH_HIGH gates behind the AFK-mode approval prompt. Defense-in-depth is preserved. The regex
[^|;&]*boundary stop prevents cross-pipeline misattribution. Read:safe-destruct-patterns.ts,risk-classifier.ts. - API-compat — no issues found.
DESTRUCTIVE_PATTERNSis areadonlyarray — new append-only entries are backward compatible. Pattern ids (curl-write-method,curl-data-flag) are new stable identifiers.BASH_HIGHis a module-privatereadonly string[]. No exported type or function signatures changed. - Correctness — no issues found. The
curl-write-methodregex correctly handles all curl method-specification forms: spaced (-X POST), no-space (-XPOST), long-form (--request POST). The\bterminal anchor preventsPOSTfrom matchingPOSTFIXor similar. Thecurl-data-flagregex uses\bword boundaries to prevent-dfrom matching-data-binaryand similar. The[^|;&]*character class correctly scopes the match to a single shell command segment. - Test-coverage — no issues found. All new patterns and BASH_HIGH entries have corresponding positive test assertions. Benign commands (
curl GET,curl -s) confirmed to not trigger. The two-tier hook integration tests confirm OBSERVE patterns returnapprove, notblock. Coverage gap from prior review (curl -X GETnegative test) is closed. - Perf-observability — no issues found. 17 new substring entries in the BASH_HIGH table (linear
includesscan), 2 new regex patterns in the DESTRUCTIVE_PATTERNS array — negligible additions to existing linear scans.[^|;&]*character classes are bounded and cannot catastrophically backtrack.
What was not checked
- Citations verified inline against branch HEAD
19c6d14228dc7e3bb681492f57a69364ddcbb962viagit show. - Stated intent: PR #1302 title+body — spec-compliance assessed.
- Not checked: runtime test execution — static review only. CI has not yet run on this commit (only Vercel preview checks are present).
- Not checked: interaction between the new BASH_HIGH entries and the existing
readonly-bash.tscurl-segment classification (the two systems run in different contexts). - Not checked: whether the
\bword boundary afterDELETEprevents matchingDELETE_ALLor similar — confirmed by inspection that no such curl method exists, but not runtime-verified.
Severity arithmetic: 0 critical, 0 high, 0 medium → MERGE; 1 nit.
Decision: MERGE — 0 blocking (1 nit).
Clean, well-scoped security hardening that correctly addresses all 4 findings from the prior review. The curl-write-method regex now handles all three method-specification forms (-X POST, -XPOST, --request POST), and BASH_HIGH has comprehensive substring coverage including prefixless variants that catch intervening flags. The sole nit (\n in character class) is a cosmetic convention divergence with no functional impact.
1 participant
Summary
Adds safety coverage for outbound HTTP write commands (
curl -X POST,curl -d,wget --post-data, etc.) that previously passed through every guard layer in normal sessions.Problem
The readonly-bash classifier already blocks these patterns, but only runs for read-only skill forks. In normal sessions, an agent can make arbitrary POST/PUT/PATCH/DELETE requests — including through localhost proxies that forward to production APIs — with no approval, no recording, and no hooks firing.
Contributing factor in the 2026-08-24 incident where
curl -X POSTto a localhost review server proxied to production Substack.Changes
OBSERVE-tier patterns (
safe-destruct-patterns.ts):curl-write-method— matches-X POST/PUT/PATCH/DELETEcurl-data-flag— matches-d/--data/-F/--form+ wget--post-data/--post-fileRecords the event without hard-blocking — legitimate dev-server testing still works.
BASH_HIGH classification (
risk-classifier.ts):Tests
Design decision
OBSERVE + BASH_HIGH, not BLOCK — per the issue's recommendation. Hard-blocking
curl -X POSTeverywhere would break legitimate agent workflows that test local dev servers during implementation.Companion to #1281 (prompt-layer) and #1279/#1298 (launchctl/systemctl BLOCK patterns).
Closes #1280.