diff --git a/fluid_build/cli/_init_discover_handler.py b/fluid_build/cli/_init_discover_handler.py index b275781a..bfd1f121 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,90 @@ 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 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.) """ - 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. + + 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 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: + return replace( + exc, + 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={**_masked_extras(exc.extras, mask), "uri": safe_uri}, + ) + 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/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index 3519c92d..62a10ab9 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 74828ccf..24e0c219 100644 --- a/tests/cli/discover/test_init_discover_redaction.py +++ b/tests/cli/discover/test_init_discover_redaction.py @@ -40,9 +40,14 @@ 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 from fluid_build.providers._sql_safety import libpq_escape, quote_ansi_string_literal from fluid_build.schema_manager import FluidSchemaManager @@ -257,6 +262,80 @@ 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_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 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=f"a declared remote prefix would grant a local directory: {uri}", + why=f"Refused here: {uri} resolves to /somewhere", + 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 + # 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. @@ -1052,12 +1131,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 +1258,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 00000000..11b0ea22 --- /dev/null +++ b/tests/providers/test_duckdb_sandbox_remote_prefix.py @@ -0,0 +1,297 @@ +# 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. + +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 +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, + _Reporting, + _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() + + +@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 ─────────────────────────────────────────────── + + +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)