33203 - Form Submission Observer Changes

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Doug Turner (:dougt)]

I am working on integrating security into the browser.  One of the needed 
security dialogs is to alert the user when they are posting to an insecure site.  
They way that one is notified of a form submission currently is through the 
nsIFormSubmitObserver interface.  There are a couple requirements that are 
missing in this current mechanism.

1.  Need to be able to cancel a form submission.  I need to be able to return a 
status code from the notification which will prevent the form submission from 
occurring.  This is so that when I present a dialog to the user, they will have 
an option to cancel the form submit and prevent information being posted to an 
insecure site.

2.  Need to be able to register on a per nsIDOMWindow bases.  Currently, to 
receive notifications, you register with a global service, nsIObserverService.  
This is suboptimal because every form submission is sent to every observer in 
every context.  I would like to be notified only when a form submission happens 
'in' a particular nsIDOMWindow.  So, the registration of the 
nsIFormSubmitObserver should be based on a window.

?? Maybe it is possible to QI the nsIContent for the containing nsIDOMWindow, 
then ask if the nsIDOMWindow that I am concerned with contains the nsIConent's 
DOM window.  

3.  Lastly, I need to know how to go from a nsIContent in Notify(nsIContent* 
formNode) to a nsIURL.  Eric mentioned that he may be able to pass this via the 
interface.

Comment 1

[Eric Pollmann]

Let's aim for M15 - if all goes according to plan!  :)

Doug, you mentioned a per-window security object that monitors the current 
security state of the page - can you point me to where that lives and possibly 
explain relevent parts of it so that I can understand how this form submit 
observer mechanism should work?  Thanks!

Comment 2

[Doug Turner (:dougt)]

sure.  Take a look at:

http://lxr.mozilla.org/seamonkey/source/extensions/psm-glue/src/nsSecureBrowserU
IImpl.cpp#117

This is called from javascript the first time the page loads.  As you can see, I 
make myself the DocLoaderObserver of the nsIDOMWindow I pass in which is 
|window.content|.  What I would like to do is only recieve form post events on 
this nsIDOMWindow.

Comment 3

[Eric Pollmann]

I'll have to push this off until M16, sorry!

Comment 4

[Eric Pollmann]

I have this pretty much done.  I'll attach the diffs tomorrow.  I decided that 
it would be simplest to just use the existing nsIFormSubmitObserver mechanism, 
and add an extra parameter (for the nsIDOMWindow) on the notify method.

This means you'll have to check if the nsIDOMWindow is the same one the security 
glue is hooked up to.  I figure this check has to be made, either in the 
security glue or in the form frame and it is a *lot* easier in the security 
glue.

I've made all the needed changes in psm-glue, wallet, and layout (including code 
to register as an observer in nsSecureBrowserUIImpl::Init and check if the 
submit is for this nsIDOMWindow in ::Notify).  It is running fine in my simple 
tests, at least, as well as form submission works in general with bug 33952.  
Printing "Got a notify" whenever a form submits.  Will attach diffs shortly...

Comment 5

[Eric Pollmann]

Oh, yeah, and to stop a form from submitting you just need to return 
any NS_ERROR_XXX code from the Notify method.  :)

Comment 6

[Eric Pollmann]

Attachment: Patch for extensions/wallet

Comment 7

[Eric Pollmann]

Attachment: Patch for extension/psm-glue

Comment 8

[Eric Pollmann]

Attachment: Patch for layout/html/forms

Comment 9

[Eric Pollmann]

Oops, one update:

NS_IMETHODIMP nsSecureBrowserUIImpl::Notify(nsIContent* formNode, nsIDOMWindow* 
window) 
{
  if (!formNode) {
    return NS_ERROR_FAILURE;
  }
  if (!window || (mWindow != window)) {
       return NS_ERROR_FAILURE;
  }
  printf("Got notified of form submit on window %p.\n",window);
  return NS_OK;
}

Should read:

NS_IMETHODIMP nsSecureBrowserUIImpl::Notify(nsIContent* formNode, nsIDOMWindow* 
window) 
{
  if (!formNode) {
    return NS_ERROR_FAILURE;
  }
  if (!window || (mWindow != window)) {
       return NS_OK;
  }
  printf("Got notified of form submit on window %p.\n",window);
  return NS_OK;
}

This will allow forms to submit if there is more than one window open.  *blush*

Comment 10

[Eric Pollmann]

Hi Doug,

Well I just got back to reading the original requirements.  One more question, 
for part 3) when you say you need to get the nsIURL, do you need to get the url 
that the request is coming from, the URL that the form is being submitted to, or 
both?  Thanks.

Comment 11

[Doug Turner (:dougt)]

the post destination would be required.

Comment 12

[Eric Pollmann]

Thanks, no problem, I'll attach the updated diffs.

Comment 13

[Eric Pollmann]

Attachment: Updated patch for wallet/src

Comment 14

[Eric Pollmann]

Attachment: Updated patch for psm-glue/src

Comment 15

[Eric Pollmann]

Attachment: Updated patch for forms

Comment 16

[Doug Turner (:dougt)]

changes look good.  r=dougt

Comment 17

[Eric Pollmann]

Fixes checked in.  The tree was red every time I looked at it from the time I've 
had the changes done until now.  :S

Doug, I've also changed nsIFormSubmitObserver so it derives from nsIObserver, 
per the review.  The changes in psm-glue were not checked in (you will now not 
need to implement nsIObserver):

+class nsSecureBrowserUIImpl : public nsIDocumentLoaderObserver,
+                              public nsIFormSubmitObserver,
+                              public nsIObserver,
+                              public nsSecureBrowserUI

becomes:

+class nsSecureBrowserUIImpl : public nsIDocumentLoaderObserver,
+                              public nsIFormSubmitObserver,
+                              public nsSecureBrowserUI

and

+NS_IMPL_ISUPPORTS4(nsSecureBrowserUIImpl,
+                                  nsIDocumentLoaderObserver,
+                                  nsIFormSubmitObserver,
+                                  nsIObserver,
+                                  nsSecureBrowserUI); 

becomes

+NS_IMPL_ISUPPORTS3(nsSecureBrowserUIImpl,
+                                  nsIDocumentLoaderObserver,
+                                  nsIFormSubmitObserver,
+                                  nsSecureBrowserUI); 

otherwise, psm-glue diffs are pretty much the same.  Good luck!

(QA note: there is no way to verify this change by running the program.)

Comment 18

[Eric Pollmann]

Backed out the 'streamlining' changes mentioned above per bug 36830.  Form 
submit observers will have to implement nsIObserver until/unless the problem 
behind bug 36830 is fixed.

Comment 19

[ckritzer (gone)]

dougt, do you have a specific site/testcase I can use to verify this bug?  
Thanks! - ckritzer

Comment 20

[Doug Turner (:dougt)]

Just verify that wallet still works.  Also, you can see if you get a warning 
message when you try to post to an insecure site.

Comment 21

[Stephen P. Morse]

Change "wallet" in above comment to "single signon".  Wallet's capturing of data 
is called into play explicitly and is not dependent on an observer.  Single 
signon, on the other hand, is dependent on the form-submission observer.

Comment 22

[ckritzer (gone)]

test mac

Comment 23

[ckritzer (gone)]

test linux

Comment 24

[ckritzer (gone)]

test windows

Comment 25

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- MacOS9 2000-07-06-08-M17 Commercial
- Linux6 2000-07-07-10-M17 Commercial
- Win98  2000-07-07-13-M17 Commercial