π‘οΈ Sentinel: [HIGH] Fix CSV formula injection bypass via NUL byte - #1121
π‘οΈ Sentinel: [HIGH] Fix CSV formula injection bypass via NUL byte#1121seonghobae wants to merge 1 commit 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. |
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: Youβve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: βοΈ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: π Files selected for processing (3)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR closes a CSV formula injection bypass by adding NUL byte (\x00) coverage to the leading-whitespace and trigger character classes in the escapeCsvField regex. The change is minimal, well-tested, and does not introduce behavioral regressions for legitimate fields. The added tests confirm correct escaping for NUL-prefixed formula triggers and standalone NUL bytes.
Reviewed changed lines
apps/desktop/src/lib/export.ts:26 (RIGHT): The regex now includes \x00 in both the leading character class and the trigger class. This ensures that any field starting with NUL bytes (optionally preceded by whitespace/BOM/NBSP) followed by a formula trigger (or a NUL itself) is prefixed with a single quote, preventing spreadsheet formula execution. The eslint-disable comment is appropriate for the control character.apps/desktop/src/lib/export.test.ts:70 (RIGHT): Test case verifies that a NUL byte followed by '=' is escaped correctly, confirming the regex matches and prefixes a single quote.apps/desktop/src/lib/export.test.ts:71 (RIGHT): Test case verifies that a NUL byte followed by '@' is escaped correctly, covering another common formula trigger.apps/desktop/src/lib/export.test.ts:72 (RIGHT): Test case verifies that a standalone NUL byte is escaped, ensuring the trigger class correctly matches NUL alone.
Adversarial validation
apps/desktop/src/lib/export.ts:26 (RIGHT)falsified: The updated regex fails to escape a field starting with NUL followed by a formula trigger, leaving the injection vector open. β Source-traced: the regex /^[\s\uFEFF\xA0\x00]*[=+-@\t\r\n\x00]/ matches because \x00 is in the leading class and '=' is in the trigger class. The function returns "'\x00=1+2", as confirmed by the new test case on line 70.apps/desktop/src/lib/export.test.ts:70 (RIGHT)falsified: The test case for NUL followed by '=' does not actually verify the escaping behavior, leaving a gap in coverage. β Source-traced: the test expects escapeCsvField('\x00=1+2') to be "'\x00=1+2". The implementation returns that because the regex matches and prefixes a quote. The test is correct and would fail if the regex did not include \x00.apps/desktop/src/lib/export.test.ts:72 (RIGHT)falsified: A field consisting solely of NUL bytes is not escaped, leaving a potential injection vector. β Source-traced: the regex matches because \x00 is in the trigger class, so the function returns "'\x00", as verified by the test on line 72.- Residual risk: low
Findings
-
No blocking findings.
-
Result: APPROVE
-
Head SHA:
930eca395aeb03548d73638524a701c88003df0d -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
1 participant
π‘οΈ Sentinel: [HIGH] Fix CSV formula injection bypass via NUL byte
π¨ Severity: HIGH
π‘ Vulnerability: The existing regex designed to mitigate CSV formula injection in
escapeCsvFieldmissed control characters like the NUL byte (\x00).π― Impact: An attacker could craft a payload starting with a NUL byte, bypassing the current sanitizer. Some spreadsheet applications ignore the leading NUL byte and execute the subsequent formula.
π§ Fix: Expanded the existing prefixing regex to include
\x00in both the leading whitespace list and the trigger character list. Added the appropriate ESLint disable comment.β Verification: Unit tests covering the NUL byte bypass were added to
export.test.ts, and full coverage (100%) was maintained. All monorepo quickchecks (lint, test, build) passed.Superseded β 2026-09-01
Closed unmerged because canonical security PR #1103 owns the same desktop
escapeCsvFieldboundary on the same protecteddevelopbase and already contains the stronger repeated-NUL and whitespace+repeated-NUL regressions. The only unique executable edge from this PR, the NUL-only cell assertion, was transferred to #1103 in normal non-force history at commitfcb7a8c6a3f518426f647a6e3c0badeb119f75febefore this close. #1103 therefore preserves the complete useful security behavior without keeping two competing writers for the same three files.No check/review/approval evidence from this PR transfers to #1103. #1103 must pass fresh exact-head required checks and independent review on its own current head.
PR created automatically by Jules for task 13408169584317490020 started by @seonghobae