Page MenuHomePhabricator

MWHttpRequest check for noproxy is inverted
Open, MediumPublic

Description

Bug T46113 claims to address this very statement which now reads

if ( $this->proxy || !$this->noProxy ) {
   return;

To break this out:

if (                        # if
     $this->proxy           # the proxy is already set
     ||                     # or
     !$this->noProxy        # noProxy is not set
   ) {
    return;

Instead, it should read "If the proxy is already set or noProxy is set":

if ( $this->proxy || $this->noProxy ) {
   return;

Details

Reference
bz66757

Related Objects

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:20 AM
bzimport set Reference to bz66757.
bzimport added a subscriber: Unknown Object (MLST).

Change 141366 had a related patch set uploaded by devunt:
Use correct condition for MWHttpRequest::proxySetup() function

https://gerrit.wikimedia.org/r/141366

Change 141374 had a related patch set uploaded by MarkAHershberger:
Use correct condition for MWHttpRequest::proxySetup() function

https://gerrit.wikimedia.org/r/141374

Change 141375 had a related patch set uploaded by MarkAHershberger:
Use correct condition for MWHttpRequest::proxySetup() function

https://gerrit.wikimedia.org/r/141375

Change 141376 had a related patch set uploaded by MarkAHershberger:
Use correct condition for MWHttpRequest::proxySetup() function

https://gerrit.wikimedia.org/r/141376

I think this change affects the logic in a non-intended way. The two conditions produce different results in case a proxy is set and proxy is disabled. In the original logic, the statement results in "false", and the following code is executed. In this proposed change, the proxy is unset, even if it was set before.

The change you are suggesting here leads to the proxy remaining set, if it was set before, even if we have the no proxy flag.

As Aaron Schulz and T.Gries pointed out in bug 44113, the former behaviour is inteded to bypass default settings on local urls.

The code that this repairs simply doesn't work in my testing. Granted, I may not be testing correctly, but I'd like to be shown that.

Set up a MW server that can only connect to the outside world via a proxy. Now, set $wgHTTPProxy so the MW can use the proxy.

Call Http::get( "http://www.google.com" )

The call will fail because noProxy is false by default (meaning, use the proxy!) and so the condition

if ( $this->proxy || !$this->noProxy ) {

will be true and setupProxy() will return without setting $this->proxy to $wgHttpProxy.

Change 141376 abandoned by Legoktm:
Use correct condition for MWHttpRequest::proxySetup() function

Reason:
Not merged into master yet

https://gerrit.wikimedia.org/r/141376

Change 141375 abandoned by Legoktm:
Use correct condition for MWHttpRequest::proxySetup() function

Reason:
Not merged into master yet

https://gerrit.wikimedia.org/r/141375

Change 141374 abandoned by Legoktm:
Use correct condition for MWHttpRequest::proxySetup() function

Reason:
Not merged into master yet

https://gerrit.wikimedia.org/r/141374

Clearing backport flag since nothing has been merged into master yet, and the patch has a -2 on it.

matej_suchanek removed a project: Patch-For-Review.
matej_suchanek set Security to None.
matej_suchanek removed a subscriber: wikibugs-l-list.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 27 2016, 10:06 AM