Bobby Holley (:bholley) Assignee
	Description • 11 years ago

Right now, the enum of actions that we pass to BaseProxyHandler::enter is pretty coarse. There are a number of traps that don't really fit into the GET/SET/CALL model, and so we generally just pass GET and JSID_VOID for those.

While implementing the new cross-origin behavior for bug 965898, I'm finding that I need an explicit trap for enumerate-like operations. While I'm at it, I'm going to beef up the verification machinery so that we also pass an Action to assertEnteredPolicy.

	Bobby Holley (:bholley) Assignee
	Comment 1 • 11 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 2 • 11 years ago

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

	Gabor Krizsanits (INACTIVE)
	Comment 5 • 11 years ago

Looks all sane to me, but shouldn't we use this new enumerate flag also in Filter to call policy with? (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/FilteringWrapper.cpp#37 ) Not sure if it would worth it, just curious about your thoughts on this one...

	Bobby Holley (:bholley) Assignee
	Comment 6 • 11 years ago

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> Looks all sane to me, but shouldn't we use this new enumerate flag also in
> Filter to call policy with?
> (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/
> FilteringWrapper.cpp#37 ) Not sure if it would worth it, just curious about
> your thoughts on this one...

Hm, interesting question.

Thinking about it, I'm pretty sure that it should stay the way it is. The ENUMERATE hook is more of a general question of "can I list properties for this object"? In the FilteringWrapper case, that lets us say "ok, sure, but we'll override those traps so that you only get to see the properties that we would allow you to GET".

In particular, the ENUMERATE hook as it stands here just take JSID_VOID, and I'm not really clear what it would mean to pass something else.

	Gabor Krizsanits (INACTIVE)
	Comment 7 • 11 years ago

Comment on attachment 8371730 [details] [diff] [review]
Part 2 - Add an ENUMERATE policy action. v1

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

(In reply to Bobby Holley (:bholley) from comment #6)
> Thinking about it, I'm pretty sure that it should stay the way it is. The
> ENUMERATE hook is more of a general question of "can I list properties for
> this object"? In the FilteringWrapper case, that lets us say "ok, sure, but
> we'll override those traps so that you only get to see the properties that
> we would allow you to GET".
> 
> In particular, the ENUMERATE hook as it stands here just take JSID_VOID, and
> I'm not really clear what it would mean to pass something else.

What if let's say a property is exposed with flag w? Should that be enumerable?
__exposedProps__ will be gone, and the w flag without an r is not too common...
I guess in the web world there is no such case either, so I guess it does
not worth it...

	Bobby Holley (:bholley) Assignee
	Comment 8 • 11 years ago

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> What if let's say a property is exposed with flag w? Should that be
> enumerable?
> __exposedProps__ will be gone, and the w flag without an r is not too
> common...
> I guess in the web world there is no such case either, so I guess it does
> not worth it...

Yeah, it's all kinda wishy washy. We only have 2 kinds of interesting FilteringWrapper policies: COWs and XOWs. COWs are going away, and XOWs will end up with a special wrapper that custom-handles enumerate hooks and whatnot (in bug 9658980). We might as well leave this infrastructure in place, but it's kinda hard to design it perfectly without use cases.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 9 • 11 years ago

Comment on attachment 8371730 [details] [diff] [review]
Part 2 - Add an ENUMERATE policy action. v1

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

::: js/xpconnect/wrappers/AccessCheck.h
@@ +91,5 @@
>  struct ExposedPropertiesOnly : public Policy {
>      static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act);
>  
>      static bool deny(js::Wrapper::Action act, JS::HandleId id) {
> +        // Fail silently for GETs and ENUMERATEs.

Given that COWs are supposed to be going away anyway, I'm OK with this, but don't we have a test that enumerating a COW iterates the exposed props (and only the exposed props)?

	Bobby Holley (:bholley) Assignee
	Comment 10 • 11 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> Given that COWs are supposed to be going away anyway, I'm OK with this, but
> don't we have a test that enumerating a COW iterates the exposed props (and
> only the exposed props)?

I think you're misreading the code - this is just about what happens in the situation where |check| returns false, and whether we throw or fail silently. In the enumerate case, we'll fail if and only if there's no __exposedProps__.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 11 • 11 years ago

Oops, right.

	Bobby Holley (:bholley) Assignee
	Comment 12 • 11 years ago

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=7a14732b3cb2

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ada4e3d61c3b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/804ed7a7eb9f

	Bobby Holley (:bholley) Assignee
	Comment 13 • 11 years ago

Followup windows bustage fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a54af35040

	Blake Kaplan (:mrbkap) (inactive)
	Comment 14 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #13)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a54af35040

For what it's worth, I prefer the new name :)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/ada4e3d61c3b
https://hg.mozilla.org/mozilla-central/rev/804ed7a7eb9f
https://hg.mozilla.org/mozilla-central/rev/a1a54af35040