[WEB-4943] refactor: enhance URL validation and redirection logic in authentication views#7815
Conversation
…ion views * Updated authentication views (SignInAuthSpaceEndpoint, GitHubCallbackSpaceEndpoint, GitLabCallbackSpaceEndpoint, GoogleCallbackSpaceEndpoint, and MagicSignInSpaceEndpoint) to include url_has_allowed_host_and_scheme checks for safer redirection. * Improved URL construction by ensuring proper formatting and fallback to base host when necessary. * Added get_allowed_hosts function to path_validator.py for better host validation.
|
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 allowed-host validation to space OAuth redirects (GitHub, GitLab, Google) using Django's url_has_allowed_host_and_scheme with get_allowed_hosts; refactors get_allowed_hosts and get_safe_redirect_url to assemble query parts and prefer netlocs; minor rstrip/import style tweaks in email and magic endpoints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant V as Auth View (Initiate/Callback)
participant PV as path_validator.get_safe_redirect_url
participant DH as Django url_has_allowed_host_and_scheme
participant GA as path_validator.get_allowed_hosts
C->>V: Request (next_path, params)
V->>PV: Build safe_url = get_safe_redirect_url(base_url, next_path, params)
PV-->>V: safe_url
V->>GA: get_allowed_hosts()
V->>DH: url_has_allowed_host_and_scheme(safe_url, allowed_hosts)
alt Allowed
V-->>C: Redirect to safe_url
else Disallowed
V-->>C: Redirect to base_host (preserve encoded params if present)
end
sequenceDiagram
autonumber
participant PV as get_safe_redirect_url
participant VA as validate_next_path
participant ENC as urlencode(params)
PV->>VA: validate next_path
VA-->>PV: validated_path or empty
PV->>ENC: urlencode(params) if params
ENC-->>PV: encoded_params
PV->>PV: assemble query_parts = [validated_path?, encoded_params?]
PV-->>Caller: return base_url[?query_parts] or base_url?encoded_params on disallowed next_path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 enhances URL validation and redirection logic in authentication views to improve security by implementing proper host validation and safe URL construction.
- Added
url_has_allowed_host_and_schemechecks to all authentication callback endpoints - Improved URL construction with proper query parameter handling in
get_safe_redirect_url - Enhanced
get_allowed_hostsfunction to extract netloc from base origin URLs
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/utils/path_validator.py | Enhanced host validation and URL construction logic |
| apps/api/plane/authentication/views/space/magic.py | Added host validation checks and fixed string quotes |
| apps/api/plane/authentication/views/space/google.py | Added URL validation with fallback redirection |
| apps/api/plane/authentication/views/space/gitlab.py | Added URL validation with fallback redirection |
| apps/api/plane/authentication/views/space/github.py | Added URL validation with fallback redirection |
| apps/api/plane/authentication/views/space/email.py | Added host validation import and fixed string quotes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on September 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
* Updated comments for clarity in the get_safe_redirect_url function. * Removed unnecessary blank line to enhance
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/plane/authentication/views/space/google.py (1)
61-61: Bug: base_host function shadowed by local variable (TypeError risk)Line 61 assigns base_host = request.session.get("host"), shadowing the imported function. Subsequent calls to base_host(...) will crash.
Apply this diff to remove the shadowing (the variable is unused):
- base_host = request.session.get("host") + # host saved during initiate is available via request.session["host"] if neededapps/api/plane/authentication/views/space/github.py (1)
61-61: Bug: base_host function shadowed by local variable (TypeError risk)Same issue as Google callback: assigning base_host = request.session.get("host") shadows the imported function and will break later calls.
Apply this diff:
- base_host = request.session.get("host") + # host saved during initiate is available via request.session["host"] if neededapps/api/plane/authentication/views/space/gitlab.py (1)
62-76: Critical:base_hostname shadowing breaks redirects.At Line 62,
base_host(string) shadows the imported function, causing'str' object is not callableat Lines 72/85/107.Apply:
- base_host = request.session.get("host") + session_base_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=session_base_host or base_host(request=request, is_space=True), next_path=next_path, params=params ) @@ - url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=session_base_host or base_host(request=request, is_space=True), next_path=next_path, params=params ) @@ - url = get_safe_redirect_url( - base_url=base_host(request=request, is_space=True), + url = get_safe_redirect_url( + base_url=session_base_host or base_host(request=request, is_space=True), next_path=next_path, params=params )
🧹 Nitpick comments (8)
apps/api/plane/utils/path_validator.py (3)
110-110: Remove unused importquote is unused after the refactor.
Apply this diff:
- from urllib.parse import urlencode, quote + from urllib.parse import urlencode
99-99: Avoid mutable default for paramsUse a None default to prevent accidental state bleed.
Apply this diff:
-def get_safe_redirect_url(base_url: str, next_path: str = "", params: dict = {}) -> str: +def get_safe_redirect_url(base_url: str, next_path: str = "", params: dict | None = None) -> str:No further changes needed since the body already guards with truthiness checks.
50-62: Filter empty hosts and deduplicate allowed_hostsurlparse(...).netloc can be empty if a URL lacks a scheme. Also dedupe to avoid redundant checks.
Apply this diff:
- allowed_hosts = [] - if base_origin: - host = urlparse(base_origin).netloc - allowed_hosts.append(host) + allowed_hosts = [] + if base_origin: + host = urlparse(base_origin).netloc + if host: + 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 host: + 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 + if host: + allowed_hosts.append(host) + # Preserve order while deduping + return list(dict.fromkeys(allowed_hosts))apps/api/plane/authentication/views/space/google.py (2)
24-25: next_path read but not persisted to sessionInitiate reads next_path but doesn’t store it for use in the callback. If callbacks rely on session-stored next_path, persist it here.
Apply this diff:
next_path = request.GET.get("next_path") + request.session["next_path"] = next_path
21-47: Optional: tighten HTTPS requirement when appropriateIf deployments enforce HTTPS, consider passing require_https=True to url_has_allowed_host_and_scheme via a helper to align with security posture. Leave off in dev.
I can wire this through a small utility that consults settings.SECURE_SSL_REDIRECT.
apps/api/plane/authentication/views/space/email.py (1)
103-109: Inconsistent post-sign-in redirect style vs other endpointsHere you redirect to base_url with ?next_path=..., while other flows redirect directly to base_url + next_path. Consider unifying for predictable UX (either always direct-path or always query-param).
I can submit a follow-up diff to align this with the direct-path pattern plus allowed-host check.
apps/api/plane/authentication/views/space/github.py (1)
24-26: next_path read but not persisted to sessionInitiate reads next_path but doesn’t save it for the callback. Persist if callbacks depend on it.
Apply this diff:
request.session["host"] = base_host(request=request, is_space=True) next_path = request.GET.get("next_path") + request.session["next_path"] = next_pathapps/api/plane/authentication/views/space/gitlab.py (1)
99-103: Allow‑list check: good; add https requirement and consistent base URL usage.Use the session host when available and require https when the request is secure.
Apply:
- url = f"{base_host(request=request, is_space=True).rstrip('/')}{next_path}" - 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)) + base_origin = (session_base_host or base_host(request=request, is_space=True)).rstrip("/") + url = f"{base_origin}{next_path or '/'}" + if url_has_allowed_host_and_scheme( + url, + allowed_hosts=get_allowed_hosts(), + require_https=request.is_secure(), + ): + return HttpResponseRedirect(url) + return HttpResponseRedirect(base_origin + "/")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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)apps/api/plane/authentication/views/space/magic.py(2 hunks)apps/api/plane/utils/path_validator.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/plane/authentication/views/space/google.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(99-144)validate_next_path(65-96)get_allowed_hosts(46-62)
apps/api/plane/authentication/views/space/github.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(99-144)validate_next_path(65-96)get_allowed_hosts(46-62)
apps/api/plane/authentication/views/space/email.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(99-144)validate_next_path(65-96)get_allowed_hosts(46-62)
apps/api/plane/authentication/views/space/gitlab.py (1)
apps/api/plane/utils/path_validator.py (3)
get_safe_redirect_url(99-144)get_allowed_hosts(46-62)validate_next_path(65-96)
⏰ 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: Analyze (javascript)
🔇 Additional comments (8)
apps/api/plane/authentication/views/space/magic.py (1)
99-103: Redirect allowed-host guardrails look goodUsing url_has_allowed_host_and_scheme with get_allowed_hosts and falling back to base_host is correct and closes open-redirect vectors.
Also applies to: 161-165
apps/api/plane/utils/path_validator.py (1)
144-144: Confirm intent: dropping next_path on fallbackOn disallowed URLs you return base_url with params but omit next_path. If UX expects preserving the intended destination (still as a query param on the safe origin), consider keeping next_path encoded in the fallback too.
Would you like me to adjust fallback to include next_path as a query param on base_url?
apps/api/plane/authentication/views/space/google.py (1)
95-99: LGTM: safe redirect with allowed-host checkCorrectly validates the constructed absolute URL and falls back to base host.
apps/api/plane/authentication/views/space/email.py (1)
202-206: LGTM: post-signup redirect hardenedAllowed-host check with fallback is correct and consistent with other endpoints.
apps/api/plane/authentication/views/space/github.py (1)
98-102: LGTM: safe redirect with allowed-host checkThe guard plus fallback looks correct.
apps/api/plane/authentication/views/space/gitlab.py (3)
7-7: Import for host allow‑listing: looks good.Correct utility for validating redirect targets.
18-18: Centralized validators import: good call.Keeps all redirect safety helpers in one place.
25-26: Persistnext_pathacross the OAuth round‑trip.Callback reads
request.session["next_path"], but initiate view doesn’t set it here.If not set elsewhere, add:
request.session["host"] = base_host(request=request, is_space=True) next_path = request.GET.get("next_path") + request.session["next_path"] = next_path
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/utils/path_validator.py (1)
99-99: Avoid mutable default for params.Use None default and normalize inside.
-def get_safe_redirect_url(base_url: str, next_path: str = "", params: dict = {}) -> str: +def get_safe_redirect_url(base_url: str, next_path: str = "", params: dict | None = None) -> str:(Initialization added in the previous diff: params = params or {}.)
♻️ Duplicate comments (2)
apps/api/plane/utils/path_validator.py (2)
143-143: Preserve the full query (including next_path) on fallback.Current fallback only keeps encoded_params; it drops next_path. Reuse the computed query_string.
- return base_url + (f"?{encoded_params}" if encoded_params else "") + return f"{base_url}?{query_string}" if query_string else base_url
118-137: Encode next_path; build the query once; and drop the '/?' (it mutates the path).
- next_path is appended unencoded and can break the query.
- '/?' inserts an extra slash, potentially changing routes (…/callback → …/callback/).
- urlencode called without doseq mishandles list values.
Apply:
- # Prepare the query parameters - query_parts = [] - encoded_params = "" - - # Add the next path to the parameters - if validated_path: - query_parts.append(f"next_path={validated_path}") - - # Add additional parameters - if params: - encoded_params = urlencode(params) - query_parts.append(encoded_params) - - # Construct the url query string - if query_parts: - query_string = "&".join(query_parts) - url = f"{base_url}/?{query_string}" - else: - url = base_url + # Assemble query once (encode next_path; support multi-value params) + params = params or {} + merged_params = dict(params) # shallow copy + if validated_path: + # ensure encoding by funneling through urlencode + merged_params["next_path"] = validated_path + query_string = urlencode(merged_params, doseq=True) + url = f"{base_url}?{query_string}" if query_string else base_url
🧹 Nitpick comments (3)
apps/api/plane/utils/path_validator.py (3)
50-62: Normalize and de‑dupe allowed hosts; handle URLs without a scheme.urlparse("example.com") yields empty netloc; also duplicates and case variance can creep in.
- 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 + allowed_hosts: list[str] = [] + for candidate in [base_origin, settings.ADMIN_BASE_URL, settings.SPACE_BASE_URL]: + if not candidate: + continue + parsed = urlparse(candidate.strip()) + host = (parsed.netloc or parsed.path).lower() # tolerate missing scheme + if host: + allowed_hosts.append(host) + # de-dup while preserving order + return list(dict.fromkeys(allowed_hosts))
110-110: Remove unused import.quote is not used.
- from urllib.parse import urlencode, quote + from urllib.parse import urlencode
139-141: Optionally restrict schemes.If production requires HTTPS, pass allowed_schemes={"https"}.
Would you like me to gate this by settings (e.g., SECURE_SSL_REDIRECT) and wire allowed_schemes accordingly?
…authentication views (makeplane#7815) * refactor: enhance URL validation and redirection logic in authentication views * Updated authentication views (SignInAuthSpaceEndpoint, GitHubCallbackSpaceEndpoint, GitLabCallbackSpaceEndpoint, GoogleCallbackSpaceEndpoint, and MagicSignInSpaceEndpoint) to include url_has_allowed_host_and_scheme checks for safer redirection. * Improved URL construction by ensuring proper formatting and fallback to base host when necessary. * Added get_allowed_hosts function to path_validator.py for better host validation. * refactor: improve comments and clean up code in path_validator.py * Updated comments for clarity in the get_safe_redirect_url function. * Removed unnecessary blank line to enhance
Description
path_validator.pyfor better host validation.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
WEB-4943
Summary by CodeRabbit