proxy.onRequest not honored for addon updates

VERIFIED FIXED in Firefox -esr60

Status

P1
normal
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: hermes1983, Assigned: mixedpuppy)

Tracking

({regression})

61 Branch
mozilla63
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr6062+ verified, firefox61 wontfix, firefox62+ verified, firefox63+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 months ago
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.

Updated

8 months ago
Component: Untriaged → Request Handling
Product: Firefox → WebExtensions

Updated

8 months ago
Flags: needinfo?(mixedpuppy)
Summary: proxy.onRequest not honored for about:extensions → proxy.onRequest not honored for addon updates
(Assignee)

Comment 1

8 months ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mixedpuppy)
Priority: -- → P1
(Assignee)

Updated

8 months ago
Assignee: nobody → mixedpuppy
(Assignee)

Comment 2

8 months ago
The fix would be to move the canAccess check inside the !isProxy block.  See what kris thinks.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 3

8 months ago
That or don't pass the policy into matches.
(Assignee)

Comment 4

8 months ago
The fix can happen in the extension proxy code.
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

8 months ago
mozreview-review
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?
(Assignee)

Updated

8 months ago
Depends on: 1479565
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8994494 - Attachment is obsolete: true
Attachment #8994494 - Flags: review?(aswan)

Comment 11

8 months ago
mozreview-review
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"
Attachment #8997084 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)

Comment 13

8 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/491a946ba31f
allow proxy to work on restricted domains, r=aswan

Comment 14

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/491a946ba31f
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1484178
(Assignee)

Comment 16

7 months ago
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
Attachment #8997084 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(mixedpuppy)
Attachment #8997084 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

7 months ago
Blocks: 1415644
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 18

7 months ago
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.
Attachment #8997084 - Flags: approval-mozilla-esr60?
status-firefox61: --- → wontfix
status-firefox62: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → affected
tracking-firefox62: --- → +
tracking-firefox63: --- → +
tracking-firefox-esr60: --- → 62+
Keywords: regression
Flags: qe-verify+

Comment 20

7 months ago
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.
Status: RESOLVED → VERIFIED

Updated

7 months ago
status-firefox62: fixed → verified
status-firefox63: fixed → verified
Flags: qe-verify+

Comment 21

7 months ago
Posted image Postfix video
Comment on attachment 8997084 [details]
Bug 1476570 - allow proxy to work on restricted domains,

Thanks, taking it for ESR 60.2 as well.
Attachment #8997084 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Comment 23

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr60/rev/594d22a711b0
status-firefox-esr60: affected → fixed
Flags: in-testsuite+
Flags: qe-verify+

Comment 24

7 months ago
Verified as fixed in ESR 60.2 as well.

Updated

7 months ago
status-firefox-esr60: fixed → verified
Flags: qe-verify+

Comment 25

7 months ago
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?
You need to log in before you can comment on or make changes to this bug.