Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#17490 closed defect (bug) (fixed)

cURL fails with timeout when posting to a location that redirects

Reported by: simsmaster's profile simsmaster Owned by: dd32's profile dd32
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.2
Component: HTTP API Keywords: needs-testing
Focuses: Cc:

Description

I have the following code in my plugin:

$response = $request->request($fetch_url, array('method' => 'POST', 'body' => $post_data));

If $post_data == array(); everything works fine. But when I set something to that array, like this:

$post_data['tmp'] = 'abc';

$response is WP_Error with an timeout message.

I use PHP 5.3.1 on Windows (XAMPP)

Sounds to me like a bug...

Attachments (2)

17490.diff (1.2 KB) - added by sivel 13 years ago.
17490-2.diff (1.7 KB) - added by simonwheatley 13 years ago.
Handle redirections outside Curl

Download all attachments as: .zip

Change History (21)

#1 @nacin
13 years ago

Why are you doing $request->request()?

#2 @sivel
13 years ago

  • Keywords reporter-feedback added

I would assume that $request is the WP_Http object, which brings up the question of why wp_remote_post, or wp_remote_request aren't being used.

Can you please try the helper functions and also try this on trunk or the nightlies?

My test code, in which I have tested each transport individually works without issue:

$post_data['tmp'] = 'abc';                                                                                                        
$args = array(
    'timeout' => 30,
    'body' => $post_data
);

$response = wp_remote_post('http://localhost/post.php', $args);
var_dump($response);

#3 @simsmaster
13 years ago

  • Version set to 3.2

Yes, Response is that Object.

I don't now, that I could use custom post data with the helper functions, but I will test that tomorrow.

I used WP Trunk, just forgot to set the version tag ;)

#4 @simsmaster
13 years ago

  • Keywords reporter-feedback removed

Ok, i've done some research:

First, my code:

$post_data['tip'] = 'adfadf';
$fetch_url = 'http://mantisbt.org/bugs/login.php';
$response = wp_remote_post($fetch_url, array('body' => $post_data));
fb($response); //FirePHP
die();

This produces the timeout error, if $post_data isn't empty. But the interesting thing: It works well, with every non-Mantis URL.

This might be a bug in MantisBT itself. You can try it with the URL above, it the same behaviour as with my own Mantis Installation.

I found this in the apache error log: (70014)End of file found: mod_fcgid: can't get data from http client

Since Mantis uses POST for login, too, it looks like a WP Bug for me...

#5 @simsmaster
13 years ago

And even more research:
I compared the WP Request with a normal login in Wireshark:
WP send the POST Request --> Mantis send 302 Moved Temp. (with the url of the login page) --> 5sec later Mantis sends 500 Internal Error and logs, the message noted above.

Browser sends POST --> Mantis send 302 to login_cookie_test.php --> GET cookie_test --> 302 Startpage of Mantis.

Maybe this is a "security" feature in Mantis... I will continue researching.

#6 @dd32
13 years ago

hi simsmaster,
I'll do some testing on this later in the week, but in the meantime, here's a few things I'd like you to try:

  1. Instal my core Control plugin from the plugin repo, Enable the HTTP Access module (You'll find it under Tools I believe)
  2. Post the current list of available transports.
  3. Try disabling the current POST handler transport, then try your code again, see if it passes.

The other thing you can try, is setting a higher than normal timeout, and see if that helps:

set_time_limit(120);
$post_data['tip'] = 'adfadf';
$fetch_url = 'http://mantisbt.org/bugs/login.php';
$response = wp_remote_post($fetch_url, array('body' => $post_data, 'timeout' => 90));
fb($response); //FirePHP
die();

Also, Trunk includes a fair few bug fixes to the HTTP API, so it's worthwhile testing there first before delving deep into code. Sorry, didn't read close enough, you're using trunk :)

Also, If you're using the cURL transport (from step 2 above) can you post the curl version from phpinfo()? (Well, all the curl info would be useful).

Last edited 13 years ago by dd32 (previous) (diff)

#7 @simsmaster
13 years ago

I installed you plugin, but the "External HTTP Access" Module produces just a fatal error:

Fatal error: Call to undefined method WP_Http::_getTransport() in \wp-content\plugins\core-control\modules\core_control_http.php on line 79

This method does not exists...


But I logged the transports in class-http.php, and curl was used. If I disable curl (function test() return false;) streams is used, and there is no timeout anymore, but the login to mantis failed. The page returned is just the login page with a error message. Maybe streams has a problem handling cookies...

If I disable streams, fsockopen is used, but it can't login either...

When the timeout is increased to 30sec, curl fetches a Error 500 page, the other transports the login page as described above...

And here are the cURL infos:

cURL support 	enabled
cURL Information 	7.19.6
Age 	3
Features
AsynchDNS 	No
Debug 	No
GSS-Negotiate 	No
IDN 	Yes
IPv6 	No
Largefile 	Yes
NTLM 	Yes
SPNEGO 	No
SSL 	Yes
SSPI 	No
krb4 	No
libz 	Yes
CharConv 	No
Protocols 	tftp, ftp, telnet, dict, ldap, ldaps, http, file, https, ftps
Host 	i686-pc-mingw32
SSL Version 	OpenSSL/0.9.8l
ZLib Version 	1.2.3

#8 @dd32
13 years ago

I installed you plugin, but the "External HTTP Access" Module produces just a fatal error:

Ah right.. The stable release doesnt work on Trunk, and the Development version doesnt work on stable, only trunk.. I'll have to fix that up shortly.

Thanks for the extra information, I'll take a look at it today, see if i can reproduce and narrow it down a bit.

#9 @dd32
13 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from HTTP API fails with timeout, if POST-Headers are present to cURL fails with timeout when posting to a location that redirects

The problem is only occuring with cURL.

Some background first: When you make a POST request, If you recieve a 3xx w/ Location in response, The correct thing to do next, is to perform a GET request to the new url, which is not what most people expect (Expecting the class to follow the Location with a new POST).

Because of that, Posting to that login URL will result in nothing good anyway as you have to post to www.mantis.., but thats not the bug in question here..

The problem is, that cURL is having an issue with the Content-Length header being set, It appears that cURL attempts to use the specified headers on the subsequent GET request, and since it can't find 10 characters of data to send (as it discarded it as it's now doing the GET request to the redirection...), it seems to sit there and timeout, and never make the request.

The Streams transport seems to be smart in it's following, It doesn't send the Content-Length header on the subsequent request.

The FSockopen transport breaks tradition, and performs a POST to the new Location, with the post fields and content-length, Which all works and causes no ill effects (other than breaking spec).

It seems to me a combination of these 3 things:

  • It's a cURL bug, They should disregard the Content-Length header on the subsequent redirection
  • We should potentially let cURL take care of setting the Content-Length header (we currently do this ourselves), When doing this, cURL sets the header itself when required (ie. when there's a post body) - We set this ourselves to work around a few server bugs, Some servers refuse POST requests without Content-Length headers (even though the post body is empty - cURL doesnt send the header when the body is empty). Fix some, Break others.
  • Fsockopen should be updated to follow spec

#10 @dd32
13 years ago

  • Fsockopen should be updated to follow spec

Created #17588 to deal with that.

The alternative here, Is to remove cURL's redirection support, and implement it ourselves, We currently do this when operating under safe mode (Since safe mode enforces some restrictions on the redirections which can take place). This would also reduce the potential for there being different bugs between safe mode, and no safe mode.. (ie. This bug shouldn't exist on servers where open_basedir is set, or safe mode is enabled).

#11 follow-up: @sivel
13 years ago

It looks like the issue is the PHP implementation of cURL keeps the same Content-Type request header and recalculates the Content-Length for the GET.

Here is a sample of the verbose output from cURL:

* About to connect() to localhost port 80 (#0)
*   Trying ::1... * connected
* Connected to localhost (::1) port 80 (#0)
> POST /post.php HTTP/1.0
User-Agent: WordPress/3.2-beta2; http://wordpress.trunk
Host: localhost
Accept: */*
Accept-Encoding: deflate;q=1.0, compress;q=0.5
Content-Type: application/x-www-form-urlencoded; charset=UTF-8
Content-Length: 7

< HTTP/1.1 302 Found
< Date: Fri, 27 May 2011 14:24:14 GMT
< Server: Apache/2.2.17 (Unix) PHP/5.3.4 mod_ssl/2.2.17 OpenSSL/0.9.8l DAV/2 mod_wsgi/3.3 Python/2.7.1
< X-Powered-By: PHP/5.3.4
< Location: http://localhost/get.php
< Content-Length: 43
< Connection: close
< Content-Type: text/html
< 
* Closing connection #0
* Issue another request to this URL: 'http://localhost/get.php'
* Violate RFC 2616/10.3.3 and switch from POST to GET
* About to connect() to localhost port 80 (#0)
*   Trying ::1... * connected
* Connected to localhost (::1) port 80 (#0)
> GET /get.php HTTP/1.0
User-Agent: WordPress/3.2-beta2; http://wordpress.trunk
Host: localhost
Accept: */*
Accept-Encoding: deflate;q=1.0, compress;q=0.5
Content-Type: application/x-www-form-urlencoded; charset=UTF-8
Content-Length: 7

* Operation timed out after 5000 milliseconds with 0 bytes received
* Closing connection #0

It looks like there is no way to change this behavior via cURL options. So we can check if it is a POST request, if so then don't follow location, if a redirect is returned from the POST, call wp_remote_get minus some of the bits that would cause it to fail again, and return it's response.

A proof of concept patch will follow in a few seconds.

@sivel
13 years ago

#12 @dd32
13 years ago

It looks like the issue is the PHP implementation of cURL keeps the same Content-Type request header and recalculates the Content-Length for the GET.

Close, It's not recalculating the Content-Length header, which is the problem. It just uses all the headers specified for the following requests.

That patch looks like it will work (aside from the fact I don't think it'll abide by max redirects = 1). It does however make sense to me to tell curl not to follow redirects at all and just handle it ourself, since we already have to do that for safe mode. If we add another branch for posts, thats 3 possible locations for redirection handling that could break.

#13 @simsmaster
13 years ago

Just a short question: Has anything changed with 3.2? I've read there were one change in the HTTP API, but I'm not sure, if it is affecting this bug...

#14 @dd32
13 years ago

I've read there were one change in the HTTP API, but I'm not sure, if it is affecting this bug...

There were many changes in 3.2, this is not one of them.

#15 @mdawaffe
13 years ago

Check out cURL's CURLOPT_POSTREDIR (added in cURL 7.19.1). The constant is defined in PHP (as of 5.3.2) but undocumented.

For example, the following tells cURL to maintain the HTTP method after either a 301 or 302 redirect.

curl_setopt( $handle, CURLOPT_POSTREDIR, 3 );

In the absence of CURLOPT_POSTREDIR support, doing

curl_setopt( $handle, CURLOPT_CUSTOMREQUEST, 'POST' );

rather than relying on

curl_setopt( $handle, CURLOPT_POST, true );

may help in some cases as well.

#16 in reply to: ↑ 11 @simonwheatley
13 years ago

  • Cc simon@… added

Replying to sivel:

A proof of concept patch will follow in a few seconds.

I've tested this patch, and it's working for me. (Hit the issue with a gov.uk API today.)

I'll work up an alternative which follows DD32's suggestions re handling redirects in WP rather than with CURLOPT_FOLLOWLOCATION,

#17 @simonwheatley
13 years ago

  • Keywords needs-testing added

Here's a patch which has WP handling the redirections in all cases, rather than using CURLOPT_FOLLOWLOCATION.

I'm specifying CURLOPT_MAXREDIRS as 0, is this too cautious given CURLOPT_FOLLOWLOCATION is false?

@simonwheatley
13 years ago

Handle redirections outside Curl

#18 @dd32
13 years ago

  • Milestone changed from Future Release to 3.4

See #17588 for a ticket detailing POST -> GET behaviours of HTTP Requests.

#19 @dd32
13 years ago

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

In [20208]:

WP_HTTP: Curl: Handle Redirections in PHP rather than internally in Curl, Simplifies code flow between safe_mode On and Off, and works around certain bugs. Props simonwheatley for initial patch. Fixes #20219, Fixes #17490

Note: See TracTickets for help on using tickets.