1547271 - Write Test for CSP serialization within Expanded Principals

Status: NEW

(Core :: DOM: Security, task, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 965637 we are going to move the CSP from the Principal into the Client with the exception of Expanded Principals. We should write a test for CSP serialization within Expanded Principals to ensure we are not regressing that after Bug 965637.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1547271_serialize_csp_expandedprincipal.patch

Andrew, Jonathan, as discussed within Bug 965637 we want to move the CSP from the Principal into the client, except for ExpandedPrincipals.

When Andrew pointed out that we will stop serializing the CSP within the Principal because we updated the CSP serialization parts within ContentPrincipal::Read and ContentPrincipal::Write [1] I thought there is bug. In fact we do not stop serializing the CSP for ExpandedPrincipals, because we have never done that in the first place because ExpandedPrincipal holds it's own ::Read() and ::Write() (see attached patch).

I wrote a test (see attachment) and updated the code which would make CSP serialization within ExpandedPrincipals work. The way I see it, we have two options:

1. We start serializing the CSP within ExpandedPrincipals now and also land the attached code updates and tests, or
2. We change the bug dependency and rather have Bug 965637 blocking this one.

I am rather for option (2) since we have never serialized CSPs for Expanded Principals. I also think it's less error prone if we fix this bug after Bug 965637, because if that serialization of CSPs within ExpandedPrincipals starts to causing problems now then we have to drag that problem around exactly at the time we are dealing with Bug 965637.

What do you think - do you agree with me and move forward with option (2)?

[1] https://phabricator.services.mozilla.com/D27654

Comment 2

[Jonathan Kingston [:jkt] he/him]

I would go for 2. This has only been an issue since we started being a little stricter around requiring principals for session store leading to the expanded principal implementation being added in Bug 1452666. Prior to all of this we were using System Principal for this. My understanding is that we won't upgrade insecure resources but I suspect we were also not back then as the System Principal doesn't have the CSP either.

Is there any other implications on navigations caused by extensions that would need to be session restored?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonathan Kingston [:jkt] from comment #2)

I would go for 2. This has only been an issue since we started being a little stricter around requiring principals for session store leading to the expanded principal implementation being added in Bug 1452666. Prior to all of this we were using System Principal for this.

Indeed, which in turn also favors option 2 - no need to start fixing CSP on ExpandedPrincipals now - we have been living with that behavior for a long time.

Is there any other implications on navigations caused by extensions that would need to be session restored?

I can't think of any other implications regarding extensions.

Comment 4

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Ah, good point. I remember looking around for whether it called the parent serialization method, but I guess I lost that train of thought. Option 2 does seem better.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

OK, as agreed, we move forward with option (2) - changing the dependency hierarchy so this bug actually depends on Bug 965637