1476570 - proxy.onRequest not honored for addon updates

Status: VERIFIED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Adrien CLERC]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180621125625

Steps to reproduce:

Using Firefox 61 and SwitchyOmega 2.5.16, using the new proxy.onRequest API.
I have no special proxy rule for addons.mozilla.org, the default proxy is the one of my company.
No proxy is set into Firefox preferences panel.


Actual results:

I am unable to update my addons. Using netstat, I see that Firefox is trying to do some direct connections to addons.mozilla.org.


Expected results:

Firefox should use the proxy rules as defined by SwitchyOmega.

This was working with SwitchyOmega 2.5.15, using the old API.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Took a little digging, but yes, there are urls that will not proxy via onRequest.  We do not bypass the canAccess check on the finalUrl.  So extensions in the pref extensions.webextensions.restrictedDomains will not get proxied this way.

https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/toolkit/components/extensions/ProxyScriptContext.jsm#314

https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#523

Comment 2

[Shane Caraveo (:mixedpuppy)]

The fix would be to move the canAccess check inside the !isProxy block.  See what kris thinks.

Comment 3

[Shane Caraveo (:mixedpuppy)]

That or don't pass the policy into matches.

Comment 4

[Shane Caraveo (:mixedpuppy)]

The fix can happen in the extension proxy code.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1476570 - allow proxy to work on restricted domains,

Proxies must work with all requests, however the new onRequest proxy api
passed the policy, which results in a check against restricted domains.
This change keeps url filtering (choosen by extension) in place while
removing any other restrictions.

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

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8994494 [details]
Bug 1476570 - allow proxy to work on restricted domains,

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8994494 [details]
Bug 1476570 - allow proxy to work on restricted domains,

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8994494 [details]
Bug 1476570 - allow proxy to work on restricted domains,

https://reviewboard.mozilla.org/r/259048/#review267376

::: toolkit/components/extensions/ProxyScriptContext.jsm:314
(Diff revision 3)
> -      if (wrapper.matches(filter, this.extension.policy, {isProxy: true})) {
> +      // We need to handle all the matching ourselves since wrapper.matches
> +      // calls WebExtensionPolicy::CanAccessURI which, in addition to
> +      // checking allowedOrigins also rejects restricted URIs.

how about adding a flag to ChannelWrapper::Matches() instead of duplicating (part of) its logic here?

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8994494 [details]
Bug 1476570 - allow proxy to work on restricted domains,

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

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1476570 - allow proxy to work on restricted domains,

Proxies must work with all requests, however the new onRequest proxy api
has maching logic using ChannelWrapper which uses WebExtensionPolicy which
checks against restricted domains.  We need to bypass that check when
matching for proxy requests.

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

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

https://reviewboard.mozilla.org/r/261006/#review268070

::: dom/chrome-webidl/WebExtensionPolicy.webidl:100
(Diff revision 1)
>    static readonly attribute boolean isExtensionProcess;
>  
>    /**
>     * Returns true if the extension has cross-origin access to the given URI.
>     */
> -  boolean canAccessURI(URI uri, optional boolean explicit = false);
> +  boolean canAccessURI(URI uri, optional boolean explicit = false, optional boolean checkRestricted = true);

You don't need to change the webidl if you're not ever passing this flag from js...

::: toolkit/components/extensions/test/xpcshell/test_proxy_listener.js:182
(Diff revision 1)
>  add_task(async function test_passthrough() {
>    let ext1 = await getExtension(null);
>    let ext2 = await getExtension({host: "1.2.3.4", port: 8888, type: "http"});
>  
> -  let proxyInfo = await getProxyInfo();
> +  // Also use a restricted url to test the ability to proxy those.
> +  let proxyInfo = await getProxyInfo("http://addons.mozilla.org");

nit: https and trailing slash

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:525
(Diff revision 1)
>      return false;
>    }
>  
>    if (aExtension) {
> -    if (!aExtension->CanAccessURI(urlInfo)) {
> +    bool isProxy = aOptions.mIsProxy && aExtension->HasPermission(nsGkAtoms::proxy);
> +    // We do not want to fail accessing restricted domains when matching for a

double negative, how about "Proxies are allowed to access all domains"

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/491a946ba31f
allow proxy to work on restricted domains, r=aswan

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/491a946ba31f

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

I completely forgot to request this uplift until we got another bug report for it.

Approval Request Comment
[Feature/Bug causing the regression]: addon updating with proxies
[User impact if declined]: addons wont update if using proxies
[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]: steps are in the bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: it's pretty low risk
[Why is the change risky/not risky?]: minimal change, been on nightly a while
[String changes made/needed]: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

Approved for 62 RC1. Does this need an ESR60 request as well?

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: addons may not update if using proxy
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca4677eb2510 (FIREFOX_62b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/6bbd47033815

Comment 20

[Victor Carciu]

Verified the issue with following results:

1.I was able to reproduce it in FF61 with SwitchyOmega 2.5.16
2.I was not able to reproduce in FF61 with SwitchyOmega 2.5.16, using the old API.
3.Issue is fixed in FF 62 and FF63 using SwitchyOmega 2.5.16 with the new API

I will mark the issue as verified and I will add a postfix screenshot.

Comment 21

[Victor Carciu]

Attachment: Postfix video

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

Thanks, taking it for ESR 60.2 as well.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/594d22a711b0

Comment 24

[Victor Carciu]

Verified as fixed in ESR 60.2 as well.

Comment 25

[Andrew Ducker]

This hasn't fixed 1484178 for me.

Tab Center Redux is showing as 0.6.3 in my Addons page, as 0.7.1 on https://addons.mozilla.org/en-US/firefox/addon/tab-center-redux/ - but clicking "Check for Updates" still says "No updates found".

Shall I open a new bug?  Reopen 1484178 or something else?