- Revision
- 200493
- Author
- [email protected]
- Date
- 2016-05-05 16:27:08 -0700 (Thu, 05 May 2016)
Log Message
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (200492 => 200493)
--- trunk/LayoutTests/ChangeLog 2016-05-05 23:25:39 UTC (rev 200492)
+++ trunk/LayoutTests/ChangeLog 2016-05-05 23:27:08 UTC (rev 200493)
@@ -1,3 +1,18 @@
+2016-05-05 Chris Dumez <[email protected]>
+
+ 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.
+
2016-05-05 Zalan Bujtas <[email protected]>
Stop traversing at the container block when computing RTL inline static distance.
Added: trunk/LayoutTests/http/tests/media/resources/reference.mov (0 => 200493)
--- 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)
@@ -0,0 +1 @@
+\x8Fmoov\x87rmrarmdaCrdrfurl /http://127.0.0.1:8000/media/resources/test.mp4rmdr\xE0$rmcdsdecmp4a
Added: trunk/LayoutTests/http/tests/security/resources/reference-movie-cross-origin-allow.php (0 => 200493)
--- 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)
@@ -0,0 +1,16 @@
+<?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");
+
+?>
Added: trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt (0 => 200493)
--- 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)
@@ -0,0 +1,7 @@
+
+This test passes if you do not see a CORS error.
+
+EVENT(playing)
+EVENT(playing)
+END OF TEST
+
Added: trunk/LayoutTests/http/tests/security/video-cross-origin-caching.html (0 => 200493)
--- 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)
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+ <head>
+ <script src=""
+ <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 = ""
+ video.play();
+ }, 0);
+ });
+
+ function start() {
+ findMediaElement();
+ video.src = ""
+ 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>
Modified: trunk/Source/WebCore/ChangeLog (200492 => 200493)
--- 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)
@@ -1,3 +1,45 @@
+2016-05-05 Chris Dumez <[email protected]>
+
+ 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.
+
2016-05-05 Zalan Bujtas <[email protected]>
Stop traversing at the container block when computing RTL inline static distance.
Modified: trunk/Source/WebCore/loader/MediaResourceLoader.cpp (200492 => 200493)
--- 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)
@@ -66,9 +66,17 @@
RequestOriginPolicy corsPolicy = !m_crossOriginMode.isNull() ? PotentiallyCrossOriginEnabled : UseDefaultOriginRestrictionsForType;
StoredCredentials allowCredentials = m_crossOriginMode.isNull() || equalLettersIgnoringASCIICase(m_crossOriginMode, "use-credentials") ? AllowStoredCredentials : DoNotAllowStoredCredentials;
+ 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
+
// FIXME: Skip Content Security Policy check if the element that inititated this request
// is in a user-agent shadow tree. See <https://bugs.webkit.org/show_bug.cgi?id=155505>.
- CachedResourceRequest cacheRequest(request, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, allowCredentials, DoNotAskClientForCrossOriginCredentials, ClientDidNotRequestCredentials, DoSecurityCheck, corsPolicy, DoNotIncludeCertificateInfo, ContentSecurityPolicyImposition::DoPolicyCheck, DefersLoadingPolicy::AllowDefersLoading, CachingPolicy::AllowCaching));
+ CachedResourceRequest cacheRequest(updatedRequest, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, allowCredentials, DoNotAskClientForCrossOriginCredentials, ClientDidNotRequestCredentials, DoSecurityCheck, corsPolicy, DoNotIncludeCertificateInfo, ContentSecurityPolicyImposition::DoPolicyCheck, DefersLoadingPolicy::AllowDefersLoading, CachingPolicy::AllowCaching));
if (!m_crossOriginMode.isNull())
updateRequestForAccessControl(cacheRequest.mutableResourceRequest(), m_document->securityOrigin(), allowCredentials);
Modified: trunk/Source/WebCore/platform/network/ResourceRequestBase.cpp (200492 => 200493)
--- 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)
@@ -545,6 +545,8 @@
bool ResourceRequestBase::isConditional() const
{
+ updateResourceRequest();
+
for (auto headerName : conditionalHeaderNames) {
if (m_httpHeaderFields.contains(headerName))
return true;
@@ -555,6 +557,8 @@
void ResourceRequestBase::makeUnconditional()
{
+ updateResourceRequest();
+
for (auto headerName : conditionalHeaderNames)
m_httpHeaderFields.remove(headerName);
}
