Potential fix for code scanning alert no. 636: URL redirection from remote source

[sriramveeraghanta]

Potential fix for https://github.com/makeplane/plane/security/code-scanning/636

To fully fix this issue and prevent untrusted URL redirection, we should improve the validation logic in validate_next_path such that it matches browser behaviors—specifically, removing backslashes, robustly checking for absolute URLs, and only allowing safe, relative paths. We should adjust validate_next_path in apps/api/plane/utils/path_validator.py so it replaces backslashes with nothing (or converts to forward slashes), then uses urlparse to ensure no scheme/netloc is present. It should also only allow paths that start with a forward slash, do not contain path traversal (..), and are not empty.

You should update validate_next_path accordingly so all places where it is called (notably the redirect in the SignUpAuthEndpoint class in apps/api/plane/authentication/views/app/email.py) are using improved validation that blocks malformed or dangerous input.

No changes are required in the view file—just improve the underlying validator so its contract is strong and future-proof.

---

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

Summary by CodeRabbit

• New Features
  • Improved handling of redirect paths from absolute URLs and Windows-style paths for consistent behavior across platforms.
• Bug Fixes
  • Strengthened validation to require non-empty, leading-slash paths and reject unsafe patterns, reducing invalid redirects.
  • Maintained protection against path traversal for safer navigation.
• Documentation
  • Clarified description of what constitutes a safe relative path for redirection.

[coderabbitai]

Walkthrough

The path validator in apps/api/plane/utils/path_validator.py was updated to sanitize and validate redirect paths: normalize backslashes, extract paths from absolute URLs, enforce non-empty paths starting with "/", and continue blocking ".." traversal. Docstrings and comments were adjusted; return behavior remains the sanitized path.

Changes

Cohort / File(s)	Summary
Path validation utility apps/api/plane/utils/path_validator.py	Update docstring; normalize backslashes; if absolute URL, use parsed path; require non-empty path starting with "/"; maintain rejection of ".."; return sanitized path.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant Validator as PathValidator
  participant URL as URLParser

  Client->>Validator: next_path
  Note right of Validator: Normalize backslashes

  Validator->>URL: parse(next_path)
  alt Has scheme or netloc
    URL-->>Validator: parsed_url
    Note right of Validator: Use parsed_url.path
  else Relative path
    URL-->>Validator: parsed_url
    Note right of Validator: Keep as-is (post-normalization)
  end

  alt Invalid: empty OR not starting with "/" OR contains ".."
    Validator-->>Client: Reject/None (invalid)
  else Valid
    Validator-->>Client: Sanitized path
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• pushya22

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The current PR description does not follow the repository template because it omits the required section headings for “### Type of Change,” “### Screenshots and Media,” “### Test Scenarios,” and “### References,” and it lacks the checkbox under Type of Change to indicate the nature of the update. Although it contains a detailed narrative of the changes, it is not organized according to the prescribed structure, so reviewers cannot quickly verify that all template fields are addressed.	Please update the PR description to match the repository template by adding the following headings: “### Type of Change” with the appropriate checkbox ticked, “### Screenshots and Media” if applicable, “### Test Scenarios” detailing how changes were tested, and “### References” linking to the related security alert or issue. Make sure to include a descriptive narrative under “### Description” summarizing the implemented changes and rationale. This will ensure consistency and completeness for reviewers.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The current title clearly identifies the intent to address code scanning alert 636 and specifies the URL redirection vulnerability, which aligns with the main change in the PR; it is specific and directly related to the changeset. Although it could be slightly shorter and more action-oriented, it accurately conveys the purpose of the update. Therefore, it meets the criteria for summarizing the primary change without being misleading or off-topic.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Poem

I hop through slashes, neat and trim,
Backslashes tamed on a cautious whim.
Schemes stripped away, just paths in sight,
No sneaky dot-dot on my nightly flight.
A safe little burrow of “/” to stay—
Redirects secure, hip-hop-hooray! 🐇✨

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch alert-autofix-636

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull Request Overview

This PR addresses a security vulnerability (code scanning alert #636) related to URL redirection from remote sources by improving the validation logic in the validate_next_path function. The fix strengthens the validation to prevent untrusted URL redirection attacks.

Key changes:

• Enhanced path validation to handle backslash interpretation like browsers
• Improved validation logic to block absolute URLs and dangerous path traversal
• Added explicit checks for empty paths and proper relative path format

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

Removing backslashes entirely may be too aggressive and could break legitimate paths that contain escaped characters. Consider replacing backslashes with forward slashes instead: next_path = next_path.replace('\\', '/') to maintain path structure while preventing bypass attempts.

Suggested change

	# Browsers interpret backslashes as forward slashes. Remove all backslashes.
	next_path = next_path.replace("\\", "")
	# Browsers interpret backslashes as forward slashes. Replace backslashes with forward slashes.
	next_path = next_path.replace("\\", "/")

[Copilot]

The current logic extracts the path component from absolute URLs instead of rejecting them entirely. This could still allow malicious redirects if the extracted path contains dangerous sequences. Consider returning an empty string immediately when scheme or netloc is detected: return ''.

Suggested change

	next_path = parsed_url.path # Extract only the path component
	return ""

[coderabbitai]

Actionable comments posted: 2

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)

5-23: Enforce validate_next_path on all next_path assignments

• Replace raw str(next_path) with str(validate_next_path(next_path)) in space/github.py (line 51), space/gitlab.py (line 51) and space/magic.py (line 103).
• Ensure when validate_next_path returns "", redirects fall back to a safe default (e.g. /).

🧹 Nitpick comments (4)

apps/api/plane/utils/path_validator.py (4)

6-8: Normalize backslashes to '/' instead of deleting them.

Dropping backslashes can merge tokens unexpectedly (e.g., "a..\b" -> "a..\b"). Using '/' mirrors browser behavior without altering token boundaries.

-    # Browsers interpret backslashes as forward slashes. Remove all backslashes.
-    next_path = next_path.replace("\\", "")
+    # Browsers interpret backslashes as forward slashes. Normalize backslashes.
+    next_path = next_path.replace("\\", "/")

---

15-17: Trim whitespace before leading-slash check.

Prevents false negatives on inputs like ' /foo'.

-    # Must start with a forward slash and not be empty
-    if not next_path or not next_path.startswith("/"):
+    # Must start with a forward slash and not be empty
+    next_path = next_path.strip()
+    if not next_path or not next_path.startswith("/"):
         return ""

---

5-23: Optional: tighten invariants and add minimal input guard.

If non-strings can reach here (e.g., None), .replace will throw. Consider a quick type/length guard.

 def validate_next_path(next_path: str) -> str:
-    """Validates that next_path is a safe relative path for redirection."""
+    """Validates that next_path is a safe relative path for redirection."""
+    if not isinstance(next_path, str):
+        return ""

---

5-23: Test vectors to cover before merge.

Suggest adding unit tests for:

• "/projects" -> "/projects"
• " //evil.com/x" (with leading space) -> ""
• "//evil.com/x" -> ""
• "////evil.com/x" -> ""
• "https://evil.com/a?b=1" -> "/a" (or confirm desired behavior)
• "/%2e%2e/admin" -> ""
• "/a/%2E%2E/b" -> ""
• "/a..b" -> allowed (confirm requirements) or disallow if you want a strict policy
• "http:\evil.com\foo" -> ""
• "/..\admin" -> "" (after normalization)

Do you want me to add a pytest module with these cases?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db18c35 and dab36a9.

📒 Files selected for processing (1)

• apps/api/plane/utils/path_validator.py (1 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). (1)

• GitHub Check: Build and lint web apps

🔇 Additional comments (1)

apps/api/plane/utils/path_validator.py (1)

11-13: Confirm intent: query/fragment from absolute URLs are dropped.

For 'https://ex.com/a?b=1#x' you now return '/a' (query lost). If callers expect to retain safe query params, consider preserving parsed_url.query after validating. Otherwise, please confirm this behavior is desired.

[coderabbitai]

🛠️ Refactor suggestion

Block protocol-relative URLs ('//...') and multi-slash variants explicitly.

Cases like '////evil.com/path' may be parsed as a path but are treated by browsers as network-path references. Guard them.

     if parsed_url.scheme or parsed_url.netloc:
         next_path = parsed_url.path  # Extract only the path component
+
+    # Disallow protocol-relative/network-path references
+    if next_path.startswith("//"):
+        return ""

📝 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.

Suggested change

	# Block absolute URLs or anything with scheme/netloc
	if parsed_url.scheme or parsed_url.netloc:
	next_path = parsed_url.path # Extract only the path component
	# Block absolute URLs or anything with scheme/netloc
	if parsed_url.scheme or parsed_url.netloc:
	next_path = parsed_url.path # Extract only the path component
	# Disallow protocol-relative/network-path references
	if next_path.startswith("//"):
	return ""

🤖 Prompt for AI Agents

In apps/api/plane/utils/path_validator.py around lines 11 to 13, the current
check only blocks absolute URLs via scheme/netloc but misses protocol-relative
or multi-leading-slash inputs like '//evil.com' or '////evil.com/path' which
browsers treat as network-path references; update the validator to explicitly
detect and reject any input whose original string or parsed_url.path begins with
two or more slashes (e.g. startswith('//') or re.match(r'^/{2,}', ...)) and
treat them as invalid before accepting parsed_url.path so these network-path
references cannot slip through.

[coderabbitai]

🛠️ Refactor suggestion

Catch encoded traversal ('%2e%2e') by decoding once before checking.

Naively searching for '..' misses percent-encoded variants. Decode once for detection; still return the original sanitized path.

-    # Prevent path traversal
-    if ".." in next_path:
+    # Prevent path traversal (including percent-encoded variants)
+    if ".." in unquote(next_path):
         return ""

Add import (outside this hunk):

-from urllib.parse import urlparse
+from urllib.parse import urlparse, unquote

📝 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.

Suggested change

	# Prevent path traversal
	if ".." in next_path:
	return ""
	++ b/apps/api/plane/utils/path_validator.py
	@@ imports
	from urllib.parse import urlparse, unquote
	@@ def validate_next_path(...):
	- # Prevent path traversal
	# Prevent path traversal (including percent-encoded variants)
	if ".." in unquote(next_path):
	return ""

🤖 Prompt for AI Agents

In apps/api/plane/utils/path_validator.py around lines 19 to 21, the traversal
check only looks for literal ".." and misses percent-encoded variants; decode
the incoming path once (e.g., using urllib.parse.unquote) into a local variable
and check that decoded string for ".." (and other traversal patterns if
desired), but continue to return the original sanitized path as before; also add
the required import (from urllib.parse import unquote) at the top of the file
outside this hunk.