(superset) 18/23: reject a next containing stripped control characters

[ma...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

madhushreeag pushed a commit to branch madhu-one-time-login-tokens
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 2afdbf1ab06a4fadfeec61c82a3a223c2abf2264
Author: madhushree agarwal <ma...@apple.com>
AuthorDate: Tue Oct 6 10:53:04 2026 -0700

    reject a next containing stripped control characters
---
 superset/security/login_token.py                   | 27 +++++++++++++++++-----
 .../security/login_token_api_tests.py              |  8 +++++++
 tests/unit_tests/security/login_token_test.py      | 11 ++++++---
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/superset/security/login_token.py b/superset/security/login_token.py
index 8a8bf0dda65..e83dbb06c84 100644
--- a/superset/security/login_token.py
+++ b/superset/security/login_token.py
@@ -88,8 +88,8 @@ LoginTokenIdentityResolver = Callable[..., LoginTokenUserInfo | None]
 
 # Characters a WHATWG URL parser strips before resolving, plus their
 # percent-encoded forms, which some browsers also remove when following a
-# Location header. Normalizing them first stops `/\tx` or `/%09x` smuggling a
-# different target past the checks below.
+# Location header. Their presence is grounds for rejection, not something to
+# normalize away -- see :func:`is_safe_next_path`.
 _URL_STRIPPED_CONTROL_CHARS = re.compile(r"[\t\n\r]|%09|%0[ADad]")
 
 
@@ -107,18 +107,33 @@ def is_safe_next_path(url: str) -> bool:
     Requiring a relative path avoids the question entirely: the parent
     application always knows the path it wants, the browser resolves it against
     Superset's own origin, and there is no host to compare.
+
+    A value carrying a character a URL parser strips is **rejected outright**
+    rather than stripped and accepted. Accepting it would mean the value this
+    returns ``True`` for is not the value the caller redirects to, and that gap
+    is exploitable in both directions: a ``CR``/``LF`` survives into
+    ``redirect()``, which Werkzeug refuses with a ``ValueError`` -- a 500 raised
+    after the token has already been burned -- while anything else differing
+    from its normalized form is a request to send the browser somewhere other
+    than what was asked for. Rejecting keeps the checked value and the
+    redirected value identical.
     """
     if not url or not url.strip():
         return False
 
-    normalized = _URL_STRIPPED_CONTROL_CHARS.sub("", url.strip())
+    candidate = url.strip()
+    if _URL_STRIPPED_CONTROL_CHARS.search(candidate):
+        return False
+
     # Browsers treat backslashes as forward slashes in special schemes, so
-    # `/\evil.com` would resolve as the protocol-relative `//evil.com`.
-    normalized = normalized.replace("\\", "/")
+    # `/\evil.com` would resolve as the protocol-relative `//evil.com`. Folded
+    # only to decide, never to rewrite: a value needing the fold is rejected by
+    # the check below rather than redirected to in its folded form.
+    folded = candidate.replace("\\", "/")
 
     # A single leading slash, and nothing that could be read as a host or a
     # scheme. `//host`, `https://host` and `mailto:x` are all rejected.
-    return normalized.startswith("/") and not normalized.startswith("//")
+    return folded.startswith("/") and not folded.startswith("//")
 
 
 def is_enabled() -> bool:
diff --git a/tests/integration_tests/security/login_token_api_tests.py b/tests/integration_tests/security/login_token_api_tests.py
index a7e3ce4f671..8cfe5981857 100644
--- a/tests/integration_tests/security/login_token_api_tests.py
+++ b/tests/integration_tests/security/login_token_api_tests.py
@@ -425,6 +425,14 @@ class TestLoginTokenApi(SupersetTestCase):
             "//evil.example.com/",
             "/\\evil.example.com/",
             "javascript:alert(1)",
+            # Encoded CR/LF. These must reject rather than normalize: a
+            # surviving newline reaches `redirect()`, which Werkzeug refuses
+            # with a ValueError, and @safe turns that into a 500 -- after the
+            # token has been burned and the session cookie set, so the parent
+            # application cannot retry.
+            "%2Fdashboard%2F1%2F%0D%0AX-Injected:+yes",
+            "%2F%0Adashboard%2F",
+            "%2F%09dashboard",
         ):
             with self.subTest(next=requested_next):
                 token = self._mint()
diff --git a/tests/unit_tests/security/login_token_test.py b/tests/unit_tests/security/login_token_test.py
index 707e63ce22b..300a192a37c 100644
--- a/tests/unit_tests/security/login_token_test.py
+++ b/tests/unit_tests/security/login_token_test.py
@@ -347,9 +347,6 @@ def test_consume_burn_survives_a_later_rollback(
         "/sqllab/",
         "/dashboard/list/?pageIndex=0",
         "/dashboard/1/#anchor",
-        # Stripped control characters leave a plain relative path, which is
-        # what a browser resolves this to.
-        "/\tdashboard",
     ],
 )
 def test_is_safe_next_path_accepts_relative_paths(path: str) -> None:
@@ -371,8 +368,16 @@ def test_is_safe_next_path_accepts_relative_paths(path: str) -> None:
         "/\\evil.example.com/",
         "\\\\evil.example.com/",
         # Control characters a URL parser strips, raw and percent-encoded.
+        # Rejected outright rather than stripped: the value checked must be the
+        # value redirected to. A surviving CR/LF reaches `redirect()`, which
+        # Werkzeug refuses with a ValueError -- a 500 after the token is burned.
         "/\t/evil.example.com",
         "/%09/evil.example.com",
+        "/\tdashboard",
+        "/dashboard/1/\r\nX-Injected: yes",
+        "/\ndashboard/",
+        "/%0Adashboard/",
+        "/%0D%0Adashboard/",
         # Schemes that never start with a slash.
         "javascript:alert(1)",
         "data:text/html,<script>alert(1)</script>",