WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 9 years ago

Last modified 8 years ago

#4011 closed task (blessed) (fixed)

add global proxy support in options

Reported by: tmountjr Owned by: jacobsantos
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.2
Component: HTTP API Keywords: proxy has-patch needs-testing
Focuses: Cc:

Description

I use the 2.2-bleeding SVN build of WP on a local install to test upcoming changes to the system before putting them on my live blog. In order to test accurately, I installed FeedWordPress to pull my live RSS feed. I couldn't figure out why it wouldn't pull my feeds (even after adding all the appropriate snoopy proxy variables in the plugin) unless I set the proxy in wp-includes/class-snoopy.php. Ticket 3082 details the same problem. Perhaps a set of define statements could be added to wp-config.php:

define('WP_PROXYHOST', 'proxy.server.com');
define('WP_PROXYPORT', '8080');
define('WP_USEPROXY', 'TRUE');

In wp-includes/class-snoopy.php, for instance, the current code could be changed to respect the proxy settings if entered:

var $proxy_host = WP_PROXYHOST;
var $proxy_port = WP_PROXYPORT;
var $_isproxy = (WP_USEPROXY == 'TRUE') : true ? false;

These settings could also be carried over to Akismet and any other class that uses snoopy or fsockopen; I successfully tested this in class-snoopy.php. Besides not remembering my ternary operators for the _isproxy variable, the change works and applies to all instances where WP is using snoopy to get something from the web.

I'd be happy to work up something official if anyone thinks it's worth it...

Attachments (11)

wp-config-sample.php.diff (689 bytes) - added by tmountjr 11 years ago.
sample wp-config file with added support for proxy variables
class-snoppy.php.diff (1.2 KB) - added by tmountjr 11 years ago.
updated Snoopy class with support for proxy variables
class-IXR.php.diff (1.0 KB) - added by tmountjr 11 years ago.
updated IXR-RPC class file with proxy support for fsockopen
functions.php.diff (2.5 KB) - added by tmountjr 11 years ago.
updated to include support in get_headers and curl libraries
4011.diff (42.8 KB) - added by jacobsantos 9 years ago.
Incomplete patch for testing proxy support.
4011.2.diff (40.5 KB) - added by jacobsantos 9 years ago.
HTTP API cleanup with proxy support for HTTP, CURL and Streams transports.
4011.3.diff (30.9 KB) - added by jacobsantos 9 years ago.
Update patch for current trunk
4011.4.diff (25.3 KB) - added by DD32 9 years ago.
4011.5.diff (21.7 KB) - added by DD32 9 years ago.
4011.6.diff (23.7 KB) - added by DD32 9 years ago.
4011.spelling.diff (2.1 KB) - added by jbsil 9 years ago.
Changes to $accessible_hosts and WP_ACCESSIBLE_HOSTS in http.php

Download all attachments as: .zip

Change History (51)

#1 @foolswisdom
11 years ago

  • Milestone set to 2.3

@tmountjr
11 years ago

sample wp-config file with added support for proxy variables

@tmountjr
11 years ago

updated Snoopy class with support for proxy variables

@tmountjr
11 years ago

updated IXR-RPC class file with proxy support for fsockopen

#2 @tmountjr
11 years ago

  • Keywords has-patch needs-testing added

I've created and attached a few patches:

/wp-config-sample.php - adds support for proxy variables in the sample config file.

/wp-includes/functions.php - adds proxy support for fsockopen

/wp-includes/class-snoopy.php - adds proxy support for snoopy

/wp-includes/class-IXR.php - adds proxy support for fsockopen in the IXR-RPC class

These have been tested on mirror installations - one live (no proxy, so no options in wp-config.php set) and one local (with proxy, options set in wp-config.php). Both appear to work well. I think if no proxy is set the change would be transparent. What needs further testing is how the changes respond in a proxy environment. At the moment Snoopy works well in my environment, but I'm a little less clear on fsockopen and how that will respond, particularly in an environment where HTTP authentication is used.

@tmountjr
11 years ago

updated to include support in get_headers and curl libraries

#3 @tmountjr
11 years ago

I just modified functions.php.diff so that both instances of fsockopen were proxied. I also added the support for curl in the same file.

#4 @ryan
10 years ago

  • Milestone changed from 2.3 to 2.4 (next)

#5 @Johnny_one_eye
10 years ago

I think this patch is great, as I could really use it. I've been trying it out, and I only have 1 problem with it. My proxy doesn't require authentication (user/pass). I think you should probably edit it so that it would check if I need a user/pass and adjust accordingly...

#6 follow-up: @dubelclique
10 years ago

the patch worked for me on my pre-2.3 installs, but i can't get it to work on my 2.3 install; i'm positive i'm patching the right code, but i think there's something else in need of revision in 2.3.

any ideas? is the 2.4 milestone definite? i'm pretty surprised this isn't more of an issue for folks.

#7 in reply to: ↑ 6 @westi
10 years ago

  • Keywords needs-patch added; has-patch removed

Replying to dubelclique:

the patch worked for me on my pre-2.3 installs, but i can't get it to work on my 2.3 install; i'm positive i'm patching the right code, but i think there's something else in need of revision in 2.3.

any ideas? is the 2.4 milestone definite? i'm pretty surprised this isn't more of an issue for folks.

The 2.4 milestone is suggested not definite - if this issue is not resolved when it comes to releasing 2.4 then it will be bumped to 2.5

I think a better fix to this issue is to move all direct uses of fsockopen to a wrapped version which provides the proxy support (and possibly uses curl is fsockopen is disabled)

Then all places where we have duplicated fsockopen code can use this one route.

#8 @thee17
10 years ago

  • Milestone changed from 2.5 to 2.6

#9 @futtta
9 years ago

just bumping up, support for proxies is very important if we want to enable intranet-deployments of wordpress. servers on an intranet rarely have a direct connection to the internet (proxy or even no connection at all). currently this means wordpress will stop working after some time, with timeouts caused by attempts to connect to the internet to check for updates for core/ plugins.

not only should one be able to configure a proxy (+ proxy authentication info), you should also be able to specify there is no connection to the internet at all?

i tried to add 'return false' to a number of update functions, but ended up disabling fsockopen in php.ini all together. a pain, really.

#10 @DD32
9 years ago

In the event of #4779 being implemented, It should allow a global proxy setting to be added with ease without too much trouble.

#11 @DD32
9 years ago

  • Keywords snoopy needs-testing removed
  • Type changed from enhancement to feature request

Should be possible now that everythings being moved to the HTTP API, Hopefully all the transports can support proxies though (I have a feeling there may be 1 which will not handle it too well)

#12 @DD32
9 years ago

  • Component changed from General to HTTP
  • Owner anonymous deleted

See Also: #8927 (Allow the disabling of external HTTP Access)

If no-one else gets around to it, I'll implement this in 2.9

@jacobsantos
9 years ago

Incomplete patch for testing proxy support.

@jacobsantos
9 years ago

HTTP API cleanup with proxy support for HTTP, CURL and Streams transports.

#13 @jacobsantos
9 years ago

  • Owner set to jacobsantos

#14 @jacobsantos
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#15 @DD32
9 years ago

Problem with cURL:

Doesnt look like you can rely upon the headers length returned from it for proxy requests.. Eg, Full document returned:

HTTP/1.0 200 Connection established
Proxy-agent: CCProxy 6.64

HTTP/1.1 200 OK
Date: Sun, 22 Feb 2009 09:30:52 GMT
Server: Apache/2.2.8 (CentOS)
X-Powered-By: PHP/5.2.6
Content-Length: 4
Connection: close
Content-Type: text/html; charset=UTF-8

1563

$headerLength gets set to 190, which chops the headers off at the end of Content-Length, resulting in:

Headers:

HTTP/1.0 200 Connection established
Proxy-agent: CCProxy 6.64

HTTP/1.1 200 OK
Date: Sun, 22 Feb 2009 09:30:52 GMT
Server: Apache/2.2.8 (CentOS)
X-Powered-By: PHP/5.2.6
Content-Length

Body:

: 4
Connection: close
Content-Type: text/html; charset=UTF-8

1563

#16 @DD32
9 years ago

I couldnt get fsockopen() working either, The proxy server seems to be returning ..nothing:

array(4) { ["headers"]=> array(0) { } ["body"]=> NULL ["response"]=> array(2) { ["code"]=> int(0) ["message"]=> string(0) "" } ["cookies"]=> array(0) { } }

It might be the proxy server i'm testing with.. but it works with browsers.

#17 @DD32
9 years ago

Doesnt look like you can rely upon the headers length returned from it for proxy requests..

Hm.. I think its a cURL bug there, Those new lines are throwing it off. I'll test with a different proxy server and see what its output is. I'm not too sure on how standards-complient this rarely used proxy server is.

#18 @DD32
9 years ago

  • Keywords commit added; needs-testing removed
  • Milestone changed from 2.9 to 2.8

Alright, I've done some messing around, And i've got it to work 100% with Apache as a proxy server.

The proxy server i was testing with before (ccProxy) now works with HTTP, CURL, and Fopen() - But fails with streams and fsockopen().

I've not tested Squid, which is really probably the main server that needs testing.. I'll test that if i can find a test server, Or i can manage to configure it myself.

I've made a fair few modifications to the original patch( 4011.3.diff ), So What i'm thinking, Is Get that commited first (props jacob), once thats commited, I can make a patch based off that of my changes,

I'm aiming to support HTTP Proxies with this patch, Its near impossible to support every different type of proxy it seems, so on some hosts, disabling some of the HTTP transports -may- be required for clean operation, but only IF they're using a proxy too, So for the majority of users it'll not be a problem. For the majority of proxy users, it'll work fine (provided squid acts nicely), for a small set of the proxy users, they may have to disable them.

Once my patch is applied, The following items are affected:
Apache (As a proxy server, No Username/password): All transports work
CCProxy (With, and without, username/password): Http, Curl, and fopen() work. Streams and fsockopen() do not.

I've not tested apache with a username/password yet.

#19 @westi
9 years ago

  • Cc westi added

I agree with DD32 about applying one patch and then another to give jacob the due props.

Currently the patch doesn't apply and some of the comments could do with a tidy up ;-)

jacob could you update the patch for current trunk?

#20 @jacobsantos
9 years ago

Yes. Will do in a few hours.

#21 @DD32
9 years ago

I've cleaned up some of the comments myself, and some formatting as well (Just FYI)

@jacobsantos
9 years ago

Update patch for current trunk

#22 @jacobsantos
9 years ago

I actually like the comments.

#23 @westi
9 years ago

(In [10692]) First pass of HTTP Proxy support. See #4011 props jacobsantos.

@DD32
9 years ago

#24 @DD32
9 years ago

  • Keywords needs-testing added

attachment 4011.4.diff added

  • Lots of formatting changes..
  • Fixes a fair few Proxy items
  • Removed the filter 'http_request_port' from the fsockopen transport, Seems to not be used anywhere else, Seemed rather pointless there.. The port should be set in the URL to start with.
  • Some proxies require the HTTP header to be in the form of "GET http://blah/blah/ HTTP/1.0" instead of just utilising "GET /blah HTTP/1.0" with a "Host: blah"
  • HTTP Tunneling can cause issues, Many proxies do not allow tunneling either (Its used to fetch non-http data, so shouldnt be needed anyway)
  • Better ::test() functions, ::test() can be called from plugins or other code now without requiring the 'local'/'ssl' $args to be set(Well.. Why not, Else whats the point of the isset()'s?). Should make the ::test() functions much more robust now.
  • SSL Verification fixes, use the local ssl verification filter for local requests.. Make sure all the transports abide by it
  • Streams transport supports PHP 5.0+, however, Streams + SSL + Proxy requires PHP 5.1.0+
  • s/str_replace/preg_replace in certain locations to only replace the scheme part, NOT to replace the scheme in other places (ie. scheme://somewhere/scheme.php)
  • Passes $args into the filter functions for ::test()
  • A huge patch that needs testing.

#25 @jacobsantos
9 years ago

There are a bit more problems and that is part of the reasons I didn't want to undertake this project. There inconsistencies are in the various proxy implementations and has to be handled by the Library.

Doing so would really be dirty, so I think the most popular ones should be supported and all of the rest should be ignored.

#26 @DD32
9 years ago

  • Keywords needs-patch stale-patch added; has-patch removed

Patch is aparantly now stale.

@DD32
9 years ago

#27 @DD32
9 years ago

  • Keywords has-patch added; needs-patch stale-patch removed

Patch Refreshed.

#28 @bforchhammer
9 years ago

Wow, I love the work you're doing here... We are running one of those corporate wordpress installs behind a firewall and this is helping us very much already. Thanks!

I had a look at http.php HEAD & patch 4011.5.diff and here's a few things I stumbled on:

line 121 and 169: if has_filter() condition then do_action() call.

> Shouldn't that be either has_action() and do_action() or has_filter() and apply_filter() ?

lines 549 and 1498, for example is_array( WP_ACCESSABLE_HOSTS ).

> I am not sure that php allows to set constants to "arrays". Afaik only scalar values (integer, float, string, boolean) are accepted. Solution might be to serialise & unserialize the array or use a string instead (e.g. pipe | separated?).

lines 536 and 1513: get_bloginfo('site_url')

> 'site_url' is not a valid parameter; according to the codex doc on get_bloginfo it should be 'url' since 2.7 (or 'siteurl' should work but is deprecated apparently).

#29 @bforchhammer
9 years ago

And another one:

line 539: if ( $uri == $home['host'] )

> Comparing the full $uri with only the hostname of the current site will always(?) result in false. Should probably be: if ( $check['host'] == $home['host'] ).

#30 @ryan
9 years ago

  • Type changed from feature request to task (blessed)

#31 @DD32
9 years ago

line 121 and 169: if has_filter() condition then do_action() call.
Shouldn't that be either has_action() and do_action() or has_filter() and apply_filter() ?

Action, Filter, Same thing to WordPress. The difference is that when you call filters, it passes the value around, When you call action, it doesnt bother.

lines 549 and 1498, for example is_array( WP_ACCESSABLE_HOSTS ).

You are indeed correct, Thats why people should test the damn patches before submitting them!

I'm thinking a Comma seperated list would be better: define(.., 'localhost, api.wordpress.org, etc.someplace');

get_bloginfo('site_url') > 'site_url' is not a valid parameter

*Sigh* I'll get that fixed too.

line 539: if ( $uri == $homehost? )

Hm, Looking at the code, and the fact it checks specifically for 'localhost' I think its supposed to only pick up requests straight to localhost.. But you're right, It would be better based on parsing request.

I'll get a new patch based off this (and any other comments in the next 12~48hrs)

@DD32
9 years ago

#32 @DD32
9 years ago

attachment 4011.6.diff added

Refreshed patch with additions of comments from bforchhammer. (Yes, I tested that the Proxy allow/bypass code works too)

#33 @ryan
9 years ago

(In [10864]) Proxy support. Props DD32. see #4011

#34 @ionfish
9 years ago

Would it be possible to rename the constant to WP_ACCESSIBLE_HOSTS? 'Accessable' is a spelling mistake.

@jbsil
9 years ago

Changes to $accessible_hosts and WP_ACCESSIBLE_HOSTS in http.php

#35 @jbsil
9 years ago

  • Keywords commit removed

Patch above changes all occurrences of "accessable" to "accessible" in http.php. My search of other files in /wordpress +R did not find the word, so I think this covers it.

#36 @DD32
9 years ago

Patch above changes all occurrences of "accessable" to "accessible" in http.php

Cheers for that, I was wondering about the spelling of that whilst changing, but forgot to look it up..

#37 @jbsil
9 years ago

Should it be changed anywhere else that I missed? If not, should this be flagged as ready to commit again?

#38 @ryan
9 years ago

(In [10870]) Correct spelling. Props jbsil. see #4011

#39 @ryan
9 years ago

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

#40 @dd32
8 years ago

(In [13484]) Support non-BASIC Authentication schemes in the HTTP API if server supports them. Props ssandison, See #4011, Fixes #12200

Note: See TracTickets for help on using tickets.