feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier - #1298
feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier#1298griffinwork40 wants to merge 3 commits into
Conversation
…nylist, and risk-classifier (#1279) Add launchd (macOS) and systemd (Linux) service registration directories to write-denylist, bash-restriction-hook sensitive roots, risk-classifier high-risk patterns, and safe-destruct destructive patterns. Prevents agents from silently installing persistent services that survive reboots. The sanctioned path is `afk service install` via /service-setup. - write-denylist: LaunchAgents, LaunchDaemons, systemd/user, /etc/systemd - bash-restriction-hook: same paths added to builtinBashSensitiveRoots() - risk-classifier: launchctl load/bootstrap/submit/start, systemctl enable/start/daemon-reload - safe-destruct-patterns: launchctl-load and systemctl-enable block patterns - Read-only commands (launchctl list, systemctl status) remain allowed Closes #1279
|
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: 4cff02cd75
ℹ️ 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".
| }, | ||
| { | ||
| id: 'systemctl-enable', | ||
| re: /\bsystemctl\s+(enable|start|daemon-reload)\b/i, |
There was a problem hiding this comment.
On Linux, the normal user-service form bypasses this block because --user appears between systemctl and the command: systemctl --user enable --now malicious.service and systemctl --user daemon-reload do not match this regex. This ordering is explicitly supported by the local help syntax, systemctl [OPTIONS...] COMMAND ..., and is the form this repository itself executes in src/service/systemd/install.ts:43-46 and documents at lines 210-212. The corresponding risk-classifier literals make the same adjacency assumption, so these commands remain unblocked and are only classified medium, defeating the Linux user-service protection added by this change.
Useful? React with 👍 / 👎.
|
No critical or high findings, and no overridden blocking values — shadow-verify is not triggered. The review is complete. PR #1298 Review:
|
| # | Sev | Blocking | Dim | Location | Finding |
|---|---|---|---|---|---|
| 1 | low | false | correctness | bash-restriction-hook.ts:480–491 |
builtinBashSensitiveRoots() adds LaunchAgents/LaunchDaemons/.config/systemd/user unconditionally cross-platform, unlike the write-denylist which platform-gates them. This is consistent with the established pattern (~/Library/Application Support has the same shape); these entries are grant-filtered noise on non-matching platforms, not a security gap. |
| 2 | low | false | correctness | bash-restriction-hook.test.ts:432–444 |
New builtinBashSensitiveRoots assertions are platform-unconditional, consistent with the function's design. Adding it.runIf(process.platform === ...) guards would make the tests more precise but is not required given the function's documented contract. |
| 3 | low | false | test-coverage | risk-classifier.test.ts:82 |
Only launchctl list has a read-only negative test in the risk-classifier layer. launchctl print, systemctl status, systemctl is-enabled are tested in safe-destruct-detect.test.ts but not here. Adding them would strengthen the safety net. Suggestion: add 3 negative test cases to the classifyRisk — bash high describe block. |
What was checked
- All four security layers verified: write-denylist (platform-gated ✓), bash-restriction-hook (SENSITIVE_PATH_SIGNAL + builtinBashSensitiveRoots ✓), risk-classifier (BASH_HIGH substring match ✓), safe-destruct-patterns (regex block-tier ✓).
- Read-only commands confirmed unblocked at all layers:
launchctl list/print,systemctl status/is-enabled. - Spec-compliance: fully met — all stated-intent requirements implemented and tested.
- API compat: additive-only changes to arrays/regex; no type signature changes.
- Citations: verified inline against branch HEAD
4cff02cd.
What was not checked
- The sanctioned path (
afk service install→/service-setup) was not traced end-to-end to confirm it correctly bypasses all four layers. - No integration test covering the full four-layer chain in a single scenario.
- No changelog or docs surface reviewed.
Done
- What was done: Read-only review of PR feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier #1298 completed. MERGE recommended with 3 non-blocking low findings.
- Evidence: findings verified against ref
4cff02cd75ebe33bef690d1f471e6e75db35278e; all citations confirmed at reviewed ref; all absence claims grep-verified. - What changed: nothing — review is read-only per contract.
- Deferred: none.
🤖 Posted by agent-afk /review --post github
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 #1298 — feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier
Target: afk/issue-1279 → main · ref 4cff02cd · ~+150/−2 · 8 files
Regime: light (feature — security hardening)
Stated intent: PR #1298 title+body — spec-compliance assessed.
Findings
1 · low · blocking:false · confidence:high · security · src/agent/risk-classifier.ts:70-73 + src/agent/safe-destruct-patterns.ts:216-230 · ref:4cff02cd · diff-context
Service deregistration verbs absent — launchctl unload/bootout and systemctl disable/mask/stop not blocked. The PR blocks service installation/start verbs but not their inverses. An agent invoking launchctl unload /System/Library/LaunchDaemons/com.apple.auditd.plist or systemctl disable auditd can deregister security-critical services without triggering any of the four guards. This is a follow-up hardening opportunity, not a deficiency in the stated intent (which specifically targets preventing silent installation of persistent services). Separate from the registration concern this PR addresses.
// risk-classifier.ts — only registration verbs present:
'launchctl load', 'launchctl bootstrap', 'launchctl submit', 'launchctl start'
'systemctl enable', 'systemctl start', 'systemctl daemon-reload'
// unload, bootout, disable, mask, stop are absent
suggestion: Open a follow-up issue to extend coverage to service-removal verbs (launchctl unload|bootout, systemctl disable|mask|stop|restart) in a separate PR. These are a distinct threat class (disabling protections) from the installation threat this PR closes.
2 · low · blocking:false · confidence:medium · correctness · src/agent/risk-classifier.ts:73 · ref:4cff02cd · diff-context
'launchctl start' substring in BASH_HIGH over-matches compound subcommands. classifyBash at line 163 uses cmd.includes(p) with no word-boundary anchor. The string 'launchctl start' matches launchctl start-session, launchctl start-ui, or any command containing the literal prefix launchctl start. Pre-existing pattern — the same substring technique is used by all BASH_HIGH entries (e.g. 'rm -rf', 'sudo'). The blast radius is narrow because launchctl start-* subcommands are uncommon in practice.
for (const p of BASH_HIGH) {
if (cmd.includes(p)) return 'high'; // line 163 — raw substring
}suggestion: No immediate action needed (pre-existing pattern). If false positives are observed, append a trailing space: 'launchctl start '.
3 · low · blocking:false · confidence:high · correctness · src/agent/safe-destruct-patterns.ts:224 · ref:4cff02cd · diff-context
Pattern id: 'systemctl-enable' names only one of three matched verbs. The regex covers enable|start|daemon-reload, but the id references only enable. A block on systemctl daemon-reload emits blocked [systemctl-enable] in logs and audit traces — diagnostically misleading. The launchctl counterpart (id: 'launchctl-load') has the same asymmetry but load is the canonical/primary verb, making it less confusing.
suggestion: Rename to 'systemctl-service' or 'systemctl-lifecycle' to reflect the multi-verb scope. Alternatively, leave as-is — the blockReason text is accurate and the id is primarily a stable key for dedup, not a user-facing label.
4 · low · blocking:false · confidence:high · security · src/agent/tools/hooks/bash-restriction-hook.ts:132 · ref:4cff02cd · file-state (verified)
/etc/systemd/system absent from SENSITIVE_PATH_SIGNAL. The system-wide systemd unit directory is blocked in write-denylist.ts:76 and reaches builtinBashSensitiveRoots() via the ...BUILTIN_READ_DENYLIST spread (check 2), but SENSITIVE_PATH_SIGNAL (check 1 — the lexical interpreter-guard regex) does not include it. Gap is narrowly scoped: only affects interpreter one-liners (python -c "open('/etc/systemd/system/x')") on headless surfaces with forceInterpreterGuard=1 where restrictedSubstrings is empty and the signal is the sole protection. On interactive surfaces, check 2's substring scan covers it.
// SENSITIVE_PATH_SIGNAL includes:
\.config\/systemd\/user\b ← user dir ✓
// but NOT:
\/etc\/systemd\/system\b ← system dir ✗
suggestion: Add \/etc\/systemd\/system\b to SENSITIVE_PATH_SIGNAL for completeness. Low urgency — the gap requires the rare conjunction of headless + forced interpreter guard + interpreter one-liner targeting the system unit dir.
5 · low · blocking:false · confidence:high · test-coverage · src/agent/risk-classifier.test.ts · ref:4cff02cd · diff-context
No negative test for the substring boundary case in risk-classifier. Searched git grep 'start-' 4cff02cd -- src/agent/risk-classifier.test.ts — zero matches. The launchctl list → NOT high test covers the read-only command case, but no test asserts that launchctl start-session (or similar compound subcommands) are classified correctly. This verifies or documents the over-match behavior from finding 2.
suggestion: Add: it('launchctl start-session → NOT high (start prefix, not start subcommand)'). If the over-match is intentional (conservative), document it.
6 · nit · blocking:false · confidence:high · spec-compliance · diff-context
PR title says "registration paths" but the change blocks start/execution verbs too. launchctl start and systemctl start are lifecycle/execution verbs — they operate on already-registered services, not the registration itself. The body's bullet list correctly names them, but the title's framing is narrower than the actual scope. Cosmetic — the implementation is the correct behavior.
Spec-compliance
Stated intent: PR #1298 title+body — spec-compliance assessed.
| Requirement | Status |
|---|---|
| write-denylist: LaunchAgents, LaunchDaemons (macOS) | ✅ 4 paths, platform-guarded |
| write-denylist: systemd/user, /etc/systemd (Linux) | ✅ 2 paths, platform-guarded |
| bash-restriction-hook: same paths in builtinBashSensitiveRoots() | ✅ 3 entries added |
| bash-restriction-hook: SENSITIVE_PATH_SIGNAL updated | ✅ LaunchAgents, LaunchDaemons, .config/systemd/user added |
| risk-classifier: launchctl load/bootstrap/submit/start | ✅ 4 BASH_HIGH entries |
| risk-classifier: systemctl enable/start/daemon-reload | ✅ 3 BASH_HIGH entries |
| safe-destruct: launchctl-load pattern (block tier) | ✅ Regex covers load/bootstrap/submit/start |
| safe-destruct: systemctl-enable pattern (block tier) | ✅ Regex covers enable/start/daemon-reload |
| Read-only commands remain allowed | ✅ Negative tests for launchctl list/print, systemctl status/is-enabled |
| Tests for all four layers | ✅ All 4 production files have corresponding test additions |
No unmet intent. Scope slightly wider than title implies (start verbs, not just registration) — implementation is correct, title is cosmetically narrow.
Dimensions with no issues
- api-compat — no issues found. All changes are additive (new array entries, regex alternations). No exported symbol signatures changed.
BASH_HIGH,DESTRUCTIVE_PATTERNS,BUILTIN_WRITE_DENYLIST,SENSITIVE_PATH_SIGNAL,builtinBashSensitiveRoots()retain existing types. Read: all 4 changed production files. - perf-observability — no issues found. 7 new substring entries in a small static array (O(n) scan), 3 regex alternations, 6 denylist entries — all negligible additions to existing linear scans. No hot-path regressions. Read: risk-classifier.ts, bash-restriction-hook.ts.
What was not checked
- Citations verified inline against branch HEAD
4cff02cd75ebe33bef690d1f471e6e75db35278e. - Stated intent: PR #1298 title+body — spec-compliance assessed.
- Not checked: runtime behavior. Static review only; no tests executed.
- Not checked: whether the
afk service installsanctioned path correctly exempts itself from these guards (pre-existing behavior, not introduced by this PR). - Not checked: interaction between the new BASH_HIGH entries and the safe-destruct BLOCK patterns when both fire on the same command (the risk-classifier rates the command
highAND safe-destruct blocks it — both gates activate independently; no ordering conflict).
Decision: MERGE — 0 blocking (4 low, 1 nit, 1 low security follow-up).
Clean, well-structured security hardening that correctly layers service-registration blocking across all four guard surfaces (write-denylist, bash-restriction-hook, risk-classifier, safe-destruct). Platform guards are correct (macOS-only paths guarded by process.platform === 'darwin', Linux-only by 'linux'). Read-only commands are explicitly carved out with negative tests. The low findings are follow-up hardening opportunities (service-deregistration verbs, substring boundary, pattern id naming, SENSITIVE_PATH_SIGNAL completeness) — none are regressions introduced by this PR.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (scheduled sweep), generated by the /review tool — a maintainer will follow up.
Prior Automated Rounds
This is the first automated review of PR #1298. No prior findings to carry forward.
Decision
✅ MERGE (with follow-ups)
Severity arithmetic: 0 critical, 0 high → MERGE (with follow-ups); 1 medium advisory (non-blocking), 6 low advisory, 2 nit advisory.
Per this sweep's policy: only critical and high findings block merge. The medium finding below is non-blocking and suggests a follow-up issue.
Review Summary
PR: feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier
Reviewed ref: 4cff02cd75ebe33bef690d1f471e6e75db35278e
Regime: light (125 changed lines, 7 files: 3 production, 4 test)
CI: All checks passing ✅
The change correctly adds OS service registration path blocking across four independent security layers (write-denylist, bash-restriction-hook, risk-classifier, safe-destruct-patterns). The defense-in-depth approach is sound, tests are adequate, and no critical or high issues were found.
Findings (all advisory — non-blocking)
Medium (advisory — suggest a follow-up issue)
M1 · Test coverage gap in two-tier hook · src/agent/safe-destruct-detect.test.ts
The createSafeDestructDetect two-tier hook tests add BLOCK cases for launchctl load, launchctl bootstrap, systemctl enable, systemctl daemon-reload but do not add the read-only non-block negative cases (launchctl list, launchctl print, systemctl status, systemctl is-enabled) to the two-tier describe block. Those negative cases exist only in the detectDestructiveCommands section. If a future regression is introduced at the two-tier wrapper level, the read-only safety property would be untested at that layer.
Suggestion: Add launchctl list, launchctl print, systemctl status, systemctl is-enabled to the "passes through benign bash commands" test section inside createSafeDestructDetect.
Low (advisory)
L1 · /etc/systemd/system redundant in write-denylist · src/agent/tools/handlers/write-denylist.ts
/etc/systemd/system is added under a Linux platform guard, but /etc is already in BUILTIN_WRITE_DENYLIST unconditionally. The new entry is a strict path-prefix subset of /etc and is dead code — any write attempt is already blocked by the broader /etc entry. No functional impact, but it implies the path needs special handling when none is required.
Suggestion: Remove /etc/systemd/system from the Linux guard block; retain ~/.config/systemd/user (not subsumed by /etc).
L2 · /etc/systemd/system not in builtinBashSensitiveRoots() · src/agent/tools/hooks/bash-restriction-hook.ts
builtinBashSensitiveRoots() covers ~/.config/systemd/user (user-level systemd) but not /etc/systemd/system (system-level systemd). A bash command with the literal path /etc/systemd/system (e.g., cat /etc/systemd/system/myservice.service) is not caught by the bash-hook sensitive-root substring scanner. systemctl commands are still caught by the risk-classifier and safe-destruct layers, so this is a defense-in-depth gap, not an exploitable bypass.
Suggestion: Add '/etc/systemd/system' to builtinBashSensitiveRoots() alongside the other system-level Linux paths.
L3 · /etc/systemd/system not in SENSITIVE_PATH_SIGNAL · src/agent/tools/hooks/bash-restriction-hook.ts
SENSITIVE_PATH_SIGNAL was extended with Library\/LaunchAgents, Library\/LaunchDaemons, and \.config\/systemd\/user but not \/etc\/systemd\/system. The regex already covers system-level credential paths for Linux — adding the systemd system path follows the same pattern. Same defense-in-depth gap as L2.
Suggestion: Add \/etc\/systemd\/system\b to SENSITIVE_PATH_SIGNAL.
L4 · launchctl enable not blocked · src/agent/risk-classifier.ts + src/agent/safe-destruct-patterns.ts
launchctl enable <service> (marks a service enabled in the disabled database) is not covered by BASH_HIGH or the launchctl-load safe-destruct pattern. Less severe than load/bootstrap (does not activate the service in-session), but it mutates service-registration state that persists across reboots.
Suggestion: Add 'launchctl enable' to BASH_HIGH and add enable to the launchctl-load safe-destruct regex alternation.
L5 · BASH_HIGH substring matching inconsistency with safe-destruct · src/agent/risk-classifier.ts
BASH_HIGH uses raw .includes() (pre-existing mechanism), while safe-destruct-patterns.ts uses word-boundary regex. The new entries 'launchctl start' and 'systemctl start' will match these substrings inside comments and echo statements — consistent with the pre-existing mechanism's accepted tradeoff, but the inconsistency between layers is worth a comment.
Suggestion: Add a comment to BASH_HIGH noting that the entries for launchctl/systemctl use the intentionally conservative substring match and listing known acceptable false-positive classes.
Nit
N1 · systemctl-enable pattern id misleading · src/agent/safe-destruct-patterns.ts
The id systemctl-enable implies only systemctl enable is matched, but the pattern also covers start and daemon-reload. Operators searching telemetry for why systemctl daemon-reload was blocked will not find this id intuitively.
Suggestion: Rename to systemctl-service-mutate or systemctl-activate.
N2 · Block reason omits daemon-reload · src/agent/safe-destruct-patterns.ts
Block reason reads "enables or starts a persistent systemd service" — daemon-reload is not mentioned despite being matched by the pattern. An agent blocked on systemctl daemon-reload receives an inaccurate explanation.
Suggestion: Update to "enables, starts, or reloads (daemon-reload) a persistent systemd service configuration".
Spec-compliance
All stated blocking/allowing behaviors confirmed implemented:
- ✅
launchctl load|bootstrap|submit|start→ blocked (safe-destruct, risk-classifier, bash-hook signal) - ✅
systemctl enable|start|daemon-reload→ blocked (safe-destruct, risk-classifier, bash-hook signal) - ✅
launchctl list,launchctl print→ NOT blocked (confirmed by tests) - ✅
systemctl status,systemctl is-enabled→ NOT blocked (confirmed by tests) - ✅ Write-denylist S5 paths added with platform guards (darwin + linux)
- ✅
builtinBashSensitiveRoots()andSENSITIVE_PATH_SIGNALupdated
What Was Not Checked
- Citations verified inline against branch HEAD
4cff02cd75ebe33bef690d1f471e6e75db35278e. - Stated intent: PR #1298 title + body — spec-compliance assessed (see above).
- Tests were not run locally; CI is green on all active platforms (ubuntu, macos).
- Did not verify that
afk service install//service-setupcorrectly bypasses the new blocks (out of scope for this PR). - Windows platform paths were not reviewed (no Windows-specific changes in this PR).
…sk-classifier `systemctl --user enable/start/daemon-reload` bypassed both the safe-destruct regex and the BASH_HIGH substring match because `--user` (or any global option) sits between `systemctl` and the verb. Fix: - safe-destruct-patterns: update regex with optional-flags quantifier so `systemctl --user enable`, `systemctl --no-block --user start`, etc. are caught - risk-classifier: add explicit `systemctl --user` substring entries (the substring table cannot express optional flags; the safe-destruct regex covers the general case) - Tests for both layers covering the --user variant The repo's own systemd installer (`src/service/systemd/install.ts`) uses `systemctl --user daemon-reload` and `systemctl --user enable`, confirming this is the canonical form for user-service management. Credit: chatgpt-codex-connector review comment on PR #1298.
|
Finding 1 is independently verified: the regex bug is real, and the suggested fix ( PR #1298 — Review:
|
| # | Sev | Blocking | Dim | Location | Finding |
|---|---|---|---|---|---|
| 1 | medium | true | security | safe-destruct-patterns.ts:228 |
systemctl-enable regex false-positives on systemctl enable-linger. The terminal \b word-boundary fires between enable and - (a non-word char), so enable in enable-linger satisfies the alternation. Confirmed via live regex test: re.test('systemctl enable-linger') → true. The correct command is loginctl enable-linger (recommended by /service-setup at src/service/systemd/install.ts:102), but a model that misspells it as systemctl enable-linger gets hard-blocked with a misleading "installs a persistent systemd service" message. Fix verified: replacing \b with `(?:\s |
| 2 | low | false | correctness | risk-classifier.ts:76 |
'systemctl start' substring in BASH_HIGH false-positives on systemctl start-transient (classified high instead of medium). Zero production callers; impact is friction only. |
| 3 | low | false | correctness | bash-restriction-hook.ts:483-486 |
builtinBashSensitiveRoots() adds Library/LaunchAgents, Library/LaunchDaemons, .config/systemd/user unconditionally — no process.platform guard. On Linux, ~/Library/LaunchAgents is a spurious restricted substring; on macOS, ~/.config/systemd/user is spurious. BUILTIN_WRITE_DENYLIST uses platform guards for the same paths. |
| 4 | nit | false | test-coverage | write-denylist.test.ts (new) |
Linux else-branch filter !p.includes('/etc') allows /etc/systemd paths to pass undetected on non-linux. |
| 5 | low | false | test-coverage | safe-destruct-detect.test.ts (new) |
Two-tier hook test missing entries: launchctl submit, launchctl start, systemctl start, systemctl --user start. |
| 6 | low | false | test-coverage | risk-classifier.test.ts (new) |
Missing launchctl submit/launchctl start positive cases; missing launchctl print/systemctl status/systemctl is-enabled negative cases. |
| 7 | nit | false | security | safe-destruct-patterns.ts:218 |
launchctl enable gui/501/com.foo (DB-persistence, distinct from load) is not in the launchctl-load alternation. Confirmed absent via grep. |
Decision: DO NOT MERGE — 1 medium blocking (security); 3 low, 2 nit non-blocking.
Required before merge: Fix the systemctl-enable regex terminal anchor. Change:
re: /\bsystemctl\s+(?:--?\w[\w-]*(?:[= ]\S+)?\s+)*(enable|start|daemon-reload)\b/i,
to:
re: /\bsystemctl\s+(?:--?\w[\w-]*(?:[= ]\S+)?\s+)*(enable|start|daemon-reload)(?:\s|$)/i,
This one-character fix (\b → (?:\s|$)) eliminates the enable-linger false-positive while preserving all legitimate matches. Verified by live test against all positive and negative cases.
What was not checked
- Citations verified inline against branch HEAD
43a01c0bviagit showand live Node.js regex execution. - Stated intent: PR feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier #1298 title+body — spec-compliance assessed. All four named layers (write-denylist, bash-restriction-hook, risk-classifier, safe-destruct-patterns) are implemented. Read-only commands pass. No unmet intent or scope creep detected.
- Not checked: Integration tests exercising all four layers together; Windows platform behavior;
launchctl bootout/launchctl disablecoverage; whetherafk service installproperly bypasses the new blocks at runtime.
Done
- What was done: Reviewed PR feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier #1298 across security, correctness, spec-compliance, test-coverage, and perf-observability dimensions with Wave 1 (single research agent), Wave 1.5 (inline citation + absence-claim verification), Wave 2 (synthesis), and post-synthesis shadow-verification of the blocking finding.
- Evidence: All findings verified at ref
43a01c0b— the blocking regex bug confirmed by live Node.js execution, the suggested fix validated against all positive/negative cases. - What changed: nothing (read-only review)
- Deferred: none
🤖 Posted by agent-afk /review --post github
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.
Prior Automated Rounds
5 prior automated reviews exist on this PR (3 on commit 4cff02cd, 2 on commit 43a01c0b). Current commit 716d4bc5 is a merge of main into the feature branch — no implementation changes since prior reviews. This round re-confirms findings against the merged HEAD.
Decision: DO NOT MERGE — 1 high blocking (security); 1 medium non-blocking, 2 low, 0 nit.
Review Summary
PR: feat(security): block OS service registration paths in bash, write-denylist, and risk-classifier
Reviewed ref: 716d4bc529a1638c648509a96de7cd2ea6d5f5d8
Regime: light (~150 changed lines, 8 files: 4 production, 4 test)
CI: Lint & Build ✅, Tests (ubuntu, PTY scrollback, Docs, Publish) ✅, Tests (macos) IN_PROGRESS
Findings
⛔ F1 · high · blocking:true · confidence:high · security · src/agent/safe-destruct-patterns.ts:225 + src/agent/risk-classifier.ts:74 · ref:716d4bc5 · file-state (shadow-verified: CONFIRMED)
systemctl enable-linger false-positive across TWO independent security layers. This is a persistent finding from prior review rounds — still unfixed at the merged HEAD.
Layer 1 (safe-destruct): The systemctl-enable regex /\bsystemctl\s+(?:--?\w[\w-]*(?:[= ]\S+)?\s+)*(enable|start|daemon-reload)\b/i false-positives on systemctl enable-linger because \b (word boundary) fires between enable and - (a non-word character). Live regex test: re.test('systemctl enable-linger') → true.
Layer 2 (risk-classifier): The BASH_HIGH substring 'systemctl enable' matches via .includes(): 'systemctl enable-linger'.includes('systemctl enable') → true. This independently classifies systemctl enable-linger as high risk.
The /service-setup sanctioned flow references loginctl enable-linger (the correct command), but a user who runs systemctl enable-linger (a common typo/alias) hits a double-layer false block with a misleading "enables or starts a persistent systemd service" message.
// safe-destruct-patterns.ts:225 — terminal \b fires at enable|hyphen boundary
re: /\bsystemctl\s+(?:--?\w[\w-]*(?:[= ]\S+)?\s+)*(enable|start|daemon-reload)\b/i,
// risk-classifier.ts:74 — substring containment
'systemctl enable', // 'systemctl enable-linger'.includes('systemctl enable') → trueShadow-verify verdict: CONFIRMED — independently re-derived via live Node.js regex execution against the reviewed ref. The proposed fix was also validated:
Fix (safe-destruct): Replace \b with (?:\s|$):
re: /\bsystemctl\s+(?:--?\w[\w-]*(?:[= ]\S+)?\s+)*(enable|start|daemon-reload)(?:\s|$)/i,
Confirmed: passes all positive cases (enable my-svc, start my-svc, daemon-reload, --user enable --now svc) and rejects enable-linger.
Fix (risk-classifier): Append a trailing space: 'systemctl enable ' (and likewise 'systemctl start ', 'systemctl daemon-reload' — the last already lacks this issue since daemon-reload has no hyphenated variant).
F2 · medium · blocking:false · confidence:high · correctness · src/agent/tools/hooks/bash-restriction-hook.ts:484–488 · ref:716d4bc5 · file-state (verified) · waived: bounded — Windows is not a production target; the paths resolve to non-existent directories on non-matching platforms, producing false-positive substring noise but no security regression
builtinBashSensitiveRoots() adds LaunchAgents/LaunchDaemons/systemd paths unconditionally cross-platform, unlike write-denylist which platform-gates them. write-denylist.ts:65–78 uses process.platform === 'darwin' / 'linux' guards, but builtinBashSensitiveRoots() adds all three paths on every platform. On Linux, ~/Library/LaunchAgents is a spurious restricted substring; on macOS, ~/.config/systemd/user is spurious. The asymmetry between layers complicates future audits.
// bash-restriction-hook.ts:484-488 — no platform guard
path.join(home, 'Library', 'LaunchAgents'),
path.join(home, 'Library', 'LaunchDaemons'),
path.join(home, '.config', 'systemd', 'user'),
// write-denylist.ts:65-78 — platform-guarded
...(process.platform === 'darwin' ? [LaunchAgents, LaunchDaemons] : []),
...(process.platform === 'linux' ? [systemd/user] : []),suggestion: Wrap the three new entries in matching platform guards for parity with write-denylist.
F3 · low · blocking:false · confidence:high · correctness · src/agent/tools/handlers/write-denylist.ts:76 · ref:716d4bc5 · file-state (verified)
/etc/systemd/system in S5 linux block is a strict subset of the unconditional /etc entry at line 39. The denylist already blocks all of /etc on all platforms. The S5 entry is documentation intent, not additional coverage.
suggestion: Add a comment: // Explicit for documentation; already covered by /etc above.
F4 · low · blocking:false · confidence:high · test-coverage · src/agent/safe-destruct-detect.test.ts:117–120 + src/agent/risk-classifier.test.ts · ref:716d4bc5 · file-state (verified, absence confirmed via git grep)
No negative test for systemctl enable-linger in either safe-destruct or risk-classifier test suites. This is the test gap that allowed the F1 false-positive to survive. git grep 'enable-linger' <ref> -- src/agent/ returns zero matches.
suggestion: Add negative test cases in both suites:
safe-destruct-detect.test.ts:['systemctl enable-linger root']in the "does not flag benign" blockrisk-classifier.test.ts:it('systemctl enable-linger → NOT high', ...)
Spec-compliance
Stated intent: PR #1298 title+body — spec-compliance assessed.
| Requirement | Status |
|---|---|
| write-denylist: LaunchAgents, LaunchDaemons (macOS) | ✅ 4 paths, platform-guarded |
| write-denylist: systemd/user, /etc/systemd (Linux) | ✅ 2 paths, platform-guarded |
| bash-restriction-hook: builtinBashSensitiveRoots() | ✅ 3 entries added |
| bash-restriction-hook: SENSITIVE_PATH_SIGNAL | ✅ LaunchAgents, LaunchDaemons, .config/systemd/user added |
| risk-classifier: launchctl load/bootstrap/submit/start | ✅ 4 BASH_HIGH entries |
| risk-classifier: systemctl enable/start/daemon-reload | ✅ 6 BASH_HIGH entries (bare + --user variants) |
| safe-destruct: launchctl-load block pattern | ✅ Regex covers load/bootstrap/submit/start |
| safe-destruct: systemctl-enable block pattern | |
| Read-only commands remain allowed | launchctl list/print and systemctl status/is-enabled pass; systemctl enable-linger does NOT pass (false block) |
| Tests for all four layers | ✅ All 4 production files have test additions — missing negative case for enable-linger |
Dimensions with no issues
- api-compat — no issues found. All changes are additive (new array entries, regex alternations). No exported symbol signatures changed. Read: all 4 production files.
- perf-observability — no issues found. 10 new substring entries in a small static array, 2 regex alternations, 6 denylist entries — all negligible additions to existing linear scans. The
(?:...)*quantifier in the systemctl-enable regex is not catastrophically backtrackable. Read:risk-classifier.ts,safe-destruct-patterns.ts.
What was not checked
- Citations verified inline against branch HEAD
716d4bc529a1638c648509a96de7cd2ea6d5f5d8. - Stated intent: PR #1298 title+body — spec-compliance assessed.
- Shadow-verify invoked on F1 (high) — verdict CONFIRMED via live Node.js regex execution.
- Not checked: runtime integration tests (static review only; CI is green on ubuntu/PTY/docs).
- Not checked: whether
afk service install//service-setupcorrectly bypasses all four layers at runtime. - Not checked:
launchctl enable(distinct fromload) — not in the safe-destruct or risk-classifier coverage. - Not checked: Windows platform behavior (Windows is not a production target for these paths).
1 participant
Summary
Blocks OS service registration paths across all four security layers — preventing agents from silently installing persistent services (launchd on macOS, systemd on Linux) that survive reboots and session termination.
The sanctioned path for service installation is
afk service installvia/service-setup.Changes
write-denylist (
S5tier)~/Library/LaunchAgents,~/Library/LaunchDaemons(macOS)/Library/LaunchAgents,/Library/LaunchDaemons(macOS)~/.config/systemd/user(Linux)/etc/systemd/system(Linux)bash-restriction-hook
builtinBashSensitiveRoots()SENSITIVE_PATH_SIGNALregex updated to coverLaunchAgents,LaunchDaemons,.config/systemd/userrisk-classifier (high-risk bash patterns)
launchctl load|bootstrap|submit|startsystemctl enable|start|daemon-reloadsafe-destruct-patterns (block tier)
launchctl-load: blockslaunchctl load|bootstrap|submit|startsystemctl-enable: blockssystemctl enable|start|daemon-reloadRead-only commands remain allowed
launchctl list,launchctl print— no blocksystemctl status,systemctl is-enabled— no blockTests
All four layers have dedicated test coverage, including negative cases for read-only commands and platform guards (macOS-only paths don't appear on Linux, and vice versa).
Closes #1279