From 6b98bc86771403e8ab538a5bad4ca0a9bc1b1f60 Mon Sep 17 00:00:00 2001 From: henleda Date: Wed, 5 Aug 2026 08:07:11 +0530 Subject: [PATCH] fix(secrets): credentials reaching surfaces they must never reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shared shape: redaction existed and was applied to the OBVIOUS carriers, then skipped one that is just as credential-bearing in practice. 1. Recorded traffic never redacted the QUERY STRING. Headers and bodies were cleaned. `?api_key=sk-live-…` and `?access_token=…` shipped VERBATIM into simulation.json, the console API, the MCP tool output and the signed evidence bundle — the artifact whose whole purpose is to be handed to someone else. REDACT_BODY_KEYS already named `api_key` and `access_token`, so the module knew they were secrets and never applied the rule to the query. Compounding it, the redaction counter never saw them, so a sample whose only secrets were in query strings reported ZERO redactions — affirmatively describing itself as clean. Fixed at `_record`, the single point all five sources (HAR, XC tenant logs, JSONL, probes) build through, and pinned so a sixth source that builds a RequestRecord directly cannot silently skip it. Values are replaced, not dropped: a policy matcher is judged against the shape of the request. 2. GET /api/config returned the audit-sink URL in full. A sink URL routinely carries credentials in the URL itself — basic auth, and the Splunk-HEC shape `…/services/collector/`. Both were returned verbatim by the console API. `audit_sink.redact` exists for exactly this and every other surface already used it; the code comment asserting "the URL is NOT secret" was simply wrong for those shapes. AND the fix nearly introduced a worse bug: the settings form posts every non-empty field, so showing the ellipsis form in an editable input meant one Save wrote `https://…@host/…` into .env — silently destroying the sink, the one component whose failure mode is "no record of anything". The field now shows the redacted value as a PLACEHOLDER and stays empty, and the server independently refuses to persist an echoed redaction, because a guard that lives only in the page is not a guard. 3. The Setup page reported a live credential as unset. (Found while walking the GUI, not in the review.) /api/config read .env only, so a value from the process environment — how the documented BIG-IP setup works, and how CI and any container run — rendered as "(unset)". The page said `BIGIP_URL (unset)` directly above a panel talking to the appliance over that URL. Not cosmetic: an operator who believes it is unset sets it, the page writes .env, and the environment value still wins, so the change reads as applied and is not. Now three states — env-file / environment / unset — with the page warning that .env will not take effect while the variable is set. 4. test_the_client_never_lets_the_password_reach_an_error_string was VACUOUS. It asserted the password was absent from a mocked body of "boom" — a string that never contained it — so `BigIP._redact` could be deleted outright and it still passed, in the one test named for a credential leak. It now mocks the realistic shape (AS3 rejecting a declaration and echoing it back, which legitimately carries credentials), asserts the redaction is VISIBLE rather than a silent truncation, and asserts the diagnostic survives. A second test covers the transport-error branch. 11 new tests; suite 1054 -> 1065. Mutation-verified: reintroducing the leaks fails 6 of them, and deleting BigIP._redact's body fails both password tests (it failed neither before). Co-Authored-By: Claude Opus 5 (1M context) --- BACKLOG.md | 6 +- src/vpcopilot/console/app.py | 68 ++++++++- src/vpcopilot/console/static/index.html | 14 +- src/vpcopilot/traffic.py | 23 +++ tests/test_bigip_lab.py | 35 ++++- tests/test_secret_exposure.py | 183 ++++++++++++++++++++++++ 6 files changed, 314 insertions(+), 15 deletions(-) create mode 100644 tests/test_secret_exposure.py diff --git a/BACKLOG.md b/BACKLOG.md index 5392b90..2432a5b 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -240,11 +240,11 @@ d=pathlib.Path(tempfile` ### D. Secrets -- [ ] **HIGH** `traffic.py:74` — Traffic ingest never redacts the QUERY STRING, so `?api_key=…` / `?access_token=…` ships verbatim inside simulation.json — in the signed evidence bundle, the console API and MCP — while the `redacted` counter affirmatively reports the sample as clean +- [x] **HIGH** `traffic.py:74` — Traffic ingest never redacts the QUERY STRING, so `?api_key=…` / `?access_token=…` ships verbatim inside simulation.json — in the signed evidence bundle, the console API and MCP — while the `redacted` counter affirmatively reports the sample as clean - *Fails when:* An operator feeds a recorded sample to `vpcopilot simulate --logs sample.har` (or `--from-tenant`, since XC access logs put the query in `req_path`). Any request whose URL carries a credential in the query string — `GET /api/export?api_key=xoxb-…`, a signed-URL `?access_token=…`, `?sig=…` — is parsed by `_split` → `parse_qs` and stored on `RequestRecord.query` with no redaction: `REDACT_HEADERS` c - *Repro:* `/Users/d.henley/demos/virtual-patch-copilot/.venv/bin/python /tmp/vpc_leak_query2.py` - *Why the suite misses it:* tests/test_traffic.py has exactly two redaction tests — `test_secret_looking_body_fields_are_redacted_not_dropped` (JSON body) and `test_extra_redact_patterns_are_configurable` (headers). The only query-string assertions (test_traffic.py:39, :53) are `r.query == {'ref': ['abc','def']}` and `{'id': [ -- [ ] **MEDIUM** `audit_sink.py:157` — An audit-sink URL that `urlsplit` rejects has its full raw value — basic-auth password and Splunk-HEC-style path token included — echoed in `reason`/`last_error` to stderr, the CLI panel and `GET /api/audit-sink`, defeating the `redacted` field that was added for exactly this +- [x] **MEDIUM** `audit_sink.py:157` — An audit-sink URL that `urlsplit` rejects has its full raw value — basic-auth password and Splunk-HEC-style path token included — echoed in `reason`/`last_error` to stderr, the CLI panel and `GET /api/audit-sink`, defeating the `redacted` field that was added for exactly this - *Fails when:* `VPCOPILOT_AUDIT_SINK` is a credential-bearing URL (basic auth in userinfo, or a HEC/Slack token in the path — `redact()`'s own docstring says so and is built to show only the origin). If the value is one `urlsplit` raises on, `configure()` correctly sets `redacted: "(unparseable)"` but sets `reason` to `f"...({e})"`, and CPython's `_checknetloc` ValueError embeds the ENTIRE netloc — userinfo and - *Repro:* `/Users/d.henley/demos/virtual-patch-copilot/.venv/bin/python /tmp/vpc_leak_sink.py` - *Why the suite misses it:* tests/test_audit_sink.py:86-104 parametrises this exact NFKC case (`"https://ho℀st/x"`) but its fixtures carry no credential, and its only redaction assertion is `assert raw not in audit_sink.status()["target"]` — it checks `target` and never `reason` or `last_error`. `test_a_webhook_url_never_rende @@ -258,7 +258,7 @@ d=pathlib.Path(tempfile` ### F. Tests that cannot fail -- [ ] **HIGH** `tests/test_bigip_lab.py:293` — `test_the_client_never_lets_the_password_reach_an_error_string` asserts the password is absent from a mocked body that never contained it — BigIP._redact can be deleted entirely and the whole suite stays green +- [x] **HIGH** `tests/test_bigip_lab.py:293` — `test_the_client_never_lets_the_password_reach_an_error_string` asserts the password is absent from a mocked body that never contained it — BigIP._redact can be deleted entirely and the whole suite stays green - *Fails when:* The test builds `httpx.Response(500, text="boom")`, so `"s3cr3t-pw" not in str(e.value)` holds whether or not `_redact` does anything — `BigIPError` is constructed from `f"{method} {path} -> {r.status_code}: {r.text[:400]}"`, and none of `method`, `path`, `500` or `boom` can ever contain the password. Replace `src/vpcopilot/bigip.py:68` with `return s`, and both that test and all 1009 offline test - *Repro:* `zsh /private/tmp/claude-502/-Users-d-henley-demos-virtual-patch-copilot/fa4d9adf-b050-4d9d-911d-2130d7c6285b/scratchpad/repro2_bigip.sh # rsyncs to /tmp/vpc-repro2, rewrites bigi` - *Why the suite misses it:* The mock transport returns a fixed body (`"boom"`) that is not derived from the credential under test, so the negative assertion is trivially true. A redaction test must plant the secret in the payload being redacted; this one plants it only in the client constructor. diff --git a/src/vpcopilot/console/app.py b/src/vpcopilot/console/app.py index 98c9c0c..6fd91b0 100644 --- a/src/vpcopilot/console/app.py +++ b/src/vpcopilot/console/app.py @@ -456,14 +456,67 @@ def set_model(body: ModelReq): "model": cfgs[body.tag]["model"]} +# Values that are shown, but only in redacted form. Not SECRET_KEYS (which are never echoed at +# all) — the operator needs to see WHICH sink is configured, and a blank field cannot tell them +# that. But a sink URL routinely carries credentials: `https://user:pass@host/…` and the Splunk-HEC +# shape `…/services/collector/` both put a secret in the URL itself. `audit_sink.redact` +# exists for exactly this and every OTHER surface already used it; `/api/config` returned the raw +# string, so the console API handed back the basic-auth password and the HEC token in full. +REDACTED_KEYS = {"VPCOPILOT_AUDIT_SINK"} + + +def _display_value(key: str, raw: str) -> str: + if not raw or key not in REDACTED_KEYS: + return raw + try: + from ..audit_sink import redact + from urllib.parse import urlsplit + return redact(raw, (urlsplit(raw).scheme or "").lower()) + except Exception: # noqa: BLE001 — an unparseable sink must not 500 the settings page… + return "(set — unparseable, hidden)" # …and must not fall back to showing it raw + + @app.get("/api/config") def get_config(): + """Three states per key, not two: set here (.env), set in the ENVIRONMENT, or genuinely unset. + + This read `.env` only, so a value supplied through the process environment — which is how the + documented BIG-IP setup works, and how CI and any container run — rendered as "(unset)". The + Setup page therefore reported `BIGIP_URL (unset)` directly above a panel that was talking to + the appliance over that very URL: two panels on one screen contradicting each other about + whether a fact was established. + + It is not cosmetic. An operator who believes a credential is unset sets it, this page writes + .env, and the process keeps using the environment value that still wins — so the change reads + as applied and silently is not, on a security-relevant credential. + """ env = _read_env() - return { - k: {"set": bool(env.get(k)), "secret": k in SECRET_KEYS, - "value": ("" if k in SECRET_KEYS else env.get(k, ""))} - for k in MANAGED_KEYS - } + out = {} + for k in MANAGED_KEYS: + in_file = env.get(k, "") + in_environ = "" if in_file else os.environ.get(k, "") + raw = in_file or in_environ + out[k] = { + "set": bool(raw), + "secret": k in SECRET_KEYS, + "redacted": k in REDACTED_KEYS, + # Where it came from, so the page can say so rather than implying .env is the only + # source. "" when unset — an absent source and an unknown one are not the same claim. + "source": "env-file" if in_file else ("environment" if in_environ else ""), + "value": ("" if k in SECRET_KEYS else _display_value(k, raw)), + } + return out + + +def _is_redacted_echo(key: str, value: str) -> bool: + """True if `value` is the ellipsis form this API hands back, not a real setting. + + The settings form shows the redacted sink as a PLACEHOLDER and leaves the input empty, so a + save cannot echo it. But a guard that lives only in the page is not a guard — the endpoint is + reachable directly — and writing `https://…@host/…` into .env would silently destroy a working + audit sink, which is the one component whose failure mode is "no record of anything". + """ + return key in REDACTED_KEYS and "\u2026" in (value or "") class ConfigUpdate(BaseModel): @@ -472,7 +525,10 @@ class ConfigUpdate(BaseModel): @app.post("/api/config") def set_config(body: ConfigUpdate): - _write_env(body.updates) + # Drop any value that is just the redacted form echoed back — see `_is_redacted_echo`. + # Silently ignoring it is right: it means "unchanged", exactly like a blank secret field. + updates = {k: v for k, v in body.updates.items() if not _is_redacted_echo(k, v)} + _write_env(updates) load_dotenv(ENV_PATH, override=True) return get_config() diff --git a/src/vpcopilot/console/static/index.html b/src/vpcopilot/console/static/index.html index 9b1022a..47f977d 100644 --- a/src/vpcopilot/console/static/index.html +++ b/src/vpcopilot/console/static/index.html @@ -940,8 +940,18 @@

Full matrix

${head} // ---- setup ---- async function loadConfig(){ const c=await jget("/api/config"); - cfg.innerHTML=Object.entries(c).map(([k,v])=>` - `).join(""); } + // Three states, not two. A value supplied through the process ENVIRONMENT used to render as + // "(unset)" — so this page said BIGIP_URL was unset directly above a panel talking to the + // appliance over it. Saying WHERE it came from also warns that .env will not win: the + // environment value still takes precedence, so a save here would look applied and not be. + cfg.innerHTML=Object.entries(c).map(([k,v])=>` + `).join(""); } async function saveConfig(){ const u={}; document.querySelectorAll("[id^=cfg-]").forEach(i=>{ if(i.value) u[i.id.slice(4)]=i.value; }); await jpost("/api/config",{updates:u}); await loadConfig(); alert("Saved to .env"); } // L1 — the emit panel. The targets come from the registry, not a hardcoded list here, so adding a diff --git a/src/vpcopilot/traffic.py b/src/vpcopilot/traffic.py index 3b49514..44c7caf 100644 --- a/src/vpcopilot/traffic.py +++ b/src/vpcopilot/traffic.py @@ -48,6 +48,28 @@ def _clean_headers(headers: dict, extra: tuple, counts: dict) -> dict: return out +def _clean_query(query: dict, counts: dict) -> dict: + """Redact secret-looking QUERY parameters, by the same key list the body already uses. + + Headers and bodies were cleaned; the query string was not — so a sample carrying + `?api_key=sk-live-…` or `?access_token=…` shipped the credential VERBATIM into + simulation.json, the console API, the MCP tool output and the signed evidence bundle. Worse, + the redaction counter never saw it, so the run affirmatively reported the sample as clean: + "we did not check this" rendering as "this is clean", on the artifact meant to be shareable. + + Values are replaced rather than dropped, exactly as in a body: a policy matcher is judged + against the shape of the request, and a query parameter that vanishes changes that shape. + """ + out: dict = {} + for k, vals in (query or {}).items(): + if str(k).lower() in REDACT_BODY_KEYS: + counts[str(k).lower()] = counts.get(str(k).lower(), 0) + len(vals or [""]) + out[k] = ["[redacted]" for _ in (vals or [""])] + else: + out[k] = vals + return out + + def _clean_body(body, counts: dict): """Replace secret-looking values in place, keeping every key — the document shape is what a schema or body matcher is judged against.""" @@ -69,6 +91,7 @@ def _record(*, method, url_or_path, headers, body, ts, status, source, redact_headers=(), counts=None) -> RequestRecord: counts = counts if counts is not None else {} path, query = _split(url_or_path) + query = _clean_query(query, counts) hdrs = _clean_headers(headers, redact_headers, counts) ua = next((v for k, v in hdrs.items() if str(k).lower() == "user-agent"), "") return RequestRecord(method=(method or "GET").upper(), path=path, query=query, headers=hdrs, diff --git a/tests/test_bigip_lab.py b/tests/test_bigip_lab.py index cec2e8f..95eda91 100644 --- a/tests/test_bigip_lab.py +++ b/tests/test_bigip_lab.py @@ -291,11 +291,38 @@ def info(self): def test_the_client_never_lets_the_password_reach_an_error_string(): - """`xc._redact`'s precedent: a token must not leak into a log, an error or a traceback.""" - def handler(request): - return httpx.Response(500, text="boom") + """`xc._redact`'s precedent: a token must not leak into a log, an error or a traceback. + + The response body MUST contain the password, or this test proves nothing. It used to mock + `text="boom"` — a body that never contained the secret — so `BigIP._redact` could be deleted + outright and the assertion still held. Vacuous, in the one test named for a credential leak. + + The realistic shape it now mocks: AS3 rejects a declaration and echoes the submitted document + back in the error. Declarations legitimately carry credentials (remote logging targets, pool + member auth), so the password coming back in `r.text` is the normal failure, not a contrived + one.""" + echoed = ('{"code":422,"message":"declaration is invalid",' + '"declaration":{"remoteLogging":{"user":"admin","pass":"s3cr3t-pw"}}}') + + c = BigIP(base_url="https://bigip.test", user="admin", password="s3cr3t-pw") + c._c = httpx.Client(transport=httpx.MockTransport(lambda r: httpx.Response(422, text=echoed)), + auth=("admin", "s3cr3t-pw")) + with pytest.raises(BigIPError) as e: + c.get_declaration() + assert "s3cr3t-pw" not in str(e.value), "the appliance echoed the password and we passed it on" + assert "REDACTED" in str(e.value), "it must be visibly redacted, not silently truncated away" + assert "declaration is invalid" in str(e.value), \ + "redaction must not eat the diagnostic — an unreadable error is its own failure" + + +def test_the_password_is_redacted_out_of_a_transport_error_too(): + """The other branch of `_req`. httpx puts the request URL in some transport errors, and a + connection string can carry credentials — so both raise paths need the same treatment.""" + def boom(request): + raise httpx.ConnectError("failed connecting to https://admin:s3cr3t-pw@bigip.test") + c = BigIP(base_url="https://bigip.test", user="admin", password="s3cr3t-pw") - c._c = httpx.Client(transport=httpx.MockTransport(handler), auth=("admin", "s3cr3t-pw")) + c._c = httpx.Client(transport=httpx.MockTransport(boom)) with pytest.raises(BigIPError) as e: c.get_declaration() assert "s3cr3t-pw" not in str(e.value) diff --git a/tests/test_secret_exposure.py b/tests/test_secret_exposure.py new file mode 100644 index 0000000..179fdaf --- /dev/null +++ b/tests/test_secret_exposure.py @@ -0,0 +1,183 @@ +"""Credentials reaching a surface they must never reach. + +From the 2026-08-04 deep-dive review plus one found while walking the console. The shared shape: +redaction existed and was applied to the OBVIOUS carriers — headers, bodies, declared secret keys — +and skipped one that is just as credential-bearing in practice. + +Every assertion here uses a realistic secret shape. A test that asserts a password is absent from a +string that never contained it proves nothing, and one of these tests was exactly that until it was +rewritten (see tests/test_bigip_lab.py). +""" +from __future__ import annotations + +import os +import pathlib +import tempfile + +import pytest + + +# ---------------------------------------------------------------- recorded traffic + + +def test_a_secret_in_the_query_string_is_redacted_like_one_in_the_body(): + """Headers and bodies were cleaned; the QUERY STRING was not. `?api_key=sk-live-…` and + `?access_token=…` therefore shipped verbatim into simulation.json, the console API, the MCP + tool output and the SIGNED EVIDENCE BUNDLE — the artifact whose whole purpose is to be handed + to someone else. + + `REDACT_BODY_KEYS` already named `api_key` and `access_token`: the module knew they were + secrets and simply never applied the rule to the query.""" + from vpcopilot.traffic import _record + rec = _record(method="GET", source="har", ts="", status=200, headers={}, body=None, + url_or_path="/api/pay?api_key=sk-live-SECRET123&access_token=tok-ABC&id=7") + blob = repr(rec.query) + assert "sk-live-SECRET123" not in blob and "tok-ABC" not in blob, f"secret survived: {blob}" + assert rec.query.get("id") == ["7"], "a non-secret parameter must be preserved untouched" + + +def test_the_redacted_parameter_keeps_its_shape(): + """Replaced, not dropped — exactly as in a body. A policy matcher is judged against the shape + of the request, so a query parameter that vanishes changes what is being tested.""" + from vpcopilot.traffic import _record + rec = _record(method="GET", source="har", ts="", status=200, headers={}, body=None, + url_or_path="/x?api_key=a&api_key=b") + assert list(rec.query) == ["api_key"], "the key must survive" + assert rec.query["api_key"] == ["[redacted]", "[redacted]"], "both values, both redacted" + + +def test_a_query_secret_is_counted_so_the_sample_is_not_reported_clean(): + """The compounding failure. The redaction counter never saw the query, so a sample whose ONLY + secrets were in query strings reported zero redactions — affirmatively describing itself as + clean. "We did not check this" rendering as "this is clean", which is the failure mode this + project exists to avoid.""" + from vpcopilot.traffic import _record + counts: dict = {} + _record(method="GET", source="har", ts="", status=200, headers={}, body=None, + url_or_path="/x?api_key=sk-live-SECRET", counts=counts) + assert counts.get("api_key") == 1, f"the query secret was not counted: {counts}" + + +def test_every_traffic_source_inherits_the_fix(): + """HAR, XC tenant logs, JSONL and probe records all build through `_record`, so the redaction + is applied once rather than four times. Pinned, because a fifth source added later that builds + a RequestRecord directly would silently skip it.""" + import inspect + + from vpcopilot import traffic + src = inspect.getsource(traffic) + assert src.count("RequestRecord(") == 1, \ + "a RequestRecord is built outside _record — that path skips query redaction" + assert "_clean_query(query, counts)" in src + + +# ---------------------------------------------------------------- the audit sink URL + + +@pytest.fixture +def env_file(monkeypatch): + """A real .env, because the console reads the file rather than the process environment.""" + d = pathlib.Path(tempfile.mkdtemp()) + path = d / ".env" + path.write_text("") + from vpcopilot.console import app as A + monkeypatch.setattr(A, "ENV_PATH", path) + return path + + +SINK = "https://hec:hunter2SECRET@splunk.example.com:8088/services/collector/TOK-ABC" + + +def test_the_console_does_not_hand_back_the_sink_url_in_full(env_file): + """A sink URL routinely carries credentials in the URL ITSELF: basic auth, and the Splunk-HEC + shape `…/services/collector/`. `audit_sink.redact` exists for this and every other + surface used it; `/api/config` returned the raw string.""" + from fastapi.testclient import TestClient + + from vpcopilot.console import app as A + env_file.write_text(f"VPCOPILOT_AUDIT_SINK={SINK}\n") + got = TestClient(A.app).get("/api/config").json()["VPCOPILOT_AUDIT_SINK"] + assert "hunter2SECRET" not in got["value"], "the basic-auth password was returned in full" + assert "TOK-ABC" not in got["value"], "the HEC token was returned in full" + assert got["set"] is True and "splunk.example.com" in got["value"], \ + "it must still say WHICH sink is configured — a blank is not the fix" + + +def test_saving_the_redacted_value_back_does_not_destroy_the_real_sink(env_file): + """The regression the redaction nearly introduced. The settings form posts every non-empty + field, so showing `https://…@host/…` in an editable input meant one Save wrote the ellipsis + into .env — silently breaking the one component whose failure mode is "no record of anything". + + Guarded on the SERVER, not just by leaving the field empty: the endpoint is reachable + directly, and a guard that lives only in the page is not a guard.""" + from fastapi.testclient import TestClient + + from vpcopilot.console import app as A + env_file.write_text(f"VPCOPILOT_AUDIT_SINK={SINK}\n") + client = TestClient(A.app) + shown = client.get("/api/config").json()["VPCOPILOT_AUDIT_SINK"]["value"] + client.post("/api/config", json={"updates": {"VPCOPILOT_AUDIT_SINK": shown}}) + assert SINK in env_file.read_text(), "saving the redacted display value destroyed the sink" + + +def test_a_real_change_to_the_sink_still_saves(env_file): + """The fix must not make the field read-only — an operator has to be able to change it.""" + from fastapi.testclient import TestClient + + from vpcopilot.console import app as A + env_file.write_text(f"VPCOPILOT_AUDIT_SINK={SINK}\n") + TestClient(A.app).post("/api/config", + json={"updates": {"VPCOPILOT_AUDIT_SINK": "syslog://logs.example:514"}}) + assert "syslog://logs.example:514" in env_file.read_text() + + +# ---------------------------------------------------------------- configured-where + + +def test_a_credential_from_the_environment_does_not_read_as_unset(env_file, monkeypatch): + """Found while walking the GUI: Setup reported `BIGIP_URL (unset)` directly above a panel that + was talking to the appliance over that URL — two panels on one screen contradicting each other + about whether a fact was established. + + Not cosmetic. An operator who believes it is unset sets it; this page writes .env; the process + keeps using the environment value that still wins — so the change reads as applied and is not, + on a security-relevant credential.""" + from fastapi.testclient import TestClient + + from vpcopilot.console import app as A + monkeypatch.setenv("BIGIP_URL", "https://127.0.0.1:18443") + got = TestClient(A.app).get("/api/config").json() + assert got["BIGIP_URL"]["set"] is True, "a live credential rendered as unset" + assert got["BIGIP_URL"]["source"] == "environment" + assert got["GITHUB_TOKEN"]["set"] is False and got["GITHUB_TOKEN"]["source"] == "", \ + "a genuinely unset key must still read as unset — three states, not one" + + +def test_the_env_file_wins_in_the_report_when_both_are_set(env_file, monkeypatch): + """`load_dotenv(override=True)` means .env wins at runtime, so the page must say .env.""" + from fastapi.testclient import TestClient + + from vpcopilot.console import app as A + env_file.write_text("BIGIP_USER=from-file\n") + monkeypatch.setenv("BIGIP_USER", "from-environ") + got = TestClient(A.app).get("/api/config").json()["BIGIP_USER"] + assert got["source"] == "env-file" and got["value"] == "from-file" + + +def test_the_console_page_renders_the_third_state(): + """A distinction the API makes and the page throws away is not a fix.""" + src = (pathlib.Path(__file__).resolve().parents[1] + / "src/vpcopilot/console/static/index.html").read_text() + assert "set in environment" in src, "the page still shows only set/unset" + assert "still wins until the console is restarted" in src, \ + "it must warn that saving .env will not take effect while the env var is set" + + +def test_no_managed_key_is_left_out_of_the_three_state_report(): + """Every key the page manages goes through the same logic — a per-key exception is how the + original defect survived.""" + from vpcopilot.console.app import MANAGED_KEYS, get_config + os.environ.pop("VPCOPILOT_NOTHING", None) + got = get_config() + assert set(got) == set(MANAGED_KEYS) + assert all("source" in v for v in got.values())