WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 4 months ago

Last modified 3 months ago

#49810 closed enhancement (fixed)

Remove workaround for $HTTP_RAW_POST_DATA bug present in PHP < 5.2.2

Reported by: skoskie Owned by: SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests has-dev-note
Focuses: rest-api Cc:

Description

This version of PHP is no longer officially supported, so the workaround need not be present in core.

Attachments (2)

0001-XML-RPC-Removed-workaround-for-PHP-5.2.2-bug.patch (1.0 KB) - added by skoskie 7 months ago.
49810.diff (2.6 KB) - added by desrosj 4 months ago.

Download all attachments as: .zip

Change History (39)

#2 @prbot
7 months ago

skoskie commented on PR #213:

Looks like my base branch didn't get set correctly ... Delete this PR.

This ticket was mentioned in PR #214 on WordPress/wordpress-develop by skoskie.


7 months ago

Removed workaround for $HTTP_RAW_POST_DATA bug present in PHP < 5.2.2, since that version is no longer supported.

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

#4 @prbot
7 months ago

ocean90 commented on PR #214:

Now the workaround is always used which is the wrong way to "fix" this. See also https://www.php.net/manual/en/reserved.variables.httprawpostdata.php

#5 @SergeyBiryukov
7 months ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Hi there, welcome to WordPress Trac! Thanks for the patch.

As noted in the PHP manual, $HTTP_RAW_POST_DATA was deprecated in PHP 5.6.0, and removed as of PHP 7.0.0.

The code relying on it should be updated to always use php://input instead.

#6 @skoskie
6 months ago

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

Apologies as this is my first contribution.

Since WP still supports PHP 5.6, and therefore the $HTTP_RAW_POST_DATA variable, should we not ensure it remains set for those still depending on it?

Anyway, this ticket can be closed. Thank you.

#7 @SergeyBiryukov
6 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Thanks for the contribution, no need for apologies :)

I think this ticket is valid, and some changes should be made here, just not exactly the currently proposed patch.

#8 @desrosj
6 months ago

Found this searching Trac before creating my own ticket for the same issue.

While working on the currently flagged PHPCompatibility issues in #49922, this came up.

I think this one is tricky to fix. A plugin directory search yields ~1,500 instances of HTTP_RAW_POST_DATA in the directory. Any plugin or custom code that relies on this variable being available could potentially experience problems when it is removed. There's even a test within the REST API tests making sure content in that global variable is properly unslashed.

This ticket was mentioned in PR #305 on WordPress/wordpress-develop by jrfnl.


5 months ago

See extensive comment in the related Trac ticket: https://core.trac.wordpress.org/ticket/49810

#10 @jrf
5 months ago

I've had a look at this and all other occurrences of $HTTP_RAW_POST_DATA in WP.

First off, this is a PHP Core removed global variable. Any plugin which relies on WP core backfilling that variable is most definitely doing it wrong™.

In reality, WP doesn't actually backfill the variable except for the XML-RPC entry point and select REST requests, which aren't the most common entry point for WP anyway, and on top of that, XML-RPC is very often disabled.

So, let's look at the three different files in which the issue occurs:

src/wp-includes/IXR/class-IXR-server.php

This is a file from an external dependency. I'm not sure by heart if this dependency is still maintained externally or abandoned and now maintained in WP Core only.

This file doesn't actually backfill the variable, it just uses it if available.

Conclusion: This should just be switched out for php://input.

src/wp-includes/rest-api/class-wp-rest-server.php

While this function - WP_REST_Server::get_raw_data() - does actually backfill the global, this looks more like an oversight than an architectural choice.

The important thing here is that the static WP_REST_Server::get_raw_data() method is in place to handle the potential non-availability of the $HTTP_RAW_POST_DATA variable and that any REST related code should already use this method instead of relying on the availability of the $HTTP_RAW_POST_DATA variable.

Conclusion: This should just be switched out for php://input.

src/xmlrpc.php

This is the only one where a case could be made for letting the variable remain as-is. The variable gets backfilled in the global scope for a very select entry point and is subsequently never used in WP Core, though the class-IXR-server.php file may have used it, but as it already had its own BC-code in place that was never an issue.

Conclusion: As it is never used in Core, this might be an entry point for which plugins expect the variable to exist and leaving it in place doesn't do any actual harm.

Patch

I've attached a new patch via GitHub to address this issue based on the above analysis.

PHPCompatibility report

FILE: src\wp-includes\IXR\class-IXR-server.php                                                                                                      
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
FOUND 3 ERRORS AFFECTING 3 LINES                                                                                                                    
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
 50 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
 51 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
 55 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
----------------------------------------------------------------------------------------------------------------------------------------------------

FILE: src\wp-includes\rest-api\class-wp-rest-server.php                                                                                             
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
FOUND 4 ERRORS AFFECTING 4 LINES                                                                                                                    
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
 1374 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                
 1380 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                
 1381 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                
 1384 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
FILE: src\xmlrpc.php                                                                                                                                
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
FOUND 5 ERRORS AFFECTING 4 LINES                                                                                                                    
----------------------------------------------------------------------------------------------------------------------------------------------------
                                                                                                                                                    
 20 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
 21 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
 25 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
 26 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
 26 | ERROR | Global variable '$HTTP_RAW_POST_DATA' is deprecated since PHP 5.6 and removed since PHP 7.0; Use php://input instead                  
----------------------------------------------------------------------------------------------------------------------------------------------------

#11 @prbot
5 months ago

jrfnl commented on PR #305:

For some weird reason the second commit I added to this branch does not show up. Opening a new ticket.

This ticket was mentioned in PR #306 on WordPress/wordpress-develop by jrfnl.


5 months ago

  • Keywords has-unit-tests added

See extensive comment in the related Trac ticket: https://core.trac.wordpress.org/ticket/49810

Replaces #305

#13 @TimothyBlynJacobs
5 months ago

  • Focuses rest-api added

I don't see how the XML-RPC logic is any different than the REST API logic. The request object isn't available immediately, notably for authentication. They may be relying on the raw post data to do a signature check.

WooCommerce does something similar to this, but they use WP_REST_Server::get_raw_data() so they wouldn't be effected: https://wpdirectory.net/search/01E9W6WA98EVCS5AFW5KKW89F5 But I think the logic still holds for someone wanting to get the raw body before the REST request is available or they intentionally want to get data for the global request.

#14 @prbot
5 months ago

ocean90 commented on PR #305:

@jrfnl For future PRs: Please use a fork of this repo as branches may get deleted due to the git mirror syncing.

Related: https://core.trac.wordpress.org/ticket/50307

#15 @prbot
5 months ago

jrfnl commented on PR #305:

@ocean90 Thanks for the heads-up. I must have pushed to the wrong remote. Will keep it mind for the future.

#16 @desrosj
5 months ago

In 47902:

General: Continuing to work towards a passing PHP Compatibility scan.

This is a final pass to fix PHP compatibiilty issues in the codebase with code changes or adding phpcs:ignore comments.

With this change, all PHP compatibility warnings and errors without specific tickets have been addressed (see #49810 and #41750).

Props desrosj, johnbillion, jrf.
See #49922.

#17 @jrf
5 months ago

But I think the logic still holds for someone wanting to get the raw body before the REST request is available or they intentionally want to get data for the global request.

@TimothyBlynJacobs They can without any problem by using file_get_contents('php://input') which is the recommended alternative for the removed PHP global.

#18 @TimothyBlynJacobs
5 months ago

They can without any problem by using file_get_contents('php://input') which is the recommended alternative for the removed PHP global.

Yes, of course they can update their code to do that. Developers using the XML-RPC based one could as well. It is still a BC break that costs us very little to maintain.

#19 @jrf
5 months ago

@TimothyBlynJacobs This is not a WP BC-break though, but a PHP Core one, which most developers will have mitigated against already anyway.

#20 @TimothyBlynJacobs
5 months ago

This is not a WP BC-break though, but a PHP Core one,

What is the distinction? If you were expecting that global to be backfilled, your code will break.

Conclusion: As it is never used in Core, this might be an entry point for which plugins expect the variable to exist and leaving it in place doesn't do any actual harm.

Do you no longer think that then?

#21 @jrf
5 months ago

If you were expecting that global to be backfilled, your code will break.

Noone should ever have expected that though. The backfilling by WP in select places was only WP's way to mitigate the PHP BC-break when PHP 5.2 still needed to be supported.

Do you no longer think that then?

That's the only place where the variable is explicitely declared in the global namespace, which is why I made an exception.
Removing the mitigation there would involve renaming the variable and introducing a new variable in the global namespace which is risky as well, which is why I left it as is, but yes, realistically, renaming that variable is on the table.

#22 @jrf
5 months ago

  • Owner changed from SergeyBiryukov to jrf
  • Status changed from reopened to assigned

The whole thing with the variable in the src/xmlrpc.php file is, is that the variable is unused in WP.

To move forward there are three options:

  • Remove the variable all together as it is unused.
  • Rename the variable.
  • Leave as is.

As the variable is unused in WP core, both option 1 and 2 feel like a pretty big break.

This is different from the REST API situation where there was a function in place already which people should have used instead from the beginning.

#23 @jrf
5 months ago

  • Owner changed from jrf to SergeyBiryukov

Sorry, my browser is being awkward and changing things I never intended to change. Hoping I've set things back to how they were before correctly now.

#24 @desrosj
5 months ago

The variable was deprecated in PHP 5.6 and removed from PHP in version 7.0.

What about the following plan:

  • Leave as is for now (Core still supports 5.6).
  • Publish a post on Make/Core and cross post to the Make/Plugins blog detailing that this variable should not be used, and php://input should be used instead.
  • Detailing in that post that it would be removed from core when the minimum required version of PHP is bumped to 7.0.
  • Removing the variable in Core when the version requirement is bumped to 7.0.

If by chance someone is utilizing the variable, it will still work as expected for now, but there is documentation around how to stop using it, and they are given some time to make an adjustment. The comments of that post on Make/Core may also give us some BC guidance if someone is using it unconventionally.

#25 @jrf
5 months ago

@desrosj I can live with that.

Note: as the change to src/wp-includes/IXR/class-IXR-server.php does not involve a BC break in any way, that change should be made already. The other two can then wait for the next WP version.

This ticket was mentioned in PR #313 on WordPress/wordpress-develop by desrosj.


5 months ago

Addresses occurrences of $HTTP_RAW_POST_DATA in the codebase.

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

#27 @desrosj
5 months ago

@jrf What about an approach like in my linked PR (unit tests need to get fixed).

It it follows the plan I outlined above, but if the variable is set in those two situations the variable would remain, a _doing_it_wrong() is triggered to notify that the variable was removed in PHP 7.0.

#28 @jrf
5 months ago

@desrosj Sorry, but no, that doesn't seem like a good idea as that will throw an error on all PHP 5.6 installs without users being able to do anything to mitigate it.
While the variable was deprecated in PHP by that time, it still existed.

#29 @desrosj
5 months ago

Ah, yes. I was trying to be too clever there.

I updated my PR above to what reflect what I believe to be the current desired approach. Tests pass locally, there is just a huge backlog of builds in Travis due to WCEU contributor day.

@TimothyBlynJacobs does the plan I detailed make sense and feel reasonable to you?

#30 @TimothyBlynJacobs
5 months ago

does the plan I detailed make sense and feel reasonable to you?

I'm more comfortable with that plan.

@desrosj
4 months ago

#31 @desrosj
4 months ago

  • Component changed from XML-RPC to General
  • Keywords needs-dev-note added
  • Severity changed from trivial to normal

@TimothyBlynJacobs Great! @jrf was kind enough to give another review to the attached PR. This is good to go for now.

I'm also going to move this to General because it affects both XML-RPC and the REST API.

#32 @desrosj
4 months ago

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

In 47926:

General: Remove or add inline comments to $HTTP_RAW_POST_DATA occurrences.

The $HTTP_RAW_POST_DATA global was deprecated in PHP 5.6 and removed completely in PHP 7.0. In general, php://input should be used instead of $HTTP_RAW_POST_DATA.

Because WordPress Core still supports PHP 5.6, some plugins or sites may still rely on this variable being present and populated with the expected data. For that reason, occurrences of the variable will remain with updated inline documentation until support for PHP 5.6 is officially dropped in WordPress.

Props skoskie, jrf, desrosj, TimothyBlynJacobs.
See #49922.
Fixes #49810.

#33 @desrosj
4 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version trunk deleted

Reopening and assigning to myself for a developer note.

#35 @prbot
4 months ago

jrfnl commented on PR #306:

Closing in favour of merged patch: https://core.trac.wordpress.org/changeset/47926

#36 @desrosj
4 months ago

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

I am in the process of drafting the dev note and just confirmed it is in the list of notes being tracked by the release squad. Going to close this out to help clear the milestone in advance of beta 1 tomorrow.

#37 @desrosj
3 months ago

  • Keywords has-dev-note added; needs-testing needs-dev-note removed
Note: See TracTickets for help on using tickets.