Closed
Bug 304089
Opened 18 years ago
Closed 18 years ago
possible to hide security UI by resizing the browser offscreen via js
Categories
(Core Graveyard :: Widget: Mac, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: jaas, Assigned: mark)
References
Details
(Keywords: fixed1.8, Whiteboard: [sg:spoof])
Attachments
(6 files, 8 obsolete files)
721 bytes,
patch
|
mconnor
:
approval1.8b4+
|
Details | Diff | Splinter Review |
2.90 KB,
patch
|
Details | Diff | Splinter Review | |
4.53 KB,
patch
|
Details | Diff | Splinter Review | |
1.35 KB,
patch
|
Details | Diff | Splinter Review | |
1.64 KB,
patch
|
Details | Diff | Splinter Review | |
18.88 KB,
patch
|
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
Window size constraining is broken on the Mac. window.resizeTo(2000,2000); This will make the bottom of the window go offscreen by something like 20-22 pixels, enough to hide our secure site UI. Sites could quietly resize a full-screen browser window and fake the security UI. (I found this with Jesse Ruderman)
Updated•18 years ago
|
Group: security
Updated•18 years ago
|
Whiteboard: [sg:fix]
For one thing, we shouldn't be setting an initial window height that is taller than the available screen height. This patch fixes that for screens with (screen.availHeight > 600). This patch does not fix the whole bug. That is coming up (I think I've almost got it).
Attachment #192178 -
Flags: review?(mconnor)
Comment 2•18 years ago
|
||
Comment on attachment 192178 [details] [diff] [review] initial window size fix, v1.0 This is why resizing is evil, but ok. If this is going to be fixed the right way later, please amend the comment with the bug number you're going to fix it under.
Attachment #192178 -
Flags: review?(mconnor)
Attachment #192178 -
Flags: review+
Attachment #192178 -
Flags: approval1.8b4+
Comment 3•18 years ago
|
||
I imagine this is a 1.0x problem, too? Does the same thing happen in the 1.7 Suite?
Flags: blocking1.8b4+
Flags: blocking-aviary1.0.7?
mconnor: Right now there is no better way to fix what "initial window size fix" fixes without adding some sort of DOM mechanism for consistently taking into account title bar heights. I have filed bug 304184 for adding such functionality. For now I agree that we should go with my patch. dveditz: yes, it is a problem on FF 1.0.x. Haven't tried suite.
This patch actually fixes the problem. I am going to get some food - I'll look it over again before requesting review. Basically, we used old bad Quickdraw API stuff to get available screen real estate, which didn't take into account the Mac OS X menu bar or the dock. Then we didn't take a window's title bar into account.
Comment on attachment 192317 [details] [diff] [review] real fix, v1.0 Requesting review. All I see in my own patch is that we don't need to capture aScreenX or aScreenY, and this comment could be more clear: // we want available content size, so subtract off space for a title bar This patch has r=shaver.
Attachment #192317 -
Flags: review?(sfraser_bugs)
BTW - this patch fixes a couple other Mac OS X bugs, which I will mark as fixed when this lands.
Comment 8•18 years ago
|
||
(In reply to comment #7) > BTW - this patch fixes a couple other Mac OS X bugs, which I will mark as fixed > when this lands. Can you mark them as dependencies please.
Attachment #192317 -
Flags: review?(sfraser_bugs)
My patch has some issues with multiple monitors. Posting a new one soon.
Reporter | ||
Comment 10•18 years ago
|
||
*** Bug 302197 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 11•18 years ago
|
||
initial window size fix v1.0 checked in to trunk
Reporter | ||
Comment 12•18 years ago
|
||
I changed my mind about "real fix, v1.0" breaking multiple monitors. I think it is fine. We only have 2 ways to deal with available screen space in Carbon. If we use GetGrayRgn, like we were before, we could hard-code some value for the menu bar but we could never account for the dock. If we use GetAvailableWindowPositioningBounds(), we can account for the dock and the menu bar nicely, but only for a single screen. That is what my patch does now. That means that we limit the size of the window to the size of its main screen. You can still drag a window between screens, even split it between screens, but you could not make it the size of both screens, vertically or horizontally. Under normal multiple monitor use, that is fine really. Hardly anybody makes a browser window so big that it could not possibly fit on one window. While doing so might be a neat technical feat, allowing for that should not cause us to sacrifice usability for the vast majority of users who only have one screen. In theory we could iterate through all screens, figure out their relative positions, and coalesce their avaialable rectangles into one big one. We'd have to cache that value, since doing that 10-20 times in a normal drag-resize operation would be slow, but then if anything changed the cached value would stick and users would have a funny UI. Even if we got acceptable perf without caching, I don't think that it is a good idea and I certainly don't want to write and debug that code for a ridiculously small audience, if anyone. In summary, this patch is great for single-monitor users and basically every multiple-monitor user. We'll be free of this limitation when we can use Cocoa anyway. Also, I did not prefix a carbon function call with "::" - I'll fix that on checkin.
Attachment #192317 -
Flags: review?(bugs.mano)
Attachment #192317 -
Flags: superreview?(dveditz)
Comment 13•18 years ago
|
||
Comment on attachment 192317 [details] [diff] [review] real fix, v1.0 With this patch, one can only a window to be under the dock (not offscreen) until that part (which is under the dock) has the same height as the height between the window titlebar to the menubar. We should either allow to resize windows until the bottom of the screen or disallow to resize windows to be under the dock (I prefer the later, which is also the way Safari behaves).
Attachment #192317 -
Flags: review?(bugs.mano) → review-
Comment 14•18 years ago
|
||
s/one can only a window/one can only resize a window
Attachment #192317 -
Flags: superreview?(dveditz)
Reporter | ||
Comment 15•18 years ago
|
||
This patch isn't ready for review yet. It fixes Mano's problem, but has another problem. If you move a window offscreen by any amount and then try to make it bigger via window.resizeTo(), it will snap to a size that fits entirely on screen. It might work to test whether or not the window is offscreen at all and prevent a snapback if it is (allow the resize). If that doesn't work, we may be stuck with the behavior of v1.0, which wouldn't be too bad. I'll try it tomorrow.
Comment 16•18 years ago
|
||
Comment on attachment 192350 [details] [diff] [review] real fix, v1.1 Let's add the following: 1. If the window is not offscreen, but is out of the available rect, it should be possible to resize it until screen limits. 2. If it's already offscreen, don't block offscreen resizing.
When the security part is fixed, please mark FIXED. File a followup on the size constraints for multiple monitors elsewhere, if desired.
Comment 18•18 years ago
|
||
i just want to make a comment that as a widget module owner, i totally disagree with any patch going in that limits the size of a window to that of a single monitor. I have used that before, that breaks all of apple's guidelines. Doing so to fix a "security" issue is not a valid reason, IMHO.
Reporter | ||
Comment 19•18 years ago
|
||
So, this is a really complicated UI issue and it will be difficult to please everyone. I have spent a long time examining the issues, and I have 2 proposals. The patch I am attaching now is the more involved of my 2 proposals. It takes a crack at fixing our problem on multiple monitor systems and adds support for not allowing scripts to resize windows under the dock. See the big comment in the patch for one limitation, concerning the menu bar's placement in a multiple monitor setup. Also, the patch does not allow windows to become so big that they could not fit entirely onscreen without going under the dock. In many other applications they can. Problem is, we can't determine whether or not the resize is coming from script or a person. I recommend actually playing around with both solutions I am proposing instead of judging their merit based on my descriptions. The behavior is complex and full of tradeoffs.
Attachment #192350 -
Attachment is obsolete: true
Reporter | ||
Comment 20•18 years ago
|
||
This is a much simpler patch that fixes the immediate problem only. It shouldn't have any major behavior regressions as a tradeoff for attempting a more involved fix. I recommend just doing this and moving on. Bugs in window sizing will remain, but they will in the other patch too. Essentially, we were not accounting for the title bar of the window requesting a resize, nor were we accounting for the height of the menu bar. This fixes that, and that only. So, this is my recommended solution but I will let the module owner decide whether or not to go with this or go the route of "real fix, v2.0" with or without some behavioral adjustments. Also, maybe somebody else can come up with a totally different idea .
Attachment #192499 -
Flags: review?(mark)
Reporter | ||
Comment 21•18 years ago
|
||
With "real fix v3.0", if you have a screen that is not the main screen or above or below it, but is larger that the vertical column that includes the main screen, you will not be able to fill 22 pixels of its height (the menu bar's height). This might annoy people, though it is a tradeoff for guaranteeing vertical visibility in the main screen's vertical column. There is not a decent way around this to my knowledge if we wish to take the menu bar into account on multi-screen systems. Here is an even more conservative patch, that does the same thing but only subtracts off the menu bar's height if we are on a single-screen system. Probably better to just leave multi-monitors alone altogether (except for the title bar adjustment which is always safe the way I have implemented it). This is my new recommended patch, over v3.0. It should't cause any behavior that is even arguably a regression on single or multiple screen systems, and cleans up our single-screen story nicely. Sorry for all the patches, but this is very complicated UI and everyone has their own idea of what is and isn't an acceptable tradeoff or compromise. For that reason, this (the most conservative patch yet) should be our choice I think.
Attachment #192500 -
Flags: review?(mark)
Attachment #192499 -
Flags: review?(mark)
Assignee | ||
Comment 22•18 years ago
|
||
Comment on attachment 192500 [details] [diff] [review] real fix, v3.1 v3.1 doesn't take the dock into account. If we only solve it for one display, we should at least do it properly by not allowing resizes to be obscured by the dock. v2 stops working properly when any portion of the desktop is above the menu bar. I know it's an unconventional setup, but we should't break it as part of an incomplete fix here. I think I have a way to solve this problem for the single- and multi-monitor cases, but it requires getting the menu bar and dock bounds. I can think of various ways to get that information, but what I'd really like (especially for the dock) is a single clean function. We'd need to build the desktop's bounding rect and then clip out the menu bar and dock from the appropriate edges iff the menu bar or dock is positioned on the edge. (It's not a guarantee with multiple monitors.)
Attachment #192500 -
Flags: review?(mark) → review-
Reporter | ||
Comment 23•18 years ago
|
||
v3.1 wasn't meant to solve the dock problem. It was only meant to stop windows from getting too big to be onscreen, which is a more serious problem than them going under the dock.
Assignee | ||
Comment 24•18 years ago
|
||
This is an implementation of the logic I was describing. It disallows any resize that would put the window beyond the desktop rect, where the desktop rect does not include the dock and menu bar if they're at the desktop edges. This is similar to what Safari does, except Safari never allows a script to make a window larger than the screen it's primarily on. Without being able to discern between script- and user-triggered resizes, we don't have that luxury, but I prefer it this way anyway - the philosophy is to not care about what screen something is on if you can avoid it. One pain that this creates is that it prevents vertical resizes that put the window's bottom lower than the top of the space that the dock might occupy, even if the window is entirely left or right of the dock. Safari and most other apps share this quirk, which may be the correct behavior depending on how you look at it. Very safe patch, I think.
Reporter | ||
Comment 25•18 years ago
|
||
A fix for this bug fixes 211551, concerning the height of the window in full screen mode.
Blocks: 211551
Reporter | ||
Comment 26•18 years ago
|
||
Comment on attachment 192564 [details] [diff] [review] Handle multiple monitors gracefully - and dock too You cannot restrict resizing based on where a window happens to be positioned. Example: using this patch, put a window so its title bar is about 100 pixels above the bottom of the screen (lets assume the dock is on the right for now). Then go to View->Toolbars->Customize. You have now chopped off that dialog (try to drag it to center screen), so that it is not possible to close it, and put the menu bar in a permanently unusable state.
Attachment #192564 -
Flags: review-
Assignee | ||
Comment 27•18 years ago
|
||
That's our behavior on the trunk (and now branch), under 1.0.x, and with any of the patches posted here so far. I didn't realize we were trying to solve that problem too. I'd be happy to help fix it - even here - but I don't think it's really within the scope of this bug, and I don't think that Resize is the correct place to handle it.
Reporter | ||
Comment 28•18 years ago
|
||
Comment on attachment 192564 [details] [diff] [review] Handle multiple monitors gracefully - and dock too I didn't know that was our current behavior on the trunk. I just suspected it and tried it after reading your patch. Just a nit: - if (mWindowMadeHere) { - // Sanity check against screen size - Rect screenRect; - ::GetRegionBounds(::GetGrayRgn(), &screenRect); + if (mWindowMadeHere) + { Plz put "{" on the same line with the "if" test, as in the code you removed.
Attachment #192564 -
Flags: review-
Comment 29•18 years ago
|
||
Comment on attachment 192564 [details] [diff] [review] Handle multiple monitors gracefully - and dock too So, this has the same issue as v1.1 of josh's patch (see comment 15). Also, the behavior of resizing the window when it's already under the dock is still unexpected.
Assignee | ||
Comment 30•18 years ago
|
||
To address comment 15 (offscreen window snaps back), I'm now using max(window, desktop) to set the max right edge. If the window is already offscreen, it won't be forced to shrink in order to remain on-screen. This seems to be exactly what Safari does. To address the dock issue as most other applications do, we'd need to know the window bounds at the beginning of a live resize. This would need an event handler. However, we can handle the dock as the Finder and some other apps do by letting the system do the hard work for us. Here, I'm allowing the window to resize into the dock area if it's already there: by the time we get aWidth and aHeight during a user-initiated resize, they'll overlap the dock if the system's own constraints allow it. This effectively lets us retain our existing behavior for UI resizes, but beware: the system's constraints aren't perfect.
Assignee | ||
Updated•18 years ago
|
Attachment #192674 -
Flags: review?(joshmoz)
Comment 31•18 years ago
|
||
Comment on attachment 192674 [details] [diff] [review] Address Mano's concerns Looks good; please fix the error message here: >+ if (::GetAvailableWindowPositioningBounds(screen, &availableRect) >+ != noErr) >+ { >+ NS_ERROR("::GetAvailableWindowPositioningBounds couldn't");
Updated•18 years ago
|
Assignee: joshmoz → mark
Component: General → Widget: Mac
Flags: review?(joshmoz)
Flags: review-
Flags: review+
Product: Firefox → Core
QA Contact: general → mac
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Comment 32•18 years ago
|
||
The error message in comment 31 was partially intentional. :) I also added a check for an empty device rect. That, and Josh's braces in comment 28 are cleaned up here. (I'm having a hard time keeping on top of "prevailing style", nsMacWindow is a mishmash.) I'll take review from Mano, Josh, or both. Core doesn't seem to have an addl.review flag field.
Attachment #192564 -
Attachment is obsolete: true
Attachment #192674 -
Attachment is obsolete: true
Attachment #192682 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 33•18 years ago
|
||
Comment on attachment 192682 [details] [diff] [review] Nits picked + aWidth = PR_MAX(PR_MIN(aWidth, maxWidth), kMinWindowWidth); + aHeight = PR_MAX(PR_MIN(aHeight, maxHeight), kMinWindowHeight); Enforcing the min heights is no good here, because there are nsMacWindows that need to be smaller. This uglifies the autofill popup. So, I won't use the PR_MAX(..., kMin) wrappers - we'll retain the present behavior of allowing resizes down to the OS' own minimum, and we can worry about this if we want for 1.9.
Attachment #192682 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 34•18 years ago
|
||
As above, r from either Josh or Mano (or Mike, who's been quiet).
Attachment #192682 -
Attachment is obsolete: true
Attachment #192689 -
Flags: review?(bugs.mano)
Comment 35•18 years ago
|
||
Comment on attachment 192689 [details] [diff] [review] v4.5 + screen = ::DMGetFirstScreenDevice(TRUE); + while (screen != NULL) { + Rect screenRect = (*screen)->gdRect; + if (::EmptyRect(&screenRect)) { + NS_ERROR("Couldn't determine screen dimensions"); + return NS_ERROR_FAILURE; + } Better to warn here instead of error-and-return. + if (!::EmptyRect(&dockRect)) { + foundDock = PR_TRUE; + } Let's declare |foundDock| here and change this to: PRBool foundDock = !::EmptyRect(&dockRect); + // desktopRect is now clear of the menu bar and dock, iff they s/iff/if + if ((currentWidth != aWidth) || (currentHeight != aHeight)) { please remove extra parens. r=mano.
Attachment #192689 -
Flags: review?(bugs.mano) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #192689 -
Flags: superreview?(sfraser_bugs)
Assignee | ||
Updated•18 years ago
|
Attachment #192689 -
Flags: review?(joshmoz)
Reporter | ||
Comment 36•18 years ago
|
||
Comment on attachment 192689 [details] [diff] [review] v4.5 My original concern in this bug was that windows could be made bigger than could fit onto a screen. While this patch fixes that bug, I am uncomfortable with the fact that it does not always allow windows to expand to the size of the screen. An example of what I mean is, say putting a 600 pixel wide window the the middle of a monitor so that there is a at least a couple hundred pixels to the left before the screen's edge. Then enter this js in the URL bar: javascript: window.resizeTo(6000,6000) Notice that the window stops growing at the dock, and does not become as large as the screen's available space. This is a new behavior in this patch, and I'm not comfortable with it as the surprise factor for developers at the whim of their users' window positioning is much greater than before. I looked at the code for this patch, and it looks fine. While I would be willing to try this patch on the trunk, I do not want to r+ it for the 1.5 branch as I think the behavior is too risky. I think for the branch we should be more conservative about behavior changes. Something along the lines of my patch v3.1 but with more support for the dock. While the code for this patch looks good, I am going to abstain from giving it r+ or r- right now. Changing the review flag to point at pink.
Attachment #192689 -
Flags: review?(joshmoz) → review?(pinkerton)
Comment 37•18 years ago
|
||
(In reply to comment #36) > Notice that the window stops growing at the dock, and does not become as large > as the screen's available space. This is a new behavior in this patch, and I'm > not comfortable with it as the surprise factor for developers at the whim of > their users' window positioning is much greater than before. Josh, there are only two in-tree chrome callers for this code (neither do maybe-offscreen resizes), I also don't see why would we want to support this for web developers (Note that |window.resizeTo| turned off by default for content, at least on the trunk). I doubt the existence of extensions which rely on offscreen resizes.
Reporter | ||
Comment 38•18 years ago
|
||
I don't see any immediate problems with the patch either, but changes of that magnitude have a way of causing problems. I do not think the benefit (that which goes beyond my original problem from comment 36) is worth that risk for 1.5 right now. Not my decision though.
Comment 39•18 years ago
|
||
Comment on attachment 192689 [details] [diff] [review] v4.5 looks like it covers all the bases. i wish there was a way such that it wasn't so complicated but i'm not certain that's possible. do we need to do something similar to cocoa widget? r=pink
Attachment #192689 -
Flags: review?(pinkerton) → review+
Comment 40•18 years ago
|
||
Comment on attachment 192689 [details] [diff] [review] v4.5 I really think that it's pointless to do this without fixing the changing of the window origin by JS as well. Also, the logic seems overly complicated, and it doesn't prevent the hiding of the status bar on multi-screen configurations.
Attachment #192689 -
Flags: superreview?(sfraser_bugs) → superreview-
Comment 41•18 years ago
|
||
Comment on attachment 192689 [details] [diff] [review] v4.5 I really think that it's pointless to do this without fixing the changing of the window origin by JS as well. Also, the logic seems overly complicated, and it doesn't prevent the hiding of the status bar on multi-screen configurations.
Assignee | ||
Comment 42•18 years ago
|
||
smfr suggested that if a window is already contained entirely within a single screen, then that screen's available area should be used instead of the desktop's area. That's implemented here. This only affects script resizing, the user is still able to manually resize a window on one screen to be as large as the desktop area. Note that on multi-monitor systems, if you position a window so that it's entirely on one screen except for a portion which is not on any screen, you'll still fall into the "not on a single screen" case and your usable desktop area won't be constrained to the single screen. I agree that there should also be restraints on (top,left) moves, but that's almost certainly something for another patch, if not another bug entirely. In preparation for such a bug, I've moved the code that determines the usable desktop bounds into its own method, GetDesktopRect, which comes with a big juicy fat comment to help readers (and reviewers!) figure out what's going on inside. I don't have a spare monitor here to test with at the moment, but considering the types of changes I've made, the only thing I'd want to verify is that manual resizes allow the window to grow beyond a single screen. (They should, but paranoia...)
Attachment #192689 -
Attachment is obsolete: true
Attachment #194361 -
Flags: superreview?(sfraser_bugs)
Assignee | ||
Updated•18 years ago
|
Attachment #194361 -
Flags: superreview?(sfraser_bugs)
Assignee | ||
Comment 43•18 years ago
|
||
> Note that on multi-monitor systems, if you position a window so that it's > entirely on one screen except for a portion which is not on any screen, you'll > still fall into the "not on a single screen" case and your usable desktop area > won't be constrained to the single screen. For my next trick, I've changed this: windows are now thought to be on a single screen as long as they're visible on exactly one screen. They're permitted to leave that screen and have portions that aren't visible without being treated as though they're on multiple monitors (unless, of course, multiple monitors contain the window.)
Attachment #194361 -
Attachment is obsolete: true
Attachment #194402 -
Flags: review?(sfraser_bugs)
Assignee | ||
Updated•18 years ago
|
Attachment #194402 -
Flags: review?(sfraser_bugs) → superreview?(sfraser_bugs)
Assignee | ||
Updated•18 years ago
|
Attachment #194402 -
Flags: superreview?(sfraser_bugs)
Assignee | ||
Comment 44•18 years ago
|
||
Restricting the size based on the window's position on screen breaks the variable-height preferences window, when the window is too close to the bottom of the desktop. If this is adjusted to not take position into account, always allowing resizes to the desktop size, things break down in the areas intended to solve Mano's (comment 29) and Simon's (comment 40) concerns. For example, even on a single-screen system, with the dock at the bottom of the monitor, place the window vertically halfway down the screen and size it so it is not obscured by the dock. Then, do javascript:window.resizeTo(10000,10000); to get it to max size. Because the window was not obscured by the dock, it will grow so that it will be as tall as the screen, less the height of the dock and menu bar, so that if it were moved to (0,0), it would fit perfectly. But its position is not (0,0), and it's now covered by the dock at its present position, so a subsequent resizeTo max dimensions will make the window even taller, so that it will be obscured by the dock even if it's moved to (0,0). It's not correct to get different maxima on a subsequent resize attempt when nothing else has changed. What this really needs is a way to distinguish between resizes that come out of content (JS resizes that should take window position into account) and those that come out of chrome and from the UI (trusted "do-what-I-said" resizes with less restrictive checking). That means digging into nsXULWindow/nsIBaseWindow. I'm not sure if that's the type of change people will be comfortable with on the branch at this point. The other option is to not worry about dock overlap, but that's barely better than what happens now.
Assignee | ||
Comment 45•18 years ago
|
||
To fix this properly, I need to add an enum argument to nsIBaseWindow::setSize and nsIBaseWindow::setPositionAndSize, and for congruity, nsIBaseWindow::setPosition too, even if it's unused at this point. The enum needs to identify the source of the resize/reposition request: content, chrome, or UI. Resize requests from content need to be treated most restrictively. Requests from chrome need sanity checking, and requests from the UI are to be treated least restrictively because the system enforces whatever constraints are appropriate to the platform. Although nsIBaseWindow isn't frozen, it's probably pretty widely used. So, nsIBaseWindow2, which will be implemented at least by nsXULWindow.
Comment 46•18 years ago
|
||
Why isn't this a problem for other platforms?
Assignee | ||
Comment 47•18 years ago
|
||
It is on Windows, as far as I can tell, but it seems to be less of a problem due to the "correct" behavior of windows on that platform. I haven't yet tested an X build. (Someone more familiar with Windows is welcome to step in and correct any incorrect assumptions or conclusions here.) On Windows, gecko passes the resize values straight down to the system without checking them or enforcing any maxima. The system's behavior crimps the resize at roughly the size of the screen (or desktop?) - it actually allows it to be a little bit larger, such that the window will fill the screen entirely when its border is offscreen. It even enforces this on resizes that come from the UI: it's impossible to drag a window offscreen to the left and then make it wider than the screen. The Mac doesn't work like this. On the Mac, windows can be arbitrarily sized. If you ask the system to give you SHRT_MAX x SHRT_MAX pixels, you'll get it, regardless of your screen or desktop size. A user is free to make wide windows from within the UI by dragging them offscreen and expanding them. Gecko on Windows is more than willing to let the window be obscured by the taskbar, or to allow windows not positioned at 0,0 to have their bottom and right edges pushed offscreen. WinIE takes the window origin into account and won't allow JS resizes to push the window's bottom or right edges offscreen or under the taskbar. It also prevents moves from pushing the window offscreen or under the taskbar; they're snapped back to the closest on-screen position to the target given the current size. Note that WinIE enforces this behavior even when the window is already partially offscreen. So do we have a security problem on Windows here too? You betcha. Considering the way bug 309251 seems to be heading, the severity is about to get more critical here.
Assignee | ||
Comment 48•18 years ago
|
||
Here's my plan: For 1.8, I'd like to fix this on the Mac by crimping all content and chrome resizes to the desktop dimensions without taking its origin into account. I can handle this entirely in widget, without changing any public interfaces. For 1.9, I'd like to fix moves and resizes on all platforms by separating the various classes of callers. I do think that allowing resizes and moves from content to push the window offscreen is a security flaw, especially if move/resize will be turned back on by default, but it's the security group's call. I'd like my proposed 1.9 fix for 1.8, but the issue doesn't seem to have gotten any attention so far.
Status: NEW → ASSIGNED
Assignee | ||
Comment 49•18 years ago
|
||
This is the plan for 1.8. It's mostly the same as my previous patches, with a couple of tweaks. No checking is applied to resizes that come from the UI. For other resizes, the maximum size is the size of the desktop. If the window is already taller or wider than the desktop, then the window's larger dimension is used instead, so resizes won't force windows to shrink. Assuming the user hasn't manually resized a window to be larger than the desktop, this patch always results in windows that can be dragged to fit entirely on the desktop without being obscured by the dock. There's one gotcha with this patch: if there are multiple screens but the window is only on one of them, then its home screen is treated as the desktop. If it's not positioned at the top left of its screen, then one resize to max would push it off of its screen (so that it has the dimensions of its former home screen) and on to another, so a subsequent resize to max will cause it to be resized so that it can fill all screens. This is what we get for not being able to take the origin into account. There's no clean way around this without implementing my 1.9 plan. Even so, in no case will a window grow so large that it won't fit on the desktop or will be obscured. If there's another platform-agnostic bug on how window resizing is broken in general, could someone please Cc me? I haven't found any, but it may be marked security too.
Attachment #194402 -
Attachment is obsolete: true
Attachment #196952 -
Flags: superreview?(sfraser_bugs)
Attachment #196952 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 50•18 years ago
|
||
Comment on attachment 196952 [details] [diff] [review] Safe fix, v.N I'll put a better comments in around the preprocessor conditional: +#if 1 + // Use the desktop's dimensions as the maximum size without taking + // the window's position into account. + // [...] +#else + // Consider the window's position when computing the maximum size. + // The goal is to use this on resizes that come from content, but + // it's currently not possible to distinguish between resizes that + // come from chrome and content. + //
Comment 51•18 years ago
|
||
mano and smfr, time is getting really short for 1.8b5. Can you guys help out with some reviews here?
Comment 52•18 years ago
|
||
Comment on attachment 196952 [details] [diff] [review] Safe fix, v.N Index: mozilla/widget/src/mac/nsMacEventHandler.cpp =================================================================== - mTopLevelWidget->Resize(macRect.right - macRect.left, macRect.bottom - macRect.top, PR_FALSE); + mTopLevelWidget->Resize(macRect.right - macRect.left, macRect.bottom - macRect.top, PR_FALSE, PR_TRUE); // resize came from the UI via an event this line is a bit too long Index: mozilla/widget/src/mac/nsMacWindow.cpp =================================================================== +NS_IMETHODIMP nsMacWindow::Resize(PRInt32 aWidth, PRInt32 aHeight, PRBool aRepaint, PRBool aFromUI) make it nsresult. +// GetDesktopRect +// +// Determines a suitable desktop size for this window. The desktop +// size can be used to restrict resizes and moves. +// +// If a window is already lies on a single screen (possibly with portions is already lies? +nsresult nsMacWindow::GetDesktopRect(Rect* desktopRect) { ... + if (::EmptyRect(&screenRect)) { + NS_WARNING("Couldn't determine screen dimensions"); continue to the next screen here. r=mano with those fixed.
Attachment #196952 -
Flags: review?(bugs.mano) → review+
Attachment #196952 -
Flags: review+
Comment 53•18 years ago
|
||
Comment on attachment 196952 [details] [diff] [review] Safe fix, v.N > Index: mozilla/widget/src/mac/nsMacWindow.cpp > =================================================================== > + > +#if 1 > + // The maximum window dimensions are the desktop area, but if the > + // window is already larger than the desktop, don't force it to Add a comment explaining the #if 1, and when/if it will be changed. > +nsresult nsMacWindow::GetDesktopRect(Rect* desktopRect) { Opening brace on new line please. > + // This is how to figure out where the dock and menu bar are, in global > + // coordinates, when there seems to be no way to get that information > + // directly from the system. Compare the real dimensions of each screen > + // with the dimensions of the screen less the space used for the dock (as > + // given by GetAvailableWindowPositioningBounds) and apply deductive > + // reasoning. While doing that, it's a convenient time to add each > + // screen's full rect to the union in desktopRect. > + > + ::SetRect(desktopRect, 0, 0, 0, 0); > + Rect menuBarRect = {0, 0, 0, 0}; > + Rect dockRect = {0, 0, 0, 0}; > + > + Rect windowRect; > + if (::GetWindowBounds(mWindowPtr, kWindowStructureRgn, &windowRect) > + != noErr) { > + NS_ERROR("::GetWindowBounds() didn't get window bounds"); > + return NS_ERROR_FAILURE; > + } > + > + Rect tempRect; // for various short-lived purposes, such as holding > + // unimportant overlap areas in ::SectRect calls > + > + PRUint32 windowOnScreens = 0; // number of screens containing window > + Rect windowOnScreenRect = {0, 0, 0, 0}; // only valid if windowOnScreens==1 > + GDHandle screen = ::DMGetFirstScreenDevice(TRUE); > + while (screen != NULL) { > + Rect screenRect = (*screen)->gdRect; > + if (::EmptyRect(&screenRect)) { > + NS_WARNING("Couldn't determine screen dimensions"); > + } > + > + ::UnionRect(desktopRect, &screenRect, desktopRect); > + > + if(::SectRect(&screenRect, &windowRect, &tempRect)) { if<space>( > + // The window is at least partially on this screen. Set > + // windowOnScreenRect to the bounds of this screen. > + // If, after checking any other screens, the window is only on one > + // screen, then windowOnScreenRect will be used as the bounds, > + // limiting the window to the screen that contains it. > + windowOnScreens++; > + ::SetRect(&windowOnScreenRect, screenRect.left, screenRect.top, > + screenRect.right, screenRect.bottom); Why not windowOnScreenRect = screenRect; ? > + if (windowOnScreens == 1) { > + // The window is contained entirely on a single screen. It may actually > + // be partly offscreen, the key is that no part of the window is on a > + // screen other than the one with screen bounds in windowOnScreenRect. > + // Use that screen's bounds. > + // > + // It's tempting to save the screen's available positioning bounds and > + // return early here, but we still need to check to see if the window is > + // under the dock already, and permit it to remain there if so. > + // > + // Keep in mind that if the resize came from the UI as opposed to a > + // script, the window's dimensions will already appear to exceed the > + // screen it had formerly been contained within if the user has resized > + // it to cover multiple screens. In other words, this won't constrain > + // UI resizes to a single screen. That's perfect, because we only > + // want to police script resizing. > + ::SetRect(desktopRect, windowOnScreenRect.left, windowOnScreenRect.top, > + windowOnScreenRect.right, windowOnScreenRect.bottom); *desktopRect = windowOnScreenRect; ? > + // Use kWindowContentRgn to get past the title bar to the "meat." > + if (::GetWindowBounds(mWindowPtr, kWindowContentRgn, &windowRect) > + != noErr) { > + NS_ERROR("::GetWindowBounds() didn't get window bounds"); > + return NS_ERROR_FAILURE; > + } > + > + // Clip where appropriate. > + > + if (::SectRect(desktopRect, &menuBarRect, &tempRect) && > + menuBarRect.top == desktopRect->top) { > + // The menu bar is in the desktop rect at the very top. Adjust the > + // top of the desktop to clip out the menu bar. > + // menuBarRect.bottom is the height of the menu bar because > + // menuBarRect.top is 0, so there's no need to subtract top > + // from bottom. When using multiple monitors, the menu bar > + // might not be at the desktop top - in that case, don't clip it. > + desktopRect->top += menuBarRect.bottom; > + } > + > + if (::SectRect(desktopRect, &dockRect, &tempRect)) { > + // There's a dock and it's somewhere in the desktop rect. > + // If the window already overlaps the dock, allow it to continue > + // overlapping the dock. If the resize operation comes from the > + // user through the UI, windowRect will already be adjusted to the > + // desired bounds and will already overlap the dock if the system > + // allows it even if the original bounds did not, so the system's > + // resize constraints will be in effect. > + > + // Determine the orientation of the dock before checking to see if > + // it's on an edge, so that the proper edge can be clipped. If > + // the dock is on an edge corresponding to its orientation, clip > + // the dock out of the available desktop. Couldn't you have saved the orientation from earlier? sr=me with those issues examined.
Attachment #196952 -
Flags: superreview?(sfraser_bugs) → superreview+
Updated•18 years ago
|
Attachment #196952 -
Flags: approval1.8b5?
Assignee | ||
Comment 54•18 years ago
|
||
This addresses all of the review comments.
Attachment #196952 -
Attachment is obsolete: true
Attachment #198001 -
Flags: approval1.8b5?
Assignee | ||
Updated•18 years ago
|
Attachment #196952 -
Flags: approval1.8b5?
Assignee | ||
Comment 55•18 years ago
|
||
Fixed on trunk. Also, from comment 49: > If there's another platform-agnostic bug on how window resizing is broken in > general, could someone please Cc me? I haven't found any, but it may be marked > security too.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Attachment #198001 -
Flags: approval1.8b5? → approval1.8b5+
Assignee | ||
Comment 56•18 years ago
|
||
fixed1.8, will open a new bug to address content resizes on all (tier 1) platforms.
Keywords: fixed1.8
Updated•18 years ago
|
Flags: blocking-aviary1.0.8? → blocking-aviary1.0.8-
Whiteboard: [sg:fix] → [sg:spoof]
Updated•17 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•