Closed Bug 994872 Opened 6 years ago Closed 6 years ago

CSP in C++: Remove documentPrincipal from SetRequestContext

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ckerschb, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 925004
Assignee: nobody → sstamm
Blocks: CSP
No longer blocks: 925004
Depends on: 994782
(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...
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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?
Attached patch remove-documentPrincipal (obsolete) — Splinter Review
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
Summary: CSP in CPP: Remove documentPrincipal from SetRequestContext → CSP in C++: Remove documentPrincipal from SetRequestContext
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.
Depends on: 1038756
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
Attachment #8431956 - Attachment is obsolete: true
Attachment #8462908 - Flags: review?(mozilla)
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.
Attachment #8462908 - Flags: review?(mozilla) → review+
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).
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/e16508cdfcf0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.