From 87a26196dde86df089ad6fb6b82facf53fad0fdf Mon Sep 17 00:00:00 2001 From: nac Date: Mon, 13 Jul 2026 02:21:19 +0000 Subject: [PATCH] fix: harden project writes and uploads --- nocobase_crm_mcp/nocobase_http.py | 12 ++++++- nocobase_crm_mcp/projects.py | 21 ++++++++++-- nocobase_crm_mcp/uploads.py | 53 +++++++++++++++++++++++++++--- tests/test_nocobase_http.py | 25 ++++++++++++++ tests/test_projects.py | 39 ++++++++++++++++++++++ tests/test_server.py | 27 ++++++++++++++-- tests/test_uploads.py | 54 +++++++++++++++++++++++++++++++ 7 files changed, 221 insertions(+), 10 deletions(-) diff --git a/nocobase_crm_mcp/nocobase_http.py b/nocobase_crm_mcp/nocobase_http.py index 3fc8849..b0eb21b 100644 --- a/nocobase_crm_mcp/nocobase_http.py +++ b/nocobase_crm_mcp/nocobase_http.py @@ -2,6 +2,7 @@ from __future__ import annotations from pathlib import Path from typing import Any, Sequence +from urllib.parse import quote import httpx @@ -57,8 +58,9 @@ class NocoBaseHttpClient: self._post_attachment_ids(project_id, attachment_ids, "remove") def _post_attachment_ids(self, project_id: Any, attachment_ids: Sequence[Any], action: str) -> None: + project_id_segment = _project_id_segment(project_id) response = self._client.post( - f"/t_projects/{project_id}/f_projects_attachments:{action}", + f"/t_projects/{project_id_segment}/f_projects_attachments:{action}", json={"tk": list(attachment_ids)}, ) self._raise_for_error(response) @@ -81,3 +83,11 @@ class NocoBaseHttpClient: def _redact(self, value: str) -> str: return value.replace(self.jwt, "") + + +def _project_id_segment(project_id: Any) -> str: + if isinstance(project_id, bool) or not isinstance(project_id, (str, int)): + raise ValueError("project_id must be a scalar identifier") + if isinstance(project_id, str) and not project_id.strip(): + raise ValueError("project_id must not be blank") + return quote(str(project_id), safe="") diff --git a/nocobase_crm_mcp/projects.py b/nocobase_crm_mcp/projects.py index 6d48f2a..e2ba1af 100644 --- a/nocobase_crm_mcp/projects.py +++ b/nocobase_crm_mcp/projects.py @@ -97,6 +97,7 @@ class ProjectService: remove_attachment_ids: Sequence[Any] | None = None, ) -> dict[str, Any]: try: + _validate_identifier(project_id, "project_id") values = self._project_values( name=name, customer_id=customer_id, @@ -153,6 +154,7 @@ class ProjectService: raise _InvalidProjectInput("name is required") if customer_id is not None: + _validate_identifier(customer_id, "customer_id") values[self.config.customer_field] = customer_id if inquiry_date is not None: if not isinstance(inquiry_date, str): @@ -165,7 +167,9 @@ class ProjectService: except ValueError as error: raise _InvalidProjectInput("inquiry_date must be a YYYY-MM-DD date") from error if progress is not None: - if not isinstance(progress, str) or progress not in self.config.progress_values: + if not isinstance(progress, str): + raise _InvalidProjectInput("progress must be a scalar value") + if progress not in self.config.progress_values: raise _InvalidProgress values[self.config.progress_field] = self.config.progress_values[progress] @@ -176,6 +180,8 @@ class ProjectService: (details, self.config.details_field), ): if value is not None: + if not isinstance(value, str): + raise _InvalidProjectInput(f"{field} must be a scalar value") values[field] = value return values @@ -188,7 +194,11 @@ def _deduplicated_attachment_ids(attachment_ids: Sequence[Any] | None) -> list[A unique_ids: list[Any] = [] for attachment_id in attachment_ids: - if attachment_id is None or isinstance(attachment_id, (Mapping, Sequence)): + try: + _validate_identifier(attachment_id, "attachment IDs") + except _InvalidProjectInput as error: + raise _InvalidProjectInput("attachment IDs must be scalar values") from error + if attachment_id is None: raise _InvalidProjectInput("attachment IDs must be scalar values") if attachment_id not in unique_ids: unique_ids.append(attachment_id) @@ -206,6 +216,13 @@ def _project_id(response: Mapping[str, Any], id_field: str) -> Any: return data[id_field] +def _validate_identifier(value: Any, name: str) -> None: + if isinstance(value, bool) or not isinstance(value, (str, int)): + raise _InvalidProjectInput(f"{name} must be a scalar identifier") + if isinstance(value, str) and not value.strip(): + raise _InvalidProjectInput(f"{name} must not be blank") + + def _attachment_error(project_id: Any, error: Exception) -> dict[str, Any]: if isinstance(error, NocoBaseHttpError): if error.status_code == 404: diff --git a/nocobase_crm_mcp/uploads.py b/nocobase_crm_mcp/uploads.py index f6a4ee6..0a2d202 100644 --- a/nocobase_crm_mcp/uploads.py +++ b/nocobase_crm_mcp/uploads.py @@ -14,6 +14,11 @@ from .nocobase_http import NocoBaseHttpError _COPY_CHUNK_BYTES = 1024 * 1024 +_MULTIPART_OVERHEAD_BYTES = 64 * 1024 + + +class _UploadTooLarge(ValueError): + pass class UploadEndpoint: @@ -38,17 +43,20 @@ class UploadEndpoint: return _error_response("invalid_upload", "Uploads require multipart form data", 400) try: + request = await _bounded_request(request, self._max_bytes + _MULTIPART_OVERHEAD_BYTES) form = await request.form() + except _UploadTooLarge: + return _error_response("upload_too_large", "Upload exceeds the configured size limit", 413) except Exception: return _error_response("invalid_upload", "Could not read upload form data", 400) - uploads = [(name, value) for name, value in form.multi_items() if isinstance(value, UploadFile)] - file_values = form.getlist("file") - if len(uploads) != 1 or len(file_values) != 1 or uploads[0][0] != "file": + items = list(form.multi_items()) + uploads = [(name, value) for name, value in items if isinstance(value, UploadFile)] + if len(items) != 1 or len(uploads) != 1 or uploads[0][0] != "file": await _close_uploads(value for _, value in uploads) return _error_response("invalid_upload", "Provide exactly one file upload", 400) - upload = file_values[0] + upload = uploads[0][1] if not isinstance(upload, UploadFile): return _error_response("invalid_upload", "Provide exactly one file upload", 400) @@ -94,6 +102,43 @@ def _is_multipart(request: Request) -> bool: return content_type.split(";", 1)[0].strip().lower() == "multipart/form-data" +async def _bounded_request(request: Request, max_body_bytes: int) -> Request: + content_length = request.headers.get("content-length") + if content_length is not None: + try: + if int(content_length) > max_body_bytes: + raise _UploadTooLarge + except ValueError: + pass + + received_bytes = 0 + chunks: list[bytes] = [] + while True: + message = await request.receive() + if message.get("type") == "http.request": + body = message.get("body", b"") + received_bytes += len(body) + if received_bytes > max_body_bytes: + raise _UploadTooLarge + chunks.append(body) + if not message.get("more_body", False): + break + elif message.get("type") == "http.disconnect": + break + + body = b"".join(chunks) + replayed = False + + async def receive() -> dict[str, Any]: + nonlocal replayed + if replayed: + return {"type": "http.disconnect"} + replayed = True + return {"type": "http.request", "body": body, "more_body": False} + + return Request(request.scope, receive=receive) + + def _attachment_id(attachment: Any) -> int | None: if not isinstance(attachment, Mapping): return None diff --git a/tests/test_nocobase_http.py b/tests/test_nocobase_http.py index 080ec06..c033c0b 100644 --- a/tests/test_nocobase_http.py +++ b/tests/test_nocobase_http.py @@ -67,6 +67,31 @@ class NocoBaseHttpClientTests(unittest.TestCase): self.assertEqual(str(requests[1].url), "https://nb.example/api/t_projects/7/f_projects_attachments:remove") self.assertEqual(json.loads(requests[1].content), {"tk": [10]}) + def test_attachment_association_encodes_project_id_as_one_path_segment(self) -> None: + requests: list[httpx.Request] = [] + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(request) + return httpx.Response(204) + + client = NocoBaseHttpClient("https://nb.example/api", "jwt", transport=httpx.MockTransport(handler)) + client.add_attachment_ids("../project/7?#", [8]) + + self.assertEqual( + str(requests[0].url), + "https://nb.example/api/t_projects/..%2Fproject%2F7%3F%23/f_projects_attachments:add", + ) + + def test_attachment_association_rejects_non_scalar_project_id_before_request(self) -> None: + client = NocoBaseHttpClient( + "https://nb.example/api", + "jwt", + transport=httpx.MockTransport(lambda request: self.fail("request must not be sent")), + ) + + with self.assertRaisesRegex(ValueError, "project_id"): + client.add_attachment_ids({"id": 7}, [8]) + def test_rejects_blank_upload_credentials(self) -> None: with self.assertRaises(ValueError): NocoBaseHttpClient(" ", "jwt") diff --git a/tests/test_projects.py b/tests/test_projects.py index 3e45ca7..9095b3b 100644 --- a/tests/test_projects.py +++ b/tests/test_projects.py @@ -113,6 +113,45 @@ class ProjectServiceTests(unittest.TestCase): self.assertEqual(result["error"]["code"], "invalid_project_input") self.assertEqual(self.nb.update_calls, []) + def test_create_rejects_nested_scalar_inputs_before_writing(self) -> None: + cases = { + "name": {"name": "x"}, + "customer_id": {"id": 3}, + "inquiry_date": ["2026-07-12"], + "progress": ("SOP",), + "applications": ["EV"], + "part_numbers": ("123",), + "period_usage": {"annual"}, + "details": {"note": "x"}, + } + + for field, value in cases.items(): + with self.subTest(field=field): + self.setUp() + arguments = {"name": "x", **{field: value}} + result = self.service.create_project(**arguments) + + self.assertEqual(result["error"]["code"], "invalid_project_input") + self.assertEqual(self.nb.create_calls, []) + self.assertEqual(self.http.add_calls, []) + + def test_update_rejects_nested_project_id_before_any_client_call(self) -> None: + for project_id in ({"id": 41}, [41], {41}, (41,)): + with self.subTest(project_id=project_id): + self.setUp() + result = self.service.update_project(project_id, name="x", add_attachment_ids=[8]) + + self.assertEqual(result["error"]["code"], "invalid_project_input") + self.assertEqual(self.nb.update_calls, []) + self.assertEqual(self.http.add_calls, []) + + def test_create_rejects_nested_attachment_id_before_any_client_call(self) -> None: + result = self.service.create_project(name="x", attachment_ids=[{"id": 8}]) + + self.assertEqual(result["error"]["code"], "invalid_project_input") + self.assertEqual(self.nb.create_calls, []) + self.assertEqual(self.http.add_calls, []) + def test_update_writes_scalars_then_deduplicated_attachment_changes_in_order(self) -> None: result = self.service.update_project( 41, diff --git a/tests/test_server.py b/tests/test_server.py index f52ec74..aa9cdaf 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -2,6 +2,9 @@ import asyncio import unittest from unittest.mock import patch +import httpx +from starlette.responses import JSONResponse + from nocobase_crm_mcp.server import ( DEFAULT_HTTP_HOST, DEFAULT_HTTP_PATH, @@ -77,9 +80,8 @@ class FakeProjectService: class FakeUploadEndpoint: - async def __call__(self, scope, receive, send): - await send({"type": "http.response.start", "status": 200, "headers": []}) - await send({"type": "http.response.body", "body": b"uploaded"}) + async def __call__(self, request): + return JSONResponse({"uploaded": True}) class ServerTests(unittest.TestCase): @@ -150,6 +152,25 @@ class ServerTests(unittest.TestCase): self.assertEqual(run_asgi_request(app, path="/uploads")[0]["status"], 401) + def test_upload_route_accepts_the_matching_bearer_token(self) -> None: + if FastMCP is None: + self.skipTest("MCP SDK is not installed") + + server = create_server(crm_service=FakeCrmService(), readonly=True) + + async def upload_endpoint(request): + return JSONResponse({"uploaded": True}) + + app = create_http_app(server, "secret", upload_endpoint=upload_endpoint) + + async def request_upload() -> httpx.Response: + async with httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://testserver") as client: + return await client.post("/uploads", headers={"Authorization": "Bearer secret"}) + + response = asyncio.run(request_upload()) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), {"uploaded": True}) + def test_http_write_mode_requires_upload_api_base_url(self) -> None: with patch.dict("os.environ", {"NOCOBASE_MCP_TOKEN": "secret"}, clear=True): with patch("uvicorn.run") as uvicorn_run: diff --git a/tests/test_uploads.py b/tests/test_uploads.py index 5f196e6..1e7a117 100644 --- a/tests/test_uploads.py +++ b/tests/test_uploads.py @@ -3,6 +3,7 @@ import unittest import warnings from pathlib import Path from tempfile import TemporaryDirectory +from unittest.mock import patch import httpx from starlette.applications import Starlette @@ -103,6 +104,59 @@ class UploadEndpointTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(temp_paths, []) self.assertFalse(any(issubclass(warning.category, ResourceWarning) for warning in caught)) + async def test_upload_rejects_non_file_form_fields(self) -> None: + http_client = FakeHttpClient() + with TemporaryDirectory() as temp_dir: + async with await self._client_for(http_client, temp_dir) as client: + response = await client.post( + "/uploads", + files={"file": ("quote.pdf", b"pdf", "application/pdf")}, + data={"description": "quote"}, + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json()["error"]["code"], "invalid_upload") + self.assertEqual(http_client.calls, []) + self.assertEqual(list(Path(temp_dir).iterdir()), []) + + async def test_upload_rejects_oversized_misreported_chunked_body_before_multipart_parsing(self) -> None: + http_client = FakeHttpClient() + endpoint = UploadEndpoint(http_client, max_bytes=5, temp_dir="/tmp") + parsed = False + + async def forbidden_form(self): + nonlocal parsed + parsed = True + self.fail("multipart parsing must not begin for an oversized body") + + body = b"x" * (5 + 65537) + scope = { + "type": "http", + "method": "POST", + "path": "/uploads", + "headers": [ + (b"content-type", b"multipart/form-data; boundary=test"), + (b"content-length", b"1"), + ], + } + messages = [ + {"type": "http.request", "body": body[:10], "more_body": True}, + {"type": "http.request", "body": body[10:], "more_body": False}, + ] + + async def receive(): + return messages.pop(0) + + from starlette.requests import Request + + starlette_request = Request(scope, receive=receive) + with patch("nocobase_crm_mcp.uploads.Request.form", new=forbidden_form): + response = await endpoint(starlette_request) + + self.assertEqual(response.status_code, 413) + self.assertFalse(parsed) + self.assertEqual(http_client.calls, []) + async def test_upload_rejects_disallowed_mime_type_before_calling_nocobase(self) -> None: http_client = FakeHttpClient() with TemporaryDirectory() as temp_dir: