fix(mcp): redact error text crossing the trust boundary — a handler exception leaked credentials by neosun100 · Pull Request #35 · aws-samples/sample-sentinel-harness

neosun100 · 2026-08-04T15:15:51Z

mcp_server.py was the lowest-covered core module at 80.3%. The uncovered lines were not incidental — they were the two error paths, and both handed an exception's text to the untrusted MCP peer verbatim.

_invoke_tool      json.dumps({"error": ..., "message": str(exc)})
_discover_tools   description = f"[LOAD ERROR: {exc}]"    <- SERVED by list_tools

Reproduced

A handler raising RuntimeError("connect failed: postgresql://svc:SUPERSECRET_PW@db.internal:5432/soc (token=ABSK_LIVE_deadbeef)") delivered the password, the token and the internal hostname to the peer:

{"error": "RuntimeError", "message": "connect failed: postgresql://svc:SUPERSECRET_PW@..."}

The second path is worse for being quieter — an import-time failure becomes a tool description broadcast over the protocol, somewhere nobody looks for secrets.

This is INV-TICKET-1 a second time

Round 20 found this shape in create_ticket (a tracker URL with a token echoed into a response message), fixed it at that one call site, and recorded "a fix applied to one call site is not an invariant" in the same commit. The redaction now lives once in _safe_error_text and both boundary paths use it.

A denylist by design. The exception type and ordinary diagnostics survive intact — a peer told only "an error occurred" cannot tell a bad argument from an outage, and an over-redacting channel gets routed around.

Hostnames are deliberately not redacted (stdio transport, operator-configured peer, and a hostname is not replayable the way a credential is). That trade-off is written down and asserted, so it is a decision rather than an accident — and the test says where to revisit it if a network transport appears.

Coverage 80% → 86%; repo total 91.85% → 92%. What remains uncovered is the mcp ImportError branch and main()/run(), which bind real stdio.

Four of my own errors, every one caught by a control

My first Authorization pattern redacted the word "Bearer" and left the JWT in the clear — caught by my own parametrised case. A redactor that removes the scheme name and keeps the credential is worse than none, because the output looks sanitised.
Mutation testing then showed "disable the Authorization rule" SURVIVING. Investigating rather than patching the test: the generic key=value rule also lists bearer, so it covered the one case I had written. The rule is not redundant — it uniquely handles Basic, SharedKey, Proxy-Authorization and bare authorization=. Added those; the mutation is now caught by 4 assertions.
Adding them immediately failed on SharedKey acct:<sig>: my scheme alternation was an allowlist, so an unlisted scheme was read as the value and the signature survived. The scheme is now matched generically — a credential redactor must not depend on having enumerated every auth scheme, since the unfamiliar ones are the risky ones.
AKIAIOSFODNN7EXAMPLE as a parametrize argument failed the secret scan — and removing it from the source did not fix it. pytest persists parametrised test IDs to .pytest_cache/v/cache/nodeids, and the scanner walks every file. So a secret-shaped value must never be a parametrize argument, not merely never appear in a committed line. The key id is now assembled at runtime inside the test body.

(I also briefly believed .pytest_cache was not gitignored — that was me misreading git check-ignore's exit code on a path I had just deleted. It is ignored; verified by recreating the directory.)

Mutations 6/6 caught

mutation
revert `_invoke_tool` to raw `str(exc)`	caught
revert the load-error description	caught
neuter `_safe_error_text`	caught
drop the length bound	caught
over-redact everything	caught
disable the Authorization rule	caught (after #2/#3 above)

Testing

3806 passed / 6 skipped in BOTH fixed and random order
MCP protocol round-trip E2E over the real in-memory transport — the redacted text verified as the peer receives it, not just from the helper
scenario E2E 25/25 · README CLI E2E 13/13 · IaC: tsc 0, 8/8 stack tests, cdk synth 9 stacks
ruff clean · both mypy gates clean · make ci green

New invariant: INV-MCP-4.

Update: CI was failing for TWO reasons, and the second was user-facing

The first CI run on this branch failed all four Pythons. Diagnosing it uncovered a
pre-existing defect larger than the one this PR started with.

1. CI installed a partial dependency list, so the MCP E2E layer never ran

ci.yml and release.yml hand-copied their test deps — five of the test extra's nine
entries. The two omitted were mcp and anyio[trio], and test_mcp_protocol.py
opens with importorskip("mcp"). The entire MCP protocol E2E layer skipped on every CI
run: 7 tests covering the one surface an untrusted peer reaches. CI reported green.

Reproduced in a uv project pinned to CI's exact dep list. Local showed 6 skips, CI showed
12, and nothing compared the two numbers. Fixed at the source — pip install -e ".[test]"
cannot drift from pyproject.toml because it IS pyproject.toml — plus a post-install
python -c "import mcp, anyio, ..." so a resolver failure FAILS instead of degrading to a
skip. ruff stays separately pinned so the lint verdict is byte-identical local vs CI.

2. `pip install sentinel-harness[mcp]` could not start the server

With mcp finally installed, 9 tests failed for real. pyproject.toml said mcp>=1.0
unbounded, and mcp 2.0.0 removed Server.list_tools() / Server.call_tool() — the two
decorators create_server() registers its handlers with:

mcp 1.28.1   Server("x").list_tools   -> present
mcp 2.0.0    Server("x").list_tools   -> AttributeError

Verified against a real 2.0.0 install: create_server() raises
AttributeError: 'Server' object has no attribute 'list_tools'. So anyone running
pip install sentinel-harness[mcp] && sentinel mcp serve today got a server that could not
start. Now capped mcp>=1.0,<2 in both extras; a fresh resolve gives 1.29.0 and
create_server() succeeds.

The silent skip was not hiding a stale test — it was hiding a broken published dependency
contract. Those failures were latent from the day mcp 2.0 shipped, and nothing could
surface them because the tests that check the contract never ran.

Three errors of mine, each caught by a control rather than by a reviewer

My new async test file carried neither half of its sibling's convention — no
module-level importorskip, no local anyio_backend fixture. Omitting the fixture does
not say "fixture not found"; pytest reports async def functions are not natively supported and fails, and an importorskip inside the body never runs. That is this PR's
own subject a third time, landing on a testing CONVENTION — so it is now a check with a
positive control, not a comment.
"An import check is not a compatibility check." I concluded mcp 2.0 compatibility
because all three imports in mcp_server.py still resolve, and acted on it — wrote a
version-tolerant shim, and argued in a commit message that pinning would be "the tail
wagging the dog". The shim did not fix the failures. An import resolving proves a module
attribute exists, not that the API behind it has the same shape. The guard now CALLS the
decorator surface.
A guard I wrote passed while verifying nothing. Syncing counts revealed
FIDELITY-REPORT.md claiming "137 test files" against a real 157. I added a guard reusing
_TEST_COUNT_TOLERANCE = 60 — sensible as 1.6% of ~3800 tests, but ±38% of 157 files.
The mutation "revert 157 → 137" SURVIVED while the run reported 9 passed. A tolerance is
calibrated to a magnitude; borrowing one across two magnitudes yields a check that runs
and confirms nothing. Now ±3.

Had I trusted the green run instead of mutating it, I would have shipped a decorative
guard in the very commit about guards that only look like they work.

Also

8 inline imports of the SDK session helper collapsed into tests/mcp_session.py
removed dead _create_session (defined, never called, sole user of raise_exceptions=True)
the 2.x fallback in the shim was deliberately not kept — unreachable under the pin, and
code excluded from ever running is code nobody maintains and everybody trusts

Verification

3817 passed / 6 skipped locally in BOTH fixed and random order; all 14 CI checks green
CI now installs mcp 1.29.0 and RUNS the layer (was: skipped it entirely)
CI's 11 skips vs local 6 accounted for: 3 in test_coverage_doc.py (needs a .coverage
file from a prior run — in CI it is being written by the enclosing run) + 2 in
test_lazy_clients.py. Both carry honest reason strings. Left as-is and stated rather than
quietly ignored, since an unexplained local/CI skip delta is exactly what hid defect 1.
mutations 8/8 then 4/4 caught, controls valid in both harnesses

New invariants: INV-CI-1, INV-CI-2, INV-MCP-5.

…xception leaked credentials `mcp_server.py` was the lowest-covered core module at 80.3%. The uncovered lines were not incidental: they were the two ERROR paths, and both handed an exception's text to the untrusted MCP peer verbatim. _invoke_tool json.dumps({"error": ..., "message": str(exc)}) _discover_tools description = f"[LOAD ERROR: {exc}]" <- SERVED by list_tools Reproduced. A handler raising RuntimeError("connect failed: postgresql://svc:SUPERSECRET_PW@db.internal:5432/soc (token=ABSK_LIVE_deadbeef)") delivered the password, the token and the internal hostname to the peer: {"error": "RuntimeError", "message": "connect failed: postgresql://svc:SUPERSECRET_PW@..."} The second path is worse for being quieter — an import-time failure becomes a tool DESCRIPTION broadcast over the protocol, somewhere nobody looks for secrets. This is INV-TICKET-1's shape a SECOND time. Round 20 found it in `create_ticket` (a tracker URL with a token echoed into a response `message`), fixed it at that one call site, and recorded "a fix applied to one call site is not an invariant" in the same commit. So the redaction now lives once in `_safe_error_text` and both boundary paths use it. A DENYLIST by design. The exception TYPE and ordinary diagnostics survive intact — a peer told only "an error occurred" cannot tell a bad argument from an outage, and an over-redacting channel gets routed around. Hostnames are deliberately NOT redacted (stdio transport, an operator-configured peer, and a hostname is not replayable the way a credential is); that trade-off is written down AND asserted, so it is a decision rather than an accident, and the test says where to revisit it if a network transport ever appears. Coverage 80% -> 86%; repo total 91.85% -> 92%. The lines still uncovered are the `mcp` ImportError branch and `main()`/`run()`, which bind real stdio. FOUR of my own errors, every one caught by a control rather than by review: 1. My first Authorization pattern redacted the word "Bearer" and left the JWT in the clear — caught by my own parametrised case. A redactor that removes the scheme name and keeps the credential is worse than none, because the output LOOKS sanitised. 2. Mutation testing then showed "disable the Authorization rule" SURVIVING. Investigating rather than patching the test: the generic key=value rule also lists "bearer", so it covered the one case I had written. The rule is NOT redundant — it uniquely handles `Basic`, `SharedKey`, `Proxy-Authorization` and bare `authorization=`. Added those; the mutation is now caught by 4 assertions. 3. Adding them immediately failed on `SharedKey acct:<sig>`: my scheme alternation was an allowlist, so an unlisted scheme was read as the value and the signature survived. The scheme is now matched generically — a credential redactor must not depend on having enumerated every auth scheme, since the unfamiliar ones are the risky ones. 4. `AKIAIOSFODNN7EXAMPLE` as a parametrize argument failed the secret scan. Removing it from the source did NOT fix it: pytest persists parametrised test IDs to `.pytest_cache/v/cache/nodeids`, and the scanner walks every file. So a secret-shaped value must never be a parametrize ARGUMENT, not merely never appear in a committed line. The key id is now assembled at runtime inside the test body. (I also briefly believed `.pytest_cache` was not gitignored — that was me misreading `git check-ignore`'s exit code on a path I had just deleted. It is ignored; verified by recreating the directory.) Mutations 6/6 caught: reverting either boundary path to raw `str(exc)`, neutering the redactor, dropping the length bound, over-redacting everything, and disabling the Authorization rule. Tested: 3806 passed / 6 skipped in BOTH fixed and random order; MCP protocol round-trip E2E over the real in-memory transport (a raising tool's redacted text verified as the peer receives it, not just from the helper); scenario E2E 25/25; README CLI E2E 13/13; IaC tsc 0 / 8-of-8 / synth 9 stacks; ruff clean; both mypy gates clean; make ci green. INV-MCP-4

… in CI PR #35's CI failed on all four Pythons while passing locally. The failure was mine, but diagnosing it uncovered a larger pre-existing defect underneath. ## Defect B (pre-existing, the serious one): CI installed a partial dep list `ci.yml` and `release.yml` both installed their test deps as a hand-copied list: pip install -e . pip install pytest pytest-randomly coverage ruff==0.15.20 hypothesis That is five of the `test` extra's nine entries. The two omitted were **`mcp`** and **`anyio[trio]`** — and `tests/test_mcp_protocol.py` opens with a module-level `importorskip("mcp")`. So on every CI run the ENTIRE MCP protocol E2E layer skipped: 7 tests covering the one surface an untrusted MCP peer reaches. CI reported green. Reproduced, not inferred: a throwaway uv project pinned to CI's exact dependency list gives `mcp: ABSENT`, `anyio: ABSENT`, and `test_mcp_protocol.py` collapsing to `1 skipped`. Local showed **6 skips**, CI showed **12**, and nothing compared the two numbers — which is the whole reason this survived. A skip is the one outcome that looks identical whether the code is fine, the test is broken, or the test never existed. I have reported "MCP protocol E2E green" in several prior rounds. That was true locally and false in CI. Fixed at the source rather than the symptom — adding `mcp` to the list would leave the second source of truth in place and the next dependency would drift the same way: - `pip install -e ".[test]"` cannot drift from pyproject.toml because it IS pyproject.toml - the install step ends with `python -c "import mcp, anyio, trio, ..."`, so a resolver hiccup FAILS the job instead of degrading to a skip - `ruff` stays separate and exactly pinned, so the lint verdict is byte-identical between pre-commit, `make ci` and CI ## Defect A (mine): a new async test file carried neither half of the convention `test_mcp_protocol.py` does two things to make async tests work — a module-level `importorskip("mcp")` AND a local `anyio_backend` fixture. I wrote a sibling file and carried neither. Omitting the fixture does not give "fixture not found": pytest reports `async def functions are not natively supported` and FAILS, and the `importorskip` I put inside the test body never gets to run. It passed locally only because `mcp`/`anyio` are installed here. Which is **this PR's own subject a third time** — "a fix applied to one call site is not an invariant" — now landing on a testing CONVENTION. So it is a check, not a comment: `test_every_async_test_file_pins_a_backend` scans every file using `pytest.mark.anyio`, and fails its own positive control if it finds zero. The fixture must be local, not inherited from anyio's plugin, whose `anyio_backend` is parametrised over every installed backend and would silently run each async test twice. ## A third finding, and a mutation that caught me Syncing the test counts showed `docs/FIDELITY-REPORT.md` claiming "across **137** test files" against a real **157** — 13% understated. `test_docs_drift.py` guards the test COUNT and caught every drift in it, but never captured the file count in the same sentence. Same shape again, at the level of a claim. My first version of that new guard reused `_TEST_COUNT_TOLERANCE = 60`, and the mutation "revert 157 back to 137" **SURVIVED**: ±60 is 1.6% of ~3800 tests (sensible) but ±38% of 157 files, so the guard accepted almost any number while reporting `9 passed`. A tolerance is calibrated to a magnitude; borrowing one across two magnitudes in the same file produces a check that runs and verifies nothing. Now ±3, and the mutation is caught. Had I trusted the green run instead of mutating it, I would have shipped a decorative guard in the very commit whose subject is guards that only look like they work. ## Mutations 8/8 caught, control valid | mutation | | |---|---| | ci.yml reverts to the hand-copied dep list | caught | | release.yml reverts to the hand-copied dep list | caught | | ci.yml drops the post-install import assertion | caught | | ruff loses its exact pin | caught | | the async test file loses its `anyio_backend` fixture | caught | | FIDELITY file count 157 -> 137 | caught *(after the tolerance fix)* | | ROADMAP file count 157 -> 137 | caught | | the file-count claim is deleted entirely (positive control) | caught | ## Testing - **3812 passed / 6 skipped in BOTH fixed and random order** - verified in a REPRODUCED CI environment (uv project with CI's exact deps): the async tests now degrade to a visible SKIP rather than a failure, and the guard still runs - E2E layers: scenario 25/25 · README CLI 13/13 · MCP protocol + redaction 74 combined - `make ci` green · both mypy gates clean · ruff clean · secret scan clean - counts synced 3806 -> 3812 and the stale 137 -> 157 across README / FIDELITY-REPORT / ROADMAP / README-coverage New invariants: **INV-CI-1**, **INV-CI-2**.

…t start the server With CI finally installing the `test` extra (previous commit), the MCP protocol tests ran for the first time — and 9 of them failed. The failures were real, and they exposed a user-facing defect that the silent skip had been hiding all along. ## `pip install sentinel-harness[mcp]` was broken on the current PyPI release `pyproject.toml` declared `mcp>=1.0` with no upper bound. mcp 2.0.0 is a breaking rewrite of the low-level server API this package is built on: mcp 1.28.1 Server("x").list_tools -> present mcp 2.0.0 Server("x").list_tools -> AttributeError `create_server()` registers both handlers with `@server.list_tools()` and `@server.call_tool()`. Verified against a real 2.0.0 install: AttributeError: 'Server' object has no attribute 'list_tools' So `pip install sentinel-harness[mcp] && sentinel mcp serve` **could not start at all** for anyone installing today. Not a test artifact — an install-time break on the default version a new user gets. Now `mcp>=1.0,<2` in BOTH the `mcp` and `test` extras. Verified end to end: a fresh resolve of `sentinel-harness[mcp]` gives mcp 1.29.0 and `create_server()` succeeds. ## What the silent skip was actually hiding CI never installed `mcp`, so `tests/test_mcp_protocol.py`'s `importorskip("mcp")` fired on every run and the whole layer skipped. The skip was not concealing a stale test — it was concealing a **broken published dependency contract**. Those 9 failures were latent from the day mcp 2.0 shipped; nothing could surface them because the tests that check the contract never ran. This is the strongest case in this repo for "a skip must never be read as a pass". ## An error of mine worth recording: an import check is not a compatibility check My first diagnosis was that production code was 2.0-compatible and only a test helper had moved, because all three imports in `mcp_server.py` still resolve on 2.0.0. I acted on that: wrote a version-tolerant shim rebuilding the removed `create_connected_server_and_client_session` from primitives that survive in 2.x, and concluded pinning would be "the tail wagging the dog". The shim did not fix the failures. `from mcp.server import Server` resolving proves a module attribute exists — it says nothing about the shape of the API behind it. Compatibility has to be probed by CALLING the surface, which is what `test_the_code_still_depends_on_the_1x_decorator_api` now does. Had I trusted the import check, I would have shipped an unbounded dependency that cannot start. ## Other changes - **8 inline imports -> 1**: both test files imported the SDK's session helper at eight separate call sites. They now route through `tests/mcp_session.py`, so the next SDK break is fixed in one place. The 2.x fallback is deliberately NOT kept: with `<2` pinned it is unreachable, and code excluded from ever running is code nobody maintains and everybody trusts. - **removed dead `_create_session`** from `test_mcp_protocol.py` — defined, never called, and the only user of `raise_exceptions=True`. - the guard asserts the bound's PREMISE, so the pin is lifted deliberately when the code is ported rather than lingering as a constraint nobody dares touch. To lift it, port to 2.0's request-handler API and re-verify with the command in the pyproject comment. ## Mutations 4/4 caught, control valid | mutation | | |---|---| | the `mcp` extra loses its upper bound | caught | | the `test` extra loses its upper bound | caught | | the two extras disagree on the bound | caught | | mcp_server stops using `@server.list_tools()` | caught | ## Testing - **3817 passed / 6 skipped in BOTH fixed and random order** - verified in a reproduced mcp 2.0.0 environment (the failure mode) AND a fresh `sentinel-harness[mcp]` resolve (the fix): 1.29.0, `create_server()` OK - 61 MCP tests green · `make ci` green · both mypy gates clean · ruff clean - `uv lock --check` clean · counts synced to 3817 / 158 files New invariant: **INV-MCP-5**.

…laptops (#36) `tests/test_coverage_doc.py` re-measures every figure in `tests/README-coverage.md`. It was written after five of that table's rows were found wrong by 16 to 61 points, every one of them understating the truth. It is the only thing keeping those numbers honest. **It never ran in CI.** CI's test step is `coverage run -m pytest tests`, and coverage writes `.coverage` only when it EXITS — so while the suite is running there is no data file, and those three assertions called `pytest.skip`. They executed on maintainer laptops, where `make ci` had already produced the file, and skipped on every CI run. The guard that keeps the coverage doc honest was only ever verified on the machine of the person who might have let it drift. Measured, not inferred: replicating CI's exact invocation locally reproduces `SKIPPED [3]`. This is INV-CI-1's shape a second time — a check that silently no-ops precisely where it matters — which I flagged in #35 as deserving its own change rather than being bolted on. ## The fix is two-part, because part 1 alone would decay 1. `ci.yml` runs the module as a dedicated step AFTER `coverage report`, when the data exists. 2. That step sets `SENTINEL_REQUIRE_COVERAGE_DATA=1`, under which absent or stale data RAISES instead of skipping. Without part 2 a later change to the data-file path would quietly restore the no-op and the new step would still report green — fixing the symptom while leaving the failure mode intact. A dedicated step that is still allowed to skip is the same failure wearing a different hat. Both unavailable-data paths (missing/stale file, and an unresolvable `coverage` launcher) now route through ONE `_unavailable()` helper, and the guard asserts there is at most one bare `pytest.skip(` left in the module. "A fix applied to one call site is not an invariant" — and I had read that launcher skip as a success three times before it became an assertion. Local developer experience is unchanged: without the flag, a missing `.coverage` still gives the friendly skip with instructions. Over-strictness gets routed around. ## A negative result worth recording Before fixing this I asked the broader question — how many guards in this suite pass vacuously? — and mechanised it: an AST scan for tests that discover a collection, assert it is empty, and never assert it is non-empty. First pass flagged 13. **All 13 were false positives**, and inspecting them taught me my scanner did not understand two legitimate control forms: a `@pytest.mark.parametrize` driving the iteration (an empty set collapses to zero collected tests, which is visible), and a sibling assertion pinning the collection's size. After teaching it both, the count was **0**. So I positive-controlled the scanner itself: planted a deliberately vacuous guard in `tests/` and confirmed it was caught, then removed it. The scanner works and the finding is genuinely zero — this category is clean and needs no change. A scan finding nothing is indistinguishable from a broken scan, so the control is what makes the zero worth reporting. ## Mutations 5/5 caught, control valid | mutation | | |---|---| | ci.yml drops the dedicated coverage-doc step | caught | | the step stops setting the require flag | caught | | the flag is set to a falsey value | caught | | the helper stops raising (skip restored) | caught | | a second bare `pytest.skip` reappears | caught | The falsey-value mutation matters: `bool("false")` is True in Python and this repo has recorded that trap three times (INV-COERCE), so the flag's parsing is asserted both ways — truthy spellings raise, falsey spellings still skip. ## Testing - **3830 passed / 6 skipped in BOTH fixed and random order** - **E2E: the full CI three-step sequence replicated locally** — `coverage run -m pytest` → `coverage report --fail-under=88` (92%) → the new step, which reports **6 passed, ZERO skipped**. That is the assertion this whole change is about, verified as CI will run it. - all four flag/data combinations verified: data+flag passes, no-data+flag FAILS, no-data without flag skips, data without flag passes - `make ci` green · both mypy gates clean · ruff clean · secret scan clean - `README-coverage.md` now states the 9-vs-6 skip delta and WHY, instead of quoting a number that only held under one invocation New invariant: **INV-DOC-5**.

neosun100 added 3 commits August 4, 2026 23:15

neosun100 merged commit 50c98f2 into main Aug 4, 2026
14 checks passed

neosun100 mentioned this pull request Aug 5, 2026

fix(ci): the coverage-doc guard never ran in CI — only on maintainer laptops #36

Merged

Navigation Menu

Uh oh!

fix(mcp): redact error text crossing the trust boundary — a handler exception leaked credentials - #35

fix(mcp): redact error text crossing the trust boundary — a handler exception leaked credentials#35
neosun100 merged 3 commits into
mainfrom
fix/mcp-error-redaction

neosun100 commented Aug 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

neosun100 commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reproduced

This is INV-TICKET-1 a second time

Four of my own errors, every one caught by a control

Mutations 6/6 caught

Testing

Update: CI was failing for TWO reasons, and the second was user-facing

1. CI installed a partial dependency list, so the MCP E2E layer never ran

2. pip install sentinel-harness[mcp] could not start the server

Three errors of mine, each caught by a control rather than by a reviewer

Also

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

neosun100 commented Aug 4, 2026 •

edited

Loading

2. `pip install sentinel-harness[mcp]` could not start the server