fix(exporters): prevent zip-slip via asset path traversal#72
Merged
Conversation
`exportZip` previously joined caller-supplied `asset.path` straight onto the staging dir, so an asset like `../escape.txt` (or `assets/../../x`) could write outside the staging area and embed escaping paths in the archive. Resolve the target and verify it stays under the staging dir; throw EXPORTER_ZIP_UNSAFE_PATH otherwise. Drop dead `typeof === 'string' ? a : a` ternary while here. Tests: add traversal cases; existing tests still pass.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Findings
- [Blocker] Backslash-based traversal can still bypass ZIP entry validation on Windows-style paths — the guard validates the resolved staging path, but
safeRelis written to the archive unchanged. Inputs likeassets\..\..\escape.txtcan remain inside staging on POSIX while still becoming traversal-like ZIP entry names for some extractors, so zip-slip risk is not fully closed. Evidence:packages/exporters/src/zip.ts:75,packages/exporters/src/zip.ts:86
Suggested fix:const safeRel = asset.path.replace(/^\/+/, ''); const normalizedRel = path.posix.normalize(safeRel.replace(/\\/g, '/')); if ( !normalizedRel || normalizedRel === '.' || normalizedRel.startsWith('../') || normalizedRel.includes('/../') || normalizedRel.startsWith('/') ) { throw new CodesignError( `ZIP export rejected unsafe asset path: ${asset.path}`, 'EXPORTER_ZIP_UNSAFE_PATH', ); } const localPath = path.resolve(stagingDir, normalizedRel); // ...keep resolved-path containment check... zip.addFile(localPath, normalizedRel);
Summary
- Review mode: initial
- 1 blocker found in latest diff: ZIP path traversal mitigation is incomplete for backslash-based paths.
- Required docs
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
- Add a Vitest case for
assets\\..\\..\\escape.txtexpectingEXPORTER_ZIP_UNSAFE_PATH.
open-codesign Bot
| if (opts.assets) { | ||
| const stagingResolved = path.resolve(stagingDir); | ||
| for (const asset of opts.assets) { | ||
| const safeRel = asset.path.replace(/^\/+/, ''); |
Contributor
There was a problem hiding this comment.
asset.path is normalized only for leading /, but backslashes are left as-is. On Windows-style paths, inputs like assets\\..\\..\\escape.txt can still become traversal-like archive entry names because zip.addFile(..., safeRel) uses the unsanitized entry path. Normalize separators to / and reject any .. segments in the archive entry path before writing to disk and adding to ZIP.
ZIP entries authored on Windows can carry `..\..\etc\passwd` style backslash separators. On POSIX, `path.resolve` treats `\` as a literal character, so the previous containment check on the resolved path missed this vector. Normalize backslashes to forward slashes before resolving so the relative-path guard sees the real traversal.
Contributor
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: follow-up after new commits
- No issues found in the latest diff at this head.
- Residual risk/testing gap: automated checks were not run in this review environment; assessment is based on static diff inspection only.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
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
exportZipjoined caller-suppliedasset.pathdirectly onto the staging dir, so paths like../escape.txtorassets/../../xwould write outside the staging directory and end up at escaping locations inside the archive (zip-slip).EXPORTER_ZIP_UNSAFE_PATHotherwise. Re-throwCodesignErrorfrom outer catch so the wrapper does not mask the explicit code.typeof asset.content === 'string' ? a : aternary while here.Test plan
pnpm --filter @open-codesign/exporters test -- --run zip.test(4/4 pass, includes new traversal cases)PRINCIPLES checks