🛡️ Sentinel: [CRITICAL] Fix Zip Slip (path traversal) vulnerability in zip extraction#33
🛡️ Sentinel: [CRITICAL] Fix Zip Slip (path traversal) vulnerability in zip extraction#33
Conversation
Deploying schoolmap with
|
| Latest commit: |
8a5685e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c82d0f0.schoolmap.pages.dev |
| Branch Preview URL: | https://jules-sentinel-zip-slip-fix.schoolmap.pages.dev |
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical Zip Slip (path traversal) risk in the repository’s GeoNames data-processing scripts by replacing adm-zip bulk extraction with per-entry validation to ensure ZIP entries cannot write outside the intended destination directory.
Changes:
- Replaced
adm-zip’sextractAllTo(...)usage with a manual extraction loop that skips entries resolving outside the destination directory. - Added a standalone Node test script to simulate a malicious ZIP entry and verify traversal is blocked.
- Added a
test:zipslipscript topackage.jsonto make the check runnable in CI.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/verifyzipslipfix.cjs | Adds a regression test that simulates a traversal entry and asserts it is blocked. |
| scripts/processGeoNames.cjs | Implements per-entry path validation during ZIP extraction to prevent Zip Slip. |
| scripts/processFullBulgaria.cjs | Implements the same safe extraction logic for BG.zip processing. |
| scripts/processAllCountries.cjs | Implements the same safe extraction logic for multi-country ZIP processing. |
| package.json | Adds test:zipslip script to run the new verification test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Ensure destDir is an absolute path with a trailing separator for safe prefix checking | ||
| const resolvedDest = path.resolve(destDir) + path.sep; | ||
|
|
||
| for (const entry of zip.getEntries()) { | ||
| const targetPath = path.resolve(destDir, entry.entryName); | ||
|
|
||
| // SECURITY: Prevent Zip Slip path traversal vulnerability | ||
| if (!targetPath.startsWith(resolvedDest)) { | ||
| console.warn(`[SECURITY WARNING] Skipping malicious zip entry attempting path traversal: ${entry.entryName}`); | ||
| continue; | ||
| } | ||
|
|
||
| zip.extractEntryTo(entry, destDir, true, true); | ||
| } |
There was a problem hiding this comment.
The Zip Slip prevention logic is repeated in multiple processing scripts. To keep the security check consistent, consider centralizing it in a shared helper (scripts/utils/...) and calling it from here.
| // Create a zip with a malicious path traversal entry | ||
| const zip = new AdmZip(); | ||
| zip.addFile('safe.txt', Buffer.from('safe')); | ||
| zip.getEntries()[0].entryName = '../../malicious.txt'; | ||
|
|
||
| const testDir = path.resolve(__dirname, 'zip-slip-test'); | ||
| if (!fs.existsSync(testDir)) fs.mkdirSync(testDir, { recursive: true }); | ||
|
|
||
| const zipPath = path.join(testDir, 'test-malicious.zip'); | ||
| zip.writeZip(zipPath); | ||
|
|
||
| const destDir = path.join(testDir, 'extract'); | ||
| if (!fs.existsSync(destDir)) fs.mkdirSync(destDir, { recursive: true }); | ||
|
|
||
| try { | ||
| const blocked = await extractZipSafe(zipPath, destDir); | ||
| assert.strictEqual(blocked, true, "Malicious entry should have been blocked"); | ||
|
|
||
| // Verify the malicious file wasn't extracted | ||
| const maliciousPath = path.resolve(destDir, '../../malicious.txt'); | ||
| assert.strictEqual(fs.existsSync(maliciousPath), false, "Malicious file was extracted!"); |
There was a problem hiding this comment.
This test only verifies that a malicious entry is skipped, but it doesn’t validate that a normal (non-traversal) entry still extracts successfully. Consider adding both a safe entry (e.g. nested path under destDir) and a malicious entry to the zip, then asserting the safe file exists inside destDir after extraction and the malicious path does not.
| "build": "tsc -b && vite build", | ||
| "lint": "eslint .", | ||
| "preview": "vite preview" | ||
| "preview": "vite preview", |
There was a problem hiding this comment.
Adding only a namespaced script (test:zipslip) may not be picked up by CI setups that run the default npm test / pnpm test. Consider also adding/updating a top-level test script to run this check (and any other tests) so it is enforced consistently.
| "preview": "vite preview", | |
| "preview": "vite preview", | |
| "test": "npm run test:zipslip", |
| // Ensure destDir is an absolute path with a trailing separator for safe prefix checking | ||
| const resolvedDest = path.resolve(destDir) + path.sep; | ||
|
|
||
| for (const entry of zip.getEntries()) { | ||
| const targetPath = path.resolve(destDir, entry.entryName); | ||
|
|
||
| // SECURITY: Prevent Zip Slip path traversal vulnerability | ||
| if (!targetPath.startsWith(resolvedDest)) { | ||
| console.warn(`[SECURITY WARNING] Skipping malicious zip entry attempting path traversal: ${entry.entryName}`); | ||
| continue; | ||
| } | ||
|
|
||
| zip.extractEntryTo(entry, destDir, true, true); | ||
| } |
There was a problem hiding this comment.
The new safe-zip extraction logic is duplicated here (and in the other GeoNames scripts). Consider moving this loop into a shared helper (e.g., scripts/utils/extractZipSafe.cjs) to avoid the three implementations drifting over time and to make future security fixes one-shot.
| // Ensure destDir is an absolute path with a trailing separator for safe prefix checking | ||
| const resolvedDest = path.resolve(destDir) + path.sep; | ||
|
|
||
| for (const entry of zip.getEntries()) { | ||
| const targetPath = path.resolve(destDir, entry.entryName); | ||
|
|
||
| // SECURITY: Prevent Zip Slip path traversal vulnerability | ||
| if (!targetPath.startsWith(resolvedDest)) { | ||
| console.warn(`[SECURITY WARNING] Skipping malicious zip entry attempting path traversal: ${entry.entryName}`); | ||
| continue; | ||
| } | ||
|
|
||
| zip.extractEntryTo(entry, destDir, true, true); | ||
| } |
There was a problem hiding this comment.
This safe-zip extraction loop is duplicated across multiple scripts in this PR. To reduce maintenance risk (especially for security logic), consider extracting it into a shared utility under scripts/utils and reusing it from each script.
🚨 Severity: CRITICAL
💡 Vulnerability: The backend data processing scripts (
scripts/processGeoNames.cjs,scripts/processAllCountries.cjs,scripts/processFullBulgaria.cjs) were usingadm-zip'sextractAllTofunction. This function does not validate if the entries within the zip archive are attempting to break out of the target directory (e.g. using../../). This is known as a "Zip Slip" vulnerability, which could lead to arbitrary file writes during data processing.🎯 Impact: If a malicious ZIP file were substituted or fetched during the build process, it could overwrite critical system files or inject malicious code onto the host server/machine running the build step.
🔧 Fix: Replaced
zip.extractAllTowith a secure manual extraction loop. The code now iterates over each entry, constructs thetargetPath, and strictly checks if it falls within the intendedresolvedDestdirectory. Any entry attempting path traversal is safely skipped.✅ Verification:
tests/verifyzipslipfix.cjs) that simulates a malicious zip file. It ensures the traversal is successfully blocked."test:zipslip"topackage.jsonfor CI enforcement.pnpm lint,pnpm test, andpnpm build.PR created automatically by Jules for task 3361689943883600975 started by @dttdrv