tech-debt-backlog §7.15: extend MCP path-scrubbing regex

[jascal]

Summary

One [M]-sized item from PR #74's review log
(logs/pr-review-2026-05-27.log), targeting the MCP server's
exception-message sanitizer.

Extends _ABS_PATH_RE in q_orca/mcp_server.py with four new
alternations covering four of the five path shapes the v1 regex
missed:

• Home-relative ~/... — was partially scrubbed as
  ~<path>; now consumed whole as <path>.
• Windows UNC \\server\share\... — was completely missed
  by the v1 regex (no double-backslash anchor); now scrubbed.
• file:// URIs — was partially scrubbed as file://<path>;
  now consumed whole.
• Quoted paths-with-spaces — both single- and double-quoted
  variants. Python's stdlib FileNotFoundError quotes path
  arguments with single quotes (verified empirically with a
  bogus open() call), so '/Users/Alice Smith/foo.txt' is the
  shape that matters — not the bare unquoted form, which the
  v1 regex stopped at the first whitespace. Each quoted
  alternation requires ≥2 slash-or-backslash separators inside
  the quotes so a stray 'foo/bar' module reference is not a
  false positive.

Shape (1) from the §7.15 spec body — multi-slash numerics like
1/2/3 — is deliberately left at the v1 partial-scrub behaviour
(1<path>). Adding a digit-only multi-slash anchor would
over-scrub real dates and version strings (2025/01/15,
v1/2/3-rc4), and a bare 1/2/3 partial scrub leaks no path
information anyway. Decision documented in the regex comment
block and pinned by test_keeps_date_like_numeric_triple so a
future stricter pass doesn't silently eat dates whole.

Test plan

• pytest tests/test_mcp_server.py -q: 18 passed, 1 skipped.
• pytest -q: 1289 passed, 8 skipped (was 1282 / 8 on
  main — delta is exactly the 7 new tests).
• Seven new tests in
  TestSanitizeExceptionMessage pin one shape each plus the
  boundary cases:
  • test_replaces_home_relative_path
  • test_replaces_windows_unc_path
  • test_replaces_file_uri
  • test_replaces_quoted_path_with_spaces (single quotes)
  • test_replaces_double_quoted_path_with_spaces
  • test_keeps_short_quoted_non_path_tokens (boundary —
    'foo/bar' stays clean)
  • test_keeps_date_like_numeric_triple (boundary — dates
    stay readable)
• §7.15 marked [x] with a closing note in
  openspec/changes/tech-debt-backlog/tasks.md per the
  file's convention (§6.1).

🤖 Generated with Claude Code

[jascal]

Code Review — Claude Sonnet 4.6

Scope: security-hardening. Three files: the _ABS_PATH_RE sanitizer regex + its comment block in q_orca/mcp_server.py, seven new tests in tests/test_mcp_server.py::TestSanitizeExceptionMessage, and the [x] closure note for §7.15 in openspec/changes/tech-debt-backlog/tasks.md. Production code changed (the regex), so I held this to a higher bar than the recent docs-only siblings.

Verdict

Request a change before merge — this PR introduces a catastrophic-backtracking (ReDoS) vulnerability. The intent and the four added alternations are correct, the seven tests are well-targeted, lint is clean, and the suite is green (1289 passed / 8 skipped, exactly matching the test plan). But the two new quoted-path alternations have overlapping quantifiers, and the resulting exponential backtracking is attacker-reachable through the very tools/call error path this helper guards. The fix is a two-character-class edit per branch. Everything else is genuinely good and I'd ship it as-is once the regex is hardened. (I can't --request-changes from this automation — comment only — so treat the BLOCKER label as the verdict.)

---

🔴 BLOCKER — ReDoS in the two quoted-path alternations

The new branches are:

r'"[^"]*(?:[/\\][^"]*){2,}"'
r"|'[^']*(?:[/\\][^']*){2,}'"

Both have the classic evil-regex shape [^q]*(?:[/\\][^q]*){2,}: the separator class [/\\] is a subset of the run class [^q], so the same / can be consumed either by a run or by a separator. When the closing quote is absent, the engine must explore every way to partition the run of slashes among the {2,} iterations — exponential in the input length.

I measured it three ways. Isolated regex, then end-to-end through the public sanitize_exception_message:

name len=17  ->  sanitize() took      4.54 ms
name len=21  ->  sanitize() took     71.59 ms     (~16x per +4 chars)
name len=25  ->  sanitize() took   1151.17 ms
name len=29  ->  sanitize() took  18577.19 ms

A 29-character payload hangs the call for 18 seconds; ~37 chars is minutes; ~45 chars is effectively permanent.

This is attacker-reachable, not theoretical. The chain is fully closed:

1. call_tool's default arm raises ValueError(f"Unknown tool: {name}") — q_orca/mcp_server.py:209 — where name is params["name"], fully attacker-controlled.
2. handle_request catches it and calls sanitize_exception_message(e) — :322-327.
3. The regex runs on raw = str(exc) before the 200-char truncation (:282 runs before :283), so the _MAX_SANITIZED_LENGTH cap does not protect against this — the full malicious message reaches the regex.
4. main() reads newline-delimited requests serially in a single asyncio loop (:358-359), so one such request stalls the whole server.

So a client sends tools/call with name = "'" + "/"*40 (one unclosed quote + ~40 slashes, ~41 bytes) and the server is wedged. The irony is sharp: a PR whose entire purpose is hardening the error path against a LOW-severity info leak ships a DoS that's strictly more serious than the bug it fixes.

Fix (verified): make the run-class disjoint from the separator class so each character has exactly one home — this removes the ambiguity and makes matching linear, with no behavior change on any shape the PR pins:

r'"[^"/\\]*(?:[/\\][^"/\\]*){2,}"'
r"|'[^'/\\]*(?:[/\\][^'/\\]*){2,}'"

Measured against the same inputs:

backtracking (unclosed quote, 30 slashes):
  vuln : 73258.60 ms
  fixed:     0.0240 ms          (~3,000,000x faster)

functional equivalence on the PR's own pinned shapes:
  single-quoted space-path   vuln='<path>'         fixed='<path>'
  double-quoted space-path   vuln='x <path> y'     fixed='x <path> y'
  1-slash token survives     vuln="...'foo/bar'"   fixed="...'foo/bar'"

All three quoted-path tests (test_replaces_quoted_path_with_spaces, test_replaces_double_quoted_path_with_spaces, test_keeps_short_quoted_non_path_tokens) still pass with the fixed pattern, because the only characters that move between classes are the separators themselves — and they were always going to be consumed by the separator token anyway.

Please also add a regression test that pins the linear-time property, so a future regex tweak can't silently reintroduce the blowup, e.g.:

def test_unclosed_quote_path_is_not_pathological(self):
    # ReDoS guard: an unclosed quote followed by many separators must not
    # trigger catastrophic backtracking in the quoted-path alternations.
    import time
    exc = ValueError("Unknown tool: '" + "/" * 64)
    t0 = time.perf_counter()
    sanitize_exception_message(exc)
    assert time.perf_counter() - t0 < 1.0

Defense-in-depth (optional, independent of the fix): scrub a bounded prefix rather than the whole raw. Truncating to ~_MAX_SANITIZED_LENGTH before _ABS_PATH_RE.sub would have capped the damage of any future pathological pattern at a constant, and the output is truncated to 200 anyway — so almost no information is lost. Not a substitute for the class fix, but cheap insurance.

---

Correctness of the rest (verified, all good)

I traced each non-quoted alternation against its test and the leftmost-longest matching semantics, since the value of this PR is precision:

• file:// ordering is correct. For file:///Users/secret/foo.txt, the scan reaches f before any /, and only the file:// branch matches at that offset — so the POSIX branch never gets a chance to leave a file:// prefix behind. test_replaces_file_uri's "file://" not in out assertion confirms it. ✓
• Windows UNC \\\\[A-Za-z0-9_.\-]+(?:\\[A-Za-z0-9_.\-]+)+ correctly requires host + ≥1 segment, so \\fileserver\team-secret\spec.xlsx scrubs whole while a stray \\foo artefact does not trip. ✓
• Home-relative ~(?:/[A-Za-z0-9_.\-]+)+ consumes the ~ so nothing dangles as ~<path> — matches the comment's stated intent. ✓
• Shape (1) deferral is the right call and is pinned, not just asserted in prose: test_keeps_date_like_numeric_triple locks in that 2025-01-15 survives, so a future stricter pass that tried to eat dates whole would fail loudly. Good defensive instinct.
• The match-order quirk (quoted first) is sound: [^"]/[^'] can't cross their closing quote, so "a/b/c" ... "d/e/f" scrubs as two separate <path> tokens rather than one span. ✓

Two minor notes (non-blocking)

1. Quoted alternations scrub relative multi-slash content, unlike the bare form. The bare POSIX branch requires a leading / (absolute), but the quoted branch matches any quoted token with ≥2 separators — so 'a/b/c' or "orca/compile/v2" (relative, non-sensitive) get scrubbed to <path>, while bare a/b/c does not. Over-scrubbing is the safe direction, so this isn't a bug — but test_keeps_short_quoted_non_path_tokens only pins the 1-slash case ('foo/bar'), leaving this 2-slash false-positive boundary unspecified. If the relative-quoted case matters, consider a test documenting the intended behavior either way.
2. ~user/foo (BSD ~username) is not covered — the ~ branch requires / immediately after the tilde, and the bare branch needs ≥2 absolute segments. Out of scope for the five enumerated shapes; flagging only for completeness.

Lint (ruff)

.venv/bin/ruff check q_orca/mcp_server.py tests/test_mcp_server.py → All checks passed! The full-tree ruff check . reports the same 18 pre-existing errors confined to untouched test files (test_compiler.py, test_noise_model_section.py, test_verifier.py E402, test_qpc_convergence.py, test_examples.py — unused pytest imports + E402) flagged in the #135/#136/#137/#138 reviews. This PR adds zero lint debt and remains a good candidate for a dedicated cleanup task.

Tests

.venv/bin/pytest --tb=short -q: 1289 passed, 8 skipped, 0 failed (~21s) — matches the test plan exactly, and the +7 delta over the 1282 baseline is precisely the seven new tests. The directly-touched file is green on its own: tests/test_mcp_server.py → 18 passed, 1 skipped. The seven new tests are well-structured: one shape per test, each asserting both that the sensitive segment is gone and that <path> landed (not just one side). The gap is the adversarial axis — there's no test for malicious/pathological input, which is exactly the class of bug that slipped through here. The regression test proposed above closes it.

Security

The headline finding above. To be explicit about the trade the PR makes as written: it converts a LOW-severity info-leak (a few residual path shapes) into a DoS reachable by an unauthenticated tools/call with a ~40-byte payload. With the disjoint-class fix, the PR is a net security win — it closes four real leak shapes with no new exposure. Without it, I'd block the merge.

What's good

• Each of the four added shapes is real, correctly scoped, and individually pinned — home-relative, UNC, file://, and quoted-space — with the leftmost-match ordering (file:// and quoted-first) thought through rather than accidental.
• The shape-(1) deferral is handled with discipline: rejected with a concrete rationale (over-scrubbing dates/versions), documented in the regex comment, and locked by test_keeps_date_like_numeric_triple so the conservative behavior can't silently drift. That's exactly how to defer a known gap.
• The FileNotFoundError-quotes-with-single-quotes insight is empirically grounded (verified, per the PR body), which is why prioritizing the quoted form over bare-spaces is the right practical call — the design reasoning is sound; only the implementation of that quoted matcher has the backtracking flaw.
• Comments name the shape each alternation catches, per §7.14's convention — the regex stays readable despite growing to seven branches.

This review was posted automatically by Claude Sonnet 4.6.