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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: ericjung, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [proxy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 months ago
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.
(Reporter)

Updated

9 months ago
Blocks: 1283639

Updated

9 months ago
Assignee: nobody → mwein
Priority: -- → P3
Whiteboard: [proxy][triaged]
Comment hidden (mozreview-request)
(Reporter)

Updated

9 months ago
Attachment #8865151 - Flags: review?(kmaglione+bmo)
(Reporter)

Updated

9 months ago
Assignee: mwein → eric

Comment 2

8 months ago
mozreview-review
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+

Comment 3

8 months ago
I've given a preliminary review - this will still need review by a peer (e.g. kmag).
(Reporter)

Comment 4

8 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Attachment #8865151 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)

Comment 7

8 months ago
mozreview-review
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)
Comment hidden (mozreview-request)
(Reporter)

Updated

8 months ago
Attachment #8866862 - Attachment is obsolete: true
(Reporter)

Updated

8 months ago
Attachment #8865151 - Attachment is obsolete: true
Attachment #8865151 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
(Reporter)

Updated

8 months ago
Attachment #8867308 - Flags: review?(mixedpuppy)

Comment 10

8 months ago
mozreview-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/#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 11

8 months ago
mozreview-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+
(Reporter)

Updated

8 months ago
Attachment #8867308 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Reporter)

Updated

8 months ago
Attachment #8867345 - Flags: review?(mixedpuppy)
(Reporter)

Comment 13

8 months ago
> run try via reviewboard

You do not have the required scm level to trigger a try build
https://reviewboard.mozilla.org/r/138870/

Comment 14

8 months ago
mozreview-review
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+
Duplicate of this bug: 1362798
Comment hidden (mozreview-request)

Comment 17

8 months ago
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)

Updated

8 months ago
Flags: needinfo?(sescalante)

Comment 18

8 months ago
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 hidden (mozreview-request)

Comment 20

8 months ago
mozreview-review
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+

Comment 21

8 months ago
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
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.