fix(codex-app-server): honor approvals.mode/yolo for gateway-context approval routing (salvage of #26533 by @simpolism, closes #26530) - #448
Conversation
…approval routing (salvage of NousResearch#26533 by @simpolism, closes NousResearch#26530)
|
Review Complete Files Reviewed: 3 By Severity:
Three medium-severity findings: a session-routing staleness bug where /yolo toggles have no effect on codex app-server auto-approvals, a PowerShell encoded-command detection regex that misses valid abbreviation variants, and a defense-in-depth gap where codex auto-approve skips Hermes' hardline command floor. Files Reviewed (3 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (58/100) — 3 medium findings · 254 LOC across 3 files
Summary
This PR modifies 3 files (agent/codex_runtime.py, tests/run_agent/test_codex_app_server_integration.py, tools/approval.py) introducing codex app-server execution routing and shell-command detection refinements. The review identified 3 medium-severity findings requiring attention before merge.
Finding-001: Stale routing after /yolo toggle
agent/codex_runtime.py:277-311 — The _ServerRequestRouting dataclass is computed once at session creation via is_approval_bypass_active(). The codex session is reused across turns (lazy-init guard at line 255). If a gateway user toggles /yolo after session creation, the frozen routing remains stale — auto_approve_exec stays False and the routing never updates until session retirement. The /yolo handler does not evict the _agent_cache, so the stale routing persists.
Finding-002: Incomplete PowerShell abbreviation coverage
tools/approval.py:513 — The regex (?:encodedcommand|enc|e)\b only matches three literal spellings of -EncodedCommand. PowerShell resolves any unambiguous prefix, so -enco, -encode, -encoded, and -en all bypass detection. An attacker can issue encoded commands using abbreviated flags undetected. The git reset and sudo patterns in the same patch already use proper abbreviation-resolution patterns.
Finding-003: Codex auto-approve skips hardline floor
agent/codex_runtime.py:281 — When auto_approve_exec is True, _decide_exec_approval returns 'accept' immediately without running detect_hardline_command(). The normal terminal path checks hardline commands (rm -rf /, mkfs, dd to raw device, etc.) before yolo bypass. The codex path has no equivalent hardline floor, undermining defense-in-depth.
| @@ -281,6 +305,10 @@ def _on_codex_event(note: dict) -> None: | |||
| agent._codex_session = CodexAppServerSession( | |||
| cwd=cwd, | |||
| approval_callback=approval_callback, | |||
| request_routing=_ServerRequestRouting( | |||
| auto_approve_exec=auto_approve_requests, | |||
| auto_approve_apply_patch=auto_approve_requests, | |||
| ), | |||
There was a problem hiding this comment.
🟡 Codex app-server auto-approval routing frozen at session creation — stale after /yolo toggle (bug)
The _ServerRequestRouting dataclass with auto_approve_exec and auto_approve_apply_patch is computed via is_approval_bypass_active() only when the CodexAppServerSession is first created (agent/codex_runtime.py lines 277-311). The session is then reused across turns via a lazy-init guard at line 255. If a gateway user toggles /yolo after the session is created, is_current_session_yolo_enabled() becomes True, but the frozen routing still has auto_approve_exec = auto_approve_apply_patch = False. In the codex session's _decide_exec_approval (line 697-698) and _decide_apply_patch_approval (line 720-721), the routing is checked first; with no approval_callback wired in gateway contexts, both methods fall through to fail-closed decline (lines 718, 759). The /yolo handler in gateway/slash_commands.py (line 2857-2872) only calls enable_session_yolo()/disable_session_yolo() and does not evict the _agent_cache, so the stale routing persists until session retirement (line 347 should_retire) or agent restart.
💡 Suggestion: Re-evaluate is_approval_bypass_active() at the start of each turn (before run_turn at line 320) and update the codex session's _routing in-place. Alternatively, refactor _decide_exec_approval and _decide_apply_patch_approval to call is_approval_bypass_active() dynamically instead of reading the frozen dataclass.
📋 Prompt for AI Agents
In agent/codex_runtime.py, before agent._codex_session.run_turn() at line 320, add a block that re-evaluates the routing: agent._codex_session._routing = _ServerRequestRouting(auto_approve_exec=is_approval_bypass_active(), auto_approve_apply_patch=is_approval_bypass_active()). Wrap in try/except to preserve existing routing on failure. Alternatively, refactor agent/transports/codex_app_server_session.py _decide_exec_approval (line 697) and _decide_apply_patch_approval (line 720) to call is_approval_bypass_active() directly instead of checking self._routing.auto_approve_exec / self._routing.auto_approve_apply_patch.
| # so bare invocations are caught while a benign path arg containing | ||
| # "del"/"rm" (e.g. `-File c:\del-logs\run.ps1`) is not. | ||
| (r'\b(?:powershell|pwsh)(?:\.exe)?\b(?:\s+-\S+)*\s+(?:-(?:command|c)\s+)?["\']?(?:remove-item|rmdir|erase|del|rd|ri|rm)\b', "Windows PowerShell destructive delete"), | ||
| (r'\b(?:powershell|pwsh)(?:\.exe)?\b.*\s-(?:encodedcommand|enc|e)\b', "PowerShell encoded command execution"), |
There was a problem hiding this comment.
🟡 PowerShell encoded command detection misses valid -EncodedCommand abbreviations (-enco, -encode, -en) (security)
The regex at tools/approval.py line 513 (?:encodedcommand|enc|e)\b only matches three literal spellings. PowerShell's own flag parser resolves ANY unambiguous prefix of a long flag name, so -enco, -encode, -encoded, and -en are all valid abbreviations for -EncodedCommand. An attacker can issue pwsh -enco <base64_payload> to execute arbitrary encoded commands without triggering the encoded-command detection rule, bypassing the entire dangerous-pattern scan for the decoded payload. The git reset pattern in the same patch correctly handles abbreviation resolution (--h(?:a(?:r(?:d)?)?)?) and the sudo stdin pattern uses --st[a-z]* for prefix matching, but the PowerShell pattern was not updated with the same technique.
💡 Suggestion: Expand the abbreviation alternation to match any unambiguous prefix of EncodedCommand starting from e. Use the same nested-optional-group pattern as the git reset rule: e(?:n(?:c(?:o(?:d(?:e(?:d(?:c(?:o(?:m(?:m(?:a(?:n(?:d)?)?)?)?)?)?)?)?)?)?)?)?\b — this matches every unambiguous abbreviation of -EncodedCommand starting from -e, consistent with how PowerShell itself and the git/sudo abbreviation patterns in this same patch handle option-prefix resolution.
📋 Prompt for AI Agents
In tools/approval.py line 513, change the regex from:
(r'\b(?:powershell|pwsh)(?:.exe)?\b.\s-(?:encodedcommand|enc|e)\b', ...)
to:
(r'\b(?:powershell|pwsh)(?:.exe)?\b.\s-e(?:n(?:c(?:o(?:d(?:e(?:d(?:c(?:o(?:m(?:m(?:a(?:n(?:d)?)?)?)?)?)?)?)?)?)?)?)?\b', ...)
This matches every unambiguous abbreviation of -EncodedCommand starting from -e, consistent with PowerShell's own parser and the git/sudo abbreviation patterns already in this file.
| try: | ||
| from tools.approval import is_approval_bypass_active | ||
|
|
||
| auto_approve_requests = is_approval_bypass_active() |
There was a problem hiding this comment.
🟡 Codex app-server auto-approve under yolo bypasses Hermes hardline command floor (security)
At agent/codex_runtime.py line 281, is_approval_bypass_active() gates auto_approve_exec and auto_approve_apply_patch. When True, _decide_exec_approval in agent/transports/codex_app_server_session.py line 698 returns 'accept' immediately, skipping the entire Hermes approval pipeline. The normal terminal path (check_all_command_guards at approval.py:2231-2238, check_dangerous_command at 1974-1982) checks for hardline commands (rm -rf /, mkfs, dd to raw device, shutdown/reboot, fork bomb, kill -1) BEFORE the yolo bypass — these catastrophic commands are unconditionally blocked even under yolo. The codex path has no such hardline floor: when auto_approve_exec is True, any exec request is accepted without Hermes-side inspection. Codex's own permission profile (mapped from HERMES_TERMINAL_SECURITY_MODE at codex_app_server_session.py:55-61: auto→workspace-write, yolo/unrestricted→full-access) provides a separate gate, but with 'full-access' the mitigation is removed entirely. The PR's design intentionally delegates hardline enforcement to codex's sandbox, but this creates an inconsistency with the defense-in-depth the terminal path provides.
💡 Suggestion: Run the Hermes hardline command detection on the codex exec command before auto-approving. Simplest approach: when bypass is active, pass the approval_callback that runs the Hermes pipeline (which preserves the hardline floor while honoring yolo for non-hardline commands), rather than setting auto_approve_exec directly to True. Alternatively, add detect_hardline_command(command) into _decide_exec_approval before the auto_approve_exec early return.
📋 Prompt for AI Agents
In agent/codex_runtime.py, after computing auto_approve_requests and before constructing the CodexAppServerSession (lines 305-313), do NOT set request_routing auto_approve_* to True unconditionally. Instead, when auto_approve_requests is True, wire the approval_callback through the existing Hermes pipeline — this preserves the hardline floor while still honoring yolo bypass for non-hardline commands, matching how the normal terminal path works. Or, add a hardline check into agent/transports/codex_app_server_session.py _decide_exec_approval at line 697 by importing and calling detect_hardline_command before the auto_approve_exec early return.
1 participant
Summary
Codex app-server exec/apply_patch requests on gateway/cron contexts now honor
approvals.mode: off//yolo/HERMES_YOLO_MODE=1instead of silently failing closed.Root cause: On non-CLI contexts no approval-UI callback is wired, so
CodexAppServerSession._decide_exec_approval/_decide_apply_patch_approvalhit their fail-closedreturn "decline"path. Codex sees a synthetic "user denied" and drops to read-only — the bot appears responsive but can't write files, and no prompt surfaces anywhere.Fix: When the user has explicitly opted out of Hermes approvals, build the session with
_ServerRequestRouting(auto_approve_exec=True, auto_approve_apply_patch=True)so codex's own sandbox permission profile (~/.codex/config.toml) becomes the boundary. Defaults (manual/smart/unset) keep the current fail-closed behavior — a no-op for anyone who hasn't opted out.Salvage of NousResearch#26533 by @simpolism (also the reporter of NousResearch#26530). Re-authored onto current
main: the construction site the original PR targeted (run_agent.py) has since moved intoagent/codex_runtime.py::run_codex_app_server_turn(). Authorship preserved.Changes
agent/codex_runtime.py: resolve opt-out at session-build time via the canonicaltools.approval._get_approval_mode()(which already normalizes the YAML-1.1 bare-off→Falsecase) +is_current_session_yolo_enabled()+HERMES_YOLO_MODE; passrequest_routing. Reading at build time means a mid-session/yolotoggle is honored too.tests/run_agent/test_codex_app_server_integration.py: 5 tests — configoff, YAMLFalse,HERMES_YOLO_MODE, session/yolo, plus the defaultmanualfail-closed regression guard.Validation
offacceptmanual/smart(default)declineHERMES_YOLO_MODE=1accept/yolosession toggleacceptHERMES_HOMEconfig: the gateway-context exec decision flipsdecline→acceptonly when opted out; fail-closed preserved otherwise.Notes
mcpServer/elicitation/requestfail-closed path for non-hermes-toolsservers (reported on codex app-server tool calls fail closed on gateway with no surfaceable approval prompt NousResearch/hermes-agent#26530 by @TAE58). That path declines by hard-coded server-name gating (arguably intentional) rather than_routing— a separate design question.Closes NousResearch#26530
Follow-up commit (self-review,
hermes-pr-reviewPhase 2)refactor(approval): extract is_approval_bypass_active(); use frozen-env bypass in codex routingPhase-2 review flagged that the initial adaptation re-read
os.getenv("HERMES_YOLO_MODE")at runtime, which diverges from the repo's security invariant —HERMES_YOLO_MODEis frozen into_YOLO_MODE_FROZENat import time precisely so a mid-process skill can't set the env var and flip the approval bypass (prompt-injection escalation path). Fixed architecturally: extracted the canonical three-source bypass check intotools.approval.is_approval_bypass_active()(this was the 4th inline copy of that OR-chain;codex_runtime.pynow calls the shared helper). Env-yolo test updated to patch_YOLO_MODE_FROZEN(canonical pattern) instead ofsetenv. 28 integration + 77 session/yolo tests pass; E2E re-confirmed.Mirror-of: NousResearch#56534
NousResearch#56534