From 871e1e8963b9d1d940aa59fe3f1403dcd9f7525f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 9 Aug 2026 10:38:16 -0500 Subject: [PATCH 1/2] fix(config): close the last two redaction holes -- env() in headers, URL userinfo (#1207) Both sit INSIDE surfaces the redactor already claimed to handle, which is why neither was visible from the outside. (b) The headers branch had no EnvRef arm, so an env() ref in a headers table came back as the RAW object with its `default` intact -- not JSON-safe, and the default is a FALLBACK SECRET by construction. The same env() on a top-level credential correctly emits {"env": key} with the default dropped. A hole inside the one container the header rule already covered. THE DEFAULT IS NOW DROPPED FOR EVERY HEADER, not only credential-shaped ones. The measured instance used X-Vendor-Thing, which matches no substring in the name rule -- so gating the EnvRef arm on that rule would have left this exact case open. A header 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. (c) url="https://user:SECRET@host" was returned verbatim by both serializers while proxy_password on the SAME object masked -- safe in the typed field, disclosed in the URL beside it. Only the password half of the userinfo is replaced: the user, host and path survive, because an operator diagnosing a connection needs to see which account and which host, and masking the whole URL destroys the view rather than protecting it. A control test asserts a URL WITHOUT userinfo is left byte-identical, since a masker that rewrites every URL would satisfy the leak assertions while mangling ordinary configuration. `proxy` is another parameter-to-setting rename, noticed in passing: the parameter is `proxy`, the emitted setting is `proxy_url`. Same boundary with_signing crosses (#1106), which is why the URL rule is a NAME set plus a suffix rule rather than a suffix rule alone. 95 tests green across the redaction and connection-API suites. Both found by the asvs-tracking-rework session's assessment of ASVS 15.3.1, alongside the odbc_params disclosure fixed as #1206. With these closed, all three surfaces holding that cell at partial are addressed and it is due a re-read -- by that session, not me, since I authored all three fixes. PROCESS NOTE AGAINST MYSELF: the code comments here cited #1207 BEFORE the number was allocated. It happened to be next, so nothing collided -- but "happened to be next" is exactly the reasoning alloc.ps1 exists to eliminate, and two sessions doing it at once is the documented failure. Allocate, then write. --- docs/BACKLOG.md | 51 +++++++++++++++++++ messagefoundry/config/wiring.py | 49 +++++++++++++++++- ...est_connection_factory_redaction_domain.py | 47 +++++++++++++++++ 3 files changed, 146 insertions(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 85fe84cf..a346d518 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8887,3 +8887,54 @@ 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. 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" From 06a32a0b2e56e98f0ce46e59efa92226d1ecb7d8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 9 Aug 2026 10:43:43 -0500 Subject: [PATCH 2/2] backlog: file #1208, the rename boundary itself needs a guard proxy -> proxy_url makes THREE measured instances of one shape: a factory takes a credential PARAMETER and emits it under a different SETTING name, while every redaction control operates on the setting name. private_key -> sign_private_key was #1106; proxy -> proxy_url is harmless only because the URL rule happens to cover it, which is luck rather than design. Neither existing guard covers the MAPPING. One reads parameter names, an abstraction level from where redaction operates -- that is how #1106 walked through it. The other reads emitted settings end to end and proves the OUTCOME per factory, not that the parameter arrived anywhere at all. Filed into this branch rather than its own, to avoid a fourth three-way conflict on docs/BACKLOG.md while the engine queue is serialized; it is the route-onward from the same finding as #1207. Raised by the asvs-tracking-rework session, filed before it was forgotten. --- docs/BACKLOG.md | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index a346d518..99d03eb0 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8938,3 +8938,46 @@ is due a re-read - by that session, not by me, since I authored all three fixes. 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.