andreahlert commented on code in PR #61776:
URL: https://github.com/apache/airflow/pull/61776#discussion_r2794954027
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -260,15 +260,38 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({"msg", "args"})
- def _redact_exception_with_context(self, exception):
+ def _redact_exception_with_context_or_cause(self, exception, visited=None):
# Exception class may not be modifiable (e.g. declared by an
# extension module such as JDBC).
with contextlib.suppress(AttributeError):
+ if visited is None:
+ visited = set()
+
+ if id(exception) in visited:
+ # already visited - it was redacted earlier
+ return exception
+
+ # Check depth before adding to visited to ensure we skip
exceptions beyond the limit
+ if len(visited) >= self.MAX_RECURSION_DEPTH:
+ return RuntimeError(
+ f"Stack trace redaction hit recursion limit of
{self.MAX_RECURSION_DEPTH} "
+ f"when processing exception of type
{type(exception).__name__}. "
+ f"Redaction of this exception will be skipped to avoid "
+ f"infinite recursion."
+ )
+
+ visited.add(id(exception))
+
exception.args = (self.redact(v) for v in exception.args)
Review Comment:
nit: I think this assigns a generator to `args` instead of a tuple. If
`args` is accessed more than once, it'd be empty on the second read since the
generator gets consumed. Not introduced by this PR, but since we're touching
this area it might be worth wrapping it in `tuple()`. WDYT?
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -260,15 +260,38 @@ def _record_attrs_to_ignore(self) -> Iterable[str]:
)
return frozenset(record.__dict__).difference({"msg", "args"})
- def _redact_exception_with_context(self, exception):
+ def _redact_exception_with_context_or_cause(self, exception, visited=None):
# Exception class may not be modifiable (e.g. declared by an
# extension module such as JDBC).
with contextlib.suppress(AttributeError):
+ if visited is None:
+ visited = set()
+
+ if id(exception) in visited:
+ # already visited - it was redacted earlier
+ return exception
+
+ # Check depth before adding to visited to ensure we skip
exceptions beyond the limit
+ if len(visited) >= self.MAX_RECURSION_DEPTH:
+ return RuntimeError(
+ f"Stack trace redaction hit recursion limit of
{self.MAX_RECURSION_DEPTH} "
+ f"when processing exception of type
{type(exception).__name__}. "
Review Comment:
Small thing on the message: it says "redaction of this exception will be
skipped" but IIUC the original exception is actually being replaced by this
sentinel RuntimeError, not skipped. Something like "remainder of exception
chain has been truncated" might be more accurate? Not sure if I'm reading this
correctly though.
##########
shared/secrets_masker/tests/secrets_masker/test_secrets_masker.py:
##########
@@ -238,6 +238,213 @@ def test_masking_in_explicit_context_exceptions(self,
logger, caplog):
"""
)
+ def test_redact_exception_with_context_simple(self):
+ """
+ Test _redact_exception_with_context with a simple exception without
context.
+ """
+ masker = SecretsMasker()
+ configure_secrets_masker_for_test(masker)
+ masker.add_mask(PASSWORD)
+
+ exc = RuntimeError(f"Cannot connect to user:{PASSWORD}")
+ masker._redact_exception_with_context_or_cause(exc)
+
+ assert "password" not in str(exc.args[0])
+ assert "user:***" in str(exc.args[0])
+
+ def test_redact_exception_with_implicit_context(self):
+ """
+ Test _redact_exception_with_context with exception __context__
(implicit chaining).
+ """
+ masker = SecretsMasker()
+ configure_secrets_masker_for_test(masker)
+ masker.add_mask(PASSWORD)
+
+ try:
+ try:
+ raise RuntimeError(f"Inner error with {PASSWORD}")
+ except RuntimeError:
+ raise RuntimeError(f"Outer error with {PASSWORD}")
+ except RuntimeError as exc:
+ captured_exc = exc
+
+ masker._redact_exception_with_context_or_cause(captured_exc)
+ assert "password" not in str(captured_exc.args[0])
+ assert "password" not in str(captured_exc.__context__.args[0])
+
+ def test_redact_exception_with_explicit_cause(self):
+ """
+ Test _redact_exception_with_context with exception __cause__ (explicit
chaining).
+ """
+ masker = SecretsMasker()
+ configure_secrets_masker_for_test(masker)
+ masker.add_mask(PASSWORD)
+
+ try:
+ inner = RuntimeError(f"Cause error: {PASSWORD}")
+ raise RuntimeError(f"Main error: {PASSWORD}") from inner
+ except RuntimeError as exc:
+ captured_exc = exc
+
+ masker._redact_exception_with_context_or_cause(captured_exc)
+ assert "password" not in str(captured_exc.args[0])
+ assert "password" not in str(captured_exc.__cause__.args[0])
+
+ def test_redact_exception_with_circular_context_reference(self):
+ """
+ Test _redact_exception_with_context handles circular references
without infinite recursion.
+ """
+ masker = SecretsMasker()
+ configure_secrets_masker_for_test(masker)
+ masker.add_mask(PASSWORD)
+
+ exc1 = RuntimeError(f"Error with {PASSWORD}")
+ exc2 = RuntimeError(f"Another error with {PASSWORD}")
+ # Create circular reference
+ exc1.__context__ = exc2
+ exc2.__context__ = exc1
+
+ # Should not raise RecursionError
+ masker._redact_exception_with_context_or_cause(exc1)
+
+ assert "password" not in str(exc1.args[0])
+ assert "password" not in str(exc2.args[0])
+
+ def test_redact_exception_with_max_context_recursion_depth(self):
+ """
+ Test _redact_exception_with_context respects MAX_RECURSION_DEPTH.
+ Exceptions beyond the depth limit should be skipped (not redacted).
+ """
+ masker = SecretsMasker()
+ configure_secrets_masker_for_test(masker)
+ masker.add_mask(PASSWORD)
+
+ # Create a long chain of exceptions
+ exc_chain = RuntimeError(f"Error 0 with {PASSWORD}")
+ current = exc_chain
+ for i in range(1, 10):
+ new_exc = RuntimeError(f"Error {i} with {PASSWORD}")
+ current.__context__ = new_exc
+ current = new_exc
+
+ masker._redact_exception_with_context_or_cause(exc_chain)
+
+ # Verify redaction happens up to MAX_RECURSION_DEPTH
+ # The check is `len(visited) >= MAX_RECURSION_DEPTH` before adding
current exception
+ # So it processes exactly MAX_RECURSION_DEPTH exceptions (0 through
MAX_RECURSION_DEPTH-1)
+ current = exc_chain
+ for i in range(10):
+ assert current, "We should always get some exception here"
+ if i < masker.MAX_RECURSION_DEPTH:
+ # Should be redacted within the depth limit
+ assert "password" not in str(current.args[0]), f"Exception {i}
should be redacted"
+ else:
+ assert "hit recursion limit" in str(current.args[0]), (
+ f"Exception {i} should indicate recursion depth limit hit"
+ )
+ assert "password" not in str(current.args[0]), f"Exception {i}
should not be present"
+ assert current.__context__ is None, (
+ f"Exception {i} should not have a context due to depth
limit"
+ )
+ break
+ current = current.__context__
+
+ def test_redact_exception_with_circular_cause_reference(self):
+ """
+ Test _redact_exception_with_context_or_cause handles circular
__cause__ references without infinite recursion.
+ """
+ masker = SecretsMasker()
+ configure_secrets_masker_for_test(masker)
+ masker.add_mask(PASSWORD)
+
+ exc1 = RuntimeError(f"Error with {PASSWORD}")
+ exc2 = RuntimeError(f"Another error with {PASSWORD}")
+ # Create circular reference using __cause__
+ exc1.__cause__ = exc2
+ exc2.__cause__ = exc1
+
+ # Should not raise RecursionError
+ masker._redact_exception_with_context_or_cause(exc1)
+
+ assert "password" not in str(exc1.args[0])
+ assert "password" not in str(exc2.args[0])
Review Comment:
Not a reviewer here, just reading through the PR. I noticed there are tests
for circular `__context__` and circular `__cause__` separately, but I don't see
one for the cross-circular case (e.g. `exc1.__cause__ = exc2` and
`exc2.__context__ = exc1`). The `visited` set should handle it fine, but might
be worth having an explicit test? Feel free to ignore if that's overkill.
--
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]
