Closed Bug 1455705 Opened 2 years ago Closed 2 years ago

proxyConfig settings store incorrect prefs

Categories

(WebExtensions :: Untriaged, defect, P1)

58 Branch
defect

Tracking

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

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- verified
firefox59 --- wontfix
firefox60 + verified
firefox61 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(2 files)

We passed urls into prefs where hostnames are required. non-socks settings fail due to this.
Assignee: nobody → mixedpuppy
Priority: -- → P1
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+
Blocks: 1455755
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
Duplicate of this bug: 1455636
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1bd0b3c256ab
fix how browserSettings.proxyConfig sets network prefs, r=rpl
https://hg.mozilla.org/mozilla-central/rev/1bd0b3c256ab
Status: NEW → RESOLVED
Closed: 2 years ago
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.
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
Shane responded and looks that everything is ok for FF 60, esr60 and Nightly. Marking issue as verified
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.