upgrade ruff and limit icon upload

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

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

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"]

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:<mimetype>;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() :])

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:

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=["*"],
     )

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]:

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):

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(
             {

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(
             {

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()

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()