CVE-2024-34074: Insufficient Information (NVD-CWE-noinfo)
value: Frappe is a full-stack web application framework. Prior to 15.26.0 and 14.74.0, the login page accepts redirect argument and it allowed redirect to untrusted external URls. This behaviour can be used by malicious actors for phishing. This vulnerability is fixed in 15.26.0 and 14.74.0.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-05-14T15:38Z
Repository: https://github.com/frappe/frappe
Language: Python
Files changed: frappe/tests/test_api.py, frappe/www/login.py
Commit date: 2024-05-02 18:02:18 +0530
Commit message: fix: only redirect to same domain (#26304)

This limits post login redirects to same domain to avoid social engineering attempts.
Diff: diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py
index 193a55c641..913c70169c 100644
--- a/frappe/tests/test_api.py
+++ b/frappe/tests/test_api.py
@@ -5,11 +5,11 @@ from contextlib import contextmanager
 from functools import cached_property
 from random import choice
 from threading import Thread
 from time import time
 from unittest.mock import patch
-from urllib.parse import urljoin
+from urllib.parse import urlencode, urljoin
 
 import requests
 from filetype import guess_mime
 from werkzeug.test import TestResponse
 
@@ -452,10 +452,23 @@ class TestResponse(FrappeAPITestCase):
 		file.insert()
 
 		self.assertEqual(self.get(file.unique_url, {"sid": self.sid}).text, test_content)
 		self.assertEqual(self.get(file.file_url, {"sid": self.sid}).text, test_content)
 
+	def test_login_redirects(self):
+		expected_redirects = {
+			"/app/user": "/app/user",
+			"/app/user?enabled=1": "/app/user?enabled=1",
+			"http://example.com": "/app",  # No external redirect
+			"https://google.com": "/app",
+			"http://localhost:8000": "/app",
+			"http://localhost/app": "http://localhost/app",
+		}
+		for redirect, expected_redirect in expected_redirects.items():
+			response = self.get(f"/login?{urlencode({'redirect-to':redirect})}", {"sid": self.sid})
+			self.assertEqual(response.location, expected_redirect)
+
 
 def generate_admin_keys():
 	from frappe.core.doctype.user.user import generate_keys
 
 	generate_keys("Administrator")
diff --git a/frappe/www/login.py b/frappe/www/login.py
index a1bf04bf06..e4b873b25b 100644
--- a/frappe/www/login.py
+++ b/frappe/www/login.py
@@ -1,8 +1,11 @@
 # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
 
+
+from urllib.parse import urlparse
+
 import frappe
 import frappe.utils
 from frappe import _
 from frappe.auth import LoginManager
 from frappe.rate_limiter import rate_limit
@@ -17,10 +20,11 @@ from frappe.website.utils import get_home_page
 no_cache = True
 
 
 def get_context(context):
 	redirect_to = frappe.local.request.args.get("redirect-to")
+	redirect_to = sanitize_redirect(redirect_to)
 
 	if frappe.session.user != "Guest":
 		if not redirect_to:
 			if frappe.session.data.user_type == "Website User":
 				redirect_to = get_home_page()
@@ -177,5 +181,26 @@ def login_via_key(key: str):
 			_("Not Permitted"),
 			_("The link you trying to login is invalid or expired."),
 			http_status_code=403,
 			indicator_color="red",
 		)
+
+
+def sanitize_redirect(redirect: str | None) -> str | None:
+	"""Only allow redirect on same domain.
+
+	Allowed redirects:
+	- Same host e.g. https://frappe.localhost/path
+	- Just path e.g. /app
+	"""
+	if not redirect:
+		return redirect
+
+	parsed_redirect = urlparse(redirect)
+	if not parsed_redirect.netloc:
+		return redirect
+
+	parsed_request_host = urlparse(frappe.local.request.url)
+	if parsed_request_host.netloc == parsed_redirect.netloc:
+		return redirect
+
+	return None

Vulnerable code: from urllib.parse import urljoin
Fixed code: from urllib.parse import urlencode, urljoin
	def test_login_redirects(self):
		expected_redirects = {
			"/app/user": "/app/user",
			"/app/user?enabled=1": "/app/user?enabled=1",
			"http://example.com": "/app",  # No external redirect
			"https://google.com": "/app",
			"http://localhost:8000": "/app",
			"http://localhost/app": "http://localhost/app",
		}
		for redirect, expected_redirect in expected_redirects.items():
			response = self.get(f"/login?{urlencode({'redirect-to':redirect})}", {"sid": self.sid})
			self.assertEqual(response.location, expected_redirect)


from urllib.parse import urlparse

	redirect_to = sanitize_redirect(redirect_to)


def sanitize_redirect(redirect: str | None) -> str | None:
	"""Only allow redirect on same domain.

	Allowed redirects:
	- Same host e.g. https://frappe.localhost/path
	- Just path e.g. /app
	"""
	if not redirect:
		return redirect

	parsed_redirect = urlparse(redirect)
	if not parsed_redirect.netloc:
		return redirect

	parsed_request_host = urlparse(frappe.local.request.url)
	if parsed_request_host.netloc == parsed_redirect.netloc:
		return redirect

	return None