Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#49810 closed enhancement (fixed)

Remove workaround for $HTTP_RAW_POST_DATA bug present in PHP < 5.2.2

Reported by: skoskie's profile skoskie Owned by: sergeybiryukov's profile 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 5 years ago.
49810.diff (2.6 KB) - added by desrosj 5 years ago.

Download all attachments as: .zip

Change History (39)

skoskie commented on PR #213:


5 years ago
#2

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.


5 years ago
#3

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

ocean90 commented on PR #214:


5 years ago
#4

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
5 years 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
5 years 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
5 years 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
5 years 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 years ago
#9

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

#10 @jrf
5 years 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                  
----------------------------------------------------------------------------------------------------------------------------------------------------

jrfnl commented on PR #305:


5 years ago
#11

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 years ago
#12

  • 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 years 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.

ocean90 commented on PR #305:


5 years ago
#14

@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

jrfnl commented on PR #305:


5 years ago
#15

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

#16 @desrosj
5 years 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 years 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 years 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 years 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 years 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 years 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 years 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 years 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 years 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 years 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 years ago
#26

Addresses occurrences of $HTTP_RAW_POST_DATA in the codebase.

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

#27 @desrosj
5 years 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 years 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 years 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 years ago

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

I'm more comfortable with that plan.

@desrosj
5 years ago

#31 @desrosj
5 years 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
5 years 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
5 years ago

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

Reopening and assigning to myself for a developer note.

jrfnl commented on PR #306:


5 years ago
#35

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

#36 @desrosj
5 years 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
5 years ago

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