Security: replace xlsx (SheetJS) with exceljs#13
Merged
Conversation
Closes the last open Dependabot alert on the TypeScript CLI. xlsx@0.18.5 is the latest published-to-npm version; SheetJS moved patches to their own CDN and the npm distribution still ships: - GHSA-4r6h-8v6p-xvw6 (Prototype Pollution) - GHSA-5pgg-2g8v-p4x9 (ReDoS) Replaced the two call sites (compiler asset extraction and binary asset packaging) with exceljs through a shared excel.ts helper that preserves the existing Markdown-wrapped CSV output format. Behavior changes: - exceljs is stricter than SheetJS — invalid .xlsx now throws rather than parsing arbitrary input as a single-sheet CSV. This is a security improvement; the corresponding test was updated to assert the throw (matches the existing extractWord/extractPdf pattern). - CSV cell formatting handles Excel-specific cell value types (RichText, formula result, hyperlink, error, Date) explicitly. Verified: - npm audit: 0 vulnerabilities (down from 1) - npm test: 20/20 suites pass, 331 tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
sbaker
added a commit
that referenced
this pull request
Apr 20, 2026
Removes the Express 4 path-to-regexp 0.1.x override (was a workaround for GHSA-37ch-88jc-xwx2). Express 5 ships with a modern router that uses path-to-regexp 8.x, which is patched. The RPC server (src/lib/rpc-server.ts, deployed via Dockerfile CMD node dist/server.js) only uses simple :param routes with no ?/*/regex syntax, so the path-syntax breaking changes between Express 4 and 5 do not apply. All middleware (helmet 7, cors 2.8, compression 1.8, express-rate-limit 7) supports Express 5. Bumps: - express: ^4.18.2 -> ^5.0.0 (resolves to 5.2.1) - @types/express: ^4.17.21 -> ^5.0.0 Override consolidation: - Removed: express > path-to-regexp ^0.1.13 (no longer needed) - Removed: router > path-to-regexp ^8.4.0 (folded into top-level) - Added: path-to-regexp ^8.4.0 (single top-level pin covers both the express 5 router and the MCP SDK chains) Verified: - npm run build passes - npm test: 20/20 suites pass, 331 tests pass - npm audit: 1 alert (xlsx, no fix available — handled in PR #13) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
sbaker
added a commit
that referenced
this pull request
Apr 20, 2026
Removes the Express 4 path-to-regexp 0.1.x override (was a workaround for GHSA-37ch-88jc-xwx2). Express 5 ships with a modern router that uses path-to-regexp 8.x, which is patched. The RPC server (src/lib/rpc-server.ts, deployed via Dockerfile CMD node dist/server.js) only uses simple :param routes with no ?/*/regex syntax, so the path-syntax breaking changes between Express 4 and 5 do not apply. All middleware (helmet 7, cors 2.8, compression 1.8, express-rate-limit 7) supports Express 5. Bumps: - express: ^4.18.2 -> ^5.0.0 (resolves to 5.2.1) - @types/express: ^4.17.21 -> ^5.0.0 Override consolidation: - Removed: express > path-to-regexp ^0.1.13 (no longer needed) - Removed: router > path-to-regexp ^8.4.0 (folded into top-level) - Added: path-to-regexp ^8.4.0 (single top-level pin covers both the express 5 router and the MCP SDK chains) Verified: - npm run build passes - npm test: 20/20 suites pass, 331 tests pass - npm audit: 1 alert (xlsx, no fix available — handled in PR #13) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
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
1 participant
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
Closes the last remaining open Dependabot alert on the TypeScript CLI.
The npm-published
xlsx@0.18.5is unpatched against GHSA-4r6h-8v6p-xvw6 (Prototype Pollution) and GHSA-5pgg-2g8v-p4x9 (ReDoS). SheetJS stopped publishing to npm and moved patches to their own CDN — pinning the npm version is a dead end.Swapped to
exceljs@^4.4.0(actively maintained, MIT, ~5M weekly downloads, no open advisories).Implementation
src/lib/excel.ts— sharedexcelToMarkdownSheets(filePath)helper used by both call sites, preserves the existing Markdown-wrapped CSV output format.src/lib/compiler/stages/assets.ts— asset extraction stagesrc/commands/package.ts— binary asset pre-extraction during packagingBehavior change (intentional)
exceljsis stricter than SheetJS — passing arbitrary non-Excel content to a.xlsxextension now throws instead of silently parsing it as a single-sheet CSV. This is a security improvement. The one test that asserted the lenient behavior was updated to assert the throw, matching the existingextractWord/extractPdfpattern.Test plan
npm audit— 0 vulnerabilities (down from 1)npm run build— passesnpm test— 20/20 suites pass, 331 tests pass.prmdthat references a real.xlsxasset🤖 Generated with Claude Code