shashbha14 opened a new pull request, #71279:
URL: https://github.com/apache/airflow/pull/71279
_redact_all is the path the masker takes once should_hide_value_for_key()
decides a key name is sensitive. It's supposed to be the fail-closed one, but
it only replaces str and hands back every other scalar untouched:
if depth > max_depth or isinstance(item, str):
return replacement
...
return item # int, float, bool, bytes all escape through here
So a numeric secret under a sensitive key comes straight back out of the API:
airflow variables set test-password-a "abcd" -> ***
airflow variables set test-password-b "1234" -> 1234
json.loads("1234") gives you an int, and the int walks out through that last
return item. A PIN, an account number, or an all-digit API key stored under
*_password / *_token leaks, while the exact same value with a letter in it gets
masked. The comment above the call site already says this path "must fail
closed at any nesting level" — it does that for depth, but not for type.
The fix
Invert 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 makes the trailing return item unreachable. I kept a return there but
changed it to replacement, so if someone later adds a container type to the
walk and misses a branch, it fails closed instead of leaking.
Worth noting only one file needed touching — the issue mentions the copy
under airflow/sdk/_shared/, but shared/ is symlinked into both trees.
On merge()
The issue asked for a maintainer to confirm this doesn't break merge(), so I
traced it. _merge restores the original whenever the redacted value is still
the *** sentinel, and it never inspects the original's type — so 1234 -> "***"
-> merge -> 1234 round-trips fine. Two tests pin that.
One judgement call
None under a sensitive key now comes back as *** instead of null. I did that
deliberately, since leaving it lets you tell "no secret set" apart from "secret
hidden", but it's the debatable part of this change — happy to special-case it
if you'd rather.
Connection.password isn't affected either way; redact_password already
short-circuits None before it reaches the masker.
Tests
Updated test_redact_all_directly, which was pinning the old behaviour, and
added TestNonStringRedactionForSensitiveKeys covering int / float / bool / zero
/ None / bytes / str, nested containers, a sensitive key nested inside a
non-sensitive dict, and both merge round-trips.
154 pass locally. There's one pre-existing failure, test_redact_filehandles,
which opens /dev/null and so fails on Windows regardless of this change.
Closes #71275
Used Claude Code to help write and test this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
