Closed
Bug 497780
Opened 14 years ago
Closed 14 years ago
onmouseout's MouseEvent.relatedTarget is a chrome element and is completely inaccessible.
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.1 | --- | ? |
People
(Reporter: knorton, Assigned: smaug)
References
()
Details
(Keywords: regression, testcase)
Attachments
(1 file, 4 obsolete files)
5.25 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_7; en-us) AppleWebKit/531.0+ (KHTML, like Gecko) Version/4.0 Safari/530.17 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 Wiring up an onmouseout to an element with scrollbar, the MouseEvent that is delivered when you move the mouse off of the scroll bar has a chrome element as the relatedTarget. Accessing any property on that element throws "Permission denied to access property 'nodeName' from non-chrome context" The element (Firebug is able to display it) is one of <thumb/> or <slider/>. Reproducible: Always Steps to Reproduce: 1. Go to the URL. 2. Mouse over the scrollbar, then mouseout. Actual Results: An error is thrown when the onmouseout callback tries to execute 'event.relatedTarget.nodeName' Expected Results: The relatedTarget should be a accessible element from the pages DOM.
Comment 1•14 years ago
|
||
(On windows at least) I see it only when I move my mouse above the scrollbar, and then the regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1a1611bb1063&tochange=e167d6ca2023
Component: General → DOM
Keywords: regression,
testcase
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1?
Assignee | ||
Comment 2•14 years ago
|
||
Assignee | ||
Comment 3•14 years ago
|
||
We do retargeting for .target, but not for .relatedTarget. When mouse moves from native anon to non-native anon, mouseover/out messages are suppressed. So as far as I see, .relatedTarget is the problematic case with mouse events.
Assignee | ||
Comment 4•14 years ago
|
||
One thing I'm not sure yet is what happens with videocontrols.xml. It uses .relatedTarget.
Assignee | ||
Comment 5•14 years ago
|
||
when using the patch and moving mouse over videocontrols does cause .relatedTarget retargeting - as I excepted. Blake, is there some easy way to detect if caller JS can access native anon content? If yes, that could be used and not nsContentUtils::IsCallerChrome() If not, we could add .mozOriginalRelatedTarget. (I know, that is an ugly solution.)
Assignee | ||
Comment 6•14 years ago
|
||
Maybe something like this.
Assignee | ||
Comment 7•14 years ago
|
||
Comment on attachment 382938 [details] [diff] [review] better Could we do this, especially for branch. Using .relatedTarget is common. Though, I expect that it doesn't work too well always with FF3.0 either.
Attachment #382938 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•14 years ago
|
||
Tryserver builds will appear here https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_2/
![]() |
||
Comment 9•14 years ago
|
||
So this is a duplicate of bug 101197, right?
![]() |
||
Comment 10•14 years ago
|
||
The patch doesn't make sense to me. Why not just check for the capability and then if missing look for chrome:// or some such? The capability check is wrong, since it needs to walk up the stack...
Comment 11•14 years ago
|
||
Don't see any reason why we would block release on this; feel free to renominate with rationale
Flags: wanted1.9.1.x?
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #10) > The patch doesn't make sense to me. I copied the check from bug 475864. IMHO the checks should be the same for the wrapper and for retargeting.
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #9) > So this is a duplicate of bug 101197, right? Sort of. Nowadays we throw an exception in a bit different place.
![]() |
||
Comment 14•14 years ago
|
||
> I copied the check from bug 475864. Mmm... yeah. That check doesn't make sense to me either: it requires an enablePrivilege in the function doing the access, not just in any caller. How come? I agree that the code should match (which to me implies it should either use the existing secman API or be refactored in some other way; copy-paste is not the right way to get things to match, imo). > Sort of. Nowadays we throw an exception in a bit different place. Sure; the real bug is that we hand out native anon content via relatedTarget in the first place.
Reporter | ||
Comment 15•14 years ago
|
||
(In reply to comment #11) > Don't see any reason why we would block release on this; feel free to > renominate with rationale FWIW, I filed this because we're getting reports in GWT of frontends throwing exceptions in ff3.5b4. It's a hard bug to work around since there already exists a lot of code that gets a reference to relatedTarget and then calls other DOM related functions with it. Adding pessimistic checks everywhere in our DOM library to guard against the possibility of a toxic element (one that denies access to every property) would be a horrible outcome and would also be a pretty sad performance/size hit for Firefox users. I have to imagine that this effects other libraries as well since most of them make use of relatedTarget.
![]() |
||
Comment 16•14 years ago
|
||
All those cases should also throw in Fx3.0, no? We certainly hand back native anon relatedTarget there, and don't have quickstubs to make it sometimes work...
Assignee | ||
Comment 17•14 years ago
|
||
On FF3.0 we throw later, when using native anonymous element.
![]() |
||
Comment 18•14 years ago
|
||
Ah, so if you just pass it to some DOM method (as opposed to trying to get props off it) it works? That really sucks, actually. :(
Comment 19•14 years ago
|
||
Only if the caller of the DOM method passes the security check -- see XPCWrapper::Unwrap (and XPCWrapper::UnwrapSOW; in particular, the call to AllowedToAct).
Updated•14 years ago
|
Attachment #382938 -
Flags: review?(mrbkap) → review-
Comment 20•14 years ago
|
||
Comment on attachment 382938 [details] [diff] [review] better >+PRBool >+nsContentUtils::CanAccessNativeAnon(nsIContent* aContent) >+{ >+ NS_ASSERTION(aContent && aContent->IsInNativeAnonymousSubtree(), >+ "Fix the caller!"); >+ JSContext* cx = nsnull; >+ sThreadJSContextStack->Peek(&cx); >+ if (!cx) { >+ return PR_TRUE; >+ } >+ JSStackFrame* fp; >+ nsIPrincipal* principal = >+ sSecurityManager->GetCxSubjectPrincipalAndFrame(cx, &fp); >+ NS_ENSURE_TRUE(principal, PR_FALSE); >+ void* annotation = JS_GetFrameAnnotation(cx, fp); >+ PRBool privileged; >+ if (fp && >+ NS_SUCCEEDED(principal->IsCapabilityEnabled("UniversalXPConnect", >+ annotation, >+ &privileged)) && >+ privileged) { >+ // UniversalXPConnect things are allowed to touch us. >+ return PR_TRUE; >+ } This differs subtly from the code in XPCSystemOnlyWrapper.cpp. In particular, its handling of no JS code running and only native code running differs. Also, don't you need to null check fp here? Other than that, I can't come up with a better solution here. My only thought would be to have the "system only wrappers" do the re-targeting done here if they notice invalid access. That would make them less general purpose, but would centralize the re-targeting. Not sure if that's necessarily "better", though.
Comment 21•14 years ago
|
||
Any update on this issue? Will it be fixed before the final release?
Assignee | ||
Comment 22•14 years ago
|
||
because I'm hoping to get this to 1.9.1.x, I didn't move the code to script security manager (just copied it). That could be done later on trunk.
Assignee: nobody → Olli.Pettay
Attachment #385345 -
Flags: review?(mrbkap)
Assignee | ||
Comment 23•14 years ago
|
||
https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-relatedTarget_3/
Updated•14 years ago
|
Attachment #385345 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 24•14 years ago
|
||
Comment on attachment 385345 [details] [diff] [review] better Bz, do you think this would be ok for the branch?
Attachment #385345 -
Flags: superreview?(bzbarsky)
![]() |
||
Updated•14 years ago
|
Attachment #385345 -
Flags: superreview?(bzbarsky) → superreview+
![]() |
||
Comment 25•14 years ago
|
||
Comment on attachment 385345 [details] [diff] [review] better The new function needs documentation. Its aContent argument is unused; get rid of it. Why "chrome://global/" and not just "chrome://"? Seems weird to lock extensions out here... is that on purpose? With those nits, looks ok. Not sure about branch yet; I _think_ it should be safe, but let's get trunk baking and security banging on it first.
Assignee | ||
Comment 26•14 years ago
|
||
> Why "chrome://global/" and not just "chrome://"? Seems weird to lock
> extensions out here... is that on purpose?
Because that is what XPCSystemOnlyWrapper does.
![]() |
||
Comment 27•14 years ago
|
||
Yeah, ok. Add comments here and in that code that the two must match?
Assignee | ||
Comment 28•14 years ago
|
||
Well, aContent was used for the assertion, but sure, not really needed.
Attachment #382932 -
Attachment is obsolete: true
Attachment #382938 -
Attachment is obsolete: true
Attachment #385345 -
Attachment is obsolete: true
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #385450 -
Attachment is obsolete: true
Assignee | ||
Comment 30•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bfa072ef0065
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Attachment #385451 -
Flags: approval1.9.1.1?
Comment 31•14 years ago
|
||
Comment on attachment 385451 [details] [diff] [review] +comments Has the security banging referred to in comment 25 happened? If so, please post results and renominate for branch.
Attachment #385451 -
Flags: approval1.9.1.1?
Updated•14 years ago
|
status1.9.1:
--- → ?
Flags: wanted1.9.1.x?
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•