Bug 172733 – CSP: Apply img-src directive to favicon loads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

For consistency we should apply the CSP img-src directive to favicon loads.

[Daniel Bates]

Part of <rdar://problem/32082654>

[Daniel Bates]

Created attachment 311547 [details]
Patch and layout tests

[Build Bot]

Comment on attachment 311547 [details]
Patch and layout tests

Attachment 311547 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3844914

New failing tests:
http/tests/security/contentSecurityPolicy/allow-favicon.html

[Build Bot]

Created attachment 311552 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311547 [details]
Patch and layout tests

Attachment 311547 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3844925

New failing tests:
http/tests/security/contentSecurityPolicy/allow-favicon.html

[Build Bot]

Created attachment 311553 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Daniel Bates]

Created attachment 311600 [details]
Patch and layout tests

[Darin Adler]

Comment on attachment 311600 [details]
Patch and layout tests

Why do we even have favicon support in WebKit any more? What client is using it? I am pretty sure Safari does not want to use this any more. WebKIt should just parse the filename; icon loading should be done outside WebKit.

[Brady Eidson]

(In reply to Darin Adler from comment #8)
> Comment on attachment 311600 [details]
> Patch and layout tests
> 
> Why do we even have favicon support in WebKit any more? What client is using
> it? I am pretty sure Safari does not want to use this any more. WebKIt
> should just parse the filename; icon loading should be done outside WebKit.

This is not right. 

Moving to a system where the client app gathers URLs on its own and loads them in its own networking context(s), many of the loads would fail or return incorrect results.

While there is tons of legacy "IconDatabase" code we can remove from WebKit soon related to disk storage and load decision making, WebKit still needs to do the actual loading of the icons itself, as directed by the client app.

Safari *is* using this new client interface.

[Brady Eidson]

Comment on attachment 311600 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=311600&action=review

> Source/WebKit2/ChangeLog:9
> +        Return 0 seconds for the maximum buffering time for favicons (no change in behavior).

This was going to be my comment - Looking at the source changes, it appears there's not actually a behavior change here.

What part(s) of the source changes are actually relevant here?

[Brady Eidson]

Comment on attachment 311600 [details]
Patch and layout tests

Removing the comment seems to be the only relevant part of this? Or I'm missing something.

[Brady Eidson]

Comment on attachment 311600 [details]
Patch and layout tests

Dan helped me realize where the behavior change was. Looks fine.

[Daniel Bates]

Committed r218015: <http://trac.webkit.org/changeset/218015>

[Daniel Bates]

(In reply to Daniel Bates from comment #13)
> Committed r218015: <http://trac.webkit.org/changeset/218015>

Removed CONSOLE MESSAGE line I inadvertently left in the expected result for test block-favicon.html from an earlier iteration of the test that did not call testRunner.queueReload() and committed this in <http://trac.webkit.org/changeset/218026>.

[Sam Weinig]

We have traditionally shied away from using the term favicon, and generally just used icon (or site icon sometimes I think). I don't see a compelling reason to start using this term now.

Also, does this new type represent all the different types of icons that may be in this class:
- traditional known location site icons (favicon.ico)
- <link rel="icon"> icons
- <link rel="mask-icon"> icons
- <link rel="apple-touch-icon"> icons

(Am I forgetting any?)

[Brady Eidson]

(In reply to Sam Weinig from comment #15)

> Also, does this new type represent all the different types of icons that may
> be in this class:...?

Yes, IconLoader is used for all of these.

[Sam Weinig]

(In reply to Brady Eidson from comment #16)
> (In reply to Sam Weinig from comment #15)
> 
> > Also, does this new type represent all the different types of icons that may
> > be in this class:...?
> 
> Yes, IconLoader is used for all of these.

Cool cool. Can we not use the name 'favicon' then?

[Daniel Bates]

(In reply to Sam Weinig from comment #17)
> (In reply to Brady Eidson from comment #16)
> > (In reply to Sam Weinig from comment #15)
> > 
> > > Also, does this new type represent all the different types of icons that may
> > > be in this class:...?
> > 
> > Yes, IconLoader is used for all of these.
> 
> Cool cool. Can we not use the name 'favicon' then?

Filed bug #173400 to use "icon" instead of "favicon".