diff --git a/.github/workflows/static-checks.yml b/.github/workflows/static-checks.yml index cd51a225..4a6f2b2e 100644 --- a/.github/workflows/static-checks.yml +++ b/.github/workflows/static-checks.yml @@ -42,3 +42,16 @@ jobs: exit 1 fi echo "No undefined names." + + # Fail-open contract gate (WEB-4808). packaging/ holds the hermetic + # boundary tests for the scheduled root discovery daemon's credential + # channel: the SACRED fail-open path (absent/unreadable/malformed config + # must idle-and-exit-0) and the writer side (atomic 0600 file, 0750 dir). + # These tests need no built bundle and no upstream package, so they run + # here on EVERY PR — the parity suite under binary/tests/ only runs on + # the macOS release build, which would let a fail-open regression merge. + - name: Install pytest + run: pip install pytest==9.0.3 + + - name: Run packaging boundary tests (discovery fail-open + writer) + run: python -m pytest packaging/ -q diff --git a/binary/src/unbound_hook/setup_cmd.py b/binary/src/unbound_hook/setup_cmd.py index f7cb9fd2..33b562f8 100644 --- a/binary/src/unbound_hook/setup_cmd.py +++ b/binary/src/unbound_hook/setup_cmd.py @@ -516,6 +516,77 @@ def _setup_copilot(opts): return ("configured", None) +# Root-only credential file the scheduled discovery LaunchDaemon reads to find +# its api key + domain (WEB-4808). Lives in the pkg-provisioned config dir +# (postinstall: install -d -m 0750 root:wheel /opt/unbound/etc). The recurring +# daemon runs as root with no shell env and a world-readable plist that +# deliberately carries no secrets, so this file is its only credential channel. +# The entry point (packaging/unbound_discovery_entry.py:DISCOVERY_CONFIG_PATH) +# reads it as an OPTIONAL fallback; keep the two paths in sync. +DISCOVERY_CONFIG_PATH = Path("/opt/unbound/etc/discovery.json") + + +def _write_discovery_config(opts): + """Persist the discovery key + domain for the scheduled daemon. + + Writes /opt/unbound/etc/discovery.json atomically as 0600 root:wheel (it + holds a credential). The `etc` dir itself is created/enforced as 0750 + root-owned (not the umask-default 0755) so it is not world-traversable. + Best-effort: a failure here is reported but never aborts onboarding — the + one-shot scan below still runs, and a daemon with no config simply idles + fail-open. Returns None on success or a short reason string on failure. + """ + import tempfile + + payload = json.dumps( + {"api_key": opts["discovery_key"], "domain": opts["backend_url"]}, + indent=2, + ) + "\n" + parent = DISCOVERY_CONFIG_PATH.parent + try: + # Create ONLY the leaf `etc` dir, not the whole tree: /opt/unbound is + # provisioned (and owned) by the pkg postinstall, so its absence means + # something is wrong — building the tree here would risk leaving an + # ancestor world-readable. Refuse rather than mask that. + if not parent.parent.is_dir(): + return (f"could not persist discovery config: parent " + f"{parent.parent} missing (expected pkg-provisioned)") + # 0750 (not the umask default 0755) so the dir is not world-traversable + # — it reinforces the directory-level symlink/TOCTOU barrier the 0600 + # credential file relies on. When root (the production path), enforce + # mode + root:root ownership even if the dir already existed; tolerate + # non-root/dev contexts where chmod/chown would be denied. + os.makedirs(parent, mode=0o750, exist_ok=True) + if hasattr(os, "geteuid") and os.geteuid() == 0: + try: + os.chmod(parent, 0o750) + os.chown(parent, 0, 0) + except OSError: + pass # best-effort hardening; never block the credential write + # Write to a temp file in the same dir (mkstemp is already 0600 by + # default), set explicit perms + owner, then atomically rename so a + # reader never observes a partial or wrong-permission file. + fd, tmp = tempfile.mkstemp(dir=str(parent), prefix=".discovery.", suffix=".json") + try: + os.fchmod(fd, 0o600) + try: + os.fchown(fd, 0, 0) # root:wheel; no-op-equivalent if already root + except (PermissionError, OSError): + pass # not root (shouldn't happen — setup requires sudo); keep going + with os.fdopen(fd, "w") as f: + f.write(payload) + os.replace(tmp, str(DISCOVERY_CONFIG_PATH)) + except Exception: + try: + os.unlink(tmp) + except OSError: + pass + raise + except Exception as e: + return f"could not persist discovery config: {e}" + return None + + def _run_discovery(opts): """Run the locally installed discovery binary (no install.sh download). Mirrors onboard.py's process-group + backstop-kill discipline.""" @@ -523,6 +594,12 @@ def _run_discovery(opts): return ("skipped", "no --discovery-key provided") if not DISCOVERY_BINARY.is_file(): return ("deferred", f"discovery binary not installed at {DISCOVERY_BINARY}") + # Persist creds so the scheduled LaunchDaemon (which has no shell env and a + # secret-free plist) can run the recurring 12h scan, not just this one-shot + # (WEB-4808). Fail-open: a persist failure is logged, not fatal. + config_err = _write_discovery_config(opts) + if config_err: + print(f"[setup] discovery: {config_err}", file=sys.stderr) # Key via env, never argv — the scan runs up to 90 min and argv is # visible to every local user via ps (same contract the hook modules' # frozen discovery dispatch uses). diff --git a/packaging/test-discovery-daemon.sh b/packaging/test-discovery-daemon.sh index 5de3c50f..405d6e61 100755 --- a/packaging/test-discovery-daemon.sh +++ b/packaging/test-discovery-daemon.sh @@ -28,10 +28,26 @@ PORT="${SINK_PORT:-18924}" TIMEOUT_S="${DAEMON_TEST_TIMEOUT:-900}" FAIL=0 +# Variant 3 (WEB-4808) writes the real production config path so the frozen +# binary's own fallback read is exercised. Back up any pre-existing config and +# restore it on exit so running this on a real install is non-destructive. +DISCOVERY_CONFIG="/opt/unbound/etc/discovery.json" +CONFIG_BACKUP="/var/tmp/$LABEL.discovery.json.bak" +CONFIG_WAS_PRESENT=0 +CONFIG_WE_CREATED=0 +[ -f "$DISCOVERY_CONFIG" ] && { cp -p "$DISCOVERY_CONFIG" "$CONFIG_BACKUP" && CONFIG_WAS_PRESENT=1; } + cleanup() { launchctl bootout "system/$LABEL" 2>/dev/null || true rm -f "$PLIST" rm -rf "$STAGE" + # Restore the host's discovery config exactly as we found it. + if [ "$CONFIG_WAS_PRESENT" -eq 1 ]; then + cp -p "$CONFIG_BACKUP" "$DISCOVERY_CONFIG" 2>/dev/null || true + rm -f "$CONFIG_BACKUP" + elif [ "$CONFIG_WE_CREATED" -eq 1 ]; then + rm -f "$DISCOVERY_CONFIG" + fi [ -n "${SINK_PID:-}" ] && kill "$SINK_PID" 2>/dev/null || true } trap cleanup EXIT @@ -208,7 +224,45 @@ else FAIL=1 fi -echo "daemon logs: $LOG, $NOCONF_LOG; sink artifacts: $SINK_DIR" +# --- variant 3: production repro (WEB-4808) ---------------------------------- +# The exact production shape: plist carries the `scan` token and NO creds +# (secret-free, world-readable plist), creds live ONLY in the root-only config +# file. The daemon must read the file, strip `scan`, scan, and POST — proving +# the scheduled run no longer idles. This is the case the live plist ships. +launchctl bootout "system/$LABEL" 2>/dev/null || true +rm -f /var/tmp/unbound-0/discovery-cache.json /var/root/.unbound/discovery-cache.json +REPORTS_BEFORE=$(ls "$SINK_DIR"/report_*.json 2>/dev/null | wc -l | tr -d ' ') +install -d -o root -g wheel -m 0750 /opt/unbound/etc 2>/dev/null || true +umask 077 +cat > "$DISCOVERY_CONFIG" </dev/null || true +chmod 600 "$DISCOVERY_CONFIG" +[ "$CONFIG_WAS_PRESENT" -eq 1 ] || CONFIG_WE_CREATED=1 + +CFG_LOG="/var/tmp/$LABEL.cfgfile.log" +rm -f "$CFG_LOG" +# `scan` token in argv + HOME=/var/empty + NO creds in the plist — production. +write_plist "$CFG_LOG" varempty scan +launchctl bootstrap system "$PLIST" +echo "daemon bootstrapped (system/$LABEL, scan token, creds via config file); waiting..." +wait_for_daemon_exit "$TIMEOUT_S" || { tail -20 "$CFG_LOG" 2>/dev/null; exit 1; } + +CFG_REPORTS=$(ls "$SINK_DIR"/report_*.json 2>/dev/null | wc -l | tr -d ' ') +echo "--- variant 3 results (WEB-4808 production repro) -----------" +echo "daemon exit code: $DAEMON_EXIT; new report payloads: $((CFG_REPORTS - REPORTS_BEFORE))" +if [ "$DAEMON_EXIT" = "0" ] && [ "$CFG_REPORTS" -gt "$REPORTS_BEFORE" ] && \ + ! grep -q "No configuration provided" "$CFG_LOG" 2>/dev/null && \ + ! grep -q "unrecognized arguments" "$CFG_LOG" 2>/dev/null; then + echo "PASS: config-file daemon (scan token, no plist creds) scanned + POSTed" +else + echo "FAIL: config-file daemon — exit $DAEMON_EXIT, reports $CFG_REPORTS, log:" + tail -8 "$CFG_LOG" 2>/dev/null + FAIL=1 +fi + +echo "daemon logs: $LOG, $NOCONF_LOG, $CFG_LOG; sink artifacts: $SINK_DIR" if [ "$FAIL" -eq 0 ]; then echo "DAEMON TEST PASSED" else diff --git a/packaging/test_discovery_entry.py b/packaging/test_discovery_entry.py new file mode 100644 index 00000000..01a9c41e --- /dev/null +++ b/packaging/test_discovery_entry.py @@ -0,0 +1,177 @@ +"""Boundary tests for the unbound-discovery entry point's config resolution. + +Exercises packaging/unbound_discovery_entry.py at its outermost in-process +seams — `main()` and the config resolver it delegates to — without needing a +built PyInstaller bundle or the upstream `coding_discovery_tools` package +(which is fetched only at build time from packaging/discovery.lock). + +The behavior under test is WEB-4808: the scheduled root LaunchDaemon runs +`unbound-discovery scan` with NO --api-key/--domain flags and NO shell env, so +it must instead pick up credentials from the persisted root-only config file +/opt/unbound/etc/discovery.json. The two anchors: + + * config file present -> resolves creds, proceeds to the real sweep + * config file absent -> idles fail-open and exits 0 (unchanged contract) + +The upstream sweep is stubbed so we can assert main() reached it (the +credentialed path) and observe the creds it would run with, while keeping the +test hermetic. +""" +import importlib.util +import sys +import types +from pathlib import Path + +import pytest + +ENTRY_PATH = Path(__file__).resolve().parent / "unbound_discovery_entry.py" + + +def _load_entry(): + """Import the entry module fresh from source (it is not a package member).""" + spec = importlib.util.spec_from_file_location("unbound_discovery_entry", ENTRY_PATH) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +@pytest.fixture +def entry(tmp_path, monkeypatch): + """Entry module with the config path redirected into a temp dir and a + stubbed `coding_discovery_tools.ai_tools_discovery` so the credentialed + path is observable without the real upstream package.""" + mod = _load_entry() + cfg_path = tmp_path / "discovery.json" + monkeypatch.setattr(mod, "DISCOVERY_CONFIG_PATH", str(cfg_path)) + + calls = {"sweep": 0} + + def fake_sweep(): + calls["sweep"] += 1 + # Capture what the sweep would actually run with: the entry feeds the + # key via UNBOUND_API_KEY env and the domain via --domain argv. + import argparse + import os + + p = argparse.ArgumentParser(add_help=False) + p.add_argument("--domain") + ns, _ = p.parse_known_args(sys.argv[1:]) + calls["api_key"] = os.environ.get("UNBOUND_API_KEY", "") + calls["domain"] = ns.domain or "" + return 0 + + pkg = types.ModuleType("coding_discovery_tools") + sub = types.ModuleType("coding_discovery_tools.ai_tools_discovery") + sub.main = fake_sweep + pkg.ai_tools_discovery = sub + monkeypatch.setitem(sys.modules, "coding_discovery_tools", pkg) + monkeypatch.setitem(sys.modules, "coding_discovery_tools.ai_tools_discovery", sub) + + # Default to a clean env so flags/env never leak across cases. + monkeypatch.delenv("UNBOUND_API_KEY", raising=False) + + mod._test_cfg_path = cfg_path + mod._test_calls = calls + return mod + + +def _run(mod, argv, monkeypatch): + monkeypatch.setattr(sys, "argv", ["unbound-discovery", *argv]) + return mod.main() + + +# --- config file present: the daemon's `scan` invocation now proceeds --------- + +def test_config_file_supplies_creds_for_daemon_scan(entry, monkeypatch): + """The production case: `scan` with no flags/env, creds only in the file. + main() must strip `scan`, resolve creds from the file, and run the sweep.""" + entry._test_cfg_path.write_text( + '{"api_key": "file-key", "domain": "https://backend.example.com"}' + ) + rc = _run(entry, ["scan"], monkeypatch) + assert rc == 0 + assert entry._test_calls["sweep"] == 1, "credentialed sweep did not run" + assert entry._test_calls["api_key"] == "file-key" + assert entry._test_calls["domain"] == "https://backend.example.com" + + +def test_config_file_resolves_without_scan_token(entry, monkeypatch): + """Bare invocation (no `scan`) with file-only creds also proceeds.""" + entry._test_cfg_path.write_text( + '{"api_key": "file-key", "domain": "https://backend.example.com"}' + ) + assert entry._missing_required_config([]) == [] + rc = _run(entry, [], monkeypatch) + assert rc == 0 + assert entry._test_calls["sweep"] == 1 + + +def test_flags_win_over_config_file(entry, monkeypatch): + """Explicit flags/env take precedence; the file only fills absent fields.""" + entry._test_cfg_path.write_text( + '{"api_key": "file-key", "domain": "https://file.example.com"}' + ) + rc = _run( + entry, + ["--api-key", "flag-key", "--domain", "https://flag.example.com"], + monkeypatch, + ) + assert rc == 0 + assert entry._test_calls["api_key"] == "flag-key" + assert entry._test_calls["domain"] == "https://flag.example.com" + + +def test_config_file_fills_only_missing_domain(entry, monkeypatch): + """Key from env, domain from file -> both resolve, sweep runs.""" + entry._test_cfg_path.write_text('{"domain": "https://file.example.com"}') + monkeypatch.setenv("UNBOUND_API_KEY", "env-key") + rc = _run(entry, ["scan"], monkeypatch) + assert rc == 0 + assert entry._test_calls["api_key"] == "env-key" + assert entry._test_calls["domain"] == "https://file.example.com" + + +# --- config file absent / broken: fail-open idle must hold (exit 0) ---------- + +def test_no_config_file_idles_exit_0(entry, monkeypatch, caplog): + """No file, no flags, no env: idle fail-open, exit 0, never touch sweep.""" + assert not entry._test_cfg_path.exists() + rc = _run(entry, ["scan"], monkeypatch) + assert rc == 0 + assert entry._test_calls["sweep"] == 0, "sweep ran without any config (should idle)" + + +def test_partial_config_file_idles_when_key_missing(entry, monkeypatch): + """Domain-only file, no env key: still missing the key -> idle, exit 0.""" + entry._test_cfg_path.write_text('{"domain": "https://file.example.com"}') + rc = _run(entry, ["scan"], monkeypatch) + assert rc == 0 + assert entry._test_calls["sweep"] == 0 + + +@pytest.mark.parametrize( + "contents", + ["not json at all", "[1, 2, 3]", '{"api_key": 123, "domain": 456}', ""], +) +def test_unreadable_or_malformed_config_idles(entry, monkeypatch, contents): + """A corrupt/empty/wrong-typed file must not crash; it idles fail-open.""" + entry._test_cfg_path.write_text(contents) + rc = _run(entry, ["scan"], monkeypatch) + assert rc == 0 + assert entry._test_calls["sweep"] == 0 + + +def test_load_discovery_config_never_raises_on_missing(entry): + """The loader returns {} (not an exception) for a path that does not exist.""" + assert entry._load_discovery_config("/no/such/path/discovery.json") == {} + + +# --- the `scan` token strip must not disturb other routing ------------------- + +def test_version_short_circuits_before_config(entry, monkeypatch, capsys): + """--version prints and exits 0 even with a config file present (WEB-4802).""" + entry._test_cfg_path.write_text('{"api_key": "k", "domain": "d"}') + rc = _run(entry, ["--version"], monkeypatch) + assert rc == 0 + assert "unbound-discovery" in capsys.readouterr().out + assert entry._test_calls["sweep"] == 0 diff --git a/packaging/test_discovery_writer.py b/packaging/test_discovery_writer.py new file mode 100644 index 00000000..6f15d546 --- /dev/null +++ b/packaging/test_discovery_writer.py @@ -0,0 +1,118 @@ +"""Hermetic tests for setup_cmd._write_discovery_config (WEB-4808). + +This is the WRITER side of the discovery-credential contract; its READER side +(the daemon entry point) is covered by test_discovery_entry.py. Together they +guard the only credential channel the scheduled root LaunchDaemon has. + +What's asserted here: + + * the persisted file round-trips api_key + domain + * the file is mode 0600 (it holds a credential) + * the leaf `etc` dir is created 0750, NOT the umask-default world-traversable + 0755 (the directory-level barrier the 0600 file relies on — M1 hardening) + * a missing grandparent (/opt/unbound absent) returns a reason string rather + than silently building the whole tree, and never raises + * a non-writable parent returns a reason string rather than raising + (fail-open: a persist failure must never abort onboarding) + +Hermetic: DISCOVERY_CONFIG_PATH is redirected into a tmp dir; nothing touches +/opt. The module is imported from binary/src without a built bundle. +""" +import json +import os +import stat +import sys +from pathlib import Path + +import pytest + +SRC = Path(__file__).resolve().parents[1] / "binary" / "src" +if str(SRC) not in sys.path: + sys.path.insert(0, str(SRC)) + +from unbound_hook import setup_cmd # noqa: E402 + + +def _opts(discovery_key="disc-key", backend_url="https://backend.example.com"): + # _write_discovery_config only reads these two keys. + return {"discovery_key": discovery_key, "backend_url": backend_url} + + +def _provisioned_cfg_path(tmp_path): + """A tmp analog of /opt/unbound/etc/discovery.json where the grandparent + (the pkg-provisioned /opt/unbound) already exists but `etc` does not — the + real production precondition.""" + opt_unbound = tmp_path / "opt" / "unbound" + opt_unbound.mkdir(parents=True) + return opt_unbound / "etc" / "discovery.json" + + +# --- happy path: round-trip, file mode, dir mode ----------------------------- + +def test_writes_creds_and_roundtrips(tmp_path, monkeypatch): + cfg = _provisioned_cfg_path(tmp_path) + monkeypatch.setattr(setup_cmd, "DISCOVERY_CONFIG_PATH", cfg) + + err = setup_cmd._write_discovery_config(_opts()) + + assert err is None, f"expected success, got reason: {err}" + assert cfg.is_file() + data = json.loads(cfg.read_text()) + assert data == {"api_key": "disc-key", "domain": "https://backend.example.com"} + + +def test_credential_file_is_0600(tmp_path, monkeypatch): + cfg = _provisioned_cfg_path(tmp_path) + monkeypatch.setattr(setup_cmd, "DISCOVERY_CONFIG_PATH", cfg) + + assert setup_cmd._write_discovery_config(_opts()) is None + + mode = stat.S_IMODE(os.stat(cfg).st_mode) + assert mode == 0o600, f"credential file mode {oct(mode)} != 0600" + + +def test_leaf_dir_created_0750_not_world_traversable(tmp_path, monkeypatch): + cfg = _provisioned_cfg_path(tmp_path) + monkeypatch.setattr(setup_cmd, "DISCOVERY_CONFIG_PATH", cfg) + + assert setup_cmd._write_discovery_config(_opts()) is None + + mode = stat.S_IMODE(os.stat(cfg.parent).st_mode) + # The leaf etc dir must not be world-traversable (no o+x); 0750 is the + # target. (Root chmod-enforces 0750 exactly; non-root relies on makedirs' + # mode arg, which umask may tighten further but never loosen the o-bits.) + assert not (mode & stat.S_IXOTH), f"etc dir {oct(mode)} is world-traversable" + assert not (mode & stat.S_IROTH), f"etc dir {oct(mode)} is world-readable" + + +# --- fail-open: a persist failure returns a reason, never raises ------------- + +def test_missing_grandparent_returns_reason(tmp_path, monkeypatch): + """If /opt/unbound is absent, refuse (return reason) instead of building + the whole tree world-readable — and do NOT raise.""" + # grandparent (the "/opt/unbound" analog) intentionally NOT created + cfg = tmp_path / "opt" / "unbound" / "etc" / "discovery.json" + monkeypatch.setattr(setup_cmd, "DISCOVERY_CONFIG_PATH", cfg) + + err = setup_cmd._write_discovery_config(_opts()) + + assert isinstance(err, str) and err, "expected a reason string" + assert not cfg.exists() + assert not cfg.parent.exists(), "should not have created the etc tree" + + +def test_non_writable_parent_returns_reason(tmp_path, monkeypatch): + """A pre-existing but non-writable etc dir yields a reason, not a crash.""" + if os.geteuid() == 0: + pytest.skip("root bypasses directory write permissions") + etc = tmp_path / "opt" / "unbound" / "etc" + etc.mkdir(parents=True) + etc.chmod(0o500) # r-x: cannot create the temp file inside + monkeypatch.setattr(setup_cmd, "DISCOVERY_CONFIG_PATH", etc / "discovery.json") + + try: + err = setup_cmd._write_discovery_config(_opts()) + finally: + etc.chmod(0o700) # restore so tmp_path cleanup can proceed + + assert isinstance(err, str) and err, "expected a reason string on write failure" diff --git a/packaging/unbound_discovery_entry.py b/packaging/unbound_discovery_entry.py index 07690885..3165e252 100644 --- a/packaging/unbound_discovery_entry.py +++ b/packaging/unbound_discovery_entry.py @@ -8,20 +8,38 @@ script itself would exit 1; a daemon must instead idle cleanly (fail-open) — log one line and exit 0. -Configuration is considered present when both the API key (--api-key flag or -the UNBOUND_API_KEY env var, mirroring upstream) and --domain are non-empty. +Configuration is considered present when both the API key (--api-key flag, the +UNBOUND_API_KEY env var mirroring upstream, or the persisted root-only config +file) and --domain (flag or config file) are non-empty. The config file +(/opt/unbound/etc/discovery.json, written 0600 by `unbound-hook setup`) is the +credential channel for the scheduled root LaunchDaemon, which sees no shell env +and whose world-readable plist intentionally carries no secrets (WEB-4808). Subcommand routing mirrors upstream install.sh at the pinned SHA: a leading "mcp-scan" runs the on-demand single-server scan (scan_single_mcp_server, -used by the agent hooks when the gateway reports an unknown MCP server); -anything else runs the full discovery sweep. Everything else (argument -semantics, detection, reporting via curl) is the upstream script, unchanged. +used by the agent hooks when the gateway reports an unknown MCP server). A +leading "scan" token (the LaunchDaemon's invocation) is stripped — the full +sweep is the bare/default invocation and upstream's strict argparse would +otherwise reject "scan" once creds resolve. Anything else runs the full +discovery sweep. Everything else (argument semantics, detection, reporting via +curl) is the upstream script, unchanged. """ import argparse +import json import logging import os import sys +# Root-only credential file written by `unbound-hook setup` at onboard time +# (setup_cmd._write_discovery_config). The scheduled LaunchDaemon runs as root +# with no shell environment, so a key in a user's shell rc is invisible to it, +# and the plist deliberately carries no secrets (it is world-readable). This +# file is the credential channel for the recurring scan (WEB-4808). It is an +# OPTIONAL fallback: when it is absent or unreadable (pre-onboarding, partial +# install) credential resolution falls through to the existing missing-config +# idle path — discovery must never crash or block dev work because of it. +DISCOVERY_CONFIG_PATH = "/opt/unbound/etc/discovery.json" + # Discovery's user-facing version. Mirrors unbound-hook's __version__ # (binary/src/unbound_hook/__init__.py) so both binaries report the same # string. The release workflow's publish-safety assert requires @@ -34,23 +52,74 @@ __version__ = "0.1.0" -def _missing_required_config(argv): - """Return human-readable names of required config that is absent/empty. +def _load_discovery_config(path=None): + """Best-effort read of the root-only discovery config (api_key, domain). + + Returns a {"api_key": str, "domain": str} dict with only the non-empty + string values present. Any failure — file absent, unreadable, not JSON, + not an object, wrong value types — returns {} so the caller falls through + to the fail-open idle path. This function NEVER raises: a credential file + problem must not turn a quiet idle into a crash on a root daemon. - Uses an argparse mirror (not a hand-rolled scan) so '=', value-consumption - and prefix-abbreviation semantics match the upstream parser, and honors - the UNBOUND_API_KEY env fallback exactly like upstream main(). + `path` defaults to the module-level DISCOVERY_CONFIG_PATH resolved at CALL + time (not bound at def time) so tests can redirect it. + """ + if path is None: + path = DISCOVERY_CONFIG_PATH + try: + with open(path, "r") as f: + data = json.load(f) + except Exception: + return {} + if not isinstance(data, dict): + return {} + out = {} + for key in ("api_key", "domain"): + value = data.get(key) + if isinstance(value, str) and value.strip(): + out[key] = value + return out + + +def _resolve_config(argv): + """Resolve (api_key, domain) from flags/env, then the config file. + + Precedence per field, highest first: explicit flag / env (the existing + contract) > the persisted root-only config file (WEB-4808 daemon channel). + The config file only FILLS fields that are otherwise absent; it never + overrides an explicit flag or env value. Returns (api_key, domain) as + possibly-empty strings. """ parser = argparse.ArgumentParser(add_help=False) parser.add_argument("--api-key") parser.add_argument("--domain") ns, _ = parser.parse_known_args(argv) - api_key = ns.api_key or os.environ.get("UNBOUND_API_KEY") or "" + api_key = (ns.api_key or os.environ.get("UNBOUND_API_KEY") or "").strip() + domain = (ns.domain or "").strip() + + if not api_key or not domain: + cfg = _load_discovery_config() + if not api_key: + api_key = cfg.get("api_key", "") + if not domain: + domain = cfg.get("domain", "") + return api_key, domain + + +def _missing_required_config(argv): + """Return human-readable names of required config that is absent/empty. + + Resolution honors the UNBOUND_API_KEY env fallback exactly like upstream + main() and, when a field is still absent, the persisted root-only config + file (WEB-4808) so the scheduled root daemon — which sees no shell env and + has no secrets in its world-readable plist — can find its credentials. + """ + api_key, domain = _resolve_config(argv) missing = [] - if not api_key.strip(): + if not api_key: missing.append("--api-key (or UNBOUND_API_KEY env)") - if not (ns.domain or "").strip(): + if not domain: missing.append("--domain") return missing @@ -90,6 +159,18 @@ def main(): _log_crash() return 1 + # Strip a leading "scan" token the same way "mcp-scan" is shifted off: + # the production LaunchDaemon invokes `unbound-discovery scan`, but the + # full-sweep is the default (bare) invocation and upstream's strict + # argparse rejects an unknown positional. With no creds the lenient + # parse_known_args tolerated it (idle path), masking the bug; the moment + # creds resolve, an unstripped "scan" would SystemExit(2). Stripping it + # here keeps the world-readable plist free of secrets AND lets the + # credentialed sweep run (WEB-4808). No-op when "scan" is absent. + if argv and argv[0] == "scan": + argv = argv[1:] + sys.argv = [sys.argv[0]] + argv + if "-h" in argv or "--help" in argv: from coding_discovery_tools.ai_tools_discovery import main as discovery_main @@ -109,6 +190,20 @@ def main(): ) return 0 + # Config is present (from flags/env and/or the persisted config file). + # Upstream main() reads creds only from --api-key/UNBOUND_API_KEY/--domain, + # so feed any file-sourced values through those same channels. The key goes + # via env (the argv-free channel) to keep it out of `ps`; the domain is not + # secret and goes via argv. Explicit flags/env already in sys.argv win — + # _resolve_config only fills absent fields — so this never overrides them. + api_key, domain = _resolve_config(argv) + if not os.environ.get("UNBOUND_API_KEY"): + os.environ["UNBOUND_API_KEY"] = api_key + if "--domain" not in sys.argv and not any( + a.startswith("--domain=") for a in sys.argv + ): + sys.argv = sys.argv + ["--domain", domain] + from coding_discovery_tools.ai_tools_discovery import main as discovery_main try: