Webserver: Further Sanitize values passed to origin param (apache#12459)

kaxil · kaxil · commit 4c27334a586b · 2020-11-24T19:24:05.000Z
Follow-up of apache#10334 (cherry picked from commit 9818932)
diff --git a/airflow/www/views.py b/airflow/www/views.py
@@ -55,7 +55,7 @@
 from pygments import highlight, lexers
 import six
 from pygments.formatters.html import HtmlFormatter
-from six.moves.urllib.parse import quote, unquote, urlparse
+from six.moves.urllib.parse import parse_qsl, quote, unquote, urlencode, urlparse
 
 from sqlalchemy import or_, desc, and_, union_all
 from wtforms import (
@@ -335,7 +335,13 @@ def get_safe_url(url):
         valid_schemes = ['http', 'https', '']
         valid_netlocs = [request.host, '']
 
+        if not url:
+            return "/admin/"
+
         parsed = urlparse(url)
+
+        query = parse_qsl(parsed.query, keep_blank_values=True)
+        url = parsed._replace(query=urlencode(query)).geturl()
         if parsed.scheme in valid_schemes and parsed.netloc in valid_netlocs:
             return url
     except Exception as e:  # pylint: disable=broad-except
diff --git a/airflow/www_rbac/views.py b/airflow/www_rbac/views.py
@@ -32,7 +32,7 @@
 
 import pkg_resources
 import six
-from six.moves.urllib.parse import quote, urlparse
+from six.moves.urllib.parse import parse_qsl, quote, urlencode, urlparse
 
 import pendulum
 import sqlalchemy as sqla
@@ -96,7 +96,13 @@ def get_safe_url(url):
         valid_schemes = ['http', 'https', '']
         valid_netlocs = [request.host, '']
 
+        if not url:
+            return url_for('Airflow.index')
+
         parsed = urlparse(url)
+
+        query = parse_qsl(parsed.query, keep_blank_values=True)
+        url = parsed._replace(query=urlencode(query)).geturl()
         if parsed.scheme in valid_schemes and parsed.netloc in valid_netlocs:
             return url
     except Exception as e:  # pylint: disable=broad-except
diff --git a/tests/www/test_views.py b/tests/www/test_views.py
@@ -1119,6 +1119,11 @@ def test_trigger_serialized_dag(self, mock_os_isfile, mock_dagrun):
     @parameterized.expand([
         ("javascript:alert(1)", "/admin/"),
         ("http://google.com", "/admin/"),
+        (
+            "%2Fadmin%2Fairflow%2Ftree%3Fdag_id%3Dexample_bash_operator"
+            "&dag_id=example_bash_operator';alert(33)//",
+            "/admin/airflow/tree?dag_id=example_bash_operator"
+        ),
         (
             "%2Fadmin%2Fairflow%2Ftree%3Fdag_id%3Dexample_bash_operator&dag_id=example_bash_operator",
             "/admin/airflow/tree?dag_id=example_bash_operator"
@@ -1127,7 +1132,6 @@ def test_trigger_serialized_dag(self, mock_os_isfile, mock_dagrun):
             "%2Fadmin%2Fairflow%2Fgraph%3Fdag_id%3Dexample_bash_operator&dag_id=example_bash_operator",
             "/admin/airflow/graph?dag_id=example_bash_operator"
         ),
-        ("", ""),
     ])
     def test_trigger_dag_form_origin_url(self, test_origin, expected_origin):
         test_dag_id = "example_bash_operator"
diff --git a/tests/www_rbac/test_views.py b/tests/www_rbac/test_views.py
@@ -2247,9 +2247,12 @@ def test_trigger_serialized_dag(self, mock_os_isfile, mock_dagrun):
     @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"),
-        ("", ""),
     ])
     def test_trigger_dag_form_origin_url(self, test_origin, expected_origin):
         test_dag_id = "example_bash_operator"
