Fix zip-slip vulnerability in extractVsix (vibe-kanban)#10
Conversation
Replace AdmZip.extractAllTo with a per-entry loop that pre-computes each entry's resolved destination and an isSafe check against the target dir (with trailing separator to avoid prefix-confusion). The per-entry extract call and the policy for hostile entries (reject vs. skip) are left as TODO(human) pending a design decision. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d16f1aeb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const entry of entries) { | ||
| const destPath = resolve(targetDir, entry.entryName); | ||
| const isSafe = destPath === resolve(targetDir) || destPath.startsWith(safeRoot); | ||
|
|
||
| // TODO(human): handle this entry. | ||
| // If `isSafe`, extract it (use zip.extractEntryTo(entry, targetDir, true, true)). | ||
| // If not safe, decide what to do with the malicious entry — see Learn by Doing. |
There was a problem hiding this comment.
Restore VSIX extraction after path validation
The new loop validates each zip entry but never extracts anything, so extractVsix now returns a directory where no VSIX contents were unpacked. Every VSIX scan path in src/index.ts and src/cli.ts depends on this function, so analysis runs against an empty/nonexistent tree and silently produces empty or incomplete results. This is a universal functional regression introduced by this commit, not just a corner case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2978ce2 (now pushed): the loop body now calls zip.extractEntryTo(entry, targetDir, true, true) for safe entries and throws on unsafe ones, so extraction is restored and the regression is closed.
There was a problem hiding this comment.
Code Review
This pull request modifies the extractVsix function in src/analyzer/static.ts to mitigate Zip Slip vulnerabilities by validating entry paths. However, the current implementation is incomplete and introduces a regression because it removes the original extraction call and replaces it with a loop that only performs checks without actually extracting the files. Feedback indicates that the code should be updated to extract safe entries and throw an error if a malicious path is detected.
| for (const entry of entries) { | ||
| const destPath = resolve(targetDir, entry.entryName); | ||
| const isSafe = destPath === resolve(targetDir) || destPath.startsWith(safeRoot); | ||
|
|
||
| // TODO(human): handle this entry. | ||
| // If `isSafe`, extract it (use zip.extractEntryTo(entry, targetDir, true, true)). | ||
| // If not safe, decide what to do with the malicious entry — see Learn by Doing. | ||
| } |
There was a problem hiding this comment.
The implementation is incomplete and introduces a regression. While the safety check logic is correctly identified, the code currently only iterates through the entries without performing any extraction, leaving a TODO in place. This will cause the function to return a path to an empty directory, breaking the analyzer's subsequent steps. You should extract the entry if it is safe and throw an error if a malicious path is detected.
| for (const entry of entries) { | |
| const destPath = resolve(targetDir, entry.entryName); | |
| const isSafe = destPath === resolve(targetDir) || destPath.startsWith(safeRoot); | |
| // TODO(human): handle this entry. | |
| // If `isSafe`, extract it (use zip.extractEntryTo(entry, targetDir, true, true)). | |
| // If not safe, decide what to do with the malicious entry — see Learn by Doing. | |
| } | |
| for (const entry of entries) { | |
| const destPath = resolve(targetDir, entry.entryName); | |
| const isSafe = destPath === resolve(targetDir) || destPath.startsWith(safeRoot); | |
| if (!isSafe) { | |
| throw new Error("Security Error: Potential Zip Slip detected in entry " + entry.entryName); | |
| } | |
| zip.extractEntryTo(entry, targetDir, true, true); | |
| } |
There was a problem hiding this comment.
Addressed in 2978ce2 (now pushed). Final implementation matches the suggested form: throw on traversal entries, zip.extractEntryTo for safe ones. Fail-closed was chosen deliberately since this is a security analyzer — a hostile entry should abort the scan loudly, not be silently dropped.
Extract safe entries one at a time via extractEntryTo, and throw on any entry whose resolved path escapes targetDir. Fail-closed is the right policy here: extractVsix feeds a security analyzer, so a traversal entry is itself evidence of malicious intent — aborting the scan with a clear error gives the operator a louder signal than a console.warn buried in logs, and matches the existing failure shape (AdmZip already throws on corrupt archives). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
In
src/analyzer/static.ts~line 1046,AdmZip.extractAllTo(targetDir, overwrite=true)is called with no validation that extracted paths stay withintargetDir. A malicious VSIX could contain entries with paths like../../.bashrcthat escape the extraction directory.\n\nFix: after listing zip entries, verify thatpath.resolve(targetDir, entry.entryName)starts withpath.resolve(targetDir) + path.sepbefore extracting. Reject or skip any entry that fails this check.