<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[200493] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/200493">200493</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2016-05-05 16:27:08 -0700 (Thu, 05 May 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>CORS check is sometimes incorrectly failing for media loads
https://bugs.webkit.org/show_bug.cgi?id=157370
<rdar://problem/26071607>
Reviewed by Alex Christensen.
Source/WebCore:
When the media library is issuing a conditional request for a media
element that had the 'crossorigin' attribute, we would fail the CORS
check and log an error if the server were to respond with a "304 Not
Modified" response because the 304 response usually does not have
the necessary "Access-Control-Allow-Origin: *" header (At least for
Apache) and we cannot use the cached headers either since WebKit
does not have them.
To work around the problem in the short term, we now drop the
conditional headers from the request that the media library is
giving us when the media element has the 'crossorigin' attribute
set. As a result, the server will never respond with a 304 and we
will be able to do a CORS check on the full (e.g. 206) response.
In the long term, we need to deal with this better as this means
we may sometimes fail to reuse cached data. For now, this is only
potentially inefficient in the cases that were broken (i.e. no
video would play and we would log an error in the console).
Test: http/tests/security/video-cross-origin-caching.html
* loader/MediaResourceLoader.cpp:
(WebCore::MediaResourceLoader::requestResource):
Make the request unconditional if the media element has the
'crossorigin' attribute set.
* platform/network/ResourceRequestBase.cpp:
(WebCore::ResourceRequestBase::isConditional):
(WebCore::ResourceRequestBase::makeUnconditional):
When fixing the bug above, I noticed that those method do not do
the right thing if the m_httpHeaderFields data member has not
been populated yet. m_httpHeaderFields is lazily initialized so
we need to call updateResourceRequest() before using it.
LayoutTests:
Add a regression test for <rdar://problem/26071607>.
* http/tests/media/resources/reference.mov: Added.
* http/tests/security/resources/reference-movie-cross-origin-allow.php: Added.
* http/tests/security/video-cross-origin-caching-expected.txt: Added.
* http/tests/security/video-cross-origin-caching.html: Added.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreloaderMediaResourceLoadercpp">trunk/Source/WebCore/loader/MediaResourceLoader.cpp</a></li>
<li><a href="#trunkSourceWebCoreplatformnetworkResourceRequestBasecpp">trunk/Source/WebCore/platform/network/ResourceRequestBase.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestshttptestsmediaresourcesreferencemov">trunk/LayoutTests/http/tests/media/resources/reference.mov</a></li>
<li><a href="#trunkLayoutTestshttptestssecurityresourcesreferencemoviecrossoriginallowphp">trunk/LayoutTests/http/tests/security/resources/reference-movie-cross-origin-allow.php</a></li>
<li><a href="#trunkLayoutTestshttptestssecurityvideocrossorigincachingexpectedtxt">trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt</a></li>
<li><a href="#trunkLayoutTestshttptestssecurityvideocrossorigincachinghtml">trunk/LayoutTests/http/tests/security/video-cross-origin-caching.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (200492 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2016-05-05 23:25:39 UTC (rev 200492)
+++ trunk/LayoutTests/ChangeLog 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -1,3 +1,18 @@
</span><ins>+2016-05-05 Chris Dumez <cdumez@apple.com>
+
+ CORS check is sometimes incorrectly failing for media loads
+ https://bugs.webkit.org/show_bug.cgi?id=157370
+ <rdar://problem/26071607>
+
+ Reviewed by Alex Christensen.
+
+ Add a regression test for <rdar://problem/26071607>.
+
+ * http/tests/media/resources/reference.mov: Added.
+ * http/tests/security/resources/reference-movie-cross-origin-allow.php: Added.
+ * http/tests/security/video-cross-origin-caching-expected.txt: Added.
+ * http/tests/security/video-cross-origin-caching.html: Added.
+
</ins><span class="cx"> 2016-05-05 Zalan Bujtas <zalan@apple.com>
</span><span class="cx">
</span><span class="cx"> Stop traversing at the container block when computing RTL inline static distance.
</span></span></pre></div>
<a id="trunkLayoutTestshttptestsmediaresourcesreferencemov"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/media/resources/reference.mov (0 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/media/resources/reference.mov (rev 0)
+++ trunk/LayoutTests/http/tests/media/resources/reference.mov 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -0,0 +1 @@
</span><ins>+\x8Fmoov\x87rmrarmdaCrdrfurl /http://127.0.0.1:8000/media/resources/test.mp4rmdr\xE0$rmcdsdecmp4a
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestssecurityresourcesreferencemoviecrossoriginallowphp"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/security/resources/reference-movie-cross-origin-allow.php (0 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/security/resources/reference-movie-cross-origin-allow.php (rev 0)
+++ trunk/LayoutTests/http/tests/security/resources/reference-movie-cross-origin-allow.php 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -0,0 +1,16 @@
</span><ins>+<?php
+
+if (isset($_SERVER["HTTP_IF_MODIFIED_SINCE"]) || isset($_SERVER["HTTP_IF_NONE_MATCH"])) {
+ // Always respond to conditional requests with a 304.
+ header("HTTP/1.1 304 Not Modified");
+ return;
+}
+
+header("Access-Control-Allow-Origin: *");
+header("ETag: foo");
+header("Last-Modified: Thu, 01 Jan 2000 00:00:00 GMT");
+header("Cache-Control: max-age=0");
+
+@include("../../media/resources/reference.mov");
+
+?>
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestssecurityvideocrossorigincachingexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt (0 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -0,0 +1,7 @@
</span><ins>+
+This test passes if you do not see a CORS error.
+
+EVENT(playing)
+EVENT(playing)
+END OF TEST
+
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestssecurityvideocrossorigincachinghtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/security/video-cross-origin-caching.html (0 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/security/video-cross-origin-caching.html (rev 0)
+++ trunk/LayoutTests/http/tests/security/video-cross-origin-caching.html 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -0,0 +1,34 @@
</span><ins>+<!DOCTYPE html>
+<html>
+ <head>
+ <script src=../../media-resources/video-test.js></script>
+ <script>
+ var isFirstLoad = true;
+ var testURL = "http://localhost:8000/security/resources/reference-movie-cross-origin-allow.php";
+
+ waitForEvent('playing', function() {
+ if (!isFirstLoad) {
+ endTest();
+ return;
+ }
+ isFirstLoad = false;
+
+ video.src = "";
+ setTimeout(function() {
+ video.src = testURL;
+ video.play();
+ }, 0);
+ });
+
+ function start() {
+ findMediaElement();
+ video.src = testURL;
+ video.play();
+ }
+ </script>
+ </head>
+ <body onload="start()">
+ <video crossorigin></video>
+ <p>This test passes if you do not see a CORS error.</p>
+ </body>
+</html>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (200492 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2016-05-05 23:25:39 UTC (rev 200492)
+++ trunk/Source/WebCore/ChangeLog 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -1,3 +1,45 @@
</span><ins>+2016-05-05 Chris Dumez <cdumez@apple.com>
+
+ CORS check is sometimes incorrectly failing for media loads
+ https://bugs.webkit.org/show_bug.cgi?id=157370
+ <rdar://problem/26071607>
+
+ Reviewed by Alex Christensen.
+
+ When the media library is issuing a conditional request for a media
+ element that had the 'crossorigin' attribute, we would fail the CORS
+ check and log an error if the server were to respond with a "304 Not
+ Modified" response because the 304 response usually does not have
+ the necessary "Access-Control-Allow-Origin: *" header (At least for
+ Apache) and we cannot use the cached headers either since WebKit
+ does not have them.
+
+ To work around the problem in the short term, we now drop the
+ conditional headers from the request that the media library is
+ giving us when the media element has the 'crossorigin' attribute
+ set. As a result, the server will never respond with a 304 and we
+ will be able to do a CORS check on the full (e.g. 206) response.
+
+ In the long term, we need to deal with this better as this means
+ we may sometimes fail to reuse cached data. For now, this is only
+ potentially inefficient in the cases that were broken (i.e. no
+ video would play and we would log an error in the console).
+
+ Test: http/tests/security/video-cross-origin-caching.html
+
+ * loader/MediaResourceLoader.cpp:
+ (WebCore::MediaResourceLoader::requestResource):
+ Make the request unconditional if the media element has the
+ 'crossorigin' attribute set.
+
+ * platform/network/ResourceRequestBase.cpp:
+ (WebCore::ResourceRequestBase::isConditional):
+ (WebCore::ResourceRequestBase::makeUnconditional):
+ When fixing the bug above, I noticed that those method do not do
+ the right thing if the m_httpHeaderFields data member has not
+ been populated yet. m_httpHeaderFields is lazily initialized so
+ we need to call updateResourceRequest() before using it.
+
</ins><span class="cx"> 2016-05-05 Zalan Bujtas <zalan@apple.com>
</span><span class="cx">
</span><span class="cx"> Stop traversing at the container block when computing RTL inline static distance.
</span></span></pre></div>
<a id="trunkSourceWebCoreloaderMediaResourceLoadercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/MediaResourceLoader.cpp (200492 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/MediaResourceLoader.cpp 2016-05-05 23:25:39 UTC (rev 200492)
+++ trunk/Source/WebCore/loader/MediaResourceLoader.cpp 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -66,9 +66,17 @@
</span><span class="cx"> RequestOriginPolicy corsPolicy = !m_crossOriginMode.isNull() ? PotentiallyCrossOriginEnabled : UseDefaultOriginRestrictionsForType;
</span><span class="cx"> StoredCredentials allowCredentials = m_crossOriginMode.isNull() || equalLettersIgnoringASCIICase(m_crossOriginMode, "use-credentials") ? AllowStoredCredentials : DoNotAllowStoredCredentials;
</span><span class="cx">
</span><ins>+ auto updatedRequest = request;
+#if HAVE(AVFOUNDATION_LOADER_DELEGATE) && PLATFORM(MAC)
+ // FIXME: Workaround for <rdar://problem/26071607>. We are not able to do CORS checking on 304 responses because they
+ // are usually missing the headers we need.
+ if (corsPolicy == PotentiallyCrossOriginEnabled)
+ updatedRequest.makeUnconditional();
+#endif
+
</ins><span class="cx"> // FIXME: Skip Content Security Policy check if the element that inititated this request
</span><span class="cx"> // is in a user-agent shadow tree. See <https://bugs.webkit.org/show_bug.cgi?id=155505>.
</span><del>- CachedResourceRequest cacheRequest(request, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, allowCredentials, DoNotAskClientForCrossOriginCredentials, ClientDidNotRequestCredentials, DoSecurityCheck, corsPolicy, DoNotIncludeCertificateInfo, ContentSecurityPolicyImposition::DoPolicyCheck, DefersLoadingPolicy::AllowDefersLoading, CachingPolicy::AllowCaching));
</del><ins>+ CachedResourceRequest cacheRequest(updatedRequest, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, allowCredentials, DoNotAskClientForCrossOriginCredentials, ClientDidNotRequestCredentials, DoSecurityCheck, corsPolicy, DoNotIncludeCertificateInfo, ContentSecurityPolicyImposition::DoPolicyCheck, DefersLoadingPolicy::AllowDefersLoading, CachingPolicy::AllowCaching));
</ins><span class="cx">
</span><span class="cx"> if (!m_crossOriginMode.isNull())
</span><span class="cx"> updateRequestForAccessControl(cacheRequest.mutableResourceRequest(), m_document->securityOrigin(), allowCredentials);
</span></span></pre></div>
<a id="trunkSourceWebCoreplatformnetworkResourceRequestBasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/platform/network/ResourceRequestBase.cpp (200492 => 200493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/platform/network/ResourceRequestBase.cpp 2016-05-05 23:25:39 UTC (rev 200492)
+++ trunk/Source/WebCore/platform/network/ResourceRequestBase.cpp 2016-05-05 23:27:08 UTC (rev 200493)
</span><span class="lines">@@ -545,6 +545,8 @@
</span><span class="cx">
</span><span class="cx"> bool ResourceRequestBase::isConditional() const
</span><span class="cx"> {
</span><ins>+ updateResourceRequest();
+
</ins><span class="cx"> for (auto headerName : conditionalHeaderNames) {
</span><span class="cx"> if (m_httpHeaderFields.contains(headerName))
</span><span class="cx"> return true;
</span><span class="lines">@@ -555,6 +557,8 @@
</span><span class="cx">
</span><span class="cx"> void ResourceRequestBase::makeUnconditional()
</span><span class="cx"> {
</span><ins>+ updateResourceRequest();
+
</ins><span class="cx"> for (auto headerName : conditionalHeaderNames)
</span><span class="cx"> m_httpHeaderFields.remove(headerName);
</span><span class="cx"> }
</span></span></pre>
</div>
</div>
</body>
</html>