#16606 closed defect (bug) (fixed)
WP_Http_Streams::test doesn't check enough to confirm if it can do HTTPS
Reported by: | westi | Owned by: | dd32 |
---|---|---|---|
Milestone: | 3.7 | Priority: | high |
Severity: | major | Version: | 3.0.5 |
Component: | HTTP API | Keywords: | |
Focuses: | Cc: |
Description
The WP_Http_Streams doesn't check for all it's prerequisites before saying it can process HTTPS request.
It seems to be common for the openssl extension to no be loaded.
Example Warnings:
[21-Feb-2011 08:06:48] PHP Warning: fopen() [<a href='function.fopen'>function.fopen</a>]: Unable to find the wrapper "https" - did you forget to enable it when you configured PHP? in .../wp-includes/class-http.php on line 1063 [21-Feb-2011 08:06:48] PHP Warning: fopen(https://example.com/feed/) [<a href='function.fopen'>function.fopen</a>]: failed to open stream: No such file or directory in .../wp-includes/class-http.php on line 1063
We need to at least check: extension_loaded( 'openssl' )
Attachments (9)
Change History (32)
#2
in reply to:
↑ 1
@
14 years ago
- Keywords has-patch added; needs-patch removed
Replying to dd32:
Should investigate WP_HTTP_Fsockopen() as well here
WP_Http_Fsockopen::test() tests for openssl library; Related: [10864]
#3
@
14 years ago
Related: 3:ticket:16959
#4
follow-up:
↓ 5
@
14 years ago
The Streams class only accepts http:// and https:// scheme's:
if ( 'http' != $arrURL['scheme'] && 'https' != $arrURL['scheme'] ) $url = preg_replace('|^' . preg_quote($arrURL['scheme'], '|') . '|', 'http', $url);
That renders most of the content of these patches moot.
Suggested:
function test($args = array()) { $use = function_exists('fopen'); if ( function_exists('ini_get') && true != ini_get('allow_url_fopen') ) $use = false; if ( !empty($args['ssl']) && !extension_loaded('openssl') ) $use = false; return apply_filters('use_streams_transport', $use, $args); }
#5
in reply to:
↑ 4
@
14 years ago
Replying to dd32:
The Streams class only accepts http:// and https:// scheme's:
if ( 'http' != $arrURL['scheme'] && 'https' != $arrURL['scheme'] ) $url = preg_replace('|^' . preg_quote($arrURL['scheme'], '|') . '|', 'http', $url);That renders most of the content of these patches moot.
Hm?
As this is the public test() function, it should return false if you test it with a URL with a scheme that is not supported, than that function should first of all return false for that tested URL.
If the class can technically not support the scheme of the URL because there is no stream wrapper for that scheme registered, it should return false. Existing wrappers are available via stream_get_wrappers().
If you want to check for SSL transport, my stance is to make use of stream_get_transports() to check for SSL. That check is more generic than checking for the openssl extension in specific.
I'll attach an updated patch that takes care of all of those.
For your suggested fopen exists + ini_get exists + allow_url_fopen check I suggest we create a function in either a base class for all transports or into WP_HTTP as this is used more and more often.
We could add some for SSL as well because until now, it's checked against the open_ssl library only. A ->has($what) like wpdb::has_cap() probably. Or on both classes, so that WP_Http can check on all supported transports. Could be added to blocking as well, just an idea.
For what reason does the http stream transport class not support other wrappers btw?
@
14 years ago
All checks, result truly filterable line in WP_Http_Curl::test() and WP_Http_ExtHttp::test()
#6
follow-up:
↓ 7
@
14 years ago
- Cc mdawaffe added
What's the use case for the filter when function_exists( 'fopen' )
returns false
?
Can we turn this into a generic ticket that adds an SSL check to the ::test()
methods for all the WP_HTTP transports?
Attached does so, but does not have some of the additional checks of hakre's patch (I wrote it before finding this ticket).
- Return false if the required function doesn't exist or could never work.
- Check for the openssl extension or CURL_VERSION_SSL as needed.
- Return filtered boolean.
#7
in reply to:
↑ 6
@
14 years ago
Replying to mdawaffe:
What's the use case for the filter when
function_exists( 'fopen' )
returnsfalse
?
Hooks can still filter it. E.g. reporting, enable / creating functions etc. (dl etc.).
And this is for streamline reasons: Not all test() functions were filterable in full.
Can we turn this into a generic ticket that adds an SSL check to the
::test()
methods for all the WP_HTTP transports?
Attached does so, but does not have some of the additional checks of hakre's patch (I wrote it before finding this ticket).
- Return false if the required function doesn't exist or could never work.
- Check for the openssl extension or CURL_VERSION_SSL as needed.
- Return filtered boolean.
The additional checks in my last patch are mostly as a "complete" check, so to pick from those as needed.
I think it's good to streamline all tests. I think it's worth to let them all run after a common principle (do one check after the other unless false) then then the filter at the end of the function, one return statement at the end only. So all test() functions have the same level of quality.
#8
follow-up:
↓ 9
@
14 years ago
16606.6.patch containted the $scheme check in each concrete ::test() function.
In 16606.7.patch I reduced the code now by only checking the $scheme where applicable (only Streams so far). Next to that there were some minor formatting / comment / coding issues in 16606.6.patch I fixed in this new patch.
The WP_Http_ExtHttp::test() function was lacking check of request and SSL support. The WP_Http_ExtHttp::request() made use of http_inflate which is only available if the http extension supports encodings.
Looks like that now each of the four WP_Http_Concrete::test() is checking for SSL support for the first time.
#9
in reply to:
↑ 8
@
14 years ago
Replying to hakre:
Replying to mdawaffe:
What's the use case for the filter when
function_exists( 'fopen' )
returnsfalse
?
Hooks can still filter it. E.g. reporting, enable / creating functions etc. (dl etc.).
OK.
The curl_version()
(with falllback to extension_loaded( 'openssl' )
) check in 16606.5.diff is a better check for the ability to make an SSL request in WP_Http_Curl
.
PS: for the inflate code, rather than changing WP_Http_ExtHttp::request()
, should WP_Http_Encoding::decompress()
just use http_inflate
if it's available and supported?
#10
@
14 years ago
What's the use case for the filter when function_exists( 'fopen' ) returns false?
Hooks can still filter it. E.g. reporting, enable / creating functions etc. (dl etc.).
For what it's worth here, I personally believe only a true
response should be filterable. If our test rules say the transport can't be used, it can't be used, it's that simple. The test rules are nothing extravagant, basic cap checks..
#11
@
14 years ago
fsockopen()
can't verify SSL certificates so its ::test()
should reflect that.
Streams can verify SSL certificates, though the code is currently broken (see 16606.8.diff). Unfortunately, setting verify_peer
is useless unless cafile
or capath
is also set, and we can't know what to set those to (also verify_host
doesn't exist). So the streams ::test()
should also bail if sslverify
is true
.
Only cURL and the HTTP extension can verify SSL certs.
#13
follow-up:
↓ 15
@
14 years ago
For streams, perhaps allow_self_signed can be of use. As far as requiring cafile or capath, I don't remember that being the case from my testing.
#14
@
14 years ago
So it basically looks like the only transport we have that really supports SSL verification is cURL. Since the http extension is really just a wrapper for cURL and is being considered for removal from the HTTP API (See #16978)
#15
in reply to:
↑ 13
@
14 years ago
Replying to sivel:
For streams, perhaps allow_self_signed can be of use.
We can't enable allow_self_signed by default. That would allow MITM attacks. There's currently no filter on the options array, but there should be.
Replying to sivel:
As far as requiring cafile or capath, I don't remember that being the case from my testing.
Were you testing with the faulty code addressed in 16606.8.diff? Try this test case.
#17
@
14 years ago
- Milestone changed from Future Release to 3.2
before anyone says something, That commit was designed to be a basic check for SSL capability and streamlining the current test methods, not to cover all the SSL issues brought up here.
#19
@
14 years ago
- Keywords 3.3-early added; 3.2-early removed
- Milestone changed from 3.2 to Future Release
Some patching was done for 3.2, but there's a bigger question of SSL handling in requests, specifically SSL signing left, Punting the signing to 3.3
#20
@
12 years ago
- Cc kurtpayne added
Is this still needed? Is it still considered "severity major" ?
#21
@
11 years ago
- Keywords 3.3-early has-patch removed
- Milestone changed from Future Release to 3.7
Most of this has been rendered fixed, or invalid by #25007
Should investigate WP_HTTP_Fsockopen() as well here, this is in the request method: