diff --git a/src/opendox/runtime/config.py b/src/opendox/runtime/config.py index 5a832e12..7ae28936 100644 --- a/src/opendox/runtime/config.py +++ b/src/opendox/runtime/config.py @@ -180,6 +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 `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 @@ -1288,6 +1296,118 @@ 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) -> 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 + 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. + """ + if not dsn: + return + 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 " + "(`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)") + # 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( + 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 + 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 not migration: + return + 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 +1520,47 @@ 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. + + `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) - # 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 = _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. 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_ + # 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")), @@ -1464,6 +1610,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 4a6d9d13..c3e13bb6 100644 --- a/tests_runtime/test_runtime_cli.py +++ b/tests_runtime/test_runtime_cli.py @@ -206,6 +206,61 @@ 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" @@ -331,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.""" @@ -2127,7 +2205,9 @@ def test_two_dsns_that_select_different_schemas_are_refused() -> None: 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. + # 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 @@ -2140,6 +2220,154 @@ 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"}) + + +@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 + 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. + + `_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.