contentWindow.location (and href) throws DOM security error

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
DOM
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: James Ross, Assigned: bz)

Tracking

Trunk
mozilla1.8beta2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
Since the latest change in bug 281988, ChatZilla's been having a little problem
with getting the location of its content windows.

To get the error, in any very recent Firefox build, open ChatZilla and do this:
  /eval this.frame.contentWindow.location.href

It seems  /eval this.frame.contentWindow shows that the Window object is being
wrapped by XPCNativeWrapper (which I'm told is perfectly correct), however the
exception is not.
The problem here is that we end up in XPC_NW_NewResolve, decide we need to
delegate to the unwrapped object, go to do the OBJ_DEFINE_PROPERTY() property
thing.  This calls into nsWindowSH::AddProperty, which throws for the "location"
property.  So just getting window.location fails.

So the problem, it seems, is that the 

  // All we need to do is define the property in obj if it exists in
  // the wrapped native's object.

comment isn't what we're doing.  We're trying to define the property in the
wrapped native (because our AddProperty hook just passes things along here).

I tried just skipping the OBJ_DEFINE_PROPERTY call, but that breaks other things...

Perhaps we should consider forwarding to the other class hook first, and if that
doesn't resolve anything doing what we do now?  Or would that not work?
Flags: blocking1.8b2?
OS: Windows 2000 → All
Hardware: PC → All
Blocks: 295040
Created attachment 184257 [details] [diff] [review]
fix?
Not enough... document.location has the same issue.  And the AddProperty hook on
nsNodeSH does weird wrapper-preserve stuff, like I said on IRC.

It really feels like we don't want to be calling AddProperty here when the
property is "already there".  The question is whether we can detect this last case.

I guess we can do this for now if we have no better ideas...
Created attachment 184262 [details] [diff] [review]
Patch, per discussion with brendan

brendan says r+a=him
Attachment #184257 - Attachment is obsolete: true
Attachment #184262 - Flags: review+
Assignee: general → bzbarsky
Target Milestone: --- → mozilla1.8beta2
Fixed.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 6

13 years ago
Verified in trunk Firefox, dated 2005-05-22 23:30:05.76 BST.
Status: RESOLVED → VERIFIED
Created attachment 184290 [details] [diff] [review]
followup fix for correct delete property bypass

I'd like to get this in now, since we know the JSClass.delProperty forwarding
is not useful (delProperty, like addProperty, is a notification callback that
does not actually remove the id'd property).

/be

Updated

13 years ago
Attachment #184290 - Flags: review?(bzbarsky)
Attachment #184290 - Flags: approval1.8b2+
Comment on attachment 184290 [details] [diff] [review]
followup fix for correct delete property bypass

r+sr=jst
Attachment #184290 - Flags: superreview+
Attachment #184290 - Flags: review?(bzbarsky)
Attachment #184290 - Flags: review+
Comment on attachment 184290 [details] [diff] [review]
followup fix for correct delete property bypass

Checked in, thanks.

/be
Flags: blocking1.8b2?
You need to log in before you can comment on or make changes to this bug.