From 444deaa21df5ff1e088ef30e1d651671e11ddc85 Mon Sep 17 00:00:00 2001 From: Dhevenddra Date: Mon, 27 Jul 2026 20:13:42 +0530 Subject: [PATCH] security: apply the Windows ACL protection to the workspace trust store WorkspaceTrustStore.set_trusted protected its state file with a bare os.chmod(tmp, 0o600). On Windows that is a silent no-op, because os.chmod only toggles the read-only bit there, so workspace_trust.json kept the broad ACEs it inherited from its parent directory: NT AUTHORITY\SYSTEM:(I)(F) BUILTIN\Administrators:(I)(F) :(I)(F) coworker/secrets.py already documents this hazard and solves it in _restrict_to_user() via icacls /inheritance:r /grant:r, exposed as the write_private_text() helper. server/run.py uses that helper for the sidecar auth token, but the trust store did not. Since this file is the allowlist deciding which workspaces get auto-approved command execution, route it through the same helper. Afterwards the file carries a single ACE: :(F) The two tests meant to catch this asserted st_mode & 0o777 == 0o600, which is unsatisfiable on Windows. os.stat cannot see ACLs there, so every writable file reports 0o666, including the correctly protected sidecar token. Replace both with a shared assert_user_only() that checks mode bits on POSIX and the DACL on Windows. --- coworker/workspace_trust.py | 17 +++++++++-------- tests/conftest.py | 26 ++++++++++++++++++++++++++ tests/test_config.py | 4 +++- tests/test_server.py | 3 ++- 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/coworker/workspace_trust.py b/coworker/workspace_trust.py index c749d654..155898ca 100644 --- a/coworker/workspace_trust.py +++ b/coworker/workspace_trust.py @@ -9,11 +9,10 @@ from __future__ import annotations import json -import os from pathlib import Path from typing import Optional -from .secrets import state_dir +from .secrets import state_dir, write_private_text class WorkspaceTrustStore: @@ -51,12 +50,14 @@ def set_trusted(self, workspace: str | Path, trusted: bool) -> str: values.add(canonical) else: values.discard(canonical) - self.path.parent.mkdir(parents=True, exist_ok=True) - tmp = self.path.with_name(f".{self.path.name}.{os.getpid()}.tmp") - tmp.write_text( + # `write_private_text` owns the atomic temp-write plus the platform-correct + # restriction. A bare `os.chmod(0o600)` here was a silent no-op on Windows, + # where os.chmod only toggles the read-only bit, leaving this file with the + # inherited SYSTEM/Administrators ACEs its parent carries. Since this file is + # the allowlist governing auto-approved command execution, it needs the same + # protection the SecretStore already applies to the sidecar token. + write_private_text( + self.path, json.dumps({"trusted_workspaces": sorted(values)}, indent=2) + "\n", - encoding="utf-8", ) - os.chmod(tmp, 0o600) - tmp.replace(self.path) return canonical diff --git a/tests/conftest.py b/tests/conftest.py index 61abb579..6d86496f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,12 +8,38 @@ from __future__ import annotations +import subprocess +import sys +from pathlib import Path + import pytest import pytest_asyncio from coworker.testing.fake_slack import FakeSlack +def assert_user_only(path: Path) -> None: + """Assert `path` is readable by the current user alone, per the platform's own model. + + POSIX says this with mode bits. Windows has no such bits: `os.stat` derives `st_mode` + from the read-only attribute only, so every writable file reports 0o666 and a + `st_mode & 0o777 == 0o600` check can never pass there, not even for a file that + `secrets._restrict_to_user` has correctly locked down with `icacls`. Assert the ACL + instead, mirroring how the protection is applied in the first place. + """ + if sys.platform == "win32": + acl = subprocess.run( + ["icacls", str(path)], capture_output=True, text=True + ).stdout + # `/inheritance:r` strips inherited ACEs; their absence is what proves the file + # was restricted rather than left to inherit SYSTEM/Administrators from its parent. + assert "(I)" not in acl, f"{path} still carries inherited ACEs:\n{acl}" + for principal in ("NT AUTHORITY\\SYSTEM", "BUILTIN\\Administrators"): + assert principal not in acl, f"{path} grants {principal}:\n{acl}" + else: + assert (path.stat().st_mode & 0o777) == 0o600 + + @pytest.fixture(autouse=True) def _isolated_state_dir(tmp_path, monkeypatch): """EVERY test gets an isolated SecretStore/state dir. Without this, any test that builds diff --git a/tests/test_config.py b/tests/test_config.py index 22915ba6..ac1da7e6 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -4,6 +4,8 @@ from pathlib import Path +from conftest import assert_user_only + from coworker.config import load_config @@ -77,7 +79,7 @@ def test_workspace_trust_is_canonical_and_user_owned(tmp_path): assert canonical == str(real.resolve()) assert store.is_trusted(real) assert store.list() == [str(real.resolve())] - assert (store.path.stat().st_mode & 0o777) == 0o600 + assert_user_only(store.path) store.set_trusted(real, False) assert not store.is_trusted(alias) diff --git a/tests/test_server.py b/tests/test_server.py index af870c51..467f3a61 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -3,6 +3,7 @@ from __future__ import annotations import pytest +from conftest import assert_user_only from fastapi.testclient import TestClient from coworker.providers import ( @@ -477,7 +478,7 @@ def test_standalone_server_token_file_is_user_only(tmp_path, monkeypatch): assert path == tmp_path / "coworker-state" / "sidecar-9876.token" assert path.read_text().strip() == os.environ["COWORKER_API_TOKEN"] assert len(path.read_text().strip()) == 64 - assert (path.stat().st_mode & 0o777) == 0o600 + assert_user_only(path) finally: path.unlink(missing_ok=True) os.environ.pop("COWORKER_API_TOKEN", None)