Sanitize MCP write tool failure responses - #2467
Conversation
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 LOW, declined on this thread (coverage notes, no defect, no commit):
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. |
Summary
WriteToolsreturned ten failed applicationResultvalues straight into the MCP JSONerrorfield via
Error(x.ErrorMessage). When a result was classifiedUnexpectedError, raw exceptiontext — 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 appliesSensitiveDataRedactor.SanitizeLlmFailureMessage(result.ErrorCode, result.ErrorMessage), matchingthe 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):canWrite—CreateCard,CreateColumn(2)result(proposal creation) —CreateCard,MoveCard,UpdateCard,ArchiveCard,CreateColumn(5)result(capture creation) —CreateCapture(1)appendPositionResult—CreateColumn(1)contractValidation—CreateColumn(1)An
UnexpectedErrorresult now yields exactlySensitiveDataRedactor.GenericUnexpectedFailureMessage. Invalid-input strings, theNot authorized ...literals, and NotFound/Forbidden/validation/conflict/known-domain messagesare 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-existingBatchExecuteProposalsApiTests.ExecuteProposals_MissingAndUnreadableIds_HaveMatchingWarmedCommandShapescommand-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.mjs—Docs governance check passed.git diff --check— clean.New test file
backend/tests/Taskdeck.Api.Tests/WriteToolsErrorSafetyTests.cs(17 facts) coversevery changed return path: hostile
UnexpectedErrorpayloads containingsk-live-ABC123, aC:\Users\alice\AppData\taskdeck.dbpath,SQLite Error 19: UNIQUE constraint failed, andhttps://provider.example/v1/internalare asserted absent from the response with the genericmessage present; known-domain
NotFound/Forbidden/ValidationErrorfailures and the twoNot authorized ...literals are asserted preserved exactly; the append-positionConflictmessage is asserted preserved verbatim.
Documentation
One bullet appended to the "Runtime Enforcement" section of
docs/security/SECURITY_LOGGING_REDACTION.mdrecording the WriteTools boundary, in the same styleas the existing MCP bullets. Append-only; no restructuring. No shipped-reality, sequencing, or
architectural-decision change, so
docs/STATUS.md,docs/IMPLEMENTATION_MASTERPLAN.md, anddocs/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 toone Api file plus a new test file, and CI repeats the broader suites. The focused-check filter list
inside
SECURITY_LOGGING_REDACTION.mdwas deliberately not extended (append-only scope), so thenew test class is not named there.