1335654 - Fix the exceptions we throw for our cross-origin objects to actually mostly follow the spec

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Got tired of this.  The current exceptions are thrown in general JS Wrapper code, so can't be the DOMExceptions we want.  We just need to throw on the XPConnect side instead.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Propagate AutoEnterPolicy's mayThrow argument to the enter() methods of proxy handlers, so they know whether it's OK to throw some sort of custom exception or whether they should just silently deny

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change the deny() methods of our wrapper security policies to take a JSContext and a mayThrow boolean

Comment 3

[Boris Zbarsky [:bzbarsky]]

More parts (that actually change behavior) coming once I finish the try run.

I do have one unfixed bit left: the exception thrown by [[SetPrototype]].  In our code, we land in XrayWrapper<Base, Traits>::setPrototype, Base::hasSecurityPolicy() is true, so we call SecurityWrapper<Base>::setPrototype which happily throws a non-TypeError for us.  Now I could change that to throw a TypeError.  Or I could try to take over the throwing myself based on the traits or something.  But I don't understand this setPrototype setup well enough to decide which is better....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Implement actually throwing a SecurityError when cross-origin property accesses are denied

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Fix some CrossOriginXrayWrapper error reporting bits to follow the HTML spec for cross-origin objects

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8832357 [details] [diff] [review]
part 3.  Implement actually throwing a SecurityError when cross-origin property accesses are denied

Review of attachment 8832357 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +295,5 @@
> +        // matches what AutoEnterPolicy::reportErrorIfExceptionIsNotPending
> +        // does.
> +        JS::RootedValue idVal(cx, js::IdToValue(id));
> +        nsAutoJSString propName;
> +        if (!propName.init(cx, JS_ValueToSource(cx, idVal))) {

It doesn't seem entirely obvious to be that JS_ValueToSource can't return null

Comment 7

[Boris Zbarsky [:bzbarsky]]

You are absolutely right; it can OOM.  I meantto have null-checks there, then forgot to...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Implement actually throwing a SecurityError when cross-origin property accesses are denied

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Fix some CrossOriginXrayWrapper error reporting bits to follow the HTML spec for cross-origin objects

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8832344 [details] [diff] [review]
part 2.  Change the deny() methods of our wrapper security policies to take a JSContext and a mayThrow boolean

Review of attachment 8832344 [details] [diff] [review]:
-----------------------------------------------------------------

It's a little weird to maintain the non-JSAPI calling convention here while taking a cx. But I guess it's ok...

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +447,5 @@
>      // Fail silently for GET, ENUMERATE, and GET_PROPERTY_DESCRIPTOR.
>      if (act == js::Wrapper::GET || act == js::Wrapper::ENUMERATE ||
>          act == js::Wrapper::GET_PROPERTY_DESCRIPTOR)
>      {
> +        // XXXbz Do we really want to do that if mayThrow is false?

Yes. ReportWrapperDenial is about logging and stuff, which we want to do even if we don't interrupt script over the violation. Please remove this comment.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8832365 [details] [diff] [review]
part 3.  Implement actually throwing a SecurityError when cross-origin property accesses are denied

Review of attachment 8832365 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +295,5 @@
> +        // matches what AutoEnterPolicy::reportErrorIfExceptionIsNotPending
> +        // does.
> +        JS::RootedValue idVal(cx, js::IdToValue(id));
> +        nsAutoJSString propName;
> +        JSString* idStr = JS_ValueToSource(cx, idVal);

Does this need to be rooted?

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8832366 [details] [diff] [review]
part 4.  Fix some CrossOriginXrayWrapper error reporting bits to follow the HTML spec for cross-origin objects

Review of attachment 8832366 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +296,5 @@
> +        return false;
> +    }
> +    if (!propName.init(cx, idStr)) {
> +        return false;
> +    }

This junk is in 3 places now. Can you move it to a helper?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Does this need to be rooted?

I don't think so, but I guess there's no real harm in rooting it.  Will do that.

> This junk is in 3 places now. Can you move it to a helper?

Good point.  Will add an AccessCheck method for this.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8832343 [details] [diff] [review]
part 1.  Propagate AutoEnterPolicy's mayThrow argument to the enter() methods of proxy handlers, so they know whether it's OK to throw some sort of custom exception or whether they should just silently deny

Review of attachment 8832343 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing as discussed on IRC.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Please remove this comment.

Replaced it with:

        // Note that ReportWrapperDenial doesn't do any _exception_ reporting,
        // so we want to do this regardless of the value of mayThrow.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f366b5d467
part 1.  Propagate AutoEnterPolicy's mayThrow argument to the enter() methods of proxy handlers, so they know whether it's OK to throw some sort of custom exception or whether they should just silently deny.  r=bholley,jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8049495d3d
part 2.  Change the deny() methods of our wrapper security policies to take a JSContext and a mayThrow boolean. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/89525b069d92
part 3.  Implement actually throwing a SecurityError when cross-origin property accesses are denied.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33dc84d6386
part 4.  Fix some CrossOriginXrayWrapper error reporting bits to follow the HTML spec for cross-origin objects.  r=bholley

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47f366b5d467
https://hg.mozilla.org/mozilla-central/rev/4b8049495d3d
https://hg.mozilla.org/mozilla-central/rev/89525b069d92
https://hg.mozilla.org/mozilla-central/rev/d33dc84d6386

Comment 18

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> More parts (that actually change behavior) coming once I finish the try run.
> 
> I do have one unfixed bit left: the exception thrown by [[SetPrototype]]. 
> In our code, we land in XrayWrapper<Base, Traits>::setPrototype,
> Base::hasSecurityPolicy() is true, so we call
> SecurityWrapper<Base>::setPrototype which happily throws a non-TypeError for
> us.  Now I could change that to throw a TypeError.  Or I could try to take
> over the throwing myself based on the traits or something.  But I don't
> understand this setPrototype setup well enough to decide which is better....

Per IRC discussion, we should override setPrototype in CrossOriginXrayWrapper.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Per IRC discussion, we should override setPrototype in CrossOriginXrayWrapper.

Filed bug 1344443 on this.