Work around WeakMap not accepting some object keys.

* If the browser (particularly Firefox 20 & 21) provides a WeakMap which
  does not accept certain objects as keys (particularly events), then
  define a dual-implementation WeakMap constructor which uses the
  browser's WeakMap unless it rejects the key, then falls back to our
  WeakMap emulation.

* Domado contained code for working around being able to see the
  emulated WeakMap's magic property lookups using Proxy handlers, which
  had gotten out of date since it was never exercised as we have had no
  browsers which have Proxy but not WeakMap; update it to use results
  from actual proxies to obtain the magic property name.

* Domado's ProxyHandler.prototype now has a .constructor property so
  that tamperProof knows its properties should be overridable.

@r5353

[MarkM, 2013-04-15 18:36:05]

Looks good enough for now. For this, if urgent, a detailed review can be
post-commit.

[MarkM, 2013-04-15 18:37:50]

Please also add another reviewer for more eyeballs and more eyeballing time.

[kpreid2, 2013-04-15 18:39:20]

On 2013/04/15 18:37:50, MarkM wrote:
> Please also add another reviewer for more eyeballs and more eyeballing time.

OK. Ihab, can you take a look?

[ihab.awad, 2013-04-15 18:39:40]

I'm on it. :)


On Mon, Apr 15, 2013 at 11:39 AM, <kpreid.switchb.org@gmail.com> wrote:

> On 2013/04/15 18:37:50, MarkM wrote:
>
>> Please also add another reviewer for more eyeballs and more eyeballing
>>
> time.
>
> OK. Ihab, can you take a look?
>
>
https://codereview.appspot.**com/8612048/<https://codereview.appspot.com/8612...
>



-- 
Ihab A.B. Awad, Palo Alto, CA

[MarkM, 2013-04-15 19:46:12]

All but domado.js reviewed for real.
On domado.js, I just don't have enough context (or time to learn) to give this a
proper review.

Once WeakMap.prototype.constructor is fixed, all but domado LGTM.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:574: var ProxyHandler =
domitaModules.ProxyHandler;
Please place this declaration/initialization after the following conditional,
since it is only relevant if the condition falls through.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:581: if (/^weakmap:/.test(name)) {
weakMapMagicName = name; }
This seems totally bizarre. I don't understand the purpose of this or why it is
safe.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap...
src/com/google/caja/ses/WeakMap.js:500: WeakMap = DoubleWeakMap;
after:
WeakMap.prototype.constructor = WeakMap;
Otherwise, WeakMap.prototype.constructor would be something OurWeakMap.

[ihab.awad, 2013-04-15 20:36:53]

lgtm++

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:571: */
I guess one thing that concerns me is that we do not seem to be force-testing
(or explicitly failing early on) the Cartesian product of all the diverse and
sundry combinations of browser features (WeakMaps, Proxies, ...). Clearly, this
is a long-term concern but this CL highlights its importance.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:591: w.get(proxy);
The trick that is happening here -- using a secretive channel to get at the
otherwise secret WeakMap property name -- is very subtle and might benefit from
some explanation.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap...
src/com/google/caja/ses/WeakMap.js:462: if (typeof HostWeakMap === 'function')
(function() {
Please use curlies after condition, as a stylistic thing to avoid dangling
statement problems....

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap...
src/com/google/caja/ses/WeakMap.js:500: WeakMap = DoubleWeakMap;
Is this clever wonderfulness actually exercised by any tests in our codebase at
the moment?

[kpreid2, 2013-04-15 21:06:26]

* If the browser (particularly Firefox 20 & 21) provides a WeakMap which
  does not accept certain objects as keys (particularly events), then
  define a dual-implementation WeakMap constructor which uses the
  browser's WeakMap unless it rejects the key, then falls back to our
  WeakMap emulation.

* Domado contained code for working around being able to see the
  emulated WeakMap's magic property lookups using Proxy handlers, which
  had gotten out of date since it was never exercised as we have had no
  browsers which have Proxy but not WeakMap; update it to use results
  from actual proxies to obtain the magic property name.

* Domado's ProxyHandler.prototype now has a .constructor property so
  that tamperProof knows its properties should be overridable.

[kpreid2, 2013-04-15 21:08:51]

* If the browser (particularly Firefox 20 & 21) provides a WeakMap which
  does not accept certain objects as keys (particularly events), then
  define a dual-implementation WeakMap constructor which uses the
  browser's WeakMap unless it rejects the key, then falls back to our
  WeakMap emulation.

* Domado contained code for working around being able to see the
  emulated WeakMap's magic property lookups using Proxy handlers, which
  had gotten out of date since it was never exercised as we have had no
  browsers which have Proxy but not WeakMap; update it to use results
  from actual proxies to obtain the magic property name.

* Domado's ProxyHandler.prototype now has a .constructor property so
  that tamperProof knows its properties should be overridable.

[kpreid2, 2013-04-15 21:09:40]

New snapshot.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:574: var ProxyHandler =
domitaModules.ProxyHandler;
On 2013/04/15 19:46:12, MarkM wrote:
> Please place this declaration/initialization after the following conditional,
> since it is only relevant if the condition falls through.

I don't like putting “imports” anywhere but the top, but OK.

(Really, we should eliminate domitaModules entirely — it is vestigial — but not
as part of this CL.)

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:581: if (/^weakmap:/.test(name)) {
weakMapMagicName = name; }
On 2013/04/15 19:46:12, MarkM wrote:
> This seems totally bizarre. I don't understand the purpose of this or why it
is
> safe.

I am uncertain what part you are referring to.

- This proxy is designed to capture the magic property name so that other
proxies can handle it correctly.

- The regexp test ensures that we don't fail by capturing some other, not
WeakMap related, implicit property access triggered by the test code.

- If you are concerned about the magic property being visible here, that is not
indicative of a vulnerability because we do not whitelist Proxy for guest code,
because of this issue (as noted in WeakMap.js comments).

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:591: w.get(proxy);
On 2013/04/15 20:36:53, ihab.awad wrote:
> The trick that is happening here -- using a secretive channel to get
> at the otherwise secret WeakMap property name -- is very subtle
> and might benefit from some explanation.

Done.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap...
src/com/google/caja/ses/WeakMap.js:462: if (typeof HostWeakMap === 'function')
(function() {
On 2013/04/15 20:36:53, ihab.awad wrote:
> Please use curlies after condition, as a stylistic thing to avoid dangling
> statement problems....

The function thingy is intended to substitute for the block entirely; note the
indentation would make it difficult to accidentally add a dangling statement.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap...
src/com/google/caja/ses/WeakMap.js:500: WeakMap = DoubleWeakMap;
On 2013/04/15 19:46:12, MarkM wrote:
> after:
> WeakMap.prototype.constructor = WeakMap;
> Otherwise, WeakMap.prototype.constructor would be something OurWeakMap.

Done. Added a test too.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/ses/WeakMap...
src/com/google/caja/ses/WeakMap.js:500: WeakMap = DoubleWeakMap;
On 2013/04/15 20:36:53, ihab.awad wrote:
> Is this clever wonderfulness actually exercised by any tests in our codebase
at
> the moment?

Done.

[ihab.awad, 2013-04-15 21:17:34]

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
src/com/google/caja/ses/WeakMap.js:462: if (typeof HostWeakMap === 'function')
(function() {
Still with the syntactic neologisms. Gefurfifier. :(

[kpreid2, 2013-04-15 21:19:55]

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
src/com/google/caja/ses/WeakMap.js:462: if (typeof HostWeakMap === 'function')
(function() {
On 2013/04/15 21:17:34, ihab.awad wrote:
> Still with the syntactic neologisms. Gefurfifier. :(

OK, OK, changed.

[MarkM, 2013-04-15 21:33:29]

Non-domado parts LGTM. I leave the domado parts to Ihab.

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/8612048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:581: if (/^weakmap:/.test(name)) {
weakMapMagicName = name; }
On 2013/04/15 21:09:41, kpreid2 wrote:
> I am uncertain what part you are referring to.

Nevermind.

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
src/com/google/caja/ses/WeakMap.js:462: if (typeof HostWeakMap === 'function')
(function() {
On 2013/04/15 21:19:55, kpreid2 wrote:
> On 2013/04/15 21:17:34, ihab.awad wrote:
> > Still with the syntactic neologisms. Gefurfifier. :(
> 
> OK, OK, changed.

I don't see the curlies. Did you forget to snapshot?

[kpreid2, 2013-04-15 21:35:49]

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/Wea...
src/com/google/caja/ses/WeakMap.js:462: if (typeof HostWeakMap === 'function')
(function() {
On 2013/04/15 21:33:30, MarkM wrote:
> On 2013/04/15 21:19:55, kpreid2 wrote:
> > OK, OK, changed.
> 
> I don't see the curlies. Did you forget to snapshot?

Yup, and I just blew away my local copy after committing (Ihab said lgtm++), so
I can't.

[MarkM, 2013-04-15 21:40:34]

Please consider a CL just adding these curlies to already be LGTMed, so
commit when ready without need for further feedback.


On Mon, Apr 15, 2013 at 2:35 PM, <kpreid.switchb.org@gmail.com> wrote:

>
> https://codereview.appspot.**com/8612048/diff/16001/src/**
>
com/google/caja/ses/WeakMap.js<https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/WeakMap.js>
> File src/com/google/caja/ses/**WeakMap.js (right):
>
> https://codereview.appspot.**com/8612048/diff/16001/src/**
>
com/google/caja/ses/WeakMap.**js#newcode462<https://codereview.appspot.com/8612048/diff/16001/src/com/google/caja/ses/WeakMap.js#newcode462>
> src/com/google/caja/ses/**WeakMap.js:462: if (typeof HostWeakMap ===
> 'function') (function() {
> On 2013/04/15 21:33:30, MarkM wrote:
>
>> On 2013/04/15 21:19:55, kpreid2 wrote:
>> > OK, OK, changed.
>>
>
>  I don't see the curlies. Did you forget to snapshot?
>>
>
> Yup, and I just blew away my local copy after committing (Ihab said
> lgtm++), so I can't.
>
>
https://codereview.appspot.**com/8612048/<https://codereview.appspot.com/8612...
>



-- 
Text by me above is hereby placed in the public domain

  Cheers,
  --MarkM