WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#11613 closed defect (bug) (fixed)

HTTP API may make a request on an unsupporting transport

Reported by: dd32 Owned by:
Milestone: 3.2 Priority: normal
Severity: normal Version: 2.9
Component: HTTP API Keywords: has-patch
Focuses: Cc:

Description

At present, When the HTTP API is called, It iterates through the transports testing each one to see if it can support the current request params (eg. SSL). This result is then stored for future requests.

The issue i've run into, Is that i've added a case to a transport, that it should not be used if its a HEAD request (As it simply doesnt support it on that transport), The problem is, if a request is made before me, for say, a GET request, that transport gets cached for it, even if it doesnt support it..

The attached patch adds a $key to the cache, which is a md5 of the serialized args.

In most cases, this doesnt cause any issues, but if a host is using a lesser transport which cannot do everything (Such as FOpen) then errors can be hit that the transport doesnt support it.

Attachments (6)

11613.diff (5.7 KB) - added by dd32 5 years ago.
11613.2.diff (5.7 KB) - added by dd32 5 years ago.
logic typo fix in posttransports empty check
11613.3.diff (5.7 KB) - added by dd32 5 years ago.
11613.4.diff (5.9 KB) - added by dd32 5 years ago.
11613.5.diff (5.7 KB) - added by Denis-de-Bernardy 5 years ago.
11613.patch (19.0 KB) - added by jacobsantos 5 years ago.
Non-tested prototype for better solution.

Download all attachments as: .zip

Change History (28)

@dd325 years ago

@dd325 years ago

logic typo fix in posttransports empty check

comment:1 @Denis-de-Bernardy5 years ago

won't the serialized args based key result in each transport being tested on every call?

comment:2 @dd325 years ago

  • Keywords needs-patch added; has-patch removed

won't the serialized args based key result in each transport being tested on every call?

Just the calls which have varying args. The only non-arg in that list is the post body, which can probably be nulled out, that way multiple post requests will be able to use the cache again. It doesnt contain the URL, so not a problem there.. But does differentiate between GET/POST/etc.

It is unavoidable that they get tested multiple times, It'd be much simpler if the fallback to the next transport should the first fail worked properly, #8622 that way, the arg could run ::test() itself when being called, and if it wasnt up for the job, simply pass it onto the next best transport.

Most pages will only have a small number of requests made, while it may add another couple of ms to the function, in my opinion, the benifit of the request actually suceeding and returning the correct data is worth more.

@dd325 years ago

comment:3 @dd325 years ago

  • Keywords has-patch added; needs-patch removed

Latest patch generates keys as such string(8) "GET,1,,1" much simpler, and only contains information which the ::test() args need.

@dd325 years ago

@Denis-de-Bernardy5 years ago

comment:4 @Denis-de-Bernardy5 years ago

I've uploaded 11613.5.diff, which is based on the .4.diff. The difference is it checks for isset() rather than empty(), since checking for empty when there are no valid transports could potentially lead the tests to be redone each time.

Re tests done, I merely stuck to verifying that RSS widgets and update checking still work.

comment:5 @dd325 years ago

The difference is it checks for isset() rather than empty(), since checking for empty when there are no valid transports could potentially lead the tests to be redone each time.

Thats a fair enough change.

comment:6 @hakre5 years ago

Is it possible to deactivate caching for a certain transport operation so an author an actively prevent such race-conditions in the cache?

comment:7 @dd325 years ago

Is it possible to deactivate caching for a certain transport operation so an author an actively prevent such race-conditions in the cache?

No. And probably shouldnt be supported.

However, With this patch, It'd be possible to break the cache if the code is calling with different args which the current transport doesnt understand.

Developers can always call a specific transport directly if they require, but its highly unsupported.

comment:8 @dd325 years ago

  • Milestone changed from 3.0 to 3.1

comment:9 @jacobsantos5 years ago

If this is the case, then why won't you just do the check in the transport and then change the HTTPS scheme to a HTTP one? The transports should only be checking whether it supports SSL, supports sending a body and supported by PHP. Everything else should just be done in the transport, unless it is something that needs to be supported by every transport then needs to be a method or a class.

comment:10 @dd325 years ago

then why won't you just do the check in the transport and then change the HTTPS scheme to a HTTP one?

Changing from HTTPS to HTTP is a no-no, If someone asks for a HTTPS connection, You give them a HTTPS connection.

The problem here is, that not all the transports can support a HTTPS connection, If you have multiple connections being made on a pageload, the first request determines which transport is used for that connection

So if the first request is a HTTP request, It may be made over Class_NoSSL, Then a HTTPS request comes along (of the same GET/POST type), it'll be passed to Class_NoSSL as ::test() passed last time.

This is due to #8622, Which should probably be fixed instead of this, or along side.

comment:11 @jacobsantos5 years ago

I see, thanks for the clarification. To me the ticket seemed to be that servers did not support secure HEAD requests and should be non-SSL instead. Given that it is quite frankly improbable that a WordPress installation is going to change servers any time soon, it seems fair to say that a refactoring of the code is in order. Right now it seems quite the mess.

comment:12 @dd325 years ago

To me the ticket seemed to be that servers did not support secure HEAD requests and should be non-SSL instead.

That was just an example.

It can happen with any functionality which one transport supports that another doesnt.

HEAD requests, SSL, SSL options, Custom headers, etc. If the transport chosen for the first request doesnt support the functions the 2nd request requires, the 2nd request will fail.

@jacobsantos5 years ago

Non-tested prototype for better solution.

comment:14 @nacin5 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:15 @dd324 years ago

  • Milestone changed from Future Release to 3.2

With the removal of the fopen transport (Which only supported GET requests), there is no longer a requirement for a difference in the GET/POST transport orders.

This in addition to #8622 is preventing unit testing of the HTTP API as well.

The following commit replaces _getTransport() & _postTransport() and adds a _dispatch_request() private method, the simple purpose of _dispatch_request() is to take the $args and determine a transport which the request can be made on, and then dispatch the request to that transport worker. (In addition, it clarifys the PHPDoc on the return of WP_HTTP::request() that it can return a WP_Error instance upon failure)

comment:16 @dd324 years ago

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

(In [17550]) Fix WP_HTTP to only make a request upon a working transport, as well as to allow Unit Testing. Removes the getTransport() & postTransport() methods as they're no longer needed, replaces them with a single _dispatch_request() method. Fixes #11613

comment:17 @hakre4 years ago

Follow-Up; Related: #16959

comment:18 @hakre4 years ago

In [17550] I've spotted a bug in the new introduced function _dispatch_request().

Please see Related: #16978

Last edited 4 years ago by hakre (previous) (diff)

comment:19 @dd324 years ago

(In [17566]) Use the correct variable name; Simpler static initialisation. Props hakre. See #11613 & #16978

comment:20 @westi4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The changes in [17550] break any plugin which relies on http_transport_post_debug or http_transport_get_debug to work around bugs in the SSL detection code in the WP_HTTP class like Jetpack does.

We don't normally remove actions or filters like this.

comment:21 @westi4 years ago

  • Cc mdawaffe westi added

mdawaffe pointed out that the patches on http://core.trac.wordpress.org/ticket/16606 negate the need for these actions in Jetpack so we should get those in and then all will be ok :)

comment:22 @westi4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.