From 2aeeed5e77e513aed3346f1e58a3ecac4fde38ec Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 4 Nov 2026 06:55:11 +0100 Subject: [PATCH 1/4] fix(welcome-scan): bound the specialization walk and end it with its scan (#810) The specialization probe took its contracts from sorted(start.rglob(...))[:8], which reads the whole cwd tree before it can sort. Under a directory holding many repos that was 279 s and 7,816 hits. #800 made the scan return at its 1.0 s cap, but Python cannot kill a thread, so the walker kept running at about half a core, and the mode picker, the interview signals and the welcome panel each start a scan: up to three walkers overlapped. The probe now takes its contracts from _find_contracts: - depth-first with each directory's entries in name order, which yields exactly sorted(rglob(...)). The walk stops at the 8th contract and the 8 are the ones the old code tallied, wherever the bounds below do not cut; - it does not enter the directories on the repo's list (upstream_discovery IGNORED_DIRS | BUILD_OUTPUT_DIRS), lists at most 6 levels below the start and reads at most 10,000 entries; - symlinks are not followed and only a regular file counts as a contract (rglob opens a symlink to /dev/zero, or a FIFO named like a contract, and blocks); - run_welcome_scan sets a threading.Event when it returns. The walk ends at its next directory, and the probe stops parsing on it too; - a process-wide lock runs one walk at a time. A walker stuck in a filesystem call (a dead mount) cannot be stopped; the walks of later scans wait behind it and leave with their own scan (a caller with no scan gives up after one scan budget), so a process holds at most one. Measured from ~/Documents/open-source-community, three scans in one process after build_parser(): walker threads alive 3 -> 0, CPU over a 3 s idle 1.42 s -> 0.00 s, scan wall time 1.0 s -> 0.25-0.67 s (it no longer waits out the budget for the walk). tests/test_welcome_scan_walk.py pins the order against rglob, the skip list, the caps, symlinks, the stop flag, the wait for the gate and the tally rule. tests/test_welcome_scan_budget.py pins that the walker ends within a second of its scan, that consecutive scans never have two walks reading at once, and that three scans over a hung filesystem leave one walker. --- CHANGELOG.md | 14 ++ CLAUDE.md | 5 +- fluid_build/cli/_welcome_scan.py | 135 +++++++++++-- tests/test_welcome_scan_budget.py | 144 +++++++++++++- tests/test_welcome_scan_walk.py | 316 ++++++++++++++++++++++++++++++ 5 files changed, 594 insertions(+), 20 deletions(-) create mode 100644 tests/test_welcome_scan_walk.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b0c46352..80148ceb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -262,6 +262,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The `fluid forge` welcome scan kept walking the whole directory tree after it had returned.** + Its type-suggestion probe listed contracts with `sorted(cwd.rglob("contract.fluid.yaml"))[:8]`, + which reads every file under the current directory (279 s under a folder holding many repos) and + goes into `.venv`, `.git` and `node_modules`. The scan returns at its 1.0 s budget, but Python + cannot kill a thread, so the walker went on at about half a core, and the mode picker, the + interview and the welcome panel each start a scan: up to three overlapped. The walk is now + bounded and ends with its scan. It skips the directories `util/upstream_discovery` already skips + (`.git`, `.venv`, `site-packages`, `node_modules`, `dist`, `build`, ...), lists at most 6 levels + below the current directory, reads at most 10,000 directory entries, and stops at its next + directory once the scan has returned; one walk runs at a time per process. It tallies the same + first 8 contracts as before wherever those limits do not cut the walk. A symlink named + `contract.fluid.yaml`, or one that is not a regular file (a FIFO, a device), is no longer + opened. + - **`fluid apply --mode amend-and-build` provisioned a product, then refused its id.** The build phase keeps run records, cursors and watermarks under `.fluid/runs///`, so its ids must start with a letter or underscore, use only letters, digits, `_`, `.` and `-`, diff --git a/CLAUDE.md b/CLAUDE.md index 551f490d..5ea36c5e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -420,10 +420,11 @@ hand-rolling the protocol. `cli/_welcome_scan.py::run_welcome_scan` now has a budget that means what it says: -- **One wall-clock cap.** `_TOTAL_TIMEOUT_S` (1.0 s) is the only scan budget; the old per-probe `_PROBE_TIMEOUT_S` is gone. Probes run on daemon threads (`_spawn_probe`: one `Future` each, deliberately not a `ThreadPoolExecutor`, whose non-daemon workers are joined at interpreter exit and would hold the CLI open for an abandoned probe). The scan returns at the deadline and drops stragglers. Python cannot kill a thread, so an abandoned probe keeps running in the background until it finishes; the one that can run long is `_probe_specialization_suggestion` (unbounded `rglob`), and the picker, the interview signals and the welcome panel each start a scan, so up to three walkers can overlap on a huge cwd. Probes must not write to stdout/stderr (a daemon thread caught mid-write at interpreter exit can abort the process). +- **One wall-clock cap.** `_TOTAL_TIMEOUT_S` (1.0 s) is the only scan budget; the old per-probe `_PROBE_TIMEOUT_S` is gone. Probes run on daemon threads (`_spawn_probe`: one `Future` each, deliberately not a `ThreadPoolExecutor`, whose non-daemon workers are joined at interpreter exit and would hold the CLI open for an abandoned probe). The scan returns at the deadline and drops stragglers. Python cannot kill a thread, so an abandoned probe keeps running until it finishes (the one that used to run for minutes is the tree walk, below). Probes must not write to stdout/stderr (a daemon thread caught mid-write at interpreter exit can abort the process). - **Cold imports are off the clock.** `_warm_probe_imports` imports `_WARM_MODULES` (the AI-config pydantic chain, the dbt runner) from the main thread while the probes run, then the budget starts. Probe imports stay function-local: if a probe starts importing something new, list it in `_WARM_MODULES` or `tests/test_welcome_scan_budget.py::test_probes_import_nothing_new_after_the_warm_up` fails. NB `build_parser()` already imports `ai_setup`, the dbt runner and pydantic, so in a real CLI process only `copilot.unified_config` is still cold; the warm-up matters for tests and library callers. - **dbt has its own cap.** `_DBT_VERSION_TIMEOUT_S` (0.32 s) sizes the `dbt --version` subprocess and is not derived from the scan budget and must stay below it, so the child is killed by its own timeout instead of being orphaned. With Python dbt-core on PATH it usually waits out that cap, which is the scan's floor (~0.35 s). -- `scan_duration_ms` is the real wall-clock of the call, imports included. Pins: `tests/test_welcome_scan_budget.py`. +- **The specialization walk is bounded and ends with its scan.** `_probe_specialization_suggestion` used to `rglob` the whole cwd tree (279 s under a directory holding many repos), and the picker, the interview signals and the welcome panel each start a scan, so up to three abandoned walkers overlapped (~0.5 core). Its contracts now come from `_find_contracts`: depth-first in name order, so the first 8 are exactly `sorted(rglob(...))[:8]` wherever the bounds do not cut. It skips `util/upstream_discovery`'s `IGNORED_DIRS | BUILD_OUTPUT_DIRS`, lists at most `_SPECIALIZATION_MAX_DEPTH` (6) levels, reads at most `_SPECIALIZATION_MAX_ENTRIES` (10,000) entries, and does not follow symlinks or take anything but a regular file. `run_welcome_scan` sets a `threading.Event` when it returns and the walk ends at its next directory; `_WALK_GATE` runs one walk at a time, so a walker stuck in a filesystem call (a dead mount) is the only one a process can hold. `tests/fragments/test_no_raw_readers.py` registers `_probe_specialization_suggestion` as an accepted raw reader. The scanner finds it through the word `contract` in that function (the `contract.fluid.yaml` in its docstring, the `_find_contracts` call); the registry only checks that the function exists, so losing both silently orphans the entry. +- `scan_duration_ms` is the real wall-clock of the call, imports included. Pins: `tests/test_welcome_scan_budget.py`, `tests/test_welcome_scan_walk.py`. ## Working style diff --git a/fluid_build/cli/_welcome_scan.py b/fluid_build/cli/_welcome_scan.py index 4c2f58e1..abfd5eaa 100644 --- a/fluid_build/cli/_welcome_scan.py +++ b/fluid_build/cli/_welcome_scan.py @@ -31,8 +31,10 @@ Every probe is best-effort and never raises. Budget: once the one-time module imports are done, the probes share ``_TOTAL_TIMEOUT_S`` of wall-clock -and the scan returns by then, dropping any probe still running. The dbt probe -sets the floor: with Python dbt-core on PATH it usually waits out +and the scan returns by then, dropping any probe still running. A dropped probe +is abandoned, not killed (Python cannot kill a thread); the one that walks the +directory tree bounds itself and ends when the scan returns (``_find_contracts``). +The dbt probe sets the floor: with Python dbt-core on PATH it usually waits out ``_DBT_VERSION_TIMEOUT_S`` and reports nothing. ``scan_duration_ms`` is the measured wall-clock of the whole call, imports included. """ @@ -48,6 +50,7 @@ import time from concurrent.futures import Future, wait from dataclasses import asdict, dataclass, field +from itertools import islice from pathlib import Path from typing import Any, Callable, Dict, List, Optional, Tuple @@ -73,11 +76,30 @@ # the AI probe's cold cost (~0.1s CPU, several times that on a loaded host). "fluid_build.copilot.unified_config", "fluid_build.build_runners.dbt.runner", # _probe_dbt_engine + # ``_walk_contracts`` takes the contract filename from ``cli.artifact_paths``, which + # ``workspace_config`` above already imports. + "fluid_build.util.upstream_discovery", # _walk_contracts (directories to skip) ) # Repeat-user threshold — at this many forges we stop showing the panel. _REPEAT_USER_THRESHOLD = 5 # File extensions we recognise as candidate sample data. _SAMPLE_EXTENSIONS = (".csv", ".parquet", ".json", ".jsonl", ".tsv", ".duckdb") +# The specialization probe tallies the productType of the first contracts under the +# start directory. Its tree walk (``_find_contracts``) is bounded three ways: contracts +# taken, directory levels below the start, and directory entries read. At the caps an +# ordinary tree costs ~0.1s (a pathological one, 10,000 one-entry directories, ~0.6s of +# CPU), and the scan's stop flag ends the walk early either way. +_SPECIALIZATION_SAMPLE = 8 +_SPECIALIZATION_MAX_DEPTH = 6 +_SPECIALIZATION_MAX_ENTRIES = 10_000 +# One specialization walk at a time in the process. A walk the scan gave up on ends at +# its next directory once the scan's stop flag is set, but one stuck in a filesystem +# call (a hung network mount) never does, and each later scan would add another. Behind +# this lock the later walks wait and leave when their own scan returns (a caller with no +# scan gives up after one scan budget), so a process holds at most one stuck walker. +_WALK_GATE = threading.Lock() +# How often a walk waiting for the gate looks at its scan's stop flag. +_WALK_GATE_POLL_S = 0.05 # CLI binaries we report on. _CLI_BINARIES = ( "dbt", @@ -361,24 +383,99 @@ def _probe_return_user() -> Dict[str, Any]: return {"return_user": False, "forge_count": 0} -def _probe_specialization_suggestion(start: Path) -> Dict[str, Any]: - """Suggest a productType based on recent forge history. +def _walk_contracts(start: Path, stop: Optional[threading.Event]) -> List[Path]: + """The bounded walk behind :func:`_find_contracts`: depth-first, entries in name order.""" + from fluid_build.cli.artifact_paths import CONTRACT_FILENAME + from fluid_build.util.upstream_discovery import BUILD_OUTPUT_DIRS, IGNORED_DIRS - Reads the last 5 ``.fluid/forge-receipt.json`` entries under the - workspace and proposes the dominant type. Only fires when ≥4/5 - are the same type. + skip = IGNORED_DIRS | BUILD_OUTPUT_DIRS + found: List[Path] = [] + unread = _SPECIALIZATION_MAX_ENTRIES + + def visit(directory: str, depth: int) -> bool: + """Read ``directory``, then its subdirectories; True once the whole walk is over.""" + nonlocal unread + if unread <= 0 or (stop is not None and stop.is_set()): + return True + try: + with os.scandir(directory) as listing: + entries = list(islice(listing, unread)) + except OSError: + return False + unread -= len(entries) + # Names compare as path components do, so a contract file falls among the + # subdirectories exactly where ``sorted(rglob(...))`` would put it. + for entry in sorted(entries, key=lambda e: e.name): + try: + if entry.is_dir(follow_symlinks=False): + if depth < _SPECIALIZATION_MAX_DEPTH and entry.name not in skip: + if visit(entry.path, depth + 1): + return True + elif entry.name == CONTRACT_FILENAME and entry.is_file(follow_symlinks=False): + found.append(Path(entry.path)) + if len(found) >= _SPECIALIZATION_SAMPLE: + return True + except OSError: + continue + return False + + visit(str(start), 0) + return found + + +def _find_contracts(start: Path, stop: Optional[threading.Event] = None) -> List[Path]: + """The first ``_SPECIALIZATION_SAMPLE`` contracts under ``start``, in ``rglob`` order. + + Depth-first with each directory's entries in name order, which lists the contracts + exactly as ``sorted(start.rglob("contract.fluid.yaml"))`` does, so the walk can end + at the last one it needs instead of reading the whole tree first. It also ends on + its own bounds, and only the contracts it reached are returned: + + * directories on the repo's VCS / dependency / build-output list + (``upstream_discovery.IGNORED_DIRS`` and ``BUILD_OUTPUT_DIRS``: ``.git``, + ``.venv``, ``site-packages``, ``node_modules``, ``dist``, ...) are not entered; + * nothing deeper than ``_SPECIALIZATION_MAX_DEPTH`` levels below ``start`` is + listed, and at most ``_SPECIALIZATION_MAX_ENTRIES`` entries are read in all (a + directory larger than what is left of that is read in part); + * symlinks are neither followed nor tallied, and only a regular file counts as a + contract: ``rglob`` opens a symlink to ``/dev/zero`` or a FIFO named like one; + * ``stop`` is looked at before every directory. Python cannot kill a thread, so + this is how a scan ends the walk it abandoned. + + One walk runs at a time per process (``_WALK_GATE``); a walk that is made to wait + leaves with ``[]`` as soon as its ``stop`` is set, or after one scan budget if it + has none. + """ + deadline = time.monotonic() + _TOTAL_TIMEOUT_S + while not _WALK_GATE.acquire(timeout=_WALK_GATE_POLL_S): + if (stop is not None and stop.is_set()) or time.monotonic() >= deadline: + return [] + try: + return _walk_contracts(start, stop) + finally: + _WALK_GATE.release() + + +def _probe_specialization_suggestion( + start: Path, stop: Optional[threading.Event] = None +) -> Dict[str, Any]: + """Suggest a productType from the ``contract.fluid.yaml`` files under ``start``. + + Tallies ``metadata.productType`` over the first ``_SPECIALIZATION_SAMPLE`` + contracts ``_find_contracts`` returns and proposes the dominant type when at + least 4 of at least 5 typed contracts agree. ``stop`` is the scan's flag for a + probe it has given up on: the walk and the parse loop both end on it. """ try: - # Cheapest possible: glob for nearby contracts and tally their - # productType. Limited to a small radius so the scan stays - # under budget. - contracts = sorted(start.rglob("contract.fluid.yaml"))[:8] + contracts = _find_contracts(start, stop) if not contracts: return {} import yaml as _yaml types: List[str] = [] for c in contracts: + if stop is not None and stop.is_set(): + return {} try: doc = _yaml.safe_load(c.read_text(encoding="utf-8")) or {} pt = (doc.get("metadata") or {}).get("productType") @@ -456,7 +553,9 @@ def run_welcome_scan(*, start: Optional[Path] = None) -> WelcomeFindings: the dbt subprocess included); the probes then get ``_TOTAL_TIMEOUT_S`` of wall-clock. A probe still running at the deadline is dropped — its fields stay at their defaults — and abandoned, so a hung probe delays neither the - prompt nor the process exit. + prompt nor the process exit. The probe that walks the directory tree is told + to stop when the scan returns and ends at its next directory; the others run + on to their own end. Wall-clock is the cold imports plus at most ``_TOTAL_TIMEOUT_S``; ``scan_duration_ms`` reports all of it. @@ -464,6 +563,7 @@ def run_welcome_scan(*, start: Optional[Path] = None) -> WelcomeFindings: target = (start or Path.cwd()).resolve() findings = WelcomeFindings() started = time.monotonic() + stop = threading.Event() # set when the scan returns, see ``_find_contracts`` probes: Dict[str, Tuple[Callable[..., Dict[str, Any]], Tuple[Any, ...]]] = { "workspace": (_probe_workspace, (target,)), @@ -474,12 +574,15 @@ def run_welcome_scan(*, start: Optional[Path] = None) -> WelcomeFindings: "samples": (_probe_sample_data, (target,)), "cloud": (_probe_cloud_creds, ()), "return_user": (_probe_return_user, ()), - "specialization": (_probe_specialization_suggestion, (target,)), + "specialization": (_probe_specialization_suggestion, (target, stop)), } - futures = {name: _spawn_probe(name, fn, args) for name, (fn, args) in probes.items()} - _warm_probe_imports() - done, _ = wait(futures.values(), timeout=_TOTAL_TIMEOUT_S) + try: + futures = {name: _spawn_probe(name, fn, args) for name, (fn, args) in probes.items()} + _warm_probe_imports() + done, _ = wait(futures.values(), timeout=_TOTAL_TIMEOUT_S) + finally: + stop.set() for name, future in futures.items(): if future not in done: LOG.debug("welcome_scan_probe_timeout: %s", name) diff --git a/tests/test_welcome_scan_budget.py b/tests/test_welcome_scan_budget.py index d194a4c0..572a622f 100644 --- a/tests/test_welcome_scan_budget.py +++ b/tests/test_welcome_scan_budget.py @@ -14,13 +14,15 @@ """Welcome-scan budget semantics. -Two promises, both measured against a real clock rather than a mocked one: +Three promises, all measured against a real clock rather than a mocked one: * the cold imports the probes trigger are paid before the budget starts, so the first scan of a process returns what a warm scan returns (on a loaded machine the AI probe's pydantic import used to blow the old per-probe window); * ``_TOTAL_TIMEOUT_S`` is a real cap: the scan returns on time and a probe it - gave up on cannot keep the process alive. + gave up on cannot keep the process alive; +* the one probe that can run long, the tree walk, ends when its scan does, and a + process holds at most one walker however many scans it runs. """ from __future__ import annotations @@ -141,6 +143,144 @@ def test_abandoned_probe_does_not_keep_the_process_alive(tmp_path): ) +# --------------------------------------------------------------------------- +# The tree walk ends with its scan +# --------------------------------------------------------------------------- + + +def _walkers(): + return [t for t in threading.enumerate() if t.name.startswith("welcome-scan-specialization")] + + +def _wait_until(predicate, timeout): + deadline = time.monotonic() + timeout + while not predicate(): + if time.monotonic() >= deadline: + return False + time.sleep(0.01) + return True + + +def _fan_tree(root: Path, fan: int = 20) -> int: + """``fan`` directories under ``root`` holding ``fan`` empty ones each; no contract anywhere. + + Returns how many directories a complete walk would list. + """ + for i in range(fan): + for j in range(fan): + (root / f"d{i:02d}" / f"e{j:02d}").mkdir(parents=True) + return 1 + fan + fan * fan + + +class _Listing: + """``os.scandir`` for the walker thread only; every other caller gets the real one. + + Each directory the walker lists waits ``delay`` seconds first. With ``hang`` the + first listing waits for ``release`` instead, as a call stuck on a dead network + mount would. ``release`` cuts every wait short, so cleanup is instant. + """ + + def __init__(self, delay: float = 0.0, hang: bool = False): + self.delay, self.hang = delay, hang + self.release = threading.Event() + self.listed: list = [] + self._real = os.scandir + + def __call__(self, path="."): + if threading.current_thread().name.startswith("welcome-scan-specialization"): + self.listed.append(str(path)) + self.release.wait(60 if self.hang and len(self.listed) == 1 else self.delay) + return self._real(path) + + +@pytest.fixture +def short_scan(monkeypatch): + """A 0.3s scan with no dbt and the imports paid: it returns at the cap while a walk is going.""" + monkeypatch.setattr(ws, "_TOTAL_TIMEOUT_S", 0.3) + monkeypatch.setattr( + "fluid_build.build_runners.dbt.runner._resolve_dbt_executable", lambda: None + ) + ws._warm_probe_imports() + assert _wait_until(lambda: not _walkers(), 2.0), "a walker from an earlier test is still alive" + + +def test_a_walk_the_scan_gave_up_on_ends_within_a_second_of_the_scan( + tmp_path, monkeypatch, short_scan +): + """Python cannot kill the walker, so the scan tells it to stop and it ends at its next directory. + + The tree takes 21s to walk at 50 ms a directory and the scan gives up after 0.3s. + Without the stop flag the walker reads on for the other 20s. + """ + directories = _fan_tree(tmp_path) + stand_in = _Listing(delay=0.05) + monkeypatch.setattr(os, "scandir", stand_in) + try: + ws.run_welcome_scan(start=tmp_path) + returned = time.monotonic() + gone = _wait_until(lambda: not _walkers(), timeout=2.0) + ended_after = time.monotonic() - returned + listed = len(stand_in.listed) + time.sleep(0.3) + assert gone, f"the walker was still reading {ended_after:.1f}s after the scan returned" + assert len(stand_in.listed) == listed # not one more directory once it ended + assert listed < directories / 4 # nowhere near the whole tree + finally: + stand_in.release.set() + + +def test_consecutive_scans_never_have_two_walks_reading_at_once(tmp_path, monkeypatch, short_scan): + """Each scan abandons its walk; the next scan's walk waits for it instead of overlapping.""" + _fan_tree(tmp_path) + stand_in = _Listing(delay=0.05) + monkeypatch.setattr(os, "scandir", stand_in) + reading = {"now": 0, "most": 0, "walks": 0} + lock = threading.Lock() + real_walk = ws._walk_contracts + + def counted(start, stop): + with lock: + reading["now"] += 1 + reading["walks"] += 1 + reading["most"] = max(reading["most"], reading["now"]) + try: + return real_walk(start, stop) + finally: + with lock: + reading["now"] -= 1 + + monkeypatch.setattr(ws, "_walk_contracts", counted) + try: + for _ in range(3): + ws.run_welcome_scan(start=tmp_path) + assert _wait_until(lambda: len(_walkers()) <= 1, timeout=1.0) + assert _wait_until(lambda: not _walkers(), timeout=2.0) # none outlives the last scan + finally: + stand_in.release.set() + + assert reading["walks"] >= 1 + assert reading["most"] == 1 + + +def test_three_scans_over_a_hung_filesystem_leave_one_walker(tmp_path, monkeypatch, short_scan): + """A walk stuck inside a filesystem call (a dead network mount) cannot be stopped. + + The scans after it must not add a stuck thread each: their walks wait behind the + first one and leave with their own scan. Before the gate: three walkers. + """ + _fan_tree(tmp_path, fan=2) + stand_in = _Listing(hang=True) + monkeypatch.setattr(os, "scandir", stand_in) + try: + for _ in range(3): + ws.run_welcome_scan(start=tmp_path) + assert _wait_until(lambda: len(_walkers()) == 1, timeout=2.0), _walkers() + assert len(stand_in.listed) == 1 # only the first walker ever reached the filesystem + finally: + stand_in.release.set() + assert _wait_until(lambda: not _walkers(), timeout=2.0) # unstuck, it sees its stop and ends + + # --------------------------------------------------------------------------- # Cold imports are off the clock # --------------------------------------------------------------------------- diff --git a/tests/test_welcome_scan_walk.py b/tests/test_welcome_scan_walk.py new file mode 100644 index 00000000..22d40a61 --- /dev/null +++ b/tests/test_welcome_scan_walk.py @@ -0,0 +1,316 @@ +# 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. + +"""The tree walk behind the welcome scan's specialization probe. + +``_find_contracts`` replaced ``sorted(start.rglob("contract.fluid.yaml"))[:8]``, which +read the whole tree (279 s under a directory holding many repos). It has to return the +same contracts in the same order wherever its bounds do not cut the walk, and stop +where they do. +""" + +from __future__ import annotations + +import os +import sys +import threading +import time +from pathlib import Path + +import pytest + +from fluid_build.cli import _welcome_scan as ws +from fluid_build.util.upstream_discovery import BUILD_OUTPUT_DIRS, IGNORED_DIRS + +pytestmark = pytest.mark.unit + +_BODY = "metadata:\n productType: SDP\n" +_posix_only = pytest.mark.skipif(sys.platform == "win32", reason="needs symlinks / FIFOs") + + +def _contract(root: Path, *parts: str, body: str = _BODY) -> Path: + directory = root.joinpath(*parts) + directory.mkdir(parents=True, exist_ok=True) + path = directory / "contract.fluid.yaml" + path.write_text(body) + return path + + +@pytest.fixture +def listings(tmp_path, monkeypatch): + """The directories under ``tmp_path`` that the walk hands to ``os.scandir``, in order.""" + seen = [] + real = os.scandir + + def spy(path="."): + if str(path).startswith(str(tmp_path)): + seen.append(Path(path)) + return real(path) + + monkeypatch.setattr(os, "scandir", spy) + return seen + + +# --------------------------------------------------------------------------- +# Same contracts, same order as rglob +# --------------------------------------------------------------------------- + + +def test_the_contracts_come_out_as_rglob_lists_them(tmp_path): + """Depth-first in name order is ``sorted(rglob(...))``, so the first 8 are unchanged. + + The names are the ones where a plain string sort of the full paths and a path + comparison disagree ("a.b" sorts before "a/x" as text, after it as components), and + a contract file falls between its sibling directories ("b" < "contract..." < "z"). + """ + for parts in [ + ("a",), + ("a", "b"), + ("a", "z"), + ("a.b",), + ("a-b",), + ("B",), + ("b", "c"), + ("m",), + ("m", "n"), + ("m", "n", "o"), + ("x y",), + (".hidden",), + ("z",), + ]: + _contract(tmp_path, *parts) + _contract(tmp_path) # the start directory's own + + expected = sorted(tmp_path.rglob("contract.fluid.yaml")) + assert len(expected) > ws._SPECIALIZATION_SAMPLE # the cut at 8 is exercised + assert ws._find_contracts(tmp_path) == expected[: ws._SPECIALIZATION_SAMPLE] + + +def test_fewer_than_the_sample_are_all_returned(tmp_path): + for name in ("p1", "p2", "p3"): + _contract(tmp_path, name) + assert ws._find_contracts(tmp_path) == sorted(tmp_path.rglob("contract.fluid.yaml")) + + +def test_the_walk_ends_at_the_last_contract_it_needs(tmp_path, listings): + """Twenty products, eight wanted: the other twelve directories are never read.""" + for i in range(20): + _contract(tmp_path, f"p{i:02d}") + + found = ws._find_contracts(tmp_path) + + assert [p.parent.name for p in found] == [f"p{i:02d}" for i in range(8)] + assert listings == [tmp_path] + [tmp_path / f"p{i:02d}" for i in range(8)] + + +def test_a_missing_start_has_no_contracts(tmp_path): + assert ws._find_contracts(tmp_path / "nowhere") == [] + + +# --------------------------------------------------------------------------- +# What it does not enter +# --------------------------------------------------------------------------- + + +def test_the_repos_skip_list_is_not_entered(tmp_path): + """A venv holds a copy of every shipped template; a build tree holds copies of contracts. + + Every name on the repo's list (``upstream_discovery``), contract directly inside + and one level down, and none of them is tallied. + """ + skipped = sorted(IGNORED_DIRS | BUILD_OUTPUT_DIRS) + assert {".git", ".venv", "venv", "site-packages", "node_modules", "__pycache__"} <= set(skipped) + assert {"build", "dist", "target"} <= set(skipped) + for name in skipped: + _contract(tmp_path, name) + _contract(tmp_path, name, "pkg") + kept = _contract(tmp_path, "products", "orders") + + assert ws._find_contracts(tmp_path) == [kept] + + +def test_directories_below_the_depth_cap_are_not_listed(tmp_path): + cap = ws._SPECIALIZATION_MAX_DEPTH + levels = [f"l{i}" for i in range(cap + 1)] + at_cap = _contract(tmp_path, *levels[:cap]) # `cap` levels below the start: listed + _contract(tmp_path, *levels) # one more level: not listed + + assert ws._find_contracts(tmp_path) == [at_cap] + + +def test_entries_beyond_the_cap_are_not_read(tmp_path, monkeypatch, listings): + """Once the entry budget is spent the walk ends, wherever it is.""" + monkeypatch.setattr(ws, "_SPECIALIZATION_MAX_ENTRIES", 50) + (tmp_path / "a").mkdir() + for i in range(200): + (tmp_path / "a" / f"f{i:03d}.csv").write_text("x") + _contract(tmp_path, "b") # sorts after "a", but the budget is gone by then + + assert ws._find_contracts(tmp_path) == [] + assert listings == [tmp_path, tmp_path / "a"] # "a" read in part, "b" never listed + + +@_posix_only +def test_symlinks_and_special_files_are_not_tallied(tmp_path): + """``rglob`` would open a symlink to /dev/zero or a FIFO named like a contract, and block.""" + real = _contract(tmp_path, "real") + os.symlink(tmp_path / "real", tmp_path / "linked_dir") # not followed: no second copy + (tmp_path / "linked_file").mkdir() + os.symlink(real, tmp_path / "linked_file" / "contract.fluid.yaml") # not a regular file + (tmp_path / "fifo").mkdir() + os.mkfifo(tmp_path / "fifo" / "contract.fluid.yaml") # reading it would hang + + assert ws._find_contracts(tmp_path) == [real] + + +def test_an_unreadable_directory_is_skipped(tmp_path, monkeypatch): + locked = tmp_path / "a_locked" + locked.mkdir() + kept = _contract(tmp_path, "b_open") + real = os.scandir + + def scandir(path="."): + if Path(path) == locked: + raise PermissionError(13, "Permission denied", str(path)) + return real(path) + + monkeypatch.setattr(os, "scandir", scandir) + + assert ws._find_contracts(tmp_path) == [kept] + + +# --------------------------------------------------------------------------- +# The stop flag +# --------------------------------------------------------------------------- + + +def test_a_stop_flag_that_is_already_set_reads_nothing(tmp_path, listings): + _contract(tmp_path, "p") + stop = threading.Event() + stop.set() + + assert ws._find_contracts(tmp_path, stop) == [] + assert listings == [] + + +def test_the_stop_flag_is_checked_before_every_directory(tmp_path, monkeypatch): + for i in range(10): + _contract(tmp_path, f"p{i}") + stop = threading.Event() + read = [] + real = os.scandir + + def scandir(path="."): + if str(path).startswith(str(tmp_path)): + read.append(Path(path)) + if len(read) == 2: # the scan is over while the second directory is being read + stop.set() + return real(path) + + monkeypatch.setattr(os, "scandir", scandir) + + found = ws._find_contracts(tmp_path, stop) + + assert read == [tmp_path, tmp_path / "p0"] # the third directory is never read + assert found == [tmp_path / "p0" / "contract.fluid.yaml"] + + +def test_a_walk_waiting_for_the_gate_leaves_as_soon_as_its_scan_ends(tmp_path, monkeypatch): + """The budget is not what ends this wait: the scan's stop flag does, e.g. on Ctrl-C.""" + _contract(tmp_path, "p") + monkeypatch.setattr(ws, "_TOTAL_TIMEOUT_S", 60.0) + stop = threading.Event() + result = [] + worker = threading.Thread(target=lambda: result.append(ws._find_contracts(tmp_path, stop))) + worker.daemon = True + assert ws._WALK_GATE.acquire(timeout=5), "a walker from an earlier test holds the gate" + try: + worker.start() + time.sleep(0.15) + assert worker.is_alive() # it is waiting behind the held gate + stop.set() + worker.join(timeout=3) + assert not worker.is_alive(), "kept waiting after its scan ended" + assert result == [[]] + finally: + ws._WALK_GATE.release() + + +def test_a_walk_with_no_stop_flag_gives_up_on_a_held_gate_after_one_scan_budget( + tmp_path, monkeypatch +): + """A direct caller has no scan to end its wait; it must not poll a stuck walker forever. + + A worker thread, so a regression fails the assertion instead of hanging the run. + """ + _contract(tmp_path, "p") + monkeypatch.setattr(ws, "_TOTAL_TIMEOUT_S", 0.2) + result = [] + worker = threading.Thread(target=lambda: result.append(ws._find_contracts(tmp_path))) + worker.daemon = True + assert ws._WALK_GATE.acquire(timeout=5), "a walker from an earlier test holds the gate" + try: + worker.start() + worker.join(timeout=3) + assert not worker.is_alive(), "still waiting on the gate long after the scan budget" + assert result == [[]] + finally: + ws._WALK_GATE.release() + + +# --------------------------------------------------------------------------- +# The probe keeps its tally +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("types", "suggested"), + [ + (["SDP"] * 4, ""), # four typed contracts are too few + (["SDP"] * 4 + ["ADP"], "SDP"), # 4 of 5 + (["SDP"] * 3 + ["ADP"] * 2, ""), # 3 of 5 + (["SDP"] * 4 + ["ADP"] * 2 + ["CDP"] * 2, "SDP"), # 4 of 8: a count, not a ratio + (["SDP", None, None, None, None], ""), # untyped contracts do not count + # only the first 8 are tallied: the 4 SDP after them would make 7 + (["SDP"] * 3 + ["ADP"] * 3 + ["CDP"] * 2 + ["SDP"] * 4, ""), + ], +) +def test_the_tally_rule_is_unchanged(tmp_path, types, suggested): + for i, product_type in enumerate(types): + body = "id: x\n" if product_type is None else f"metadata:\n productType: {product_type}\n" + _contract(tmp_path, f"p{i:02d}", body=body) + + result = ws._probe_specialization_suggestion(tmp_path) + + assert result == ({"suggested_data_product_type": suggested} if suggested else {}) + + +def test_the_parse_loop_ends_when_the_scan_does(tmp_path, monkeypatch): + """A parse cannot be interrupted, so the probe looks at the flag before each contract.""" + import yaml + + for i in range(6): + _contract(tmp_path, f"p{i}") + stop = threading.Event() + parsed = [] + + def safe_load(text): + parsed.append(text) + stop.set() # the scan returns while the first contract is being parsed + return {"metadata": {"productType": "SDP"}} + + monkeypatch.setattr(yaml, "safe_load", safe_load) + + assert ws._probe_specialization_suggestion(tmp_path, stop) == {} + assert len(parsed) == 1 From e7f6bcafc0bf1bb29da41daea4d4c9184f63c2c3 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 4 Nov 2026 07:04:05 +0100 Subject: [PATCH 2/4] test(security): fail when a whole-string validator ends in $ under .match (#816) Python's $ also matches just before a final "\n", so re.match(r"^[a-z]+$", "orders\n") succeeds. #788 and #797 fixed the known validators with \Z or fullmatch, but nothing kept the class from coming back: no ruff, pylint, bandit, dlint or Semgrep CE rule covers it (astral-sh/ruff#15017 proposes one). tests/security/test_no_dollar_anchored_validators.py walks the AST of fluid_build/ and follows the pattern of every .match / .search call back to where it is written: a literal or constant, a dict or tuple, an import, a helper's return value, a property, a re.Pattern parameter, or a pattern compiled at run time. It reads each pattern with re's own parser, so alternatives, groups, escapes and VERBOSE count the way re reads them. Calls whose $ is deliberate are listed in _ACCEPTED with the reason. An entry that no longer matches a call fails as well, so the list only shrinks. On main before #797 it fails on 44 calls, including the shapes the sweep missed: a pattern held in a dict, schema patterns compiled at run time, and a pattern the caller passes in. It also caught the $ + .match sites merged since (#793, #801, #799). Each was read and is listed with its reason. --- .../test_no_dollar_anchored_validators.py | 2190 +++++++++++++++++ 1 file changed, 2190 insertions(+) create mode 100644 tests/security/test_no_dollar_anchored_validators.py diff --git a/tests/security/test_no_dollar_anchored_validators.py b/tests/security/test_no_dollar_anchored_validators.py new file mode 100644 index 00000000..888d8c02 --- /dev/null +++ b/tests/security/test_no_dollar_anchored_validators.py @@ -0,0 +1,2190 @@ +# 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. + +"""Static gate: no whole-string validator in ``fluid_build/`` ends in ``$`` under ``.match``. + +Python's ``$`` matches at the end of the string and also just before a final +``\\n``. So ``re.match(r"^[a-z]+$", "orders\\n")`` succeeds, and the caller goes on +to use ``"orders\\n"``. Contract YAML produces such values easily (a ``|`` block +scalar ends in one), and so do ``.env`` files and ``%0A`` in a URL. #788 and #797 +fixed the known validators with ``\\Z`` or ``fullmatch``. This test keeps the +class from coming back. + +**The scan** walks the AST and imports nothing from ``fluid_build/``. It finds +every ``re.match`` / ``re.search`` call, and every ``.match`` / ``.search`` call on a +compiled pattern or reference to one (``filter(RX.match, names)``). It follows the +pattern back to where it was written: a literal, a module or class constant, a +dict, tuple or comprehension of patterns, an import from another module, a +helper's return value, a property. A call is reported when its pattern + +* has an alternative anchored at the start (``^`` or ``\\A``; ``.match`` anchors + there anyway) that can end at an unescaped ``$``. It is not reported under + ``MULTILINE``, or when the ``$`` follows a repeat that consumes a newline anyway + (``\\s*$``, ``[^/]*$``). The pattern is read with the parser ``re`` itself uses, + so groups, alternatives, escapes and ``VERBOSE`` read the way ``re`` reads them; or +* cannot be read: it is compiled at run time from data (a schema ``pattern``), or + it is a ``re.Pattern`` parameter, so the caller chooses it. + +A call that uses ``fullmatch``, ``sub``, ``findall`` or ``split`` is not reported, +since ``$`` cannot leave a newline unchecked there. + +**Fix a report** with ``\\Z`` or ``fullmatch``. Where the ``$`` is deliberate, add +the call to ``_ACCEPTED`` with the reason. The key is (file, function, pattern), +so a new function that reuses a listed pattern is reported again. An entry that no +longer matches a call fails ``test_every_accepted_entry_still_matches_a_call``, +so the list can only shrink. + +**Prior art.** No ruff, pylint, bandit or dlint rule covers this +(astral-sh/ruff#15017 proposes one). CodeQL's ``py/regex/unmatchable-dollar`` is +a different bug. The Semgrep registry rule ``check-validation-regex``, ported from +Brakeman's ``CheckValidationRegex``, covers Ruby only, and Semgrep CE cannot +follow a pattern across files or through a helper's return value. This test +borrows Brakeman's rule (a validator anchors at both ends of the string) and the +shape of this repo's other AST gates: ``tests/test_no_unencoded_text_io.py``, +``tests/test_no_reserved_tld_defaults.py`` and +``tests/fragments/test_no_raw_readers.py``. + +**Not covered.** An unanchored ``.search(r"\\.ya?ml$")`` is a suffix check, not a +whole-string validator, so it is not reported. The scan does not follow a pattern +into a registry filled by ``.append`` or item assignment, into a rule object +(``Rule(name, re.compile(...))``), into an attribute only a subclass sets, or +through ``+=``, ``getattr``, ``functools.partial`` or a star import, and a ``.match`` +on a parameter that is not annotated ``re.Pattern`` is not reported. +``fluid_build/`` holds no validator of these shapes today. A pattern built with +``%`` or ``str.format`` cannot be read, so it is reported as built at run time. +JSON-schema ``pattern`` keywords run inside jsonschema, out of this scan's sight: +``schema_manager.schema_validator`` reads their ``$`` as ECMA-262 does, and a +validation that does not go through it is a separate gap. +""" + +from __future__ import annotations + +import ast +import pathlib +import re +import textwrap +import warnings +from collections import Counter +from dataclasses import dataclass, field +from typing import ( + Callable, + Dict, + Iterator, + List, + NamedTuple, + Optional, + Sequence, + Set, + Tuple, + Union, +) + +import pytest + +try: # Python 3.11+: the parser ``re`` itself uses + _sre = re._constants + _sre_parse = re._parser +except AttributeError: # Python 3.10, where ``re`` is one module + import sre_constants as _sre # type: ignore[no-redef] + import sre_parse as _sre_parse # type: ignore[no-redef] + +pytestmark = [pytest.mark.unit] + +_PACKAGE = pathlib.Path(__file__).resolve().parents[2] / "fluid_build" + +#: The calls that leave a final newline unchecked when the pattern ends in ``$``. +_MATCHERS = frozenset({"match", "search"}) + +#: The modules whose ``match`` / ``search`` / ``compile`` the scan reads. +_REGEX_MODULES = frozenset({"re", "regex"}) + +#: At most this many values are kept per expression, so a long ``+`` chain stays cheap. +_MAX_VALUES = 32 + +#: At most this much of a string's start and of its end is kept. +_MAX_TEXT = 4096 + +#: Calls that hand back what they are given: ``tuple([...])``, ``MappingProxyType({...})``. +_PASS_THROUGH = frozenset( + {"dict", "frozenset", "list", "MappingProxyType", "reversed", "set", "sorted", "tuple"} +) + +_NEWLINE = ord("\n") +#: ``\s``, ``\D`` and ``\W`` contain ``\n``. +_CLASSES_WITH_NEWLINE = frozenset( + {_sre.CATEGORY_SPACE, _sre.CATEGORY_NOT_DIGIT, _sre.CATEGORY_NOT_WORD} +) +_REPEATS = frozenset( + {_sre.MAX_REPEAT, _sre.MIN_REPEAT, getattr(_sre, "POSSESSIVE_REPEAT", _sre.MAX_REPEAT)} +) +_ATOMIC_GROUP = getattr(_sre, "ATOMIC_GROUP", None) + + +# ── values ────────────────────────────────────────────────────────────── + + +@dataclass(frozen=True) +class _Str: + """A string as far as the scan can read it: a known prefix and suffix, or all of it. + + ``head`` / ``tail`` are ``None`` when nothing is known of that end.""" + + head: Optional[str] + tail: Optional[str] + whole: bool + origin: str = field(compare=False) + + @classmethod + def of(cls, text: str, origin: str) -> "_Str": + return cls(text, text, True, origin) + + def show(self) -> str: + if self.whole: + return self.head or "" + return f"{self.head or ''}…{self.tail or ''}" + + +@dataclass(frozen=True) +class _Pattern: + """A regex: the argument of ``re.compile``, or the pattern given to ``re.match``.""" + + source: Optional[_Str] # None: built at run time from a value the scan cannot read + flags: Optional[int] # None: the flags argument cannot be read + what: str = field(compare=False) # source text of the pattern argument + origin: str = field(compare=False) + + +@dataclass(frozen=True) +class _Passed: + """A parameter annotated ``re.Pattern``: the caller chooses the pattern.""" + + name: str + origin: str = field(compare=False) + + +@dataclass(frozen=True) +class _Seq: + """A tuple, list or set display, one value set per position.""" + + items: Tuple[Tuple["_Value", ...], ...] + + +@dataclass(frozen=True) +class _Map: + """A dict display. A key is ``None`` when it is not a string literal.""" + + entries: Tuple[Tuple[Optional[str], Tuple["_Value", ...]], ...] + + +@dataclass(frozen=True, eq=False) +class _ModuleRef: + name: str + + +@dataclass(frozen=True, eq=False) +class _Function: + node: Union[ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda] + scope: "_Scope" # the function's own scope + + +@dataclass(frozen=True, eq=False) +class _Class: + scope: "_Scope" # the class body + + +@dataclass(frozen=True, eq=False) +class _Instance: + scope: "_Scope" # the class body + + +_Value = Union[_Str, _Pattern, _Passed, _Seq, _Map, _ModuleRef, _Function, _Class, _Instance] + + +class _Binding(NamedTuple): + """One way a name gets its value, evaluated only when the name is read.""" + + expr: Optional[ast.expr] = None + scope: Optional["_Scope"] = None + iterate: bool = False # bound by a ``for``: the value is an element of ``expr`` + path: Tuple[int, ...] = () # the position inside a tuple target + value: Optional[_Value] = None # known when the binding is made (def, class) + imported: Optional[Tuple[str, str]] = None # (module, name) of a ``from`` import + + +class _Scope: + """The names one module, class, function or comprehension binds.""" + + def __init__(self, module: "_Module", kind: str, parent: Optional["_Scope"]) -> None: + self.module = module + self.kind = kind + self.parent = parent # the next scope a name is looked up in (classes are skipped) + self.bindings: Dict[str, List[_Binding]] = {} + self.params: Dict[str, Tuple[ast.arg, Optional[ast.expr], "_Scope"]] = {} + self.globals: Set[str] = set() + self.self_name: Optional[str] = None # a method's first parameter + self.cls: Optional["_Scope"] = None # a method's class body + self.attrs: Dict[str, List[_Binding]] = {} # a class's ``self.X = ...`` assignments + self.bases: List[Tuple[ast.expr, "_Scope"]] = [] # a class's base classes + + def bind(self, name: str, binding: _Binding) -> None: + self.bindings.setdefault(name, []).append(binding) + + +class _Module: + def __init__(self, name: str, path: pathlib.Path) -> None: + self.name = name + self.path = path + #: Held for the whole scan: the scan keys nodes by ``id()``, which a freed node gives up. + self.tree: Optional[ast.Module] = None + self.scope = _Scope(self, "module", None) + #: Every ``.match`` / ``.search`` call or reference: (node, scope, function name). + self.calls: List[Tuple[ast.expr, _Scope, str]] = [] + #: id(node) -> the scope of a comprehension or lambda. + self.scopes: Dict[int, _Scope] = {} + + +class Finding(NamedTuple): + path: str # relative to the checkout, e.g. ``fluid_build/cli/security.py`` + function: str # the qualified name of the function that makes the call + pattern: str # the pattern text, or ```` / ```` + line: int + call: str + origin: str # where the pattern was written or compiled + + @property + def key(self) -> Tuple[str, str, str]: + return (self.path, self.function, self.pattern) + + def show(self) -> str: + return ( + f"{self.path}:{self.line} in {self.function}: {self.call}\n" + f" pattern {self.pattern!r}, from {self.origin}" + ) + + +# ── reading a module ──────────────────────────────────────────────────── + + +class _Builder(ast.NodeVisitor): + """Record every binding and every ``.match`` / ``.search`` call of one module.""" + + def __init__(self, module: _Module) -> None: + self.module = module + self.scope = module.scope + self.names: List[str] = [] + self._called: Set[int] = set() # id() of the ``x.match`` nodes that are called + + # scopes + + def _lookup_parent(self) -> _Scope: + """The scope a nested function looks names up in: Python skips class bodies.""" + scope = self.scope + while scope.kind == "class": + assert scope.parent is not None + scope = scope.parent + return scope + + def _function(self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda]) -> None: + outer = self.scope + for expr in getattr(node, "decorator_list", []): + self.visit(expr) + args = node.args + for expr in args.defaults + [d for d in args.kw_defaults if d is not None]: + self.visit(expr) + inner = _Scope(self.module, "function", self._lookup_parent()) + positional = args.posonlyargs + args.args + defaults: List[Optional[ast.expr]] = [None] * (len(positional) - len(args.defaults)) + defaults += list(args.defaults) + for arg, default in zip(positional, defaults, strict=True): + inner.params[arg.arg] = (arg, default, outer) + for arg, default in zip(args.kwonlyargs, args.kw_defaults, strict=True): + inner.params[arg.arg] = (arg, default, outer) + for arg in (args.vararg, args.kwarg): + if arg is not None: + inner.params[arg.arg] = (arg, None, outer) + if outer.kind == "class" and positional and not _decorated(node, "staticmethod"): + inner.self_name = positional[0].arg + inner.cls = outer + if isinstance(node, ast.Lambda): + self.module.scopes[id(node)] = inner + else: + self._bind_name(node.name, _Binding(value=_Function(node, inner))) + self.scope = inner + self.names.append(getattr(node, "name", "")) + for stmt in node.body if isinstance(node.body, list) else [node.body]: + self.visit(stmt) + self.names.pop() + self.scope = outer + + def visit_FunctionDef(self, node: ast.FunctionDef) -> None: + self._function(node) + + def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None: + self._function(node) + + def visit_Lambda(self, node: ast.Lambda) -> None: + self._function(node) + + def visit_ClassDef(self, node: ast.ClassDef) -> None: + for expr in node.decorator_list + node.bases + [kw.value for kw in node.keywords]: + self.visit(expr) + outer = self.scope + body = _Scope(self.module, "class", self._lookup_parent()) + body.bases = [(base, outer) for base in node.bases] + self._bind_name(node.name, _Binding(value=_Class(body))) + self.scope = body + self.names.append(node.name) + for stmt in node.body: + self.visit(stmt) + self.names.pop() + self.scope = outer + + def _comprehension(self, node: ast.AST, generators: List[ast.comprehension]) -> None: + outer = self.scope + inner = _Scope(self.module, "function", self._lookup_parent()) + self.module.scopes[id(node)] = inner + for index, gen in enumerate(generators): + # The first iterable is evaluated outside the comprehension. + self.scope = outer if index == 0 else inner + self.visit(gen.iter) + self.scope = inner + self._bind_target(gen.target, gen.iter, outer if index == 0 else inner, True, ()) + for cond in gen.ifs: + self.visit(cond) + for child in ("elt", "key", "value"): + expr = getattr(node, child, None) + if expr is not None: + self.visit(expr) + self.scope = outer + + def visit_ListComp(self, node: ast.ListComp) -> None: + self._comprehension(node, node.generators) + + def visit_SetComp(self, node: ast.SetComp) -> None: + self._comprehension(node, node.generators) + + def visit_GeneratorExp(self, node: ast.GeneratorExp) -> None: + self._comprehension(node, node.generators) + + def visit_DictComp(self, node: ast.DictComp) -> None: + self._comprehension(node, node.generators) + + # bindings + + def _bind_name(self, name: str, binding: _Binding) -> None: + if name in self.scope.globals: + self.module.scope.bind(name, binding) + else: + self.scope.bind(name, binding) + + def _bind_target( + self, + target: ast.expr, + expr: ast.expr, + scope: _Scope, + iterate: bool, + path: Tuple[int, ...], + ) -> None: + if isinstance(target, ast.Name): + self._bind_name(target.id, _Binding(expr, scope, iterate, path)) + elif isinstance(target, (ast.Tuple, ast.List)): + for index, elt in enumerate(target.elts): + if not isinstance(elt, ast.Starred): + self._bind_target(elt, expr, scope, iterate, path + (index,)) + elif ( + isinstance(target, ast.Attribute) + and isinstance(target.value, ast.Name) + and target.value.id == self.scope.self_name + and self.scope.cls is not None + ): + binding = _Binding(expr, scope, iterate, path) + self.scope.cls.attrs.setdefault(target.attr, []).append(binding) + + def visit_Assign(self, node: ast.Assign) -> None: + self.visit(node.value) + for target in node.targets: + self._bind_target(target, node.value, self.scope, False, ()) + self.visit(target) + + def visit_AnnAssign(self, node: ast.AnnAssign) -> None: + if node.value is not None: + self.visit(node.value) + self._bind_target(node.target, node.value, self.scope, False, ()) + + def visit_NamedExpr(self, node: ast.NamedExpr) -> None: + self.visit(node.value) + self._bind_target(node.target, node.value, self.scope, False, ()) + + def visit_For(self, node: Union[ast.For, ast.AsyncFor]) -> None: + self.visit(node.iter) + self._bind_target(node.target, node.iter, self.scope, True, ()) + for stmt in node.body + node.orelse: + self.visit(stmt) + + visit_AsyncFor = visit_For + + def visit_Global(self, node: ast.Global) -> None: + self.scope.globals.update(node.names) + + def visit_Import(self, node: ast.Import) -> None: + for alias in node.names: + if alias.asname: + self._bind_name(alias.asname, _Binding(value=_ModuleRef(alias.name))) + else: + top = alias.name.split(".")[0] + self._bind_name(top, _Binding(value=_ModuleRef(top))) + + def visit_ImportFrom(self, node: ast.ImportFrom) -> None: + base = node.module or "" + if node.level: + package = self.module.name.split(".") + if self.module.path.name != "__init__.py": + package = package[:-1] + package = package[: len(package) - (node.level - 1)] + base = ".".join(package + ([base] if base else [])) + for alias in node.names: + if alias.name != "*": + binding = _Binding(imported=(base, alias.name)) + self._bind_name(alias.asname or alias.name, binding) + + # calls + + def _record(self, node: ast.expr) -> None: + self.module.calls.append((node, self.scope, ".".join(self.names) or "")) + + def visit_Call(self, node: ast.Call) -> None: + if isinstance(node.func, ast.Attribute) and node.func.attr in _MATCHERS: + self._record(node) + self._called.add(id(node.func)) + self.generic_visit(node) + + def visit_Attribute(self, node: ast.Attribute) -> None: + loaded = isinstance(node.ctx, ast.Load) + if node.attr in _MATCHERS and loaded and id(node) not in self._called: + self._record(node) # a reference, such as ``filter(RX.match, names)`` + self.generic_visit(node) + + +# ── following a value back ────────────────────────────────────────────── + + +def _dedupe(values: Sequence[_Value]) -> List[_Value]: + out: List[_Value] = [] + for value in values: + if value not in out: + out.append(value) + if len(out) >= _MAX_VALUES: + break + return out + + +def _concat(left: _Str, right: _Str, origin: str) -> _Str: + """``left + right``, keeping what is known of the start and of the end.""" + if left.whole and right.whole: + text = f"{left.head}{right.head}" + if len(text) <= _MAX_TEXT: + return _Str.of(text, origin) + return _Str(text[:_MAX_TEXT], text[-_MAX_TEXT:], False, origin) + head, tail = left.head, right.tail + if left.whole: + head = f"{left.head}{right.head}" if right.head is not None else left.head or None + if right.whole: + tail = f"{left.tail}{right.tail}" if left.tail is not None else right.tail or None + return _Str( + head[:_MAX_TEXT] if head is not None else None, + tail[-_MAX_TEXT:] if tail is not None else None, + False, + origin, + ) + + +def _is_regex_module(values: Sequence[_Value]) -> bool: + return any(isinstance(v, _ModuleRef) and v.name in _REGEX_MODULES for v in values) + + +def _decorated(node: ast.AST, *names: str) -> bool: + """Whether ``node`` carries a decorator called one of ``names`` (``functools.x`` too).""" + for decorator in getattr(node, "decorator_list", []): + if isinstance(decorator, ast.Call): + decorator = decorator.func + if getattr(decorator, "id", None) in names or getattr(decorator, "attr", None) in names: + return True + return False + + +def _keyword(call: ast.Call, name: str) -> Optional[ast.expr]: + return next((kw.value for kw in call.keywords if kw.arg == name), None) + + +def _own_returns(node: ast.AST) -> Iterator[ast.Return]: + """The ``return`` statements of a function, not of the functions nested in it.""" + stack = list(ast.iter_child_nodes(node)) + while stack: + child = stack.pop() + if isinstance(child, ast.Return) and child.value is not None: + yield child + elif not isinstance( + child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef) + ): + stack.extend(ast.iter_child_nodes(child)) + + +class _Scan: + def __init__(self, package: pathlib.Path) -> None: + self.package = package + self.checkout = package.parent + self._modules: Dict[str, Optional[_Module]] = {} + self._active: Set[Tuple[int, str]] = set() + self._memo: Dict[Tuple[int, str], List[_Value]] = {} + self._cut = False # a cycle guard cut the evaluation in progress short + self.calls_seen = 0 + self.unreadable: List[str] = [] + + # modules + + def _rel(self, path: pathlib.Path) -> str: + return path.relative_to(self.checkout).as_posix() + + def _origin(self, scope: _Scope, node: ast.AST) -> str: + return f"{self._rel(scope.module.path)}:{getattr(node, 'lineno', 0)}" + + def _path_of(self, name: str) -> Optional[pathlib.Path]: + if name != self.package.name and not name.startswith(self.package.name + "."): + return None + rel = pathlib.Path(*name.split(".")) + for candidate in ( + self.checkout / rel.with_suffix(".py"), + self.checkout / rel / "__init__.py", + ): + if candidate.is_file(): + return candidate + return None + + def module(self, name: str) -> Optional[_Module]: + if name not in self._modules: + path = self._path_of(name) + self._modules[name] = self._load(name, path) if path else None + return self._modules[name] + + def _load(self, name: str, path: pathlib.Path) -> Optional[_Module]: + module = _Module(name, path) + try: + with warnings.catch_warnings(): + # An invalid escape sequence is the linter's business, not this gate's. + warnings.simplefilter("ignore") + module.tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + _Builder(module).visit(module.tree) + except (SyntaxError, UnicodeDecodeError, ValueError, RecursionError) as exc: + self.unreadable.append(f"{self._rel(path)}: {type(exc).__name__}: {exc}") + return None + return module + + # names + + def _guarded(self, key: Tuple[int, str], compute: Callable[[], List[_Value]]) -> List[_Value]: + """``compute()`` once per key. A key met again while it is being computed reads as + nothing, and a result cut short that way is not kept. Callers get a copy.""" + if key in self._memo: + return list(self._memo[key]) + if key in self._active: + self._cut = True + return [] + outer_cut, self._cut = self._cut, False + self._active.add(key) + try: + result = compute() + if not self._cut: + self._memo[key] = result + return list(result) + finally: + self._active.discard(key) + self._cut = outer_cut or self._cut + + def _name(self, name: str, scope: _Scope) -> List[_Value]: + current: Optional[_Scope] = scope + while current is not None: + if name in current.params or name in current.bindings: + return self._bound(current, name) + current = current.parent + return [] + + def _bound(self, scope: _Scope, name: str) -> List[_Value]: + return self._guarded((id(scope), name), lambda: self._bound_now(scope, name)) + + def _bound_now(self, scope: _Scope, name: str) -> List[_Value]: + out: List[_Value] = [] + if name in scope.params: + arg, default, outer = scope.params[name] + if name == scope.self_name and scope.cls is not None: + return [_Instance(scope.cls)] + if arg.annotation is not None and self._names_pattern(arg.annotation, outer): + out.append(_Passed(name, self._origin(scope, arg))) + out += self.values(default, outer) + for binding in scope.bindings.get(name, []): + out += self._evaluate(binding) + return _dedupe(out) + + def _names_pattern(self, annotation: ast.expr, scope: _Scope) -> bool: + """Whether an annotation names ``re.Pattern`` / ``typing.Pattern``, or an alias of it.""" + if isinstance(annotation, ast.Constant) and isinstance(annotation.value, str): + try: + annotation = ast.parse(annotation.value, mode="eval").body + except SyntaxError: + return False + for node in ast.walk(annotation): + if isinstance(node, ast.Attribute) and node.attr == "Pattern": + return True + if isinstance(node, ast.Name) and ( + node.id == "Pattern" or self._imports_pattern(node.id, scope) + ): + return True + return False + + @staticmethod + def _imports_pattern(name: str, scope: _Scope) -> bool: + current: Optional[_Scope] = scope + while current is not None and name not in current.bindings: + current = current.parent + bindings = current.bindings[name] if current is not None else [] + return any(b.imported is not None and b.imported[1] == "Pattern" for b in bindings) + + def _evaluate(self, binding: _Binding) -> List[_Value]: + if binding.value is not None: + return [binding.value] + if binding.imported is not None: + return self._module_attr(*binding.imported) + values = self.values(binding.expr, binding.scope) + if binding.iterate: + values = self._elements(values) + for index in binding.path: + values = [ + item + for value in values + if isinstance(value, _Seq) and index < len(value.items) + for item in value.items[index] + ] + return values + + def _module_attr(self, module_name: str, attr: str) -> List[_Value]: + """``module.attr``: what the module binds, or else its submodule of that name.""" + module = self.module(module_name) + out: List[_Value] = [] + if module is not None and attr in module.scope.bindings: + out = self._bound(module.scope, attr) + if not out and self._path_of(f"{module_name}.{attr}") is not None: + # Also the case for ``from . import sub`` in a package's own ``__init__``. + out = [_ModuleRef(f"{module_name}.{attr}")] + return out + + def _class_attr(self, body: _Scope, attr: str) -> List[_Value]: + return self._guarded((id(body), f".{attr}"), lambda: self._class_attr_now(body, attr)) + + def _class_attr_now(self, body: _Scope, attr: str) -> List[_Value]: + out: List[_Value] = [] + if attr in body.bindings: + out += self._bound(body, attr) + for binding in body.attrs.get(attr, []): + out += self._evaluate(binding) + for base, scope in body.bases: + if isinstance(base, ast.Subscript): # a generic base, ``Base[int]`` + base = base.value + for value in self.values(base, scope): + if isinstance(value, _Class): + out += self._class_attr(value.scope, attr) + return _dedupe(out) + + def _attribute(self, bases: Sequence[_Value], attr: str) -> List[_Value]: + out: List[_Value] = [] + for base in bases: + if isinstance(base, _ModuleRef): + out += self._module_attr(base.name, attr) + elif isinstance(base, _Pattern) and attr == "pattern" and base.source is not None: + out.append(base.source) + elif isinstance(base, _Instance) and attr == "__class__": + out.append(_Class(base.scope)) + elif isinstance(base, (_Class, _Instance)): + for value in self._class_attr(base.scope, attr): + is_property = isinstance(value, _Function) and _decorated( + value.node, "property", "cached_property" + ) + if is_property and isinstance(base, _Instance): + out += self._returns(value) + else: + out.append(value) + return _dedupe(out) + + @staticmethod + def _elements(values: Sequence[_Value]) -> List[_Value]: + return _dedupe( + [item for v in values if isinstance(v, _Seq) for pos in v.items for item in pos] + ) + + # expressions: one reader per node type, dispatched the way ast.NodeVisitor does + + def values(self, expr: Optional[ast.expr], scope: Optional[_Scope]) -> List[_Value]: + """Every value ``expr`` can have that the scan can read, flow-insensitively.""" + if expr is None or scope is None: + return [] + reader = getattr(self, f"_read_{type(expr).__name__}", None) + return reader(expr, scope) if reader is not None else [] + + def _read_Constant(self, expr: ast.Constant, scope: _Scope) -> List[_Value]: + value = expr.value + if isinstance(value, bytes): + value = value.decode("latin-1") + return [_Str.of(value, self._origin(scope, expr))] if isinstance(value, str) else [] + + def _read_JoinedStr(self, expr: ast.JoinedStr, scope: _Scope) -> List[_Value]: + origin = self._origin(scope, expr) + result: Optional[_Str] = None + for part in expr.values: + if isinstance(part, ast.Constant) and isinstance(part.value, str): + piece = _Str.of(part.value, origin) + else: + inner: List[_Str] = [] + if ( + isinstance(part, ast.FormattedValue) + and part.format_spec is None + and part.conversion == -1 + ): + inner = [v for v in self.values(part.value, scope) if isinstance(v, _Str)] + piece = inner[0] if len(inner) == 1 else _Str(None, None, False, origin) + result = piece if result is None else _concat(result, piece, origin) + return [result if result is not None else _Str.of("", origin)] + + def _read_BinOp(self, expr: ast.BinOp, scope: _Scope) -> List[_Value]: + if not isinstance(expr.op, ast.Add): + return [] + origin = self._origin(scope, expr) + lefts = [v for v in self.values(expr.left, scope) if isinstance(v, _Str)] + rights = [v for v in self.values(expr.right, scope) if isinstance(v, _Str)] + return _dedupe([_concat(a, b, origin) for a in lefts for b in rights]) + + def _read_Name(self, expr: ast.Name, scope: _Scope) -> List[_Value]: + return self._name(expr.id, scope) + + def _read_Attribute(self, expr: ast.Attribute, scope: _Scope) -> List[_Value]: + return self._attribute(self.values(expr.value, scope), expr.attr) + + def _read_Subscript(self, expr: ast.Subscript, scope: _Scope) -> List[_Value]: + index = expr.slice + key = index.value if isinstance(index, ast.Constant) else None + out: List[_Value] = [] + for base in self.values(expr.value, scope): + if isinstance(base, _Map): + out += self._lookup(base, key if isinstance(key, str) else None) + elif isinstance(base, _Seq): + known = isinstance(key, int) and -len(base.items) <= key < len(base.items) + if known and len(base.items) > 1: + out += base.items[key] + else: + out += [v for pos in base.items for v in pos] + return _dedupe(out) + + @staticmethod + def _lookup(mapping: _Map, key: Optional[str]) -> List[_Value]: + hits = [vs for k, vs in mapping.entries if key is not None and k == key] + chosen = hits if hits else [vs for _, vs in mapping.entries] + return _dedupe([v for vs in chosen for v in vs]) + + def _read_Dict(self, expr: ast.Dict, scope: _Scope) -> List[_Value]: + entries: List[Tuple[Optional[str], Tuple[_Value, ...]]] = [] + for key, value in zip(expr.keys, expr.values, strict=True): + if key is None: # ``**other`` + entries += [ + e for m in self.values(value, scope) if isinstance(m, _Map) for e in m.entries + ] + continue + text = key.value if isinstance(key, ast.Constant) else None + entries.append( + (text if isinstance(text, str) else None, tuple(self.values(value, scope))) + ) + return [_Map(tuple(entries))] + + def _read_Tuple(self, expr: Union[ast.Tuple, ast.List, ast.Set], scope: _Scope) -> List[_Value]: + items: List[Tuple[_Value, ...]] = [] + for elt in expr.elts: + if isinstance(elt, ast.Starred): + items.append(tuple(self._elements(self.values(elt.value, scope)))) + else: + items.append(tuple(self.values(elt, scope))) + keeps_positions = isinstance(expr, ast.Tuple) and not any( + isinstance(elt, ast.Starred) for elt in expr.elts + ) + if not keeps_positions: + items = [tuple(_dedupe([v for pos in items for v in pos]))] + return [_Seq(tuple(items))] + + _read_List = _read_Set = _read_Tuple + + def _read_ListComp( + self, expr: Union[ast.ListComp, ast.SetComp, ast.GeneratorExp], scope: _Scope + ) -> List[_Value]: + inner = scope.module.scopes.get(id(expr)) + return [_Seq((tuple(self.values(expr.elt, inner)),))] if inner else [] + + _read_SetComp = _read_GeneratorExp = _read_ListComp + + def _read_DictComp(self, expr: ast.DictComp, scope: _Scope) -> List[_Value]: + inner = scope.module.scopes.get(id(expr)) + return [_Map(((None, tuple(self.values(expr.value, inner))),))] if inner else [] + + def _read_Lambda(self, expr: ast.Lambda, scope: _Scope) -> List[_Value]: + inner = scope.module.scopes.get(id(expr)) + return [_Function(expr, inner)] if inner else [] + + def _read_IfExp(self, expr: ast.IfExp, scope: _Scope) -> List[_Value]: + return _dedupe(self.values(expr.body, scope) + self.values(expr.orelse, scope)) + + def _read_BoolOp(self, expr: ast.BoolOp, scope: _Scope) -> List[_Value]: + return _dedupe([v for operand in expr.values for v in self.values(operand, scope)]) + + def _read_NamedExpr(self, expr: ast.NamedExpr, scope: _Scope) -> List[_Value]: + return self.values(expr.value, scope) + + # calls + + def _read_Call(self, expr: ast.Call, scope: _Scope) -> List[_Value]: + func = expr.func + if isinstance(func, ast.Attribute): + bases = self.values(func.value, scope) + if _is_regex_module(bases): + return self._regex_call(func.attr, expr, scope) + known = self._method(func.attr, bases, expr, scope) + if known is not None: + return known + # Read the callee from ``bases``: reading ``func`` again would evaluate the + # receiver twice, which doubles the work at every call in a chain. + callees, name = self._attribute(bases, func.attr), func.attr + else: + callees, name = self.values(func, scope), getattr(func, "id", "") + if not callees: + return self._builtin(name, expr, scope) + out: List[_Value] = [] + for callee in callees: + if isinstance(callee, _Function): + out += self._returns(callee) + elif isinstance(callee, _Class): + out.append(_Instance(callee.scope)) + return _dedupe(out) + + def _regex_call(self, attr: str, expr: ast.Call, scope: _Scope) -> List[_Value]: + if attr == "compile": + return self._compiled(expr, scope, flags_at=1) + if attr == "escape": + # An escaped string anchors nothing: its ``$`` comes out as ``\$``. + return [_Str("", "", False, self._origin(scope, expr))] + return [] + + def _method( + self, attr: str, bases: Sequence[_Value], expr: ast.Call, scope: _Scope + ) -> Optional[List[_Value]]: + """``str.join``, and the dict methods that hand back what the dict holds.""" + separators = [b for b in bases if isinstance(b, _Str)] + if attr == "join" and separators and expr.args: + parts = self._elements(self.values(expr.args[0], scope)) + return self._joined(separators, parts, self._origin(scope, expr)) + maps = [b for b in bases if isinstance(b, _Map)] + if not maps: + return None + if attr == "get" and expr.args: + key = expr.args[0] + text = key.value if isinstance(key, ast.Constant) else None + out = [ + v for m in maps for v in self._lookup(m, text if isinstance(text, str) else None) + ] + return _dedupe(out + self.values(expr.args[1] if len(expr.args) > 1 else None, scope)) + if attr == "values": + return [_Seq((tuple(_dedupe([v for m in maps for _, vs in m.entries for v in vs])),))] + if attr == "items": + origin = self._origin(scope, expr) + pairs = [ + _Seq(((_Str.of(k, origin),) if k else (), vs)) for m in maps for k, vs in m.entries + ] + return [_Seq((tuple(pairs),))] + return None + + @staticmethod + def _joined(separators: List[_Str], parts: List[_Value], origin: str) -> List[_Value]: + """``sep.join(parts)``. Joined by ``|``, the parts are the alternatives of one + pattern, so each is read on its own. Otherwise the result starts and ends the + way the parts do, where they agree.""" + strings = [p for p in parts if isinstance(p, _Str)] + if not strings or len(strings) != len(parts): + return [] + if all(s.whole and s.head == "|" for s in separators): + return list(strings) + heads = {s.head for s in strings} + tails = {s.tail for s in strings} + head = heads.pop() if len(heads) == 1 else None + tail = tails.pop() if len(tails) == 1 else None + return [_Str(head, tail, False, origin)] + + def _builtin(self, name: str, expr: ast.Call, scope: _Scope) -> List[_Value]: + """The builtins (and ``MappingProxyType``) that pass a container through.""" + first = expr.args[0] if expr.args else None + if name == "type" and first is not None: + return [_Class(v.scope) for v in self.values(first, scope) if isinstance(v, _Instance)] + if name == "enumerate" and first is not None: + elements = self._elements(self.values(first, scope)) + return [_Seq((tuple(_Seq(((), (e,))) for e in elements),))] + if name not in _PASS_THROUGH: + return [] + out = self.values(first, scope) + if name == "dict" and expr.keywords: + keywords = [(kw.arg, tuple(self.values(kw.value, scope))) for kw in expr.keywords] + out.append(_Map(tuple(keywords))) + return out + + def _compiled(self, expr: ast.Call, scope: _Scope, flags_at: int) -> List[_Value]: + """The pattern(s) ``re.compile`` / ``re.match`` / ``re.search`` is given.""" + pattern = expr.args[0] if expr.args else _keyword(expr, "pattern") + flags_expr = expr.args[flags_at] if len(expr.args) > flags_at else _keyword(expr, "flags") + flags = 0 if flags_expr is None else self._flags(flags_expr, scope) + what = ast.unparse(pattern) if pattern is not None else "?" + origin = self._origin(scope, expr) + out: List[_Value] = [] + for value in self.values(pattern, scope): + if isinstance(value, (_Pattern, _Passed)): + out.append(value) + elif isinstance(value, _Str) and value.tail is not None: + out.append(_Pattern(value, flags, what, value.origin)) + elif isinstance(value, _Str): + # A string whose end the scan cannot read counts as built at run time. + out.append(_Pattern(None, flags, what, origin)) + if not out: + out.append(_Pattern(None, flags, what, origin)) + return _dedupe(out) + + def _flags(self, expr: ast.expr, scope: _Scope) -> Optional[int]: + if isinstance(expr, ast.Constant) and isinstance(expr.value, int): + return expr.value + if isinstance(expr, ast.BinOp) and isinstance(expr.op, ast.BitOr): + left, right = self._flags(expr.left, scope), self._flags(expr.right, scope) + return None if left is None or right is None else left | right + if isinstance(expr, ast.Attribute) and _is_regex_module(self.values(expr.value, scope)): + flag = getattr(re, expr.attr, None) + return int(flag) if isinstance(flag, int) else None + if isinstance(expr, ast.Name): + current: Optional[_Scope] = scope + while current is not None and expr.id not in current.bindings: + current = current.parent + key = (id(current), f"") + if current is None or key in self._active: + return None + self._active.add(key) + try: + found = { + self._flags(b.expr, b.scope) + for b in current.bindings[expr.id] + if b.expr is not None and b.scope is not None and not b.path + } + finally: + self._active.discard(key) + if len(found) == 1: + return found.pop() + return None + + def _returns(self, function: _Function) -> List[_Value]: + key = (id(function.node), "") + return self._guarded(key, lambda: self._returns_now(function)) + + def _returns_now(self, function: _Function) -> List[_Value]: + node = function.node + if isinstance(node, ast.Lambda): + return self.values(node.body, function.scope) + out: List[_Value] = [] + for ret in _own_returns(node): + out += self.values(ret.value, function.scope) + return _dedupe(out) + + # the check + + def findings(self) -> List[Finding]: + out: List[Finding] = [] + for path in sorted(self.package.rglob("*.py")): + if "__pycache__" in path.parts: + continue + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + self.unreadable.append(f"{self._rel(path)}: {type(exc).__name__}: {exc}") + continue + if ".match" not in text and ".search" not in text: + continue + rel = path.relative_to(self.checkout).with_suffix("") + parts = list(rel.parts[:-1]) if rel.name == "__init__" else list(rel.parts) + module = self.module(".".join(parts)) + if module is None: + continue + for node, scope, function in module.calls: + out += self._check(module, node, scope, function) + return sorted(set(out)) + + def _check( + self, module: _Module, node: ast.expr, scope: _Scope, function: str + ) -> List[Finding]: + func = node.func if isinstance(node, ast.Call) else node + assert isinstance(func, ast.Attribute) + rel = self._rel(module.path) + try: + bases = self.values(func.value, scope) + if not _is_regex_module(bases): + patterns = [b for b in bases if isinstance(b, (_Pattern, _Passed))] + elif isinstance(node, ast.Call): + patterns = self._compiled(node, scope, flags_at=2) + else: + patterns = [] # ``re.match`` handed on: its pattern is given elsewhere + except RecursionError: + self.unreadable.append(f"{rel}:{node.lineno}: too deeply nested to follow") + return [] + if patterns: + self.calls_seen += 1 + shown = ast.unparse(node) + if len(shown) > 100: + shown = shown[:97] + "..." + out = [] + for pattern in patterns: + label = _gap(pattern, func.attr) + if label is not None: + out.append(Finding(rel, function, label, node.lineno, shown, pattern.origin)) + return out + + +# ── the anchor rule ───────────────────────────────────────────────────── +# +# Items come from ``re``'s own parser: ``(opcode, argument)`` pairs, where a group, +# a repeat or an alternation holds parsed sub-patterns of its own. + + +def _matches_newline(item: Tuple[object, object], flags: int) -> bool: + """Whether one parsed character item (``.``, ``\\n``, ``[^/]``, ``\\s``) matches ``\\n``.""" + op, av = item + if op is _sre.ANY: + return bool(flags & re.DOTALL) + if op is _sre.LITERAL: + return av == _NEWLINE + if op is _sre.NOT_LITERAL: + return av != _NEWLINE + if op is _sre.IN: + negated = any(sub_op is _sre.NEGATE for sub_op, _ in av) + hit = any( + (sub_op is _sre.LITERAL and sub_av == _NEWLINE) + or (sub_op is _sre.RANGE and sub_av[0] <= _NEWLINE <= sub_av[1]) + or (sub_op is _sre.CATEGORY and sub_av in _CLASSES_WITH_NEWLINE) + for sub_op, sub_av in av + ) + return hit != negated + return False + + +def _absorbs_newline(items: List[Tuple[object, object]], flags: int) -> bool: + """True when the last item repeats, without bound, something that matches ``\\n``.""" + if not items: + return False + op, av = items[-1] + if op in _REPEATS: + _, high, sub = av + return ( + high == _sre.MAXREPEAT and len(sub.data) == 1 and _matches_newline(sub.data[0], flags) + ) + if op is _sre.SUBPATTERN: + _, add, delete, sub = av + return _absorbs_newline(sub.data, (flags | add) & ~delete) + if op is _sre.BRANCH: + return all(_absorbs_newline(branch.data, flags) for branch in av[1]) + return False + + +def _ends_at_dollar( + items: List[Tuple[object, object]], flags: int, before: Sequence[Tuple[object, object]] = () +) -> bool: + """True when a match can end at a ``$`` that lets a final newline through. + + ``before`` holds the items ahead of ``items`` in the enclosing sequence: what + precedes a ``$`` that opens a group or an alternative.""" + if not items: + return False + op, av = items[-1] + preceding = [*before, *items[:-1]] + if op is _sre.AT and av is _sre.AT_END: + return not flags & re.MULTILINE and not _absorbs_newline(preceding, flags) + if op is _sre.SUBPATTERN: + _, add, delete, sub = av + return _ends_at_dollar(sub.data, (flags | add) & ~delete, preceding) + if op is _sre.BRANCH: + return any(_ends_at_dollar(branch.data, flags, preceding) for branch in av[1]) + if op is _ATOMIC_GROUP: + return _ends_at_dollar(av.data, flags, preceding) + return False + + +def _anchored_at_start(items: List[Tuple[object, object]]) -> bool: + if not items: + return False + op, av = items[0] + if op is _sre.AT: + return av in (_sre.AT_BEGINNING, _sre.AT_BEGINNING_STRING) + if op is _sre.SUBPATTERN: + return _anchored_at_start(av[3].data) + if op is _sre.BRANCH: + return all(_anchored_at_start(branch.data) for branch in av[1]) + return False + + +def _alternatives( + items: List[Tuple[object, object]], flags: int +) -> Iterator[Tuple[List[Tuple[object, object]], int]]: + """The top-level alternatives, looking into a group that wraps the whole pattern.""" + if len(items) == 1: + op, av = items[0] + if op is _sre.BRANCH: + for branch in av[1]: + yield from _alternatives(branch.data, flags) + return + if op is _sre.SUBPATTERN: + _, add, delete, sub = av + yield from _alternatives(sub.data, (flags | add) & ~delete) + return + yield items, flags + + +def _gap(pattern: Union[_Pattern, _Passed], method: str) -> Optional[str]: + """The finding label when this pattern lets a final newline through ``method``.""" + if isinstance(pattern, _Passed): + return f"" + source = pattern.source + if source is None: + return f"" + # A string read only in part stands an inert group in for the part it cannot read. + text = source.head if source.whole else f"{source.head or ''}(?:x){source.tail or ''}" + try: + parsed = _sre_parse.parse(text or "", pattern.flags or 0) + except (re.error, OverflowError, RecursionError): + return source.show() if (source.tail or "").endswith("$") else None + for items, flags in _alternatives(parsed.data, parsed.state.flags): + anchored = method == "match" or _anchored_at_start(items) + if anchored and _ends_at_dollar(items, flags): + return source.show() + return None + + +# ── the gate ──────────────────────────────────────────────────────────── + +#: Calls whose ``$`` stays, keyed by (file, function, pattern), each with its reason. +_ACCEPTED: Dict[Tuple[str, str, str], str] = { + # The value cannot end in a newline when it is matched: it was stripped, split + # or rewritten just before, and that value is the one used afterwards. + ( + "fluid_build/__init__.py", + "_version_from_git_checkout", + r"(?P[^-]+)-(?P\d+)-g(?P[0-9a-f]+)$", + ): "stripped `git describe` output; only the captured tag and distance are used", + ( + "fluid_build/build_runners/_databricks_load.py", + "staging_volume", + r"^/Volumes/([^/]+)/([^/]+)/([^/]+)/?$", + ): "matches the stripped variable, and each captured part must then pass validate_ident", + ( + "fluid_build/build_runners/_late_arrival.py", + "parse_iso_duration", + r"^P(?:(?P\d+)D)?(?:T(?:(?P\d+)H)?(?:(?P\d+)M)?" + r"(?:(?P\d+(?:\.\d+)?)S)?)?$", + ): "matches text.strip(); the span is built from the captured numbers", + ( + "fluid_build/cli/_acquisition_stage_ext.py", + "_validate_cron", + r"^[0-9*/,\-?LW#]+$", + ): "each field comes from value.split()", + ( + "fluid_build/cli/forge_db_tools.py", + "_resolve_connection_uri", + r"^[A-Za-z0-9_]+$", + ): "matches the stripped connection name, which is the one used", + ( + "fluid_build/cli/import_workflow/dbt.py", + "_safe_tags", + r"^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$", + ): "every run of characters outside [a-z0-9] was replaced with '-' just before", + ( + "fluid_build/cli/provider_init.py", + "run", + r"^[a-z][a-z0-9_]*$", + ): "matches the stripped provider name, which is the one used", + ( + "fluid_build/cli/schedule_sync.py", + "_parse_commit_author", + r"^(.+?)\s*<([^<>@\s]+@[^<>@\s]+)>$", + ): "the author is stripped, and control characters (\\n too) are refused just before", + ( + "fluid_build/copilot/agents/ai_ready_agent.py", + "_normalize_allowed_models", + r"^[a-z0-9][a-z0-9-_.]*[a-z0-9]$|^[a-z0-9]$", + ): "the token was stripped and its whitespace replaced with '-' just before", + ( + "fluid_build/copilot/missions/spec.py", + "load_mission_spec_from_path", + r"^[a-z0-9][a-z0-9_-]*$", + ): "matches the stripped mission name, which is the one kept", + ( + "fluid_build/engines/dbt/_types.py", + "split_parameterized", + r"^([A-Za-z_][A-Za-z0-9_ ]*?)\s*\(\s*([^()]*)\s*\)$", + ): "matches fluid_type.strip() and returns the captured parts", + ( + "fluid_build/exporters/sodacl.py", + "_convert_dq_rule", + r"^[A-Za-z_][A-Za-z0-9_]*$", + ): "the selector is stripped where it is read", + ( + "fluid_build/forge/federation.py", + "_secret_ref_allowlist", + r"^(?:.*@)?(?:\[[^\]]+\]|[^:\[\]]+):\d+$", + ): "matches the stripped host, which is the one kept", + ( + "fluid_build/iac/provider_match.py", + "detect_cloud_declarations", + r"^[a-z]{2}-[a-z]+-\d+$", + ): "matches region.strip(), and a match only records that AWS is declared", + ( + "fluid_build/iac/providers/databricks_jobs.py", + "_notifications", + r"^[^@\s]+@[^@\s]+\.[^@\s]+$", + ): "the target is stripped where it is read", + ( + "fluid_build/iac/providers/databricks_jobs.py", + "_run_as", + r"^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$", + ): "the account is stripped where it is read", + ( + "fluid_build/iac/providers/databricks_jobs.py", + "_seconds", + r"^(\d+)\s*([smhd]?)$", + ): "the text is stripped; the seconds come from the captured numbers", + ( + "fluid_build/iac/providers/databricks_jobs.py", + "_seconds", + r"^PT(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?$", + ): "the text is stripped; the seconds come from the captured numbers", + ( + "fluid_build/iac/providers/databricks_retention.py", + "iso_duration_seconds", + r"^P(?!$)(?:(\d{1,12})Y)?(?:(\d{1,12})M)?(?:(\d{1,12})W)?(?:(\d{1,12})D)?" + r"(?:T(?=\d)(?:(\d{1,12})H)?(?:(\d{1,12})M)?(?:(\d{1,12})S)?)?$", + ): "matches text.strip(); the seconds come from the captured numbers", + ( + "fluid_build/llm/providers.py", + "_parse_param_size", + r"^([\d.]+)\s*([BM]?)$", + ): "the text is stripped; the size comes from the captured number", + ( + "fluid_build/providers/__init__.py", + "_is_valid_name", + r"^[a-z0-9_]+$", + ): "both callers pass _normalize_name() output, which is stripped", + ( + "fluid_build/providers/_path_safety.py", + "safe_filename_stem", + r"^[A-Za-z0-9_][A-Za-z0-9_.-]*[A-Za-z0-9_]$|^[A-Za-z0-9_]$", + ): "matches the stripped text and returns that", + ( + "fluid_build/providers/gcp/util/names.py", + "normalize_bucket_name", + r"^[a-z0-9][a-z0-9\-]*[a-z0-9]$", + ): "every character outside [a-z0-9-] was replaced with '-' just before", + ( + "fluid_build/providers/odcs/mappers/sla.py", + "_as_availability_pct", + r"^(100(\.0+)?|\d{2}(\.\d+)?|\d{1}\d(\.\d+)?)%$", + ): "matches stripped text, or a percentage formatted from a float", + ( + "fluid_build/providers/odcs/mappers/sla.py", + "_as_iso_duration", + r"^P(?!$)(\d+Y)?(\d+M)?(\d+W)?(\d+D)?(T(\d+H)?(\d+M)?(\d+S)?)?$", + ): "matches value.strip() and returns that", + ( + "fluid_build/providers/quality_engine.py", + "_parse_duration_seconds", + r"^(\d+)\s*([smhd])$", + ): "the text is stripped; the seconds come from the captured number", + ( + "fluid_build/providers/quality_engine.py", + "_parse_duration_seconds", + r"^P(\d+)W$", + ): "the text is stripped; the seconds come from the captured number", + ( + "fluid_build/providers/snowflake/plan/planner.py", + "_validate_policy_signature", + r"^\(\s*[A-Za-z_][A-Za-z0-9_]*\s+[A-Za-z_][A-Za-z0-9_(),\s]*\)" + r"(\s+RETURNS\s+[A-Za-z_][A-Za-z0-9_]*)?$", + ): "matches the stripped signature and returns that", + ( + "fluid_build/schema_manager.py", + "SchemaVersion.parse", + r"^(\d+)\.(\d+)(?:\.(\d+))?(?:-(.+))?$", + ): "matches version_str.strip() and keeps the captured parts", + ( + "fluid_build/util/cron.py", + "_values", + r"^(\*|[A-Za-z0-9]+(?:-[A-Za-z0-9]+)?)(?:/([0-9]+))?$", + ): "the expression's whitespace is collapsed and it is split into fields first", + # The value is another function's output, which cannot end in a newline. + ( + "fluid_build/iac/providers/aws.py", + "_hive_decimal", + r"^\((\d+)(?:,(\d+))?\)$", + ): "the only caller passes column_types.split_type() output, which holds no whitespace", + ( + "fluid_build/iac/providers/gcp.py", + "_bq_outcome", + r"^\((\d+)(?:,(\d+))?\)$", + ): "the value is column_types.split_type() output, which holds no whitespace", + ( + "fluid_build/iac/providers/databricks.py", + "_scale_zero_warnings", + r"^decimal(\([0-9]+\))?$", + ): "the value is dbt_types.sql_type() output", + ( + "fluid_build/iac/providers/databricks.py", + "_type_problem", + r"^[a-z_]+(<[a-z_]+>)?(\([0-9]{1,4}(,[0-9]{1,4})?\))?$", + ): "the only caller passes dbt_types.sql_type() output", + # Refusals and detectors: a match rejects or labels the value, so `$` errs on + # the safe side here, and \Z would loosen them. + ( + "fluid_build/build_runners/hooks/dlp_scan.py", + "_classify_value", + r"^[^\s@]+@[^\s@]+\.[^\s@]+$", + ): "a PII detector: a match labels the value as an email", + ( + "fluid_build/build_runners/hooks/dlp_scan.py", + "_classify_value", + r"^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$", + ): "a PII detector: a match labels the value as an IP address", + ( + "fluid_build/build_runners/hooks/dlp_scan.py", + "_classify_value", + r"^\d{3}-\d{2}-\d{4}$", + ): "a PII detector: a match labels the value as an SSN", + ( + "fluid_build/providers/aws/util/validation.py", + "ResourceValidator.validate_s3_bucket_name", + r"^\d+\.\d+\.\d+\.\d+$", + ): "refuses a bucket name shaped like an IP address", + ( + "fluid_build/providers/gcp/util/names.py", + "normalize_bucket_name", + r"^\d+\.\d+\.\d+\.\d+$", + ): "refuses a bucket name shaped like an IP address", + ( + "fluid_build/providers/gcp/util/names.py", + "validate_name", + r"^\d+\.\d+\.\d+\.\d+$", + ): "refuses a bucket name shaped like an IP address", + # Parsers: only the captured groups are used, and nothing is validated. + ( + "fluid_build/build_runners/_retention.py", + "parse_iso_duration", + r"^P(?!$)(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?)?$", + ): "returns a timedelta built from the captured numbers; every caller keeps that", + ( + "fluid_build/build_runners/duckdb/runner.py", + "_canonical_cursor_text", + r"(?P.*?)(?PZ|[+-]\d{2}:?(?:\d{2})?)$", + ): "rebuilds a cursor timestamp from its captured parts and refuses nothing", + ( + "fluid_build/cli/version_cmd.py", + "_bundled_schema_versions", + r"^fluid-schema-(\d+\.\d+(?:\.\d+)?)\.json$", + ): "reads the version out of a bundled schema's file name", + ( + "fluid_build/iac/providers/databricks.py", + "_canonical_type", + r"^decimal\(([0-9]{1,4})(?:,([0-9]{1,4}))?\)$", + ): "spells decimal(p) as decimal(p,0) from the captured digits and refuses nothing", + ( + "fluid_build/iac/providers/databricks_retention.py", + "_part_selects", + r"^(@)?(\d*\+)?(.*?)(\+\d*)?$", + ): "the selectors are split on whitespace first, and only the captured parts are used", + ( + "fluid_build/providers/_duckdb_sandbox.py", + "DuckDBAllowlist.with_locations", + r"^(?P[a-z][a-z0-9+.-]*)://(?P[^/?#]+)(?P/[^?#]*)?$", + ): "the host and path classes admit \\n, so the groups take a final newline under \\Z too", + ( + "fluid_build/providers/_duckdb_sandbox.py", + "_remote", + r"^(?P[a-z][a-z0-9+.-]*)://(?P[^/?#]+)(?P/[^?#]*)?$", + ): "the host and path classes admit \\n, so the groups take a final newline under \\Z too", + ( + "fluid_build/providers/_duckdb_sandbox.py", + "is_remote_location", + r"^(?P[a-z][a-z0-9+.-]*)://(?P[^/?#]+)(?P/[^?#]*)?$", + ): "the host and path classes admit \\n, so the groups take a final newline under \\Z too", + ( + "fluid_build/schema_manager.py", + "FluidSchemaManager._load_bundled_schemas", + r"^fluid-schema-(\d+\.\d+(?:\.\d+)?)\.json$", + ): "reads the version out of a bundled schema's file name", + ( + "fluid_build/schema_manager.py", + "_discover_bundled_versions", + r"^fluid-schema-(\d+\.\d+(?:\.\d+)?)\.json$", + ): "reads the version out of a bundled schema's file name", + ( + "fluid_build/schema_manager.py", + "_schema_version_sort_key", + r"^(\d+)\.(\d+)(?:\.(\d+))?(?:-(.+))?$", + ): "turns the captured numbers into a sort key", + # Judgment calls, and a known gap left for a follow-up. + ( + "fluid_build/providers/snowflake/util/names.py", + "quote_identifier", + r"^[A-Z_][A-Z0-9_]*$", + ): ( + "decides quoting and refuses nothing: under \\Z, ORDERS\\n would be quoted as the " + "object 'ORDERS\\n'. The identifier validators upstream are the guard" + ), + ( + "fluid_build/cli/import_workflow/dbt.py", + "_recency_window", + r"^P(?!$)(\d+Y)?(\d+M)?(\d+W)?(\d+D)?(T(\d+H)?(\d+M)?(\d+S)?)?$", + ): ( + "follow-up: returns fluid_window verbatim, and the duration parsers downstream " + "accept a final newline, so \\Z would drop the window. Strip it and return the " + "stripped value, as sla._as_iso_duration does" + ), + # Patterns read at run time. + ( + "fluid_build/build_runners/hooks/quality_gate.py", + "_check", + "", + ): ( + "follow-up: a contract's regex gate runs with re.match, so '^[A-Z]{3}$' passes " + "'ABC\\n'. The hook is not wired into a live chain yet (see its module note); " + "settle match against fullmatch before it is" + ), + ( + "fluid_build/cli/forge_copilot_contract_helpers.py", + "apply_overlay_validator_rules", + "", + ): "an overlay rule searches the serialized contract for text, not a whole value", + ( + "fluid_build/cli/forge_copilot_contract_helpers.py", + "apply_overlay_validator_rules", + "", + ): "an overlay rule searches the serialized contract for text, not a whole value", + ( + "fluid_build/schema_manager.py", + "_locate_error", + "", + ): ( + "mirrors jsonschema, which applies patternProperties with re.search; anything " + "else would point at a different key from the one the validator rejected" + ), + ( + "fluid_build/schema_manager.py", + "_pattern_keyword", + "", + ): ( + "jsonschema's pattern keyword: _end_anchored() turns every $ into \\Z, and .search " + "is what JSON Schema itself runs" + ), + ( + "fluid_build/schema_manager.py", + "_pattern_keyword", + "", + ): "only picks the error message, after the \\Z check above has refused the value", +} + +#: The entries that cover more than one call, with the number read; every other entry +#: covers one. So a new call with a listed pattern in a listed function is reported. +_ACCEPTED_CALLS: Dict[Tuple[str, str, str], int] = { + ( + "fluid_build/providers/odcs/mappers/sla.py", + "_as_availability_pct", + r"^(100(\.0+)?|\d{2}(\.\d+)?|\d{1}\d(\.\d+)?)%$", + ): 2, +} + + +@pytest.fixture(scope="module") +def tree() -> Tuple[_Scan, List[Finding]]: + scanner = _Scan(_PACKAGE) + return scanner, scanner.findings() + + +def test_the_scan_reads_every_module_and_follows_patterns( + tree: Tuple[_Scan, List[Finding]], +) -> None: + """A module the scan could not read, or a walk that followed no pattern, would let + the gate pass without checking anything.""" + scanner, _ = tree + assert scanner.unreadable == [], "the scan could not read:\n " + "\n ".join( + scanner.unreadable + ) + assert scanner.calls_seen > 150, f"only {scanner.calls_seen} calls had a pattern to follow" + + +def test_no_validator_lets_a_final_newline_through(tree: Tuple[_Scan, List[Finding]]) -> None: + _, found = tree + new = [f for f in found if f.key not in _ACCEPTED] + assert not new, ( + "These calls run a pattern that ends in `$` with .match or .search, so a value " + "with a final newline passes. End the pattern in \\Z or call fullmatch. If the " + "`$` is deliberate, add the call to _ACCEPTED with the reason:\n " + + "\n ".join(f.show() for f in new) + ) + + +def test_every_accepted_entry_still_matches_a_call(tree: Tuple[_Scan, List[Finding]]) -> None: + _, found = tree + keys = {f.key for f in found} + stale = sorted(key for key in _ACCEPTED if key not in keys) + assert not stale, "These _ACCEPTED entries match no call any more. Remove them:\n " + ( + "\n ".join(map(repr, stale)) + ) + + +def test_every_accepted_entry_covers_the_calls_it_was_read_for( + tree: Tuple[_Scan, List[Finding]], +) -> None: + """A second call with the same pattern in the same function was not read when the + entry was written, so it must not pass on the entry's reason.""" + _, found = tree + counts = Counter(f.key for f in found if f.key in _ACCEPTED) + changed = sorted(key for key, n in counts.items() if n != _ACCEPTED_CALLS.get(key, 1)) + assert not changed, ( + "These _ACCEPTED entries now cover a different number of calls. Read the new " + "call; if its `$` is deliberate too, set the count in _ACCEPTED_CALLS:\n " + + "\n ".join(f.show() for key in changed for f in found if f.key == key) + ) + + +def test_every_accepted_entry_gives_its_reason() -> None: + assert all(reason.strip() for reason in _ACCEPTED.values()) + assert set(_ACCEPTED_CALLS) <= set(_ACCEPTED) + + +def test_fluid_build_imports_re_as_a_module() -> None: + """The scan reads ``re.match(...)``, not a bare ``match(...)`` from ``from re import``.""" + offenders = [] + for path in sorted(_PACKAGE.rglob("*.py")): + text = path.read_text(encoding="utf-8") + if "from re import" not in text and "from regex import" not in text: + continue + for node in ast.walk(ast.parse(text)): + if isinstance(node, ast.ImportFrom) and node.module in _REGEX_MODULES: + offenders.append(f"{path.relative_to(_PACKAGE.parent)}:{node.lineno}") + assert not offenders, ( + "Import the regex module itself (`import re`), so this gate can read the calls, " + "or teach the scan the new form:\n " + "\n ".join(offenders) + ) + + +# ── the scan itself ───────────────────────────────────────────────────── + + +def _scan(tmp_path: pathlib.Path, **modules: str) -> List[str]: + """Scan a throwaway ``fluid_build`` package; one ``function: pattern`` line per report. + + A module name may name a subpackage: ``pkg__init__`` becomes ``pkg/__init__.py``.""" + package = tmp_path / "fluid_build" + package.mkdir() + (package / "__init__.py").write_text("", encoding="utf-8") + for name, source in modules.items(): + path = package / f"{name.replace('__init__', '/__init__')}.py" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(textwrap.dedent(source), encoding="utf-8") + scanner = _Scan(package) + found = scanner.findings() + assert scanner.unreadable == [] + return [f"{f.function}: {f.pattern}" for f in found] + + +@pytest.mark.parametrize( + "source, expected", + [ + pytest.param( + """ + import re + _NAME_RE = re.compile(r"^[a-z]+$") + + def check(value): + if not _NAME_RE.match(value): + raise ValueError(value) + """, + ["check: ^[a-z]+$"], + id="a-new-validator", + ), + pytest.param( + """ + import re + + def check(value): + return re.match(r"^[a-z]+$", value) + """, + ["check: ^[a-z]+$"], + id="re-match", + ), + pytest.param( + """ + import re as _re + + def check(value): + return _re.search(r"\\A[a-z]+$", value) + """, + [r"check: \A[a-z]+$"], + id="re-search-anchored", + ), + pytest.param( + """ + import regex + + def check(value): + return regex.match(r"^[a-z]+$", value) + """, + ["check: ^[a-z]+$"], + id="the-regex-module", + ), + pytest.param( + """ + import re + + def check(value): + return re.match(r"[a-z]+$", value) + """, + ["check: [a-z]+$"], + id="match-anchors-the-start-itself", + ), + pytest.param( + """ + import re + NAME = r"^[a-z]+$" + + def check(value): + return re.match(NAME, value) + """, + ["check: ^[a-z]+$"], + id="string-constant", + ), + pytest.param( + """ + import re + _PATTERNS = {"table": re.compile(r"^[a-z_]+$"), "project": re.compile(r"^[a-z-]+$")} + + def check(kind, value): + pattern = _PATTERNS.get(kind) + return pattern is not None and pattern.match(value) + """, + ["check: ^[a-z-]+$", "check: ^[a-z_]+$"], + id="dict-get", + ), + pytest.param( + """ + import re + _PATTERNS = {"table": re.compile(r"^[a-z_]+$"), "project": re.compile(r"^[a-z-]+\\Z")} + + def check(value): + return _PATTERNS["table"].match(value) + """, + ["check: ^[a-z_]+$"], + id="dict-key", + ), + pytest.param( + """ + import re + _RAW = {"table": r"^[a-z_]+$"} + _PATTERNS = {kind: re.compile(raw) for kind, raw in _RAW.items()} + + def check(kind, value): + return _PATTERNS[kind].match(value) + """, + ["check: ^[a-z_]+$"], + id="dict-comprehension", + ), + pytest.param( + """ + import re + from types import MappingProxyType + _PATTERNS = MappingProxyType({"table": re.compile(r"^[a-z_]+$")}) + + def check(kind, value): + return _PATTERNS[kind].match(value) + """, + ["check: ^[a-z_]+$"], + id="mapping-proxy", + ), + pytest.param( + """ + import re + _PATTERNS = dict(table=re.compile(r"^[a-z_]+$")) + + def check(kind, value): + return _PATTERNS[kind].match(value) + """, + ["check: ^[a-z_]+$"], + id="dict-keywords", + ), + pytest.param( + """ + import re + _DENY = (re.compile(r"^admin$"), re.compile(r"^root$")) + + def refused(value): + return any(rx.match(value) for rx in _DENY) + """, + ["refused: ^admin$", "refused: ^root$"], + id="tuple-in-a-comprehension", + ), + pytest.param( + """ + import re + _DENY = tuple([re.compile(r"^admin$")]) + + def refused(value): + for index, rx in enumerate(sorted(_DENY)): + if rx.match(value): + return index + """, + ["refused: ^admin$"], + id="tuple-sorted-enumerate", + ), + pytest.param( + """ + import re + _FIRST = (re.compile(r"^admin$"),) + _ALL = (*_FIRST, re.compile(r"^root\\Z")) + + def refused(value): + return _ALL[1].match(value) + """, + ["refused: ^admin$"], + id="starred-tuple", + ), + pytest.param( + """ + import re + _KINDS = {"email": re.compile(r"^[a-z]+@[a-z]+$")} + + def kinds(value): + for name, rx in _KINDS.items(): + if rx.match(value): + yield name + """, + ["kinds: ^[a-z]+@[a-z]+$"], + id="dict-items-loop", + ), + pytest.param( + """ + import functools + import re + + @functools.lru_cache(maxsize=None) + def _pattern(): + return re.compile(r"^[a-z]+$") + + def check(value): + return _pattern().match(value) + """, + ["check: ^[a-z]+$"], + id="helper-return", + ), + pytest.param( + """ + import re + _pattern = lambda: re.compile(r"^[a-z]+$") + + def check(value): + return _pattern().match(value) + """, + ["check: ^[a-z]+$"], + id="lambda-return", + ), + pytest.param( + """ + import re + + class Validator: + NAME = re.compile(r"^[a-z]+$") + + def check(self, value): + return self.NAME.match(value) + """, + ["Validator.check: ^[a-z]+$"], + id="class-attribute", + ), + pytest.param( + """ + import re + + class Validator: + def __init__(self): + self._name = re.compile(r"^[a-z]+$") + + def check(self, value): + return self._name.match(value) + """, + ["Validator.check: ^[a-z]+$"], + id="instance-attribute", + ), + pytest.param( + """ + import functools + import re + + class Validator: + @functools.cached_property + def name(self): + return re.compile(r"^[a-z]+$") + + def check(self, value): + return self.name.match(value) + """, + ["Validator.check: ^[a-z]+$"], + id="cached-property", + ), + pytest.param( + """ + import re + from typing import Generic, TypeVar + + T = TypeVar("T") + + class Base(Generic[T]): + NAME = re.compile(r"^[a-z]+$") + + class Child(Base[int]): + def check(self, value): + return self.NAME.match(value) or type(self).NAME.match(value) + """, + ["Child.check: ^[a-z]+$", "Child.check: ^[a-z]+$"], + id="generic-base-and-type-of-self", + ), + pytest.param( + """ + import re + + class Validator: + NAME = re.compile(r"^[a-z]+$") + + def check(self, value): + return self.__class__.NAME.match(value) + """, + ["Validator.check: ^[a-z]+$"], + id="dunder-class", + ), + pytest.param( + """ + import re + _NAME_RE = re.compile(r"^[a-z]+$") + _AGAIN_RE = re.compile(_NAME_RE.pattern, re.IGNORECASE) + + def check(value): + return _AGAIN_RE.match(value) + """, + ["check: ^[a-z]+$"], + id="pattern-attribute", + ), + pytest.param( + """ + import re + _NAME_RE = re.compile(r"^[a-z]+$") + + def names(values): + return list(filter(_NAME_RE.match, values)) + """, + ["names: ^[a-z]+$"], + id="method-reference", + ), + pytest.param( + """ + import re + + def check(prefix, value): + return re.match(rf"^{re.escape(prefix)}-[0-9]+$", value) + """, + ["check: ^…-[0-9]+$"], + id="f-string", + ), + pytest.param( + """ + import re + + def check(value): + return re.match("|".join([r"^[a-z]+$", r"^[0-9]+\\Z"]), value) + """, + ["check: ^[a-z]+$"], + id="joined-alternatives", + ), + pytest.param( + """ + import re + + def check(value): + return re.compile(r"^[a-z]+$|^[0-9]+$").match(value) + """, + ["check: ^[a-z]+$|^[0-9]+$"], + id="alternation", + ), + pytest.param( + """ + import re + + def check(value): + return re.search(r"^[a-z]+$|^[0-9]+\\Z", value) + """, + [r"check: ^[a-z]+$|^[0-9]+\Z"], + id="half-fixed-alternation", + ), + pytest.param( + """ + import re + + def check(value): + return re.search(r"^[a-z]+$|[0-9]+", value) + """, + ["check: ^[a-z]+$|[0-9]+"], + id="anchored-alternative-beside-an-unanchored-one", + ), + pytest.param( + """ + import re + + def check(value): + return re.search(r"(?:^[a-z]+$)", value) or re.match(r"^(?:[a-z]+$|[0-9]+)", value) + """, + ["check: (?:^[a-z]+$)", "check: ^(?:[a-z]+$|[0-9]+)"], + id="dollar-inside-a-group", + ), + pytest.param( + """ + import re + + def check(value): + return re.match(r"^ [a-z]+ $ # the name, and nothing after it", value, re.X) + """, + ["check: ^ [a-z]+ $ # the name, and nothing after it"], + id="verbose", + ), + pytest.param( + """ + import re + + def check(value): + return re.match(r"^a\\\\s*$", value) + """, + [r"check: ^a\\s*$"], + id="escaped-backslash-before-s-star", + ), + pytest.param( + """ + import re + + def check(value): + return re.compile(r"(?i)^[a-z]+$").match(value) + """, + ["check: (?i)^[a-z]+$"], + id="inline-flags", + ), + pytest.param( + """ + import re + _FLAGS = re.MULTILINE + _FLAGS = _FLAGS | re.IGNORECASE + + def check(value): + return re.search(r"^[a-z]+$", value, _FLAGS) + """, + ["check: ^[a-z]+$"], + id="flags-the-scan-cannot-settle-count-as-none", + ), + pytest.param( + """ + import re + if re.match(r"^v[0-9]+$", "v1"): + pass + """, + [": ^v[0-9]+$"], + id="module-level-call", + ), + pytest.param( + """ + import re + + def check(schema, value): + return re.compile(schema["pattern"]).match(value) + """, + ["check: "], + id="schema-pattern-at-run-time", + ), + pytest.param( + """ + import functools + import re + + @functools.lru_cache(maxsize=None) + def _semver_pattern(schema_text): + return re.compile(schema_text) + + def is_semver(value, schema_text): + return bool(_semver_pattern(schema_text).match(value)) + """, + ["is_semver: "], + id="schema-pattern-from-a-helper", + ), + pytest.param( + """ + import re + + def check(rule, value): + return re.search(rule["regex"], value) + """, + ["check: "], + id="re-search-at-run-time", + ), + pytest.param( + """ + import re + + def check(suffix, value): + return re.compile(f"{suffix}").match(value) + """, + ["check: "], + id="f-string-with-an-unknown-end", + ), + pytest.param( + """ + import re + from typing import Optional + + _DEFAULT = re.compile(r"^[A-Za-z_]+\\Z") + + def check(value, pattern: Optional[re.Pattern[str]] = None): + rx = pattern or _DEFAULT + return rx.match(value) + """, + ["check: "], + id="caller-supplied-pattern", + ), + pytest.param( + """ + from typing import Pattern as P + + def check(value, rx: "P[str]"): + return rx.search(value) + """, + ["check: "], + id="pattern-annotation-alias", + ), + ], +) +def test_the_scan_reports(tmp_path: pathlib.Path, source: str, expected: List[str]) -> None: + assert _scan(tmp_path, check=source) == expected + + +@pytest.mark.parametrize( + "source", + [ + pytest.param('_NAME_RE = re.compile(r"^[a-z]+\\Z")\n_NAME_RE.match(v)', id="backslash-z"), + pytest.param('re.match(r"^[a-z]+\\Z|^[0-9]+\\Z", v)', id="alternation-fixed"), + pytest.param('_NAME_RE = re.compile(r"^[a-z]+$")\n_NAME_RE.fullmatch(v)', id="fullmatch"), + pytest.param('re.fullmatch(r"[a-z]+$", v)', id="re-fullmatch"), + pytest.param('re.findall(r"^[a-z]+$", v)', id="findall"), + pytest.param('re.compile(r"^[a-z]+$", re.MULTILINE | re.I).search(v)', id="multiline"), + pytest.param('_FLAGS = re.M\nre.search(r"^[a-z]+$", v, _FLAGS)', id="multiline-constant"), + pytest.param('re.search(r"(?m)^[a-z]+$", v)', id="inline-multiline"), + pytest.param('re.search(r"(?m:^[a-z]+$)", v)', id="scoped-multiline"), + pytest.param('re.match(r"^key:\\s*$", v)', id="whitespace-tail"), + pytest.param('re.match(r"^[a-z]+://[^/]*$", v)', id="negated-class-tail"), + pytest.param('re.match(r"(?is)^\\s*distinct\\s+(.+)$", v)', id="dotall-tail-in-a-group"), + pytest.param('re.match(r"^(?:a\\s*|b\\s+)$", v)', id="every-branch-absorbs"), + pytest.param('re.search(r"\\.ya?ml$", v)', id="unanchored-suffix-search"), + pytest.param('re.match(r"^[0-9]+\\$", v)', id="escaped-dollar"), + pytest.param('re.match(r"^[a-z]+", v)', id="no-end-anchor"), + pytest.param( + 'words = ["a", "b"]\nre.compile("|".join(re.escape(w) for w in words)).search(v)', + id="escaped-alternatives", + ), + pytest.param( + 'import pathlib\npathlib.PurePath(v).match("*.py")\nstore.search("ns", v)', + id="not-a-regex", + ), + pytest.param( + "class GlobPattern:\n pass\n\ndef inner(p: GlobPattern):\n return p.match(v)", + id="an-annotation-that-only-contains-the-word", + ), + ], +) +def test_the_scan_does_not_report(tmp_path: pathlib.Path, source: str) -> None: + body = textwrap.indent(source, " ") + assert _scan(tmp_path, check=f"import re\n\ndef check(v, store):\n{body}\n") == [] + + +@pytest.mark.parametrize( + "importer", + [ + pytest.param("from .consts import NAME_RE\n", id="relative-import"), + pytest.param("from fluid_build.consts import NAME_RE\n", id="absolute-import"), + pytest.param("from fluid_build import consts\nNAME_RE = consts.NAME_RE\n", id="module"), + pytest.param("import fluid_build.consts as c\nNAME_RE = c.NAME_RE\n", id="import-as"), + ], +) +def test_the_scan_follows_a_pattern_across_modules(tmp_path: pathlib.Path, importer: str) -> None: + consts = 'import re\nNAME_RE = re.compile(r"^[a-z]+$")\n' + check = importer + "\ndef check(value):\n return NAME_RE.match(value)\n" + assert _scan(tmp_path, consts=consts, check=check) == ["check: ^[a-z]+$"] + + +def test_the_scan_follows_a_submodule_its_package_imports(tmp_path: pathlib.Path) -> None: + """``from . import consts`` in ``pkg/__init__.py`` binds the submodule itself.""" + found = _scan( + tmp_path, + pkg__init__="from . import consts\n", + **{"pkg/consts": 'import re\nNAME_RE = re.compile(r"^[a-z]+$")\n'}, + check=( + "from fluid_build.pkg import consts\n\n" + "def check(value):\n return consts.NAME_RE.match(value)\n" + ), + ) + assert found == ["check: ^[a-z]+$"] + + +@pytest.mark.timeout(60) +def test_the_scan_reads_long_chains_in_linear_time(tmp_path: pathlib.Path) -> None: + """Reading a receiver twice per call, or a name again at each use, doubles the work at + every step, and 40 steps would never finish. A string doubled 40 times is cut short.""" + reused = "\n".join(f"X{i + 1} = X{i} or X{i}" for i in range(40)) + doubled = "\n".join(f"Y{i + 1} = Y{i} + Y{i}" for i in range(40)) + source = ( + f'import re\nX0 = re.compile(r"^[a-z]+$")\nY0 = "a"\n{reused}\n{doubled}\n\n' + "def check(v):\n" + f" return v{'.strip()' * 40}.match(v) or X40.match(v) or re.match(Y40, v)\n" + ) + assert _scan(tmp_path, check=source) == ["check: ^[a-z]+$"] + + +def test_a_module_the_scan_cannot_read_is_listed(tmp_path: pathlib.Path) -> None: + package = tmp_path / "fluid_build" + package.mkdir() + (package / "__init__.py").write_text("", encoding="utf-8") + (package / "broken.py").write_text("def f(:\n RX.match(v)\n", encoding="utf-8") + (package / "latin1.py").write_bytes(b"# caf\xe9\nRX.match(v)\n") + deep = " + ".join(['"a"'] * 3000) + (package / "deep.py").write_text(f"import re\nre.match({deep}, v)\n", encoding="utf-8") + scanner = _Scan(package) + assert scanner.findings() == [] + listed = sorted(entry.split(":")[0] for entry in scanner.unreadable) + assert listed == ["fluid_build/broken.py", "fluid_build/deep.py", "fluid_build/latin1.py"] From 6f68f64f5889473abb3827d207e32f65fa426e3b Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 4 Nov 2026 07:06:38 +0100 Subject: [PATCH 3/4] security(iceberg-sink): a catalogAuth variable reaches only hosts the operator paired it with (#818) * security(iceberg-sink): pair a catalogAuth variable with every host it reaches On the Kafka Connect and embedded Debezium Iceberg sinks, sink.catalogAuth names an environment variable and the same contract names the catalog (location.uri) and the OAuth2 token endpoint. Iceberg resolves the emitted env: on the Connect worker, or on embedded Debezium from the runner's whole environment, so a contract could send any variable there to a host it picked. A probe with a catalog at https://catalog.attacker.example returned rc 0 on both engines and both auth types. The sink preflight, and fluid validate as a warning, now require for the catalog and an OAuth2 token endpoint: - https, loopback included, as on the managed Flink lane; - NAME@host in FLUID_ACQUISITION_SECRET_REF_ALLOWLIST (loopback exempt); - a hostname listed in FLUID_ACQUISITION_DEPLOYMENT_HOST_ALLOWLIST, since the worker resolves it itself; IP literals keep the #758 rules. The check runs before any Connect REST call, application.properties write or read of the variable. Location text the derived config carries may not start with env: or hold ${ or a control character, and client.region must be one host label. A {{ env.* }} template is checked at run, where the dispatcher renders it. Ported from the managed Flink lane's policy (#809, streaming/policy.py). Host-scoped credentials follow Terraform's per-host TF_TOKEN_; an operator list of readable variables follows Kafka's KIP-887 EnvVarConfigProvider allowlist.pattern. The host rule and the https scope were chosen by the founder on 2026-11-04. * docs(changelog): an Iceberg sink's catalog credential needs an operator pairing * fix(iceberg-sink): the destination refusal gives one export line to paste Both operator lists now come in a single export, as on the managed Flink lane, and the sentence about where Iceberg resolves the variable no longer runs two relative clauses together. * refactor(iceberg-sink): split the destination check so each helper decides one thing No behaviour change. _check_catalog_auth keeps its size: one call checks the hosts, then reads the embedded Debezium variable. The destinations, the hard refusals, the host-list rule and the message are separate helpers (ripwire flagged complexity 34 and a verbosity regression on _check_catalog_auth). * docs(iceberg-sink): name no lane main lacks; say which refusals validate warns about The https refusal no longer cites the managed Flink lane, which is not on main. The module docstring says https and link-local are errors in fluid validate too, the env: note is scoped to the REST catalog with the rule kept for every kind as hardening, and the CHANGELOG adds the location-text upgrade note and says a bare NAME or a parent domain grants more than NAME@host. * security(iceberg-sink): refuse a credential host the refusal cannot repeat safely The refusal's export line repeated the contract's host inside double quotes, so a catalog uri such as https://x"$(id)".example/ turned the operator's paste into a command, and a comma in the host added a bare entry that lets another variable go to any host. A credential destination must now name a plain DNS name or IP address, with no zone id and no user:password@ part (Python and the JVM split such an authority differently). The binding.location text check runs only for the two engines whose runners derive the sink config; any other engine raised KeyError there. Found by the scoped codebase audit. * fix(iceberg-sink): the judge's lows: no rendered region in a message, path templates checked now, no read for a non-REST kind A run-time region refusal no longer quotes the value, which a {{ env.* }} template may have rendered from a secret. fluid validate defers a templated URL only when the template sits in the scheme or authority, so http://169.254.169.254/{{ env.X }} is refused at validate. A catalogAuth on a catalog not reached over REST is already refused, and the embedded preflight now reads no variable for it. * fix(iceberg-sink): check the fields the sink reads, and a written-out scheme From the scoped defect scan: - sink.partitionBy, which the runner's SinkSpec reads over the binding's, is now the partition text checked when set; it reached both emitters unchecked. - Only the namespace and table fields the sink reads are checked (the first set, as iceberg_namespace reads them), so an unused field no longer fails a build, and a derived warehouse names the location fields it came from. - A template in a URL's host still leaves the host to the run, but an http:// scheme written out is refused by fluid validate. - An IP literal is parsed as written: 10.0.0.1%25.example is a name. - The refusal says its export line keeps the entries set where it ran. - test_catalog_auth_sink extends the host list after kafka_connect_mock, which may set it for its own server (the pinned-address branch does). * test(iceberg-sink): pin the cases the test audit found open; skip the live OAuth2 trio until TLS The destination tests now clear an ambient host list, assert that the export line keeps the operator's host entries, and cover a URL with no readable host, a token endpoint judged apart from a loopback catalog (remote, link-local, unspecified), the validate warning on both engines and both types, and a Glue region. tests/integration/test_lakekeeper_oauth_live.py serves Lakekeeper and Keycloak over http, which a catalogAuth credential no longer reaches. The three tests that derive a sink config are skipped until the stack runs TLS; the founder chose that on 2026-11-04 over moving the stack in this PR. The fourth test talks to Lakekeeper directly and still runs. * test(iceberg-sink): mark the synthetic secrets detect-secrets flags AWS's documented example key and a u:p4ss@ userinfo are test values; each pragma sits on the line that holds the value, where black leaves it. * test(iceberg-sink): reuse the catalogAuth sink test contracts and fake server The destination tests cloned _kc_contract, _dbz_contract and the fake Debezium server from test_catalog_auth_sink.py. They now call those, which take an optional location, and share one installer for the fake server (ripwire flagged the clones). * fix(iceberg-sink): read sink.partitionBy exactly as SinkSpec does A mapping-shaped partitionBy reaches the config as its keys (SinkSpec.from_dict lists it), so the check lists it the same way rather than falling back to the binding's. A value SinkSpec cannot list stops the run anyway. From the judge's re-review. * test(iceberg-sink): point the skipped live trio at its follow-up card --- CHANGELOG.md | 23 + fluid_build/build_runners/_secret_policy.py | 11 + fluid_build/build_runners/debezium/runner.py | 3 +- .../kafka_connect/iceberg_sink_validation.py | 463 +++++++++++- .../build_runners/kafka_connect/runner.py | 2 + .../test_catalog_auth_redaction.py | 7 +- tests/build_runners/test_catalog_auth_sink.py | 69 +- .../test_catalog_auth_validation.py | 25 +- .../test_iceberg_sink_catalog_destinations.py | 675 ++++++++++++++++++ .../integration/test_lakekeeper_oauth_live.py | 16 + 10 files changed, 1266 insertions(+), 28 deletions(-) create mode 100644 tests/build_runners/test_iceberg_sink_catalog_destinations.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 80148ceb..8a3002e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -605,6 +605,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **A contract chose both the variable and the hosts an Iceberg sink's catalog credential went + to.** On the Kafka Connect and embedded Debezium Iceberg sinks, `sink.catalogAuth` names an + environment variable (`tokenEnv` / `credentialEnv`), and the same contract names the catalog + (`binding.location.uri`) and the OAuth2 token endpoint (`oauth2ServerUri`). The config forge-cli + pushes or writes carries `env:`, which Iceberg resolves on the Connect worker, or on + embedded Debezium from the runner's whole environment, so any variable there could be sent to a + host the contract picked. Every host the credential reaches must now be https (a loopback one + included) and allowed by the operator: the variable paired with it as `NAME@host` in + `FLUID_ACQUISITION_SECRET_REF_ALLOWLIST`, and a hostname listed in + `FLUID_ACQUISITION_DEPLOYMENT_HOST_ALLOWLIST`, because the worker resolves the name itself, + later. An IP address keeps that list's rules (link-local never, a private one with no entry, a + public one only when listed), and a loopback host needs neither entry. A bare `NAME` entry + allows every host and a listed hostname also covers its subdomains, so prefer `NAME@host` and + the exact host. The run refuses before any Connect REST call, `application.properties` write or + read of the variable; `fluid validate` warns with the `export` line the run needs, and refuses + an `http://` or link-local host outright. `binding.location` text that reaches the derived sink + config may no longer start with `env:` or hold `${` or a control character, and its `region` + must be one host label. **Upgrade:** a sink whose `catalogAuth` reaches a non-loopback host + fails its run until the operator adds both entries, and one on an `http://` catalog or token + endpoint fails `fluid validate`. A derived sink whose `binding.location` holds such text, or a + region that is not one label, now fails `fluid validate` and the run, `catalogAuth` or not; + write the value itself. + - **`validate_ident` let a name with a trailing newline through.** Python's `$` also matches just before a trailing newline, so the identifier allowlist in `providers/_sql_safety.py`, which the DDL and OpenTofu emitters route names through, accepted `orders\n` and passed it on with the diff --git a/fluid_build/build_runners/_secret_policy.py b/fluid_build/build_runners/_secret_policy.py index 5b66402b..41031c42 100644 --- a/fluid_build/build_runners/_secret_policy.py +++ b/fluid_build/build_runners/_secret_policy.py @@ -192,6 +192,17 @@ def allowlist(environ: Optional[Mapping[str, str]] = None) -> List[Tuple[str, Op return _entries(env.get(ALLOWLIST_ENV, "") or "") +def variable_allowed( + variable: str, host: Optional[str], *, environ: Optional[Mapping[str, str]] = None +) -> bool: + """Whether ``variable`` may be sent to ``host``: the decision, without wording. + + For a caller that phrases its own refusal (the Iceberg sinks' catalog token, + ``kafka_connect/iceberg_sink_validation.py``). A loopback host needs no entry. + """ + return is_loopback(host) or _allows(variable, host, allowlist(environ)) + + def _allows(variable: str, host: Optional[str], entries: List[Tuple[str, Optional[str]]]) -> bool: target = (host or "").strip().lower().rstrip(".") for name, allowed_host in entries: diff --git a/fluid_build/build_runners/debezium/runner.py b/fluid_build/build_runners/debezium/runner.py index 4f24ad57..4fb0dec7 100644 --- a/fluid_build/build_runners/debezium/runner.py +++ b/fluid_build/build_runners/debezium/runner.py @@ -706,7 +706,8 @@ def _execute_debezium_server( # The server inherits this process's environment, which is where Iceberg # resolves the ``env:`` catalog properties ``sink.catalogAuth`` - # emits, and where the preflight checked the variable is set. The + # emits, and where the preflight checked the variable is set, once the + # operator had paired it with every host it reaches. The # ``FLUID_DBZ_*`` credentials the file refers to are added to it. No # secret is written to application.properties. try: diff --git a/fluid_build/build_runners/kafka_connect/iceberg_sink_validation.py b/fluid_build/build_runners/kafka_connect/iceberg_sink_validation.py index 9c9a2ffa..f213f31d 100644 --- a/fluid_build/build_runners/kafka_connect/iceberg_sink_validation.py +++ b/fluid_build/build_runners/kafka_connect/iceberg_sink_validation.py @@ -63,7 +63,17 @@ properties it emits; an embedded Debezium run also needs its variable set (the Kafka Connect variable lives on the worker, which forge-cli cannot see); and an override map that inlines a ``credential`` or ``token`` value - is a warning. + is a warning; +* every host a ``sink.catalogAuth`` credential reaches (the catalog, and an + OAuth2 token endpoint) must be https and must not be link-local, an error + here and in the run's preflight; and the operator must allow it: the variable + paired with the host (``NAME@host``), and a hostname listed, because the + worker or the server resolves it itself. Those lists are the operator's, so + a missing entry is a warning here and an error in the preflight, which + checks it before the variable is read; +* the ``binding.location`` text a derived sink config carries must not start + with ``env:`` or hold ``${`` or a control character, and ``client.region`` + must be one host label. Debezium builds in ``bring-your-own`` / ``managed`` mode create only the SOURCE connector (no sink is derived), so none of this applies to them; an embedded @@ -86,8 +96,10 @@ import contextlib import contextvars +import ipaddress import logging import os +import re from dataclasses import dataclass from typing import ( AbstractSet, @@ -100,9 +112,13 @@ Optional, Sequence, Tuple, + Union, ) +from urllib.parse import urlsplit +from ..._net import is_public_address from ...providers._iceberg_catalog import ( + BIGQUERY_LOCATION, BIGQUERY_PROJECT_ID, BUCKET_WAREHOUSE_KINDS, DYNAMODB_CATALOG_IMPL, @@ -111,6 +127,7 @@ FAMILY_UNKNOWN, GLUE_CATALOG_IMPL, CatalogKind, + ResolvedIcebergCatalog, binding_catalog_kind, canonical_catalog_kind, catalog_kind_info, @@ -125,12 +142,20 @@ AUTH_OWNED_PROPERTIES, AUTHORIZATION_HEADER_PROPERTY, CATALOG_AUTH_FIELD, + FORBIDDEN_TEXT_PATTERN, INLINE_CHECKED_KEYS, CatalogAuth, is_literal_secret, parse_catalog_auth, raw_catalog_auth, ) +from ...util.binding_paths import ENV_PLACEHOLDER_RE +from .._deployment_host_policy import ALLOWLIST_ENV as HOST_ALLOWLIST_ENV +from .._deployment_host_policy import _on_allowlist +from .._deployment_host_policy import allowlist as host_allowlist +from .._secret_policy import ALLOWLIST_ENV as SECRET_REF_ALLOWLIST_ENV +from .._secret_policy import allowlist as secret_ref_allowlist +from .._secret_policy import is_loopback, variable_allowed LOG = logging.getLogger("fluid.acquire.iceberg_sink") @@ -436,7 +461,7 @@ def _check_build( "streamingSink.routeField" ) - _check_catalog_auth(bid, sink, runtime, targets, errors) + _check_catalog_auth(bid, sink, runtime, targets, errors, warnings) _check_inlined_secrets(bid, sink, runtime, warnings) # The catalog checks read each expose the sink writes. A Debezium Server @@ -506,6 +531,8 @@ def _check_catalog( f"use one of: {', '.join(known_catalog_kinds())}" ) return + if runtime.derives and runtime.engine in _CATALOG_AUTH_ENGINES: + _check_location_text(bid, binding, sink, runtime, errors) # The sink must write through the catalog dbt and the IaC read. Both read # only the expose, so a ``sink.catalog``, an override or a hand-written @@ -1136,9 +1163,10 @@ def _check_catalog_auth( runtime: IcebergSinkPlan, targets: Sequence[Mapping[str, Any]], errors: List[str], + warnings: List[str], ) -> None: - """``sink.catalogAuth``: shape, catalog, derivation, overrides, and the - embedded Debezium variable at run time. HARD.""" + """``sink.catalogAuth``: shape, catalog, derivation, overrides, the hosts it + reaches, and the embedded Debezium variable at run time. HARD.""" raw = raw_catalog_auth(sink) if raw is None: return @@ -1214,7 +1242,7 @@ def _check_catalog_auth( f"keys from {label}, or remove sink.{CATALOG_AUTH_FIELD}" ) - _check_catalog_auth_env_at_run(head, auth, runtime, errors) + _check_catalog_auth_reach(head, auth, refused, runtime, targets, errors, warnings) def _check_catalog_auth_env_at_run( @@ -1252,6 +1280,298 @@ def _check_catalog_auth_env_at_run( run_only.append(issue) +#: The environment each engine resolves ``sink.catalogAuth``'s ``env:`` from. +_RESOLVED_FROM: Mapping[str, str] = { + "kafka-connect": "the Kafka Connect worker's environment", + "debezium": "this runner's environment, which the embedded Debezium Server inherits,", +} + +_OPERATOR_ONLY = ( + "in the OPERATOR environment (a shell or CI variable, not a project .env, which is " + "ignored for these settings)" +) + + +#: A URL's scheme and authority: what decides where a request goes. +_URL_HEAD_RE = re.compile(r"[^/]*(?://[^/?#]*)?") + + +def _rendered_at_run(value: str) -> bool: + """Under ``fluid validate``, does ``value`` wait on a ``{{ env.* }}`` template? + + The acquisition dispatcher renders every such template before a runner + and its preflight see the contract (``build_runners/base.py``), so the + preflight checks the value the sink receives. ``fluid validate`` sees the + template itself and leaves the check to the run, but only when the + template is where it changes the answer: in a URL's scheme or authority, + not in its path. + """ + if _AT_RUN.get() is not None: + return False + head = _URL_HEAD_RE.match(value) + return bool(ENV_PLACEHOLDER_RE.search(head.group(0) if head else value)) + + +def _catalog_uri_reaching(binding: Mapping[str, Any], runtime: IcebergSinkPlan) -> Tuple[str, str]: + """The catalog uri the sink's REST client calls, and the setting that names it. + + ``binding.location.uri``, unless an override map merged over the derived + config sets the uri property: the last one wins, as the runner merges them. + The credential goes wherever this points. + """ + key = f"{_property_prefix(runtime)}uri" + uri = str((binding.get("location") or {}).get("uri") or "") + source = "binding.location.uri" + for label, mapping in runtime.overrides: + if key in mapping: + uri, source = str(mapping[key] or ""), f"{label} {key}" + return uri, source + + +def _ip_literal(host: str) -> Optional[Union[ipaddress.IPv4Address, ipaddress.IPv6Address]]: + """``host`` as an address, an IPv4-mapped IPv6 one as its IPv4; ``None`` for a name. + + Parsed as written: ``10.0.0.1%25.example`` is a name, not 10.0.0.1. + """ + try: + address = ipaddress.ip_address(host) + except ValueError: + return None + if isinstance(address, ipaddress.IPv6Address) and address.ipv4_mapped is not None: + return address.ipv4_mapped + return address + + +def _check_catalog_auth_reach( + head: str, + auth: Optional[CatalogAuth], + refused: Sequence[str], + runtime: IcebergSinkPlan, + targets: Sequence[Mapping[str, Any]], + errors: List[str], + warnings: List[str], +) -> None: + """The hosts the credential reaches, then the embedded Debezium variable. + + In that order, so a refused destination never reads the variable. + ``refused`` names the catalog kinds not reached over REST: that is already + an error, and nothing there reads the credential, so nothing reads the + variable either. + """ + if auth is None or refused: + return + if not _check_catalog_auth_destinations(head, auth, runtime, targets, errors, warnings): + _check_catalog_auth_env_at_run(head, auth, runtime, errors) + + +def _credential_destinations( + auth: CatalogAuth, runtime: IcebergSinkPlan, targets: Sequence[Mapping[str, Any]] +) -> List[Tuple[str, str, str]]: + """``(role, setting, url)`` for each host the credential reaches. + + The catalog the sink calls (:func:`_catalog_uri_reaching`), once per uri, + and for ``oauth2`` the token endpoint, which receives the credential itself + while the catalog receives a token minted from it. + """ + destinations: List[Tuple[str, str, str]] = [] + for target in targets: + uri, source = _catalog_uri_reaching(target.get("binding") or {}, runtime) + if uri.strip() and all(uri != url for _role, _source, url in destinations): + destinations.append(("Iceberg catalog", source, uri)) + if auth.type == "oauth2": + source = f"sink.{CATALOG_AUTH_FIELD}.oauth2ServerUri" + destinations.append(("OAuth2 token endpoint", source, auth.oauth2_server_uri)) + return destinations + + +def _received(auth: CatalogAuth, role: str) -> str: + """What the ``role`` host receives of the credential, for the messages.""" + if role == "OAuth2 token endpoint": + return f"the credential in {auth.env_var} (the client id and secret)" + if auth.type == "oauth2": + return f"a token minted from the credential in {auth.env_var}" + return f"the token in {auth.env_var}" + + +#: A host the operator can list: a DNS name (``urlsplit`` has lowercased it) or +#: an IP address, in plain characters. The refusal's ``export`` line repeats +#: the host, so a quote, ``$(`` or comma there would turn the operator's paste +#: into a command, or into a bare entry that lets some other variable go to any +#: host. A ``%`` zone id names an interface, never a remote catalog. +_PLAIN_HOST_RE = re.compile(r"[a-z0-9._-]+|[0-9a-f:.]+") + + +def _not_https(source: str, role: str, receives: str) -> str: + return ( + f"{source} must be an https:// URL: the {role} receives {receives} on every " + "request, and http would carry it in clear text, so a loopback host needs https too" + ) + + +def _destination_host(url: str, source: str, role: str, receives: str) -> Tuple[str, str]: + """``(host, "")`` for a destination the operator may allow, else ``("", why not)``. + + "Why not" is a refusal no operator entry changes: the URL is not https (a + loopback one included), carries a ``user:password@`` part, names no plain + host (:data:`_PLAIN_HOST_RE`), or names a link-local or unspecified + address, where cloud instance metadata lives (the deployment host policy's + rule). Python and the JVM split a URL's authority alike only when it holds + no such part, so the host checked here is the one the sink connects to. + """ + no_host = f"{source} names no host, so where the {role} sends {receives} cannot be checked" + try: + parts = urlsplit(url) + host = parts.hostname or "" + except ValueError: # an unbalanced "[" in the netloc: no host can be read + return "", no_host + if parts.scheme != "https": + return "", _not_https(source, role, receives) + if "@" in parts.netloc: + return "", ( + f"{source} carries a user:password@ part (not shown). Remove it: the {role} " + f"receives {receives}, and a credential belongs in that variable, not in the URL" + ) + if not host: + return "", no_host + if not _PLAIN_HOST_RE.fullmatch(host): + return "", ( + f"{source} names a host that is not a plain DNS name or IP address (letters, " + "digits, '.', '-' and '_'; a non-ASCII name in its xn-- form; no zone id). It " + f"is not shown, and the {role} would receive {receives}" + ) + address = _ip_literal(host) + if address is not None and (address.is_link_local or address.is_unspecified): + return "", ( + f"the {role} host {host!r} ({source}) is link-local or unspecified, where cloud " + "instance metadata lives; this is refused even with an allowlist entry" + ) + return host, "" + + +def _unlisted_host(host: str) -> Optional[str]: + """Why ``host`` needs a host-list entry the operator has not given, or ``None``. + + The rule the founder chose for these sinks on 2026-11-04. The Kafka Connect + worker or the Debezium Server + resolves a hostname itself, after this check, so no DNS answer seen here + shows where it will connect: a name passes only when the operator lists it + in ``FLUID_ACQUISITION_DEPLOYMENT_HOST_ALLOWLIST``, or when it is + ``localhost``. An IP literal keeps the deployment host policy's rules + (``_deployment_host_policy.py``): a private or loopback address needs no + entry, a public one is listed (link-local is refused before this, by + :func:`_destination_host`). Nothing is resolved. A listed host is matched by + that policy's own suffix rule, so the two lists cannot read differently. + """ + if _on_allowlist(host, host_allowlist()): + return None + if _ip_literal(host) is not None: + return ( + f"is a public address not in {HOST_ALLOWLIST_ENV}" if is_public_address(host) else None + ) + return None if is_loopback(host) else f"is a name not in {HOST_ALLOWLIST_ENV}" + + +def _check_catalog_auth_destinations( + head: str, + auth: CatalogAuth, + runtime: IcebergSinkPlan, + targets: Sequence[Mapping[str, Any]], + errors: List[str], + warnings: List[str], +) -> bool: + """Every host the credential reaches must be https and allowed by the operator. + + The contract names the variable (``tokenEnv`` / ``credentialEnv``) and the + hosts (:func:`_credential_destinations`). Iceberg resolves ``env:`` + where the sink runs (on embedded Debezium, from this runner's whole + environment) and sends the value there, so without an operator boundary any + variable there could go to a host the contract chose. Each host needs: + + * https, and an address no entry has to rule out (:func:`_destination_host`). + HARD; + * the variable paired with it in ``FLUID_ACQUISITION_SECRET_REF_ALLOWLIST`` + (``NAME@host``), unless it is loopback (``_secret_policy.variable_allowed``); + * a hostname listed in ``FLUID_ACQUISITION_DEPLOYMENT_HOST_ALLOWLIST`` + (:func:`_unlisted_host`). + + Both lists are operator-only (``credentials/protected_env.py`` ignores a + project dotenv file's value) and are read where the sink config is + derived: the run's preflight refuses, and ``fluid validate``, which may run + without them, warns with the entries the run needs. Returns ``True`` when + the run must stop. + """ + hard: List[str] = [] + reasons: List[str] = [] + pairs: List[str] = [] + hosts: List[str] = [] + for role, source, url in _credential_destinations(auth, runtime, targets): + if _rendered_at_run(url): + # A template decides the host, which the run checks once rendered; + # a scheme written out is decided now. + scheme, written, _rest = url.partition("://") + if written and not ENV_PLACEHOLDER_RE.search(scheme): + if scheme.strip().lower() != "https": + hard.append(f"{head}: {_not_https(source, role, _received(auth, role))}") + continue + host, problem = _destination_host(url, source, role, _received(auth, role)) + if problem: + hard.append(f"{head}: {problem}") + continue + unlisted = _unlisted_host(host) + if unlisted is not None: + reasons.append(f"the {role} host {host!r} {unlisted}") + hosts.append(host) + if not variable_allowed(auth.env_var, host): + reasons.append( + f"{auth.env_var} is not paired with the {role} host {host!r} in " + f"{SECRET_REF_ALLOWLIST_ENV}" + ) + pairs.append(f"{auth.env_var}@{host}") + errors.extend(hard) + if reasons: + issue = _unallowed_destinations(head, auth, runtime, reasons, pairs, hosts) + run_only = _AT_RUN.get() + if run_only is None: + warnings.append(issue) + else: + errors.append(issue) + run_only.append(issue) + return bool(hard or reasons) + + +def _unallowed_destinations( + head: str, + auth: CatalogAuth, + runtime: IcebergSinkPlan, + reasons: Sequence[str], + pairs: Sequence[str], + hosts: Sequence[str], +) -> str: + """The refusal of the destinations the operator has yet to allow, with one export.""" + settings: List[str] = [] + if pairs: + # Rebuilt from the parsed list, so an entry the parser ignores is not echoed back. + current = [f"{n}@{h}" if h else n for n, h in secret_ref_allowlist()] + entries = ",".join(dict.fromkeys([*current, *pairs])) + settings.append(f'{SECRET_REF_ALLOWLIST_ENV}="{entries}"') + if hosts: + entries = ",".join(dict.fromkeys([*host_allowlist(), *hosts])) + settings.append(f'{HOST_ALLOWLIST_ENV}="{entries}"') + return ( + f"{head}: sink.{CATALOG_AUTH_FIELD}.{auth.env_field} names {auth.env_var}. Iceberg " + f"resolves it from {_RESOLVED_FROM[runtime.engine]} and sends it to hosts this " + "contract names, and the run refuses until the operator allows each: " + f"{'; '.join(dict.fromkeys(reasons))}. A contract names both the variable and the " + "hosts, so without the operator's lists any variable there could go to a host the " + "contract chose; a hostname needs an entry because the sink resolves it itself, " + "later, where no lookup made here applies. If this is intended, add the entries " + f"{_OPERATOR_ONLY}; this keeps the ones set here: export {' '.join(settings)}. Leave " + "the port off: a host written " + "with a port never matches. A loopback host needs no entry, and a private IP address " + "needs only the pairing" + ) + + def _check_inlined_secrets( bid: Any, sink: Mapping[str, Any], @@ -1287,3 +1607,136 @@ def _check_inlined_secrets( f"Use sink.{CATALOG_AUTH_FIELD}, or set {key} to env:, which Iceberg " "resolves from the environment of the process that runs the sink" ) + + +# --------------------------------------------------------------------------- +# binding.location text in the derived sink config +# --------------------------------------------------------------------------- + +#: One DNS label (RFC 1123): letters, digits and inner hyphens, at most 63. +#: Any case, because the sinks pass the region for every catalog kind and a +#: label ignores case. +_HOST_LABEL_RE = re.compile(r"[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?") + +#: The binding setting each catalog-specific property is derived from. +_EXTRA_PROPERTY_SOURCES: Mapping[str, str] = { + BIGQUERY_PROJECT_ID: "binding.location.project", + BIGQUERY_LOCATION: "binding.location.region", +} + +#: Who resolves a reference in the derived config, for the location messages. +_REFERENCES_RESOLVED_BY: Mapping[str, str] = { + "kafka-connect": ( + "the Kafka Connect worker, where Iceberg's REST catalog replaces a value that starts " + "with env: by that variable from the worker's environment and Kafka Connect resolves " + "a ${provider:key} expression with the worker's config providers" + ), + "debezium": ( + "the embedded Debezium Server, where Iceberg's REST catalog replaces a value that " + "starts with env: by that variable and SmallRye Config expands a ${...} expression, " + "both from the server's environment, which is this runner's" + ), +} + + +def _derived_location_text( + binding: Mapping[str, Any], + sink: Mapping[str, Any], + resolved: ResolvedIcebergCatalog, + runtime: IcebergSinkPlan, +) -> List[Tuple[str, str]]: + """``(setting, value)`` for each contract value the derived config carries. + + Read off the resolved catalog both emitters build from, so a value is + checked as it reaches the sink, a warehouse derived from a bucket included, + and only the field the sink reads where several could name it: the first + namespace field set, as ``iceberg_namespace`` reads them, and + ``sink.partitionBy`` over ``binding.location.partitionBy``, as the runner's + ``SinkSpec`` does. Kafka Connect also names the table; Debezium Server + composes it. + """ + loc = binding.get("location") or {} + text: List[Tuple[str, str]] = [] + if resolved.uri: + text.append(("binding.location.uri", str(resolved.uri))) + if resolved.warehouse: + written = str(loc.get("warehouse") or "").strip() + source = ( + "binding.location.warehouse" + if written and resolved.warehouse == written + else "the warehouse derived from binding.location (bucket, path, database, table)" + ) + text.append((source, resolved.warehouse)) + if resolved.region: + text.append(("binding.location.region", str(resolved.region))) + for prop, value in resolved.extra_catalog_props.items(): + text.append((_EXTRA_PROPERTY_SOURCES.get(prop, f"binding.location ({prop})"), str(value))) + namespaces = ( + ("binding.location.database", loc.get("database")), + ("binding.database", binding.get("database")), + ("binding.location.dataset", loc.get("dataset")), + ) + tables = (("binding.location.table", loc.get("table")), ("binding.table", binding.get("table"))) + used = [next(((s, v) for s, v in namespaces if v), None)] + if runtime.engine == "kafka-connect": # Debezium Server composes the table itself + used.append(next(((s, v) for s, v in tables if v), None)) + text.extend((source, str(value)) for source, value in filter(None, used)) + raw_partition = sink.get("partitionBy") + try: # as SinkSpec.from_dict reads it: a mapping gives its keys, a string its letters + partition = list(raw_partition) if raw_partition else [] + except TypeError: # SinkSpec refuses it too, so the run stops before any config + partition = [] + if partition: + text.extend(("sink.partitionBy", str(item)) for item in partition) + else: + text.extend(("binding.location.partitionBy", str(item)) for item in resolved.partition_by) + return text + + +def _check_location_text( + bid: Any, + binding: Mapping[str, Any], + sink: Mapping[str, Any], + runtime: IcebergSinkPlan, + errors: List[str], +) -> None: + """The ``binding.location`` text a derived sink config carries. HARD. + + Iceberg's REST catalog replaces a catalog property that starts with ``env:`` + by that variable, in the process that builds the catalog + (EnvironmentUtil.resolveAll, from RESTSessionCatalog.initialize: + apache-iceberg-1.10.0 RESTSessionCatalog.java:195). A warehouse is then sent + to the catalog and a region becomes part of the S3 and Glue endpoint hosts, + so location text that references a variable could send any variable the + worker or the server holds to a host the contract chose; ``${`` is the Kafka + Connect and SmallRye form of the same reference, read for every catalog + kind. The ``env:`` rule is applied to every kind too, as hardening: no real + value starts with it, and a kind that later resolves it needs no new check. + Only ``sink.catalogAuth`` names a variable, and the operator pairs it with + its hosts (:func:`_check_catalog_auth_destinations`). A value is never + quoted: it names a variable. ``client.region`` must also be one host label. + """ + resolved = resolve_iceberg_catalog(binding, sink=sink, account_ref="") + referenced: set[str] = set() + for source, value in _derived_location_text(binding, sink, resolved, runtime): + if not re.search(FORBIDDEN_TEXT_PATTERN, value) or source in referenced: + continue + referenced.add(source) + errors.append( + f"iceberg sink (build {bid!r}): {source} starts with env:, or holds ${{ or a " + "control character (the value is not shown). The derived sink config hands it to " + f"{_REFERENCES_RESOLVED_BY[runtime.engine]}, so it could send any variable there to " + f"a host the contract chose. Write the value itself; only sink.{CATALOG_AUTH_FIELD} " + "may name a variable, and the operator pairs it with the hosts it reaches" + ) + region = str(resolved.region or "") + if region and "binding.location.region" not in referenced and not _rendered_at_run(region): + if not _HOST_LABEL_RE.fullmatch(region): + errors.append( + f"iceberg sink (build {bid!r}): binding.location.region is not a region name " + "such as eu-central-1 (the value is not shown: at run it may have come from a " + "{{ env.* }} template). The derived sink config passes it to " + "Iceberg's AWS client as client.region, which builds the S3 and Glue endpoint " + "hosts from it, so it must be one host label: letters, digits and inner " + "hyphens, at most 63 characters" + ) diff --git a/fluid_build/build_runners/kafka_connect/runner.py b/fluid_build/build_runners/kafka_connect/runner.py index fee82975..7f8eb022 100644 --- a/fluid_build/build_runners/kafka_connect/runner.py +++ b/fluid_build/build_runners/kafka_connect/runner.py @@ -440,6 +440,8 @@ def _execute(ctx: RunContext, runner: KafkaConnectRunner) -> RunResult: # ``env:`` references only; the variable lives on the # Connect worker, out of this process's sight, so there is no # run check for it here (the embedded Debezium runner has one). + # The preflight above refused it unless the operator pairs it + # with every host it reaches. catalog_auth=catalog_auth_of(props.get("sink")), ) # A bucket whose variable is unset or empty here resolved to one the diff --git a/tests/build_runners/test_catalog_auth_redaction.py b/tests/build_runners/test_catalog_auth_redaction.py index 1b721cd7..537f44eb 100644 --- a/tests/build_runners/test_catalog_auth_redaction.py +++ b/tests/build_runners/test_catalog_auth_redaction.py @@ -80,7 +80,12 @@ def test_an_override_warehouse_password_is_masked(): def test_the_embedded_preflight_registers_the_variable_value(monkeypatch): monkeypatch.setenv("FW_LK_CREDENTIAL", f"spark:{_PASSWORD}") - binding = _binding("lakekeeper", uri="http://c:8181/catalog", warehouse="analytics") + # The operator pairs the variable with both hosts it reaches. + monkeypatch.setenv( + "FLUID_ACQUISITION_SECRET_REF_ALLOWLIST", "FW_LK_CREDENTIAL@c,FW_LK_CREDENTIAL@kc.example" + ) + monkeypatch.setenv("FLUID_ACQUISITION_DEPLOYMENT_HOST_ALLOWLIST", "c,kc.example") + binding = _binding("lakekeeper", uri="https://c:8443/catalog", warehouse="analytics") contract = _contract( binding, debezium={"deployment": {"mode": "embedded"}, "server": {"sink": {}}} ) diff --git a/tests/build_runners/test_catalog_auth_sink.py b/tests/build_runners/test_catalog_auth_sink.py index 57a6d1e0..02e8c36b 100644 --- a/tests/build_runners/test_catalog_auth_sink.py +++ b/tests/build_runners/test_catalog_auth_sink.py @@ -25,12 +25,15 @@ from __future__ import annotations import logging +import os import subprocess from pathlib import Path -from typing import Any, Dict, List +from typing import Any, Dict, List, Optional import pytest +from fluid_build.build_runners._deployment_host_policy import ALLOWLIST_ENV as HOST_ALLOWLIST +from fluid_build.build_runners._secret_policy import ALLOWLIST_ENV as SECRET_REF_ALLOWLIST from fluid_build.build_runners.debezium.iceberg_sink import emit_debezium_iceberg_sink_config from fluid_build.build_runners.kafka_connect.iceberg_sink import emit_iceberg_sink_config from fluid_build.providers._iceberg_catalog import ResolvedIcebergCatalog @@ -38,7 +41,22 @@ pytestmark = [pytest.mark.unit] -_KEYCLOAK = "http://keycloak:8080/realms/iceberg/protocol/openid-connect/token" + +@pytest.fixture(autouse=True) +def _operator_allows_the_fixture_hosts(request, monkeypatch): + """The operator pairs each fixture variable with each fixture host. Where a + credential may go is pinned in test_iceberg_sink_catalog_destinations.py.""" + if "kafka_connect_mock" in request.fixturenames: + # The mock may set the host list for its own server: extend it after. + request.getfixturevalue("kafka_connect_mock") + hosts = ("lakekeeper", "keycloak") + names = ("LAKEKEEPER_CREDENTIAL", "UC_TOKEN") + monkeypatch.setenv(SECRET_REF_ALLOWLIST, ",".join(f"{n}@{h}" for n in names for h in hosts)) + current = [h for h in os.environ.get(HOST_ALLOWLIST, "").split(",") if h.strip()] + monkeypatch.setenv(HOST_ALLOWLIST, ",".join([*current, *hosts])) + + +_KEYCLOAK = "https://keycloak:8443/realms/iceberg/protocol/openid-connect/token" _SECRET_VALUE = "spark:2OR3eRvYfSZzzZ16MlPd95jhLnOaLM52" # pragma: allowlist secret _OAUTH2 = { "type": "oauth2", @@ -52,7 +70,7 @@ catalog_type="rest", warehouse="demo", fq_table="bronze.orders", - uri="http://lakekeeper:8181/catalog", + uri="https://lakekeeper:8443/catalog", kind="lakekeeper", ) @@ -118,7 +136,12 @@ def test_emitters_are_deterministic_with_auth(): # ── Kafka Connect runner ──────────────────────────────────────────────────── -def _kc_contract(catalog_auth: Dict[str, Any], **kc: Any) -> Dict[str, Any]: +def _kc_contract( + catalog_auth: Optional[Dict[str, Any]], *, location: Optional[Dict[str, Any]] = None, **kc: Any +) -> Dict[str, Any]: + sink: Dict[str, Any] = {"format": "iceberg"} + if catalog_auth is not None: + sink["catalogAuth"] = catalog_auth return { "fluidVersion": "0.7.6", "kind": "DataProduct", @@ -137,7 +160,7 @@ def _kc_contract(catalog_auth: Dict[str, Any], **kc: Any) -> Dict[str, Any]: "mode": "incremental_append", "streams": ["public.orders"], }, - "sink": {"format": "iceberg", "catalogAuth": catalog_auth}, + "sink": sink, "kafka-connect": { "deployment": {"server_url": "http://kafka-connect.test:8083"}, "connector_name": "src", @@ -154,13 +177,17 @@ def _kc_contract(catalog_auth: Dict[str, Any], **kc: Any) -> Dict[str, Any]: "binding": { "platform": "local", "format": "iceberg", - "location": { - "catalog": "lakekeeper", - "uri": "http://lakekeeper:8181/catalog", - "warehouse": "demo", - "database": "bronze", - "table": "orders", - }, + "location": ( + dict(location) + if location is not None + else { + "catalog": "lakekeeper", + "uri": "https://lakekeeper:8443/catalog", + "warehouse": "demo", + "database": "bronze", + "table": "orders", + } + ), }, "contract": {"schema": []}, } @@ -216,8 +243,13 @@ def test_kafka_connect_runner_refuses_an_override_that_sets_auth( # ── embedded Debezium Server runner ───────────────────────────────────────── -def _dbz_contract(catalog_auth: Dict[str, Any], server_sink: Any = None) -> Dict[str, Any]: - contract = _kc_contract(catalog_auth) +def _dbz_contract( + catalog_auth: Optional[Dict[str, Any]], + server_sink: Any = None, + *, + location: Optional[Dict[str, Any]] = None, +) -> Dict[str, Any]: + contract = _kc_contract(catalog_auth, location=location) contract["id"] = "bronze.dbz_auth" build = contract["builds"][0] build["engine"] = "debezium" @@ -251,8 +283,8 @@ def __call__(self, argv: List[str], **kwargs: Any) -> subprocess.CompletedProces return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") -@pytest.fixture -def fake_server(monkeypatch): +def _install_fake_server(monkeypatch) -> _RecordingRun: + """Fake the Debezium Server binary and its start; returns the recorder.""" from fluid_build.build_runners.debezium import runner as dbz_runner recorder = _RecordingRun() @@ -261,6 +293,11 @@ def fake_server(monkeypatch): return recorder +@pytest.fixture +def fake_server(monkeypatch): + return _install_fake_server(monkeypatch) + + def test_embedded_writes_env_references_and_the_server_inherits_the_environment( tmp_path: Path, monkeypatch, fake_server, caplog ): diff --git a/tests/build_runners/test_catalog_auth_validation.py b/tests/build_runners/test_catalog_auth_validation.py index aa77af3b..88d9e8c8 100644 --- a/tests/build_runners/test_catalog_auth_validation.py +++ b/tests/build_runners/test_catalog_auth_validation.py @@ -23,10 +23,13 @@ from __future__ import annotations +import os from typing import Any, Dict, Optional import pytest +from fluid_build.build_runners._deployment_host_policy import ALLOWLIST_ENV as HOST_ALLOWLIST +from fluid_build.build_runners._secret_policy import ALLOWLIST_ENV as SECRET_REF_ALLOWLIST from fluid_build.build_runners.kafka_connect.iceberg_sink_validation import ( iceberg_sink_preflight, validate_iceberg_sink, @@ -35,7 +38,19 @@ pytestmark = [pytest.mark.unit] -_KEYCLOAK = "http://keycloak:8080/realms/iceberg/protocol/openid-connect/token" + +@pytest.fixture(autouse=True) +def _operator_allows_the_fixture_hosts(monkeypatch): + """The operator pairs each fixture variable with each fixture host. Where a + credential may go is pinned in test_iceberg_sink_catalog_destinations.py.""" + hosts = ("lakekeeper", "keycloak", "catalog", "c") + names = ("LAKEKEEPER_CREDENTIAL", "UC_TOKEN", "LK_CRED") + monkeypatch.setenv(SECRET_REF_ALLOWLIST, ",".join(f"{n}@{h}" for n in names for h in hosts)) + current = [h for h in os.environ.get(HOST_ALLOWLIST, "").split(",") if h.strip()] + monkeypatch.setenv(HOST_ALLOWLIST, ",".join([*current, *hosts])) + + +_KEYCLOAK = "https://keycloak:8443/realms/iceberg/protocol/openid-connect/token" _OAUTH2 = { "type": "oauth2", "credentialEnv": "LAKEKEEPER_CREDENTIAL", @@ -44,7 +59,7 @@ } _LAKEKEEPER_LOCATION = { "catalog": "lakekeeper", - "uri": "http://lakekeeper:8181/catalog", + "uri": "https://lakekeeper:8443/catalog", "warehouse": "demo", "database": "bronze", "table": "orders", @@ -114,7 +129,7 @@ def _dbz(catalog_auth: Optional[Dict[str, Any]], *, server_sink=None, mode="embe def test_every_rest_kind_accepts_catalog_auth(catalog): location = {**_LAKEKEEPER_LOCATION, "catalog": catalog} if catalog != "lakekeeper": - location["uri"] = "http://catalog:8181/api/catalog" + location["uri"] = "https://catalog:8443/api/catalog" errors, _warnings = validate_iceberg_sink(_kc(dict(_OAUTH2), location=location)) assert errors == [] @@ -194,7 +209,7 @@ def test_a_handwritten_sink_config_is_refused(contract, where): assert f"{prefix}rest.auth.type to oauth2" in hit[0] # A contract value is named by its field, not echoed (it could hold a secret). assert f"{prefix}oauth2-server-uri to " in hit[0] - assert "keycloak:8080" not in hit[0] + assert "keycloak:8443" not in hit[0] def test_a_handwritten_config_with_a_userinfo_uri_is_refused_without_echoing_it(): @@ -265,7 +280,7 @@ def test_debezium_override_of_an_auth_property_is_refused(key): def test_an_unrelated_override_key_is_not_a_conflict(): kc = { "iceberg_catalog_overrides": { - "iceberg.catalog.uri": "http://c:8181/catalog", + "iceberg.catalog.uri": "https://c:8443/catalog", "s3.region": "y", } } diff --git a/tests/build_runners/test_iceberg_sink_catalog_destinations.py b/tests/build_runners/test_iceberg_sink_catalog_destinations.py new file mode 100644 index 00000000..c0ccf278 --- /dev/null +++ b/tests/build_runners/test_iceberg_sink_catalog_destinations.py @@ -0,0 +1,675 @@ +# 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. + +"""Where a ``sink.catalogAuth`` credential may go, and what location text may say. + +A contract names the variable (``tokenEnv`` / ``credentialEnv``) and every host +it is sent to (``binding.location.uri``, ``oauth2ServerUri``). The Kafka Connect +worker or the embedded Debezium Server resolves ``env:`` from its own +environment, which for Debezium is this runner's whole environment, and sends +the value there. So the operator pairs the variable with each host +(``NAME@host``), a hostname needs a host-list entry (the remote JVM resolves it +itself, later), an IP literal keeps the deployment host rules, loopback is +exempt, and the credential travels over https only. Text from +``binding.location`` that reaches the derived sink config may not reference a +variable itself (``env:`` / ``${``) or hold a control character. + +Every refusal happens before any Connect REST call, ``application.properties`` +write or read of the variable. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any, Dict, List, Optional + +import pytest + +from fluid_build.build_runners._deployment_host_policy import ALLOWLIST_ENV as HOST_ALLOWLIST +from fluid_build.build_runners._secret_policy import ALLOWLIST_ENV as SECRET_REF_ALLOWLIST +from fluid_build.build_runners.kafka_connect.iceberg_sink_validation import ( + iceberg_sink_preflight, + validate_iceberg_sink, +) +from tests.build_runners import test_catalog_auth_sink as _sink_tests +from tests.build_runners.test_catalog_auth_sink import _RecordingRun + +pytestmark = [pytest.mark.unit] + +_SECRET = "OPERATOR_SYNTH_SECRET" # pragma: allowlist secret - a variable NAME +_SECRET_VALUE = "operator-synth-value-7Hq2" # pragma: allowlist secret +#: AWS's documented example secret access key: the shape a rendered secret takes. +_AWS_DOC_KEY = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" # pragma: allowlist secret +_CATALOG_HOST = "catalog.attacker.example" +_IDP_HOST = "idp.attacker.example" +_CATALOG = f"https://{_CATALOG_HOST}/catalog" +_IDP = f"https://{_IDP_HOST}/realms/iceberg/protocol/openid-connect/token" + +_AUTHS: Dict[str, Dict[str, str]] = { + "bearer": {"type": "bearer", "tokenEnv": _SECRET}, + "oauth2": { + "type": "oauth2", + "credentialEnv": _SECRET, + "oauth2ServerUri": _IDP, + "scope": "lakekeeper", + }, +} +#: The property each type's secret reference lands in. +_SECRET_PROPERTY = {"bearer": "token", "oauth2": "credential"} + + +def _hosts(auth: str) -> List[str]: + """Every host the ``auth`` credential reaches: the catalog, and an OAuth2 IdP.""" + return [_CATALOG_HOST, _IDP_HOST] if auth == "oauth2" else [_CATALOG_HOST] + + +@pytest.fixture(autouse=True) +def _operator_environment(monkeypatch): + """No operator entry unless a test adds one, whatever the shell exports. The + variable is set, so the embedded Debezium preflight's own "set?" check + passes. Autouse runs first, so a mock fixture's own host entry survives.""" + monkeypatch.delenv(SECRET_REF_ALLOWLIST, raising=False) + monkeypatch.delenv(HOST_ALLOWLIST, raising=False) + monkeypatch.setenv(_SECRET, _SECRET_VALUE) + + +def _operator_allows(monkeypatch, *, pairs=(), hosts=()) -> None: + """Set the operator's lists. Host entries are ADDED: a fixture may list its + own synthetic Connect host there.""" + if pairs: + monkeypatch.setenv(SECRET_REF_ALLOWLIST, ",".join(pairs)) + if hosts: + current = [h for h in os.environ.get(HOST_ALLOWLIST, "").split(",") if h.strip()] + monkeypatch.setenv(HOST_ALLOWLIST, ",".join([*current, *hosts])) + + +def _allow_everything(monkeypatch, auth: str) -> None: + hosts = _hosts(auth) + _operator_allows(monkeypatch, pairs=[f"{_SECRET}@{h}" for h in hosts], hosts=hosts) + + +def _location(uri: str = _CATALOG, **extra: Any) -> Dict[str, Any]: + return { + "catalog": "lakekeeper", + "uri": uri, + "warehouse": "demo", + "database": "bronze", + "table": "orders", + **extra, + } + + +def _kc_contract( + catalog_auth: Optional[Dict[str, Any]] = None, + *, + location: Optional[Dict[str, Any]] = None, + **kc: Any, +) -> Dict[str, Any]: + """The catalogAuth sink tests' Kafka Connect contract, aimed at a remote catalog.""" + return _sink_tests._kc_contract( + catalog_auth, location=location if location is not None else _location(), **kc + ) + + +def _dbz_contract( + catalog_auth: Optional[Dict[str, Any]] = None, + *, + location: Optional[Dict[str, Any]] = None, + server_sink: Optional[Dict[str, Any]] = None, +) -> Dict[str, Any]: + """The catalogAuth sink tests' embedded Debezium contract, aimed at a remote catalog.""" + return _sink_tests._dbz_contract( + catalog_auth, server_sink, location=location if location is not None else _location() + ) + + +@pytest.fixture +def fake_server(monkeypatch) -> _RecordingRun: + return _sink_tests._install_fake_server(monkeypatch) + + +class _Engine: + """One streaming runner: builds its contract, runs it, reads what it delivered.""" + + def __init__(self, name: str, kc_mock: Any, server: _RecordingRun, tmp_path: Path) -> None: + self.name = name + self._kc_mock = kc_mock + self._server = server + self._tmp_path = tmp_path + + def contract(self, catalog_auth=None, **kwargs: Any) -> Dict[str, Any]: + if self.name == "kafka-connect": + return _kc_contract(catalog_auth, **kwargs) + return _dbz_contract(catalog_auth, **kwargs) + + def run(self, contract: Dict[str, Any]) -> int: + if self.name == "kafka-connect": + from fluid_build.build_runners.kafka_connect.runner import execute_kafka_connect_build + + return execute_kafka_connect_build(contract["builds"][0], contract, self._tmp_path) + from fluid_build.build_runners.debezium.runner import execute_debezium_build + + return execute_debezium_build(contract["builds"][0], contract, self._tmp_path) + + def _properties(self, contract: Dict[str, Any]) -> Path: + return ( + self._tmp_path + / ".fluid" + / "debezium" + / contract["id"] + / "ingest" + / "application.properties" + ) + + def touched(self, contract: Dict[str, Any]) -> bool: + """Did the run reach Connect, write application.properties or start the server?""" + if self.name == "kafka-connect": + return bool(self._kc_mock.calls) + return self._properties(contract).exists() or bool(self._server.calls) + + def delivered(self, contract: Dict[str, Any]) -> Dict[str, str]: + """The catalog properties the run pushed or wrote, as bare keys.""" + if self.name == "kafka-connect": + cfg = self._kc_mock.connectors["src-sink"]["config"] + prefix = "iceberg.catalog." + else: + lines = self._properties(contract).read_text().splitlines() + cfg = dict(line.split("=", 1) for line in lines if "=" in line) + prefix = "debezium.sink.iceberg." + return {k[len(prefix) :]: v for k, v in cfg.items() if k.startswith(prefix)} + + +@pytest.fixture(params=["kafka-connect", "debezium"]) +def engine(request, kafka_connect_mock, fake_server, tmp_path) -> _Engine: + return _Engine(request.param, kafka_connect_mock, fake_server, tmp_path) + + +# ── the confirmed defect: a contract-chosen variable to a contract-chosen host ── + + +@pytest.mark.parametrize("auth", sorted(_AUTHS)) +def test_an_unpaired_variable_is_refused_before_anything_is_sent(engine, auth): + contract = engine.contract(dict(_AUTHS[auth])) + assert engine.run(contract) == 1 + assert not engine.touched(contract) + + +@pytest.mark.parametrize("auth", sorted(_AUTHS)) +def test_listed_pairs_and_hosts_reach_the_sink(engine, auth, monkeypatch): + _allow_everything(monkeypatch, auth) + contract = engine.contract(dict(_AUTHS[auth])) + assert engine.run(contract) == 0 + delivered = engine.delivered(contract) + assert delivered[_SECRET_PROPERTY[auth]] == f"env:{_SECRET}" + assert delivered["uri"] == _CATALOG + + +@pytest.mark.parametrize("engine_name", ["kafka-connect", "debezium"]) +def test_oauth2_pairs_the_variable_with_the_token_endpoint_too(engine_name, monkeypatch): + """The IdP receives the credential itself, the catalog a token minted from it.""" + _operator_allows( + monkeypatch, pairs=[f"{_SECRET}@{_CATALOG_HOST}"], hosts=[_CATALOG_HOST, _IDP_HOST] + ) + build = _kc_contract if engine_name == "kafka-connect" else _dbz_contract + error = iceberg_sink_preflight(build(dict(_AUTHS["oauth2"])), "ingest") + assert error is not None + assert f'{SECRET_REF_ALLOWLIST}="{_SECRET}@{_CATALOG_HOST},{_SECRET}@{_IDP_HOST}"' in error + assert "OAuth2 token endpoint" in error + + +@pytest.mark.parametrize("auth", sorted(_AUTHS)) +def test_a_paired_hostname_still_needs_a_host_entry(auth, monkeypatch): + """The remote JVM resolves the name itself, later: only the operator can vouch for it.""" + _operator_allows(monkeypatch, pairs=[f"{_SECRET}@{h}" for h in _hosts(auth)]) + error = iceberg_sink_preflight(_kc_contract(dict(_AUTHS[auth])), "ingest") + assert error is not None + assert f'{HOST_ALLOWLIST}="' in error + assert _CATALOG_HOST in error + + +def test_a_bare_name_entry_does_not_open_every_host(monkeypatch): + """``NAME`` (any host) still leaves the destination to the host list.""" + _operator_allows(monkeypatch, pairs=[_SECRET]) + assert iceberg_sink_preflight(_kc_contract(dict(_AUTHS["bearer"])), "ingest") is not None + _operator_allows(monkeypatch, hosts=[_CATALOG_HOST]) + assert iceberg_sink_preflight(_kc_contract(dict(_AUTHS["bearer"])), "ingest") is None + + +def test_a_parent_domain_entry_covers_the_catalog_host(monkeypatch): + _operator_allows(monkeypatch, pairs=[f"{_SECRET}@{_CATALOG_HOST}"], hosts=["attacker.example"]) + assert iceberg_sink_preflight(_kc_contract(dict(_AUTHS["bearer"])), "ingest") is None + + +@pytest.mark.parametrize("auth", sorted(_AUTHS)) +def test_a_loopback_catalog_needs_no_entry(engine, auth): + raw = dict(_AUTHS[auth]) + if auth == "oauth2": + raw["oauth2ServerUri"] = ( + "https://127.0.0.1:8443/realms/iceberg/protocol/openid-connect/token" + ) + contract = engine.contract(raw, location=_location("https://localhost:8443/catalog")) + assert engine.run(contract) == 0 + assert engine.delivered(contract)[_SECRET_PROPERTY[auth]] == f"env:{_SECRET}" + + +def test_validate_warns_where_the_run_refuses(): + """``fluid validate`` runs where the operator's lists may not be; the run is where.""" + contract = _kc_contract(dict(_AUTHS["bearer"])) + errors, warnings = validate_iceberg_sink(contract) + assert errors == [] + assert any(f"{_SECRET}@{_CATALOG_HOST}" in w for w in warnings) + error = iceberg_sink_preflight(contract, "ingest") + assert error is not None + assert error.startswith("iceberg sink preflight failed: ") # run-only, no "see validate" + # One export line sets both, ready to paste. + assert ( + f'export {SECRET_REF_ALLOWLIST}="{_SECRET}@{_CATALOG_HOST}" ' f'{HOST_ALLOWLIST}="' in error + ) + assert _SECRET_VALUE not in error + + +def test_the_suggestion_keeps_the_operators_entries(monkeypatch): + _operator_allows(monkeypatch, pairs=["OTHER@db.example.com"], hosts=["db.example.com"]) + error = iceberg_sink_preflight(_kc_contract(dict(_AUTHS["bearer"])), "ingest") + assert error is not None + assert f'{SECRET_REF_ALLOWLIST}="OTHER@db.example.com,{_SECRET}@{_CATALOG_HOST}"' in error + assert f'{HOST_ALLOWLIST}="db.example.com,{_CATALOG_HOST}"' in error + + +def test_a_refused_variable_is_never_read(monkeypatch, fake_server, tmp_path): + """The embedded preflight reads the variable to test it is set; not when refused.""" + from fluid_build.build_runners.debezium.runner import execute_debezium_build + + registered: List[Any] = [] + monkeypatch.setattr( + "fluid_build.observability.secret_redactor.register_secret", registered.append + ) + contract = _dbz_contract(dict(_AUTHS["bearer"])) + assert execute_debezium_build(contract["builds"][0], contract, tmp_path) == 1 + assert registered == [] + + +def test_the_pairing_follows_an_override_uri(monkeypatch): + """The credential goes to the uri that reaches the sink, not to the binding's.""" + _allow_everything(monkeypatch, "bearer") + kc = _kc_contract( + dict(_AUTHS["bearer"]), + iceberg_catalog_overrides={"iceberg.catalog.uri": "https://other.attacker.example/c"}, + ) + error = iceberg_sink_preflight(kc, "ingest") + assert error is not None and "other.attacker.example" in error + dbz = _dbz_contract( + dict(_AUTHS["bearer"]), + server_sink={ + "type": "iceberg", + "iceberg_sink_enabled": True, + "config": {"uri": "https://other.attacker.example/c"}, + }, + ) + error = iceberg_sink_preflight(dbz, "ingest") + assert error is not None and "other.attacker.example" in error + + +# ── https ─────────────────────────────────────────────────────────────────── + + +@pytest.mark.parametrize( + "auth, field", + [("bearer", "uri"), ("oauth2", "uri"), ("oauth2", "oauth2ServerUri")], +) +@pytest.mark.parametrize("engine_name", ["kafka-connect", "debezium"]) +def test_a_credential_travels_over_https_only(auth, field, engine_name): + """Loopback included, as on the managed Flink lane: refused by ``fluid validate``.""" + raw = dict(_AUTHS[auth]) + location = _location("https://localhost:8443/catalog") + if field == "uri": + location["uri"] = "http://localhost:8181/catalog" + else: + raw["oauth2ServerUri"] = "http://127.0.0.1:8080/realms/x/protocol/openid-connect/token" + build = _kc_contract if engine_name == "kafka-connect" else _dbz_contract + errors, _warnings = validate_iceberg_sink(build(raw, location=location)) + label = "sink.catalogAuth.oauth2ServerUri" if field != "uri" else "binding.location.uri" + assert any(label in e and "https://" in e for e in errors), errors + + +def test_an_unauthenticated_http_catalog_is_unchanged(): + contract = _kc_contract(None, location=_location("http://lakekeeper:8181/catalog")) + assert validate_iceberg_sink(contract) == ([], []) + assert iceberg_sink_preflight(contract, "ingest") is None + + +# ── IP literals keep the deployment host rules ───────────────────────────── + + +@pytest.mark.parametrize( + "host", ["169.254.169.254", "[fe80::1]", "0.0.0.0", "[::ffff:169.254.169.254]"] +) +def test_a_link_local_catalog_is_refused_even_when_listed(host, monkeypatch): + bare = host.strip("[]") + _operator_allows(monkeypatch, pairs=[_SECRET, f"{_SECRET}@{bare}"], hosts=[bare]) + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location(f"https://{host}/catalog")) + errors, _warnings = validate_iceberg_sink(contract) + assert any("link-local" in e for e in errors), errors + + +def test_a_public_ip_needs_a_host_entry(monkeypatch): + contract = _kc_contract( + dict(_AUTHS["bearer"]), location=_location("https://93.184.216.34/catalog") + ) + _operator_allows(monkeypatch, pairs=[f"{_SECRET}@93.184.216.34"]) + error = iceberg_sink_preflight(contract, "ingest") + assert error is not None and f'{HOST_ALLOWLIST}="' in error + _operator_allows(monkeypatch, hosts=["93.184.216.34"]) + assert iceberg_sink_preflight(contract, "ingest") is None + + +def test_a_private_ip_needs_only_the_pairing(monkeypatch): + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location("https://10.0.0.5/catalog")) + assert iceberg_sink_preflight(contract, "ingest") is not None + _operator_allows(monkeypatch, pairs=[f"{_SECRET}@10.0.0.5"]) + assert iceberg_sink_preflight(contract, "ingest") is None + + +# ── binding.location text in the derived sink config ─────────────────────── + + +_REFERENCE_TEXT = [ + ("region", f"env:{_SECRET}"), + ("warehouse", f"env:{_SECRET}"), + ("uri", f"env:{_SECRET}"), + ("warehouse", f"${{{_SECRET}}}"), + ("uri", f"https://${{{_SECRET}}}.attacker.example/catalog"), + ("database", f"${{env:{_SECRET}}}"), + ("warehouse", "demo\nx"), +] + + +@pytest.mark.parametrize("field, value", _REFERENCE_TEXT) +def test_location_text_may_not_reference_a_variable(engine, field, value): + contract = engine.contract(None, location=_location(**{field: value})) + errors, _warnings = validate_iceberg_sink(contract) + mine = [e for e in errors if f"binding.location.{field}" in e and "env:" in e] + assert mine, errors + assert not any(_SECRET in e for e in mine) # the value is not echoed + assert engine.run(contract) == 1 + assert not engine.touched(contract) + + +def test_kafka_connect_table_and_partition_text_is_checked_too(): + contract = _kc_contract(None, location=_location(table=f"${{env:{_SECRET}}}")) + assert any("binding.location.table" in e for e in validate_iceberg_sink(contract)[0]) + contract = _kc_contract(None, location=_location(partitionBy=[f"env:{_SECRET}"])) + assert any("binding.location.partitionBy" in e for e in validate_iceberg_sink(contract)[0]) + + +def test_bigquery_catalog_properties_are_checked(): + location = { + "catalog": "bigquery", + "project": f"env:{_SECRET}", + "dataset": "bronze", + "table": "orders", + "bucket": "lake", + } + contract = _kc_contract(None, location=location) + contract["exposes"][0]["binding"]["platform"] = "gcp" + errors, _warnings = validate_iceberg_sink(contract) + assert any("binding.location.project" in e for e in errors), errors + + +@pytest.mark.parametrize( + "region, accepted", + [ + ("eu-central-1", True), + ("us-gov-west-1", True), + ("westeurope", True), + ("EU", True), # a host label ignores case + ("attacker.example", False), + ("us-east-1/", False), + ("us-east-1#", False), + ("-eu", False), + ("a" * 64, False), + ], +) +@pytest.mark.parametrize("engine_name", ["kafka-connect", "debezium"]) +def test_client_region_is_a_host_label(region, accepted, engine_name): + """``client.region`` becomes part of the S3 and Glue endpoint hosts.""" + build = _kc_contract if engine_name == "kafka-connect" else _dbz_contract + errors, _warnings = validate_iceberg_sink(build(None, location=_location(region=region))) + refused = any("binding.location.region" in e for e in errors) + assert refused is not accepted, errors + + +def _rendered(contract: Dict[str, Any]) -> Dict[str, Any]: + """The contract as the acquisition dispatcher hands it to a runner.""" + from fluid_build.build_runners.base import _resolve_env_placeholders + + return _resolve_env_placeholders(contract) + + +@pytest.mark.parametrize( + "rendered_to, refused", + [("eu-central-1", False), ("attacker.example", True)], +) +def test_a_templated_region_is_checked_where_it_is_rendered(monkeypatch, rendered_to, refused): + """``fluid validate`` sees the template; the run's preflight sees the value.""" + contract = _kc_contract(None, location=_location(region="{{ env.LAKE_REGION }}")) + assert not any("binding.location.region" in e for e in validate_iceberg_sink(contract)[0]) + monkeypatch.setenv("LAKE_REGION", rendered_to) + error = iceberg_sink_preflight(_rendered(contract), "ingest") + assert (error is not None and "binding.location.region" in error) is refused + + +def test_a_templated_catalog_uri_is_checked_where_it_is_rendered(monkeypatch): + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location("{{ env.CATALOG_URI }}")) + errors, warnings = validate_iceberg_sink(contract) + assert errors == [] + assert not any(_SECRET in w for w in warnings) + monkeypatch.setenv("CATALOG_URI", _CATALOG) + error = iceberg_sink_preflight(_rendered(contract), "ingest") + assert error is not None and f"{_SECRET}@{_CATALOG_HOST}" in error + _allow_everything(monkeypatch, "bearer") + assert iceberg_sink_preflight(_rendered(contract), "ingest") is None + + +def test_a_handwritten_sink_config_reads_no_location_text(): + """Nothing is derived, so the binding's text never reaches the sink config.""" + contract = _kc_contract( + None, + location=_location(region="attacker.example"), + sink_connector_config={"connector.class": "x"}, + ) + assert not any("binding.location.region" in e for e in validate_iceberg_sink(contract)[0]) + + +# ── a host the refusal can repeat safely ─────────────────────────────────── + + +@pytest.mark.parametrize( + "uri", + [ + 'https://x"$(id)".attacker.example/catalog', + "https://x,OTHER_VAR.attacker.example/catalog", + "https://faß.example/catalog", + "https://[fd00::1%25eth0]/catalog", + ], + ids=["shell", "comma", "non-ascii", "zone-id"], +) +def test_a_host_that_is_not_plain_is_refused_and_never_echoed(uri, monkeypatch): + """The refusal's export line repeats the host, so it must be safe to paste.""" + _operator_allows(monkeypatch, pairs=[_SECRET], hosts=["attacker.example", "example"]) + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location(uri)) + errors, _warnings = validate_iceberg_sink(contract) + mine = [e for e in errors if "not a plain DNS name or IP address" in e] + assert mine, errors + error = iceberg_sink_preflight(contract, "ingest") + assert error is not None + for text in (*mine, error): + assert "export" not in text + assert "$(id)" not in text and "other_var" not in text.lower() + assert "faß" not in text and "eth0" not in text + + +@pytest.mark.parametrize( + "uri", + [ + "https://evil.example\\@127.0.0.1/catalog", + "https://u:p4ss@catalog.attacker.example/c", # pragma: allowlist secret + ], + ids=["backslash", "userinfo"], +) +def test_a_user_password_part_is_refused(uri): + """Python and the JVM read such an authority differently; it never reaches a check.""" + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location(uri)) + errors, _warnings = validate_iceberg_sink(contract) + mine = [e for e in errors if "user:password@ part" in e] + assert mine, errors + assert not any("p4ss" in e for e in mine) + + +def test_an_engine_that_derives_no_sink_config_skips_the_location_text(): + """Only the Kafka Connect and Debezium runners derive the config from the binding.""" + contract = _kc_contract(None, location=_location(warehouse=f"env:{_SECRET}")) + contract["builds"][0]["engine"] = "airbyte" + errors, _warnings = validate_iceberg_sink(contract) + assert not any("binding.location.warehouse" in e for e in errors), errors + + +def test_a_rendered_region_is_not_echoed(monkeypatch): + """At run a region may hold whatever a {{ env.* }} template rendered.""" + contract = _kc_contract(None, location=_location(region="{{ env.LAKE_REGION }}")) + monkeypatch.setenv("LAKE_REGION", _AWS_DOC_KEY) + error = iceberg_sink_preflight(_rendered(contract), "ingest") + assert error is not None and "binding.location.region" in error + assert _AWS_DOC_KEY[:13] not in error + + +def test_validate_defers_a_template_only_where_it_decides_the_host(): + """A template in the path leaves the scheme and host to check now.""" + raw = dict(_AUTHS["bearer"]) + contract = _kc_contract(raw, location=_location("http://169.254.169.254/{{ env.X }}")) + errors, _warnings = validate_iceberg_sink(contract) + assert any("must be an https:// URL" in e for e in errors), errors + + +def test_a_catalog_not_reached_over_rest_never_reads_the_variable(monkeypatch): + """catalogAuth on Glue is already refused; the embedded preflight reads nothing.""" + registered: List[Any] = [] + monkeypatch.setattr( + "fluid_build.observability.secret_redactor.register_secret", registered.append + ) + location = {"database": "bronze", "table": "orders", "bucket": "lake", "region": "us-east-1"} + contract = _dbz_contract(dict(_AUTHS["bearer"]), location=location) + contract["exposes"][0]["binding"]["platform"] = "aws" + error = iceberg_sink_preflight(contract, "ingest") + assert error is not None and "not reached over REST" in error + assert registered == [] + + +@pytest.mark.parametrize("engine_name", ["kafka-connect", "debezium"]) +def test_sink_partition_by_is_the_text_checked_when_set(engine_name): + """The runner's SinkSpec reads sink.partitionBy over the binding's.""" + build = _kc_contract if engine_name == "kafka-connect" else _dbz_contract + contract = build(None, location=_location(partitionBy=["day"])) + contract["builds"][0]["properties"]["sink"]["partitionBy"] = [f"${{env:{_SECRET}}}"] + errors, _warnings = validate_iceberg_sink(contract) + assert any("sink.partitionBy" in e for e in errors), errors + contract = build(None, location=_location(partitionBy=[f"env:{_SECRET}"])) + contract["builds"][0]["properties"]["sink"]["partitionBy"] = ["day"] + assert not any("partitionBy" in e for e in validate_iceberg_sink(contract)[0]) + + +def test_only_the_namespace_field_the_sink_reads_is_checked(): + """iceberg_namespace reads the first one set; the others reach nothing.""" + contract = _kc_contract(None, location=_location(dataset=f"${{{_SECRET}}}")) + assert not any("dataset" in e for e in validate_iceberg_sink(contract)[0]) + location = _location() + del location["database"] + contract = _kc_contract(None, location={**location, "dataset": f"${{{_SECRET}}}"}) + assert any("binding.location.dataset" in e for e in validate_iceberg_sink(contract)[0]) + + +def test_a_written_out_scheme_is_checked_while_the_host_waits_on_a_template(): + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location("http://{{ env.H }}/c")) + errors, _warnings = validate_iceberg_sink(contract) + assert any("must be an https:// URL" in e for e in errors), errors + + +def test_a_percent_suffix_does_not_make_a_name_an_address(monkeypatch): + """10.0.0.1%25.attacker.example is a name, and not a plain one.""" + _operator_allows(monkeypatch, pairs=[_SECRET]) + contract = _kc_contract( + dict(_AUTHS["bearer"]), location=_location("https://10.0.0.1%25.attacker.example/c") + ) + error = iceberg_sink_preflight(contract, "ingest") + assert error is not None and "not a plain DNS name or IP address" in error + + +def test_a_derived_warehouse_names_where_it_came_from(): + location = {"database": "bronze", "table": "orders", "bucket": "lake", "path": "p/${X}/"} + contract = _kc_contract(None, location=location) + contract["exposes"][0]["binding"]["platform"] = "aws" + errors, _warnings = validate_iceberg_sink(contract) + assert any("the warehouse derived from binding.location" in e for e in errors), errors + + +@pytest.mark.parametrize( + "uri", ["https:///catalog", "https://[::1/catalog"], ids=["empty", "bracket"] +) +def test_a_destination_with_no_readable_host_is_refused(uri): + contract = _kc_contract(dict(_AUTHS["bearer"]), location=_location(uri)) + errors, _warnings = validate_iceberg_sink(contract) + assert any("names no host" in e for e in errors), errors + + +@pytest.mark.parametrize( + "token_endpoint, refused", + [ + (_IDP, "a remote IdP beside a loopback catalog"), + ("https://169.254.169.254/token", "a link-local IdP"), + ("https://[::]/token", "an unspecified IdP"), + ], +) +def test_the_token_endpoint_is_judged_on_its_own(token_endpoint, refused): + raw = {**_AUTHS["oauth2"], "oauth2ServerUri": token_endpoint} + contract = _kc_contract(raw, location=_location("https://localhost:8443/catalog")) + error = iceberg_sink_preflight(contract, "ingest") + assert error is not None and "OAuth2 token endpoint" in error, refused + + +@pytest.mark.parametrize("engine_name", ["kafka-connect", "debezium"]) +def test_validate_warns_for_both_engines_and_both_types(engine_name): + build = _kc_contract if engine_name == "kafka-connect" else _dbz_contract + for auth in sorted(_AUTHS): + errors, warnings = validate_iceberg_sink(build(dict(_AUTHS[auth]))) + assert errors == [], (auth, errors) + assert any(f"{_SECRET}@{_CATALOG_HOST}" in w for w in warnings), (auth, warnings) + + +@pytest.mark.parametrize("region, refused", [("eu-west-1", False), ("eu-west-1.evil", True)]) +def test_a_glue_region_is_a_host_label(region, refused): + """Glue is where client.region builds the AWS endpoint hosts.""" + location = {"database": "bronze", "table": "orders", "bucket": "lake", "region": region} + contract = _kc_contract(None, location=location) + contract["exposes"][0]["binding"]["platform"] = "aws" + errors, _warnings = validate_iceberg_sink(contract) + assert any("binding.location.region" in e for e in errors) is refused, errors + + +def test_sink_partition_by_is_read_as_sinkspec_reads_it(): + """A mapping reaches the config as its keys (SinkSpec.from_dict lists it).""" + contract = _kc_contract(None, location=_location(partitionBy=["day"])) + contract["builds"][0]["properties"]["sink"]["partitionBy"] = {f"${{env:{_SECRET}}}": 1} + errors, _warnings = validate_iceberg_sink(contract) + assert any("sink.partitionBy" in e for e in errors), errors diff --git a/tests/integration/test_lakekeeper_oauth_live.py b/tests/integration/test_lakekeeper_oauth_live.py index 277ca0bc..60761f86 100644 --- a/tests/integration/test_lakekeeper_oauth_live.py +++ b/tests/integration/test_lakekeeper_oauth_live.py @@ -145,6 +145,19 @@ ), ] +#: The three tests that derive a sink config. Since the Iceberg sink destination +#: policy (CHANGELOG Unreleased, Security), a ``sink.catalogAuth`` credential +#: reaches only https hosts, a loopback one included, and this stack serves +#: Lakekeeper and Keycloak over http. The founder chose (2026-11-04) to skip them +#: until the stack runs TLS (Trello XgF5xhrM, "Live Lakekeeper OAuth2 test: run +#: the stack over TLS"). The fourth test talks to Lakekeeper directly. +_NEEDS_TLS_STACK = pytest.mark.skip( + reason=( + "catalogAuth hosts must be https; this stack serves Lakekeeper and Keycloak over " + "http until the TLS follow-up lands" + ) +) + _REALM = "fluid" _CLIENT_ID = "fluid-sink" #: The client scope whose Audience mapper adds :data:`_AUDIENCE` to ``aud``. @@ -593,6 +606,7 @@ def _wait_failed(stack: _AuthStack, name: str, *, deadline_s: float = 180) -> Di # --------------------------------------------------------------------------- +@_NEEDS_TLS_STACK def test_catalog_auth_oauth2_streams_into_an_auth_required_lakekeeper( request: pytest.FixtureRequest, ): @@ -647,6 +661,7 @@ def test_catalog_auth_oauth2_streams_into_an_auth_required_lakekeeper( _delete_connector(stack, connector) +@_NEEDS_TLS_STACK def test_stored_connector_config_holds_the_reference_not_the_secret( auth_stack: _AuthStack, request: pytest.FixtureRequest ): @@ -697,6 +712,7 @@ def test_stored_connector_config_holds_the_reference_not_the_secret( _delete_connector(stack, connector) +@_NEEDS_TLS_STACK def test_unset_credential_variable_fails_the_task_and_writes_nothing( auth_stack: _AuthStack, request: pytest.FixtureRequest ): From 188096b7291875113dfbf04fc7f44990d746b9cb Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 4 Nov 2026 07:07:37 +0100 Subject: [PATCH 4/4] security(credentials): list litellm's per-kind endpoint routes, and say why Databricks M2M is refused (#822) tests/test_litellm_endpoint_names.py read only names ending in _API_BASE, so DASHSCOPE_, QWENCLOUD_ and QWEN_AI_PLATFORM_API_BASE_{IMAGE,RERANK,VIDEO} (read by litellm's dashscope provider code with get_secret_str; the VIDEO three are new in 1.108.0, the rest date from 1.83.14 to 1.105.0) were never in LITELLM_ENDPOINT_NAMES. The check now reads a kind after _API_BASE and the nine are listed; with the wider check alone, the drift test fails on litellm 1.108.0. Nothing reached them: FLUID calls only litellm.completion. The Databricks model-serving group also lists DATABRICKS_CLIENT_SECRET: litellm's OAuth M2M path (llms/databricks/common_utils.py) takes the workspace from DATABRICKS_API_BASE and posts the secret there. The name-shape rule already refused a project DATABRICKS_API_BASE while the operator holds that secret; a new test pins it, and the group names the reason. Both found by an Opus review of #820. --- CHANGELOG.md | 9 +++++++++ fluid_build/credentials/litellm_endpoints.py | 12 ++++++++++- fluid_build/credentials/protected_env.py | 13 ++++++++++-- tests/test_litellm_endpoint_names.py | 21 ++++++++++++++++++-- 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a3002e0..5de747b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -658,6 +658,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 project dotenv file cannot point them elsewhere while `FLUID_LLM_API_KEY` or a stored key is in play, as for every other provider endpoint. +- **The litellm endpoint check could not see a provider's per-kind route.** It looked for names + ending in `_API_BASE`, so the nine `DASHSCOPE_`, `QWENCLOUD_` and `QWEN_AI_PLATFORM_API_BASE_IMAGE`, + `_RERANK` and `_VIDEO` names litellm reads (three of them new in 1.108.0) were never listed, and + a project dotenv file could set them unpaired. They are listed now and the check reads a kind + after `_API_BASE`. FLUID calls only `litellm.completion`, which none of them routes, so nothing + reached them. The Databricks model-serving group also names `DATABRICKS_CLIENT_SECRET`, which + litellm's OAuth M2M path posts to the workspace `DATABRICKS_API_BASE` names; the name-shape rule + already refused that pairing, and the group now says why. + ### Upgrade notes - New validate warning, so `--strict` fails: a contract with builds whose `contract.id` or a diff --git a/fluid_build/credentials/litellm_endpoints.py b/fluid_build/credentials/litellm_endpoints.py index 53d71d68..598d7de2 100644 --- a/fluid_build/credentials/litellm_endpoints.py +++ b/fluid_build/credentials/litellm_endpoints.py @@ -30,7 +30,8 @@ * ``_API_BASE`` for every ``litellm.provider_list`` entry (``-`` read as ``_``); -* every ``*_API_BASE`` name its provider code reads; +* every ``*_API_BASE`` name its provider code reads, with a kind after it + too (``DASHSCOPE_API_BASE_VIDEO``, ``QWENCLOUD_API_BASE_RERANK``); * every name its provider code reads through ``get_secret``, ``get_secret_str``, ``get_secret_bool``, ``os.getenv`` or ``os.environ`` that ends in ``_URL``, ``_ENDPOINT``, ``_HOST``, ``_BASE``, ``_ACCOUNT_ID`` @@ -110,6 +111,9 @@ "CUSTOM_OPENAI_API_BASE", "DARKBLOOM_API_BASE", "DASHSCOPE_API_BASE", + "DASHSCOPE_API_BASE_IMAGE", + "DASHSCOPE_API_BASE_RERANK", + "DASHSCOPE_API_BASE_VIDEO", "DATABRICKS_AGENT_API_BASE", "DATABRICKS_API_BASE", "DATAFORSEO_API_BASE", @@ -220,7 +224,13 @@ "PRISM_API_BASE", "PUBLICAI_API_BASE", "QWENCLOUD_API_BASE", + "QWENCLOUD_API_BASE_IMAGE", + "QWENCLOUD_API_BASE_RERANK", + "QWENCLOUD_API_BASE_VIDEO", "QWEN_AI_PLATFORM_API_BASE", + "QWEN_AI_PLATFORM_API_BASE_IMAGE", + "QWEN_AI_PLATFORM_API_BASE_RERANK", + "QWEN_AI_PLATFORM_API_BASE_VIDEO", "RAGFLOW_API_BASE", "RECRAFT_API_BASE", "REDUCTO_API_BASE", diff --git a/fluid_build/credentials/protected_env.py b/fluid_build/credentials/protected_env.py index 9c425457..126c8e39 100644 --- a/fluid_build/credentials/protected_env.py +++ b/fluid_build/credentials/protected_env.py @@ -1256,9 +1256,18 @@ class EndpointGroup: EndpointGroup( service="Databricks model serving", endpoints=("DATABRICKS_API_BASE",), - credentials=("DATABRICKS_API_KEY", "DATABRICKS_TOKEN", _LLM_KEY), + credentials=( + "DATABRICKS_API_KEY", + "DATABRICKS_TOKEN", + # litellm's OAuth M2M path posts it to the workspace this base names + "DATABRICKS_CLIENT_SECRET", + _LLM_KEY, + ), ambient=True, - read_by=f"litellm's databricks/ route (DATABRICKS_API_BASE); {_LLM_KEYRING}", + read_by=( + "litellm's databricks/ route (DATABRICKS_API_BASE, and its OAuth M2M " + f"token request with DATABRICKS_CLIENT_SECRET); {_LLM_KEYRING}" + ), paired_by=((_LLM_KEY,),), ), EndpointGroup( diff --git a/tests/test_litellm_endpoint_names.py b/tests/test_litellm_endpoint_names.py index c8b22d4a..ee86591c 100644 --- a/tests/test_litellm_endpoint_names.py +++ b/tests/test_litellm_endpoint_names.py @@ -80,6 +80,18 @@ def test_a_project_s_own_api_base_loads_with_its_own_key(tmp_path: Path) -> None assert os.environ["MYAPP_API_KEY"] == PROJECT_KEY +def test_databricks_api_base_is_refused_while_the_operator_holds_the_m2m_secret() -> None: + # litellm's Databricks OAuth M2M path takes the workspace from + # DATABRICKS_API_BASE and posts DATABRICKS_CLIENT_SECRET there + # (litellm 1.108.0 llms/databricks/common_utils.py), whatever key pairs it. + refused = unpaired_endpoints( + {"DATABRICKS_API_BASE": "https://evil.test", "FLUID_LLM_API_KEY": PROJECT_KEY}, + {"DATABRICKS_CLIENT_SECRET": OPERATOR_KEY}, + ) + + assert "DATABRICKS_CLIENT_SECRET" in refused["DATABRICKS_API_BASE"].operator_credentials + + def test_a_project_api_base_still_follows_the_name_shape_rule() -> None: # The operator's own MYAPP_API_KEY would go to the project's host. refused = unpaired_endpoints( @@ -102,10 +114,13 @@ def test_a_project_api_base_still_follows_the_name_shape_rule() -> None: "TENSORMESH_SERVERLESS_BASE_URL", "ANTHROPIC_AWS_BASE_URL", "COMETAPI_BASE_URL", + # a provider's route for one kind of call + "DASHSCOPE_API_BASE_VIDEO", + "QWENCLOUD_API_BASE_RERANK", ], ) def test_a_litellm_provider_endpoint_pairs_only_with_fluid_llm_api_key(endpoint: str) -> None: - prefix = re.sub(r"_(API_BASE|BASE_URL)$", "", endpoint) + prefix = re.sub(r"_(API_BASE(?:_[A-Z0-9]+)*|BASE_URL)$", "", endpoint) decoy = {endpoint: "https://llm.project.test", f"{prefix}_API_KEY": PROJECT_KEY} paired = {endpoint: "https://llm.project.test", "FLUID_LLM_API_KEY": PROJECT_KEY} @@ -237,7 +252,9 @@ def test_hydration_does_not_import_litellm(tmp_path: Path) -> None: "rust_bridge/", "router_strategy/", ) -_READ = re.compile(r"""["']([A-Z][A-Z0-9_]*_API_BASE)["']""") +# ``_API_BASE``, and the same with a kind after it (``DASHSCOPE_API_BASE_VIDEO``, +# ``QWENCLOUD_API_BASE_RERANK``): one provider's route for one kind of call. +_READ = re.compile(r"""["']([A-Z][A-Z0-9_]*_API_BASE(?:_[A-Z0-9]+)*)["']""") # A get_secret* / os.getenv / os.environ read of a name ending like a host, a # URL, an endpoint or an account id (the host Snowflake's is part of). _ENV_READ = re.compile(