Closed Bug 1301123 Opened 6 years ago Closed 6 years ago

Add an assertion ensuring that documents and friends can never end up with an expanded principal


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox52 --- fixed


(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)




(1 file)

(Obviously after the existing cases where this can happen have been fixed.)

Boris, I'm currently thinking of adding the assertion to nsNodeInfoManager::SetDocumentPrincipal().  Can you think of other places where we'd want to assert this as well?
Flags: needinfo?(bzbarsky)
That one location should gate all the document stuff.

I guess you could add something to where a window creates its global JSObject, to catch cases in which a window has a principal that doesn't match its document for some reason.
Flags: needinfo?(bzbarsky)
Comment on attachment 8795884 [details] [diff] [review]
Add assertions ensuring that documents and inner windows cannot end up with an expanded principal

Review of attachment 8795884 [details] [diff] [review]:

r=me with the window stuff removed. If there's a reason for it that I'm missing let's discuss further.

::: dom/base/nsGlobalWindow.cpp
@@ +2664,5 @@
>      bool sameOrigin = false;
>      nsIPrincipal *existing =
>        nsJSPrincipals::get(JS_GetCompartmentPrincipals(compartment));
> +    MOZ_DIAGNOSTIC_ASSERT(!nsContentUtils::IsExpandedPrincipal(existing),
> +                          "The inner window shouldn't have an expanded principal");

This doesn't seem right - we're in an #ifdef DEBUG (so the telemetry will never accumulate), and it seems a bit odd to me to check the _old_ principal in the inner window reuse case (not that it's wrong, but I just don't understand why we're checking it specifically).

I'd think the check to make sure NodePrincipals are never expanded should be sufficient.

::: toolkit/components/telemetry/Histograms.json
@@ +10497,5 @@
> +    "expires_in_version": "58",
> +    "kind": "count",
> +    "description": "Number of documents encountered using an expanded principal."
> +  },

Again, it seems like this is subsumed by the document case and should be removed.
Attachment #8795884 - Flags: review?(bobbyholley) → review+
I added the window code based on comment 1.

Boris, are you OK with only a check in nsNodeInfoManager::SetDocumentPrincipal()?
Flags: needinfo?(bzbarsky)
I'm probably fine with that, yes.
Flags: needinfo?(bzbarsky)
Pushed by
Add assertions ensuring that documents and inner windows cannot end up with an expanded principal; r=bholley
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1307730
Depends on: 1307730
See Also: 1307730
Depends on: 1397371
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.