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.

[oglekler]

This ticket was discussed during bug scrub.

It needs manual testing. @shooper please provide steps for testers.
We have 12 days before RC1 and it will be very helpful. Thank you!

Add props: @mukesh27

[oglekler]

@kadamwhite and @timothyblynjacobs, can you please look at this ticket. Thank you!

[shooper]

Replying to oglekler:

This ticket was discussed during bug scrub.

It needs manual testing. @shooper please provide steps for testers.
We have 12 days before RC1 and it will be very helpful. Thank you!

Add props: @mukesh27

Thanks @oglekler - I can provide these instructions next week. I'm away until Tuesday.

[nicolefurlan]

I discussed this with @oglekler and we think the ticket description is sufficient for testing. Adding clear testing instructions here to make things easier for testers.

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce

1. Issue a GET request to /wp/v2/posts with an invalid Authorization header (as described in the ticket description).

Expected Results

• ✅ The warning no longer appears.

When reproducing a bug:

• ❌ The warning appears.

cc @shooper :)

[kadamwhite]

Tested on PHP 8.1 and patch is working as described.

Update of https://core.trac.wordpress.org/attachment/ticket/57512/57512_with_tests_2.diff with minor changes to comment formatting.

Trac ticket: https://core.trac.wordpress.org/ticket/57512

Patch updated to resolve some PHPCS errors noted in the original patch, and to make the naming of one of the tests more specific.

[kadamwhite]

In 56804:

REST API: Correct parsing of password from Authorization header when processing Application Password credentials.

Exit early when parsing Application Password credentials if Authorization header value does not contain at least one colon. The Authorization Basic header must use a colon to separate the username and password components per RFC 7617, so a username-only string is malformed and should not be processed.

Split Authorization header only on the first colon, properly handling passwords containing colons.

Resolves PHP 8.0 warning when list() was called on an exploded credentials array containing only one element.

Props kalpeshh, shooper, sc0ttkclark, jrf, mukesh27, oglekler, nicolefurlan.
Fixes #57512.