Warning as a part of API response

[kalpeshh]

When I pass incorrect value in Authorization header, I see the warning as a part of REST response.

Sample API Request

GET http://localhost/wordpress/wp-json/wp/v2/posts

Authorization: Basic testheader

Response:

<br />
<b>Warning</b>:  Undefined array key 1 in <b>C:\xampp\8.2.0\htdocs\wordpress\wp-includes\load.php</b> on line <b>125</b><br />

Can we set proper response code for this case with non 200 error code?

[shooper]

Patch for PHP 8.0 support

[shooper]

I was able to replicate this bug in PHP 8.0, but not in 7.4.

It appears that the list() function now throws a warning if the explode() function does not return at least the number of array elements that list() is expecting.

According to the RFC for Basic Authentication, both a username and password are required.

I've patched the wp_populate_basic_auth_from_authorization_header() function to return false if both are not provided.

I chose not to return a non-200 error for now, as that matches the current functionality. This patch just eliminates the warning.

[kalpeshh]

I have tested the patch and works fine.

[sc0ttkclark]

Code looks good here, seems like a concise solution.

[jrf]

To prevent a regression being introduced at a later stage, I think it would be great if a test could be added to cover this change (with various inputs to cover all situations, i.e. no :, one :, multiple :... well, you get my drift).

[kalpeshh]

I will add unit tests for this.

Also, patch needs to be updated to handle the multiple : case

[shooper]

Adds checks for additional parts in the decoded base64, and adds unit tests

[jrf]

@shooper Thanks for making those updates to the patch.

I do wonder whether this is correct ?

<?php
if ( 2 !== count($exploded_basic_auth) ) {
    return;
}

Why not use ​`explode()` with the $limit parameter set to 2 ?

I also think it would be good to have a closer look at some of URL/URI RFCs to see what those say about the user and password field encoding and possible get some more test cases based on those RFCs.

[shooper]

Reviewed RFC 7617 which covers this use case.

​https://www.rfc-editor.org/rfc/rfc7617

Relevant part: “ Furthermore, a user-id containing a colon character is invalid, as the first colon in a user-pass string separates user-id and password from one another; text after the first colon is part of the password. User-ids containing colons cannot be encoded in user-pass strings.”

I’ll update this patch to match this description. The change being that a additional colons should be part of the password.

[kalpeshh]

Count should be exactly 2, instead of limit for BasicAuth.

@shooper what are your thoughts on below change to make code more readable?

Instead of,

$exploded_basic_auth = explode( ':', $userpass );
if ( 2 !== count($exploded_basic_auth) ) {
    return;
}

$_SERVER['PHP_AUTH_USER'] = $exploded_basic_auth[0];
$_SERVER['PHP_AUTH_PW']   = $exploded_basic_auth[1];

can we use,

$exploded_basic_auth = explode( ':', $userpass );
if ( 2 !== count($exploded_basic_auth) ) {
    return;
}

list( $user, $pass ) = $exploded_basic_auth;
$_SERVER['PHP_AUTH_USER'] = $user;
$_SERVER['PHP_AUTH_PW']   = $pass;

[shooper]

Handles passwords that contain a colon properly

[shooper]

Updated the patch to match the specs of RFC 7617.

It now handles the case properly where a password contains a colon, which my original patch broke. @jrf Thanks for the tip about the $limit arg. I'd misunderstood how that works, it was exactly what we needed.

It also handles gracefully the case of a token with only a username and no password, which was the original problem.

[jrf]

@shooper Thanks for the update! Happy to see this moving forward in the right direction.

[kalpeshh]

Thanks @shooper. I have tested this and works perfectly.

[hellofromTonya]

Changes to the ticket:

• Removed 2nd-opinion. Not seeing where or why that keyword as added.
• Changed php8 to php80 to denote PHP 8.0 as the version that increases the error severity.
• Added needs-testing to get a Test Report, which as one more check in the process.
• Moved it into 6.4, given the 57512_with_tests_2.diff​ could be considered for commit.

Is this a PHP 8.0 incompatibility?
I did not add php-compatibility to identify this issue as a Core incompatibility with PHP 8.0. I think this issue is more of a code improvement to be defensive for an unhappy path of an invalid value, i.e. given both parts are required.

[nicolefurlan]

If anyone has time to test and submit a test report for this, that would be great! A committer could also pick this up and test it during the 6.4 beta cycle.