Going to remove the checkbox, set the default behavior to deny, and allow a per-site whitelist.

blocking/P1 per blocked bug.

Gavin, could I get you to look over the XUL stuff here?

Comment on attachment 297649 [details] [diff] [review] Patch, v1 Wrong pref! Hang on...

Better patch, at least removes the correct pref.

Comment on attachment 297655 [details] [diff] [review] Patch, v2 Gavin, the XUL parts should all be here now.

Comment on attachment 297655 [details] [diff] [review] Patch, v2 + if (!CanMoveResizeWindows(GetPrincipal())) { This'll use the wrong principal. GetPrincipal() in a window returns the principal of the window (object principal), you want the principal of the caller (subject principal). To do that you could make CanMoveResizeWindows() take no arguments and use nsIScriptSecurityManager::GetSubjectPrincipal() there to find out who's calling this code and then go on based on that. nsContentUtils has a getter for the security manager for you... Other than that this looks good (though I'll let Gavin speak for the UI code changes which I know next to nothing about).

Oops. How about this?

Comment on attachment 297670 [details] [diff] [review] Patch, v3 r+sr=jst for the DOM changes and preference removal.

Comment on attachment 297670 [details] [diff] [review] Patch, v3 >--- mozilla.fb2474e53222/browser/locales/en-US/chrome/browser/preferences/advanced-scripts.dtd 2008-01-17 18:46:23.000000000 -0800 >+++ mozilla.2fb3b6a2196c/browser/locales/en-US/chrome/browser/preferences/advanced-scripts.dtd 2008-01-17 18:46:23.000000000 -0800 >-<!ENTITY moveResizeWindows.label "Move or resize existing windows"> >-<!ENTITY moveResizeWindows.accesskey "M"> >+<!ENTITY moveResizeWindows.label "Scripts may not move or resize existing windows"> Change the entity name too, otherwise localizers won't notice that this string's meaning has changed. Beltzner/mconnor should UI-review this (sounds like you've already talked to them about it?) I wonder if we might want to file a followup to show a notification bar when a site tries to move/resize a window but fails because of this? Seems like sites that rely on being able to do this are going to be broken, without really any indication of what's wrong unless you're familiar with this rather hidden pref UI and know to add an exception. Not sure how common that is, or how much impact it will really have.

Comment on attachment 297670 [details] [diff] [review] Patch, v3 Beltzner, you and mconnor are in agreement on this approach, yes?

Hm, I thought we were talking about raise/lower (which we'd already moved to default-deny) not move/resize, when we talked face to face on this one. I'm not sure we can just blanket prevent all windows from resizing without breaking some web apps out there :( (In reply to comment #10) > Change the entity name too, otherwise localizers won't notice that this > string's meaning has changed. Why did we change the string? I would expect it to still stay indented beneath the existing label header ..: Allow scripts to: [ ] Raise or lower windows [x] Disable or replace context menus [ ] Hide the status bar [ ] Change status bar text [ ] Move or resize existing windows (Exceptions...) or [ ] Move or resize existing windows (Exceptions...) > Beltzner/mconnor should UI-review this (sounds like you've already talked to > them about it?) I wonder if we might want to file a followup to show a > notification bar when a site tries to move/resize a window but fails because Yeah, I think that if we're flipping this preference we'll need a way to work around it, or at least to indicate to users that we blocked something.

(In reply to comment #12) > Hm, I thought we were talking about raise/lower Ha, so did I. See comment #4 :( > I'm not sure we can just blanket prevent all windows from resizing > without breaking some web apps out there :( The discussion in bug 329385 led to blanket deny. This is an attempt to soften that a bit. > Why did we change the string? I would expect it to still stay indented beneath > the existing label header ..: It's no longer a checkbox. It's off for all sites except those you specify in the whitelist. This doesn't make sense to me: > Allow scripts to: > > [ ] Raise or lower windows > [x] Disable or replace context menus > [ ] Hide the status bar > [ ] Change status bar text > Move or resize existing windows (Exceptions...) > Yeah, I think that if we're flipping this preference we'll need a way to work > around it, or at least to indicate to users that we blocked something. I filed bug 413792.

(In reply to comment #13) > It's no longer a checkbox. It's off for all sites except those you specify in > the whitelist. This doesn't make sense to me: Wow. That seems *really* extreme. I would expect (like all our other restrictions along these lines) that there would be a checkbox for the default behaviour, and then exceptions for sites which are allowed. That means I wrote the pref the wrong way. We'd probably want to reverse all of these, actually, so: Prevent scripts from: [ ] Modifying context menus [ ] Changing status bar text [x] Hiding the status bar [x] Raising or lowering windows [x] Moving or resizing existing windows (Exceptions...) And I'm not sure that we could do this without 413792. Again, I'm working in the blind, here, but I'm sure I've encountered some gallery sites which change the viewport size to match the image inside of them. Move seems to be the more dangerous operation - can we split that out?

(In reply to comment #14) > (In reply to comment #13) > > It's no longer a checkbox. It's off for all sites except those you specify in > > the whitelist. This doesn't make sense to me: > > Wow. That seems *really* extreme. I would expect (like all our other > restrictions along these lines) that there would be a checkbox for the default > behaviour, and then exceptions for sites which are allowed. We shouldn't expose a default of allowing sites to move and resize windows as there are security problems with that (see dependent bug). We can let people enable it on a per site basis, as long as they know what they're doing, which probably means there needs to be words of caution somewhere here. The intent with this whole thing was that this would primarily be done for intranets and the like that depend on this... It's ugly overall if you ask me (not speaking of UI here, just the mess we're in with this stuff), but here we are...

So we're still talking about different solutions here. On hold until some agreement is reached.

Comment on attachment 297670 [details] [diff] [review] Patch, v3 Let's put this in for beta, and make sure we blog about the change a little, and see the reaction. (I think I'd still be more comfortable with something like other global prefs which *can* be enabled, globally, if a user wants.)

Fixed.

(In reply to comment #17) >Let's put this in for beta, and make sure we blog about the change a little, >and see the reaction. A heads up, rather than a post-facto blog, would have been nice... Also, this bug is in the wrong component, because it's a core change.

Just trying out the 3 beta 3 and discovered this new whitelisting behavior, which is fine, but how do you white-list javascript bookmarklets in your own bookmarks toolbar? I've got a few for site development reasons that I rely on which have now ceased to work. Is there a work-around for these? Thanks, Michael

(In reply to comment #20) Hi Michael, I think that you have an excellent point. Would you please file a new bug and make it block this one?

I added it as Bug 417824, but don't see how to make it block this one. Please advise. -mpm

found it... it's been blocked :)

I just found out about the fact that resizing windows via JS is denied by in FF3 and only allowed for exceptions This is going to break many sites, and from the comments above I've seen some comments on whether this is not an extreme measure. I agree that this is an extreme measure that will break many sites. Some examples that will break with FF3. All these examples (most from content from my company, I admit, but not all) need to auto-resize the window to fit the content: http://www.amazon.com/Logitech-Revolution-Cordless-Laser-Mouse/dp/B000HCT12O/ref=pd_bbs_sr_1?ie=UTF8&s=electronics&qid=1203412323&sr=8-1 (click "watch it in action") http://www.circuitcity.com/ssm/Xbox-360-Elite-High-Definition-Gaming-and-Entertainment-System-882224390118/sem/rpsm/oid/177538/catOid/-16487/rpem/ccd/productDetail.do (click "demo") http://www.officedepot.com/a/products/680575/Widescreen-Notebook-Computer-With-Duo-Processor/ (click "more about vista") http://www.ritzcamera.com/product/591162854.htm (click "interactive tour") http://www.xbox.com/en-US/games/d/devilmaycryfour/ (click on one of the screenshots)

I understand why this is a whitelist instead of a blacklist, but is there any reason a user can't choose to globally enable this preference anymore? I think it's weird that resizing and moving is the only JS preference which a user can't turn on and off.

Comment 15 covers that exact question. I assume you _did_ read the bug before commenting, yes?

Yes, I did. Sorry. I must have missed that one. I did more of a skim than a read every word.

Based on the site breakage, re-opening and suggesting we back out this change.

What site breakage? Is it really enough to justify the annoyance and security hit that would come with reverting this change? I've seen minor problems due to this change on http://www.myfreecams.com/, but that site barely works on trunk anyway. I haven't heard reports of any other sites breaking.

(By the way, one would hope that any site breakage is temporary, while the annoyance and possible security hit would not be.)

If this is reverted, * We should always disallow resizes/moves for non-popup windows (bug 186708), especially given the recent attacks mentioned in bug 186708 comment 10. * "Really" fixing bug 329385 will become more urgent. * We should consider giving users the *option* of using a whitelist, like we do with popup blocking.

Jesse, Site breakage examples can be found in comment #24. In all these cases, the popups implement code that resizes the window based on the size of the content. And those are not minor sites... Since the content is HTML, and not only a static image whose size is fixed, the popups can only know the size of the popup in an approximate manner and have to "fine-tune" the size of the window by resizing it. I love the idea of disallowing resize/moves for non-popup windows, i.e. allow resizes/moves *only* for windows that were opened using window.open (although if this is technically difficult the compromise in bug 186708 seems fine too).

Adding back those strings that were removed here to beat the freeze.

Comment on attachment 307764 [details] [diff] [review] Add back strings [checked in] Oh, this is r+sr+a=jst.

Comment on attachment 307764 [details] [diff] [review] Add back strings [checked in] This is late-l10n, so it needs explicit driver approval.

agree with 31. only prevent main windows from being resized - not popups. as a developer i've created many sites whereby a popup image automatically resizes the window to the size of the image. this is just another example of the many things that are breaking with this turned off by default.

Guys, please read the bug before commenting. This isn't just being done for the fun of it. It's being done to fix a specific security bug. Once there is a different fix for bug 329385 there will be something to discuss.

I may be doing something wrong, but I can't access bug 329385 - I get an "access denied" error.

> I may be doing something wrong, but I can't access bug 329385 - I get an > "access denied" error. As Boris said it's a security bug. If you are not on the security team you can't see it until it's been fixed and the security sensitive flag removed.

I joined this discussion too late to know what is the topic of the secret bug 329385, but I would like to be one of the fighters for resizing the window by JavaScript. For sure, some sites abuse this functionality but there are many many sites that use it to improve their visual appearance and usability. I guess that only very few developers and designers complain about losing this functionality as almost no one has the time to check out every beta version of a new browser and no one expects having to change his entire user interface. For more than 2 years now, or company has promoted the use of Firefox to all of our 250.000+ registered users and millions of visitors. Please guys don't make us regret this.

I don't think you understand. The point is that this is not just abusable (as in, annoying) but can actually be used to exploit the user. If it were just an annoyance thing the tradeoffs would be much simpler (e.g. one could in fact allow it for popup windows).

Would the option from comment #31 help with this super-secret bug? To repeat it - allow a window to resize itself only if it was opened with window.open. I totally agree with Alex B on comment #40, and am one of those that *did* review FF 3 and found, amongst mostly great functionality, this problem. I also gave a list of links from major sites which will break if this goes live (see comment #24). Please please please reconsider.

> Would the option from comment #31 help with this super-secret bug? No. It would take care of most of the annoyance, and it would mostly take care of attacks like in bug 186708 comment 10, but it wouldn't help much against bug 329385. I hope we can fix bug 329385 and then allow (some) window resizing for Firefox 3, but I don't know how likely it is that we'll be able to fix bug 329385 in time. I agree that it would be unfortunate to break sites that resize popup windows, just for one release, because we couldn't fix bug 329385 in time...

i wonder if this super-secret bug affects FF1 + 2 or other browsers like IE/Saf/Opera?

Is there any chance that bug 329385 will be fixed in FF 3? I know that I may be nagging too much, but this is going to be a *major* headache for many sites that use popups to show secondary content. The only alternative to this is moving to a "lightbox" style popup, which is a considerable development effort, especially for something that may be fixed in a future release of FF!

And it was backed out. WONTFIX.

Per discussion on #developers: Even removing strings is a bad idea this close to the lock down for B5. We opted for not putting the strings back in, as our l10n reporting just warns about obsolete strings, so we're not going to miss out on any localization due to this change. It does add noise to the process though, so it's not a good idea. It's just that doing it twice is worse. Posted to .l10n to ease the minds of our localizers.