amoghrajesh commented on code in PR #70716:
URL: https://github.com/apache/airflow/pull/70716#discussion_r3681535335
##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1315,6 +1315,42 @@ def test_masks_passwords(self, command: str, expected:
str) -> None:
# Then
assert command_masked == expected
+ @pytest.mark.parametrize(
+ "payload",
+ [
+ # No "secret"/"password" trigger word at all: the historical PoC
for
+ # GHSA / issue #70676 (quadratic blowup while scanning for the
+ # trigger word).
+ "a" * 50_000 + "!",
+ # Trigger word present, but never followed by "=" or whitespace:
+ # this bypassed a naive "skip if trigger word absent" fast path.
+ "a" * 25_000 + "password" + "a" * 25_000,
+ # Trigger word present with an opening quote that's never closed.
+ 'password="' + "a" * 50_000,
+ ],
+ ids=["no_trigger_word", "trigger_word_no_delimiter",
"unterminated_quote"],
+ )
+ @pytest.mark.db_test
+ def test_mask_cmd_does_not_hang_on_adversarial_input(self, payload: str)
-> None:
+ """Regression test for ReDoS in _mask_cmd (issue #70676).
Review Comment:
Check for coding standards for writing tests in rest of the file, we do not
mention the issue this is testing regression instead.
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -513,25 +513,46 @@ def _get_spark_binary_path(self) -> list[str]:
# Assume that spark-submit is present in the path to the executing user
return [self._connection["spark_binary"]]
+ # Bounds used by ``_mask_cmd`` below to keep the masking regex O(n)
instead of
+ # O(n^2)/catastrophic. They are generous enough for any realistic CLI flag
+ # name or secret value while capping the amount of backtracking the regex
+ # engine can ever do on adversarial input (see GHSA / issue #70676).
Review Comment:
Since #70676 is not a security issue, we shouldnt mention "catastrophic" or
even keep it this descriptive. Please shorten the prose.
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -513,25 +513,46 @@ def _get_spark_binary_path(self) -> list[str]:
# Assume that spark-submit is present in the path to the executing user
return [self._connection["spark_binary"]]
+ # Bounds used by ``_mask_cmd`` below to keep the masking regex O(n)
instead of
+ # O(n^2)/catastrophic. They are generous enough for any realistic CLI flag
+ # name or secret value while capping the amount of backtracking the regex
+ # engine can ever do on adversarial input (see GHSA / issue #70676).
+ _MASK_CMD_KEY_BOUND = 100
+ _MASK_CMD_VALUE_BOUND = 1024
+
+
Review Comment:
```suggestion
```
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -513,25 +513,46 @@ def _get_spark_binary_path(self) -> list[str]:
# Assume that spark-submit is present in the path to the executing user
return [self._connection["spark_binary"]]
+ # Bounds used by ``_mask_cmd`` below to keep the masking regex O(n)
instead of
+ # O(n^2)/catastrophic. They are generous enough for any realistic CLI flag
+ # name or secret value while capping the amount of backtracking the regex
+ # engine can ever do on adversarial input (see GHSA / issue #70676).
+ _MASK_CMD_KEY_BOUND = 100
+ _MASK_CMD_VALUE_BOUND = 1024
+
+
Review Comment:
This will fail the prek hook.
##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1315,6 +1315,42 @@ def test_masks_passwords(self, command: str, expected:
str) -> None:
# Then
assert command_masked == expected
+ @pytest.mark.parametrize(
+ "payload",
+ [
+ # No "secret"/"password" trigger word at all: the historical PoC
for
+ # GHSA / issue #70676 (quadratic blowup while scanning for the
+ # trigger word).
+ "a" * 50_000 + "!",
+ # Trigger word present, but never followed by "=" or whitespace:
+ # this bypassed a naive "skip if trigger word absent" fast path.
+ "a" * 25_000 + "password" + "a" * 25_000,
+ # Trigger word present with an opening quote that's never closed.
+ 'password="' + "a" * 50_000,
+ ],
+ ids=["no_trigger_word", "trigger_word_no_delimiter",
"unterminated_quote"],
+ )
+ @pytest.mark.db_test
+ def test_mask_cmd_does_not_hang_on_adversarial_input(self, payload: str)
-> None:
+ """Regression test for ReDoS in _mask_cmd (issue #70676).
+
+ Previously-vulnerable, unbounded quantifiers in the masking regex made
+ this take *minutes* on crafted, user-controlled ``application_args``
+ (e.g. via the DAG trigger ``conf``). It must now complete quickly.
+ """
+ import time
Review Comment:
Top level import pls.
##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1315,6 +1315,42 @@ def test_masks_passwords(self, command: str, expected:
str) -> None:
# Then
assert command_masked == expected
+ @pytest.mark.parametrize(
+ "payload",
+ [
+ # No "secret"/"password" trigger word at all: the historical PoC
for
+ # GHSA / issue #70676 (quadratic blowup while scanning for the
+ # trigger word).
+ "a" * 50_000 + "!",
+ # Trigger word present, but never followed by "=" or whitespace:
+ # this bypassed a naive "skip if trigger word absent" fast path.
+ "a" * 25_000 + "password" + "a" * 25_000,
+ # Trigger word present with an opening quote that's never closed.
+ 'password="' + "a" * 50_000,
+ ],
+ ids=["no_trigger_word", "trigger_word_no_delimiter",
"unterminated_quote"],
+ )
+ @pytest.mark.db_test
+ def test_mask_cmd_does_not_hang_on_adversarial_input(self, payload: str)
-> None:
+ """Regression test for ReDoS in _mask_cmd (issue #70676).
+
+ Previously-vulnerable, unbounded quantifiers in the masking regex made
+ this take *minutes* on crafted, user-controlled ``application_args``
+ (e.g. via the DAG trigger ``conf``). It must now complete quickly.
+ """
+ import time
+ hook = SparkSubmitHook()
+
+ start = time.monotonic()
+ hook._mask_cmd((payload,))
+ elapsed = time.monotonic() - start
+
+ # Generous bound: correct, linear-time behavior finishes in well under
+ # a second; the original vulnerable regex took ~57s for a 50k char
+ # payload, growing quadratically with input size.
Review Comment:
```suggestion
```
--
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]
