From b4dc87588b07e32e3988f97342edf23cdbfdbfc3 Mon Sep 17 00:00:00 2001 From: Erfouni Date: Sun, 20 Sep 2026 10:52:15 +0330 Subject: [PATCH] Test the secret-path gate the README's security model rests on server.py states the whole security model in its docstring: the transport is mounted at an unguessable URL path, and that path is the gate because ChatGPT's connectors cannot send an Authorization header. Nothing tested it. A gate that quietly stops gating looks exactly like one that works, and this server is deliberately exposed through an ngrok tunnel. tests/test_secret_path_gate.py drives SecretPathAuth as plain ASGI middleware with a stub downstream app -- no uvicorn, no COM, no AutoCAD. 14 cases: - the exact secret path and paths beneath it pass through; - unrelated paths, the root, and the secret appearing later in the path 404; - `/x`, `/-extra` and `/.json` 404. A bare startswith(prefix) check would admit all three and turn the secret into a guessable stem, so this is the case worth pinning down; - non-http scopes (lifespan, websocket) pass without being path-checked; - a wrong bearer is 401, a right one passes, a missing one passes by design; - a wrong bearer on a wrong path is still 404, so a bad path cannot be used to probe the token. tests/test_config.py gains 8 cases for _load_or_create_secret. The connector URLs pasted into ChatGPT and Manus embed the secret path, so if a restart regenerated it every connector would start 404ing with no obvious cause: generated once and written, reused on the next start, read back without the trailing newline, blank file replaced, env override wins and writes no file, override beats a stored secret, generated values do not repeat, and MCP_PATH derives from the secret. The tests were checked against deliberately broken copies rather than just run green. Replacing the prefix check with a bare startswith fails 4; removing the bearer check fails 2; skipping the path check entirely fails 8; dropping the .secret reuse fails 2; ignoring the env override fails 3. The repo had no CI, so none of this would run on its own. The workflow uses windows-latest -- config and tools deal in Windows paths and server imports pywin32 -- and adds compileall and a PowerShell parse check. Actions are pinned by SHA, matching helios-llm-orchestrator. No production code changes in this commit. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 43 ++++++++ tests/test_config.py | 66 +++++++++++- tests/test_secret_path_gate.py | 189 +++++++++++++++++++++++++++++++++ 3 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/ci.yml create mode 100644 tests/test_secret_path_gate.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..ca010d0 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,43 @@ +name: CI + +on: + push: + pull_request: + +permissions: + contents: read + +jobs: + test: + # windows-latest because config and tools deal in Windows paths, and the + # server imports pywin32. No AutoCAD is needed -- nothing here talks COM. + runs-on: windows-latest + env: + # Without this the config import writes a .secret into the checkout. + ACAD_MCP_SECRET_PATH: ci-secret + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 + with: + python-version: "3.13" + - run: pip install -r requirements.txt + - name: Byte-compile every module + run: python -m compileall -q acad.py config.py server.py tools.py test_draw.py + - name: Unit tests + run: python -m unittest discover -s tests -t . -v + - name: Parse PowerShell scripts + shell: pwsh + run: | + $failed = $false + Get-ChildItem *.ps1 | ForEach-Object { + $errors = $null + [System.Management.Automation.Language.Parser]::ParseFile( + $_.FullName, [ref]$null, [ref]$errors) | Out-Null + if ($errors) { + Write-Host "::error file=$($_.Name)::$($errors -join '; ')" + $failed = $true + } else { + Write-Host "ok: $($_.Name)" + } + } + if ($failed) { exit 1 } diff --git a/tests/test_config.py b/tests/test_config.py index e14c009..4143a05 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,4 +1,4 @@ -"""Defaults in config.py. +"""Defaults and secret-path persistence in config.py. config reads the environment once, at import, so each case reloads the module under a patched environment. ACAD_MCP_SECRET_PATH is always set: without it the @@ -62,6 +62,70 @@ def test_empty_variable_does_not_mean_current_directory(self): self.assertEqual(save_dir.name, "AutoCAD-MCP-Out") +class SecretPathPersistence(unittest.TestCase): + """`.secret` is what keeps connector URLs valid across a restart. + + The URLs pasted into ChatGPT and Manus embed the secret path. If a restart + regenerated it, every connector would start 404ing with no obvious cause, + so the generate-once-then-reuse behaviour is load-bearing. + """ + + def setUp(self): + self._dir = tempfile.TemporaryDirectory() + self.addCleanup(self._dir.cleanup) + self.secret_file = Path(self._dir.name) / ".secret" + + def _load_secret(self, **env): + patched = {k: v for k, v in os.environ.items() + if k != "ACAD_MCP_SECRET_PATH"} + patched.update(env) + with mock.patch.dict(os.environ, patched, clear=True): + with mock.patch.object(config, "SECRET_FILE", self.secret_file): + return config._load_or_create_secret() + + def test_generated_when_no_file_exists(self): + secret = self._load_secret() + self.assertTrue(secret.startswith("mcp-")) + self.assertTrue(self.secret_file.exists()) + self.assertEqual(self.secret_file.read_text(encoding="utf-8"), secret) + + def test_reused_on_the_next_start(self): + first = self._load_secret() + self.assertEqual(self._load_secret(), first) + + def test_stored_value_is_read_back_without_trailing_newline(self): + self.secret_file.write_text("mcp-from-disk" + chr(10), encoding="utf-8") + self.assertEqual(self._load_secret(), "mcp-from-disk") + + def test_blank_file_is_replaced(self): + self.secret_file.write_text(" " + chr(10), encoding="utf-8") + secret = self._load_secret() + self.assertTrue(secret.startswith("mcp-")) + self.assertEqual(self.secret_file.read_text(encoding="utf-8"), secret) + + def test_environment_override_wins_and_writes_nothing(self): + self.assertEqual( + self._load_secret(ACAD_MCP_SECRET_PATH="/chosen/"), "chosen" + ) + self.assertFalse(self.secret_file.exists()) + + def test_override_beats_a_stored_secret(self): + self.secret_file.write_text("mcp-from-disk", encoding="utf-8") + self.assertEqual( + self._load_secret(ACAD_MCP_SECRET_PATH="chosen"), "chosen" + ) + + def test_generated_secrets_are_not_predictable(self): + produced = set() + for _ in range(5): + self.secret_file.unlink(missing_ok=True) + produced.add(self._load_secret()) + self.assertEqual(len(produced), 5) + + def test_mcp_path_is_derived_from_the_secret(self): + self.assertEqual(load(ACAD_MCP_SECRET_PATH="abc").MCP_PATH, "/abc/mcp") + + def tearDownModule(): # Leave one known state behind for whatever imports config next. load() diff --git a/tests/test_secret_path_gate.py b/tests/test_secret_path_gate.py new file mode 100644 index 0000000..cfafac4 --- /dev/null +++ b/tests/test_secret_path_gate.py @@ -0,0 +1,189 @@ +"""The request gate in server.SecretPathAuth. + +The README's security model rests on one claim: the transport is mounted at an +unguessable URL path and everything not addressed to that path is refused. That +claim was asserted but never tested, so nothing would notice if the prefix check +loosened -- and a gate that quietly stops gating looks exactly like one that +works. + +SecretPathAuth is plain ASGI middleware, so these drive it directly with a stub +downstream app rather than standing up uvicorn or touching AutoCAD. + + python -m unittest discover -s tests -t . +""" + +import asyncio +import os +import unittest +from unittest import mock + +# Must be in place before importing config: without it, the import writes a +# .secret file into the source tree. +os.environ.setdefault("ACAD_MCP_SECRET_PATH", "unit-test-secret") + +import config # noqa: E402 +import server # noqa: E402 + + +class StubApp: + """Downstream app that records whether the gate let a request through.""" + + def __init__(self): + self.calls = [] + + async def __call__(self, scope, receive, send): + self.calls.append(scope) + await send({"type": "http.response.start", "status": 200, "headers": []}) + await send({"type": "http.response.body", "body": b"through"}) + + @property + def reached(self): + return bool(self.calls) + + +def call(gate, path, headers=None, scope_type="http", method="POST"): + """Drive the gate once; return (status, body) as seen by the client.""" + scope = {"type": scope_type, "path": path, "method": method, + "client": ("127.0.0.1", 51234), + "headers": list(headers or [])} + sent = [] + + async def receive(): + return {"type": "http.request", "body": b"", "more_body": False} + + async def send(message): + sent.append(message) + + asyncio.run(gate(scope, receive, send)) + + status = next((m["status"] for m in sent if m["type"] == "http.response.start"), None) + body = b"".join(m.get("body", b"") for m in sent if m["type"] == "http.response.body") + return status, body + + +def build(secret="unit-test-secret"): + """A gate wired to a fresh stub, for the given secret path.""" + downstream = StubApp() + with mock.patch.object(config, "SECRET_PATH", secret): + gate = server.SecretPathAuth(downstream) + return gate, downstream + + +class PathGate(unittest.TestCase): + """Only the secret path, and paths beneath it, may reach the transport.""" + + def test_exact_secret_path_passes(self): + gate, downstream = build() + status, _ = call(gate, "/unit-test-secret") + self.assertEqual(status, 200) + self.assertTrue(downstream.reached) + + def test_path_below_the_secret_passes(self): + gate, downstream = build() + status, _ = call(gate, "/unit-test-secret/mcp") + self.assertEqual(status, 200) + self.assertTrue(downstream.reached) + + def test_unrelated_path_is_refused(self): + gate, downstream = build() + status, body = call(gate, "/mcp") + self.assertEqual(status, 404) + self.assertEqual(body, b"Not Found") + self.assertFalse(downstream.reached) + + def test_root_is_refused(self): + gate, downstream = build() + self.assertEqual(call(gate, "/")[0], 404) + self.assertFalse(downstream.reached) + + def test_prefix_confusion_is_refused(self): + """`/x` must not pass as `/`. + + A bare startswith(prefix) check would let every path that merely begins + with the secret through, which turns the secret into a guessable stem. + """ + gate, downstream = build() + for path in ("/unit-test-secretx", + "/unit-test-secret-extra", + "/unit-test-secret.json"): + with self.subTest(path=path): + self.assertEqual(call(gate, path)[0], 404) + self.assertFalse(downstream.reached) + + def test_secret_elsewhere_in_the_path_is_refused(self): + gate, downstream = build() + self.assertEqual(call(gate, "/public/unit-test-secret")[0], 404) + self.assertFalse(downstream.reached) + + def test_non_http_scope_passes_untouched(self): + """Lifespan and websocket scopes must not be path-checked.""" + gate, downstream = build() + call(gate, "", scope_type="lifespan") + self.assertTrue(downstream.reached) + self.assertEqual(downstream.calls[0]["type"], "lifespan") + + +class BearerToken(unittest.TestCase): + """The optional bearer check: enforced when presented, never required.""" + + def test_no_token_configured_ignores_any_header(self): + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", ""): + status, _ = call(gate, "/unit-test-secret", + headers=[(b"authorization", b"Bearer whatever")]) + self.assertEqual(status, 200) + self.assertTrue(downstream.reached) + + def test_correct_token_passes(self): + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", "s3cret"): + status, _ = call(gate, "/unit-test-secret", + headers=[(b"authorization", b"Bearer s3cret")]) + self.assertEqual(status, 200) + self.assertTrue(downstream.reached) + + def test_wrong_token_is_rejected(self): + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", "s3cret"): + status, body = call(gate, "/unit-test-secret", + headers=[(b"authorization", b"Bearer wrong")]) + self.assertEqual(status, 401) + self.assertEqual(body, b"Unauthorized") + self.assertFalse(downstream.reached) + + def test_token_without_the_bearer_scheme_is_rejected(self): + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", "s3cret"): + status, _ = call(gate, "/unit-test-secret", + headers=[(b"authorization", b"s3cret")]) + self.assertEqual(status, 401) + self.assertFalse(downstream.reached) + + def test_missing_header_is_allowed_by_design(self): + """ChatGPT connectors cannot send headers; the path is the real gate.""" + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", "s3cret"): + status, _ = call(gate, "/unit-test-secret") + self.assertEqual(status, 200) + self.assertTrue(downstream.reached) + + def test_surrounding_whitespace_is_tolerated(self): + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", "s3cret"): + status, _ = call(gate, "/unit-test-secret", + headers=[(b"authorization", b" Bearer s3cret ")]) + self.assertEqual(status, 200) + self.assertTrue(downstream.reached) + + def test_bad_token_on_a_bad_path_is_still_a_404(self): + """The path check runs first; a wrong path must not reveal the token.""" + gate, downstream = build() + with mock.patch.object(config, "AUTH_TOKEN", "s3cret"): + status, _ = call(gate, "/wrong", + headers=[(b"authorization", b"Bearer wrong")]) + self.assertEqual(status, 404) + self.assertFalse(downstream.reached) + + +if __name__ == "__main__": + unittest.main()