security(csv): harden spreadsheet formula export boundary - #570
security(csv): harden spreadsheet formula export boundary#570seonghobae wants to merge 6 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
@jules Please continue this existing Draft branch in place from exact head Implement the RED→GREEN acceptance now recorded in the PR body. Start with benign tests through the actual session CSV export/build path for ordinary values; Do not preserve the old unconditional HIGH/RCE claim: impact is spreadsheet/application dependent, and current OWASP guidance warns that no universal sanitizer is reliable across all spreadsheet/save-reopen workflows. If you choose an Excel-specific mitigation, document that product target and the data-mutating trade-off rather than generalizing it. Run applicable pnpm focused/full tests, lint/typecheck/build and keep Draft until exact-head hosted gates/review are terminal. Adopt intervening valid deltas; normal descendant only. No force-push, destructive rebase, no-op retrigger, self-approval or gate weakening. |
Acknowledged. I have updated the branch in place per your instructions, applying OWASP's recommended mitigation of prepending a single quote before potential spreadsheet formula triggers ( |
Current exact authority
developmental@722fb505d384405dd5e2e757cc4bf4371a957e54b8dc9f8a7b97827a4ba61db814e8de3533ec91c1ahead_by=5,behind_by=0), merge base exactly protecteddevelopmentalThe underlying risk is valid: session CSV export embeds values that may be attacker/user controlled, and spreadsheet applications can interpret formula-leading cell values. This lane treats the defect as CSV/formula-injection prevention at the human-opened spreadsheet boundary. It does not retain the earlier unconditional
HIGH/RCE claim because actual impact depends on the spreadsheet application, version, configuration, warnings and user interaction.Current production contract
csvField()neutralizes formula-leading=,+,-,@, C0/whitespace-prefixed forms and the explicitly selected full-width=+-@variants by prepending a single quote before normal CSV quoting. Existing comma/double-quote/CR/LF encoding remains the RFC-4180-style field boundary.OWASP's current CSV Injection guidance identifies untrusted CSV/spreadsheet exports as the relevant sink and warns that spreadsheet behavior varies. OWASP WSTG
WSTG-INPV-21likewise treats impact as spreadsheet/client dependent. ASVS 5.0.0v5.0.0-1.2.10requires CSV escaping plus single-quote neutralization for formula-leading special characters including=,+,-,@, TAB and NUL.Primary references:
TDD / evidence repair in this lineage
The owner-generated predecessor
6a6aa83b2b80e4f8a7c8a346c54e3527abe30db5added the sanitizer and a test file, but the test duplicated its own localcsvField()implementation. That test could remain GREEN even if the production export boundary regressed, so it was not causal product evidence.8dea52107f7bf2bae7b09ef76e7dbdbdf8c9a3cb: removes the copied sanitizer from the test and instead imports the realbuildSessionsCsvboundary. The regression constructs one realistic session projection and exercises benign values, CSV quoting,= + - @, TAB/CR/LF, leading-space/control variants, full-width=+-@, and comma/quote/CRLF cell-breakout payloads through the production row builder.8908388b0e072e8ffbc0438a9ace1d715954beb3: exports the existing purebuildSessionsCsvfunction so the regression can execute the canonical product boundary. No CSV runtime semantics, authorization, query or response behavior is changed by this seam.b8dc9f8a7b97827a4ba61db814e8de3533ec91c1: the first integration assertion initially split a quoted CRLF field with a naive row delimiter. The current test keeps exact-row compatibility on a framing-safe benign payload and verifies embedded CRLF separately as a quoted-field containment case.The current effective delta is three files only:
.jules/sentinel.md, the real-boundary regression, androute.ts. The branch is an ordinary ahead-only descendant; no force update or destructive rebase was used.Promotion boundary
Fresh exact-head workflows have materialized but are non-passing: Dependency Review is pending and CI, Security Scan, SAST Semgrep, OSV/OSV-Scanner PR and Scorecard are queued. No predecessor workflow/review result transfers to
b8dc9f8....Keep Draft until this unchanged exact head has terminal applicable tests/build/lint/typecheck/security/dependency evidence, zero valid unresolved findings, and then-live qualifying review/protected-branch admission. Do not use no-op retriggers, self-approval, administrator bypass, force-push, gate weakening, or fabricated spreadsheet-execution evidence.