Bug 88349 – Plumb CORS attribute information from HTMLMediaElement to media players so it can be used

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ami Fischman]

Today media elements ignore the crossorigin attribute because MediaPlayer implementations can't get at it.
This bug is the webkit side of http://crbug.com/123369, and will require https://chromiumcodereview.appspot.com/10543007 to land first.

[Ami Fischman]

Created attachment 145843 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 145843 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:306
> -        m_webMediaPlayer->load(KURL(ParsedURLString, m_url));
> +        m_webMediaPlayer->load(KURL(ParsedURLString, m_url),
> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerHasCrossOriginAttribute(),
> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerGetCrossOriginAttribute());

Why do the CORS bits take a different path than m_url?  Would it make more sense to add them as a parameter to WebMediaPlayerClientImpl::load ?

[Ami Fischman]

Comment on attachment 145843 [details]
Patch

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

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:306
>> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerGetCrossOriginAttribute());
> 
> Why do the CORS bits take a different path than m_url?  Would it make more sense to add them as a parameter to WebMediaPlayerClientImpl::load ?

It wasn't a conscious decision, but making the change you propose would require touching every port's MediaPlayerPrivateInterface impl and would result in a much more invasive change, and it's not clear to me that it'll result in better code.

[Adam Barth]

Comment on attachment 145843 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:431
> +    virtual bool mediaPlayerHasCrossOriginAttribute() const OVERRIDE;
> +    virtual String mediaPlayerGetCrossOriginAttribute() const OVERRIDE;

There's no need to have two functions here.  In WebKit, we can distinguish between a null string and an empty string.  We use null strings to represent the lack of an attribute.  Therefore, you can just have one function that returns a String.  If the String isNull(), then you'll know that the attribute didn't exist.

> Source/WebKit/chromium/public/WebMediaPlayer.h:106
> +    virtual void load(const WebURL&, bool hasCrossOriginAttr, const WebString& crossOriginAttr) = 0;

Would it make sense to pass a http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebURLLoaderOptions.h object here (or a similar options object) rather than passing DOM-level concepts like the value of an attribute?

[Adam Barth]

Comment on attachment 145843 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:306
>>> -        m_webMediaPlayer->load(KURL(ParsedURLString, m_url));
>>> +        m_webMediaPlayer->load(KURL(ParsedURLString, m_url),
>>> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerHasCrossOriginAttribute(),
>>> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerGetCrossOriginAttribute());
>> 
>> Why do the CORS bits take a different path than m_url?  Would it make more sense to add them as a parameter to WebMediaPlayerClientImpl::load ?
> 
> It wasn't a conscious decision, but making the change you propose would require touching every port's MediaPlayerPrivateInterface impl and would result in a much more invasive change, and it's not clear to me that it'll result in better code.

Hum...  I wonder if we should extend the load function to take an options object, similar to how ThreadableLoader works.  Then we could extend this list in the future without too much trouble.

It's also likely that we shouldn't be dealing with DOM-level concepts like attributes at this layer.  We don't want every port to need to parse and understand this attribute.  Instead, some object closer to the element itself should read its properties and set some loader flags appropriately according to the semantics of the attribute.

[Eric Carlson]

(In reply to comment #6)
> 
> Hum...  I wonder if we should extend the load function to take an options object, similar to how ThreadableLoader works.  Then we could extend this list in the future without too much trouble.
> 
> It's also likely that we shouldn't be dealing with DOM-level concepts like attributes at this layer.  We don't want every port to need to parse and understand this attribute.  Instead, some object closer to the element itself should read its properties and set some loader flags appropriately according to the semantics of the attribute.

I think this is a great idea.

[Ami Fischman]

Created attachment 145873 [details]
Patch

[Ami Fischman]

(In reply to comment #6)
> (From update of attachment 145843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145843&action=review
> 
> >>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:306
> >>> -        m_webMediaPlayer->load(KURL(ParsedURLString, m_url));
> >>> +        m_webMediaPlayer->load(KURL(ParsedURLString, m_url),
> >>> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerHasCrossOriginAttribute(),
> >>> +                               m_mediaPlayer->mediaPlayerClient()->mediaPlayerGetCrossOriginAttribute());
> >> 
> >> Why do the CORS bits take a different path than m_url?  Would it make more sense to add them as a parameter to WebMediaPlayerClientImpl::load ?
> > 
> > It wasn't a conscious decision, but making the change you propose would require touching every port's MediaPlayerPrivateInterface impl and would result in a much more invasive change, and it's not clear to me that it'll result in better code.
> 
> Hum...  I wonder if we should extend the load function to take an options object, similar to how ThreadableLoader works.  Then we could extend this list in the future without too much trouble.
> 
> It's also likely that we shouldn't be dealing with DOM-level concepts like attributes at this layer.  We don't want every port to need to parse and understand this attribute.  Instead, some object closer to the element itself should read its properties and set some loader flags appropriately according to the semantics of the attribute.

I replaced the explicit has/get string approach with a 3-value enum (Unspecified/Anonymous/UseCredentials) and updated the chromium-side patch to match.  This is half-way between my first approach and a LoaderOptions approach; it achieves the goal of containing attribute parsing to the HTMLMediaElement, but doesn't require me to touch every port.

[Ami Fischman]

Created attachment 145876 [details]
Patch

[Adam Barth]

Comment on attachment 145876 [details]
Patch

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

Looks fine, with one naming nit.  Once you land the Chromium-side change, can you write a test that shows the crossorigin attribute working correctly?

> Source/WebCore/ChangeLog:8
> +        No new tests; do we have a version of http/tests supporting different origins?

Yes.  Take a look in LayoutTests/http for tests related to access control and/or CORS.  We also have tests for the crossorigin atribute for img.

> Source/WebCore/platform/graphics/MediaPlayer.h:104
> +    enum CrossOriginAttribute { Unspecified, Anonymous, UseCredentials };

CrossOriginAttribute => CORSMode ?

[WebKit Review Bot]

Comment on attachment 145876 [details]
Patch

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

[Ami Fischman]

Created attachment 146074 [details]
Patch

[Ami Fischman]

Added test, renamed enum to match WebKitAPI style, moved COMPILE_ASSERT to the unified file, and updated CanvasRenderingContext to use CORSMode for HTMLVideoElements.

Adam: PTAL?

[Eric Carlson]

Comment on attachment 146074 [details]
Patch

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

> LayoutTests/http/tests/security/video-cross-origin-readback.html:7
> +        if (window.layoutTestController)
> +            layoutTestController.overridePreference("WebKitWebGLEnabled", "1");

Does this test require WebGL? Aren't there ports the implement <video> that don't implement WebGL?

You could also test it  by trying to read pixels from a standard <canvas > into which <video> pixels have been rendered.

[Ami Fischman]

Created attachment 146089 [details]
Patch

[Ami Fischman]

(In reply to comment #15)
> (From update of attachment 146074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146074&action=review
> 
> > LayoutTests/http/tests/security/video-cross-origin-readback.html:7
> > +        if (window.layoutTestController)
> > +            layoutTestController.overridePreference("WebKitWebGLEnabled", "1");
> 
> Does this test require WebGL? Aren't there ports the implement <video> that don't implement WebGL?
> 
> You could also test it  by trying to read pixels from a standard <canvas > into which <video> pixels have been rendered.

Great idea; done.

[Eric Carlson]

Comment on attachment 146089 [details]
Patch

Looks good to me but I don't have one of the magic email addresses so I won't r+ it. 

Thanks Ami!

[Adam Barth]

Comment on attachment 146089 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:356
> +    m_corsModeAtLastLoad = MediaPlayerClient::Unspecified;
> +    if (m_mediaPlayerClient)
> +        m_corsModeAtLastLoad = m_mediaPlayerClient->mediaPlayerCORSMode();

What about the following case:

1) I kick off a non-CORS enabled load.  It succeeds.  My HTMLVideoElement now contains sensitive information.
2) I kick off a CORS enabled load, but then I cancel it immediately after.

It seems like there's a risk that we'll now think that the sensitive data in the HTMLVideoElement was loaded with CORS.  Can this occur?  If this can't occur, how do we know that the code won't change in the future in such a way to make it possible?

It seems like a better design is to tie this state to the response and then having a way to ask the video element whether the thing it is currently displaying came in an response that was a result of a CORS-enabled request.

[Ami Fischman]

(In reply to comment #19)
> (From update of attachment 146089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146089&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:356
> > +    m_corsModeAtLastLoad = MediaPlayerClient::Unspecified;
> > +    if (m_mediaPlayerClient)
> > +        m_corsModeAtLastLoad = m_mediaPlayerClient->mediaPlayerCORSMode();
> 
> What about the following case:
> 
> 1) I kick off a non-CORS enabled load.  It succeeds.  My HTMLVideoElement now contains sensitive information.
> 2) I kick off a CORS enabled load, but then I cancel it immediately after.
> It seems like there's a risk that we'll now think that the sensitive data in the HTMLVideoElement was loaded with CORS.  Can this occur?  

No.  The first thing load() does is overwrite m_corsModeAtLastLoad back to Unspecified, which causes the stricter checks to be performed downstream.

> If this can't occur, how do we know that the code won't change in the future in such a way to make it possible?

Of course this is never knowable, but I take your point that robustness is a good thing.

> It seems like a better design is to tie this state to the response and then having a way to ask the video element whether the thing it is currently displaying came in an response that was a result of a CORS-enabled request.

I don't know how you're visualizing this, but the way I see it is: make MediaPlayerPrivateInterface::load() return an asyncly-populated response object, and pass that back up through the call-chain to HTMLMediaElement::loadResource so that future questions can be asked of HTMLMediaElement about the options that were attached to the "currently displayed" response.  But I don't see how that'd be any more robust than the current patch, since you still have to hold on to something that was "current as of the last load" as a proxy for what's "currently displaying", and it'd have the same characteristics in terms of load/cancel/etc.

Maybe I'm misunderstanding your suggestion, in which case can you make it more explicit/concrete?

[Adam Barth]

> Maybe I'm misunderstanding your suggestion, in which case can you make it more explicit/concrete?

I don't have a specific design in mind for media.  That's just the approach we use for everything else that uses CORS.

[Adam Barth]

Comment on attachment 146089 [details]
Patch

How does your patch work for ports that don't implement CORS in their media loading pipelines?  It seems like we'll assume that the load was valid w.r.t. CORS without any assurances from the network stack.  In that case, we'll see that we tried to use CORS to kick off the last load and assume that the result is safe to draw on to the canvas...

[Ami Fischman]

Created attachment 146129 [details]
Patch

[Ami Fischman]

(In reply to comment #22)
> (From update of attachment 146089 [details])
> How does your patch work for ports that don't implement CORS in their media loading pipelines?  It seems like we'll assume that the load was valid w.r.t. CORS without any assurances from the network stack.  In that case, we'll see that we tried to use CORS to kick off the last load and assume that the result is safe to draw on to the canvas...

Fair enough.  Since media loading is heavily port-specific (unfortunately) and since my goal here is to get a patch that can be safely merged, I opted to make the platform-specificity fueled by an #if PLATFORM(CHROMIUM) instead of the alternative (overhaul the load API across all ports backed by a dummy impl in all but chromium).  WDYT?

[Adam Barth]

Adding more PLATFORM(CHROMIUM) ifdefs isn't the right approach.  I'm sorry that I'm distracted today and not able to provide you with more concrete guidance.

I'm not familiar with the media loading process, but is there no information that flow back from the loader into WebCore that indicates, for example, that the load is complete or provides a reference to the underlying video retrieved from the network?  If there is such a pathway or data structure, that would seems like a better place for the network stack to indiciate that a CORS-enabled load succeeded.  A non-CORS-aware network stack would presumably indicate that the CORS status of this response is unknown.

[Eric Carlson]

(In reply to comment #25)
> Adding more PLATFORM(CHROMIUM) ifdefs isn't the right approach. 
> 
I agree!

> I'm not familiar with the media loading process, but is there no information that flow back from the loader into WebCore that indicates, for example, that the load is complete or provides a reference to the underlying video retrieved from the network?  If there is such a pathway or data structure, that would seems like a better place for the network stack to indiciate that a CORS-enabled load succeeded.  A non-CORS-aware network stack would presumably indicate that the CORS status of this response is unknown.
> 
What if you don't store any CORS state in MediaPlayer, but instead query the media engine for the CORS status of the current load? 


MediaPlayerClient::CORSMode MediaPlayer::corsModeAtLastLoad() const
{
    return m_private->corsMode();
}

Make the new MediaPlayerPrivateInterface method return Unspecified by default, so ports will automatically signal that they don't have a CORS-aware network stack until they override it.

    virtual MediaPlayerClient::CORSMode corsMode() const { return NoPlatformMedia; }

[Ami Fischman]

Created attachment 146153 [details]
Patch

[Adam Barth]

Comment on attachment 146153 [details]
Patch

This looks great.  Thanks.

[Ami Fischman]

I expected to need to modify the ports that don't use MPPI (QTMovie, QTMovieGWorld, QTMovieVisualContext, GStreamerGWorld, AVPlayer, AVCFPlayer).  I clearly don't understand how MediaPlayer.h:PlatformMedia is meant to work.  We'll see what EWS says.

[Andrew Scherkus]

Comment on attachment 146153 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:83
> +    if (!video->player()->didPassCORSAccessCheck() && wouldTaintOrigin(video->currentSrc()))

AFAIK if video hasn't loaded a src yet then player() is NULL we'd crash

Want to take a page out of HTMLMediaElement::hasSingleSecurityOrigin()'s book, which checks for m_player ptr before delegating to the player?

[Ami Fischman]

Created attachment 146174 [details]
Patch

[Ami Fischman]

(In reply to comment #30)
> (From update of attachment 146153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146153&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext.cpp:83
> > +    if (!video->player()->didPassCORSAccessCheck() && wouldTaintOrigin(video->currentSrc()))
> 
> AFAIK if video hasn't loaded a src yet then player() is NULL we'd crash
> 
> Want to take a page out of HTMLMediaElement::hasSingleSecurityOrigin()'s book, which checks for m_player ptr before delegating to the player?

Done.

[WebKit Review Bot]

Comment on attachment 146174 [details]
Patch

Clearing flags on attachment: 146174

Committed r119694: <http://trac.webkit.org/changeset/119694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

Newly added test fails on Qt,Lion, GTK, EFL after r119694:
http/tests/security/video-cross-origin-readback.html

--- /ramdisk/qt-linux-release/build/layout-test-results/http/tests/security/video-cross-origin-readback-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/http/tests/security/video-cross-origin-readback-actual.txt 
@@ -1,3 +1,4 @@
-EVENT(playing)
+EVENT(error)
+error: {"code":4} FAIL
 END OF TEST

[WebKit Review Bot]

Re-opened since this is blocked by 88529

[Ami Fischman]

Created attachment 146308 [details]
Patch

[Ami Fischman]

Yesterday's patch got rolled out for breaking a chromium-side test, which was then DISABLED_.  This patch is a revert of the rollout with the addition of TestExpectations/Skipped edits for non-chromium ports to skip the new layouttest (since no port other than chromium supports CORS for media).

[WebKit Review Bot]

Comment on attachment 146308 [details]
Patch

Clearing flags on attachment: 146308

Committed r119742: <http://trac.webkit.org/changeset/119742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.