WEB-1025: sanitize CSV/XLSX exports to prevent formula injection#3706
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Sanitization utility src/app/core/utils/csv.utils.ts, src/app/core/utils/csv.utils.spec.ts |
Adds sanitizeCsvValue and tests null/undefined handling, formula-trigger escaping, safe values, and coercion behavior. |
Run report XLSX export sanitization src/app/reports/run-report/run-report.component.ts |
Imports and applies sanitizeCsvValue to row cell values and header entries in exportToXLS. |
Table-and-SMS XLSX and CSV export sanitization src/app/reports/run-report/table-and-sms/table-and-sms.component.ts |
Imports and applies sanitizeCsvValue to cell/header values in exportToXLS and downloadCSV. |
Audit trails CSV export sanitization src/app/system/audit-trails/audit-trails.component.ts |
Removes the old JSON.stringify replacer and refactors downloadCSV to resolve, format, sanitize, then stringify each field value. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: sanitizing CSV/XLSX exports to prevent formula injection. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/core/utils/csv.utils.ts (1)
22-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the security-critical
sanitizeCsvValueutility.This function is the central defense against formula injection (CWE-1236) and is now relied upon by three export paths. Unit tests would guard against regressions if the regex or prefix logic is modified.
🧪 Suggested test cases
import { sanitizeCsvValue } from './csv.utils'; describe('sanitizeCsvValue', () => { it('should return empty string for null', () => { expect(sanitizeCsvValue(null)).toBe(''); }); it('should return empty string for undefined', () => { expect(sanitizeCsvValue(undefined)).toBe(''); }); it('should prefix = with single quote', () => { expect(sanitizeCsvValue('=cmd|"/C calc"!A0')).toBe("'=cmd|\"/C calc\"!A0"); }); it('should prefix + with single quote', () => { expect(sanitizeCsvValue('+1+1')).toBe("'+1+1"); }); it('should prefix - with single quote', () => { expect(sanitizeCsvValue('-5')).toBe("'-5"); }); it('should prefix @ with single quote', () => { expect(sanitizeCsvValue('`@SUM`(A1:A3)')).toBe("'`@SUM`(A1:A3)"); }); it('should prefix | with single quote', () => { expect(sanitizeCsvValue('|cmd')).toBe("'|cmd"); }); it('should prefix % with single quote', () => { expect(sanitizeCsvValue('%calc')).toBe("'%calc"); }); it('should prefix tab with single quote', () => { expect(sanitizeCsvValue('\t=cmd')).toBe("'\t=cmd"); }); it('should prefix carriage return with single quote', () => { expect(sanitizeCsvValue('\r=cmd')).toBe("'\r=cmd"); }); it('should not modify safe values', () => { expect(sanitizeCsvValue('hello')).toBe('hello'); expect(sanitizeCsvValue(123)).toBe('123'); expect(sanitizeCsvValue(true)).toBe('true'); }); it('should stringify objects', () => { expect(sanitizeCsvValue({})).toBe('[object Object]'); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/core/utils/csv.utils.ts` around lines 22 - 33, Add unit tests for the security-sensitive sanitizeCsvValue utility to lock down its null/undefined handling, stringification, and formula-injection prefixing behavior. Create tests around sanitizeCsvValue in csv.utils.ts covering the documented trigger characters (=, +, -, @, |, %, tab, carriage return) plus safe values and object/string coercion, so regressions in the regex or quote-prefix logic are caught early.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/core/utils/csv.utils.ts`:
- Around line 22-33: Add unit tests for the security-sensitive sanitizeCsvValue
utility to lock down its null/undefined handling, stringification, and
formula-injection prefixing behavior. Create tests around sanitizeCsvValue in
csv.utils.ts covering the documented trigger characters (=, +, -, @, |, %, tab,
carriage return) plus safe values and object/string coercion, so regressions in
the regex or quote-prefix logic are caught early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a3619bc-c0de-4c11-96ec-2a4c2734b876
📒 Files selected for processing (4)
src/app/core/utils/csv.utils.tssrc/app/reports/run-report/run-report.component.tssrc/app/reports/run-report/table-and-sms/table-and-sms.component.tssrc/app/system/audit-trails/audit-trails.component.ts
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
755b433 to
89186ba
Compare
Description
Exported CSV and XLSX files were built by writing field values directly into CSV rows and spreadsheet cells. Any value beginning with a formula-trigger character (
= + - @ | %) is interpreted as a formula by Excel/LibreOffice, which exposes the app to CSV / Formula Injection (CWE-1236) — potentially client-side command execution, data exfiltration via spreadsheet functions, and tampering with exported report/audit content.This change makes exports safe by neutralizing such values so spreadsheets
treat them as plain text:
sanitizeCsvValue()utility (src/app/core/utils/csv.utils.ts) that prefixes values starting with a formula-trigger character with a single quote, per OWASP CSV Injection guidance.reports/run-report/table-and-sms/table-and-sms.component.ts—exportToXLS()anddownloadCSV()reports/run-report/run-report.component.ts—exportToXLS()system/audit-trails/audit-trails.component.ts—downloadCSV()(raw value is sanitized beforeJSON.stringifyquoting, otherwise the leading"would hide the trigger character)Server-generated downloads (BIRT/Pentaho outputs, receipts, bulk-import files) are out of scope, as their content is produced by the backend and cannot be sanitized on the front end.
No new dependencies are required; normal values export unchanged (no regression).
Related issues and discussion
WEB-1025
Screenshots, if any
N/A — no UI changes (export logic only).
Checklist
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
null/undefinedas empty values before generating files.