diff --git a/CHANGELOG.md b/CHANGELOG.md index 15217f7..83b217d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- Icon upload (`data_url`) now only accepts base64-encoded `data:` URLs. + ### Changed - Minimal Python version is now 3.10 diff --git a/README.md b/README.md index 6564751..80a6f23 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ RESOURCE_DIRECTORY = "/path/to/resource/directory/" EWOKS_EXECUTION = {"handlers": ...} -CELERY = {"broker_url":...} +CELERY = {"broker_url": ...} ``` Specify the configuration file through the CLI diff --git a/pyproject.toml b/pyproject.toml index 12a4d9d..49f473e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,8 +45,7 @@ test = [ ] dev = [ "ewoksserver[test]", - "ruff", - "isort", + "ruff>=0.16.0", ] doc = [ "ewoksserver[test]", @@ -76,12 +75,21 @@ ewoks-server = "ewoksserver.__main__:main" ewoks-server-spec = "ewoksserver.spec:save" [tool.ruff.lint] -select = ["E", "F", "I"] -ignore = ["E501"] +select = [ + "E", # pycodestyle errors + "F", # pyflakes + "I", # isort + "S", # flake8-bandit +] +ignore = [ + "E501", # line too long +] [tool.ruff.lint.per-file-ignores] -# Ignore `S101` (assert used violations) in all test files -"src/ewoksserver/tests/*.py" = ["S101"] +"src/ewoksserver/tests/*.py" = [ + "S101" # allow asserts +] [tool.ruff.lint.isort] force-single-line = true +known-first-party = ["ewoksserver"] diff --git a/src/ewoksserver/app/backends/binary_backend.py b/src/ewoksserver/app/backends/binary_backend.py index 92fd153..14b103e 100644 --- a/src/ewoksserver/app/backends/binary_backend.py +++ b/src/ewoksserver/app/backends/binary_backend.py @@ -1,9 +1,9 @@ import base64 import logging import mimetypes +import re from pathlib import Path from typing import Iterator -from urllib import request ResourceIdentifierType = str ResourceUrlType = Path @@ -72,8 +72,13 @@ def _delete_url(url: ResourceUrlType) -> ResourceContentType: url.unlink() -def _identifier_to_url(root: ResourceUrlType, identifier: ResourceIdentifierType): - return root / identifier +def _identifier_to_url( + root: ResourceUrlType, identifier: ResourceIdentifierType +) -> ResourceUrlType: + url = (root / identifier).resolve() + if not url.is_relative_to(root.resolve()): + raise ValueError(f"Invalid resource identifier: {identifier!r}") + return url def _url_to_identifier(url: ResourceUrlType) -> ResourceIdentifierType: @@ -102,7 +107,20 @@ def _load_url(url: ResourceUrlType) -> ResourceContentType: def _save_url(url: ResourceUrlType, resource: ResourceContentType) -> None: _logger.debug("Save file '%s'", url) url.parent.mkdir(parents=True, exist_ok=True) - with request.urlopen(resource["data_url"]) as f: - data = f.read() + data = _decode_data_url(resource["data_url"]) with open(url, "wb") as f: f.write(data) + + +# Matches the "data:;base64," prefix produced by `_load_url` above. +# Exposed so API models (e.g. EwoksIcon) can validate this at the request boundary. +DATA_URL_PREFIX = re.compile(r"^data:[^;,]*;base64,", re.IGNORECASE) + + +def _decode_data_url(data_url: str) -> bytes: + # Decode directly instead of using `urllib.request.urlopen`, which would + # also follow "file:", "http:" and other schemes for client-provided input. + match = DATA_URL_PREFIX.match(data_url) + if not match: + raise ValueError("Only base64-encoded 'data:' URLs are supported") + return base64.b64decode(data_url[match.end() :]) diff --git a/src/ewoksserver/app/backends/json_backend.py b/src/ewoksserver/app/backends/json_backend.py index 6e6b5d5..534efae 100644 --- a/src/ewoksserver/app/backends/json_backend.py +++ b/src/ewoksserver/app/backends/json_backend.py @@ -66,8 +66,13 @@ def delete_resource(root: ResourceUrlType, identifier: ResourceIdentifierType) - _delete_url(url) -def _identifier_to_url(root: ResourceUrlType, identifier: ResourceIdentifierType): - return root / (identifier + ".json") +def _identifier_to_url( + root: ResourceUrlType, identifier: ResourceIdentifierType +) -> ResourceUrlType: + url = (root / (identifier + ".json")).resolve() + if not url.is_relative_to(root.resolve()): + raise ValueError(f"Invalid resource identifier: {identifier!r}") + return url def _url_to_identifier(url: ResourceUrlType) -> ResourceIdentifierType: diff --git a/src/ewoksserver/app/cors.py b/src/ewoksserver/app/cors.py index 6c70547..cad664a 100644 --- a/src/ewoksserver/app/cors.py +++ b/src/ewoksserver/app/cors.py @@ -7,7 +7,9 @@ def enable_cors(app: FastAPI) -> None: app.add_middleware( CORSMiddleware, allow_origins=["*"], - allow_credentials=True, + # Browsers reject `allow_credentials=True` combined with a wildcard + # origin anyway, and no client uses cookies/credentials with this API. + allow_credentials=False, allow_methods=["*"], allow_headers=["*"], ) diff --git a/src/ewoksserver/app/routes/backend.py b/src/ewoksserver/app/routes/backend.py index 3646272..20f8ef6 100644 --- a/src/ewoksserver/app/routes/backend.py +++ b/src/ewoksserver/app/routes/backend.py @@ -31,7 +31,8 @@ def get_routes( major_routes = dict() major_versions = dict() for full_version, router in reversed(sorted(routers.items())): - assert len(full_version) == 3, full_version + if len(full_version) != 3: + raise ValueError(f"version should have 3 elements: {full_version}") major, minor, patch = full_version route_key = major, minor, patch, 0 path_version = "v" + "_".join(map(str, full_version)) @@ -74,7 +75,8 @@ def get_routes( def assert_route_versions(*all_routes: Mapping[VersionTuple, RouterType]) -> None: versions = {tuple(sorted(routes)) for routes in all_routes} - assert len(versions) == 1, "Not all routes have the same versions" + if len(versions) != 1: + raise RuntimeError("Not all routes have the same versions") def extract_version_tags(all_routes: list[dict[VersionTuple, Route]]) -> set[str]: diff --git a/src/ewoksserver/app/routes/icons/models.py b/src/ewoksserver/app/routes/icons/models.py index 0275738..75c5a1f 100644 --- a/src/ewoksserver/app/routes/icons/models.py +++ b/src/ewoksserver/app/routes/icons/models.py @@ -1,9 +1,15 @@ from pydantic import BaseModel from pydantic import Field +from ...backends.binary_backend import DATA_URL_PREFIX + class EwoksIcon(BaseModel): - data_url: str = Field(title="Icon data url") + data_url: str = Field( + title="Icon data url", + description="Base64-encoded data URL (e.g. 'data:image/png;base64,...')", + pattern=DATA_URL_PREFIX.pattern, + ) class EwoksIconIdentifiers(BaseModel): diff --git a/src/ewoksserver/app/routes/tasks/router.py b/src/ewoksserver/app/routes/tasks/router.py index c206618..8d17d09 100644 --- a/src/ewoksserver/app/routes/tasks/router.py +++ b/src/ewoksserver/app/routes/tasks/router.py @@ -218,9 +218,19 @@ def create_task( status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, ) - exists = json_backend.resource_exists( - settings.resource_directory / "tasks", ridentifier - ) + try: + exists = json_backend.resource_exists( + settings.resource_directory / "tasks", ridentifier + ) + except ValueError: + return JSONResponse( + { + "message": f"Task identifier '{ridentifier}' is not valid", + "type": "task", + "identifier": ridentifier, + }, + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + ) if exists: return JSONResponse( { diff --git a/src/ewoksserver/app/routes/workflows/router.py b/src/ewoksserver/app/routes/workflows/router.py index 116f50c..480d40b 100644 --- a/src/ewoksserver/app/routes/workflows/router.py +++ b/src/ewoksserver/app/routes/workflows/router.py @@ -257,9 +257,19 @@ def create_workflow( status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, ) - exists = json_backend.resource_exists( - settings.resource_directory / "workflows", ridentifier - ) + try: + exists = json_backend.resource_exists( + settings.resource_directory / "workflows", ridentifier + ) + except ValueError: + return JSONResponse( + { + "message": f"Workflow identifier '{ridentifier}' is not valid", + "type": "workflow", + "identifier": ridentifier, + }, + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + ) if exists: return JSONResponse( { diff --git a/src/ewoksserver/tests/test_tasks.py b/src/ewoksserver/tests/test_tasks.py index f3a6316..518d99f 100644 --- a/src/ewoksserver/tests/test_tasks.py +++ b/src/ewoksserver/tests/test_tasks.py @@ -98,6 +98,16 @@ def test_task_creation_errors(rest_client, default_task_identifiers, api_root): assert data["message"] == f"Task '{existing_id}' already exists." +def test_task_creation_path_traversal(rest_client, api_root, tmpdir): + task_with_traversal_id = { + "task_identifier": "../../../../../../tmp/evil", + "task_type": "class", + } + response = rest_client.post(f"{api_root}/tasks", json=task_with_traversal_id) + assert response.status_code == 422 + assert not (tmpdir / ".." / ".." / "evil.json").check() + + def test_multiple_tasks(rest_client, default_task_identifiers, api_root): response = rest_client.get(f"{api_root}/tasks") data = response.json() diff --git a/src/ewoksserver/tests/test_workflows.py b/src/ewoksserver/tests/test_workflows.py index f11abf2..1c7c2e5 100644 --- a/src/ewoksserver/tests/test_workflows.py +++ b/src/ewoksserver/tests/test_workflows.py @@ -63,6 +63,15 @@ def test_workflow_creation_errors(rest_client, default_workflow_identifiers, api assert data["message"] == f"Workflow '{existing_id}' already exists." +def test_workflow_creation_path_traversal(rest_client, api_root, tmpdir): + workflow_with_traversal_id = {"graph": {"id": "../../../../../../tmp/evil"}} + response = rest_client.post( + f"{api_root}/workflows", json=workflow_with_traversal_id + ) + assert response.status_code == 422 + assert not (tmpdir / ".." / ".." / "evil.json").check() + + def test_multiple_workflows(rest_client, default_workflow_identifiers, api_root): response = rest_client.get(f"{api_root}/workflows") data = response.json()