From 8537b6edfd2075f010397f3cc99e04698b7026d1 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 28 Aug 2026 15:59:06 +0530 Subject: [PATCH] [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 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) {