diff --git a/apps/api/plane/tests/unit/utils/test_path_validator.py b/apps/api/plane/tests/unit/utils/test_path_validator.py new file mode 100644 index 00000000000..a5e15f5dd16 --- /dev/null +++ b/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" + ) diff --git a/apps/api/plane/utils/path_validator.py b/apps/api/plane/utils/path_validator.py index 2ea71c18f23..ac7a231d033 100644 --- a/apps/api/plane/utils/path_validator.py +++ b/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 "" diff --git a/apps/web/core/lib/wrappers/authentication-wrapper.tsx b/apps/web/core/lib/wrappers/authentication-wrapper.tsx index fc142c3002c..96bdbb2ed76 100644 --- a/apps/web/core/lib/wrappers/authentication-wrapper.tsx +++ b/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();