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

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 <noreply@plane.so>
This commit is contained in:
Manish Gupta
2026-08-28 15:59:06 +05:30
parent 3478d4fac4
commit 8537b6edfd
3 changed files with 92 additions and 2 deletions

View File

@@ -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"

View File

@@ -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 ""

View File

@@ -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) {