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

Redirect URI comparison for native OAuth clients (RFC 8252) #107

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

lionick
Copy link
Contributor

@lionick lionick commented Jun 10, 2024

Native Applications have to use ephemeral ports in the redirect_uri. This is something that is already addressed by the RFCs:

Draft-ietf-oauth-security-topics-27: This document therefore advises simplifying the required logic and configuration by using exact redirect URI matching ... The only exception is native apps using a localhost URI: In this case, the authorization server MUST allow variable port numbers as described in [RFC8252], Section 7.3.

RFC 8252: OAuth 2.0 for Native Apps (rfc-editor.org): Native apps that are able to open a port on the loopback network interface without needing special permissions (typically, those on desktop operating systems) can use the loopback interface to receive the OAuth redirect. Loopback redirect URIs use the "http" scheme and are constructed with the loopback IP literal and whatever port the client is listening on. That is, "http://127.0.0.1:{port}/{path}" for IPv4, and "http://[::1]:{port}/{path}" for IPv6.

@lionick lionick requested review from c00kiemon5ter and rohe June 10, 2024 06:21
Copy link
Member

@c00kiemon5ter c00kiemon5ter left a comment

Choose a reason for hiding this comment

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

We have discussed these changes with @lionick
and the implementation works.

The changes are contained within verify_uri function.
The changes come with tests for different cases where this aspect can come up.

The rest of the changes are about making sure the right application type is used, by using a variable rather than duplicating strings. I think that this is better and keeps the codebase tidier.

There is a failing test that is not related to this PR. A dedicated issue highlights this - see #106

@lionick lionick force-pushed the add_native_option_upstream branch from 3731c16 to 237bbea Compare September 16, 2024 08:30
@lionick
Copy link
Contributor Author

lionick commented Sep 16, 2024

@rohe I have also rebased this PR.

@rohe rohe merged commit 42e3b95 into IdentityPython:main Oct 1, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants