From aaf1e60abea8d83e0d46d073291cee846812fa57 Mon Sep 17 00:00:00 2001 From: codex Date: Mon, 27 Jul 2026 05:33:44 +0000 Subject: [PATCH] fix: close consolidated quality review findings --- bot_bottle/orchestrator/api.py | 14 +++++++++ bot_bottle/store/db_store.py | 41 ++++++++++++++++++-------- tests/unit/test_db_store.py | 16 +++++++++- tests/unit/test_git_http_backend.py | 1 + tests/unit/test_orchestrator_server.py | 9 ------ tests/unit/test_supervise_edge.py | 4 +-- 6 files changed, 61 insertions(+), 24 deletions(-) diff --git a/bot_bottle/orchestrator/api.py b/bot_bottle/orchestrator/api.py index f2aefa05..04576bf8 100644 --- a/bot_bottle/orchestrator/api.py +++ b/bot_bottle/orchestrator/api.py @@ -7,6 +7,7 @@ import asyncio import math import sys from fastapi import FastAPI, HTTPException +from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse from pydantic import BaseModel, ConfigDict, StrictStr from starlette.types import ASGIApp, Message, Receive, Scope, Send @@ -208,6 +209,19 @@ def create_app(orch: OrchestratorCore, *, signing_key: str) -> FastAPI: ) app.add_middleware(ControlPlaneBoundary, signing_key=key) + @app.exception_handler(RequestValidationError) + async def invalid_request( + _request: object, exc: RequestValidationError, + ) -> JSONResponse: + errors = exc.errors() + location = errors[0].get("loc", ()) if errors else () + field = str(location[1]) if len(location) > 1 else "" + suffix = f": {field}" if field else "" + return JSONResponse( + {"error": f"invalid request body{suffix}"}, + status_code=400, + ) + @app.get("/health") def health() -> dict[str, str]: return {"status": "ok"} diff --git a/bot_bottle/store/db_store.py b/bot_bottle/store/db_store.py index a41bd74a..56374e20 100644 --- a/bot_bottle/store/db_store.py +++ b/bot_bottle/store/db_store.py @@ -29,8 +29,13 @@ class DbStore: """Create and verify the private parent directory.""" parent = self.db_path.parent parent.mkdir(mode=0o700, parents=True, exist_ok=True) + if parent.is_symlink(): + raise PermissionError(f"database directory must not be a symlink: {parent}") parent.chmod(0o700) - if stat.S_IMODE(parent.stat().st_mode) != 0o700: + parent_stat = parent.lstat() + if not stat.S_ISDIR(parent_stat.st_mode): + raise PermissionError(f"database parent is not a directory: {parent}") + if stat.S_IMODE(parent_stat.st_mode) != 0o700: raise PermissionError(f"database directory is not mode 0700: {parent}") def _secure_db_file(self) -> None: @@ -40,22 +45,34 @@ class DbStore: This store contains control-plane identity tokens, so both creation and repair are fail-closed rather than best-effort. """ + flags = os.O_RDWR | os.O_CREAT | os.O_NOFOLLOW + fd = os.open( + self.db_path, + flags, + stat.S_IRUSR | stat.S_IWUSR, + ) try: - fd = os.open( - self.db_path, - os.O_WRONLY | os.O_CREAT | os.O_EXCL, - stat.S_IRUSR | stat.S_IWUSR, - ) - except FileExistsError: - pass - else: + self._secure_open_file(fd) + finally: os.close(fd) - self._chmod() def _chmod(self) -> None: """Enforce and verify the private database mode after every write.""" - self.db_path.chmod(0o600) - if stat.S_IMODE(self.db_path.stat().st_mode) != 0o600: + fd = os.open(self.db_path, os.O_RDWR | os.O_NOFOLLOW) + try: + self._secure_open_file(fd) + finally: + os.close(fd) + + def _secure_open_file(self, fd: int) -> None: + """Pin, validate, and secure an opened database filesystem object.""" + file_stat = os.fstat(fd) + if not stat.S_ISREG(file_stat.st_mode): + raise PermissionError( + f"database must be a regular file: {self.db_path}" + ) + os.fchmod(fd, stat.S_IRUSR | stat.S_IWUSR) + if stat.S_IMODE(os.fstat(fd).st_mode) != 0o600: raise PermissionError(f"database is not mode 0600: {self.db_path}") def _connect(self) -> sqlite3.Connection: diff --git a/tests/unit/test_db_store.py b/tests/unit/test_db_store.py index b421d8a1..d6c86b06 100644 --- a/tests/unit/test_db_store.py +++ b/tests/unit/test_db_store.py @@ -48,10 +48,24 @@ class TestDbStoreIsMigrated(unittest.TestCase): with tempfile.TemporaryDirectory() as d: parent = Path(d) / "store" parent.mkdir() - with patch.object(Path, "chmod", side_effect=OSError("denied")): + (parent / "test.db").touch() + with patch("os.fchmod", side_effect=OSError("denied")): with self.assertRaisesRegex(OSError, "denied"): _store(parent) + def test_rejects_database_symlink_without_changing_target(self): + with tempfile.TemporaryDirectory() as d: + parent = Path(d) / "store" + parent.mkdir() + target = Path(d) / "target" + target.touch(mode=0o644) + (parent / "test.db").symlink_to(target) + + with self.assertRaises(OSError): + _store(parent) + + self.assertEqual(0o644, stat.S_IMODE(target.stat().st_mode)) + def test_returns_false_when_schema_versions_missing(self): # DB file exists but has no schema_versions table → OperationalError → False. with tempfile.TemporaryDirectory() as d: diff --git a/tests/unit/test_git_http_backend.py b/tests/unit/test_git_http_backend.py index 427e469b..973f9cd5 100644 --- a/tests/unit/test_git_http_backend.py +++ b/tests/unit/test_git_http_backend.py @@ -5,6 +5,7 @@ import subprocess import tempfile import threading import unittest +import urllib.error import urllib.request from pathlib import Path from unittest import mock diff --git a/tests/unit/test_orchestrator_server.py b/tests/unit/test_orchestrator_server.py index f5b28f33..573920f6 100644 --- a/tests/unit/test_orchestrator_server.py +++ b/tests/unit/test_orchestrator_server.py @@ -70,15 +70,6 @@ def dispatch( response = asyncio.run(request()) payload = response.json() - if response.status_code == 422: - detail = payload.get("detail", []) if isinstance(payload, dict) else [] - field = "" - if isinstance(detail, list) and detail and isinstance(detail[0], dict): - location = detail[0].get("loc", ()) - if isinstance(location, (list, tuple)) and len(location) > 1: - field = str(location[1]) - suffix = f": {field}" if field else "" - return 400, {"error": f"invalid request body{suffix}"} if isinstance(payload, dict) and "detail" in payload and "error" not in payload: payload = {"error": payload["detail"]} return response.status_code, payload diff --git a/tests/unit/test_supervise_edge.py b/tests/unit/test_supervise_edge.py index e7a6d740..ac3af3b4 100644 --- a/tests/unit/test_supervise_edge.py +++ b/tests/unit/test_supervise_edge.py @@ -140,7 +140,7 @@ class TestStoreGuardBranches(unittest.TestCase): with tempfile.TemporaryDirectory() as d: db = Path(d) / "q.db" store = QueueStore("key", db_path=db) - with patch("pathlib.Path.chmod", side_effect=OSError("ro")): + with patch("os.fchmod", side_effect=OSError("ro")): with self.assertRaisesRegex(OSError, "ro"): store.migrate() @@ -156,7 +156,7 @@ class TestStoreGuardBranches(unittest.TestCase): with tempfile.TemporaryDirectory() as d: db = Path(d) / "a.db" store = AuditStore(db_path=db) - with patch("pathlib.Path.chmod", side_effect=OSError("ro")): + with patch("os.fchmod", side_effect=OSError("ro")): with self.assertRaisesRegex(OSError, "ro"): store.migrate()