Fix adversarial review findings for CSV export#794
Conversation
…urface 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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Chris0Jeky
merged commit 1617980
into
feature/analytics-export-reports-78
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the metrics export functionality, including the addition of cancellation token support in the API, a more efficient byte array construction for CSV files with a UTF-8 BOM, and enhanced CSV injection protection that sanitizes embedded newlines. Additionally, the frontend now includes explicit error handling and toast notifications for failed exports. A security concern was identified in the new sanitization logic where bare carriage returns (\r) are not correctly handled as line separators, potentially allowing formula injection in certain spreadsheet applications.
| 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); | ||
| } |
There was a problem hiding this comment.
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);
}There was a problem hiding this comment.
Pull request overview
Addresses adversarial review findings in the board-metrics CSV export by tightening CSV-injection sanitization, improving request cancellation propagation, optimizing UTF-8 BOM handling, and surfacing export failures in the UI.
Changes:
- Harden
SanitizeCsvFieldagainst embedded-newline CSV injection and add new test cases. - Forward
CancellationTokenfrom the API controller into the export service. - Optimize UTF-8 BOM prepending and show export errors via frontend toast notifications.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/taskdeck-web/src/views/MetricsView.vue | Adds toast-based error surfacing for failed CSV export requests. |
| backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs | Adds theory cases covering embedded-newline injection vectors in CSV sanitization. |
| backend/src/Taskdeck.Application/Services/MetricsExportService.cs | Improves CSV field sanitization for embedded lines and replaces BOM .Concat().ToArray() with direct array copy. |
| backend/src/Taskdeck.Api/Controllers/MetricsController.cs | Threads request cancellation token through to the export service call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); |
There was a problem hiding this comment.
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.
| 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(); |
Summary
SanitizeCsvField(strips dangerous leading chars from all lines, not just the first)CancellationTokenfrom controller to export service.Concat().ToArray()BOM prepend with direct array copyContext
Fixes findings from adversarial review of PR #787 (comment: #787 (comment))
Test plan