859957 - Simplify [[DefaultValue]] security wrapping setup

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Now that we have SecurityWrapper, our situation for the [[DefaultValue]] trap can be simplified a lot.

Given that we chemspilled on this code in October, I'm going to get several reviews here. :-)

Comment 1

[Bobby Holley (:bholley)]

Attachment: Simplify [[DefaultValue]] security wrapping setup. v1

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a86429901fbf

Comment 3

[Gabor Krizsanits (INACTIVE)]

Before I r+ I must ask this since I don't remember the history of this chemspill. Are you sure we have tests for all the cases that is mentioned in the removed comment in and around Wrapper::defaultValue? (A assume you are just want to double check it since I see no tests here)

So if I get it correctly in case of same compartment transparent wrapper the extra AutoCompartment call(cx, wrappedObject(wrapper)); that we stop doing post-patch is not doing anything interesting anyway, right?

There is one thing that might be a good idea, since most of the explanation comment is gone, a warning comment at setSafeToUnwrap that if this flag is set to false on a wrapper class that is not derived from SecurityWrapper, one must keep an eye open for defaultValue implementation. (pre-patch this was handled at wrapper level, post-patch it is not)

Comment 4

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> Before I r+ I must ask this since I don't remember the history of this
> chemspill. Are you sure we have tests for all the cases that is mentioned in
> the removed comment in and around Wrapper::defaultValue? (A assume you are
> just want to double check it since I see no tests here)

I sure hope so. We've got test_bug802557.html, test_bug720619.html, and test_sameOriginPolicy.html, as well as some others. Hopefully that's enough.

> So if I get it correctly in case of same compartment transparent wrapper the
> extra AutoCompartment call(cx, wrappedObject(wrapper)); that we stop doing
> post-patch is not doing anything interesting anyway, right?

In the case of a same-compartment transparent wrapper, the AutoCompartment was a no-op. In the case of CrossCompartmentWrapper, it now happens in PIERCE.
 
> There is one thing that might be a good idea, since most of the explanation
> comment is gone, a warning comment at setSafeToUnwrap that if this flag is
> set to false on a wrapper class that is not derived from SecurityWrapper,
> one must keep an eye open for defaultValue implementation. (pre-patch this
> was handled at wrapper level, post-patch it is not)

Well, we depend on the fact that SecurityWrappers have !isSafeToUnwrap() for pretty much everything these days (that's the basis of our security wrapper computation in WrapperFactory::Rewrap). safeToUnwrap basically means "the wrapper is not hiding anything from a security perspective". DefaultValue is only one of many things that are affected by this, so I don't think it makes sense to talk about it explicitly here. The fact that it was previously in Wrapper was purely an implementation / historical quirk.

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/270ad24960d3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/270ad24960d3