security: create secret files private, never chmod them after (#143)

[Mr-Neutr0n]

Fixes #143.

Both writers in secrets.py created the temp file at the umask default and only restricted it afterwards:

tmp.write_text(json.dumps(store, indent=2))   # umask default -> 0644
_restrict_to_user(tmp, is_dir=False)          # chmod 0600, too late
os.replace(tmp, self.path)

Measured

Sampling the mode at the moment _restrict_to_user is called — i.e. while the file already holds the plaintext:

before:  temp held the plaintext at 0o644  -> WORLD/GROUP READABLE
after:   temp held the plaintext at 0o600  -> user-only

Every local process could read every stored API key for the duration of the write, as could anything indexing or backing up that directory.

The fix

Both writers now route through _atomic_private_write, built on tempfile.mkstemp: the file is created 0600 and empty, the Windows ACL is applied while it is still empty, and only then is the content written. The Windows path matters — mkstemp gives no mode bits there, so ordering the icacls call before the write is what makes it equivalent.

Two more problems fall out of the same change

Both are proven by tests that fail on the current code:

1. The temp name was predictable. It was a fixed <name>.tmp. Pre-creating that path as a symlink redirected the write — the test writes victim.txt, symlinks secrets.json.tmp at it, and on current code the victim comes back containing:

{"openai": {"api_key": "sk-live"}}

So it's an arbitrary file overwrite and it discloses every key to a location the attacker chose. mkstemp uses O_EXCL plus a random suffix, so a pre-existing path is never followed.

2. A failed write left the temp behind. Now removed on any exception, with the previous secrets.json left intact.

Scope

This does not address #287 (secrets being plaintext-at-rest at all). That needs an OS keychain and is a much larger change. This is strictly about not widening the window further than the design already implies.

Tests

tests/test_secrets_file_mode.py — 7 cases. Three fail on the current code: mid-write mode, the hostile temp symlink, and cleanup after a failed write. The others pin the final mode, round-tripping, and no leftover temps. POSIX-mode assertions are skipped on Windows, which uses the ACL path.

Full suite: 923 passed. The 14 failures reproduce on a clean checkout — test_fake_slack.py (dead-port timeouts, #252) and test_bedrock_provider.py (missing bedrock extra, #284/#285).

---

Reviewed and tested locally; drafted with AI assistance.