Bug 88349

Created attachment 145843 [details] Patch

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 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 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 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 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.

(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.

Created attachment 145873 [details] Patch

(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.

Created attachment 145876 [details] Patch

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 on attachment 145876 [details] Patch Attachment 145876 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12893869

Created attachment 146074 [details] Patch

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 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.

Created attachment 146089 [details] Patch

(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 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 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.

(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?

> 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 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...

Created attachment 146129 [details] Patch

(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?

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.

(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; }

Created attachment 146153 [details] Patch

Comment on attachment 146153 [details] Patch This looks great. Thanks.

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 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?

Created attachment 146174 [details] Patch

(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 on attachment 146174 [details] Patch Clearing flags on attachment: 146174 Committed r119694: <http://trac.webkit.org/changeset/119694>

All reviewed patches have been landed. Closing bug.

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

Re-opened since this is blocked by 88529

Created attachment 146308 [details] Patch

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 on attachment 146308 [details] Patch Clearing flags on attachment: 146308 Committed r119742: <http://trac.webkit.org/changeset/119742>

All reviewed patches have been landed. Closing bug.