Changeset 200493 in webkit

Timestamp:

May 5, 2016 4:27:08 PM (8 years ago)

Author:

Chris Dumez

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/media/resources/reference.mov (added)
• LayoutTests/http/tests/security/resources/reference-movie-cross-origin-allow.php (added)
• LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt (added)
• LayoutTests/http/tests/security/video-cross-origin-caching.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/MediaResourceLoader.cpp (modified) (1 diff)
• Source/WebCore/platform/network/ResourceRequestBase.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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.
+
 2016-05-05  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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.
+
 2016-05-05  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/loader/MediaResourceLoader.cpp

@@ -67 +67 @@
     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())

trunk/Source/WebCore/platform/network/ResourceRequestBase.cpp

@@ -546 +546 @@
 bool ResourceRequestBase::isConditional() const
 {
+    updateResourceRequest();
+
     for (auto headerName : conditionalHeaderNames) {
         if (m_httpHeaderFields.contains(headerName))
@@ -556 +558 @@
 void ResourceRequestBase::makeUnconditional()
 {
+    updateResourceRequest();
+
     for (auto headerName : conditionalHeaderNames)
         m_httpHeaderFields.remove(headerName);