Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions airflow-core/newsfragments/69792.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Reject deferred-task trigger classpaths that do not resolve to a ``BaseTrigger`` subclass before the class is instantiated in the triggerer, so a deferred task cannot cause an arbitrary importable callable to be invoked in the triggerer process.
Comment thread
hypnguyen1209 marked this conversation as resolved.
16 changes: 15 additions & 1 deletion airflow-core/src/airflow/jobs/triggerer_job_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -1696,8 +1696,22 @@ def get_trigger_by_classpath(self, classpath: str) -> type[BaseTrigger]:
"""
Get a trigger class by its classpath ("path.to.module.classname").

The resolved object must be a :class:`~airflow.triggers.base.BaseTrigger`
subclass. This is validated before the class is cached and, crucially,
before it is ever instantiated in ``create_triggers`` -- ``classpath``
originates from the (attacker-influenceable) deferred-task payload, so
without this check an arbitrary importable callable could be invoked in
the triggerer process.

Uses a cache dictionary to speed up lookups after the first time.
"""
if classpath not in self.trigger_cache:
self.trigger_cache[classpath] = import_string(classpath)
trigger_class = import_string(classpath)
if not (isinstance(trigger_class, type) and issubclass(trigger_class, BaseTrigger)):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be pre-import check, unfortunately - by the time we get here the class is already imported, and the whole idea is to avoid even importing it - because just importing it can have side-effects. I am not sure if that one is even reasonably doable - because in order to get class hierarchy you need to import it. Just AST parsing will not solve it.

So I am not sure if that is solving such defense-in-depth is even doable.

raise TypeError(
f"The trigger classpath {classpath!r} does not resolve to a "
f"{BaseTrigger.__module__}.{BaseTrigger.__qualname__} subclass; "
f"refusing to load it."
)
self.trigger_cache[classpath] = trigger_class
return self.trigger_cache[classpath]
20 changes: 20 additions & 0 deletions airflow-core/tests/unit/jobs/test_triggerer_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,26 @@ def test_shared_stream_cohort_grace_period_config_wiring(self) -> None:
trigger_runner = TriggerRunner()
assert trigger_runner._shared_streams._cohort_grace_period == 3.0

def test_get_trigger_by_classpath_requires_basetrigger_subclass(self) -> None:
"""
``classpath`` comes from the (attacker-influenceable) deferred-task payload, so
``get_trigger_by_classpath`` must refuse anything that is not a ``BaseTrigger``
subclass before it is cached and instantiated -- otherwise an arbitrary importable
callable (e.g. ``subprocess.check_output``) could be invoked in the triggerer.
"""
trigger_runner = TriggerRunner()

# A real BaseTrigger subclass resolves and is cached.
assert (
trigger_runner.get_trigger_by_classpath("airflow.triggers.testing.SuccessTrigger")
is SuccessTrigger
)

# An arbitrary importable callable is rejected and never cached.
with pytest.raises(TypeError, match="does not resolve to a"):
trigger_runner.get_trigger_by_classpath("subprocess.check_output")
assert "subprocess.check_output" not in trigger_runner.trigger_cache

@pytest.mark.asyncio
async def test_block_watchdog_does_not_log_when_threshold_is_not_exceeded(self) -> None:
with conf_vars({("triggerer", "blocked_main_thread_warning_threshold"): "0.5"}):
Expand Down