A new attribute was recently added to the HTML spec: crossorigin: 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 Supporting it is needed for bug 662599. Can you please explain me in detail (assume you're talking to a chimp) the steps that I need to follow to implement that? Or let me know if you plan to do that yourself, but that is a high priority for WebGL.

Note: WebKit patch here, including testcases: https://bugs.webkit.org/show_bug.cgi?id=61015 https://bugs.webkit.org/attachment.cgi?id=95078

You probably don't have to do anything to the parser. You need to add "crossorigin" to nsIDOMHTMLImageElement.idl. You need to add NS_IMPL_STRING_ATTR(nsHTMLImageElement, CrossOrigin, crossorigin) to nsHTMLImageElement.cpp. You'll want to add a method to nsIImageLoadingContent to get the "CORS mode" for the load. The default would be "No CORS", nsHTMLImageElement would override it to return something based on the current attribute value. When CORS mode is not "No CORS", nsImageLoadingContent::LoadImage needs to set up CORS when loading the image. See nsMediaChannelStream::OpenChannel for an example of this. You probably want to pass the CORS mode and the requesting principal to nsContentUtils::LoadImage and then into imgILoader::LoadImage. The hardest part that I can give you the least advice about is integrating with the image cache. For image requests made with CORS, the incoming principal needs to become part of the cache key so that results of CORS loads don't get mixed up with results of non-CORS loads, and results of CORS loads with different principals don't get mixed up with each other.

(In reply to comment #2) > You need to add "crossorigin" to nsIDOMHTMLImageElement.idl. crossOrigin > You need to add NS_IMPL_STRING_ATTR(nsHTMLImageElement, CrossOrigin, > crossorigin) to nsHTMLImageElement.cpp. NS_IMPL_ENUM_ATTR_DEFAULT_VALUE

Thanks a lot for the explanation, I'm slowly working my way through that. One thing I don't understand is that at some point we're supposed to parse some HTTP header to see if we're allowed to use the image where cross-domain images would normally not be allowed. Where is that part supposed to happen? I don't see where in comment 2's outline it fits.

nsCORSListenerProxy does all that for you. If it detects an improper attempt to use a cross-site resource it will cancel your channel with NS_ERROR_DOM_BAD_URI, and you would detect that in your OnStartRequest callback. See nsMediaChannelStream::OnStartRequest for an example.

(In reply to comment #2) > You probably don't have to do anything to the parser. Confirmed. No need to touch the parser. (A tiny, tiny atom interning optimization could be done, but I can do it sometime later. No need to worry about it here.)

OK, I only got back to this today as I was taken by security bugs. Here's where I'm currently stuck: (In reply to comment #2) > When CORS mode is not "No CORS", nsImageLoadingContent::LoadImage needs to > set up CORS when loading the image. See nsMediaChannelStream::OpenChannel > for an example of this. You probably want to pass the CORS mode and the > requesting principal to nsContentUtils::LoadImage and then into > imgILoader::LoadImage. The code in nsMediaChannelStream::OpenChannel is doing: http://mxr.mozilla.org/mozilla-central/source/content/media/nsMediaStream.cpp#449 460 nsCORSListenerProxy* crossSiteListener = 461 new nsCORSListenerProxy(mListener, 462 element->NodePrincipal(), 463 mChannel, 464 PR_FALSE, 465 &rv); 466 listener = crossSiteListener; so it's all about 'listeners'. But in nsImageLoadingContent::LoadImage, there's no 'listener'. However, there's one in LoadImageWithChannel. Questions: 1) What's a 'listener'? 2) Do I only need to edit LoadImageWithChannel or do I really need to edit LoadImage?

The listener is the thing that receives callbacks from Necko when something happens to the channel. You can probably get by with changing LoadImageWithChannel.

LoadImageWithChannel is only used when an image is loaded directly in an iframe or toplevel window. You can probably not worry about CORS stuff at all on that codepath for now. Changing it won't affect <img src> loads. The code where you have a channel to work with is inside imagelib, not in nsImageLoadingContent. See the imgILoader::LoadImage call that nsImageLoadingContent makes....

Guys, thanks for the help, but I'm still stuck. Good to know that I only need to edit LoadImage and not LoadImageWithChannel. But as far as I can see, the way to check if there is CORS approval is to use nsCORSListenerProxy, and this wants a Listener to work with. My problem is that there is no Listener around in nsImageLoadingContent::LoadImage, as far as I can see.

See comment 9 paragraph 2.

Getting Joe to do Part 2 - actually do something useful in imagelib; and there will be a part 3 - webgl side changes

This adds a load flag for the two CORS styles, and adds our CORS listener between our listener and the channel when necessary. It also makes sure that when we try to load from the image cache, we only allow an existing imgRequest to be used when both the CORS style is the same and the document principal is the same. (Otherwise, we reload from the network.)

This is an updated version of Benoit's patch that a) builds and b) sets the CORS load flags on LoadImage. I'm leaving it up to Benoit to request review.

Note: These patches are tested only to run. I have not tested whether the CORS stuff works at all. Angry Birds for the web is still broken, even in Chrome 14.

Comment on attachment 543815 [details] [diff] [review] part 0: support CORS in imagelib >+// Returns true if this request is compatible with the given CORS mode on the >+// given initial document URI, and false if the request may not be reused due >+// to CORS. >+static bool >+ValidateCORS(imgRequest* request, CorsMode corsmode, nsIURI* initialDocumentURI) >+{ >+ // If the entry's CORS mode doesn't match, or the CORS mode matches but the >+ // document principal isn't the same, we can't use this request. >+ if (request->GetCorsMode() != corsmode) { >+ return false; >+ } else if (request->GetCorsMode() != CORS_NONE) { >+ nsCOMPtr<nsIScriptSecurityManager> secman = nsContentUtils::GetSecurityManager(); >+ nsCOMPtr<nsIPrincipal> docprincipal; >+ secman->GetCodebasePrincipal(initialDocumentURI, getter_AddRefs(docprincipal)); Seems like you could just do: nsContentUtils::GetSecurityManager()->GetCodebasePrincipal(initialDocumentURI, getter_AddRefs(docprincipal)); and do the same elsewhere. >+ nsCOMPtr<nsIScriptSecurityManager> secman = nsContentUtils::GetSecurityManager(); >+ nsCOMPtr<nsIPrincipal> docprincipal; >+ secman->GetCodebasePrincipal(aInitialDocumentURI, getter_AddRefs(docprincipal)); >+ Same here. > >- NS_ADDREF(pl); >+ nsCOMPtr<nsIStreamListener> listener = pl; >+ >+ if (corsmode != CORS_NONE) { >+ if (NS_FAILED(rv)) { >+ return NS_ERROR_FAILURE; >+ } Checking if we've failed here seems a little paranoid :)

Comment on attachment 543817 [details] [diff] [review] part 1: add the crossorigin attribute, and set the flag on LoadImage >+ /** >+ * Used to get the CORS mode for the load. >+ * >+ * @return The CORS mode. >+ */ >+ long getCORSMode(); I'd prefer this to be [noscript, notxpcom] That should also let the implementation return the long instead of having to return the NS_RESULT.

Updated for review comments.

Why are we calling getCodebasePrincipal at all? Do we not know what document (or at least what principal) that load is associated with at that point? If so, can we just fix that? All the callers have to know that info, and imagelib could do fix the longstanding data: image in canvas issue too if it just had the principal and stuck it on the channel as needed before opening it.... (which is a separate bug, but we still want the principal on this side of the imagelib API for that). I'll look at the patch in detail tomorrow.

Good point. Let's pass the loading principal to loadImage.

Building on part 0, now we can just use the nsIPrincipal passed in to loadImage to properly support CORS.

What caching policy does this implement? I.e. do we ever cache an image fetched from one of the three CORS modes such that it can be used in another CORS mode? From a security perspective it might be safe to cache an image fetched using CORS_ANONYMOUS and CORS_USE_CREDENTIALS such that it's used for CORS_NONE requests, but I'm not sure that that's desirable from a behavioral point of view.

As currently implemented we only reuse cached elements with identical principals and CORS modes; this can be changed if we want. From a behavioural point of view I can't imagine using valid cached elements being a bad idea.

Comment on attachment 544026 [details] [diff] [review] part 0: pass the nsIPrincipal to loadImage This seems ok, but please document in the various APIs involved that a null pointer is allowed and what it means. And is there a reason GetLoadingPrincipal() can't just return the nsIPrincipal* without addrefing? r=me with those nits.

Comment on attachment 544027 [details] [diff] [review] part 0.5: support CORS in imagelib v3 So this is relying on the fact that loadImage only takes the nsIRequest flags, so bits 16-31 are safe to reuse. That needs to be documented somewhere. The enum values there (both in the IDL and in the C++) need documenting too. r=me with that; I didn't check the details of how you're using CORS here.

Ok, as long as we separate by loading principal and CORS type we should be fine. There are lots of ways to screw up otherwise, with results ranging from DoS to leaking private data, so it's something I think we should avoid.

This new version makes the crossOrigin attribute actually work: in particular, since it's an enum attribute, we had to implement nsHTMLImageElement::ParseAttribute

unfortunately I still get failures: TEST-UNEXPECTED-FAIL | | texImage2D on http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-star.png with crossOrigin=anonymous: expected no error, got security error TEST-UNEXPECTED-FAIL | | texImage2D on http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-credentials.png with crossOrigin=use-credentials: expected no error, got security error can someone more competent than me please have a look?

With the attached patches, the CORS mode is passed into imagelib, imagelib enforces the CORS restrictions, but the canvas same-origin checks are still there, no? Those need to be changed to take into account the CORS info. In particular, the canvas same-origin checks should only be performed if the image was loaded CORS_NONE, I believe. Note that you have to get this information from the imgIRequest, not the element, because the CORS mode at load start may not match the current CORS mode. (Do we need spec changes here to restart the load if @crossorigin changes, by the way?) On a separate note, nsHTMLImageElement::GetCORSMode should use GetParsedAttr instead of what it does now; if the parsed value is an enum just return it, and otherwise return CORS_NONE.

And can you please add a test for crossOrigin reflection, using content/html/content/test/reflect.js?

In order to correctly allow cross-origin textures, we have to expose the CORS mode on imgIRequest. This patch does that by moving the CORS mode types into the idl file.

This is an interdiff between v3 and v4 of part 0.5 for the easier reviewing.

It seems we had a misunderstanding about what "allow credentials" means in CORS. It doesn't mean that anonymous CORS isn't allowed, but rather that we can send along cookies and such. Thus, I'm adjusting Benoit's test to allow anonymous loads of "allow credentials" CORS-using resources, and similarly to allow "allow credentials" loads of anonymous CORS-using resources.

Comment on attachment 544902 [details] [diff] [review] part 2: mochitest v2 Fantastic!

This implements the changes asked for by Boris. I've left the ParseAttribute body alone, though, presuming it's important. I don't actually know who should review this; whoever does should probably also review the mochitest, though!

This relaxes the same-origin requirement that we had in WebGL to also allow CORS-verified cross-origin images as textures. (We still disallow tainted canvases, though.)

Comment on attachment 544917 [details] [diff] [review] part 1: crossorigin attribute v2 > I've left the ParseAttribute body alone, though, presuming it's important. It is, yeah. ;) I'd really like to see some documentation for what these modes mean _somewhere_ here. I don't know the CORS stuff well enough to suggest wording myself; maybe ping sicking? The getter also needs documentation explaining that this is the CORS mode a load would use if you started one right now.... which is an odd thing to be exposing. Why do we need this on the interface in the first place? Assuming we do need this on the interface, you need to change the iid of the interface. And is there a reason this is not a |readonly attribute long CORSMode| ?

So if I read the CORS listener code correctly, the CORS_USE_CREDENTIALS case should fail the load if the server does not respond with "Access-Control-Allow-Credentials: true". > and similarly to allow "allow credentials" loads of anonymous CORS-using > resources. Except those aren't allowed, per above. What you did was change the test so it has two instances of this: + testTexture("http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-credentials.png", + "use-credentials", + OK); One of those should go and probably be replaced with an instance of this: + testTexture("http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-star.png", + "use-credentials", + SECURITY_ERR); It's also worth testing that using a bogus value of the crossorigin attribute is treated just like having it not set at all.

Comment on attachment 544919 [details] [diff] [review] part 1.5: relax the same-origin requirement in WebGL I'd rather we explicitly set the mode to NONE if the getter throws instead of relying on it not touching the mode. r=me with that.

(In reply to comment #37) > I'd really like to see some documentation for what these modes mean > _somewhere_ here. I don't know the CORS stuff well enough to suggest > wording myself; maybe ping sicking? I was able to put something together from my readings of the HTML5 spec. > The getter also needs documentation > explaining that this is the CORS mode a load would use if you started one > right now.... which is an odd thing to be exposing. Why do we need this on > the interface in the first place? As far as I can tell, it's required because nsHTMLImageElement uses nsImageLoadingContent to actually load images, and in order to have CORS passed to imgLoader, we need that information at the nsImageLoadingContent layer. > Assuming we do need this on the interface, you need to change the iid of the > interface. And is there a reason this is not a |readonly attribute long > CORSMode| ? I can't explain it - I just inherited this code from Benoit. I'll change it. :) Also, I'm going to change the uuid of nsIHTMLImageElement.

> and in order to have CORS passed to imgLoader, we need that information at the > nsImageLoadingContent layer. We need it on nsImageLoadingContent, sure. So make it a virtual method on that class. But it doesn't need to be on nsIImageLoadingContent at all, I think.

Very good! This implements those requested changes.

(In reply to comment #38) > So if I read the CORS listener code correctly, the CORS_USE_CREDENTIALS case > should fail the load if the server does not respond with > "Access-Control-Allow-Credentials: true". That matches my reading of the CORS spec too. > What you did was change the test so it has two instances of this: Yeah, I was probably sleep-deprived. > It's also worth testing that using a bogus value of the crossorigin > attribute is treated just like having it not set at all. Will test.

Turns out that we get an error (as in, the image gets onerror and not onload) while loading images if we have Access-Control-Allow-Origin: * and ask for credentials with CORS, so we have to handle that too.

Comment on attachment 545474 [details] [diff] [review] part 1: crossorigin attribute v3 The nsAlertsIconListener.cpp change needs to be in the same patch as the interface change; otherwise we'll be pushing non-compiling changesets. r=me with that fixed.

Comment on attachment 545501 [details] [diff] [review] part 2: mochitest v3 r=me

http://hg.mozilla.org/integration/mozilla-inbound/rev/5977286eda3a http://hg.mozilla.org/integration/mozilla-inbound/rev/24b7b80bb3c3 http://hg.mozilla.org/integration/mozilla-inbound/rev/1dd4abff0da3 http://hg.mozilla.org/integration/mozilla-inbound/rev/1d535d70b54d http://hg.mozilla.org/integration/mozilla-inbound/rev/255eaf3959f5

The dev doc we need here is that the HTML5 crossorigin attribute on images is now supported; links regarding it are in comment 1.

(In reply to comment #30) > And can you please add a test for crossOrigin reflection, using > content/html/content/test/reflect.js?

http://hg.mozilla.org/mozilla-central/rev/5977286eda3a http://hg.mozilla.org/mozilla-central/rev/24b7b80bb3c3 http://hg.mozilla.org/mozilla-central/rev/1dd4abff0da3 http://hg.mozilla.org/mozilla-central/rev/1d535d70b54d http://hg.mozilla.org/mozilla-central/rev/255eaf3959f5

(In reply to comment #49) > (In reply to comment #30) > > And can you please add a test for crossOrigin reflection, using > > content/html/content/test/reflect.js? Sorry, totally forgot.

Comment on attachment 546269 [details] [diff] [review] test that crossOrigin is reflected properly >Bug 664299 - Test that the crossOrigin attribute is probably reflected into JS. r=Ms2ger "is properly reflected" >--- a/content/html/content/test/Makefile.in >+++ b/content/html/content/test/Makefile.in >@@ -271,12 +271,13 @@ _TEST_FILES = \ > test_bug586786.html \ > test_bug649134.html \ > test_bug658746.html \ > test_bug659596.html \ > test_bug659743.xml \ > test_bug660663.html \ > test_bug666666.html \ > test_restore_from_parser_fragment.html \ >+ test_bug664299.html \ This one should go between 660663 and 666666. r=me with that

(In reply to comment #52) > >--- a/content/html/content/test/Makefile.in > >+++ b/content/html/content/test/Makefile.in > >@@ -271,12 +271,13 @@ _TEST_FILES = \ > > test_bug586786.html \ > > test_bug649134.html \ > > test_bug658746.html \ > > test_bug659596.html \ > > test_bug659743.xml \ > > test_bug660663.html \ > > test_bug666666.html \ > > test_restore_from_parser_fragment.html \ > >+ test_bug664299.html \ > > This one should go between 660663 and 666666. Note that the list is not in sorted order in general, though the quoted section is.

I know; I'd still like to keep that section sorted.

The reflection test is inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/86dd8454de0b

And now the reflection test's been merged to m-c: http://hg.mozilla.org/mozilla-central/rev/86dd8454de0b

Any luck to have this in Firefox 6?

Feature freeze for Firefox 6 was May 24. So without a time machine, no. Also probably not going to be in 7 (feature freeze was July 5).

Thanks a bunch for driving this. I would like very much this to go into Firefox 7, but at the same time I understand that it's branched already. A possible reason to make an exception for this is that it helps our relation with web developers to offer as soon as possible this way to get their sites to resume working in Firefox. I know that major web developers, including at Google, are very interested in us shipping this feature ASAP. Anyway I'm not the one deciding on this :)

Benoit, if you think some set of patches here should be landing on Aurora, please nominate them accordingly.

Nominated.

This needs to go in with the patches in bug 671906 or not at all; that bug unbroke this.

During triage today we decided to let this go through the normal process rather than pushing it into Aurora. The decision hurts and we know it isn't ideal from a web developer standpoint but we think it's the right one.

Some notes on add-on compatibility: - the interfaces that changed are: imgILoader and imgIRequest and nsIDOMHTMLImageElement - nsIDOMHTMLImageElement has an added attribute - this shouldn't hurt script addons - imgIRequest has an added attribute - this shouldn't hurt script addons - this interface is used by addons like firebug, a request viewer, an image saver. I don't know if there are a lot of binary addons that would use it. - imgILoader changes the signature of LoadImage - Two addons on AMO use imgILoader, one use is commented out and the other does not use LoadImage

Have we checked if CoolIris or stuff like HPSmartWebPrinting use these interfaces?

(In reply to comment #65) > Have we checked if CoolIris or stuff like HPSmartWebPrinting use these > interfaces? How do we do that?

fligtar and jorge may still have the Cooliris contacts, and maybe kev has HP contacts

Adding some known Cooliris contacts to the CC list.

removed sec-review flag as we are comfortable with CORS usage here

I will check the Cooliris code. This is will be changing in FF/Gecko 8.0?

(In reply to Jeff from comment #70) > I will check the Cooliris code. This is will be changing in FF/Gecko 8.0? Yes.

We are not currently using imgILoader, imgIRequest or nsIDOMHTMLImageElement. Thanks for the heads up. If you'd like a complete list of interfaces we do use, I can provide that. :)

(In reply to Jeff from comment #72) > We are not currently using imgILoader, imgIRequest or > nsIDOMHTMLImageElement. Thanks for the heads up. If you'd like a complete > list of interfaces we do use, I can provide that. :) That would be great.

Need to schedule this for a full secteam review who should I talk to make this happen

Benoit Jacob or me.

We have the following documentation for this: CORS-enabled image definition: https://developer.mozilla.org/en/CORS_Enabled_Image CORS-enabled settings attribute definition: https://developer.mozilla.org/en/HTML/CORS_settings_attributes The crossOrigin attribute on the HTMLImageElement page: https://developer.mozilla.org/en/HTML/CORS_settings_attributes The crossorigin attribut on the <img> element: https://developer.mozilla.org/En/HTML/Element/Img And an entry on Firefox 8 for developers: https://developer.mozilla.org/en/firefox_8_for_developers I think it is enough to change the keyword to dev-doc-complete, though we should, in the future, revisit all CORS documentation to make it more coherent and understandable (lots of Web Developers have problems understanding CORS) (Most of this doc was written by Sheppy, and Paul Irish; I just did the <img> HTML attribute crossorigin bit)

Benoit: since joedrew is away for a while do you want to just schedule this and get it done or do we need him for the review too?

Joe would have been a better person since, contrary to me, he understand the ImageLib side of things, but given that this is for Firefox 8, I suppose we need to schedule this ASAP. How do we go about scheduling this? I don't know what day works for you. Let me make a random suggestion: how about this Thursday morning? 10 AM east coast time?

Generally you pick a date from https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html, lucky for you this Thu at 10am is still open. I will schedule it.

Benoit: sorry that review slot is 10am Pacific, not eastern does that still work for you? 10am EST would be 7am Pacific and noone will attend that

Oh right, sorry. 10 am Pacific works for me.

SecReview scheduled for 20-Oct-2011 at 10am PST

Notes from review: https://wiki.mozilla.org/Security/Reviews/crossoriginAttribute

The test that verifies the fix for this bug (test_bug664299.html) is passing on all OS: Firefox 8.0 (beta): https://tbpl.mozilla.org/php/getParsedLog.php?id=7158461&full=1&branch=mozilla-beta https://tbpl.mozilla.org/php/getParsedLog.php?id=7160589&full=1&branch=mozilla-beta https://tbpl.mozilla.org/php/getParsedLog.php?id=7158593&full=1&branch=mozilla-beta https://tbpl.mozilla.org/php/getParsedLog.php?id=7158600&full=1&branch=mozilla-beta Firefox 9.0 (aurora): https://tbpl.mozilla.org/php/getParsedLog.php?id=7155263&full=1&branch=mozilla-aurora https://tbpl.mozilla.org/php/getParsedLog.php?id=7155779&full=1&branch=mozilla-aurora https://tbpl.mozilla.org/php/getParsedLog.php?id=7156339&full=1&branch=mozilla-aurora https://tbpl.mozilla.org/php/getParsedLog.php?id=7156332&full=1&branch=mozilla-aurora Firefox 10.0 (central): https://tbpl.mozilla.org/php/getParsedLog.php?id=7148007&full=1&branch=services-central https://tbpl.mozilla.org/php/getParsedLog.php?id=7150553&full=1&branch=services-central https://tbpl.mozilla.org/php/getParsedLog.php?id=7148795&full=1&branch=services-central https://tbpl.mozilla.org/php/getParsedLog.php?id=7148858&full=1&branch=services-central