RESOLVED FIXED 61403
Video fails to play on Vimeo
https://bugs.webkit.org/show_bug.cgi?id=61403
Summary Video fails to play on Vimeo
Jer Noble
Reported 2011-05-24 16:22:18 PDT
Video fails to play on Vimeo
Attachments
Patch (9.63 KB, patch)
2011-05-24 16:31 PDT, Jer Noble
webkit.review.bot: commit-queue-
Patch (13.50 KB, patch)
2011-05-24 17:21 PDT, Jer Noble
no flags
Patch (15.70 KB, patch)
2011-05-25 02:22 PDT, Jer Noble
no flags
Patch (14.76 KB, patch)
2011-05-25 09:40 PDT, Jer Noble
no flags
Patch (14.52 KB, patch)
2011-05-25 10:08 PDT, Jer Noble
no flags
Patch (360.58 KB, patch)
2011-05-25 11:54 PDT, Jer Noble
no flags
Patch (361.25 KB, patch)
2011-05-25 14:05 PDT, Jer Noble
darin: review+
Jer Noble
Comment 1 2011-05-24 16:23:06 PDT
Jer Noble
Comment 2 2011-05-24 16:31:23 PDT
Jer Noble
Comment 3 2011-05-24 17:21:35 PDT
WebKit Review Bot
Comment 4 2011-05-24 17:47:23 PDT
Comment on attachment 94710 [details] Patch Attachment 94710 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8732554
WebKit Review Bot
Comment 5 2011-05-24 20:08:33 PDT
Comment on attachment 94720 [details] Patch Attachment 94720 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8733424
WebKit Commit Bot
Comment 6 2011-05-24 20:43:43 PDT
Maciej Stachowiak
Comment 7 2011-05-24 21:23:39 PDT
Comment on attachment 94720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94720&action=review r- because this appears to break the build. I am not entirely sure why, and otherwise it seems ok though see other comment. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:259 > - [NSNumber numberWithInt:AVAssetReferenceRestrictionForbidCrossSiteReference], AVURLAssetReferenceRestrictionsKey, > + [NSNumber numberWithInt:AVAssetReferenceRestrictionForbidRemoteReferenceToLocal | AVAssetReferenceRestrictionForbidLocalReferenceToRemote], AVURLAssetReferenceRestrictionsKey, Is there any way we can allow cross-origin redirects (which seem safe enough) while still preventing cross-origin auxiliary resource references (e.g. reference movies)? I think that should be the goal, but if that's not achievable, this approach seems ok.
Jer Noble
Comment 8 2011-05-25 01:52:22 PDT
(In reply to comment #7) > (From update of attachment 94720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94720&action=review > > r- because this appears to break the build. I am not entirely sure why, and otherwise it seems ok though see other comment. Looks like it's because of a missing newline from WebCoreSystemInterface.mm. I'll get that fixed. > Is there any way we can allow cross-origin redirects (which seem safe enough) while still preventing cross-origin auxiliary resource references (e.g. reference movies)? I think that should be the goal, but if that's not achievable, this approach seems ok. Unfortunately, QTKit doesn't make any distinction between reference movies and redirects. We could, in theory, take over loading somewhat by checking for HTTP redirects before passing the request on to QTKit, but this would be a major disruptive change.
Jer Noble
Comment 9 2011-05-25 02:22:48 PDT
Created attachment 94758 [details] Patch Fixed mac build, and made the changes to MediaPlayerPrivateQTKit >SnowLeopard only.
WebKit Review Bot
Comment 10 2011-05-25 02:40:23 PDT
Comment on attachment 94758 [details] Patch Attachment 94758 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8731660
WebKit Commit Bot
Comment 11 2011-05-25 02:41:25 PDT
Jer Noble
Comment 12 2011-05-25 09:40:51 PDT
WebKit Commit Bot
Comment 13 2011-05-25 09:51:00 PDT
Jer Noble
Comment 14 2011-05-25 10:08:54 PDT
Created attachment 94800 [details] Patch One last piece wrapped with >SL conditionals.
Jer Noble
Comment 15 2011-05-25 11:06:38 PDT
Comment on attachment 94800 [details] Patch After talking with Eric Carlson, we've decided that the QTKit fix should be platform independent. So I'm taking down this patch for review until I whip up a new one.
Jer Noble
Comment 16 2011-05-25 11:54:30 PDT
WebKit Review Bot
Comment 17 2011-05-25 11:57:22 PDT
Attachment 94822 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 WebKitLibraries/WebKitSystemInterface.h:210: The parameter name "movie" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 18 2011-05-25 13:24:24 PDT
Comment on attachment 94822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94822&action=review > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:785 > + return requestedURL.host() == resolvedURL.host() && requestedURL.port() == resolvedURL.port(); Normally, security origin checks need to cover protocol as well as host and port. It’s probably safest to do security origin checks by constructing SecurityOrigin objects and using the member functions of the SecurityOrigin class rather than writing custom logic here. But if we have to have custom logic, it should be sure to check protocol. > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1621 > + return requestedURL.host() == resolvedURL.host() && requestedURL.port() == resolvedURL.port(); Same comment.
Jer Noble
Comment 19 2011-05-25 14:05:21 PDT
Darin Adler
Comment 20 2011-05-25 14:08:04 PDT
Comment on attachment 94853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94853&action=review > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:786 > + return resolvedOrigin->isSameSchemeHostPort(requestedOrigin.get()); This check is probably too strict. The correct check would probably be to check if the original security origin SecurityOrigin::canRequest the resolved asset URL. The original assertURL doesn’t matter at all. We can live with this for now.
Adam Barth
Comment 21 2011-05-25 14:41:45 PDT
Comment on attachment 94853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94853&action=review >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:786 >> + return resolvedOrigin->isSameSchemeHostPort(requestedOrigin.get()); > > This check is probably too strict. > > The correct check would probably be to check if the original security origin SecurityOrigin::canRequest the resolved asset URL. The original assertURL doesn’t matter at all. > > We can live with this for now. Yep.
Jer Noble
Comment 22 2011-05-25 15:53:37 PDT
Darin Adler
Comment 23 2011-05-26 10:36:23 PDT
When we want to improve the security check further, there are three considerations I can think of: 1) We will have to double check whether canAccess, canRequest, or canDisplay is the correct check to make for media. My initial thought is that it should be canDisplay, despite the fact that my earlier comment mentioned canRequest. Someone needs to think it through and presumably check what we already do in the other media code paths. 2) The canDisplay check establishes that it’s OK for the page to display the media, but I’m not sure if it sufficiently checks that the media can be trusted to affect the page. Sorry this is worded so vaguely; I’m not sure what language to use to talk about this type of issue or how we already deal with it. 3) There are resources other than the mail asset URL and we would like WebKit to perform the security checks for all resources. Even though QuickTime has an ability to do its own security checks, we would instead like to plumb the checks back through to WebKit so it can establish the policy.
Adam Barth
Comment 24 2011-05-26 10:46:47 PDT
I'm not sure I fully understand the issues here. > 1) We will have to double check whether canAccess, canRequest, or canDisplay is the correct check to make for media. My initial thought is that it should be canDisplay, despite the fact that my earlier comment mentioned canRequest. Someone needs to think it through and presumably check what we already do in the other media code paths. Certainly we should check whether the web page can display the media (which involves checking each hop in the redirect chain). > 2) The canDisplay check establishes that it’s OK for the page to display the media, but I’m not sure if it sufficiently checks that the media can be trusted to affect the page. Sorry this is worded so vaguely; I’m not sure what language to use to talk about this type of issue or how we already deal with it. How can the media affect the page? My understanding is the media cannot script the embedding page, so the affect that it can have on the embedding page is minimal. If the media tries to script the page (i.e., if that's possible), we need to check whether the media can access the page, like we do for iframes. > 3) There are resources other than the mail asset URL and we would like WebKit to perform the security checks for all resources. Even though QuickTime has an ability to do its own security checks, we would instead like to plumb the checks back through to WebKit so it can establish the policy. If the media's security context cannot be described by a single URL, it's going to be complicated to do the access checks correctly. For example, images have a notion of whether they have a single security origin. In cases where they have more than one security origin, we just cause all access checks to fail (e.g., tainting canvases). I guess the main thing I don't understand is how media can affect the embedding page. I was under the impression that embedding a video was much like embedding an image. Is there more that media can do to the enclosing page?
Darin Adler
Comment 25 2011-05-26 16:11:49 PDT
(In reply to comment #24) > Certainly we should check whether the web page can display the media (which involves checking each hop in the redirect chain). Makes sense. We should call canDisplay on each. > > 2) The canDisplay check establishes that it’s OK for the page to display the media, but I’m not sure if it sufficiently checks that the media can be trusted to affect the page. Sorry this is worded so vaguely; I’m not sure what language to use to talk about this type of issue or how we already deal with it. > > How can the media affect the page? My understanding is the media cannot script the embedding page, so the affect that it can have on the embedding page is minimal. > > If the media tries to script the page (i.e., if that's possible), we need to check whether the media can access the page, like we do for frames. OK, makes total sense. I think the media can’t. I’m thinking back to QuickTime and the QuickTime plug-in, which at one point could host more-active media with scripts in it. But I don’t think this is true for the media element. > > 3) There are resources other than the mail asset URL and we would like WebKit to perform the security checks for all resources. Even though QuickTime has an ability to do its own security checks, we would instead like to plumb the checks back through to WebKit so it can establish the policy. > > If the media's security context cannot be described by a single URL, it's going to be complicated to do the access checks correctly. For example, images have a notion of whether they have a single security origin. In cases where they have more than one security origin, we just cause all access checks to fail (e.g., tainting canvases). I see. This might apply to some media, but again I may be thinking more of the more-complex QuickTime past here. > I guess the main thing I don't understand is how media can affect the embedding page. I was under the impression that embedding a video was much like embedding an image. Is there more that media can do to the enclosing page? I think that was my mistake, and the answer is no.
Ademar Reis
Comment 26 2011-06-03 14:02:31 PDT
Revision r87328 cherry-picked into qtwebkit-2.2 with commit b7169cc <http://gitorious.org/webkit/qtwebkit/commit/b7169cc>
Note You need to log in before you can comment on or make changes to this bug.