[INFRA-779] fix(security): reject authority-relative next_path redirects

apps/api/plane/tests/unit/utils/test_path_validator.py

@@ -0,0 +1,92 @@
+# Copyright (c) 2023-present Plane Software, Inc. and contributors
+# SPDX-License-Identifier: AGPL-3.0-only
+# See the LICENSE file for details.
+
+"""Regression test for authority-relative open-redirect via next_path.
+
+Root cause: urlparse("///example.com/") returns both scheme and netloc
+empty (a quirk of Python's URL parser for exactly-three-or-more leading
+slashes), so validate_next_path's "extract only the path component" branch
+(gated on scheme or netloc being truthy) never fires, and the original,
+unmodified "///example.com/" string passes every remaining check unchanged.
+Browsers still resolve a leading "//" as authority-relative against an
+http(s) base, so the accepted value silently navigates off-domain.
+
+Fixed by rejecting any next_path starting with "//" outright, after the
+existing "must start with /" check.
+"""
+
+import pytest
+
+from plane.utils.path_validator import validate_next_path
+
+pytestmark = pytest.mark.unit
+
+
+class TestValidateNextPathAuthorityRelative:
+    @pytest.mark.parametrize(
+        # Exactly three or more leading slashes: urlparse() returns both
+        # scheme and netloc empty for these (the actual bug — verified
+        # directly against Python's urlparse before writing this fix), so
+        # the existing "extract only the path component" branch never fires
+        # and the raw, still-dangerous string must be caught by the new
+        # explicit "//" check instead.
+        "malicious_next_path",
+        [
+            "///example.com/",
+            "////example.com/",
+            "/////example.com/",
+        ],
+    )
+    def test_rejects_authority_relative_paths_urlparse_misses(self, malicious_next_path):
+        assert validate_next_path(malicious_next_path) == "", (
+            f"{malicious_next_path!r} must be rejected — a browser resolves a leading '//' "
+            "as authority-relative and navigates off-domain regardless of what urlparse() made of it"
+        )
+
+    def test_exactly_two_slashes_was_already_safely_downgraded(self):
+        """Positive control: urlparse() DOES detect a netloc for exactly two
+        leading slashes, so the pre-existing branch already strips this down
+        to a harmless same-origin path — this case never needed the new
+        check and must keep working exactly as before."""
+        assert validate_next_path("//example.com/") == "/"
+
+    @pytest.mark.parametrize(
+        "safe_next_path",
+        [
+            "/workspace/abc",
+            "/",
+            "/projects/123/issues",
+        ],
+    )
+    def test_accepts_genuine_relative_paths(self, safe_next_path):
+        assert validate_next_path(safe_next_path) == safe_next_path
+
+    def test_still_downgrades_absolute_urls_with_a_scheme_to_a_safe_path(self):
+        """Positive control: the pre-existing scheme/netloc branch already
+        strips the host from a fully-qualified URL, leaving only a harmless
+        same-origin path — this fix must not change that behavior."""
+        assert validate_next_path("https://evil.com/phish") == "/phish"
+        assert validate_next_path("http://evil.com/phish") == "/phish"
+
+    @pytest.mark.parametrize(
+        # A tab between each slash defeats both urlparse()'s own netloc
+        # detection (verified directly: "/\t/\t/evil.com" -> scheme='',
+        # netloc='') AND a literal next_path.startswith("//") check, since
+        # the second character is a tab, not a slash. Per the WHATWG URL
+        # spec, browsers strip every ASCII tab/CR/LF from a URL before
+        # parsing it, so what the browser actually navigates on is
+        # "///evil.com" — authority-relative, off-origin — even though this
+        # function never sees a literal "//" prefix.
+        "obfuscated_next_path",
+        [
+            "/\t/\t/evil.com",
+            "/\r/\r/evil.com",
+            "/\n/\n/evil.com",
+        ],
+    )
+    def test_rejects_tab_cr_lf_obfuscated_authority_relative_paths(self, obfuscated_next_path):
+        assert validate_next_path(obfuscated_next_path) == "", (
+            f"{obfuscated_next_path!r} must be rejected — browsers strip tab/CR/LF before parsing, "
+            "so this collapses to an authority-relative '///evil.com' navigation"
+        )

apps/api/plane/utils/path_validator.py

@@ -113,6 +113,15 @@ def validate_next_path(next_path: str) -> str:
         return ""
 
     next_path = next_path.replace("\\", "")
+
+    # Browsers (per the WHATWG URL spec) strip every ASCII tab/CR/LF from a
+    # URL before parsing it, so "/\t/\t/evil.com" is what the browser
+    # actually navigates on, even though urlparse() sees a netloc-free,
+    # scheme-free string here and a literal .startswith("//") below would
+    # miss it too (the second character is a tab, not a slash). Strip them
+    # here so every check downstream sees what the browser will.
+    next_path = next_path.translate(str.maketrans("", "", "\t\r\n"))
+
     parsed_url = urlparse(next_path)
 
     # Block absolute URLs or anything with scheme/netloc
@@ -123,6 +132,17 @@ def validate_next_path(next_path: str) -> str:
     if not next_path or not next_path.startswith("/"):
         return ""
 
+    # Reject authority-relative paths (//, ///, ////, ...). urlparse() only
+    # treats a leading "//" as a netloc when what follows still looks like a
+    # bare host (e.g. "//example.com/"); for "///example.com/" both scheme
+    # and netloc come back empty, so the branch above never fires and this
+    # string would otherwise sail through every check below unmodified. The
+    # browser itself still resolves any leading "//" as authority-relative
+    # against an http(s) base, navigating off-domain regardless of what
+    # urlparse() made of it server-side.
+    if next_path.startswith("//"):
+        return ""
+
     # Prevent path traversal
     if ".." in next_path:
         return ""

apps/web/core/lib/wrappers/authentication-wrapper.tsx

@@ -11,6 +11,7 @@ import useSWR from "swr";
 // components
 import { LogoSpinner } from "@/components/common/logo-spinner";
 // helpers
+import { isValidNextPath } from "@plane/utils";
 import { EPageTypes } from "@/helpers/authentication.helper";
 // hooks
 import { useWorkspace } from "@/hooks/store/use-workspace";
@@ -24,10 +25,19 @@ type TAuthenticationWrapper = {
   pageType?: TPageType;
 };
 
-const isValidURL = (url: string): boolean => {
-  const disallowedSchemes = /^(https?|ftp):\/\//i;
-  return !disallowedSchemes.test(url);
-};
+// Delegates to the shared isValidNextPath (@plane/utils) instead of a local
+// reimplementation. A from-scratch version here previously resolved the
+// value against location.origin and required the result to stay
+// same-origin — which has its own gap: a next_path like "http:evil.com"
+// resolves AS IF relative whenever the input's scheme happens to match the
+// real origin's own scheme, e.g. any self-hosted deployment actually
+// serving over plain http (verified directly: this bypasses the
+// location.origin-based check on an http:// origin, though not on https://,
+// since the schemes then differ). isValidNextPath closes this by requiring
+// a literal leading "/" (and rejecting "//") before any URL-based
+// comparison, so it doesn't depend on which scheme the real origin happens
+// to use.
+const isValidURL = isValidNextPath;
 
 export const AuthenticationWrapper = observer(function AuthenticationWrapper(props: TAuthenticationWrapper) {
   const pathname = usePathname();