832014 - Convert Location to WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This needs some wrappers/security work first.

Comment 1

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

(In reply to Boris Zbarsky (:bz) from comment #0)
> This needs some wrappers/security work first.

Still? If so, what?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bobby?

Comment 3

[Bobby Holley (:bholley)]

The XPCWN situation is now in a good place for this, but we still need a WebIDL equivalent of UnwrapThisIfAllowed.

Basically, to make call/apply work, cross-origin-accessible methods need to be able to do an unsafe unwrap of security wrappers. The simplest thing from a bindings perspective is just to add an annotation to the WebIDL of the method that will cause Codegen to do the right thing. But then we end up storing our ACL in two places (in the WebIDL, and in IsPermitted).

What we could do is to Codegen IsPermitted from WebIDL annotations in some sort of generated BindingUtils. Does that seem reasonable?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Possibly, yes.

Note that right now the unwrapping code is NOT codegenned, though the calls to it are; we'd need to figure out how to jam the codegen-determined security checks in there.  What form do those take, exactly?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Possibly, yes.
> 
> Note that right now the unwrapping code is NOT codegenned, though the calls
> to it are; we'd need to figure out how to jam the codegen-determined
> security checks in there.  What form do those take, exactly?

Which ones?

The UnwrapThisIfAllowed stuff can be implementing by replacing CheckedUnwrap with UncheckedUnwrap in the codegened stubs for XO-accessible methods.

For IsPermitted, it's probably best to just look at the existing function. We want something that does that, more or less:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp#144

Comment 6

[Boris Zbarsky [:bzbarsky]]

The point is there are no CheckedUnwrap calls in the codegen.  The codegen calls some other code, which is what does the unwrap calls.

Futhermore, the call to the code that does the unwrap is in the shared (codegenned) genericMethod code, not in the per-method stubs.

So we'll need to write some sort of entirely new codegen for "these cases".  My question is what "these cases" actually are.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So for example, whether there's a sane way to do it without a separate genericMethod for them.

I guess if it's just annotation in the IDL, there really is not.

Comment 8

[Bobby Holley (:bholley)]

Can we just make genericCrossOriginMethod and call that for the right methods?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sure.  Do we need that for getters and setters too, or just methods?

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Sure.  Do we need that for getters and setters too, or just methods?

getters and setters as well.

Comment 11

[Johnny Stenback (:jst)]

Attachment: diff

This is the mechanical part of this conversion, or at least the beginning of such. But it does *not* address some of the fundamental missing pieces here, such as the need for Unforgeable or the security wrapper pieces.

Comment 12

[Bobby Holley (:bholley)]

For posterity, here's what I recently suggested for the security bits on a private thread:

* Add a new WebIDL extended attribute called [AllowCrossOrigin] that we apply to various properties and methods on Window and Location to indicate that they're cross-origin accessible.
* Use the above to Codegen the IsPermitted policy table that's currently hardcoded in AccessCheck.cpp. This prevents any duplication of sensitive security invariants.
* For [AllowCrossOrigin] properties/methods, we use a special JSNative that uses UncheckedUnwrap instead of CheckedUnwrap to unwrap |this|. This ensures that cross-origin-accessible methods can be call/apply-ed however the caller pleases.
* Add a per-interface attribute called [ExplicitSecurityCheck=Func]. If this is set on an interface, _every_ method that operates the interface (except those marked [AllowCrossOrigin]) must codegen a call to |Func| during this-unwrapping, which is allowed to veto. For Location, Func() will return SubjectPrincipal->Subsumes(PrincipalOfTheActiveInnerForOurOuter). Note the semantics here apply to all parent interfaces, which for us is basically just URLUtils, but the check should only be performed if the object implements an interface that is at least as concrete as the interface for which [ExplicitSecurityCheck] is set. This prevents the security checks from interfering with any other consumers of URLUtils.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Notes to self on what needs to be done here:

1)  Need support for unforgeable methods in codegen.
2)  Need support for unforgeable interfaces.  Specifically, need to either require or fake
    a stringifier, force an undefined toJSON, and require or fake a valueOf.  Since this
    interface is likely to be the only unforgeable interface in the platform ever, we
    should just do whatever is easiest for these.
3)  Fix up AccessCheck.
4)  Do the actual conversion.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add parser support for an [Unforgeable] annotation on WebIDL interfaces

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add codegen support for [Unforgeable] interfaces

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Start using nsLocation for the Location interface in bindings

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make the URLUtils APIs able to throw, since Location needs to be able to do that so far

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Add WebIDL API to nsLocation

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Make it possible to specify that only a particular interface that implements a given consequential interface gets cross-origin-settable behavior for a particular property

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Fix Xray support for unforgeable methods

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Switch Location to WebIDL bindings

Bobby, could you look over the test changes here?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Switch AccessCheck to using the generated LocationBinding::IsPermitted method

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8452524 [details] [diff] [review]
part 6.  Make it possible to specify that only a particular interface that implements a given consequential interface gets cross-origin-settable behavior for a particular property

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

r=me modulo those issues.

::: dom/bindings/Codegen.py
@@ +2202,5 @@
>  
> +def IsCrossOriginWritable(attr, descriptor):
> +    """
> +    Return whether the IDLAttribute in question is cross-origin writable on the
> +    interface represented by descriptor.

I'd at least mention Location/URLUtils explicitly here, since that's the only case where this matters (and that's not likely to change).

@@ +2210,5 @@
> +        return False
> +    if crossOriginWritable == True:
> +        return True
> +    assert (isinstance(crossOriginWritable, list) and
> +            len(crossOriginWritable) == 1)

The second part of this assertion seems weird. Why is this a list if we're only planning on it being one entry? Should it be a string? Or alternatively, should we have a comment saying that this assertion reflects the fact that this is only used for Location?

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8452527 [details] [diff] [review]
part 8.  Switch Location to WebIDL bindings

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

::: dom/tests/mochitest/bugs/test_bug541530.html
@@ +64,5 @@
>     "shouldn't be able to delete the property off of the prototype");
>  
>  try {
>      this.__defineGetter__.call(Object.getPrototypeOf(window.location), 'href', function(){});
> +    ok(true, "should be able to define things on the prototype");

I'd just nix the try block at this point.

::: js/xpconnect/tests/mochitest/file_bug802557.html
@@ +23,5 @@
>      getLocationApply3: function() {
>        return Function.call.apply(gTS, [loc()]);
>      },
>      getLocationViaPrototype: function() {
> +      return location.toString.call(loc());

This name is no longer correct, and the test duplicates getLocationApply1.

Can you remove this and add a test with URL.prototype.toString (to make sure that the share URLUtils sharing doesn't somehow make that method usable on Location)?

@@ +32,5 @@
>      getHrefViaApply: function() {
>        return Function.call.apply(gGHR, [loc()]);
>      },
>      getHrefViaPrototype: function() {
> +      return Object.getOwnPropertyDescriptor(location, 'href').get.call(loc());

Same here.

::: js/xpconnect/tests/mochitest/test_bug793969.html
@@ +37,5 @@
>  checkThrows(function() { "use strict"; location.valueOf = function() { return {a: 'hah'};} }, 'Shadow with function', /* skipMessageCheck = */ true);
>  checkThrows(function() { Object.defineProperty(location, 'valueOf', { value: function() { return 'hah'; } }); }, 'defineProperty with value');
>  checkThrows(function() { delete location.valueOf; Object.defineProperty(location, 'valueOf', { value: function() { return 'hah'; } }); }, 'delete + defineProperty with value');
> +checkThrows(function() { Object.defineProperty(location, 'valueOf', { get: function() { return 'hah'; } }); }, 'defineProperty with getter');
> +checkThrows(function() { delete location.valueOf; Object.defineProperty(location, 'valueOf', { get: function() { return 'hah'; } }); }, 'delete + defineProperty with getter');

Doh, thanks for fixing those.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I'd at least mention Location/URLUtils explicitly here

Will do.

> Why is this a list

Because the webidl parser produces a value of True if the extended attribute is used with no value and otherwise a list of the values (typically only one of them).

> I'd just nix the try block at this point.

Will do.

> Can you remove this and add a test with URL.prototype.toString (to make sure that the
> share URLUtils sharing doesn't somehow make that method usable on Location)?

Can do.

> Same here.

OK.

Comment 28

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

Comment on attachment 8452527 [details] [diff] [review]
part 8.  Switch Location to WebIDL bindings

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

::: dom/tests/mochitest/bugs/test_bug291377.html
@@ +25,5 @@
>  } catch (e) {
>    /* Check that we can touch various properties */
>    isnot(e.lineNumber, undefined, "Unexpected line number"); //This line number is dependent on the implementation of the SpecialPowers API
>    is(e.name, "NS_ERROR_MALFORMED_URI", "Unexpected exception name");
> +  is(e.message, "", "Should have a message for this case");

Should not? And ise, please

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Should not? And ise, please

Done.

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 8452517 [details] [diff] [review]
part 3.  Start using nsLocation for the Location interface in bindings

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

We don't want to rename nsLocation to mozilla::dom::Location?

Comment 31

[Peter Van der Beken [:peterv]]

Comment on attachment 8452521 [details] [diff] [review]
part 5.  Add WebIDL API to nsLocation

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

::: dom/base/nsLocation.h
@@ +158,2 @@
>    nsWeakPtr mDocShell;
>    nsWeakPtr mOuter;

When would this not be a weak reference to mInnerWindow->GetOuterWindow()? Otherwise we might be able to drop this member?

::: dom/webidl/Location.webidl
@@ +19,2 @@
>    void replace(DOMString url);
> +  // XXXbz there is no forceget argument in the spec!

Bug filed?

@@ +23,2 @@
>  };
> +// No support for .searchParams on Location yet.

Bug filed?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> We don't want to rename nsLocation to mozilla::dom::Location?

Oh, I thought I said something about that.

We have a number of classes called Location in the tree, in other namespaces.  Given how C++ (or maybe our CC macros' use thereof?) treats class name collisions (really really badly; you silently get things like ~Location calling the wrong destructor in some cases!) I decided it was better to leave the name for now.  :(

> When would this not be a weak reference to mInnerWindow->GetOuterWindow()?

Is GetOuterWindow on an inner immutable, even for inners that have been unloaded?  If so, then never, and we can nix mOuter.

> Bug filed?

Good point.  Bug 1037715.

Comment 33

[Boris Zbarsky [:bzbarsky]]

I guess the outer is immutable until the inner is unlinked, right?  Given that, I think we can in fact nix mOuter.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Oh, and filed bug 1037721 for forceGet.

Comment 35

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/e437d7fa90a5
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d52ad359c132
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b517ace62a49
   https://hg.mozilla.org/integration/mozilla-inbound/rev/20e4769e2106
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bce1d778cf
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfd6eb4518f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/460159ffbb79
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb30689a1d8c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebd702e6442

Comment 36

[Jan de Mooij [:jandem]]

Comment on attachment 8452528 [details] [diff] [review]
part 9.  Switch AccessCheck to using the generated LocationBinding::IsPermitted method

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

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +109,1 @@
>          case 'W':

Nit: add a "break;" here to avoid falling through to strcmp(name, "Window").

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e437d7fa90a5
https://hg.mozilla.org/mozilla-central/rev/d52ad359c132
https://hg.mozilla.org/mozilla-central/rev/b517ace62a49
https://hg.mozilla.org/mozilla-central/rev/20e4769e2106
https://hg.mozilla.org/mozilla-central/rev/f7bce1d778cf
https://hg.mozilla.org/mozilla-central/rev/ddfd6eb4518f
https://hg.mozilla.org/mozilla-central/rev/460159ffbb79
https://hg.mozilla.org/mozilla-central/rev/fb30689a1d8c
https://hg.mozilla.org/mozilla-central/rev/7ebd702e6442

Comment 38

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #36)
> Nit: add a "break;" here to avoid falling through to strcmp(name, "Window").

I'll handle this in my patches for bug 1036185.