mobuchowski commented on code in PR #75860:
URL: https://github.com/apache/airflow/pull/75860#discussion_r4403595374
##########
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##########
@@ -951,8 +953,13 @@ def _include_fields(self):
"nor is defined as an attrs class."
)
for field, val in obj_fields.items():
+ # Keys that aren't identifiers are set by libraries rather
than being attributes, e.g. the
+ # unpicklable ``__wire|<class>|<method>`` caches of
``methodtools.lru_cache``.
+ if not field.isidentifier():
+ continue
if field not in self._fields and field not in self.excludes
and field not in self.renames:
- setattr(self, field, val)
+ # Any attribute may hold live Airflow objects, even nested
ones that can't be pickled
+ setattr(self, field,
_convert_to_json_safe(self._cast_basic_types(val), field))
Review Comment:
`OpenLineageRedactor._redact` sends anything that is JSON-serializable and
has a `__dict__` to a branch that only redacts `item.__dict__`, then returns
the item. A dict subclass always has `__dict__`, and after
`self.__dict__.clear()` it's always empty, so that branch never masks anything
for info objects. Before this change, `TaskInfoComplete` almost always held at
least one non-JSON value (even a plain `BashOperator` has a function among its
attributes), so it failed `is_json_serializable` and fell through to
`SecretsMasker._redact`. That method walks the dict items and masks registered
secrets and sensitive key names. Now that every value is JSON-native,
`TaskInfoComplete` always takes the branch that masks nothing.
I reproduced it with `mask_secret("s3cr3tvalue")`, a
`BashOperator(bash_command="curl -u user:s3cr3tvalue …")` and an operator
attribute holding a `Connection(password="s3cr3tvalue")`, then ran
`OpenLineageRedactor.from_masker(_secrets_masker()).redact(...)`:
| | `bash_command` | `conn` |
|---|---|---|
| base (`27c76244`) | `curl -u user:*** …` | `Connection(...,
password='***', ...)` |
| previous head (`a3692d6e`) | `curl -u user:*** …` | `Connection(...,
password='***', ...)` |
| this head (`9c22a0a8`) | `curl -u user:s3cr3tvalue …` | `Connection(...,
password='s3cr3tvalue', ...)` |
So with `include_full_task_info` on, rendered templated fields and secrets
held on the operator are emitted in plain text. A plain `BashOperator`, with no
extra attribute, leaks its rendered `bash_command` too. The `default=str`
conversion makes this worse: values the client's `Serde` used to emit as
`<<non-serializable: Connection>>` are now emitted as their full `repr`, and
attrs classes such as `Connection` include every field in it.
One-line fix I tested (the rest of the openlineage unit tests still pass):
don't send dicts to the attribute-walking branch, so the base masker walks
their items:
```python
if is_json_serializable(item) and hasattr(item, "__dict__") and not
isinstance(item, dict):
```
A test that redacts a `TaskInfoComplete` with a masked secret in a templated
field would guard against this regressing again.
--
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]
