Closed
Bug 965901
Opened 11 years ago
Closed 11 years ago
Introduce an ENUMERATE wrapper action
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files)
35.79 KB,
patch
|
gkrizsanits
:
review+
mrbkap
:
superreview+
|
Details | Diff | Splinter Review |
16.57 KB,
patch
|
gkrizsanits
:
review+
mrbkap
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8371729 -
Flags: review?(mrbkap)
Attachment #8371729 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8371730 -
Flags: review?(mrbkap)
Attachment #8371730 -
Flags: review?(gkrizsanits)
Assignee | ||
Updated•11 years ago
|
Attachment #8371729 -
Flags: review?(mrbkap) → superreview?(mrbkap)
Assignee | ||
Updated•11 years ago
|
Attachment #8371730 -
Flags: review?(mrbkap) → superreview?(mrbkap)
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...
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.
Updated•11 years ago
|
Attachment #8371729 -
Flags: review?(gkrizsanits) → review+
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...
Attachment #8371730 -
Flags: review?(gkrizsanits) → review+
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.
Updated•11 years ago
|
Attachment #8371729 -
Flags: superreview?(mrbkap) → superreview+
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)?
Attachment #8371730 -
Flags: superreview?(mrbkap) → superreview+
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__.
Comment 11•11 years ago
|
||
Oops, right.
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Followup windows bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a54af35040
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 :)
https://hg.mozilla.org/mozilla-central/rev/ada4e3d61c3b
https://hg.mozilla.org/mozilla-central/rev/804ed7a7eb9f
https://hg.mozilla.org/mozilla-central/rev/a1a54af35040
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•