Constrain the request fields that become directory names, and stop publishing /datasets/import-roots

docs/architecture/public-studio.md

@@ -85,7 +85,11 @@ Dataset/code/package/checkpoint path imports, shell execution, RF control and ex
 
 The public entrypoint is `python -m opendpd.web`, **not** `opendpd gui`.
 An explicit route allowlist sits ahead of separate local-app instances; new
-desktop routes are not automatically public. Each app has its own workspace,
+desktop routes are not automatically public. Because an id pattern such as
+`/datasets/<id>` also matches a static sibling route, routes that must never be
+published are listed by name in `policy.NEVER_PUBLIC`, and
+`tests/unit/test_public_policy_surface.py` fails if a static route becomes
+reachable without being listed. Each app has its own workspace,
 SQLite database and supervisor. Read, download, event polling, cancel and retry
 requests all resolve within the authenticated workspace.
 

docs/architecture/threat-model.md

@@ -22,6 +22,7 @@ ASGI application (`tests/integration/test_hardening.py` unless stated).
 | Script injection into the page (XSS) | run names, notes, dataset ids, log lines, package contents rendered in the UI | React escapes user strings; the sole reviewed HTML sink is the bundled KaTeX renderer, which escapes source text and trusts only catalog coefficient classes; arbitrary URLs, resources and HTML/style commands are disabled; server-side diagnostic pages HTML-escape their text; a Content-Security-Policy on every response allows scripts only from this origin (no inline scripts, no `eval`, no CDN), plus `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `frame-ancestors 'none'`, `Referrer-Policy: same-origin` | `test_every_response_carries_the_security_headers`, `test_diagnostic_pages_escape_their_text`, `test_frontend_html_sinks_are_confined_to_the_audited_math_renderer`, `MathFormula.test.tsx`, `test_user_text_is_stored_and_returned_as_data`; the real-server journey (`frontend/e2e/live.spec.ts`) fails on any console error, which is where CSP violations surface |
 | Guessing the session | brute force | 256-bit random ids; bootstrap token printed only to the local console/URL; sessions die with the process | `tests/unit/test_security.py` |
 | Path traversal / symlink escape on download | `GET /artifacts/{run}/{id}` | downloads by registered artifact id; the resolved path (symlinks followed) must stay inside the run directory | `test_studio_api.py::test_artifact_download_by_id_only`, `test_artifact_symlink_outside_the_run_is_refused` |
+| Identifier fields used as directory names | `dataset_id` on `POST /datasets/import-builtin`, `version`/`base_version` on `POST /datasets/{id}/preprocess` and the `version` query on `/datasets/{id}/analysis` and `/diagnostics`, `quantization.label` on `POST /runs` | each names one directory entry, so each is an identifier and never a path. The request contracts type them `Slug`, and `Workspace.dataset_dir`, `Workspace.dataset_version_dir` and `datasets._materialise` re-check through `checked_identifier`, so the guarantee holds for every caller rather than for the routes alone. Without this the directory was created and its files written first and the `Slug` on the stored model rejected the name only afterwards, and because `os.path.join`/`Path.__truediv__` discard everything before an absolute component, an absolute value relocated the write instead of merely traversing | `test_builtin_dataset_ids_cannot_escape_the_workspace`, `test_dataset_versions_cannot_escape_the_workspace`, `test_quantization_labels_cannot_escape_the_run_directory` |
 | Reading files outside the import roots | `../` or a symlink inside a root pointing elsewhere | every source path is resolved and must stay inside its root; a symlink that leaves the root is neither listed nor readable | `tests/unit/test_datasets_service.py::test_import_roots_refuse_traversal_and_unknown_roots`, `test_import_root_symlink_escapes_are_invisible_and_unreadable` |
 | Malicious experiment package | traversal member names, symlink/device members, decompression bombs, member floods, damaged files | member names are checked lexically and after resolution; entries typed as anything but a regular file or directory are refused; every member is read at most one byte past its manifest-recorded size (hash and size verified before any write, and again bounded during extraction); at most 10 000 members; the manifest is capped at 8 MB; the declared total must fit the free space of the workspace volume; nothing is written before every check passed | `test_symlink_members_are_refused`, `test_members_larger_than_recorded_are_refused_early`, `test_member_count_is_bounded`, `test_traversal_members_are_refused_before_any_write`, `tests/integration/test_packages.py::test_damaged_packages_are_refused_with_a_specific_diagnostic` |
 | Arbitrary code via checkpoints or data files | pickled objects in `.pt`/`.npy` | every checkpoint goes through one loader, `opendpd.services.legacy_adapter.load_checkpoint`, which uses `torch.load(weights_only=True)` and turns a refusal into a workspace error; there is **no** trusted/unrestricted loading path anywhere in the tree (legacy `main.py` checkpoints are plain state_dicts and load under the same restriction); NumPy sources are opened with `allow_pickle=False` and object arrays are refused | `test_malicious_checkpoints_are_refused_not_executed`, `test_legacy_state_dict_checkpoints_still_load_under_the_restriction`, `test_no_unrestricted_pickle_loading_in_the_tree`, `test_datasets_service.py::test_numpy_imports_and_object_arrays_refused`, `tests/golden/test_legacy_checkpoint_loads.py` |

docs/contracts/openapi.json

@@ -6009,6 +6009,7 @@
           "dataset_id": {
             "anyOf": [
               {
+                "pattern": "^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$",
                 "type": "string"
               },
               {
@@ -8814,6 +8815,7 @@
         "properties": {
           "base_version": {
             "default": "raw-v1",
+            "pattern": "^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$",
             "title": "Base Version",
             "type": "string"
           },
@@ -8824,6 +8826,7 @@
             "anyOf": [
               {
                 "maxLength": 64,
+                "pattern": "^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$",
                 "type": "string"
               },
               {
@@ -8966,6 +8969,7 @@
           },
           "label": {
             "default": "",
+            "pattern": "^$|^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$",
             "title": "Label",
             "type": "string"
           },
@@ -13491,6 +13495,7 @@
             "schema": {
               "default": "raw-v1",
               "maxLength": 64,
+              "pattern": "^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$",
               "title": "Version",
               "type": "string"
             }
@@ -13592,6 +13597,7 @@
             "schema": {
               "default": "raw-v1",
               "maxLength": 64,
+              "pattern": "^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$",
               "title": "Version",
               "type": "string"
             }

opendpd/schemas/experiment.py

@@ -98,7 +98,10 @@ class QuantizationConfig(StrictModel):
     pretrained_run_id: Optional[Slug] = None
     pretrained_checkpoint_artifact_id: Optional[Slug] = Field(default=None, exclude_if=lambda value: value is None)
     pretrained_checkpoint_sha256: Optional[Sha256] = Field(default=None, exclude_if=lambda value: value is None)
-    label: str = ""
+    # Names a directory under the run's save/ and log/ trees, so it is an
+    # identifier and not a path: an absolute label would otherwise win the
+    # os.path.join in modules/paths.py and escape the run directory.
+    label: str = Field(default="", pattern=r"^$|^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$")
 
 
 class PAReference(StrictModel):

opendpd/server/routes.py

@@ -58,6 +58,10 @@
 from opendpd.schemas.analysis import DatasetAnalysis
 from opendpd.schemas.importing import BuiltinDatasetInfo, CsvInspection, CsvOptions, DatasetImportDefaults
 from opendpd.schemas.common import Slug, Sha256
+
+# The Slug pattern as a plain string: Query() needs it stated, an Annotated
+# Field inside Slug does not reach the query validator.
+SLUG_PATTERN = r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$"
 from opendpd.services import experiments
 from opendpd.services import packages
 from opendpd.services.evaluation import available_profiles, compare_results, comparison_csv
@@ -243,7 +247,8 @@ def recipes():
 
 class ImportBuiltinRequest(BaseModel):
     name: str
-    dataset_id: Optional[str] = None
+    # An identifier, never a path fragment: it names a directory under datasets/.
+    dataset_id: Optional[Slug] = None
 
 
 class ImportRootInfo(BaseModel):
@@ -311,8 +316,10 @@ class ManifestUpdate(BaseModel):
 
 class PreprocessRequest(BaseModel):
     params: PreprocessingParams
-    base_version: str = "raw-v1"
-    version: Optional[str] = Field(default=None, max_length=64)   # required to create, ignored for preview
+    # Both name a directory under the dataset's versions/, so both are
+    # identifiers rather than paths.
+    base_version: Slug = "raw-v1"
+    version: Optional[Slug] = Field(default=None, max_length=64)   # required to create, ignored for preview
 
 
 class PreprocessPreview(BaseModel):
@@ -446,7 +453,7 @@ def dataset_get(dataset_id: str, request: Request):
 
 @router.get("/datasets/{dataset_id}/analysis", response_model=DatasetAnalysis, tags=["datasets"],
             dependencies=[Depends(require_session)])
-def dataset_analysis(dataset_id: str, request: Request, version: str = Query("raw-v1", max_length=64)):
+def dataset_analysis(dataset_id: str, request: Request, version: str = Query("raw-v1", max_length=64, pattern=SLUG_PATTERN)):
     return analyze_dataset(_ws(request), dataset_id, version)
 
 
@@ -466,7 +473,7 @@ def dataset_diagnostics_latest(dataset_id: str, request: Request):
 
 @router.post("/datasets/{dataset_id}/diagnostics", response_model=DiagnosticReport, tags=["datasets"],
              dependencies=[Depends(require_csrf)])
-def dataset_diagnostics_run(dataset_id: str, request: Request, version: str = Query("raw-v1", max_length=64)):
+def dataset_diagnostics_run(dataset_id: str, request: Request, version: str = Query("raw-v1", max_length=64, pattern=SLUG_PATTERN)):
     return datasets_service.run_doctor(_ws(request), dataset_id, version)
 
 

opendpd/services/datasets.py

@@ -31,8 +31,8 @@
     SignalSpec,
     SplitSpec,
 )
-from opendpd.services.workspace import Workspace, WorkspaceError, combined_sha256, read_json, sha256_file, slugify, \
-    write_json_atomic
+from opendpd.services.workspace import Workspace, WorkspaceError, checked_identifier, combined_sha256, read_json, \
+    sha256_file, slugify, write_json_atomic
 from opendpd.schemas.importing import CsvOptions, DatasetImportDefaults
 
 LOGICAL = ("I_in", "Q_in", "I_out", "Q_out")
@@ -329,7 +329,7 @@ def _materialise(ws: Workspace, manifest: DatasetManifest, version: str, x: np.n
     n = len(x)
     boundaries = contiguous_boundaries(n, ratios, guard)
     split = SplitSpec(version=SPLIT_VERSION, ratios=ratios, guard_samples=guard, boundaries=boundaries)
-    directory = ws.dataset_dir(manifest.dataset_id) / "versions" / version
+    directory = ws.dataset_dir(manifest.dataset_id) / "versions" / checked_identifier(version, "version")
     if directory.exists():
         raise ImportError_(f"version '{version}' already exists for dataset '{manifest.dataset_id}'")
     files = _write_split_dir(directory, x, y, boundaries, _legacy_spec(manifest.signal, split, manifest.display_name))

opendpd/services/workspace.py

@@ -19,6 +19,7 @@
 import json
 import os
 import platform
+import re
 import secrets
 import shutil
 import sys
@@ -145,6 +146,19 @@ def _git_state():
         return None, None
 
 
+# Mirrors schemas.common.Slug. Identifiers that name a directory are checked
+# here, at the one place every caller goes through, so a caller that forgets
+# cannot turn an identifier into a path separator or an absolute path.
+IDENTIFIER = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$")
+
+
+def checked_identifier(value: str, field: str = "identifier") -> str:
+    """Return ``value`` if it can only ever name one directory entry."""
+    if not isinstance(value, str) or not IDENTIFIER.fullmatch(value):
+        raise WorkspaceError(f"{field} '{value}' is not an identifier; it must match {IDENTIFIER.pattern}")
+    return value
+
+
 def slugify(name: str) -> str:
     out = "".join(c.lower() if c.isalnum() else "-" for c in name).strip("-")
     while "--" in out:
@@ -241,7 +255,7 @@ def save_settings(self, settings: WorkspaceSettings) -> WorkspaceSettings:
 
     # -- datasets ----------------------------------------------------------
     def dataset_dir(self, dataset_id: str) -> Path:
-        return self.datasets_dir / dataset_id
+        return self.datasets_dir / checked_identifier(dataset_id, "dataset id")
 
     def list_datasets(self) -> List[DatasetManifest]:
         out = []
@@ -265,7 +279,7 @@ def dataset_version_dir(self, dataset_id: str, version: str = "raw-v1") -> Path:
         """Directory in the trainer's split-CSV layout for one data version.
         Built-in datasets keep raw-v1 in ``raw/``; imports materialise every
         version (raw-v1 included) under ``versions/<name>/``."""
-        candidate = self.dataset_dir(dataset_id) / "versions" / version
+        candidate = self.dataset_dir(dataset_id) / "versions" / checked_identifier(version, "version")
         if candidate.is_dir():
             return candidate
         if version == "raw-v1":

opendpd/web/policy.py

@@ -92,8 +92,20 @@ def __post_init__(self):
             raise ValueError("tunnel host must begin with a random, private 32+ character label")
 
 
+# A dataset id contains no separator, so `/datasets/<id>` also matches static
+# sibling routes registered under the same prefix. FastAPI dispatches those to
+# the static endpoint, never to `dataset_get`, so an id pattern silently
+# publishes a route nobody listed. `import-roots` answers with absolute host
+# paths, which is exactly what `app.state.workspace_label` exists to keep out
+# of `/system/capabilities`. Keep such routes out by name.
+# `tests/unit/test_public_policy_surface.py` fails if a new one appears.
+NEVER_PUBLIC = ("/datasets/import-roots",)
+
+
 def allowed(method: str, path: str) -> bool:
-    return ".." not in path and any(re.fullmatch(pattern, path) for pattern in ROUTES.get(method, []))
+    if ".." in path or any(path == p or path.startswith(p + "/") for p in NEVER_PUBLIC):
+        return False
+    return any(re.fullmatch(pattern, path) for pattern in ROUTES.get(method, []))
 
 
 def check_slug(value, field: str):

tests/integration/test_hardening.py

@@ -179,6 +179,60 @@ def test_import_root_symlink_escapes_are_invisible_and_unreadable(client, ws, tm
         assert r.status_code in (404, 409) and "escapes" in r.text, (path, r.text)
 
 
+def test_builtin_dataset_ids_cannot_escape_the_workspace(client, ws, tmp_path):
+    """`dataset_id` names a directory under datasets/; it is never a path.
+
+    The directory was created and the built-in's seven files copied into it
+    before `DatasetManifest` rejected the id, so the refusal has to happen
+    before any write, not at manifest construction.
+    """
+    outside = tmp_path / "escaped"
+    for dataset_id in (str(outside), "../../escaped", "..", "a/b", "/etc/opendpd-escape"):
+        r = client.post("/api/v1/datasets/import-builtin",
+                        json={"name": "DPA_200MHz", "dataset_id": dataset_id})
+        assert r.status_code == 422, (dataset_id, r.status_code, r.text)
+    assert not outside.exists(), "a refused import still wrote outside the workspace"
+    assert sorted(p.name for p in ws.datasets_dir.iterdir()) == ["dpa-200mhz"], "a stray dataset directory was created"
+
+
+
+def test_dataset_versions_cannot_escape_the_workspace(client, ws, tmp_path):
+    """`version` names a directory under the dataset's versions/, so it is an identifier.
+
+    `_materialise` wrote the nine split files and only then let
+    `DatasetVersion` reject the name, so the refusal has to come first. The
+    read side is guarded at `Workspace.dataset_version_dir` for every caller.
+    """
+    before = sorted(p.name for p in (ws.dataset_dir("dpa-200mhz") / "versions").glob("*")) \
+        if (ws.dataset_dir("dpa-200mhz") / "versions").is_dir() else []
+    for version in ("../../../../escaped", "a/b", "/tmp/opendpd-escape", ".."):
+        r = client.post("/api/v1/datasets/dpa-200mhz/preprocess",
+                        json={"params": {}, "base_version": "raw-v1", "version": version})
+        assert r.status_code == 422, (version, r.status_code, r.text)
+    for version in ("../../../../etc", "a/b"):
+        assert client.get("/api/v1/datasets/dpa-200mhz/analysis",
+                          params={"version": version}).status_code == 422, version
+    after = sorted(p.name for p in (ws.dataset_dir("dpa-200mhz") / "versions").glob("*")) \
+        if (ws.dataset_dir("dpa-200mhz") / "versions").is_dir() else []
+    assert before == after, "a refused preprocess still materialised a version directory"
+
+def test_quantization_labels_cannot_escape_the_run_directory(client):
+    """`quantization.label` is joined into the run's save/ and log/ trees.
+
+    `os.path.join` discards everything before an absolute component, so an
+    absolute label would relocate the checkpoint write out of the run
+    directory that the supervisor's `cwd` and `run_in_directory` confine.
+    """
+    cfg = json.loads(instantiate("pa-gru-smoke-v1", "dpa-200mhz").model_dump_json())
+    for label in ("/tmp/opendpd-escape", "../../escape", "a/b", "."):
+        cfg["quantization"] = {"enabled": True, "label": label}
+        r = client.post("/api/v1/runs", json={"config": cfg, "name": "escape"})
+        assert r.status_code == 422, (label, r.status_code, r.text)
+        assert any("label" in d["field"] for d in r.json()["error"]["details"]), (label, r.text)
+    cfg["quantization"] = {"enabled": True, "label": "ci_quant"}
+    assert client.post("/api/v1/experiments/validate", json={"config": cfg}).status_code == 200
+
+
 # --- archives ----------------------------------------------------------------------------------
 
 def test_symlink_members_are_refused(share_package, tmp_path, client):