security: a declared remote prefix handed a local directory to contract SQL

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)

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 ``<cwd>/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,22 +1022,31 @@ 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
-    ``<cwd>/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
+    ``<cwd>/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
         if (entry not in allow.remote_prefixes)
         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)}")

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