pkotwicz: Would you be the right owner for this? Assigning to you since this seemed at least somewhat similar to issue 440322.

Chromium folks: our canonical test site for this is https://mixed-favicon.badssl.com/

So, I'm not sure what the right behaviour should be.
On the second page load, there are no insecure requests.

Yes, the page shows cached data that was retrieved over HTTP, but there are other ways the page could be using locally stored/cached data from an insecure source.

In any case, this is definitely known and should be unrestricted.

pkotwicz@, do you know how our favicon retrieval and caching works? Maybe someone from our team should sit down and look at all of our favicon handling for edge cases.

So I get where this is coming from.  There's a Chromium 'flag' in v49.0.2623.110 that lets the user 'mark insecure origins as insecure' or 'dubious'.

Perhaps the flag could be removed and instead mark insecure origins as insecure by default.

Sorry that I missed the messages. Yes, I know how favicon retrieval and caching works. Favicons are stored in a special cache (not the HTTP cache) because the favicons associated with bookmarks should not be cleared when a user clears history.

The favicon cache is a SQLite database. The comment at the top of components/history/core/browser/thumbnail_database.cc
 has a description of the database schema.

It would not be too hard to add FaviconStatus::insecure_ in NavigationEntryImpl::favicon_

In order for the "lock icon in the location bar" to take the favicon into account:
- A new column needs to be added to the favicon SQLite database to store whether a favicon is secure or not (can this be determined from the favicon URL???)
- The "lock icon in the location bar" would need to be updated once the favicon is determined

Assigning to lgarron@ to decide whether this bug is worth pursuing.

Assigning to lgarron@ to decide whether this bug is worth pursuing and what priority this bug should have

Tentatively assigning priority.

One proposal we discussed this weekend is simply forbidding mixed content favicons, since the impact is small, and the downgrade of the page's security state probably trumps the impact of not showing the favicon in most cases.

+1 to #11

 Issue 795288  has been merged into this issue.

I think the simplest CL for this would change WebURLRequest::kRequestContextFavicon to be "Blockable" instead of "Optionally-blockable" in WebMixedContent.cpp. My reading of the mixed content spec [1] is this is not really disallowed, although an alternate reading could be that favicons fall under the category of "images" and so should be considered optionally blockable [2].

This is a +1/-1 CL for that: https://crrev.com/c/849292

The other side effect of this proposed change would be that a mixed-content Favicon would cause a mixed-content shield and console error (before it would just cause a console warning).

It could be possible to separately check for this case in the favicon component before it tried to initiate the fetch for the favicon in the renderer (for content, that might be in ContentFaviconDriver::DownloadImage [3]; for iOS the equivalent is WebFaviconDriver::DownloadImage [4]). The upside there is we could also handle this in the same way on iOS.

[1] https://w3c.github.io/webappsec-mixed-content/#category-optionally-blockable
[2] https://fetch.spec.whatwg.org/#concept-request-destination
[3] https://cs.chromium.org/chromium/src/components/favicon/content/content_favicon_driver.cc?l=125
[4] https://cs.chromium.org/chromium/src/components/favicon/ios/web_favicon_driver.mm?l=70

@mkwst: As someone more familiar with the vagaries of mixed content, does #16 seem reasonable to you? Would we need to go through an Intent-to-Deprecate?

Blocking non-secure favicons sounds totally reasonable to me. I thought we were already doing that, honestly. :)

Great! I'll add some tests (since making this change didn't break any existing tests...).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06f50527479fd046529e2c10d1d05c7215af823a

commit 06f50527479fd046529e2c10d1d05c7215af823a
Author: Christopher Thompson <cthomp@chromium.org>
Date: Thu Jan 25 01:24:19 2018

Treat mixed Favicon requests as Blockable

This changes the handling of kRequestContextFavicon to be Blockable
mixed content rather than Optionally-blockable. This means that insecure
Favicons requested for secure sites will be blocked and an error message
logged to the console, but they will not downgrade the security state of
the page.

Bug:  611731 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ibd6422ddc9e4bafd0b03ead587ad6b843471096a
Reviewed-on: https://chromium-review.googlesource.com/849292
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531772}
[modify] https://crrev.com/06f50527479fd046529e2c10d1d05c7215af823a/chrome/browser/favicon/content_favicon_driver_browsertest.cc
[add] https://crrev.com/06f50527479fd046529e2c10d1d05c7215af823a/chrome/test/data/favicon/page_with_favicon_by_url.html
[modify] https://crrev.com/06f50527479fd046529e2c10d1d05c7215af823a/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/06f50527479fd046529e2c10d1d05c7215af823a/third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp
[modify] https://crrev.com/06f50527479fd046529e2c10d1d05c7215af823a/third_party/WebKit/Source/platform/exported/WebMixedContent.cpp

Fix LGTM in 66.0.3331.0 on https://mixed-favicon.badssl.com