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

Define security around Window, WindowProxy, and Location properly #638

Merged
merged 1 commit into from Feb 27, 2016

Conversation

7 participants
@annevk
Copy link
Member

annevk commented Feb 6, 2016

This is a relative large change to the way Window, WindowProxy, and
Location objects work, at least from a standards’ perspective. This
should more closely match implementations and define all the relevant
details.

A rough summary of the changes:

  • Window indexed getters have moved to WindowProxy.
  • WindowProxy is now a JavaScript exotic object and it handles all the
    security aspects for Window so Window can become an ordinary object.
  • Cross-origin named properties for Window are now exposed (though on
    WindowProxy). This addresses #544 and
    https://www.w3.org/Bugs/Public/show_bug.cgi?id=21674 which that PR was
    intended to replace.
  • Location is now an exotic object with all its internal methods
    overridden to handle the cross-origin security aspects.
  • Location no longer uses Unforgeable on the interface, allowing that
    to be removed from IDL. (It moved to all of its properties instead and
    the remaining details are defined through prose.)

This should also address these bugs:

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 6, 2016

Things I need to look into:

  • Now WindowProxy handles indexed properties directly, we should expose them through [[OwnPropertyKeys]] too. Currently we only do that for the cross-origin case.
  • I need to get tc39/ecma262#356 landed and add some references.

Things that made me ponder:

  • Although I used <span> around a bunch of Ordinary* abstract operation references, the build script is not complaining about missing <dfn>s. What is up with that?

In any event, this is definitely ready for review.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 6, 2016

If tc39/ecma262#367 lands we need to figure out what to do with [[Enumerate]]. It's not entirely clear to me what we can do then.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 6, 2016

Although I used around a bunch of Ordinary* abstract operation references, the build script is not complaining about missing s. What is up with that?

I've noticed this too. I think the build script thinks that empty spans are just meant to be using span to mark up a length of text, not auto-linking. That is why we should move to longer term.

Will try to review Monday.

@sideshowbarker

This comment has been minimized.

Copy link
Member

sideshowbarker commented Feb 8, 2016

Although I used around a bunch of Ordinary* abstract operation references, the build script is not complaining about missing s. What is up with that?

I've noticed this too. I think the build script thinks that empty spans are just meant to be using span to mark up a length of text, not auto-linking. That is why we should move to longer term.

To be clear, that’s an issue with wattsi, right? (Not something else in the build.sh script or the Perl scripts it calls.) If so, I wonder if whatwg/wattsi#5 regressed this, or if it’s always been this way, or what.

source Outdated
<p>The <dfn>number of child browsing contexts</dfn> is the number of <span data-x="child browsing
context">child browsing contexts</span> that are <span data-x="browsing context nested
through">nested through</span> elements that are <span data-x="in a document">in the
<code>Document</code></span> that is the <span>active document</span> of the <code>Window</code>

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 8, 2016

Member

A Window does not have an "active document"; a browsing context does.

<span>browsing context</span> had a single <code>Window</code> object, while still keeping
separate <code>Window</code> objects for each <code>Document</code>.</p>
<p>Every <code>WindowProxy</code> object has a
[[<dfn data-x="concept-windowproxy-window">Window</dfn>]] internal slot representing the wrapped

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 8, 2016

Member

[[Window]] is never set, only read.

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

+1, setting [[Window]] on navigation is pretty important :). Seems like it's in "Initialising a new Document object", step 2. Also "create a new browsing context", step 1.

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

Thank for you for pointing out the locations, fixup1 has the fix.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 10, 2016

WindowProxy needs to use https://heycam.github.io/webidl/#dfn-named-property-visibility (or a variant thereof) where it implements cross-origin named properties. In particular I think we don't want to have [OverrideBuiltins] behavior if I understood @bzbarsky's comments in https://bugzilla.mozilla.org/show_bug.cgi?id=1245554 correctly.

@annevk annevk force-pushed the cross-origin-objects branch from 52a6d48 to 442575a Feb 10, 2016

@bzbarsky

This comment has been minimized.

Copy link
Collaborator

bzbarsky commented Feb 10, 2016

We should probably test what UAs actually do with cross-origin named properties carefully. Defining how this should behave will be a bit tricky. For example, consider this testcase:

<!DOCTYPE html>
<body>
  <iframe name="foo"></iframe>
  <script>
    Object.prototype.foo = 5;
    onload = function() { alert(window.foo); }
  </script>
</body> 

In browsers this alerts "5" (at least in Chrome, Safari, and Firefox). What should happen on cross-origin access to the "foo" property on this window? Chrome and Firefox return the subframe, looks like. Safari returns undefined (or 5, depending on what the accessing thing actually is; yes, it leaks info cross-origin in some cases). Of course maybe Safari returns undefined for cross-origin named access in general? We really need some exhaustive tests. :(

source Outdated
@@ -2976,16 +2977,23 @@ a.setAttribute('href', 'http://example.com/'); // change the content attribute d
<li>The <dfn data-noexport="" data-x="js-prod-Module" data-x-href="https://tc39.github.io/ecma262/#prod-Module"><i>Module</i></dfn> production</li>
<li>The <dfn data-noexport="" data-x="js-prod-Pattern" data-x-href="https://tc39.github.io/ecma262/#prod-Pattern"><i>Pattern</i></dfn> production</li>
<li>The <dfn data-noexport="" data-x="js-prod-Script" data-x-href="https://tc39.github.io/ecma262/#prod-Script"><i>Script</i></dfn> production</li>
<li><dfn data-noexport="" data-x-href="https://tc39.github.io/ecma262/#sec-property-descriptor-specification-type">PropertyDescriptor</dfn></li>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

I've been curating this section, perhaps a bit obsessively. This should be "The [PropertyDescriptor] specification type"

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

This is meant to refer to the tag literal descriptions of Property Descriptor records.

This comment has been minimized.

Copy link
@domenic

domenic Feb 12, 2016

Member

Yeah but ES conflates them

This comment has been minimized.

Copy link
@domenic

domenic Feb 12, 2016

Member

Still would like to see this fixed. The fact that ES uses specification type names as tags when creating literals is just notation; the actual thing being referenced is the PropertyDescriptor specification type.

This comment has been minimized.

Copy link
@annevk

annevk Feb 14, 2016

Author Member

Should it then say "Property Descriptor specification type"? But the references can use the literal PropertyDescriptor?

This comment has been minimized.

Copy link
@domenic
source Outdated
<li>The <dfn data-noexport="" data-x-href="https://tc39.github.io/ecma262/#sec-source-text-module-records">Source Text Module Record</dfn> specification type and its <dfn data-noexport="" data-x="js-ModuleEvaluation" data-x-href="https://tc39.github.io/ecma262/#sec-moduleevaluation">ModuleEvaluation</dfn> and <dfn data-noexport="" data-x="js-ModuleDeclarationInstantiation" data-x-href="https://tc39.github.io/ecma262/#sec-moduledeclarationinstantiation">ModuleDeclarationInstantiation</dfn> methods</li>
<li><dfn data-noexport="" data-x-href="https://tc39.github.io/ecma262/#current-realm">The current Realm Record</dfn></li>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

This should move up to the "untyped" section at the top

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

I'm not sure what this means, but I made a best guess.

source Outdated
<li>The <dfn data-noexport="" data-x="js-NewObjectEnvironment" data-x-href="https://tc39.github.io/ecma262/#sec-newobjectenvironment">NewObjectEnvironment</dfn> abstract operation</li>
<li>The <dfn data-noexport="" data-x="js-ParseModule" data-x-href="https://tc39.github.io/ecma262/#sec-parsemodule">ParseModule</dfn> abstract operation
<li>The <dfn data-noexport="" data-x-href="https://tc39.github.io/ecma262/#sec-ordinarydefineownproperty">OrdinaryDefineOwnProperty</dfn> abstract operation</li>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Add in all the other OrdinaryXs. Consider sticking with the js- prefix, even though it's kind of a pain and I might take it back if I were starting over.

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

Perhaps it's better to move away from it over time?


<h4>Integration with IDL</h4>

<p>When <span>perform a security check</span> is invoked, with a <var>platformObject</var>,

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

This isn't linking anywhere

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

This is fixed by fixup1.

source Outdated
<b>true</b>, then:</p>

<ol>
<li><p>If <var>type</var> is "method" and <var>e</var> has neither [[needsGet]] nor

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Do we want to do the typical "<code data-x="">method</code>" style here? Or just leave it un-coded?

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

I went with IDL's precedent, but since we're not copying ECMAScript's precedent I'll use <code>. Hopefully at some point we'll get all standards to align, since this is just unnecessarily confusing readers.

source Outdated
<ol>
<li>
<p>If <span>SameValue</span>(<var>e</var>.[[property]], <var>identifier</var>) is
<b>true</b>, then:</p>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

HTML does not typically bold "true" (or "null"). In the few ES-spec-ese sections I've written (HostResolveImportedModule and HostPromiseRejectionTracker), I've stuck with not-bold.

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

Okay, removed.

source Outdated
</li>

<li><p>If <var>platformObject</var>'s global object's <span>effective script origin</span> is not
<span>same origin</span> with <span>the current Realm Record</span>'s global object's

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

s/global object/[[globalObject]]/

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

I can't find where global objects get effective script origins, only Documents. Maybe you need to add "responsible document" to the chain??

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

I guess I should start using "relevant settings object" here now that we changed to work for platform objects as well. It still doesn't feel like the best of setups though.

See also the discussion in https://www.w3.org/Bugs/Public/show_bug.cgi?id=28375.

source Outdated
<!-- we should probably remove the realm argument from IDL -->


<h4>Shared internal slot: [[<span>crossOriginPropertyDescriptorMap</span>]]</h4>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

I think the link in the heading looks bad and doesn't really add much since it's just linking a sentence down.

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

HTML does this all the time though.

This comment has been minimized.

Copy link
@domenic

domenic Feb 12, 2016

Member

Links close by, yes, but in headings?

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

E.g., "The Window object".

This comment has been minimized.

Copy link
@domenic

domenic Feb 12, 2016

Member

Yeah, for <code>s it is. But e.g. for "Browsing contexts" (and all of its subheadings, which also have terms corresponding to them) it is not.

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

Ok, will remove the <span>.

source Outdated

<p><code>Window</code> and <code>Location</code> objects both have a
[[<dfn>crossOriginPropertyDescriptorMap</dfn>]] internal slot, whose value is initially an empty
list.

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Capitalize List, and add an entry to the references section for the List specification type that you can cross-ref.

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

This should say map.

source Outdated
list.

<p>User agents should allow a value held in the map to be garbage collected along with its
corresponding key when nothing holds a reference to any part of the value. I.e., as long as

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

The hanging i.e. is a bit awkward. Maybe make it a parenthetical sentence: "(That is, as long as garbage collection is not observable.)"

source Outdated
corresponding key when nothing holds a reference to any part of the value. I.e., as long as
garbage collection is not observable.</p>

<p>E.g., with <code data-x="">const href = Object.getOwnPropertyDescriptor(crossOriginLocation,

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Make this class="example" and rephrase to "For example, if author code did

...
the value... collected, as that would be observable"

source Outdated
"href").set</code> the value and its corresponding key in the map cannot be garbage collected as
that would be observable.</p>

<p>User agents may have an optimization whereby they remove key-value pairs from the map when

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Another class="example" paragraph would be pretty great for this.

source Outdated
object's <span>effective script origin</span>, and <b>false</b> otherwise.</p></li>
</ol>

<h5><dfn>CrossOriginGetPrototypeOf</dfn> ( <var>O</var> )</h5>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

I think these need different names if they're planning to handle both same origin and cross-origin. WindowProxyOrLocationGetPrototypeOf? :-/

caller needs to throw a <code>SecurityError</code> exception.</p>

<ol>
<li><p>If <var>P</var> is <span>@@toStringTag</span>, <span>@@hasInstance</span>, or

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

The symbols aren't linking correctly

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

This should be addressed by fixup1.

source Outdated
data-x="dom-document-domain">document.domain</code> cannot revisit an earlier value.</p>


<h4>Shared abstract operations</h4>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

This needs a note about how all of the abstract operations here apply only to ordinary objects.

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Meh, trying to figure out how to phrase such a note made me give up on the idea. Maybe a note later when defining WindowProxy. We'll see when I get there.

source Outdated
<li><p>Return <b>undefined</b>.</p></li>
</ol>

<h5><dfn>CrossOriginPropertyDescriptor</dfn> ( <var>crossOriginProperty</var>,

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

I would nest all three of CrossOriginPropertyDescriptor, CrossOriginFunctionWrapper, and cross-origin wrapper functions under CrossOriginGetOwnPropertyHelper. (No need to nest cross-origin wrapper functions under CrossOriginFunctionWrapper. In fact maybe it's OK to not give it its own section.)

source Outdated
</li>

<li>
<p>Otherwise, then:</p>

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

Just "Otherwise,"

This comment has been minimized.

Copy link
@zcorpan

zcorpan Feb 12, 2016

Member

Or Otherwise: to be consistent with other parts of the spec.

@@ -76785,7 +77151,6 @@ dictionary <dfn>DragEventInit</dfn> : <span>MouseEventInit</span> {
[Replaceable] readonly attribute <span>WindowProxy</span> <span data-x="dom-parent">parent</span>;
readonly attribute <span>Element</span>? <span data-x="dom-frameElement">frameElement</span>;
<span>WindowProxy</span>? <span data-x="dom-open">open</span>(optional DOMString url = "about:blank", optional DOMString target = "_blank", [TreatNullAs=EmptyString] optional DOMString features = "", optional boolean replace = false);
<span data-x="dom-window-item">getter</span> <span>WindowProxy</span> (unsigned long index);
<span data-x="dom-window-namedItem">getter</span> object (DOMString name);

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

What do you think of adding a comment like "// indexed access to subframes is taken care of by WindowProxy" after this line?

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

The named getter here means Window is exotic, which means you need to delegate to it per our earlier discussions instead of using OrdinaryX(). Did you mean to move the named getter elsewhere?

This comment has been minimized.

Copy link
@bzbarsky

bzbarsky Feb 11, 2016

Collaborator

For global objects, the named getter adds an exotic NamedPropertiesObject on the proto chain instead of making the global itself exotic.

This comment has been minimized.

Copy link
@domenic

domenic Feb 11, 2016

Member

That is some of the spookiest action at a distance I have seen in a while -_-. Maybe a comment? "// since this is a global, per IDL, this adds an exotic NamedPropertiesObject to the prototype chain"

This comment has been minimized.

Copy link
@annevk

annevk Feb 12, 2016

Author Member

I have added a comment in fixup1 explaining both.

This comment has been minimized.

Copy link
@domenic

domenic Feb 12, 2016

Member

Comment looks good. Maybe add a blank line between open() and the getter.

This comment has been minimized.

Copy link
@annevk

annevk Feb 15, 2016

Author Member

We should do that separately, since currently each newline is indicative of a new section. So that would require some extra annotations here and there.

bterlson added a commit to tc39/ecma262 that referenced this pull request Feb 24, 2016

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 24, 2016

OK, now that ES has the appropriate hook, this LGTM!!!!! I'll let you do the honors, @annevk. wooohoooo!!!

@domenic domenic assigned annevk and unassigned domenic Feb 24, 2016

@annevk annevk force-pushed the cross-origin-objects branch from 573c619 to ff6a7c1 Feb 25, 2016

@annevk annevk assigned domenic and unassigned annevk Feb 25, 2016

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 25, 2016

@domenic I had a few more issues opened against ECMAScript that are now resolved (two PRs still pending). The biggest change is the removal of [[HasProperty]] overrides due to an upstream change. Since OrdinaryHasProperty will invoke [[GetOwnProperty]] now instead of OrdinaryGetOwnProperty.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 25, 2016

In another PR @domenic noted:

I think for these kind of informal maps, I much prefer the phrasing I used for the module map:

  • If module map contains an entry with key url, ...
  • If module map contains an entry with key url whose value is "fetching", ...
  • Create an entry in module map with key url and value "fetching"

Pretty sure we're not using that terminology for the internal map we are introducing here.

@annevk annevk force-pushed the cross-origin-objects branch from 6f64898 to 7447524 Feb 25, 2016

source Outdated
<li><p>Return false.</p></li>
</ol>

<h5 id="location-get-receiver">[[Get]] ( <var>P</var>, <var>Receiver</var> )</h5>

This comment has been minimized.

Copy link
@domenic

domenic Feb 25, 2016

Member

Can this be "location-get"?

source Outdated
<var>Receiver</var>).</p></li>
</ol>

<h5 id="location-set-v-receiver">[[Set]] ( <var>P</var>, <var>V</var>, <var>Receiver</var> )</h5>

This comment has been minimized.

Copy link
@domenic

domenic Feb 25, 2016

Member

"location-set"?

@annevk annevk force-pushed the cross-origin-objects branch from 392492c to 918cd71 Feb 26, 2016

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 26, 2016

Is the "do not merge yet" label there because of tc39/ecma262#416?

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 26, 2016

Yep

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 26, 2016

Dependencies merged; land at will @annevk!

Define security around Window, WindowProxy, and Location properly
This is a relative large change to the way Window, WindowProxy, and Location objects work, at least
from a standards perspective. This should more closely match implementations and define all their
relevant security details.

A rough summary of the changes:

* Window indexed getters have moved to WindowProxy.
* WindowProxy is now a JavaScript exotic object and it handles all the security aspects for Window
  so Window can become an ordinary object.
* Cross-origin named properties for Window are now exposed (though on WindowProxy).
* Location is now an exotic object with all its internal methods overridden to handle the
  cross-origin security aspects.
* Location no longer uses Unforgeable on the interface, allowing that to be removed from IDL. (It
  moved to all of its properties instead and the remaining details are defined through prose.)

This should also address these bugs:

* https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701
* https://www.w3.org/Bugs/Public/show_bug.cgi?id=21674
* https://www.w3.org/Bugs/Public/show_bug.cgi?id=22346
* https://www.w3.org/Bugs/Public/show_bug.cgi?id=27128
* https://www.w3.org/Bugs/Public/show_bug.cgi?id=27502

@annevk annevk force-pushed the cross-origin-objects branch from 918cd71 to acae3df Feb 27, 2016

@annevk annevk merged commit acae3df into master Feb 27, 2016

@annevk annevk deleted the cross-origin-objects branch Feb 27, 2016

@sideshowbarker

This comment has been minimized.

Copy link
Member

sideshowbarker commented Feb 27, 2016

Rock n rollーVery cool to see this landing.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 27, 2016

This might deserve a blog post :D

@Ms2ger

This comment has been minimized.

Copy link
Member

Ms2ger commented May 13, 2016

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.