From feb0bae40ad16a252381bc17d41d4c646f1d57e4 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:57:27 -0700 Subject: [PATCH 1/2] fix(core): serialise record_installation, format the credential-store 500 (#1085) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps #954/#1084 left as out of scope. 1. record_installation did an unlocked read-modify-write #954 made the write atomic, so a crash cannot truncate the file. It does not stop two concurrent installs reading the same base and the second write dropping the first tool. The lock pattern lives in atomic_io now rather than being copied a third time from credentials.py — read_modify_write_lock(lock_path), thread lock plus an optional cross-process filelock, degrading to thread-only with one warning when filelock is absent. atomic_io is where "an atomic write is not a serialised write" belongs, and both callers already import from it. 2. An unreadable credential store gave a bare 500 CredentialManager's constructor runs the machine-wide migration, which can raise CredentialStoreUnreadableError since #954. Raised from a FastAPI *dependency* it bypasses each route's own try/except, so the client got an unformatted 500 instead of the api_error(...) shape every other path produces — and lost the recovery text the CLI already prints. All four dependency functions (both routers define their own get_credential_manager and _readonly) now catch it. The race tests use a threading.Barrier so both threads provably read the same base, rather than hoping for an unlucky interleaving; removing the lock fails both, including the ten-thread case. One of my own tests was passing vacuously: it wrote to "credentials.enc" while the loader reads "credentials.encrypted", so it exercised nothing. It uses the ENCRYPTED_FILE_NAME constant now. Full suite: 6439 passed, 49 skipped. --- codeframe/core/atomic_io.py | 64 ++++++ codeframe/core/installer.py | 60 +++--- .../ui/routers/github_integrations_v2.py | 32 ++- codeframe/ui/routers/settings_v2.py | 27 ++- .../core/test_concurrent_write_guards_1085.py | 196 ++++++++++++++++++ 5 files changed, 347 insertions(+), 32 deletions(-) create mode 100644 tests/core/test_concurrent_write_guards_1085.py diff --git a/codeframe/core/atomic_io.py b/codeframe/core/atomic_io.py index 382c95f5..2a595039 100644 --- a/codeframe/core/atomic_io.py +++ b/codeframe/core/atomic_io.py @@ -24,11 +24,21 @@ """ import json +import logging import os import tempfile +import threading +from contextlib import contextmanager from pathlib import Path from typing import Any, Union +try: # optional, exactly as credentials.py treats it + from filelock import FileLock +except ImportError: # pragma: no cover - exercised by the no-filelock path + FileLock = None # type: ignore[assignment] + +logger = logging.getLogger(__name__) + __all__ = [ "atomic_write_bytes", "atomic_write_text", @@ -148,3 +158,57 @@ def atomic_write_text( def atomic_write_json(path: Union[str, Path], payload: Any, mode: int | None = None) -> None: """Durably replace ``path`` with ``payload`` serialized as indented JSON.""" atomic_write_text(path, json.dumps(payload, indent=2), mode=mode) + + +# --------------------------------------------------------------------------- +# Serialising a read-modify-write +# --------------------------------------------------------------------------- +# +# An atomic write stops a crash from truncating the file. It does NOT stop two +# writers from losing each other's work: both read the same base, both write a +# whole new file, and the second one wins (#1085). Anything doing +# read-modify-write on a shared file needs this as well as atomic_write_*. +# +# `filelock` is optional. Without it this degrades to thread-only +# serialisation, which is honest for a single-process CLI and logged once so +# the weaker guarantee is not silent. + +_THREAD_LOCKS: dict[Path, threading.Lock] = {} +_FILELOCK_WARNED = False + + +def read_modify_write_lock(lock_path: Union[str, Path]): + """Serialise a read-modify-write keyed on ``lock_path``. + + Returns a context manager holding a process-wide thread lock and, when + ``filelock`` is installed, a cross-process file lock on the same path. + + ``lock_path`` is the lock file itself, not the data file — callers pass + something like ``dir / ".history.lock"``. + """ + return _ReadModifyWriteLock(Path(lock_path)) + + +@contextmanager +def _ReadModifyWriteLock(lock_path: Path): # noqa: N802 - reads as a class at call sites + global _FILELOCK_WARNED + + lock_path.parent.mkdir(parents=True, exist_ok=True) + thread_lock = _THREAD_LOCKS.setdefault(lock_path, threading.Lock()) + + file_lock = None + if FileLock is not None: + file_lock = FileLock(str(lock_path)) + elif not _FILELOCK_WARNED: + _FILELOCK_WARNED = True + logger.warning( + "filelock is not installed — read-modify-write is serialised within " + "this process only. Concurrent processes can still lose an entry." + ) + + with thread_lock: + if file_lock is None: + yield + else: + with file_lock: + yield diff --git a/codeframe/core/installer.py b/codeframe/core/installer.py index e4bbcbfc..dd819300 100644 --- a/codeframe/core/installer.py +++ b/codeframe/core/installer.py @@ -27,7 +27,7 @@ from pathlib import Path from typing import Optional -from codeframe.core.atomic_io import atomic_write_json +from codeframe.core.atomic_io import atomic_write_json, read_modify_write_lock logger = logging.getLogger(__name__) @@ -614,32 +614,38 @@ def record_installation(self, result: InstallResult) -> None: """ self.history_dir.mkdir(parents=True, exist_ok=True) - # Load existing history. The file is on disk and can be anything — - # hand-edited, truncated, or written by an older version — and this runs - # AFTER a successful install, so a bad shape here must not turn a working - # install into a crash (#954). Anything that is not the expected - # dict-of-dicts is treated as "no usable history" rather than indexed - # into (a list or a string raised TypeError). - installations: dict = {} - if self.history_file.exists(): - try: - with open(self.history_file, encoding="utf-8") as f: - loaded = json.load(f) - except (json.JSONDecodeError, OSError, UnicodeDecodeError): - loaded = None - if isinstance(loaded, dict) and isinstance(loaded.get("installations"), dict): - installations = loaded["installations"] - - installations[result.tool_name] = { - "status": result.status.value, - "installed_at": datetime.now(UTC).isoformat(), - "command": result.command_used, - "message": result.message, - } - - # Atomic: an in-place rewrite truncated the history first, so a crash - # mid-write lost every previously recorded install (#954). - atomic_write_json(self.history_file, {"installations": installations}) + # Serialised, not just atomic (#1085). atomic_write_json stops a crash + # truncating the file; it does not stop two concurrent installs reading + # the same base and the second write dropping the first tool. Matters + # once `cf env install` runs tools in parallel. + with read_modify_write_lock(self.history_dir / ".history.lock"): + + # Load existing history. The file is on disk and can be anything — + # hand-edited, truncated, or written by an older version — and this runs + # AFTER a successful install, so a bad shape here must not turn a working + # install into a crash (#954). Anything that is not the expected + # dict-of-dicts is treated as "no usable history" rather than indexed + # into (a list or a string raised TypeError). + installations: dict = {} + if self.history_file.exists(): + try: + with open(self.history_file, encoding="utf-8") as f: + loaded = json.load(f) + except (json.JSONDecodeError, OSError, UnicodeDecodeError): + loaded = None + if isinstance(loaded, dict) and isinstance(loaded.get("installations"), dict): + installations = loaded["installations"] + + installations[result.tool_name] = { + "status": result.status.value, + "installed_at": datetime.now(UTC).isoformat(), + "command": result.command_used, + "message": result.message, + } + + # Atomic: an in-place rewrite truncated the history first, so a crash + # mid-write lost every previously recorded install (#954). + atomic_write_json(self.history_file, {"installations": installations}) def get_installation_history(self) -> dict: """Get the installation history. diff --git a/codeframe/ui/routers/github_integrations_v2.py b/codeframe/ui/routers/github_integrations_v2.py index 6b796237..9bd9aa4d 100644 --- a/codeframe/ui/routers/github_integrations_v2.py +++ b/codeframe/ui/routers/github_integrations_v2.py @@ -27,7 +27,11 @@ from codeframe.auth.api_keys import SCOPE_ADMIN from codeframe.auth.dependencies import require_auth, require_scope -from codeframe.core.credentials import CredentialManager, CredentialProvider +from codeframe.core.credentials import ( + CredentialManager, + CredentialProvider, + CredentialStoreUnreadableError, +) from codeframe.core.github_connect_service import ( GitHubConnectError, InsufficientScopeError, @@ -67,7 +71,18 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana store. Overridden in tests to point at an isolated temp directory. Runs the machine-wide migration — use only on write paths. """ - return CredentialManager(user_id=auth.get("user_id"), migrate=True) + # CredentialManager's constructor runs the machine-wide migration, which + # can raise CredentialStoreUnreadableError since #954. Raised from a + # DEPENDENCY it bypasses each route's own try/except, so the client got a + # bare 500 instead of the formatted error every other path produces (#1085). + # The exception's message carries the recovery text the CLI already prints. + try: + return CredentialManager(user_id=auth.get("user_id"), migrate=True) + except CredentialStoreUnreadableError as e: + raise HTTPException( + status_code=500, + detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + ) def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager: @@ -76,7 +91,18 @@ def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> Crede Used on GET endpoints so that a plain status check cannot trigger a credential write into a new tenant's store (#790). """ - return CredentialManager(user_id=auth.get("user_id"), migrate=False) + # CredentialManager's constructor runs the machine-wide migration, which + # can raise CredentialStoreUnreadableError since #954. Raised from a + # DEPENDENCY it bypasses each route's own try/except, so the client got a + # bare 500 instead of the formatted error every other path produces (#1085). + # The exception's message carries the recovery text the CLI already prints. + try: + return CredentialManager(user_id=auth.get("user_id"), migrate=False) + except CredentialStoreUnreadableError as e: + raise HTTPException( + status_code=500, + detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + ) class ConnectRequest(BaseModel): diff --git a/codeframe/ui/routers/settings_v2.py b/codeframe/ui/routers/settings_v2.py index a89e2b08..92b6f165 100644 --- a/codeframe/ui/routers/settings_v2.py +++ b/codeframe/ui/routers/settings_v2.py @@ -40,6 +40,7 @@ save_environment_config, ) from codeframe.core.credentials import ( + CredentialStoreUnreadableError, CredentialManager, CredentialProvider, CredentialSource, @@ -87,7 +88,18 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana store. Overridden in tests to point at an isolated temp directory. Runs the machine-wide migration — use only on write (admin-scoped) paths. """ - return CredentialManager(user_id=auth.get("user_id"), migrate=True) + # CredentialManager's constructor runs the machine-wide migration, which + # can raise CredentialStoreUnreadableError since #954. Raised from a + # DEPENDENCY it bypasses each route's own try/except, so the client got a + # bare 500 instead of the formatted error every other path produces (#1085). + # The exception's message carries the recovery text the CLI already prints. + try: + return CredentialManager(user_id=auth.get("user_id"), migrate=True) + except CredentialStoreUnreadableError as e: + raise HTTPException( + status_code=500, + detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + ) def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager: @@ -96,7 +108,18 @@ def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> Crede Used on GET endpoints so that a plain status check cannot trigger a credential write into a new tenant's store (#790). """ - return CredentialManager(user_id=auth.get("user_id"), migrate=False) + # CredentialManager's constructor runs the machine-wide migration, which + # can raise CredentialStoreUnreadableError since #954. Raised from a + # DEPENDENCY it bypasses each route's own try/except, so the client got a + # bare 500 instead of the formatted error every other path produces (#1085). + # The exception's message carries the recovery text the CLI already prints. + try: + return CredentialManager(user_id=auth.get("user_id"), migrate=False) + except CredentialStoreUnreadableError as e: + raise HTTPException( + status_code=500, + detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + ) def _config_to_response(config: EnvironmentConfig) -> AgentSettingsResponse: diff --git a/tests/core/test_concurrent_write_guards_1085.py b/tests/core/test_concurrent_write_guards_1085.py new file mode 100644 index 00000000..88fbdfbe --- /dev/null +++ b/tests/core/test_concurrent_write_guards_1085.py @@ -0,0 +1,196 @@ +"""#1085 — atomic is not the same as serialised. + +Two gaps left out of #954 as out of scope: + +1. `record_installation` did an unlocked read-modify-write. #954 made the write + atomic, so a crash cannot truncate the file — but two concurrent installs + still read the same base and the second write drops the first tool. + +2. `get_credential_manager` runs the machine-wide migration in a FastAPI + *dependency*, so a `CredentialStoreUnreadableError` bypassed each route's own + try/except and reached the client as a bare 500 rather than the formatted + `api_error(...)` every other path produces. +""" + +import json +import threading +from unittest.mock import patch + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from codeframe.core.atomic_io import read_modify_write_lock +from codeframe.core.credentials import CredentialStoreUnreadableError +from codeframe.core.installer import InstallResult, InstallStatus, ToolInstaller + +pytestmark = pytest.mark.v2 + + +class TestRecordInstallationIsSerialised: + """AC: a two-thread test records both tools with neither lost.""" + + def _installer(self, tmp_path): + installer = ToolInstaller() + installer.history_dir = tmp_path + installer.history_file = tmp_path / "environment.json" + return installer + + def _result(self, name: str) -> InstallResult: + return InstallResult( + tool_name=name, + status=InstallStatus.SUCCESS, + message="ok", + command_used=f"install {name}", + ) + + def test_two_concurrent_installs_both_survive(self, tmp_path): + installer = self._installer(tmp_path) + + # A barrier makes both threads read the same base before either writes, + # which is exactly the interleaving that loses an entry. Without the + # lock this is not a race that "might" happen — it is the common case. + both_ready = threading.Barrier(2, timeout=10) + errors: list = [] + + def record(name: str): + try: + both_ready.wait() + installer.record_installation(self._result(name)) + except Exception as exc: # surfaced below rather than swallowed + errors.append(exc) + + threads = [ + threading.Thread(target=record, args=(name,)) + for name in ("ruff", "mypy") + ] + for t in threads: + t.start() + for t in threads: + t.join(timeout=15) + + assert errors == [], errors + recorded = json.loads(installer.history_file.read_text())["installations"] + assert set(recorded) == {"ruff", "mypy"}, recorded + + def test_many_concurrent_installs_all_survive(self, tmp_path): + """The two-thread case can pass by luck; ten is a harder target.""" + installer = self._installer(tmp_path) + names = [f"tool-{i}" for i in range(10)] + ready = threading.Barrier(len(names), timeout=15) + + def record(name: str): + ready.wait() + installer.record_installation(self._result(name)) + + threads = [threading.Thread(target=record, args=(n,)) for n in names] + for t in threads: + t.start() + for t in threads: + t.join(timeout=20) + + recorded = json.loads(installer.history_file.read_text())["installations"] + assert set(recorded) == set(names), sorted(set(names) - set(recorded)) + + def test_the_history_file_stays_valid_json(self, tmp_path): + """A half-written file would be worse than a lost entry.""" + installer = self._installer(tmp_path) + installer.record_installation(self._result("ruff")) + assert json.loads(installer.history_file.read_text())["installations"] + + +class TestTheLockItself: + def test_it_serialises_a_read_modify_write(self, tmp_path): + """The guard is real, not a no-op context manager.""" + counter = {"value": 0} + lock_path = tmp_path / ".test.lock" + + def increment(): + for _ in range(200): + with read_modify_write_lock(lock_path): + current = counter["value"] + counter["value"] = current + 1 + + threads = [threading.Thread(target=increment) for _ in range(4)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=20) + + assert counter["value"] == 800 + + def test_it_works_without_filelock_installed(self, tmp_path, monkeypatch): + """Degrades to thread-only serialisation rather than failing.""" + import codeframe.core.atomic_io as atomic_io + + monkeypatch.setattr(atomic_io, "FileLock", None) + with read_modify_write_lock(tmp_path / ".nofilelock.lock"): + pass # must not raise + + +class TestAnUnreadableStoreIsAFormattedError: + """AC: the v2 routers return api_error(...), not a bare 500.""" + + @pytest.fixture + def client(self): + from codeframe.auth.dependencies import require_auth + from codeframe.ui.routers import settings_v2 + + app = FastAPI() + app.include_router(settings_v2.router) + app.dependency_overrides[require_auth] = lambda: {"user_id": None} + return TestClient(app, raise_server_exceptions=False) + + def test_the_response_body_has_the_standard_shape(self, client): + with patch( + "codeframe.ui.routers.settings_v2.CredentialManager", + side_effect=CredentialStoreUnreadableError( + "credential store unreadable; re-enter with `cf auth setup`" + ), + ): + res = client.get("/api/v2/settings/keys") + + assert res.status_code == 500 + body = res.json() + # The formatted shape every other route produces, not a bare string. + assert "detail" in body + assert isinstance(body["detail"], dict), body + assert "code" in body["detail"], body["detail"] + + def test_the_recovery_text_reaches_the_client(self, client): + """The CLI already prints this; the HTTP surface dropped it.""" + with patch( + "codeframe.ui.routers.settings_v2.CredentialManager", + side_effect=CredentialStoreUnreadableError( + "credential store unreadable; re-enter with `cf auth setup`" + ), + ): + res = client.get("/api/v2/settings/keys") + + assert "cf auth setup" in json.dumps(res.json()) + + +class TestAnUnreadableStoreIsNeverOverwritten: + """AC: assert #954's guarantee still holds.""" + + def test_store_raises_rather_than_clobbering(self, tmp_path, monkeypatch): + from codeframe.core.credentials import CredentialStore + + monkeypatch.setenv("HOME", str(tmp_path)) + store = CredentialStore(storage_dir=tmp_path / "creds") + store.storage_dir.mkdir(parents=True, exist_ok=True) + from codeframe.core.credentials import ENCRYPTED_FILE_NAME + + # The real constant, not a guessed filename — my first version + # wrote to a path the loader never reads, so it "passed" by + # never exercising anything. + encrypted = store.storage_dir / ENCRYPTED_FILE_NAME + encrypted.write_bytes(b"not decryptable by any key") + before = encrypted.read_bytes() + + with pytest.raises(CredentialStoreUnreadableError): + store._load_encrypted_store() + + # The ciphertext is still there — recoverable if the key material + # comes back. Treating unreadable as empty would have erased it. + assert encrypted.read_bytes() == before From 0b4ed2b8981c0b64ea38ae3060da9e3fd981e84b Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:14:45 -0700 Subject: [PATCH 2/2] fix(api): do not render the credential-store path into the response (#1085) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security finding from review, and a disclosure this PR introduced. The bare 500 it replaced leaked nothing; api_error(str(e), ...) leaked the absolute store path — /home//.codeframe/users//credentials.encrypted — to any authenticated tenant hitting GET /api/v2/settings/keys, /connect, /status or /issues. That is the operator's home directory and the per-tenant storage layout. All four sites now use #934's internal_error(): the full message goes to the operator's log under a correlation id, and the client gets the id plus the one actionable step (`cf auth setup`), which contains no path. Formatted, without being informative to the wrong audience. My first verification of this was wrong and worth recording: I reverted ONE of the two sites in settings_v2.py, saw the leak tests pass, and could have read that as confirmation. GET /keys uses the readonly dependency and I had reverted the write one — so the test never reached the reverted code. Reverting both fails both leak tests, which is the actual evidence. Four tests now: the standard shape, no path in the body, a correlation id that appears in the detail, and the recovery step still reaching the client. Full suite: 6441 passed, 49 skipped. --- .../ui/routers/github_integrations_v2.py | 32 ++++++++--- codeframe/ui/routers/settings_v2.py | 32 ++++++++--- .../core/test_concurrent_write_guards_1085.py | 53 ++++++++++++------- 3 files changed, 85 insertions(+), 32 deletions(-) diff --git a/codeframe/ui/routers/github_integrations_v2.py b/codeframe/ui/routers/github_integrations_v2.py index 9bd9aa4d..db968830 100644 --- a/codeframe/ui/routers/github_integrations_v2.py +++ b/codeframe/ui/routers/github_integrations_v2.py @@ -57,7 +57,7 @@ from codeframe.core.workspace import Workspace from codeframe.lib.rate_limiter import rate_limit_ai, rate_limit_standard from codeframe.ui.dependencies import get_v2_workspace, resolve_github_pat -from codeframe.ui.response_models import ErrorCodes, api_error +from codeframe.ui.response_models import ErrorCodes, api_error, internal_error logger = logging.getLogger(__name__) @@ -79,10 +79,19 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana try: return CredentialManager(user_id=auth.get("user_id"), migrate=True) except CredentialStoreUnreadableError as e: - raise HTTPException( - status_code=500, - detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + # internal_error, NOT str(e) (#934): the exception message embeds the + # absolute store path — /home//.codeframe/users//... — + # so rendering it would hand an authenticated tenant the operator's + # home directory and the per-tenant storage layout. The full message + # goes to the operator's log under the correlation id; the client gets + # the recovery step, which is the part that is actually actionable and + # contains no path. + body = internal_error(e, operation="read the credential store", logger=logger) + body["detail"] += ( + " The credential store could not be read; re-enter your keys with " + "`cf auth setup`." ) + raise HTTPException(status_code=500, detail=body) def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager: @@ -99,10 +108,19 @@ def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> Crede try: return CredentialManager(user_id=auth.get("user_id"), migrate=False) except CredentialStoreUnreadableError as e: - raise HTTPException( - status_code=500, - detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + # internal_error, NOT str(e) (#934): the exception message embeds the + # absolute store path — /home//.codeframe/users//... — + # so rendering it would hand an authenticated tenant the operator's + # home directory and the per-tenant storage layout. The full message + # goes to the operator's log under the correlation id; the client gets + # the recovery step, which is the part that is actually actionable and + # contains no path. + body = internal_error(e, operation="read the credential store", logger=logger) + body["detail"] += ( + " The credential store could not be read; re-enter your keys with " + "`cf auth setup`." ) + raise HTTPException(status_code=500, detail=body) class ConnectRequest(BaseModel): diff --git a/codeframe/ui/routers/settings_v2.py b/codeframe/ui/routers/settings_v2.py index 92b6f165..e0692e77 100644 --- a/codeframe/ui/routers/settings_v2.py +++ b/codeframe/ui/routers/settings_v2.py @@ -73,7 +73,7 @@ VerifyKeyRequest, VerifyKeyResponse, ) -from codeframe.ui.response_models import ErrorCodes, api_error +from codeframe.ui.response_models import ErrorCodes, api_error, internal_error from codeframe.core.notifications_config import redact_webhook_url logger = logging.getLogger(__name__) @@ -96,10 +96,19 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana try: return CredentialManager(user_id=auth.get("user_id"), migrate=True) except CredentialStoreUnreadableError as e: - raise HTTPException( - status_code=500, - detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + # internal_error, NOT str(e) (#934): the exception message embeds the + # absolute store path — /home//.codeframe/users//... — + # so rendering it would hand an authenticated tenant the operator's + # home directory and the per-tenant storage layout. The full message + # goes to the operator's log under the correlation id; the client gets + # the recovery step, which is the part that is actually actionable and + # contains no path. + body = internal_error(e, operation="read the credential store", logger=logger) + body["detail"] += ( + " The credential store could not be read; re-enter your keys with " + "`cf auth setup`." ) + raise HTTPException(status_code=500, detail=body) def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager: @@ -116,10 +125,19 @@ def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> Crede try: return CredentialManager(user_id=auth.get("user_id"), migrate=False) except CredentialStoreUnreadableError as e: - raise HTTPException( - status_code=500, - detail=api_error(str(e), ErrorCodes.INTERNAL_ERROR), + # internal_error, NOT str(e) (#934): the exception message embeds the + # absolute store path — /home//.codeframe/users//... — + # so rendering it would hand an authenticated tenant the operator's + # home directory and the per-tenant storage layout. The full message + # goes to the operator's log under the correlation id; the client gets + # the recovery step, which is the part that is actually actionable and + # contains no path. + body = internal_error(e, operation="read the credential store", logger=logger) + body["detail"] += ( + " The credential store could not be read; re-enter your keys with " + "`cf auth setup`." ) + raise HTTPException(status_code=500, detail=body) def _config_to_response(config: EnvironmentConfig) -> AgentSettingsResponse: diff --git a/tests/core/test_concurrent_write_guards_1085.py b/tests/core/test_concurrent_write_guards_1085.py index 88fbdfbe..6c15afcb 100644 --- a/tests/core/test_concurrent_write_guards_1085.py +++ b/tests/core/test_concurrent_write_guards_1085.py @@ -129,7 +129,19 @@ def test_it_works_without_filelock_installed(self, tmp_path, monkeypatch): class TestAnUnreadableStoreIsAFormattedError: - """AC: the v2 routers return api_error(...), not a bare 500.""" + """AC: the v2 routers return the standard error shape, not a bare 500. + + Formatted, but NOT by rendering str(e). The exception message embeds the + absolute store path (/home//.codeframe/users//...), so + echoing it would hand an authenticated tenant the operator's home directory + and the per-tenant layout — a disclosure the bare 500 did not have. #934's + internal_error() correlation-id pattern exists for exactly this. + """ + + LEAKY_MESSAGE = ( + "Cannot read /home/operator/.codeframe/users/5/credentials.encrypted; " + "re-enter with `cf auth setup`" + ) @pytest.fixture def client(self): @@ -141,33 +153,38 @@ def client(self): app.dependency_overrides[require_auth] = lambda: {"user_id": None} return TestClient(app, raise_server_exceptions=False) - def test_the_response_body_has_the_standard_shape(self, client): + def _response(self, client): with patch( "codeframe.ui.routers.settings_v2.CredentialManager", - side_effect=CredentialStoreUnreadableError( - "credential store unreadable; re-enter with `cf auth setup`" - ), + side_effect=CredentialStoreUnreadableError(self.LEAKY_MESSAGE), ): - res = client.get("/api/v2/settings/keys") + return client.get("/api/v2/settings/keys") + + def test_the_response_body_has_the_standard_shape(self, client): + res = self._response(client) assert res.status_code == 500 body = res.json() - # The formatted shape every other route produces, not a bare string. - assert "detail" in body assert isinstance(body["detail"], dict), body assert "code" in body["detail"], body["detail"] - def test_the_recovery_text_reaches_the_client(self, client): - """The CLI already prints this; the HTTP surface dropped it.""" - with patch( - "codeframe.ui.routers.settings_v2.CredentialManager", - side_effect=CredentialStoreUnreadableError( - "credential store unreadable; re-enter with `cf auth setup`" - ), - ): - res = client.get("/api/v2/settings/keys") + def test_it_does_not_leak_the_store_path(self, client): + """The bare 500 leaked nothing; a formatted error must not do worse.""" + rendered = json.dumps(self._response(client).json()) + + assert "/home/operator" not in rendered + assert "credentials.encrypted" not in rendered + assert "users/5" not in rendered + + def test_it_carries_a_correlation_id(self, client): + """The id is what ties a user's report to the full traceback in the log.""" + body = self._response(client).json()["detail"] + assert body.get("correlation_id") + assert body["correlation_id"] in body["detail"] - assert "cf auth setup" in json.dumps(res.json()) + def test_the_actionable_recovery_step_still_reaches_the_client(self, client): + """The path is the secret; `cf auth setup` is the useful part.""" + assert "cf auth setup" in json.dumps(self._response(client).json()) class TestAnUnreadableStoreIsNeverOverwritten: