From 1b5a116afa257ec5572170d0265d57218c5762c4 Mon Sep 17 00:00:00 2001 From: xmap <16776958+xmap@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:36:17 -0500 Subject: [PATCH] Validate storage-root and scan-probe-host settings at boot posix_checksum_roots and scan_probe_allowed_roots had no field validators, so a relative path or a bare "/" passed Settings() and only failed later: "/" normalizes to the empty string (normalize_storage_root), and the run_capture_path vault's CHECK constraint forbids an empty root, so the failure landed on the first write instead of at startup. Both now require every entry to be absolute and to normalize to a non-empty string; a trailing slash is still accepted since normalization already handles it. scan_probe_remote_host="" also passed validation silently: the paired _validate_scan_probe_remote_python check tests "if scan_probe_remote_host and not value", which treats "" as falsy and skips the check, and active_scan_transport's guard is "if host is not None", so "" was accepted as a configured remote host with nothing to connect to and failed the vault's CHECK constraint (host length 1-255) on upsert. Reject empty/whitespace-only host instead of coercing it to None, so the misconfiguration surfaces at boot rather than being silently papered over. Co-Authored-By: Claude Sonnet 5 --- apps/api/src/cora/infrastructure/config.py | 76 +++++++++++++++++ apps/api/tests/unit/test_settings.py | 98 ++++++++++++++++++++++ 2 files changed, 174 insertions(+) diff --git a/apps/api/src/cora/infrastructure/config.py b/apps/api/src/cora/infrastructure/config.py index 50970927ab5..5c0395c5f8b 100644 --- a/apps/api/src/cora/infrastructure/config.py +++ b/apps/api/src/cora/infrastructure/config.py @@ -14,6 +14,7 @@ from cora.infrastructure.auth.config import IdentityProviderConfig from cora.infrastructure.control_port_route import ControlPortRoute from cora.shared.capture_phase import CapturePhase +from cora.shared.storage_root import normalize_storage_root _ALLOWED_DATABASE_SCHEMES = ("postgresql://", "postgres://") @@ -1217,6 +1218,81 @@ def _validate_scan_probe_remote_python( raise ValueError(msg) return value + @field_validator("scan_probe_remote_host") + @classmethod + def _validate_scan_probe_remote_host(cls, value: str | None) -> str | None: + """Reject an empty or whitespace-only host rather than silently + treating it as unset. + + `SCAN_PROBE_REMOTE_HOST=""` is a different + signal than the variable being absent -- in a deployment's + settings template it usually means an interpolation that + resolved to nothing -- and `active_scan_transport` tests + `is not None`, so a bare "" was passing through as a configured + remote transport with no host to connect to, then failing the + vault's own CHECK constraint on the first upsert instead of at + boot. Coercing "" to `None` was the other option; rejecting is + chosen instead so the misconfiguration surfaces immediately + rather than being silently papered over. Leave the setting + unset to mean "no remote probe." + """ + if value is not None and not value.strip(): + msg = ( + "scan_probe_remote_host is set to an empty or " + "whitespace-only string. Leave it unset to disable the " + "remote scan probe, rather than setting it to an empty value." + ) + raise ValueError(msg) + return value + + @field_validator("posix_checksum_roots") + @classmethod + def _validate_posix_checksum_roots(cls, value: tuple[str, ...]) -> tuple[str, ...]: + """Refuse a root that cannot name a real storage tier at boot, + not at the first attestation. A relative path is meaningless to + `PosixChecksumAdapter`, which treats every root as absolute, and + `normalize_storage_root("/")` collapses to the empty string, + which the run_capture_path vault's own CHECK constraint forbids + -- so bare "/" would otherwise pass here and fail on the first + write instead of at boot. A trailing slash is fine: normalization + (`cora.shared.storage_root`) handles it. + """ + for root in value: + if not root.startswith("/"): + msg = f"posix_checksum_roots entry {root!r} is not an absolute path." + raise ValueError(msg) + if not normalize_storage_root(root): + msg = ( + f"posix_checksum_roots entry {root!r} normalizes to the " + "empty string. A root must name a facility-level storage " + "tier, not the filesystem root itself." + ) + raise ValueError(msg) + return value + + @field_validator("scan_probe_allowed_roots") + @classmethod + def _validate_scan_probe_allowed_roots(cls, value: tuple[str, ...]) -> tuple[str, ...]: + """Same rule as `posix_checksum_roots`, applied to the roots + allowlisted on `scan_probe_remote_host`: a relative path or a + bare "/" is a misconfiguration worth refusing at boot rather + than discovering when the remote probe's own resolver refuses + every locator. A trailing slash is fine: normalization + (`cora.shared.storage_root`) handles it. + """ + for root in value: + if not root.startswith("/"): + msg = f"scan_probe_allowed_roots entry {root!r} is not an absolute path." + raise ValueError(msg) + if not normalize_storage_root(root): + msg = ( + f"scan_probe_allowed_roots entry {root!r} normalizes to the " + "empty string. A root must name a facility-level storage " + "tier, not the filesystem root itself." + ) + raise ValueError(msg) + return value + @field_validator("capture_experiment_identity_pvs") @classmethod def _validate_capture_experiment_identity_pvs( diff --git a/apps/api/tests/unit/test_settings.py b/apps/api/tests/unit/test_settings.py index 2812f017893..c6b2f657392 100644 --- a/apps/api/tests/unit/test_settings.py +++ b/apps/api/tests/unit/test_settings.py @@ -623,3 +623,101 @@ def test_settings_scan_probe_remote_host_with_remote_python_is_accepted( settings = Settings() assert settings.scan_probe_remote_host == "tomdet" assert settings.scan_probe_remote_python == "/venv/bin/python3" + + +@pytest.mark.unit +def test_settings_scan_probe_remote_host_rejects_empty_string( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`SCAN_PROBE_REMOTE_HOST=""` used to pass validation and then + `active_scan_transport` (`is not None`) treated it as a configured + remote host, failing the vault's CHECK constraint on first upsert + instead of at boot.""" + import pydantic + + monkeypatch.setenv("SCAN_PROBE_REMOTE_HOST", "") + with pytest.raises(pydantic.ValidationError, match="scan_probe_remote_host is set to"): + Settings() + + +@pytest.mark.unit +def test_settings_scan_probe_remote_host_rejects_whitespace_only( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import pydantic + + monkeypatch.setenv("SCAN_PROBE_REMOTE_HOST", " ") + with pytest.raises(pydantic.ValidationError, match="scan_probe_remote_host is set to"): + Settings() + + +# --------------------------------------------------------------------------- +# posix_checksum_roots / scan_probe_allowed_roots: absolute-path boot checks +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_settings_posix_checksum_roots_accepts_trailing_slash( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Normalization (`cora.shared.storage_root`) handles the trailing + slash; the validator must not reject what normalization already fixes.""" + monkeypatch.setenv("POSIX_CHECKSUM_ROOTS", '["/local1/2BM/"]') + settings = Settings() + assert settings.posix_checksum_roots == ("/local1/2BM/",) + + +@pytest.mark.unit +def test_settings_posix_checksum_roots_rejects_relative_path( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import pydantic + + monkeypatch.setenv("POSIX_CHECKSUM_ROOTS", '["local1/2BM"]') + with pytest.raises(pydantic.ValidationError, match="is not an absolute path"): + Settings() + + +@pytest.mark.unit +def test_settings_posix_checksum_roots_rejects_bare_root( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A bare "/" normalizes to the empty string, which the + run_capture_path vault's CHECK constraint forbids; refuse it at + boot instead of at the first write.""" + import pydantic + + monkeypatch.setenv("POSIX_CHECKSUM_ROOTS", '["/"]') + with pytest.raises(pydantic.ValidationError, match="normalizes to the empty string"): + Settings() + + +@pytest.mark.unit +def test_settings_scan_probe_allowed_roots_accepts_trailing_slash( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("SCAN_PROBE_ALLOWED_ROOTS", '["/local1/2BM/"]') + settings = Settings() + assert settings.scan_probe_allowed_roots == ("/local1/2BM/",) + + +@pytest.mark.unit +def test_settings_scan_probe_allowed_roots_rejects_relative_path( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import pydantic + + monkeypatch.setenv("SCAN_PROBE_ALLOWED_ROOTS", '["local1/2BM"]') + with pytest.raises(pydantic.ValidationError, match="is not an absolute path"): + Settings() + + +@pytest.mark.unit +def test_settings_scan_probe_allowed_roots_rejects_bare_root( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import pydantic + + monkeypatch.setenv("SCAN_PROBE_ALLOWED_ROOTS", '["/"]') + with pytest.raises(pydantic.ValidationError, match="normalizes to the empty string"): + Settings()