From e6e655335e8d8e9b260683b7861ad6a99f45e812 Mon Sep 17 00:00:00 2001 From: disqualifier Date: Mon, 6 Jul 2026 17:54:33 -0400 Subject: [PATCH] fix: mask_proxy/mask_url never return a credential in the clear (structure-preserving) three credential-leak shapes where a redactor returned the secret verbatim (all 718c8a7): - mask_proxy host:port:cred (3 parts) fell through to a verbatim return; and a >4-part spec bulleted only the LAST field, leaking a colon-bearing password's earlier halves. now everything after host:port is credential material and collapses to a single :**** for any shape (3/4/n parts), so no credential field survives. plain host:port/bare host unchanged. - mask_url's fragment used a lossless parse_qsl->urlencode round-trip gate that a =-padded base64 token failed, passing the whole fragment (token included) through verbatim. _mask_qs is now structure-preserving: it splits on & and the first =, replaces only a sensitive key's value with *** (covering a =-padded/embedded-= token whole), and leaves every other segment byte-identical - so a non-secret value is never corrupted and the round-trip gate (the root of the leak) is gone. fragment and query mask identically now. invariant: never emit a credential in the clear; over-mask when the shape is ambiguous. Signed-off-by: disqualifier --- src/commons/masking.py | 85 +++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 46 deletions(-) diff --git a/src/commons/masking.py b/src/commons/masking.py index 9cd6c9b..2a5002e 100644 --- a/src/commons/masking.py +++ b/src/commons/masking.py @@ -3,7 +3,7 @@ DISPLAY only, not a security control: the underlying value is unchanged and still needs proper handling (encryption at rest, etc.). """ -from urllib.parse import parse_qsl, quote_plus, urlencode, urlsplit, urlunsplit +from urllib.parse import urlsplit, urlunsplit SENSITIVE_QUERY_KEYS = frozenset( {"apikey", "api_key", "key", "token", "access_token", "refresh_token", @@ -66,24 +66,23 @@ def provider(card_number: str) -> str: def _mask_qs(qs: str, sensitive: "frozenset[str]") -> str: - """mask sensitive key=value pairs in a query-string-shaped blob (query or fragment)""" - pairs = parse_qsl(qs, keep_blank_values=True) - return urlencode( - [(k, "***" if k.lower() in sensitive else v) for k, v in pairs], - safe="*", - quote_via=quote_plus, - ) + """mask sensitive ``key=value`` values in a query/fragment blob, structure-preserving - -def _is_query_shaped(qs: str) -> bool: - """true when a blob is genuinely ``key=value&key=value`` shaped, not merely containing ``=`` - - a lossless parse_qsl -> urlencode round-trip proves the blob was already in canonical - query form; anything else (a spa hash route like ``#/page?x=1``, a value that itself - contains ``=`` like ``#a=b=c``) fails the round-trip and must not be rewritten. + splits on ``&`` and each pair on its FIRST ``=``; a sensitive key's value (everything + after that first ``=``, so a ``=``-padded base64 token is covered whole) becomes ``***``. + non-sensitive segments are left BYTE-IDENTICAL - no re-encoding, so a value with a + reserved char, an embedded ``=``, or a non-kv segment (a spa hash route like ``/page``) + is never corrupted. a sensitive token in the fragment is therefore always masked, never + passed through verbatim. """ - pairs = parse_qsl(qs, keep_blank_values=True) - return urlencode(pairs, safe="*", quote_via=quote_plus) == qs + out = [] + for segment in qs.split("&"): + key, sep, _ = segment.partition("=") + if sep and key.lower() in sensitive: + out.append(f"{key}=***") + else: + out.append(segment) + return "&".join(out) def mask_url(url: str, *, keys: "frozenset[str] | None" = None) -> str: @@ -91,16 +90,12 @@ def mask_url(url: str, *, keys: "frozenset[str] | None" = None) -> str: and fragment values param-name matching is case-insensitive; ``keys`` overrides the default sensitive-name - set. non-url or unparseable input is returned unchanged. non-sensitive query values are - re-percent-encoded on the way out (``quote_plus``), so a value containing a reserved - character (e.g. ``&``, ``=``, a space) round-trips correctly instead of corrupting the - rebuilt url. the fragment is masked the same way as the query WHEN it is genuinely - ``key=value&key=value`` shaped - this covers oauth implicit-grant callbacks that put - ``access_token`` in the fragment instead of the query. a fragment that merely contains - an ``=`` without being canonical query shape (a spa hash route like ``#/page?x=1``, a - value with an embedded ``=`` like ``#a=b=c``) is left untouched rather than risking a - lossy rewrite of a non-secret value; a plain anchor fragment (e.g. ``#section``, no - ``=`` at all) also passes through unchanged. + set. non-url or unparseable input is returned unchanged. masking is structure-preserving + (see ``_mask_qs``): only a sensitive key's value is replaced, everything else is left + byte-identical, so a non-secret value is never corrupted and a non-kv fragment (a spa + hash route like ``#/page``, a plain anchor ``#section``) passes through unchanged. the + fragment is masked the same way as the query, covering oauth implicit-grant callbacks + that put ``access_token`` (including a ``=``-padded base64 token) in the fragment. """ sensitive = SENSITIVE_QUERY_KEYS if keys is None else frozenset(k.lower() for k in keys) try: @@ -113,9 +108,7 @@ def mask_url(url: str, *, keys: "frozenset[str] | None" = None) -> str: if "@" in netloc: netloc = netloc.rsplit("@", 1)[1] query = _mask_qs(parts.query, sensitive) if parts.query else parts.query - fragment = parts.fragment - if fragment and "=" in fragment and _is_query_shaped(fragment): - fragment = _mask_qs(fragment, sensitive) + fragment = _mask_qs(parts.fragment, sensitive) if parts.fragment else parts.fragment return urlunsplit((parts.scheme, netloc, parts.path, query, fragment)) @@ -134,32 +127,32 @@ def _split_host(rest: str) -> "tuple[str, list[str]]": def _bullet_trailing_fields(rest: str) -> str: - """bullet the trailing colon-separated credential field(s) of a ``host:port[:...]`` tail""" + """redact any credential fields of a ``host:port[:user:password...]`` tail + + keeps ``host`` and the ``port`` (the first colon field after host) in the clear; every + field AFTER the port is credential material and is replaced with a single ``****``, + regardless of how many there are (so a colon-bearing password can't leak a fragment). + a bare ``host`` or ``host:port`` (no trailing fields) passes through unchanged - fails + safe: anything with a credential field is bulleted, never returned verbatim. + """ host, fields = _split_host(rest) if fields == [""]: return host if len(fields) == 1: return f"{host}:{fields[0]}" - if len(fields) == 3: - port, user, _ = fields - return f"{host}:{port}:{user}:****" - if len(fields) > 3: - return host + ":" + ":".join(fields[:-1]) + ":****" - return rest + port = fields[0] + return f"{host}:{port}:****" def mask_proxy(spec: str) -> str: """redact credentials in a proxy spec for logging - the canonical ``host:port:user:password`` (exactly 4 colon-separated parts) bullets - the password to ``:****``. a plain ``host:port`` (no auth) passes through unchanged. - any other shape that carries credentials - ``user:pass@host:port`` userinfo, a - scheme-prefixed url with trailing colon fields, or a colon-separated spec with more - than 4 parts - is masked rather than ever returned verbatim: a ``scheme://`` prefix - is dropped, then any ``user:pass@`` userinfo is dropped, then the remaining - ``host:port[:user:password]`` tail has its trailing field(s) bulleted when there are - more than 2 colon-separated parts. fails safe: this masker never returns a still- - credential-bearing string verbatim. + drops a ``scheme://`` prefix and any ``user:pass@`` userinfo, then keeps only + ``host:port`` in the clear and bullets EVERY trailing colon field (user/password/etc.) + to a single ``:****`` - so ``host:port:user:password`` and any longer spec (a + colon-bearing password, extra fields) all collapse to ``host:port:****``. a plain + ``host:port`` or bare ``host`` (no auth) passes through unchanged. fails safe: whenever + a credential field is present it is bulleted, never returned verbatim. """ rest = spec if "://" in rest: