Do not leak private property names to proxy traps and interceptors.

Do not leak private property names to proxy traps and interceptors.

R=rossberg
BUG=

Committed: https://crrev.com/2b352bb84f0f2d21997c18cd361b95e905c39400
Cr-Commit-Position: refs/heads/master@{#33212}

[rossberg, 2016-01-11 15:43:22]

https://codereview.chromium.org/1573143002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1573143002/diff/1/src/objects.cc#newcode4164
src/objects.cc:4164: NewTypeError(MessageTemplate::kIllegalInvocation));
Nit: Maybe use a more explicit error message?

https://codereview.chromium.org/1573143002/diff/1/src/objects.cc#newcode6232
src/objects.cc:6232: if (it->GetReceiver()->IsJSProxy()) {
Nit: maybe put a short explanatory comment here.

https://codereview.chromium.org/1573143002/diff/1/test/mjsunit/harmony/privat...
File test/mjsunit/harmony/private-symbols.js (right):

https://codereview.chromium.org/1573143002/diff/1/test/mjsunit/harmony/privat...
test/mjsunit/harmony/private-symbols.js:33: var proxy = new Proxy({}, new
Proxy({}, {get() {return () => {throw 666}}}));
Shouldn't the handler have all the traps?

[neis, 2016-01-11 16:00:04]

See new patchset.

https://codereview.chromium.org/1573143002/diff/1/test/mjsunit/harmony/privat...
File test/mjsunit/harmony/private-symbols.js (right):

https://codereview.chromium.org/1573143002/diff/1/test/mjsunit/harmony/privat...
test/mjsunit/harmony/private-symbols.js:33: var proxy = new Proxy({}, new
Proxy({}, {get() {return () => {throw 666}}}));
On 2016/01/11 15:43:21, rossberg wrote:
> Shouldn't the handler have all the traps?

The handler is itself a proxy.

[rossberg, 2016-01-11 16:01:42]

lgtm

https://codereview.chromium.org/1573143002/diff/20001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1573143002/diff/20001/src/messages.h#newcode260
src/messages.h:260: T(ProxyPrivate, "Cannot not pass private property name to
proxy trap")       \
Too many negations, dude.

[neis, 2016-01-11 16:22:28]

The CQ bit was checked by neis@chromium.org

[neis, 2016-01-11 16:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1573143002/#ps40001
(title: "Typo")

[commit-bot: I haz the power, 2016-01-11 16:22:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1573143002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1573143002/40001

[commit-bot: I haz the power, 2016-01-11 16:47:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-11 16:47:33]

Description was changed from

==========
Do not leak private property names to proxy traps and interceptors.

R=rossberg
BUG=
==========

to

==========
Do not leak private property names to proxy traps and interceptors.

R=rossberg
BUG=

Committed: https://crrev.com/2b352bb84f0f2d21997c18cd361b95e905c39400
Cr-Commit-Position: refs/heads/master@{#33212}
==========

[commit-bot: I haz the power, 2016-01-11 16:47:34]

Patchset 3 (id:??) landed as
https://crrev.com/2b352bb84f0f2d21997c18cd361b95e905c39400
Cr-Commit-Position: refs/heads/master@{#33212}

[Jakob Kummerow, 2016-01-11 17:40:26]

jkummerow@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Jakob Kummerow, 2016-01-11 17:40:27]

DBC.

Wouldn't it be easier to put the checks into {Get,Set,Has,Delete}Private() in
the API? AFAICS those are the only possible entry points for private symbols
into the system. (The unit test in this CL isn't representative, because as soon
as there's a "var symbol" on the JS side that contains a private symbol you've
lost anyway and all bets are off, with or without Proxy traps. So a cctest would
be more appropriate, I think.)

Or am I missing something?

https://codereview.chromium.org/1573143002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1573143002/diff/40001/src/objects.cc#newcode4872
src/objects.cc:4872: if (name->IsPrivate()) return Just(false);
According to the DCHECK above, this can never be the case here.

[neis, 2016-01-13 09:03:35]

On 2016/01/11 17:40:27, Jakob wrote:
> DBC.
> 
> Wouldn't it be easier to put the checks into {Get,Set,Has,Delete}Private() in
> the API? AFAICS those are the only possible entry points for private symbols
> into the system. (The unit test in this CL isn't representative, because as
soon
> as there's a "var symbol" on the JS side that contains a private symbol you've
> lost anyway and all bets are off, with or without Proxy traps. So a cctest
would
> be more appropriate, I think.)
> 
> Or am I missing something?

As far as I understand, private symbols are also used internally e.g. in the
implementation of iterators (see testGetInternalIterators() in
mjsunit/harmony/proxies-get.js).


> https://codereview.chromium.org/1573143002/diff/40001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/1573143002/diff/40001/src/objects.cc#newcode4872
> src/objects.cc:4872: if (name->IsPrivate()) return Just(false);
> According to the DCHECK above, this can never be the case here.

Yep, thanks.