🛡️ Sentinel: [HIGH] Fix CSV Formula Injection (DDE) bypass via leading whitespace - #378
Conversation
…g whitespace * Updated the sanitization regex in `server/app.mjs` for CSV exports from `/^[=+\-@|]/` to `/^\s*[=+\-@|]/`. * This prevents attackers from bypassing formula injection neutralization (e.g. ` =cmd|' /C calc'!A0`) because spreadsheet software ignores leading whitespace when executing DDE payloads. * Added a learning entry to `.jules/sentinel.md`. * Updated `tests/api/smoke.mjs` to reflect the new strict regex assertion.
|
👋 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 reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMS Project XML 태그 추출이 정규식 매칭에서 위치 기반 방식으로 변경되었습니다. 감사 로그 CSV는 선행 공백 뒤의 수식·DDE 접두를 탐지하도록 중화 규칙, 테스트, 보안 지침이 갱신되었습니다. ChangesXML 태그 추출
CSV 수식 주입 탐지
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…g whitespace * Updated the sanitization regex in server/app.mjs for CSV exports from /^[=+\-@|]/ to /^\s*[=+\-@|]/. * This prevents attackers from bypassing formula injection neutralization (e.g. =cmd|' /C calc'!A0) because spreadsheet software ignores leading whitespace when executing DDE payloads. * Added a learning entry to .jules/sentinel.md. * Updated tests/api/smoke.mjs to reflect the new strict regex assertion. * Fixed Semgrep SAST warning regarding RegExp construction in cloud-sync.js.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/api/smoke.mjs (1)
500-500: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win중화된 셀 값을 직접 검증하도록 테스트를 보강해 주세요.
현재 검사는 두 번째 물리적 줄이 위험한 접두로 시작하지 않는지만 확인합니다. 선행 공백 payload가 실제로 존재하고 해당 셀이
'로 시작하도록 중화됐는지도 별도로 검증해야, 필드 누락이나 fixture 변경으로 테스트가 거짓 양성되는 것을 막을 수 있습니다.🤖 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 `@tests/api/smoke.mjs` at line 500, 보안 검증 테스트에서 auditCsv의 두 번째 물리적 줄 존재 여부와 선행 공백 payload가 포함된 대상 셀의 값을 직접 검증하도록 보강하세요. 해당 셀이 작은따옴표로 시작해 중화됐는지 별도 assertion으로 확인하고, 기존 위험 접두사 검사도 유지해 필드 누락이나 fixture 변경으로 인한 거짓 양성을 방지하세요.
🤖 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 `@tests/api/smoke.mjs`:
- Line 500: 보안 검증 테스트에서 auditCsv의 두 번째 물리적 줄 존재 여부와 선행 공백 payload가 포함된 대상 셀의 값을
직접 검증하도록 보강하세요. 해당 셀이 작은따옴표로 시작해 중화됐는지 별도 assertion으로 확인하고, 기존 위험 접두사 검사도 유지해 필드
누락이나 fixture 변경으로 인한 거짓 양성을 방지하세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f38280a-78e6-4ff6-b2fc-c46561146e9e
📒 Files selected for processing (4)
.jules/sentinel.mdcloud-sync.jsserver/app.mjstests/api/smoke.mjs
…g whitespace and ReDoS SAST warning * Updated the sanitization regex in server/app.mjs for CSV exports from /^[=+\-@|]/ to /^\s*[=+\-@|]/. * This prevents attackers from bypassing formula injection neutralization (e.g. =cmd|' /C calc'!A0) because spreadsheet software ignores leading whitespace when executing DDE payloads. * Added a learning entry to .jules/sentinel.md. * Updated tests/api/smoke.mjs to reflect the new strict regex assertion. * Fixed Semgrep SAST warning regarding RegExp construction in cloud-sync.js. * Updated @hono/node-server to fix Trivy SAST warning.
![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 cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headfd517a70bb309a99ff54fb0fed554dcdd6413bf8. -
Head SHA:
fd517a70bb309a99ff54fb0fed554dcdd6413bf8 -
Workflow run: 30514361201
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (6 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (6 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: smoke.mjs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: smoke.mjs"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (6 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (6 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: smoke.mjs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: smoke.mjs"]
R2 --> V2["targeted test run"]
|
|
Superseded by #386 — clean combined fix: @hono/node-server ^2.0.12 (GHSA-frvp-7c67-39w9) + hono ^4.12.32 + server audit CSV whitespace formula guard with regression smoke. Closing to reduce PR queue noise. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: The backend CSV export for audit logs mitigated CSV injection by prefixing fields beginning with formula characters (
=, +, -, @, |) with a single quote. However, the regex/^[=+\-@|]/failed to account for leading whitespace.🎯 Impact: Spreadsheet software like Excel and Google Sheets typically ignores leading spaces when executing DDE (Dynamic Data Exchange) formulas. An attacker could input a payload like
=cmd|' /C calc'!A0, bypassing the filter entirely and causing arbitrary code execution on the machine of an admin downloading the audit log.🔧 Fix: Updated the regex to
/^\s*[=+\-@|]/acrossserver/app.mjs(matching the frontend logic inapp.js) to ensure all payloads with malicious command prefixes are properly neutralized, regardless of preceding whitespace.✅ Verification: Verified via updated test assertion in
tests/api/smoke.mjs. Tests passed.PR created automatically by Jules for task 18083188102511554276 started by @seonghobae
Summary by CodeRabbit
버그 수정
테스트
=,+,-,@,|문자를 포함한 CSV 보안 검증을 강화했습니다.문서