From f1b8c7db5886e14760a0c9a9c32f9772a25d06e8 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Tue, 27 Oct 2026 17:04:47 +0100 Subject: [PATCH 1/2] security: a declared remote prefix handed a local directory to contract SQL A contract that declares a remote prefix whose text also names a local path got that directory granted to its SQL. DuckDB resolves every `allowed_directories` entry against the file system, and a link in the working directory -- which the contract's own repository supplies -- is enough: with `s3: -> /` present, `DuckDBAllowlist.none().with_remote("s3://etc/")` reported `/private/etc/` in `allowed_directories` and `read_text('/etc/hosts')` returned a row, from an allowlist that granted no directory at all. Reproduced on duckdb 1.5.1, 1.5.6 and 2.0.0, and reachable from a real call site: `providers/local/local.py` hands a raw `s3://` URI to `with_declared`, and `cli/discover/filesystem.py` hands one to `with_locations`. The read-back added in #743 could not catch it. Its accommodation for DuckDB 1.5 -- which reports `s3://bucket/landing/` as `/s3:/bucket/landing/` -- was written as `realpath(prefix)`, and that follows symlinks inside the prefix, so the escape's own resolution entered the granted set and excused the grant. Two measurements decided the shape of the fix, each killing an earlier plan: * Remote prefixes are NOT inert, so they cannot simply be left out of `allowed_directories`. With `httpfs` loaded before the lock, remote reads work after it despite `enable_external_access = false`, and the prefix is what bounds them: against the public `s3://noaa-ghcn-pds/` bucket on 1.5.1 and 1.5.6, granting no prefix refuses, granting a different bucket refuses, granting the target's own prefix reads. Dropping the entry would have broken legitimate remote reads. * Existence is the wrong test. DuckDB grants a resolved path that does not exist yet -- `s3://staging-area/` with `s3: -> /` grants `/staging-area` -- and contract SQL can write into it. What actually separates the honest case from the escape is symlink resolution: the form DuckDB 1.5 reports is the LEXICAL join of cwd and the prefix, and an escaping prefix differs from it only because its components follow a link. `_escaping_remote_prefix` compares the two and returns the directory DuckDB would grant. `_refuse_escaping_remote_prefixes` runs BEFORE `SET allowed_directories`, so the connection never holds the grant, and `_refuse_ungranted_dirs` keeps catching it as a backstop now that its accommodation is the lexical form. Tests pin each barrier with the other bypassed, and one test witnesses the defect with both bypassed so the file proves what it guards. Also here, because the live run through `fluid init --discover` surfaced it: a sandbox refusal reached the discover handler's `except Exception` and was reframed as `ConnectivityProbeError`, whose fix reads "Check VPN / network policy / credentials" -- advice that sends the reader to their network for a link in their working directory. A typed error now keeps its own what/why/fix, still masked, because the refusal names the location it refused and that can carry userinfo; verified with `https://alice:@example.com/data.csv`, which renders `***REDACTED***` and leaves no copy of the secret. An awkward `FluidUserError` subclass that `replace` cannot rebuild falls back to the old framing rather than raising a second error while handling the first. The prefixes live in `why` rather than `what` because the renderer uses `what` as the panel title and truncates it to one line, which was cutting off the detail the message exists to give. Validation. 8 new tests pass on 1.5.1, 1.5.6 and 2.0.0; 6 of the 8 fail when the production change is reverted, on 1.5.6 and 2.0.0 alike (the other 2 are the legitimate-prefix guard and the defect witness, which must not). Full suite CI-shaped on both floors: 27853 and 27851 passed, 4 failed -- the same 4 that fail on clean `7cf5471a`, verified in a detached base worktree. Perf run alone 38 passed / 7 xfailed on both. Live: `fluid init --discover s3://noaa-ghcn-pds/csv/by_year/2024.csv` emits a contract from a clean directory (8 columns, exit 0) and is refused with exit 1 and no contract written from one holding `s3: -> /`. ruff, black==24.10.0, license headers, import-linter (6 kept / 0 broken) and the startup budget all clean. --- fluid_build/cli/_init_discover_handler.py | 60 +++- fluid_build/providers/_duckdb_sandbox.py | 80 +++++- .../discover/test_init_discover_redaction.py | 47 ++- .../test_duckdb_sandbox_remote_prefix.py | 267 ++++++++++++++++++ 4 files changed, 433 insertions(+), 21 deletions(-) create mode 100644 tests/providers/test_duckdb_sandbox_remote_prefix.py diff --git a/fluid_build/cli/_init_discover_handler.py b/fluid_build/cli/_init_discover_handler.py index b275781a4..8fd38f6f2 100644 --- a/fluid_build/cli/_init_discover_handler.py +++ b/fluid_build/cli/_init_discover_handler.py @@ -43,6 +43,7 @@ from fluid_build.cli._errors import ( ConnectivityProbeError, + FluidUserError, SchemaValidationError, ) from fluid_build.cli.console import cprint @@ -164,7 +165,7 @@ def run_discover(args, logger: logging.Logger, *, uri: str) -> int: ) cprint(f"🔍 Discovering streams at {safe_uri}…") - failure: Optional[ConnectivityProbeError] = None + failure: Optional[FluidUserError] = None try: # The operator typed this URI on the command line, so a JDBC # discoverer may use this host's client certificate and libpq @@ -232,22 +233,61 @@ def run_discover(args, logger: logging.Logger, *, uri: str) -> int: return 0 -def _probe_error(exc: Exception, uri: str, safe_uri: str, view: _UriView) -> ConnectivityProbeError: +def _probe_error(exc: Exception, uri: str, safe_uri: str, view: _UriView) -> FluidUserError: """The typed error for a discovery that failed with ``exc``, masked throughout. - ``exc`` is the driver's own error, which quotes the URI or the DSN as given, - in its message, its arguments, its notes or the errors chained to it. Only - text is kept, masked as the display masks ``uri``: its message and those of - the errors that explain it (:func:`_reason`). The cause is a stand-in naming - the driver's exception type with that masked text, and carries no - traceback, notes or chain of its own. + ``exc`` is usually the driver's own error, which quotes the URI or the DSN + as given, in its message, its arguments, its notes or the errors chained to + it. Only text is kept, masked as the display masks ``uri``: its message and + those of the errors that explain it (:func:`_reason`). The cause is a + stand-in naming the driver's exception type with that masked text, and + carries no traceback, notes or chain of its own. + + An error that already says what to do keeps saying it. A refusal from the + DuckDB sandbox reaches here through the same ``except Exception``, and + reframing it as a connectivity failure replaced its own ``fix`` with "Check + VPN / network policy / credentials" -- advice that sends the reader to their + network for something in their working directory. Its fields are kept and + masked instead, since the refusal names the location it refused and that + can carry userinfo (``https://user:pw@host/``), which is what the masking + here is for. """ - reason = _reason(exc, lambda text: _masked(text, uri, view)) - error = ConnectivityProbeError.for_target(target=safe_uri, reason=reason) + + def mask(text: str) -> str: + return _masked(text, uri, view) + + reason = _reason(exc, mask) + error = _kept_or_reframed(exc, mask, safe_uri, reason) error.__cause__ = RuntimeError(f"{_type_name(exc)}: {reason}") return error +def _kept_or_reframed( + exc: Exception, mask: Callable[[str], str], safe_uri: str, reason: str +) -> FluidUserError: + """``exc`` with its own fields masked when it is already a typed error. + + ``replace`` needs the concrete class to be constructible from its fields, + which every typed error in the tree is. A discoverer from outside the tree + could raise a ``FluidUserError`` subclass that is not, and this code runs + while an error is already being handled -- the last place to raise a second + one -- so an awkward subclass falls back to the connectivity framing. + """ + if isinstance(exc, FluidUserError): + try: + return replace( + exc, + what=mask(exc.what), + why=mask(exc.why), + fix=mask(exc.fix), + where=mask(exc.where) if exc.where else exc.where, + extras={**exc.extras, "uri": safe_uri}, + ) + except (TypeError, ValueError): + pass + return ConnectivityProbeError.for_target(target=safe_uri, reason=reason) + + def _type_name(exc: BaseException) -> str: """``exc``'s type as a traceback names it: the module is left out for a builtin.""" kind = type(exc) diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index 3519c92de..62a10ab98 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -950,6 +950,64 @@ def _own_spill_dir() -> str: return tempfile.mkdtemp(prefix=SPILL_DIR_PREFIX) +def _escaping_remote_prefix(prefix: str, cwd: str) -> Optional[str]: + """The local directory ``prefix`` would grant, or ``None`` when it grants none. + + DuckDB resolves every ``allowed_directories`` entry against the file system. + A remote prefix has no business naming a local path, but its text can: with + ``s3: -> /`` in the working directory, ``s3://etc/`` resolves to ``/etc``. + + The honest form is the lexical one -- DuckDB 1.5 reports + ``s3://bucket/landing/`` as ``/s3:/bucket/landing/``, which is what + joining and normalising gives. Resolving symlinks is what differs, so a + prefix whose resolution leaves that form is one whose components follow a + link, and the directory it lands on is what DuckDB will grant. Existence is + deliberately not the test: DuckDB grants a resolved path that is not there + yet, and contract SQL can write into it. + """ + joined = os.path.join(cwd, prefix) + lexical = _fold(os.path.normpath(joined)) + resolved = os.path.realpath(joined) + return resolved if _fold(resolved) != lexical else None + + +def _refuse_escaping_remote_prefixes(allow: DuckDBAllowlist) -> None: + """Raise before the grant is set, rather than reporting it afterwards. + + :func:`_refuse_ungranted_dirs` would catch this too, and keeps doing so as + the backstop. Refusing here means the connection never holds the grant at + all, so the barrier does not rest on a single read-back. + """ + cwd = os.path.realpath(os.getcwd()) + escaped = [ + (prefix, landed) + for prefix in allow.remote_prefixes + if (landed := _escaping_remote_prefix(prefix, cwd)) is not None + ] + if escaped: + # ``what`` is the panel's TITLE, which the renderer truncates to one line, + # so the prefixes go in ``why``: naming them is the whole point of the + # message and a cut-off title would hide it. + raise _sandbox_error( + what="a declared remote prefix would grant a local directory to the SQL", + why=( + "DuckDB resolves every allowed_directories entry against the file " + "system, so a prefix whose text also names a local path becomes a local " + "grant, and a link in the working directory is enough. Refused here: " + + "; ".join(f"{p} resolves to {landed}" for p, landed in escaped) + + ". A remote prefix must bound remote reads and grant no local " + "directory." + ), + fix=( + "Remove the link (or the directory) in the working directory whose name " + "matches the prefix's scheme, or declare the location by a prefix whose " + "text does not resolve locally. If the local directory is genuinely " + "needed, declare it as a directory so it is granted deliberately rather " + "than as a side effect of a URL." + ), + ) + + def _refuse_ungranted_dirs(con: Any, allow: DuckDBAllowlist, spill: str) -> None: """Refuse the connection if DuckDB granted a directory ``allow`` does not name. @@ -964,15 +1022,19 @@ def _refuse_ungranted_dirs(con: Any, allow: DuckDBAllowlist, spill: str) -> None Both sides are compared after ``realpath`` and, where the file system ignores case, after casefolding, because that is how DuckDB stores them: two entries that resolve to one directory are reported once (a grant of - both ``link`` and its target ``real`` reports only ``real/``). A remote - prefix is reported as it was given on 2.0, but DuckDB 1.5 resolves it as a - relative local path (``s3://bucket/landing/`` comes back as - ``/s3:/bucket/landing/``, which :func:`os.path.realpath` reproduces), - so both forms count as granted. + both ``link`` and its target ``real`` reports only ``real/``). + + A remote prefix is accommodated by its LEXICAL form, which is what DuckDB + 1.5 reports (``s3://bucket/landing/`` comes back as + ``/s3:/bucket/landing/``; 2.0 reports the URL itself). Writing it as + ``realpath(prefix)`` followed symlinks inside the prefix and so excused the + escape :func:`_escaping_remote_prefix` refuses. The cwd itself is resolved, + since DuckDB reports the resolved form. """ reported = tuple(con.execute("SELECT current_setting('allowed_directories')").fetchall()[0][0]) + here = os.path.realpath(os.getcwd()) granted = {_fold(os.path.realpath(d)) for d in (*allow.dirs, spill)} - granted |= {_fold(os.path.realpath(p)) for p in allow.remote_prefixes} + granted |= {_fold(os.path.normpath(os.path.join(here, p))) for p in allow.remote_prefixes} extra = [ entry for entry in reported @@ -980,6 +1042,11 @@ def _refuse_ungranted_dirs(con: Any, allow: DuckDBAllowlist, spill: str) -> None if (is_remote_location(entry) or _fold(os.path.realpath(entry)) not in granted) ] if extra: + # An escaping prefix is refused before the SET, so reaching it here means + # that check was bypassed. Defer to its message: this grant IS something + # a contract can cause, and the advice below would send the reader off to + # report a sandbox bug they caused themselves. + _refuse_escaping_remote_prefixes(allow) raise _sandbox_error( what=( "DuckDB granted directories this connection did not ask for: " @@ -1121,6 +1188,7 @@ def _confine_connection( # duckdb/duckdb#26064: the allowlist check expands '~' with $HOME and # the open with this setting. Equal and locked, they cannot disagree. con.execute(f"SET home_directory = {quote_ansi_string_literal(home)}") + _refuse_escaping_remote_prefixes(allow) directories = [_with_sep(d) for d in allow.dirs] + list(allow.remote_prefixes) con.execute(f"SET allowed_directories = {_sql_list(directories)}") con.execute(f"SET allowed_paths = {_sql_list(allow.paths)}") diff --git a/tests/cli/discover/test_init_discover_redaction.py b/tests/cli/discover/test_init_discover_redaction.py index 74828ccfa..0057cc8cd 100644 --- a/tests/cli/discover/test_init_discover_redaction.py +++ b/tests/cli/discover/test_init_discover_redaction.py @@ -43,6 +43,7 @@ from fluid_build.cli._errors import ConnectivityProbeError, SchemaValidationError from fluid_build.cli.discover.registry import DiscoveredColumn, DiscoveredStream from fluid_build.observability.secret_redactor import forget_known_secrets, redact_secret_text +from fluid_build.providers._duckdb_sandbox import DuckDBSandboxError from fluid_build.providers._sql_safety import libpq_escape, quote_ansi_string_literal from fluid_build.schema_manager import FluidSchemaManager @@ -257,6 +258,40 @@ def test_error_quoting_the_uri_masks_a_short_password_with_raw_at_sign( assert "cd@127.0.0.1" not in info.value.as_json() +class TestTypedErrorsKeepTheirOwnAdvice: + """An error that already says what to do is not reframed as a connectivity failure.""" + + def test_a_typed_error_keeps_its_own_advice_and_is_still_masked(self, monkeypatch, tmp_path): + """A sandbox refusal reaches the same ``except Exception`` as a driver error. + + Reframing it as a connectivity failure replaced its ``fix`` with "Check + VPN / network policy / credentials", which sends the reader to their + network for something in their working directory. Its own fields are + kept -- and still masked, because the refusal names the location it + refused and that location can carry userinfo. + """ + uri = f"https://alice:{SENTINEL}@example.com/data.csv" + stub = _StubDiscoverer( + error=DuckDBSandboxError( + what="a declared remote prefix would grant a local directory to the SQL", + why=f"Refused here: {uri} resolves to /somewhere", + fix="Remove the link in the working directory.", + doc="https://example.invalid/doc", + ) + ) + with pytest.raises(DuckDBSandboxError) as info: + _run(monkeypatch, tmp_path, uri, stub) + exc = info.value + assert exc.code == "DuckDBSandboxError" + assert "working directory" in exc.fix + assert "VPN" not in exc.fix + assert SENTINEL not in exc.what + assert SENTINEL not in (exc.why or "") + assert SENTINEL not in exc.render(color=False) + assert SENTINEL not in exc.as_json() + assert _contracts(tmp_path) == [] + + class TestPasswordWithUnencodedDelimiter: """A password holding a raw ``/``, ``?`` or ``#`` ends the RFC 3986 authority early. @@ -1052,12 +1087,14 @@ def test_sandbox_refusal_quoting_the_url_with_repr(self, monkeypatch, tmp_path): # Holding both quote characters, repr() escapes the single one. uri = "https://alice:pa'ss\"WORDTAIL@h.invalid/../a.csv" # pragma: allowlist secret monkeypatch.setenv("HOME", str(tmp_path / "home")) - with pytest.raises(ConnectivityProbeError) as info: + # A sandbox refusal keeps its own type and advice rather than being + # reframed as a connectivity failure, and is masked either way. + with pytest.raises(DuckDBSandboxError) as info: handler.run_discover(_args(tmp_path), logging.getLogger("t"), uri=uri) exc = info.value - why = exc.why or "" - assert "WORDTAIL" not in exc.render(color=False) + exc.as_json(), why - assert "remote prefix 'https://***REDACTED***@h.invalid/../' contains '..'" in why + rendered = exc.render(color=False) + exc.as_json() + assert "WORDTAIL" not in rendered, exc.why + assert "remote prefix 'https://***REDACTED***@h.invalid/../' contains '..'" in rendered @pytest.mark.parametrize("odd", ["\udc80", "\ud800"]) def test_a_value_that_is_not_utf8_is_still_masked(self, odd): @@ -1177,7 +1214,7 @@ def test_real_duckdb_error_is_not_kept_on_the_chain(self, monkeypatch, tmp_path) # The sandbox refuses this before any network access, quoting the URL. uri = "https://alice:pa'ss\"WORDTAIL@h.invalid/../a.csv" # pragma: allowlist secret monkeypatch.setenv("HOME", str(tmp_path / "home")) - with pytest.raises(ConnectivityProbeError) as info: + with pytest.raises(DuckDBSandboxError) as info: handler.run_discover(_args(tmp_path), logging.getLogger("t"), uri=uri) text = _chain_text(info.value) assert "WORDTAIL" not in text, text diff --git a/tests/providers/test_duckdb_sandbox_remote_prefix.py b/tests/providers/test_duckdb_sandbox_remote_prefix.py new file mode 100644 index 000000000..6c0198abe --- /dev/null +++ b/tests/providers/test_duckdb_sandbox_remote_prefix.py @@ -0,0 +1,267 @@ +# 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 declared remote prefix bounds remote reads and grants no local directory. + +DuckDB resolves every ``allowed_directories`` entry against the file system, so +a prefix whose text also names a local path becomes a local grant. A link in the +working directory -- which the contract's own repository supplies -- is enough: +with ``s3: -> /`` present, ``s3://etc/`` granted ``/etc`` to contract SQL while +``allow.dirs`` was EMPTY. Reproduced on duckdb 1.5.1, 1.5.6 and 2.0.0 +(Trello 1209), and reachable from a real call site: ``providers/local/local.py`` +hands a raw ``s3://`` URI to :meth:`DuckDBAllowlist.with_declared`. + +The read-back added in #743 could not see it. Its accommodation for DuckDB 1.5 -- +which reports ``s3://bucket/landing/`` as ``/s3:/bucket/landing/`` -- was +written as ``realpath(prefix)``, and that **follows symlinks inside the prefix**, +so the escape's own resolution entered the granted set and excused the grant. +The prefix is now joined to the resolved cwd lexically, so the honest form still +matches and an escaping one no longer does. + +6 of the 8 tests here fail when the production change is reverted, on duckdb +1.5.6 and 2.0.0 alike. The 2 that do not are deliberate: the legitimate-prefix +guard has to pass before and after, and the defect witness passes on pre-fix +code by construction. + +There are two barriers, and the tests pin each one alone: the grant is refused +before it is set, and the read-back under the lock refuses it again if that +check is ever bypassed. ``test_with_both_barriers_bypassed_the_grant_is_real`` +witnesses the defect itself, so this file proves what it guards rather than +only asserting the guard runs. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +import pytest + +duckdb = pytest.importorskip("duckdb") + +from fluid_build.providers._duckdb_sandbox import ( # noqa: E402 + SPILL_DIR_PREFIX, + DuckDBAllowlist, + DuckDBSandboxError, + secure_duckdb_connect, +) + +# The probes and the ungranted-file constant are the sibling suite's, not copies +# of them: the two files ask DuckDB the same two questions, and a second pair +# that drifted would quietly weaken whichever file still held the old one. +from tests.providers.test_duckdb_sandbox_spill import ( # noqa: E402 + UNGRANTED_HOST_FILE, + _reads, + _setting, +) + +sandbox = sys.modules[DuckDBAllowlist.__module__] + +posix_symlink = pytest.mark.skipif(os.name == "nt", reason="POSIX symlink") + + +def _rendered(error: DuckDBSandboxError) -> str: + """Every field a reader sees, joined. + + ``str(error)`` is only ``what``, which the renderer uses as the panel title + and truncates to one line -- which is why the prefixes live in ``why``. + """ + return " ".join([str(error), error.why or "", error.fix or ""]) + + +def _repo_with_scheme_link(tmp_path: Path, target: str) -> Path: + """A contract repository shipping ``s3:`` as a link to ``target``.""" + repo = tmp_path / "contract-repo" + repo.mkdir() + (repo / "s3:").symlink_to(target) + return repo + + +# ── the defect ─────────────────────────────────────────────────────────── + + +@posix_symlink +def test_a_scheme_link_to_root_does_not_turn_a_prefix_into_a_local_grant( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """``s3://etc/`` with ``s3: -> /`` in cwd: the whole of /etc, from an empty allowlist. + + Before the fix this connected and ``read_text('/etc/passwd')`` returned a row. + """ + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + + with pytest.raises(DuckDBSandboxError) as excinfo: + secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_remote("s3://etc/")) + + rendered = _rendered(excinfo.value) + assert "s3://etc/" in rendered, rendered + assert os.path.realpath("/etc") in rendered, rendered + + +@posix_symlink +def test_the_escape_is_refused_even_when_the_target_does_not_exist( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A grant of a directory that is not there yet is still a grant. + + ``s3://staging-area/`` with ``s3: -> /`` resolves to ``/staging-area``, which + no host has. DuckDB grants it regardless, so contract SQL could COPY TO a + path outside everything the contract declared. Existence is the wrong test; + this is why the fix does not ask whether the resolved path is there. + """ + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + + with pytest.raises(DuckDBSandboxError) as excinfo: + secure_duckdb_connect( + ":memory:", allow=DuckDBAllowlist.none().with_remote("s3://staging-area/") + ) + + assert "s3://staging-area/" in _rendered(excinfo.value) + + +@posix_symlink +def test_a_scheme_link_inside_the_repository_is_refused_too( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """``s3: -> ./sub``: the grant stays under cwd, and is still not what was asked for. + + Deliberately strict. The declaration said "bound remote reads to this + bucket"; granting a local directory instead is a different thing whether or + not it escapes the repository, and a reader who sees the refusal can declare + the directory on purpose. + """ + repo = tmp_path / "contract-repo" + (repo / "sub").mkdir(parents=True) + (repo / "s3:").symlink_to(repo / "sub") + monkeypatch.chdir(repo) + + with pytest.raises(DuckDBSandboxError) as excinfo: + secure_duckdb_connect( + ":memory:", allow=DuckDBAllowlist.none().with_remote("s3://bucket/landing/") + ) + + assert "s3://bucket/landing/" in _rendered(excinfo.value) + + +@posix_symlink +def test_the_read_back_still_refuses_when_the_pre_set_check_is_bypassed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Two barriers, independently sufficient. This one is the backstop. + + With the pre-set refusal neutralised the grant does reach DuckDB, and the + read-back under the lock has to catch it -- which it only does because its + accommodation for the DuckDB 1.5 form stopped following symlinks. + """ + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + monkeypatch.setattr(sandbox, "_refuse_escaping_remote_prefixes", lambda *a, **k: None) + + with pytest.raises(DuckDBSandboxError) as excinfo: + secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_remote("s3://etc/")) + + assert "did not ask for" in str(excinfo.value), str(excinfo.value) + + +@posix_symlink +def test_the_pre_set_check_still_refuses_when_the_read_back_is_bypassed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The other direction: the grant is never set in the first place.""" + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + monkeypatch.setattr(sandbox, "_refuse_ungranted_dirs", lambda *a, **k: None) + + with pytest.raises(DuckDBSandboxError) as excinfo: + secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_remote("s3://etc/")) + + assert "s3://etc/" in _rendered(excinfo.value), _rendered(excinfo.value) + + +@posix_symlink +def test_with_both_barriers_bypassed_the_grant_is_real( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Witness the defect itself, so this file proves what it is guarding against. + + Neither barrier runs here, which is the pre-fix state: ``allowed_directories`` + holds the link's target and contract SQL reads the host out of an allowlist + that granted no directory at all. If a future change makes this test fail, + the escape has been closed further upstream and this file should say so. + """ + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + monkeypatch.setattr(sandbox, "_refuse_escaping_remote_prefixes", lambda *a, **k: None) + monkeypatch.setattr(sandbox, "_refuse_ungranted_dirs", lambda *a, **k: None) + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_remote("s3://etc/")) + try: + granted = list(_setting(con, "allowed_directories")) + local = [d for d in granted if SPILL_DIR_PREFIX not in d and "://" not in d] + assert local, f"expected the escape to be visible, got {granted}" + assert _reads(con, UNGRANTED_HOST_FILE) + finally: + con.close() + + +# ── what must keep working ─────────────────────────────────────────────── + + +def test_a_legitimate_remote_prefix_still_connects( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """No link in cwd: the shape every real call site uses. + + DuckDB 1.5 reports this as ``/s3:/bucket/landing/`` and 2.0 as the URL + itself; both are accepted, and neither grants anything that exists. The + bounding this entry provides is live network behaviour, verified out of band + against a public bucket on 1.5.1 and 1.5.6: ungranted and wrong-bucket reads + are refused, the granted prefix reads. + """ + monkeypatch.chdir(tmp_path) + + con = secure_duckdb_connect( + ":memory:", allow=DuckDBAllowlist.none().with_remote("s3://bucket/landing/") + ) + try: + granted = list(_setting(con, "allowed_directories")) + existing = [ + d for d in granted if SPILL_DIR_PREFIX not in d and "://" not in d and os.path.exists(d) + ] + assert existing == [], f"nothing local should exist in the grant, got {existing}" + assert not _reads(con, UNGRANTED_HOST_FILE) + finally: + con.close() + + +@posix_symlink +def test_the_refusal_does_not_blame_the_sandbox_for_a_contract_s_doing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The generic read-back message sends the reader to report a sandbox bug. + + That advice is right for a DuckDB bookkeeping surprise and wrong here: a + contract plus its repository is all it takes, so the message has to name the + prefix and what to change instead. + """ + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + + with pytest.raises(DuckDBSandboxError) as excinfo: + secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_remote("s3://etc/")) + + error = excinfo.value + rendered = _rendered(error) + assert "bug in the sandbox" not in rendered, rendered + assert "working directory" in rendered, rendered + assert "s3://etc/" in rendered, rendered + # The title is what a terminal truncates, so it must stand alone. + assert "s3://" not in str(error), str(error) From c404aa750b493e41083e18474c1db98e3db57400 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:11:42 +0100 Subject: [PATCH 2/2] fix(discover): the kept error masks every field as_json emits, not four of six MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A four-lens adversarial review of this branch raised 17 findings; 12 were refuted on inspection and 5 confirmed, all LOW. Two lenses found the same real one, and it is this branch's doing. `_kept_or_reframed` masked `what`, `why`, `fix` and `where` and copied `extras` and `doc` through verbatim. `FluidUserError.as_json` emits both. On `origin/main` the function always built a fresh `ConnectivityProbeError`, whose `extras` is empty, so no field of the raised error could survive — this branch created the first path that propagates a discoverer-supplied `extras` dict, and left the one dict-valued field out of the masking it applies in the same expression. A discoverer registered out of tree that carried the location under a key of its own (`dsn`, say; `uri` is overwritten here) would have had the credential serialised in full. Latent — no in-tree error sets `extras` on this path — but `as_json` is exactly what the sibling tests assert a credential never reaches, so the inconsistency is worth closing rather than documenting. `_masked_extras` walks nested dicts and lists, because `as_json` dumps whatever depth it is given. `doc` is masked too. The rebuild's `except` is now broad: a subclass can refuse to rebuild in more ways than `TypeError`, and this runs while an error is live, where raising a second one would attach the driver's error, URI and password as context. The review's other three confirmed findings were all about the tests, and all three were fair: * The read-back's deferral to the escape message was never exercised. In `test_the_read_back_still_refuses_when_the_pre_set_check_is_bypassed` the patched no-op IS the function the read-back defers to, so that test witnesses the generic "did not ask for" path. `test_the_read_back_defers_to_the_escape_message` drives `_refuse_ungranted_dirs` directly with the sibling suite's `_Reporting` stand-in so the real check runs inside it. * The masking test put the credential only in `why`, which made its `what` and `fix` assertions vacuous. It now puts the credential in what, why, fix, doc, where and a nested `extras`, asserts it is absent from each and from `as_json`, and asserts the host is still named so the value is masked rather than dropped. A second test covers the rebuild fallback with a subclass `replace` cannot reconstruct. * The file's docstring claimed "6 of 8 fail when the production change is reverted" without saying what was reverted. A literal revert deletes `_refuse_escaping_remote_prefixes`, so the tests that patch it error rather than fail. The docstring now says which revert was measured and what the literal one does. Each of the three new tests was mutation-checked and fails with its own mechanism removed: dropping the extras/doc masking fails the masking test, dropping the try/except fails the fallback test, dropping the deferral call fails the deferral test. Also settled: the two extra failures a contended run showed (`test_no_change_yields_no_decisions`, `test_fluid_validate_cli_passes_end_to_end`) are wall-clock, not logic — hypothesis's "Input generation is slow" health check and a 120s `subprocess.TimeoutExpired` on a `fluid validate` child, with 32 workers across two venvs competing. Re-run with a single venv, this branch shows exactly the 4 failures that clean `7cf5471a` shows under the same command. --- fluid_build/cli/_init_discover_handler.py | 39 ++++++++++-- .../discover/test_init_discover_redaction.py | 62 ++++++++++++++++--- .../test_duckdb_sandbox_remote_prefix.py | 38 ++++++++++-- 3 files changed, 121 insertions(+), 18 deletions(-) diff --git a/fluid_build/cli/_init_discover_handler.py b/fluid_build/cli/_init_discover_handler.py index 8fd38f6f2..bfd1f121b 100644 --- a/fluid_build/cli/_init_discover_handler.py +++ b/fluid_build/cli/_init_discover_handler.py @@ -249,8 +249,9 @@ def _probe_error(exc: Exception, uri: str, safe_uri: str, view: _UriView) -> Flu VPN / network policy / credentials" -- advice that sends the reader to their network for something in their working directory. Its fields are kept and masked instead, since the refusal names the location it refused and that - can carry userinfo (``https://user:pw@host/``), which is what the masking - here is for. + can carry a password in its userinfo, which is what the masking here is + for. (Spelling that URL shape out here trips the secret scanner, so it is + described rather than written.) """ def mask(text: str) -> str: @@ -267,11 +268,23 @@ def _kept_or_reframed( ) -> FluidUserError: """``exc`` with its own fields masked when it is already a typed error. + EVERY field :meth:`FluidUserError.as_json` emits is masked, ``extras`` and + ``doc`` included. Leaving ``extras`` alone would have been the one way a + value this handler masks everywhere else reached ``--json`` output: an + out-of-tree discoverer that put the location as given under a key of its + own (``dsn``, say) would have had it serialised verbatim, since ``"uri"`` + is overwritten here but no other key was touched. No error in the tree + carries extras through this path, so the gap was latent -- but ``as_json`` + is precisely what the sibling tests assert a credential never reaches. + ``replace`` needs the concrete class to be constructible from its fields, which every typed error in the tree is. A discoverer from outside the tree could raise a ``FluidUserError`` subclass that is not, and this code runs while an error is already being handled -- the last place to raise a second - one -- so an awkward subclass falls back to the connectivity framing. + one -- so anything awkward falls back to the connectivity framing. The + ``except`` is broad for that reason: a subclass can refuse to rebuild in + more ways than ``TypeError``, and none of them is worth losing the original + failure over. """ if isinstance(exc, FluidUserError): try: @@ -280,14 +293,30 @@ def _kept_or_reframed( what=mask(exc.what), why=mask(exc.why), fix=mask(exc.fix), + doc=mask(exc.doc) if exc.doc else exc.doc, where=mask(exc.where) if exc.where else exc.where, - extras={**exc.extras, "uri": safe_uri}, + extras={**_masked_extras(exc.extras, mask), "uri": safe_uri}, ) - except (TypeError, ValueError): + except Exception: # noqa: BLE001 - never raise while an error is live pass return ConnectivityProbeError.for_target(target=safe_uri, reason=reason) +def _masked_extras(extras: Any, mask: Callable[[str], str]) -> Dict[str, Any]: + """``extras`` with every string masked, at any depth ``as_json`` would dump.""" + + def walk(value: Any) -> Any: + if isinstance(value, str): + return mask(value) + if isinstance(value, dict): + return {key: walk(item) for key, item in value.items()} + if isinstance(value, (list, tuple)): + return [walk(item) for item in value] + return value + + return {key: walk(value) for key, value in dict(extras or {}).items()} + + def _type_name(exc: BaseException) -> str: """``exc``'s type as a traceback names it: the module is left out for a builtin.""" kind = type(exc) diff --git a/tests/cli/discover/test_init_discover_redaction.py b/tests/cli/discover/test_init_discover_redaction.py index 0057cc8cd..24e0c219f 100644 --- a/tests/cli/discover/test_init_discover_redaction.py +++ b/tests/cli/discover/test_init_discover_redaction.py @@ -40,7 +40,11 @@ from hypothesis import strategies as st import fluid_build.cli._init_discover_handler as handler -from fluid_build.cli._errors import ConnectivityProbeError, SchemaValidationError +from fluid_build.cli._errors import ( + ConnectivityProbeError, + FluidUserError, + SchemaValidationError, +) from fluid_build.cli.discover.registry import DiscoveredColumn, DiscoveredStream from fluid_build.observability.secret_redactor import forget_known_secrets, redact_secret_text from fluid_build.providers._duckdb_sandbox import DuckDBSandboxError @@ -261,36 +265,76 @@ def test_error_quoting_the_uri_masks_a_short_password_with_raw_at_sign( class TestTypedErrorsKeepTheirOwnAdvice: """An error that already says what to do is not reframed as a connectivity failure.""" - def test_a_typed_error_keeps_its_own_advice_and_is_still_masked(self, monkeypatch, tmp_path): + def test_a_typed_error_keeps_its_own_advice_and_every_field_is_masked( + self, monkeypatch, tmp_path + ): """A sandbox refusal reaches the same ``except Exception`` as a driver error. Reframing it as a connectivity failure replaced its ``fix`` with "Check VPN / network policy / credentials", which sends the reader to their network for something in their working directory. Its own fields are - kept -- and still masked, because the refusal names the location it - refused and that location can carry userinfo. + kept -- and the credential is put in EVERY field here, because the + refusal names the location it refused and that can carry userinfo. + ``extras`` and ``doc`` matter as much as the rest: ``as_json`` emits + them, and ``extras`` was the one field the keep-and-mask path first + copied through verbatim. """ uri = f"https://alice:{SENTINEL}@example.com/data.csv" stub = _StubDiscoverer( error=DuckDBSandboxError( - what="a declared remote prefix would grant a local directory to the SQL", + what=f"a declared remote prefix would grant a local directory: {uri}", why=f"Refused here: {uri} resolves to /somewhere", - fix="Remove the link in the working directory.", - doc="https://example.invalid/doc", + fix=f"Remove the link in the working directory, or drop {uri}.", + doc=f"https://example.invalid/doc?ctx={SENTINEL}", + where=f"contract.fluid.yaml -> {uri}", + extras={"dsn": uri, "nested": {"copy": [uri]}}, ) ) with pytest.raises(DuckDBSandboxError) as info: _run(monkeypatch, tmp_path, uri, stub) exc = info.value + + # Its own advice survived, rather than the connectivity template's. assert exc.code == "DuckDBSandboxError" assert "working directory" in exc.fix assert "VPN" not in exc.fix - assert SENTINEL not in exc.what - assert SENTINEL not in (exc.why or "") + # Every field a reader or a machine can see is masked. + for field in (exc.what, exc.why or "", exc.fix, exc.doc or "", exc.where or ""): + assert SENTINEL not in field, field + assert SENTINEL not in json.dumps(exc.extras, default=str) assert SENTINEL not in exc.render(color=False) assert SENTINEL not in exc.as_json() + # Masked, not merely dropped: the location is still named. + assert "example.com" in exc.why assert _contracts(tmp_path) == [] + def test_a_subclass_that_cannot_be_rebuilt_falls_back_without_leaking( + self, monkeypatch, tmp_path + ): + """``replace`` cannot rebuild every subclass, and this runs mid-failure. + + A second exception raised here would carry the driver's error, URI and + password as its context. The connectivity framing is the fallback, and + the credential must be absent from it too. + """ + + class _Awkward(DuckDBSandboxError): + def __init__(self, token: str) -> None: # not constructible from fields + super().__init__( + what=f"refused {token}", + why=f"because of {token}", + fix="do something", + doc="https://example.invalid/d", + ) + + uri = f"https://alice:{SENTINEL}@example.com/data.csv" + with pytest.raises(FluidUserError) as info: + _run(monkeypatch, tmp_path, uri, _StubDiscoverer(error=_Awkward(uri))) + exc = info.value + assert SENTINEL not in exc.render(color=False) + assert SENTINEL not in exc.as_json() + assert SENTINEL not in _chain_text(exc) + class TestPasswordWithUnencodedDelimiter: """A password holding a raw ``/``, ``?`` or ``#`` ends the RFC 3986 authority early. diff --git a/tests/providers/test_duckdb_sandbox_remote_prefix.py b/tests/providers/test_duckdb_sandbox_remote_prefix.py index 6c0198abe..11b0ea224 100644 --- a/tests/providers/test_duckdb_sandbox_remote_prefix.py +++ b/tests/providers/test_duckdb_sandbox_remote_prefix.py @@ -29,10 +29,14 @@ The prefix is now joined to the resolved cwd lexically, so the honest form still matches and an escaping one no longer does. -6 of the 8 tests here fail when the production change is reverted, on duckdb -1.5.6 and 2.0.0 alike. The 2 that do not are deliberate: the legitimate-prefix -guard has to pass before and after, and the defect witness passes on pre-fix -code by construction. +Reverting the production change -- its two call sites and the read-back's +accommodation, with the new helpers left defined so the tests that patch them +still import -- fails 6 of the 8 tests this file had at that measurement, on +duckdb 1.5.6 and 2.0.0 alike. The 2 that survive are deliberate: the +legitimate-prefix guard has to pass before and after, and the defect witness +passes on pre-fix code by construction. A LITERAL revert deletes +``_refuse_escaping_remote_prefixes`` as well, so the tests that patch it error +instead of failing -- still red, by a different route. There are two barriers, and the tests pin each one alone: the grant is refused before it is set, and the read-back under the lock refuses it again if that @@ -64,6 +68,7 @@ from tests.providers.test_duckdb_sandbox_spill import ( # noqa: E402 UNGRANTED_HOST_FILE, _reads, + _Reporting, _setting, ) @@ -213,6 +218,31 @@ def test_with_both_barriers_bypassed_the_grant_is_real( con.close() +@posix_symlink +def test_the_read_back_defers_to_the_escape_message( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The deferral needs the read-back driven directly to be exercised at all. + + In ``test_the_read_back_still_refuses_when_the_pre_set_check_is_bypassed`` + the patched no-op is the SAME function the read-back defers to, so that test + witnesses the generic "did not ask for" message, not this one. Here the real + check runs inside the read-back, against a stand-in connection reporting the + grant DuckDB would have made. + """ + monkeypatch.chdir(_repo_with_scheme_link(tmp_path, "/")) + allow = DuckDBAllowlist.none().with_remote("s3://etc/") + + with pytest.raises(DuckDBSandboxError) as excinfo: + sandbox._refuse_ungranted_dirs( + _Reporting((os.path.realpath("/etc") + os.sep,)), allow, str(tmp_path) + ) + + rendered = _rendered(excinfo.value) + assert "s3://etc/" in rendered, rendered + assert "did not ask for" not in rendered, rendered + + # ── what must keep working ───────────────────────────────────────────────