From 8537b6edfd2075f010397f3cc99e04698b7026d1 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 15:59:06 +0530 Subject: [PATCH 1/4] [INFRA-779] fix(security): reject authority-relative next_path redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server: validate_next_path (apps/api/plane/utils/path_validator.py) calls urlparse(next_path) and only extracts .path when scheme or netloc is truthy. For "///example.com/" (three or more leading slashes), urlparse() returns both scheme and netloc empty, so that branch never fires and the raw string passes every remaining check unchanged. Fixed by rejecting any next_path starting with "//" outright, right after the existing "must start with /" check. Client: isValidURL (apps/web/core/lib/wrappers/authentication-wrapper.tsx) only regex-blocked a literal http(s)/ftp scheme prefix, so the same authority-relative string passed and was handed to router.push(). Fixed by resolving the URL against location.origin and requiring the result to actually still be same-origin, instead of pattern-matching the input. Browsers resolve a leading "//" as authority-relative even when neither validator's own URL parsing detected a host — the accepted value silently navigates off-domain post-login, a same-origin-trust phishing vector. Checked the advisory's other listed next_path consumers (auth-form components, oauth hooks, api.service.ts) — they only forward the value to a server-side auth redirect or a hidden form field, no independent client-side navigation, so they're covered by the server-side fix. 8 new server-side tests, fail-before verified. Client-side fix verified empirically via Node's URL parser (WHATWG-compliant, matches browser behavior) — no test harness exists for apps/web in this repo. Co-authored-by: Plane AI --- .../tests/unit/utils/test_path_validator.py | 70 +++++++++++++++++++ apps/api/plane/utils/path_validator.py | 11 +++ .../lib/wrappers/authentication-wrapper.tsx | 13 +++- 3 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 apps/api/plane/tests/unit/utils/test_path_validator.py 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..ce9c8b445c1 --- /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 2ea71c18f23..b3a2c80e1aa 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 fc142c3002c..f252db5a60d 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) { From 838341158fb031f8e1bc3948b6953fb0770a3297 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 16:07:17 +0530 Subject: [PATCH 2/4] [INFRA-779] strip tab/CR/LF from next_path before the authority-relative check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address /code-review finding on PR #9709: a tab (or CR/LF) placed between each slash — e.g. "/\t/\t/evil.com" — defeats both urlparse()'s netloc detection (verified: scheme='', netloc='' for this exact input) and the new literal next_path.startswith("//") check (the second character is a tab, not a slash). Browsers strip every ASCII tab/CR/LF from a URL before parsing it per the WHATWG spec, so what actually gets navigated to is "///evil.com" — the same authority-relative bypass this PR set out to close, just obfuscated with whitespace instead of extra literal slashes. Strip tab/CR/LF alongside the existing backslash removal, before urlparse and the "//" check both run, so every check downstream sees what the browser will. 3 new tests, fail-before verified. Co-authored-by: Plane AI --- .../tests/unit/utils/test_path_validator.py | 22 +++++++++++++++++++ apps/api/plane/utils/path_validator.py | 9 ++++++++ 2 files changed, 31 insertions(+) diff --git a/apps/api/plane/tests/unit/utils/test_path_validator.py b/apps/api/plane/tests/unit/utils/test_path_validator.py index ce9c8b445c1..a5e15f5dd16 100644 --- a/apps/api/plane/tests/unit/utils/test_path_validator.py +++ b/apps/api/plane/tests/unit/utils/test_path_validator.py @@ -68,3 +68,25 @@ def test_still_downgrades_absolute_urls_with_a_scheme_to_a_safe_path(self): 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 b3a2c80e1aa..b59daab8594 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 = "".join(char for char in next_path if char not in "\t\r\n") + parsed_url = urlparse(next_path) # Block absolute URLs or anything with scheme/netloc From 124103743e56800f949e01f5b339e499c266ea55 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 16:28:13 +0530 Subject: [PATCH 3/4] [INFRA-779] use str.translate instead of a char-by-char rebuild for tab/CR/LF stripping Address /code-review cleanup finding, following the same fix applied to the plane-ee port (INFRA-780): single-pass str.translate is shorter and matches the terse style of the adjacent .replace("\\", "") line. Also verified, and declining, the review's other finding on the EE port (delegate to Django's url_has_allowed_host_and_scheme instead of hand-rolling the "//" check): Django's own _url_has_allowed_host_and_scheme does not strip tab/CR/LF before its startswith("///") check either, so it would reintroduce the exact bypass this PR fixed, and validate_next_path also does path-traversal/suspicious-pattern checks Django's helper doesn't attempt at all. Co-authored-by: Plane AI --- apps/api/plane/utils/path_validator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/api/plane/utils/path_validator.py b/apps/api/plane/utils/path_validator.py index b59daab8594..ac7a231d033 100644 --- a/apps/api/plane/utils/path_validator.py +++ b/apps/api/plane/utils/path_validator.py @@ -120,7 +120,7 @@ def validate_next_path(next_path: str) -> str: # 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 = "".join(char for char in next_path if char not in "\t\r\n") + next_path = next_path.translate(str.maketrans("", "", "\t\r\n")) parsed_url = urlparse(next_path) From 43f4dc0a6a6e41adc3acf9af35b80168882bbf15 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Mon, 31 Aug 2026 09:53:09 +0530 Subject: [PATCH 4/4] [INFRA-779] delegate isValidURL to the shared isValidNextPath instead of a local reimplementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address /code-review findings on the plane-ee port (PR #9286), which apply equally here: the from-scratch location.origin-based check had 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. On this repo's real fix that meant any self-hosted deployment actually serving over plain http (not just the EE port's hardcoded-http placeholder-base variant) — verified directly: bypasses the check on an http:// origin, though not on https://, since the schemes then differ. isValidNextPath (@plane/utils, already used by apps/space for this identical purpose) 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. Also removes a second, independently-bug-prone implementation of the same check. Co-authored-by: Plane AI --- .../lib/wrappers/authentication-wrapper.tsx | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/apps/web/core/lib/wrappers/authentication-wrapper.tsx b/apps/web/core/lib/wrappers/authentication-wrapper.tsx index f252db5a60d..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,19 +25,19 @@ type TAuthenticationWrapper = { pageType?: TPageType; }; -const isValidURL = (url: string): boolean => { - // 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; - } -}; +// 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();