[WEB-4943]fix: next path url redirection#7817
Conversation
vamsikrishnamathala
requested review from
prateekshourya29 and
sriramveeraghanta
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds client-side next_path redirect handling in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page as SpacePage (page.tsx)
participant URL as URLUtils (isValidNextPath)
participant Auth as Auth State
participant Router
User->>Page: GET /space?next_path=/projects/123
Page->>Auth: read currentUser / isAuthenticated
Page->>Page: read searchParams.next_path
Page->>URL: isValidNextPath(next_path)
alt authenticated AND valid next_path
Page->>User: render full-screen spinner
Page->>Router: router.replace(next_path)
Router-->>User: navigate to next_path
else not authenticated OR invalid next_path
alt authenticated
Page->>User: render UserLoggedIn
else
Page->>User: render AuthView / init spinner
end
end
sequenceDiagram
autonumber
participant Client
participant Server as SpaceAuthEndpoint
participant Validator as get_allowed_hosts()/url_has_allowed_host_and_scheme
Client->>Server: OAuth/signin callback (with next_path)
Server->>Server: validate_next_path(next_path) -> build redirect URL
Server->>Validator: url_has_allowed_host_and_scheme(url, allowed_hosts)
alt allowed
Server-->>Client: HttpResponseRedirect(url)
else not allowed
Server-->>Client: HttpResponseRedirect(base_host)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/utils/src/url.ts (2)
286-295: Reduce false positives in malicious pattern checks (optional)Current patterns will reject benign paths containing “javascript” in query/text (e.g.,
/docs/javascript:the-good-parts). If desired, anchor scheme checks to the start.- const maliciousPatterns = [ - /javascript:/i, - /data:/i, - /vbscript:/i, - /<script/i, - /on\w+=/i, // Event handlers like onclick=, onload= - ]; + // Allow words like "javascript" in content; only block when used as a scheme + const maliciousPatterns = [ + /^(?:\/|\.)*(?:javascript|data|vbscript):/i, + ];
276-296: Add targeted tests for next_path validationPlease cover:
- Accept:
/,/dashboard,/workspace/123?x=1#y- Reject:
//example.com,https://x.com,ftp://x,javascript:alert(1),/javascript:alert(1),\evil,/leadingspace,/trailingspaceI can draft Vitest/Jest cases under
packages/utils/__tests__/url.spec.ts.apps/space/app/page.tsx (1)
2-2: ComputeisSafeNextPathonce (minor)Avoid double evaluation and tighten conditions.
-import { useEffect } from "react"; +import { useEffect, useMemo } from "react"; @@ - const nextPath = searchParams.get("next_path"); + const nextPath = searchParams.get("next_path"); + const isSafeNextPath = useMemo(() => (nextPath ? isValidNextPath(nextPath) : false), [nextPath]); @@ - if (currentUser && isAuthenticated && nextPath && isValidNextPath(nextPath)) { + if (currentUser && isAuthenticated && isSafeNextPath) { router.replace(nextPath); } - }, [currentUser, isAuthenticated, nextPath, router]); + }, [currentUser, isAuthenticated, isSafeNextPath, nextPath, router]); @@ - if (nextPath && isValidNextPath(nextPath)) { + if (isSafeNextPath) { return ( <div className="flex h-screen min-h-[500px] w-full justify-center items-center"> <LogoSpinner /> </div> ); }Also applies to: 16-19, 20-24, 33-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/space/app/page.tsx(3 hunks)packages/utils/src/url.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/utils/src/url.ts (1)
apps/api/plane/settings/storage.py (1)
url(16-17)
apps/space/app/page.tsx (2)
packages/utils/src/url.ts (1)
isValidNextPath(276-296)apps/space/core/components/account/user-logged-in.tsx (1)
UserLoggedIn(14-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/space/app/page.tsx (2)
20-24: Verify no external redirect with next_path=//example.comUntil
isValidNextPathblocks protocol‑relative URLs,router.replace(nextPath)may navigate off‑origin. After updating the util, please re-check this path manually.Steps:
- Open the app at
/?next_path=//example.com.- Ensure no navigation occurs and the page falls back to
UserLoggedIn.Also applies to: 33-41
6-6: Confirm barrel export for isValidNextPath in @plane/utilsFound a named export in packages/utils/src/url.ts:276, but I couldn't confirm it's re-exported from the package entry — ensure packages/utils/src/index.ts (or the package "exports" field) re-exports it (e.g.
export { isValidNextPath } from './url'orexport * from './url'), or change the import toimport { isValidNextPath } from '@plane/utils/url'.
Updated SignInAuthSpaceEndpoint, GitHubCallbackSpaceEndpoint, GitLabCallbackSpaceEndpoint, and GoogleCallbackSpaceEndpoint to include checks for allowed hosts and schemes before redirecting. This improves the security of URL redirection by ensuring only valid URLs are used.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/api/plane/authentication/views/space/github.py (2)
57-102: Critical: local var ‘base_host’ shadows the imported function and will crash the callback.At Line 61 you assign
base_host = request.session.get("host"), then later callbase_host(request=request, is_space=True)(Lines 71, 84, 98, 102). This will raise “str object is not callable”. Rename/remove the local or alias the import.Apply:
-from plane.authentication.utils.host import base_host +from plane.authentication.utils.host import base_host as get_base_host- base_host = request.session.get("host") + session_host = request.session.get("host")- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=get_base_host(request=request, is_space=True), next_path=next_path, params=params )- url = f"{base_host(request=request, is_space=True).rstrip("/")}{next_path}" + url = f"{get_base_host(request=request, is_space=True).rstrip('/')}{next_path}" - if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): + if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): return HttpResponseRedirect(url) else: - return HttpResponseRedirect(base_host(request=request, is_space=True)) + return HttpResponseRedirect(get_base_host(request=request, is_space=True))- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=get_base_host(request=request, is_space=True), next_path=next_path, params=params )
21-26: Persist a sanitized next_path in session during initiate.Without storing it, the callback sees
Noneand won’t redirect as intended.Apply:
request.session["host"] = base_host(request=request, is_space=True) - next_path = request.GET.get("next_path") + next_path = request.GET.get("next_path") + request.session["next_path"] = validate_next_path(next_path or "")apps/api/plane/authentication/views/space/google.py (2)
57-100: Same callable shadowing bug as GitHub flow.Local
base_host = request.session.get("host")(Line 61) shadows the function used on Lines 71, 83, 95, 99 — will crash.Apply:
-from plane.authentication.utils.host import base_host +from plane.authentication.utils.host import base_host as get_base_host- base_host = request.session.get("host") + session_host = request.session.get("host")- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=get_base_host(request=request, is_space=True), next_path=next_path, params=params )- url = f"{base_host(request=request, is_space=True).rstrip("/")}{next_path}" + url = f"{get_base_host(request=request, is_space=True).rstrip('/')}{next_path}" - if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): + if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): return HttpResponseRedirect(url) else: - return HttpResponseRedirect(base_host(request=request, is_space=True)) + return HttpResponseRedirect(get_base_host(request=request, is_space=True))- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=get_base_host(request=request, is_space=True), next_path=next_path, params=params )
21-26: Also persist sanitized next_path on initiate.Apply:
request.session["host"] = base_host(request=request, is_space=True) - next_path = request.GET.get("next_path") + next_path = request.GET.get("next_path") + request.session["next_path"] = validate_next_path(next_path or "")apps/api/plane/authentication/views/space/gitlab.py (2)
58-103: Same callable shadowing bug breaks callback redirect.
base_hostlocal (Line 62) shadows the imported function used later. Crash risk.Apply:
-from plane.authentication.utils.host import base_host +from plane.authentication.utils.host import base_host as get_base_host- base_host = request.session.get("host") + session_host = request.session.get("host")- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=get_base_host(request=request, is_space=True), next_path=next_path, params=params )- url = f"{base_host(request=request, is_space=True).rstrip("/")}{next_path}" + url = f"{get_base_host(request=request, is_space=True).rstrip('/')}{next_path}" - if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): + if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): return HttpResponseRedirect(url) else: - return HttpResponseRedirect(base_host(request=request, is_space=True)) + return HttpResponseRedirect(get_base_host(request=request, is_space=True))- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=get_base_host(request=request, is_space=True), next_path=next_path, params=params )
21-26: Persist next_path in session on initiate.Ensures callback has the intended target.
Apply:
request.session["host"] = base_host(request=request, is_space=True) - next_path = request.GET.get("next_path") + next_path = request.GET.get("next_path") + request.session["next_path"] = validate_next_path(next_path or "")
🧹 Nitpick comments (2)
apps/api/plane/authentication/views/space/gitlab.py (1)
100-103: Allowed-host check depends on fixing get_allowed_hosts().See prior comment; otherwise this likely falls back to base every time.
apps/api/plane/authentication/views/space/email.py (1)
103-109: LGTM with one small guard for empty next_path.If
validate_next_pathreturns"", current code redirects to the base origin without trailing “/”. Consider defaulting to root path.Apply:
- next_path = validate_next_path(next_path=next_path) + next_path = validate_next_path(next_path=next_path) or "/" - url = f"{base_host(request=request, is_space=True).rstrip("/")}{next_path}" + url = f"{base_host(request=request, is_space=True).rstrip('/')}{next_path}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/plane/authentication/views/space/email.py(2 hunks)apps/api/plane/authentication/views/space/github.py(3 hunks)apps/api/plane/authentication/views/space/gitlab.py(3 hunks)apps/api/plane/authentication/views/space/google.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/plane/authentication/views/space/github.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(95-124)validate_next_path(61-92)get_allowed_hosts(46-58)
apps/api/plane/authentication/views/space/email.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(95-124)validate_next_path(61-92)get_allowed_hosts(46-58)
apps/api/plane/authentication/views/space/gitlab.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(95-124)validate_next_path(61-92)get_allowed_hosts(46-58)
apps/api/plane/authentication/views/space/google.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(95-124)validate_next_path(61-92)get_allowed_hosts(46-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
apps/api/plane/authentication/views/space/github.py (1)
7-8: Imports for safety checks look good.Also applies to: 18-18
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/authentication/views/space/github.py (1)
61-63: Blocker: local variable shadows base_host() callable, breaking redirects
base_host = request.session.get("host")shadows the imported function. Subsequent calls likebase_host(request=..., is_space=True)will raise “str object is not callable”. Remove or rename the local variable.- base_host = request.session.get("host") + # Avoid shadowing the imported base_host() callable. + # If you need the stored value, rename it (e.g., host_url) and use explicitly. + # host_url = request.session.get("host")Also applies to: 99-103
♻️ Duplicate comments (2)
apps/api/plane/authentication/views/space/github.py (1)
99-103: Allowed-hosts normalization may still fail (origins vs hosts)If
get_allowed_hosts()includes empty strings (e.g., settings set without scheme),url_has_allowed_host_and_schemewill always fail and force fallback. Fix inplane/utils/path_validator.pyby parsing to netloc and skipping empties. (Echoing earlier feedback.)Suggested change in plane/utils/path_validator.py:
def get_allowed_hosts() -> list[str]: """Get the allowed hosts from the settings.""" - base_origin = settings.WEB_URL or settings.APP_BASE_URL - - allowed_hosts = [] - if base_origin: - host = urlparse(base_origin).netloc - allowed_hosts.append(host) - if settings.ADMIN_BASE_URL: - # Get only the host - host = urlparse(settings.ADMIN_BASE_URL).netloc - allowed_hosts.append(host) - if settings.SPACE_BASE_URL: - # Get only the host - host = urlparse(settings.SPACE_BASE_URL).netloc - allowed_hosts.append(host) - return allowed_hosts + def _host(val: str | None) -> str | None: + if not val: + return None + parsed = urlparse(val) + host = parsed.netloc or (val if "://" not in val else "") + return host or None + + candidates = [ + settings.WEB_URL or settings.APP_BASE_URL, + settings.ADMIN_BASE_URL, + settings.SPACE_BASE_URL, + ] + hosts = [h for h in map(_host, candidates) if h] + return list(dict.fromkeys(hosts)) # de-dupe, keep orderapps/api/plane/authentication/views/space/email.py (1)
103-109: Redirect pattern looks good; ensure allowed_hosts normalizationThe construct/validate/redirect flow is sound. Verify
get_allowed_hosts()returns netlocs (no empties) so the check doesn’t always fail when settings lack schemes.If needed, apply the
get_allowed_hosts()normalization patch shared in the GitHub callback comment.
🧹 Nitpick comments (3)
apps/api/plane/authentication/views/space/github.py (1)
97-103: Safer redirect with HTTPS enforcement (optional)Consider requiring HTTPS in non-dev to tighten redirect checks.
- if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): + if url_has_allowed_host_and_scheme( + url, + allowed_hosts=get_allowed_hosts(), + require_https=request.is_secure(), + ):apps/api/plane/authentication/views/space/email.py (2)
103-109: Optional: enforce HTTPS when appropriateMirror production expectations by requiring HTTPS when the request is secure.
- if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): + if url_has_allowed_host_and_scheme( + url, + allowed_hosts=get_allowed_hosts(), + require_https=request.is_secure(), + ):
202-208: DRY the redirect blockThe build/validate/redirect block duplicates earlier logic. Extract a small helper (e.g.,
safe_redirect_to_next_path(request, next_path)) or reuse a common util to reduce drift.I can craft a tiny shared helper in
plane/utils/path_validator.pyor a local module to centralize this pattern. Want me to propose a patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/plane/authentication/views/space/email.py(2 hunks)apps/api/plane/authentication/views/space/github.py(1 hunks)apps/api/plane/authentication/views/space/gitlab.py(2 hunks)apps/api/plane/authentication/views/space/google.py(1 hunks)packages/utils/src/url.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/plane/authentication/views/space/gitlab.py
- packages/utils/src/url.ts
- apps/api/plane/authentication/views/space/google.py
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/authentication/views/space/email.py (1)
apps/api/plane/utils/path_validator.py (2)
validate_next_path(65-96)get_allowed_hosts(46-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/authentication/views/space/github.py (1)
25-26: next_path not persisted; confirm intent
GitHubOauthInitiateSpaceEndpointreadsnext_pathbut doesn’t persist it to session, while callback readsrequest.session["next_path"]. If client-side next_path handling is the sole path now, this is fine; otherwise persist it during initiate.- next_path = request.GET.get("next_path") + next_path = request.GET.get("next_path") + if next_path: + request.session["next_path"] = next_pathAlso applies to: 62-63
* fix: next path url redirection * fix: enhance URL redirection safety in authentication views Updated SignInAuthSpaceEndpoint, GitHubCallbackSpaceEndpoint, GitLabCallbackSpaceEndpoint, and GoogleCallbackSpaceEndpoint to include checks for allowed hosts and schemes before redirecting. This improves the security of URL redirection by ensuring only valid URLs are used. * chore: updated uitl to handle double / --------- Co-authored-by: pablohashescobar <nikhilschacko@gmail.com> Co-authored-by: Nikhil <118773738+pablohashescobar@users.noreply.github.com>
4 participants
Description
This update fixes the
next_pathredirection for space app.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit