Cache Storage: restrict access to secure origins (Blink-side)

Source/modules/cachestorage/InspectorCacheStorageAgent.cpp

Index: Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
diff --git a/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp b/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
index f9ed2ba4e3882067bb61613770a84a24cfabcc7f..003c0316e0021879f5d9072692a7e633247392c2 100644
--- a/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
+++ b/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
@@ -7,6 +7,11 @@
 
 #include "core/InspectorBackendDispatcher.h"
 #include "core/InspectorTypeBuilder.h"
+#include "core/dom/Document.h"
+#include "core/dom/ExecutionContext.h"
+#include "core/frame/Frame.h"
+#include "core/page/Page.h"
+#include "modules/cachestorage/CacheStorage.h"
 #include "platform/JSONValues.h"
 #include "platform/heap/Handle.h"
 #include "platform/weborigin/DatabaseIdentifier.h"
@@ -66,25 +71,6 @@ bool parseCacheId(ErrorString* errorString, const String& id, String* securityOr
     return true;
 }
 
-PassOwnPtr<WebServiceWorkerCacheStorage> assertCacheStorage(ErrorString* errorString, const String& securityOrigin)
-{
-    RefPtr<SecurityOrigin> secOrigin = SecurityOrigin::createFromString(securityOrigin);
-    String identifier = createDatabaseIdentifierFromSecurityOrigin(secOrigin.get());
-    OwnPtr<WebServiceWorkerCacheStorage> cache = adoptPtr(Platform::current()->cacheStorage(identifier));
-    if (!cache)
-        *errorString = "Could not find cache storage.";
-    return cache.release();
-}
-
-PassOwnPtr<WebServiceWorkerCacheStorage> assertCacheStorageAndNameForId(ErrorString* errorString, const String& cacheId, String* cacheName)
-{
-    String securityOrigin;
-    if (!parseCacheId(errorString, cacheId, &securityOrigin, cacheName)) {
-        return nullptr;
-    }
-    return assertCacheStorage(errorString, securityOrigin);
-}
-
 CString serviceWorkerCacheErrorString(WebServiceWorkerCacheError* error)
 {
     switch (*error) {
@@ -118,10 +104,10 @@ public:
 
     void onSuccess(WebVector<WebString>* caches)
     {
-        RefPtr<Array<Cache>> array = Array<Cache>::create();
+        RefPtr<Array<TypeBuilder::CacheStorage::Cache>> array = Array<TypeBuilder::CacheStorage::Cache>::create();
         for (size_t i = 0; i < caches->size(); i++) {
             String name = String((*caches)[i]);
-            RefPtr<Cache> entry = Cache::create()
+            RefPtr<TypeBuilder::CacheStorage::Cache> entry = TypeBuilder::CacheStorage::Cache::create()
                 .setSecurityOrigin(m_securityOrigin)
                 .setCacheName(name)
                 .setCacheId(buildCacheId(m_securityOrigin, name));
@@ -392,8 +378,9 @@ private:
 
 } // namespace
 
-InspectorCacheStorageAgent::InspectorCacheStorageAgent()
+InspectorCacheStorageAgent::InspectorCacheStorageAgent(Page* page)
     : InspectorBaseAgent<InspectorCacheStorageAgent, InspectorFrontend::CacheStorage>("CacheStorage")
+    , m_page(page)
 {
 }
 
@@ -406,6 +393,19 @@ DEFINE_TRACE(InspectorCacheStorageAgent)
 
 void InspectorCacheStorageAgent::requestCacheNames(ErrorString* errorString, const String& securityOrigin, PassRefPtrWillBeRawPtr<RequestCacheNamesCallback> callback)
 {
+    RefPtr<SecurityOrigin> secOrigin = SecurityOrigin::createFromString(securityOrigin);
+    ExecutionContext* executionContext = assertExecutionContextForOrigin(errorString, secOrigin.get());
+    if (!executionContext) {
+        callback->sendFailure(*errorString);
+        return;
+    }
+
+    if (!CacheStorage::canAccessCacheStorage(executionContext)) {

[pfeldman, 2015/08/03 23:53:49]

Inspector should have access to everything, but we might want to let user know
that the page does not have access to these.

[jsbell, 2015/08/04 00:14:53]

Implicitly, entries won't exist if the page doesn't have access since the page
can't create them. I'm not sure what you're thinking here.

+        // Don't treat this as an error, just don't attempt to open and enumerate the caches.
+        callback->sendSuccess(Array<TypeBuilder::CacheStorage::Cache>::create());
+        return;
+    }
+
     OwnPtr<WebServiceWorkerCacheStorage> cache = assertCacheStorage(errorString, securityOrigin);
     if (!cache) {
         callback->sendFailure(*errorString);
@@ -451,5 +451,40 @@ void InspectorCacheStorageAgent::deleteEntry(ErrorString* errorString, const Str
     cache->dispatchOpen(new GetCacheForDeleteEntry(request, cacheName, callback), WebString(cacheName));
 }
 
+ExecutionContext* InspectorCacheStorageAgent::assertExecutionContextForOrigin(ErrorString* error, SecurityOrigin* origin)
+{
+    for (Frame* frame = m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {

[pfeldman, 2015/08/03 23:53:48]

This approach is super-confusing, I was so happy it went away. Could you explain
what is happening here that we need to go via the blink core and execution
context? What are you trying to do here?

[jsbell, 2015/08/04 00:14:53]

Top level: don't try to and query CacheStorage if the origin isn't secure, else
the browser will nuke this process. (i.e. same thing the script-facing API does)

CacheStorage is calling ExecutionContext::isPrivilegedContext() which is (so far
as know) how we want to validate that the origin is permitted to use the API.
That does more than simply calling SecurityOrigin::isPotentiallyTrustworthy()
though - it walks the document ancestor chain.

But... given that we have a SecurityOrigin here anyway, perhaps we don't want to
try and mirror the exact check done by the document; calling
SecurityOrigin::isPotentiallyTrustworthy() should match the check done on the
Chromium side, which is sufficient to achieve the goal above.

palmer@?

(That would let us nuke most of the changes here, at the risk of having
different checks here vs. in CacheStorage, although the ones here would be a
strict subset.)

+        if (!frame->isLocalFrame())
+            continue;
+        LocalFrame* localFrame = toLocalFrame(frame);
+        if (localFrame->document() && localFrame->document()->securityOrigin()->isSameSchemeHostPort(origin))
+            return localFrame->document();
+    }
+
+    *error = "No frame is available for the request";
+    return 0;
+}
+
+PassOwnPtr<WebServiceWorkerCacheStorage> InspectorCacheStorageAgent::assertCacheStorage(ErrorString* errorString, const String& securityOrigin)
+{
+    RefPtr<SecurityOrigin> secOrigin = SecurityOrigin::createFromString(securityOrigin);
+    ExecutionContext* executionContext = assertExecutionContextForOrigin(errorString, secOrigin.get());
+    if (!executionContext || !CacheStorage::canAccessCacheStorage(executionContext, errorString))
+        return nullptr;
+
+    String identifier = createDatabaseIdentifierFromSecurityOrigin(secOrigin.get());
+    OwnPtr<WebServiceWorkerCacheStorage> cache = adoptPtr(Platform::current()->cacheStorage(identifier));
+    if (!cache)
+        *errorString = "Could not find cache storage.";
+    return cache.release();
+}
+
+PassOwnPtr<WebServiceWorkerCacheStorage> InspectorCacheStorageAgent::assertCacheStorageAndNameForId(ErrorString* errorString, const String& cacheId, String* cacheName)
+{
+    String securityOrigin;
+    if (!parseCacheId(errorString, cacheId, &securityOrigin, cacheName))
+        return nullptr;
+    return assertCacheStorage(errorString, securityOrigin);
+}
 
 } // namespace blink