Add interceptor to set authentication from URI #209
Conversation
| if ($userInfo !== '') { | ||
| [$username, $password] = \explode(':', $userInfo) + ['', null]; | ||
| $redirectUri = $redirectUri->withUserInfo($username, $password); | ||
| } |
There was a problem hiding this comment.
Maybe we should just copy over any Authorization header here?
There was a problem hiding this comment.
In the current stack, the authorization header hasn't yet been added from the user info. I thought that it made more sense to do so as a NetworkInterceptor (so run after redirects), but it could be moved to be an ApplicationInterceptor instead.
There was a problem hiding this comment.
Given that using user info is deprecated, I'm not sure what should be supported. Maybe we should explicitly forbid the user info component in URIs instead.
There was a problem hiding this comment.
See also RFC 7230:
The URI generic syntax for authority also includes a deprecated
userinfo subcomponent ([RFC3986], Section 3.2.1) for including user
authentication information in the URI. Some implementations make use
of the userinfo component for internal configuration of
authentication information, such as within command invocation
options, configuration files, or bookmark lists, even though such
usage might expose a user identifier or password. A sender MUST NOT
generate the userinfo subcomponent (and its "@" delimiter) when an
"http" URI reference is generated within a message as a request
target or header field value. Before making use of an "http" URI
reference received from an untrusted source, a recipient SHOULD parse
for userinfo and treat its presence as an error; it is likely being
used to obscure the authority for the sake of phishing attacks.
There was a problem hiding this comment.
Maybe we should explicitly forbid the user info component in URIs instead.
This might be a better option, with an error message suggesting to set the Authorization header instead.
Do you have an opinion @enumag?
There was a problem hiding this comment.
I wasn't aware it was deprecated. With this information in mind, I agree with forbidding the user info component outright and recommending the authorization header instead. 👍
Implements #200.