Bug 69732 – <img crossorigin> should fail to load when CORS check fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[WebKit Review Bot]

<img crossorigin> should fail to load when CORS check fails
Requested by abarth on #webkit.

[Adam Barth]

Created attachment 112119 [details]
work in progress

[Adam Barth]

Created attachment 112130 [details]
Patch

[Darin Adler]

Comment on attachment 112130 [details]
Patch

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

It seems a little strange to do the entire load and then report it as if it was a failure once the load is done. It seems the load should instead fail as soon as we get the HTTP header before reading any more data, loosely speaking.

> Source/WebCore/loader/ImageLoader.cpp:243
> +    if (m_element->fastHasAttribute(HTMLNames::crossoriginAttr) && !resource->passesAccessControlCheck(m_element->document()->securityOrigin())) {

We normally do "using namespace HTMLNames" at the top of the file, and leave off the prefix.

[Adam Barth]

> It seems a little strange to do the entire load and then report it as if it was a failure once the load is done. It seems the load should instead fail as soon as we get the HTTP header before reading any more data, loosely speaking.

Yeah, we could do that.  It's easier to do at this layer because it handles the cache-hit and cache-miss cases uniformly.  I'd imagine the common case is for the check to pass though.

[WebKit Review Bot]

Comment on attachment 112130 [details]
Patch

Clearing flags on attachment: 112130

Committed r98215: <http://trac.webkit.org/changeset/98215>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #4)
> > It seems a little strange to do the entire load and then report it as if it was a failure once the load is done. It seems the load should instead fail as soon as we get the HTTP header before reading any more data, loosely speaking.
> 
> Yeah, we could do that.  It's easier to do at this layer because it handles the cache-hit and cache-miss cases uniformly.  I'd imagine the common case is for the check to pass though.

The CachedResource class could be changed call a function that says “we have the header but not the data” for both the cache-miss case and the cache-hit case. For the cache-hit case it would be called right before the call to notifyFinished.

What I like about this is not just the performance but the clarity that processing of the header goes in the header function and processing of the complete data and header goes in the data function and processing of partial data goes in the partial data function. Obviously notifyFinished could use a better name too.

[Adam Barth]

Yeah, that approach is consistent with CachedResource's design of synchronously generating the various callbacks during a cache hit in addClient.

I'll keep this issue in mind when looking at Nate's patch to make XMLHttpRequest over to the CachedResourceLoader pipeline.  XMLHttpRequest needs a full-featured CORS implementation, so it should be easier for ImageLoader to do this work once that's done.

[Boris Zbarsky]

Adam, should I expect the fix for this bug to show up in this Chrome version:

  Google Chrome	17.0.942.0 (Official Build 110446) dev
  OS	Mac OS X
  WebKit	535.8 (@100508)

?  As far as I can tell that Chrome version is loading the various images linked from the list on the left of http://maps.google.it/maps/ms?msid=211095647232644570760.00047466b28e95f25449b&msa=0 that use @crossorigin but don't send CORS headers....

[Adam Barth]

Thanks Boris.  I'll investigate.