1.64 KB, application/vnd.mozilla.xul+xml
16.76 KB, patch
|Details | Diff | Splinter Review|
16.44 KB, patch
|Details | Diff | Splinter Review|
From bug 326877 comment 72, user content can generate menu popups that float over all windows and can be positioned at arbitrary window positions. This allows spoofing things such as CST's BSOD testcase https://bugzilla.mozilla.org/attachment.cgi?id=253104 and the phisher's favorite, covering up the URL-bar, http://ctho.ath.cx/tmp/moz/326877evil2.xul from bug 326877 comment 76 We need to restrict non-privileged popups to the area of the content window.
Do you mean just adjust the size of popups to the rectangle of the parent document (by resizing the popup), or make the popups work more like absolute positioned items that are just overlaid into the page (such that one would need to scroll if the popup is off the border)?
I mean adjust the size and position of the popup to the parent document, it can still be a popup (I saw your "in page" Mac patch in bug 326877 and I think people who use popups would rebel). We'd also have to add position checks if popups can be moved--can they? I had a partially working patch from bug 326877, I'll try to find and attach it.
OK, that's actualy easier to implement.
Created attachment 259238 [details] [diff] [review] This seems to work Not sure if GetScreenRect is right here for xul documents, but it seems to work. Want to test it a bit more before it gets reviewed. It constrains popups from content to the window area.
Created attachment 259240 [details] testcase This testcase shows some menus and moves and resizes them when the mouse is moved over them. The patch currently just prevents popups from being moved, but resizing should be constrained to the window.
Comment on attachment 259238 [details] [diff] [review] This seems to work I tested this some more and it seems to work
+ !GetStyleDisplay()->mAppearance && !mInContentShell; Do we need this check anymore? Seems to me that with this patch transparent popups are OK, right? What if someone displays a popup and then moves the window away? The popup stays where it is, right? That seems like a problem.
I suppose what should happen if the window is moved is that the popups should be closed, perhaps by calling DocumentViewerImpl::HideViewIfPopup Do we need to be concerned only when a script moves the window, or when the user does? Note also that the popup will shuffle position back to be constrained within the frame area anyway when it gets reflowed.
I'm not at all concerned if the user manually moves the window, but wouldn't popups normally close when the user clicks on the window to drag it anyway?
Yes, it should do, although only the one that the widget system is keeping track of.
I'm primarily concerned about the script moving the window, right.
Created attachment 260587 [details] [diff] [review] this version closes popups when the parent window is moved
+ !GetStyleDisplay()->mAppearance && !mInContentShell; We can allow transparent popups for content now, right? On trunk, instead of HideViewIfPopup, how about using nsPresContext::GetActivePopups, or whatever the equivalent is after the popups rework? I guess we can land this first if you promise to fix that later.
Comment on attachment 260587 [details] [diff] [review] this version closes popups when the parent window is moved OK, so this patch is acceptable as-is.
(In reply to comment #13) > + !GetStyleDisplay()->mAppearance && !mInContentShell; > > We can allow transparent popups for content now, right? > Don't know. I didn't follow the implementing of transparent popups. > On trunk, instead of HideViewIfPopup, how about using > nsPresContext::GetActivePopups, or whatever the equivalent is after the popups > rework? I guess we can land this first if you promise to fix that later. The list in GetActivePopups doesn't contain all the popups. Once the popup rework is done, HideViewIfPopup won't be needed either so it won't need to mess about with views.
dveditz, does this patch fix what's needed?
Finally got a chance to test and it addresses the cases I was worried about. The testcase attachment 259240 [details] doesn't open menus "up" in a small window as advertised by the text in the page, dunno if that's anything to worry about from a regression POV.
> The testcase attachment 259240 [details] doesn't open menus "up" in a small window as > advertised by the text in the page, dunno if that's anything to worry about > from a regression POV. > Which text says this? Maybe you linked to the wrong attachment?
Attachment 259240 [details], which is the second attachment in this bug ("testcase"), says "Try shrinking the window down and opening either of the three menus. They should move to open upwards instead of down if there is room, otherwise shrink to the frame size." I'm happy with what they're doing post-patch, just noting that nothing goes "up" no matter how small I make the window. Maybe the testcase description is wrong.
What kind of interface changes are allowed for the branch now? There's a change in nsIPresShell in this patch.
I *think* changing nsIPresShell is OK because it's not an .idl or otherwise available externally. We haven't made promises about internal layout interfaces as far as I know. But we should double-check that with one of the layout owners like roc or dbaron.
Absent approval to change nsIPresShell on the branch (and dbaron was not sure) you could add the new function to a nsIPresShell_MOZILLA_1_8_BRANCH interface.
I think we have avoided changing nsIPresShell on the branch because extensions can (and I think do) use it.
There actually already is a nsIPresShell_MOZILLA_1_8_BRANCH interface, should this just be modified or should yet another interface be added? Why are extensions using nsIPresShell? It's an internal non-scriptable interface.
I think we can modify nsIPresShell_MOZILLA_1_8_BRANCH. Maybe it's embedders that use it, not extensions. I'm pretty sure gnome-web-photo and Epiphany use it.
Created attachment 262652 [details] [diff] [review] branch patch
The branch patch has the same missing return as the trunk one: see bug 378827. Fix that when you land?
Comment on attachment 262652 [details] [diff] [review] branch patch approved for 220.127.116.11 and 18.104.22.168, a=dveditz for release-drivers
I don't know if I get it right. But if I go to http://ctho.ath.cx/tmp/moz/326877evil2.xul I get a bar that tries to cover the url-bar. Now this has been patched and it just comes on the screen. But is it normal that if I go to another tab, it still shows the bar? Isn't that a possible way to fake a login screen? 1) load an xul showing a login screen (like the one of gmail) 2) open gmail in a new tab 3) The login screen of the XUL is on top of the gmail one and has the same look. Nobody expects a fake one?!!!!
It's also the case that: 1) goto http://ctho.ath.cx/tmp/moz/326877evil2.xul - pop-up appears in window 2) click back button - back to previous page, with no pop-up 3) forward button (ie: return to pop-up page) - there is now no pop-up window The pop-up returns if you click on the window (well, in the main part of the window - it doesn't return if you clock in the entry box at the bottom). I can't see any security issue in this at the moment, just reporting and oddity.