Open Bug 413792 Opened 12 years ago Updated 5 years ago

Provide user notification if a script is denied access to move or resize a window

Categories

(Firefox :: Security, defect)

defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: bent.mozilla, Assigned: Squally425)

References

Details

(Keywords: student-project)

Attachments

(3 files, 15 obsolete files)

26.90 KB, patch
Details | Diff | Splinter Review
36.23 KB, patch
Details | Diff | Splinter Review
27.19 KB, patch
Details | Diff | Splinter Review
See bug 412862 where the ability to move or resize a window was changed to only allow whitelisted sites. Some are concerned that webapps may break without this ability and so users should be notified and directed to the whitelist if they encounter such a site.
Would that mean that users, who have never allowed this, suddenly get messages with the notification bar? Or will this only happen if people haven chosen for the option to allow this for white listed sites?
Maybe a statusbar message would be less obtrusive, or maybe on the Larry pop-up (see bug 403961)?
I vote for WONTFIX.  Ignoring requests to resize windows doesn't really break anything now that bug 177838 is fixed.
Here's what I said in bug 329385 on 2008-01-26:

"If the window is too small, users can figure that out themselves and resize the
window.  Showing a notification bar about the blocked resize() call would just
make it less likely for the content to fit and add a step for users."

I've kinda changed my mind, though.

If you're a *frequent* visitor of a site that tries to resize windows for *legitimate* reasons, resizing it manually over and over can end up wasting a lot of your time.  For example, http://www.myfreecams.com/ enlarges private-chat windows when you turn on video for them.  If we ship Firefox 3 with scripted resizes silently ignored, that site will probably be forced to make all chat windows large enough to accommodate video, which would hurt user experience a little.

On the other hand, there are a lot of sites that try to resize windows for no good reason.  To keep a notification bar from being annoying on these sites, it would probably need an "always deny" button.  Or maybe we can be sneakily clever and only notify users about attempted scripted resizes if they *do* manually resize the window the first time.

At least on Mac, manually resizing windows is usually accomplished by dragging from the lower-right of the window.  So maybe we could put the notification on the right side of the status bar instead of making it a notification bar, on the theory that users going for the resize grippy will notice something in the status bar.

We need a real fix for bug 329385 so it isn't so dangerous for users to allow script resizes :(
On the other hand, with my proposal in bug 454779 comment 6, most legitimate resizes would be allowed by default.
Hmm, what about this?
If FireFox disables move/resize window (as in disable_window_move_resize is currently enabled), then there is no infobar.
Otherwise, if it is enabled, one of the following could happen:

1) First time the site is accessed, an infobar is displayed to tell the user the site would like to move/resize the current window. User can select allow, disable, always allow, always disable

2) If a site is already on the always allow or always disabled list, then nothing will happen and the proper action is taken, otherwise until the user selects always allow, or always disable for that particular URI, the infobar is displayed whenever a site tries to move/resize the current window

3) We can also include an option to disable the infobar for users who'd rather just allow all sites to resize/move their window whenever its triggered.
Duplicate of this bug: 459614
This is the same patch that was attached to Bug 459614 with styling fixed.
The review can be found in Bug 459614 Comment 3 by Gavin Sharp.

Only one functional change was made:
The infobar only displays when disable_move_resize_window is true.
Previously, it displayed the infobar all the time.
Attachment #350602 - Flags: review?(jst)
Same patch as above, with the whitespace changes removed.
Attachment #350602 - Attachment is obsolete: true
Attachment #350611 - Flags: review?(jst)
Attachment #350602 - Flags: review?(jst)
Sorry, the previous patch was in zip format
Attachment #350611 - Attachment is obsolete: true
Attachment #350612 - Flags: review?(jst)
Attachment #350611 - Flags: review?(jst)
Attached patch Infobarpatch Version 0.3 (obsolete) — Splinter Review
- Infobar is no longer displayed all the time (From Gavin's review)
- A new preference window now to replace Advanced Javascript settings in the infobar
- Windows Move / Window Resized has two different labels, it paves the ground for when the new event arrives (Which I will work on even after this course is over)
- The preference can be turned off in about:config but I did not add it to Javascript settings
- All code tabs have been removed, replaced with spaces
- A new property, resizeReport was implemented so that the infobar does not interfere with pageReport
- A new event for move events was created
- New functions for browser.js, most of the functions were rewritten to remove **** which wasn't needed after some changes
- Most of the properties have been moved to the dtd localization files
- Browser-sets.inc 's broadcasters have been removed.
- Menuitem labels are not defined in the xul file anymore, they have been defined in browser.dtd
- A new option: Disableinfobar added to the menu. At the moment it toggles (between disable, and turn on), it will be changed in the future
- An area to add urls to, which should theoretically bypass the javascript property check if a site is added (to be implemented). At the moment, the interface is there, but lacks functionality.
Attachment #350612 - Attachment is obsolete: true
Attachment #351575 - Flags: review?(jst)
Attachment #350612 - Flags: review?(jst)
Attached patch New events added (obsolete) — Splinter Review
Two new events created that fires when any of the commands ResizeTo, ResizeBy, MoveTo, MoveBy.
These events can be captured and used for other extensions.
The infobar has a new option which when clicked will apply the blocked command to the window, called "Allow Once".

For example, if a website tries to run the javascript command "resizeTo(640, 480);" and its blocked, then selecting "Allow Once" will cause the browser window to resize to 640 by 480, but does not alter any preferences for the user.
All future tries to resize/move the browser will still continue to be blocked.
Attachment #351575 - Attachment is obsolete: true
Attachment #358118 - Flags: review?
Attachment #351575 - Flags: review?(jst)
Attachment #358118 - Flags: review? → review?(jst)
Assignee: nobody → Squally425
Comment on attachment 358118 [details] [diff] [review]
New events added

Sorry for the slow response here.

On a general note about this patch I think this is going in the right direction. There's some nits and details I saw while skimming through it, but I don't have the time right now to list those, and I don't think that's important yet.

The one thing that stood out in the implementation of the back end code here is that the events nsDOMWindowMoveEvent and nsDOMWindowResizeEvent (and nsWindowMoveEvent vs nsWindowResizeEvent similarily) look very much alike, and I'm wondering if we wouldn't be better off combining the two into one event (call it DOMWindowResizeMoveEvent maybe?), with one interface that's flexible enough to expose the command (resize, resizeBy, move, moveBy), and the x/y values. I think that would end up being less code, and fewer interfaces to deal with, only one new entry added to the classinfo mess, and probably less code in nsGlobalWindow.cpp too.

And I think you'd probably be better off if you split this patch in two. One part for the C++/IDL changes, and one for the UI part of things, as I'm unlikely to review the UI side of things, and the people who would review the UI side of things are unlikely to review the back end changes.

And also, I would encourage you to read through the diffs before attaching them and eliminating things like spurious white space changes, and using consistent indentation etc (which you do for the most part, but there's some places where your indentation style differs from surrounding code).

I'm fairly busy with other stuff right now, but I'd gladly review an updated patch for the back end changes here. If you don't see responses from me in bugzilla, feel free to drop me email directly. r- for now, but keep up the good work!
Attachment #358118 - Flags: review?(jst) → review-
Thanks Johnny, your comments meant a lot to me!
I discussed with David last week and I mentioned about combining the two events too.  While I was working on it I noticed that the two events were practically same event split by a small fine line.  So I fully agree with you that the events should be put together (and frankly, would make my job much easier!)

I like the idea of splitting this patch into two as well, especially since I'm starting to see myself focusing more on the backend rather than the UI. (The backend looks a lot more enjoyable to code with)

I'll fix the indentations asap!
Release Notes:
    * Fixed all spurious white space changes
    * Fixed all extra unwanted lines
    * Removed any problematic tabbing and fixed the format
    * Removed all extra white spaces after lines of code (so that the line ends right where the semi colons are)
    * Merged the two events nsDOMWindowMoveEvent and nsDOMWindowResizeEvent into nsDOMWindowMoveResizeEvent
    * Deleted nsDOMWindowResizeEvent from code
    * Altered all User Interfaces to interact with the new event
    * Splitted patch between Backend (Events, and cpp code) versus Frontend (User interface, event captures)
Attachment #358118 - Attachment is obsolete: true
Attachment #360909 - Flags: review?(jst)
Front end code (user interface)
Same release as notes above
Attachment #360910 - Flags: review?(mconnor)
Comment on attachment 360909 [details] [diff] [review]
0.5 release, Back end Events code only

On the whole this is starting to look really good! But I did find some issues, and some cleanup and naming issues to still deal with here.

- In content/events/src/Makefile.in:

                 nsQueryContentEventHandler.cpp \
                 nsDOMProgressEvent.cpp \
+        nsDOMWindowMoveResizeEvent.cpp \

Fix indentation (tab, or lack thereof?)

- In nsDOMEvent::DuplicatePrivateData():

+    case NS_WINDOWMOVERESIZE_EVENT:
+    {
+      newEvent = new nsWindowMoveResizeEvent(PR_FALSE, msg);
+      break;

This code needs to actually copy the data (i.e. the members in nsWindowMoveResizeEvent) too, not just create a new event, right?

- In content/events/src/nsDOMWindowMoveResizeEvent.cpp:

+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 1998
+ * the Initial Developer. All Rights Reserved.

You should use a fresh copy of the license boilerplate in all new files, please see http://www.mozilla.org/MPL/boilerplate-1.1/

+ * Contributor(s):
+ *   Steve Clark (buster@netscape.com)
+ *   Ilya Konstantinov (mozilla-code@future.shiny.co.il)

Neither contributed to this file, right? Take credit yourself here :)

+  /*For course OSD700 reference, temporary: NS_ASSERTION = "Test", 2nd part = error text*/

Comments like these are fine for now as long as they serve a purpose for you, but these should be removed before landing this in the Mozilla repository.

+  //If this event is a windowmoveresize event (to prevent errors), then we can set the aIsMove parameter

Please wrap this long comment (and any other long comments or lines of code you write as well) to fit in 80 columns.

+/* readonly attribute boolean isBy; */
+NS_IMETHODIMP nsDOMWindowMoveResizeEvent::GetIsBy(PRBool *aIsBy)

"isBy" doesn't make much sense here w/o a lot of context. Maybe invert its meaning and rename it something like "isAbsolute"?

+  if (mEvent->eventStructType == NS_WINDOWMOVERESIZE_EVENT) {
+    nsWindowMoveResizeEvent* event = static_cast<nsWindowMoveResizeEvent*>(mEvent);
+    *aIsBy = event->mIsBy;
+    return NS_OK;
+  }
+  return NS_OK;  // Don't throw an exception

You should set *aIsBy (or *aIsAbsolute) to something here. PR_FALSE is probably fine. Not setting it could lead to undefined behavior in calling code. Same goes for all other getters that follow this pattern.

+/* readonly attribute long paramX; */
+NS_IMETHODIMP nsDOMWindowMoveResizeEvent::GetParamX(PRInt32 *aParamX)

Maybe just name this "x", instead of "paramX"? Or did that conflict with some existing code or something?

- In content/events/src/nsDOMWindowMoveResizeEvent.h:

+class nsDOMWindowMoveResizeEvent : public nsIDOMWindowMoveResizeEvent,
+                             public nsDOMEvent

Fix the indentation on the second line there.

- In dom/public/idl/events/Makefile.in:

         nsIDOMCommandEvent.idl            \
         nsIDOMMessageEvent.idl            \
+    nsIDOMWindowMoveResizeEvent.idl       \
         $(NULL)

Fix indentation.

- In dom/public/idl/events/nsIDOMWindowMoveResizeEvent.idl:

+interface nsIURI;

I don't see this interface used in this file, is this forward declaration needed?

+  readonly attribute boolean isBy;
...
+  readonly attribute long paramX;

See naming comment earlier...

- In dom/src/base/nsDOMClassInfo.cpp:

+  // windowMove/Resize Events

There's only one event now. Update the comment.

- In nsDOMClassInfo::Init():

+  //Window Move / Resize Events

Ditto.

- In FireWindowMoveResizeEvent():

+  //Update 0.4

Remove this comment at some point...

+  //This now uses the two custom events nsDOMWindowResizeEvent and nsDOMWindowMoveEvent

There's only one event... And this file (probably most of the files you're changing), typically have a space after the comment marker '//' for better readability. Please follow that convention when it applies.

- In nsGlobalWindow::MoveTo():

   if (!CanMoveResizeWindows() || IsFrame()) {
+    nsCOMPtr<nsIDOMDocument> topDoc;
+    this->GetDocument(getter_AddRefs(topDoc));
+    FireWindowMoveResizeEvent(topDoc, PR_TRUE, PR_FALSE, aXPos, aYPos);

This isn't quite what we want. We'll get here if the IsFrame() check above returns true. You can't move or resize frames with these APIs, so that case should remain a silent no-op. We only want to do this if !CanMoveResizeWindows(). Also, this->GetDocument() doesn't return the top document as the name topDoc makes it seem like you want here. And I think you're right, we want the top document, like nsGlobalWindow::FireAbuseEvents() does. Maybe the best option here would be to make FireWindowMoveResizeEvent() a non-static member function of nsGlobalWindow, and have it dig out the top document, like nsGlobalWindow::FireAbuseEvents() does. Same goes for all other places where FireWindowMoveResizeEvent() is called.
Attachment #360909 - Flags: review?(jst) → review-
Sorry for the late reply, Johnny!
Thank you very much for your review, this clarifies a lot for me!
I'll get to work asap and fix these problems.

Just to clarify though, does this mean tab characters are okay in the make.in files?
- In nsDOMEvent::DuplicatePrivateData():

+    case NS_WINDOWMOVERESIZE_EVENT:
+    {
+      newEvent = new nsWindowMoveResizeEvent(PR_FALSE, msg);
+      break;

To be honest, Johnny, I don't think this code is even needed.
Is there a certain reason this might ever be ran?
I don't know the code too well, and since I saw all the other events mentioned here I assumed I might need it here also.  But... assumptions are rarely true =)
(In reply to comment #18)
> Just to clarify though, does this mean tab characters are okay in the make.in
> files?

Some versions of make (e.g. GNU make) require tabs on command lines.
http://www.gnu.org/software/make/manual/make.html#Rule-Syntax
*  Fixed all lines with more than 80 chars (moved em to next line!) in the backend code.
    * Added new backend code: Checks a URI for its permissions and capacity before firing event.
    * Allows URIs with "Allow_Action" to bypass "CanMoveResizeWindow()" function
    * Added new in UI, permissions management for move/resizes.
    * New menu items in the Infobar: Always Allow, Always Deny. Adds the current URI into the permissions management
    * Removed resizedWindow.xul. We don't need it... really. 

This also includes all the changes based on your feedback, Johnny.
Thanks again for the review =)
Attachment #360909 - Attachment is obsolete: true
Attachment #363223 - Flags: review?(jst)
Front end code.
Same as the Backend for release details.
Attachment #360910 - Attachment is obsolete: true
Attachment #363225 - Flags: review?
Attachment #360910 - Flags: review?(mconnor)
Attachment #363225 - Flags: review? → review?(mconnor)
Attached patch Infobar Backend code Release 0.7 (obsolete) — Splinter Review
Release Notes:

    * Fixed a compilation error in the locales makefile that was overlooked last patch
    * Events are now fired regardless if the uri is stored in permissionmanager or not
    * Renamed several labels and menu options
    * Removed a lot of whitespace/80 line column problems in front end code
    * Removed extra text in browser.dtd
    * Removed "Do not show infobar", "Allow sites to move/resize window", "Disable Sites moving/resizing window" options from the infobar
    * Removed 4 functions from gWindowResizedObserver
Attachment #363223 - Attachment is obsolete: true
Attachment #367137 - Flags: review?(jst)
Attachment #363223 - Flags: review?(jst)
Attached patch Front End UI Infobar Release 0.7 (obsolete) — Splinter Review
Same release notes as above
Attachment #363225 - Attachment is obsolete: true
Attachment #367138 - Flags: review?
Attachment #363225 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Keywords: student-project
Attachment #367138 - Flags: review? → review?(mconnor)
Replaces the previous Front end code.
Some access keys were changed.
Also fixed a bug where the infobar would stop displaying properly.

Mochitest included with 28 tests.
Attachment #367138 - Attachment is obsolete: true
Attachment #370342 - Flags: review?
Attachment #367138 - Flags: review?(mconnor)
Comment on attachment 370342 [details] [diff] [review]
Front End UI Infobar Release 0.8 Mochitest edition

adding mconnor, as this was missed (again).
Attachment #370342 - Flags: review? → review?(mconnor)
Comment on attachment 370342 [details] [diff] [review]
Front End UI Infobar Release 0.8 Mochitest edition

I'll try to get to this in the next week, sorry for the delays (though patch v0.8 makes me think this is premature... :))
Attached patch Front End UI Infobar Release 0.9 (obsolete) — Splinter Review
*  In the Options, Content Tab:
          o Added a checkbox to options: Notify Move/Resize. This governs the resizeWindow.show_infobar preference
          o Added Exceptions button beside it, which opens the permissions manager to add sites to the allow/block list 
    * This patch ties all the work together and gives the features a fully accessible interface
    * Added one new test to Mochitest which tests the preference
    * Fixed a bug where there were two of the same preference names in about:config (pref was added to the wrong file)
    * Changed some variable naming conventions (in nsGlobalWindow.cpp is isAbsolute)
    * Fixed a bug where isAbsolute is flipped when sent from nsGlobalWindow.cpp
    * Added a confirmation to "Allow Once", and it shows the x and y coordinates as well as the command being blocked
          o Reason being users weren't able to see what parameters were blocked.
          o To allow users some freedom and knowledge about what the blocked command was before allowing it 
    * Fixed a bug where javascript can crash if you called "Edit options", because it was missing a try and catch block
Attachment #370342 - Attachment is obsolete: true
Attachment #372189 - Flags: review?
Attachment #370342 - Flags: review?(mconnor)
Attachment #372189 - Flags: review? → review?(mconnor)
Same release notes as above.
Attachment #367137 - Attachment is obsolete: true
Attachment #372190 - Flags: review?(jst)
Attachment #367137 - Flags: review?(jst)
*  This patch is mostly changes to the code to make it work in the latest firefox repository
    * Changed the event number to 39. This number will need to be changed if 39 is taken later
    * Changed the order of the nsClassInfo.cpp declarations (leaving mine always at the bottom)
    * Fixed a number of issues where code conflicted with the newer code in the newer repository.
Attachment #372190 - Attachment is obsolete: true
Attachment #374550 - Flags: review?(jst)
Attachment #372190 - Flags: review?(jst)
Attachment #374550 - Attachment is obsolete: true
Attachment #374551 - Flags: review?(mconnor)
Attachment #374550 - Flags: review?(jst)
Attachment #374550 - Attachment is obsolete: false
Attachment #374550 - Flags: review?(jst)
Attachment #372189 - Attachment is obsolete: true
Attachment #372189 - Flags: review?(mconnor)
Okay, the open source development course is now completed.
I will call this patch completed.

Woohoo.
Sorry for the slow responses here Tony, I'm planning on looking at your patch next week.
I haven't audited the whole patch, but...

(From update of attachment 374551 [details] [diff] [review])
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
...
>+    var message = "";
>+    message += isMove ? "Reposition your browser" : "Resize your browser" ;
>+    message += isAbsolute ? " to " : " by ";
>+    message += "(" + x + ", " + y + ")?";
>+    var confirm = window.confirm(message);

The strings in dialogs/message windows shown to the user shoud be localizable. Or am I missing something?
That is very true, I'll get to fixing that asap.
Sorry for the lack of updates lately.
I've made a change and moved the text to the properties file.

Thanks, 
Tony Lai
Attachment #374551 - Attachment is obsolete: true
Attachment #386835 - Flags: review?
Attachment #374551 - Flags: review?(mconnor)
Attachment #386835 - Flags: review? → review?(mconnor)
mconnor/jst,

This bug has gotten pretty stale.  Any thoughts on when you might be able to do these reviews?  Also, any advice on how we can avoid this sort of thing with other similar student project bugs in future?
Just thought I'd add a comment here to show I'm still here.
If there are any immediate changes required to the patch, please let me know here asap.

Thanks!
Tony Lai
Since I've failed over and over again to review this in any reasonably timely manner and happened to find this in my still large, but much more manageable, request queue, I decided to do the legwork to bring this up to current m-c myself, to save Tony the effort here. In doing so, I found a couple of things that I decided was worth dealing with here, so I did that as well. What changed was:

  - event name changed from WindowMoveResizeEvent to WindowMoveResizeBlockedEvent
  - event API changed from having two boolean attributes (isMove and isRelative)
    to one string argument named "operation", since I found more cases that users
    of this should be able to deal with that couldn't be covered with the
    original API. Those cases are innerWidth/Height, outerWidth/Height, and
    sizeToContent

Tony, could you have a look at this and see if you agree with this change to your patch? Obviously the frontend patch will need updating to reflect this change as well, but we can wait with that until this is decided upon and landed IMO. If you agree with this change to your original approach, I can go ahead and land this backend change.

And again, apologies for leaving this fall through the cracks way too many times :(
Attachment #426410 - Flags: review?(Squally425)
Attachment #426410 - Attachment is patch: true
Attachment #426410 - Attachment mime type: application/octet-stream → text/plain
Hmm, I just realized that I had a couple of patches in my queue when diffing, so one hunk in that patch fails to apply in nsDOMClassInfo.cpp. Trivial to apply by hand though, but I can re-diff if needed.
Comment on attachment 386835 [details] [diff] [review]
Move/Resize FrontEnd UserInterface Version 1.0 Fixed

I am no longer actively working on Firefox, please find another reviewer.
Attachment #386835 - Flags: review?(mconnor)
Attachment #374550 - Flags: review?(jst)
Hi Johnny, sorry, I've been really busy lately.
I'll take a look at the changes as soon as I can.
Really appreciate the update and review though.  Thank you very much!
This really should have someone from UX reviewing before it goes any further. It's not clear to me that this is a change we need.
Not sure what the current status is since most activity occurred more than a year ago.  In terms of UX, we are moving away from info bars.  A possible approach could be similar to this: https://bug588317.bugzilla.mozilla.org/attachment.cgi?id=466965 but with a small resize arrow in the corner of the icon instead of the blocked glyph.  Advantage is that it doesn't shift the contents of the content area, isn't as distracting, and provides a consistent location of the user to act on the preference.
Hi everyone,

Sorry, I haven't worked on this bug in over a year.
I've been really busy with everything else.
If anyone else would like to pick up where I left off, that'd be great.
I'm more than happy to discuss with anyone who'd like to look at this bug again.

I apologize for any coding that appears to be difficult to understand.
This was a project back when I was a student in Seneca, and I've moved on to other projects.

Thanks
Tony Lai
well, it's pointless if resizeBy or sizeToContent doesn't work :)

i'll create a seperate bug for that!
Comment on attachment 426410 [details] [diff] [review]
Updated version of Tony's fix.

Clearing review request as this no doubt needs a bunch of work to move forward again at this point.
Attachment #426410 - Flags: review?(Squally425)
You need to log in before you can comment on or make changes to this bug.