mirror of
https://github.com/rookiestar28/ComfyUI-OpenClaw.git
synced 2026-08-14 00:48:07 +00:00
fix(security): add input validation to pack API route handlers
The `delete_pack_handler` and `export_pack_handler` in `api/packs.py` passed user-controlled URL route parameters (`name`, `version`) directly to `PackRegistry` methods without validation. An attacker with admin access could supply path traversal sequences to delete arbitrary directories or read arbitrary paths. This patch adds: - `_is_safe_pack_segment()` validator at the API layer rejecting empty values, `.`/`..`, slashes, backslashes, and characters outside `[a-zA-Z0-9._-]`. - Validation calls in both `delete_pack_handler` and `export_pack_handler` before any registry interaction. - Sanitization of the `Content-Disposition` header filename to prevent HTTP response header injection. - Regression tests in `test_packs_integrity.py` covering traversal rejection in both delete and export handlers, plus unit tests for the segment validator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
f3cd94204e
commit
3acdbfba83
+22
-1
@@ -32,6 +32,7 @@ except ImportError:
|
||||
web = MockWeb()
|
||||
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import tempfile
|
||||
|
||||
@@ -45,6 +46,16 @@ else:
|
||||
from services.packs.pack_archive import PackArchive, PackError
|
||||
from services.packs.pack_registry import PackRegistry
|
||||
|
||||
# Strict pattern for pack name/version URL route parameters.
|
||||
_SAFE_SEGMENT_RE = re.compile(r"^[a-zA-Z0-9._-]+$")
|
||||
|
||||
|
||||
def _is_safe_pack_segment(value: str) -> bool:
|
||||
"""Check if a pack name or version segment is safe for filesystem use."""
|
||||
if not value or value in (".", ".."):
|
||||
return False
|
||||
return bool(_SAFE_SEGMENT_RE.match(value))
|
||||
|
||||
|
||||
if web:
|
||||
|
||||
@@ -155,6 +166,11 @@ class PacksHandlers:
|
||||
{"ok": False, "error": "Missing name/version"}, status=400
|
||||
)
|
||||
|
||||
if not _is_safe_pack_segment(name) or not _is_safe_pack_segment(version):
|
||||
return web.json_response(
|
||||
{"ok": False, "error": "Invalid name or version format"}, status=400
|
||||
)
|
||||
|
||||
try:
|
||||
success = self.registry.uninstall_pack(name, version)
|
||||
if success:
|
||||
@@ -182,6 +198,11 @@ class PacksHandlers:
|
||||
{"ok": False, "error": "Missing name/version"}, status=400
|
||||
)
|
||||
|
||||
if not _is_safe_pack_segment(name) or not _is_safe_pack_segment(version):
|
||||
return web.json_response(
|
||||
{"ok": False, "error": "Invalid name or version format"}, status=400
|
||||
)
|
||||
|
||||
pack_path = self.registry.get_pack_path(name, version)
|
||||
if not pack_path:
|
||||
return web.json_response(
|
||||
@@ -206,7 +227,7 @@ class PacksHandlers:
|
||||
return CleanupFileResponse(
|
||||
temp_zip,
|
||||
headers={
|
||||
"Content-Disposition": f'attachment; filename="{name}-{version}.zip"',
|
||||
"Content-Disposition": f'attachment; filename="{name.replace(chr(34), "")}-{version.replace(chr(34), "")}.zip"',
|
||||
"Content-Type": "application/zip",
|
||||
},
|
||||
)
|
||||
|
||||
@@ -7,7 +7,7 @@ import unittest
|
||||
import zipfile
|
||||
from unittest.mock import ANY, AsyncMock, MagicMock, patch
|
||||
|
||||
from api.packs import CleanupFileResponse, PacksHandlers
|
||||
from api.packs import CleanupFileResponse, PacksHandlers, _is_safe_pack_segment
|
||||
from services.packs.pack_archive import PackArchive, PackError
|
||||
from services.packs.pack_manifest import create_manifest
|
||||
|
||||
@@ -193,5 +193,60 @@ class TestPacksApiAsync(unittest.IsolatedAsyncioTestCase):
|
||||
self.assertFalse(os.path.exists(path), "File should be deleted")
|
||||
|
||||
|
||||
class TestPacksApiInputValidation(unittest.IsolatedAsyncioTestCase):
|
||||
"""Test that API handlers reject path traversal in URL route parameters."""
|
||||
|
||||
def setUp(self):
|
||||
self.test_dir = tempfile.mkdtemp()
|
||||
self.handlers = PacksHandlers(self.test_dir)
|
||||
self.handlers._check_auth = AsyncMock(return_value=True)
|
||||
|
||||
def tearDown(self):
|
||||
shutil.rmtree(self.test_dir)
|
||||
|
||||
def test_safe_segment_rejects_traversal(self):
|
||||
self.assertFalse(_is_safe_pack_segment("../../etc"))
|
||||
self.assertFalse(_is_safe_pack_segment(".."))
|
||||
self.assertFalse(_is_safe_pack_segment("."))
|
||||
self.assertFalse(_is_safe_pack_segment(""))
|
||||
self.assertFalse(_is_safe_pack_segment("foo/bar"))
|
||||
self.assertFalse(_is_safe_pack_segment("foo\\bar"))
|
||||
|
||||
def test_safe_segment_accepts_valid(self):
|
||||
self.assertTrue(_is_safe_pack_segment("my-pack"))
|
||||
self.assertTrue(_is_safe_pack_segment("1.0.0"))
|
||||
self.assertTrue(_is_safe_pack_segment("pack_v2.1"))
|
||||
|
||||
@patch("api.packs.web")
|
||||
async def test_delete_rejects_traversal_name(self, mock_web):
|
||||
mock_web.json_response = MagicMock()
|
||||
request = MockRequest()
|
||||
request.match_info = {"name": "../../etc", "version": "1.0.0"}
|
||||
await self.handlers.delete_pack_handler(request)
|
||||
mock_web.json_response.assert_called_with(
|
||||
{"ok": False, "error": "Invalid name or version format"}, status=400
|
||||
)
|
||||
|
||||
@patch("api.packs.web")
|
||||
async def test_delete_rejects_traversal_version(self, mock_web):
|
||||
mock_web.json_response = MagicMock()
|
||||
request = MockRequest()
|
||||
request.match_info = {"name": "my-pack", "version": "../.."}
|
||||
await self.handlers.delete_pack_handler(request)
|
||||
mock_web.json_response.assert_called_with(
|
||||
{"ok": False, "error": "Invalid name or version format"}, status=400
|
||||
)
|
||||
|
||||
@patch("api.packs.web")
|
||||
async def test_export_rejects_traversal(self, mock_web):
|
||||
mock_web.json_response = MagicMock()
|
||||
request = MockRequest()
|
||||
request.match_info = {"name": "../../etc", "version": "passwd"}
|
||||
await self.handlers.export_pack_handler(request)
|
||||
mock_web.json_response.assert_called_with(
|
||||
{"ok": False, "error": "Invalid name or version format"}, status=400
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user