Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better vomnibar favicons (fix for domains and security issues) #1210

Closed
wants to merge 24 commits into from
Closed

Better vomnibar favicons (fix for domains and security issues) #1210

wants to merge 24 commits into from

Conversation

smblott-github
Copy link
Collaborator

Fixes #1208; or, at least, it much approves on the situation. And it does handle domains.

This fetches favicons from Google. Helpfully, Google serves up a nice little "world" icon for anything it doesn't know what else to do with. So we seem to get at least an icon for pretty much every site.

I don't think this is the final word on the matter.

Stephen Blott added 3 commits October 29, 2014 13:19
Repair insert mode.
b27bd39 
I broke it here: 77e1ded
Revert 2c7bebb
a277fa6 
On reflection, this seems too dangerous:

   - Somebody excluded flash for a reason, and without knowing that
     reason, it seems dangerous.

   - Imagine a flash game with key bindings.  Perhaps it uses ? to
     show the key bindings, and ESC to hide them again.  With
     2c7bebb, you can never hide the
     key bindings (I think).

All in all, this just seems too risky.
Better vomnibar favicons.
a720ad9
@smblott-github smblott-github changed the title Better vomnibar favicons Better vomnibar favicons (particularly: favicons for domains) Oct 29, 2014
@mrmr1993
Copy link
Contributor

There are some serious privacy considerations to be discussed around using the google service for these. I am uncomfortable with the idea of shipping (a matching subset of) the user's open tabs, history, bookmarks or custom search engines to a 3rd party service every time vomnibar is opened.

Have you looked into using chrome://favicon/?

@smblott-github
Copy link
Collaborator Author

Have you looked into using chrome://favicon/

Yes, I looked at it. It wasn't very reliable, and (apparently) only deals up favicons which are actually in the cache.

@mrmr1993
Copy link
Contributor

Okay. I can get behind this if we shield the user from this by default with an option in the background page, then.

@philc
Copy link
Owner

philc commented Oct 29, 2014

Not comment on the privacy implications, but could we request /favicon.ico
from each domain, and show a globe icon if it doesn't load?
On Oct 29, 2014 3:17 PM, "Matthew Ryan" notifications@github.com wrote:

Okay. I can get behind this if we shield the user from this by default
with an option in the background page, then.

—
Reply to this email directly or view it on GitHub
#1210 (comment).

@smblott-github
Copy link
Collaborator Author

Not comment on the privacy implications, but could we request /favicon.ico
from each domain, and show a globe icon if it doesn't load?

I tried this, @philc. Misses are common enough that I saw a fair few of them.

For example, I have https://chrome.google.com/ in my history, but https://chrome.google.com/favicon.ico gives a 404 -- because the original page is actually redirected to https://www.google.com/chrome/.

@mrmr1993
Copy link
Contributor

If /favicon.ico doesn't load, couldn't we fall back to chrome://favicon. That way, we get a few extra urls matched and get consistency with chrome's no-favicon icon.

@mrmr1993
Copy link
Contributor

@smblott-github, presumably those are for bookmarks, since redirects don't register in history?

Since chrome caches icons for history and bookmarks to show in their UI, the ideal solution is to get the devs to expose them through the corresponding APIs. I'll go chromium issue hunting now.

@smblott-github
Copy link
Collaborator Author

and show a globe icon if it doesn't load?

If /favicon.ico doesn't load, couldn't we fall back to

(I wrote a long response, then deleted it, because it became too complicated.)

Bottom line: The Google URL in this PR is the best source of favicons I've seen. It's better than chrome://favicon (which misses stuff), and better than guessing (which sometimes gets it wrong). And the Google URL returns a nice little globe if it doesn't know what else to do -- which is the best we could do as a fallback position, in any case.

@deiga
Copy link
Contributor

deiga commented Oct 30, 2014

@smblott-github But @mrmr1993 is right, there is a huge privacy concern with sending all URLs directly to Google. There are a lot of people who would object to such automatic invasion of privacy. If we can't come up with a better solution, then I'd suggest at least make it an optional feature, which has to be enabled by the user. The option also needs a disclaimer that all URLs are sent to a Google server in case of activation

@smblott-github
Copy link
Collaborator Author

Yes, the privacy issue @mrmr1993 raises (and @deiga) supports is important. It's a shame, because Google does a pretty good job of finding the right favicon.

I'll revert to guessing the favicon URL, and see if we can't do a better job filling in the gaps where this doesn't work.

@smblott-github
Copy link
Collaborator Author

There's a serious security issue associated with guessing favicons.

Favicons are fetched in the security context of the host page. If that's https, then fetching http favicons would break the security policy. We can get around this by using scheme-independent URLs for favicons (e.g. //www.bbc.com/favicon.ico).

Unfortunately, for some sites, including www.bbc.com, the https URL is redirected to http, breaking the page's security policy anyway. And I don't think we can prevent such redirection, can we? I came across two examples of this, without searching too hard.

@mrmr1993: Would #1139 fix this issue?

@mrmr1993
Copy link
Contributor

@smblott-github yes, #1139 should fix this.

@smblott-github
Copy link
Collaborator Author

@mrmr1993: Unfortunately, #1139 doesn't fix this. I'll give more details over in #1139.

This was referenced Nov 1, 2014
Closed
Closed
@smblott-github
Copy link
Collaborator Author

This works now:

  • Favicons for domains (as was @philc's original point).
  • Favicons without errors due to the content security policy in effect.

It ended up being way more code than seems necessary (or, perhaps, is warranted) for this feature. All HTTP GET's are now performed on the background page. This gets around the security issue. In addition, partly to reduce the number of messages, but mainly to reduce the number of XMLHttpRequests, favicons are cached in the content script. (There are circumstances in which chrome itself appears not to be caching them.)

Every suggestion gets a favicon, although it may be a little globe (which here is "cached", but courtesy of Google). No requests are sent to Google (except those pertaining to Google itself).

mrmr1993
mrmr1993 reviewed Nov 2, 2014
View reviewed changes
background_scripts/main.coffee
@@ -56,11 +56,26 @@ chrome.runtime.onConnect.addListener((port, name) ->
port.onMessage.addListener(portHandlers[port.name])
)

chrome.runtime.onMessage.addListener((request, sender, sendResponse) ->
# With asynchronous responses to messages, we *must* call sendResponse (or we'll get a memory leak).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're worried about garbage collection because for some reason neither xhr.onerror nor xhr.onload might be called, I would recommend:

  • using ports (via chrome.runtime.connect):
    • ports have an onDisconnect listener; we can use this to do any cleanup we need in the frontend.
    • even without our intervention, the garbage collector will cause onDisconnect to be called for the port. (I can expand on this if it would help.)
    • we don't have to have the sendResponseWithTimeout machinery.
  • listening on xhr.onreadystatechange instead of seperately on xhr.onerror and xhr.onload, so we're less likely to have missed an event (and miss sending a response).
  • setting the timeout on xhr instead, with xhr.timeout = 121000, and possibly binding the xhr.ontimeout handler, if having a response within 2 minutes is still important.

@smblott-github smblott-github changed the title Better vomnibar favicons (particularly: favicons for domains) Better vomnibar favicons (fix for domains and security issues) Nov 4, 2014
@smblott-github
Copy link
Collaborator Author

This a biggish change, so I've been holding off from merging it. It seems to be working nicely for me. Has anybody else tried it out? (Or, as @philc might say, taken it for a spin?).

I intend to merge this in the next few days. So, please let me know if there are problems.

@smblott-github
Copy link
Collaborator Author

Reasons for merging:

  • Provides nice favicons; and always provides at least a pretty little globe.
  • Functionally, favicons add little value. But they do look great.
  • This PR tries more ways of fetching the page's favicon, including chrome://favicon/. So it's better at finding favicons than master.
  • Fixes a serious CSP issue that's currently in master.
  • If we don't merge, then we need to back out favicon support as it currently exists in master (which is not really a great reason to merge).

Reasons against merging:

  • It's quite a large chunk of new code.
  • Chrome seems (sometimes?) not to cache XMLHttpRequests. So we can be generating quite a few requests every time the vomnibar opens. Which is not being a great net citizen.

Possible improvements:

  • Implement a trivial cache of favicons (fetched via XMLHttpRequest) on the background page. This can be a simple map of URLs to base64-encoded favicons. No need for a smart cache. Just clear out all entries once a day and start again. However, this would be yet more code.

@deiga
Copy link
Contributor

deiga commented Nov 8, 2014

Would it be better to use the fetch API instead of XMLHttpRequests? It' supported in all latest browsers, and here's a polyfill for those that don't spport it https://github.com/github/fetch

@smblott-github
Copy link
Collaborator Author

Closing this for now. I'll resubmit a simpler solution later.

Conclusion:

  • Guessing favicon URLs is a bad idea. You make wrong guesses, so you have to bend over backwards to handle those elegantly. And even then you sometimes get the wrong favicon. An example is Google Docs, where there should different favicons depending upon the type of document in the tab.

We should either ditch favicons completely, or just rely on the best-effort favicons we get from chrome://favicon/ (as @mrmr1993 suggested, quite some time ago).

@smblott-github smblott-github mentioned this pull request Nov 9, 2014
Closed
@smblott-github
Copy link
Collaborator Author

Discussion moved to #1232.

@smblott-github smblott-github mentioned this pull request Nov 12, 2014
Merged
@smblott-github
Copy link
Collaborator Author

See #1239.

@mrmr1993 mrmr1993 mentioned this pull request May 9, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should domains have favicons shown in the vomnibar?
4 participants
@smblott-github @mrmr1993 @philc @deiga