potiuk commented on code in PR #68511:
URL: https://github.com/apache/airflow/pull/68511#discussion_r3711649182
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -664,9 +701,14 @@ def deserialize(cls, encoded_var: Any) -> Any:
kwargs = deser["kwargs"]
del deser
if type_ == DAT.AIRFLOW_EXC_SER:
Review Comment:
Yes — #68662. It removes the encode side entirely and makes decode
legacy-only, returning `str(BaseException(*args))` without resolving or calling
anything.
That is a stronger position than this PR, which still resolves a
payload-supplied name — constrained to loaded `AirflowException` subclasses —
and then calls it. The cost is that a legacy node deserializes to a string
rather than an exception object.
So the real question is whether anything still needs a real exception object
back. If not, #68662 is the better fix and this should close in its favour. If
something does, this keeps round-trip fidelity and #68662 breaks it.
No attachment to this one either way — flagging it so the two do not both
land.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -242,6 +242,43 @@ def _decode_priority_weight_strategy(var: str) ->
PriorityWeightStrategy:
return priority_weight_strategy_class()
+# Builtin exceptions a ``BASE_EXC_SER`` node can be rebuilt into. The encode
side matches
+# ``KeyError`` / ``AttributeError`` *and their subclasses* while storing the
concrete class name,
+# so a user-defined subclass serializes to a name that is absent here and
cannot be rebuilt --
+# which was equally true when the name was imported, since ``builtins`` does
not hold it either.
+# Resolving against this map keeps ``builtins.eval`` / ``builtins.exec`` out
without importing.
+_DESERIALIZABLE_BUILTIN_EXCEPTIONS: dict[str, type[BaseException]] = {
+ "KeyError": KeyError,
+ "AttributeError": AttributeError,
+}
+
+
+def _resolve_airflow_exception(exc_cls_name: str) -> type[AirflowException]:
+ """
+ Resolve a serialized ``AirflowException`` class name to the loaded class,
without importing it.
+
+ The module part is looked up in ``sys.modules`` and the class is read out
of that module's
+ namespace, so a name in the stored blob can never cause an import: a
module that is not already
+ loaded simply fails to resolve. The result must be an ``AirflowException``
subclass, so an
+ attacker's ``subprocess.check_output`` is rejected even when
``subprocess`` is loaded.
+
+ The namespace is read directly rather than through ``getattr`` so that a
module-level
+ ``__getattr__`` -- which Airflow uses for deprecation shims and lazy
provider re-exports -- stays
+ out of the path, since those hooks do import on access.
+
+ Resolving the name instead of matching it against a prebuilt map is also
what keeps blobs
+ written by older versions readable: these exceptions moved to
``airflow.sdk.exceptions`` in
+ 3.2.0 and are re-exported from ``airflow.exceptions``, so a 3.0/3.1 blob
naming the old module
+ still resolves, exactly as it did when the name was imported.
+ """
Review Comment:
All five taken. One character changed from the suggestion: `wont` → `won't`,
since codespell rejects the contraction without the apostrophe.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
