Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix open redirect with backslash URL #94

Merged
merged 5 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
Changelog
=========

Version 0.5.6
-------------

- Reject invalid ``next`` URLs with backslashes that could be used to trick browsers into
redirecting to an otherwise disallowed host when doing client-side redirects

Version 0.5.5
-------------

Expand Down
7 changes: 6 additions & 1 deletion flask_multipass/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,14 @@ def validate_next_url(self, url):
If you override this and want to allow more hosts, make sure to use
a whitelist of trusted hosts to avoid creating an open redirector.
"""
url_info = urlsplit(url)
# Browsers treat backslashes like forward slashes, while urllib doesn't.
# Since we just want to validate scheme and netloc here, we normalize
# slashes to those recognized by urllib.
url_info = urlsplit(url.replace('\\', '/'))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if there are cases where the backslash has a function like in older/unusual browsers (e.g. some Japanese institutions still use Internet Explorer :( ).
So I didn't use replace() but yes it seems logical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in case of Indico there aren't any cases where we'd ever expect backslashes in the next URL, and I'd expect anything else using this library to not do so either.

PS: Indico no longer supports IE for many years now, so if things there break even more, so be it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahah wise decision 👍

if url_info.scheme and url_info.scheme not in {'http', 'https'}:
return False
if url_info.scheme and not url_info.netloc:
return False
return not url_info.netloc or url_info.netloc == request.host

def process_login(self, provider=None):
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = 'Flask-Multipass'
version = '0.5.5'
version = '0.5.6'
description = 'A pluggable solution for multi-backend authentication with Flask'
readme = 'README.rst'
license = 'BSD-3-Clause'
Expand Down
4 changes: 4 additions & 0 deletions tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ def test_next_url_invalid():
('//evil.com:80', False),
('http://evil.com', False),
('https://evil.com', False),
(r'http:\\evil.com', False),
(r'http:\evil.com', False),
(r'https:\\evil.com', False),
(r'https:\evil.com', False),
('javascript:alert("eeeeeeeevil")', False),
))
def test_validate_next_url(url, valid):
Expand Down