OAuthServerException::invalidClient() produces browser login prompt

[symbioquine]

In experimenting with the Drupal's simple_oauth module, I have come across some surprising behavior.

Suppose site A uses thephpleague/oauth2-server and site B wants to connect to site A on behalf of site A's users. If the developers of site B use an oauth client id which is missing from site A, their users will get a HTTP error code 401 access denied - browsers respond to that code by showing an authentication prompt like this;

This doesn't make sense because nothing site A's users can enter here will fix the problem in site B's configuration.

The code which returns an HTTP code 401 is src/Exception/OAuthServerException.php#L154;

/**
 * Invalid client error.
 *
 * @param ServerRequestInterface $serverRequest
 *
 * @return static
 */
public static function invalidClient(ServerRequestInterface $serverRequest)
{
    $exception = new static('Client authentication failed', 4, 'invalid_client', 401);

    $exception->setServerRequest($serverRequest);

    return $exception;
}

According to RFC 6749 section-4.1.2.1;

4.2.2.1. Error Response

If the request fails due to a missing, invalid, or mismatching redirection URI, or if the client identifier is missing or invalid, the authorization server SHOULD inform the resource owner of the error and MUST NOT automatically redirect the user-agent to the invalid redirection URI.

To effectively inform the resource owners (site A's users), it would make more sense to return an HTTP 400 error and a message like;"Request to access resources by invalid client 'example_client_id'."

[Sephster]

I see your concern here but the server is acting as expected in the spec. Because the client identifier sent to the server is invalid this triggers an invalid_client error response as the client is unknown and should issue a 401 response.

Surfacing this information to the resource owner should, in most situations, not be useful. The resource owner would likely not have access to site B's client code to make any meaningful adjustments but I suppose they could inform the site B admin. However, I agree with you that an endless login prompt could well be confusing.

This is the section in the RFC we are following:

invalid_client Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method). The authorization server MAY return an HTTP 401 (Unauthorized) status code to indicate which HTTP authentication schemes are supported. If the client attempted to authenticate via the "Authorization" request header field, the authorization server MUST respond with an HTTP 401 (Unauthorized) status code and include the "WWW-Authenticate" response header field matching the authentication scheme used by the client.

In the response from the server, what is your www-authenticate header? It should be bearer in most situations. I'm surprised an http basic login is appearing for you.

[symbioquine]

Hi @Sephster, thanks for the detailed response! I hadn't dug into the browser basic auth behavior (or the RFC) deeply enough it seems... :)

I can confirm that if the server returns a HTTP 401 - either without the WWW-Authenticate header, or with WWW-Authenticate: Bearer realm="OAuth" - then the browser does not show the basic auth dialog.

Only when the header is WWW-Authenticate: Basic does that dialog appear.

Test cases for those assertions (handles a single request to http://localhost:1500);

Just plain HTTP 401:

echo -e "HTTP/1.1 401 Unauthorized\n\n Something went wrong" | nc -l -c -vv -p 1500 localhost

HTTP 401 with WWW-Authenticate: Bearer header:

echo -e "HTTP/1.1 401 Unauthorized\nWWW-Authenticate: Bearer realm=\"OAuth\"\n\n Something went wrong" | nc -l -c -vv -p 1500 localhost

HTTP 401 with WWW-Authenticate: Basic header:

echo -e "HTTP/1.1 401 Unauthorized\nWWW-Authenticate: Basic realm=\"OAuth\"\n\n Something went wrong" | nc -l -c -vv -p 1500 localhost

In my case, this was caused by two coinciding factors;

1. Drupal Core is vended with a .htaccess file which includes the following lines;

  # Make sure Authorization HTTP header is available to PHP
  # even when running as CGI or FastCGI.
  RewriteRule ^ - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]

2. The thephpleague/oauth2-server code to generate the invalid client exception WWW-Authenticate header is imprecise in how it detects a meaningful Authorization header value in the request;

        if ($this->errorType === 'invalid_client' && $this->serverRequest->hasHeader('Authorization') === true) {
            $authScheme = \strpos($this->serverRequest->getHeader('Authorization')[0], 'Bearer') === 0 ? 'Bearer' : 'Basic';

            $headers['WWW-Authenticate'] = $authScheme . ' realm="OAuth"';
        }

In my case, the Authorization header value was an empty string as a result of the .htaccess configuration.

It might be worth re-evaluating the logic above to make sure it really makes sense to return a WWW-Authenticate: Basic realm="OAuth" header in this case, given the confusing user experience of that a basic auth prompt provides in this case. It's pretty easy to imagine a multitude of server/reverse-proxy configurations which could result in an empty Authorization header value.

[Sephster]

Thanks @symbioquine. That's really interesting. Do you know why Drupal is forcing an empty Authorization header out of interest? I can't think of a reason for this.

I think this is probably not standard procedure so might be better fixed in the Drupal simple oauth module but would welcome any further insight you have on the reasoning for this behaviour in case my assumptions here are incorrect. Thanks so much

[symbioquine]

According to the git blame that rewrite rule was added with this change which in turn was associated with this issue.

There seem to be a lot of related discussions, but the gist of it is that the rewrite rule is the lowest common denominator way to ensure that the Authorization header is passed through in CGI/FCGI environments while taking on the least complexity/dependencies - the trade-off of course being that the Authorization header is getting populated with the empty string (when no Authorization header was present in the request) as far as PHP is concerned.

References;

• https://www.reddit.com/r/laravel/comments/9jvcwd/what_does_this_htaccess_rule_do_from_laravels/
• https://www.reddit.com/r/PHP/comments/3raldh/til_that_apachefastcgi_strips_out_the/
• https://www.php.net/manual/en/features.http-auth.php

I tend to agree that the behavior of populating an empty string for the Authorization header is suboptimal, but it's starting to look like the ship has pretty much sailed on fixing that at the source given the amount of places that .htaccess rewrite rule seems to be recommended. Surely it's better for thephpleague/oauth2-server to "just work" in environments it might typically run under?

If so, I'd recommend the following change revision to OAuthServerException.php;

-        if ($this->errorType === 'invalid_client' && $this->serverRequest->hasHeader('Authorization') === true) {
+        if ($this->errorType === 'invalid_client' && $this->serverRequest->hasHeader('Authorization') === true && !empty($this->serverRequest->getHeader('Authorization'))) {
             $authScheme = \strpos($this->serverRequest->getHeader('Authorization')[0], 'Bearer') === 0 ? 'Bearer' : 'Basic';

             $headers['WWW-Authenticate'] = $authScheme . ' realm="OAuth"';
         }

Happy to send a pull request if that sounds at all reasonable...

[Sephster]

Great find and thanks so much for providing so much information! I would very much welcome a fix as it seems this rewrite rule is pretty common.

Because there would be three conditions in the if statement, it might be better to put the authorization header check in a private method and add a comment about why we are doing this.

If you don't have time to do this, no worries at all. I will get to this on the weekend. Thank you so much for your help again. Really appreciated

[symbioquine]

Thanks @Sephster! I've opened https://github.com/thephpleague/oauth2-server/pull/1170 with the proposed fix - looking forward to any feedback...

[bradjones1]

Drupal simple_oauth maintainer here, let me know if I can be a resource on this. (I'm bradjones1 on Drupal Slack, too.)