chore(security): replace SheetJS xlsx with exceljs#2945
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe API spreadsheet extraction path now uses ChangesExcelJS Migration
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant extractFileText
participant ExcelJS
participant sheetToCsv
Caller->>extractFileText: uploaded spreadsheet buffer
extractFileText->>ExcelJS: workbook.xlsx.load(buffer)
ExcelJS-->>extractFileText: worksheets
extractFileText->>sheetToCsv: convert each worksheet
sheetToCsv-->>extractFileText: CSV text
extractFileText-->>Caller: "# sheet name\nCSV"
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/lib/file-extract.spec.ts (1)
42-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests are solid — consider adding a quote-escaping case.
The comma-quoting and unreadable-data rejection tests cover the key new behaviors well. One minor gap:
csvEscapedoubles embedded double quotes (value.replace(/"/g, '""')), but no test exercises that code path. A field likeHe said "hello"should produce"He said ""hello"""in the CSV output.🧪 Suggested test for quote escaping
it("quotes CSV fields containing commas", async () => { const buffer = await buildXlsx("Places", [ ["city", "country"], ["Lund, Skåne", "Sweden"], ]); const text = await extractFileText( "places.xlsx", "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", buffer, ); expect(text).toContain('"Lund, Skåne",Sweden'); }); + it("escapes embedded double quotes in CSV fields", async () => { + const buffer = await buildXlsx("Quotes", [ + ["phrase"], + ['He said "hello"'], + ]); + + const text = await extractFileText( + "quotes.xlsx", + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + buffer, + ); + expect(text).toContain('"He said ""hello"""'); + });🤖 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 `@apps/api/src/lib/file-extract.spec.ts` around lines 42 - 63, Add a test in file-extract.spec that covers csvEscape handling of embedded double quotes, since the current suite only checks comma quoting and unreadable spreadsheet rejection. Use extractFileText with buildXlsx in the existing file-extract.spec flow, and assert that a value like He said "hello" is serialized with doubled quotes in the CSV output. This should exercise the csvEscape path directly and fit alongside the existing quotes CSV fields containing commas test.
🤖 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 `@apps/api/src/lib/file-extract.spec.ts`:
- Around line 42-63: Add a test in file-extract.spec that covers csvEscape
handling of embedded double quotes, since the current suite only checks comma
quoting and unreadable spreadsheet rejection. Use extractFileText with buildXlsx
in the existing file-extract.spec flow, and assert that a value like He said
"hello" is serialized with doubled quotes in the CSV output. This should
exercise the csvEscape path directly and fit alongside the existing quotes CSV
fields containing commas test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48233680-a46c-4e40-9bbf-c5ab6d48615a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/api/package.jsonapps/api/src/lib/file-extract.spec.tsapps/api/src/lib/file-extract.tsapps/playground/src/components/playground/projects-page-client.tsx
steebchen
force-pushed
the
chore/security
branch
2 times, most recently
from
cffcdb1 to
2415f22
Compare
SheetJS xlsx (GHSA-4r6h-8v6p-xvw6 prototype pollution, GHSA-5pgg-2g8v-p4x9 ReDoS) has no fixed version published to the npm registry, so Dependabot flags every version. Migrate knowledge-base spreadsheet extraction to the maintained exceljs package and drop the xlsx dependency. - apps/api/src/lib/file-extract.ts: read .xlsx via exceljs, convert each worksheet to CSV with standard quoting. - Drop advertised legacy binary .xls upload (exceljs cannot parse BIFF); such files are rejected cleanly by the upload route. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015JboNg5ryVipY8M6nzJykR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the only outstanding high/critical Dependabot security alerts in the repository, both against the SheetJS
xlsxpackage:Why a version bump can't fix this
Both advisories are recorded with
introduced: 0and no fixed version in the npm ecosystem. SheetJS stopped publishing to the npm registry after0.18.5; patched builds live only on the SheetJS CDN. The repo already pinned the CDN-hosted, patchedxlsx@0.20.3, but Dependabot/GHSA still flag it because there is no registry release that clears the advisory range. The only durable remediation is to stop depending onxlsx.Change
apps/api/src/lib/file-extract.ts— migrate knowledge-base spreadsheet extraction fromxlsxto the actively-maintainedexceljs(no open high/critical advisories). Each worksheet is converted to CSV with standard field quoting (commas, quotes, and newlines are escaped).apps/api/package.json/pnpm-lock.yaml— dropxlsx, addexceljs@4.4.0.apps/playground/.../projects-page-client.tsx— stop advertising legacy binary.xlsin the upload picker.file-extract.spec.ts— build fixtures withexceljs; add a CSV-quoting case and a case asserting unreadable spreadsheet data is rejected.Behavior note
exceljsreads modern.xlsxbut cannot parse the legacy binary.xls(BIFF) format..xlsxextraction is unchanged. Legacy.xlsuploads (rare; Excel has defaulted to.xlsxsince 2007) are no longer advertised in the picker and are rejected cleanly by the existing upload error path rather than being indexed as garbage.Verification
osv-scanneron the updated lockfile: 0 high/critical remaining (previously 2).pnpm format— cleanturbo run build(full monorepo) — passesapps/apiunit tests (file-extract.spec.ts) — 6/6 pass;tsc --noEmitclean🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
.xlsxfiles, including workbooks with multiple sheets (combined output with sheet names)..xlsfrom the upload flow.