[EddieMurphy89]

Summary

In a --yes or auto_for_me session, apodex relies on the persisted deny list in ~/.config/apodex/permissions.json as the last control that stops a command the user permanently blocked. If that file exists but cannot be read or parsed, PermissionStore.load() currently catches the error and returns an empty store, so a saved deny rule can silently disappear and the previously blocked command can run unattended.

This PR makes the read fail closed and the write atomic:

• FileNotFoundError (first run) still yields an empty policy.
• A file that exists but is unreadable, unparseable, or has the wrong shape records load_error; denies() returns True and the tool gate returns RISK_DENY with a clear reason instead of falling back to an empty policy.
• save() writes to a temporary file, fsyncs, and os.replaces it, so an interrupted write leaves the previous file intact instead of truncating it.

Impact

What the bypass gives the attacker

• In a --yes or auto_for_me session, a corrupted rule file turns the user's permanent "never" into "run it unattended", with the user's own rights.
• The surface is the whole deny list rather than one command: deny accepts command prefixes and bare tool names (_name_matches), and the catalogue includes bash, write_file, file_editor, run_python_code, web_fetch/download_file.
• A deployment that implements read-only or offline operation by denying those tools loses that posture in one step: files become writable, the network tools usable, and code execution available. Restored curl/ssh/git push-class commands let an agent steered by repository content or a fetched page send what it can read (source, .env, tokens) to an external host, and an unbanned write_file reaches build scripts and CI configuration.
• The rules are re-read at every session start, so the widened surface persists for every later session until someone rewrites the file by hand. Nothing in the UI or the logs reports the loss, and the rules do not come back on their own.
• auto_for_me is the CLI's stock toggle for Docker/trusted environments; it makes assess_with_rules() return RISK_SAFE for every call that no saved deny rule rejects, so those deployments are in exactly the same state as a --yes run.
• The same root cause also removes plan_mode from settings.json, the "no edits until I approve a plan" gate: write_file then executes and writes the file. This PR keeps that settings change as a follow-up, but the impact is the same class of lost security state.

Affected versions

Tested on main at cc32d880ff (2026-11-03), pyproject.toml version 0.1.0. The repository has no release tag; the vulnerable code is still present on the current main as of this writing.

Reproduction

The decision layer can be checked without a model or a full session:

import json, tempfile
from pathlib import Path
from apodex.permissions import PermissionStore
from apodex.agent_tools import assess_with_rules

d = Path(tempfile.mkdtemp())
p = d / "permissions.json"

# Control: a valid file with a permanent deny.
p.write_text(json.dumps({"allow": [], "deny": ["Bash(git push)"]}), encoding="utf-8")
print(PermissionStore.load(str(p)).denies("bash", {"command": "git push"}))  # True

# Fault: the file exists but is truncated / invalid.
p.write_text('{"allow": [], "deny": ["Bash(git push)"]', encoding="utf-8")
store = PermissionStore.load(str(p))
print(store.denies("bash", {"command": "git push"}))
print(assess_with_rules("write_file", {"path": str(d / "x")}, str(d), store).level)

Before this PR: the fault prints False and confirm; under --yes the confirm is auto-approved. After this PR: it prints True and deny, with the reason "saved permission rules could not be read; refusing to run with an unknown policy".

A full CLI-level reproduction is the same sequence with the real binary: write a deny rule, truncate ~/.config/apodex/permissions.json, start apodex --yes, and request the denied command. The control run blocks; the fault run executes before the patch and blocks after it.

Fix

• apodex/permissions.py:304-319 — load() distinguishes missing from unreadable/invalid; load_error is recorded for the latter; denies() fails closed and allows() returns False in that state. save() now uses a temporary file + fsync + os.replace, creating the temporary file with 0600 (and preserving existing owner-only bits) so the atomic replace does not widen permissions.
• apodex/agent_tools.py:440 — assess_with_rules() checks load_error before the normal rule lookup and returns RISK_DENY.
• apodex/tests/test_permissions.py — new regression tests for missing/corrupt/wrong-shape/unreadable files, the gate decision, and the atomic-save failure path.

The same loader shape also exists in UserSettings.load() for settings.json (plan_mode); this PR deliberately keeps the change focused on the permission store, and I can split the settings change into a follow-up if you prefer.

Testing

• pytest apodex/tests/test_permissions.py -q — 10 passed (including restrictive-mode preservation)
• The new tests fail on unpatched main and pass with the patch.
• I could not run the full repository suite in my environment; CI will cover it. I am happy to adjust if any existing test expects the old empty-store behaviour.

[EddieMurphy89]

Scope

This PR has stayed focused: 3 files, the fail-closed behavior change plus its regression test. No extra hardening was added in response to review comments. The only pending items are workflow approval and review.

[zhanghanduo]

The fail-closed behavior looks good. I found two things to address before merging:

• In PermissionStore.save(), replacing the existing file with a newly created temporary file drops the original file permissions. I reproduced a 0600 policy becoming 0664 when saving under umask 0002. That would let other members of the group edit the allow/deny rules if they can access the directory. Could we preserve the existing restrictive permissions, or create the temporary file with mode 0600?

• The directory-content assertions in test_failed_save_keeps_the_previous_file and test_save_replaces_the_file_atomically fail because the existing _isolated_user_dirs autouse fixture creates a home directory inside tmp_path. Both tests see ['home', 'permissions.json'] rather than just ['permissions.json']. Using a separate subdirectory for the policy file, or checking specifically that no temporary files remain, should fix this.

I checked the PR files in an isolated copy alongside the existing feature tests; the focused run finished with 114 passed and these 2 tests failing.

[EddieMurphy89]

Thanks for the review and for reproducing both issues. I pushed 5a9bb9c:

• PermissionStore.save() now creates the temporary file with os.open(..., mode) instead of a umask-dependent open(). The mode defaults to 0600; if an existing file is present, its owner read/write bits are preserved (so 0400/0600 stay restrictive) and group/other bits are not inherited. A 0600 policy now remains 0600 when saved under umask 0002.
• test_failed_save_keeps_the_previous_file and test_save_replaces_the_file_atomically now put the policy in a separate policy/ subdirectory and assert the contents of that directory, so the autouse home directory no longer breaks them.
• Added test_save_keeps_restrictive_permissions to pin the 0600 behavior.

pytest apodex/tests/test_permissions.py -q — 10 passed. I did not run the broader focused suite locally; if you want, I can run that before the next push.