Skip to content

Allow loopback redirect URIs using ports as described in RFC8252 #953

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

Merged
merged 8 commits into from
Apr 12, 2021
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
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Jonas Nygaard Pedersen
Jonathan Steffan
Jun Zhou
Kristian Rune Larsen
Paul Dekkers
Paul Oswald
Pavel Tvrdík
Rodney Richardson
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* #524 Restrict usage of timezone aware expire dates to Django projects with USE_TZ set to True.
* #955 Avoid doubling of `oauth2_provider` urls mountpath in json response for OIDC view `ConnectDiscoveryInfoView`.
Breaks existing OIDC discovery output
* #953 Allow loopback redirect URIs with random ports using http scheme, localhost address and no explicit port
configuration in the allowed redirect_uris for Oauth2 Applications (RFC8252)

## [1.5.0] 2021-03-18

Expand Down
5 changes: 5 additions & 0 deletions docs/settings.rst
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ Default: ``["http", "https"]``
A list of schemes that the ``redirect_uri`` field will be validated against.
Setting this to ``["https"]`` only in production is strongly recommended.

For Native Apps the ``http`` scheme can be safely used with loopback addresses in the
Application (``[::1]`` or ``127.0.0.1``). In this case the ``redirect_uri`` can be
configured without explicit port specification, so that the Application accepts randomly
assigned ports.

Note that you may override ``Application.get_allowed_schemes()`` to set this on
a per-application basis.

Expand Down
65 changes: 48 additions & 17 deletions oauth2_provider/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,23 +125,7 @@ def redirect_uri_allowed(self, uri):

:param uri: Url to check
"""
parsed_uri = urlparse(uri)
uqs_set = set(parse_qsl(parsed_uri.query))
for allowed_uri in self.redirect_uris.split():
parsed_allowed_uri = urlparse(allowed_uri)

if (
parsed_allowed_uri.scheme == parsed_uri.scheme
and parsed_allowed_uri.netloc == parsed_uri.netloc
and parsed_allowed_uri.path == parsed_uri.path
):

aqs_set = set(parse_qsl(parsed_allowed_uri.query))

if aqs_set.issubset(uqs_set):
return True

return False
return redirect_to_uri_allowed(uri, self.redirect_uris.split())

def clean(self):
from django.core.exceptions import ValidationError
Expand Down Expand Up @@ -674,3 +658,50 @@ def clear_expired():

access_tokens.delete()
grants.delete()


def redirect_to_uri_allowed(uri, allowed_uris):
"""
Checks if a given uri can be redirected to based on the provided allowed_uris configuration.

On top of exact matches, this function also handles loopback IPs based on RFC 8252.

:param uri: URI to check
:param allowed_uris: A list of URIs that are allowed
"""

parsed_uri = urlparse(uri)
uqs_set = set(parse_qsl(parsed_uri.query))
for allowed_uri in allowed_uris:
parsed_allowed_uri = urlparse(allowed_uri)

# From RFC 8252 (Section 7.3)
#
# Loopback redirect URIs use the "http" scheme
# [...]
# The authorization server MUST allow any port to be specified at the
# time of the request for loopback IP redirect URIs, to accommodate
# clients that obtain an available ephemeral port from the operating
# system at the time of the request.

allowed_uri_is_loopback = (
parsed_allowed_uri.scheme == "http"
and parsed_allowed_uri.hostname in ["127.0.0.1", "::1"]
and parsed_allowed_uri.port is None
)
if (
allowed_uri_is_loopback
and parsed_allowed_uri.scheme == parsed_uri.scheme
and parsed_allowed_uri.hostname == parsed_uri.hostname
and parsed_allowed_uri.path == parsed_uri.path
) or (
parsed_allowed_uri.scheme == parsed_uri.scheme
and parsed_allowed_uri.netloc == parsed_uri.netloc
Copy link
Contributor

Choose a reason for hiding this comment

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

at first it looks like both of these branches could be merged! And for scheme and path it can, but netloc is actually inclusive of the port (it's roughly login info + hostname + port) , and for loopback IPs we don't want to be requiring the port to be the same (the whole point is wanting to support ephemeral IPs)

and parsed_allowed_uri.path == parsed_uri.path
):

aqs_set = set(parse_qsl(parsed_allowed_uri.query))
if aqs_set.issubset(uqs_set):
return True

return False
21 changes: 21 additions & 0 deletions tests/test_oauth2_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from django.test import RequestFactory, TestCase

from oauth2_provider.backends import get_oauthlib_core
from oauth2_provider.models import redirect_to_uri_allowed
from oauth2_provider.oauth2_backends import JSONOAuthLibCore, OAuthLibCore


Expand Down Expand Up @@ -110,3 +111,23 @@ def test_validate_authorization_request_unsafe_query(self):

oauthlib_core = get_oauthlib_core()
oauthlib_core.verify_request(request, scopes=[])


@pytest.mark.parametrize(
"uri, expected_result",
# localhost is _not_ a loopback URI
[
("http://localhost:3456", False),
# only http scheme is supported for loopback URIs
("https://127.0.0.1:3456", False),
("http://127.0.0.1:3456", True),
("http://[::1]", True),
("http://[::1]:34", True),
],
)
def test_uri_loopback_redirect_check(uri, expected_result):
allowed_uris = ["http://127.0.0.1", "http://[::1]"]
if expected_result:
assert redirect_to_uri_allowed(uri, allowed_uris)
else:
assert not redirect_to_uri_allowed(uri, allowed_uris)