From a2229bc26902e79d63a368e8ab002ebefd4f0863 Mon Sep 17 00:00:00 2001 From: Codex Lead Engineer Date: Thu, 30 Jul 2026 03:15:06 +0200 Subject: [PATCH] fix: address independent review findings --- CHANGELOG.md | 10 ++++- docs/ARCHITECTURE.md | 6 +-- docs/PRODUCTION_PROFILE.md | 5 ++- docs/THREAT_MODEL.md | 9 ++-- evidence/WP-PA-001-REWORK-EVIDENCE.md | 44 +++++++++++++++++++ src/project_bus/server.py | 46 +++++++++++++++++++- src/project_bus/service.py | 6 +++ tests/test_mcp_http.py | 62 ++++++++++++++++++++++++++- tests/test_service.py | 28 ++++++++++++ 9 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 evidence/WP-PA-001-REWORK-EVIDENCE.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 2125b2b..6af3ded 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## Unreleased + +- Bound JSON nesting before parsing and return a JSON-RPC parse error instead + of dropping the request connection. +- Bound request-body reads and return HTTP 408 for incomplete slow requests. +- Enforce the advertised object-item schema for result evidence and review + findings. +- Correct the documented post-`CHANGES_REQUESTED` remediation path. + All notable changes to this project are documented here. ## 0.1.0 — 2026-07-30 @@ -14,4 +23,3 @@ Initial review candidate: - transport authentication abstraction and BASDM separation controls; - tests, container example, contract, threat model, production profile, and WP-PA-001 review evidence. - diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fe83dc8..1a67aa3 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -49,8 +49,9 @@ Work packages move through: `HOLD` leaves a reviewed package in `REVIEWED`. A new review can be requested after a completed review. This MVP treats corrections after -`CHANGES_REQUESTED` as a new work package or a new explicitly governed -iteration; it does not silently overwrite submitted result evidence. +`CHANGES_REQUESTED` as a new work package. It does not support replacing the +result on the reviewed work package and never silently overwrites submitted +result evidence. ## Trust boundaries @@ -58,4 +59,3 @@ iteration; it does not silently overwrite submitted result evidence. - The database is trusted to enforce integrity and append-only triggers. - Artifact URIs and commit hashes are references, not trusted content. - Reverse proxy identity headers are not consumed by the built-in adapter. - diff --git a/docs/PRODUCTION_PROFILE.md b/docs/PRODUCTION_PROFILE.md index a519ab4..e0ba2c0 100644 --- a/docs/PRODUCTION_PROFILE.md +++ b/docs/PRODUCTION_PROFILE.md @@ -13,7 +13,9 @@ hardening work package. 2. Replace `StaticTokenAuthProvider` with validated OIDC/JWT or mTLS workload identity. Bind issuer, audience, subject, expiry, and revocation policy. 3. Put the service behind TLS and authenticated ingress. Do not trust arbitrary - forwarded identity headers. + forwarded identity headers. The ingress must enforce connection limits, + header/body read deadlines, and request quotas in addition to the + application's five-second request-body timeout. 4. Run multiple stateless instances only after PostgreSQL conformance and concurrency tests pass. 5. Add structured audit export, metrics, tracing correlation, alerts, backup, @@ -33,4 +35,3 @@ production gate acceptance. Chat threads are not push subscribers. A client, automation, or PA invocation must call `sync_since(last_cursor)`. The cursor itself belongs in durable client state and is advanced only after successful processing. - diff --git a/docs/THREAT_MODEL.md b/docs/THREAT_MODEL.md index 98d7f9f..63ca9bb 100644 --- a/docs/THREAT_MODEL.md +++ b/docs/THREAT_MODEL.md @@ -37,6 +37,8 @@ | Cross-project reads | project filter plus membership resolution | | Browser cross-origin request | exact configurable Origin allowlist | | Oversized request | one MiB body limit | +| Excessive JSON nesting | pre-parse nesting limit plus guarded decoder | +| Slow/incomplete body | five-second application read timeout | | MIME confusion/caching | strict JSON input, nosniff, no-store | | SQL injection | parameterized SQL; one controlled placeholder expansion | @@ -57,8 +59,10 @@ fetched or executed by the bus. 4. The event payload is not cryptographically chained or signed. Add hash chaining/signatures only if the agreed threat model requires tamper evidence against privileged database operators. -5. No application rate limiter exists. Apply per-identity limits and request - quotas at the trusted edge. +5. No application rate limiter or bounded worker pool exists. The application + closes incomplete request bodies after five seconds, but production ingress + must additionally enforce connection limits, header/body read deadlines, + per-identity limits, and request quotas. 6. The server emits access metadata but no security audit sink or metrics. Integrate structured logs, alerts, and privacy-aware retention. 7. Membership deactivation and credential revocation are not exposed as MVP @@ -80,4 +84,3 @@ fetched or executed by the bus. - review log contents for sensitive project data; - add abuse limits and operational monitoring; - decide whether privileged-operator tamper evidence is required. - diff --git a/evidence/WP-PA-001-REWORK-EVIDENCE.md b/evidence/WP-PA-001-REWORK-EVIDENCE.md new file mode 100644 index 0000000..935099a --- /dev/null +++ b/evidence/WP-PA-001-REWORK-EVIDENCE.md @@ -0,0 +1,44 @@ +# WP-PA-001 Rework Evidence + +Date: 2026-07-30 + +## Review input + +- Independent reviewer: Claude Code +- Reviewed commit: `bc55924198865502203a992b6ea914a38fd9d173` +- Verdict: `CHANGES_REQUESTED` +- Findings addressed: M-1, M-2, L-1, L-2 +- Optional observation O-1: no correction required + +## Corrections + +| Finding | Correction | Regression evidence | +|---|---|---| +| M-1 | Added a pre-parse JSON nesting limit and retained `RecursionError` handling. Excessively nested JSON returns JSON-RPC `-32700`. Brackets inside strings are ignored by the nesting scanner. | `test_deeply_nested_json_returns_parse_error`; `test_brackets_inside_json_string_do_not_count_as_nesting` | +| M-2 | Added a five-second connection/request-body read timeout. Incomplete bodies receive HTTP 408 and the connection is closed. Production ingress requirements now explicitly include connection limits and read deadlines. | `test_incomplete_body_times_out` | +| L-1 | Corrected the architecture document: post-`CHANGES_REQUESTED` corrections require a new work package in this MVP. | Documentation inspection | +| L-2 | Enforced object items for `evidence` and `findings`, matching the advertised MCP schemas. | `test_evidence_and_findings_items_must_be_objects` | + +## Verification + +Runtime: Python 3.12.13 + +```text +PYTHONPATH=src python3.12 -m unittest discover -v +Ran 20 tests in 5.696s +OK + +PYTHONPATH=src python3.12 -m compileall -q src tests +exit 0 + +pip install --no-cache-dir --no-deps --target . +PYTHONPATH= PROJECT_BUS_MIGRATIONS=/migrations \ + python3.12 -m unittest discover -v tests +Ran 20 tests in 5.394s +OK + +git diff --check +exit 0 +``` + +The rework must receive a fresh independent review before PM gate acceptance. diff --git a/src/project_bus/server.py b/src/project_bus/server.py index ff3bc06..c054959 100644 --- a/src/project_bus/server.py +++ b/src/project_bus/server.py @@ -14,6 +14,8 @@ from .mcp import MCPApplication LOGGER = logging.getLogger("project_bus.http") MAX_BODY_BYTES = 1_048_576 +MAX_JSON_NESTING_DEPTH = 128 +DEFAULT_REQUEST_READ_TIMEOUT_SECONDS = 5.0 class ProjectBusHTTPServer(ThreadingHTTPServer): @@ -25,17 +27,25 @@ class ProjectBusHTTPServer(ThreadingHTTPServer): application: MCPApplication, auth_provider: AuthProvider, allowed_origins: set[str], + request_read_timeout_seconds: float = DEFAULT_REQUEST_READ_TIMEOUT_SECONDS, ): + if request_read_timeout_seconds <= 0: + raise ValueError("request_read_timeout_seconds must be positive") super().__init__(server_address, ProjectBusRequestHandler) self.application = application self.auth_provider = auth_provider self.allowed_origins = allowed_origins + self.request_read_timeout_seconds = request_read_timeout_seconds class ProjectBusRequestHandler(BaseHTTPRequestHandler): server: ProjectBusHTTPServer protocol_version = "HTTP/1.1" + def setup(self) -> None: + super().setup() + self.connection.settimeout(self.server.request_read_timeout_seconds) + def log_message(self, format_string: str, *args: Any) -> None: LOGGER.info("%s - %s", self.address_string(), format_string % args) @@ -73,13 +83,20 @@ class ProjectBusRequestHandler(BaseHTTPRequestHandler): self._json(HTTPStatus.REQUEST_ENTITY_TOO_LARGE, {"error": "invalid_body_size"}) return try: - request = json.loads(self.rfile.read(length)) - except (json.JSONDecodeError, UnicodeDecodeError): + body = self.rfile.read(length) + if _exceeds_json_nesting_limit(body): + raise RecursionError("JSON nesting limit exceeded") + request = json.loads(body) + except (json.JSONDecodeError, UnicodeDecodeError, RecursionError): self._json( HTTPStatus.OK, {"jsonrpc": "2.0", "id": None, "error": {"code": -32700, "message": "Parse error"}}, ) return + except TimeoutError: + self.close_connection = True + self._json(HTTPStatus.REQUEST_TIMEOUT, {"error": "request_body_timeout"}) + return if not isinstance(request, dict): self._json( HTTPStatus.OK, @@ -134,3 +151,28 @@ def allowed_origins_from_environment() -> set[str]: raw = os.environ.get("PROJECT_BUS_ALLOWED_ORIGINS", "") return {item.strip() for item in raw.split(",") if item.strip()} + +def _exceeds_json_nesting_limit( + body: bytes, limit: int = MAX_JSON_NESTING_DEPTH +) -> bool: + depth = 0 + in_string = False + escaped = False + for byte in body: + if in_string: + if escaped: + escaped = False + elif byte == 0x5C: # backslash + escaped = True + elif byte == 0x22: # double quote + in_string = False + continue + if byte == 0x22: + in_string = True + elif byte in (0x5B, 0x7B): # [ { + depth += 1 + if depth > limit: + return True + elif byte in (0x5D, 0x7D): # ] } + depth -= 1 + return False diff --git a/src/project_bus/service.py b/src/project_bus/service.py index 06ab513..5ac457b 100644 --- a/src/project_bus/service.py +++ b/src/project_bus/service.py @@ -451,6 +451,8 @@ class ProjectBusService: summary = self._require_text("summary", summary) if not isinstance(evidence, list): raise ValidationError("evidence must be a list") + if any(not isinstance(item, dict) for item in evidence): + raise ValidationError("evidence items must be objects") request = {"work_package_id": work_package_id, "summary": summary, "evidence": evidence} def action(connection: sqlite3.Connection, context: ActorContext) -> dict[str, Any]: @@ -600,6 +602,10 @@ class ProjectBusService: summary: str, idempotency_key: str, ) -> dict[str, Any]: + if not isinstance(findings, list): + raise ValidationError("findings must be a list") + if any(not isinstance(item, dict) for item in findings): + raise ValidationError("findings items must be objects") try: review_verdict = ReviewVerdict(verdict) except ValueError as error: diff --git a/tests/test_mcp_http.py b/tests/test_mcp_http.py index 530b7b0..2d66d7d 100644 --- a/tests/test_mcp_http.py +++ b/tests/test_mcp_http.py @@ -1,6 +1,8 @@ from __future__ import annotations import json +import http.client +import socket import tempfile import threading import unittest @@ -24,7 +26,11 @@ class MCPHTTPTestCase(unittest.TestCase): {"bootstrap-secret-at-least-24": TokenIdentity("system", "system", "System")} ) self.server = ProjectBusHTTPServer( - ("127.0.0.1", 0), MCPApplication(service), auth, {"https://chat.example"} + ("127.0.0.1", 0), + MCPApplication(service), + auth, + {"https://chat.example"}, + request_read_timeout_seconds=0.2, ) self.thread = threading.Thread(target=self.server.serve_forever, daemon=True) self.thread.start() @@ -115,6 +121,60 @@ class MCPHTTPTestCase(unittest.TestCase): self.assertEqual(status, 403) self.assertEqual(response["error"], "origin_not_allowed") + def test_deeply_nested_json_returns_parse_error(self) -> None: + body = b"[" * 200_000 + b"]" * 200_000 + connection = http.client.HTTPConnection( + "127.0.0.1", self.server.server_port, timeout=2 + ) + connection.request( + "POST", + "/mcp", + body=body, + headers={"Content-Type": "application/json"}, + ) + response = connection.getresponse() + payload = json.loads(response.read()) + connection.close() + + self.assertEqual(response.status, 200) + self.assertEqual(payload["error"]["code"], -32700) + + def test_brackets_inside_json_string_do_not_count_as_nesting(self) -> None: + status, response = self.request( + { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": {"text": "[{" * 500 + "}]" * 500}, + } + ) + self.assertEqual(status, 200) + self.assertIn("result", response) + + def test_incomplete_body_times_out(self) -> None: + connection = socket.create_connection( + ("127.0.0.1", self.server.server_port), timeout=2 + ) + connection.settimeout(2) + connection.sendall( + b"POST /mcp HTTP/1.1\r\n" + b"Host: 127.0.0.1\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 100\r\n" + b"Connection: close\r\n\r\n" + b'{"jsonrpc":' + ) + response = b"" + while True: + chunk = connection.recv(4_096) + if not chunk: + break + response += chunk + connection.close() + + self.assertIn(b"HTTP/1.1 408 Request Timeout", response) + self.assertIn(b'"error":"request_body_timeout"', response) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_service.py b/tests/test_service.py index 82369f9..aed0baf 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -256,6 +256,34 @@ class ServiceTestCase(unittest.TestCase): idempotency_key="premature-gate", ) + def test_evidence_and_findings_items_must_be_objects(self) -> None: + self.issue() + self.service.claim_work_package( + self.principals["lead"], + project_id="sandbox", + work_package_id="WP-001", + idempotency_key="claim", + ) + with self.assertRaises(ValidationError): + self.service.submit_result( + self.principals["lead"], + project_id="sandbox", + work_package_id="WP-001", + summary="Invalid evidence.", + evidence=["not-an-object"], # type: ignore[list-item] + idempotency_key="invalid-evidence", + ) + with self.assertRaises(ValidationError): + self.service.submit_review( + self.principals["reviewer"], + project_id="sandbox", + review_id="rev-does-not-matter", + verdict="APPROVE", + findings=[42], # type: ignore[list-item] + summary="Invalid findings.", + idempotency_key="invalid-findings", + ) + def test_idempotent_replay_and_key_mismatch(self) -> None: first = self.service.issue_work_package( self.principals["pm"],