From 1c2e4baecbea55b4333dd18294a119111365448f Mon Sep 17 00:00:00 2001
From: Kaxil Naik <kaxilnaik@gmail.com>
Date: Thu, 11 Mar 2021 01:27:57 +0000
Subject: [PATCH] Fix tests for all urllib versions with only '&' as separator
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Turns out #14698 did not fix the issue as Master failed again. After
digging a bit more I found that the CVE was fixed in all
Python versions: 3.6.13, 3.7.10 & 3.8.8

The solution in this PR/commit checks the `parse_qsl` behavior with
following tests:

```
❯ docker run -it python:3.8-slim bash
root@41120dfd035e:/# python
Python 3.8.8 (default, Feb 19 2021, 18:07:06)
>>> from urllib.parse import parse_qsl
>>> parse_qsl(";a=b")
[(';a', 'b')]
>>>

```
❯ docker run -it python:3.8.7-slim bash
root@68e527725610:/# python
Python 3.8.7 (default, Feb  9 2021, 08:21:15)
>>> from urllib.parse import parse_qsl
>>> parse_qsl(";a=b")
[('a', 'b')]
>>>
```
---
 tests/www/test_views.py | 88 ++++++++++-------------------------------
 1 file changed, 21 insertions(+), 67 deletions(-)

diff --git a/tests/www/test_views.py b/tests/www/test_views.py
index 517d4bc56f997..5ef5a2193f111 100644
--- a/tests/www/test_views.py
+++ b/tests/www/test_views.py
@@ -32,7 +32,7 @@
 from typing import Any, Dict, Generator, List, NamedTuple
 from unittest import mock
 from unittest.mock import PropertyMock
-from urllib.parse import quote_plus
+from urllib.parse import parse_qsl, quote_plus
 
 import jinja2
 import pytest
@@ -2772,33 +2772,6 @@ def test_trigger_dag_form(self):
         resp = self.client.get(f'trigger?dag_id={test_dag_id}')
         self.check_content_in_response(f'Trigger DAG: {test_dag_id}', resp)
 
-    @parameterized.expand(
-        [
-            ("javascript:alert(1)", "/home"),
-            ("http://google.com", "/home"),
-            (
-                "%2Ftree%3Fdag_id%3Dexample_bash_operator';alert(33)//",
-                "/tree?dag_id=example_bash_operator%27&amp;alert%2833%29%2F%2F=",
-            ),
-            ("%2Ftree%3Fdag_id%3Dexample_bash_operator", "/tree?dag_id=example_bash_operator"),
-            ("%2Fgraph%3Fdag_id%3Dexample_bash_operator", "/graph?dag_id=example_bash_operator"),
-        ]
-    )
-    @pytest.mark.skipif(
-        sys.version_info < (3, 8, 8),
-        reason='Vulnerability was fixed in Python 3.8.8 which changed the query string separator: bpo-42967',
-    )
-    def test_trigger_dag_form_origin_url_py_lte_387(self, test_origin, expected_origin):
-        test_dag_id = "example_bash_operator"
-
-        resp = self.client.get(f'trigger?dag_id={test_dag_id}&origin={test_origin}')
-        self.check_content_in_response(
-            '<button type="button" class="btn" onclick="location.href = \'{}\'; return false">'.format(
-                expected_origin
-            ),
-            resp,
-        )
-
     @parameterized.expand(
         [
             ("javascript:alert(1)", "/home"),
@@ -2811,13 +2784,16 @@ def test_trigger_dag_form_origin_url_py_lte_387(self, test_origin, expected_orig
             ("%2Fgraph%3Fdag_id%3Dexample_bash_operator", "/graph?dag_id=example_bash_operator"),
         ]
     )
-    @pytest.mark.skipif(
-        sys.version_info > (3, 8, 7),
-        reason='Vulnerability was fixed in Python 3.8.8 which changed the query string separator: bpo-42967',
-    )
-    def test_trigger_dag_form_origin_url_py_gt_387(self, test_origin, expected_origin):
+    def test_trigger_dag_form_origin_url(self, test_origin, expected_origin):
         test_dag_id = "example_bash_operator"
 
+        # https://github.com/python/cpython/pull/24297/files
+        # Check if tests are running with a Python version containing the above fix
+        # where ";" is removed as a separator
+        if parse_qsl(";a=b") != [(';a', 'b')]:
+            expected_url = expected_origin.replace("%3B", "&")
+            expected_url += "="
+
         resp = self.client.get(f'trigger?dag_id={test_dag_id}&origin={test_origin}')
         self.check_content_in_response(
             '<button type="button" class="btn" onclick="location.href = \'{}\'; return false">'.format(
@@ -3345,31 +3321,6 @@ def test_action_logging_post(self):
 
 
 class TestHelperFunctions(TestBase):
-    @parameterized.expand(
-        [
-            ("", "/home"),
-            ("http://google.com", "/home"),
-            (
-                "http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3Fdag_id%test_dag';alert(33)//",
-                "http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3F"
-                "dag_id%25test_dag%27&alert%2833%29%2F%2F=",
-            ),
-            (
-                "http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3Fdag_id%test_dag",
-                "http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3Fdag_id%25test_dag",
-            ),
-        ]
-    )
-    @pytest.mark.skipif(
-        sys.version_info < (3, 8, 8),
-        reason='Vulnerability was fixed in Python 3.8.8 which changed the query string separator: bpo-42967',
-    )
-    def test_get_safe_url_py_lte_387(self, test_url, expected_url):
-        with mock.patch("airflow.www.views.url_for") as mock_url_for:
-            mock_url_for.return_value = "/home"
-            with self.app.test_request_context(base_url="http://localhost:8080"):
-                assert get_safe_url(test_url) == expected_url
-
     @parameterized.expand(
         [
             ("", "/home"),
@@ -3385,15 +3336,18 @@ def test_get_safe_url_py_lte_387(self, test_url, expected_url):
             ),
         ]
     )
-    @pytest.mark.skipif(
-        sys.version_info > (3, 8, 7),
-        reason='Vulnerability was fixed in Python 3.8.8 which changed the query string separator: bpo-42967',
-    )
-    def test_get_safe_url_py_gt_387(self, test_url, expected_url):
-        with mock.patch("airflow.www.views.url_for") as mock_url_for:
-            mock_url_for.return_value = "/home"
-            with self.app.test_request_context(base_url="http://localhost:8080"):
-                assert get_safe_url(test_url) == expected_url
+    @mock.patch("airflow.www.views.url_for")
+    def test_get_safe_url(self, test_url, expected_url, mock_url_for):
+        # https://github.com/python/cpython/pull/24297/files
+        # Check if tests are running with a Python version containing the above fix
+        # where ";" is removed as a separator
+        if parse_qsl(";a=b") != [(';a', 'b')]:
+            expected_url = expected_url.replace("%3B", "&")
+            expected_url += "="
+
+        mock_url_for.return_value = "/home"
+        with self.app.test_request_context(base_url="http://localhost:8080"):
+            assert get_safe_url(test_url) == expected_url
 
     @parameterized.expand(
         [