Bug 75043 - XSLT-created HTML documents do not inherit content-security-policy from originally loaded XML.
Summary: XSLT-created HTML documents do not inherit content-security-policy from origi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thomas Sepez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-21 14:45 PST by Thomas Sepez
Modified: 2011-12-23 02:40 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Sepez 2011-12-21 14:45:10 PST
Will be uploading test cases presently that show the problem.
Comment 1 Thomas Sepez 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>
Comment 2 Thomas Sepez 2011-12-22 12:12:36 PST
Created attachment 120360 [details]
Patch + tests
Comment 3 Adam Barth 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) ?
Comment 4 Thomas Sepez 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.
Comment 5 Adam Barth 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.
Comment 6 Adam Barth 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).
Comment 7 Thomas Sepez 2011-12-22 12:41:05 PST
Created attachment 120365 [details]
Patch + tests

You've convinced me.
Comment 8 Adam Barth 2011-12-22 12:50:43 PST
Are those null checks needed?
Comment 9 Thomas Sepez 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.
Comment 10 Adam Barth 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.
Comment 11 Thomas Sepez 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?
Comment 12 Adam Barth 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.  :)
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2011-12-23 02:40:49 PST
All reviewed patches have been landed.  Closing bug.