RESOLVED FIXED 75043
XSLT-created HTML documents do not inherit content-security-policy from originally loaded XML. 
https://bugs.webkit.org/show_bug.cgi?id=75043
Summary XSLT-created HTML documents do not inherit content-security-policy from origi...
Thomas Sepez
Reported 2011-12-21 14:45:10 PST
Will be uploading test cases presently that show the problem.
Attachments
Patch + tests (6.07 KB, patch)
2011-12-22 12:12 PST, Thomas Sepez 		abarth: review+
DetailsFormatted DiffDiff
Patch + tests (6.10 KB, patch)
2011-12-22 12:41 PST, Thomas Sepez 		abarth: review+
abarth: commit-queue-
DetailsFormatted DiffDiff
Patch + tests (6.01 KB, patch)
2011-12-22 14:09 PST, Thomas Sepez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Thomas Sepez
Comment 1 2011-12-21 15:14:04 PST
xsl-img-blocked.php: <?php header("Content-Type: text/xml"); header("X-WebKit-CSP: img-src 'none'"); echo '<?xml version="1.0" encoding="UTF-8"?>'; echo '<?xml-stylesheet type="text/xsl" href="resources/transform-to-img.xsl"?>'; ?> <body/> resources/transform-to-img.xsl: <?xml version="1.0" encoding="ISO-8859-1"?> <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="/"> <html> <head> <script> //<![CDATA[ if (window.layoutTestController) layoutTestController.dumpAsText(); //]]> </script> </head> <body> Here is an image: <img src="../resources/abe.png"/> </body> </html> </xsl:template> </xsl:stylesheet>
Thomas Sepez
Comment 2 2011-12-22 12:12:36 PST
Created attachment 120360 [details] Patch + tests
Adam Barth
Comment 3 2011-12-22 12:14:45 PST
Comment on attachment 120360 [details] Patch + tests View in context: https://bugs.webkit.org/attachment.cgi?id=120360&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:498 > +{ Should we ASSERT(!m_havePolicy) ?
Thomas Sepez
Comment 4 2011-12-22 12:19:39 PST
Comment on attachment 120360 [details] Patch + tests View in context: https://bugs.webkit.org/attachment.cgi?id=120360&action=review >> Source/WebCore/page/ContentSecurityPolicy.cpp:498 >> +{ > > Should we ASSERT(!m_havePolicy) ? Don't think so -- didReceiveHeader silently ignores if set -- follow same first header rules rule here.
Adam Barth
Comment 5 2011-12-22 12:21:39 PST
So, the copy operation can fail if we already have a policy? If so, that seems like "copy" is the wrong verb to use for this function. We should either use a name that expresses that this operation can fail or we should make the operation always succeed.
Adam Barth
Comment 6 2011-12-22 12:23:13 PST
A third option is to make this function illegal to call in any situation in which it would fail (i.e., add the ASSERT).
Thomas Sepez
Comment 7 2011-12-22 12:41:05 PST
Created attachment 120365 [details] Patch + tests You've convinced me.
Adam Barth
Comment 8 2011-12-22 12:50:43 PST
Are those null checks needed?
Thomas Sepez
Comment 9 2011-12-22 12:57:05 PST
(In reply to comment #8) > Are those null checks needed? I put them in because SecurityContext's constructor creates SecurityContexts with NULL CSP refpointers. Didn't want to rely on the flow up to this point always setting them for now and down the road.
Adam Barth
Comment 10 2011-12-22 13:46:06 PST
Comment on attachment 120365 [details] Patch + tests View in context: https://bugs.webkit.org/attachment.cgi?id=120365&action=review > Source/WebCore/xml/XSLTProcessor.cpp:95 > + if (result->contentSecurityPolicy() && oldDocument->contentSecurityPolicy()) In the case where result->contentSecurityPolicy() is null, won't we fail to copy over the security policy? Isn't that bad? It seems like we should remove these null checks. If initSecurityOrigin hasn't finished by this point, we're in big trouble and we want to crash so we'll know that.
Thomas Sepez
Comment 11 2011-12-22 14:09:55 PST
Created attachment 120378 [details] Patch + tests Fair enough. I suppose in a perfect world SecurityContext::contentSecurityPoliy() should ASSERT non-nullness of its return to allow callers to omit these kinds of checks, and that setContentSecurityPolicy() would ASSERT its arg is non-NULL, no?
Adam Barth
Comment 12 2011-12-22 14:12:39 PST
Comment on attachment 120378 [details] Patch + tests Probably. If you'd like to add those ASSERTs, I'd be happy to review the patch. :)
WebKit Review Bot
Comment 13 2011-12-23 02:40:43 PST
Comment on attachment 120378 [details] Patch + tests Clearing flags on attachment: 120378 Committed r103617: <http://trac.webkit.org/changeset/103617>
WebKit Review Bot
Comment 14 2011-12-23 02:40:49 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.