Skip to content

Redact non-string values when the key name is sensitive (#71275) - #71279

Open
shashbha14 wants to merge 3 commits into
apache:mainfrom
shashbha14:fix/secrets-masker-redact-non-strings-71275
Open

Redact non-string values when the key name is sensitive (#71275)#71279
shashbha14 wants to merge 3 commits into
apache:mainfrom
shashbha14:fix/secrets-masker-redact-non-strings-71275

Conversation

@shashbha14

@shashbha14 shashbha14 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

_redact_all only replaces strings. Everything else falls through the return item at the bottom untouched, so a Variable with a sensitive-looking key comes back in the clear if its value happens to be numeric:

airflow variables set test-password-a "abcd" -> ***
airflow variables set test-password-b "1234" -> 1234
The variables endpoint runs json.loads() on the value first, so "1234" is already an int by the time it reaches the masker, while "abcd" fails to parse and stays a string. A numeric PIN or an all-digit API key under a *_password key leaks; the same value with a letter in it doesn't. The comment above the call site says this path has to fail closed at any nesting level, and it does that for depth, just not for type.

Fixed by flipping the check so only containers get walked and everything else is replaced:

if depth > max_depth or not isinstance(item, (dict, tuple, set, list)):
return replacement
That leaves the old return item unreachable. I kept a return there but made it replacement, so it still fails closed if a container type gets added to the walk later without a branch.

One file only, by the way — shared/ is symlinked into both airflow-core and the task SDK, so the copy under airflow/sdk/_shared/ that the issue mentions is the same file.

The issue asked whether this breaks merge(). It doesn't. _merge restores the original whenever the redacted value is still , and it never looks at the original's type, so 1234 -> "" -> 1234 round-trips fine. Two tests for that.

One thing to flag: None under a sensitive key now becomes *** instead of staying null. I think that's the right call, since otherwise you can still tell "no secret set" apart from "secret hidden", but say the word if you'd rather I special-cased it. Connection passwords aren't affected either way — redact_password already returns early on None.

Tests: updated test_redact_all_directly, which was pinning the old behaviour, and added coverage for int/float/bool/None/bytes scalars, nested containers, a sensitive key nested in a non-sensitive one, and the merge round-trip. 154 pass locally, along with mypy and ruff.

closes: #71275

Was generative AI tooling used to co-author this PR?
Yes claude was used.

@shashbha14
shashbha14 marked this pull request as ready for review August 7, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Secrets masker: non-string values are not redacted when the key name is sensitive

1 participant