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

Clarify redirection proposal. See #117. #118

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ioggstream
Copy link
Contributor

This draft PR

  • clarifies redirection and request/response terminology

or in the authorization server's metadata document ({{RFC8414}}).

The endpoint URI MAY include an "application/x-www-form-urlencoded"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it's a location, it's an URL, right?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is in there to make sure clients don't make assumptions that the authorization server URL might not contain a query string. Also note that just below this there's a limit that the URL can't contain a fragment, so I think it's worth pointing out still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is in there to make sure clients don't make assumptions that the authorization server URL might not contain a query string

I don't understand how this is related to the query string. IIUC, both URI and URL can have a query component:

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but without an explicit mention, I suspect a lot of people would assume the URL would not contain a query string, especially because it's pretty uncommon for it to do so in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without an explicit mention

we explicitly mention the query component in the lines below - that are not removed - so people are told that URLs have query component.

@aaronpk


Request and response parameters
defined by this specification MUST NOT be included more than once.
Parameters sent without a value MUST be treated as if they were
omitted from the request.

An authorization server that redirects a request potentially containing
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this section applies to the authorization endpoints, not to all the authorization server, right?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it wouldn't refer to the token endpoint, only to endpoints that a user might interact with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So is it correct in line 992 to use "authorization endpoint" ?

An authorization server that redirects a request potentially containing
user credentials MUST avoid forwarding these user credentials accidentally
An authorization endpoints that redirects a request potentially containing
user credentials MUST NOT forward these credentials to untrusted parties
Copy link
Member

Choose a reason for hiding this comment

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

I don't think adding "untrusted parties" here is beneficial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we say that "it MUST NOT forward these credentials" never, in any case?

Copy link
Member

Choose a reason for hiding this comment

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

That gets a bit tricky, because it might need to "forward" the credentials to itself, and then what are the boundaries of its "self" becomes challenging to describe concisely. I think that's the reason the original text said "accidentally".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronpk "MUST avoid to accidentally" sounds like SHOULD CONSIDER. IMHO does not provide any interoperability guidance because it is related to an unintended behavior.

In general, I'd track down RFC6919-sounding statements in this document and address them before AD / IESG review will ask us to do it.

I don't think adding "untrusted parties" here is beneficial

imho helps avoiding the fallacies of "accidentally", since it suggests the AS to internally define a trusted boundary for credentials.

draft-ietf-oauth-v2-1.md Outdated Show resolved Hide resolved

Request and response parameters
defined by this specification MUST NOT be included more than once.
Parameters sent without a value MUST be treated as if they were
omitted from the request.

An authorization server that redirects a request potentially containing
user credentials MUST avoid forwarding these user credentials accidentally
An authorization endpoint that redirects a request potentially containing
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronpk I reworded it removing normative language. This is because we cannot forbid to accidentally do something ;) instead it is better to suggest how to mitigate the risk.

@ioggstream ioggstream marked this pull request as ready for review July 21, 2022 09:50
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.

2 participants