Skip to content

Sanitize MCP write tool failure responses - #2467

Merged
Chris0Jeky merged 2 commits into
mainfrom
issue-2351/mcp-write-tools-error-safety
Sep 4, 2026
Merged

Sanitize MCP write tool failure responses#2467
Chris0Jeky merged 2 commits into
mainfrom
issue-2351/mcp-write-tools-error-safety

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

WriteTools returned ten failed application Result values straight into the MCP JSON error
field via Error(x.ErrorMessage). When a result was classified UnexpectedError, raw exception
text — secrets, absolute Windows paths, SQLite constraint detail, provider URLs — reached the
model verbatim.

This routes all ten through a new private Error(Result) overload that applies
SensitiveDataRedactor.SanitizeLlmFailureMessage(result.ErrorCode, result.ErrorMessage), matching
the pattern already shipped for ReadTools (#2459), ProposalTools (#2432), ProposalResources
(#2436), and CaptureResources/BoardResources (#2443).

Changed return paths (all in backend/src/Taskdeck.Api/Mcp/WriteTools.cs):

  • canWriteCreateCard, CreateColumn (2)
  • result (proposal creation) — CreateCard, MoveCard, UpdateCard, ArchiveCard,
    CreateColumn (5)
  • result (capture creation) — CreateCapture (1)
  • appendPositionResultCreateColumn (1)
  • contractValidationCreateColumn (1)

An UnexpectedError result now yields exactly
SensitiveDataRedactor.GenericUnexpectedFailureMessage. Invalid-input strings, the
Not authorized ... literals, and NotFound/Forbidden/validation/conflict/known-domain messages
are unchanged byte-for-byte. No DTO shape, logging, or authorization-ordering change.

Part of #2351. This PR intentionally does not close the umbrella issue.

Verification

  • dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~WriteToolsErrorSafetyTests|FullyQualifiedName~McpToolsTests|FullyQualifiedName~ReadToolsErrorSafetyTests"58 passed, 0 failed, 0 skipped (run before the final marker tweak; superseded by the full run below).
  • dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --no-build (full API project) — 2,808 passed, 1 failed, 4 skipped, 2,813 total. The single failure is the pre-existing BatchExecuteProposalsApiTests.ExecuteProposals_MissingAndUnreadableIds_HaveMatchingWarmedCommandShapes command-shape flake tracked in [Backend][Testing] Isolate intermittent batch command-shape samples on Windows #2399 (fix in PR Fix API command-shape test isolation #2454); it is unrelated to this diff. Rerun in isolation: 1 passed, 0 failed.
  • node scripts/check-docs-governance.mjsDocs governance check passed.
  • git diff --check — clean.

New test file backend/tests/Taskdeck.Api.Tests/WriteToolsErrorSafetyTests.cs (17 facts) covers
every changed return path: hostile UnexpectedError payloads containing sk-live-ABC123, a
C:\Users\alice\AppData\taskdeck.db path, SQLite Error 19: UNIQUE constraint failed, and
https://provider.example/v1/internal are asserted absent from the response with the generic
message present; known-domain NotFound/Forbidden/ValidationError failures and the two
Not authorized ... literals are asserted preserved exactly; the append-position Conflict
message is asserted preserved verbatim.

Documentation

One bullet appended to the "Runtime Enforcement" section of
docs/security/SECURITY_LOGGING_REDACTION.md recording the WriteTools boundary, in the same style
as the existing MCP bullets. Append-only; no restructuring. No shipped-reality, sequencing, or
architectural-decision change, so docs/STATUS.md, docs/IMPLEMENTATION_MASTERPLAN.md, and
docs/decisions/ are untouched.

Not verified

No live MCP stdio/HTTP session was exercised against a real provider, and no
frontend/E2E/full-solution (backend/Taskdeck.sln) run was performed — the change is confined to
one Api file plus a new test file, and CI repeats the broader suites. The focused-check filter list
inside SECURITY_LOGGING_REDACTION.md was deliberately not extended (append-only scope), so the
new test class is not named there.

Route the ten directly returned failed Results in WriteTools through a private Error(Result) overload that applies SensitiveDataRedactor.SanitizeLlmFailureMessage, so an UnexpectedError collapses to the generic message while known domain, validation, conflict, and authorization strings stay unchanged. Part of #2351.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context independent review at exact head 93707e0 against base 350f595.

Verdict: no CRITICAL or HIGH correctness, security, or data-loss finding. Merge-blocking: none.

Checked: overload binding of Error(Result) versus Error(string) at every call site (Result<T> derives from Result, no implicit string conversion); completeness of the ten direct failed-Result returns; the remaining Error(string) sites (dueDateError from the static parameter parser and the label_ids not found on board string, which is built only from caller-supplied parsed GUIDs); Redact() behavior on every reachable known-domain message from the contract validator, capture, proposal, and authorization services (no message matches a redaction rule, so known-domain text is byte-for-byte preserved); existing McpToolsTests assertions; three-file diff hygiene.

LOW, declined on this thread (coverage notes, no defect, no commit):

  • The C:\Users\alice marker in WriteToolsErrorSafetyTests can never fire because JSON escapes backslashes; the bare alice marker on the next line already catches the leak.
  • The appendPositionResult and contractValidation sites have only preservation tests; both producers are static validators that never emit UnexpectedError, so there is no concrete leak scenario to pin.
  • The doc bullet lists the literal Error(string) sites but not the GUID-only label_ids not found on board string; incomplete, not false.

Remaining gate: exact-head hosted ci-required green and the three-minute aging floor, then a merge commit. Part of #2351; the umbrella stays open.

@Chris0Jeky
Chris0Jeky merged commit a1ed795 into main Sep 4, 2026
35 checks passed
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 4, 2026
@Chris0Jeky
Chris0Jeky deleted the issue-2351/mcp-write-tools-error-safety branch September 6, 2026 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant