-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: main
Are you sure you want to change the base?
Conversation
or in the authorization server's metadata document ({{RFC8414}}). | ||
|
||
The endpoint URI MAY include an "application/x-www-form-urlencoded" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
draft-ietf-oauth-v2-1.md
Outdated
|
||
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" ?
draft-ietf-oauth-v2-1.md
Outdated
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
9364769
to
03d5c09
Compare
|
||
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 |
There was a problem hiding this comment.
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.
This draft PR