Skip to content

fix(config): atomic write + 0o600 enforcement in persist_current #674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hernandez42 wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −3
Open

fix(config): atomic write + 0o600 enforcement in persist_current #674

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions strix/config/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import json
import logging
import os
import uuid
from pathlib import Path
from typing import TYPE_CHECKING, Any

Expand Down Expand Up @@ -71,9 +72,8 @@ def persist_current() -> None:
env_block[alias.upper()] = value
break

target.write_text(json.dumps({"env": env_block}, indent=2), encoding="utf-8")
with contextlib.suppress(OSError):
target.chmod(0o600)
payload = json.dumps({"env": env_block}, indent=2)
atomic_write_secure(target, payload)


def _aliases_for(finfo: FieldInfo) -> list[str]:
Expand All @@ -88,6 +88,41 @@ def _aliases_for(finfo: FieldInfo) -> list[str]:
aliases.append(va)
return aliases

def atomic_write_secure(target: "Path", payload: str) -> None:
"""Atomically write *payload* to *target* with mode 0o600.

Guarantees:
1. Sensitive content is NEVER observable with mode != 0o600.
2. A crash leaves either the old file or the new file, never a
half-written file.
3. Concurrent readers see either old or new contents, never torn reads.
4. O_NOFOLLOW defends against a pre-planted symlink at the tmp path.
"""
target.parent.mkdir(parents=True, exist_ok=True)
tmp_path = target.with_name(f"{target.name}.{os.getpid()}.{uuid.uuid4().hex[:8]}.tmp")
try:
fd = os.open(
tmp_path,
flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW,

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.

mode=0o600,
)
try:
with os.fdopen(fd, "w", encoding="utf-8") as f:
f.write(payload)
f.flush()
os.fsync(f.fileno())
except BaseException:
# os.fdopen takes ownership; suppress in case it failed before taking fd
with contextlib.suppress(OSError):
os.close(fd)
raise
os.replace(tmp_path, target)

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

# Belt-and-suspenders: enforce mode in case os.replace inherited anything odd
target.chmod(0o600)

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.

finally:
with contextlib.suppress(OSError):
tmp_path.unlink()


def _read_json_overrides(path: Path) -> dict[str, dict[str, Any]]:
"""Read ``{"env": {...}}`` from ``path`` and remap to nested kwargs.
Expand Down