Bug 169868 – [QuickLook] Subresources should be in the same origin as the main document

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Andy Estes]

[QuickLook] Subresources should be in the same origin as the main document

[Andy Estes]

rdar://problem/29898214

[Andy Estes]

Created attachment 304906 [details]
Patch

[Andy Estes]

Created attachment 304907 [details]
Patch

[Build Bot]

Comment on attachment 304907 [details]
Patch

Attachment 304907 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3365686

New failing tests:
quicklook/powerpoint.html
quicklook/invalid-ql-id-url.html
quicklook/word.html
quicklook/word-legacy.html
quicklook/powerpoint-legacy.html

[Build Bot]

Created attachment 304911 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Andy Estes]

Created attachment 304930 [details]
Patch

[Daniel Bates]

Comment on attachment 304930 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6823
> +    const URL& responseURL = m_frame->loader().activeDocumentLoader()->responseURL();
> +    ASSERT(responseURL.protocolIs(QLPreviewProtocol()));
> +    setSecurityOriginPolicy(SecurityOriginPolicy::create(SecurityOrigin::create(responseURL)));

This will allow QuickLook documents to use local storage/databases/cookies. Is this intentional? Currently we disallow such storage access because QuickLook documents have a unique origin.

[Andy Estes]

(In reply to comment #7)
> Comment on attachment 304930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304930&action=review
> 
> > Source/WebCore/dom/Document.cpp:6823
> > +    const URL& responseURL = m_frame->loader().activeDocumentLoader()->responseURL();
> > +    ASSERT(responseURL.protocolIs(QLPreviewProtocol()));
> > +    setSecurityOriginPolicy(SecurityOriginPolicy::create(SecurityOrigin::create(responseURL)));
> 
> This will allow QuickLook documents to use local storage/databases/cookies.
> Is this intentional? Currently we disallow such storage access because
> QuickLook documents have a unique origin.

Yeah, that's a side effect of this change, but since QuickLook documents won't share an origin with the hosting site, I don't think this is a problem.

[Daniel Bates]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 304930 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=304930&action=review
> > 
> > > Source/WebCore/dom/Document.cpp:6823
> > > +    const URL& responseURL = m_frame->loader().activeDocumentLoader()->responseURL();
> > > +    ASSERT(responseURL.protocolIs(QLPreviewProtocol()));
> > > +    setSecurityOriginPolicy(SecurityOriginPolicy::create(SecurityOrigin::create(responseURL)));
> > 
> > This will allow QuickLook documents to use local storage/databases/cookies.
> > Is this intentional? Currently we disallow such storage access because
> > QuickLook documents have a unique origin.
> 
> Yeah, that's a side effect of this change, but since QuickLook documents
> won't share an origin with the hosting site, I don't think this is a problem.

Would it make sense to disable such storage access for QuickLook documents using SecurityOrigin::setStorageBlockingPolicy()?

[Andy Estes]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 304930 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=304930&action=review
> > > 
> > > > Source/WebCore/dom/Document.cpp:6823
> > > > +    const URL& responseURL = m_frame->loader().activeDocumentLoader()->responseURL();
> > > > +    ASSERT(responseURL.protocolIs(QLPreviewProtocol()));
> > > > +    setSecurityOriginPolicy(SecurityOriginPolicy::create(SecurityOrigin::create(responseURL)));
> > > 
> > > This will allow QuickLook documents to use local storage/databases/cookies.
> > > Is this intentional? Currently we disallow such storage access because
> > > QuickLook documents have a unique origin.
> > 
> > Yeah, that's a side effect of this change, but since QuickLook documents
> > won't share an origin with the hosting site, I don't think this is a problem.
> 
> Would it make sense to disable such storage access for QuickLook documents
> using SecurityOrigin::setStorageBlockingPolicy()?

Since QuickLook origins are ephemeral, using storage doesn't seem useful, so I agree that this would make sense.

[Andy Estes]

Created attachment 304940 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304940 [details]
Patch

Clearing flags on attachment: 304940

Committed r214189: <http://trac.webkit.org/changeset/214189>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.