Magnus Melin [:mkmelin] Reporter
	Description • 21 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040608 MultiZilla/1.6.3.0e
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040608 MultiZilla/1.6.3.0e

On the given URL nested frames are used. (One frame contains another framed
page.) If you open the the inner frame URL http://www.hut.fi/~mkmelin/vn/
directly the links will work.

Reproducible: Always
Steps to Reproduce:
On the given URL, click on any of the left column navigation links. They won't
open, and JavaScript console outputs

Error: uncaught exception: [Exception... "Access to property denied"  code:
"1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)"  location:
"http://www.hut.fi/~mkmelin/vn/menu.html Line: 39"]

Actual Results:  
The links should work. 

Expected Results:  
Links whould work, as they do in moz (up to) 1.7rc3.

This problem is present in firefox 0.9, also in trunk builds 20040615 of both
the suite and firefox. 

Yes, I know about same origin policy, but this should not be an issue here.

	Magnus Melin [:mkmelin] Reporter
	Comment 1 • 21 years ago

This is also present in 1.7 final, but not in 1.7rc3, tested both linux and
windows. I think this is pretty bad, as not being able to follow links severely
cripples the browser.

	Johnny Stenback (:jst) Assignee
	Comment 2 • 21 years ago

This is not as designed, this happened due to a problem with the last-minute fix
for bug 246923, the change was too restrictive. I'm working on a fix for this.

A workaround for this particular problem might be to redirect
http://www.helsinki.fi/jarj/vasa/ to http://www.hut.fi/~mkmelin/vn/. That way,
the whole frameset is from the same origin, and your links would work again.

	Johnny Stenback (:jst) Assignee
	Comment 3 • 21 years ago

This makes the criteria for whether we permit a load from a link click or
targetted form submission the same as if the load comes from JS, something that
should've been done in the initial change that caused this bug, but didn't
happen due to time pressure.

This also fixes the bug that caused this particular bug, which was that the
code that compared if the running JS came from a docshell within the targets
root docshell didn't get the root from the docshell where the JS came from, and
compared the actual docshell that the JS was running in to the root of the
target, and not the root of the docshell where the JS was running.

This is somewhat scary in that this code now sometimes passes non-nsIPrincipal
objects as the owner of a URI load. I've digged through the code and tried to
make sure that the owner is reset to the principal where needed, but it's still
not pretty. But hey, this is docshell code, so... :-)

	Johnny Stenback (:jst) Assignee
	Comment 4 • 21 years ago

For the record, this was caused by bug 246448.

	Darin Fisher
	Comment 5 • 21 years ago

Comment on attachment 151410 [details] [diff] [review]
Trunk fix.

This patch makes sense to me.  It's a hefty change to the docshell code, but it
feels like the right thing to do.  The biggest risk I guess is what code
outside of docshell will do with the nsIChannel::owner attribute.


>Index: docshell/base/nsDocShell.cpp

>+        nsIDocShellTreeItem *tmp = item;
>+        item->GetSameTypeParent(&tmp);
>+        item.swap(tmp);
>+    } while (item);

nit: looks to me as though you do not need to initialize |tmp|
since GetSameTypeParent sets it to NULL on entry to the method.


>+nsDocShell::GetCurrentDocumentPrincipal(nsIPrincipal** aPrincipal)
...
>+        docViewer->GetDocument(getter_AddRefs(document));

nit: what happens if this returns a null document?  (looks like
the old code did not have any null checks or |rv| checks either.)


>+    *aPrincipal = document->GetPrincipal();
>+    NS_IF_ADDREF(*aPrincipal);
> }

looks like we could crash.  maybe add an ASSERTION if this should
never happen.


sr=darin

	Johnny Stenback (:jst) Assignee
	Comment 6 • 21 years ago

Dveditz and I found out that there's already code in the docshell that checks
wether a targetted load should be permitted or not, that change went in as the
fix for bug 13871 in late 2001, only to be disabled by some code reordering 6
months later (bug 65777).

This patch brings the original check back to life, but that check didn't fix
the case where JS tried to change the location of a frame within a frameset it
didn't own. This fixes that as well.

The changes to nsWebShell are merely a backout of the initial changes that
caused this bug, and now that we figured out a better way to prevent (or
redirect to a new window, rather) targetted links and form submissions, the JS
part got much simpler, and no more scary owner-is-not-always-an-nsIPrincipal
changes.

	Johnny Stenback (:jst) Assignee
	Comment 7 • 21 years ago

I forgot to add the null check for document in
nsDocShell::GetCurrentDocumentOwner() that darin mentioned. I've added that
locally, so that's now crash safe.

	Daniel Veditz [:dveditz]
	Comment 8 • 21 years ago

nominating for the aviary branch

	Daniel Veditz [:dveditz]
	Comment 9 • 21 years ago

Comment on attachment 151493 [details] [diff] [review]
Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

>Index: docshell/base/nsDocShell.cpp
>+                    nsIDocShellTreeItem *t = nsnull;
>+                    tmp->GetSameTypeParent(&t);
>+                    tmp.swap(t);

This leaks, t should be a nsCOMPtr<>

>+        nsIDocShellTreeItem *tmp = item;
>+        item->GetSameTypeParent(&tmp);
>+        item.swap(tmp);

Same here. (Not sure why you bother to initialize *tmp.)

>+    // The caller is not from the same origin as this item, or any if
>+    // this items ancestors. Only permit loading content if both are
>+    // part of the same window, assuming we can find the window of the
>+    // caller.
> 
>     nsCOMPtr<nsIJSContextStack> stack =
>         do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>     if (!stack) {
>         return rv;
>     }

What's rv at this point? I guess it's some sort of error from the call to
CheckSameOriginPrincipal (else you'd've returned already) but you should
be explicit that you're blocking the load with an error. Otherwise it risks
someone adding code that sets rv in between.

Or use the rv-setting form of do_GetService() and check rv instead of stack.

>     JSContext *cx = nsnull;
>     stack->Peek(&cx);
> 
>     if (!cx) {
>         // No caller docshell reachable, disallow load.
> 
>         return rv;

ditto here (sorry I didn't catch these in bug 246448).

r=dveditz with those changes.

	Darin Fisher
	Comment 10 • 21 years ago

Comment on attachment 151493 [details] [diff] [review]
Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

sr=darin with dveditz's fixes.

	Johnny Stenback (:jst) Assignee
	Comment 11 • 21 years ago

Fix checked in on the trunk.

	Johnny Stenback (:jst) Assignee
	Comment 12 • 21 years ago

*** Bug 248318 has been marked as a duplicate of this bug. ***

	Hermann Schwab
	Comment 14 • 21 years ago

I assume this bugfix caused regression bug 248753 on Mozilla, 2004062309
working, 2004062323 failing, Firefox 2004062408 failing.

Instead of opening a popup and loading an image into it, an empty popup is
created, a new window is created, and the image loaded in the new window.
testcase: http://bugzilla.mozilla.org/attachment.cgi?id=151784&action=view

	Daniel Veditz [:dveditz]
	Comment 15 • 21 years ago

Comment on attachment 151493 [details] [diff] [review]
Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

a=dveditz for 1.7.1

	Johnny Stenback (:jst) Assignee
	Comment 16 • 21 years ago

Fixed on the trunk, 1.7 branch, and aviary branch.

	Magnus Melin [:mkmelin] Reporter
	Comment 17 • 21 years ago

Changing the URL, since I put in a workaround to break out of inner frames for
June builds in cases "debug" parameter is not given. With debug parameter it
functions as before.

	Luis Alberto
	Comment 18 • 21 years ago

*** Bug 246844 has been marked as a duplicate of this bug. ***

	Stig Nygaard
	Comment 19 • 20 years ago

As far as I remember this was fixed on the 1.7.x branch already *before* the
1.7.1 release, but the fix doesn't seems to be included in either 1.7.1, 1.7.2
or 1.7.3 (or Firefox 1.0PR1). Why? I've been running nightly builds from the
1.7.x branch the latest months because this bug prevent me from using some sites
I visit regulary. I haven't found any problems with these branch-nightlies, but
are there some major regression known with this fix which prevents it from being
included in the actually release-versions?
BTW. I know 1.7.1 and 1.7.2 was rushed out very quickly due to security bugs, so
maybe the included fixes in this builds were choosen very conservative, but I've
got the impression thar 1.7.3 (and Firefox 1.0PR1) were planned in a long time.

Anyway, this bug is marked fixed, but ain't fixed in any official release even
though there has been quite a few releases since the bug was fixed (and even
though it has been fixed in the 1.7.x branch nightlies in a long time).

	Magnus Melin [:mkmelin] Reporter
	Comment 20 • 20 years ago

Works fine in Firefox 1.0PR1. In 1.7.1-3 there were only security fixes.

	Stig Nygaard
	Comment 21 • 20 years ago

#20, Magnus

You're right, it DOES actually works in Firefox PR1. I would have sworn it
didn't last time I tried. Sorry for the spam, and thanks for the quick respons:-)

(BTW, shouldn't the checkins for 1.7.3 and Firefox 1.0PR be pretty much
synchronized? Or it's only the 1.7.x nightlies which are?)

	Magnus Melin [:mkmelin] Reporter
	Comment 22 • 20 years ago

Firefox 1.0PR is made of the aviary branch. Things fixed on the aviary branch
don't automatically get fixed on the 1.7 branch (and the other way around). 

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 23 • 20 years ago

(Perhaps confusingly, 1.7.[123] weren't cut from the 1_7_BRANCH, but instead
from the 1.7(.0) release minibranch, based only on the security fixes.  An
upcoming 1.7.x release will feature all these good bits, and align very tightly
with firefox's 1.0.)

	Michael Lefevre
	Comment 24 • 20 years ago

*** Bug 247016 has been marked as a duplicate of this bug. ***

	Ion CIONCA
	Comment 25 • 20 years ago

(In reply to comment #19)
...
> Anyway, this bug is marked fixed, but ain't fixed in any official release even
> though there has been quite a few releases since the bug was fixed (and even
> though it has been fixed in the 1.7.x branch nightlies in a long time).

Any news concerning this "bug" ? fixed or not yet ? 

The Swiss Post refuses to support Mozilla and Firefox for their on-line credit
card paiment because of this "bug"

Thnx

Ion CIONCA
__________
Ion CIONCA, web & database development
DIT/KIS (Knowledge Information Services), EPFL, 1015 Lausanne, Switzerland
phone: (+41) 21 6934586, fax: (+41) 21 6932220
mailto:Ion.Cionca@epfl.ch, http://kis.epfl.ch/page44412.html

	Magnus Melin [:mkmelin] Reporter
	Comment 26 • 20 years ago

It's fixed. The fix is in Moz 1.7.5 or firefox 1.0 and up. 

	Fabian
	Comment 27 • 20 years ago

In reply to comments #25 #26:

it ist still not possible to use the Swiss Post on-line credit card payment.
Firefox browser (Debian Linux) 1.0.4 can't navigate back to main window after 
having finished payment in popup window.

They say it does not work because of this bug since firefox 0.9 and mozilla 1.7:
<http://bugzilla.mozilla.org/show_bug.cgi?id=246923>

So I tried firefox 0.8 und mozilla 1.7: does not work either.

Regards,

Fabian Lienert
fabian@lienert.org