T1334951 - devextreme-exceljs-fork - Path traversal in Workbook.addImage({ filename }) - #155
Conversation
bit-byte0
added
bug
|
Size Change: +1.92 kB (+0.13%) Total Size: 1.45 MB 📦 View Changed
|
bit-byte0
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The traversal validation is bypassable via non-string fs.readFile inputs and is not yet applied/covered for the streaming WorkbookWriter code path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR mitigates a path traversal risk where Workbook.addImage({ filename }) could pass attacker-controlled paths to fs.readFile, enabling unintended file reads and embedding into generated XLSX files.
Changes:
- Introduces
utils.assertSafeMediaPath(filename)and enforces it inWorkbook#addImageand the XLSX media write path. - Adds unit and integration coverage for rejecting filenames containing
..path segments. - Updates integration tests to use
path.resolvefor stable image fixture paths and documents the new throwing behavior in the TypeScript declarations.
File summaries
| File | Description |
|---|---|
| spec/unit/utils/utils.spec.js | Adds unit coverage for assertSafeMediaPath behavior. |
| spec/integration/workbook/images.spec.js | Adds integration regression test for Workbook#addImage traversal rejection. |
| spec/integration/workbook-xlsx-writer/workbook-xlsx-writer.spec.js | Updates fixture path handling for streaming writer integration tests. |
| lib/xlsx/xlsx.js | Enforces safe-path assertion before reading media from disk during XLSX write. |
| lib/utils/utils.js | Introduces assertSafeMediaPath helper for traversal-segment rejection. |
| lib/doc/workbook.js | Calls safe-path assertion in Workbook#addImage. |
| index.d.ts | Documents that addImage can throw on traversal segments. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The added traversal guard leaves other user-controlled path components (notably image.extension) unvalidated even though they are interpolated into XLSX internal paths, which can still permit path-segment injection in generated archives.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The security checks are consistently applied at the relevant entry points and write-time read sites, with accompanying unit and integration tests.
Review details
Suppressed comments (1)
lib/utils/utils.js:83
assertSafeMediaPaththrows an error that says "Unsafe image filename", but this helper is named for (and used as) a general media guard. Align the error message with the function/API naming to reduce confusion when it’s thrown from non-image media contexts.
throw new Error(`Unsafe image filename: expected a string but received ${typeof filename}`);
}
if (filename.split(/[\\/]/).includes('..')) {
throw new Error(`Unsafe image filename "${filename}": path traversal ("..") segments are not allowed`);
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new safeJoin assertions hard-code POSIX expectations that will fail on Windows CI, and the TypeScript export for utils.safeJoin doesn’t align with the browser/bare entrypoints.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new ExcelJS.utils.safeJoin typing/export appears to be Node-only while the browser/bare bundles don’t export utils, creating a likely runtime/type mismatch for browser consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lib/utils/utils.js:74
getRelsPathnow uses a local variable namedpath, which shadows the newly imported Nodepathmodule in this file and makes the code harder to read/maintain (especially with the newsafeJoinimplementation that relies onpath). Rename the local variable to avoid the shadowing.
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new typings and Node-only ExcelJS.utils.safeJoin export currently mismatch the browser entrypoint/runtime surface and the updated addImage throw behavior is under-documented in index.d.ts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Lite
…er bundle (T1334951)
| }); | ||
| ``` | ||
|
|
||
| `addImage` validates `filename` and `extension` values to prevent path traversal attacks. These parameters cannot contain the following strings: `..`, `\`, and `/`. You can also use `ExcelJS.utils.safeJoin()` instead of `path.join()` to validate file paths upon generation. |
There was a problem hiding this comment.
| `addImage` validates `filename` and `extension` values to prevent path traversal attacks. These parameters cannot contain the following strings: `..`, `\`, and `/`. You can also use `ExcelJS.utils.safeJoin()` instead of `path.join()` to validate file paths upon generation. | |
| `addImage` validates `filename` and `extension` values to prevent path traversal attacks. These parameters cannot contain the following strings: | |
| `filename`: `\0` and `..` | |
| `extension`: `\0`, `..`, `\`, and `/` | |
| You can also use `ExcelJS.utils.safeJoin()` instead of `path.join()` to validate file paths upon generation. |
| const hasTraversalSegment = /(^|[\\/])\.\.([\\/]|$)/.test(filename); | ||
| const hasWindowsDriveRelative = /^[a-zA-Z]:\.\./.test(filename); | ||
| if (hasTraversalSegment || hasWindowsDriveRelative) { | ||
| throw new Error(`File path cannot contain "..", "/", or "\\": "${filename}"`); |
There was a problem hiding this comment.
| throw new Error(`File path cannot contain "..", "/", or "\\": "${filename}"`); | |
| throw new Error(`File path cannot contain "..": "${filename}"`); |
| /** | ||
| * Add Image to Workbook and return the id | ||
| * Adds Image to Workbook and returns an id. | ||
| * Fails if `img.filename` contains ".." |
There was a problem hiding this comment.
| * Fails if `img.filename` contains ".." | |
| * Fails if `img.filename` contains ".." or "\\0" | |
| * or if `img.extension` contains "..", "\\", "/", or "\\0" |
There was a problem hiding this comment.
🟡 Changes recommended
The new documentation/typings currently misstate or omit key details of the implemented validation rules (notably how .. is detected and the Node-only availability of safeJoin), which could mislead consumers of this security fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
|
|
||
| /** | ||
| * Add Image to Workbook and return the id | ||
| * Adds Image to Workbook and returns an id. |
There was a problem hiding this comment.
| * Adds Image to Workbook and returns an id. | |
| * Adds an image to the workbook and returns an ID. |
|
|
||
| export namespace utils { | ||
| /** | ||
| * Checks that `userPath` does not resolve to a path outside of `baseDir`. |
There was a problem hiding this comment.
| * Checks that `userPath` does not resolve to a path outside of `baseDir`. | |
| * Checks to ensure that `userPath` does not resolve to a path outside of `baseDir`. |
There was a problem hiding this comment.
🟡 Changes recommended
safeJoin can be bypassed through symlinked paths, and the documentation overstates validation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
README.md:2020
- The documented rule is broader than the implementation:
assertSafeMediaPathrejects..only when it forms a traversal segment, and the added unit test explicitly acceptsa..b.png. Please describe the segment rule rather than saying any..string is forbidden.
`filename`: `\0` and `..`
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
| `extension`: `\0`, `..`, `\`, and `/` | ||
|
|
||
| You can also use `ExcelJS.utils.safeJoin()` instead of `path.join()` to validate file paths upon generation. | ||
|
|
There was a problem hiding this comment.
| > [!Important] | |
| > `safeJoin()` cannot detect symbolic links. If you pass a symbolic link to this function that escapes the base directory, `safeJoin()` does not throw an error. | |
| /** | ||
| * Checks to ensure that `userPath` does not resolve to a path outside of `baseDir`. | ||
| * Use instead of `path.join` when you calculate a path for `addImage({ filename })` | ||
| * from user input. Fails if the resolved path escapes `baseDir`. |
There was a problem hiding this comment.
| * from user input. Fails if the resolved path escapes `baseDir`. | |
| * from user input. Fails if the resolved path escapes `baseDir`. | |
| * Cannot detect symbolic links that escape the base directory. |
| `filename`: `\0` and `..` | ||
| `extension`: `\0`, `..`, `\`, and `/` |
There was a problem hiding this comment.
| `filename`: `\0` and `..` | |
| `extension`: `\0`, `..`, `\`, and `/` | |
| - `filename`: `\0` and `..` | |
| - `extension`: `\0`, `..`, `\`, and `/` |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings remain for write-time regression coverage and documentation alignment.
Review details
Suppressed comments (5)
README.md:2020
- This list implies that any
..substring is forbidden infilename, while the guard deliberately accepts names such asa..b.pngand only rejects traversal segments. Clarify the README so users are not given a stricter contract than the implementation enforces.
`filename`: `\0` and `..`
README.md:2034
- The new standalone Node example leaves both
userInputandworkbookundefined, so copying it fails before demonstratingsafeJoin. Define the input and instantiate a workbook (or explicitly state that these come from surrounding application code).
const filename = ExcelJS.utils.safeJoin('/app/assets', userInput);
const imageId = workbook.addImage({ filename, extension: 'png' });
lib/stream/xlsx/workbook-writer.js:235
- The streaming writer's write-time guard is not covered by the new tests. Because
getImage()returns the stored media object, callers can mutatefilenameafter registration; add a regression test that mutates it beforecommit()and asserts the commit rejects the unsafe path, so this second enforcement point is protected from regressions.
mediaPathGuard.assertSafeImage(medium);
lib/xlsx/xlsx.js:429
- The write-time guard is not covered by the new integration tests.
getImage()exposes the stored media object, so a caller can changefilenameafteraddImage()has returned; add a regression test that mutates that object before writing and verifies the write rejects the unsafe path, otherwise this security check could regress while the current entry-point tests still pass.
mediaPathGuard.assertSafeImage(medium);
package.json:3
- Please keep the package version unchanged in a feature/fix PR. The repository guidance in
README.md:64-65says versions are updated at release time to avoid merge collisions, so this security change should not bump the package from 4.4.13 here.
"version": "4.5.0",
- Files reviewed: 12/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Absolute paths remain permitted, leaving the arbitrary-file-read issue unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
README.md:2020
- The guard accepts filenames such as
a..b.pngbecause it only rejects..path-traversal segments (as the new unit test onmedia-path-guard.spec.js:11confirms), so this documentation overstates the restriction. Describe the filename rule in terms of traversal segments rather than saying it cannot contain the..string anywhere.
- `filename`: `\0` and `..`
README.md:2034
workbookis not declared in this newly added standalone Node.js example, so copying it produces aReferenceErroraftersafeJoinsucceeds. Instantiate the workbook before callingaddImageor show the required existing setup.
const imageId = workbook.addImage({ filename, extension: 'png' });
lib/stream/xlsx/workbook-writer.js:235
- The streaming test only verifies rejection during
addImage, not at this write site. Mutatingwb.getImage(id).filenameafter insertion is a supported way to reach this second defense; add a commit-time regression test so thezip.fileread path remains protected.
mediaPathGuard.assertSafeImage(medium);
lib/xlsx/xlsx.js:429
- The added entry-point tests do not exercise this second defense. A caller can mutate
wb.getImage(id).filenameafteraddImage, and this is the check that must prevent the subsequentfs.readFile; add a regression test that performs that mutation before writing so this protection cannot be removed without detection.
mediaPathGuard.assertSafeImage(medium);
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
What
addImage/addMedianow reject unsafe media paths (a..traversal segment, a null byte, or an extension containing path separators) across both the regular and streaming writers, closing the arbitrary file read reported in CVE-2026-78208 (CWE-73). A newutils.safeJoin(baseDir, userPath)helper is also exported so applications can safely combine a trusted base directory with user inputHow
a shared guard in utils.js runs at both
addImageentry points and theaddMediaread sites, andsafeJoinresolves the path then verifies it stays inside the given base directory