From e406a006f7334c86c1ff84b3256c0a32d982ead4 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:50:14 +0000 Subject: [PATCH 1/5] =?UTF-8?q?T071:=2013.2=20and=2013.3=20=E2=80=94=20loa?= =?UTF-8?q?d=5Fsettings=20refuses=20a=20non-PostgreSQL=20DSN=20and=20a=20c?= =?UTF-8?q?ollapsed=20DSN=20pair=20(plan=20034)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Realizes #1144 13.2 and 13.3, falsifier F13.1's `load_settings` block. - 13.2: `_refuse_non_postgresql_dsn` refuses either DSN (`OPENDOX_DATABASE_URL` or `OPENDOX_MIGRATION_DATABASE_URL`) whose URI scheme is not `postgresql://` or `postgres://`, naming the setting and the dialect kept. The keyword/value conninfo form (`host=h dbname=d …`) names no dialect at all and is unaffected — that syntax is libpq's own grammar, and no other driver reads it. - 13.3: `OPENDOX_MIGRATION_DATABASE_URL` stops being optional in `load_settings` (the `Setting` row's `required` flag, `_require` in place of `_optional`, and `RuntimeSettings.migration_database_url`'s type). A new `_refuse_the_same_dsn_in_both_settings` refuses the two DSNs being the exact same STRING, naming `OPENDOX_MIGRATION_DATABASE_URL`, once they are already known to agree on where they land (`_refuse_two_dsns_that_select_different_schemas`, unchanged, now called first): two DIFFERENT secrets for one role still pass, as the existing "single-role install" case documents. - Explicitly NOT in this task: 13.4-13.6 (`OPENDOX_INSTALL_MODE`, T070). Nothing here reads or names that setting, and `load_settings`'s only new required input is the migration DSN itself. Every existing call site that built an environment without `OPENDOX_MIGRATION_DATABASE_URL` needed one once it became required: `tests_runtime/conftest.py` gains a `migration_dsn` fixture (a `postgres_dsn` distinguished by a URI fragment, invisible to every DSN reader this module has); `test_api_endpoints.py`, `test_migrations_apply.py`, `test_runtime_cli.py` and `test_runtime_surface.py` thread it or a literal peer through. `test_two_dsns_that_select_different_schemas_are_refused`'s "a migration DSN that is simply absent" case is rewritten from accepted to refused, which is the behavior 13.3 changes. Two new tests (`test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept`, `test_the_same_dsn_in_both_settings_is_refused_naming_the_migration_one`) cover the two new refusals directly. Measured locally against this change (own Postgres container, bridge IP — this sandbox's host-mapped loopback ports are unreachable): `python -m pytest -q` reports 2469 passed, 11 skipped, 1 failed — the one failure is `tests/test_model_provider_broker.py::test_the_broker_child_inherits_no_ credential_shaped_environment`, already red against unmodified `main` (2d116415) in the same environment (an `LC_CTYPE` ambient in this sandbox, unrelated to runtime/config.py). Against `main`'s own reading (2479 selected / 2468 passed / 11 skipped, matching this repo's last recorded CI triple), this change is +2/+2/+0 for the two new tests — `validate.yml`'s `Pin the triple` floors (`MIN_SELECTED=2476`, `MIN_PASSED=2465`, `EXPECT_SKIPPED=11`) permit the rise unchanged. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 104 ++++++++++++++++++--- tests_runtime/conftest.py | 31 ++++++- tests_runtime/test_api_endpoints.py | 18 +++- tests_runtime/test_migrations_apply.py | 7 ++ tests_runtime/test_runtime_cli.py | 124 ++++++++++++++++++++++++- tests_runtime/test_runtime_surface.py | 2 + 6 files changed, 264 insertions(+), 22 deletions(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 5a832e12..046a46ab 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -81,7 +81,7 @@ class Setting: "it must not be the identity migrations run as", ), Setting( - PREFIX + "MIGRATION_DATABASE_URL", None, False, True, + PREFIX + "MIGRATION_DATABASE_URL", None, True, True, "the PRIVILEGED DSN ordered-SQL migrations are applied with, used by " "`opendox runtime migrate` alone and never by the served application", ), @@ -180,10 +180,14 @@ class RuntimeSettings: """The resolved configuration of one runtime process. Construct with :func:`load_settings`; the fields are in `SETTINGS` order. + + `migration_database_url` is never `None` from either loader (plan 034, + 13.3): `load_settings` requires it exactly as it requires `database_url`, + and `load_migration_settings` already refused to load without one. """ database_url: str - migration_database_url: str | None + migration_database_url: str oidc_issuer: str oidc_audience: str oidc_jwks_url: str | None @@ -1288,6 +1292,63 @@ def effective_schema(dsn: str) -> str | None: return user_named_by(dsn) +#: THE ONLY DIALECT THIS RUNTIME KEEPS (plan 034, 13.2). `psycopg` is the one +#: driver `runtime` depends on and it speaks PostgreSQL alone, but a DSN is a +#: string and nothing stopped an operator writing `sqlite:///…` into either +#: setting and discovering the mismatch however far the code got before the +#: driver refused it. RULING Q1 keeps this database DOCUMENT-FREE, which is +#: why a second dialect is refused HERE rather than supported: it would double +#: every migration and every schema test forever, for a database that holds no +#: document. `postgres://` is accepted beside `postgresql://` because libpq +#: treats the two as one scheme. +POSTGRESQL_SCHEMES = frozenset({"postgresql", "postgres"}) + + +def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: + """`name`'s DSN is refused unless it selects a PostgreSQL scheme. + + A DSN in the keyword/value form (`host=h dbname=d …`) names NO DIALECT AT + ALL — that syntax is libpq's own conninfo grammar, and no other driver + reads it — so only the URI form is checked: `urlsplit` reports an EMPTY + scheme for the keyword/value form (there is no `://` to split on), and an + empty scheme is read as "says nothing" here, exactly as `schema_selected_by` + reads a DSN that names no schema as `None` rather than as a refusal. + """ + scheme = urllib.parse.urlsplit(dsn).scheme + if scheme and scheme not in POSTGRESQL_SCHEMES: + raise ConfigurationError( + f"{name} names the {scheme!r} dialect. PostgreSQL " + "(`postgresql://` or `postgres://`) is the only dialect this " + "runtime keeps: a second one would double every migration and " + "every schema test forever, for a database that holds no " + "document (RULING Q1)") + + +def _refuse_the_same_dsn_in_both_settings(served: str, migration: str) -> None: + """One credential pasted into both settings is refused (plan 034, 13.3). + + `OPENDOX_DATABASE_URL` is the least-privileged identity the API serves + with; `OPENDOX_MIGRATION_DATABASE_URL` is the privileged one ordered-SQL + migrations run as — the whole point of keeping two settings. A + single-user install is not a reason to collapse them into one: this is + the two settings simply BEING each other, which is different from + `_refuse_two_dsns_that_select_different_schemas` below, where they + DISAGREE about where they land. It is different too from the accepted + "single-role install" (`test_a_dsn_that_names_no_database_still_reaches_ + one`): two DSNs for the same ROLE with two DIFFERENT secrets are two + credentials, not one pasted twice, and this checks the value actually + given, not the identity it happens to resolve to. + """ + if served == migration: + raise ConfigurationError( + f"{PREFIX}MIGRATION_DATABASE_URL is the same value as " + f"{PREFIX}DATABASE_URL. The identity migrations run as must not " + "also be the identity the API serves with; give the migration " + "credential its own DSN, even where both reach the same " + "database (the values are not repeated: a DSN carries a " + "password)") + + def _refuse_two_dsns_that_select_different_schemas( served: str, migration: str | None) -> None: """Both DSNs must land in one schema, or neither answer means anything. @@ -1400,21 +1461,42 @@ def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: accept `HS256` would verify a token signed with the public key anybody can fetch from the broker's JWKS, and discovering that on the first request means it is already serving. + + BOTH DSNs ARE REQUIRED (plan 034, 13.3): `OPENDOX_MIGRATION_DATABASE_URL` + used to default to `None`, so the collapsed, single-role shape this + refuses (below) was reachable only by accident, through a caller who + happened to set both to the same value — an install that left the + migration credential unset was never asked the question at all. Naming it + explicitly, even at the same database a served DSN already names, is what + keeps the two identities two DECISIONS rather than one remembered twice. """ env = os.environ if env is None else env algorithms = _algorithms(env) - # AND THE TWO DSNs LAND IN ONE SCHEMA. See - # `_refuse_two_dsns_that_select_different_schemas`: this is the half of - # that invariant a string can answer, and it is asked here because this is - # the one loader that holds BOTH values. - _refuse_two_dsns_that_select_different_schemas( - _require(env, _by_name(PREFIX + "DATABASE_URL")), - _optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL"))) + served = _require(env, _by_name(PREFIX + "DATABASE_URL")) + migration = _require(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) + # THE DIALECT FIRST: a scheme this module cannot parse as PostgreSQL is not + # yet a DSN worth comparing at all. + _refuse_non_postgresql_dsn(PREFIX + "DATABASE_URL", served) + _refuse_non_postgresql_dsn(PREFIX + "MIGRATION_DATABASE_URL", migration) + # THEN WHETHER THEY DISAGREE. See `_refuse_two_dsns_that_select_different_ + # schemas`: this is the half of that invariant a string can answer, and it + # is asked here because this is the one loader that holds BOTH values. A + # DSN compared against ITSELF can never disagree, so this step passes + # silently on exactly the pair the next one exists to catch. + _refuse_two_dsns_that_select_different_schemas(served, migration) + # AND, LAST, WHETHER THEY ARE SIMPLY EACH OTHER. Two DSNs that agree on + # where they land are ordinarily two credentials for the one database + # (`test_a_dsn_that_names_no_database_still_reaches_one`'s "single-role + # install" is exactly that, two DIFFERENT secrets for one role) — but + # agreement bought by pasting the SAME value into both settings is not a + # second decision at all, and this is the check the one before it cannot + # make. + _refuse_the_same_dsn_in_both_settings(served, migration) return RuntimeSettings( - database_url=_require(env, _by_name(PREFIX + "DATABASE_URL")), - migration_database_url=_optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")), + database_url=served, + migration_database_url=migration, oidc_issuer=_broker_url(env, _by_name(PREFIX + "OIDC_ISSUER"), required=True, is_a_base_url=True) or "", oidc_audience=_require(env, _by_name(PREFIX + "OIDC_AUDIENCE")), diff --git a/tests_runtime/conftest.py b/tests_runtime/conftest.py index 2c6e0b7b..93f53a4d 100644 --- a/tests_runtime/conftest.py +++ b/tests_runtime/conftest.py @@ -219,6 +219,29 @@ def postgres_dsn() -> str: return dsn +@pytest.fixture(scope="session") +def migration_dsn(postgres_dsn: str) -> str: + """A DSN for `OPENDOX_MIGRATION_DATABASE_URL`, distinct from `postgres_dsn`. + + Plan 034, 13.3: `load_settings` now REQUIRES this setting and refuses a + value that is simply `postgres_dsn` repeated (13.3's own collapse + refusal), so every fixture built from the pair below needs a second, + genuinely different string — not a second database. None of this + module's fixtures apply migrations through this identity: `database` + applies them directly with `MigrationRunner`, and the served app under + test is never asked to `migrate`. So `load_settings` is the only reader + that cares about this value at all, and what it asks of a DSN is a + PostgreSQL scheme (13.2), a STRING distinct from `postgres_dsn` (13.3), + and a database and schema that agree with it + (`_refuse_two_dsns_that_select_different_schemas`). A URI FRAGMENT is + invisible to `database_named_by`, `user_named_by` and `schema_selected_by` + alike — none of the three inspects `urlsplit(...).fragment` — so + appending one changes the STRING without moving the identity those + functions compare. + """ + return postgres_dsn + "#opendox-test-migration-identity" + + @pytest.fixture def database(postgres_dsn: str) -> Iterator[object]: """A `Database` on a throwaway schema, with the migrations already applied.""" @@ -357,7 +380,7 @@ def verifier(jwks_path: str): @pytest.fixture() -def client(database, postgres_dsn: str, verifier): +def client(database, postgres_dsn: str, migration_dsn: str, verifier): """A `TestClient` over the REAL application, on this test's own schema. The application is given its OWN `Database` on the same schema rather than @@ -376,6 +399,7 @@ def client(database, postgres_dsn: str, verifier): settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -399,8 +423,8 @@ def project_repository_root(tmp_path): @pytest.fixture() -def client_with_repositories(database, postgres_dsn: str, verifier, - project_repository_root): +def client_with_repositories(database, postgres_dsn: str, migration_dsn: str, + verifier, project_repository_root): """`client`, with the repository root pointed at this test's own directory.""" fastapi_testclient = _import_fastapi_testclient() from opendox.runtime.app import create_app @@ -409,6 +433,7 @@ def client_with_repositories(database, postgres_dsn: str, verifier, settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PROJECT_REPOSITORY_ROOT": str(project_repository_root), diff --git a/tests_runtime/test_api_endpoints.py b/tests_runtime/test_api_endpoints.py index 21897203..84caccfe 100644 --- a/tests_runtime/test_api_endpoints.py +++ b/tests_runtime/test_api_endpoints.py @@ -425,7 +425,7 @@ def test_the_unauthenticated_surface_is_exactly_the_two_probes( def test_the_schema_viewers_appear_only_when_the_install_says_so( - database, postgres_dsn: str, verifier) -> None: + database, postgres_dsn: str, migration_dsn: str, verifier) -> None: from fastapi.testclient import TestClient from opendox.runtime.app import create_app @@ -435,6 +435,7 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PUBLISH_OPENAPI": "true", @@ -448,7 +449,7 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( def test_readiness_refuses_an_unmigrated_database_by_name( - postgres_dsn: str, verifier) -> None: + postgres_dsn: str, migration_dsn: str, verifier) -> None: """`select 1` succeeds against a schema with no tables in it at all. Readiness without a migration check therefore turns a fresh install READY @@ -471,6 +472,7 @@ def test_readiness_refuses_an_unmigrated_database_by_name( try: settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -588,7 +590,8 @@ def test_draft_pagination_returns_the_principals_drafts_not_an_empty_page( def test_readiness_refuses_a_database_whose_migration_file_has_changed( - database, postgres_dsn: str, verifier, tmp_path) -> None: + database, postgres_dsn: str, migration_dsn: str, verifier, + tmp_path) -> None: """Nothing pending is not the same as matching this tree.""" import shutil @@ -609,6 +612,7 @@ def test_readiness_refuses_a_database_whose_migration_file_has_changed( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(tmp_path), @@ -852,7 +856,8 @@ def test_a_hidden_user_and_a_missing_one_answer_byte_for_byte_the_same( def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( - database, postgres_dsn: str, verifier, tmp_path) -> None: + database, postgres_dsn: str, migration_dsn: str, verifier, + tmp_path) -> None: """An EMPTY directory made `plan()` and `drift()` both empty. `discover()` returns `[]` for a directory that exists and holds no @@ -874,6 +879,7 @@ def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( empty.mkdir() settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(empty), @@ -1552,7 +1558,8 @@ def transaction(): def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( - database, postgres_dsn: str, rsa_key_pair, tmp_path: Path) -> None: + database, postgres_dsn: str, migration_dsn: str, rsa_key_pair, + tmp_path: Path) -> None: """The end of A25-2, measured where it was reported: at the HTTP boundary. A realm publishing an RSA and an EC signing key — Keycloak, the moment a @@ -1593,6 +1600,7 @@ def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, + PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) diff --git a/tests_runtime/test_migrations_apply.py b/tests_runtime/test_migrations_apply.py index 2afc0fe1..1b657628 100644 --- a/tests_runtime/test_migrations_apply.py +++ b/tests_runtime/test_migrations_apply.py @@ -345,6 +345,11 @@ def test_status_reports_a_reachable_database_and_its_applied_migrations( # libpq startup parameter `Database(schema=…)` sets for the harness. scoped = (f"{postgres_dsn}?options=-c%20search_path%3D{database.schema}") monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) + # A DISTINCT STRING (13.3's collapse refusal) THAT SELECTS THE SAME SCHEMA + # (`_refuse_two_dsns_that_select_different_schemas`): a URI FRAGMENT moves + # neither, since neither reader inspects one. + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker.test/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) @@ -720,6 +725,8 @@ def test_status_calls_an_unmigrated_database_unhealthy_and_blames_the_tree( conn.execute(f"create schema {schema}") try: monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 4a6d9d13..363caced 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -211,6 +211,8 @@ def test_init_creates_the_project_repository_root_and_touches_no_database( root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -362,6 +364,8 @@ def test_status_reports_every_declared_setting_and_none_as_null( from opendox.runtime.config import SETTING_NAMES monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://u:pw@127.0.0.1:1/x") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://m:pw@127.0.0.1:1/x") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "RUNTIME_PG_ROLE", "opendox_runtime") @@ -432,6 +436,8 @@ def test_init_refuses_a_repository_root_that_is_not_a_directory( root.write_text("not a directory", encoding="utf-8") monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -496,6 +502,8 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -541,6 +549,8 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -624,6 +634,8 @@ def __exit__(self, *exc: object) -> None: db_stub.Database = _Exploding # type: ignore[attr-defined] monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", dsn) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@db.internal:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -677,6 +689,8 @@ def __init__(self, config: object) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody:hunter2@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -794,6 +808,8 @@ def connection(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime:hunter2@127.0.0.1:5432/opendox") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", @@ -862,6 +878,8 @@ def transaction(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime@127.0.0.1:5432/opendox") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(tmp_path)) @@ -1148,6 +1166,7 @@ def test_no_broker_url_this_runtime_prints_can_carry_a_credential() -> None: redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ("OIDC_ISSUER", "OIDC_JWKS_URL"): env = dict(base, **{PREFIX + "OIDC_ISSUER": "https://broker/realms/x"}) @@ -1195,6 +1214,7 @@ def test_a_credential_in_a_broker_urls_query_is_refused_like_one_in_its_userinfo redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} for name, value, why in ( @@ -1243,6 +1263,7 @@ def test_the_broker_url_must_be_https_because_it_is_the_trust_anchor() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker/realms/x", "http://localhost:8080/realms/x", "http://127.0.0.1:8080/realms/x", "http://[::1]:8080/x"): @@ -1424,6 +1445,7 @@ def test_a_broker_url_that_names_no_host_is_refused_at_the_door() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https:///realms/x", "https://", "https:///", "broker/realms/x", "https://user:hunter2@/realms/x"): @@ -1509,6 +1531,7 @@ def test_a_broker_url_whose_port_is_not_a_number_is_refused_at_the_door( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker:not-a-port/realm", "https://broker:99999/realm", @@ -1554,6 +1577,7 @@ def test_the_issuer_carries_no_query_or_fragment_because_paths_are_appended( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer, component in (("https://broker/realms/x?tenant=a", "query"), ("https://broker/realms/x#frag", "fragment"), @@ -1599,6 +1623,7 @@ def test_the_loopback_exception_is_for_http_and_not_for_every_other_scheme( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("ftp://localhost/realms/x", "file://127.0.0.1/realms/x", "ws://localhost:8080/realms/x", "ftp://[::1]/realms/x"): @@ -1910,6 +1935,7 @@ def test_a_malformed_broker_url_never_prints_its_own_password() -> None: issuer = "https://svc:hunter2@broker\u2100evil.example/realms/x" env = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": issuer} @@ -2126,9 +2152,16 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db"}) - # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is the documented single-role - # deployment, not a mismatch. - assert load_settings({**base, PREFIX + "DATABASE_URL": served}) + # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is refused NOW (plan 034, + # 13.3), where it used to be accepted as "the documented single-role + # deployment, not a mismatch": that reading is exactly what `load_settings` + # reading the setting as OPTIONAL bought, and the setting stopped being + # optional. A single-role install still names both DSNs, distinctly — + # `test_a_dsn_that_names_no_database_still_reaches_one`'s "two different + # secrets for one role" is what single-role now looks like. + with pytest.raises(ConfigurationError) as absent: + load_settings({**base, PREFIX + "DATABASE_URL": served}) + assert PREFIX + "MIGRATION_DATABASE_URL" in str(absent.value) # THE OTHER DIRECTION IS REFUSED TOO: a served DSN that names no schema # beside a migration DSN that names one is the same split, and the @@ -2140,6 +2173,86 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: assert "the connection default" in str(either_way.value) +def test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept() -> None: + """13.2: a second dialect is refused, not supported. + + RULING Q1 keeps this database DOCUMENT-FREE, so a second dialect would + double every migration and every schema test forever for a database that + holds nothing. `_refuse_non_postgresql_dsn` asks it of both DSNs + `load_settings` holds, before either reaches the checks above that + compare them. + """ + from opendox.runtime.config import ConfigurationError, load_settings + + base = {PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox"} + with pytest.raises(ConfigurationError) as served: + load_settings({**base, + PREFIX + "DATABASE_URL": "sqlite:///x.db", + PREFIX + "MIGRATION_DATABASE_URL": + "postgresql://m:p@h/db"}) + assert "postgres" in str(served.value).lower() + assert PREFIX + "DATABASE_URL" in str(served.value) + + with pytest.raises(ConfigurationError) as migration: + load_settings({**base, + PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "mysql://m:p@h/db"}) + assert "postgres" in str(migration.value).lower() + assert PREFIX + "MIGRATION_DATABASE_URL" in str(migration.value) + + # `postgres://` IS THE OTHER SPELLING LIBPQ ACCEPTS, not a second dialect. + assert load_settings({**base, + PREFIX + "DATABASE_URL": "postgres://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": + "postgres://m:p@h/db"}) + + # AND THE KEYWORD/VALUE FORM NAMES NO DIALECT AT ALL, so it is not refused + # here: libpq's own conninfo grammar reaches no other driver, and this + # module already reads an empty scheme as "says nothing" the way + # `schema_selected_by` does for a DSN that names no schema. + assert load_settings({**base, + PREFIX + "DATABASE_URL": "host=h dbname=db", + PREFIX + "MIGRATION_DATABASE_URL": + "host=h dbname=db user=m"}) + + +def test_the_same_dsn_in_both_settings_is_refused_naming_the_migration_one( +) -> None: + """13.3: one credential pasted into both settings is refused. + + `_refuse_the_same_dsn_in_both_settings` is asked only once the two DSNs + are known to AGREE on where they land + (`_refuse_two_dsns_that_select_different_schemas`, above it): agreement + bought by two DIFFERENT secrets for the one role is the accepted + single-role shape + (`test_a_dsn_that_names_no_database_still_reaches_one`'s last case); + agreement bought by writing the SAME value into both settings is this + refusal instead. + """ + from opendox.runtime.config import ConfigurationError, load_settings + + base = {PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox"} + one = "postgresql://one:hunter2@h/opendox" + with pytest.raises(ConfigurationError) as collapsed: + load_settings({**base, + PREFIX + "DATABASE_URL": one, + PREFIX + "MIGRATION_DATABASE_URL": one}) + message = str(collapsed.value) + assert PREFIX + "MIGRATION_DATABASE_URL" in message + assert PREFIX + "DATABASE_URL" in message + # THE VALUE IS NOT REPEATED: a DSN carries a password. + assert "hunter2" not in message and "one:" not in message + + # DIFFERENT STRINGS THAT STILL AGREE are NOT this refusal, whether the + # difference is the secret alone (single-role) or the whole identity. + assert load_settings({**base, + PREFIX + "DATABASE_URL": "postgresql://a:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": + "postgresql://b:p@h/db"}) + + def test_two_dsns_naming_different_databases_are_refused_too() -> None: """The schema comparison means nothing across two databases. @@ -2348,6 +2461,7 @@ def test_a_credential_shaped_parameter_name_is_a_WORD_and_not_a_substring() -> N assert redacted_url("https://broker/certs?token=x" ) == "https://broker/certs?token=" base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox"} settings = load_settings({**base, @@ -2548,6 +2662,8 @@ def test_init_creates_the_repository_root_private_whatever_the_umask_is( root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -2584,6 +2700,8 @@ def test_init_reports_an_existing_root_s_mode_and_does_not_change_it( os.chmod(root, 0o755) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", + "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) diff --git a/tests_runtime/test_runtime_surface.py b/tests_runtime/test_runtime_surface.py index ff6a73a3..b36c6203 100644 --- a/tests_runtime/test_runtime_surface.py +++ b/tests_runtime/test_runtime_surface.py @@ -258,6 +258,7 @@ def test_only_asymmetric_algorithms_can_be_configured_and_they_keep_pyjwts_spell ) base = {PREFIX + "DATABASE_URL": "postgresql://x/y", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m@x/y", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ASYMMETRIC_ALGORITHMS: @@ -318,6 +319,7 @@ def test_every_integer_setting_is_bounded_above_as_well_as_below() -> None: ) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} From 91f797329ce81fa325f3c9b24512f209478316de Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:04:30 +0000 Subject: [PATCH 2/5] Fix round: _refuse_non_postgresql_dsn never raises a bare ValueError (Copilot review of this PR) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `urlsplit` itself raises for a DSN it cannot parse — MEASURED, ValueError("Invalid IPv6 URL") for an unbracketed IPv6 host, which tests_runtime/conftest.py's own postgres_dsn docstring names as "the ordinary way to mis-set this variable". `_refuse_non_postgresql_dsn` called `urlsplit(dsn).scheme` unguarded, so that ValueError escaped load_settings as a bare exception instead of the promised ConfigurationError — the CLI's boundary catches only ConfigurationError, so a malformed OPENDOX_DATABASE_URL or OPENDOX_MIGRATION_DATABASE_URL would have printed a traceback instead of a redacted refusal. Wrapped the same way _split_url already wraps it for the broker settings (Copilot review of openDox-code#25, round 24), with DSN-appropriate wording rather than reused verbatim ("set it to the broker endpoint" does not fit a database DSN). New test test_an_unparseable_dsn_is_refused_and_never_raises_a_bare_valueerror proves both DSNs are covered and that the value is never repeated in the message. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 17 ++++++++++++- tests_runtime/test_runtime_cli.py | 40 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 046a46ab..ecc53d65 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1313,8 +1313,23 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: scheme for the keyword/value form (there is no `://` to split on), and an empty scheme is read as "says nothing" here, exactly as `schema_selected_by` reads a DSN that names no schema as `None` rather than as a refusal. + + `urlsplit` ITSELF RAISES for a DSN it cannot parse — MEASURED, + `ValueError("Invalid IPv6 URL")` for an unbracketed IPv6 host, which + `tests_runtime/conftest.py`'s own `postgres_dsn` docstring names as "the + ordinary way to mis-set this variable". `_split_url` exists for exactly + this shape in the broker settings (Copilot review of openDox-code#25, + round 24); this is its DSN-flavoured twin; a bad `OPENDOX_DATABASE_URL` + is not "set it to the broker endpoint", so it is not reused verbatim. """ - scheme = urllib.parse.urlsplit(dsn).scheme + try: + scheme = urllib.parse.urlsplit(dsn).scheme + except ValueError as exc: + raise ConfigurationError( + f"{name} is not a DSN this runtime can parse " + f"({type(exc).__name__}); the value is not repeated here, " + "because a DSN this runtime cannot parse can still carry a " + "password") from None if scheme and scheme not in POSTGRESQL_SCHEMES: raise ConfigurationError( f"{name} names the {scheme!r} dialect. PostgreSQL " diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 363caced..2c3c97eb 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -2217,6 +2217,46 @@ def test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept() -> None: "host=h dbname=db user=m"}) +def test_an_unparseable_dsn_is_refused_and_never_raises_a_bare_valueerror( +) -> None: + """`urlsplit` itself raises for a DSN it cannot parse, and this module's + whole contract is that a bad variable produces a named + `ConfigurationError`, never a bare exception the CLI's boundary does not + catch (Copilot review of this PR). + + MEASURED: `urllib.parse.urlsplit("postgresql://u:p@[::1/db")` raises + `ValueError("Invalid IPv6 URL")` — an unbracketed IPv6 host, which + `tests_runtime/conftest.py`'s own `postgres_dsn` docstring names as "the + ordinary way to mis-set this variable". `_split_url` exists for exactly + this shape in the broker settings (Copilot review of openDox-code#25, + round 24); `_refuse_non_postgresql_dsn` is its own boundary for the two + DSNs, asked of both. + """ + from opendox.runtime.config import ConfigurationError, load_settings + + base = {PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox"} + broken = "postgresql://opendox:hunter2@[::1/opendox" + + with pytest.raises(ConfigurationError) as served: + load_settings({**base, + PREFIX + "DATABASE_URL": broken, + PREFIX + "MIGRATION_DATABASE_URL": + "postgresql://m:p@h/db"}) + message = str(served.value) + assert PREFIX + "DATABASE_URL" in message + assert "ValueError" in message + # THE VALUE IS NOT REPEATED: a DSN this runtime cannot parse can still + # carry a password. + assert "hunter2" not in message and "opendox:" not in message + + with pytest.raises(ConfigurationError) as migration: + load_settings({**base, + PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": broken}) + assert PREFIX + "MIGRATION_DATABASE_URL" in str(migration.value) + + def test_the_same_dsn_in_both_settings_is_refused_naming_the_migration_one( ) -> None: """13.3: one credential pasted into both settings is refused. From b5296f91c5df003964bf5c8f0126f6005da1fbbd Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 22:45:45 +0000 Subject: [PATCH 3/5] Rework #60 to Brett's ruling: OPENDOX_MIGRATION_DATABASE_URL required only for migrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brett ruled on the held conflict (openxFactory#656, on the claim thread for plan 034's T071, 2026-09-28), choosing "Required only for migrate (Recommended)" over making the setting required everywhere: - OPENDOX_MIGRATION_DATABASE_URL goes back to OPTIONAL in `load_settings` (the `Setting` row's `required` flag, `RuntimeSettings.migration_ database_url`'s type back to `str | None`, `_optional` in place of `_require`). `load_migration_settings` is unaffected either way — it already independently required one, for `migrate`/`reset` alone. - Both refusals from the previous commits stay, and are now no-ops on an ABSENT migration DSN rather than being unreachable: `_refuse_non_ postgresql_dsn` and `_refuse_the_same_dsn_in_both_settings` each return early when the migration value is falsy, exactly the way `_refuse_two_ dsns_that_select_different_schemas` already treated "nothing to compare" as nothing to fault. When BOTH are given, every check still runs, in the same order as before (dialect, then schema-mismatch, then collapse). It is never defaulted from OPENDOX_DATABASE_URL. - This matches #1144 13.3's own text and `deploy/compose/docker-compose. yaml`'s separation (the `opendox` service never gets a migration DSN; `docs/runtime.md` § 3 never lists it as required) — neither file needed a change; both already said the now-ruled behavior. The plan's "stops being optional" line is a holder-side correction, not part of this PR, and #1144's own wording is unchanged. Reverted the 27-call-site ripple the `required` flip had forced, now that it is not needed: `tests_runtime/conftest.py`'s `migration_dsn` fixture is gone; `test_api_endpoints.py`, `test_migrations_apply.py`, `test_runtime_ cli.py` and `test_runtime_surface.py` are back to threading only the served DSN through every call site that does not itself test the migration path. All four files after conftest.py are byte-for-byte `main` again. `test_two_dsns_that_select_different_schemas_are_refused`'s "absent migration" case is back to ACCEPTED (with a note on why it was briefly the opposite), which is what the setting being optional again means for that test. Added three tests showing the ruled behavior, at the CLI dispatch level rather than only `load_settings` directly, next to the existing `migrate` counterpart: - `test_serve_and_status_load_with_no_migration_dsn_configured`: `status` reports no configuration refusal and `settings[…MIGRATION_DATABASE_URL] ` as `null` with only the served DSN set; `serve` starts (`ok: true`) the same way. - `test_the_collapse_is_refused_through_the_served_workload_too`: 13.3's collapse refusal still fires through `status`, not only through `load_settings` called directly, the moment both DSNs are given and are the same value. - `test_migrate_refuses_rather_than_borrowing_the_served_identity` (pre-existing, untouched) already covers "migrate refuses without it". Measured locally against this change (own Postgres container, bridge IP): `python -m pytest -q` reports 2472 passed, 11 skipped, 1 failed — the one failure is the same `tests/test_model_provider_broker.py:: test_the_broker_child_inherits_no_credential_shaped_environment` LC_CTYPE sandbox artifact already characterized as pre-existing and unrelated in the first commit on this branch. Against main's 2479 selected / 11 skipped in this same environment, this change is +5/+5/+0 (five tests: the three already on this branch plus the two new ones above) — `validate.yml`'s `Pin the triple` floors (`MIN_SELECTED=2476`, `MIN_PASSED=2465`, `EXPECT_SKIPPED=11`) permit the rise unchanged, and the exact skip count is unchanged. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Sonnet 5 --- src/opendox/runtime/config.py | 61 +++++++++++---- tests_runtime/conftest.py | 31 +------- tests_runtime/test_api_endpoints.py | 18 ++--- tests_runtime/test_migrations_apply.py | 7 -- tests_runtime/test_runtime_cli.py | 101 +++++++++++++++---------- tests_runtime/test_runtime_surface.py | 2 - 6 files changed, 113 insertions(+), 107 deletions(-) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index ecc53d65..deaf53e9 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -81,7 +81,7 @@ class Setting: "it must not be the identity migrations run as", ), Setting( - PREFIX + "MIGRATION_DATABASE_URL", None, True, True, + PREFIX + "MIGRATION_DATABASE_URL", None, False, True, "the PRIVILEGED DSN ordered-SQL migrations are applied with, used by " "`opendox runtime migrate` alone and never by the served application", ), @@ -181,13 +181,17 @@ class RuntimeSettings: Construct with :func:`load_settings`; the fields are in `SETTINGS` order. - `migration_database_url` is never `None` from either loader (plan 034, - 13.3): `load_settings` requires it exactly as it requires `database_url`, - and `load_migration_settings` already refused to load without one. + `migration_database_url` is `None` from `load_settings` whenever + `OPENDOX_MIGRATION_DATABASE_URL` is unset — RULED "required only for + migrate" (openxFactory#656, on the claim thread for plan 034's T071, + 2026-09-28): the served workload never needs it, `deploy/compose/ + docker-compose.yaml`'s `opendox` service and `docs/runtime.md` § 3 never + supply it, and `load_migration_settings` is the loader that actually + requires one (unaffected by this: it already refused to load without one). """ database_url: str - migration_database_url: str + migration_database_url: str | None oidc_issuer: str oidc_audience: str oidc_jwks_url: str | None @@ -1304,9 +1308,16 @@ def effective_schema(dsn: str) -> str | None: POSTGRESQL_SCHEMES = frozenset({"postgresql", "postgres"}) -def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: +def _refuse_non_postgresql_dsn(name: str, dsn: str | None) -> None: """`name`'s DSN is refused unless it selects a PostgreSQL scheme. + `None` OR EMPTY IS A NO-OP, not a refusal: `OPENDOX_MIGRATION_DATABASE_URL` + is optional for `load_settings` (RULED "required only for migrate", + openxFactory#656, on the claim thread for plan 034's T071, 2026-09-28), + so an absent migration DSN has no dialect to check — the same shape + `_refuse_two_dsns_that_select_different_schemas` below already reads as + "nothing to compare" rather than as a fault. + A DSN in the keyword/value form (`host=h dbname=d …`) names NO DIALECT AT ALL — that syntax is libpq's own conninfo grammar, and no other driver reads it — so only the URI form is checked: `urlsplit` reports an EMPTY @@ -1322,6 +1333,8 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: round 24); this is its DSN-flavoured twin; a bad `OPENDOX_DATABASE_URL` is not "set it to the broker endpoint", so it is not reused verbatim. """ + if not dsn: + return try: scheme = urllib.parse.urlsplit(dsn).scheme except ValueError as exc: @@ -1339,9 +1352,18 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str) -> None: "document (RULING Q1)") -def _refuse_the_same_dsn_in_both_settings(served: str, migration: str) -> None: +def _refuse_the_same_dsn_in_both_settings( + served: str, migration: str | None) -> None: """One credential pasted into both settings is refused (plan 034, 13.3). + A NO-OP WHEN MIGRATION IS ABSENT, exactly like + `_refuse_two_dsns_that_select_different_schemas` below: with nothing to + compare, there is nothing to have collapsed. `OPENDOX_MIGRATION_DATABASE_ + URL` is optional (RULED "required only for migrate", openxFactory#656, on + the claim thread for plan 034's T071, 2026-09-28) — but WHEN BOTH ARE + GIVEN, this refusal still applies, on every path `load_settings` serves, + not only the falsifier's. + `OPENDOX_DATABASE_URL` is the least-privileged identity the API serves with; `OPENDOX_MIGRATION_DATABASE_URL` is the privileged one ordered-SQL migrations run as — the whole point of keeping two settings. A @@ -1354,6 +1376,8 @@ def _refuse_the_same_dsn_in_both_settings(served: str, migration: str) -> None: credentials, not one pasted twice, and this checks the value actually given, not the identity it happens to resolve to. """ + if not migration: + return if served == migration: raise ConfigurationError( f"{PREFIX}MIGRATION_DATABASE_URL is the same value as " @@ -1477,21 +1501,26 @@ def load_settings(env: Mapping[str, str] | None = None) -> RuntimeSettings: fetch from the broker's JWKS, and discovering that on the first request means it is already serving. - BOTH DSNs ARE REQUIRED (plan 034, 13.3): `OPENDOX_MIGRATION_DATABASE_URL` - used to default to `None`, so the collapsed, single-role shape this - refuses (below) was reachable only by accident, through a caller who - happened to set both to the same value — an install that left the - migration credential unset was never asked the question at all. Naming it - explicitly, even at the same database a served DSN already names, is what - keeps the two identities two DECISIONS rather than one remembered twice. + `OPENDOX_MIGRATION_DATABASE_URL` STAYS OPTIONAL HERE (RULED "required only + for migrate", openxFactory#656, on the claim thread for plan 034's T071, + 2026-09-28): the served workload never needs it — + `deploy/compose/docker-compose.yaml`'s `opendox` service and + `docs/runtime.md` § 3 never supply it, keeping the two identities in + different containers — and `load_migration_settings` below is the loader + that actually requires one. It is never DEFAULTED from + `OPENDOX_DATABASE_URL` either way. WHEN BOTH ARE GIVEN, though, the two + checks below still apply: a non-PostgreSQL migration DSN is refused + (13.2), and the two being the exact same value is refused (13.3) — + optional does not mean unchecked. """ env = os.environ if env is None else env algorithms = _algorithms(env) served = _require(env, _by_name(PREFIX + "DATABASE_URL")) - migration = _require(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) + migration = _optional(env, _by_name(PREFIX + "MIGRATION_DATABASE_URL")) # THE DIALECT FIRST: a scheme this module cannot parse as PostgreSQL is not - # yet a DSN worth comparing at all. + # yet a DSN worth comparing at all. A no-op on an ABSENT migration DSN — + # see `_refuse_non_postgresql_dsn`. _refuse_non_postgresql_dsn(PREFIX + "DATABASE_URL", served) _refuse_non_postgresql_dsn(PREFIX + "MIGRATION_DATABASE_URL", migration) # THEN WHETHER THEY DISAGREE. See `_refuse_two_dsns_that_select_different_ diff --git a/tests_runtime/conftest.py b/tests_runtime/conftest.py index 93f53a4d..2c6e0b7b 100644 --- a/tests_runtime/conftest.py +++ b/tests_runtime/conftest.py @@ -219,29 +219,6 @@ def postgres_dsn() -> str: return dsn -@pytest.fixture(scope="session") -def migration_dsn(postgres_dsn: str) -> str: - """A DSN for `OPENDOX_MIGRATION_DATABASE_URL`, distinct from `postgres_dsn`. - - Plan 034, 13.3: `load_settings` now REQUIRES this setting and refuses a - value that is simply `postgres_dsn` repeated (13.3's own collapse - refusal), so every fixture built from the pair below needs a second, - genuinely different string — not a second database. None of this - module's fixtures apply migrations through this identity: `database` - applies them directly with `MigrationRunner`, and the served app under - test is never asked to `migrate`. So `load_settings` is the only reader - that cares about this value at all, and what it asks of a DSN is a - PostgreSQL scheme (13.2), a STRING distinct from `postgres_dsn` (13.3), - and a database and schema that agree with it - (`_refuse_two_dsns_that_select_different_schemas`). A URI FRAGMENT is - invisible to `database_named_by`, `user_named_by` and `schema_selected_by` - alike — none of the three inspects `urlsplit(...).fragment` — so - appending one changes the STRING without moving the identity those - functions compare. - """ - return postgres_dsn + "#opendox-test-migration-identity" - - @pytest.fixture def database(postgres_dsn: str) -> Iterator[object]: """A `Database` on a throwaway schema, with the migrations already applied.""" @@ -380,7 +357,7 @@ def verifier(jwks_path: str): @pytest.fixture() -def client(database, postgres_dsn: str, migration_dsn: str, verifier): +def client(database, postgres_dsn: str, verifier): """A `TestClient` over the REAL application, on this test's own schema. The application is given its OWN `Database` on the same schema rather than @@ -399,7 +376,6 @@ def client(database, postgres_dsn: str, migration_dsn: str, verifier): settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -423,8 +399,8 @@ def project_repository_root(tmp_path): @pytest.fixture() -def client_with_repositories(database, postgres_dsn: str, migration_dsn: str, - verifier, project_repository_root): +def client_with_repositories(database, postgres_dsn: str, verifier, + project_repository_root): """`client`, with the repository root pointed at this test's own directory.""" fastapi_testclient = _import_fastapi_testclient() from opendox.runtime.app import create_app @@ -433,7 +409,6 @@ def client_with_repositories(database, postgres_dsn: str, migration_dsn: str, settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PROJECT_REPOSITORY_ROOT": str(project_repository_root), diff --git a/tests_runtime/test_api_endpoints.py b/tests_runtime/test_api_endpoints.py index 84caccfe..21897203 100644 --- a/tests_runtime/test_api_endpoints.py +++ b/tests_runtime/test_api_endpoints.py @@ -425,7 +425,7 @@ def test_the_unauthenticated_surface_is_exactly_the_two_probes( def test_the_schema_viewers_appear_only_when_the_install_says_so( - database, postgres_dsn: str, migration_dsn: str, verifier) -> None: + database, postgres_dsn: str, verifier) -> None: from fastapi.testclient import TestClient from opendox.runtime.app import create_app @@ -435,7 +435,6 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "PUBLISH_OPENAPI": "true", @@ -449,7 +448,7 @@ def test_the_schema_viewers_appear_only_when_the_install_says_so( def test_readiness_refuses_an_unmigrated_database_by_name( - postgres_dsn: str, migration_dsn: str, verifier) -> None: + postgres_dsn: str, verifier) -> None: """`select 1` succeeds against a schema with no tables in it at all. Readiness without a migration check therefore turns a fresh install READY @@ -472,7 +471,6 @@ def test_readiness_refuses_an_unmigrated_database_by_name( try: settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) @@ -590,8 +588,7 @@ def test_draft_pagination_returns_the_principals_drafts_not_an_empty_page( def test_readiness_refuses_a_database_whose_migration_file_has_changed( - database, postgres_dsn: str, migration_dsn: str, verifier, - tmp_path) -> None: + database, postgres_dsn: str, verifier, tmp_path) -> None: """Nothing pending is not the same as matching this tree.""" import shutil @@ -612,7 +609,6 @@ def test_readiness_refuses_a_database_whose_migration_file_has_changed( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(tmp_path), @@ -856,8 +852,7 @@ def test_a_hidden_user_and_a_missing_one_answer_byte_for_byte_the_same( def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( - database, postgres_dsn: str, migration_dsn: str, verifier, - tmp_path) -> None: + database, postgres_dsn: str, verifier, tmp_path) -> None: """An EMPTY directory made `plan()` and `drift()` both empty. `discover()` returns `[]` for a directory that exists and holds no @@ -879,7 +874,6 @@ def test_readiness_refuses_a_migrations_directory_without_the_pinned_0001( empty.mkdir() settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, PREFIX + "MIGRATIONS_DIR": str(empty), @@ -1558,8 +1552,7 @@ def transaction(): def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( - database, postgres_dsn: str, migration_dsn: str, rsa_key_pair, - tmp_path: Path) -> None: + database, postgres_dsn: str, rsa_key_pair, tmp_path: Path) -> None: """The end of A25-2, measured where it was reported: at the HTTP boundary. A realm publishing an RSA and an EC signing key — Keycloak, the moment a @@ -1600,7 +1593,6 @@ def test_an_anonymous_token_naming_a_key_of_the_wrong_type_is_401_not_500( settings = load_settings({ PREFIX + "DATABASE_URL": postgres_dsn, - PREFIX + "MIGRATION_DATABASE_URL": migration_dsn, PREFIX + "OIDC_ISSUER": TEST_ISSUER, PREFIX + "OIDC_AUDIENCE": TEST_AUDIENCE, }) diff --git a/tests_runtime/test_migrations_apply.py b/tests_runtime/test_migrations_apply.py index 1b657628..2afc0fe1 100644 --- a/tests_runtime/test_migrations_apply.py +++ b/tests_runtime/test_migrations_apply.py @@ -345,11 +345,6 @@ def test_status_reports_a_reachable_database_and_its_applied_migrations( # libpq startup parameter `Database(schema=…)` sets for the harness. scoped = (f"{postgres_dsn}?options=-c%20search_path%3D{database.schema}") monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) - # A DISTINCT STRING (13.3's collapse refusal) THAT SELECTS THE SAME SCHEMA - # (`_refuse_two_dsns_that_select_different_schemas`): a URI FRAGMENT moves - # neither, since neither reader inspects one. - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker.test/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) @@ -725,8 +720,6 @@ def test_status_calls_an_unmigrated_database_unhealthy_and_blames_the_tree( conn.execute(f"create schema {schema}") try: monkeypatch.setenv(PREFIX + "DATABASE_URL", scoped) - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - scoped + "#opendox-test-migration-identity") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", str(ROOT / "migrations")) diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 2c3c97eb..3b220eee 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -206,13 +206,66 @@ def test_migrate_refuses_rather_than_borrowing_the_served_identity( assert PREFIX + "MIGRATION_DATABASE_URL" in evidence["message"] +def test_serve_and_status_load_with_no_migration_dsn_configured( + monkeypatch: pytest.MonkeyPatch) -> None: + """Brett's ruling on #1144 13.3 ("Required only for migrate + (Recommended)"): `OPENDOX_MIGRATION_DATABASE_URL` stays OPTIONAL for the + served workload — `serve` and `status` both go through `load_settings`, + and this is the CLI-level proof that neither refuses at configuration + when only the served DSN is set. This is the shape + `deploy/compose/docker-compose.yaml`'s `opendox` service and + `docs/runtime.md` § 3 already document: the migration credential lives + only in the separate `migrate` service/profile. + """ + monkeypatch.delenv(PREFIX + "MIGRATION_DATABASE_URL", raising=False) + monkeypatch.setenv(PREFIX + "DATABASE_URL", + "postgresql://nobody@127.0.0.1:1/none") + monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") + monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") + + code, evidence = _run(cli.build_parser().parse_args( + ["runtime", "status", "--probe-timeout", "0.2"])) + assert evidence.get("refusal") != "configuration", evidence + assert "settings" in evidence, evidence + assert evidence["settings"][PREFIX + "MIGRATION_DATABASE_URL"] is None + + def _ok(self) -> None: + self.started = True + + _stub_uvicorn(monkeypatch, _ok) + monkeypatch.delenv(PREFIX + "MIGRATION_DATABASE_URL", raising=False) + code, evidence = _run_serve() + assert code == 0, evidence + assert evidence["ok"] is True, evidence + + +def test_the_collapse_is_refused_through_the_served_workload_too( + monkeypatch: pytest.MonkeyPatch) -> None: + """13.3's collapse refusal is `load_settings`'s own, not the falsifier's + special case: it fires for `status` (and every other served verb) too, + the moment an operator gives BOTH DSNs and they happen to be the exact + same value — even though migration is optional here (Brett's ruling, + above). + """ + same = "postgresql://opendox:hunter2@127.0.0.1:1/none" + monkeypatch.setenv(PREFIX + "DATABASE_URL", same) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", same) + monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") + monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") + + code, evidence = _run(cli.build_parser().parse_args( + ["runtime", "status", "--probe-timeout", "0.2"])) + assert code == 1, evidence + assert evidence["refusal"] == "configuration", evidence + assert PREFIX + "MIGRATION_DATABASE_URL" in evidence["message"] + assert "hunter2" not in evidence["message"], evidence["message"] + + def test_init_creates_the_project_repository_root_and_touches_no_database( monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -364,8 +417,6 @@ def test_status_reports_every_declared_setting_and_none_as_null( from opendox.runtime.config import SETTING_NAMES monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://u:pw@127.0.0.1:1/x") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://m:pw@127.0.0.1:1/x") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "RUNTIME_PG_ROLE", "opendox_runtime") @@ -436,8 +487,6 @@ def test_init_refuses_a_repository_root_that_is_not_a_directory( root.write_text("not a directory", encoding="utf-8") monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -502,8 +551,6 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -549,8 +596,6 @@ def run(self) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") code, evidence = _run(cli.build_parser().parse_args(["runtime", "serve"])) @@ -634,8 +679,6 @@ def __exit__(self, *exc: object) -> None: db_stub.Database = _Exploding # type: ignore[attr-defined] monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", dsn) - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@db.internal:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -689,8 +732,6 @@ def __init__(self, config: object) -> None: monkeypatch.setitem(sys.modules, "opendox.runtime.app", app_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody:hunter2@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") @@ -808,8 +849,6 @@ def connection(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime:hunter2@127.0.0.1:5432/opendox") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "MIGRATIONS_DIR", @@ -878,8 +917,6 @@ def transaction(self): monkeypatch.setitem(sys.modules, "opendox.runtime.db", db_stub) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://runtime@127.0.0.1:5432/opendox") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:5432/opendox") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(tmp_path)) @@ -1166,7 +1203,6 @@ def test_no_broker_url_this_runtime_prints_can_carry_a_credential() -> None: redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ("OIDC_ISSUER", "OIDC_JWKS_URL"): env = dict(base, **{PREFIX + "OIDC_ISSUER": "https://broker/realms/x"}) @@ -1214,7 +1250,6 @@ def test_a_credential_in_a_broker_urls_query_is_refused_like_one_in_its_userinfo redacted_url) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} for name, value, why in ( @@ -1263,7 +1298,6 @@ def test_the_broker_url_must_be_https_because_it_is_the_trust_anchor() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker/realms/x", "http://localhost:8080/realms/x", "http://127.0.0.1:8080/realms/x", "http://[::1]:8080/x"): @@ -1445,7 +1479,6 @@ def test_a_broker_url_that_names_no_host_is_refused_at_the_door() -> None: from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https:///realms/x", "https://", "https:///", "broker/realms/x", "https://user:hunter2@/realms/x"): @@ -1531,7 +1564,6 @@ def test_a_broker_url_whose_port_is_not_a_number_is_refused_at_the_door( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("https://broker:not-a-port/realm", "https://broker:99999/realm", @@ -1577,7 +1609,6 @@ def test_the_issuer_carries_no_query_or_fragment_because_paths_are_appended( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer, component in (("https://broker/realms/x?tenant=a", "query"), ("https://broker/realms/x#frag", "fragment"), @@ -1623,7 +1654,6 @@ def test_the_loopback_exception_is_for_http_and_not_for_every_other_scheme( from opendox.runtime.config import ConfigurationError, load_settings base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for issuer in ("ftp://localhost/realms/x", "file://127.0.0.1/realms/x", "ws://localhost:8080/realms/x", "ftp://[::1]/realms/x"): @@ -1935,7 +1965,6 @@ def test_a_malformed_broker_url_never_prints_its_own_password() -> None: issuer = "https://svc:hunter2@broker\u2100evil.example/realms/x" env = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": issuer} @@ -2152,16 +2181,11 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db"}) - # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is refused NOW (plan 034, - # 13.3), where it used to be accepted as "the documented single-role - # deployment, not a mismatch": that reading is exactly what `load_settings` - # reading the setting as OPTIONAL bought, and the setting stopped being - # optional. A single-role install still names both DSNs, distinctly — - # `test_a_dsn_that_names_no_database_still_reaches_one`'s "two different - # secrets for one role" is what single-role now looks like. - with pytest.raises(ConfigurationError) as absent: - load_settings({**base, PREFIX + "DATABASE_URL": served}) - assert PREFIX + "MIGRATION_DATABASE_URL" in str(absent.value) + # AND A MIGRATION DSN THAT IS SIMPLY ABSENT is the documented single-role + # deployment, not a mismatch (RULED "required only for migrate", + # openxFactory#656, on the claim thread for plan 034's T071, 2026-09-28 — + # this assertion was briefly the opposite of itself, reverted here). + assert load_settings({**base, PREFIX + "DATABASE_URL": served}) # THE OTHER DIRECTION IS REFUSED TOO: a served DSN that names no schema # beside a migration DSN that names one is the same split, and the @@ -2501,7 +2525,6 @@ def test_a_credential_shaped_parameter_name_is_a_WORD_and_not_a_substring() -> N assert redacted_url("https://broker/certs?token=x" ) == "https://broker/certs?token=" base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox"} settings = load_settings({**base, @@ -2702,8 +2725,6 @@ def test_init_creates_the_repository_root_private_whatever_the_umask_is( root = tmp_path / "projects" monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) @@ -2740,8 +2761,6 @@ def test_init_reports_an_existing_root_s_mode_and_does_not_change_it( os.chmod(root, 0o755) monkeypatch.setenv(PREFIX + "DATABASE_URL", "postgresql://nobody@127.0.0.1:1/none") - monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", - "postgresql://migrate@127.0.0.1:1/none") monkeypatch.setenv(PREFIX + "OIDC_ISSUER", "https://broker/realms/x") monkeypatch.setenv(PREFIX + "OIDC_AUDIENCE", "opendox-runtime") monkeypatch.setenv(PREFIX + "PROJECT_REPOSITORY_ROOT", str(root)) diff --git a/tests_runtime/test_runtime_surface.py b/tests_runtime/test_runtime_surface.py index b36c6203..ff6a73a3 100644 --- a/tests_runtime/test_runtime_surface.py +++ b/tests_runtime/test_runtime_surface.py @@ -258,7 +258,6 @@ def test_only_asymmetric_algorithms_can_be_configured_and_they_keep_pyjwts_spell ) base = {PREFIX + "DATABASE_URL": "postgresql://x/y", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m@x/y", PREFIX + "OIDC_ISSUER": "https://broker/realms/x", PREFIX + "OIDC_AUDIENCE": "opendox-runtime"} for name in ASYMMETRIC_ALGORITHMS: @@ -319,7 +318,6 @@ def test_every_integer_setting_is_bounded_above_as_well_as_below() -> None: ) base = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", - PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:p@h/db", PREFIX + "OIDC_AUDIENCE": "opendox", PREFIX + "OIDC_ISSUER": "https://broker/realms/x"} From f097fd889c1b07bad291794d41bad5a1969f6c5c Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 28 Sep 2026 22:57:28 +0000 Subject: [PATCH 4/5] Fix round: load_migration_settings gets the same dialect gate load_settings has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review of this PR (thread on _refuse_non_postgresql_dsn's own definition): 13.2's dialect gate was wired into `load_settings` only. `load_migration_settings` — the loader `runtime migrate`/`reset` actually use — read OPENDOX_MIGRATION_DATABASE_URL, checked only that it was non-empty, and handed it straight to `Database`, so a non-PostgreSQL migration DSN (`sqlite:///x.db`, say) reached the driver instead of being refused by name at configuration. That is the same un-named failure 13.2 exists to prevent for the served loader, just reachable through the one path F13.1's falsifier does not call. One call to the existing `_refuse_non_postgresql_dsn`, right after the existing empty-DSN refusal and before `database_url`/`migration_database_ url` are both set to the same value. New test `test_migrate_refuses_a_non_postgresql_migration_dsn_at_configuration` is the dialect-refused twin of the existing `test_migrate_and_reset_need_ no_served_identity_and_no_broker`, which already shows an unreachable but valid-dialect migration DSN getting PAST configuration — this one shows a wrong-dialect one refused AT configuration, naming the setting and never repeating the DSN. Measured locally (own Postgres container, bridge IP): 2473 passed (+1), 11 skipped, 1 failed (the same pre-existing, unrelated LC_CTYPE sandbox artifact) — the new test is the only change to the count. Arc: neutral-product-standalone-operability Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Co-Authored-By: Claude Sonnet 5 --- src/opendox/runtime/config.py | 8 ++++++++ tests_runtime/test_runtime_cli.py | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index deaf53e9..5c755006 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1590,6 +1590,14 @@ def load_migration_settings(env: Mapping[str, str] | None = None) -> RuntimeSett f"{PREFIX}MIGRATION_DATABASE_URL is required to apply migrations; " f"{PREFIX}DATABASE_URL is the served runtime's least-privileged " "identity and is deliberately not used for schema changes") + # THE SAME DIALECT GATE `load_settings` ASKS, asked here too (Copilot + # review of this PR): this loader is the one path 13.2's own falsifier + # does not reach, and without this call a non-PostgreSQL migration DSN + # sailed past configuration entirely and reached `Database` instead, + # which is exactly the un-named, un-refused failure 13.2 exists to + # prevent for `load_settings`. `database_url` is set to this same `dsn` + # immediately below, so one call here covers both fields. + _refuse_non_postgresql_dsn(PREFIX + "MIGRATION_DATABASE_URL", dsn) return RuntimeSettings( database_url=dsn, migration_database_url=dsn, diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index 3b220eee..feadfa1a 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -386,6 +386,29 @@ def test_migrate_and_reset_need_no_served_identity_and_no_broker( assert evidence["ok"] is False +def test_migrate_refuses_a_non_postgresql_migration_dsn_at_configuration( + monkeypatch: pytest.MonkeyPatch) -> None: + """13.2 covers `load_migration_settings` too, not only `load_settings`. + + The test above shows an unreachable but VALID-dialect migration DSN + getting past configuration; this is its dialect-refused twin (Copilot + review of this PR): `load_migration_settings` read the DSN and handed it + straight to `Database` with no dialect check of its own, so a + non-PostgreSQL migration DSN reached the driver instead of being refused + by name here — the same un-named failure 13.2 exists to prevent for the + served loader. + """ + for name in ("DATABASE_URL", "OIDC_ISSUER", "OIDC_AUDIENCE"): + monkeypatch.delenv(PREFIX + name, raising=False) + monkeypatch.setenv(PREFIX + "MIGRATION_DATABASE_URL", "sqlite:///x.db") + code, evidence = _run(cli.build_parser().parse_args( + ["runtime", "migrate", "--connect-timeout", "0.2"])) + assert code == 1, evidence + assert evidence["refusal"] == "configuration", evidence + assert "postgres" in evidence["message"].lower(), evidence + assert PREFIX + "MIGRATION_DATABASE_URL" in evidence["message"] + + def test_the_entrypoint_turns_an_escaped_exception_into_evidence( monkeypatch: pytest.MonkeyPatch) -> None: """Every outcome is one redacted JSON object; none is a traceback.""" From c39d960e4144a934fb2d39c2361ddd2b5ed0f8d4 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:28:35 +0000 Subject: [PATCH 5/5] Fix round: a PostgreSQL scheme libpq would not read as a URI is refused (Copilot review) Copilot's review at the merge-from-main head (adeb6fed) noted that `postgresql:foo` is still accepted, although the supported URI forms need `://`. Measured, it is worse than that. urlsplit reads `postgresql:` with no `//`, and any capitalized `PostgreSQL://` or `POSTGRES://`, as the PostgreSQL scheme, so the dialect gate passed all of them. libpq reads none of them as a URI. It recognizes only the exact, lower-case `postgresql://` and `postgres://`, parses the rest as keyword/value, and refuses them with a message that repeats the whole value (psycopg 3.3.6: `missing "=" after "postgresql:svc:hunter2@db/x" in connection info string`). That is the un-named failure at the driver that 13.2 exists to stop, and it carries the password. _refuse_non_postgresql_dsn now refuses a PostgreSQL scheme in any spelling other than libpq's two. The refusal names the setting and the two spellings, and does not repeat the value. Both loaders ask it. The new case covers four spellings for each of the two settings. All 8 fail against adeb6fed's config.py and pass here. The two accepted spellings and the keyword/value form still load. Five mutants are killed: the check dropped, case-insensitive matching, a check of only the `//`, every PostgreSQL DSN refused, and the value repeated. Full suite: 3069 selected, 3058 passed, 11 skipped, 0 failed. Lane: openxfactory-4 (openXfactory-4-openDox_extraction) Arc: neutral-product-standalone-operability Co-Authored-By: Claude Opus 5.5 (1M context) --- src/opendox/runtime/config.py | 20 ++++++++++++++++++++ tests_runtime/test_runtime_cli.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 5c755006..7ae28936 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -1350,6 +1350,26 @@ def _refuse_non_postgresql_dsn(name: str, dsn: str | None) -> None: "runtime keeps: a second one would double every migration and " "every schema test forever, for a database that holds no " "document (RULING Q1)") + # A POSTGRESQL SCHEME IS A URI ONLY IN LIBPQ'S OWN SPELLING (Copilot + # review of openDox-code#60, at its merge-from-main round). `urlsplit` + # reads `postgresql:` with no `//`, and any capitalized `PostgreSQL://`, + # as the PostgreSQL scheme. libpq does not: it recognizes a URI only by + # the exact, lower-case `postgresql://` or `postgres://`, and parses + # anything else as keyword/value, which it then refuses with a message + # that REPEATS THE WHOLE VALUE (measured, psycopg 3.3.6: + # `missing "=" after "postgresql:svc:hunter2@db/x" in connection info + # string`). That is the un-named failure at the driver that 13.2 exists to + # stop, and it carries the password with it. So it is refused here, named, + # and the value is not repeated. + if scheme in POSTGRESQL_SCHEMES and not dsn.startswith( + tuple(f"{known}://" for known in sorted(POSTGRESQL_SCHEMES))): + raise ConfigurationError( + f"{name} reads as the PostgreSQL scheme but is not a URI libpq " + "reads: libpq recognizes only the exact, lower-case " + "`postgresql://` or `postgres://` prefix, and would refuse any " + "other spelling with a message that repeats the whole value. " + "Write the scheme as one of those two (the value is not " + "repeated here, because it can carry a password)") def _refuse_the_same_dsn_in_both_settings( diff --git a/tests_runtime/test_runtime_cli.py b/tests_runtime/test_runtime_cli.py index feadfa1a..c3e13bb6 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -2264,6 +2264,34 @@ def test_a_non_postgresql_dsn_is_refused_naming_the_dialect_kept() -> None: "host=h dbname=db user=m"}) +@pytest.mark.parametrize("dsn", ["postgresql:svc:hunter2@db.invalid/x", + "postgresql:/svc:hunter2@db.invalid/x", + "PostgreSQL://svc:hunter2@db.invalid/x", + "POSTGRES://svc:hunter2@db.invalid/x"]) +@pytest.mark.parametrize("setting", ["DATABASE_URL", "MIGRATION_DATABASE_URL"]) +def test_a_postgresql_scheme_libpq_would_not_read_as_a_uri_is_refused( + dsn: str, setting: str) -> None: + """`urlsplit` reads each of these as the PostgreSQL scheme. libpq reads + none of them as a URI: it knows only the exact, lower-case + `postgresql://` and `postgres://`, so it parses the rest as keyword/value + and refuses them with a message that repeats the whole value, password + and all. So each is refused at configuration, named, and the value is + not repeated (Copilot review of this PR, at its merge-from-main round). + The two spellings libpq does read stay accepted (the case above).""" + from opendox.runtime.config import ConfigurationError, load_settings + + good = {PREFIX + "DATABASE_URL": "postgresql://u:p@h/db", + PREFIX + "MIGRATION_DATABASE_URL": "postgresql://m:q@h/db"} + with pytest.raises(ConfigurationError) as refused: + load_settings({PREFIX + "OIDC_ISSUER": "https://broker/realms/x", + PREFIX + "OIDC_AUDIENCE": "opendox", + **good, PREFIX + setting: dsn}) + message = str(refused.value) + assert PREFIX + setting in message, message + assert "postgresql://" in message, message + assert "hunter2" not in message and dsn not in message, message + + def test_an_unparseable_dsn_is_refused_and_never_raises_a_bare_valueerror( ) -> None: """`urlsplit` itself raises for a DSN it cannot parse, and this module's