Make WordPress Core

Opened 8 months ago

Last modified 4 days ago

#57512 new defect (bug)

Warning as a part of API response

Reported by: kalpeshh's profile kalpeshh 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)

57512.diff (602 bytes) - added by shooper 8 months ago.
Patch for PHP 8.0 support
57512_with_tests.diff (2.4 KB) - added by shooper 8 months ago.
Adds checks for additional parts in the decoded base64, and adds unit tests
57512_with_tests_2.diff (2.2 KB) - added by shooper 8 months ago.
Handles passwords that contain a colon properly

Download all attachments as: .zip

Change History (22)

@shooper
8 months ago

Patch for PHP 8.0 support

#1 @shooper
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.

Last edited 8 months ago by shooper (previous) (diff)

#2 @shooper
8 months ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core-php by shooper. View the logs.


8 months ago

#4 @shooper
8 months ago

  • Keywords php8 added

#5 @kalpeshh
8 months ago

I have tested the patch and works fine.

#6 @sc0ttkclark
8 months ago

Code looks good here, seems like a concise solution.

#7 @jrf
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).

#8 @jrf
8 months ago

  • Keywords php8 added; php81 removed

#9 @kalpeshh
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

@shooper
8 months ago

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

#10 @shooper
8 months ago

  • Keywords has-unit-tests has-patch added; needs-unit-tests needs-patch changes-requested removed

#11 @jrf
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 @shooper
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 @kalpeshh
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;

@shooper
8 months ago

Handles passwords that contain a colon properly

#14 @shooper
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 @jrf
8 months ago

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

#16 @kalpeshh
8 months ago

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

#17 @hellofromTonya
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 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.

This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.


4 days ago

#19 @nicolefurlan
4 days ago

  • Keywords has-testing-info added

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.

Note: See TracTickets for help on using tickets.