Consider setting 403 Forbidden on failed authBasic()

[ramsey]

Consider the following code example using the authBasic() routine:

<?php
$r3 = new \Respect\Rest\Router();

$r3->get('/', function() {
    echo 'Hello';
})->authBasic('My Realm', function($user, $pass) {
    return $user === 'admin' && $pass === 'pass';
});

When I query this without providing basic auth headers, I get the correct 401 Authorization Required response that I expect:

HTTP/1.1 401 Authorization Required
Date: Tue, 15 May 2012 22:55:38 GMT
Server: Apache/2.2.20 (Ubuntu)
X-Powered-By: PHP/5.4.0-3~oneiric+4
WWW-Authenticate: Basic realm="My Realm"
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 20
Content-Type: text/html

However, if I do provide authentication headers, but the username and password are incorrect, causing the authBasic() routine to return false, then I get back a 200 OK response with no content.

HTTP/1.1 200 OK
Date: Tue, 15 May 2012 22:57:08 GMT
Server: Apache/2.2.20 (Ubuntu)
X-Powered-By: PHP/5.4.0-3~oneiric+4
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 20
Content-Type: text/html

There is no security concern here, since you are properly blocking the client from receiving the content, but the 200 OK doesn't seem appropriate. Perhaps a 403 Forbidden is more appropriate by default, or we can continue to leave it up to the application developer.

Here's an example of how I'm accomplishing this right now:

<?php
$r3 = new \Respect\Rest\Router();

$r3->get('/', function() {
    echo 'Hello';
})->authBasic('My Realm', function($user, $pass) {
    if ($user === 'admin' && $pass === 'pass') {
        return true;
    } else {
        header('HTTP/1.1 403 Forbidden');
        return false;
    }   
});

[ramsey]

Unfortunately, I just realized my example has the negative effect of always setting 403 Forbidden in the response if you do not provide auth credentials, so the 401 Authorization Required is never set. With a bit more logic, though, you can set the 403 only if a $user and $pass are present but fail to authorize.

[alganet]

You're right! Shouldn't be hard to implement. I'll do this by the end of this week =)

[alganet]

I've reconsidered this after reading the RFC2616 again. Section 10.4.2 says (about 401):

   If the request already included Authorization credentials, then the 401
   response indicates that authorization has been refused for those
   credentials. 

Section 10.4.4 also says that (about 403):

   The server understood the request, but is refusing to fulfill it.
   Authorization will not help and the request SHOULD NOT be repeated.

I'm more towards returning 401 again on failure instead of 403. What do you guys think? @ramsey @augustohp

[mta59066]

Yes, I think 401 would be the correct return code. Here is how I did it.

public function by(Request $request, $params)
{
    $result = false;
    if (isset($_SERVER['HTTP_AUTHORIZATION']))
        $result = call_user_func_array(
            $this->callback,
            explode(':', base64_decode(substr($_SERVER['HTTP_AUTHORIZATION'], 6))
        ));
    elseif (isset($_SERVER['PHP_AUTH_USER']))
        $result = call_user_func($this->callback, $_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);

    if($result !== true) {
        header('HTTP/1.1 401');
        header("WWW-Authenticate: Basic realm=\"{$this->realm}\"");
        return $result;
    }
}

[mta59066]

Actually 401 needs to be the return code if authentication was tried and not successful. If authentication was not even tried, than perhaps should be left to the application developer like what ramsey suggested in the initial post, but maybe the expected behavior by the application developer is to end up at 403? Not sure how to implement that on AuthBasic.php.

[ramsey]

I'm switching my original request and agreeing with everyone that the return code should be 401, rather than 403, for failed authentication. As @mta59066 has suggested, I've changed my own code to return 401 and WWW-Authentication headers.

[nickl-]

Does this mean the issue is resolved and we can close it? If not does anyone care to roll a patch and make a pull request, please. If AuthBasic is in need of some TLC lets do that.

[ramsey]

The issue wasn't resolved, so I quickly created a patch and issued a pull request. :-)

[nickl-]

Awesome Tx! Will have a look see... you rock! =)