You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by "divyanshus2404 (via GitHub)" <gi...@apache.org> on 2026/08/17 11:21:04 UTC

[PR] fix(spark): replace ReDoS-vulnerable regex in _mask_cmd with linear-time token parser [airflow]

divyanshus2404 opened a new pull request, #71711:
URL: https://github.com/apache/airflow/pull/71711

   ## Summary
   Replace the backtracking-prone regex in `SparkSubmitHook._mask_cmd()` with a linear-time token-based parser that eliminates the ReDoS vulnerability.
   
   ## Problem
   The original regex used nested lazy quantifiers (`\S*?`) with a lookahead `(?:(?!\2\s).)*` that causes **quadratic backtracking** on large inputs without `secret`/`password` keywords:
   
   | Input size | Original time | New time |
   |-----------|--------------|----------|
   | 10,000 chars | ~2s | <1ms |
   | 50,000 chars | ~57s | <1ms |
   | 100,000 chars | ~4min | ~1ms |
   
   An authenticated user with DAG trigger permissions could exploit this by passing large `application_args` via the REST API, blocking worker slots.
   
   ## Solution
   Replace the single regex with a token-based approach:
   1. Split the command string by whitespace into tokens
   2. Identify tokens containing `secret` or `password` (case-insensitive)
   3. Mask the associated value (handling `key=value`, `key='quoted value'`, and `key value` forms)
   4. Single forward pass — guaranteed O(n)
   
   ## Test plan
   - [x] All 8 existing parametrized `test_masks_passwords` cases pass with identical output
   - [x] `test_submit_log_tail_formats_and_masks_captured_lines` passes
   - [x] ReDoS payload (50k chars) completes in <1ms vs 57s before
   - [ ] CI passes
   
   Closes #70676


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5559865056

   Thanks for the review! Both comments addressed in the latest push.
   
   Fixed the CI failure — MyPy providers was the only failing check. `_mask_cmd` is typed `str | list[str]` and the two performance tests passed a tuple; they're lists now. Also merged `main` to bring the branch up to date.
   
   Locally: 143/143 spark hook tests pass, mypy and ruff clean.
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Replace regex in `_mask_cmd` with linear-time token parser [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5412482859

   Simplified the implementation in ca82cacc.
   
   The tokeniser turned out to be unnecessary: the quadratic behaviour came from the leading `\S*?` retrying at every offset in the string, so anchoring the match at a token boundary with `(?<!\S)` is enough to make it linear. The value is then matched with explicit single-quoted / double-quoted / bare alternatives instead of the backreference-driven lookahead, with a quote closing the value only when whitespace or end-of-string follows it — which is what preserves masking of values that themselves contain quotes.
   
   That brings the change down to 21 lines and keeps it a single pattern, which should be easier to review and maintain than the hand-written scan.
   
   Timings on a single long token with no sensitive keyword (the worst case for the old pattern):
   
   | Input size | Original | This PR |
   |-----------|----------|---------|
   | 10,000 chars | 0.8s | <1ms |
   | 50,000 chars | 18.7s | 1.1ms |
   
   Checked against the previous regex over 200k generated command lines — including tabs, repeated spaces, newlines, and embedded quotes — with no output differences. Spark provider suite: 271 passed, 2 skipped.
   
   ---
   Drafted-by: Claude Code (no human review before posting)
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(spark): replace ReDoS-vulnerable regex in _mask_cmd with linear-time token parser [airflow]

Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.

boring-cyborg[bot] commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5315365392

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](https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example Dag that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

--
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Replace regex in `_mask_cmd` with linear-time token parser [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5412169950

   Added unit tests covering each form the parser handles — `key=value`, dotted and uppercase keys, space-separated values, multi-token quoted values, and a sensitive flag with no following value — plus a large-input test guarding against a regression to the original quadratic behaviour.
   
   While writing them I ran the new parser differentially against the old regex and found two masking gaps in my first implementation:
   
   | input | before this PR | first implementation |
   |---|---|---|
   | `password  hunter2` (repeated spaces) | `password  ******` | `password ****** hunter2` |
   | `--password<TAB>hunter2` | `--password<TAB>******` | not masked |
   
   Splitting on a single literal space meant a run of spaces produced an empty field that was consumed as the value, and a tab never split at all. Both leak through `_submit_log_tail`, which masks raw spark-submit output where column-aligned spacing is common.
   
   Fixed in 2fa05d9 by splitting on runs of whitespace and keeping the separators, with regression tests for both shapes. This also preserves the original spacing instead of normalising it. Full spark provider suite passes (271 passed, 2 skipped).
   
   ---
   Drafted-by: Claude Code (Opus 5) (no human review before posting)
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5438321792

You're right, and thanks for the concrete payload — I've dropped the linear-time claim.

Reproduced it: with `"secret" * n` as a single token the timing clearly quadruples as the input doubles, so it is still O(n²).

| `"secret" * n` | this PR |
|---|---|
| 6,019 chars | 39ms |
| 12,019 chars | 149ms |
| 24,019 chars | 598ms |
| 48,019 chars | 2,424ms |

The anchor removes the retry-at-every-offset factor, but a token packing many keyword occurrences still forces the inner `\S*?` to rescan from each one, so the worst case remains quadratic — just with a much smaller constant. On that same payload the previous pattern was 32.8ms at 319 chars and 5,511ms at 2,419 chars, so it degrades far more steeply, but "mitigated" is the accurate word rather than "linear".

Updated accordingly:
- Title no longer says linear — now "Reduce `_mask_cmd` backtracking on large inputs".
- The code comment states plainly that this is not strictly O(n) and why.
- `test_masks_passwords_is_linear_on_large_input` renamed to `test_masks_passwords_stays_fast_on_large_input`.
- Added `test_masks_passwords_stays_fast_on_repeated_keywords` using your input shape, sized so it stays quick while still catching a regression to the old pattern.

If you'd prefer a genuinely linear implementation I'm happy to do it — splitting on whitespace and testing each token for the keyword before matching is O(n) with no pathological case, at the cost of more code than a single pattern. Let me know which trade-off you'd rather have here.

---
Drafted-by: Claude Code; reviewed by @divyanshus2404 before posting

--
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5565261439

   Status update, in case this one has slipped down the queue.
   
   The unit tests requested in review are in, the pattern has been reworked after review feedback (quoted values no longer span newlines, and the complexity claim has been corrected to match what it actually does), and CI is green. A second reviewer has approved.
   
   Happy to make further changes if anything else is wanted here.
   
   ---
   Drafted-by: Claude Code; reviewed by @divyanshus2404 before posting
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "potiuk (via GitHub)" <gi...@apache.org>.

potiuk commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r4076988119


##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -516,26 +567,96 @@ def _get_spark_binary_path(self) -> list[str]:
     def _mask_cmd(self, connection_cmd: str | list[str]) -> str:
         # Mask any password related fields in application args with key value pair
         # where key contains password (case insensitive), e.g. HivePassword='abc'
-        connection_cmd_masked = re.sub(
-            r"("
-            r"\S*?"  # Match all non-whitespace characters before...
-            r"(?:secret|password)"  # ...literally a "secret" or "password"
-            # word (not capturing them).
-            r"\S*?"  # All non-whitespace characters before either...
-            r"(?:=|\s+)"  # ...an equal sign or whitespace characters
-            # (not capturing them).
-            r"(['\"]?)"  # An optional single or double quote.
-            r")"  # This is the end of the first capturing group.
-            r"(?:(?!\2\s).)*"  # All characters between optional quotes
-            # (matched above); if the value is quoted,
-            # it may contain whitespace.
-            r"(\2)",  # Optional matching quote.
-            r"\1******\3",
-            " ".join(connection_cmd),
-            flags=re.I,
-        )
-
-        return connection_cmd_masked
+        #
+        # Tokenise on whitespace so each token is examined once.  For key=value
+        # tokens the unanchored _SENSITIVE_KV_RE handles multiple sensitive keys
+        # inside a single token (e.g. ``Config(secret="x",password=y)``).
+        # Space-separated key/value pairs and quoted values that span multiple
+        # tokens are handled by manual lookahead.  The overall scan is O(n).
+        if isinstance(connection_cmd, str):
+            connection_cmd = [connection_cmd]
+        cmd = " ".join(connection_cmd)
+        # re.split with a capturing group keeps the delimiters in the result list,
+        # so we can reassemble the string without altering whitespace.
+        parts = re.split(r"(\s+)", cmd)
+        i = 0
+        while i < len(parts):
+            part = parts[i]
+            if part and not part.isspace() and _SENSITIVE_KEYWORD_RE.search(part):
+                if "=" in part:
+                    # key=value in the same token.
+                    open_info = _find_open_quote_key(part)
+                    if open_info is not None:
+                        # The value's opening quote is not closed within this token,
+                        # so the quoted value spans multiple whitespace-delimited
+                        # tokens.  Consume them until the closing quote, but stop
+                        # at newlines so an unterminated quote doesn't swallow
+                        # subsequent log lines.
+                        key_start, eq_idx, quote = open_info
+                        before_key = part[:key_start]
+                        key_eq = part[key_start : eq_idx + 1]
+
+                        # Mask any earlier key=value pairs in the prefix.
+                        if before_key and _SENSITIVE_KEYWORD_RE.search(before_key):
+                            before_key = _SENSITIVE_KV_RE.sub(_mask_sensitive_kv, before_key)
+
+                        found_close = False
+                        j = i + 1
+                        while j < len(parts):
+                            if parts[j].isspace() and "\n" in parts[j]:
+                                break
+                            if not parts[j].isspace() and parts[j].endswith(quote):

Review Comment:
   **Blocking:** `endswith(quote)` only recognises a closing quote that is the last character of the token, so `"x",`, `'x')` and `'x'}` never close the value. The loop then runs to the newline and falls back to masking only the first token, leaving the rest of the secret in the clear:
   
   ```
   IN  : Config(password="my pass word", user=x)
   main: Config(password="******", user=x)
   PR  : Config(password=****** pass word", user=x)
   
   IN  : {'password': 'a b'}
   main: {'password': '******'}
   PR  : {'password': ****** b'}
   ```
   
   The second case goes through the space-separated branch at line 645, which has the same check. Please pin both payloads in `test_masks_passwords`.
   



##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,57 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Quick check for whether a token might contain a sensitive key.
+_SENSITIVE_KEYWORD_RE = re.compile(r"secret|password", re.IGNORECASE)
+
+# Per-token pattern for key=value forms where the value is fully contained in
+# the token.  Intentionally *unanchored* so that it finds multiple sensitive
+# keys inside a single whitespace-delimited token (e.g.
+# ``Config(secret="x",password=y)``).  Running it only on tokens that pass
+# the cheap ``_SENSITIVE_KEYWORD_RE`` check keeps the overall scan O(n).
+_SENSITIVE_KV_RE = re.compile(
+    r"(\S*?(?:secret|password)\S*?=)"

Review Comment:
   **Major:** because this is unanchored and `\S*?` can cross `=`, a token with an `=` ahead of many keywords is still super-linear:
   
   ```
   "a=" + "secret" * n     n=100: 33 ms   n=200: 244 ms   n=400: 1.9 s   n=800: 15 s
   ```
   
   `main` is 4.5 s at n=400 on the same payload, so this is a constant-factor gain here, not the O(n) that the comments at lines 65 and 575 claim. Keeping `=` out of the key (e.g. `[^\s=]*?(?:secret|password)[^\s=]*?=`) keeps each attempt bounded. A timing test on this payload would stop it regressing.
   



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "aaron-y-chen (via GitHub)" <gi...@apache.org>.

aaron-y-chen commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r3943647558


##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,29 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Values to mask are anchored at a token boundary: without the lookbehind the leading
+# \S*? retries at every offset in the string, which is what made masking pathologically
+# slow on long arguments and log lines. Anchoring does not make this strictly O(n) -- a
+# token packing many "secret"/"password" occurrences still backtracks quadratically --
+# but it removes the retry-per-offset factor and is orders of magnitude faster in
+# practice. A quote only closes the value when whitespace or the end of the string
+# follows it, so quoted values may themselves contain quotes.
+_SENSITIVE_VALUE_RE = re.compile(
+    r"(?<!\S)(\S*?(?:secret|password)\S*?(?:=|\s+))"
+    r"(?:'((?:[^']|'(?!\s|$))*)'|\"((?:[^\"]|\"(?!\s|$))*)\"|(\S*))",

Review Comment:
   I think the regex pattern should be modified as follows:
   ```suggestion
       r"(?:'((?:[^'\n]|'(?!\s|$))*)'|\"((?:[^\"\n]|\"(?!\s|$))*)\"|(\S*))"
   ```
   
   Otherwise, a multiline application argument can cause unrelated command-log content to be swallowed:
   ```
   in : 'spark-submit --conf password="abc\nERROR: job failed\n--other=1 "tail'
   old: 'spark-submit --conf password=******\nERROR: job failed\n--other=1 "tail'
   new: 'spark-submit --conf password="******"tail'
   ```



##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1315,6 +1356,35 @@ def test_masks_passwords(self, command: str, expected: str) -> None:
         # Then
         assert command_masked == expected
 
+    @pytest.mark.db_test
+    def test_masks_passwords_stays_fast_on_large_input(self) -> None:
+        # The previous pattern retried at every offset on long inputs, taking tens of
+        # seconds for this payload and blocking the worker slot.
+        hook = SparkSubmitHook()
+        payload = ("spark-submit", "--arg", "x " * 25_000)

Review Comment:
   Why leave trailing whitespace? Should it be `"x" * 25_000`?



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r3944235400


##########
providers/apache/spark/tests/unit/apache/spark/hooks/test_spark_submit.py:
##########
@@ -1315,6 +1356,35 @@ def test_masks_passwords(self, command: str, expected: str) -> None:
         # Then
         assert command_masked == expected
 
+    @pytest.mark.db_test
+    def test_masks_passwords_stays_fast_on_large_input(self) -> None:
+        # The previous pattern retried at every offset on long inputs, taking tens of
+        # seconds for this payload and blocking the worker slot.
+        hook = SparkSubmitHook()
+        payload = ("spark-submit", "--arg", "x " * 25_000)

Review Comment:
   The trailing space is deliberate. `"x " * 25_000` produces 25,000 separate tokens, and it's the number of token boundaries that exercises the per-offset retry the old pattern did — `"x" * 25_000` is one long token, which is a much weaker case for this regression. I've added a comment on the test saying so.
   
   There's a separate test right below (`test_masks_passwords_stays_fast_on_repeated_keywords`) covering the single-long-token shape.



##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,29 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Values to mask are anchored at a token boundary: without the lookbehind the leading
+# \S*? retries at every offset in the string, which is what made masking pathologically
+# slow on long arguments and log lines. Anchoring does not make this strictly O(n) -- a
+# token packing many "secret"/"password" occurrences still backtracks quadratically --
+# but it removes the retry-per-offset factor and is orders of magnitude faster in
+# practice. A quote only closes the value when whitespace or the end of the string
+# follows it, so quoted values may themselves contain quotes.
+_SENSITIVE_VALUE_RE = re.compile(
+    r"(?<!\S)(\S*?(?:secret|password)\S*?(?:=|\s+))"
+    r"(?:'((?:[^']|'(?!\s|$))*)'|\"((?:[^\"]|\"(?!\s|$))*)\"|(\S*))",

Review Comment:
   Good catch, applied. Added `test_masks_passwords_does_not_swallow_following_lines` using your exact example as a regression test.
   
   One note: the `old:` / `new:` labels in your example look swapped. I reproduced it before changing anything — `password="******"tail` (the swallow) is what the *current* pattern produces, and your `\n` exclusion is what yields the correct `password=******\nERROR: job failed\n--other=1 "tail`. Same conclusion, just flagging it so nobody reads it as a regression.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(spark): replace ReDoS-vulnerable regex in _mask_cmd with linear-time token parser [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5315621214

   For any query:
   https://www.linkedin.com/in/divyanshu-singh-2404s/


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Mask `_mask_cmd` secrets in linear time [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r4097419297


##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -516,26 +567,96 @@ def _get_spark_binary_path(self) -> list[str]:
     def _mask_cmd(self, connection_cmd: str | list[str]) -> str:
         # Mask any password related fields in application args with key value pair
         # where key contains password (case insensitive), e.g. HivePassword='abc'
-        connection_cmd_masked = re.sub(
-            r"("
-            r"\S*?"  # Match all non-whitespace characters before...
-            r"(?:secret|password)"  # ...literally a "secret" or "password"
-            # word (not capturing them).
-            r"\S*?"  # All non-whitespace characters before either...
-            r"(?:=|\s+)"  # ...an equal sign or whitespace characters
-            # (not capturing them).
-            r"(['\"]?)"  # An optional single or double quote.
-            r")"  # This is the end of the first capturing group.
-            r"(?:(?!\2\s).)*"  # All characters between optional quotes
-            # (matched above); if the value is quoted,
-            # it may contain whitespace.
-            r"(\2)",  # Optional matching quote.
-            r"\1******\3",
-            " ".join(connection_cmd),
-            flags=re.I,
-        )
-
-        return connection_cmd_masked
+        #
+        # Tokenise on whitespace so each token is examined once.  For key=value
+        # tokens the unanchored _SENSITIVE_KV_RE handles multiple sensitive keys
+        # inside a single token (e.g. ``Config(secret="x",password=y)``).
+        # Space-separated key/value pairs and quoted values that span multiple
+        # tokens are handled by manual lookahead.  The overall scan is O(n).
+        if isinstance(connection_cmd, str):
+            connection_cmd = [connection_cmd]
+        cmd = " ".join(connection_cmd)
+        # re.split with a capturing group keeps the delimiters in the result list,
+        # so we can reassemble the string without altering whitespace.
+        parts = re.split(r"(\s+)", cmd)
+        i = 0
+        while i < len(parts):
+            part = parts[i]
+            if part and not part.isspace() and _SENSITIVE_KEYWORD_RE.search(part):
+                if "=" in part:
+                    # key=value in the same token.
+                    open_info = _find_open_quote_key(part)
+                    if open_info is not None:
+                        # The value's opening quote is not closed within this token,
+                        # so the quoted value spans multiple whitespace-delimited
+                        # tokens.  Consume them until the closing quote, but stop
+                        # at newlines so an unterminated quote doesn't swallow
+                        # subsequent log lines.
+                        key_start, eq_idx, quote = open_info
+                        before_key = part[:key_start]
+                        key_eq = part[key_start : eq_idx + 1]
+
+                        # Mask any earlier key=value pairs in the prefix.
+                        if before_key and _SENSITIVE_KEYWORD_RE.search(before_key):
+                            before_key = _SENSITIVE_KV_RE.sub(_mask_sensitive_kv, before_key)
+
+                        found_close = False
+                        j = i + 1
+                        while j < len(parts):
+                            if parts[j].isspace() and "\n" in parts[j]:
+                                break
+                            if not parts[j].isspace() and parts[j].endswith(quote):

Review Comment:
   Fixed in f66e620b: the token heuristics (including `endswith(quote)`) are replaced by a scanner that closes quoted values exactly as `main` does; both payloads are pinned in `test_masks_passwords`.
   
   ---
   Drafted-by: Claude Code (Opus 5.5) (no human review before posting)
   
   _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_



##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,57 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Quick check for whether a token might contain a sensitive key.
+_SENSITIVE_KEYWORD_RE = re.compile(r"secret|password", re.IGNORECASE)
+
+# Per-token pattern for key=value forms where the value is fully contained in
+# the token.  Intentionally *unanchored* so that it finds multiple sensitive
+# keys inside a single whitespace-delimited token (e.g.
+# ``Config(secret="x",password=y)``).  Running it only on tokens that pass
+# the cheap ``_SENSITIVE_KEYWORD_RE`` check keeps the overall scan O(n).
+_SENSITIVE_KV_RE = re.compile(
+    r"(\S*?(?:secret|password)\S*?=)"

Review Comment:
   Fixed in f66e620b: `_SENSITIVE_KV_RE` is gone and masking is a single linear pass (`"a=" + "secret" * n` is 1.3 ms at n=80k), the O(n) comments now hold, and a timing test on this payload is added.
   
   ---
   Drafted-by: Claude Code (Opus 5.5) (no human review before posting)
   
   _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5819486756

   Thanks for the thorough re-review. Both points are addressed in f66e620b.
   
   Rather than patching the `endswith(quote)` check and `_SENSITIVE_KV_RE` separately, I replaced the token-based heuristics with a single-pass scanner, `_mask_sensitive_values()`, that computes exactly what the original regex on `main` matches. Both problems came from the heuristics only approximating that pattern, so matching it directly closes the whole class rather than the two shapes found so far.
   
   **Blocking — quoted multi-word values**
   
   A quoted value now closes the same way as on `main`: at the last matching quote before a quote-followed-by-whitespace or a newline. Both payloads are pinned in `test_masks_passwords`:
   
   ```
   IN : Config(password="my pass word", user=x)
   OUT: Config(password="******", user=x)
   
   IN : {'password': 'a b'}
   OUT: {'password': '******'}
   ```
   
   Last round's two multi-key payloads still mask correctly. Both new cases fail against the previous head.
   
   **Major — super-linear `_SENSITIVE_KV_RE`**
   
   There is no backtracking regex left in the masking path, and the incorrect O(n) comments are gone. On the payload you gave:
   
   | `"a=" + "secret" * n` | this PR |
   |---|---|
   | n=20,000 | 0.3 ms |
   | n=40,000 | 0.7 ms |
   | n=80,000 | 1.3 ms |
   
   Other adversarial shapes (`"secret" * n`, `"x " * n`, alternating and unterminated quotes, `secret= ` repeated) also scale linearly. `test_masks_passwords_stays_fast_on_repeated_keywords` is now parametrized over `"secret" * 20_000` and `"a=" + "secret" * 20_000`; on the previous head the second one runs for minutes.
   
   **Differential check**
   
   I re-ran the comparison against the `main` regex with a generator that now emits closed-quote-then-second-key tokens, quotes followed by punctuation (`",`, `')`, `'}`), tabs, CRLF, newlines, `\xa0`, ` ` and `ſ`. Over 1.4M generated inputs, from short lines up to about 60 fragments, the output was byte-identical to `main` in every case.
   
   **Local checks**
   
   - Spark provider tests: 282 passed, 2 skipped (system tests)
   - ruff, mypy and the pre-commit prek hooks: clean. I skipped the breeze-based `check-provider-yaml-valid` because breeze isn't installed locally; `provider.yaml` is untouched.
   
   ---
   Drafted-by: Claude Code (Opus 5.5); reviewed by @divyanshus2404 before posting
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Mask `_mask_cmd` secrets in linear time [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5977474417

   Thanks — you were right, 7233363 wasn't needed. Reverted in 6f2239c3, so `_QUOTED_VALUE_LIMIT_RE` is back to `(?=\s)` and the scanner matches f66e620b again.
   
   Both shapes are now pinned:
   - `--password='Pa'$$w0rd'` is in `test_masks_passwords`.
   - `"password='x'," * 20_000` is in the `test_masks_passwords_stays_fast_on_repeated_keywords` parametrization.
   
   Both new cases fail on 7233363. The `'Pa'$$w0rd'` one leaks `$$w0rd'`, and the repeated-quote one takes 7.1 s against the 5 s limit.
   
   On the new head:
   - Your two payloads and last round's two all mask the same as `main`.
   - 200k random inputs, including doubled quotes and `$`, differ from `main` in 0 cases.
   - `"password='x'," * n` takes 1.0 / 2.0 / 4.1 ms at n = 10k / 20k / 40k.
   - Spark provider suite: 284 passed, 2 skipped.
   
   ---
   Drafted-by: Claude Code (Opus 5.5); reviewed by @divyanshus2404 before posting
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Mask `_mask_cmd` secrets in linear time [airflow]

Posted by "potiuk (via GitHub)" <gi...@apache.org>.

potiuk merged PR #71711:
URL: https://github.com/apache/airflow/pull/71711


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r3946653825


##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,26 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Values to mask are anchored at a token boundary so the scan stays linear: without

Review Comment:
   Agreed — dropped the linear-time claim.
   
   Reproduced your payload: with `"secret" * n` as a single token the timing quadruples as the input doubles (39ms → 149ms → 598ms → 2,424ms across 6k → 48k chars), so the worst case is still O(n²). The anchor removes the retry-at-every-offset factor, but a token packing many keyword occurrences still makes the inner `\S*?` rescan from each one.
   
   Changed in 5d58a97:
   - Title is now "Reduce `_mask_cmd` backtracking on large inputs"
   - The comment above the pattern states it is not strictly O(n), and why
   - `test_masks_passwords_is_linear_on_large_input` renamed to `..._stays_fast_on_large_input`
   - Added `test_masks_passwords_stays_fast_on_repeated_keywords` using your input shape
   
   Also corrected the PR description, which still described the pattern as scanning linearly.
   
   ---
   Drafted-by: Claude Code; reviewed by @divyanshus2404 before posting



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "shahar1 (via GitHub)" <gi...@apache.org>.

shahar1 commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r4054096488


##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,30 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Values to mask are anchored at a token boundary: without the lookbehind the leading
+# \S*? retries at every offset in the string, which is what made masking pathologically
+# slow on long arguments and log lines. Anchoring does not make this strictly O(n) -- a
+# token packing many "secret"/"password" occurrences still backtracks quadratically --
+# but it removes the retry-per-offset factor and is orders of magnitude faster in
+# practice. A quote only closes the value when whitespace or the end of the string
+# follows it, so quoted values may themselves contain quotes, but never a newline:
+# an unterminated quote would otherwise swallow the log lines that follow it.
+_SENSITIVE_VALUE_RE = re.compile(

Review Comment:
   There's a regression:
   
   IN : 'Config(secret="x",password=hunter2)'
   OLD: 'Config(secret="",password='
   NEW: 'Config(secret="******",password=hunter2)' ← leaks
   
   The masking regex only starts looking for secrets at whitespace boundaries. In a log snippet like:
   
   ```
   Config(secret="x",password=hunter2)
   ```
   
   it masks secret, but after finishing the quoted "x" it is still inside the same non-whitespace token (Config(...)). Because it cannot restart scanning there, it never notices the later password= field - so hunter2 is exposed.
   
   
   ---
   
   Drafted by Codex, verified by me



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Replace regex in `_mask_cmd` with linear-time token parser [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5412528890

Thanks for the review — tests added, and the implementation is simpler than the version you first looked at.

**Tests**

`test_masks_passwords` now covers every form the pattern handles: `key=value`, dotted and uppercase keys, space-separated values, values quoted across multiple tokens, values separated by tabs or repeated spaces, and a sensitive flag with no following value. `test_masks_passwords_is_linear_on_large_input` guards the backtracking this PR removes.

**Implementation**

The tokeniser I originally wrote turned out to be unnecessary. The quadratic behaviour came from the leading `\S*?` retrying at every offset in the string, so anchoring the match at a token boundary with `(?<!\S)` is enough to make the scan linear. The value is matched with explicit single-quoted / double-quoted / bare alternatives instead of the backreference-driven lookahead; a quote closes the value only when whitespace or end-of-string follows, which preserves masking of values that contain quotes.

That keeps this a single pattern at 21 changed lines rather than a hand-written scan.

**Verification**

Timings on a single long token with no sensitive keyword — the worst case for the old pattern:

| Input size | Original | This PR |
|-----------|----------|---------|
| 10,000 chars | 0.8s | <1ms |
| 50,000 chars | 18.7s | 1.1ms |

Checked against the previous regex over 200k generated command lines — tabs, repeated spaces, newlines, embedded and unterminated quotes — with no output differences, so masking behaviour is unchanged. Every pre-existing `test_masks_passwords` case passes untouched. Spark provider suite: 271 passed, 2 skipped.

Happy to adjust if you'd prefer a different shape for any of this.

---
Drafted-by: Claude Code (no human review before posting)

--
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Reduce `_mask_cmd` backtracking on large inputs [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5773827188

   Thanks for the detailed review @potiuk! I've implemented all your feedback in the latest commit:
   
   1. **Tokenised approach**: Rewrote `_mask_cmd` to split on whitespace, run a cheap keyword pre-check, and then run an unanchored regex inside the token. It handles multiple secrets per token and safely detects multi-token quoted strings.
   2. **New tests added**: Added the failing `Config(secret="x",password=hunter2)` and `--conf spark.a.secret='x',spark.b.password=hunter2` payloads to `test_masks_passwords` (they now pass).
   3. **Nitpicks addressed**: Fixed the type annotation (`re.Match[str]`) and added an `isinstance` guard to fix the string-mangling bug in `_mask_cmd`.
   
   Let me know if there's anything else needed!


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Replace regex in `_mask_cmd` with linear-time token parser [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5412220942

   Thanks — tests added in 0dc1ddf.
   
   `test_masks_passwords` now covers each form the parser handles: `key=value`, dotted and uppercase keys, space-separated values, values quoted across multiple tokens, and a sensitive flag with no following value. `test_masks_passwords_is_linear_on_large_input` guards the quadratic behaviour this PR removes.
   
   2fa05d9 hardens the split to handle tab- and multi-space-separated values, which `_submit_log_tail` encounters in column-aligned spark-submit output; regression cases for both are included. Splitting on whitespace runs and preserving the separators also keeps the original spacing intact rather than normalising it.
   
   Verified against the previous regex over 60k generated command lines with no output differences, so masking behaviour is unchanged. Full spark provider suite: 271 passed, 2 skipped.
   
   ---
   Drafted-by: Claude Code (no human review before posting)
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Replace regex in `_mask_cmd` with linear-time token parser [airflow]

Posted by "divyanshus2404 (via GitHub)" <gi...@apache.org>.

divyanshus2404 commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5412601016

Thanks for the review — tests added, and the implementation is simpler than the version you first looked at.

**Tests**

**Implementation**

That keeps this a single pattern at 21 changed lines rather than a hand-written scan.

**Verification**

Timings on a single long token with no sensitive keyword — the worst case for the old pattern:

| Input size | Original | This PR |
|-----------|----------|---------|
| 10,000 chars | 0.8s | <1ms |
| 50,000 chars | 18.7s | 1.1ms |

Happy to adjust if you'd prefer a different shape for any of this.

---
Drafted-by: Claude Code; reviewed by @divyanshus2404 before posting

--
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Make `_mask_cmd` linear on large inputs [airflow]

Posted by "aaron-y-chen (via GitHub)" <gi...@apache.org>.

aaron-y-chen commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r3871195165


##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,26 @@
 
 _K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
 
+# Values to mask are anchored at a token boundary so the scan stays linear: without

Review Comment:
   It seems the current regex is not strictly linear. For example, a payload containing many repeated "secret" substrings can still show roughly O(n^2) behavior:
   
   ```python
   payload = ("spark-submit", "--arg", "secret" * 10000)
   ```
   
   I think the current regex already greatly mitigates the original issue, so perhaps it would be better to soften the linear-time claim in the comments/title rather than claim strict O(n) complexity.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] `SparkSubmitHook`: Mask `_mask_cmd` secrets in linear time [airflow]

Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.

boring-cyborg[bot] commented on PR #71711:
URL: https://github.com/apache/airflow/pull/71711#issuecomment-5982207912

   Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org