Please see https://bugzilla.mozilla.org/show_bug.cgi?id=762593#c57

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #0) > Please see https://bugzilla.mozilla.org/show_bug.cgi?id=762593#c57 Please take a look at bug 897240 to see if that effectively solves this. It doesn't add an observer topic, but it would create the infrastructure to allow for observers to be easily added when needed.

What's the use case for having this? It's not at all clear in the other bug.

(In reply to Justin Dolske [:Dolske] from comment #2) > What's the use case for having this? It's not at all clear in the other bug. I think the OP might be the best one to answer this, but I think he wanted a service that would notify code that might use the knowledge of insecure passwords somehow i.e. reporting for example. Bug 897240 would create a security service that would notify for general security messages, not just insecure passwords. I would just close the ticket as WONTFIX, or DUP if Tetsuharu doesn't reply. I think he's had enough time to reply to comment 1

Oh... I missed comment #3... I proposed this feature for using other modules, logging, and low-coupled design. (In reply to Ivan Alagenchev :ialagenchev from comment #1) > Please take a look at bug 897240 to see if that effectively solves this. It > doesn't add an observer topic, but it would create the infrastructure to > allow for observers to be easily added when needed. O.K. When I have some free time, I'll add observers to it.

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #4) > Oh... I missed comment #3... > > I proposed this feature for using other modules, logging, and low-coupled > design. > > > (In reply to Ivan Alagenchev :ialagenchev from comment #1) > > Please take a look at bug 897240 to see if that effectively solves this. It > > doesn't add an observer topic, but it would create the infrastructure to > > allow for observers to be easily added when needed. > > O.K. When I have some free time, I'll add observers to it. The observers would be needed only when there is a need for them. The only observers we need ATM that I know of are the ones for the SecurityReportTool. I don't think anything needs to be done ATM

I am writing an addon to notify users about insecure passwords: https://github.com/alagenchev/ArktikFoxAddOn. The addon was inspired by feedback on twitter:https://twitter.com/passy/status/390790063451152384/ While a lot of use cases are covered by the addon, for it to be 100% accurate I would have to do checks across origin boundaries. As a result, such an addon cannot be truly accurate without the help of the browser. I've decided to reopen this bug to add observer notifications to support it. I'll be adding the browser support myself.

This is going to be done despite of Bug 897240, since that one is quite involved and it doesn't look like it will be implemented soon.

Adding a work in progress patch with no tests to get some initial feedback on the approach and thoughts

Comment on attachment 8333659 [details] [diff] [review] password_notifier_WIP.patch Review of attachment 8333659 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this, Ivan, The approach looks generally OK to me - but this is more Garrett's area. ::: xpcom/build/nsXPComInit.cpp @@ +82,5 @@ > #include "nsVariant.h" > > #include "nsUUIDGenerator.h" > > + Superfluous whitespace

(In reply to Mark Goodwin [:mgoodwin] from comment #9) > Comment on attachment 8333659 [details] [diff] [review] > password_notifier_WIP.patch Thanks for the feedback. I'll go back and clean up the whitespaces and add tests.

Comment on attachment 8333659 [details] [diff] [review] password_notifier_WIP.patch Review of attachment 8333659 [details] [diff] [review]: ----------------------------------------------------------------- You don't need to create a new interface for Insecure Password Notifications. The first parameter of notifyObservers is supposed to correspond to the source of the notification [0], so it would make sense for you to pass the document (domDoc) here. That way, an observer can query it for both its URI and also the markup that contains the password field, which might be useful. You can pass the offenderType in the someData parameter. [0] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIObserverService#notifyObservers%28%29 ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm @@ +153,5 @@ > + let notification = Cc["@mozilla.org/insecurepassword/notification;1"] > + .createInstance(Ci.nsIInsecurePasswordNotification); > + notification.init("form", aForm.action); > + > + Services.obs.notifyObservers(notification, "insecure-password-detected", null); This should be abstracted into a function like this._notifyObservers.

Comment on attachment 8333659 [details] [diff] [review] password_notifier_WIP.patch Review of attachment 8333659 [details] [diff] [review]: ----------------------------------------------------------------- I seem this patch cannot handle that which a tab has an insecure passwords fields. If we uses pages url for this purpose, we cannot handle the case which some tabs open pages which have same page url. We need to detect tabs not only url if we uses this notification from other modules including add-ons.

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #12) > Comment on attachment 8333659 [details] [diff] [review] > password_notifier_WIP.patch > > Review of attachment 8333659 [details] [diff] [review]: > ----------------------------------------------------------------- > > I seem this patch cannot handle that which a tab has an insecure passwords > fields. If we uses pages url for this purpose, we cannot handle the case > which some tabs open pages which have same page url. We need to detect tabs > not only url if we uses this notification from other modules including > add-ons. That's interesting... Does it really make a difference though? Wouldn't it make sense that the event is associated with the currently active tab?

If you pass the document (or an element of the document that can be QI'ed to the document) as the subject (as suggested in comment 11), you would be able to distinguish the tab that made the offending request.

(In reply to Ivan Alagenchev :ialagenchev from comment #13) > Does it really make a difference though? Wouldn't it make sense that the > event is associated with the currently active tab? If the page is changed by script, the page url is not the unique identifier to detect tabs. I feel we may be able to notify the event with the original tab if we combine e10s message manager into this observer flow: https://developer.mozilla.org/en-US/docs/The_message_manager.

(In reply to Garrett Robinson [:grobinson] from comment #14) > If you pass the document (or an element of the document that can be QI'ed to > the document) as the subject (as suggested in comment 11), you would be able > to distinguish the tab that made the offending request. Hi Garrett, I'm not clear how to do that? Could you be more specific?

Services.obs.notifyObservers(aForm, "insecure-password-detected", offenderType)

(In reply to Garrett Robinson [:grobinson] from comment #17) > Services.obs.notifyObservers(aForm, "insecure-password-detected", > offenderType) What I was unclear about is how to distinguish the tab, once I have the document. I've been looking at the methods available, but I don't see a way to get the tab, or its index in the tab container, or something that the addon can use to look up the correct tab: https://addons.mozilla.org/en-US/developers/docs/sdk/latest/modules/sdk/tabs.html

Each tab has a unique "inner window ID" (although IIRC, not every inner window ID necessarily corresponds to a tab). You should be able to to aForm->owningDocument->window->innerWindowID. I don't see how the Jetpack API is relevant here.

(In reply to Garrett Robinson [:grobinson] from comment #19) > I don't see how the Jetpack > API is relevant here. The relevance is that as suggested by https://bugzilla.mozilla.org/show_bug.cgi?id=899983#c15, an addon might need a way to identify the correct tab that contains the document which triggered the event. Do you think that might not be necessary?

Right - Jetpack is just an API on top of regular chrome (goal is to abstract away details like this). So as long as you can do it in chrome code, that's all I would argue this bug should handle. If you wanted to extend the Jetpack API to integrate, that should be a separate issue - and maybe something for that team to implement :). I'm not sure how tabs are represented inside Gecko. It would be worth doing some research (maybe dig into the Jetpack code) to see how easy it is to tie together a window/innerWindowID and other, maybe more useful, representations of "tabs".

(In reply to Garrett Robinson [:grobinson] from comment #21) I see. Thanks for the explanation. I will upload a patch that reflects your suggestions for your feedback shortly.

caught a small issue with iframes, so I redid the patch

Comment on attachment 8341434 [details] [diff] [review] 899983.patch Review of attachment 8341434 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good, Ivan! Check out my notes on the tests - you want to be careful about assumptions that could lead to race conditions. ::: toolkit/components/passwordmgr/test/test_bug_899983.html @@ +14,5 @@ > +<div id="content"> > +<p>This page is served with an iframe with insecure password field.</p> > + <iframe src > + ="http://example.com/browser/browser/devtools/webconsole/test/test-iframe-762593-insecure-frame.html"> > + </iframe> nit: put the iframe all on one line @@ +41,5 @@ > + else if ("form" == aData) { > + aSubject = aSubject.QueryInterface(Ci.nsIDOMHTMLFormElement); > + receivedFormWarning = true; > + } > + ok(aSubject != null, "a subject was passed"); You can just do ok(aSubject, "a subject was passed"), because null is "false-y" in JS. @@ +54,5 @@ > + is(receivedFormWarning, true, "received form warning"); > + SimpleTest.finish(); > +} > + > +window.onload = startTest; Are you guaranteed to have received the notifications you're looking for before the page load fires? If not, this test might have some false negatives. @@ +56,5 @@ > +} > + > +window.onload = startTest; > + > +SimpleTest.waitForExplicitFinish(); Put this at the beginning of the script to avoid race conditions.

(In reply to Garrett Robinson [:grobinson] from comment #25) > Comment on attachment 8341434 [details] [diff] [review] > 899983.patch > > Review of attachment 8341434 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for feedback and the tips. > @@ +54,5 @@ > > + is(receivedFormWarning, true, "received form warning"); > > + SimpleTest.finish(); > > +} > > + > > +window.onload = startTest; > > Are you guaranteed to have received the notifications you're looking for > before the page load fires? If not, this test might have some false > negatives. I'm hoping that won't be a problem, since I based my test on this one: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_basic_form_observer_autocomplete.html which listens for "passwordmgr-found-form" > @@ +56,5 @@ > > +} > > + > > +window.onload = startTest; > > + > > +SimpleTest.waitForExplicitFinish(); > > Put this at the beginning of the script to avoid race conditions. Done! Thanks for the suggestion.

Comment on attachment 8342724 [details] [diff] [review] 899983.patch I'm not the right person to review this, that's probably MattN. However I really don't think we should be using the observer service for notifications like this when we could have people register some callback function directly on InsecurePAsswordUtils.jsm. The observer service is a huge XPCOM kludge for passing notifications between unknown binary and JS callers and we don't need it for this.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #28) > Comment on attachment 8342724 [details] [diff] [review] > 899983.patch > > I'm not the right person to review this, that's probably MattN. However I > really don't think we should be using the observer service for notifications > like this when we could have people register some callback function directly > on InsecurePAsswordUtils.jsm. The observer service is a huge XPCOM kludge > for passing notifications between unknown binary and JS callers and we don't > need it for this. The goal of this is to expose it to add-ons. Is there a way to do that without using the observer service?

(In reply to Ivan Alagenchev :ialagenchev from comment #29) > The goal of this is to expose it to add-ons. Is there a way to do that > without using the observer service? HOw about https://developer.mozilla.org/en-US/docs/The_message_manager ? I seem this is the other ways to expose to add-ons.

Comment on attachment 8342724 [details] [diff] [review] 899983.patch Review of attachment 8342724 [details] [diff] [review]: ----------------------------------------------------------------- How about using a chrome-only DOM event (like DOMWillOpenModalDialog)? ::: toolkit/components/passwordmgr/test/test_bug_899983.html @@ +1,1 @@ > +<!DOCTYPE HTML> Please use a more descriptive filename (e.g. test_insecure_password_notification.html) and re-sort it in mochitest.ini

Would you prefer a chrome-only DOM event (like DOMWillOpenModalDialog)?

Yeah, DOM events are far better because they work well with the message manager in e10s.

Thank you everyone for your suggestions. I ran into some troubles trying to follow your tips. I've been trying to follow the documentation(https://developer.mozilla.org/en-US/docs/The_message_manager) on how to use the message manager and I sencerely fail to see how "sending a message is simple". I tried creating a message manager reference inside InsecurePasswordUtils like this: var gmm = Cc["@mozilla.org/globalmessagemanager;1"] .getService(Ci.nsIMessageSender); and that gave me NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService] I then tried to do the following: this.globalMessageManager = Cc["@mozilla.org/globalmessagemanager;1"] .getService(Ci.nsIMessageBroadcaster); after seeing another example in mxr and got: "gmm.sendAsyncMessage is not a function" lastly, using childprocessmanager simply doesn't raise any events: var gmm = Cc["@mozilla.org/childprocessmessagemanager;1"] .getService(Ci.nsIMessageSender); I tried to listen both in the addon and in the mochitest and nothing gets received when using childprocessmessagemanager: var lgmm = Cc["@mozilla.org/globalmessagemanager;1"] .getService(Ci.nsIMessageListenerManager); lgmm.addMessageListener("test", function(m) { Services.console.logStringMessage("test"); } ); Should I use a child process manager, or a global one? Does it make a difference in this case? Is there a working example anywhere of how I'm supposed to use the message manager? Any help is greatly appreciated.

Can someone please answer my last comment? I'm in the same time zone as Mozilla now, so it's impossible for me to login to irc during work hours.

Sorry, I didn't see your comment then. (In reply to Ivan Alagenchev :ialagenchev from comment #34) > I've been trying to follow the > documentation(https://developer.mozilla.org/en-US/docs/The_message_manager) > on how to use the message manager and I sencerely fail to see how "sending a > message is simple". Why aren't you going with the DOM event approach that I suggested in comment 32 and was accepted in comment 33? I'm also not sure where anyone said "sending a message is simple" in this bug but so you know how to do it: I think you would have to pass the content script's global to your JSM's function and call sendAsyncMessage on it. See https://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js?rev=08b4b4b4aef3&mark=196#191 for an example.

(In reply to Matthew N. [:MattN] from comment #36) > Sorry, I didn't see your comment then. > > (In reply to Ivan Alagenchev :ialagenchev from comment #34) > > I've been trying to follow the > > documentation(https://developer.mozilla.org/en-US/docs/The_message_manager) > > on how to use the message manager and I sencerely fail to see how "sending a > > message is simple". > > Why aren't you going with the DOM event approach that I suggested in comment > 32 and was accepted in comment 33? > > I'm also not sure where anyone said "sending a message is simple" in this > bug but so you know how to do it: I think you would have to pass the content > script's global to your JSM's function and call sendAsyncMessage on it. See > https://mxr.mozilla.org/mozilla-central/source/browser/base/content/content. > js?rev=08b4b4b4aef3&mark=196#191 for an example. Thank you for replying. The "simple" was referring to a sentence from the MDN documentation on message manager :-) I don't remember now why I didn't do the DOM event approach. I remember looking at it, but perhaps couldn't find an example of it. I will try to figure it out and will post here with questions if I have any.