diff --git a/backend/src/Taskdeck.Api/Mcp/WriteTools.cs b/backend/src/Taskdeck.Api/Mcp/WriteTools.cs index 871786885..97a64c63f 100644 --- a/backend/src/Taskdeck.Api/Mcp/WriteTools.cs +++ b/backend/src/Taskdeck.Api/Mcp/WriteTools.cs @@ -5,6 +5,7 @@ using Taskdeck.Application.Interfaces; using Taskdeck.Application.Services; using Taskdeck.Application.Services.Pipeline; +using Taskdeck.Domain.Common; using Taskdeck.Domain.Entities; namespace Taskdeck.Api.Mcp; @@ -102,7 +103,7 @@ public async Task CreateCard( var canWrite = await _authorizationService.CanWriteBoardAsync(userId, boardGuid); if (!canWrite.IsSuccess) - return Error(canWrite.ErrorMessage); + return Error(canWrite); if (!canWrite.Value) return Error("Not authorized to create cards on this board"); @@ -171,7 +172,7 @@ public async Task CreateCard( var result = await _proposalService.CreateProposalAsync(dto); if (!result.IsSuccess) - return Error(result.ErrorMessage); + return Error(result); return ProposalCreated(result.Value.Id, "Proposal created. Review and approve in Taskdeck to create the card."); } @@ -230,7 +231,7 @@ public async Task MoveCard( var result = await _proposalService.CreateProposalAsync(dto); if (!result.IsSuccess) - return Error(result.ErrorMessage); + return Error(result); return ProposalCreated(result.Value.Id, "Proposal created. Review and approve in Taskdeck to move the card."); } @@ -322,7 +323,7 @@ public async Task UpdateCard( var result = await _proposalService.CreateProposalAsync(dto); if (!result.IsSuccess) - return Error(result.ErrorMessage); + return Error(result); return ProposalCreated(result.Value.Id, "Proposal created. Review and approve in Taskdeck to update the card."); } @@ -375,7 +376,7 @@ public async Task ArchiveCard( var result = await _proposalService.CreateProposalAsync(dto); if (!result.IsSuccess) - return Error(result.ErrorMessage); + return Error(result); return ProposalCreated( result.Value.Id, @@ -414,7 +415,7 @@ public async Task CreateCapture( var result = await _captureService.CreateAsync(userId, captureDto); if (!result.IsSuccess) - return Error(result.ErrorMessage); + return Error(result); return JsonSerializer.Serialize(new { @@ -447,14 +448,14 @@ public async Task CreateColumn( var canWrite = await _authorizationService.CanWriteBoardAsync(userId, boardGuid); if (!canWrite.IsSuccess) - return Error(canWrite.ErrorMessage); + return Error(canWrite); if (!canWrite.Value) return Error("Not authorized to create columns on this board"); var columns = (await _unitOfWork.Columns.GetByBoardIdAsync(boardGuid)).ToList(); var appendPositionResult = ProposalOperationContractValidator.ResolveAppendPosition(columns); if (!appendPositionResult.IsSuccess) - return Error(appendPositionResult.ErrorMessage); + return Error(appendPositionResult); var parameters = new Dictionary { @@ -487,7 +488,7 @@ public async Task CreateColumn( boardGuid, new[] { operation }); if (!contractValidation.IsSuccess) - return Error(contractValidation.ErrorMessage); + return Error(contractValidation); var dto = new CreateProposalDto( SourceType: ProposalSourceType.Manual, @@ -503,7 +504,7 @@ public async Task CreateColumn( var result = await _proposalService.CreateProposalAsync(dto); if (!result.IsSuccess) - return Error(result.ErrorMessage); + return Error(result); return ProposalCreated(result.Value.Id, "Proposal created. Review and approve in Taskdeck to create the column."); } @@ -548,6 +549,18 @@ private static string Error(string message) return JsonSerializer.Serialize(new { error = message }, BoardResources.SerializerOptions); } + /// + /// Serializes a failed application for an MCP caller. A result + /// classified UnexpectedError collapses to the stable generic failure message so + /// unknown-exception text never reaches the model; known domain messages stay specific. + /// + private static string Error(Result result) + { + return Error(SensitiveDataRedactor.SanitizeLlmFailureMessage( + result.ErrorCode, + result.ErrorMessage)); + } + private static string ProposalCreated(Guid proposalId, string message) { return JsonSerializer.Serialize(new diff --git a/backend/tests/Taskdeck.Api.Tests/WriteToolsErrorSafetyTests.cs b/backend/tests/Taskdeck.Api.Tests/WriteToolsErrorSafetyTests.cs new file mode 100644 index 000000000..2e3ea2768 --- /dev/null +++ b/backend/tests/Taskdeck.Api.Tests/WriteToolsErrorSafetyTests.cs @@ -0,0 +1,362 @@ +using System.Text.Json; +using FluentAssertions; +using Moq; +using Taskdeck.Api.Mcp; +using Taskdeck.Application.DTOs; +using Taskdeck.Application.Interfaces; +using Taskdeck.Application.Services; +using Taskdeck.Domain.Common; +using Taskdeck.Domain.Entities; +using Taskdeck.Domain.Enums; +using Taskdeck.Domain.Exceptions; +using Xunit; + +namespace Taskdeck.Api.Tests; + +/// +/// #2351 — MCP write tools must not echo unknown-exception text back to the model. +/// Every directly returned failed goes through +/// : an +/// UnexpectedError collapses to the stable generic message, while known domain, +/// validation, conflict, and authorization strings stay byte-for-byte unchanged. +/// +public class WriteToolsErrorSafetyTests +{ + private const string HostileError = + "Bearer sk-live-ABC123 C:\\Users\\alice\\AppData\\taskdeck.db " + + "SQLite Error 19: UNIQUE constraint failed: Users.Email " + + "https://provider.example/v1/internal"; + + private static readonly string[] HostileMarkers = + [ + "sk-live-ABC123", + "C:\\Users\\alice", + // JSON-escapes the backslashes, so assert the unescaped fragment too. + "alice", + "taskdeck.db", + "SQLite Error 19", + "UNIQUE constraint failed", + "https://provider.example/v1/internal" + ]; + + // ── canWrite: unexpected authorization failure ─────────────────────────── + + [Fact] + public async Task CreateCard_UnexpectedAuthorizationFailure_ReturnsGenericError() + { + var boardId = Guid.NewGuid(); + var authorization = FailingAuthorization(boardId, ErrorCodes.UnexpectedError, HostileError); + + var json = await CreateTools(authorization: authorization.Object) + .CreateCard(boardId.ToString(), "Title"); + + AssertGenericError(json); + } + + [Fact] + public async Task CreateColumn_UnexpectedAuthorizationFailure_ReturnsGenericError() + { + var boardId = Guid.NewGuid(); + var authorization = FailingAuthorization(boardId, ErrorCodes.UnexpectedError, HostileError); + + var json = await CreateTools(authorization: authorization.Object) + .CreateColumn(boardId.ToString(), "Doing"); + + AssertGenericError(json); + } + + [Fact] + public async Task CreateCard_KnownAuthorizationFailure_PreservesStableMessage() + { + const string stableMessage = "Board not found."; + var boardId = Guid.NewGuid(); + var authorization = FailingAuthorization(boardId, ErrorCodes.NotFound, stableMessage); + + var json = await CreateTools(authorization: authorization.Object) + .CreateCard(boardId.ToString(), "Title"); + + ReadError(json).Should().Be(stableMessage); + } + + // ── canWrite false: authorization literals are not Results ─────────────── + + [Fact] + public async Task CreateCard_NotAuthorized_PreservesLiteralMessage() + { + var boardId = Guid.NewGuid(); + + var json = await CreateTools(authorization: AllowingAuthorization(boardId, canWrite: false).Object) + .CreateCard(boardId.ToString(), "Title"); + + ReadError(json).Should().Be("Not authorized to create cards on this board"); + } + + [Fact] + public async Task CreateColumn_NotAuthorized_PreservesLiteralMessage() + { + var boardId = Guid.NewGuid(); + + var json = await CreateTools(authorization: AllowingAuthorization(boardId, canWrite: false).Object) + .CreateColumn(boardId.ToString(), "Doing"); + + ReadError(json).Should().Be("Not authorized to create columns on this board"); + } + + // ── proposal creation results ──────────────────────────────────────────── + + [Fact] + public async Task CreateCard_UnexpectedProposalFailure_ReturnsGenericError() + { + var boardId = Guid.NewGuid(); + var unitOfWork = UnitOfWorkWithColumns(boardId, new Column(boardId, "Todo", 0)); + + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.UnexpectedError, HostileError).Object, + unitOfWork: unitOfWork.Object, + authorization: AllowingAuthorization(boardId).Object) + .CreateCard(boardId.ToString(), "Title"); + + AssertGenericError(json); + } + + [Fact] + public async Task CreateCard_KnownProposalFailure_PreservesStableMessage() + { + const string stableMessage = "Board not found."; + var boardId = Guid.NewGuid(); + var unitOfWork = UnitOfWorkWithColumns(boardId, new Column(boardId, "Todo", 0)); + + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.NotFound, stableMessage).Object, + unitOfWork: unitOfWork.Object, + authorization: AllowingAuthorization(boardId).Object) + .CreateCard(boardId.ToString(), "Title"); + + ReadError(json).Should().Be(stableMessage); + } + + [Fact] + public async Task MoveCard_UnexpectedProposalFailure_ReturnsGenericError() + { + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.UnexpectedError, HostileError).Object) + .MoveCard(Guid.NewGuid().ToString(), Guid.NewGuid().ToString(), Guid.NewGuid().ToString()); + + AssertGenericError(json); + } + + [Fact] + public async Task MoveCard_KnownProposalFailure_PreservesStableMessage() + { + const string stableMessage = "Card not found."; + + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.NotFound, stableMessage).Object) + .MoveCard(Guid.NewGuid().ToString(), Guid.NewGuid().ToString(), Guid.NewGuid().ToString()); + + ReadError(json).Should().Be(stableMessage); + } + + [Fact] + public async Task UpdateCard_UnexpectedProposalFailure_ReturnsGenericError() + { + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.UnexpectedError, HostileError).Object) + .UpdateCard(Guid.NewGuid().ToString(), Guid.NewGuid().ToString(), title: "New title"); + + AssertGenericError(json); + } + + [Fact] + public async Task ArchiveCard_UnexpectedProposalFailure_ReturnsGenericError() + { + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.UnexpectedError, HostileError).Object) + .ArchiveCard(Guid.NewGuid().ToString(), Guid.NewGuid().ToString()); + + AssertGenericError(json); + } + + [Fact] + public async Task ArchiveCard_ForbiddenProposalFailure_PreservesStableMessage() + { + const string stableMessage = "You do not have access to this board."; + + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.Forbidden, stableMessage).Object) + .ArchiveCard(Guid.NewGuid().ToString(), Guid.NewGuid().ToString()); + + ReadError(json).Should().Be(stableMessage); + } + + [Fact] + public async Task CreateColumn_UnexpectedProposalFailure_ReturnsGenericError() + { + var boardId = Guid.NewGuid(); + var unitOfWork = UnitOfWorkWithColumns(boardId); + + var json = await CreateTools( + proposalService: FailingProposalService(ErrorCodes.UnexpectedError, HostileError).Object, + unitOfWork: unitOfWork.Object, + authorization: AllowingAuthorization(boardId).Object) + .CreateColumn(boardId.ToString(), "Doing"); + + AssertGenericError(json); + } + + // ── capture result ─────────────────────────────────────────────────────── + + [Fact] + public async Task CreateCapture_UnexpectedFailure_ReturnsGenericError() + { + var captureService = new Mock(MockBehavior.Strict); + captureService + .Setup(service => service.CreateAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(Result.Failure(ErrorCodes.UnexpectedError, HostileError)); + + var json = await CreateTools(captureService: captureService.Object).CreateCapture("An idea"); + + AssertGenericError(json); + } + + [Fact] + public async Task CreateCapture_ValidationFailure_PreservesStableMessage() + { + const string stableMessage = "Capture text cannot be empty."; + var captureService = new Mock(MockBehavior.Strict); + captureService + .Setup(service => service.CreateAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(Result.Failure(ErrorCodes.ValidationError, stableMessage)); + + var json = await CreateTools(captureService: captureService.Object).CreateCapture("An idea"); + + ReadError(json).Should().Be(stableMessage); + } + + // ── static validator results (conflict / validation) ───────────────────── + + [Fact] + public async Task CreateColumn_AppendPositionConflict_PreservesStableMessage() + { + var boardId = Guid.NewGuid(); + var unitOfWork = UnitOfWorkWithColumns(boardId, new Column(boardId, "Last", int.MaxValue)); + + var json = await CreateTools( + unitOfWork: unitOfWork.Object, + authorization: AllowingAuthorization(boardId).Object) + .CreateColumn(boardId.ToString(), "Doing"); + + ReadError(json).Should() + .Be("Cannot append a column because the board has no higher position available"); + } + + [Fact] + public async Task CreateColumn_ContractValidationFailure_PreservesStableMessage() + { + var boardId = Guid.NewGuid(); + var unitOfWork = UnitOfWorkWithColumns(boardId); + + var json = await CreateTools( + unitOfWork: unitOfWork.Object, + authorization: AllowingAuthorization(boardId).Object) + .CreateColumn(boardId.ToString(), " "); + + var error = ReadError(json); + error.Should().NotBe(SensitiveDataRedactor.GenericUnexpectedFailureMessage); + error.Should().NotBeNullOrWhiteSpace(); + } + + // ── helpers ────────────────────────────────────────────────────────────── + + private static void AssertGenericError(string json) + { + var error = ReadError(json); + error.Should().Be(SensitiveDataRedactor.GenericUnexpectedFailureMessage); + foreach (var marker in HostileMarkers) + json.Should().NotContain(marker); + } + + private static string? ReadError(string json) + { + using var document = JsonDocument.Parse(json); + return document.RootElement.GetProperty("error").GetString(); + } + + private static Mock FailingAuthorization( + Guid boardId, + string errorCode, + string errorMessage) + { + var authorization = new Mock(MockBehavior.Strict); + authorization + .Setup(service => service.CanWriteBoardAsync(It.IsAny(), boardId)) + .ReturnsAsync(Result.Failure(errorCode, errorMessage)); + return authorization; + } + + private static Mock AllowingAuthorization(Guid boardId, bool canWrite = true) + { + var authorization = new Mock(MockBehavior.Strict); + authorization + .Setup(service => service.CanWriteBoardAsync(It.IsAny(), boardId)) + .ReturnsAsync(Result.Success(canWrite)); + return authorization; + } + + private static Mock FailingProposalService( + string errorCode, + string errorMessage) + { + var proposalService = new Mock(MockBehavior.Strict); + proposalService + .Setup(service => service.CreateProposalAsync( + It.IsAny(), + It.IsAny())) + .ReturnsAsync(Result.Failure(errorCode, errorMessage)); + return proposalService; + } + + private static Mock UnitOfWorkWithColumns(Guid boardId, params Column[] columns) + { + var columnRepository = new Mock(MockBehavior.Strict); + columnRepository + .Setup(repository => repository.GetByBoardIdAsync(boardId, It.IsAny())) + .ReturnsAsync(columns); + + var unitOfWork = new Mock(MockBehavior.Strict); + unitOfWork.SetupGet(work => work.Columns).Returns(columnRepository.Object); + return unitOfWork; + } + + private static WriteTools CreateTools( + IAutomationProposalService? proposalService = null, + ICaptureService? captureService = null, + IUnitOfWork? unitOfWork = null, + IAuthorizationService? authorization = null) + { + return new WriteTools( + proposalService ?? new Mock(MockBehavior.Strict).Object, + new FixedUserContextProvider(Guid.NewGuid()), + captureService ?? new Mock(MockBehavior.Strict).Object, + unitOfWork ?? new Mock(MockBehavior.Strict).Object, + authorization ?? new Mock(MockBehavior.Strict).Object); + } + + private sealed class FixedUserContextProvider(Guid userId) : IUserContextProvider + { + public Task GetCurrentContextAsync(CancellationToken cancellationToken = default) => + Task.FromResult(new McpUserContext(userId, ApiKeyScope.Full)); + + public Task GetCurrentUserIdAsync(CancellationToken cancellationToken = default) => + Task.FromResult(userId); + + public Task GetUserIdAsync(CancellationToken cancellationToken = default) => + Task.FromResult(userId); + } +} diff --git a/docs/security/SECURITY_LOGGING_REDACTION.md b/docs/security/SECURITY_LOGGING_REDACTION.md index 659eda034..a134ce371 100644 --- a/docs/security/SECURITY_LOGGING_REDACTION.md +++ b/docs/security/SECURITY_LOGGING_REDACTION.md @@ -46,6 +46,10 @@ It applies to API middleware, SignalR transport request logging, queue/worker lo - MCP read tools apply that boundary to directly returned failed `Result` values from board detail, board listing, and card search operations. Invalid input and known domain messages stay specific; silent child-result handling and arbitrary thrown exceptions remain separate contracts. +- MCP write tools apply that boundary to directly returned failed `Result` values from board + write authorization, proposal creation, capture creation, and the column append-position and + operation-contract validators. Invalid input strings, `Not authorized ...` literals, and known + domain messages stay specific; arbitrary thrown exceptions remain a separate contract. - Capture-source validation errors use generic wording (`Invalid capture source value`) instead of reflecting the untrusted source string. - Opt-in Sentry keeps server-side exception tracking and the existing event/breadcrumb scrubbing, but does not decorate the registered OpenAI, OpenAICompatible, Ollama, or outbound-webhook clients. - The web host enforces `Warning` as the minimum for