Created attachment 309921 [details] zip with screenshots Our site, https://online.swedbank.se is using CSP to ensure that a customers online banking session is as secured as possible. The CSP contains among other object-src 'none'; to ensure that no plugins are allowed to be executed under our domain as we currently don't have that need. But with the latest Safari (Technology Preview have the same problem) we have gotten reports from our customer that the internal PDFPlugin in Safari gets blocked in the cases we transport a customer to a separate domain. That domain don't have any CSP policy defined by itself. Also when we open a PDF, in this case a electronic invoice that is hosted by a 3de party provider, we ensure to open a new tab so the customer can view the PDF in that tab and continue the online banking presence in the original tab. Lastest Chrome, FF, Edge and IE (well not much of CSP there) opens the tab and allow the display of the PDF without the CSP policy tries to stop it. But of course this might be a different in how to interpret the CSP spec. If a domain opens a new tab and that points to another domain that serves a content the requires a plugin, should that domains CSP come into play here? Of course our site is driving traffic to the other domain but the PDF isn't served from our domain. The solution is of course to specify all domains that we load PDF from. This would most likely solve the problem. But would be good if we had one common view on how this should be handled over the different browsers. I have tried to add a couple of screenshots as well as some proxy logs on the actual traffic that is going on. 1.png -> the view of the customer before clicking to view the e-invoice 2.png -> the view of the customer after clicked the view link and a new tab has been opened 3.png -> network traffic one the POST to the external domain e-faktura1.stralfors.net which response with a 302 and redirect the traffic 4.png -> Request to the redirect to /invoiceViewer/showInvoiceSE.htm. Gives 200 5.png -> Response from the request, contains the PDF document But the end results is that the CSP served from online.swedbank.se kicks in and stops the initiation of the PDFPlugin in Safari.
Thanks for reporting, Markus! Dan, what's your take?
Created attachment 309951 [details] Test case
(In reply to John Wilander from comment #1) > Thanks for reporting, Markus! > > Dan, what's your take? This bug was caused by <http://trac.webkit.org/changeset/209608> (bug #15531). We should only have a plugin document loaded in a subframe inherit the CSP policy of its parent frame regardless of whether it inherits the security origin of its parent. This will allow a cross-origin plugin document loaded in a child window to be allowed to load regardless of the CSP policy of its opener. Additional remarks: We treat a plugin documents (direct navigation to a document that requires a plugin) as special case. In particular, a plugin document loaded in a subframe (e.g. <object>) will inherit the CSP policy of its parent page regardless of whether the document inherits the security origin of its parent page. (Bug #153160 is about changing this behavior). Following the patch for bug #15531 we apply the same inheritance policy to a plugin document loaded in a child window.
(In reply to Daniel Bates from comment #3) > (In reply to John Wilander from comment #1) > > Thanks for reporting, Markus! > > > > Dan, what's your take? > > This bug was caused by <http://trac.webkit.org/changeset/209608> (bug > #15531). err, bug #165531
Created attachment 310097 [details] Patch and layout tests
Comment on attachment 310097 [details] Patch and layout tests Attachment 310097 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3740468 New failing tests: http/tests/security/contentSecurityPolicy/same-origin-plugin-document-blocked-in-child-window.html http/tests/security/contentSecurityPolicy/plugin-blocked-in-about-blank-window.html http/tests/security/contentSecurityPolicy/cross-origin-plugin-document-allowed-in-child-window.html
Created attachment 310099 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #6) > Comment on attachment 310097 [details] > Patch and layout tests > > Attachment 310097 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/3740468 > > New failing tests: > http/tests/security/contentSecurityPolicy/same-origin-plugin-document- > blocked-in-child-window.html > http/tests/security/contentSecurityPolicy/plugin-blocked-in-about-blank- > window.html > http/tests/security/contentSecurityPolicy/cross-origin-plugin-document- > allowed-in-child-window.html Will skip these tests on iOS as iOS does not support plugins.
Created attachment 310101 [details] Patch and layout tests
Comment on attachment 310101 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=310101&action=review > Source/WebCore/dom/Document.cpp:5221 > + if (!shouldInheritSecurityOriginFromOwner(m_url) && (!isPluginDocument() || (!parentFrame && !openerFrame->document()->securityOrigin().canAccess(securityOrigin())))) This if statement is hard to parse. I think it'd read better with the plug-in document parts broken out into a function or a local variable with a descriptive name (e.g. shouldInheritSecurityOriginForPlugInDocument()).
<rdar://problem/32258262>
Comment on attachment 310101 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=310101&action=review >> Source/WebCore/dom/Document.cpp:5221 >> + if (!shouldInheritSecurityOriginFromOwner(m_url) && (!isPluginDocument() || (!parentFrame && !openerFrame->document()->securityOrigin().canAccess(securityOrigin())))) > > This if statement is hard to parse. I think it'd read better with the plug-in document parts broken out into a function or a local variable with a descriptive name (e.g. shouldInheritSecurityOriginForPlugInDocument()). Will extract logic into a member function called shouldInheritContentSecurityPolicyFromOwner(): bool Document::shouldInheritContentSecurityPolicyFromOwner() const { ASSERT(m_frame); if (shouldInheritSecurityOriginFromOwner(m_url)) return true; if (!isPluginDocument()) return true; Frame* parentFrame = m_frame->tree().parent(); Frame* openerFrame = m_frame->loader().opener(); if (!parentFrame && !openerFrame) return false; if (parentFrame) return true; return !openerFrame->document()->securityOrigin().canAccess(securityOrigin()); } Then will update the code in Document::initContentSecurityPolicy() to read: void Document::initContentSecurityPolicy() { // 1. Inherit Upgrade Insecure Requests Frame* parentFrame = m_frame->tree().parent(); if (parentFrame) contentSecurityPolicy()->copyUpgradeInsecureRequestStateFrom(*parentFrame->document()->contentSecurityPolicy()); // 2. Inherit Content Security Policy if (!shouldInheritContentSecurityPolicyFromOwner()) return; Frame* ownerFrame = parentFrame; if (!ownerFrame) ownerFrame = m_frame->loader().opener(); contentSecurityPolicy()->copyStateFrom(ownerFrame->document()->contentSecurityPolicy()); // Does not copy Upgrade Insecure Requests state. }
(In reply to Daniel Bates from comment #12) > Will extract logic into a member function called > shouldInheritContentSecurityPolicyFromOwner(): > > bool Document::shouldInheritContentSecurityPolicyFromOwner() const > { > ASSERT(m_frame); > if (shouldInheritSecurityOriginFromOwner(m_url)) > return true; > if (!isPluginDocument()) > return true; err, this should return false in this case.
(In reply to Daniel Bates from comment #12) > Will extract logic into a member function called > shouldInheritContentSecurityPolicyFromOwner(): > > bool Document::shouldInheritContentSecurityPolicyFromOwner() const > { > ASSERT(m_frame); > if (shouldInheritSecurityOriginFromOwner(m_url)) > return true; > if (!isPluginDocument()) > return true; > Frame* parentFrame = m_frame->tree().parent(); > Frame* openerFrame = m_frame->loader().opener(); > if (!parentFrame && !openerFrame) > return false; > if (parentFrame) > return true; > return > !openerFrame->document()->securityOrigin().canAccess(securityOrigin()); err, this should read: openerFrame->document()->securityOrigin().canAccess(securityOrigin())
Created attachment 310456 [details] Patch and layout tests Updated patch based on feedback from Andy Estes.
Comment on attachment 310456 [details] Patch and layout tests Attachment 310456 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3765538 Number of test failures exceeded the failure limit.
Created attachment 310464 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310456 [details] Patch and layout tests Attachment 310456 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3765537 Number of test failures exceeded the failure limit.
Created attachment 310465 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 310456 [details] Patch and layout tests Attachment 310456 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3765506 Number of test failures exceeded the failure limit.
Created attachment 310467 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310456 [details] Patch and layout tests Attachment 310456 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3765563 Number of test failures exceeded the failure limit.
Created attachment 310469 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 310481 [details] Patch and layout tests
Comment on attachment 310481 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=310481&action=review > Source/WebCore/dom/Document.cpp:5216 > + Frame* parentFrame = m_frame->tree().parent(); > + if (parentFrame) You don't need the parentFrame local variable since you don't use if after the null check.
(In reply to Andy Estes from comment #25) > Comment on attachment 310481 [details] > Patch and layout tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310481&action=review > > > Source/WebCore/dom/Document.cpp:5216 > > + Frame* parentFrame = m_frame->tree().parent(); > > + if (parentFrame) > > You don't need the parentFrame local variable since you don't use if after > the null check. ... since you don't use *it* ...
(In reply to Andy Estes from comment #25) > Comment on attachment 310481 [details] > Patch and layout tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310481&action=review > > > Source/WebCore/dom/Document.cpp:5216 > > + Frame* parentFrame = m_frame->tree().parent(); > > + if (parentFrame) > > You don't need the parentFrame local variable since you don't use if after > the null check. Will inline the value of parentFrame into the if condition before landing.
Committed r217054: <http://trac.webkit.org/changeset/217054>