diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 85fe84cf..99d03eb0 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8887,3 +8887,97 @@ leak it. That is the stronger control `odbc_params` lacks, and it is now asserte 15.3.1, which I had recused from because I authored the two fixes bearing on that cell. Reproduced here by execution before any code changed. This is the fourth instance of the class and the second time a guard written after the previous instance picked a domain narrower than the surface. + +## 1207. an `env()` ref in a headers table, and a credential in URL userinfo, both escaped redaction + +> 🔢 **Filed 2026-08-09 - FIXED IN THE SAME CHANGE.** Value **7/10** · Difficulty **2/10**. Two holes, both INSIDE surfaces the redactor already claimed to handle. **(b)** the `headers` branch had no `EnvRef` arm, so an `env()` ref in a headers table came back as the RAW object carrying its `default` intact - while the same `env()` on a top-level credential correctly emits `{"env": key}` with the default dropped. **(c)** `url="https://user:SECRET@host"` was returned verbatim by both serializers while `proxy_password` on the SAME object masked. + +**Cluster:** Security / secret disclosure. **Priority:** P1. **Verdict:** build (done). +**Severity:** on a first deployment, both would be served to any `MONITORING_READ` caller and printed +by `graph --json`. (b) discloses a FALLBACK secret - the `env()` default is the value used when the +variable is unset, so it is a credential by construction. No PHI. + +**Measured before and after, both serializers:** + +``` +(b) BEFORE headers={"X-Vendor-Thing": env("acme_key", default=S)} + -> EnvRef(key='acme_key', default='S') raw object, default intact, not JSON-safe + AFTER -> {'env': 'acme_key'} default dropped + control Content-Type: application/json untouched + +(c) BEFORE url=https://user:S@host/y verbatim + proxy_url=http://puser:S@proxy:8080 verbatim + proxy_password on the same object '***' + AFTER url=https://user:***@host/y user, host and path PRESERVED + control https://plain.invalid/path?q=1 untouched +``` + +**WHY THE DEFAULT IS DROPPED FOR EVERY HEADER, not only credential-shaped ones.** The measured +instance used `X-Vendor-Thing`, which matches no substring in the header name rule - so gating the +`EnvRef` arm on that rule would have left this exact case open. A header value sourced from `env()` is +a credential by intent; nobody `env()`-refs a `Content-Type`. The name heuristic is the wrong gate +here, and it is precisely the gate that failed. + +**WHY THE USER, HOST AND PATH SURVIVE.** Only the password half of the userinfo is replaced. An +operator diagnosing a connection needs to see which account and which host; masking the whole URL +would destroy the view rather than protect it, and nothing would report that as a loss. The control +test asserts a URL without userinfo is left byte-identical, because a masker that rewrites every URL +would satisfy the leak assertions while silently mangling ordinary configuration. + +**`proxy` is another parameter-to-setting rename**, noticed while fixing this: the factory parameter +is `proxy` and the emitted setting is `proxy_url`. That is the same boundary `with_signing` crosses +(`private_key` -> `sign_private_key`, BACKLOG #1106) - which is why the URL rule is a NAME set plus a +suffix rule rather than a suffix rule alone. + +**Source:** both found by the `asvs-tracking-rework` session's independent assessment of ASVS 15.3.1, +alongside the `odbc_params` disclosure fixed as #1206. Reproduced here by execution before any code +changed. With these closed, the three surfaces that hold 15.3.1 at `partial` are addressed and the cell +is due a re-read - by that session, not by me, since I authored all three fixes. + +**Process note against myself:** the code comments in this change cited `#1207` BEFORE the number was +allocated. It happened to be next, so nothing collided - but "happened to be next" is exactly the +reasoning `scripts/coord/alloc.ps1` exists to eliminate, and two sessions doing it simultaneously is +the documented failure. Allocate, then write. + +## 1208. no guard asserts that a credential factory PARAMETER maps to a SETTING name the redactor covers + +> 🔢 **Filed 2026-08-09 - not started. THREE MEASURED INSTANCES of one shape, not a hypothesis.** Value **7/10** · Difficulty **4/10**. A connector factory takes a credential parameter and emits it under a DIFFERENT setting name. Every redaction control operates on the SETTING name. Nothing asserts the two agree, so a rename silently moves a credential outside the control's domain. + +**Cluster:** Security / secret disclosure - prevention. **Priority:** P2. **Verdict:** build. +**Severity:** no live defect at filing - the three known instances are closed. This is the guard that +would have prevented all three, and its absence is why each was found by a person rather than by CI. + +**The three instances, all closed, all the same boundary:** + +| factory | PARAMETER (classified) | emitted SETTING (was not) | closed by | +|---|---|---|---| +| `with_signing` | `private_key` | `sign_private_key` | #1106 | +| `with_signing` | `private_key_password` | `sign_private_key_password` | #1106 | +| `Rest` | `proxy` | `proxy_url` | noticed under #1207 | + +`_is_secret_setting("private_key")` is True and `_is_secret_setting("sign_private_key")` was False. The +parameter was covered and the setting it became was not. `proxy` -> `proxy_url` is the same crossing; +it happens to be harmless because the URL rule now covers it, which is luck rather than design. + +**Why the existing guards do not cover it.** +`test_every_credential_shaped_factory_param_is_classified` reads PARAMETER names - one abstraction level +away from where redaction operates, which is precisely how #1106 walked through it. +`test_connection_factory_redaction_domain.py` reads EMITTED settings end to end and would catch a leak, +but only for a value it can inject: it proves the OUTCOME per factory, not the MAPPING. A credential +parameter that a factory silently drops, renames into a container, or folds into a composed string is +outside what either sees. + +**Shape of the guard.** For every spec-returning factory, for every credential-shaped parameter, assert +the value reaches EITHER a setting the redactor masks, OR a documented non-emitting destination. The +mapping is discoverable by injecting a unique sentinel per parameter and searching the emitted settings +for it - which is mechanical and needs no per-connector knowledge. A parameter whose sentinel appears +in NO setting is the interesting case: it was consumed, composed, or dropped, and each of those needs a +stated reason rather than silence. + +**Do NOT implement it by comparing name lists.** That is the defect one level up: `private_key` and +`sign_private_key` are different strings and any name-based comparison has to be taught the rename, +which means it cannot catch the next one. Follow the VALUE. + +**Source:** raised by the `asvs-tracking-rework` session on 2026-08-09 after `proxy` -> `proxy_url` +became the third instance: *"that is not a coincidence to note in a residual; it is an argument that the +rename boundary itself needs a guard"*. Filed before it was forgotten, per that session's request. diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 82605611..8fcd9d2d 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -873,6 +873,50 @@ def connector_secret_env_values( return out +#: Settings whose value is a URL that may carry `user:password@` userinfo. `proxy` has no `_url` +#: suffix, which is why this is a NAME set plus a suffix rule rather than a suffix rule alone. +_URL_SETTING_SUFFIXES = ("url", "_url", "_uri", "endpoint", "_endpoint") + + +def _mask_url_userinfo(value: object) -> object: + """Replace the PASSWORD half of a URL's userinfo with ``***``, keeping everything else readable. + + BACKLOG #1207. ``url="https://user:SECRET@host/path"`` was returned verbatim by both serializers + while ``proxy_password`` on the SAME object masked -- the credential was safe in the typed field + and disclosed in the URL beside it. + + The user half and the host and path are PRESERVED deliberately: an operator diagnosing a + connection needs to see which account and which host, and masking the whole URL would destroy the + view rather than protect it. Only the secret is removed. + """ + if not isinstance(value, str) or "@" not in value or "//" not in value: + return value + scheme, _, rest = value.partition("//") + userinfo, at, hostpart = rest.rpartition("@") + if not at or ":" not in userinfo: + return value # no userinfo, or a user with no password -- nothing secret to remove + user, _, _pw = userinfo.partition(":") + return f"{scheme}//{user}:***@{hostpart}" + + +def _redact_header_value(name: str, value: object) -> object: + """One header's value, scrubbed. Handles the ``EnvRef`` case the headers branch used to miss. + + BACKLOG #1207. The headers branch had no ``EnvRef`` arm, so an ``env()`` ref inside a headers + table came back as the RAW OBJECT -- not JSON-safe, and carrying its ``default`` intact. The same + ``env()`` on a top-level credential correctly emits ``{"env": key}`` with the default dropped, so + the hole was INSIDE the one container this control claims to handle. + + THE DEFAULT IS DROPPED FOR EVERY HEADER, not only credential-shaped ones. A header value sourced + from ``env()`` is a credential by intent -- nobody env-refs a ``Content-Type`` -- so the name + heuristic is the wrong gate here, and it is exactly the gate that failed: the measured instance + used ``X-Vendor-Thing``, which matches no substring rule. + """ + if isinstance(value, EnvRef): + return {"env": value.key} + return "***" if _is_secret_header(name, value) else value + + def redacted_settings(settings: Mapping[str, Any]) -> dict[str, Any]: """A JSON-safe, secret-scrubbed view of a connection's settings for the API ``/metadata`` endpoint: each EnvRef becomes ``{"env": key}`` (the value is never resolved — only the key is shown), a @@ -889,8 +933,11 @@ def redacted_settings(settings: Mapping[str, Any]) -> dict[str, Any]: out[name] = ref elif is_secret: out[name] = "***" + elif isinstance(value, str) and name.lower().endswith(_URL_SETTING_SUFFIXES): + # BACKLOG #1207 -- a credential in URL userinfo, masked without destroying the view. + out[name] = _mask_url_userinfo(value) elif name == "headers" and isinstance(value, dict): - out[name] = {k: ("***" if _is_secret_header(k, v) else v) for k, v in value.items()} + out[name] = {k: _redact_header_value(k, v) for k, v in value.items()} elif name == "odbc_params" and isinstance(value, dict): # BACKLOG #1206. This bag is documented as carrying "only static driver keywords", and the # redactor honoured that by not descending -- so a credential inside it was served verbatim diff --git a/tests/test_connection_factory_redaction_domain.py b/tests/test_connection_factory_redaction_domain.py index dfcdb31b..046c8807 100644 --- a/tests/test_connection_factory_redaction_domain.py +++ b/tests/test_connection_factory_redaction_domain.py @@ -525,3 +525,50 @@ def test_a_refusing_connector_actually_refuses_an_inline_credential() -> None: "no connector refused an inline credential. Either the env()-only refusals were removed, or " "_call_with_sentinels stopped detecting them; a green here proves neither." ) + + +# --- BACKLOG #1207: two holes INSIDE containers the control claims to handle ----------------------- + + +def test_an_env_ref_inside_a_headers_table_does_not_disclose_its_default() -> None: + """A hole inside the ONE container the header rule already covered. + + The headers branch had no `EnvRef` arm, so an `env()` ref in a headers table came back as the RAW + object -- not JSON-safe, and carrying its `default` intact -- while the same `env()` on a top-level + credential correctly emits `{"env": key}` with the default dropped. + + The measured instance used `X-Vendor-Thing`, which matches no credential substring. That is why the + default is now dropped for EVERY header rather than only credential-shaped ones: a header value + sourced from `env()` is a credential by intent, and the name heuristic is precisely the gate that + failed here. + """ + spec = messagefoundry.Rest( + url="https://example.invalid/x", + headers={"X-Vendor-Thing": messagefoundry.env("acme_key", default=SENTINEL)}, + ) + got = redacted_settings(dict(spec.settings))["headers"]["X-Vendor-Thing"] + assert got == {"env": "acme_key"}, got + assert SENTINEL not in str(got) + + +def test_a_credential_in_url_userinfo_is_masked_on_url_and_proxy_url() -> None: + """`url="https://user:SECRET@host"` was returned verbatim while `proxy_password` on the SAME + object masked -- safe in the typed field, disclosed in the URL beside it.""" + spec = messagefoundry.Rest( + url=f"https://user:{SENTINEL}@example.invalid/y", + proxy=f"http://puser:{SENTINEL}@proxy.invalid:8080", + proxy_password="pp", + ) + out = redacted_settings(dict(spec.settings)) + assert SENTINEL not in str(out.get("url")), out.get("url") + assert SENTINEL not in str(out.get("proxy_url")), out.get("proxy_url") + # The user, host and path SURVIVE. Masking the whole URL would destroy the operator's view rather + # than protect it, and nothing would report that as a loss. + assert "user:***@example.invalid/y" in str(out["url"]) + + +def test_a_url_without_userinfo_is_left_alone() -> None: + """The control half. A masker that rewrites every URL would pass the assertions above while + silently mangling ordinary configuration.""" + spec = messagefoundry.Rest(url="https://plain.invalid/path?q=1&r=2") + assert redacted_settings(dict(spec.settings))["url"] == "https://plain.invalid/path?q=1&r=2"