ashb commented on a change in pull request #12638:
URL: https://github.com/apache/airflow/pull/12638#discussion_r530821843
##########
File path: airflow/operators/bash.py
##########
@@ -19,16 +19,22 @@
import os
import signal
+import warnings
from subprocess import PIPE, STDOUT, Popen
from tempfile import TemporaryDirectory, gettempdir
-from typing import Dict, Optional
+from typing import Dict, List, Optional, Union
from airflow.exceptions import AirflowException
from airflow.models import BaseOperator
from airflow.utils.decorators import apply_defaults
from airflow.utils.operator_helpers import context_to_airflow_vars
+def _is_bash_script(input_string: str):
+ input_list = input_string.split(" ")
Review comment:
Maybe use shlex module?
##########
File path: airflow/operators/bash.py
##########
@@ -157,14 +163,37 @@ def pre_exec():
self.log.info('Running command: %s', self.bash_command)
- self.sub_process = Popen( # pylint:
disable=subprocess-popen-preexec-fn
- ['bash', "-c", self.bash_command],
- stdout=PIPE,
- stderr=STDOUT,
- cwd=tmp_dir,
- env=env,
- preexec_fn=pre_exec,
- )
+ if isinstance(self.bash_command) == str and not
_is_bash_script(self.bash_command):
Review comment:
```suggestion
if isinstance(self.bash_command, str) and
_is_bash_script(self.bash_command):
```
##########
File path: airflow/operators/bash.py
##########
@@ -157,14 +163,37 @@ def pre_exec():
self.log.info('Running command: %s', self.bash_command)
- self.sub_process = Popen( # pylint:
disable=subprocess-popen-preexec-fn
- ['bash', "-c", self.bash_command],
- stdout=PIPE,
- stderr=STDOUT,
- cwd=tmp_dir,
- env=env,
- preexec_fn=pre_exec,
- )
+ if isinstance(self.bash_command) == str and not
_is_bash_script(self.bash_command):
+ warnings.warn(
+ "Warning: Using a string in the BashOperator leaves your
system open to bash injection "
+ "attacks via escape strings. Please use a list[str]
instead."
+ )
+ self.sub_process = Popen( # pylint:
disable=subprocess-popen-preexec-fn
+ ['bash', "-c", self.bash_command],
+ stdout=PIPE,
+ stderr=STDOUT,
+ cwd=tmp_dir,
+ env=env,
+ preexec_fn=pre_exec,
+ )
+ elif isinstance(self.bash_command) == list:
+ if (
+ len(self.bash_command) >= 2
+ and self.bash_command[0] == "bash"
+ and self.bash_command[1] == "-c"
+ ):
+ warnings.warn(
+ "Warning: using \"bash -c\" for your bash command will
give this command"
+ "full access to the bash environment. Please consider
not doing this."
Review comment:
I don't understand this warning.
##########
File path: airflow/operators/bash.py
##########
@@ -157,14 +163,37 @@ def pre_exec():
self.log.info('Running command: %s', self.bash_command)
- self.sub_process = Popen( # pylint:
disable=subprocess-popen-preexec-fn
- ['bash', "-c", self.bash_command],
- stdout=PIPE,
- stderr=STDOUT,
- cwd=tmp_dir,
- env=env,
- preexec_fn=pre_exec,
- )
+ if isinstance(self.bash_command) == str and not
_is_bash_script(self.bash_command):
+ warnings.warn(
+ "Warning: Using a string in the BashOperator leaves your
system open to bash injection "
+ "attacks via escape strings. Please use a list[str]
instead."
+ )
+ self.sub_process = Popen( # pylint:
disable=subprocess-popen-preexec-fn
+ ['bash', "-c", self.bash_command],
+ stdout=PIPE,
+ stderr=STDOUT,
+ cwd=tmp_dir,
+ env=env,
+ preexec_fn=pre_exec,
+ )
+ elif isinstance(self.bash_command) == list:
+ if (
+ len(self.bash_command) >= 2
+ and self.bash_command[0] == "bash"
+ and self.bash_command[1] == "-c"
+ ):
Review comment:
Can't do multi line suggest on mobile app but
```bash_command[0:2] == ["bash", "-c"]```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
