Bug 75043

Summary:

XSLT-created HTML documents do not inherit content-security-policy from originally loaded XML.

Product:

WebKit

Reporter:

Thomas Sepez <tsepez>

Component:

XML

Assignee:

Thomas Sepez <tsepez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch + tests	abarth: review+
Patch + tests	abarth: review+, abarth: commit-queue-
Patch + tests	none

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+	Details Formatted Diff Diff
Patch + tests (6.10 KB, patch) 2011-12-22 12:41 PST, Thomas Sepez	abarth: review+ abarth: commit-queue-	Details Formatted Diff Diff
Patch + tests (6.01 KB, patch) 2011-12-22 14:09 PST, Thomas Sepez	no flags	Details Formatted Diff Diff
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.