Closed Bug 862380 Opened 11 years ago Closed 11 years ago

Enumerating cross-origin objects shouldn't throw

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(4 files, 1 obsolete file)

By my reading of the spec, enumerating cross-origin objects shouldn't throw, and we should instead return the appropriate properties, with the non-cross-origin-accessible properties filtered out. We have the capability to do that in FilteringWrapper, but currently just throw in any situations where we try to invoke traps like |enumerate| or |keys|.

If this is the spec behavior, I'm happy to try to align with it. Hixie, can you confirm that this is the case?
For Window, Document, and Location (I think those are the only objects you can access cross-origin without having once been same-origin), that sounds right. I don't think the spec actually documents this right now, though. What do browsers do?
(In reply to Hixie (not reading bugmail) from comment #1)
> For Window, Document, and Location (I think those are the only objects you
> can access cross-origin without having once been same-origin), that sounds
> right. I don't think the spec actually documents this right now, though.
> What do browsers do?

It appears that Safari and Chrome don't throw, whereas IE9 and Gecko do. I haven't tested IE10.
Do they enumerate anything?

I really don't mind what we spec here, so long as we're consistent. Throwing seems unhelpful, but an argument could be made that enumerating frame names is a security risk, so we probably shouldn't do that even if we allow access to the names if you need them? I don't know.
(In reply to Hixie (not reading bugmail) from comment #3)
> Do they enumerate anything?

It looks like they don't, and Object.getOwnPropertyNames and such provide arrays of length zero.
> I really don't mind what we spec here, so long as we're consistent. Throwing
> seems unhelpful, but an argument could be made that enumerating frame names
> is a security risk, so we probably shouldn't do that even if we allow access
> to the names if you need them?

Yeah, I think it's safest not to enumerate them. I'm going to upload some patches to align with Chrome/Safari here, and file a spec bug.
We already do this, but it's helpful to be clear about it.
Attachment #751996 - Flags: review?(mrbkap)
Attached patch Part 4 - Tests. v1 (obsolete) — Splinter Review
Attachment #751998 - Flags: review?(mrbkap)
Looks green.
Attachment #751995 - Flags: review?(mrbkap) → review+
Comment on attachment 751996 [details] [diff] [review]
Part 2 - Be more explicit about rejecting JSID_VOID for XOWs. v1

Ironically, I had to look up the meaning of EIBTI.
Attachment #751996 - Flags: review?(mrbkap) → review+
Attachment #751997 - Flags: review?(mrbkap) → review+
Comment on attachment 751998 [details] [diff] [review]
Part 4 - Tests. v1

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

::: js/xpconnect/tests/mochitest/test_bug862380.html
@@ +23,5 @@
> +      is(Object.getOwnPropertyNames(obj).length, 0, "Object.getOwnPropertyNames gives empty array");
> +      is(Object.keys(obj).length, 0, "Object.keys gives empty array");
> +      for (var i in obj)
> +        ok(false, "Enumerated something: " + i);
> +      ok(!((obj instanceof Window) || (obj instanceof Location)), "Instanceof doesn't do anything");

I was with you until here. Why should instanceof lie? It's pretty easy to figure out what kind of cross-origin object you have your hands on anyway.
Attachment #751998 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #12)
> I was with you until here. Why should instanceof lie? It's pretty easy to
> figure out what kind of cross-origin object you have your hands on anyway.

Ok, sure. I was just treating this as documenting our existing behavior, but you're right that said behavior is wrong. I'll remove that check and fix this in bug 870423.
Attachment #751998 - Attachment is obsolete: true
Attachment #753095 - Flags: review?(mrbkap)
Attachment #753095 - Flags: review?(mrbkap) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: