Webserver: Sanitize string passed to origin param (apache#14738)

kaxil · kaxil · commit 5a4270697538 · 2021-03-16T20:10:32.000Z
Follow-up of apache#12459 & apache#10334 Since https://github.com/python/cpython/pull/24297/files (bpo-42967) also removed ';' as query argument separator, we remove query arguments with semicolons. (cherry picked from commit 409c249)
diff --git a/airflow/www/views.py b/airflow/www/views.py
@@ -121,8 +121,18 @@ def get_safe_url(url):
 
     parsed = urlparse(url)
 
+    # If the url is relative & it contains semicolon, redirect it to homepage to avoid
+    # potential XSS. (Similar to https://github.com/python/cpython/pull/24297/files (bpo-42967))
+    if parsed.netloc == '' and parsed.scheme == '' and ';' in unquote(url):
+        return url_for('Airflow.index')
+
     query = parse_qsl(parsed.query, keep_blank_values=True)
-    url = parsed._replace(query=urlencode(query)).geturl()
+
+    # Remove all the query elements containing semicolon
+    # As part of https://github.com/python/cpython/pull/24297/files (bpo-42967)
+    # semicolon was already removed as a separator for query arguments by default
+    sanitized_query = [query_arg for query_arg in query if ';' not in query_arg[1]]
+    url = parsed._replace(query=urlencode(sanitized_query)).geturl()
 
     if parsed.scheme in valid_schemes and parsed.netloc in valid_netlocs:
         return url
diff --git a/tests/www/test_views.py b/tests/www/test_views.py
@@ -2749,9 +2749,10 @@ def test_trigger_dag_form(self):
         [
             ("javascript:alert(1)", "/home"),
             ("http://google.com", "/home"),
+            ("36539'%3balert(1)%2f%2f166", "/home"),
             (
                 "%2Ftree%3Fdag_id%3Dexample_bash_operator';alert(33)//",
-                "/tree?dag_id=example_bash_operator%27&amp;alert%2833%29%2F%2F=",
+                "/home",
             ),
             ("%2Ftree%3Fdag_id%3Dexample_bash_operator", "/tree?dag_id=example_bash_operator"),
             ("%2Fgraph%3Fdag_id%3Dexample_bash_operator", "/graph?dag_id=example_bash_operator"),
@@ -3289,10 +3290,14 @@ class TestHelperFunctions(TestBase):
         [
             ("", "/home"),
             ("http://google.com", "/home"),
+            ("36539'%3balert(1)%2f%2f166", "/home"),
+            (
+                "http://localhost:8080/trigger?dag_id=test&origin=36539%27%3balert(1)%2f%2f166&abc=2",
+                "http://localhost:8080/trigger?dag_id=test&abc=2",
+            ),
             (
                 "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",
             ),
             (
                 "http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3Fdag_id%test_dag",

Original file line number	Diff line number	Diff line change
`@@ -2749,9 +2749,10 @@ def test_trigger_dag_form(self):`
`2749`	`2749`	`[`
`2750`	`2750`	`("javascript:alert(1)", "/home"),`
`2751`	`2751`	`("http://google.com", "/home"),`
	`2752`	`+ ("36539'%3balert(1)%2f%2f166", "/home"),`
`2752`	`2753`	`(`
`2753`	`2754`	`"%2Ftree%3Fdag_id%3Dexample_bash_operator';alert(33)//",`
`2754`		`- "/tree?dag_id=example_bash_operator%27&alert%2833%29%2F%2F=",`
	`2755`	`+ "/home",`
`2755`	`2756`	`),`
`2756`	`2757`	`("%2Ftree%3Fdag_id%3Dexample_bash_operator", "/tree?dag_id=example_bash_operator"),`
`2757`	`2758`	`("%2Fgraph%3Fdag_id%3Dexample_bash_operator", "/graph?dag_id=example_bash_operator"),`
`@@ -3289,10 +3290,14 @@ class TestHelperFunctions(TestBase):`
`3289`	`3290`	`[`
`3290`	`3291`	`("", "/home"),`
`3291`	`3292`	`("http://google.com", "/home"),`
	`3293`	`+ ("36539'%3balert(1)%2f%2f166", "/home"),`
	`3294`	`+ (`
	`3295`	`+ "http://localhost:8080/trigger?dag_id=test&origin=36539%27%3balert(1)%2f%2f166&abc=2",`
	`3296`	`+ "http://localhost:8080/trigger?dag_id=test&abc=2",`
	`3297`	`+ ),`
`3292`	`3298`	`(`
`3293`	`3299`	`"http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3Fdag_id%test_dag';alert(33)//",`
`3294`		`- "http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3F"`
`3295`		`- "dag_id%25test_dag%27&alert%2833%29%2F%2F=",`
	`3300`	`+ "http://localhost:8080/trigger?dag_id=test_dag",`
`3296`	`3301`	`),`
`3297`	`3302`	`(`
`3298`	`3303`	`"http://localhost:8080/trigger?dag_id=test_dag&origin=%2Ftree%3Fdag_id%test_dag",`