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 0000000000..ce9c8b445c --- /dev/null +++ b/apps/api/plane/tests/unit/utils/test_path_validator.py @@ -0,0 +1,70 @@ +# 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" diff --git a/apps/api/plane/utils/path_validator.py b/apps/api/plane/utils/path_validator.py index 2ea71c18f2..b3a2c80e1a 100644 --- a/apps/api/plane/utils/path_validator.py +++ b/apps/api/plane/utils/path_validator.py @@ -123,6 +123,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 fc142c3002..f252db5a60 100644 --- a/apps/web/core/lib/wrappers/authentication-wrapper.tsx +++ b/apps/web/core/lib/wrappers/authentication-wrapper.tsx @@ -25,8 +25,17 @@ type TAuthenticationWrapper = { }; const isValidURL = (url: string): boolean => { - const disallowedSchemes = /^(https?|ftp):\/\//i; - return !disallowedSchemes.test(url); + // A prefix-only scheme check (http(s)/ftp) lets an authority-relative + // value like "///example.com/" through: it matches none of those schemes, + // but the browser still resolves a leading "//" against the current + // origin as an authority (host), navigating off-domain. Resolve against + // location.origin and require the result to actually still be same-origin + // instead of pattern-matching the input string. + try { + return new URL(url, location.origin).origin === location.origin; + } catch { + return false; + } }; export const AuthenticationWrapper = observer(function AuthenticationWrapper(props: TAuthenticationWrapper) {