You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "naruto-lgtm (via GitHub)" <gi...@apache.org> on 2026/06/17 07:50:00 UTC
[PR] validate exception type when deserializing serialized exceptions [airflow]
naruto-lgtm opened a new pull request, #68662:
URL: https://github.com/apache/airflow/pull/68662
`BaseSerialization.deserialize` rebuilds `AIRFLOW_EXC_SER`/`BASE_EXC_SER` payloads by feeding `exc_cls_name` straight into `import_string` and then calling the result as `exc_cls(*args, **kwargs)`, with the class name and arguments all taken from the serialized blob. A tampered payload can name any importable callable (`os.system`, `builtins.exec`, `builtins.eval`, ...) and have it invoked with attacker-chosen arguments, so anything that deserializes a crafted object runs arbitrary code, including the scheduler reading a serialized Dag where user code is never meant to run.
The decoders for timetables, windows and wait policies already refuse non-core import paths before handing them to `import_string`; the exception branch was the gap. This checks that the resolved object is a real exception type before instantiating it and raises `DeserializationError` otherwise. Putting the check next to the import keeps both exception branches covered in one place, and legitimate `AirflowException`/`KeyError`/`AttributeError` round-trips are unaffected.
---
##### Was generative AI tooling used to co-author this PR?
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3918814443
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -631,6 +631,11 @@ def deserialize(cls, encoded_var: Any) -> Any:
return parse_timezone(var)
elif type_ == DAT.RELATIVEDELTA:
return decode_relativedelta(var)
+ elif type_ in (DAT.AIRFLOW_EXC_SER, DAT.BASE_EXC_SER):
+ # Legacy rows only. ``exc_cls_name`` is payload-supplied, so it is never resolved or
+ # called; the recorded args are rendered with BaseException semantics, giving the same
+ # string form an exception now serializes to.
+ return str(BaseException(*cls.deserialize(var).get("args", ())))
Review Comment:
This probably shouldn't be BaseException -- `try ... except Exception:`can't catch BaseExeception.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-5083398262
On wider implications, I went back through the surfaces this could touch.
`BaseSerialization` is the Dag serialization framework, so its persisted output is the `serialized_dag` table. The two stores you'd normally worry about don't go through it: XCom values are encoded by `XComEncoder` in `airflow/utils/json.py`, which delegates to `airflow.sdk.serde`, and trigger kwargs are serialized with `airflow.sdk.serde` as well in `models/trigger.py`. Neither path ever produced or read an `airflow_exc_ser` blob, so dropping the branch doesn't reach them.
`AIRFLOW_EXC_SER` and `BASE_EXC_SER` are referenced only in `serialized_objects.py` and its own tests. No provider and no task-sdk code references either member, so nothing out of tree is decoding them by name.
The exceptions' own `serialize()` methods don't go dead either. The hash-stable fingerprint helper (`serialize_object` in `serialization/helpers.py`, and the task-sdk copy in `task_runner.py`) dispatches duck-typed on anything exposing `serialize()` and only folds the result into a JSON structure. It never imports and calls a class, so that path is unaffected and Dag hashes don't move.
The one sharp edge worth naming: for an exception to be in a `serialized_dag` row at all it has to be reachable from a Dag-level value, so a param default, something in `default_args`, an operator attribute. If such a row was written by an older version, reading it back now raises `TypeError: Invalid type ...` instead of reconstructing. New rows are written when the Dag hash changes, but older DagVersion rows stick around, so it isn't fully self-healing. My read is that failing loudly is the right behaviour there and it's hard to hit in practice, but if you'd rather no read path can raise, I can add a tolerant decode that returns the string form for those two legacy type tags. Happy to push that if you prefer it.
CI is green on the current head.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-4745764407
When is an exception class serialized such that we need to be able to throw it from another process? I can't think of any cases that need that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3645770932
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -582,7 +561,7 @@ def serialize(
elif isinstance(var, XComArg):
return cls._encode(serialize_xcom_arg(var), type_=DAT.XCOM_REF)
elif isinstance(var, LazySelectSequence):
- return cls.serialize(list(var))
+ return cls.serialize(list(var), strict=strict)
Review Comment:
What does `strict=True` (or False) even do?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-5083369558
On the wider implications, I went through the surfaces that actually carry BaseSerialization blobs:
**Serialized Dags** — regenerated on every parse, and a Dag never legitimately holds an exception object, so nothing there.
**Trigger / next_kwargs, encode side** — the current path serializes with `airflow.sdk.serde`, not BaseSerialization, so it was never emitting the exception tags anyway. `Trigger.submit_failure` already packs a formatted traceback rather than the exception instance. The one place core still encodes through BaseSerialization is the old-worker compat converter in `api_fastapi/execution_api/versions/v2026_04_06.py`, which re-serializes next_kwargs for task-sdk < 1.2. If a trigger event payload carried an actual exception instance, an old worker now receives `str(exc)` where it previously got a reconstructed instance.
**Trigger / next_kwargs, decode side** — this is the one sharp edge. `trigger.py` still falls back to `BaseSerialization.deserialize` for rows written in the old format. A row that already contains an `airflow_exc_ser` / `base_exc_ser` tag, i.e. a task deferred before the upgrade whose next_kwargs nested an exception, now raises `TypeError: Invalid type ...` instead of reconstructing it. Narrow, but it is an upgrade-path regression rather than a purely internal change, so worth naming.
If you want that closed, the smallest version is to keep the two tags recognised on the decode side only and return the payload's string form instead of importing and calling anything. Old rows keep deserializing across an upgrade, no `import_string` on a payload-supplied name comes back, and the encode side stays gone so nothing new is ever written in that format. Happy to push that if you'd prefer it over a clean break.
Otherwise: no in-tree references to the removed `DagAttributeTypes` members remain. Third-party code reaching for `DAT.AIRFLOW_EXC_SER` directly would break, but that enum isn't public API.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward strict through LazySelectSequence serialization [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-6057410464
#68511 landed while this was sitting and closed the actual hole: both branches now resolve
against already-loaded classes and a fixed builtin allowlist, with no `import_string` on a
payload-supplied name. So the original reason for this PR is gone. I've dropped those commits
and rebased, which also takes out the `str(BaseException(...))` line flagged above, since the
legacy-decode shim it belonged to went with them.
What survives is the one thing #68511 didn't touch: the `LazySelectSequence` branch recurses
with `cls.serialize(list(var))` and drops `strict`, so an unserializable element inside one of
those gets stringified instead of raising under `strict=True`. That's also the answer to the
`strict` question upthread, and why the guard test never caught it. It was breaking out of its
AST walk on `var.serialize()` in the exception encode branch, so it only ever validated 2 of
the 8 recursions and never reached that line. Walking past instead of breaking makes it cover
the method and flag the missing kwarg on its own.
On removing exception ser/deser from the general framework: I've left that out rather than
carry it here. #68511 deliberately kept that path working, including the pre-3.2.0 re-export
handling in `_resolve_airflow_exception` and the tests around it, and deleting that two months
after it merged is a bigger call than I should make on my own in a stale PR. Happy to do it as
a separate PR if it's still wanted.
---
Drafted-by: Claude Code (Opus 5) (no human review 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-5101078479
Fixed the upgrade path in a55dd765. The two tags are recognised again on the decode side only: an old `airflow_exc_ser`/`base_exc_ser` row now deserializes to the exception's message instead of raising `TypeError`. Nothing emits them anymore, so no new row is ever written in that format and there's no `import_string` on a payload-supplied name to come back.
I kept it to the plain message rather than rendering the class name, so an old row is indistinguishable from a new one: `str(exc)` is what a live exception serializes to now. That also settles the encode-side note I raised, since the old-worker compat converter in `v2026_04_06.py` and the legacy decode agree on the same form.
Tests go both ways. Legacy blobs for `AirflowException`, `AirflowRescheduleException` and `KeyError` come back as their message, and one naming `os.system` or `exec` returns the string with nothing executed. All 5 fail on the previous head.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3442013741
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -658,14 +659,18 @@ def deserialize(cls, encoded_var: Any) -> Any:
args = deser["args"]
kwargs = deser["kwargs"]
del deser
+ # ``exc_cls_name`` comes from the serialized payload, so resolve it without handing an
+ # attacker-controlled string to ``import_string`` -- that would execute the named
+ # module's top-level code before any validation. The encode side only ever emits
+ # ``airflow.*`` AirflowException subclasses for AIRFLOW_EXC_SER and builtin
+ # KeyError/AttributeError for BASE_EXC_SER, so reject anything outside those up front,
+ # mirroring the import-path guards in the timetable/window/wait-policy decoders.
if type_ == DAT.AIRFLOW_EXC_SER:
+ if not exc_cls_name.startswith("airflow."):
Review Comment:
You're right, the prefix is no defense -- airflow.evil is trivial, and it's worse than that: because the base AirflowException.serialize() is inherited, the encode side accepts any AirflowException subclass, so the import path is open-ended (provider and user-defined exceptions included). A prefix check would both miss airflow.evil and break legitimate custom exceptions. I've dropped that commit and reverted to just the post-import issubclass backstop. More on the broader question in the PR thread.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3437045939
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -658,14 +659,18 @@ def deserialize(cls, encoded_var: Any) -> Any:
args = deser["args"]
kwargs = deser["kwargs"]
del deser
+ # ``exc_cls_name`` comes from the serialized payload, so resolve it without handing an
+ # attacker-controlled string to ``import_string`` -- that would execute the named
+ # module's top-level code before any validation. The encode side only ever emits
+ # ``airflow.*`` AirflowException subclasses for AIRFLOW_EXC_SER and builtin
+ # KeyError/AttributeError for BASE_EXC_SER, so reject anything outside those up front,
+ # mirroring the import-path guards in the timetable/window/wait-policy decoders.
if type_ == DAT.AIRFLOW_EXC_SER:
+ if not exc_cls_name.startswith("airflow."):
Review Comment:
Not sure this is safe either. It's relatively trivial to have `airflow.evil` be a module.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-5083420712
The upgrade path regression needs to be fixed. Otherwise, this is a good code cleanup on its own if nothing else.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3523396518
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -662,6 +662,12 @@ def deserialize(cls, encoded_var: Any) -> Any:
exc_cls = import_string(exc_cls_name)
else:
exc_cls = import_string(f"builtins.{exc_cls_name}")
+ # ``exc_cls_name`` comes from the serialized payload. A tampered blob can name any
+ # importable callable (``os.system``, ``builtins.exec``, ...) which would then be
+ # invoked with attacker-supplied args. Only accept genuine exception types, matching
+ # the import-path guards the timetable/window/wait-policy decoders already apply.
+ if not (isinstance(exc_cls, type) and issubclass(exc_cls, BaseException)):
Review Comment:
Moot now. Pushed eef021eb, which drops the whole exception branch from BaseSerialization, so there is no import-then-validate ordering left to worry about and nothing legitimate serializes an exception through the general framework anymore.
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -658,14 +659,18 @@ def deserialize(cls, encoded_var: Any) -> Any:
args = deser["args"]
kwargs = deser["kwargs"]
del deser
+ # ``exc_cls_name`` comes from the serialized payload, so resolve it without handing an
+ # attacker-controlled string to ``import_string`` -- that would execute the named
+ # module's top-level code before any validation. The encode side only ever emits
+ # ``airflow.*`` AirflowException subclasses for AIRFLOW_EXC_SER and builtin
+ # KeyError/AttributeError for BASE_EXC_SER, so reject anything outside those up front,
+ # mirroring the import-path guards in the timetable/window/wait-policy decoders.
if type_ == DAT.AIRFLOW_EXC_SER:
+ if not exc_cls_name.startswith("airflow."):
Review Comment:
Gone in eef021eb. Rather than trying to make the prefix safe, I removed the encode and decode branches entirely, so there is no import_string call on a payload name to defend anymore.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-5430746716
gentle ping
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3967150088
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -631,6 +631,11 @@ def deserialize(cls, encoded_var: Any) -> Any:
return parse_timezone(var)
elif type_ == DAT.RELATIVEDELTA:
return decode_relativedelta(var)
+ elif type_ in (DAT.AIRFLOW_EXC_SER, DAT.BASE_EXC_SER):
+ # Legacy rows only. ``exc_cls_name`` is payload-supplied, so it is never resolved or
+ # called; the recorded args are rendered with BaseException semantics, giving the same
+ # string form an exception now serializes to.
+ return str(BaseException(*cls.deserialize(var).get("args", ())))
Review Comment:
Good catch. Switched it to Exception in 4ba235b2. It's only str()'d here rather than raised, so nothing actually escapes an except clause, but every exception that ever went through this path is an Exception subclass anyway, and the rendered string is byte-identical since __str__ comes from BaseException either way.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3652442842
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -582,7 +561,7 @@ def serialize(
elif isinstance(var, XComArg):
return cls._encode(serialize_xcom_arg(var), type_=DAT.XCOM_REF)
elif isinstance(var, LazySelectSequence):
- return cls.serialize(list(var))
+ return cls.serialize(list(var), strict=strict)
Review Comment:
It's the fall-through policy. Any value that matches no branch ends up in `default_serialization`, which with the default `strict=False` logs a debug line and casts to `str(var)`, and with `strict=True` raises `SerializationError("Encountered unexpected type")`. `test_strict_mode` is the illustration, it serializes `[[[Test()]]]` both ways.
The reason it has to be forwarded on every recursive `cls.serialize(...)` is that the unsupported object is usually nested inside a container, so if one branch drops the kwarg then the caller asks for strict and still silently gets a string back. That's the invariant `test_recursive_serialize_calls_must_forward_kwargs` exists to protect.
It shows up in this diff because that guard test wasn't actually covering the whole method. It AST-walks `serialize` and `break`s on the first `.serialize` call whose receiver isn't `cls`, which was the `var.serialize()` in the exception branch I removed. Everything later in walk order was never checked, and the `LazySelectSequence` branch was calling `cls.serialize(list(var))` with no `strict`, so a `LazySelectSequence` holding an unserializable element would get stringified even under `strict=True`. Switching the `break` to `continue` makes the walk cover the method, and the forward at that call site is what it then asks for.
Latent rather than user-visible though: nothing outside the tests passes `strict=True` today.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3437029022
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -662,6 +662,12 @@ def deserialize(cls, encoded_var: Any) -> Any:
exc_cls = import_string(exc_cls_name)
else:
exc_cls = import_string(f"builtins.{exc_cls_name}")
+ # ``exc_cls_name`` comes from the serialized payload. A tampered blob can name any
+ # importable callable (``os.system``, ``builtins.exec``, ...) which would then be
+ # invoked with attacker-supplied args. Only accept genuine exception types, matching
+ # the import-path guards the timetable/window/wait-policy decoders already apply.
+ if not (isinstance(exc_cls, type) and issubclass(exc_cls, BaseException)):
Review Comment:
Good points, both fair.
On the legit case: the encode side is narrow. `AIRFLOW_EXC_SER` only fires for `AirflowException` subclasses with a `serialize()` method (e.g. `AirflowRescheduleException` round-tripping through a serialized sensor reschedule), and the name is always the full `airflow.*` import path. `BASE_EXC_SER` only fires for `KeyError`/`AttributeError`, encoded as a bare builtin name. So nothing legitimate ever names a non-`airflow.` module or a non-exception.
On the after-import placement: you're right, validating post-import only really covers the builtins branch, and the `AIRFLOW_EXC_SER` branch still imports the module (running its top-level code) before the check. I moved the guard ahead of the import to match what `decode_window`/`decode_wait_policy` do: reject anything not under `airflow.` up front, and resolve the builtins branch via `getattr(builtins, name)` instead of `import_string`, so no attacker-named module is imported at all. The `issubclass(..., BaseException)` check stays as a backstop.
One tradeoff worth calling out: the `airflow.` prefix means a user-defined `AirflowException` subclass living outside the `airflow` namespace would no longer round-trip. That mirrors how the timetable/window/wait-policy decoders already treat non-core paths (core-only unless registered), so it seemed like the consistent line to draw, but happy to loosen it if you'd rather keep custom exceptions working.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-4756842424
Agreed, that's the right line to draw. I traced where these blobs actually flow first: the cross-boundary control-flow signals (reschedule, deferral, downstream-skip, awaiting-input) already travel as their own comms messages (RescheduleTask, DeferTask, SkipDownstreamTasks, ...) and get reconstructed explicitly in the task runner. So the AIRFLOW_EXC_SER/BASE_EXC_SER branch in BaseSerialization isn't what carries them across the worker/supervisor boundary. What it actually catches is exceptions that happen to be nested inside some other serialized value (XCom, trigger/next kwargs), which is exactly the general-case behaviour you want gone from a module that runs in so many processes.
Plan I'd go with:
- Drop both encode branches (the AirflowException-with-serialize one and the KeyError/AttributeError one) and the matching decode branch from BaseSerialization, so no process running general serialization can import-and-call an arbitrary name from a payload anymore.
- For anything that genuinely still needs a real exception instance back, add a small dedicated helper with a fixed name->class allowlist and reconstruct from that map only, no import_string, kept out of the general framework.
One thing I want to confirm before wiring it up: from the call-site sweep I don't see a runtime path that depends on getting an exception instance back out of general serialization once the comms messages are accounted for. So my inclination is to remove it outright rather than reintroduce even a narrow allowlist, and only add the allowlisted helper if there's a specific round-trip that needs it. Does that match what you had in mind, or is there a case you know of that I should keep behind the allowlist?
I'll split it so the framework removal and any targeted replacement land as separate commits.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-4750934236
On when this fires: it's a small set of control-flow exceptions that carry data across the worker/supervisor boundary, not arbitrary exceptions thrown for their own sake. AirflowRescheduleException round-trips the reschedule_date back to where the reschedule is recorded; TaskDeferred, TaskAwaitingInput, DownstreamTasksSkipped and XComNotFound serialize their payloads over the Execution API; and KeyError/AttributeError show up nested inside other serialized results. That's the BASE/AIRFLOW_EXC_SER path. So it's reconstructing data, not really needing to throw an arbitrary class from another process.
Stepping back though, I think your instinct is right that this isn't a vuln in the security-model sense. The blob only comes from the metadata DB or the Execution API, both behind auth, and anyone who can tamper with it has more direct execution paths already. So this is defense-in-depth, not a reported vulnerability.
The two tighter fixes don't hold up either: because the base AirflowException.serialize() is inherited, the encode side accepts any AirflowException subclass, so the import set is open-ended (providers and user-defined exceptions). That's why an allowlist or an airflow.* prefix can't work without breaking legitimate round-trips, as you noted.
I've dropped the prefix commit and left just the minimal post-import backstop that rejects a resolved object that isn't an exception type. If you'd rather treat the serialized store as trusted and close this, I'm fine with that too. Your call.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3652428334
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -582,7 +561,7 @@ def serialize(
elif isinstance(var, XComArg):
return cls._encode(serialize_xcom_arg(var), type_=DAT.XCOM_REF)
elif isinstance(var, LazySelectSequence):
- return cls.serialize(list(var))
+ return cls.serialize(list(var), strict=strict)
Review Comment:
`strict` only controls the fallback at the bottom of `serialize`: a value that matches no branch goes to `default_serialization`, which returns `str(var)` when strict is False (the default) and raises `SerializationError("Encountered unexpected type")` when it's True. So it's a fail-loud switch for callers who would rather get an error than silently end up with a stringified object. Nothing in tree passes `strict=True` outside `test_strict_mode`, though the kwarg is exposed on `BaseSerialization.serialize`.
The reason it turns up in this diff at all: `test_recursive_serialize_calls_must_forward_kwargs` walks the AST of `serialize` and asserts every `cls.serialize(...)` recursion forwards the kwonly args. It used to `break` on the first non-`cls` serialize call, and that was `var.serialize()` in the exception encode branch, the third call in walk order. So it only ever validated 2 of the 6 recursions and never reached the `LazySelectSequence` line, which had been dropping `strict` silently. Removing the exception branch removed the thing it was breaking on, so the walk now covers the whole method and flags it. I forwarded it and swapped the `break` for `continue` so the invariant actually holds.
No behaviour change for any current caller since strict defaults to False. With `strict=True`, a `LazySelectSequence` holding an unserializable element now raises instead of quietly stringifying it, which is the point of strict.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-4751194863
I think we should remove the ability to serialize/deserialize exceptions in the general case, and the specific exceptions that needs to cross the worker/supervisor boundary should be handled differently and with an explicit allow list only.
Crucially code in `serialized_objects.py` can run in the in any process that deals handles serialization (which is a lot) -- and this something we need or likely want in the general case.
Please can you work on removing serialization (and deser.) of exceptions via Airflow's general serialization framework in favour of something tightly scoped and targeted.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3435634025
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -662,6 +662,12 @@ def deserialize(cls, encoded_var: Any) -> Any:
exc_cls = import_string(exc_cls_name)
else:
exc_cls = import_string(f"builtins.{exc_cls_name}")
+ # ``exc_cls_name`` comes from the serialized payload. A tampered blob can name any
+ # importable callable (``os.system``, ``builtins.exec``, ...) which would then be
+ # invoked with attacker-supplied args. Only accept genuine exception types, matching
+ # the import-path guards the timetable/window/wait-policy decoders already apply.
+ if not (isinstance(exc_cls, type) and issubclass(exc_cls, BaseException)):
Review Comment:
This doesn't defend against all that much -- by validating after the import has happened, top level module code is executed.
Exactly which case under normal (non malicious) operation serializes an exception?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] validate exception type when deserializing serialized exceptions [airflow]
Posted by "naruto-lgtm (via GitHub)" <gi...@apache.org>.
naruto-lgtm commented on PR #68662:
URL: https://github.com/apache/airflow/pull/68662#issuecomment-4882650005
Pushed the removal in eef021eb.
I went with removing it outright rather than reintroducing a narrow allowlist. From the call-site sweep, once the cross-boundary control-flow signals are accounted for (they already travel as their own comms messages and get reconstructed explicitly in the task runner), nothing in the runtime depends on getting an exception instance back out of general serialization. So the AIRFLOW_EXC_SER/BASE_EXC_SER branches were only ever catching exceptions nested inside some other serialized value, which is exactly the general-case behaviour you wanted gone from a module that runs in so many processes.
What changed:
- Dropped both encode branches (the AirflowException-with-serialize one and the KeyError/AttributeError one) and the matching decode branch. No process running general serialization can import-and-call a payload name anymore; an exception now just serializes to its string form via the existing fallback.
- Removed the two now-dead DagAttributeTypes members.
- One thing the removal surfaced: the guard test (test_recursive_serialize_calls_must_forward_kwargs) was short-circuiting on the var.serialize() call in the exception branch, which had been masking a cls.serialize(list(var)) recursion for LazySelectSequence that dropped strict. I forwarded strict there so the invariant actually holds now.
I didn't add the allowlisted helper since I couldn't find a case that needs one. If you know of a round-trip I've missed, point me at it and I'll add the targeted map, but my read is nothing outside the comms path relies on it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org