Skip to content

fix(cli): enforce owner-only config ACLs on Windows#9764

Open
tianmind-studio wants to merge 1 commit into
BasedHardware:mainfrom
tianmind-studio:codex/windows-cli-private-config
Open

fix(cli): enforce owner-only config ACLs on Windows#9764
tianmind-studio wants to merge 1 commit into
BasedHardware:mainfrom
tianmind-studio:codex/windows-cli-private-config

Conversation

@tianmind-studio

@tianmind-studio tianmind-studio commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • preserve the existing POSIX 0600 path while creating Windows config files with a protected owner-rights DACL
  • pass the DACL to CreateFileW at file creation, deny handle sharing during serialization, and read it back before writing credentials
  • fail closed and remove the temp file if Windows cannot enforce the requested ACL
  • verify both the in-progress temp descriptor and the final config permissions in cross-platform tests

Why

omi-cli stores API keys, OAuth tokens, and local bearer tokens in config.toml, and its documented contract says that file is owner-only. On Windows, Python accepts the 0o600 argument and umask, but the resulting mode is 0o666; those POSIX controls do not establish a Windows ACL.

The new helper uses only the standard library. Its protected DACL grants file-all access to the object owner and permits no inherited ACEs. It also verifies the effective DACL through the open handle before tomli_w writes the first credential byte. Unsupported filesystems or altered descriptors therefore fail without leaving a credential file behind.

Testing

  • before the fix on Windows: test_config.py had 2 failed, 12 passed (0o666 != 0o600)
  • after the fix on Windows: 15 passed
  • remaining unaffected CLI subset: 116 passed, 6 deselected
  • mypy for the new module on Windows: passed
  • mypy with --platform linux: passed
  • Black and isort: passed
  • wheel build: passed and included omi_cli/_secure_file.py
  • git diff --check: passed

The broader Windows CLI baseline also exposes separate pre-existing error-rendering and OpenAPI locale failures; this PR does not change those paths.

Review in cubic

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @tianmind-studio — this is a genuinely well-built hardening for Windows credential files. A few observations for a maintainer before this merges.

What I like

  • The fail-closed approach is exactly right: CreateFileW with CREATE_NEW, share mode 0, a protected owner-only DACL (D:P(A;;FA;;;OW)), then re-verifying the effective DACL through the open handle before any credential byte is written. If verification fails, the temp file is deleted and no credential is persisted. That's the correct shape for closing a TOCTOU window.
  • The regression test that inspects the temp descriptor mid-serialization (by monkeypatching tomli_w.dump) rather than only after-the-fact is the right way to catch the "file is world-readable while bytes are being written" class of bug.
  • POSIX path is untouched, so no regression risk there.

Why I'm flagging for human review rather than approving

  • The security-critical path has no CI coverage. The repo CI is Linux/macOS, and the new Windows assertion is @pytest.mark.skipif(os.name != "nt"). So the actual ACL behavior is validated only on a local Windows run, and a subtle ctypes issue (argtypes wiring, HANDLE(-1) comparison, GetSecurityInfo path, open_osfhandle semantics, memory-freeing order) would pass CI silently. For a credential-permission change I'd want that validated on a Windows host or by a reviewer with Windows sign-off.
  • Owner-only DACL excludes SYSTEM/Administrators. D:P(A;;FA;;;OW) grants only the file owner full access — which is the right default for a credential file, but it's a backup/AV-visibility trade-off worth a maintainer's explicit nod.
  • This touches how secrets-at-rest are protected, so it's a security/permissions surface that needs human judgment before merge rather than an AI approval.

Suggested next steps

  • If feasible, add a Windows leg to CI (or at least confirm the full test suite passes on Windows in the PR). Confirming _secure_file's Win32 calls behave as intended under a real Windows environment is the highest-value check here.
  • Consider a one-line docstring note in _secure_file.py that the DACL deliberately omits SYSTEM/Administrators and why, so future readers understand the trade-off.

Marking this for security/maintainer review. Positive direction overall — just needs a Windows-confirmed human sign-off before merge.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added security-review Touches auth, provider routing, secrets, or security-sensitive surfaces needs-maintainer-review Needs human maintainer review before merge labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-maintainer-review Needs human maintainer review before merge security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants