From 5a71abc1debcbe3bbcda1781dda5262753e95c50 Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Mon, 21 Sep 2026 16:27:27 +0200 Subject: [PATCH 1/2] fix(config): reject a hostless domain and a relative state_dir - domain accepts a bare host or a full URL, so the check is on what _derive_domain yields rather than on the form of the value - a hostless domain used to fail later in OswExpress.validate_domain, quoting a regex instead of naming OSW_DOMAIN - state_dir "~/osw" created a directory literally named "~", because Ledger builds its path with Path() and never expands it - a relative state_dir resolves against a working directory the MCP client chooses, so the ledger landed in an unpredictable place - last open item from #143 --- docs/tools/configuration.md | 4 ++-- src/osw/service/config.py | 26 +++++++++++++++++++++++ tests/test_service_config.py | 40 ++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/docs/tools/configuration.md b/docs/tools/configuration.md index c605af08..11f2a683 100644 --- a/docs/tools/configuration.md +++ b/docs/tools/configuration.md @@ -100,14 +100,14 @@ set wins: | Canonical | Also accepted | Meaning | | --- | --- | --- | -| `OSW_DOMAIN` | `OSL_DOMAIN` | Instance to connect to | +| `OSW_DOMAIN` | `OSL_DOMAIN` | Instance to connect to. A bare host (`wiki.example.org`) or a full URL (`https://wiki.example.org/w/`); the host is taken from either | | `OSW_USERNAME` | `OSL_USERNAME` | Login user | | `OSW_PASSWORD` | `OSL_PASSWORD` | Login password | | `OSW_CRED_FILEPATH` | `OSW_MCP_CRED_FILEPATH`, `OSL_CRED_FILEPATH` | YAML credential file, keyed by iri (falls back to `accounts.pwd.yaml` in the working directory, CLI only) | | `OSW_ENV_FILE` | `OSW_MCP_ENV_FILE` | `.env` file to load | | `OSW_READ_ONLY` | `OSW_MCP_READ_ONLY` | `true` refuses every write | | `OSW_SPARQL_ENDPOINT` | | Endpoint for `sparql` queries | -| `OSW_STATE_DIR` | `OSW_MCP_STATE_DIR` | Where the provenance ledger is kept | +| `OSW_STATE_DIR` | `OSW_MCP_STATE_DIR` | Where the provenance ledger is kept. Must be an absolute path; a leading `~` is expanded | | `OSW_MAX_RESULTS` | `OSW_MCP_MAX_RESULTS` | Default result cap (100) | | `OSW_MAX_CHARS` | `OSW_MCP_MAX_CHARS` | Result size cap in characters (100000) | | `OSW_VERBOSE` | `OSW_MCP_VERBOSE` | `true` prints the configuration source report | diff --git a/src/osw/service/config.py b/src/osw/service/config.py index 35b79e9e..52397d42 100644 --- a/src/osw/service/config.py +++ b/src/osw/service/config.py @@ -117,6 +117,16 @@ def _validate_domain(cls, value: Optional[str]) -> Optional[str]: raise ValueError("must not contain whitespace") if any(ord(char) < 32 for char in value): raise ValueError("must not contain control characters") + # A bare host and a full URL are both legal here, so the check is on + # what _derive_domain makes of the value, not on its form. + # OswExpress.validate_domain rejects a hostless value too, but only on + # the first connection, and its message quotes a regex rather than + # naming the variable the operator has to correct. + if not _derive_domain(value): + raise ValueError( + "must contain a host name (e.g. 'wiki.example.org' or " + "'https://wiki.example.org/w/')" + ) return value @field_validator("sparql_endpoint") @@ -138,6 +148,22 @@ def _validate_state_dir(cls, value: Optional[str]) -> Optional[str]: return value if not value.strip(): raise ValueError("must not be empty or whitespace-only") + # The only validator here that rewrites its value. Ledger builds its + # file as Path(state_dir) / ... and never expands a leading ~, so + # "~/osw" used to create a directory literally named "~". + if value.startswith("~"): + try: + value = str(Path(value).expanduser()) + except RuntimeError as exc: + raise ValueError( + f"starts with '~' but the home directory cannot be " + f"determined ({exc})" + ) from exc + if not Path(value).is_absolute(): + raise ValueError( + "must be an absolute path: a relative one resolves against the " + "working directory, which for osw-mcp is chosen by the client" + ) return value @field_validator("cred_filepath") diff --git a/tests/test_service_config.py b/tests/test_service_config.py index d01c214f..677f430d 100644 --- a/tests/test_service_config.py +++ b/tests/test_service_config.py @@ -2,6 +2,7 @@ import os import sys +from pathlib import Path import pytest import yaml @@ -1055,6 +1056,45 @@ def test_domain_as_full_url_accepted(): assert settings.domain == "https://wiki.example.org/w/" +@pytest.mark.parametrize("value", ["https://", "/w/index.php", "//", "https:///w/"]) +def test_domain_without_a_host_rejected(value): + """A value _derive_domain cannot reduce to a host is unusable. + + Caught here rather than in OswExpress.validate_domain, which only runs on + the first connection and reports a regex rather than the variable name. + """ + with pytest.raises(ValidationError) as exc: + Settings(domain=value) + assert "host" in str(exc.value) + + +def test_state_dir_expands_a_leading_tilde(): + """Path(state_dir) never expands it, so '~/osw' made a directory named '~'. + + Ledger builds its file as Path(state_dir) / ... with no expanduser call + (src/osw/service/ledger.py:70), so the expansion has to happen here. + """ + settings = Settings(domain="wiki.example.org", state_dir="~/osw-state") + assert settings.state_dir == str(Path.home() / "osw-state") + + +@pytest.mark.parametrize("value", ["osw-state", "./osw-state", "../osw-state"]) +def test_state_dir_relative_rejected(value): + """A relative path resolves against a working directory the user may not own. + + The MCP client chooses the server's working directory, so the ledger would + land somewhere unpredictable. + """ + with pytest.raises(ValidationError) as exc: + Settings(domain="wiki.example.org", state_dir=value) + assert "absolute" in str(exc.value) + + +def test_state_dir_absolute_is_left_alone(tmp_path): + settings = Settings(domain="wiki.example.org", state_dir=str(tmp_path / "state")) + assert settings.state_dir == str(tmp_path / "state") + + def test_settings_is_frozen(): settings = Settings(domain="wiki.example.org") with pytest.raises(ValidationError): From 35506c39eb779fda9e6b8983a6da26bcfbf5d22b Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Mon, 21 Sep 2026 16:35:08 +0200 Subject: [PATCH 2/2] test(config): cover drive-relative state_dir and an unknown home - reject "\osw-state" and "C:osw-state", non-absolute on both platforms - assert the expanduser RuntimeError becomes a named config error - state in the docs that a domain without a host is rejected --- docs/tools/configuration.md | 2 +- tests/test_service_config.py | 37 ++++++++++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/docs/tools/configuration.md b/docs/tools/configuration.md index 11f2a683..989b6c7b 100644 --- a/docs/tools/configuration.md +++ b/docs/tools/configuration.md @@ -100,7 +100,7 @@ set wins: | Canonical | Also accepted | Meaning | | --- | --- | --- | -| `OSW_DOMAIN` | `OSL_DOMAIN` | Instance to connect to. A bare host (`wiki.example.org`) or a full URL (`https://wiki.example.org/w/`); the host is taken from either | +| `OSW_DOMAIN` | `OSL_DOMAIN` | Instance to connect to. A bare host (`wiki.example.org`) or a full URL (`https://wiki.example.org/w/`); the host is taken from either, and a value no host can be read from (`https://`, `/w/index.php`) is rejected at startup | | `OSW_USERNAME` | `OSL_USERNAME` | Login user | | `OSW_PASSWORD` | `OSL_PASSWORD` | Login password | | `OSW_CRED_FILEPATH` | `OSW_MCP_CRED_FILEPATH`, `OSL_CRED_FILEPATH` | YAML credential file, keyed by iri (falls back to `accounts.pwd.yaml` in the working directory, CLI only) | diff --git a/tests/test_service_config.py b/tests/test_service_config.py index 677f430d..e3cbb3b7 100644 --- a/tests/test_service_config.py +++ b/tests/test_service_config.py @@ -1078,18 +1078,51 @@ def test_state_dir_expands_a_leading_tilde(): assert settings.state_dir == str(Path.home() / "osw-state") -@pytest.mark.parametrize("value", ["osw-state", "./osw-state", "../osw-state"]) +@pytest.mark.parametrize( + "value", + [ + "osw-state", + "./osw-state", + "../osw-state", + # Drive-relative Windows forms: rooted without a drive, and a drive + # without a root. Both resolve against process state (the current drive, + # and that drive's working directory), so both are as unpredictable as a + # plain relative path. is_absolute() is False for both on Windows and on + # POSIX, so these parameters need no platform marker. + "\\osw-state", + "C:osw-state", + ], +) def test_state_dir_relative_rejected(value): """A relative path resolves against a working directory the user may not own. The MCP client chooses the server's working directory, so the ledger would - land somewhere unpredictable. + be created in an unpredictable place. """ with pytest.raises(ValidationError) as exc: Settings(domain="wiki.example.org", state_dir=value) assert "absolute" in str(exc.value) +def test_state_dir_reports_an_undeterminable_home(monkeypatch): + """The '~' expansion can fail, and the failure has to name the setting. + + Path.expanduser() raises RuntimeError when no home directory can be found. + Uncaught it would surface as a bare RuntimeError with no mention of + OSW_STATE_DIR. Reproducing that state differs per platform (Windows reads + USERPROFILE, POSIX falls back to the password database), so the raise itself + is patched in. + """ + + def _no_home(self): + raise RuntimeError("Could not determine home directory.") + + monkeypatch.setattr(Path, "expanduser", _no_home) + with pytest.raises(ValidationError) as exc: + Settings(domain="wiki.example.org", state_dir="~/osw-state") + assert "home directory" in str(exc.value) + + def test_state_dir_absolute_is_left_alone(tmp_path): settings = Settings(domain="wiki.example.org", state_dir=str(tmp_path / "state")) assert settings.state_dir == str(tmp_path / "state")