Fix PHP Warning if no colon is present in decoded basic-auth payload #9

BreiteSeite · 2018-07-24T10:00:03Z

This fixes #8

If the authorization header contains a base64 encoded string, that contains no colon when decoded, a PHP Warning is thrown in PHP 7.2. The new/fixed behaviour is that null is returned as authentication is not possible.

I also refactored the tests along with the authentication method. The test now include much more test-cases with increased readability by using named data-providers in PHPUnit.

I also added a test-case for a password that contains colons which also did not work before. I did not open a issue for this and fixed it silently in this PR.

Are you fixing a bug?
- Detail how the bug is invoked currently.
- Detail the original, incorrect behavior.
- Detail the new, expected behavior.
- Base your feature on the master branch, and submit against that branch.
- Add a regression test that demonstrates the bug, and proves the fix.
- Add a CHANGELOG.md entry for the fix.

* use dataProviders * added more test-cases * added return-type hints * remove unnecessary assertion

* use strict parameter of base64_decode instead of maintaining list of valid characters in regexp * use limit in explode() to allow passwords with colons * check if base64_decode failed * check that array index is set

BreiteSeite · 2018-07-24T10:04:15Z

src/BasicAccess.php

            return null;
        }

-        if (! preg_match('/Basic (?P<credentials>[a-zA-Z0-9\+\/\=]+)/', $authHeader[0], $match)) {
+        if (! preg_match('/Basic (?P<credentials>.+)/', $authHeader[0], $match)) {


I think its not good to maintain a list of valid base64 character in the regexp, because this list is also already maintained in PHP itself and checked with the strict argument in base64_decode()

Don't assume PHP is checking the header values. The code may be run under different SAPIs or alternative process managers that may or may not do such checks (e.g. Swoole, ReactPHP).

Can you elaborate what you mean exactly here? Are you implying, that the strict argument from base64_decode() may be a no-op in some environments? Because i can't see that currently. https://github.com/php/php-src/blob/a5e80b22e11c2db7fd5ff07b5b7a28f80745e89b/ext/standard/base64.c#L115

froschdesign · 2018-07-24T10:06:12Z

src/BasicAccess.php

@@ -53,15 +53,31 @@ public function __construct(
    public function authenticate(ServerRequestInterface $request) : ?UserInterface
    {
        $authHeader = $request->getHeader('Authorization');
-        if (empty($authHeader)) {
+        if (! isset($authHeader[0])) {


Is isset correct or do mean array_key_exists?

@froschdesign i guess both would work here. I can change it to array_key_exists if you like, but why would i want the execution to continue here, if it's (somehow) containing null?

From the PSR-7 spec for getHeader():

@return string[] An array of string values as provided for the given header. If the header does not appear in the message, this method MUST return an empty array.

As such, you will always have an array using this method. The question is if the array is empty; this is why we were using empty($authHeader) previously. I see no reason to change that.

@weierophinney i agree with that. However, just because it's not-empty strictly does not implies that array index 0 is set. thats why i explicitly check for this, because index 0 is accessed later in the code. If someone manipulated the headers, it could be not empty while the "first" header-value is found in index 1 or 2 or whatever.

Do you still want me to revert it to the check with empty()?

According to RFC 7235, the Authorization header MUST only be a single header; it cannot be repeated. As such, $request->getHeader('Authorization') can only ever return 0 or 1 item in the array.

We can also use $request->getHeaderLine('Authorization'), which will return a string with the header contents, or an empty string if the header is not found.

As such, we have two options:

$authHeader = $request->getHeader('Authorization'); if (empty($authHeader)) { return null; } $value = array_shift($authHeader);

or

$value = $request->getHeaderLine('Authorization'); if ('' === $value) { return null;

And from there, use $value in the regex when retrieving the credentials from the value.

@weierophinney Ah. You would avoid that by using array_shift(). I see.

I changed the code to the getHeaderLine() suggestion as it's more neat and readable in my opinion. Just pushed it. Thank you.

@weierophinney actually, after giving it second thoughts, i changed it back to use getHeader() to be easily able to early-exit in case someone sends multiple authorization headers.

After reading RFC 2617, i saw that user-id can be optional as well as the password. making the smallest valid Auth-string only the colon (:). I moved some assertions accordingly, as i previously declared some (spec-wise) valid authentication request as invalid.

I also added the test-assertion that invalid data actually never tries to hit the user-repository.

Sorry for the noise. Can you please take a 2nd look now?

If the commit-history is too polluted, i'm happy to squash them if you want.

Thank you very much.

I think this makes for a very robust basic-auth class. :)

weierophinney

The approach for fixing the problem looks fine. I've marked a number of changes I would like rolled back (as they are either out-of-scope or based on incorrect assumptions); once those are done, I can review and merge.

weierophinney · 2018-07-24T20:49:47Z

CHANGELOG.md

+
+
+- [#9](https://github.com/zendframework/zend-expressive-authentication-basic/pull/9)
+  It was not possible to use passwords that contained colons


Please use the same format as previous changelogs:

## 0.3.1 - TBD ### Added - Nothing. ### Changed - Nothing. ### Deprecated - Nothing. ### Removed - Nothing ### Fixed - ...

Notes:

There is a line between a header and the first line of text.

Only one empty line between list items.

Assume a bugfix version unless you are proposing a new feature or a breaking change.

weierophinney · 2018-07-24T20:52:05Z

src/BasicAccess.php

            return null;
        }

-        if (! preg_match('/Basic (?P<credentials>[a-zA-Z0-9\+\/\=]+)/', $authHeader[0], $match)) {
+        if (! preg_match('/Basic (?P<credentials>.+)/', $authHeader[0], $match)) {


Don't assume PHP is checking the header values. The code may be run under different SAPIs or alternative process managers that may or may not do such checks (e.g. Swoole, ReactPHP).

weierophinney · 2018-07-24T20:53:21Z

test/BasicAccessTest.php

@@ -46,7 +46,7 @@ protected function setUp()
        };
    }

-    public function testConstructor()
+    public function testConstructor(): void


We don't generally add return type hints within test classes, unless the method is a data provider or a helper method. For test cases, it's not necessary. Please remove it here and elsewhere where you have added them.

weierophinney · 2018-07-24T20:54:30Z

test/BasicAccessTest.php

-            $this->responseFactory
-        );
-        $this->assertNull($basicAccess->authenticate($this->request->reveal()));
-    }


Just a note for anybody else reviewing: this test becomes redundant, as it is covered by the empty set of the provideInvalidAuthenticationHeader provider used by the new testIsAuthenticatedWithInvalidData test case.

after review. Eliminates array offset checks.

This reverts commit 887c846.

This reverts commit 29e27fd.

minimal valid auth would contain only a semicolon (no user-id or password) that makes some of the test assertions that i thought be invalid actually valid. Move them accordingly.

The short list-syntax breaks the test when getHeader() returns only one element but with a non-zero index.

BreiteSeite · 2018-07-25T11:34:46Z

test/BasicAccessTest.php

@@ -109,7 +107,7 @@ public function testIsAuthenticatedWithValidCredential()

        $user = $basicAccess->authenticate($this->request->reveal());
        $this->assertInstanceOf(UserInterface::class, $user);
-        $this->assertEquals('Aladdin', $user->getIdentity());
+        $this->assertEquals($username, $user->getIdentity());


I think this assertion is only testing the mock and therefor worthless. Should i remove it?

Yes, please.

weierophinney

Looks good!

I saw comments in Slack and in my emails around the regex + base64_decode rationale as well as the question about what the Authorization header can contain, but for some reason, the comments are not present in the UI.

I had missed that base64_decode() has the second $strict argument; knowing that, your approach makes sense to me, as we can rely on that behavior and return early for invalid data.

With regards to the authorization header requirements, I agree with the approach of using getHeader() + array_shift(). Additionally, since explode() will return false for absence of the delimiter in the string, and you're now checking for that, this patch resolves the originally reported issue.

I've indicated that we can remove the assertion against the mock. If you want to do that, I can wait for you to push the change; otherwise, I can take care of it during merge.

weierophinney · 2018-07-25T14:31:54Z

test/BasicAccessTest.php

@@ -109,7 +107,7 @@ public function testIsAuthenticatedWithValidCredential()

        $user = $basicAccess->authenticate($this->request->reveal());
        $this->assertInstanceOf(UserInterface::class, $user);
-        $this->assertEquals('Aladdin', $user->getIdentity());
+        $this->assertEquals($username, $user->getIdentity());


Yes, please.

BreiteSeite · 2018-07-25T15:00:11Z

for some reason, the comments are not present in the UI.

@weierophinney to see the discussion about the regexp, you need to expand the comment of @froschdesign's review. GitHubs UI is a bit confusion here.

I've indicated that we can remove the assertion against the mock. If you want to do that, I can wait for you to push the change; otherwise, I can take care of it during merge.

I've pushed that.

Sounds like you approve the changes.

It would be great if you can change your review status from request-changes to approved then or tell me what else is to change to get your approval. :)

This PR will also prevent a lot of other bugs and warnings i've found when i wrote the extensive test providers. So i guess this makes the component a lot more stable, which is good, because authentication is a critical part in web applications. :)

Thanks for all the suggestions and comments!

- Rephrases them slightly to follow wording/phrasing conventions we generally use to detail changes due to pull requests. - Fixes some markdown whitespace issues that were causing rendering issues. - Sets date for 0.3.1 release.

Close #9 Fixes #8

weierophinney · 2018-07-25T15:09:24Z

Thanks, @BreiteSeite!

BreiteSeite added 3 commits July 24, 2018 11:49

#8 refactored tests

f8e478d

* use dataProviders * added more test-cases * added return-type hints * remove unnecessary assertion

#8 make authenticate method more robust

6ef417e

* use strict parameter of base64_decode instead of maintaining list of valid characters in regexp * use limit in explode() to allow passwords with colons * check if base64_decode failed * check that array index is set

#8 added changelog

f3bda5f

BreiteSeite commented Jul 24, 2018

View reviewed changes

froschdesign reviewed Jul 24, 2018

View reviewed changes

BreiteSeite changed the title ~~Issue/8~~ Fix PHP Warning if no colon is present in decoded basic-auth payload Jul 24, 2018

weierophinney suggested changes Jul 24, 2018

View reviewed changes

BreiteSeite added 14 commits July 24, 2018 23:02

#8 updated changelog skeleton

7445acd

#8 removed void return-type hints after review

f31d870

#8 use getHeaderLine() instead of getHeader()

29e27fd

after review. Eliminates array offset checks.

#8 refactored test to getHeaderLine() method

887c846

Revert "#8 refactored test to getHeaderLine() method"

2fe9738

This reverts commit 887c846.

Revert "#8 use getHeaderLine() instead of getHeader()"

5c812bc

This reverts commit 29e27fd.

#8 don't authorize if multiple auth header are sent

e541416

#8 assert that invalid data never tries to authenticate

3b538b3

#8 test with dataProvider shouldn't have static assertion

f739237

#8 fixed test-assertions after reading the spec

635e0e4

minimal valid auth would contain only a semicolon (no user-id or password) that makes some of the test assertions that i thought be invalid actually valid. Move them accordingly.

#8 use short-list syntax instead of array_shift

10c3f6d

#8 revert short list syntax

c5b0dab

The short list-syntax breaks the test when getHeader() returns only one element but with a non-zero index.

#8 check authentication handles unicode-characters

829040f

#8 small formatting improvement

5a64706

BreiteSeite commented Jul 25, 2018

View reviewed changes

weierophinney reviewed Jul 25, 2018

View reviewed changes

BreiteSeite added 2 commits July 25, 2018 16:51

#8 remove assertion that only tests mock-behaviour

17aa40e

#8 remove variable naming inconsistencies

b7a8388

weierophinney approved these changes Jul 25, 2018

View reviewed changes

weierophinney merged commit b7a8388 into zendframework:master Jul 25, 2018

weierophinney added a commit that referenced this pull request Jul 25, 2018

Merge branch 'hotfix/9'

149b70d

Close #9 Fixes #8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PHP Warning if no colon is present in decoded basic-auth payload #9

Fix PHP Warning if no colon is present in decoded basic-auth payload #9

BreiteSeite commented Jul 24, 2018 •

edited

Loading

BreiteSeite Jul 24, 2018

weierophinney Jul 24, 2018

BreiteSeite Jul 24, 2018 •

edited

Loading

froschdesign Jul 24, 2018

BreiteSeite Jul 24, 2018 •

edited

Loading

weierophinney Jul 24, 2018

BreiteSeite Jul 24, 2018 •

edited

Loading

weierophinney Jul 24, 2018

BreiteSeite Jul 24, 2018

BreiteSeite Jul 25, 2018

weierophinney left a comment

weierophinney Jul 24, 2018

weierophinney Jul 24, 2018

weierophinney Jul 24, 2018

weierophinney Jul 24, 2018

BreiteSeite Jul 25, 2018

weierophinney Jul 25, 2018

weierophinney left a comment

weierophinney Jul 25, 2018

BreiteSeite commented Jul 25, 2018

weierophinney commented Jul 25, 2018



		- [#9](https://github.com/zendframework/zend-expressive-authentication-basic/pull/9)
		It was not possible to use passwords that contained colons

Fix PHP Warning if no colon is present in decoded basic-auth payload #9

Fix PHP Warning if no colon is present in decoded basic-auth payload #9

Conversation

BreiteSeite commented Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BreiteSeite Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BreiteSeite Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BreiteSeite Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BreiteSeite commented Jul 25, 2018

weierophinney commented Jul 25, 2018

BreiteSeite commented Jul 24, 2018 •

edited

Loading

BreiteSeite Jul 24, 2018 •

edited

Loading

BreiteSeite Jul 24, 2018 •

edited

Loading

BreiteSeite Jul 24, 2018 •

edited

Loading