potiuk commented on code in PR #69792:
URL: https://github.com/apache/airflow/pull/69792#discussion_r3568970940
##########
airflow-core/src/airflow/jobs/triggerer_job_runner.py:
##########
@@ -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)):
Review Comment:
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.
--
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]
