Add exportable analytics reports (CSV) with reproducible filters#787
Add exportable analytics reports (CSV) with reproducible filters#787Chris0Jeky merged 8 commits intomainfrom
Conversation
…rotection Introduce IMetricsExportService and MetricsExportService in the Application layer. CSV output includes schema_version header, reproducible filter parameters, documented columns per section (Summary, Throughput, CycleTime, WIP, Blocked), and CSV-injection-safe field sanitization. Closes part of #78
GET /api/metrics/boards/{boardId}/export returns a downloadable CSV file
with Content-Disposition attachment header. Reuses existing authorization
and metrics computation pipeline via IMetricsExportService.
Closes part of #78
Wire frontend metrics view to the backend CSV export endpoint. Button is enabled when metrics data is loaded, triggers a browser download via blob URL, and extracts filename from Content-Disposition header. Closes part of #78
21 unit tests covering CSV building, schema version, all sections, and CSV injection protection. 8 integration tests covering auth, authorization, cross-user isolation, empty boards, date range filters, and Content-Disposition. Closes part of #78
Record the decision to ship CSV export first and defer PDF to a future iteration. PDF adds significant dependency weight for low marginal value in a developer-facing local-first tool. Closes #78
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adversarial Self-ReviewCSV Injection
Authorization
Large Dataset Memory
Content-Disposition Header
Filter Validation
Unused Import Fixed
No Issues Found Requiring Code ChangesAll identified concerns are either already mitigated or acceptable for the current data scale. |
![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 a CSV export feature for board metrics, including a new API endpoint, a MetricsExportService with schema versioning and injection protection, and a frontend "Export CSV" button. ADR-0022 is included to document the prioritization of CSV over PDF. Feedback focuses on making the CSV sanitization non-destructive by using single-quote prefixing, refactoring duplicated date-defaulting logic in the controller, and passing an explicit timestamp to the CSV builder to improve testability and ensure consistency between the filename and file headers.
| 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..]; | ||
| } | ||
|
|
||
| // If the field contains special CSV characters, quote it | ||
| var needsQuoting = sanitized.Contains(',') || | ||
| sanitized.Contains('"') || | ||
| sanitized.Contains('\n') || | ||
| sanitized.Contains('\r'); | ||
|
|
||
| if (needsQuoting) | ||
| { | ||
| sanitized = "\"" + sanitized.Replace("\"", "\"\"") + "\""; | ||
| } | ||
|
|
||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
The current implementation of CSV injection protection is destructive because it strips leading characters like =, +, -, and @. This alters legitimate data (e.g., a card title starting with a hyphen like "- Fix bug" becomes "Fix bug").
To prevent formula injection without losing data, it is recommended to prefix the field with a single quote (') if it starts with a dangerous character. Spreadsheet applications like Excel treat a leading single quote as an instruction to render the cell as a literal string.
Note: The prefix should be added before the quoting logic to ensure it is correctly handled by CSV parsers.
internal static string SanitizeCsvField(string value)
{
if (string.IsNullOrEmpty(value))
return value;
var sanitized = value;
// Prefix with a single quote if it starts with a dangerous character to prevent formula injection
if (IsDangerousLeadingChar(sanitized[0]))
{
sanitized = "'" + sanitized;
}
// If the field contains special CSV characters, quote it
var needsQuoting = sanitized.Contains(',') ||
sanitized.Contains('"') ||
sanitized.Contains('\n') ||
sanitized.Contains('\r');
if (needsQuoting)
{
sanitized = "\"" + sanitized.Replace("\"", "\"\"") + "\"";
}
return sanitized;
}| var toDate = to ?? DateTimeOffset.UtcNow; | ||
| var fromDate = from ?? toDate.AddDays(-30); |
There was a problem hiding this comment.
This date range defaulting logic is duplicated from the GetBoardMetrics method (lines 62-63). Consider refactoring this into a shared helper method or moving the default logic into the BoardMetricsQuery constructor to improve maintainability and ensure consistency across the API.
| var csv = BuildCsv(metrics); | ||
|
|
||
| var timestamp = DateTimeOffset.UtcNow.ToString("yyyyMMdd-HHmmss", CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
Pass the current timestamp to BuildCsv to ensure consistency between the filename and the header, and to improve testability by making the CSV generation deterministic.
var now = DateTimeOffset.UtcNow;
var csv = BuildCsv(metrics, now);
var timestamp = now.ToString("yyyyMMdd-HHmmss", CultureInfo.InvariantCulture);| internal static string BuildCsv(BoardMetricsResponse metrics) | ||
| { | ||
| var sb = new StringBuilder(); | ||
|
|
||
| // Schema version header (comment line) | ||
| sb.AppendLine($"# schema_version={SchemaVersion}"); | ||
| sb.AppendLine($"# board_id={metrics.BoardId}"); | ||
| sb.AppendLine($"# from={metrics.From:o}"); | ||
| sb.AppendLine($"# to={metrics.To:o}"); | ||
| sb.AppendLine($"# exported_at={DateTimeOffset.UtcNow:o}"); |
There was a problem hiding this comment.
The BuildCsv method is currently non-deterministic because it uses DateTimeOffset.UtcNow internally for the exported_at header. This makes it difficult to write precise unit tests for the CSV output.
Consider passing the export timestamp as a parameter to BuildCsv. This also ensures that the timestamp in the CSV header matches the one used in the filename.
internal static string BuildCsv(BoardMetricsResponse metrics, DateTimeOffset exportedAt)
{
var sb = new StringBuilder();
// Schema version header (comment line)
sb.AppendLine($"# schema_version={SchemaVersion}");
sb.AppendLine($"# board_id={metrics.BoardId}");
sb.AppendLine($"# from={metrics.From:o}");
sb.AppendLine($"# to={metrics.To:o}");
sb.AppendLine($"# exported_at={exportedAt:o}");There was a problem hiding this comment.
Pull request overview
Adds an exportable, schema-versioned CSV report for board metrics, including a new backend API endpoint and a frontend “Export CSV” action, plus ADR documentation and test coverage.
Changes:
- Backend: introduce
IMetricsExportService/MetricsExportServiceandGET /api/metrics/boards/{boardId}/exportreturning a downloadable CSV. - Frontend: add an “Export CSV” button on the metrics dashboard and a
metricsApi.exportBoardMetricsCsv()blob download helper. - Docs/Tests: add ADR-0022 and new unit + integration tests covering CSV output and API behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/views/MetricsView.vue | Adds export button + wiring for CSV export from the metrics dashboard |
| frontend/taskdeck-web/src/api/metricsApi.ts | Adds client call to export endpoint and triggers browser download |
| backend/src/Taskdeck.Application/Services/MetricsExportService.cs | Implements CSV generation + injection sanitization + download metadata |
| backend/src/Taskdeck.Application/Services/IMetricsExportService.cs | Defines export service contract and result shape |
| backend/src/Taskdeck.Api/Controllers/MetricsController.cs | Adds /export endpoint to serve CSV downloads |
| backend/src/Taskdeck.Api/Extensions/ApplicationServiceRegistration.cs | Registers export service in DI |
| backend/tests/Taskdeck.Application.Tests/Services/MetricsExportServiceTests.cs | Unit tests for CSV structure and sanitization |
| backend/tests/Taskdeck.Api.Tests/MetricsExportApiTests.cs | Integration tests for auth, access isolation, headers, and basic content |
| docs/decisions/ADR-0022-analytics-export-csv-first-pdf-deferred.md | ADR documenting CSV-first decision and PDF deferral |
| docs/decisions/INDEX.md | Adds ADR-0022 to the ADR index |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch { | ||
| // Error is surfaced by the store via toast |
There was a problem hiding this comment.
exportCsv() swallows errors but the comment says the store will surface a toast. Since this call goes directly to metricsApi.exportBoardMetricsCsv() (not through metricsStore), failures currently won’t show any user-facing error.
Consider mapping the error here (e.g., via useToastStore() + getErrorDisplay) or adding an exportBoardMetricsCsv() action to metricsStore to keep error/toast behavior consistent with fetchBoardMetrics().
| } catch { | |
| // Error is surfaced by the store via toast | |
| } catch (err: unknown) { | |
| metricsStore.error = | |
| err instanceof Error && err.message ? err.message : 'Failed to export metrics CSV.' |
| // Schema version header (comment line) | ||
| sb.AppendLine($"# schema_version={SchemaVersion}"); | ||
| sb.AppendLine($"# board_id={metrics.BoardId}"); | ||
| sb.AppendLine($"# from={metrics.From:o}"); | ||
| sb.AppendLine($"# to={metrics.To:o}"); | ||
| sb.AppendLine($"# exported_at={DateTimeOffset.UtcNow:o}"); | ||
| sb.AppendLine(); |
There was a problem hiding this comment.
CSV header includes board_id, from, to, and exported_at, but it does not include the optional labelId filter from the request. When a label filter is used, the export is no longer fully reproducible/auditable from the embedded headers.
Suggested fix: pass the original BoardMetricsQuery (or at least LabelId) into BuildCsv(...) and emit a # label_id=... (or # label_id= empty) header line when applicable.
| [HttpGet("boards/{boardId}/export")] | ||
| [ProducesResponseType(typeof(FileContentResult), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(typeof(ApiErrorResponse), StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(typeof(ApiErrorResponse), StatusCodes.Status401Unauthorized)] | ||
| [ProducesResponseType(typeof(ApiErrorResponse), StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(typeof(ApiErrorResponse), StatusCodes.Status404NotFound)] | ||
| public async Task<IActionResult> ExportBoardMetrics( |
There was a problem hiding this comment.
This controller has [Produces("application/json")] at the class level, but ExportBoardMetrics returns a CSV File(...) response. As-is, OpenAPI/Swagger may advertise the 200 response as application/json instead of text/csv.
Consider overriding on this action (e.g., add [Produces("text/csv")] / [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(FileContentResult), ContentTypes = new[] { "text/csv" })]) so clients and docs correctly reflect the content type.
| const query: MetricsQuery = { | ||
| boardId: selectedBoardId.value, | ||
| from: fromDate.value, | ||
| to: toDate.value, | ||
| } |
There was a problem hiding this comment.
toDate is defined as computed(() => new Date().toISOString()) with no reactive dependencies, so Vue will cache it after first evaluation. That means subsequent exports (and metric fetches) can end up using a stale to timestamp, and if the page stays open long enough the recomputed from (based on "now") can even become later than the cached to, triggering backend validation errors.
Consider generating to (and from) at the moment you build the query (e.g., inside fetchMetrics() / exportCsv()), or making toDate depend on a reactive "now" source (timer) so it updates over time.
Adversarial Review -- Round 2Findings1. [HIGH] CSV Injection: Embedded newlines can smuggle formula prefixes
Fix: After the leading-char strip loop, also strip dangerous leading chars from each line within the value. Alternatively, prefix every dangerous leading char with a single-quote ( File: 2. [MEDIUM] CancellationToken not forwarded from controller to export service
File: 3. [MEDIUM] Entire CSV buffered in memory as
The Not critical for typical board sizes but worth noting for scalability. File: 4. [MEDIUM] Frontend error silently swallowed -- comment is inaccurate
File: 5. [LOW] No The controller defaults 6. [LOW] Content-Disposition filename lacks sanitization against special chars The filename is constructed as 7. [LOW] UTF-8 BOM is present -- good Line 43 correctly prepends 8. [LOW] Schema version header present and documented -- good
Test Quality Assessment
VerdictPASS WITH FIXES -- Findings #1 (CSV injection via embedded newlines) and #4 (silent error swallowing in frontend) should be fixed before merge. Finding #2 (CancellationToken) is a good practice fix. The rest are acceptable as-is or for follow-up. |
…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
…und2 Fix adversarial review findings for CSV export
Adversarial Review -- Round 2 Fixes AppliedAll HIGH and MEDIUM findings from the review above have been fixed via PR #794 (merged into this branch):
All backend tests pass (full suite, 0 failures). |
Summary
GET /api/metrics/boards/{boardId}/exportthat reuses the existingBoardMetricsServicepipeline with validated filter parameters (boardId, from, to, labelId)1.0), documented column layout across 5 sections (Summary, Throughput, CycleTime, WIP, Blocked), reproducible filter parameters as comment headers, and CSV-injection-safe field sanitizationCloses #78
Test plan
dotnet testpasses forMetricsExportServiceTests(21 unit tests)dotnet testpasses forMetricsExportApiTests(8 integration tests)# schema_version=1.0header.csvextension=,+,-,@) are stripped from field starts