links in nested frame fail, give a false NS_ERROR_DOM_PROP_ACCESS_DENIED

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
major
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Magnus Melin, Assigned: jst)

Tracking

({fixed-aviary1.0, fixed1.7, regression})

Other Branch
x86
All
fixed-aviary1.0, fixed1.7, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

14 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.
(Reporter)

Updated

14 years ago
OS: Linux → All
(Reporter)

Comment 1

14 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.
Component: General → DOM: HTML
Product: Firefox → Browser
Version: unspecified → Other Branch
(Assignee)

Comment 2

14 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.
Assignee: firefox → jst
Keywords: regression
(Assignee)

Updated

14 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

14 years ago
Created attachment 151410 [details] [diff] [review]
Trunk fix.

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... :-)
(Assignee)

Updated

14 years ago
Attachment #151410 - Flags: superreview?(darin)
Attachment #151410 - Flags: review?(dveditz)
(Assignee)

Updated

14 years ago
Flags: blocking1.7.1?
(Assignee)

Updated

14 years ago
Attachment #151410 - Flags: approval1.7.1?
(Assignee)

Comment 4

14 years ago
For the record, this was caused by bug 246448.

Comment 5

14 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
Attachment #151410 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 6

14 years ago
Created 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.

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.
(Assignee)

Updated

14 years ago
Attachment #151410 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #151410 - Flags: superreview+
Attachment #151410 - Flags: review?(dveditz)
Attachment #151410 - Flags: approval1.7.1?
(Assignee)

Updated

14 years ago
Attachment #151493 - Flags: superreview?(darin)
Attachment #151493 - Flags: review?(dveditz)
Attachment #151493 - Flags: approval1.7.1?
(Assignee)

Comment 7

14 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.
nominating for the aviary branch
Whiteboard: needed-aviary1.0?
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.
Attachment #151493 - Flags: review?(dveditz) → review+

Comment 10

14 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.
Attachment #151493 - Flags: superreview?(darin) → superreview+

Updated

14 years ago
Blocks: 248318
(Assignee)

Comment 11

14 years ago
Fix checked in on the trunk.
(Assignee)

Comment 12

14 years ago
*** Bug 248318 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

14 years ago
Created attachment 151543 [details] [diff] [review]
Same thing for the aviary/1.7 branch.

Comment 14

14 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
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
Attachment #151493 - Flags: approval1.7.1? → approval1.7.1+
(Assignee)

Comment 16

14 years ago
Fixed on the trunk, 1.7 branch, and aviary branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed-aviary1.0, fixed1.7
Resolution: --- → FIXED
Whiteboard: needed-aviary1.0?
(Reporter)

Comment 17

14 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.

Comment 18

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

Comment 19

14 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).
(Reporter)

Comment 20

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

Comment 21

14 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?)
(Reporter)

Comment 22

14 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). 
(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.)
(Assignee)

Updated

14 years ago
Flags: blocking1.7.x?

Comment 24

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

Comment 25

14 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
(Reporter)

Comment 26

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

Comment 27

13 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

Updated

10 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: general → general
You need to log in before you can comment on or make changes to this bug.