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

Don't show redirect_uri on consent screen if not HTTP #1736

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hughns
Copy link
Member

@hughns hughns commented Sep 11, 2023

Because we don't know if it is validated by the OS at all and so could be misleading to the user

Because we don't know if it is validated by the OS at all and so could be misleading to the user
@cloudflare-pages
Copy link

cloudflare-pages bot commented Sep 11, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4010732
Status: ✅  Deploy successful!
Preview URL: https://2bbefd94.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://hughns-hide-redirect-uri-if.matrix-authentication-service-docs.pages.dev

View logs

Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

For now it is the only bit of minuscule trust we can put on that screen, and I don't want to get rid of it, even though it is flawed.
I would suggest showing at the very least the client_uri, since:

  • the OIDC dynamic reg spec says about it:

    If present, the server SHOULD display this URL to the End-User in a followable fashion.

  • the RFC for native apps says (and we're enforcing it):

    When choosing a URI scheme to associate with the app, apps MUST use a URI scheme based on a domain name under their control, expressed in reverse order

Moreover, EXI is working towards using universal links anyway, so for Element clients it becomes completely irrelevant

@hughns
Copy link
Member Author

hughns commented Sep 11, 2023

When choosing a URI scheme to associate with the app, apps MUST use a URI scheme based on a domain name under their control, expressed in reverse order

The problem is that nothing is validating that the given domain is under control of the app owner. The app can choose any domain and give a matching client_uri and reverse redirect_uri.

As such, displaying a reverse domain is of zero benefit.

What is the trust that showing it brings?

Where the value is validated I do see benefit in showing it which is why my PR shows it if it is an HTTP URI.

Copy link
Collaborator

@pmaier1 pmaier1 left a comment

Choose a reason for hiding this comment

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

👍 from product side

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.

None yet

3 participants