Bug 88349 - Plumb CORS attribute information from HTMLMediaElement to media players so it can be used
Summary: Plumb CORS attribute information from HTMLMediaElement to media players so it...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ami Fischman
URL:
Keywords:
Depends on: 88388 88523 88529
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-05 11:15 PDT by Ami Fischman
Modified: 2012-06-07 11:38 PDT (History)
13 users (show)

See Also:


Attachments
Patch (5.55 KB, patch)
2012-06-05 11:18 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (7.75 KB, patch)
2012-06-05 14:50 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2012-06-05 15:01 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (11.89 KB, patch)
2012-06-06 11:30 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2012-06-06 12:56 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (11.84 KB, patch)
2012-06-06 15:35 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (12.70 KB, patch)
2012-06-06 17:07 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2012-06-06 18:40 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (18.39 KB, patch)
2012-06-07 09:33 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2012-06-05 11:15:38 PDT
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.
Comment 1 Ami Fischman 2012-06-05 11:18:03 PDT
Created attachment 145843 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-05 11:20:40 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-06-05 11:43:46 PDT
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 ?
Comment 4 Ami Fischman 2012-06-05 12:10:28 PDT
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.
Comment 5 Adam Barth 2012-06-05 13:29:47 PDT
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?
Comment 6 Adam Barth 2012-06-05 13:33:29 PDT
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.
Comment 7 Eric Carlson 2012-06-05 13:41:26 PDT
(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.
Comment 8 Ami Fischman 2012-06-05 14:50:25 PDT
Created attachment 145873 [details]
Patch
Comment 9 Ami Fischman 2012-06-05 14:53:43 PDT
(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.
Comment 10 Ami Fischman 2012-06-05 15:01:36 PDT
Created attachment 145876 [details]
Patch
Comment 11 Adam Barth 2012-06-05 15:13:32 PDT
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 ?
Comment 12 WebKit Review Bot 2012-06-06 04:48:54 PDT
Comment on attachment 145876 [details]
Patch

Attachment 145876 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12893869
Comment 13 Ami Fischman 2012-06-06 11:30:14 PDT
Created attachment 146074 [details]
Patch
Comment 14 Ami Fischman 2012-06-06 11:32:04 PDT
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?
Comment 15 Eric Carlson 2012-06-06 12:36:37 PDT
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.
Comment 16 Ami Fischman 2012-06-06 12:56:36 PDT
Created attachment 146089 [details]
Patch
Comment 17 Ami Fischman 2012-06-06 12:57:09 PDT
(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.
Comment 18 Eric Carlson 2012-06-06 13:25:36 PDT
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!
Comment 19 Adam Barth 2012-06-06 13:58:52 PDT
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.
Comment 20 Ami Fischman 2012-06-06 14:46:11 PDT
(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?
Comment 21 Adam Barth 2012-06-06 14:50:56 PDT
> 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.
Comment 22 Adam Barth 2012-06-06 14:52:47 PDT
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...
Comment 23 Ami Fischman 2012-06-06 15:35:19 PDT
Created attachment 146129 [details]
Patch
Comment 24 Ami Fischman 2012-06-06 15:37:06 PDT
(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?
Comment 25 Adam Barth 2012-06-06 15:57:00 PDT
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.
Comment 26 Eric Carlson 2012-06-06 16:57:08 PDT
(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; }
Comment 27 Ami Fischman 2012-06-06 17:07:10 PDT
Created attachment 146153 [details]
Patch
Comment 28 Adam Barth 2012-06-06 17:09:43 PDT
Comment on attachment 146153 [details]
Patch

This looks great.  Thanks.
Comment 29 Ami Fischman 2012-06-06 17:14:37 PDT
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.
Comment 30 Andrew Scherkus 2012-06-06 18:09:56 PDT
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?
Comment 31 Ami Fischman 2012-06-06 18:40:07 PDT
Created attachment 146174 [details]
Patch
Comment 32 Ami Fischman 2012-06-06 18:40:31 PDT
(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.
Comment 33 WebKit Review Bot 2012-06-07 01:48:42 PDT
Comment on attachment 146174 [details]
Patch

Clearing flags on attachment: 146174

Committed r119694: <http://trac.webkit.org/changeset/119694>
Comment 34 WebKit Review Bot 2012-06-07 01:48:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Zoltan Arvai 2012-06-07 02:52:07 PDT
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
Comment 36 WebKit Review Bot 2012-06-07 05:53:16 PDT
Re-opened since this is blocked by 88529
Comment 37 Ami Fischman 2012-06-07 09:33:57 PDT
Created attachment 146308 [details]
Patch
Comment 38 Ami Fischman 2012-06-07 09:40:38 PDT
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).
Comment 39 WebKit Review Bot 2012-06-07 11:37:57 PDT
Comment on attachment 146308 [details]
Patch

Clearing flags on attachment: 146308

Committed r119742: <http://trac.webkit.org/changeset/119742>
Comment 40 WebKit Review Bot 2012-06-07 11:38:05 PDT
All reviewed patches have been landed.  Closing bug.