Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why are indexed properties on WindowProxy not enumerable in the spec? #2753

Closed
bzbarsky opened this Issue Jun 12, 2017 · 31 comments

Comments

5 participants
@bzbarsky
Copy link
Collaborator

commented Jun 12, 2017

Looks like they're not enumerable in Blink/WebKit, but are enumerable in Gecko/Edge. Other indexed objects have the indexed props enumerable, so why not WindowProxy?

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2017

Note that I was going to add a wpt for the behavior here initially, but holding off on that for now because I think the spec is actually wrong here.

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701#c144 ctrl+F enumerable says

Yep, that's the reason. This stuff is normally enumerable, but we decided to
make it non-enumerable for cross-origin objects. We still have to list them
for [[GetOwnPropertyKeys]] though, so we could also just make them
enumerable. It doesn't matter much.

So that at least explains things, I guess.

It does seem nicer to be consistent. Any thoughts, @yuki3 / @cdumez?

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 16, 2017

Non-enumerable in the cross-origin case makes sense. But why are they non-enumerable in the same-origin case?

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

I mean, presumably for consistency along that axis...

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 16, 2017

I should note, by the way, that the quote above is talking about the cross-origin-exposed IDL properties, not the indexed ones.

@yuki3

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

Could you guys check my understanding?

LegacyPlatformObjectGetOwnProperty defines non-WindowProxy case and it says that indexed properties must have {enumerable: true}.

WindowProxy's [[OwnPropertyKeys]] lists up indexed properties as same as OrdinaryOwnPropertyKeys, so it looks like enumerable from this point of view.

IIUC, it's quite natural, consistent and intuitive that WindowProxy's indexed properties are enumerable. I support this change.

Just FYI, (maybe this is not so relevant) Blink has another bug about WindowProxy's indexed properties:
https://bugs.chromium.org/p/chromium/issues/detail?id=695069
Probably, enumerable is something we can fix along with the above bug.

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 19, 2017

WindowProxy's [[OwnPropertyKeys]] lists up indexed properties as same as OrdinaryOwnPropertyKeys, so it looks like enumerable from this point of view.

These two things are pretty unrelated. [[OwnPropertyKeys]] just lets you know the result of Object.getOwnPropertyNames(); both enumerable and un-enumerable properties show up there.


So there are several things that are currently in conflict:

  • Consistency of same-origin WindowProxys versus cross-origin ones (currently both non-enumerable)
  • Consistency of WindowProxy vs. other platform objects (currently non-enumerable vs. enumerable)
  • Consistency of indexed properties vs. named properties (currently both non-enumerable)

If we make the change proposed here, of switching same-origin indexed properties to enumerable, this becomes:

  • Consistency of same-origin WindowProxys versus cross-origin ones (non-enumerable vs. enumerable)
  • Consistency of WindowProxy vs. other platform objects (non-enumerable for named, enumerable for indexed, vs. enumerable)
  • Consistency of indexed properties vs. named properties (non-enumerable for cross-origin, enumerable for same-origin, vs. non-enumerable)

This seems like a downgrade to me.

What would make things very consistent is making both named and indexed properties always enumerable. Alternately we could make named properties for WindowProxy enumerable same-origin too, which still would leave cross-origin WindowProxy objects as inconsistent, but at least confine that.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Consistency of WindowProxy vs. other platform objects (non-enumerable for named, enumerable for indexed, vs. enumerable)

@domenic I'm not sure what you mean here. When you say "named", do you mean IDL operations/attributes, or the properties on the NamedPropertiesObject (not the WindowProxy)?

If you mean the former, they are enumerable on same-origin windows (just like on all other platform objects) and non-enumerable on cross-origin ones. So your consistency descriptions don't look correct to me at all, for either the "before" or the "after" case, if these are the things you're talking about.

If you mean the things on the NamedPropertiesObject, those don't exist at all in the cross-origin case. In the same-origin case they are non-enumerable because the interface uses LegacyUnenumerableNamedProperties, but note that this is all about the NamedPropertiesObject, not the WindowProxy itself.

Alternately we could make named properties for WindowProxy enumerable same-origin too

Again, I'd like to understand which properties you mean by "named properties" here. The WindowProxy does not have any default non-enumerable properties with non-integer names in the spec right now, in the same-origin case.

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 19, 2017

Sorry, that definitely was confusing. I was referring to step 6 of https://html.spec.whatwg.org/multipage/window-object.html#windowproxy-getownproperty, i.e. the named browsing context properties.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Oh, we actually expose some subset of the NamedPropertiesObject, in the cross-origin case? I had forgotten about that...

I think those were supposed to be non-enumerable to avoid a cross-origin information leak. But we have that leak anyway, because ES added ways to get lists of non-enumerable properties. :(

@yuki3

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2017

I'm now catching up... Then, we need to consider a) WindowProxy versus other platform objects, b) same-origin case versus cross-origin case, and c) indexed properties versus named properties (including cross-origin's subset case).

If I'm correctly understanding the current status,

  • WindowProxy, same-origin, indexed property = non enumerable

  • WindowProxy, cross-origin, indexed property = non enumerable

  • platform object, (same-origin), indexed property = enumerable

  • WindowProxy, same-origin, named property = enumerable

  • WindowProxy, cross-origin, named property = non enumerable
    (in case of document-tree child browsing context name property set)

  • platform object, (same-origin), named property = enumerable
    (unless it's [LegacyUnenumerableNamedProperties].)

Are these correct?

And are we aiming the following state?

  • same-origin = enumerable
  • cross-origin = non enumerable
    (regardless of whether WindowProxy or other platform objects, indexed or named properties)

If so, it looks reasonable.

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

And are we aiming the following state?

I think that is the idea @bzbarsky proposes. I don't quite know why being cross-origin should make something non-enumerable, so I offered the alternative that everything should be enumerable, cross- or same-origin. How do people feel about that?

(I don't feel strongly.)

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2017

Again, the original intent of non-enumerability was to avoid cross-origin information leaks... Then ES added other ways that information leaks. :(

@yuki3

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

Again, the original intent of non-enumerability was to avoid cross-origin information leaks...

The document-tree child browsing context name property set is not leak, I guess? It must be either of a) same-origin case, or b) case that browsing context container's name matches with child browsing context name. So, either way, we already knew the names.

Then ES added other ways that information leaks.

I don't know exactly which part of ES leaks, however, assuming that it's okay to leak the document-tree child browsing context name property set because the call site already knew their names, it seems okay to me anyway.

As long as no security risk, @domenic's proposal looks good to me.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2017

So, either way, we already knew the names.

No, we didn't. The relevant case is window A getting the "document-tree child browsing context name property set" of window B, where A and B are different-origin but all the subframes of B are same-origin with B.

I don't know exactly which part of ES leaks

Object.getOwnPropertyNames.

@yuki3

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

Hmm, I see. Then, what do we want to do? I don't have a strong opinion, but @domenic's proposal seems still acceptable given that Object.GetOwnPropertyNames tells the names anyway.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2017

If we could figure out some way to have the names not show up in getOwnPropertyNames but still work via [[Get]] that would be ideal from a security point of view. Assuming that they do need to work from [[Get]], that is.

@yuki3

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

Got it. For the meanwhile (until we find the ideal solution), do we want to make the indexed properties enumerable only when it's same-origin? I'm fine with it.

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

If we could figure out some way to have the names not show up in getOwnPropertyNames but still work via [[Get]] that would be ideal from a security point of view.

I mean, we can define [[OwnPropertyKeys]]() to do whatever we want. (It appears it would not violate any JS invariants since the properties are configurable.)

Is that web-compatible? Is it worth the churn?

And what about the indexed property names; should we make those enumerable (both same- and cross-origin)?

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2017

Is that web-compatible?

Unknown. What do UAs actually do? For example, do they all even expose the named stuff in getOwnPropertyNames on cross-origin windows?

Is it worth the churn?

If we can manage to do it, then imo yes. Cross-origin information leaks are generally considered a very bad thing!

And what about the indexed property names

For the indexed ones, the security argument doesn't apply because you can enumerate them in the obvious way: start at 0 and keep doing [[Get]] until undefined comes back, right? So I would have no problem with just making them enumerable in both cases.

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

OK. It sounds like we have an idea of where we'd like the spec to end up. I will put together a PR, but I would appreciate if someone could work on web platform tests. Doing the test work should also help us understand where browsers are and thus the potential web compatibility.

domenic added a commit that referenced this issue Jun 21, 2017

Tweak the exposure of cross-origin properties
This contains two separate changes:

* It makes all cross-origin properties that would normally be enumerable
  on same-origin objects, enumerable also on WindowProxy and Location
  objects (including when accessed same-origin). This includes
  safelisted methods and attributes, browsing context name properties,
  and browsing context index properties. The motivation for making them
  non-enumerable seems to have been a mistaken impression that doing so
  would prevent a cross-origin information leak.

* It hides window names from [[OwnPropertyKeys]](), and thus
  Object.keys(), Object.getOwnPropertyNames(), etc. This actually
  prevents a cross-origin information leak.

Closes #2753.
@domenic

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@annevk do you have any opinions on this? For the TL;DR version of where we ended up, see #2777.

I'm also still hoping someone will write tests for me, but I haven't seen any signs of movement there yet...

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

In general this all seemed reasonable. I can probably write tests next week. Might need another ping.

@annevk

This comment has been minimized.

Copy link
Member

commented Jul 13, 2017

Tests are at web-platform-tests/wpt#6538. It turns out that no browser makes these properties enumerable currently and named properties were already hidden (as per PR). I guess we still want to try to change all browsers then to make them enumerable (which also makes Object.keys() work)? I'm happy to file all the relevant browser bugs provided everyone is on board.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2017

@annevk I'm not sure what "these properties" refers to in your last comment. Is it talking about what this issue is filed on (indexed properties on same-origin WindowProxy), or something else?

@annevk

This comment has been minimized.

Copy link
Member

commented Jul 13, 2017

Making all properties (indexed, named, and the safelisted bunch), same-origin and cross-origin, enumerable. (Although same-origin there's no safelist and named properties live on a prototype object.) Coupled with making [[OwnPropertyKeys]]() not return any named properties in the cross-origin case.

That's what #2777 proposes.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2017

That seems pretty reasonable to me.

@annevk annevk closed this in #2777 Jul 17, 2017

annevk added a commit that referenced this issue Jul 17, 2017

Tweak the exposure of cross-origin properties
This contains two separate changes:

* It makes all cross-origin properties that would normally be enumerable
  on same-origin objects, enumerable also on WindowProxy and Location
  objects (including when accessed same-origin). This includes
  safelisted methods and attributes, browsing context name properties,
  and browsing context index properties. The motivation for making them
  non-enumerable seems to have been a mistaken impression that doing so
  would prevent a cross-origin information leak.

* It hides window names from [[OwnPropertyKeys]](), and thus
  Object.keys(), Object.getOwnPropertyNames(), etc. This actually
  prevents that cross-origin information leak.

Closes #2753.
@domenic

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Thanks for helping finish this up, @annevk!

@cdumez

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

Are the tests correct?
web-platform-tests/wpt#6538 (comment)

Noticed this when trying to align WebKit.

@annevk

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

Created web-platform-tests/wpt#6571 as a follow-up for the tests. Thanks!

alice added a commit to alice/html that referenced this issue Jan 8, 2019

Tweak the exposure of cross-origin properties
This contains two separate changes:

* It makes all cross-origin properties that would normally be enumerable
  on same-origin objects, enumerable also on WindowProxy and Location
  objects (including when accessed same-origin). This includes
  safelisted methods and attributes, browsing context name properties,
  and browsing context index properties. The motivation for making them
  non-enumerable seems to have been a mistaken impression that doing so
  would prevent a cross-origin information leak.

* It hides window names from [[OwnPropertyKeys]](), and thus
  Object.keys(), Object.getOwnPropertyNames(), etc. This actually
  prevents that cross-origin information leak.

Closes whatwg#2753.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.