1038993 - Add UnsafeInPrerendering extended attribute to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Roshan Vidyashankar [:rvid]]

Attachment: unsafeinprerendering.patch

Comment 2

[(no longer active)]

Comment on attachment 8461691 [details] [diff] [review]
unsafeinprerendering.patch

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

For the benefit of others, here is a description of what we are trying to achieve here: We are adding a WebIDL extended attribute to designate a method/property as unsafe to call for documents that are in prerendering mode.  This is currently just a stub that doesn't do anything but it lets us add these annotations while we're working on the rest of the bits and pieces.

::: dom/bindings/BindingUtils.cpp
@@ +2327,5 @@
>    return false;
>  }
>  
>  bool
> +HandleUnsafePrerender(){

Nit: brace on its own line.

::: dom/bindings/BindingUtils.h
@@ +2913,5 @@
>  bool
>  CheckPermissions(JSContext* aCx, JSObject* aObj, const char* const aPermissions[]);
>  
> +bool
> +HandleUnsafePrerender();

Please document what this function is supposed to do and what its return value indicates.

Also, I've been thinking about a better name for this, what do you think about calling it CheckSafetyInPrerendering() and make a return value of true mean that the call is safe?

::: dom/bindings/Codegen.py
@@ +6134,5 @@
>          cgThings = []
> +        if idlNode.getExtendedAttribute("UnsafeInPrerendering"):
> +            cgThings.append(CGGeneric(fill(
> +                    """
> +                    if(mozilla::dom::HandleUnsafePrerender()){

Nit: spaces around the if condition.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do we want a whitelist or blacklist here?  It seems safer to me to have a whitelist of things that _are_ safe to call while prerendering, but maybe we'd need to annotate too much?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Could we perhaps whitelist so that one could say a certain interface is safe to use, and
then annotate certain methods/attributes in it to be unsafe?
By default all interfaces would be unsafe.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's definitely implementable.  Do we expect most interfaces to be usable or not?

One option would be to have some sort of runtime test that people have to add all usable interfaces to so we don't have gunk in the IDL files but people have to consciously make the decision at some point...

Comment 6

[(no longer active)]

(In reply to comment #5)
> That's definitely implementable.  Do we expect most interfaces to be usable or
> not?

I expect 99.xx% of our APIs to be usable in prerendering mode.  Really the list of things that we will blacklist will be in the order of tens of methods.  But still we are triaging the list carefully, and I'm reviewing Roshan's classification.

> One option would be to have some sort of runtime test that people have to add
> all usable interfaces to so we don't have gunk in the IDL files but people have
> to consciously make the decision at some point...

Is that really attainable?  I don't necessarily object to what you're suggesting, but it's going to be a *ton* of work, so I really want to understand why you think it's going to be worth it.

That aside, I don't think we can rely on runtime tests because of [NoInterfaceObject] interfaces.  Unless if you mean calling MagicFunction which calls a SafeForPrerendering() on every WebIDL C++ class (or JS function call in case of JS bindings), or something...

Comment 7

[Boris Zbarsky [:bzbarsky]]

> so I really want to understand why you think it's going to be worth it.

It basically depends on what the failure mode is of someone exposing an interface that shouldn't be exposed in prerendering.  Is that sort of thing likely to be a security bug?  If so, then we want to make sure it doesn't happen accidentally.  If it's just a functionality bug, I'd be a lot more ok with a blacklist.

Comment 8

[(no longer active)]

(In reply to comment #7)
> > so I really want to understand why you think it's going to be worth it.
> 
> It basically depends on what the failure mode is of someone exposing an
> interface that shouldn't be exposed in prerendering.  Is that sort of thing
> likely to be a security bug?  If so, then we want to make sure it doesn't
> happen accidentally.  If it's just a functionality bug, I'd be a lot more ok
> with a blacklist.

I have not thought of a security issue yet.  Do you have anything in mind?  The kinds of issues I'm worried about are functionality issues, i.e., an invisible tab play a sound, or show a UI, etc.  I would expect those kinds of issues to surface once we start exposing this on a product on Nightly.  We will definitely have some testing time on Nightly, this is not a feature that can just ride the trains as normal.  :-)

Comment 9

[Boris Zbarsky [:bzbarsky]]

My first thought was things like an invisible tab showing a webrtc prompt that makes it look like some other site is asking for the permission or otherwise tricking a user into granting it permissions it should not have

Comment 10

[(no longer active)]

(In reply to comment #9)
> My first thought was things like an invisible tab showing a webrtc prompt that
> makes it look like some other site is asking for the permission or otherwise
> tricking a user into granting it permissions it should not have

Hmm, yes, that would be something that would happen if we screwed this attribute up on the WebRTC API entry points.  :/

Comment 11

[(no longer active)]

We should also make sure that if you specify UnsafeInPrerendering on an interface which has a NavigatorProperty, we call this hook function in the generated navigator property as well.

Comment 12

[(no longer active)]

Boris, what do you think about blacklisting and whitelisting here?  Would you like Roshan to change his patch to focus on whitelisting instead?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm torn.  Whitelisting seems safer, but also seems like it would cause a lot of noise in our IDL....

I'm ok with trying a blacklist for now, I guess.

Comment 14

[Roshan Vidyashankar [:rvid]]

Attachment: 1038993_2.patch

Comment 15

[(no longer active)]

Comment on attachment 8468943 [details] [diff] [review]
1038993_2.patch

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

::: dom/bindings/BindingUtils.h
@@ +2914,5 @@
>  CheckPermissions(JSContext* aCx, JSObject* aObj, const char* const aPermissions[]);
>  
> +//Returns true if page is being prerendered.
> +bool
> +IsInPrerenderState();

Hmm, this is actually a worse name than before, since in the future we want this function to "kill" the prerendered content too, so it will do more than just checking whether something is in prerendered state.

::: dom/bindings/Codegen.py
@@ +6237,5 @@
> +        if (idlNode.getExtendedAttribute("UnsafeInPrerendering") or
> +                descriptor.interface.getExtendedAttribute("UnsafeInPrerendering") or
> +                any([i.getExtendedAttribute("UnsafeInPrerendering")
> +                    for i in descriptor.interface.getInheritedInterfaces()])):
> +                    cgThings.append(CGGeneric(fill(

Nit: four space indentation for the contents of the if block please.

@@ +6239,5 @@
> +                any([i.getExtendedAttribute("UnsafeInPrerendering")
> +                    for i in descriptor.interface.getInheritedInterfaces()])):
> +                    cgThings.append(CGGeneric(fill(
> +                        """
> +                    if (mozilla::dom::IsInPrerenderState()) {

I think you want to pass the JS context/object to this function, similar to CheckPermissions.

Comment 16

[Roshan Vidyashankar [:rvid]]

Attachment: 1038993_3.patch

Feel free to suggest a name better than CheckSafetyInPrerendering. Thanks!

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8470497 [details] [diff] [review]
1038993_3.patch

I'd prefer if peterv could look at this.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 8470497 [details] [diff] [review]
1038993_3.patch

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

::: dom/bindings/BindingUtils.cpp
@@ +2354,5 @@
>    return false;
>  }
>  
>  bool
> +CheckSafetyInPrerendering(JSContext* aCx, JSObject* aObj)

Can we name this Prerendering(...) or that at least makes it clear what the return value means? Presumably it has nothing to do with the safety, just whether we're prerendering or not?

::: dom/bindings/Codegen.py
@@ +6235,5 @@
> +        # inherited interfaces have the UnsafeInPrerendering extended attribute
> +        # and if so, we add a check to make sure it is safe.
> +        if (idlNode.getExtendedAttribute("UnsafeInPrerendering") or
> +            descriptor.interface.getExtendedAttribute("UnsafeInPrerendering") or
> +            any([i.getExtendedAttribute("UnsafeInPrerendering")

There should be no need for the [] braces inside the any(...).

@@ +6237,5 @@
> +        if (idlNode.getExtendedAttribute("UnsafeInPrerendering") or
> +            descriptor.interface.getExtendedAttribute("UnsafeInPrerendering") or
> +            any([i.getExtendedAttribute("UnsafeInPrerendering")
> +                for i in descriptor.interface.getInheritedInterfaces()])):
> +                cgThings.append(CGGeneric(fill(

I think we use dedent instead of fill if there's no substitution.

@@ +6240,5 @@
> +                for i in descriptor.interface.getInheritedInterfaces()])):
> +                cgThings.append(CGGeneric(fill(
> +                    """
> +                    if (mozilla::dom::CheckSafetyInPrerendering(cx, obj)) {
> +                        //TODO: Handle call into unsafe API duing Prerendering

Please add a reference to the bug report that tracks adding the code to handle this.

s/duing/during/

Comment 19

[Roshan Vidyashankar [:rvid]]

Peter, the function does more than check if we are prerendering. We are still trying to find a better name for it. I think it will be more obvious once its implemented.

Comment 20

[Roshan Vidyashankar [:rvid]]

Attachment: 1038993_4.patch

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing. Also, a Try run would be appreciated :)

Comment 22

[Roshan Vidyashankar [:rvid]]

Attachment: 1038993_5.patch

Done :)

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39e51fbc96e1

Thanks for the Try run! One request, for the future, please consider whether you really need to run builds and tests across all platforms or whether a smaller subset would work. Doing so can save a tremendous amount of resources. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/39e51fbc96e1