fix(secrets): credentials reaching surfaces they must never reach

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.

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/<token>` 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()
 

src/vpcopilot/console/static/index.html

@@ -940,8 +940,18 @@ <h2 style="margin-top:14px">Full matrix</h2><table><thead>${head}</thead><tbody>
 
 // ---- setup ----
 async function loadConfig(){ const c=await jget("/api/config");
-  cfg.innerHTML=Object.entries(c).map(([k,v])=>`<label>${k} ${v.set?'<span class="band">(set)</span>':'<span class="muted">(unset)</span>'}</label>
-    <input type="text" id="cfg-${k}" value="${v.secret?'':(v.value||'')}" placeholder="${v.secret?(v.set?'•••• (unchanged unless you type)':'enter value'):''}" />`).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])=>`<label>${k} ${
+      v.source==='env-file' ? '<span class="band">(set)</span>'
+    : v.source==='environment' ? '<span class="band" title="Set in this process\u2019s environment, not in .env. Saving here writes .env, but the environment value still wins until the console is restarted.">(set in environment)</span>'
+    : '<span class="muted">(unset)</span>'}</label>
+    <input type="text" id="cfg-${k}" value="${(v.secret||v.redacted)?'':(v.value||'')}" placeholder="${
+      v.secret ? (v.set?'•••• (unchanged unless you type)':'enter value')
+      : v.redacted ? (v.set? esc(v.value)+' (unchanged unless you type)' : 'enter value')
+      : ''}" />`).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

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,

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)