[WEB-4900]: validated authentication redirection paths#7798
[WEB-4900]: validated authentication redirection paths#7798sriramveeraghanta merged 3 commits intopreviewfrom
Conversation
…afer URL redirection across authentication views
|
Caution Review failedThe pull request is closed. 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. WalkthroughCentralizes redirect URL construction by adding get_safe_redirect_url and replacing ad-hoc urljoin/urlencode and validate_next_path usage across app/space authentication and admin views; validate_next_path is hardened. Multiple OAuth/email/magic flows now call the new helper for success and error redirects. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Endpoint as Auth Endpoint
participant Logic as Auth Logic
participant Host as base_host()
participant Util as get_safe_redirect_url
User->>Endpoint: Request (may include next_path)
Endpoint->>Logic: perform auth / validate input
alt Success
Endpoint->>Host: base_host(is_app|is_space|is_admin)
Endpoint->>Util: get_safe_redirect_url(base_url, next_path, params={})
Util-->>Endpoint: safe_redirect_url
Endpoint-->>User: 302 -> safe_redirect_url
else Error/Exception
Endpoint->>Host: base_host(...)
Endpoint->>Util: get_safe_redirect_url(base_url, next_path, params={error=...})
Util-->>Endpoint: safe_redirect_url
Endpoint-->>User: 302 -> safe_redirect_url
end
note over Util: internally calls validate_next_path and encodes params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the validate_next_path function with a new get_safe_redirect_url function to enhance security for URL redirection across authentication views.
- Enhanced security for redirection paths with additional suspicious pattern detection
- Centralized URL construction logic to prevent inconsistencies
- Improved validation with length limits and expanded security checks
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
apps/api/plane/utils/path_validator.py |
Added new security function and enhanced validation with suspicious pattern detection |
apps/api/plane/license/api/views/admin.py |
Replaced manual URL construction with safe redirect function |
apps/api/plane/authentication/views/space/signout.py |
Updated to use centralized redirect URL generation |
apps/api/plane/authentication/views/space/magic.py |
Refactored magic link authentication to use safe URL construction |
apps/api/plane/authentication/views/space/google.py |
Updated Google OAuth flows to use safe redirect function |
apps/api/plane/authentication/views/space/gitlab.py |
Updated GitLab OAuth flows to use safe redirect function |
apps/api/plane/authentication/views/space/github.py |
Updated GitHub OAuth flows to use safe redirect function |
apps/api/plane/authentication/views/space/email.py |
Updated email authentication to use safe redirect function |
apps/api/plane/authentication/views/app/magic.py |
Updated app magic link flows with safe URL construction |
apps/api/plane/authentication/views/app/google.py |
Updated app Google OAuth to use safe redirect function |
apps/api/plane/authentication/views/app/gitlab.py |
Updated app GitLab OAuth to use safe redirect function |
apps/api/plane/authentication/views/app/github.py |
Updated app GitHub OAuth to use safe redirect function |
apps/api/plane/authentication/views/app/email.py |
Updated app email authentication with safe URL construction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/plane/authentication/views/space/gitlab.py (1)
61-62: Name collision: localbase_hostshadows the imported function and will crash when called.Rename the local variable.
- base_host = request.session.get("host") + host = request.session.get("host")apps/api/plane/authentication/views/space/github.py (1)
60-62: Name collision: localbase_hostshadows imported function.Rename the variable to avoid runtime error.
- base_host = request.session.get("host") + host = request.session.get("host")
🧹 Nitpick comments (9)
apps/api/plane/utils/path_validator.py (1)
45-73: Hardening suggestion: canonicalize before checks.Consider decoding once before checks to catch mixed/encoded traversal and schemes.
- next_path = next_path.replace("\\", "") + from urllib.parse import unquote + next_path = unquote(next_path.replace("\\", ""))apps/api/plane/authentication/views/space/signout.py (1)
25-35: Deduplicate redirect construction; compute once.Same URL is built in both branches. Build once and return; keep try/except around user updates only.
- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), - next_path=next_path - ) - return HttpResponseRedirect(url) + base = base_host(request=request, is_space=True) + url = get_safe_redirect_url(base_url=base, next_path=next_path) + return HttpResponseRedirect(url) except Exception: - url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), - next_path=next_path - ) - return HttpResponseRedirect(url) + base = base_host(request=request, is_space=True) + url = get_safe_redirect_url(base_url=base, next_path=next_path) + return HttpResponseRedirect(url)apps/api/plane/authentication/views/space/gitlab.py (2)
70-75: Use cached host with fallback.After renaming, prefer
host or base_host(...)to avoid recomputing and to handle missing session.- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=(host or base_host(request=request, is_space=True)), next_path=next_path, params=params )Also applies to: 83-88, 105-110
63-76: Minor: tighten state check response.Optionally rotate/reset the stored
stateon mismatch to avoid reuse.apps/api/plane/authentication/views/space/github.py (1)
69-74: Use cached host with fallback.- url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=(host or base_host(request=request, is_space=True)), next_path=next_path, params=params )Also applies to: 82-87, 104-109
apps/api/plane/authentication/views/space/email.py (1)
103-104: Unify success redirects (optional).Success paths still concatenate base_host + next_path directly. For consistency and future-proofing, consider using get_safe_redirect_url here too.
Apply:
- url = f"{base_host(request=request, is_space=True)}{str(next_path) if next_path else ''}" + url = get_safe_redirect_url( + base_url=base_host(request=request, is_space=True), + next_path=next_path, + params={} + )And similarly in SignUpAuthSpaceEndpoint.
Also applies to: 197-198
apps/api/plane/authentication/views/app/google.py (1)
100-108: Optional: clear next_path after successful login.Prevents stale redirects on later sessions.
Example:
url = get_safe_redirect_url( base_url=base_host(request=request, is_app=True), next_path=path, params={} ) + request.session.pop("next_path", None)apps/api/plane/authentication/views/app/github.py (1)
99-110: Optional: clear next_path after success.Avoids redirecting future sessions unintentionally.
Add:
url = get_safe_redirect_url( base_url=base_host(request=request, is_app=True), next_path=path, params={} ) + request.session.pop("next_path", None)apps/api/plane/authentication/views/app/gitlab.py (1)
101-112: Optional: clear next_path after success.Prevent stale session redirects.
Add:
url = get_safe_redirect_url( base_url=base_host(request=request, is_app=True), next_path=path, params={} ) + request.session.pop("next_path", None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/api/plane/authentication/views/app/email.py(11 hunks)apps/api/plane/authentication/views/app/github.py(7 hunks)apps/api/plane/authentication/views/app/gitlab.py(8 hunks)apps/api/plane/authentication/views/app/google.py(5 hunks)apps/api/plane/authentication/views/app/magic.py(7 hunks)apps/api/plane/authentication/views/space/email.py(11 hunks)apps/api/plane/authentication/views/space/github.py(6 hunks)apps/api/plane/authentication/views/space/gitlab.py(6 hunks)apps/api/plane/authentication/views/space/google.py(4 hunks)apps/api/plane/authentication/views/space/magic.py(7 hunks)apps/api/plane/authentication/views/space/signout.py(2 hunks)apps/api/plane/license/api/views/admin.py(2 hunks)apps/api/plane/utils/path_validator.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
apps/api/plane/license/api/views/admin.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/app/magic.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/space/github.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/space/gitlab.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/space/magic.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/app/gitlab.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/space/signout.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/app/google.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/space/email.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/space/google.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/app/github.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
apps/api/plane/authentication/views/app/email.py (1)
apps/api/plane/utils/path_validator.py (1)
get_safe_redirect_url(76-97)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
apps/api/plane/authentication/views/space/email.py (1)
17-17: Centralized safe redirects — good change.Consolidating error redirects via get_safe_redirect_url improves consistency and reduces open‑redirect risk. Looks good.
Also applies to: 32-36, 53-57, 71-75, 88-92, 107-111, 127-131, 147-151, 165-169, 182-186, 201-205
apps/api/plane/authentication/views/app/google.py (1)
38-42: Safe redirect consolidation — looks solid.Error/success flows consistently use get_safe_redirect_url and validate next_path. Good parity with other providers.
Also applies to: 54-57, 87-91, 112-116
apps/api/plane/authentication/views/app/github.py (1)
105-110: Success redirect via helper — LGTM.Good fallback to get_redirection_path and centralized URL construction.
apps/api/plane/authentication/views/app/gitlab.py (1)
38-42: Centralized safe redirects — LGTM.Consistent use of get_safe_redirect_url across flows looks good.
Also applies to: 52-56, 107-111
apps/api/plane/authentication/views/app/magic.py (1)
120-125: LGTM: centralized safe redirect.Converging on get_safe_redirect_url for the post-login redirect is correct and reduces duplication.
apps/api/plane/authentication/views/space/magic.py (1)
66-71: LGTM: consistent error redirect via get_safe_redirect_url.Good consolidation.
apps/api/plane/authentication/views/app/email.py (1)
116-122: LGTM: success redirect funneled through get_safe_redirect_url.Reduces bespoke URL building.
| base_url=base_host(request=request, is_app=True), | ||
| next_path=next_path, | ||
| params=params | ||
| ) | ||
| return HttpResponseRedirect(url) |
There was a problem hiding this comment.
Shadowing base_host in callback breaks redirects.
base_host = request.session.get("host") shadows the function; subsequent base_host(...) calls will error.
Rename:
- base_host = request.session.get("host")
+ host_url = request.session.get("host")Also applies to: 86-90, 114-118
🤖 Prompt for AI Agents
In apps/api/plane/authentication/views/app/github.py around lines 73-77 (and
similarly at 86-90 and 114-118), a local variable is assigned as base_host =
request.session.get("host"), which shadows the imported/function name base_host
and causes subsequent calls base_host(...) to fail; rename the session variable
to something non-conflicting (e.g., session_host or host_from_session), update
all uses of that variable in the callback to the new name, and keep all calls to
base_host(...) as function calls so redirects build correctly.
| base_url=base_host(request=request, is_app=True), | ||
| next_path=next_path, | ||
| params=params | ||
| ) | ||
| return HttpResponseRedirect(url) |
There was a problem hiding this comment.
Shadowing base_host in callback breaks redirects.
base_host = request.session.get("host") shadows the function; later base_host(...) calls will fail.
Rename:
- base_host = request.session.get("host")
+ host_url = request.session.get("host")Also applies to: 87-91, 116-120
🤖 Prompt for AI Agents
In apps/api/plane/authentication/views/app/gitlab.py around lines 74-78 (and
similarly at 87-91 and 116-120), the assignment base_host =
request.session.get("host") shadows the base_host function and breaks later
calls; rename the session variable (e.g., session_host or host_from_session) and
update all subsequent uses in those blocks to the new name so you no longer
override the function, ensuring any call sites that expect base_host(...) still
invoke the function.
| base_url=base_host(request=request, is_app=True), | ||
| next_path=next_path, | ||
| params=params | ||
| ) | ||
| return HttpResponseRedirect(url) |
There was a problem hiding this comment.
Do not shadow base_host in callback.
base_host = request.session.get("host") shadows the imported function and will break calls to base_host(...).
Rename:
- base_host = request.session.get("host")
+ host_url = request.session.get("host")Also applies to: 87-91
🤖 Prompt for AI Agents
In apps/api/plane/authentication/views/app/google.py around lines 75-79 (and
similarly lines 87-91), the local assignment "base_host =
request.session.get('host')" shadows the imported base_host function and will
break subsequent calls; rename the variable (e.g., session_host or
host_from_session) and update all references within this callback to use the new
variable name so the imported base_host(...) calls remain callable.
| if next_path: | ||
| params["next_path"] = str(validate_next_path(next_path)) | ||
| url = urljoin( | ||
| base_host(request=request, is_app=True), "?" + urlencode(params) | ||
| params["next_path"] = str(next_path) | ||
| url = get_safe_redirect_url( | ||
| base_url=base_host(request=request, is_app=True), | ||
| next_path=next_path, | ||
| params=params | ||
| ) | ||
| return HttpResponseRedirect(url) |
There was a problem hiding this comment.
Don't pre-populate params["next_path"]; this bypasses validation.
You add the raw next_path to params before calling get_safe_redirect_url. If validate_next_path rejects it, the unsafe value still persists in params and gets sent. Let get_safe_redirect_url inject the validated value.
- params = exc.get_error_dict()
- if next_path:
- params["next_path"] = str(next_path)
+ params = exc.get_error_dict()
url = get_safe_redirect_url(
base_url=base_host(request=request, is_app=True),
next_path=next_path,
params=params
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if next_path: | |
| params["next_path"] = str(validate_next_path(next_path)) | |
| url = urljoin( | |
| base_host(request=request, is_app=True), "?" + urlencode(params) | |
| params["next_path"] = str(next_path) | |
| url = get_safe_redirect_url( | |
| base_url=base_host(request=request, is_app=True), | |
| next_path=next_path, | |
| params=params | |
| ) | |
| return HttpResponseRedirect(url) | |
| params = exc.get_error_dict() | |
| url = get_safe_redirect_url( | |
| base_url=base_host(request=request, is_app=True), | |
| next_path=next_path, | |
| params=params | |
| ) | |
| return HttpResponseRedirect(url) |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/views/app/magic.py around lines 166 to 173, the
code currently set params["next_path"] = str(next_path) before calling
get_safe_redirect_url which allows an unvalidated next_path to persist in
params; remove the pre-population of params["next_path"] entirely and instead
pass the raw next_path to get_safe_redirect_url (so it can validate and inject
the safe value), leaving params unchanged prior to the call and then use the
returned url for HttpResponseRedirect.
| url = get_safe_redirect_url( | ||
| base_url=base_host(request=request, is_space=True), | ||
| next_path=next_path, | ||
| params=params | ||
| ) |
There was a problem hiding this comment.
Do not shadow base_host; causes TypeError.
Earlier in this method, base_host = request.session.get("host") shadows the imported function. Subsequent calls to base_host(...) will fail.
Rename the local variable:
- base_host = request.session.get("host")
+ host_url = request.session.get("host")No other changes needed if you keep calling the function base_host(...). Optionally, use host_url as base_url to avoid recomputation.
Also applies to: 81-85, 101-105
🤖 Prompt for AI Agents
In apps/api/plane/authentication/views/space/google.py around lines 69-73 (and
also apply same fix to blocks at 81-85 and 101-105), the local variable
base_host = request.session.get("host") is shadowing the imported function
base_host and causes a TypeError when later calling base_host(...). Rename the
local variable (e.g., host_value or host_url) and update all local uses to that
new name so calls to the imported function base_host(...) remain intact;
optionally use host_url as the base_url argument to get_safe_redirect_url to
avoid recomputing.
…ignInAuthSpaceEndpoint and SignUpAuthSpaceEndpoint
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/email.py (1)
45-51: Remove PII from redirect query params — stop adding raw emails to AuthenticationException.payloadpayload={"email": ...} is ending up in redirect URLs (exc.get_error_dict() → get_safe_redirect_url) and exposes emails in logs/referrers/caches. Instances found (fix all):
- apps/api/plane/authentication/views/space/email.py — ~45–51, 65–69, 82–86, 143–149, 163–167, 180–184
- apps/api/plane/authentication/views/app/email.py — ~49–55, 70–74, 86–90, 157–163, 177–181, 195–199
- apps/api/plane/authentication/adapter/base.py — ~53–57, 66–70, 78–82, 99–103
- apps/api/plane/authentication/provider/credentials/email.py — ~64–71, 73–76, 85–89
- apps/api/plane/authentication/provider/credentials/magic_code.py — multiple occurrences (e.g. ~31–38, 40–45, 71–80, 126–158)
- apps/api/plane/license/api/views/admin.py — multiple occurrences (e.g. ~129–136, 153–160, 170–176, 262–270, 278–286, 294–302, 319–326, 332–340)
Required action (one or more of):
- Remove/omit the email from params passed to get_safe_redirect_url (do not include payload["email"]). Example: replace payload={"email": str(email)} with no payload for redirect flows.
- If the UI must pre-fill an email after redirect, store the email server-side (session/flash) and read it client-side post-redirect — do not put it in the URL.
- Or centralize redaction: change AuthenticationException.get_error_dict() (or the redirect serialization) to redact PII (or provide a flag to omit sensitive fields) while preserving internal use of payload where needed.
Do not leave raw emails in query params; this is a PII exposure.
♻️ Duplicate comments (1)
apps/api/plane/authentication/views/space/email.py (1)
17-17: Switch to get_safe_redirect_url looks good; ensure helper avoids mutable default dict.Echoing the earlier review: fix
params: dict = {}inget_safe_redirect_urltoparams: dict | None = Noneand defensively copy beforeurlencode. This file relies on that helper across all flows.Run to confirm the helper signature and default handling are fixed:
#!/bin/bash set -euo pipefail # Locate path_validator.py and show the helper signature fd -a path_validator.py rg -n -C2 -g '**/path_validator.py' -P 'def\s+get_safe_redirect_url\s*\(' # Fail if a mutable default dict is still present rg -n -g '**/path_validator.py' -P 'params\s*:\s*dict\s*=\s*\{\}' && { echo "Mutable default found"; exit 1; } || echo "OK: no mutable default"
🧹 Nitpick comments (3)
apps/api/plane/authentication/views/space/email.py (3)
32-36: Deduplicate redirect building: precompute base_url and add a tiny _redirect helper.Removes repetition and avoids recomputing
base_host(...). After fixing the helper signature, also dropparams={}to prevent trailing?when empty.Apply pattern like:
def post(self, request): - next_path = request.POST.get("next_path") + next_path = request.POST.get("next_path") + base_url = base_host(request=request, is_space=True) + def _redirect(params=None): + url = get_safe_redirect_url(base_url=base_url, next_path=next_path, params=params) + return HttpResponseRedirect(url) @@ - url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), - next_path=next_path, - params=params - ) - return HttpResponseRedirect(url) + return _redirect(params)Repeat for the other occurrences, and for success paths use
return _redirect()(no params).Also applies to: 53-57, 71-75, 88-92, 103-107, 111-115, 131-135, 151-155, 169-173, 186-190, 201-205, 209-213
40-41: Use None (default) instead of False for POST.get defaults.Slightly cleaner semantics and type expectations.
- email = request.POST.get("email", False) - password = request.POST.get("password", False) + email = request.POST.get("email") + password = request.POST.get("password")
79-82: Avoid loading model objects when only existence is needed.Use
.exists()to save a query/row load.- existing_user = User.objects.filter(email=email).first() - - if not existing_user: + user_exists = User.objects.filter(email=email).exists() + if not user_exists:For signup:
- existing_user = User.objects.filter(email=email).first() - - if existing_user: + user_exists = User.objects.filter(email=email).exists() + if user_exists:Also applies to: 179-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/authentication/views/space/email.py(11 hunks)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
| base_url=base_host(request=request, is_space=True), | ||
| next_path=next_path, | ||
| params=params | ||
| ) |
There was a problem hiding this comment.
Bug: OAuth Flow Fails Due to Undefined Variable
The get_safe_redirect_url call in the successful OAuth authentication flow uses an undefined params variable. This variable is only initialized in error handling branches, which will cause a NameError when authentication succeeds.
Additional Locations (2)
* refactor: replace validate_next_path with get_safe_redirect_url for safer URL redirection across authentication views * refactor: use get_safe_redirect_url for improved URL redirection in SignInAuthSpaceEndpoint and SignUpAuthSpaceEndpoint * fix: redirect paths --------- Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
Description
refactor: replace validate_next_path with get_safe_redirect_url for safer URL redirection across authentication views
Type of Change
Test Scenarios
References
WEB-4900
Summary by CodeRabbit