fix(sensitive_data_masker): fully mask secrets at or below the reveal threshold (#30764)

* fix(sensitive_data_masker): fully mask secrets at or below the reveal threshold

_mask_value did partial reveal by showing the first visible_prefix and last
visible_suffix characters, but for a value whose length was at or below
visible_prefix + visible_suffix (8 by default) it returned the value verbatim.
A value of exactly 8 chars fell through the length guard and computed
masked_length == 0, reconstructing the original string with no mask characters;
anything shorter hit the early return. Either way short credentials were emitted
in plaintext.

mask_dict routes real secrets through this path, so an 8-char-or-shorter redis
password, api key, or token could be written to logs and the UI unmasked. The
sibling helper mask_sensitive_keys already guards this case; _mask_value now does
the same by fully masking any value at or below the threshold.

* fix(sensitive_data_masker): add mask_short_values opt-out for truncation callers

Fully masking short values is the right default for secret masking, but
CooldownCache reuses the masker purely to truncate exception messages to the
first 50 characters, and it relies on short messages being returned readable.
Masking those blanked out short exception text and broke its tests.

Add a mask_short_values flag (default True, secure) and have CooldownCache pass
False so it keeps the truncation behavior, while every secret-masking caller
still gets short values fully masked.

* fix(mcp_debug): opt out of short-value masking to keep diagnostic token preview

MCPDebug uses the masker to preview auth tokens in debug headers and documents
that values of 10 chars or fewer are shown unchanged so token types stay
distinguishable. Pass mask_short_values=False so that diagnostic behavior is
preserved while secret maskers keep masking short values.

* fix(mcp_debug): mask short auth values in debug headers instead of echoing them

Earlier this masker opted out of short-value masking to keep a token preview, but
that echoes short authorization and token values verbatim in debug response
headers, which is the same leak this change is meant to close. Auth material
should never be emitted in full, so mask short values here too; the first/last
character preview still applies to longer tokens. Only CooldownCache keeps the
opt-out, since it truncates exception text rather than masking secrets.

* test(mcp_debug): assert masked short value preserves length
This commit is contained in:
Srivatsa Kamballa 2026-06-22 07:02:31 -05:00 committed by GitHub
parent 343ef87880
commit 45b4dca145
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 60 additions and 6 deletions

View file

@ -12,6 +12,7 @@ class SensitiveDataMasker:
visible_prefix: int = 4,
visible_suffix: int = 4,
mask_char: str = "*",
mask_short_values: bool = True,
):
self.sensitive_patterns = sensitive_patterns or {
"password",
@ -38,12 +39,17 @@ class SensitiveDataMasker:
self.visible_prefix = visible_prefix
self.visible_suffix = visible_suffix
self.mask_char = mask_char
self.mask_short_values = mask_short_values
def _mask_value(self, value: str) -> str:
if not value or len(str(value)) < (self.visible_prefix + self.visible_suffix):
return value
value_str = str(value)
if not value_str:
return value
if len(value_str) <= (self.visible_prefix + self.visible_suffix):
return (
self.mask_char * len(value_str) if self.mask_short_values else value_str
)
masked_length = len(value_str) - (self.visible_prefix + self.visible_suffix)
# Handle the case where visible_suffix is 0 to avoid showing the entire string

View file

@ -38,6 +38,7 @@ class CooldownCache:
visible_prefix=50, # Show first 50 characters
visible_suffix=0, # Show last 0 characters
mask_char="*", # Use * for masking
mask_short_values=False, # Truncate long messages only; keep short ones readable
)
def _common_add_cooldown_logic(

View file

@ -126,6 +126,49 @@ def test_lists_with_sensitive_keys_are_masked():
assert masked["tags"] == ["prod", "test"]
def test_short_secrets_are_fully_masked():
"""
Regression test: secrets at or below the reveal threshold (visible_prefix +
visible_suffix, 8 by default) were returned verbatim instead of masked.
An exactly-8-char value hit masked_length == 0 and round-tripped unchanged;
anything shorter hit the early return. Both leaked short credentials (e.g. an
8-char redis password) in plaintext through mask_dict.
"""
masker = SensitiveDataMasker()
# Boundary: exactly 8 chars previously returned verbatim.
assert masker._mask_value("abcd1234") == "********"
# Below threshold previously hit the early return and leaked verbatim.
assert masker._mask_value("sk-12") == "*****"
# Values above the threshold must still partially reveal, not over-mask.
assert masker._mask_value("abcd12345") == "abcd*2345"
masked = masker.mask_dict({"redis_password": "pass1234", "api_key": "sk-7a"})
assert masked["redis_password"] == "********"
assert masked["api_key"] == "*****"
def test_mask_short_values_false_keeps_short_values_readable():
"""
mask_short_values=False opts out of full masking so short values are returned
as-is. This preserves the truncation use (e.g. CooldownCache shows the first 50
chars of an exception and only masks longer tails), while longer values are still
partially masked.
"""
masker = SensitiveDataMasker(
visible_prefix=50, visible_suffix=0, mask_short_values=False
)
short = "Test exception for structure validation"
assert masker._mask_value(short) == short
long_value = "x" * 60
masked = masker._mask_value(long_value)
assert masked.startswith("x" * 50)
assert masked.endswith("*" * 10)
assert len(masked) == 60
def test_cost_per_token_fields_not_masked():
"""
Regression test: cost fields like input_cost_per_token contain "token" in their name

View file

@ -41,9 +41,13 @@ class TestMask:
def test_empty_returns_none_label(self):
assert MCPDebug._mask("") == "(none)"
def test_short_value_unchanged(self):
# visible_prefix=6 + visible_suffix=4 = 10, so <= 10 chars unchanged
assert MCPDebug._mask("sk-1234") == "sk-1234"
def test_short_value_masked(self):
# Short auth values must not be echoed verbatim in debug headers, even though
# visible_prefix + visible_suffix would otherwise reveal the whole value.
masked = MCPDebug._mask("sk-1234")
assert "sk-1234" not in masked
assert set(masked) == {"*"}
assert len(masked) == len("sk-1234")
def test_long_value_masked(self):
result = MCPDebug._mask("Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9")