Closed
Bug 1359417
Opened 8 years ago
Closed 7 years ago
WebExtensions ProxyAPI uses HTTPS proxy instead of HTTP proxy - blocking Foxy Proxy
Categories
(WebExtensions :: Untriaged, enhancement, P3)
WebExtensions
Untriaged
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla55
People
(Reporter: ericjung, Assigned: andy+bugzilla)
References
(Blocks 1 open bug)
Details
(Whiteboard: [proxy])
Attachments
(2 files, 3 obsolete files)
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•8 years ago
|
Blocks: webextensions-proxy-api
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Priority: -- → P3
Whiteboard: [proxy][triaged]
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8865151 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Updated•8 years ago
|
Assignee: mwein → eric
Comment 2•8 years 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 years ago
|
||
I've given a preliminary review - this will still need review by a peer (e.g. kmag).
Reporter | ||
Comment 4•8 years 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) |
Reporter | ||
Comment 6•8 years ago
|
||
New commit: https://reviewboard.mozilla.org/r/138448/
Updated•8 years ago
|
Attachment #8865151 -
Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
Comment 7•8 years 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+
Updated•8 years ago
|
Attachment #8865151 -
Attachment is obsolete: true
Attachment #8865151 -
Flags: review?(mixedpuppy)
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8866862 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8865151 -
Attachment is obsolete: true
Attachment #8865151 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8867308 -
Flags: review?(mixedpuppy)
Comment 10•8 years 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 years 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 years ago
|
Attachment #8867308 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8867345 -
Flags: review?(mixedpuppy)
Reporter | ||
Comment 13•8 years 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 years 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+
Comment hidden (mozreview-request) |
Comment 17•8 years 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 years ago
|
Flags: needinfo?(sescalante)
Comment 18•7 years 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•7 years 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•7 years 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
Comment 22•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•6 years ago
|
Assignee: nobody → andy+bugzilla
status-firefox55:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•