fix: close consolidated quality review findings

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"}

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:

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:

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

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

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()