1455705 - proxyConfig settings store incorrect prefs

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Shane Caraveo (:mixedpuppy)]

We passed urls into prefs where hostnames are required. non-socks settings fail due to this.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

proxyConfig set urls onto the pref rather than host names.  This adds a round-trip test
with a test that uses the proxy after setting the config.  Also fixes setting prefs
when httpProxyAll is true.

Review commit: https://reviewboard.mozilla.org/r/238576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238576/

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238576/diff/1-2/

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

https://reviewboard.mozilla.org/r/238576/#review244322

Thanks Shane, it looks good to me, follows some review comments (just some small nits or proposed small tweaks).

::: toolkit/components/extensions/parent/ext-browserSettings.js:202
(Diff revision 2)
> +    if (value.httpProxyAll) {
> +      // Match what about:preferences does with proxy settings
> +      // since the proxy service does not check the value
> +      // of share_proxy_settings.
> +      for (let prop of ["ftp", "ssl", "socks"]) {
> +        value[prop] = value.http;
> +      }
> +    }

Nit, How about moving this block that handle the "httpProxyAll" property right after the block that normalize all these values at line 390?

::: toolkit/components/extensions/parent/ext-browserSettings.js:394
(Diff revision 2)
> -                    new URL(url);
> +                    // Fixup in case a full url is passed.
> +                    if (host.includes("://")) {
> +                      host = value[prop] = new URL(host).host;
> +                    }
> +                    // Validate the host value.
> +                    new URL(`http://${host}`);

Given that we are executing "new URL" at line 399 in any case, to ensure the host is valid, how about avoiding the double assignment inside the `if`?

Something like:

```
// Fixup in case a full url is passed.
if (host.includes("://")) {
  host = new URL(host).host;
}

// Validate and retrieve the normalized "hostname:port" value.
value[prop] = new URL(`http://${host}`).host;

```

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:238
(Diff revision 2)
>      if (AppConstants.platform === "android") {
>        return Promise.resolve();
>      }
>  
>      let proxyConfig = {
> -      proxyType: "system",
> +      proxyType: "none",

is this change needed?
I'm not against it but it doesn't seem to make any difference based on what I saw.

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:301
(Diff revision 2)
>  
>    await testProxy(
>      {
>        proxyType: "manual",
>        http: "http://www.mozilla.org",
>        autoConfigUrl: "",

Nit, we could also add one of the other ones (e.g. ftp) to check that the http property overwrites it even when specified.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238576/diff/2-3/

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

https://reviewboard.mozilla.org/r/238576/#review244322

> is this change needed?
> I'm not against it but it doesn't seem to make any difference based on what I saw.

none is actually the default and i wanted to reflect that.  It otherwise doesn't matter.

> Nit, we could also add one of the other ones (e.g. ftp) to check that the http property overwrites it even when specified.

done

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238576/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bd0b3c256ab
fix how browserSettings.proxyConfig sets network prefs, r=rpl

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1bd0b3c256ab

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

Approval Request Comment
[Feature/Bug causing the regression]: 1425535
[User impact if declined]: some proxy configurations are unavailable
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: Possibly 1455755.  This bug fixes breakage of the api.  Since the api is broken, it is the ideal time to change the API, 1455755 does that.
[Is the change risky?]: no
[Why is the change risky/not risky?]: It changes what was set into preferences, is limited to a single api, and now has a complete test cycle that includes using a proxy after changing the settings to verify the settings work.
[String changes made/needed]: none

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

approved for 60.0b16 and 60.0rc

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: beta version of patch

Comment 13

[Shane Caraveo (:mixedpuppy)]

try run with this and bug 1455755
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44f15f0af1dc37cb26313d2537dfb9804c8d8483

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b45730352d20 (FIREFOX_60b_RELBRANCH)

Leaving 60 as affected until the m-r push.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/9bb4207e5c14
https://hg.mozilla.org/releases/mozilla-esr60/rev/9bb4207e5c14

Comment 16

[Victor Carciu]

Testing for this bug was covered by steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1455755#c15 and I am waiting for Shane's response for NI added in https://bugzilla.mozilla.org/show_bug.cgi?id=1455755#c37 in order to see if everything is okay.

Thanks

Comment 17

[Victor Carciu]

Shane responded and looks that everything is ok for FF 60, esr60 and Nightly. Marking issue as verified