From 8e4532ccfc0e2ca5b9d29388de9688caf1f4214e Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Tue, 14 Jul 2026 23:41:24 +0000 Subject: [PATCH 1/4] feat(app_installation): add app zip validation and safe extraction Custom-app uploads are user input, but nothing inspected them beyond the file extension. Add app_zip.py as the single place that decides whether an archive is a plausible app: - validate_app_zip() requires app_meta.json + docker-compose.yml.template at the archive root, parses the manifest into AppMeta, and checks the app name is safe to use as a directory name and subdomain. - Files nested under a single top-level directory are accepted; that prefix is stripped. This is the shape a user gets from zipping a folder, and it is unambiguous enough to fix silently rather than reject. - extract_app_zip() writes members one by one and refuses any that resolve outside the target dir (zip slip), instead of a blind extractall(). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../service/app_installation/app_zip.py | 93 ++++++++ .../service/app_installation/exceptions.py | 4 + tests/test_app_zip.py | 214 ++++++++++++++++++ 3 files changed, 311 insertions(+) create mode 100644 shard_core/service/app_installation/app_zip.py create mode 100644 tests/test_app_zip.py diff --git a/shard_core/service/app_installation/app_zip.py b/shard_core/service/app_installation/app_zip.py new file mode 100644 index 00000000..c9177f4b --- /dev/null +++ b/shard_core/service/app_installation/app_zip.py @@ -0,0 +1,93 @@ +import json +import logging +import re +import shutil +import zipfile +from pathlib import Path + +from shard_core.data_model.app_meta import AppMeta +from .exceptions import InvalidAppZip + +log = logging.getLogger(__name__) + +REQUIRED_FILES = ("app_meta.json", "docker-compose.yml.template") +APP_NAME_PATTERN = re.compile(r"[a-zA-Z0-9][a-zA-Z0-9_-]{0,63}") + + +def validate_app_zip(zip_file: Path) -> AppMeta: + """Read the app metadata from an archive, rejecting anything unfit to install.""" + try: + with zipfile.ZipFile(zip_file, "r") as zip_ref: + names = _member_names(zip_ref) + root = _find_root(names) + for required in REQUIRED_FILES: + if root + required not in names: + raise InvalidAppZip(f"{required} is missing from the archive") + app_meta = _parse_app_meta(zip_ref.read(root + "app_meta.json")) + except zipfile.BadZipFile as e: + raise InvalidAppZip(f"not a valid zip archive: {e}") + + if not APP_NAME_PATTERN.fullmatch(app_meta.name): + raise InvalidAppZip( + f"app name {app_meta.name!r} in app_meta.json is not a valid app name" + ) + return app_meta + + +def extract_app_zip(zip_file: Path, target_dir: Path): + """Extract an app archive, stripping a single top-level directory if present.""" + target_root = target_dir.resolve() + with zipfile.ZipFile(zip_file, "r") as zip_ref: + names = _member_names(zip_ref) + root = _find_root(names) + for name in names: + relative = name[len(root) :] + target = (target_dir / relative).resolve() + if not relative or target_root not in target.parents: + raise InvalidAppZip(f"archive member {name!r} has an illegal path") + target.parent.mkdir(parents=True, exist_ok=True) + with zip_ref.open(name) as source, open(target, "wb") as f: + shutil.copyfileobj(source, f) + + +def _member_names(zip_ref: zipfile.ZipFile) -> list[str]: + names = [ + i.filename + for i in zip_ref.infolist() + if not i.is_dir() and not _is_archiver_cruft(i.filename) + ] + if not names: + raise InvalidAppZip("the archive is empty") + for name in names: + parts = Path(name).parts + if name.startswith("/") or ".." in parts or Path(name).is_absolute(): + raise InvalidAppZip(f"archive member {name!r} has an illegal path") + return names + + +def _is_archiver_cruft(name: str) -> bool: + # macOS "Compress" adds these next to the compressed folder — without ignoring + # them, such an archive looks like it has two top-level directories + return name.startswith("__MACOSX/") or Path(name).name == ".DS_Store" + + +def _find_root(names: list[str]) -> str: + """Return the prefix the app files live under: "" or "/".""" + if "app_meta.json" in names: + return "" + top_level = {name.split("/", 1)[0] for name in names} + if len(top_level) == 1: + root = top_level.pop() + "/" + if root + "app_meta.json" in names: + return root + raise InvalidAppZip( + "app_meta.json is missing from the archive root; the app files must be at " + "the top level of the zip, or inside a single directory" + ) + + +def _parse_app_meta(raw: bytes) -> AppMeta: + try: + return AppMeta.model_validate(json.loads(raw)) + except Exception as e: + raise InvalidAppZip(f"app_meta.json is invalid: {e}") diff --git a/shard_core/service/app_installation/exceptions.py b/shard_core/service/app_installation/exceptions.py index d13e234f..ed30c1b5 100644 --- a/shard_core/service/app_installation/exceptions.py +++ b/shard_core/service/app_installation/exceptions.py @@ -12,3 +12,7 @@ class AppNotInstalled(Exception): class AppInIllegalStatus(Exception): pass + + +class InvalidAppZip(Exception): + pass diff --git a/tests/test_app_zip.py b/tests/test_app_zip.py new file mode 100644 index 00000000..f50bb43c --- /dev/null +++ b/tests/test_app_zip.py @@ -0,0 +1,214 @@ +import json +import zipfile +from pathlib import Path + +import pytest + +from shard_core.service.app_installation.app_zip import ( + extract_app_zip, + validate_app_zip, +) +from shard_core.service.app_installation.exceptions import InvalidAppZip +from tests.util import mock_app_store_path + + +def _app_meta(**overrides) -> str: + meta = { + "v": "1.3", + "app_version": "0.1.0", + "name": "some_app", + "pretty_name": "Some App", + "icon": "icon.svg", + "entrypoints": [ + { + "container_name": "some_app", + "container_port": 80, + "entrypoint_port": "http", + } + ], + "paths": {"": {"access": "private"}}, + } + meta.update(overrides) + return json.dumps(meta) + + +def _make_zip(path: Path, members: dict[str, str]) -> Path: + with zipfile.ZipFile(path, "w") as zip_ref: + for name, content in members.items(): + zip_ref.writestr(name, content) + return path + + +def _valid_members(prefix: str = "") -> dict[str, str]: + return { + f"{prefix}app_meta.json": _app_meta(), + f"{prefix}docker-compose.yml.template": "services: {}", + f"{prefix}icon.svg": "", + } + + +def test_validate_flat_zip(tmp_path): + zip_file = _make_zip(tmp_path / "upload.zip", _valid_members()) + assert validate_app_zip(zip_file).name == "some_app" + + +def test_validate_zip_with_single_top_level_dir(tmp_path): + zip_file = _make_zip(tmp_path / "upload.zip", _valid_members("some_app/")) + assert validate_app_zip(zip_file).name == "some_app" + + +def test_validate_zip_compressed_by_macos(tmp_path): + members = _valid_members("some_app/") | { + "__MACOSX/some_app/._app_meta.json": "resource fork", + "some_app/.DS_Store": "finder cruft", + } + zip_file = _make_zip(tmp_path / "upload.zip", members) + assert validate_app_zip(zip_file).name == "some_app" + + +def test_extract_skips_archiver_cruft(tmp_path): + members = _valid_members("some_app/") | { + "__MACOSX/some_app/._app_meta.json": "resource fork", + "some_app/.DS_Store": "finder cruft", + } + zip_file = _make_zip(tmp_path / "upload.zip", members) + target_dir = tmp_path / "installed_apps" / "some_app" + target_dir.mkdir(parents=True) + + extract_app_zip(zip_file, target_dir) + + assert (target_dir / "app_meta.json").exists() + assert not (target_dir / "__MACOSX").exists() + assert not (target_dir / ".DS_Store").exists() + + +def test_validate_rejects_missing_app_meta(tmp_path): + members = _valid_members() + del members["app_meta.json"] + zip_file = _make_zip(tmp_path / "upload.zip", members) + with pytest.raises(InvalidAppZip, match="app_meta.json is missing"): + validate_app_zip(zip_file) + + +def test_validate_rejects_missing_compose_template(tmp_path): + members = _valid_members() + del members["docker-compose.yml.template"] + zip_file = _make_zip(tmp_path / "upload.zip", members) + with pytest.raises(InvalidAppZip, match="docker-compose.yml.template is missing"): + validate_app_zip(zip_file) + + +def test_validate_rejects_app_meta_nested_deeper(tmp_path): + zip_file = _make_zip(tmp_path / "upload.zip", _valid_members("some_app/nested/")) + with pytest.raises(InvalidAppZip, match="app_meta.json is missing"): + validate_app_zip(zip_file) + + +def test_validate_rejects_multiple_top_level_dirs(tmp_path): + members = _valid_members("some_app/") | {"other_dir/file.txt": "x"} + zip_file = _make_zip(tmp_path / "upload.zip", members) + with pytest.raises(InvalidAppZip, match="app_meta.json is missing"): + validate_app_zip(zip_file) + + +def test_validate_rejects_broken_app_meta(tmp_path): + members = _valid_members() | {"app_meta.json": "{not json"} + zip_file = _make_zip(tmp_path / "upload.zip", members) + with pytest.raises(InvalidAppZip, match="app_meta.json is invalid"): + validate_app_zip(zip_file) + + +def test_validate_rejects_app_meta_with_missing_fields(tmp_path): + members = _valid_members() | { + "app_meta.json": json.dumps({"v": "1.3", "name": "x"}) + } + zip_file = _make_zip(tmp_path / "upload.zip", members) + with pytest.raises(InvalidAppZip, match="app_meta.json is invalid"): + validate_app_zip(zip_file) + + +def test_validate_rejects_non_zip(tmp_path): + zip_file = tmp_path / "upload.zip" + zip_file.write_bytes(b"this is not a zip file") + with pytest.raises(InvalidAppZip, match="not a valid zip archive"): + validate_app_zip(zip_file) + + +def test_validate_rejects_empty_zip(tmp_path): + zip_file = _make_zip(tmp_path / "upload.zip", {}) + with pytest.raises(InvalidAppZip, match="empty"): + validate_app_zip(zip_file) + + +@pytest.mark.parametrize("app_name", ["../evil", "foo/bar", "", "with space"]) +def test_validate_rejects_unsafe_app_name(tmp_path, app_name): + members = _valid_members() | {"app_meta.json": _app_meta(name=app_name)} + zip_file = _make_zip(tmp_path / "upload.zip", members) + with pytest.raises(InvalidAppZip, match="not a valid app name"): + validate_app_zip(zip_file) + + +@pytest.mark.parametrize( + "member", ["../escaped.txt", "some_app/../../escaped.txt", "/tmp/escaped.txt"] +) +def test_validate_rejects_path_traversal(tmp_path, member): + zip_file = _make_zip(tmp_path / "upload.zip", _valid_members() | {member: "pwned"}) + with pytest.raises(InvalidAppZip, match="illegal path"): + validate_app_zip(zip_file) + + +def test_extract_flat_zip(tmp_path): + zip_file = _make_zip(tmp_path / "upload.zip", _valid_members()) + target_dir = tmp_path / "installed_apps" / "some_app" + target_dir.mkdir(parents=True) + + extract_app_zip(zip_file, target_dir) + + assert (target_dir / "app_meta.json").exists() + assert (target_dir / "docker-compose.yml.template").exists() + assert (target_dir / "icon.svg").exists() + + +def test_extract_strips_single_top_level_dir(tmp_path): + zip_file = _make_zip(tmp_path / "upload.zip", _valid_members("some_app/")) + target_dir = tmp_path / "installed_apps" / "some_app" + target_dir.mkdir(parents=True) + + extract_app_zip(zip_file, target_dir) + + assert (target_dir / "app_meta.json").exists() + assert not (target_dir / "some_app").exists() + + +def test_extract_keeps_subdirectories(tmp_path): + members = _valid_members() | {"config/settings.yml": "foo: bar"} + zip_file = _make_zip(tmp_path / "upload.zip", members) + target_dir = tmp_path / "installed_apps" / "some_app" + target_dir.mkdir(parents=True) + + extract_app_zip(zip_file, target_dir) + + assert (target_dir / "config" / "settings.yml").read_text() == "foo: bar" + + +def test_extract_rejects_path_traversal(tmp_path): + members = _valid_members() | {"../escaped.txt": "pwned"} + zip_file = _make_zip(tmp_path / "upload.zip", members) + target_dir = tmp_path / "installed_apps" / "some_app" + target_dir.mkdir(parents=True) + + with pytest.raises(InvalidAppZip): + extract_app_zip(zip_file, target_dir) + + assert not (tmp_path / "installed_apps" / "escaped.txt").exists() + + +def test_extract_mock_app_store_zip(tmp_path): + zip_file = mock_app_store_path() / "mock_app" / "mock_app.zip" + target_dir = tmp_path / "installed_apps" / "mock_app" + target_dir.mkdir(parents=True) + + extract_app_zip(zip_file, target_dir) + + assert (target_dir / "app_meta.json").exists() + assert (target_dir / "docker-compose.yml.template").exists() From 8b98dc1a9e4cbb1858069ae5ca9dca4e8d0df4a5 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Tue, 14 Jul 2026 23:41:34 +0000 Subject: [PATCH 2/4] fix(app_installation): extract app zips through the safe extractor Every install path (store, upload, reinstall) unpacked with a blind extractall(), and a zip whose files sit in a top-level directory landed at installed_apps///, leaving the app permanently "Unknown App". Route all of them through extract_app_zip(), which strips that directory and rejects members escaping the app dir. Co-Authored-By: Claude Opus 4.8 (1M context) --- shard_core/service/app_installation/worker.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/shard_core/service/app_installation/worker.py b/shard_core/service/app_installation/worker.py index 36572603..257ea140 100644 --- a/shard_core/service/app_installation/worker.py +++ b/shard_core/service/app_installation/worker.py @@ -1,7 +1,6 @@ import asyncio import logging import shutil -import zipfile from contextlib import suppress from pathlib import Path from typing import Literal @@ -21,6 +20,7 @@ ) from shard_core.settings import settings from shard_core.util import signals +from .app_zip import extract_app_zip from .exceptions import AppDoesNotExist from .util import ( update_app_status, @@ -174,8 +174,7 @@ async def _reinstall_app(app_name: str): async def _install_app_from_zip(installed_app, zip_file): - with zipfile.ZipFile(zip_file, "r") as zip_ref: - zip_ref.extractall(zip_file.parent) + extract_app_zip(zip_file, zip_file.parent) await signals.on_apps_update.send_async() zip_file.unlink() From 4361519ddacd2c98b4f7c017e8abd6f5557c5663 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Tue, 14 Jul 2026 23:41:34 +0000 Subject: [PATCH 3/4] fix(apps): reject invalid custom-app uploads before creating any state The upload endpoint wrote the zip straight into installed_apps//, created a DB row and queued an install task, and only then found out whether the archive was an app at all. A malformed upload therefore left an "Unknown App" row plus a directory behind, which a pilot user hit and which ended in core failing to start. Buffer the upload to a temp dir, validate it there, and only move it into installed_apps once it is known to be installable. The app name now comes from app_meta.json rather than the uploaded filename, so it no longer depends on what the user called the file. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../service/app_installation/__init__.py | 13 +++++- shard_core/web/protected/apps.py | 41 ++++++++++++------- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/shard_core/service/app_installation/__init__.py b/shard_core/service/app_installation/__init__.py index bf32aef6..9b3624a5 100644 --- a/shard_core/service/app_installation/__init__.py +++ b/shard_core/service/app_installation/__init__.py @@ -1,4 +1,6 @@ import logging +import shutil +from pathlib import Path from shard_core.database import database from shard_core.database.connection import db_conn @@ -6,6 +8,7 @@ from shard_core.data_model.app_meta import InstallationReason, InstalledApp, Status from shard_core.util import signals from shard_core.settings import settings +from shard_core.service.app_tools import get_installed_apps_path from shard_core.util.subprocess import subprocess, SubprocessError from . import util, worker from .exceptions import AppAlreadyInstalled, AppDoesNotExist, AppNotInstalled @@ -42,12 +45,18 @@ async def install_app_from_store( log.info(f"created {installation_task}") -async def install_app_from_existing_zip( - name: str, installation_reason: InstallationReason = InstallationReason.CUSTOM +async def install_app_from_uploaded_zip( + name: str, + zip_file: Path, + installation_reason: InstallationReason = InstallationReason.CUSTOM, ): if await util.app_exists_in_db(name): raise AppAlreadyInstalled(name) + target_zip = get_installed_apps_path() / name / f"{name}.zip" + target_zip.parent.mkdir(parents=True, exist_ok=True) + shutil.move(zip_file, target_zip) + async with db_conn() as conn: installed_app = InstalledApp( name=name, diff --git a/shard_core/web/protected/apps.py b/shard_core/web/protected/apps.py index 48e08b2b..4e573d15 100644 --- a/shard_core/web/protected/apps.py +++ b/shard_core/web/protected/apps.py @@ -1,6 +1,8 @@ import io import logging import mimetypes +import tempfile +from pathlib import Path from typing import List import aiofiles @@ -11,9 +13,11 @@ from shard_core.database import installed_apps as db_installed_apps from shard_core.data_model.app_meta import InstalledAppWithMeta, InstalledApp from shard_core.service import app_installation +from shard_core.service.app_installation.app_zip import validate_app_zip from shard_core.service.app_installation.exceptions import ( AppAlreadyInstalled, AppNotInstalled, + InvalidAppZip, ) from shard_core.service.app_tools import ( get_installed_apps_path, @@ -112,17 +116,26 @@ async def install_custom_app(file: UploadFile): detail="Only zip files are supported", ) - file_path = get_installed_apps_path() / file.filename[:-4] / file.filename - file_path.parent.mkdir(parents=True, exist_ok=True) - - async with aiofiles.open(file_path, "wb") as f: - while chunk := await file.read(1024): - await f.write(chunk) - - try: - await app_installation.install_app_from_existing_zip(file.filename[:-4]) - except app_installation.exceptions.AppAlreadyInstalled: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=f"App {file.filename[:-4]} is already installed", - ) + with tempfile.TemporaryDirectory() as tmp_dir: + upload_path = Path(tmp_dir) / "upload.zip" + async with aiofiles.open(upload_path, "wb") as f: + while chunk := await file.read(64 * 1024): + await f.write(chunk) + + try: + app_meta = validate_app_zip(upload_path) + except InvalidAppZip as e: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"The uploaded app is invalid: {e}", + ) + + try: + await app_installation.install_app_from_uploaded_zip( + app_meta.name, upload_path + ) + except AppAlreadyInstalled: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"App {app_meta.name} is already installed", + ) From e0935463572cfd6e9444f72b8f8568d828bce8d5 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Tue, 14 Jul 2026 23:41:34 +0000 Subject: [PATCH 4/4] test(apps): cover custom-app upload validation; document the flow Co-Authored-By: Claude Opus 4.8 (1M context) --- agents.md | 1 + tests/test_app_installation.py | 82 +++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/agents.md b/agents.md index 693ebaec..7f27d51f 100644 --- a/agents.md +++ b/agents.md @@ -96,6 +96,7 @@ Blinker-based async signals defined in `util/signals.py`. DB-writing handlers ar - `async_on_peer_write` — peer added/updated ### App Installation Flow +0. Custom-app uploads (`POST /protected/apps`) are validated by `app_installation/app_zip.py` **before** any state is created: the zip is buffered to a temp dir, must carry `app_meta.json` + `docker-compose.yml.template` at its root (a single top-level directory is stripped), and the app name comes from the manifest, not the filename. Invalid uploads get a 400 and leave no row, no dir, no task. Extraction goes through `extract_app_zip`, which refuses members resolving outside the app dir. 1. Request queued → `InstallationWorker` picks it up 2. Docker Compose template rendered with Jinja2 (shard domain, data paths, etc.) 3. Traefik dynamic config generated for the app's subdomain routing diff --git a/tests/test_app_installation.py b/tests/test_app_installation.py index 6f347f4f..823b8254 100644 --- a/tests/test_app_installation.py +++ b/tests/test_app_installation.py @@ -1,9 +1,13 @@ +import io +import zipfile + import docker import pytest from docker.errors import NotFound from fastapi import status from httpx import AsyncClient +from shard_core.service.app_tools import get_installed_apps_path from tests.util import ( wait_until_app_installed, mock_app_store_path, @@ -112,16 +116,33 @@ async def test_uninstall_running_app(api_client: AsyncClient): docker_client.containers.get(app_name) +async def _upload_custom_app(api_client: AsyncClient, filename: str, content: bytes): + return await api_client.post( + "protected/apps", + files={"file": (filename, content, "application/zip")}, + ) + + +def _mock_app_zip_bytes() -> bytes: + return (mock_app_store_path() / "mock_app" / "mock_app.zip").read_bytes() + + +def _nested_mock_app_zip_bytes() -> bytes: + """The mock_app zip, but with all files inside a single top-level directory.""" + buffer = io.BytesIO() + with zipfile.ZipFile(io.BytesIO(_mock_app_zip_bytes())) as source: + with zipfile.ZipFile(buffer, "w") as target: + for name in source.namelist(): + target.writestr(f"mock_app/{name}", source.read(name)) + return buffer.getvalue() + + async def test_install_custom_app(api_client: AsyncClient): app_name = "mock_app" docker_client = docker.from_env() - app_zip = mock_app_store_path() / app_name / f"{app_name}.zip" - with open(app_zip, "rb") as f: - content = f.read() - response = await api_client.post( - "protected/apps", - files={"file": (f"{app_name}.zip", content, "application/zip")}, + response = await _upload_custom_app( + api_client, f"{app_name}.zip", _mock_app_zip_bytes() ) assert response.status_code == status.HTTP_201_CREATED @@ -131,3 +152,52 @@ async def test_install_custom_app(api_client: AsyncClient): response = (await api_client.get("protected/apps")).json() assert len(response) == 4 + + +async def test_install_custom_app_uses_name_from_app_meta(api_client: AsyncClient): + response = await _upload_custom_app( + api_client, "some-unrelated-filename.zip", _mock_app_zip_bytes() + ) + assert response.status_code == status.HTTP_201_CREATED + + await wait_until_app_installed(api_client, "mock_app") + + response = await api_client.get("protected/apps/some-unrelated-filename") + assert response.status_code == status.HTTP_404_NOT_FOUND + + +async def test_install_custom_app_with_single_top_level_dir(api_client: AsyncClient): + app_name = "mock_app" + docker_client = docker.from_env() + + response = await _upload_custom_app( + api_client, f"{app_name}.zip", _nested_mock_app_zip_bytes() + ) + assert response.status_code == status.HTTP_201_CREATED + + await wait_until_app_installed(api_client, app_name) + + docker_client.containers.get(app_name) + app = (await api_client.get(f"protected/apps/{app_name}")).json() + assert app["meta"]["name"] == app_name + + +async def test_install_custom_app_without_app_meta(api_client: AsyncClient): + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as zip_ref: + zip_ref.writestr("docker-compose.yml.template", "services: {}") + + response = await _upload_custom_app(api_client, "mock_app.zip", buffer.getvalue()) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "app_meta.json is missing" in response.json()["detail"] + + assert len((await api_client.get("protected/apps")).json()) == 3 + assert not (get_installed_apps_path() / "mock_app").exists() + + +async def test_install_custom_app_that_is_not_a_zip(api_client: AsyncClient): + response = await _upload_custom_app(api_client, "mock_app.zip", b"not a zip file") + assert response.status_code == status.HTTP_400_BAD_REQUEST + + assert len((await api_client.get("protected/apps")).json()) == 3 + assert not (get_installed_apps_path() / "mock_app").exists()