Closed Bug 1359417 Opened 7 years ago Closed 7 years ago

WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy - blocking Foxy Proxy

Categories

(WebExtensions :: Untriaged, enhancement, P3)

Product:
WebExtensions
Component:
Untriaged
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla55

People

(Reporter: ericjung, Assigned: andy+bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proxy])

Attachments

(2 files, 3 obsolete files)

Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
This code [0] should use PROXY_TYPES.PROXY not PROXY_TYPES.HTTPS:

  if (parts[0] == PROXY_TYPES.PROXY) {
    type = PROXY_TYPES.HTTPS;
  }


[0] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ProxyScriptContext.jsm#195

The logic should be something like this:

  let type = PROXY_TYPES.SOCKS;
  if (parts[0] == PROXY_TYPES.PROXY || parts[0] == PROXY_TYPES.HTTP) {
    type = PROXY_TYPES.PROXY;
  }
  else if (parts[0] == PROXY_TYPES.HTTPS) {
    type = PROXY_TYPES.HTTPS;
  }

I've added the new PROXY_TYPES.HTTP (the string "http") because PROXY_TYPE.PROXY is archaic and while it should be supported for legacy reasons, it's not modern in design.
Blocks: webextensions-proxy-api
Assignee: nobody → mwein
Priority: -- → P3
Whiteboard: [proxy][triaged]
Attachment #8865151 - Flags: review?(kmaglione+bmo)
Assignee: mwein → eric
Comment on attachment 8865151 [details]
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4

https://reviewboard.mozilla.org/r/136818/#review139850

Thanks!

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:163
(Diff revision 1)
>  
>  add_task(function* testSocksReturnType() {
>    yield testProxyScript({
>      scriptData() {
>        function FindProxyForURL(url, host) {
>          if (host === "www.mozilla.org") {

Could you update this test to check for the actual host (I thought it was supposed to be "www.mozilla.org"?) instead of setting `proxyInfo` to null?

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:190
(Diff revision 1)
>        failoverProxy: null,
>      },
>    });
>  });
>  
> +add_task(function* testSocksReturnType() {

Could you please use unique names for the tests - maybe `testSocks4ReturnType`?

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:224
(Diff revision 1)
> +      failoverProxy: null,
> +    },
> +  });
> +});
> +
> +add_task(function* testProxyReturnType() {

Please use unique names for the tests - maaybe `testHttpReturnType`?
Attachment #8865151 - Flags: review+
I've given a preliminary review - this will still need review by a peer (e.g. kmag).
Comment on attachment 8865151 [details]
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4

https://reviewboard.mozilla.org/r/136818/#review139850

> Could you update this test to check for the actual host (I thought it was supposed to be "www.mozilla.org"?) instead of setting `proxyInfo` to null?

The host can be anything hosting a proxy server. Since you specified http://www.mozilla.org/ in testProxyScript() as the host, then this code is correct. There's nothing stopped www.mozilla.org from hosting a proxy server.
Attachment #8865151 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
Comment on attachment 8866862 [details]
Bug 1359417 - WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy

https://reviewboard.mozilla.org/r/138448/#review142120

LGTM.  please run a try before landing.
Attachment #8866862 - Flags: review+
Attachment #8865151 - Attachment is obsolete: true
Attachment #8865151 - Flags: review?(mixedpuppy)
Attachment #8866862 - Attachment is obsolete: true
Attachment #8865151 - Attachment is obsolete: true
Attachment #8865151 - Flags: review?(kmaglione+bmo)
Attachment #8867308 - Flags: review?(mixedpuppy)
Comment on attachment 8867308 [details]
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4

https://reviewboard.mozilla.org/r/138834/#review142182

Looks good!

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:178
(Diff revision 1)
>        failoverProxy: null,
>      },
>    });
>  });
>  
> +add_task(function* testSocksReturnTypeNoConditional() {

nit: I think this test should be named `testSocksReturnType` and the one above it should be something like `testSocksReturnTypeWithHostCheck`.
Attachment #8867308 - Flags: review+
Comment on attachment 8867308 [details]
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4

https://reviewboard.mozilla.org/r/138834/#review142184

ok, this looks better.  run try via reviewboard, and be sure to run eslint.  Given that, r+

::: toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js:244
(Diff revision 1)
> +      port: "8080",
> +      type: "https",
> +      failoverProxy: null,
> +    },
> +  });
> +});

nit add empty line after functions, eslint should catch that.
Attachment #8867308 - Flags: review?(mixedpuppy) → review+
Attachment #8867308 - Attachment is obsolete: true
Attachment #8867345 - Flags: review?(mixedpuppy)
> run try via reviewboard

You do not have the required scm level to trigger a try build
https://reviewboard.mozilla.org/r/138870/
Comment on attachment 8867345 [details]
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4

https://reviewboard.mozilla.org/r/138872/#review142620

I also initiated a try run.
Attachment #8867345 - Flags: review?(mixedpuppy) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 78f6a16498f1 -d 27024b5ebdef: rebasing 396439:78f6a16498f1 "Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4 r=mixedpuppy" (tip)
merging toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js
warning: conflicts while merging toolkit/components/extensions/test/xpcshell/test_proxy_scripts.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(sescalante)
This patch needs to be rebased with Mercurial.  Eric estimates it would take someone familiar with Mercurial 15 minutes to do, versus hours to learn out of band.  Would it make sense for someone do the rebase (matt?, shane?) so the patch doesn't bitrot. Then at all-hands we'll do knowledge transfer in person on Mercurial process?

This unblocks FoxyProxy to migrate to webextensions.
Assignee: eric → nobody
Flags: needinfo?(sescalante)
Summary: WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy → WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy - blocking Foxy Proxy
Whiteboard: [proxy][triaged] → [proxy]
Comment on attachment 8871830 [details]
Bug 1359417 WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4

https://reviewboard.mozilla.org/r/143302/#review147030
Attachment #8871830 - Flags: review?(mixedpuppy) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6a87d5ea0d0f
WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy and Bug 1362798 WebExtensions ProxyAPI should support return strings HTTP and SOCKS4 r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/6a87d5ea0d0f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
Assignee: nobody → andy+bugzilla
status-firefox55: fixed → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: