Closed
Bug 246923
Opened 21 years ago
Closed 21 years ago
links in nested frame fail, give a false NS_ERROR_DOM_PROP_ACCESS_DENIED
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mkmelin, Assigned: jst)
References
()
Details
(Keywords: fixed-aviary1.0, fixed1.7, regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
dveditz
:
review+
darin.moz
:
superreview+
dveditz
:
approval1.7.5+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•21 years ago
|
OS: Linux → All
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.
Component: General → DOM: HTML
Product: Firefox → Browser
Version: unspecified → Other Branch
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.
Assignee: firefox → jst
Keywords: regression
Assignee | ||
Updated•21 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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... :-)
Assignee | ||
Updated•21 years ago
|
Attachment #151410 -
Flags: superreview?(darin)
Attachment #151410 -
Flags: review?(dveditz)
Assignee | ||
Updated•21 years ago
|
Flags: blocking1.7.1?
Assignee | ||
Updated•21 years ago
|
Attachment #151410 -
Flags: approval1.7.1?
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
Attachment #151410 -
Flags: superreview?(darin) → superreview+
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.
Assignee | ||
Updated•21 years ago
|
Attachment #151410 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #151410 -
Flags: superreview+
Attachment #151410 -
Flags: review?(dveditz)
Attachment #151410 -
Flags: approval1.7.1?
Assignee | ||
Updated•21 years ago
|
Attachment #151493 -
Flags: superreview?(darin)
Attachment #151493 -
Flags: review?(dveditz)
Attachment #151493 -
Flags: approval1.7.1?
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.
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.
Attachment #151493 -
Flags: review?(dveditz) → review+
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.
Attachment #151493 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 13•21 years ago
|
||
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
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
Attachment #151493 -
Flags: approval1.7.1? → approval1.7.1+
Assignee | ||
Comment 16•21 years ago
|
||
Fixed on the trunk, 1.7 branch, and aviary branch.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Keywords: fixed-aviary1.0,
fixed1.7
Resolution: --- → FIXED
Whiteboard: needed-aviary1.0?
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.
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).
Reporter | ||
Comment 20•20 years ago
|
||
Works fine in Firefox 1.0PR1. In 1.7.1-3 there were only security fixes.
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?)
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).
(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•20 years ago
|
Flags: blocking1.7.x?
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
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
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.
Description
•