diff --git a/.jules/sentinel.md b/.jules/sentinel.md index cc4bb6ba..a6d34e79 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,11 @@ 3. Disable automatic redirect following (`follow_redirects=False`). 4. Manually handle redirects, re-validating the URL at each hop. 5. Sanitize URLs before logging to avoid leaking sensitive tokens. + +## 2026-02-16 - Zip Slip Prevention in Addon Export +**Vulnerability:** Zip Slip vulnerability in `addon_exporter.py` allowing directory traversal via malicious filenames in exported zip archives. +**Learning:** Using `os.path.join` with unsanitized user input inside `zipfile` operations can lead to files being written outside the intended directory upon extraction. Even if the path doesn't escape the zip root, `../` segments can be dangerous. +**Prevention:** +1. Sanitize all filenames used in `zipfile` operations using `os.path.basename` to strip directory components. +2. Restrict filenames to a safe allowlist (alphanumeric, `._-`). +3. Apply sanitization consistently across all file types (textures, sounds, models, etc.). diff --git a/ai-engine/agents/mod_dependency_analyzer.py b/ai-engine/agents/mod_dependency_analyzer.py index 890d7941..d4a23c12 100644 --- a/ai-engine/agents/mod_dependency_analyzer.py +++ b/ai-engine/agents/mod_dependency_analyzer.py @@ -432,72 +432,41 @@ def _find_missing_dependencies( def _calculate_load_order(self, graph: DependencyGraph) -> List[str]: """ Calculate optimal load order using topological sort. - + Mods with no dependencies come first, followed by their dependents. """ - # Calculate in-degree for each mod - # We want dependencies to be loaded BEFORE dependents. - # Standard topological sort on A->B (A depends on B) gives B then A if we consider edges as "must load before". - # But our graph.edges are "A depends on B". - # So A needs B. B must come first. - # In-degree based on "Depends On" edges: - # A->B. In-degree(A)=0, In-degree(B)=1. - # Queue initial: [A]. Pop A. Decrement B. Queue: [B]. Result: A, B. (Dependent, Dependency) -> WRONG. - - # We want to reverse the logic: A depends on B implies B->A dependency edge for load order. - # Or simpler: count in-degree using reverse_edges (B->A). - # B->A. In-degree(B)=0, In-degree(A)=1. - # Queue initial: [B]. Pop B. Decrement A. Queue: [A]. Result: B, A. (Dependency, Dependent) -> CORRECT. - - in_degree = {mod_id: 0 for mod_id in graph.mods} - for mod_id in graph.mods: - # Use reverse_edges to count incoming dependencies (i.e. number of mods that must load before this one) - # Actually, reverse_edges[B] = [A] means A depends on B. - # If we iterate graph.edges (A->B), we are saying A needs B. - # So B is a prerequisite for A. - # We want to find mods that have NO prerequisites first. - # A mod has no prerequisites if it has no outgoing "depends on" edges? - # A->B. A has 1 outgoing. B has 0 outgoing. - # B is a leaf in "Depends On" graph. - # Topological sort usually processes nodes with in-degree 0. - # If we reverse the graph (B->A, B is prerequisite for A), then B has in-degree 0. - pass - - # Let's count "how many unsatisfied dependencies does this mod have?" - # If A depends on B (A->B), A has 1 dependency. B has 0. - # We should load B first. - # So we want to pick mods with 0 unsatisfied dependencies. - # That corresponds to out-degree in "Depends On" graph (A->B). - # A has out-degree 1. B has out-degree 0. - # So we pick B. - # When B is picked, we satisfy dependency for A. A's count becomes 0. Pick A. - # So we need to track out-degree. - + # Calculate dependency count (out-degree) for each mod dependency_count = {mod_id: 0 for mod_id in graph.mods} - dependents_map = defaultdict(list) # Who depends on key? - for mod_id in graph.mods: - dependencies = graph.edges.get(mod_id, []) - dependency_count[mod_id] = len(dependencies) - for dep_id in dependencies: - dependents_map[dep_id].append(mod_id) - - # Start with mods that have no dependencies (out-degree 0) + # Only count required dependencies that exist in the graph + # This handles optional dependencies or dependencies not in the list + count = 0 + for dep_id in graph.edges.get(mod_id, []): + if dep_id in graph.mods: + count += 1 + dependency_count[mod_id] = count + + # Start with mods that have no dependencies (or 0 remaining dependencies) queue = [mod_id for mod_id, count in dependency_count.items() if count == 0] load_order = [] - + while queue: # Sort to ensure consistent ordering queue.sort(key=lambda x: graph.mods.get(x, ModInfo(mod_id=x, name=x)).name) current = queue.pop(0) load_order.append(current) - - # For every mod that depends on current, decrement their dependency count - for dependent in dependents_map.get(current, []): - dependency_count[dependent] -= 1 - if dependency_count[dependent] == 0: - queue.append(dependent) - + + # Identify mods that depend on the current mod + # If mod A depends on current, we reduce A's dependency count + # In our graph: A -> current (edge A->current exists) + # So we need to find X such that X -> current. + # This is exactly what reverse_edges stores: reverse_edges[current] = [A] + for dependent in graph.reverse_edges.get(current, []): + if dependent in dependency_count: + dependency_count[dependent] -= 1 + if dependency_count[dependent] == 0: + queue.append(dependent) + # If we haven't visited all mods, there are cycles if len(load_order) != len(graph.mods): self.logger.warning( @@ -507,10 +476,10 @@ def _calculate_load_order(self, graph: DependencyGraph) -> List[str]: for mod_id in graph.mods: if mod_id not in load_order: load_order.append(mod_id) - + return load_order - - def _generate_warnings( + + def _generate_warnings( self, graph: DependencyGraph, circular_deps: List[CircularDependency], diff --git a/backend/src/services/addon_exporter.py b/backend/src/services/addon_exporter.py index 33902488..d76ae108 100644 --- a/backend/src/services/addon_exporter.py +++ b/backend/src/services/addon_exporter.py @@ -8,6 +8,22 @@ from models import addon_models as pydantic_addon_models # For type hinting with Pydantic models +def _sanitize_filename(filename: str) -> str: + """ + Sanitizes a filename to prevent path traversal and remove dangerous characters. + """ + if not filename: + return "default_filename" + # Remove directory paths + filename = os.path.basename(filename) + # Remove dangerous characters + # Allow alphanumeric, dot, underscore, hyphen + filename = "".join(c for c in filename if c.isalnum() or c in "._-") + # Ensure not empty + if not filename: + return "default_sanitized_filename" + return filename + # Constants for manifest versions, can be updated as needed MIN_ENGINE_VERSION_RP = [1, 16, 0] MIN_ENGINE_VERSION_BP = [1, 16, 0] @@ -171,8 +187,9 @@ def generate_terrain_texture_json(addon_assets: List[pydantic_addon_models.Addon # asset.path might be "textures/blocks/my_block.png" (if stored with this intent) # 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] # Path in terrain_texture.json is relative to the "textures" folder of the RP # e.g., "textures/blocks/my_custom_block_texture" @@ -245,11 +262,14 @@ def generate_sounds_json(sound_assets: List[pydantic_addon_models.AddonAsset]) - for asset in sound_assets: # Extract sound name from original_filename (without extension) - sound_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)) + sound_name = os.path.splitext(original_name)[0] # Get the sound file path relative to sounds folder - sound_path = asset.original_filename if asset.original_filename else os.path.basename(asset.path) + # Note: In Bedrock resource packs, the "name" in sounds.json is typically the path without extension, + # relative to the root of the RP (but usually matching the folder structure). + # We'll stick to the previous logic but with sanitized name. sounds_data[sound_name] = { "sounds": [ @@ -344,9 +364,9 @@ def create_mcaddon_zip( zip_texture_path_parts = ["textures", "misc"] if asset.original_filename: - zip_texture_path_parts.append(asset.original_filename) + zip_texture_path_parts.append(_sanitize_filename(asset.original_filename)) else: # Fallback if original_filename is somehow missing - zip_texture_path_parts.append(os.path.basename(asset.path)) + zip_texture_path_parts.append(_sanitize_filename(os.path.basename(asset.path))) zip_path = os.path.join(rp_folder_name, *zip_texture_path_parts) @@ -364,10 +384,9 @@ def create_mcaddon_zip( asset_disk_path = os.path.join(asset_base_path, str(addon_pydantic.id), asset.path) # Determine path within ZIP for RP - if asset.original_filename: - zip_sound_path = os.path.join(rp_folder_name, "sounds", asset.original_filename) - else: - zip_sound_path = os.path.join(rp_folder_name, "sounds", os.path.basename(asset.path)) + safe_filename = _sanitize_filename(asset.original_filename) if asset.original_filename else \ + _sanitize_filename(os.path.basename(asset.path)) + zip_sound_path = os.path.join(rp_folder_name, "sounds", safe_filename) if os.path.exists(asset_disk_path): zf.write(asset_disk_path, zip_sound_path) @@ -386,10 +405,9 @@ def create_mcaddon_zip( asset_disk_path = os.path.join(asset_base_path, str(addon_pydantic.id), asset.path) # Determine path within ZIP for RP - if asset.original_filename: - zip_model_path = os.path.join(rp_folder_name, "models", asset.original_filename) - else: - zip_model_path = os.path.join(rp_folder_name, "models", os.path.basename(asset.path)) + safe_filename = _sanitize_filename(asset.original_filename) if asset.original_filename else \ + _sanitize_filename(os.path.basename(asset.path)) + zip_model_path = os.path.join(rp_folder_name, "models", safe_filename) if os.path.exists(asset_disk_path): zf.write(asset_disk_path, zip_model_path) @@ -402,10 +420,9 @@ def create_mcaddon_zip( for asset in entity_assets: asset_disk_path = os.path.join(asset_base_path, str(addon_pydantic.id), asset.path) - if asset.original_filename: - zip_entity_path = os.path.join(rp_folder_name, "entity", asset.original_filename) - else: - zip_entity_path = os.path.join(rp_folder_name, "entity", os.path.basename(asset.path)) + safe_filename = _sanitize_filename(asset.original_filename) if asset.original_filename else \ + _sanitize_filename(os.path.basename(asset.path)) + zip_entity_path = os.path.join(rp_folder_name, "entity", safe_filename) if os.path.exists(asset_disk_path): zf.write(asset_disk_path, zip_entity_path) diff --git a/backend/src/tests/unit/test_addon_exporter_security.py b/backend/src/tests/unit/test_addon_exporter_security.py new file mode 100644 index 00000000..fe518473 --- /dev/null +++ b/backend/src/tests/unit/test_addon_exporter_security.py @@ -0,0 +1,72 @@ + +import pytest +import os +import io +import zipfile +import uuid +import datetime +from unittest.mock import MagicMock, patch +from services import addon_exporter +from models import addon_models as pydantic_addon_models + +def test_create_mcaddon_zip_prevents_zip_slip(tmp_path): + """ + Test that create_mcaddon_zip prevents Zip Slip vulnerability + by sanitizing asset filenames. + """ + # Setup mock data + mock_addon_id = uuid.uuid4() + mock_asset_base_path = str(tmp_path / "addon_assets") + mock_addon_asset_dir = os.path.join(mock_asset_base_path, str(mock_addon_id)) + os.makedirs(mock_addon_asset_dir, exist_ok=True) + + # Create a dummy asset file on disk + asset_path_on_disk = f"{uuid.uuid4()}_malicious.txt" + full_asset_path = os.path.join(mock_addon_asset_dir, asset_path_on_disk) + with open(full_asset_path, "w") as f: + f.write("malicious content") + + # Malicious filename attempting traversal + malicious_filename = "../../../../malicious.txt" + + mock_addon = pydantic_addon_models.AddonDetails( + id=mock_addon_id, + name="Malicious Addon", + description="Testing Zip Slip", + user_id="hacker", + created_at=datetime.datetime.now(), + updated_at=datetime.datetime.now(), + blocks=[], + assets=[ + pydantic_addon_models.AddonAsset( + id=uuid.uuid4(), + addon_id=mock_addon_id, + created_at=datetime.datetime.now(), + updated_at=datetime.datetime.now(), + type="texture_block", # Uses "textures/blocks" path + path=asset_path_on_disk, + original_filename=malicious_filename + ) + ], + recipes=[] + ) + + # Execute the vulnerable function + zip_buffer = addon_exporter.create_mcaddon_zip(mock_addon, mock_asset_base_path) + + # Verify the vulnerability is prevented + found_traversal = False + found_sanitized = False + with zipfile.ZipFile(zip_buffer, "r") as zf: + for name in zf.namelist(): + # Check if any file path contains traversal characters + if "../" in name or "..\\" in name: + found_traversal = True + + # Check if the sanitized version exists (malicious.txt inside textures/blocks) + if "malicious.txt" in name and not ("../" in name): + found_sanitized = True + + # After the fix, we expect NO traversal and YES sanitized file + assert not found_traversal, "Vulnerability persisted: Traversal path found in zip" + assert found_sanitized, "Sanitized file not found in zip" diff --git a/frontend/package-lock.json b/frontend/package-lock.json index b1e86179..d4286a8b 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -45,7 +45,7 @@ "eslint-plugin-prettier": "^5.5.5", "eslint-plugin-react-hooks": "^7.0.1", "eslint-plugin-react-refresh": "^0.5.0", - "eslint-plugin-storybook": "^10.2.8", + "eslint-plugin-storybook": "^10.2.9", "globals": "^17.3.0", "jsdom": "^28.1.0", "msw": "^2.12.10", @@ -5179,9 +5179,9 @@ } }, "node_modules/eslint-plugin-storybook": { - "version": "10.2.8", - "resolved": "https://registry.npmjs.org/eslint-plugin-storybook/-/eslint-plugin-storybook-10.2.8.tgz", - "integrity": "sha512-BtysXrg1RoYT3DIrCc+svZ0+L3mbWsu7suxTLGrihBY5HfWHkJge+qjlBBR1Nm2ZMslfuFS5K0NUWbWCJRu6kg==", + "version": "10.2.9", + "resolved": "https://registry.npmjs.org/eslint-plugin-storybook/-/eslint-plugin-storybook-10.2.9.tgz", + "integrity": "sha512-nmPxjPw2KfmosqAUb/W0jmEfAZzK97kyJ8W5KMuweCblwjIL0hI/GMsWSP8CCBPnhQ9LnuxtT8JtQUOsslcbwA==", "dev": true, "license": "MIT", "dependencies": { @@ -5189,7 +5189,7 @@ }, "peerDependencies": { "eslint": ">=8", - "storybook": "^10.2.8" + "storybook": "^10.2.9" } }, "node_modules/eslint-plugin-storybook/node_modules/@typescript-eslint/utils": { diff --git a/frontend/package.json b/frontend/package.json index 21a26eb6..125b6959 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -59,7 +59,7 @@ "eslint-plugin-prettier": "^5.5.5", "eslint-plugin-react-hooks": "^7.0.1", "eslint-plugin-react-refresh": "^0.5.0", - "eslint-plugin-storybook": "^10.2.8", + "eslint-plugin-storybook": "^10.2.9", "globals": "^17.3.0", "jsdom": "^28.1.0", "msw": "^2.12.10", @@ -72,4 +72,4 @@ "vitest": "^4.0.7", "web-streams-polyfill": "^4.2.0" } -} \ No newline at end of file +}