994872 - CSP in C++: Remove documentPrincipal from SetRequestContext

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

The new implementation uses the originalURI for security checks instead of the principal. There is no need for the principal to be stored anymore - remove it.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> The new implementation uses the originalURI for security checks instead of
> the principal.

Is there more written up about this change somewhere? Without any context, that sounds like the wrong direction to be going in...

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #1)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> > The new implementation uses the originalURI for security checks instead of
> > the principal.
> 
> Is there more written up about this change somewhere? Without any context,
> that sounds like the wrong direction to be going in...

The reason why we are doing this, is because we had a circular reference problem, between the nsIPrincipal and the cspContext. (See bug 994466).

Instead of calling NS_CheckContentLoadPolicy we call ShouldLoad directly which allows us to skip the principal and use the originalURI directly. We had a patch that drops the principal in nsCSPContext when calling the destructor of nsDocument. It seems that using the originalURI is the safer strategy than dropping the principal in the destructor. Don't you agree?

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-documentPrincipal

Initial patch.  This depends on test changes and of course removal of the old backend (contentSecurityPolicy.js).

Also here: http://hg.mozilla.org/users/sstamm_mozilla.com/csp-rewrite-patches/file/f33fd4b7d84e/remove-documentPrincipal

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

In bug 1038756, we change the way how channels are created, where the channel either needs a principal or a node, otherwise NS_newChannel will fail - I think this bug will become a wontfix. I will check and follow up.

Referencing bug 1038756, because it's closely tied together with this bug.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-documentPrincipal

Also, pushed this along with bug 994782, bug 991468 and bug 991474 to try to see how they do:
https://tbpl.mozilla.org/?tree=Try&rev=23c1af15cb65

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8462908 [details] [diff] [review]
remove-documentPrincipal

Review of attachment 8462908 [details] [diff] [review]:
-----------------------------------------------------------------

As a follow up regarding Comment 4: Since we are setting the requestingContext in the LoadInfo on the channel, we can query that information from the Loadinfo handed to SetReqeustContext (Needed for bug 1038756). We can safely delete the documentPrincipal.

Let's roll this out and delete the documentPrincipal.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

flagging dev-doc-needed since this is a change for the IDL.  We should probably update the dev-docs for the other bugs that change the API and land the same time as this (see comment 5).

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e16508cdfcf0

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e16508cdfcf0