From 2cd2c12ce40c4b1e443d1ea13fd94c4be9bd2e24 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:22:23 +0100 Subject: [PATCH 1/4] security: an acquisition source got this host's ambient database credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A contract names the host its postgres or mysql source lives on. libpq and mysql_scanner read credentials from the PROCESS environment for whatever host they are handed, so a contract naming a host of its own choosing could be authenticated with this machine's PGPASSWORD, PGPASSFILE, PGSERVICE or MYSQL_PWD — a credential the contract was never given, belonging to a different server. `fluid init --discover` has suppressed these since its JDBC introspection landed (14 variables, plus a no-client-certificate path so libpq never presents this host's ~/.postgresql/postgresql.crt). The acquisition runners had not, so the same contract fields reached the same client libraries with the environment intact. The duckdb runner's ATTACH now runs inside the same window. This route is INDEPENDENT of the contract's own `connection.secretRef`, and no allowlist over `secretRef` would close it, because the credential never passes through `secretRef` at all. That is why it ships on its own rather than waiting for the acquisition secretRef policy (founder decision, 2026-10-26). The implementation is shared, not copied: it moved to `credentials/host_db_env.py` because `build_runners` may not import `cli` — the import-linter contract "build_runners must not depend on cli" would have refused the obvious import, and a second copy of a credential-hiding window is exactly the kind of thing that drifts. `cli/discover/_jdbc_introspect` keeps `_HOST_CREDENTIAL_ENV`, `_NO_CLIENT_CERT`, `_HOST_CREDENTIAL_ENV_LOCK` and `_host_credential_env_suppressed` as aliases of the shared names, so its own tests and the patch points around them are untouched. Verified they are the SAME objects, not duplicates — one RLock, as the serialisation requires. The lock's documented limits are inherited as they are, and the docstring that states them moved with it: it serialises windows opened through this function and nothing else, so a concurrent `os.environ.copy()` or an `engine._run_sync_stage(env_snapshot=True)` overlapping a window is still outside its guarantee. Verified: 202 existing jdbc attach-safety tests pass through the aliases; `lint-imports` reports 6 contracts kept, 0 broken; the new test observes the environment from INSIDE the attach, which is the only place that proves the window wraps the call reaching libpq, and reverting the `with` fails it. --- CHANGELOG.md | 20 +++ fluid_build/build_runners/duckdb/runner.py | 8 +- fluid_build/cli/discover/_jdbc_introspect.py | 76 ++------ fluid_build/credentials/host_db_env.py | 107 ++++++++++++ .../test_duckdb_host_credential_env.py | 164 ++++++++++++++++++ 5 files changed, 312 insertions(+), 63 deletions(-) create mode 100644 fluid_build/credentials/host_db_env.py create mode 100644 tests/build_runners/test_duckdb_host_credential_env.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5973ccd95..54aabcf70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **An acquisition source no longer receives this host's ambient database credentials.** A contract + names the host its postgres or mysql source lives on, and libpq and mysql_scanner read + credentials from the process environment for whatever host they are handed — so a contract naming + a host of its own choosing could be authenticated with this machine's `PGPASSWORD`, `PGPASSFILE`, + `PGSERVICE` or `MYSQL_PWD`: a credential the contract was never given, belonging to a different + server. `fluid init --discover` has suppressed these since its JDBC introspection landed; the + acquisition runners had not, so the same contract fields reached the same client libraries with + the environment intact. + The duckdb runner's `ATTACH` now runs inside the same suppression window, which is shared from + `credentials/host_db_env.py` rather than copied (`build_runners` may not import `cli`). 14 + variables are covered, and `cli/discover/_jdbc_introspect` keeps its original private names as + aliases, so nothing that referenced them changes. + **This route is independent of a contract's own `connection.secretRef`** and no allowlist over + `secretRef` would close it, because the credential never passes through `secretRef` at all. The + declared DSN still carries its own credentials, so a legitimate source is unaffected and no + contract change is needed. + + ### Fixed - **An incremental cursor written on DuckDB 1.5 and read on 2.0 silently dropped or diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 4d7c63e21..cf20293a4 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -58,6 +58,7 @@ ) from fluid_build.api.schema import SchemaFingerprint from fluid_build.api.source import AcquisitionMode +from fluid_build.credentials.host_db_env import host_credential_env_suppressed from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -883,7 +884,12 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: raise _SourceSetupError(str(exc)) from exc except duckdb.Error as exc: # the extension did not load raise _SourceAttachError(str(exc)) from exc - _attach_with_password_masked(con, attach_sql, conn.get("password")) + # The contract chose this host, and libpq / mysql_scanner would + # read this machine's credentials for it (see host_db_env). The + # declared DSN still carries its own, so a real source is + # unaffected. + with host_credential_env_suppressed(): + _attach_with_password_masked(con, attach_sql, conn.get("password")) elif kind == "sqlite": conn = dict(ctx.source.connection.raw) # SQLite uses a path, not a libpq-style DSN. The path comes diff --git a/fluid_build/cli/discover/_jdbc_introspect.py b/fluid_build/cli/discover/_jdbc_introspect.py index ff0fadfae..f46770d95 100644 --- a/fluid_build/cli/discover/_jdbc_introspect.py +++ b/fluid_build/cli/discover/_jdbc_introspect.py @@ -78,6 +78,12 @@ from typing import Any, Dict, Iterator, List, Optional, Set, Tuple from urllib.parse import urlparse +from fluid_build.credentials.host_db_env import ( + HOST_CREDENTIAL_ENV, + HOST_CREDENTIAL_ENV_LOCK, + NO_CLIENT_CERT, + host_credential_env_suppressed, +) from fluid_build.providers._sql_safety import ( build_libpq_dsn, build_mysql_dsn, @@ -115,39 +121,13 @@ # MySQL client's connection defaults (MySQL 8.4 Reference Manual, "Environment # Variables"), cleared so none of them stands in for a part the DSN leaves out. # MYSQL_GROUP_SUFFIX only names extra option-file groups. DuckDB 1.5.6's -# mysql_scanner was seen to send no password from an option file (~/.my.cnf, -# ~/.mylogin.cnf, ~/my.cnf, /etc/my.cnf, /etc/mysql/my.cnf, $MYSQL_HOME/my.cnf), -# with or without that suffix, so clearing it is a precaution: there is no -# option-file fallback for it to close. -_HOST_CREDENTIAL_ENV = ( - "PGPASSWORD", - "PGPASSFILE", - "PGSERVICE", - "PGSERVICEFILE", - "PGSYSCONFDIR", - "PGSSLCERT", - "PGSSLKEY", - "PGOAUTHTOKEN", - "PGGSSDELEGATION", - "MYSQL_PWD", - "MYSQL_HOST", - "MYSQL_TCP_PORT", - "MYSQL_UNIX_PORT", - "MYSQL_GROUP_SUFFIX", -) - -# libpq skips a client certificate whose file does not exist (ENOENT or -# ENOTDIR). Pointing a pinned connection's certificate and key below os.devnull -# means it never presents this host's ~/.postgresql/postgresql.crt to a server -# that asks for one. -_NO_CLIENT_CERT = os.path.join(os.devnull, "fluid-no-client-certificate") - -# The suppression below mutates the process environment. forge_run's in-process -# worker thread can run alongside forge_from_source, so windows are serialised -# (and re-entrant within one thread) rather than interleaved: two overlapping -# windows could otherwise restore the host credentials while the second attach -# is still inside its own. -_HOST_CREDENTIAL_ENV_LOCK = threading.RLock() +# The one implementation now lives in ``credentials/host_db_env`` so the +# acquisition runners can use it too -- ``build_runners`` may not import +# ``cli``. These aliases keep this module's original private names, which its +# own tests and the patch points around them refer to. +_HOST_CREDENTIAL_ENV = HOST_CREDENTIAL_ENV +_NO_CLIENT_CERT = NO_CLIENT_CERT +_HOST_CREDENTIAL_ENV_LOCK = HOST_CREDENTIAL_ENV_LOCK # The mysql_scanner quoting lives beside the libpq one in _sql_safety, which @@ -341,35 +321,7 @@ def _uri_scheme(uri: str) -> str: ) -@contextlib.contextmanager -def _host_credential_env_suppressed() -> Iterator[None]: - """Hide this host's ambient DB credential env vars for a pinned ATTACH. - - The saved values are put back when the window closes, also when the attach - raises. Windows opened through this function in different threads are - serialised by :data:`_HOST_CREDENTIAL_ENV_LOCK`, so they never interleave - their save and restore; within one thread a nested window is safe when it - closes before the window around it. - - That is all the lock covers. Code that does not go through this function - is not serialised with it, so there is no guarantee against: - - * another thread reading these variables, which sees them unset while a - window is open; - * ``os.environ.copy()`` in another thread, which can race the pop and the - restore (and raise ``KeyError``); - * an ``engine._run_sync_stage(env_snapshot=True)`` whose snapshot and - restore overlap a window, which can delete the variables for good or - bring them back inside the window; - * same-thread windows that do not close in LIFO order, where the first to - close restores the variables inside the other. - """ - with _HOST_CREDENTIAL_ENV_LOCK: - saved = {k: os.environ.pop(k) for k in _HOST_CREDENTIAL_ENV if k in os.environ} - try: - yield - finally: - os.environ.update(saved) +_host_credential_env_suppressed = host_credential_env_suppressed @dataclass diff --git a/fluid_build/credentials/host_db_env.py b/fluid_build/credentials/host_db_env.py new file mode 100644 index 000000000..500d1b426 --- /dev/null +++ b/fluid_build/credentials/host_db_env.py @@ -0,0 +1,107 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""This host's ambient database credentials, hidden for the length of one attach. + +A contract names the host its source lives on. libpq and mysql_scanner then read +credentials from the PROCESS environment for whatever host they are handed, so a +contract naming a host of its own choosing could be authenticated with this +machine's ``PGPASSWORD`` -- a credential the contract was never given, belonging +to a different server. No allowlist over the contract's own ``secretRef`` closes +that, because the credential never passes through ``secretRef`` at all. + +``fluid init --discover`` has suppressed these since its JDBC introspection +landed. The acquisition runners had not, so the same contract fields reached the +same client libraries with the environment intact. + +This is where the one implementation lives so both use it. It sits under +``credentials`` rather than beside the discover code because ``build_runners`` +may not import ``cli`` (the import-linter contract "build_runners must not +depend on cli"), and because hiding a credential is a credentials concern rather +than a discover detail. ``cli/discover/_jdbc_introspect`` re-exports these under +their original private spellings, so the tests and patch points that grew up +there keep working unchanged. +""" + +from __future__ import annotations + +import contextlib +import os +import threading +from typing import Iterator + +# mysql_scanner was seen to send no password from an option file (~/.my.cnf, +# ~/.mylogin.cnf, ~/my.cnf, /etc/my.cnf, /etc/mysql/my.cnf, $MYSQL_HOME/my.cnf), +# with or without that suffix, so clearing it is a precaution: there is no +# option-file fallback for it to close. +HOST_CREDENTIAL_ENV = ( + "PGPASSWORD", + "PGPASSFILE", + "PGSERVICE", + "PGSERVICEFILE", + "PGSYSCONFDIR", + "PGSSLCERT", + "PGSSLKEY", + "PGOAUTHTOKEN", + "PGGSSDELEGATION", + "MYSQL_PWD", + "MYSQL_HOST", + "MYSQL_TCP_PORT", + "MYSQL_UNIX_PORT", + "MYSQL_GROUP_SUFFIX", +) + +# libpq skips a client certificate whose file does not exist (ENOENT or +# ENOTDIR). Pointing a pinned connection's certificate and key below os.devnull +# means it never presents this host's ~/.postgresql/postgresql.crt to a server +# that asks for one. +NO_CLIENT_CERT = os.path.join(os.devnull, "fluid-no-client-certificate") + +# The suppression below mutates the process environment. forge_run's in-process +# worker thread can run alongside forge_from_source, so windows are serialised +# (and re-entrant within one thread) rather than interleaved: two overlapping +# windows could otherwise restore the host credentials while the second attach +# is still inside its own. +HOST_CREDENTIAL_ENV_LOCK = threading.RLock() + + +@contextlib.contextmanager +def host_credential_env_suppressed() -> Iterator[None]: + """Hide this host's ambient DB credential env vars for a pinned ATTACH. + + The saved values are put back when the window closes, also when the attach + raises. Windows opened through this function in different threads are + serialised by :data:`HOST_CREDENTIAL_ENV_LOCK`, so they never interleave + their save and restore; within one thread a nested window is safe when it + closes before the window around it. + + That is all the lock covers. Code that does not go through this function + is not serialised with it, so there is no guarantee against: + + * another thread reading these variables, which sees them unset while a + window is open; + * ``os.environ.copy()`` in another thread, which can race the pop and the + restore (and raise ``KeyError``); + * an ``engine._run_sync_stage(env_snapshot=True)`` whose snapshot and + restore overlap a window, which can delete the variables for good or + bring them back inside the window; + * same-thread windows that do not close in LIFO order, where the first to + close restores the variables inside the other. + """ + with HOST_CREDENTIAL_ENV_LOCK: + saved = {k: os.environ.pop(k) for k in HOST_CREDENTIAL_ENV if k in os.environ} + try: + yield + finally: + os.environ.update(saved) diff --git a/tests/build_runners/test_duckdb_host_credential_env.py b/tests/build_runners/test_duckdb_host_credential_env.py new file mode 100644 index 000000000..925e4388e --- /dev/null +++ b/tests/build_runners/test_duckdb_host_credential_env.py @@ -0,0 +1,164 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""A contract's source host never gets this machine's ambient DB credentials. + +A contract names the host its postgres or mysql source lives on. libpq and +mysql_scanner then read credentials from the PROCESS environment for whatever +host they are handed, so a contract naming a host of its own choosing could be +authenticated with this machine's ``PGPASSWORD`` -- a credential it was never +given, belonging to a different server. + +This is a SECOND route, independent of the contract's own ``connection.secretRef``: +no allowlist over ``secretRef`` closes it, because the credential never passes +through ``secretRef`` at all. ``fluid init --discover`` has suppressed these +since its JDBC introspection landed; the acquisition runners had not. +""" + +from __future__ import annotations + +import os +from typing import Any, Dict, List + +import pytest + +from fluid_build.credentials.host_db_env import ( + HOST_CREDENTIAL_ENV, + host_credential_env_suppressed, +) + +#: A value no real credential store would hand out, so a leak is unmistakable. +SENTINEL = "SENTINEL-HOST-PASSWORD-DO-NOT-SEND" # pragma: allowlist secret + + +@pytest.mark.parametrize("name", ["PGPASSWORD", "PGPASSFILE", "PGSERVICE", "MYSQL_PWD"]) +def test_the_variables_libpq_and_mysql_read_are_covered(name: str) -> None: + """The set is the contract: a variable missing from it is a live hole.""" + assert name in HOST_CREDENTIAL_ENV, f"{name} is not suppressed" + + +def test_the_window_hides_and_restores(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("PGPASSWORD", SENTINEL) + monkeypatch.setenv("MYSQL_PWD", SENTINEL) + with host_credential_env_suppressed(): + assert "PGPASSWORD" not in os.environ + assert "MYSQL_PWD" not in os.environ + assert os.environ["PGPASSWORD"] == SENTINEL + assert os.environ["MYSQL_PWD"] == SENTINEL + + +def test_the_window_restores_when_the_attach_raises(monkeypatch: pytest.MonkeyPatch) -> None: + """A failed attach must not leave the operator's environment stripped.""" + monkeypatch.setenv("PGPASSWORD", SENTINEL) + with pytest.raises(RuntimeError, match="attach failed"): + with host_credential_env_suppressed(): + raise RuntimeError("attach failed") + assert os.environ["PGPASSWORD"] == SENTINEL + + +def test_an_unset_variable_is_not_invented(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("PGPASSWORD", raising=False) + with host_credential_env_suppressed(): + pass + assert "PGPASSWORD" not in os.environ + + +# ── the runner's attach, which is what this change is for ──────────────── + + +def test_the_duckdb_runner_attaches_inside_the_window( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The regression test. Reverting the `with` in the runner fails this. + + Observes the environment from inside the attach itself, which is the only + place that proves the window actually wraps the call that reaches libpq. + """ + from fluid_build.build_runners.duckdb import runner as duckdb_runner + + monkeypatch.setenv("PGPASSWORD", SENTINEL) + seen: Dict[str, Any] = {} + + def spy(_con: Any, statement: str, password: Any) -> None: + seen["pgpassword_visible_during_attach"] = os.environ.get("PGPASSWORD") + seen["statement"] = statement + seen["password"] = password + + monkeypatch.setattr(duckdb_runner, "_attach_with_password_masked", spy) + + class _Conn: + raw = { + "host": "db.example.com", + "port": 5432, + "user": "alice", + "database": "sales", + "password": "from-the-contract", # pragma: allowlist secret + } + + class _Source: + kind = "postgres" + connection = _Conn() + + class _Ctx: + build_id = "ingest" + source = _Source() + + class _Con: + def execute(self, _sql: str) -> None: + return None + + runner = duckdb_runner.DuckdbRunner() + runner._attach_external_databases(_Con(), _Ctx()) + + assert ( + seen["pgpassword_visible_during_attach"] is None + ), "the runner attached with this host's PGPASSWORD still in the environment" + # Restored afterwards, and the contract's own credential was untouched. + assert os.environ["PGPASSWORD"] == SENTINEL + assert seen["password"] == "from-the-contract" + + +def test_a_sqlite_source_needs_no_window(monkeypatch: pytest.MonkeyPatch) -> None: + """Only the libpq/mysql_scanner paths read these; sqlite takes a path. + + Pins that the change did not quietly widen to the sqlite branch, whose + confinement is a different control. + """ + from fluid_build.build_runners.duckdb import runner as duckdb_runner + + calls: List[str] = [] + + class _Con: + def execute(self, sql: str) -> None: + calls.append(sql) + + class _Conn: + raw: Dict[str, Any] = {"uri": "/nonexistent/does-not-exist.sqlite"} + + class _Source: + kind = "sqlite" + connection = _Conn() + + class _Ctx: + build_id = "ingest" + source = _Source() + contract: Dict[str, Any] = {} + workdir = "." + + runner = duckdb_runner.DuckdbRunner() + # The path is confined before any attach, so an outside path is refused + # here; what matters is that it does not reach the credential window. + with pytest.raises(Exception): + runner._attach_external_databases(_Con(), _Ctx()) + assert not any("ATTACH" in c for c in calls), calls From 17d2c9a33cb62d09e06275877bcb4d6e8fe4bd2b Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:41:41 +0100 Subject: [PATCH 2/4] test: name the contract's password, and make one control flow explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on #750, both in the new test file. The Lint & Format failure was its `Secret scan (detect-secrets on changed files)` step, not ruff or black: `assert seen["password"] == "from-the-contract"` read as a Secret Keyword. The dict entry it compares against carried a pragma but the assertion did not. Naming the value once as CONTRACT_PASSWORD removes the literal from both places, which beats sprinkling pragmas — and it says what the value IS: what the contract declared, as opposed to what the host environment holds, which is the distinction the whole test is about. CodeQL read the `raise` inside nested `pytest.raises` as making the rest of the block unreachable. Rewritten as try/except/else, which states the same intention without the warning. Not done: the two comments asking to delete the `_HOST_CREDENTIAL_ENV` and `_HOST_CREDENTIAL_ENV_LOCK` aliases as unused. They are the compatibility shim this PR exists to provide, and the first is used directly by tests/cli/discover/test_jdbc_attach_safety.py:480 and :487 — deleting it breaks 202 passing tests. The bot sees one file at a time and cannot see that use. Verified after the change: detect-secrets flags 0 in the file and the hook exits 0 over the changed-file list; 9 tests pass; reverting the `with` in the runner still fails the regression test, so the guard did not become tautological. --- .../test_duckdb_host_credential_env.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/tests/build_runners/test_duckdb_host_credential_env.py b/tests/build_runners/test_duckdb_host_credential_env.py index 925e4388e..8ce205608 100644 --- a/tests/build_runners/test_duckdb_host_credential_env.py +++ b/tests/build_runners/test_duckdb_host_credential_env.py @@ -41,6 +41,11 @@ #: A value no real credential store would hand out, so a leak is unmistakable. SENTINEL = "SENTINEL-HOST-PASSWORD-DO-NOT-SEND" # pragma: allowlist secret +#: What the CONTRACT declared, as opposed to what the host environment holds. +#: Named once so neither the fixture nor the assertion spells a literal that +#: reads as a credential to the secret scanner. +CONTRACT_PASSWORD = "from-the-contract" # pragma: allowlist secret + @pytest.mark.parametrize("name", ["PGPASSWORD", "PGPASSFILE", "PGSERVICE", "MYSQL_PWD"]) def test_the_variables_libpq_and_mysql_read_are_covered(name: str) -> None: @@ -61,9 +66,16 @@ def test_the_window_hides_and_restores(monkeypatch: pytest.MonkeyPatch) -> None: def test_the_window_restores_when_the_attach_raises(monkeypatch: pytest.MonkeyPatch) -> None: """A failed attach must not leave the operator's environment stripped.""" monkeypatch.setenv("PGPASSWORD", SENTINEL) - with pytest.raises(RuntimeError, match="attach failed"): + # Written as try/except/else rather than nested `pytest.raises`: CodeQL reads + # the inner `raise` as making the rest of the block unreachable, and the + # explicit form says the same thing without the warning. + try: with host_credential_env_suppressed(): raise RuntimeError("attach failed") + except RuntimeError as exc: + assert "attach failed" in str(exc) + else: # pragma: no cover - the raise above always fires + pytest.fail("the window swallowed the exception") assert os.environ["PGPASSWORD"] == SENTINEL @@ -103,7 +115,7 @@ class _Conn: "port": 5432, "user": "alice", "database": "sales", - "password": "from-the-contract", # pragma: allowlist secret + "password": CONTRACT_PASSWORD, } class _Source: @@ -126,7 +138,7 @@ def execute(self, _sql: str) -> None: ), "the runner attached with this host's PGPASSWORD still in the environment" # Restored afterwards, and the contract's own credential was untouched. assert os.environ["PGPASSWORD"] == SENTINEL - assert seen["password"] == "from-the-contract" + assert seen["password"] == CONTRACT_PASSWORD def test_a_sqlite_source_needs_no_window(monkeypatch: pytest.MonkeyPatch) -> None: From f3185765a999411bb09d6854deada609d6a959fe Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:42:38 +0100 Subject: [PATCH 3/4] test: drop the one alias nothing refers to CodeQL was right about `_HOST_CREDENTIAL_ENV_LOCK`: checked each alias individually and it has 0 references, while `_HOST_CREDENTIAL_ENV` has 2 (its own tests) and `_NO_CLIENT_CERT` has 2 (build_attach_dsn in the same module). The lock stays reachable as `host_db_env.HOST_CREDENTIAL_ENV_LOCK` if anything ever needs it, and the comment now says which names are kept and why. --- fluid_build/cli/discover/_jdbc_introspect.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fluid_build/cli/discover/_jdbc_introspect.py b/fluid_build/cli/discover/_jdbc_introspect.py index f46770d95..39d04be79 100644 --- a/fluid_build/cli/discover/_jdbc_introspect.py +++ b/fluid_build/cli/discover/_jdbc_introspect.py @@ -80,7 +80,6 @@ from fluid_build.credentials.host_db_env import ( HOST_CREDENTIAL_ENV, - HOST_CREDENTIAL_ENV_LOCK, NO_CLIENT_CERT, host_credential_env_suppressed, ) @@ -125,9 +124,12 @@ # acquisition runners can use it too -- ``build_runners`` may not import # ``cli``. These aliases keep this module's original private names, which its # own tests and the patch points around them refer to. +# Only the names something actually refers to: _HOST_CREDENTIAL_ENV from this +# module's own tests, _NO_CLIENT_CERT from build_attach_dsn below. The lock +# needs no alias -- nothing outside host_db_env touches it, and it is reachable +# there as HOST_CREDENTIAL_ENV_LOCK if that ever changes. _HOST_CREDENTIAL_ENV = HOST_CREDENTIAL_ENV _NO_CLIENT_CERT = NO_CLIENT_CERT -_HOST_CREDENTIAL_ENV_LOCK = HOST_CREDENTIAL_ENV_LOCK # The mysql_scanner quoting lives beside the libpq one in _sql_safety, which From e76bc6444c659ea1bcbabd5e4996cbf5ba00d3e3 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:48:42 +0100 Subject: [PATCH 4/4] test: keep the swallow guard without the branch CodeQL calls dead CodeQL flagged the `else: pytest.fail(...)` as unreachable, and statically it is -- the `raise` is unconditional. But it is reachable in the one case the test exists for: a context manager whose `__exit__` returns true SWALLOWS the exception, and CodeQL does not model that. Dropping the branch as suggested would leave a swallowing window passing this test with nothing asserted. Replaced with a `propagated` flag asserted after the `try`, which keeps the guard and gives the analyser nothing unreachable. Verified against a deliberately swallowing context manager: the flag stays false, so the assertion fails -- the guard still works. --- .../test_duckdb_host_credential_env.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/build_runners/test_duckdb_host_credential_env.py b/tests/build_runners/test_duckdb_host_credential_env.py index 8ce205608..3fc21ff09 100644 --- a/tests/build_runners/test_duckdb_host_credential_env.py +++ b/tests/build_runners/test_duckdb_host_credential_env.py @@ -66,16 +66,19 @@ def test_the_window_hides_and_restores(monkeypatch: pytest.MonkeyPatch) -> None: def test_the_window_restores_when_the_attach_raises(monkeypatch: pytest.MonkeyPatch) -> None: """A failed attach must not leave the operator's environment stripped.""" monkeypatch.setenv("PGPASSWORD", SENTINEL) - # Written as try/except/else rather than nested `pytest.raises`: CodeQL reads - # the inner `raise` as making the rest of the block unreachable, and the - # explicit form says the same thing without the warning. + # A flag rather than `else:` or a nested `pytest.raises`. Both of those read + # to CodeQL as unreachable code, because its analysis does not model a + # context manager's __exit__ returning true -- which is precisely the + # failure this checks for: a window that SWALLOWED the exception would + # otherwise leave the test passing with nothing asserted. + propagated = False try: with host_credential_env_suppressed(): raise RuntimeError("attach failed") except RuntimeError as exc: + propagated = True assert "attach failed" in str(exc) - else: # pragma: no cover - the raise above always fires - pytest.fail("the window swallowed the exception") + assert propagated, "the window swallowed the exception instead of re-raising" assert os.environ["PGPASSWORD"] == SENTINEL