[HTML Import] Respect Content Security Policy Model

[HTML Import] Respect Content Security Policy Model

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22752 for the expected behavior.

This change teaches DocumentInit about Imports:
- The security context of the imported document should be master's context (frame).
- Each imported document should have its own CSP. It is enforced by the HTTP header
   which it is served with, for example.

This change also teaches CachedResourceLoader about Imports. That is,
imports should be loaded as if it is a script in terms of CSP.

This change doesn't cover scripting on imported documents since it is yet to be implemented.
We should test CSP cases when we implemented the scripting.

BUG=240592
TEST=http/tests/htmlimports/csp-*.html
R=abarth,dglazkov

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154924

[Hajime Morrita, 2013-07-22 11:22:51]

Hi Adam, could you take a look?
This is a preparation for https://codereview.chromium.org/19762002/
which makes scripts in HTML Imports running.

This change gives imported documents their own CSP object, which is a 
copy of the master document.

This will reduce the confusion of the  coming change
where we do juggling two (imported/importing) documents.

[Mike West, 2013-07-22 14:39:42]

A few drive-by comments:

https://codereview.chromium.org/19940002/diff/4001/LayoutTests/http/tests/htm...
File LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html (right):

https://codereview.chromium.org/19940002/diff/4001/LayoutTests/http/tests/htm...
LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html:12: if
(window.target.import.querySelector('#cors').import == null)
Forgive the noob question: do HTML Imports load synchronously? If not, you might
need to wait until the load/error event has fired before querying the import's
contents.

https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/Document.c...
Source/core/dom/Document.cpp:2648: void
Document::processHttpEquivContentSecurityPolicy(const String& equiv, const
String& content)
I like this cleanup, but it's unrelated to the core of this change. Would you
mind splitting this out into a separate CL?

https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
File Source/core/dom/DocumentInit.cpp (right):

https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
Source/core/dom/DocumentInit.cpp:57: return
frameForSecurityContext()->loader()->effectiveSandboxFlags();
It looks like this can now be null?

https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
Source/core/dom/DocumentInit.cpp:62: return
frameForSecurityContext()->settings();
Ditto.

https://codereview.chromium.org/19940002/diff/4001/Source/core/page/ContentSe...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/19940002/diff/4001/Source/core/page/ContentSe...
Source/core/page/ContentSecurityPolicy.cpp:1430:
ASSERT(!m_scriptExecutionContext->isDocument() ||
toDocument(m_scriptExecutionContext)->frame());
I don't understand this assertion. Could you explain the intent?

What about workers, for instance?

[Hajime Morrita, 2013-07-23 01:27:11]

Thanks for the view, Mike!

On 2013/07/22 14:39:42, Mike West wrote:
> A few drive-by comments:
> 
>
https://codereview.chromium.org/19940002/diff/4001/LayoutTests/http/tests/htm...
> File LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html
(right):
> 
>
https://codereview.chromium.org/19940002/diff/4001/LayoutTests/http/tests/htm...
> LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html:12: if
> (window.target.import.querySelector('#cors').import == null)
> Forgive the noob question: do HTML Imports load synchronously? If not, you
might
> need to wait until the load/error event has fired before querying the import's
> contents.

It is loaded synchronously like <script>.

> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/Document.c...
> Source/core/dom/Document.cpp:2648: void
> Document::processHttpEquivContentSecurityPolicy(const String& equiv, const
> String& content)
> I like this cleanup, but it's unrelated to the core of this change. Would you
> mind splitting this out into a separate CL?

Sure! Will do.

> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
> File Source/core/dom/DocumentInit.cpp (right):
> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
> Source/core/dom/DocumentInit.cpp:57: return
> frameForSecurityContext()->loader()->effectiveSandboxFlags();
> It looks like this can now be null?

No, caller returns earlier when the frame is null.

> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
> Source/core/dom/DocumentInit.cpp:62: return
> frameForSecurityContext()->settings();
> Ditto.
> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/page/ContentSe...
> File Source/core/page/ContentSecurityPolicy.cpp (right):
> 
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/page/ContentSe...
> Source/core/page/ContentSecurityPolicy.cpp:1430:
> ASSERT(!m_scriptExecutionContext->isDocument() ||
> toDocument(m_scriptExecutionContext)->frame());
> I don't understand this assertion. Could you explain the intent?
> 
> What about workers, for instance?

Ouch. It should've been clearer by adding explanation.
This change makes CSP object of imported documents read only.
That's why I added a guard on processHttpEquivContentSecurityPolicy().
This ASSERT ensures that immutability - Frame-less document like XHR-ed one and
imported documents
shouldn't be modified. The Frame-less notion is only about document so I skipped
worker case.
Workers should behave as before.

[Mike West, 2013-07-23 05:07:45]

On 2013/07/23 01:27:11, morrita1 wrote:
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
> > Source/core/dom/DocumentInit.cpp:57: return
> > frameForSecurityContext()->loader()->effectiveSandboxFlags();
> > It looks like this can now be null?
> 
> No, caller returns earlier when the frame is null.

Can you add an ASSERT to ensure that remains true?

>
https://codereview.chromium.org/19940002/diff/4001/Source/core/page/ContentSe...
> > Source/core/page/ContentSecurityPolicy.cpp:1430:
> > ASSERT(!m_scriptExecutionContext->isDocument() ||
> > toDocument(m_scriptExecutionContext)->frame());
> > I don't understand this assertion. Could you explain the intent?
> > 
> > What about workers, for instance?
> 
> Ouch. It should've been clearer by adding explanation.
> This change makes CSP object of imported documents read only.

Hrm. Ok.

What happens if the imported document contains a policy in a meta element? It
seems like these sorts of documents should be able to tighten their policies if
desired.

[Hajime Morrita, 2013-07-23 07:02:10]

On 2013/07/23 05:07:45, Mike West wrote:
> On 2013/07/23 01:27:11, morrita1 wrote:
> >
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentIn...
> > > Source/core/dom/DocumentInit.cpp:57: return
> > > frameForSecurityContext()->loader()->effectiveSandboxFlags();
> > > It looks like this can now be null?
> > 
> > No, caller returns earlier when the frame is null.
> 
> Can you add an ASSERT to ensure that remains true?
Sure, will do.

> 
> >
>
https://codereview.chromium.org/19940002/diff/4001/Source/core/page/ContentSe...
> > > Source/core/page/ContentSecurityPolicy.cpp:1430:
> > > ASSERT(!m_scriptExecutionContext->isDocument() ||
> > > toDocument(m_scriptExecutionContext)->frame());
> > > I don't understand this assertion. Could you explain the intent?
> > > 
> > > What about workers, for instance?
> > 
> > Ouch. It should've been clearer by adding explanation.
> > This change makes CSP object of imported documents read only.
> 
> Hrm. Ok.
> 
> What happens if the imported document contains a policy in a meta element? It
> seems like these sorts of documents should be able to tighten their policies
if
> desired.

They are simply ignored, regardless the directive comes from HTTP header or
<meta.
A test actually covers that case: csp-ignored-in-import.html

This is more about how HTML Imports should behave and it might better to have a
talk https://www.w3.org/Bugs/Public/show_bug.cgi?id=22752 

But anyway...
At first I thought that combining the directive both from master and the
imported document is
good idea to give some control to the imported document. 
However, I then felt that it makes CSP (or CSP extension of HTML Import) way
more complex:
We need the notion of combining, hierarchy, etc. to specify its behavior. 
What do you think?

In my opinion,  giving such an option to imported documents is overkill
considering that external <script>s don't have such kinds of controls. 

Note that scripts in HTML Imports run on the master document's context and
any imports don't have its own scripting context.
In that sense, HTML Imports are more like <script> than <frame>.

[Hajime Morrita, 2013-07-24 01:37:36]

How should I move this forward?

[dglazkov, 2013-07-24 02:53:23]

On 2013/07/24 01:37:36, morrita1 wrote:
> How should I move this forward?

send a cupcake to abarth? put "you're next" on it to make sure he understands
you're serious.

[Hajime Morrita, 2013-07-24 02:55:54]

On 2013/07/24 02:53:23, Dimitri Glazkov wrote:
> On 2013/07/24 01:37:36, morrita1 wrote:
> > How should I move this forward?
> 
> send a cupcake to abarth? put "you're next" on it to make sure he understands
> you're serious.

I should find good circulars to pick these letters then :-)

[Hajime Morrita, 2013-07-24 06:00:42]

Jokes aside, Adam, could you take a look when you have time?
As you know, this blocks script loading on import, which is one of the essential
feature of HTML Imports.

[Mike West, 2013-07-24 09:54:39]

Code change LGTM. For clarity, I'd appreciate you taking another pass at the
tests to use js-test-{pre,post}.js (see
LayoutTests/http/tests/security/xssAuditor/block-does-not-leak-location.html as
an example of how that might look in an HTML layout test).

It's probably worth Adam taking a closer look at the changes to Document* before
you land the patch, however.

On 2013/07/23 07:02:10, morrita1 wrote:
> They are simply ignored, regardless the directive comes from HTTP header or
> <meta.
> A test actually covers that case: csp-ignored-in-import.html

Right. I think the result of that test is incorrect. :)

> This is more about how HTML Imports should behave and it might better to have
a
> talk https://www.w3.org/Bugs/Public/show_bug.cgi?id=22752 

Sure. I'll hop over there.

> However, I then felt that it makes CSP (or CSP extension of HTML Import) way
> more complex:
> We need the notion of combining, hierarchy, etc. to specify its behavior. 
> What do you think?

CSP already has such a concept:
https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification....

> In my opinion,  giving such an option to imported documents is overkill
> considering that external <script>s don't have such kinds of controls. 

In my opinion, making document behave differently depending on how they're
loaded is confusing. I don't want to block this work on that issue, however.

For this patch, I'd appreciate you adding a console message noting that the CSP
in the imported document is being ignored. We can argue about whether or not it
should be ignored in a followup patch. :)

[Mike West, 2013-07-24 09:55:21]

Hrm. These should have come out with the last message.

https://codereview.chromium.org/19940002/diff/20001/LayoutTests/http/tests/ht...
File LayoutTests/http/tests/htmlimports/csp-ignored-in-import-expected.txt
(right):

https://codereview.chromium.org/19940002/diff/20001/LayoutTests/http/tests/ht...
LayoutTests/http/tests/htmlimports/csp-ignored-in-import-expected.txt:2: PASS
In general, tests would be clearer if you included js-test-pre.js and wrote this
particular result's test as "shouldBeNull(...)". You'll get significantly more
readable output in the test results that way (e.g. 'PASS: ... should have been
'null' and was." rather than "pass"). :)

https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/DocumentI...
File Source/core/dom/DocumentInit.h (right):

https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/DocumentI...
Source/core/dom/DocumentInit.h:62: 
Nit: Extra space here.

[abarth-chromium, 2013-07-24 18:29:55]

On 2013/07/24 06:00:42, morrita1 wrote:
> Jokes aside, Adam, could you take a look when you have time?

Sure.  I was holding off so that mkwst could give you comments.  /me looks now.

[abarth-chromium, 2013-07-24 18:39:19]

https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/Document....
Source/core/dom/Document.cpp:4215: // This can occur via
document.implementation.createDocument().
Does this occur for HTML import too?  If so, it would be good to explain that in
the comment.

https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/Document....
Source/core/dom/Document.cpp:4290:
contentSecurityPolicy()->copyStateFrom(import->master()->contentSecurityPolicy());
This looks great.  My only question is about this line of code.  Rather than
taking the CSP from the master document, we're probably better off if we take
the CSP from the Content-Security-Policy header that came with the imported
Document.

Consider: Most people who use CSP will disable inline script (because that's how
you get the XSS protection).  Even if you disable inline script in your main
document, there might not be a security risk in using inline script the HTML you
import.

I'm going to say LGTM to unblock you, but please change the CL so that the
imported document gets its CSP from its own HTTP headers rather than copying it
from the master document.

[abarth-chromium, 2013-07-24 18:41:29]

I don't think there's any reason to combine the policies from the master
document and the imported document.  The imported document can just use its own
policy, the same way it would if it was loaded in an iframe.

[Hajime Morrita, 2013-07-25 00:30:46]

Adam, Mike, thanks for the review and sharing your thoughts!

I find that I was slightly confused between several concepts. That is,

- A. CSP of the master should be able to block the imports being loaded.
- B. In import, some kind of CSP should be able to block
  - B1. Loading sub-imports
  - B2. Executing scripts.

A is obvious. For B, it makes sense to make import's own CSP policies (from both
HTTP headers and <meta>) effective
because if you want run external imports/scripts, you can and should block it in
the "A" way.
And this is what Adam is suggesting. 

I'll take this way. Will write the spec in this path respectively.

[Hajime Morrita, 2013-07-25 00:42:06]

> 
> CSP already has such a concept:
>
https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification....

This is great to know! I should learn CSP1.1 (vs.1.0) to correctly model the
Imports behaviour.

[abarth-chromium, 2013-07-25 01:08:45]

On 2013/07/25 00:30:46, morrita1 wrote:
> I find that I was slightly confused between several concepts. That is,
> 
> - A. CSP of the master should be able to block the imports being loaded.
> - B. In import, some kind of CSP should be able to block
>   - B1. Loading sub-imports
>   - B2. Executing scripts.
> 
> A is obvious. For B, it makes sense to make import's own CSP policies (from
both
> HTTP headers and <meta>) effective
> because if you want run external imports/scripts, you can and should block it
in
> the "A" way.
> And this is what Adam is suggesting. 
> 
> I'll take this way. Will write the spec in this path respectively.

Sounds good.

[Hajime Morrita, 2013-07-25 04:15:59]

> 
> Sounds good.

Okay, I updated the patch which does per-import CSP enforcement.
Could you take quick look, Adam?

In implementation, notable differences from the last change are:

- For loading sub-import, now import loader uses ResouceFetcher of the import
itself,
   instead of one from the master Document.
   To make this work, I added some guards in ResourceFeathcer
   so that it works even with Documents which don't have their own
DocumentLoaders.

- Extracting CSP-related header handling from FrameLoader to
ContentSecurityPolicy.
   This helps Imports loader to share the logic.

[abarth-chromium, 2013-07-25 04:53:09]

LGTM

https://codereview.chromium.org/19940002/diff/37001/Source/core/dom/DocumentI...
File Source/core/dom/DocumentInit.h (right):

https://codereview.chromium.org/19940002/diff/37001/Source/core/dom/DocumentI...
Source/core/dom/DocumentInit.h:63: 
Looks like you've got an extra blank line here.

https://codereview.chromium.org/19940002/diff/37001/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImportsController.cpp (right):

https://codereview.chromium.org/19940002/diff/37001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImportsController.cpp:94:
CachedResourceHandle<CachedRawResource> resource =
m_owner->document()->fetcher()->requestImport(request);
requestImport  ->  fetchImport ?  :)

Not worth renaming in this CL, but an idea for your loader renaming project.

[Hajime Morrita, 2013-07-25 05:48:33]

Adam, thanks for the instant review!

On 2013/07/25 04:53:09, abarth wrote:
> LGTM
> 
>
https://codereview.chromium.org/19940002/diff/37001/Source/core/dom/DocumentI...
> File Source/core/dom/DocumentInit.h (right):
> 
>
https://codereview.chromium.org/19940002/diff/37001/Source/core/dom/DocumentI...
> Source/core/dom/DocumentInit.h:63: 
> Looks like you've got an extra blank line here.
> 
>
https://codereview.chromium.org/19940002/diff/37001/Source/core/html/HTMLImpo...
> File Source/core/html/HTMLImportsController.cpp (right):
> 
>
https://codereview.chromium.org/19940002/diff/37001/Source/core/html/HTMLImpo...
> Source/core/html/HTMLImportsController.cpp:94:
> CachedResourceHandle<CachedRawResource> resource =
> m_owner->document()->fetcher()->requestImport(request);
> requestImport  ->  fetchImport ?  :)
> 
> Not worth renaming in this CL, but an idea for your loader renaming project.
Sure. it should be consistent.

[commit-bot: I haz the power, 2013-07-25 07:47:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/19940002/44001

[commit-bot: I haz the power, 2013-07-25 08:07:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-25 08:19:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/19940002/57001

[commit-bot: I haz the power, 2013-07-25 08:37:51]

Failed to trigger a try job on mac_layout_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-25 08:38:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/19940002/55002

[commit-bot: I haz the power, 2013-07-25 18:42:16]

Change committed as 154924