fix(security): harden CSV formula control-prefix boundary - #1103
fix(security): harden CSV formula control-prefix boundary#1103seonghobae wants to merge 32 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: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesCSV 수식 주입 방지
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to CSV exports now neutralize NUL-prefixed and full-width formula-like fields before CSV quoting, with regression coverage for the relevant input variants. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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 addresses the CSV formula injection NUL byte bypass vulnerability by including the NUL byte in the regex for problematic characters, ensuring that the escapeCsvField function correctly handles NUL bytes and prevents formula injection.
Findings
-
No blocking findings.
-
Result: APPROVE
-
Head SHA:
3bf327f383a5bc8b97ede8aefc4400cc67c04c84 -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
There was a problem hiding this comment.
Noema LLM review
The PR addresses the CSV formula injection NUL byte bypass vulnerability by including the NUL byte in the regex for problematic characters, ensuring that the escapeCsvField function correctly handles repeated NUL bytes. The changes are well-tested and do not introduce any new issues. The prior review threads and CodeGraph context provide additional context and validation for the changes. The diff and changed file context are accurate and complete, and the review process has been thorough and transparent.
Findings
-
No blocking findings.
-
Result: APPROVE
-
Head SHA:
51c8b2878e898f2ed109b37edf83da8d1f8054a5 -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
|
@opencode-agent Please independently review exact current head |
Acknowledged. I have independently verified the implementation against the current head. The regex |
There was a problem hiding this comment.
Noema LLM review
The PR addresses a medium-severity security vulnerability in the CSV formula injection mitigation, specifically the NUL byte bypass. The changes are correct and the issue is resolved. The additional context provided by the review threads and CodeGraph context does not reveal any new issues or concerns. The maintainability and behavioral regressions are not affected by this change. The tests in export.test.ts cover the new scenario and ensure the fix is correct. The diff is clean and the changes are well-documented. Overall, the PR is ready for merge.
Findings
-
No blocking findings.
-
Result: APPROVE
-
Head SHA:
15a274adcfa9ab040f7a0bb0c52d43f37bb0f14e -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
|
@opencode-agent Please perform the required independent formal review for exact current head |
Acknowledged. I have independently verified the implementation against the current head |
|
Current-head dependency routing for The required Noema failure is not a BandScope source defect. Exact job Canonical owner is the already-open read-only dependency PR Acceptance for this BandScope lane remains a fresh Noema run on unchanged exact head |
Acknowledged. I understand the |
|
@opencode-agent Please perform the required independent formal review for exact current head |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.jules/sentinel.md— repository behaviorapps/desktop/src/lib/export.test.ts— TypeScript or JavaScript runtimeapps/desktop/src/lib/export.ts— TypeScript or JavaScript runtime
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: sentinel.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: sentinel.md"]
R1 --> V1["required checks"]
Evidence --> S2["TypeScript/JavaScript: export.test.ts (2 files)"]
S2 --> I2["TypeScript or JavaScript runtime"]
I2 --> R2["Review risk: TypeScript/JavaScript: export.test.ts (2 files)"]
R2 --> V2["package test plus coverage"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
2839a6b91f5603b69d1f6af0225a109f1ed928c6 - Workflow run: 33440869573
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: sentinel.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: sentinel.md"]
R1 --> V1["required checks"]
Evidence --> S2["TypeScript/JavaScript: export.test.ts (2 files)"]
S2 --> I2["TypeScript or JavaScript runtime"]
I2 --> R2["Review risk: TypeScript/JavaScript: export.test.ts (2 files)"]
R2 --> V2["package test plus coverage"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
Acknowledged. I have independently verified the implementation against the current head |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
seonghobae
changed the title
|
Succession receipt: #1177 ( |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. The semantic changes have been fully succeeded by PR #1177. |
1 participant
Security outcome
Canonical desktop CSV formula-injection hardening.
escapeCsvFieldtreats ASCII formula initiators, the full-width operator variants＝,＋,－,＠, and any leading C0 control (\x00-\x1F) after whitespace/BOM/NBSP as dangerous, then prefixes the original field before ordinary CSV structural quoting.The threat claim stays bounded. Spreadsheet behavior is consumer/product dependent; C0 handling is defense in depth against parser/representation disagreement, not by itself proof of arbitrary code execution or a severity upgrade.
Exact current identity
develop@314ddeae7b775a4957594b599358c8255617eb2e.fix-csv-nul-byte-injection-12448382976281544075.71c27721e01fd7d0fd62382f39daa1e6a1646ab4.develop...HEAD:behind_by=0; semantic delta remains exactly.jules/sentinel.md,apps/desktop/src/lib/export.ts, andapps/desktop/src/lib/export.test.ts.The branch adopted protected #1165 through ordinary two-parent descendant
419651fdee22f4575fc91ff313e3fec91eff7efb. The merge tree starts from current protecteddevelopand overlays only this owner’s three semantic files. No force update, destructive rebase, protected-workflow copy, or predecessor evidence transfer was used.Test-first C0 repair
Fresh overlap review of #1171 found one valid delta not yet represented here: a non-whitespace C0 prefix such as ESC (
\x1B) could precede a formula-shaped value and bypass the NUL-specific canonical predicate.754fa51b69a2718346161e38228937c84f98d241requires ESC-prefixed formula-shaped input, whitespace + BEL, and an ESC-only cell to fail closed while preserving the existing NUL/full-width contracts.12043a9c870fe673a741b9285dabcf4d69764fe9changes the dangerous first-token class from NUL-only to all C0 controls without weakening the ASCII or full-width operator guards.419651fdee22f4575fc91ff313e3fec91eff7efbpreserves the same three-file semantic delta on currentdevelop.71c27721e01fd7d0fd62382f39daa1e6a1646ab4updates the existing Sentinel learning note from NUL-only to the actual C0 trust boundary.Focused runtime evaluation of the exact predicate passed 12 boundary cases covering NUL, repeated NUL, ESC, BEL after whitespace, C0-only input, full-width operators, ordinary text, blank text, and newline-prefixed formula-shaped input. This is focused implementation evidence only; it is not repository exact-head GREEN.
Succession evidence
\x1B+SUM(A1)—is now carried here by executable regression, production policy, and owner documentation. 🛡️ Sentinel: [MEDIUM] C0 CSV prefix bypass — superseded by #1103 #1171’s unrelated formatting-only change intest_supply_chain_policy.pyis not a semantic/test/fixture/contract requirement and is intentionally not imported. No 🛡️ Sentinel: [MEDIUM] C0 CSV prefix bypass — superseded by #1103 #1171 check/review/status evidence transfers.Security boundary and acceptance
Attack surface is untrusted CSV field content opened later by spreadsheet software.
escapeCsvFieldremains the sole desktop CSV-cell neutralization boundary; field separators, quotes and line breaks remain structurally confined by the existing encoder. Production acceptance should distinguish raw CSV containment from spreadsheet interpretation and use harmless formula probes in supported spreadsheet products/locales, including save/re-open where applicable.No filesystem, network, subprocess, IPC, WebView, model, dependency, lockfile, workflow or vulnerability-suppression authority is added by this repair.
Verification / merge gate
Keep Draft. Evaluate repository/central CI, security, SAST, dependency, SBOM, coverage, cross-platform build/release and review evidence only on unchanged exact head
71c27721e01fd7d0fd62382f39daa1e6a1646ab4. A qualifying independent non-author last-push approval and zero actionable threads are required. Queued, pending, absent, skipped-required, cancelled, failed, stale, predecessor, protected-base, model-only, self/author or administrative-bypass evidence is non-passing.