1433700 - Favicons are still fetched even when webRequests should have been blocked by an add-on

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P3)

Description

[swleefers]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20161031133903

Steps to reproduce:

These are the steps reported by Gorhill ( https://github.com/gorhill/uMatrix/issues/925 ):

    Launch Firefox 57.0.4 with no tab opened.
        about:config settings:
            network.dns.disablePrefetch = true
            network.predictor.enabled = false
            network.prefetch-next = false
            network.http.speculative-parallel-limit = 0
        I have Firefox set to delete cache/cookies when closing Firefox.
    
    Go to about:debugging, load temporary minimalist extension: https://gist.github.com/gorhill/81e01119f78dac46ead5b8ba0c34bf18 (save locally both files in a new folder).

    Open browser console (ctrl-j), filter output with favicon. be sure "Net" and "Logging" are enabled.
    Open a new tab, load https://news.ycombinator.com/.
        Reported in console:

            GET https://news.ycombinator.com/favicon.ico

        Nothing logged by the extension, suggesting the request was not submitted to webRequest listeners.

    Open a new tab, load https://spreadprivacy.com/privacy-simplified/
        Reported in console:

            GET https://spreadprivacy.com/favicon.png

        Nothing logged by the extension, suggesting the request was not submitted to webRequest listeners.

    Open a new tab, load https://about.gitlab.com/2018/01/22/gitlab-10-4-released/
        Reported in console:

            blocking tabId=-1 requestId=757 url=https://about.gitlab.com/ico/favicon-192x192.png
            blocking tabId=-1 requestId=758 url=https://about.gitlab.com/ico/favicon-32x32.png
            onErrorOccurred: favicon request: 757
            onErrorOccurred: favicon request: 758
            GET https://about.gitlab.com/ico/favicon-32x32.png

            Extension canceled the requests, confirmed by onErrorOccurred listener, yet the browser console shows that a network request was still made for one of the favicon.



Actual results:

See above. The console reports that the favicon has been fetched. It also reports that the request was blocked in one case (not in the other cases), while it also reports that the favicon was fetched in that very same case. The extension logs that a web request was cancelled in that case. In the other cases, the extension logs nothing.


Expected results:

The favicon should not have been fetched in any of these cases. The console should not show "GET ..." either. The extension should show that the web request was cancelled in all cases. The extension should have been able to block the requests.

Comment 1

[CosminB]

Attachment: Bug1433700.png

I was able to reproduce this issue on Firefox 58.0.1 (20180128191252), Firefox 59.0b5 (20180128191456) and Firefox 60.0a1 (20180129220114) under Win 7 64-bit and Mac OS X 10.13.2.

Please have a look at the attached screenshot.

Comment 2

[Shane Caraveo (:mixedpuppy)]

I can certainly reproduce this.  ChannelWrapper.matches is returning false, presumably the favicon service is making a request with a system principal, which we do not allow access to.   DOMEventHandler.setIcon will set the principal to a system principal if it does not have a loading principal.  That is only one possible code path (I assume there are others).

This seems to have been introduced in bug 1401777, perhaps mak has input on why that was chosen.

Comment 3

[Marco Bonardo [:mak]]

In bug 1401777 I only moved the code from tabbrowser.setIcon to DOMEventHandler.setIcon. Tabbrowser was already falling back to the system principal in the initial code. The reason I think is just for the browser to directly invoke SetIcon for its own favicons without having to pass a principal explicitly. Content loaded pages should always pass a principal.
I don't know why we don't have a principal here, if that's really the problem, ContentLinkHandler.jsm seems to always pass one.
Could you please check the principal around the "Link:SetIcon" message when this happens?

Comment 4

[:Gijs (he/him)]

I expect the cause here is the same as bug 1297156, whatever the cause is...

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs (out until 26th) from comment #4)
> I expect the cause here is the same as bug 1297156, whatever the cause is...

Ah, hm, maybe there's another option. Does the page-icon protocol create its own channels, and/or pass its URI to the channels it creates (including the page-icon: bits) ? If so, perhaps we don't allow the webRequest access to it because of this.

Comment 6

[Marco Bonardo [:mak]]

page-icon only fetches local icons from the database, it never fetches remotely.

Comment 7

[Dave Townsend [:mossop]]

I'm expecting this to be fixed by bug 1297156

Comment 8

[Dave Townsend [:mossop]]

Fixed by bug 1453751