Repository navigation
Validate deferred trigger classpath resolves to a BaseTrigger subclass - #69792
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
There was a problem hiding this comment.
Pull request overview
Hardens the triggerer’s deferred-trigger loading path by validating that a deferred-task classpath resolves to a BaseTrigger subclass before caching/instantiation, preventing arbitrary importable callables from being invoked in the triggerer process.
Changes:
- Add a
BaseTrigger-subclass validation (and clearer docstring) inTriggerRunner.get_trigger_by_classpath. - Add a unit test covering both an allowed trigger class and a rejected non-trigger callable.
- Add a release-note newsfragment describing the security hardening (but the fragment filename needs correction).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| airflow-core/src/airflow/jobs/triggerer_job_runner.py | Validates imported objects are BaseTrigger subclasses before caching/instantiation. |
| airflow-core/tests/unit/jobs/test_triggerer_job.py | Adds regression test for accepting a real trigger and rejecting a non-trigger callable classpath. |
| airflow-core/newsfragments/99999.bugfix.rst | Adds release note entry for the hardening (currently named with a placeholder). |
The triggerer resolves a trigger from the ``classpath`` carried in the deferred-task Execution API payload via ``import_string(classpath)`` and then instantiates it with ``trigger_class(**kwargs)``. ``get_trigger_by_classpath`` did not check that the imported object is actually a ``BaseTrigger`` subclass, so any importable callable (e.g. ``subprocess.check_output``) could be loaded and invoked in the long-running triggerer process. Harden this by rejecting, before caching and before instantiation, any ``classpath`` that does not resolve to a ``type`` that is a ``BaseTrigger`` subclass. Legitimate triggers are unaffected (they are always ``BaseTrigger`` subclasses); an invalid classpath now fails the trigger cleanly instead of being instantiated. Adds a unit test covering both the accepted and rejected cases.
7794470 to
672b5f1
Compare
potiuk
left a comment
There was a problem hiding this comment.
Hmm... I think that's not really preventing the side effects it was supposed to prevent.
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
apache#69792) The triggerer resolves a trigger from the ``classpath`` carried in the deferred-task Execution API payload via ``import_string(classpath)`` and then instantiates it with ``trigger_class(**kwargs)``. ``get_trigger_by_classpath`` did not check that the imported object is actually a ``BaseTrigger`` subclass, so any importable callable (e.g. ``subprocess.check_output``) could be loaded and invoked in the long-running triggerer process. Harden this by rejecting, before caching and before instantiation, any ``classpath`` that does not resolve to a ``type`` that is a ``BaseTrigger`` subclass. Legitimate triggers are unaffected (they are always ``BaseTrigger`` subclasses); an invalid classpath now fails the trigger cleanly instead of being instantiated. Adds a unit test covering both the accepted and rejected cases.
The triggerer resolves a trigger from the
classpathcarried in the deferred-task Execution API payload viaimport_string(classpath)and then instantiates it withtrigger_class(**kwargs).get_trigger_by_classpathdid not check that the imported object is actually aBaseTriggersubclass, so any importable callable (e.g.subprocess.check_output) could be loaded and invoked in the long-running triggerer process.Harden this by rejecting, before caching and before instantiation, any
classpaththat does not resolve to atypethat is aBaseTriggersubclass. Legitimate triggers are unaffected (they are alwaysBaseTriggersubclasses); an invalid classpath now fails the trigger cleanly instead of being instantiated. Adds a unit test covering both the accepted and rejected cases.Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.