Make WordPress Core

Opened 17 months ago

Closed 8 months ago

#57512 closed defect (bug) (fixed)

Warning as a part of API response

Reported by: kalpeshh's profile kalpeshh Owned by: kadamwhite's profile kadamwhite
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.1.1
Component: REST API Keywords: php80 has-unit-tests has-patch has-testing-info commit
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 17 months ago.
Patch for PHP 8.0 support
57512_with_tests.diff (2.4 KB) - added by shooper 17 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 17 months ago.
Handles passwords that contain a colon properly

Download all attachments as: .zip

Change History (33)

@shooper
17 months ago

Patch for PHP 8.0 support

#1 @shooper
17 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 17 months ago by shooper (previous) (diff)

#2 @shooper
17 months ago

  • Keywords has-patch added

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


17 months ago

#4 @shooper
17 months ago

  • Keywords php8 added

#5 @kalpeshh
17 months ago

I have tested the patch and works fine.

#6 @sc0ttkclark
17 months ago

Code looks good here, seems like a concise solution.

#7 @jrf
17 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
17 months ago

  • Keywords php8 added; php81 removed

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

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

#10 @shooper
17 months ago

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

#11 @jrf
17 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
17 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
17 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
17 months ago

Handles passwords that contain a colon properly

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

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

#16 @kalpeshh
17 months ago

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

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


9 months ago

#19 @nicolefurlan
9 months 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.

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


9 months ago

#21 follow-up: @oglekler
9 months ago

  • Keywords needs-testing-info added; has-testing-info removed

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

#22 @oglekler
9 months ago

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

#23 in reply to: ↑ 21 @shooper
9 months ago

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.

#24 @nicolefurlan
9 months ago

  • Keywords has-testing-info added; needs-testing-info removed

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 :)

#25 @kadamwhite
8 months ago

  • Keywords needs-testing removed

Tested on PHP 8.1 and patch is working as described.

#26 @kadamwhite
8 months ago

  • Keywords commit added

#28 @kadamwhite
8 months ago

  • Owner set to kadamwhite
  • Status changed from new to reviewing

kadamwhite commented on PR #5438:


8 months ago
#29

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

#30 @kadamwhite
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.