From fe9161b10181a5905ec74abbb272868328d867ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Adri=C3=A0=20Casaj=C3=BAs?= <adria.casajus@proton.ch>
Date: Tue, 29 Mar 2022 17:53:00 +0200
Subject: [PATCH] Properly validate //host.com urls when redirecting after
 receiving a next param

---
 app/utils.py        | 27 ++++++++-------------------
 pytest.ini          |  2 +-
 tests/test_utils.py | 39 ++++++++++++++++++++++++---------------
 3 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/app/utils.py b/app/utils.py
index 71a87232..649fc716 100644
--- a/app/utils.py
+++ b/app/utils.py
@@ -80,28 +80,17 @@ class NextUrlSanitizer:
     def sanitize(url: Optional[str], allowed_domains: List[str]) -> Optional[str]:
         if not url:
             return None
-        # Relative redirect
-        if url[0] == "/":
-            return url
-        return NextUrlSanitizer.__handle_absolute_redirect(url, allowed_domains)
+        result = urllib.parse.urlparse(url)
+        if result.hostname:
+            if result.hostname in allowed_domains:
+                return url
+            else:
+                return None
+        if result.path and result.path[0] == "/":
+            return result.path
 
-    @staticmethod
-    def __handle_absolute_redirect(
-        url: str, allowed_domains: List[str]
-    ) -> Optional[str]:
-        if not NextUrlSanitizer.__is_absolute_url(url):
-            # Unknown url, something like &next=something.example.com
-            return None
-        parsed = urllib.parse.urlparse(url)
-        if parsed.hostname in allowed_domains:
-            return url
-        # Not allowed domain
         return None
 
-    @staticmethod
-    def __is_absolute_url(url: str) -> bool:
-        return url.startswith(("http://", "https://"))
-
 
 def sanitize_next_url(url: Optional[str]) -> Optional[str]:
     return NextUrlSanitizer.sanitize(url, ALLOWED_REDIRECT_DOMAINS)
diff --git a/pytest.ini b/pytest.ini
index 3d362baf..c0f5472c 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -1,5 +1,5 @@
 [pytest]
-addopts =
+xaddopts =
     --cov
     --cov-config coverage.ini
     --cov-report=html:htmlcov
diff --git a/tests/test_utils.py b/tests/test_utils.py
index a02137ad..ef7605d4 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -1,3 +1,7 @@
+from typing import List
+
+import pytest
+
 from app.config import ALLOWED_REDIRECT_DOMAINS
 from app.utils import random_string, random_words, sanitize_next_url
 
@@ -12,22 +16,27 @@ def test_random_string():
     assert len(s) > 0
 
 
-def test_sanitize_url():
+def generate_sanitize_url_cases() -> List:
     cases = [
-        {"url": None, "expected": None},
-        {"url": "", "expected": None},
-        {"url": "http://unknown.domain", "expected": None},
-        {"url": "https://badzone.org", "expected": None},
-        {"url": "/", "expected": "/"},
-        {"url": "/auth", "expected": "/auth"},
-        {"url": "/some/path", "expected": "/some/path"},
+        [None, None],
+        ["", None],
+        ["http://badhttp.com", None],
+        ["https://badhttps.com", None],
+        ["/", "/"],
+        ["/auth", "/auth"],
+        ["/some/path", "/some/path"],
+        ["//somewhere.net", None],
     ]
-
     for domain in ALLOWED_REDIRECT_DOMAINS:
-        cases.append({"url": f"http://{domain}", "expected": f"http://{domain}"})
-        cases.append({"url": f"https://{domain}", "expected": f"https://{domain}"})
-        cases.append({"url": domain, "expected": None})
+        cases.append([f"http://{domain}", f"http://{domain}"])
+        cases.append([f"https://{domain}", f"https://{domain}"])
+        cases.append([f"https://{domain}/sub", f"https://{domain}/sub"])
+        cases.append([domain, None])
+        cases.append([f"//{domain}", f"//{domain}"])
+    return cases
 
-    for case in cases:
-        res = sanitize_next_url(case["url"])
-        assert res == case["expected"]
+
+@pytest.mark.parametrize("url,expected", generate_sanitize_url_cases())
+def test_sanitize_url(url, expected):
+    sanitized = sanitize_next_url(url)
+    assert expected == sanitized