From 88cf0f8c46c5c629c24eb5c3261f394ab9a78839 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Wed, 8 Apr 2026 03:01:31 +0100 Subject: [PATCH] Fix CSV injection via embedded newlines, forward CancellationToken, surface export errors in UI - SanitizeCsvField now strips dangerous leading chars from each embedded line, not just the first line, preventing formula injection via multi-line cell values (e.g. "hello\n=CMD|'/C calc'!A0") - ExportBoardMetrics controller action forwards CancellationToken from HttpContext.RequestAborted to the export service - Replace inefficient .Concat().ToArray() BOM prepend with direct array copy - Frontend exportCsv() now shows toast error on failure instead of silently swallowing exceptions - Add 5 new test cases for embedded-newline CSV injection vectors --- .../Controllers/MetricsController.cs | 6 ++- .../Services/MetricsExportService.cs | 54 +++++++++++++++---- .../Services/MetricsExportServiceTests.cs | 11 ++++ .../taskdeck-web/src/views/MetricsView.vue | 8 ++- 4 files changed, 64 insertions(+), 15 deletions(-) diff --git a/backend/src/Taskdeck.Api/Controllers/MetricsController.cs b/backend/src/Taskdeck.Api/Controllers/MetricsController.cs index 65f84aa6b..2d01956c5 100644 --- a/backend/src/Taskdeck.Api/Controllers/MetricsController.cs +++ b/backend/src/Taskdeck.Api/Controllers/MetricsController.cs @@ -74,6 +74,7 @@ public async Task GetBoardMetrics( /// Start of date range (ISO 8601). /// End of date range (ISO 8601). /// Optional label filter. + /// Cancellation token. /// CSV file download. /// CSV file returned. /// Invalid query parameters. @@ -90,7 +91,8 @@ public async Task ExportBoardMetrics( Guid boardId, [FromQuery] DateTimeOffset? from, [FromQuery] DateTimeOffset? to, - [FromQuery] Guid? labelId) + [FromQuery] Guid? labelId, + CancellationToken cancellationToken) { if (!TryGetCurrentUserId(out var userId, out var errorResult)) return errorResult!; @@ -100,7 +102,7 @@ public async Task ExportBoardMetrics( var fromDate = from ?? toDate.AddDays(-30); var query = new BoardMetricsQuery(boardId, fromDate, toDate, labelId); - var result = await _exportService.ExportCsvAsync(query, userId); + var result = await _exportService.ExportCsvAsync(query, userId, cancellationToken); if (!result.IsSuccess) return result.ToErrorActionResult(); diff --git a/backend/src/Taskdeck.Application/Services/MetricsExportService.cs b/backend/src/Taskdeck.Application/Services/MetricsExportService.cs index 19634c7fe..6b03e4918 100644 --- a/backend/src/Taskdeck.Application/Services/MetricsExportService.cs +++ b/backend/src/Taskdeck.Application/Services/MetricsExportService.cs @@ -39,10 +39,13 @@ public async Task> ExportCsvAsync( var timestamp = DateTimeOffset.UtcNow.ToString("yyyyMMdd-HHmmss", CultureInfo.InvariantCulture); var fileName = $"board-metrics-{metrics.BoardId:N}-{timestamp}.csv"; - return Result.Success(new MetricsExportResult( - Encoding.UTF8.GetPreamble().Concat(Encoding.UTF8.GetBytes(csv)).ToArray(), - fileName, - "text/csv")); + var bom = Encoding.UTF8.GetPreamble(); + var csvBytes = Encoding.UTF8.GetBytes(csv); + var content = new byte[bom.Length + csvBytes.Length]; + bom.CopyTo(content, 0); + csvBytes.CopyTo(content, bom.Length); + + return Result.Success(new MetricsExportResult(content, fileName, "text/csv")); } internal static string BuildCsv(BoardMetricsResponse metrics) @@ -106,7 +109,8 @@ internal static string BuildCsv(BoardMetricsResponse metrics) /// /// Sanitize a field for safe CSV inclusion. - /// - Strips CSV injection characters (=, +, -, @, tab, carriage return) from the start. + /// - Strips CSV injection characters (=, +, -, @, tab, carriage return) from the start + /// of the value AND from the start of each embedded line (after \n or \r\n). /// - Quotes the field if it contains commas, quotes, or newlines. /// - Doubles internal quote characters. /// @@ -115,12 +119,11 @@ internal static string SanitizeCsvField(string value) if (string.IsNullOrEmpty(value)) return value; - // Strip leading characters that could trigger formula injection in spreadsheet apps - var sanitized = value; - while (sanitized.Length > 0 && IsDangerousLeadingChar(sanitized[0])) - { - sanitized = sanitized[1..]; - } + // Strip leading characters that could trigger formula injection in spreadsheet apps. + // Apply to each line within the value to prevent injection via embedded newlines + // (e.g. "hello\n=CMD|'/C calc'!A0" must sanitize the second line too). + var sanitized = StripDangerousLeadingChars(value); + sanitized = SanitizeEmbeddedLines(sanitized); // If the field contains special CSV characters, quote it var needsQuoting = sanitized.Contains(',') || @@ -136,6 +139,35 @@ internal static string SanitizeCsvField(string value) return sanitized; } + private static string StripDangerousLeadingChars(string s) + { + var i = 0; + while (i < s.Length && IsDangerousLeadingChar(s[i])) + i++; + return i == 0 ? s : s[i..]; + } + + /// + /// For each line after the first within a multi-line value, strip leading + /// dangerous characters so that embedded newlines cannot smuggle formula prefixes. + /// + private static string SanitizeEmbeddedLines(string value) + { + if (!value.Contains('\n') && !value.Contains('\r')) + return value; + + var lines = value.Split('\n'); + for (var i = 1; i < lines.Length; i++) + { + var line = lines[i]; + // Handle \r\n: the \r will be at the end of the previous line's split, + // but also strip dangerous chars that appear after a bare \n. + lines[i] = StripDangerousLeadingChars(line); + } + + return string.Join('\n', lines); + } + private static bool IsDangerousLeadingChar(char c) => c is '=' or '+' or '-' or '@' or '\t' or '\r'; } diff --git a/backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs b/backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs index 2eae7daf6..819e1a300 100644 --- a/backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs +++ b/backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs @@ -186,4 +186,15 @@ public void SanitizeCsvField_ShouldStripMultipleDangerousLeadingChars() { MetricsExportService.SanitizeCsvField("==+cmd").Should().Be("cmd"); } + + [Theory] + [InlineData("hello\n=CMD|'/C calc'!A0", "\"hello\nCMD|'/C calc'!A0\"")] + [InlineData("line1\n+cmd\nline3", "\"line1\ncmd\nline3\"")] + [InlineData("safe\nsafe2", "\"safe\nsafe2\"")] + [InlineData("ok\n@evil", "\"ok\nevil\"")] + [InlineData("a\n\t\r=bad", "\"a\nbad\"")] + public void SanitizeCsvField_ShouldStripDangerousCharsFromEmbeddedLines(string input, string expected) + { + MetricsExportService.SanitizeCsvField(input).Should().Be(expected); + } } diff --git a/frontend/taskdeck-web/src/views/MetricsView.vue b/frontend/taskdeck-web/src/views/MetricsView.vue index d9982f3ff..5fc4e4f24 100644 --- a/frontend/taskdeck-web/src/views/MetricsView.vue +++ b/frontend/taskdeck-web/src/views/MetricsView.vue @@ -2,12 +2,15 @@ import { computed, onMounted, ref, watch } from 'vue' import { useBoardStore } from '../store/boardStore' import { useMetricsStore } from '../store/metricsStore' +import { useToastStore } from '../store/toastStore' import { metricsApi } from '../api/metricsApi' +import { getErrorDisplay } from '../composables/useErrorMapper' import type { MetricsQuery } from '../types/metrics' import type { Board } from '../types/board' const boardStore = useBoardStore() const metricsStore = useMetricsStore() +const toast = useToastStore() const selectedBoardId = ref('') const dateRangeDays = ref(30) @@ -77,8 +80,9 @@ async function exportCsv() { to: toDate.value, } await metricsApi.exportBoardMetricsCsv(query) - } catch { - // Error is surfaced by the store via toast + } catch (e: unknown) { + const { message } = getErrorDisplay(e, 'Failed to export CSV') + toast.error(message) } finally { exporting.value = false }