Opened 8 months ago
Last modified 4 days ago
#57512 new defect (bug)
Warning as a part of API response
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 6.1.1 |
Component: | REST API | Keywords: | php80 has-unit-tests has-patch needs-testing has-testing-info |
Focuses: | rest-api | Cc: |
Description
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?
Attachments (3)
Change History (22)
#1
@
8 months ago
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.
This ticket was mentioned in Slack in #core-php by shooper. View the logs.
8 months ago
#7
@
8 months ago
- Keywords needs-unit-tests php81 added; php8 removed
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).
#9
@
8 months ago
- Keywords needs-patch changes-requested added; has-patch removed
I will add unit tests for this.
Also, patch needs to be updated to handle the multiple :
case
#10
@
8 months ago
- Keywords has-unit-tests has-patch added; needs-unit-tests needs-patch changes-requested removed
#11
@
8 months ago
@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.
#12
@
8 months ago
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.
#13
@
8 months ago
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;
#14
@
8 months ago
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.
#15
@
8 months ago
@shooper Thanks for the update! Happy to see this moving forward in the right direction.
#17
@
8 weeks ago
- Keywords php80 needs-testing added; 2nd-opinion php8 removed
- Milestone changed from Awaiting Review to 6.4
Changes to the ticket:
- Removed
2nd-opinion
. Not seeing where or why that keyword as added. - Changed
php8
tophp80
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.
Patch for PHP 8.0 support