Bug 61015 – Support crossorigin property for images

• Component: DOM
• Status: RESOLVED FIXED

[Kenneth Russell]

Based on feedback from the WebGL working group, the WHATWG has added a cross-origin property to image, video and audio elements allowing CORS requests to be issued for their download. This was needed because the WebGL specification will need to be updated to ban the use of cross-origin media for security reasons.

Support for this new property is needed to allow WebGL applications using cross-origin media to continue to work.

Related links:

http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#the-img-element
http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#cors-settings-attribute

[Kenneth Russell]

Note that a bug was just filed about the naming of the new attribute. This would be trivial to change so shouldn't block implementation of the core feature.

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12679

[Adam Barth]

I suspect this will be difficult to implement, but I look forward to your patch.

[Adam Barth]

I've started looking at this.

[Kenneth Russell]

Note that the name of the new attribute was in fact just changed per http://www.w3.org/Bugs/Public/show_bug.cgi?id=12679 .

[Alexey Proskuryakov]

For those of us following along at home, what's the purpose of this content attribute? How does it affect the request (which is presumably still a "simple" one)?

[Adam Barth]

> For those of us following along at home, what's the purpose of this content attribute? How does it affect the request (which is presumably still a "simple" one)?

The attribute has two effects:

1) The request becomes a CORS request, so it gets an outgoing Origin header.  I'll have to read the spec, but the image load might be blocked if CORS says no.

2) The attribute lets you control whether to make an anonymous request (as in CORS), so you can strip cookies / HTTP auth from the request if you like.

Apparently we can't just always use non-anonymous CORS for image loads because some servers get sad when you send them unexpected Origin headers.

[Adam Barth]

I forgot the mention that the point of the attribute is to let you draw images onto 2D and 3D canvases and read back the results.  In the case of 3D canvas, my understanding is that a thumbs up for CORS is going to be required for drawing the image at all.

[Adam Barth]

Created attachment 94842 [details]
Work in progress

[Adam Barth]

Created attachment 94864 [details]
Work in progress

[Adam Barth]

Created attachment 94870 [details]
Work in progress

[Adam Barth]

Audio and Video will be a separate patch.

[Adam Barth]

Created attachment 94880 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 94880 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:409
> +SecurityOrigin* HTMLCanvasElement::securityOrigin() const

Are we changing this because SecurityOrigin* can sometimes be NULL?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1584
> +    bool originClean = false;
> +    if (cachedImage->image()->hasSingleSecurityOrigin()) {
> +        originClean = cachedImage->passesAccessControlCheck(canvas()->securityOrigin())
> +            || !canvas()->securityOrigin()->taintsCanvas(cachedImage->response().url());
> +    }

I would have just made this a helper.  isOriginClean(canvas(), cachedImage); ten you can use early return, etc.

> Source/WebCore/loader/ImageLoader.cpp:168
> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);

Where is this free-function defined?  (Why is it free?)  And should we just be passing the crossOriginMode instead?  I wonder if this whole block shouldn't be a separate function in ImageLoader, and then this call is just:

updateAccessControlsForCrossOriginMode(crossOriginMode)

> Source/WebCore/loader/cache/CachedResource.cpp:179
> +    return WebCore::passesAccessControlCheck(m_response, resourceRequest().allowCookies(), securityOrigin, errorDescription);

Do you need/want the WebCore:: here?  Should this function take a String* or StringImpl* since this is an optional return?

[Adam Barth]

(In reply to comment #13)
> (From update of attachment 94880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94880&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:409
> > +SecurityOrigin* HTMLCanvasElement::securityOrigin() const
> 
> Are we changing this because SecurityOrigin* can sometimes be NULL?

No, but SecurityOrigin is a pointer-type not a reference-type everywhere else in the code.  This code is just randomly different.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1584
> > +    bool originClean = false;
> > +    if (cachedImage->image()->hasSingleSecurityOrigin()) {
> > +        originClean = cachedImage->passesAccessControlCheck(canvas()->securityOrigin())
> > +            || !canvas()->securityOrigin()->taintsCanvas(cachedImage->response().url());
> > +    }
> 
> I would have just made this a helper.  isOriginClean(canvas(), cachedImage); ten you can use early return, etc.

Ok.

> > Source/WebCore/loader/ImageLoader.cpp:168
> > +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> 
> Where is this free-function defined?

CrossOriginAccessControl.h

> (Why is it free?)

Because it has no associated state.

> And should we just be passing the crossOriginMode instead?

Nope.  updateRequestForAccessControl is a lower-level primitive shared by all CORS APIs.  crossOriginMode is specific to this API.

> I wonder if this whole block shouldn't be a separate function in ImageLoader, and then this call is just:
> 
> updateAccessControlsForCrossOriginMode(crossOriginMode)

It's eventually going to be shared by Images, Audio, and Video.  My plan was to move it somewhere else when implementing those APIs since the correct place will be more obvious then.

> > Source/WebCore/loader/cache/CachedResource.cpp:179
> > +    return WebCore::passesAccessControlCheck(m_response, resourceRequest().allowCookies(), securityOrigin, errorDescription);
> 
> Do you need/want the WebCore:: here?

It's needed to call the correct function because the free function and the member function have the same name.

> Should this function take a String* or StringImpl* since this is an optional return?

Dunno.  I'm not sure it matters that much.  I guess we could pass 0, which might be slightly better.  Seems like an issue for another patch.

[Adam Barth]

Created attachment 95011 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 95011 [details]
Patch

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

OK.  I didn't review the tests particularly closely.  If you need me to, I'm happy to take another look.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:92
> +static bool isOriginClean(CachedImage* cachedImage, SecurityOrigin* securityOrigin)

OMG, so much nicer, thank you.

> Source/WebCore/loader/ImageLoader.cpp:168
> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);

I really dislike these being free functions.  We should consider namspacing them in a class at some point.

[WebKit Review Bot]

Comment on attachment 95011 [details]
Patch

Attachment 95011 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8739244

New failing tests:
canvas/philip/tests/2d.pattern.paint.norepeat.outside.html
canvas/philip/tests/2d.pattern.paint.repeatx.outside.html
canvas/philip/tests/2d.composite.globalAlpha.imagepattern.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
canvas/philip/tests/2d.pattern.paint.norepeat.coord1.html
canvas/philip/tests/2d.pattern.paint.repeat.basic.html
canvas/philip/tests/2d.pattern.paint.repeat.coord1.html
canvas/philip/tests/2d.pattern.modify.image1.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
canvas/philip/tests/2d.pattern.animated.gif.html
canvas/philip/tests/2d.pattern.paint.repeat.outside.html
canvas/philip/tests/2d.pattern.modify.image2.html
canvas/philip/tests/2d.pattern.paint.norepeat.coord2.html
canvas/philip/tests/2d.pattern.paint.orientation.image.html
canvas/philip/tests/2d.pattern.paint.norepeat.basic.html
canvas/philip/tests/2d.pattern.paint.repeat.coord3.html
canvas/philip/tests/2d.pattern.paint.repeat.coord2.html
canvas/philip/tests/2d.pattern.paint.repeatx.basic.html
canvas/philip/tests/2d.pattern.crosscanvas.html
canvas/philip/tests/2d.pattern.basic.image.html

[WebKit Review Bot]

Created attachment 95017 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

Comment on attachment 95011 [details]
Patch

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

>> Source/WebCore/loader/ImageLoader.cpp:168
>> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> 
> I really dislike these being free functions.  We should consider namspacing them in a class at some point.

I guess beauty is in the eye of the beholder. I like them as free functions.

[Eric Seidel (no email)]

(In reply to comment #19)
> (From update of attachment 95011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review
> 
> >> Source/WebCore/loader/ImageLoader.cpp:168
> >> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> > 
> > I really dislike these being free functions.  We should consider namspacing them in a class at some point.
> 
> I guess beauty is in the eye of the beholder. I like them as free functions.

So true. :)  I find it confusing that he has to WebCore:: namespace them in one place.  We have very few free functions in WebCore.  But I admit to not having looked at these in great detail.

[Adam Barth]

> I find it confusing that he has to WebCore:: namespace them in one place.

I can tweak some of the names so they don't collide.  Then we won't need the WebCore prefix.

[Adam Barth]

> I didn't review the tests particularly closely.  If you need me to, I'm happy to take another look.

The tests are based on the existing image-canvas tainting tests, but with the expected results tweaked to match the CORS status of the request/image.

[Adam Barth]

Created attachment 95032 [details]
Patch for landing

[Darin Adler]

Comment on attachment 95011 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.h:118
> -    const SecurityOrigin& securityOrigin() const;
> +    SecurityOrigin* securityOrigin() const;

I kind of like the old signature. It’s kind of weak to offer a security origin object that you can mutate for a canvas. We don’t want a caller to get it and then call something like setDomainFromDOM on it, do we? What’s the rationale here?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> +    if (cachedImage->image()->hasSingleSecurityOrigin())
> +        return false;

Is there a ! missing here?

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:73
> -    checkOrigin(KURL(KURL(), video->currentSrc()));
> +    // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
> +    checkOrigin(KURL(ParsedURLString, video->currentSrc()));

Why change to this intermediate state? I am hoping we can get rid of ParsedURLString soon. Can’t we wait and not touch this call site until we fixed currentSrc to return a URL?

> Source/WebCore/loader/ImageLoader.cpp:164
> +        String crossOriginMode = m_element->getAttribute(HTMLNames::crossoriginAttr);

Could use fastGetAttribute. That’s always allowed for any attribute that is guaranteed to not be an SVG animated attribute or the style attribute. And it’s faster.

> Source/WebCore/loader/ImageLoader.cpp:167
> +            DEFINE_STATIC_LOCAL(String, useCredentials, ("use-credentials"));
> +            bool allowCredentials = equalIgnoringCase(crossOriginMode, useCredentials);

I’m not sure that having a static local string is a more efficient way to do the equality check. You’d think that an equality check with a const char* could be super fast.

[Adam Barth]

(In reply to comment #24)
> (From update of attachment 95011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.h:118
> > -    const SecurityOrigin& securityOrigin() const;
> > +    SecurityOrigin* securityOrigin() const;
> 
> I kind of like the old signature. It’s kind of weak to offer a security origin object that you can mutate for a canvas. We don’t want a caller to get it and then call something like setDomainFromDOM on it, do we? What’s the rationale here?

The rationale is just consistency with everywhere else where we handle SecurityOrigin objects.  In this case, we want to pass the object to other functions.  There was already code that took the address of the reference, which seems somewhat goofy.

Maybe "SecurityOrigin const*" would be the best type?

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> > +    if (cachedImage->image()->hasSingleSecurityOrigin())
> > +        return false;
> 
> Is there a ! missing here?

Yep.  I added it back in the final patch.  (It causes a massive number of test failures, thankfully.)
 
> > Source/WebCore/html/canvas/CanvasRenderingContext.cpp:73
> > -    checkOrigin(KURL(KURL(), video->currentSrc()));
> > +    // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
> > +    checkOrigin(KURL(ParsedURLString, video->currentSrc()));
> 
> Why change to this intermediate state? I am hoping we can get rid of ParsedURLString soon. Can’t we wait and not touch this call site until we fixed currentSrc to return a URL?

I started down that path, but the scope was bigger than I wanted to include in this patch.  I'll do that in Bug 61578.

> > Source/WebCore/loader/ImageLoader.cpp:164
> > +        String crossOriginMode = m_element->getAttribute(HTMLNames::crossoriginAttr);
> 
> Could use fastGetAttribute. That’s always allowed for any attribute that is guaranteed to not be an SVG animated attribute or the style attribute. And it’s faster.

Thanks.  I wasn't familiar with the details of when you can use fastGetAttribute.  I'll fix that.
 
> > Source/WebCore/loader/ImageLoader.cpp:167
> > +            DEFINE_STATIC_LOCAL(String, useCredentials, ("use-credentials"));
> > +            bool allowCredentials = equalIgnoringCase(crossOriginMode, useCredentials);
> 
> I’m not sure that having a static local string is a more efficient way to do the equality check. You’d think that an equality check with a const char* could be super fast.

Will do.

[Adam Barth]

Created attachment 95078 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 95078 [details]
Patch for landing

Clearing flags on attachment: 95078

Committed r87473: <http://trac.webkit.org/changeset/87473>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

Why crossOrigin and not webkitCrossOrigin?  Are we already certain this will become a standard?

[Ian 'Hixie' Hickson]

Once this is shipped, if the standard changes incompatibly I'll use a different name.

[Adam Barth]

(In reply to comment #29)
> Why crossOrigin and not webkitCrossOrigin?  Are we already certain this will become a standard?

It's part of HTML5, and, as Ian says, attributes are easier to rename later.