proxyConfig settings store incorrect prefs

VERIFIED FIXED in Firefox -esr60

Status

defect
P1
normal
VERIFIED FIXED
Last year
Last year

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

58 Branch
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 verified, firefox59 wontfix, firefox60+ verified, firefox61 verified)

Details

Attachments

(2 attachments)

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

Updated

Last year
Assignee: nobody → mixedpuppy
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

Last year
mozreview-review
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.
Attachment #8969765 - Flags: review?(lgreco) → review+
Assignee

Updated

Last year
Blocks: 1455755
Comment hidden (mozreview-request)
Assignee

Comment 5

Last year
mozreview-review-reply
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
Assignee

Updated

Last year
Duplicate of this bug: 1455636
Comment hidden (mozreview-request)

Comment 8

Last year
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1bd0b3c256ab
fix how browserSettings.proxyConfig sets network prefs, r=rpl

Comment 9

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/1bd0b3c256ab
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
Attachment #8969765 - Flags: approval-mozilla-beta?
Comment on attachment 8969765 [details]
Bug 1455705 fix how browserSettings.proxyConfig sets network prefs,

approved for 60.0b16 and 60.0rc
Attachment #8969765 - Flags: approval-mozilla-release+
Attachment #8969765 - Flags: approval-mozilla-beta?
Attachment #8969765 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/b45730352d20 (FIREFOX_60b_RELBRANCH)

Leaving 60 as affected until the m-r push.

Comment 16

Last year
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

Last year
Shane responded and looks that everything is ok for FF 60, esr60 and Nightly. Marking issue as verified
Status: RESOLVED → VERIFIED

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.