Generalize webhook persisted failures and align ProposalTools redaction - #2474
Conversation
|
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 c9e8d31. Verdict: no CRITICAL or HIGH correctness, security, or data-loss finding. Merge-blocking: none. Checked: the whole webhook delivery loop (only the persisted errorMessage argument changed; MarkFailure retry/backoff, dead-letter transition, and SaveChanges are byte-identical; HTTP-status, invalid-URI, and shutdown-cancellation arms still return or match before the generic arm; a client timeout with an uncancelled token already fell into the generic arm before this PR); single logging (the loop-level LogError cannot fire for a swallowed exception and the sibling arms are mutually exclusive); the log passes SummarizeException(ex), consistent with every other worker and the SEC-14 policy; every known-domain message reachable from the three ProposalTools tools was enumerated and none collides with a redaction rule; the webhook test asserts marker absence on the persisted field and the ProposalTools test fails without the fix. LOW, declined on this thread (no commit): the now-unused using Taskdeck.Domain.Exceptions in ProposalTools.cs; the generic constant's 'correlation ID' wording versus the DeliveryId the webhook log line carries (pre-existing to the constant, tracked implicitly under the #2351 residual about two generic strings). 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
Closes two residuals (R1, R2) recorded by the unknown-exception surface inventory in #2470. Both
bring a surface into line with the shipped boundary rule: an unknown exception contributes no
text to any user-visible or persisted value, while deliberate domain messages stay useful.
Per-residual disposition
R1 — persisted webhook delivery failure text was redacted, not generalized.
backend/src/Taskdeck.Api/Workers/OutboundWebhookDeliveryWorker.cspersistedRedact($"Webhook delivery threw {ex.GetType().Name}: {ex.Message}")intoOutboundWebhookDelivery.LastErrorMessage.SensitiveDataRedactor.Redactis pattern-based, so anyexception text matching no rule (Windows paths, SQLite constraint text, provider URLs, stack detail)
was stored verbatim. The
catch (Exception ...)arm that is still inProcessingnow passesSensitiveDataRedactor.GenericUnexpectedFailureMessagedirectly toMarkFailure, and logs theoriginal exception exactly once through the existing
_loggeras a redactedSummarizeExceptionsummary carrying the existing
DeliveryIdidentifier — no new correlation scheme, no double log.Retry/backoff and the dead-letter vs retry-scheduled transitions are unchanged (
MarkFailurewasnot touched). All deliberate arms keep their existing stable messages: endpoint HTTP status, invalid
URI, disallowed scheme, blocked host, inactive subscription, shutdown requeue.
The assignment uses the constant directly in the call expression, so the
scripts/check-unknown-exception-boundary.mjsguard from #2470 will accept this file when it isadded to
PERSISTED_STATE_FILES— no allowlist entry needed. The guard script itself is not addedhere; it lives on the other branch.
R2 —
ProposalTools.Error(Result)diverged from the other MCP tools.backend/src/Taskdeck.Api/Mcp/ProposalTools.cshandledUnexpectedErrorcorrectly but returnedresult.ErrorMessageverbatim for every other code, unlikeReadTools.cs/WriteTools.cs, whichroute through
SensitiveDataRedactor.SanitizeLlmFailureMessage. It now calls the same helper, sobehavior is identical across the MCP tool surface: generic constant for
UnexpectedError,Redact()otherwise.ProposalResources.cswas checked for the same asymmetry: itsPublicFailureMessage(Result)helperalready uses
SanitizeLlmFailureMessage, so no divergence exists there. (Its separatefailureReason = p.FailureReasonreplay at line 135 is inventory row R3 — legacy stored text — andis deliberately out of scope for this PR.)
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~OutboundWebhookDeliveryWorkerTests|FullyQualifiedName~ProposalToolsErrorSafetyTests|FullyQualifiedName~McpToolsTests"— 53 passed, 0 failed, 0 skipped.dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --no-build— 2812 passed, 0 failed, 4 skipped (2816 total), 6 m 1 s. No flakes.node scripts/check-docs-governance.mjs— passed.git diff --check— clean.New / changed regressions:
ProcessDueDeliveriesAsync_ShouldGeneralizePersistedFailureMessage_WhenDispatchThrowsDuringProcessing(rewritten from the old redaction assertion): a synthetic exception carrying a bearer token, a
token=value, a capture payload, a Windows path, a SQLiteUNIQUE constraint failedstring and aprovider URL.
LastErrorMessageequals the generic constant and contains none of those markers,and exactly one Error log entry is emitted containing the exception type and the delivery ID.
ProcessDueDeliveriesAsync_ShouldKeepStableStatusMessage_WhenEndpointReturnsHttp500: a deliberateHTTP 500 keeps
Webhook endpoint returned HTTP 500.andLastResponseStatusCode == 500.ProcessDueDeliveriesAsync_ShouldGeneralizePersistedFailureMessage_WhenDispatchTimesOut: a clienttimeout persists the generic constant, leaks no provider host, and logs exactly once.
GetProposalStatus_KnownDomainFailureWithRedactablePattern_IsRedacted: aValidationErrormessagecontaining
token=abc123is returned with the token redacted and its stable prefix intact, whilethe existing
NotFoundtest proves an ordinary domain message passes through unchanged.Documentation
Two append-only bullets added to the Runtime Enforcement section of
docs/security/SECURITY_LOGGING_REDACTION.md(webhook persisted failure; ProposalTools alignment).docs/STATUS.mduntouched. The inventory doc rows R1/R2 live onissue-2351/surface-inventory-guard(#2470) and are reconciled by the coordinator after both land.Not verified
exercised through the existing fake repository/unit-of-work harness only.
outside
Taskdeck.Apiand its test project were changed.compatibility is argued from its documented
PERSISTED_STATE_FILESrule, not measured.LastErrorMessagerows written before this change are not backfilled.