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

Status: RESOLVED FIXED

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

Description

[(no longer active)]

(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?

Comment 1

[Boris Zbarsky [: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.

Comment 2

[(no longer active)]

Attachment: Add assertions ensuring that documents and inner windows cannot end up with an expanded principal

Comment 3

[Bobby Holley (:bholley)]

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."
> +  },
> +  "WINDOW_WITH_EXPANDED_PRINCIPAL": {

Again, it seems like this is subsumed by the document case and should be removed.

Comment 4

[(no longer active)]

I added the window code based on comment 1.

Boris, are you OK with only a check in nsNodeInfoManager::SetDocumentPrincipal()?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm probably fine with that, yes.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a20b9fe4f9
Add assertions ensuring that documents and inner windows cannot end up with an expanded principal; r=bholley

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93a20b9fe4f9