Bug 112288 – Allow blocking of application cache in third-party contexts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vicki Pfau]

Add a method for allowing blocking of the application cache in third-party contexts, similar what was added for localStorage.

<rdar://problem/13287586>

[Vicki Pfau]

Created attachment 193007 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193007 [details]
Patch

Attachment 193007 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17118175

New failing tests:
http/tests/security/cross-origin-appcache.html

[WebKit Review Bot]

Comment on attachment 193007 [details]
Patch

Attachment 193007 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17179115

New failing tests:
http/tests/security/cross-origin-appcache.html

[Vicki Pfau]

Hmm, this patch makes it virtually impossible to test if the app cache is usable or not. This might not be the best approach, and probably explains why the layout test is failing. All it seems to do is not actually make the request and not raise any events. The applicationCache object is still accessible and still appears to expose everything, even if none of it actually works.

[James Robinson]

Diff from a chromium-linux run of these two tests:

--- /hd2/chrome/src/webkit/Debug/layout-test-results/http/tests/security/cross-origin-appcache-expected.txt
+++ /hd2/chrome/src/webkit/Debug/layout-test-results/http/tests/security/cross-origin-appcache-actual.txt
@@ -8,7 +8,7 @@
 --------
 Frame: '<!--framePath //<!--frame0-->-->'
 --------
-
+Cache found
 
 --------
 Frame: '<!--framePath //<!--frame1-->-->'

[Vicki Pfau]

Created attachment 193188 [details]
Patch

[Adam Barth]

Comment on attachment 193188 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193188&action=review

> Source/WebCore/html/HTMLHtmlElement.cpp:74
> +    if (document()->securityOrigin()->canAccessApplicationCache(document()->topOrigin())) {

Does AppCache work at all in third-party iframes?  I thought AppCache was a per-Page thing.

> Source/WebCore/html/HTMLHtmlElement.cpp:79
> +        if (manifest.isEmpty())
> +            documentLoader->applicationCacheHost()->selectCacheWithoutManifest();
> +        else
> +            documentLoader->applicationCacheHost()->selectCacheWithManifest(document()->completeURL(manifest));

Previously we called one of these two function (i.e., even if there wasn't a manifest attribute).  Now we have a mode where we call neither.  I'm not sure that's correct.

> Source/WebCore/page/DOMWindow.cpp:704
> +    Document* document = this->document();
> +    if (!document)
> +        return 0;

This can never be 0 if isCurrentlyDisplayedInFrame is true.

[Adam Barth]

I wonder if it's more appropriate to check this permission in the same places we check frame->settings()->offlineWebApplicationCacheEnabled

[Vicki Pfau]

(In reply to comment #7)
> (From update of attachment 193188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193188&action=review
> 
> > Source/WebCore/html/HTMLHtmlElement.cpp:74
> > +    if (document()->securityOrigin()->canAccessApplicationCache(document()->topOrigin())) {
> 
> Does AppCache work at all in third-party iframes?  I thought AppCache was a per-Page thing.

It seems to work. Maybe that's a bug in the first place?
 
> > Source/WebCore/html/HTMLHtmlElement.cpp:79
> > +        if (manifest.isEmpty())
> > +            documentLoader->applicationCacheHost()->selectCacheWithoutManifest();
> > +        else
> > +            documentLoader->applicationCacheHost()->selectCacheWithManifest(document()->completeURL(manifest));
> 
> Previously we called one of these two function (i.e., even if there wasn't a manifest attribute).  Now we have a mode where we call neither.  I'm not sure that's correct.

I'm not intimately familiar with how this works. Given that I prevent access to the cache elsewhere, it might make sense to select the cache without the manifest. If you know someone who knows this portion of the code better, it might make sense for me to talk to them.

> > Source/WebCore/page/DOMWindow.cpp:704
> > +    Document* document = this->document();
> > +    if (!document)
> > +        return 0;
> 
> This can never be 0 if isCurrentlyDisplayedInFrame is true.

I saw this idiom elsewhere in the code, so I thought that might not be the case. It does make sense for that to be the case, though. I can revise.

[Adam Barth]

Comment on attachment 193188 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193188&action=review

> Source/WebCore/page/DOMWindow.cpp:707
> +    if (!document->securityOrigin()->canAccessApplicationCache(document->topOrigin()))
> +        return 0;

Also, I'm not sure this line is correct.  "applicationCache" in window will return true but window.applicationCache will return null.  Instead, we should probably act as if the applicationCache feature doesn't exist, which means "applicationCache" in window should return false.

[Adam Barth]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 193188 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193188&action=review
> > 
> > > Source/WebCore/html/HTMLHtmlElement.cpp:74
> > > +    if (document()->securityOrigin()->canAccessApplicationCache(document()->topOrigin())) {
> > 
> > Does AppCache work at all in third-party iframes?  I thought AppCache was a per-Page thing.
> 
> It seems to work. Maybe that's a bug in the first place?

Oh, I could very well be wrong.  I'm not an AppCache expert.

> > > Source/WebCore/html/HTMLHtmlElement.cpp:79
> > > +        if (manifest.isEmpty())
> > > +            documentLoader->applicationCacheHost()->selectCacheWithoutManifest();
> > > +        else
> > > +            documentLoader->applicationCacheHost()->selectCacheWithManifest(document()->completeURL(manifest));
> > 
> > Previously we called one of these two function (i.e., even if there wasn't a manifest attribute).  Now we have a mode where we call neither.  I'm not sure that's correct.
> 
> I'm not intimately familiar with how this works. Given that I prevent access to the cache elsewhere, it might make sense to select the cache without the manifest. If you know someone who knows this portion of the code better, it might make sense for me to talk to them.

If you chase down selectCacheWithoutManifest, you'll see that it eventually checks frame->settings()->offlineWebApplicationCacheEnabled, which is probably where you want to do this check as well.

I didn't chase down selectCacheWithManifest, but I suspect it also checks offlineWebApplicationCacheEnabled.

It's generally better if we turn the feature off in one place instead of two different places.

> > > Source/WebCore/page/DOMWindow.cpp:704
> > > +    Document* document = this->document();
> > > +    if (!document)
> > > +        return 0;
> > 
> > This can never be 0 if isCurrentlyDisplayedInFrame is true.
> 
> I saw this idiom elsewhere in the code, so I thought that might not be the case. It does make sense for that to be the case, though. I can revise.

It's likely old.  That used to be possible, but it's no longer possible now that I've rejiggered the ownership relationships between Frame, DOMWindow, and Document.

[Vicki Pfau]

After digging into this more, it appears that if the application cache is disabled, the DOM window.applicationCache object is still exposed. I'm not sure if this is a bug, but it doesn't work when it's disabled anyway.

Tying the third-party blocking to whether it's enabled is not feasible considering the setting is accessed directly via a getter, so I need to add additional checks around where that getter is called, but that's relatively easy enough.

If we keep the applicationCache object exposed even when the application cache is blocked will make it somewhat difficult to detect if it's blocked, though, as it still tends to behave like it works, and that it just hasn't loaded anything yet.

[Vicki Pfau]

Created attachment 193409 [details]
Patch

[Adam Barth]

> If we keep the applicationCache object exposed even when the application cache is blocked will make it somewhat difficult to detect if it's blocked, though, as it still tends to behave like it works, and that it just hasn't loaded anything yet.

If that's an issue, we should solve that for the case when it's disabled by a WebCore::Setting too.

[Vicki Pfau]

Committed r146115: <http://trac.webkit.org/changeset/146115>