fix(reporting): escape +, -, tab, CR and LF prefixes in the CSV export - #8038
Open
yhabib wants to merge 4 commits into
Open
fix(reporting): escape +, -, tab, CR and LF prefixes in the CSV export#8038yhabib wants to merge 4 commits into
yhabib wants to merge 4 commits into
Conversation
escapeCsvValue prefixed a single quote only when a cell started with '=', '@' or '|'. Excel and LibreOffice also run a cell that starts with '+', '-' or a tab. A token name or symbol comes from ledger metadata and reaches the CSV without a filter. Prefix those three characters too. Exempt a plain signed number, so the amount column still shows "+1.00" and "-1'234.5678".
The spec creates a linked account whose name is a spreadsheet formula, then exports the transactions CSV. It checks that the export prefixes the name with a single quote, and that the amount column keeps its sign and no quote.
…CSV export A spreadsheet reads a cell that starts with a carriage return or a line feed as a formula, the same way it reads a leading tab. Those two characters were missing from formulaInjectionCharacters, so such a cell was quoted but never prefixed. Add them to the list. Cover the two cases in the unit test and in the FORMULA_CHARACTERS list of the e2e spec.
|
✅ No security or compliance issues detected. Reviewed everything up to 085856f. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new e2e test reads from download.path() without guarding against a possible null value, which can cause runtime failures and test flakiness in some Playwright configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens CSV export hardening against spreadsheet formula injection by escaping additional leading characters (+, -, tab, CR, LF) while preserving legitimate signed numeric amounts (e.g., +20.00, -1'234.5678) and maintaining the existing Excel bigint wrapper behavior.
Changes:
- Expanded CSV formula-injection prefix handling to include
+,-, tab, carriage return, and line feed, with a signed-number exemption for amount-like values. - Added/updated unit tests for new prefix cases, signed-number exemption, and quoting interactions.
- Added a Playwright e2e test validating end-to-end CSV escaping through the UI and updated the security changelog entry.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/utils/reporting.utils.ts | Extends CSV escaping logic for additional formula prefixes and exempts signed numeric amounts. |
| frontend/src/tests/lib/utils/reporting.utils.spec.ts | Adds unit coverage for new escaping behavior and signed-number exemption. |
| frontend/src/tests/e2e/reporting-csv-escaping.spec.ts | Adds an e2e spec that exports CSV and validates escaping in produced cells. |
| CHANGELOG-Nns-Dapp-unreleased.md | Documents the security-relevant CSV export change. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot flagged that download.path() can return null and would throw inside readFileSync. Throw a clear error instead so the failure is easy to diagnose.
Contributor
There was a problem hiding this comment.
🟢 Approved
The escaping logic change is targeted and is backed by both unit and e2e coverage that exercises the newly added prefix cases and the signed-number exemption.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
CSV export only escaped
=,@and|as formula-injection prefixes. Excel and LibreOffice also treat a cell that starts with+,-, a tab, a carriage return or a line feed as a formula. A token name or symbol can carry any of these and reach the export unescaped.Changes
+1.00and-1'234.5678stay unprefixed.+,-, tab, carriage return and line feed.Tests
npm run checkandnpm run testpass../scripts/check-relative-importspasses.reporting.utils.spec.tsandReportingTransactionsButton.spec.tscover the escaping and the amount-column exemption.Todos