1491342 - Figure out what to do with the AccessCheck::subsumesConsideringDomain calls in ShouldWaiveXray

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

In ShouldWaiveXray we do two subsumesConsideringDomain(IgnoringFPD) calls to check for same-origin compartments:

https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/js/xpconnect/wrappers/WrapperFactory.cpp#148-156

JS_GetCompartmentPrincipals is deprecated so what should we do here instead? We can't use the origin stored in CompartmentPrivate because that doesn't consider the document.domain case. I'm afraid I don't know enough about Xrays and Xray Waivers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe that we should not have waivers in any cases that involve document.domain.  That is, preserving waivers if and only if both compartments are same-origin, ignoring document.domain, is the right thing here.

Comment 2

[Bobby Holley (:bholley)]

Yeah, I don't think we need to consider domain here. We just do that because all the checks in the Xray code consider it for consistency.

That said, I don't see how this case is different from the one in WrapperFactory::Rewrap, where we _do_ need to consider the domain. There's probably a detail I've forgotten that explains this, but I'd appreciate help remembering. :-)

Comment 3

[Jan de Mooij [:jandem]]

Great, thanks!

(In reply to Bobby Holley (:bholley) from comment #2)
> That said, I don't see how this case is different from the one in
> WrapperFactory::Rewrap, where we _do_ need to consider the domain. There's
> probably a detail I've forgotten that explains this, but I'd appreciate help
> remembering. :-)

I was thinking that code will change once we do WindowProxy/Location filtering to just hand out either opaque or transparent wrappers based on the {site, had-domain-change} data on both compartments?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Great, thanks!
> 
> (In reply to Bobby Holley (:bholley) from comment #2)
> > That said, I don't see how this case is different from the one in
> > WrapperFactory::Rewrap, where we _do_ need to consider the domain. There's
> > probably a detail I've forgotten that explains this, but I'd appreciate help
> > remembering. :-)
> 
> I was thinking that code will change once we do WindowProxy/Location
> filtering to just hand out either opaque or transparent wrappers based on
> the {site, had-domain-change} data on both compartments?

Ah, right.

Though it's more complicated than just opaque-versus-transparent. What we'd do would be to replace the subsumesConsideringDomain with subsumes, and then have a special-case at the top where we see if both compartments have had d.d set _and_ the Sites are the same. In that case we'd hand out a transparent wrapper, otherwise we'd continue with the rest of the logic.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1491342 - Ignore document.domain in ShouldWaiveXray. r?bholley

We want to get rid of JS_GetCompartmentPrincipals. The origin stored in CompartmentPrivate does not account for document.domain changes because that's a per-realm thing.

Fortunately we should not have waivers in any cases that involve document.domain.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 9009611 [details]
Bug 1491342 - Ignore document.domain in ShouldWaiveXray. r?bholley

Bobby Holley (:bholley) has approved the revision.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/316a48e82d51
Ignore document.domain in ShouldWaiveXray. r=bholley

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/316a48e82d51