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

[danieltyukov]

Four request fields end up as a directory name without being constrained to one, and one local route is reachable from the public app without anyone having listed it. All four were reproduced by running the code, not by reading it.

What I found

• dataset_id on POST /datasets/import-builtin, and version / base_version on POST /datasets/{id}/preprocess, are plain strings on the request models. The Slug that rejects them sits on the stored model, which is built after the directory has been created and the files copied in. So the caller gets a 500 and the data is already on disk: seven built-in CSVs for the first, nine split files for the second.
• quantization.label on POST /runs has no pattern at all. It gets joined into the run's save/ and log/ paths in modules/paths.py, so it lands outside the run directory that cwd=run_dir and run_in_directory exist to hold writes inside.
• /datasets/import-roots is itself a valid SLUG, so the /datasets/<id> pattern in web/policy.py matches it and the public app proxies it. FastAPI dispatches to import_roots, which replies with absolute host paths. That is the thing app.state.workspace_label was added to keep out of /system/capabilities.

Worth stating plainly, because it changes the severity: neither Path.__truediv__ nor os.path.join is concatenation. Both discard everything before an absolute component, so an absolute value relocates the write outright instead of walking up with ../. The read side is disclosure only, not code execution, since every np.load on that path already passes allow_pickle=False.

What changed

• All four fields are Slug in the request contracts. The existing max_length=64 caps stay alongside the new patterns, so nothing is loosened.
• Workspace.dataset_dir, Workspace.dataset_version_dir and datasets._materialise re-check through a new checked_identifier. I put it at those choke points rather than in the handlers so the other callers are covered too. That is what caught the version case after I had already fixed dataset_id, and it also happens to close GET /datasets/{id}/sample-counts.
• Routes that should never be public are named in policy.NEVER_PUBLIC. The test next to it is the part I would keep: it walks the real app and fails if any static route is reachable only through an id pattern, so the next one cannot slip in unnoticed.

How it was checked

• Six new tests, in the S13 suite and tests/unit/test_public_policy_surface.py. I reverted each fix and confirmed the matching test fails, so none of them pass by accident.

• openapi.json carries only the six lines this actually changes. I did not regenerate it: my pydantic emits contentMediaType where the committed file has format, plus two extra ValidationError keys, and that churn would have buried the real diff.

• Full run of tests/unit and tests/integration on this branch and on unmodified d7f0837: 684 passed, 1 skipped, 5 failed on this branch against 678 passed, 1 skipped, 5 failed on d7f0837. Same five failures, by name, on both:

  • test_openapi_is_exportable_and_committed (the pydantic version skew above)
  • test_launcher_shutdown.py::test_window_shutdown_... and test_launcher_browser_fallback.py::test_browser_failure_... (both block in a headless shell; the first sits in _block_until_interrupted)
  • test_public_studio.py::test_real_training_is_private_... and test_gpu_bridge.py::test_private_gpu_lease_...

  The +6 is exactly the new tests. Environment: Linux, Python 3.12, torch 2.14.0+cpu, pytest 8.4.2, CPU only, --timeout=200 --timeout-method=signal.

Behaviour change for callers: requests that used to get a 500 (dataset_id, version) or be accepted (label) now get a 422. All of them were outside the documented contract.

Left out on purpose

Found in the same pass, but each is a separate concern. Happy to send any of them as its own PR.

• TenantManager.sweep calls self.gpu.sweep() outside the try/except, and maintenance() has no handler of its own. An OSError out of GpuBroker.finish (it opens job.root/"logs"/"worker.log" guarded only by job.root.exists()) ends the cleanup task while cleanup_healthy stays True, so /healthz keeps answering 200 and expired workspaces stop being deleted. Only reachable with the GPU bridge configured.
• SessionStore.exchange never clears bootstrap_token, though the module docstring calls it one-time.
• require_csrf compares tokens with != rather than hmac.compare_digest, unlike exchange a few lines up.

Rollback

Revert either commit on its own; they touch disjoint files apart from their own tests. No migration and no stored state involved.

[gaochangw]

Reviewed 0844353 against the request and filesystem boundaries. The identifier checks now precede materialization, quantization output names are constrained, and the explicit deny plus route-surface regression closes the import-roots disclosure. The six regression cases also pass in the 2.2.6 working tree, alongside its broader Python regression. All required checks on this PR are green; no blocking findings in this change.

The separately identified GPU-maintenance failure is also reproduced in the 2.2.6 follow-up: it now fails health/admission closed while tenant expiry continues. Bootstrap documentation is being corrected to describe its intentional launcher-lifetime secret, and CSRF comparison is being hardened there.