[tabbrowser.xml] onLocationChange: aRequest can be null for an error page.

RESOLVED FIXED in seamonkey2.14

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

({sec-moderate})

Trunk
seamonkey2.14
sec-moderate

Firefox Tracking Flags

(firefox-esr10 unaffected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
From Bug 782033:
>>       if (aRequest && this.popupNotifications &&
>>           (aWebProgress.isLoadingDocument ||
>>-           !Components.isSuccessCode(aRequest.status)))
>>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> [So, it turns out to be a little harder than that, in that aRequest can be null for an error page....]
> 
>>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>>                 this.mBrowser.mIconURL = "";
>>                 this.mFeeds = [];
> [Here, things are trickier... dealing with null aRequest is going to need a follow-up bug.]
>
Depends on: 782033

Comment 2

6 years ago
>       if (aRequest && this.popupNotifications &&
>           (aWebProgress.isLoadingDocument ||
>-           !Components.isSuccessCode(aRequest.status)))
>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
Well, the obvious replacement is
if (this.popupNotifications &&
    (aRequest && aWebProgress.isLoadingDocument ||
     aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
aRequest is null for a tab switch or some error pages.
aFlags is 0 for a tab switch or normal pages.
This way the code never fires for a tab switch because aRequest is null and aFlags is zero.
However, there is also a new LOCATION_CHANGE_SAME_DOCUMENT flag for an anchor scroll or a HTML5 history API call. So if we set aFlags to this value for a tab switch then we could simplify this to
if (this.popupNotifications &&
    !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))

>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>                 this.mBrowser.mIconURL = "";
>                 this.mFeeds = [];
This probably needs to be
if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
    !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))

However that doesn't work for the userTypedClear code which probably wants this:
if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
    (this.mBrowser.userTypedClear > 0 ||
     aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
  this.mBrowser.userTypedValue = null;

As for the rest of the code in that block it should probably die, it's there to load groupmarks which we don't support any more.
What sec rating do you think this should have, Philip?
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 4

6 years ago
Well, the original bug 623155 was sec-moderate. We implemented mitigation in the form of attachment 576901 [details] [diff] [review], although there are error pages for which that did not work. The work done in bug 782033 and here in this bug together should close that loophole.
Keywords: sec-moderate
(Assignee)

Comment 5

6 years ago
Created attachment 652482 [details] [diff] [review]
Patch v1.0 Proposed fix.

>>       if (aRequest && this.popupNotifications &&
>>           (aWebProgress.isLoadingDocument ||
>>-           !Components.isSuccessCode(aRequest.status)))
>>+           aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> Well, the obvious replacement is
> if (this.popupNotifications &&
>     (aRequest && aWebProgress.isLoadingDocument ||
>      aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
> aRequest is null for a tab switch or some error pages.
> aFlags is 0 for a tab switch or normal pages.
> This way the code never fires for a tab switch because aRequest is null and aFlags is zero.
> However, there is also a new LOCATION_CHANGE_SAME_DOCUMENT flag for an anchor scroll or a HTML5 history API call. So if we set aFlags to this value for a tab switch then we could simplify this to
> if (this.popupNotifications &&
>     !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
Done.

>>             onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
>>+              const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>>               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>>                 this.mBrowser.mIconURL = "";
>>                 this.mFeeds = [];
> This probably needs to be
> if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
>     !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
OK.

> However that doesn't work for the userTypedClear code which probably wants this:
> if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
>     (this.mBrowser.userTypedClear > 0 ||
>      aFlags & nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
>   this.mBrowser.userTypedValue = null;
OK.

> As for the rest of the code in that block it should probably die, it's there to load groupmarks which we don't support any more.
OK.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #652482 - Flags: review?(neil)
(Assignee)

Comment 6

6 years ago
Created attachment 652484 [details] [diff] [review]
Patch v1.0 Proposed fix.

Oops wrong author again.
Attachment #652482 - Attachment is obsolete: true
Attachment #652482 - Flags: review?(neil)
Attachment #652484 - Flags: review?(neil)

Comment 7

6 years ago
Comment on attachment 652484 [details] [diff] [review]
Patch v1.0 Proposed fix.

With this patch, visiting wxyz:// now correctly:
* removes transient doorhangers
* removes transient notifications
* clears popup statusbar notification
* clears feed urlbar notification
* resets the urlbar to the error url

>-                if (this.mLocationChangeCount > 0 ||
>-                    aLocation.spec != "about:blank")
>-                  ++this.mLocationChangeCount;
>-
>-                if (this.mLocationChangeCount == 2) {
>-                  this.mTabBrowser.backBrowserGroup = [];
>-                  this.mTabBrowser.forwardBrowserGroup = [];
When I said "this should probably die" I meant the whole support for browser groups should be removed all at once in its own bug, rather than doing it piecemeal. For now, just put those lines back where they were. (Having thought about it a little more it's probably best that they don't check the flags at all anyway.) r=me with that fixed.
Attachment #652484 - Flags: review?(neil) → review+

Comment 8

6 years ago
Comment on attachment 652484 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+      if (this.popupNotifications &&
>+          !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
>         this.popupNotifications.locationChange();
Oops, this doesn't quite work, because it triggers when you switch tabs. A bit of a hacky workaround is to get the tab-switching code to send the LOCATION_CHANGE_SAME_DOCUMENT flag in this case.
(Assignee)

Comment 9

6 years ago
Created attachment 652794 [details] [diff] [review]
Patch v1.1 more fixes r=Neil.

>>-                  this.mTabBrowser.forwardBrowserGroup = [];
> When I said "this should probably die" I meant the whole support for browser 
> groups should be removed all at once in its own bug, rather than doing it 
> piecemeal. For now, just put those lines back where they were. (Having thought 
> about it a little more it's probably best that they don't check the flags at all 
> anyway.) r=me with that fixed.

>>+      if (this.popupNotifications &&
>>+          !(aFlags & nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT))
>>         this.popupNotifications.locationChange();
> Oops, this doesn't quite work, because it triggers when you switch tabs. A bit 
> of a hacky workaround is to get the tab-switching code to send the 
> LOCATION_CHANGE_SAME_DOCUMENT flag in this case.

>> http://hg.mozilla.org/comm-central/rev/76ef05e53378
>>>>+                              tabListener.mStateFlags);
>>> State flags are not the same as location flags! Pass 0 for now. r=me with that fixed.
>> Fixed.
> Oops. Firstly, you passed the 0 in the wrong argument. (Partly my fault for 
> getting you to move it.) And also for bug 782516 I think you actually need to 
> pass LOCATION_CHANGE_SAME_DOCUMENT anyway.
Fixed.
Attachment #652484 - Attachment is obsolete: true
Attachment #652794 - Flags: review?(neil)
(Assignee)

Comment 10

6 years ago
Comment on attachment 652794 [details] [diff] [review]
Patch v1.1 more fixes r=Neil.

Neil says "looks ok" over IRC.
Attachment #652794 - Attachment description: Patch v1.1 more fixes. → Patch v1.1 more fixes r=Neil.
Attachment #652794 - Flags: review?(neil) → review+
(Assignee)

Comment 11

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f9f56f8caca0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.14

Updated

6 years ago
status-firefox-esr10: --- → unaffected

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.