This is an automated email from the ASF dual-hosted git repository. kaxilnaik pushed a commit to branch v2-0-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 44b65e734f71632fa1158faeb78f4893e6a05f5a Author: Kaxil Naik <kaxiln...@gmail.com> AuthorDate: Thu Mar 11 01:39:28 2021 +0000 Fix tests for all urllib versions with only '&' as separator (#14710) 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')] >>> ``` (cherry picked from commit 7bd9d477dd7c59b8efb7183050de58bcfd6fdd43) --- 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 0d50144..ce4478c 100644 --- a/tests/www/test_views.py +++ b/tests/www/test_views.py @@ -32,7 +32,7 @@ from datetime import datetime as dt, timedelta 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 @@ -2757,46 +2757,22 @@ class TestTriggerDag(TestBase): ("http://google.com", "/home"), ( "%2Ftree%3Fdag_id%3Dexample_bash_operator';alert(33)//", - "/tree?dag_id=example_bash_operator%27&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"), - ("http://google.com", "/home"), - ( - "%2Ftree%3Fdag_id%3Dexample_bash_operator';alert(33)//", "/tree?dag_id=example_bash_operator%27%3Balert%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, 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( @@ -3329,31 +3305,6 @@ class TestHelperFunctions(TestBase): ( "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"), - ("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%3Balert%2833%29%2F%2F", ), ( @@ -3362,15 +3313,18 @@ class TestHelperFunctions(TestBase): ), ] ) - @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( [