Bug 169868

Summary: [QuickLook] Subresources should be in the same origin as the main document
Product: WebKit Reporter: Andy Estes <aestes>
Component: Page LoadingAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, koivisto, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
dbates: review+
Patch none

Description Andy Estes 2017-03-19 16:33:40 PDT
[QuickLook] Subresources should be in the same origin as the main document
Comment 1 Andy Estes 2017-03-19 16:54:45 PDT
rdar://problem/29898214
Comment 2 Andy Estes 2017-03-19 16:56:21 PDT
Created attachment 304906 [details]
Patch
Comment 3 Andy Estes 2017-03-19 17:14:10 PDT
Created attachment 304907 [details]
Patch
Comment 4 Build Bot 2017-03-19 19:03:50 PDT
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
Comment 5 Build Bot 2017-03-19 19:03:55 PDT
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
Comment 6 Andy Estes 2017-03-20 10:31:25 PDT
Created attachment 304930 [details]
Patch
Comment 7 Daniel Bates 2017-03-20 11:43:36 PDT
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.
Comment 8 Andy Estes 2017-03-20 11:47:21 PDT
(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.
Comment 9 Daniel Bates 2017-03-20 11:55:32 PDT
(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()?
Comment 10 Andy Estes 2017-03-20 12:08:11 PDT
(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.
Comment 11 Andy Estes 2017-03-20 12:09:01 PDT
Created attachment 304940 [details]
Patch
Comment 12 WebKit Commit Bot 2017-03-20 12:50:13 PDT
Comment on attachment 304940 [details]
Patch

Clearing flags on attachment: 304940

Committed r214189: <http://trac.webkit.org/changeset/214189>
Comment 13 WebKit Commit Bot 2017-03-20 12:50:21 PDT
All reviewed patches have been landed.  Closing bug.