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

ISS 983 Allow for side redirects with new configuration #984

Closed
Closed
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
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ Features & Improvements
+++++++++++++++++++++++
- (:issue:`956`) Add support for changing registered user's email (:py:data:`SECURITY_CHANGE_EMAIL`).
- (:pr:`xxx`) Change default password hash to argon2 (was bcrypt). See below for details.
- (:issue:`983`) Add new configuration for (:py:data:`SECURITY_REDIRECT_MATCH_SUBDOMAINS`) to allow for
stricter redirect matching.

Fixes
+++++
Expand Down
20 changes: 20 additions & 0 deletions docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,26 @@ These configuration keys are used globally across all features.

.. versionadded:: 4.0.0

.. py:data:: SECURITY_REDIRECT_MATCH_SUBDOMAINS

This attribute specifies a list of permitted subdomains that can be the
target of redirections. Only the subdomains included in this list will be
authorized to receive redirects.

For enhanced security, configuring this setting will override the default
setting that permits all subdomains of SERVER_NAME. This is based on the
assumption that you desire to have precise control over the host names that
are permitted. It is essential that you provide a comprehensive list of all
the subdomains you intend to allow access to.

This setting requires that :py:data:`SECURITY_REDIRECT_ALLOW_SUBDOMAINS` is set to ``True``.

Examples: ``['auth.example.com']`` will only allow the auth.example.com subdomain to be redirected to.

Default: ``[]``.

.. versionadded:: 5.4.X

.. py:data:: SECURITY_CSRF_PROTECT_MECHANISMS

Authentication mechanisms that require CSRF protection.
Expand Down
1 change: 1 addition & 0 deletions flask_security/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@
"REDIRECT_HOST": None,
"REDIRECT_BEHAVIOR": None,
"REDIRECT_ALLOW_SUBDOMAINS": False,
"REDIRECT_MATCH_SUBDOMAINS": [],
"FORGOT_PASSWORD_TEMPLATE": "security/forgot_password.html",
"LOGIN_USER_TEMPLATE": "security/login_user.html",
"REGISTER_USER_TEMPLATE": "security/register_user.html",
Expand Down
59 changes: 49 additions & 10 deletions flask_security/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import hmac
import time
import typing as t
from urllib.parse import parse_qsl, quote, urlsplit, urlunsplit, urlencode
from urllib.parse import parse_qsl, quote, urlsplit, urlunsplit, urlencode, urlparse
import urllib.request
import urllib.error
import warnings
Expand Down Expand Up @@ -614,22 +614,61 @@ def validate_redirect_url(url: str) -> bool:
return False
url_next = urlsplit(url)
url_base = urlsplit(request.host_url)

if (url_next.netloc or url_next.scheme) and url_next.netloc != url_base.netloc:
base_domain = current_app.config.get("SERVER_NAME")
if (
config_value("REDIRECT_ALLOW_SUBDOMAINS")
and base_domain
and (
url_next.netloc == base_domain
or url_next.netloc.endswith(f".{base_domain}")
)
):
return True

# Some Flask implementations may not have SERVER_NAME set
if not base_domain:
base_domain = request.host
tld = get_top_level_domain(base_domain)

# Are we allowed to redirect to other hosts outside of ourself?
if config_value("REDIRECT_ALLOW_SUBDOMAINS"):
# Capture any allowed subdomains
allowed_subdomains = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is getting close! and much simpler. However - I still think this is enabling redirects to arbitrary hosts - not just subdomains. I don't think your use case needs that and would be a bigger/more risky change. To that end REDIRECT_MATCH_SUBDOMAINS should really be a list of subdomains - as in ["auth", "app", "mktg.app"] and this check should be something like:

if any(f"{n}.{base_domain}" == url_next.netloc for n in allowed_subdomains):

Of course requiring base_domain (e.g. SERVER_NAME) to be set and should always allow url_next.netloc == base_domain right?

Does that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

Hopefully the latest change resolves this, and my explanation further down explains the desired functionality.


if config_value("REDIRECT_MATCH_SUBDOMAINS"):
allowed_subdomains = config_value("REDIRECT_MATCH_SUBDOMAINS")

# If we have a list of allowed subdomains, check if the next
# URL is in the list
if (
mchineboy marked this conversation as resolved.
Show resolved Hide resolved
len(allowed_subdomains) > 0
and url_next.netloc in allowed_subdomains
and url_next.netloc.endswith(f".{tld}")
):
return True
elif (
len(allowed_subdomains) == 0
and base_domain
and (
url_next.netloc == base_domain
or url_next.netloc.endswith(f".{base_domain}")
)
):
# If we don't have a list of allowed subdomains, check if the
# next URL is the same as the base domain or a subdomain of the
# base domain. This is the original behavior.
return True
else:
return False
else:
return False

return True


def get_top_level_domain(url):
# Extract the hostname from the URL if it's not already a hostname
hostname = urlparse(url).hostname or url
# Split the hostname into parts
hostname_parts = hostname.split(".")
# The TLD is the last part
tld = hostname_parts[-1] if len(hostname_parts) > 1 else ""
return tld


def get_post_action_redirect(
config_key: str, next_loc: FlaskForm | MultiDict | dict | None
) -> str:
Expand Down
41 changes: 41 additions & 0 deletions tests/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,54 @@ def test_authenticate_with_invalid_subdomain_next(app, client, get_message):
assert get_message("INVALID_REDIRECT") in response.data


def test_authenticate_with_subdomain_match_next_strict(app, client, get_message):
app.config["SERVER_NAME"] = "lp.com"
app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True
app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["sub.lp.com"]
data = dict(email="[email protected]", password="password")
response = client.post("/login?next=http://sub.lp.com", data=data)
assert response.status_code == 302


def test_authenticate_with_subdomain_match_next_strict_invalid(
app, client, get_message
):
app.config["SERVER_NAME"] = "lp.com"
app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True
app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["foo.lp.com"]
data = dict(email="[email protected]", password="password")
response = client.post("/login?next=http://sub.lp.com", data=data)
assert get_message("INVALID_REDIRECT") in response.data


def test_authenticate_with_subdomain_next_default_config(app, client, get_message):
app.config["SERVER_NAME"] = "lp.com"
data = dict(email="[email protected]", password="password")
response = client.post("/login?next=http://sub.lp.com", data=data)
assert get_message("INVALID_REDIRECT") in response.data


def test_authenticate_with_subdomain_next_invalid_domain(app, client, get_message):
app.config["SERVER_NAME"] = "lp.com"
app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True
app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = [
"github.com",
"something.github.com",
]
data = dict(email="[email protected]", password="password")
response = client.post("/login?next=http://sub.lp.net", data=data)
assert get_message("INVALID_REDIRECT") in response.data


def test_authenticate_with_subdomain_next_app_context(app, client, get_message):
app.config["SERVER_NAME"] = "application.lp.com"
app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True
app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["sub.lp.com"]
data = dict(email="[email protected]", password="password")
response = client.post("/login?next=http://sub.lp.com", data=data)
assert response.status_code == 302


def test_authenticate_case_insensitive_email(app, client):
response = authenticate(client, "[email protected]", follow_redirects=True)
assert b"Welcome [email protected]" in response.data
Expand Down
Loading