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.

Sign up for GitHub

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

Add localhost to allowed loopback addresses #1423

Closed
wants to merge 6 commits into from
Closed

Add localhost to allowed loopback addresses #1423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c77092f
Add localhost to allowed loopback addresses for redirect URIs
Kanellaman May 21, 2024
f083d92
Add tests
Kanellaman May 21, 2024
dfb29c6
Fixed an issue in tests
Kanellaman May 21, 2024
99f083b
Removed unecessary test
Kanellaman May 21, 2024
ae54867
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 21, 2024
e7ef89c
Update documentation
Kanellaman May 21, 2024
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: 1 addition & 1 deletion docs/settings.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ 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
Application (``[::1]`` or ``127.0.0.1`` or ``localhost``). In this case the ``redirect_uri`` can be
Copy link
Member

Choose a reason for hiding this comment

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

Per comment referencing RFC8252, please change this to a setting with the default as is without localhost.

configured without explicit port specification, so that the Application accepts randomly
assigned ports.

Expand Down
2 changes: 1 addition & 1 deletion oauth2_provider/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ def redirect_to_uri_allowed(uri, allowed_uris):

allowed_uri_is_loopback = (
parsed_allowed_uri.scheme == "http"
and parsed_allowed_uri.hostname in ["127.0.0.1", "::1"]
and parsed_allowed_uri.hostname in ["127.0.0.1", "::1", "localhost"]
Copy link
Member

Choose a reason for hiding this comment

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

Please add a setting along these lines"

Suggested change
and parsed_allowed_uri.hostname in ["127.0.0.1", "::1", "localhost"]
and parsed_allowed_uri.hostname in settings.LOOPBACK_URIS

and parsed_allowed_uri.port is None
)
if (
Expand Down
5 changes: 2 additions & 3 deletions tests/test_oauth2_backends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,8 @@ def test_validate_authorization_request_unsafe_query(self):

@pytest.mark.parametrize(
"uri, expected_result",
# localhost is _not_ a loopback URI
Copy link
Member

Choose a reason for hiding this comment

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

Ugh I'm sorry I put you through creating this PR, but according to RFC8252:

While redirect URIs using localhost (i.e.,
"http://localhost:{port}/{path}") function similarly to loopback IP
redirects described in Section 7.3, the use of localhost is NOT
RECOMMENDED. Specifying a redirect URI with the loopback IP literal
rather than localhost avoids inadvertently listening on network
interfaces other than the loopback interface. It is also less
susceptible to client-side firewalls and misconfigured host name
resolution on the user's device.

Given this, I think the default should remain as is, perhaps with a setting added to allow one to override the default to allow localhost.

Copy link
Author

@Kanellaman Kanellaman May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ugh I'm sorry I put you through creating this PR

Oh it's fine, no worries at all. Thanks for taking the time looking at it!

[
("http://localhost:3456", False),
("http://localhost:3456", True), # localhost is supported
# only http scheme is supported for loopback URIs
("https://127.0.0.1:3456", False),
("http://127.0.0.1:3456", True),
Expand All @@ -216,7 +215,7 @@ def test_validate_authorization_request_unsafe_query(self):
],
)
def test_uri_loopback_redirect_check(uri, expected_result):
allowed_uris = ["http://127.0.0.1", "http://[::1]"]
allowed_uris = ["http://127.0.0.1", "http://[::1]", "http://localhost"]
if expected_result:
assert redirect_to_uri_allowed(uri, allowed_uris)
else:
Expand Down
6 changes: 4 additions & 2 deletions tests/test_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
@pytest.mark.usefixtures("oauth2_settings")
class TestValidators(TestCase):
def test_validate_good_uris(self):
validator = RedirectURIValidator(allowed_schemes=["https"])
validator = RedirectURIValidator(allowed_schemes=["https", "http"])
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you changed this to add http.

good_uris = [
"https://example.com/",
"https://example.org/?key=val",
Expand All @@ -17,20 +17,22 @@ def test_validate_good_uris(self):
"https://1.1.1.1",
"https://127.0.0.1",
"https://255.255.255.255",
"http://localhost",
]
for uri in good_uris:
# Check ValidationError not thrown
validator(uri)

def test_validate_custom_uri_scheme(self):
validator = RedirectURIValidator(allowed_schemes=["my-scheme", "https", "git+ssh"])
validator = RedirectURIValidator(allowed_schemes=["my-scheme", "https", "git+ssh", "http"])
good_uris = [
"my-scheme://example.com",
"my-scheme://example",
"my-scheme://localhost",
"https://example.com",
"HTTPS://example.com",
"git+ssh://example.com",
"http://localhost",
]
for uri in good_uris:
# Check ValidationError not thrown
Expand Down
Loading