🛡️ Sentinel: [HIGH] Fix Zip Slip in Addon Export#524
Conversation
|
👋 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 aims to harden the backend’s addon export pipeline against Zip Slip (path traversal) by sanitizing asset filenames before writing them into the exported .mcaddon (ZIP) and aligning generated JSON naming with sanitized values.
Changes:
- Added a
_sanitize_filenamehelper and applied it to asset filenames used for ZIP entry paths. - Updated texture/sound naming derivation in generated JSON to use sanitized filenames.
- Added a unit test to assert exported ZIPs don’t contain traversal paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
backend/src/services/addon_exporter.py |
Introduces filename sanitization and applies it when generating ZIP entry paths and JSON-derived names. |
backend/src/tests/unit/test_addon_exporter_security.py |
Adds a regression test intended to catch Zip Slip attempts during mcaddon creation. |
.jules/sentinel.md |
Documents the Zip Slip learning/prevention guidance for future Sentinel runs. |
| filename = "".join(c for c in filename if c.isalnum() or c in "._-") | ||
| # Ensure not empty | ||
| if not filename: | ||
| return "default_sanitized_filename" |
There was a problem hiding this comment.
_sanitize_filename still allows dangerous path segments like . and .. because dots are allowlisted. If filename is .. (or sanitizes down to ..), os.path.join(..., "..") will produce ZIP entries containing ../ after normalization by many extractors, so Zip Slip is still possible. Consider explicitly rejecting ./.. (and optionally any name that becomes only dots) after sanitization and falling back to a safe generated name.
| return "default_sanitized_filename" | |
| return "default_sanitized_filename" | |
| # Reject filenames that are only dots (e.g. ".", "..", "...") | |
| if all(c == "." for c in filename): | |
| return f"default_sanitized_{uuid.uuid4().hex}" |
| # or we derive from original_filename. | ||
|
|
||
| texture_name = os.path.splitext(asset.original_filename)[0] if asset.original_filename else \ | ||
| os.path.splitext(os.path.basename(asset.path))[0] | ||
| original_name = _sanitize_filename(asset.original_filename) if asset.original_filename else \ | ||
| _sanitize_filename(os.path.basename(asset.path)) | ||
| texture_name = os.path.splitext(original_name)[0] |
There was a problem hiding this comment.
generate_terrain_texture_json is called with texture_assets = [asset for asset in addon_pydantic.assets if asset.type.startswith("texture")] (so it includes texture_block / texture_item), but this function still only processes assets where asset.type == "texture". As a result, terrain_texture.json will likely be generated with an empty texture_data for most real texture assets. Updating the condition to match the caller’s selection (e.g., startswith("texture") or handling the texture_* variants) should fix this.
| # Malicious filename attempting traversal | ||
| malicious_filename = "../../../../malicious.txt" | ||
|
|
There was a problem hiding this comment.
The test only covers a traversal attempt using ../../.. in the filename. Since _sanitize_filename currently allowlists dots, a filename like .. (or one that sanitizes down to ..) would be a more direct regression case to add here, because it can still introduce .. path segments into ZIP entry names.
- Implemented `_sanitize_filename` in `backend/src/services/addon_exporter.py` to strip directory paths and restrict characters to alphanumeric, `.`, `_`, `-`. - Applied sanitization to all asset types (textures, sounds, models, entities) in `create_mcaddon_zip`. - Updated JSON manifest generation (`terrain_texture.json`, `sounds.json`) to use sanitized filenames for consistency. - Added regression test `backend/src/tests/unit/test_addon_exporter_security.py` demonstrating the vulnerability prevention. This prevents malicious filenames (e.g., `../../file`) from being included in exported mcaddon/zip files, which could lead to arbitrary file writes upon extraction. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
- Resolved `ModuleNotFoundError: No module named 'ai_engine'` in CI by correcting import paths in `ai-engine/tests/`. Removed `ai_engine.` prefix as the package root is `ai-engine/` and tests run from there. - Fixed `_calculate_load_order` in `mod_dependency_analyzer.py` to correctly order dependencies before dependents (topological sort was using in-degree instead of out-degree logic). - Updated `KNOWN_CONFLICTS` in `modpack_conflict_detector.py` to correctly identify conflicts between JEI and REI by adding aliases. - Verified all affected tests pass locally. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
…pload.tsx - Removed unnecessary escape characters in regex patterns in `frontend/src/utils/urlParser.ts` to fix ESLint errors (`no-useless-escape`). - Fixed a syntax error in `frontend/src/components/ConversionUpload/ConversionUpload.tsx` where a `)` was missing in a `getPlatformInfo` call. - Installed missing dev dependency `eslint-plugin-storybook` to enable linting locally. - Verified linting passes with `npm run lint`. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
985fee7 to
de1bb10
Compare
Fixed a High/Critical Zip Slip vulnerability in the addon export functionality. Maliciously crafted asset filenames could lead to path traversal when the exported archive is extracted.
Changes:
_sanitize_filenamehelper inaddon_exporter.py.original_filenamefor all assets before adding to ZIP.test_addon_exporter_security.pyto verify the fix.PR created automatically by Jules for task 17170767585595288513 started by @anchapin