Skip to content

Fix adversarial review findings for CSV export #794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Chris0Jeky merged 1 commit into from
Apr 8, 2026
Merged

Fix adversarial review findings for CSV export #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions backend/src/Taskdeck.Api/Controllers/MetricsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public async Task<IActionResult> GetBoardMetrics(
/// <param name="from">Start of date range (ISO 8601).</param>
/// <param name="to">End of date range (ISO 8601).</param>
/// <param name="labelId">Optional label filter.</param>
/// <param name="cancellationToken">Cancellation token.</param>
/// <returns>CSV file download.</returns>
/// <response code="200">CSV file returned.</response>
/// <response code="400">Invalid query parameters.</response>
Expand All @@ -90,7 +91,8 @@ public async Task<IActionResult> 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!;
Expand All @@ -100,7 +102,7 @@ public async Task<IActionResult> 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();
Expand Down
54 changes: 43 additions & 11 deletions backend/src/Taskdeck.Application/Services/MetricsExportService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,13 @@ public async Task<Result<MetricsExportResult>> 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)
Expand Down Expand Up @@ -106,7 +109,8 @@ internal static string BuildCsv(BoardMetricsResponse metrics)

/// <summary>
/// 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.
/// </summary>
Expand All @@ -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(',') ||
Expand All @@ -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..];
}

/// <summary>
/// For each line after the first within a multi-line value, strip leading
/// dangerous characters so that embedded newlines cannot smuggle formula prefixes.
/// </summary>
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);
Comment on lines +159 to +168
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SanitizeEmbeddedLines() checks for both '\n' and '\r' but only splits on '\n'. If an attacker uses a bare carriage return as the embedded line break (e.g. "safe\r=CMD(...)"), the second line is never sanitized and the formula prefix can remain. Consider sanitizing after any newline boundary (handle '\r' as a delimiter too, including '\r\n'), and add a unit test covering the '\r' (no '\n') case.

Suggested change
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);
var result = new StringBuilder(value.Length);
var segmentStart = 0;
var isFirstLine = true;
for (var i = 0; i < value.Length; i++)
{
if (value[i] != '\n' && value[i] != '\r')
continue;
var segment = value[segmentStart..i];
result.Append(isFirstLine ? segment : StripDangerousLeadingChars(segment));
if (value[i] == '\r' && i + 1 < value.Length && value[i + 1] == '\n')
{
result.Append("\r\n");
i++;
}
else
{
result.Append(value[i]);
}
isFirstLine = false;
segmentStart = i + 1;
}
var lastSegment = value[segmentStart..];
result.Append(isFirstLine ? lastSegment : StripDangerousLeadingChars(lastSegment));
return result.ToString();

Copilot uses AI. Check for mistakes.
}
Comment on lines +154 to +169
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The SanitizeEmbeddedLines method currently only splits the input by \n. While this correctly handles \n and \r\n (as the \r remains at the end of the previous segment), it fails to sanitize lines separated by a bare carriage return (\r). Since \r is explicitly listed as a dangerous character in IsDangerousLeadingChar, it should also be treated as a potential line separator to prevent formula injection in spreadsheet applications that recognize it as such. Normalizing all line endings to \n during this process is a robust and safe approach for CSV fields.

    private static string SanitizeEmbeddedLines(string value)
    {
        if (!value.Contains('\n') && !value.Contains('\r'))
            return value;

        // Normalize line endings to \n to ensure all embedded lines are caught,
        // then strip dangerous characters from the start of each line (except the first,
        // which is handled by the caller).
        var lines = value.Replace("\r\n", "\n").Replace('\r', '\n').Split('\n');
        for (var i = 1; i < lines.Length; i++)
        {
            lines[i] = StripDangerousLeadingChars(lines[i]);
        }

        return string.Join('\n', lines);
    }


private static bool IsDangerousLeadingChar(char c)
=> c is '=' or '+' or '-' or '@' or '\t' or '\r';
}
11 changes: 11 additions & 0 deletions backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
8 changes: 6 additions & 2 deletions frontend/taskdeck-web/src/views/MetricsView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>('')
const dateRangeDays = ref(30)
Expand Down Expand Up @@ -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
}
Expand Down
Loading