diff --git a/docs/architecture/public-studio.md b/docs/architecture/public-studio.md index 002f4ab..67ef3f4 100644 --- a/docs/architecture/public-studio.md +++ b/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/` 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. diff --git a/docs/architecture/threat-model.md b/docs/architecture/threat-model.md index 1d1e65a..353a408 100644 --- a/docs/architecture/threat-model.md +++ b/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` | diff --git a/docs/contracts/openapi.json b/docs/contracts/openapi.json index 1a19787..e303428 100644 --- a/docs/contracts/openapi.json +++ b/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" } diff --git a/opendpd/schemas/experiment.py b/opendpd/schemas/experiment.py index 22a2978..139583c 100644 --- a/opendpd/schemas/experiment.py +++ b/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): diff --git a/opendpd/server/routes.py b/opendpd/server/routes.py index ce1f30f..655cc83 100644 --- a/opendpd/server/routes.py +++ b/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) diff --git a/opendpd/services/datasets.py b/opendpd/services/datasets.py index 7d5fb35..7c61bb7 100644 --- a/opendpd/services/datasets.py +++ b/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)) diff --git a/opendpd/services/workspace.py b/opendpd/services/workspace.py index d3bf309..189b374 100644 --- a/opendpd/services/workspace.py +++ b/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//``.""" - 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": diff --git a/opendpd/web/policy.py b/opendpd/web/policy.py index d733303..076db07 100644 --- a/opendpd/web/policy.py +++ b/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/` 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): diff --git a/tests/integration/test_hardening.py b/tests/integration/test_hardening.py index 2c83b11..4700cf3 100644 --- a/tests/integration/test_hardening.py +++ b/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): diff --git a/tests/unit/test_public_policy_surface.py b/tests/unit/test_public_policy_surface.py new file mode 100644 index 0000000..3632787 --- /dev/null +++ b/tests/unit/test_public_policy_surface.py @@ -0,0 +1,62 @@ +"""The public allowlist must publish only routes somebody listed on purpose. + +`policy.ROUTES` matches request paths with regexes. A pattern that stands in for +an *id* (``SLUG``/``FILE_ID``) also matches any static sibling route under the +same prefix, and FastAPI then dispatches that request to the static endpoint +rather than to the by-id endpoint the pattern was written for. The route is +published without appearing in anyone's list. + +This test walks the real application and fails when a static route is reachable +*only* through an id pattern, so a new local route cannot become public by +accident. It is the standing check behind `policy.NEVER_PUBLIC`. +""" + +import re +from pathlib import Path + +from fastapi.routing import APIRoute + +from opendpd.server.app import create_app +from opendpd.web.policy import FILE_ID, NEVER_PUBLIC, ROUTES, SLUG, allowed + +API_PREFIX = "/api/v1" +ID_PLACEHOLDERS = (SLUG, FILE_ID) + + +def _static_api_routes(workspace: Path): + app = create_app(workspace, bootstrap_token="surface") + for route in app.routes: + if not isinstance(route, APIRoute) or "{" in route.path: + continue # only static routes can be shadowed + if not route.path.startswith(API_PREFIX): + continue + for method in sorted(route.methods - {"HEAD", "OPTIONS"}): + yield method, route.path[len(API_PREFIX):], route.endpoint.__name__ + + +def test_no_static_route_is_published_only_by_an_id_pattern(tmp_path): + accidental = [] + for method, path, endpoint in _static_api_routes(tmp_path): + if not allowed(method, path): + continue + matching = [p for p in ROUTES.get(method, []) if re.fullmatch(p, path)] + deliberate = [p for p in matching if not any(ph in p for ph in ID_PLACEHOLDERS)] + if not deliberate: + accidental.append(f"{method} {path} -> {endpoint}() published only by {matching}") + assert accidental == [], ( + "these local routes are reachable from the public app without being listed in " + "policy.ROUTES; add them to policy.NEVER_PUBLIC or list them deliberately:\n" + + "\n".join(accidental)) + + +def test_import_roots_is_never_published(): + """It answers with absolute host paths, which the public app hides elsewhere.""" + assert "/datasets/import-roots" in NEVER_PUBLIC + assert not allowed("GET", "/datasets/import-roots") + assert not allowed("GET", "/datasets/import-roots/data/files") + + +def test_never_public_does_not_shadow_a_real_dataset_id(): + """The guard matches whole segments, not prefixes of a longer id.""" + assert allowed("GET", "/datasets/import-roots-2024") + assert allowed("GET", "/datasets/dpa-200mhz")