Bug 99037 – [GStreamer] No CORS support for media elements

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

There is currently no support for media elements in non-Chromium ports. This bug covers adding this for the GTK port.
Covered by http/tests/security/video-cross-origin-readback.html.

[youenn fablet]

Created attachment 227027 [details]
Patch

[WebKit Commit Bot]

Attachment 227027 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1026:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.

[Jinwoo Song]

(In reply to comment #3)
> Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.

Yes, EFL port may unskip the test case with this patch!

[Xabier Rodríguez Calvar]

Comment on attachment 227027 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:456
> +    priv->passedCORSChecks = FALSE;

Should be false?

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #3)
> > Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.
> 
> Yes, EFL port may unskip the test case with this patch!

I will unskip it and test EFL accordingly

[youenn fablet]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Nice patch! Is the test also failing on EFL? I suppose it can also be unskipped for that port.
> > 
> > Yes, EFL port may unskip the test case with this patch!
> 
> I will unskip it and test EFL accordingly

(In reply to comment #5)
> (From update of attachment 227027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227027&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:456
> > +    priv->passedCORSChecks = FALSE;
> 
> Should be false?

I do not precisely know the expected coding rule.
passedCORSChecks is currently defined as a gboolean, hence FALSE.
But bool/false is fine by me if that is the preferred way.

[Philippe Normand]

We indeed usually prefer non-GLib types wherever possible.

[Martin Robinson]

Comment on attachment 227027 [details]
Patch

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

Nice fix. Some style nits:

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:62
> +        void handleResponseReceived(const ResourceResponse&, bool);

You should use an enum for this parameter.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:140
> +    gboolean passedCORSChecks;

bool is the right type here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1020
> +    RequestOriginPolicy corsPolicy = (corsMode != MediaPlayerClient::Unspecified) ? PotentiallyCrossOriginEnabled : UseDefaultOriginRestrictionsForType;
> +    StoredCredentials allowCredentials = (corsMode == MediaPlayerClient::UseCredentials) ? AllowStoredCredentials : DoNotAllowStoredCredentials;

Nit: the parenthesis are unnecessary here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1026
> +        m_origin = resourceLoader->document() ? resourceLoader->document()->securityOrigin() : NULL;

You should use nullptr instead of NULL.

[Eric Carlson]

Comment on attachment 227027 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1230
> +Document* MediaPlayer::document()
> +{
> +    if (!m_mediaPlayerClient)
> +        return 0;
> +
> +    return m_mediaPlayerClient->mediaPlayerOwningDocument();
> +}

This is a layering violation, but it doesn't look like it is used. Why is this necessary?

[youenn fablet]

(In reply to comment #10)
> (From update of attachment 227027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227027&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1230
> > +Document* MediaPlayer::document()
> > +{
> > +    if (!m_mediaPlayerClient)
> > +        return 0;
> > +
> > +    return m_mediaPlayerClient->mediaPlayerOwningDocument();
> > +}
> 
> This is a layering violation, but it doesn't look like it is used. Why is this necessary?

The goal is to get the SecurityOrigin and check CORS against it.
Would it be better to add a SecurityOrigin getter instead?

[youenn fablet]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 227027 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227027&action=review
> > 
> > > Source/WebCore/platform/graphics/MediaPlayer.cpp:1230
> > > +Document* MediaPlayer::document()
> > > +{
> > > +    if (!m_mediaPlayerClient)
> > > +        return 0;
> > > +
> > > +    return m_mediaPlayerClient->mediaPlayerOwningDocument();
> > > +}
> > 
> > This is a layering violation, but it doesn't look like it is used. Why is this necessary?
> 
> The goal is to get the SecurityOrigin and check CORS against it.
> Would it be better to add a SecurityOrigin getter instead?

Woups, I was too quick...
Right, this function is no longer used.
The security origin is retrieved from CachedResourceLoader->document().
I will remove this function

[youenn fablet]

Created attachment 227051 [details]
Patch

Fixed style issues, removed MediaPlayer::document(), Updated EFL TestExpectations

[youenn fablet]

Created attachment 227274 [details]
Disable access to CORS failing streams when crossOrigin attribute is set

[youenn fablet]

Last uploaded patch ensures that a stream that fails COR check is not played at all when crossOrigin attribute is set.
This is consistent with other loaders.

It seems to me though that these checks should best be handled at ResourceLoader level in the future.
That would allow to put in common all these CORS checks that already appear at various places in WebKit. 
That would simplify future support of CORS in other places, like in the Mac counterpart of GStreamer.

[Xabier Rodríguez Calvar]

Comment on attachment 227274 [details]
Disable access to CORS failing streams when crossOrigin attribute is set

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:843
> +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("%s", "Cross-origin stream load denied by Cross-Origin Resource Sharing policy."), (0));
> +        else
> +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("Received %d HTTP error code", response.httpStatusCode()), (0));

(0) -> (nullptr)

[youenn fablet]

Created attachment 227281 [details]
fixed nullptr style issue

[youenn fablet]

Is there any other improvement I should bring to the patch?

[Xabier Rodríguez Calvar]

Comment on attachment 227281 [details]
fixed nullptr style issue

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

Again my informal review: I have only some nits in the changelogs that you can fix when just before landing. Anyway as I am not reviewer, somebody should do a proper one and r+.

> Source/WebCore/ChangeLog:27
> +        Related test is http/tests/security/video-cross-origin-readback.html
> +        Disabled access to cross-origin streams that fail CORS check when crossorigin attribute is set.
> +        Related test is http/tests/security/video-cross-origin-accessfailure.html
> +
> +        Tests: http/tests/security/video-cross-origin-accessfailure.html
> +               http/tests/security/video-cross-origin-accesssameorigin.html
> +
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamer::didPassCORSAccessCheck): Return whether media is cross-origin (tainted) or not by querying the gstreamer source layer
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Added MediaPlayerPrivateGStreamer::didPassCORSAccessCheck declaration
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> +        (webKitWebSrcStart): Passed CORS mode parameter to the streaming client. In case of CORS check failure, stop the resource loading
> +        (webKitSrcPassedCORSAccessCheck): Return whether CORS access control check was done and successful.
> +        (StreamingClient::handleResponseReceived): Take a parameter to assign the CORS access control check result
> +        (CachedResourceStreamingClient::CachedResourceStreamingClient): Updated setting of the ResourceLoaderOptions according CORS mode
> +        (CachedResourceStreamingClient::responseReceived): Check CORS
> +        (ResourceHandleStreamingClient::didReceiveResponse): No CORS check
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.h: Added webKitSrcPassedCORSAccessCheck declaration

Nit: at the end of some sentences there is a missing ".". I say it because of coherence.

> LayoutTests/ChangeLog:17
> +        * platform/efl/TestExpectations: Enabled http/tests/security/video-cross-origin-readback.html

Nit: Missing .

> LayoutTests/ChangeLog:18
> +        * platform/gtk/TestExpectations: ditto

Nit: "Ditto."

> LayoutTests/ChangeLog:19
> +        * platform/mac/TestExpectations: Disabled http/tests/security/video-cross-origin-accessfailure.html

Nit: Missing .

[youenn fablet]

(In reply to comment #19)
> (From update of attachment 227281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227281&action=review
> 
> Again my informal review: I have only some nits in the changelogs that you can fix when just before landing. Anyway as I am not reviewer, somebody should do a proper one and r+.


Thanks for the review.
I will update the patch accordingly, then set r? and cq?.

[Xabier Rodríguez Calvar]

(In reply to comment #20)
> Thanks for the review.
> I will update the patch accordingly, then set r? and cq?.

Eric, Martin or Phil, who had commented on this, could do it.

[Philippe Normand]

Comment on attachment 227281 [details]
fixed nullptr style issue

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

Looks quite good, thanks! We just need a final iteration I think :)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:841
> +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("%s", "Cross-origin stream load denied by Cross-Origin Resource Sharing policy."), (nullptr));

What is the %s for there?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1040
> +    // FIXME: In case of PotentiallyCrossOriginEnabled policy, preflight mode should sometimes be used (not simple request headers).
> +    // But CachedResourceLoader does not support it. Use DocumentThreadableLoader instead?

Can you please file a new bug for this and mention it in the comment?

[youenn fablet]

(In reply to comment #22)
> (From update of attachment 227281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227281&action=review
> 
> Looks quite good, thanks! We just need a final iteration I think :)
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:841
> > +            GST_ELEMENT_ERROR(src, RESOURCE, READ, ("%s", "Cross-origin stream load denied by Cross-Origin Resource Sharing policy."), (nullptr));
> 
> What is the %s for there?
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1040
> > +    // FIXME: In case of PotentiallyCrossOriginEnabled policy, preflight mode should sometimes be used (not simple request headers).
> > +    // But CachedResourceLoader does not support it. Use DocumentThreadableLoader instead?
> 
> Can you please file a new bug for this and mention it in the comment?

I filed bug 131484.
Checking what others are doing, Blink decided to not use preflight (https://code.google.com/p/chromium/issues/detail?id=173727), which seems to make sense.
I have not found information for Mozilla.

[youenn fablet]

Created attachment 229048 [details]
Updating after second reviews

[WebKit Commit Bot]

Comment on attachment 229048 [details]
Updating after second reviews

Clearing flags on attachment: 229048

Committed r167073: <http://trac.webkit.org/changeset/167073>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131516

[Martin Robinson]

Tests that crashed (5): flag all

+media/video-canvas-drawing.html
+media/video-canvas-drawing-output.html
+media/W3C/audio/paused/paused_false_during_play.html
+media/video-canvas-source.html
+media/video-canvas-alpha.html

[youenn fablet]

(In reply to comment #28)
> Tests that crashed (5): flag all
> 
> +media/video-canvas-drawing.html
> +media/video-canvas-drawing-output.html
> +media/W3C/audio/paused/paused_false_during_play.html
> +media/video-canvas-source.html
> +media/video-canvas-alpha.html

Thanks for unrolling the patch.

For those 5 tests, WebKitWebSrc->priv is null when calling webKitSrcPassedCORSAccessCheck, leading to the crashes.
When running equivalent tests served through http, everything is fine.

[Philippe Normand]

Will you provide an updated patch Youenn?

[youenn fablet]

Created attachment 229130 [details]
Fixing crashes

[youenn fablet]

(In reply to comment #30)
> Will you provide an updated patch Youenn?

Patch is uploaded, which fixes MediaPlayerPrivateGStreamer::didPassCORSAccessCheck().
It is working fine with EFL-WK1 but I have a problem with my set-up and cannot test it right now for GTK-WK2.
Once double checked, I will put it on review.

[Philippe Normand]

Awesome thank for the follow-up :)

[youenn fablet]

(In reply to comment #31)
> Created an attachment (id=229130) [details]
> Fixing crashes

The identified crashes are gone with the latest patch on GTK-WK2.

[Xabier Rodríguez Calvar]

(In reply to comment #34)
> (In reply to comment #31)
> > Created an attachment (id=229130) [details] [details]
> > Fixing crashes
> 
> The identified crashes are gone with the latest patch on GTK-WK2.

Probably bug 131488.

[WebKit Commit Bot]

Comment on attachment 229130 [details]
Fixing crashes

Clearing flags on attachment: 229130

Committed r167193: <http://trac.webkit.org/changeset/167193>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.