Closed Bug 682299 Opened 13 years ago Closed 12 years ago

Implement crossOrigin attribute for <video> element

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bjacob, Assigned: jon)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [popcorn.js][sec-assigned:dveditz])

Attachments

(4 files, 9 obsolete files)

11.49 KB, patch
jon
: review+
Details | Diff | Splinter Review
19.37 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.89 KB, patch
sicking
: review-
Details | Diff | Splinter Review
9.02 KB, patch
jon
: review+
Details | Diff | Splinter Review
In order to allow for cross-domain <video> elements to be usable as WebGL textures, what's needed is support for the crossOrigin attribute on <video> elements.

See bug 664299 for the equivalent for <img> elements.

This will offer a path for some of Mozilla's own video/WebGL demos to resume working, see bug 676913.
I vote for this. CORS attribute for media should be implemented. Spec for attribute is descripted here http://www.whatwg.org/specs/web-apps/current-work/#attr-media-crossorigin .
Assignee: nobody → jon
Status: NEW → ASSIGNED
I'd like to take this bug as it's blocking some cool stuff that we want to do in Popcorn Maker
Whiteboard: [popcorn.js]
Attached patch Add CORS support to <video> tag (obsolete) — Splinter Review
Here's a patch that adds CORS support to the <video> tag. I am currently hosting some basic tests on my site at http://jbuckley.ca:8000/ . They drawImage the video into a canvas, then attempt to getImageData from the same canvas. The cors-enabled tests have CORS enabled on the server side, the cors-disabled tests do not.

One improvement that I need to make is adding an error message for when CORS is requested on a server that does not support CORS; If you run test-cors-disabled-anon.html or test-cors-disabled-credentials.html with this patch applied, you get a "Video format for MIME type is not supported" message.
Attachment #582492 - Flags: feedback?(chris)
Comment on attachment 582492 [details] [diff] [review]
Add CORS support to <video> tag

Review of attachment 582492 [details] [diff] [review]:
-----------------------------------------------------------------

bz (or maybe roc) really needs to be the one giving you feedback/review on this.

Does the mochitest content/media/test/test_access_control.html still pass with this patch? That test at least needs to be changed to not set the "enforce_same_site_origin" pref, since that's not used anymore.

It would be great if you could use nsContentUtils::ReportToConsole() to add logging upon load failure due to CORS to help web developers understand why their resources aren't loading.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1518,5 @@
>      if (ParseImageAttribute(aAttribute, aValue, aResult)) {
>        return true;
>      }
> +    if (aAttribute == nsGkAtoms::crossorigin) {
> +      return aResult.ParseEnumValue(aValue, kCrossOriginTable, false);

Does this handle the "invalid value default" of "anonymous"? Or maybe that needs to be in GetCORSMode()?

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +56,5 @@
>  #undef GetCurrentTime
>  #endif
>  %}
>  
> +[scriptable, uuid(6733a409-fab3-45e1-af23-9af8c361bdfd)]

You need to rev the uuids on all classes that extend this one too, so nsIDOMHTMLVideoElement and nsIDOMHTMLAudioElement.
Attachment #582492 - Flags: feedback?(chris) → feedback?(bzbarsky)
Comment on attachment 582492 [details] [diff] [review]
Add CORS support to <video> tag

We should probably move that attribute enum table up to nsGenericHTMLElement and rename it to kCORSAttributeTable or something.  And perhaps move the CORS_* values up to nsGenericHTMLElement as well...

Furthermore, we should reconcile the table used here and the one used in nsHTMLImageElement.  I believe the one used there is correct and the ones used here are wrong.

The vestiges of the "media.enforce_same_site_origin" should go away.

The code in SurfaceFromElement is wrong.  It's looking at the CORS mode of the element at draw time, but what's important is the CORS mode at load time.  In other words, like the image case, the CORS mode looked at in SurfaceFromElement needs to be associated with the video data, not the element.  Consider the scenario where a cross-origin video is loaded without a "crossorigin" attribute, then the page sets the attribute and calls drawImage immediately.  The patch attached to this bug wouldn't taint the canvas in that case.  There should probably be a test for this case in the patch, by the way, as well as tests for the case when we really shouldn't taint the canvas.

LookupMediaElementURITable needs to be changed to look at the CORS mode; there are actually comments in there pointing that out.

I have no idea whether the media cache needs to be taught about CORS mode somehow... roc is probably the right man for that.

Apart from the issues above, this seems to be on the right track.  I have no idea whether this makes for feedback+ or feedback-.  ;)
Attachment #582492 - Flags: feedback?(bzbarsky) → feedback+
Attachment #582492 - Attachment is obsolete: true
Attachment #586881 - Flags: review?(bzbarsky)
Attachment #586881 - Attachment is obsolete: true
Attachment #586881 - Flags: review?(bzbarsky)
Attachment #586883 - Flags: review?(bzbarsky)
Oops, this patch wasn't meant to be made obsolete..., this is the first one that needs to be applied
Attachment #586885 - Flags: review?(bzbarsky)
Attachment #586885 - Attachment description: Move all CORS-related attributes and enumerations to nsGenericHTMLElement → 1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement
Attachment #586883 - Attachment description: Implement CORS support for the <video> tag → 2 Implement CORS support for the <video> tag
Attachment #586881 - Attachment description: Move all CORS-related attributes and enumerations to nsGenericHTMLElement → 1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement
Attachment #586881 - Attachment is obsolete: false
Attachment #586881 - Flags: review?(bzbarsky)
Attachment #586885 - Attachment is obsolete: true
Attachment #586885 - Flags: review?(bzbarsky)
roc reviewed bug 712134, which is the reason for the change of reviewer here
Attachment #586886 - Flags: review?(roc)
Attached patch 4 Tests (obsolete) — Splinter Review
Attachment #586887 - Flags: review?(bzbarsky)
Attached patch 4 Tests (obsolete) — Splinter Review
Now includes the video files
Attachment #586887 - Attachment is obsolete: true
Attachment #586887 - Flags: review?(bzbarsky)
Attachment #586888 - Flags: review?(bzbarsky)
Comment on attachment 586886 [details] [diff] [review]
3 Report a console error message when a crossorigin video fails to load

Review of attachment 586886 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +328,5 @@
> +    nsAutoString src;
> +    element->GetCurrentSrc(src);
> +    const PRUnichar *params[] = { src.get() };
> +    element->ReportLoadError("MediaLoadCORSError", params, ArrayLength(params));
> +    return NS_BINDING_ABORTED;

How do you know for sure this was a CORS-related error? Seems like you need some additional checking here.
Attachment #586886 - Flags: review?(roc) → review+
Comment on attachment 586886 [details] [diff] [review]
3 Report a console error message when a crossorigin video fails to load

Review of attachment 586886 [details] [diff] [review]:
-----------------------------------------------------------------

oops, I meant to r- that
Attachment #586886 - Flags: review+ → review-
Comment on attachment 586881 [details] [diff] [review]
1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement

s/so the/so they/

The "order matters here" comment should remain.

The imagelib changes here don't look necessary.  Please undo those.

r=me with those fixed.
Attachment #586881 - Flags: review?(bzbarsky) → review+
Comment on attachment 586883 [details] [diff] [review]
2 Implement CORS support for the <video> tag

Once LoadResource is entered, does that guarantee that whatever was loaded before is no longer available?

Is LookupMediaElementURITable guaranteed to be called after LoadResource on both elements involved?

It'd be good to have someone more familiar than I with the media code review this...
Comment on attachment 586888 [details] [diff] [review]
4 Tests

Likewise, this would be better reviewed by someone who knows the media code better and knows what edge cases need testing.
Added an explicit check for NS_ERROR_DOM_BAD_URI, which is the error returned by the nsCORSListenerProxy when the CORS check fails.

roc, would you be able to review my #2 and #4 patches in this bug, and if not, who would you recommend assigning to?
Attachment #586886 - Attachment is obsolete: true
Attachment #588513 - Flags: review?(roc)
Attachment #586883 - Flags: review?(bzbarsky) → review?(roc)
Attachment #586888 - Flags: review?(bzbarsky) → review?(roc)
Unfortunately I don't think NS_ERROR_DOM_BAD_URI is only caused by CORS. I think we actually need a new "CORS failed" error code.
(or some other way to check that CORS failed)
Comment on attachment 586883 [details] [diff] [review]
2 Implement CORS support for the <video> tag

Review of attachment 586883 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks great. Just need answers to those questions.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +72,5 @@
>      CANPLAY_MAYBE,
>      CANPLAY_YES
>    };
>  
> +  nsGenericHTMLElement::CORSMode GetCORSMode();

nsGenericHTMLElement prefix should not be needed here, or anywhere else you've used it.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +944,5 @@
> +  // By default, it's nsGenericHTMLElement::CORS_NONE
> +  const nsAttrValue* value = GetParsedAttr(nsGkAtoms::crossorigin);
> +  if (value) {
> +    NS_ASSERTION(value->Type() == nsAttrValue::eEnum,
> +                 "Why is this not an enum value?");

Is this safe? What happens if you set crossOrigin to some random string? We shouldn't assert due to an author error like that, it's not a browser bug.

@@ +945,5 @@
> +  const nsAttrValue* value = GetParsedAttr(nsGkAtoms::crossorigin);
> +  if (value) {
> +    NS_ASSERTION(value->Type() == nsAttrValue::eEnum,
> +                 "Why is this not an enum value?");
> +    mCORSMode = nsGenericHTMLElement::CORSMode(value->GetEnumValue());

Shouldn't we set mCORSMode to something when the attribute is not present?

@@ +1534,5 @@
>  
> +NS_IMPL_STRING_ATTR(nsHTMLMediaElement, Crossorigin, crossorigin)
> +
> +nsGenericHTMLElement::CORSMode nsHTMLMediaElement::GetCORSMode() {
> +  return mCORSMode;

just inline this in the header file.
Comment on attachment 586888 [details] [diff] [review]
4 Tests

Review of attachment 586888 [details] [diff] [review]:
-----------------------------------------------------------------

We try to write our tests to be backend independent, so they work for people who turn off particular backends. So we don't like to hardcode WebM dependencies for example. Instead, load manifest.js and search for (e.g.) the first element of gSmallTests that is supported via canPlayType.

You still need to send the right headers, so what I'd do is instead of adding new resource files, write a .sjs module based on one of the existing ones (referer.sjs is pretty simple) that takes a resource name, MIME type, and CORS response headers as HTTP query parameters and returns the resource data with the specified CORS headers. Then you can easily load any resource and have the load get the CORS response headers you want.
> Is this safe? What happens if you set crossOrigin to some random string?

Then it'll end up with the value passed as aDefaultValue to ParseEnumValue, if one is passed.  If one is not passed, it would end up as a non-enum-type attrvalue.  In this case we pass a default value, so this code is fine.

> Shouldn't we set mCORSMode to something when the attribute is not present?

Constructor sets it to CORS_NONE, I think.
(In reply to Boris Zbarsky (:bz) from comment #25)
> Constructor sets it to CORS_NONE, I think.

I think that means there's a bug if you have <video crossorigin="...">, do a load, then remove the crossorigin attribute and do another load --- the previous CORS mode will be used.
Er, yes.  LoadResource should set mCORSMode to CORS_NONE if !value.  Good catch; we should at this to the tests.

I'm still not sure why we can't just store the CORS mode on the actual load as opposed to on the element, though.  Or do we not have an object representing the load around?
I guess storing it in MediaLoadListener might be a little cleaner.
But the media element doesn't hold a reference to MediaLoadListener directly so that'd be a bit annoying.
So I say just fix the bug but leave mCORSMode there.
I think the reason that the CORS proxy returns NS_ERROR_DOM_BAD_URI is that the spec requires it. From http://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#simple-cross-origin-request-0 :

> Perform a resource sharing check. If it returns fail, apply the network error 
> steps. Otherwise, if it returns pass, terminate this algorithm and set the
> cross-origin request status to success. Do not actually terminate the request.

In Section 8.3 (http://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#cors-api-specification-response) the spec says that the network error condition should, "Handle analogous to requests where some kind of error occured. Ensure not the reveal any further information about the request."

This sounds like we shouldn't be adding an additional error code specifically for CORS errors; we might accidentally leak information about the request.

We also do a similar type of CORS error check in http://mxr.mozilla.org/mozilla-central/source/content/media/nsMediaStream.cpp#167
(In reply to Jon Buckley [:jbuck] from comment #31)
> This sounds like we shouldn't be adding an additional error code
> specifically for CORS errors; we might accidentally leak information about
> the request.

OK, but I don't think Web apps can get access to the specific nsresult here.

Currently with your patch I think we could fail to load for some reason other than a CORS error, but report to Web developers that it was because of a CORS error. That would be bad.
Alright, I'll implement a different error code for CORS errors.

One thing I've noticed with this patch is that the entire video file needs to load before the actual CORS check can happen, because nsCrossSiteListenerProxy::OnStartRequest() only fires when the entire file has been loaded. If I'm looking at the network request in Firebug, I can see that the first request has the CORS headers. Is there a way to check the CORS headers on the first request, rather than waiting? It's really only noticeable with video files because they're usually much larger than images or fonts.
Why do we need a different error code for CORS errors? The idea is that CORS errors should be indistinguishable from network errors so that you can't use CORS requests to scan for servers behind firewalls.
(In reply to Jonas Sicking (:sicking) On leave waiting for visa from comment #34)
> Why do we need a different error code for CORS errors?

So that we can give Web developers a different message for CORS-related errors.

> The idea is that CORS
> errors should be indistinguishable from network errors so that you can't use
> CORS requests to scan for servers behind firewalls.

Indeed, the different error code must not be exposed to Web apps.

(In reply to Jon Buckley [:jbuck] from comment #33)
> One thing I've noticed with this patch is that the entire video file needs
> to load before the actual CORS check can happen, because
> nsCrossSiteListenerProxy::OnStartRequest() only fires when the entire file
> has been loaded.

... It does? That doesn't sound right.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> > One thing I've noticed with this patch is that the entire video file needs
> > to load before the actual CORS check can happen, because
> > nsCrossSiteListenerProxy::OnStartRequest() only fires when the entire file
> > has been loaded.
> 
> ... It does? That doesn't sound right.

My apologies, it only does that if the server doesn't support byte range requests, like the python SimpleHTTPServer. If you use a web server that supports byte range requests, it works as expected and fails instantly.
That still seems wrong. The channel is supposed to fire nsCrossSiteListenerProxy::OnStartRequest as we start receiving content (i.e. all headers have been received).
With the python SimpleHTTPServer, video file loading acts strangely. They need to wait for the first bit of the file to load (782 KB according to Firebug) and on the console it shows that the browser has cancelled the connection (error: [Errno 32] Broken pipe). This occurs whether or not crossorigin is set. Now that might actually be a bug, I'm not sure. But my initial thought "CrossSiteListenerProxy is slowing things down" was wrong; it's just the python SimpleHTTPServer acting strangely.
Fixs the mCORSMode bug
Attachment #586883 - Attachment is obsolete: true
Attachment #586883 - Flags: review?(roc)
Attachment #590878 - Flags: review?(roc)
Fixes an inconsistent behaviour in nsCrossSiteListenerProxy, where the wrong error would be reported if crossorigin='use-credentials' and the HTTP request failed (ie 404). A request that set crossorigin='anonymous' and 404'd would correctly print a 404 message to the web console.

I'm not sure if this is the correct behaviour; However, it does mean that subscribers of the nsCrossSiteListenerProxy can determine from the combination of status and http response status whether an error was a CORS error, or an HTTP error without implementing a new CORS error code.
Attachment #588513 - Attachment is obsolete: true
Attachment #588513 - Flags: review?(roc)
Attachment #590882 - Flags: review?(roc)
Attached patch 4v2 CORS <video> tests WIP (obsolete) — Splinter Review
Rewrote the tests to use sjs and existing video files. Still need to add tests for fiddling with CORS modes.
Attachment #586888 - Attachment is obsolete: true
Attachment #586888 - Flags: review?(roc)
Attachment #590883 - Flags: review?(roc)
(In reply to Jon Buckley [:jbuck] from comment #38)
> With the python SimpleHTTPServer, video file loading acts strangely. They
> need to wait for the first bit of the file to load (782 KB according to
> Firebug) and on the console it shows that the browser has cancelled the
> connection (error: [Errno 32] Broken pipe). This occurs whether or not
> crossorigin is set. Now that might actually be a bug, I'm not sure. But my
> initial thought "CrossSiteListenerProxy is slowing things down" was wrong;
> it's just the python SimpleHTTPServer acting strangely.

Are you testing without preload or autoplay? If you just write <video src="...">, we'll load enough to get the first frame and then stop loading.
Comment on attachment 590882 [details] [diff] [review]
3v3 Report a console error message when a crossorigin video fails to load

Review of attachment 590882 [details] [diff] [review]:
-----------------------------------------------------------------

Requesting additional review from Jonas; I'm not sure whether the change to nsCrossSiteListenerProxy is secure. Maybe a caller that's not careful might now leak information about whether a cross-origin request succeeded or not?

Maybe it would actually be better to return the same error code here but provide a separate method on nsCrossSiteListenerProxy to check whether the request failed due to CORS issues or HTTP issues?
Attachment #590882 - Flags: review?(jonas)
You can land patches 1, 2 and 4 now I guess, if that helps.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Are you testing without preload or autoplay? If you just write <video
> src="...">, we'll load enough to get the first frame and then stop loading.

Yeah, that works as expected. Sorry for the confusion I caused!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Requesting additional review from Jonas; I'm not sure whether the change to
> nsCrossSiteListenerProxy is secure. Maybe a caller that's not careful might
> now leak information about whether a cross-origin request succeeded or not?
> 
> Maybe it would actually be better to return the same error code here but
> provide a separate method on nsCrossSiteListenerProxy to check whether the
> request failed due to CORS issues or HTTP issues?

Definitely possible that we might leak information, right now it's just inconsistent. HTTP response errors should cause the CORS proxy to always fail or always succeed.

And with some testing on the try servers, patch #3 makes several cross origin XHR tests fail. I'm leaning towards making it fail on HTTP response errors now...

--

Landing the meat of the patches now will definitely help. I'm going to run them through the try server first, just to make sure everything works properly without patch #3.
Additional canvas tainting checks
Attachment #590883 - Attachment is obsolete: true
Attachment #590978 - Flags: review+
Keywords: checkin-needed
Whiteboard: [popcorn.js] → [popcorn.js], only checkin patches 1, 2, and 4
Attachment #588489 - Flags: checkin?
Attachment #590878 - Flags: checkin?
Attachment #590978 - Flags: checkin?
Keywords: dev-doc-needed
Attachment #588489 - Flags: checkin?
Attachment #590878 - Flags: checkin?
Attachment #590978 - Flags: checkin?
Blocks: 703566
This has already landed but marking sec-review-needed after discussing with Curtis since it seems like a good idea for someone from the security assurance team to review what was implemented here :)
Comment on attachment 590882 [details] [diff] [review]
3v3 Report a console error message when a crossorigin video fails to load

Review of attachment 590882 [details] [diff] [review]:
-----------------------------------------------------------------

The change to the CORS code seems wrong. Please renominate if I'm interpreting things wrongly.

::: content/base/src/nsCrossSiteListenerProxy.cpp
@@ +552,5 @@
>    // Test that things worked on a HTTP level
>    nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aRequest);
>    NS_ENSURE_TRUE(http, NS_ERROR_DOM_BAD_URI);
> +  bool succeeded;
> +  if (http && NS_SUCCEEDED(http->GetRequestSucceeded(&succeeded)) && !succeeded) {

I don't understand this change. It'll make us completely skip any security checks for non-200 responses. I.e. any 404 page will be automatically granted access to, no matter what the CORS headers say.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +330,5 @@
> +      element->GetCurrentSrc(src);
> +      const PRUnichar *params[] = { src.get() };
> +      element->ReportLoadError("MediaLoadCORSError", params, ArrayLength(params));
> +    }
> +    return NS_BINDING_ABORTED;

Is it intentional that you now no longer propagate the error code which came from the request, even for non-CORS related errors?
Attachment #590882 - Flags: review?(jonas) → review-
Whiteboard: [popcorn.js] → [popcorn.js][secr:dveditz]
Whiteboard: [popcorn.js][secr:dveditz] → [popcorn.js][sec-assigned:dveditz]
Any update on this?
Hm. Well, I thoroughly dropped the ball on completing the last bit of this ticket.

The core feature itself, allowing people to use the crossorigin attribute on <video> elements, does work in Firefox 12. I think the best thing to do now would be mark this ticket as resolved fixed, and open a new dependent bug on "Displaying an error message in the console when a CORS failure occurs".

Does that sound right?
That sounds like an excellent idea.
Blocks: 760338
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: sec-review?(dveditz)
Depends on: 837153
Attachment #590882 - Flags: review?(roc)
Depends on: 875194
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.