fix(config): atomic write + 0o600 enforcement in persist_current#674
Open
hernandez42 wants to merge 1 commit into
Open
fix(config): atomic write + 0o600 enforcement in persist_current#674hernandez42 wants to merge 1 commit into
hernandez42 wants to merge 1 commit into
Conversation
Replace write_text+chmod with atomic_write_secure() that: - Creates tmp file with os.open(mode=0o600) — OS-level atomic create - fsyncs before rename for durability - os.replace() for atomic rename (POSIX + Windows) - Belt-and-suspenders chmod after replace - O_NOFOLLOW to block symlink-plant attacks Fixes: race window where API keys could be readable during the gap between write_text() and chmod().
Contributor
Greptile SummaryThis PR changes config persistence to use a secure atomic-write helper. The main changes are:
Confidence Score: 4/5The changed config persistence path can crash on Windows and on chmod-limited filesystems.
strix/config/loader.py Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
strix/config/loader.py:106
**Windows Flag Is Missing**
When the Windows release build reaches `persist_current()`, this expression reads `os.O_NOFOLLOW`, which is not defined on Windows. The CLI can import the module, but config persistence raises `AttributeError` at startup and the scan never starts.
### Issue 2 of 3
strix/config/loader.py:121
**Chmod Failure Aborts Startup**
The old path saved the config and ignored `OSError` from `chmod`, but this new post-replace chmod is uncaught. On filesystems or mounts that allow the write but reject POSIX mode changes, `persist_current()` now exits the CLI during startup after replacing the file.
### Issue 3 of 3
strix/config/loader.py:119
**Symlinked Config Is Replaced**
When `target` is a symlinked config file, `Path.write_text()` used to update the linked file, but `os.replace()` replaces the symlink entry with a new regular file. A `--config` path or dotfile-managed `~/.strix/cli-config.json` symlink will be silently broken, leaving the original config target stale.
Reviews (1): Last reviewed commit: "fix(config): atomic write + 0o600 in per..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| fd = os.open( | ||
| tmp_path, | ||
| flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW, |
Contributor
There was a problem hiding this comment.
When the Windows release build reaches persist_current(), this expression reads os.O_NOFOLLOW, which is not defined on Windows. The CLI can import the module, but config persistence raises AttributeError at startup and the scan never starts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/config/loader.py
Line: 106
Comment:
**Windows Flag Is Missing**
When the Windows release build reaches `persist_current()`, this expression reads `os.O_NOFOLLOW`, which is not defined on Windows. The CLI can import the module, but config persistence raises `AttributeError` at startup and the scan never starts.
How can I resolve this? If you propose a fix, please make it concise.| raise | ||
| os.replace(tmp_path, target) | ||
| # Belt-and-suspenders: enforce mode in case os.replace inherited anything odd | ||
| target.chmod(0o600) |
Contributor
There was a problem hiding this comment.
The old path saved the config and ignored OSError from chmod, but this new post-replace chmod is uncaught. On filesystems or mounts that allow the write but reject POSIX mode changes, persist_current() now exits the CLI during startup after replacing the file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/config/loader.py
Line: 121
Comment:
**Chmod Failure Aborts Startup**
The old path saved the config and ignored `OSError` from `chmod`, but this new post-replace chmod is uncaught. On filesystems or mounts that allow the write but reject POSIX mode changes, `persist_current()` now exits the CLI during startup after replacing the file.
How can I resolve this? If you propose a fix, please make it concise.| with contextlib.suppress(OSError): | ||
| os.close(fd) | ||
| raise | ||
| os.replace(tmp_path, target) |
Contributor
There was a problem hiding this comment.
When target is a symlinked config file, Path.write_text() used to update the linked file, but os.replace() replaces the symlink entry with a new regular file. A --config path or dotfile-managed ~/.strix/cli-config.json symlink will be silently broken, leaving the original config target stale.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/config/loader.py
Line: 119
Comment:
**Symlinked Config Is Replaced**
When `target` is a symlinked config file, `Path.write_text()` used to update the linked file, but `os.replace()` replaces the symlink entry with a new regular file. A `--config` path or dotfile-managed `~/.strix/cli-config.json` symlink will be silently broken, leaving the original config target stale.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
strix/config/loader.py::persist_current()writes the config file (containingsensitive env vars like
LLM_API_KEY,PERPLEXITY_API_KEY) viatarget.write_text()then separatelytarget.chmod(0o600). Between these twosteps the file is on disk with default permissions (
0o644), creating a racewindow where API keys could be read by other local users.
Fix
Replace the two-line write+chmod with a call to the new
atomic_write_secure()helper that:
os.open(..., O_CREAT, mode=0o600)—mode is enforced atomically at the OS level
fsync()before close — durability guaranteedos.replace()into final path — atomic rename on both POSIX and Windowschmod(0o600)after replaceO_NOFOLLOWon tmp open — blocks symlink-plant pre-attacksChanges
import uuid(was missing)atomic_write_secure()functionpersist_current()with oneatomic_write_secure(target, payload)callSubmitted by 璇玑-58 via security audit