Skip to content

Commit

Permalink
fix: only redirect to same domain (#26304)
Browse files Browse the repository at this point in the history
This limits post login redirects to same domain to avoid social engineering attempts.
  • Loading branch information
ankush committed May 2, 2024
1 parent 4bfcccf commit 65b3c42
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 1 deletion.
15 changes: 14 additions & 1 deletion frappe/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
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
Expand Down Expand Up @@ -454,6 +454,19 @@ def test_download_private_file_with_unique_url(self):
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
Expand Down
25 changes: 25 additions & 0 deletions frappe/www/login.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# 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 _
Expand All @@ -19,6 +22,7 @@

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:
Expand Down Expand Up @@ -179,3 +183,24 @@ def login_via_key(key: str):
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

0 comments on commit 65b3c42

Please sign in to comment.