Closed Bug 1407878 Opened 2 years ago Closed 2 years ago

Add a trigger when a password field is focused

Categories

(Toolkit :: Safe Browsing, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

(Blocks 2 open bugs)

Details

(Whiteboard: pwphish-content)

Attachments

(2 files, 3 obsolete files)

For password phishing, it is triggered asynchronously when a user focuses on a password field.
So here is the entry point for the whole procedure
Priority: -- → P2
Attached patch WIP patch (obsolete) — Splinter Review
Attached patch WIP patch based on bug 1384753 (obsolete) — Splinter Review
Attachment #8917643 - Attachment is obsolete: true
Attachment #8921741 - Attachment is obsolete: true
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review200252

That looks. I've got a few inline comments and one question.

::: commit-message-c6258:1
(Diff revision 1)
> +Bug 1407878 - Add a trigger when a password field is focused. r?francois

I would suggest giving a little more context here:

"Check URLs against the login reputation service when a password field is focused."

::: toolkit/components/passwordmgr/nsILoginManager.idl:227
(Diff revision 1)
>     */
>    void stopSearch();
>  
> +
> +  /**
> +   * Query the login reputation for a password field asynchronously.

"query the login reputation service"

Also, technically, we're querying URLs, not the password field itself, so maybe this?

    Query the login reputation service for the URLs associated with a password field.

::: toolkit/components/passwordmgr/nsLoginManager.js:76
(Diff revision 1)
>  
>  
>    _storage: null, // Storage component which contains the saved logins
>    _prefBranch: null, // Preferences service
>    _remember: true,  // mirrors signon.rememberSignons preference
> +  _loginReputationEnabled: false,  // mirrows safebrowsing.passwords.enabled preference

typo: "mirrors"

::: toolkit/components/passwordmgr/nsLoginManager.js:544
(Diff revision 1)
>  
>    stopSearch() {
>      this._autoCompleteLookupPromise = null;
>    },
> +
> +  // XXX: Right now we didn't cache the request or result in content side, so if

"Right now we don't cache the request or result in the content process, so if a user focuses a password field twice, we'll send a query each time."

Does it matter? I mean it's doing more IPC than necessary, but it will be very quick once we have an in-memory cache.

Also it's async and it requires user input to trigger. Maybe we don't need to worry about this.

::: toolkit/components/passwordmgr/nsLoginManager.js:549
(Diff revision 1)
> +  // XXX: Right now we didn't cache the request or result in content side, so if
> +  //      user focuses on a password field which we have already sent a query request
> +  //      for it before, we'll still trigger another query this time.
> +  queryLoginReputationAsync(aElement, aCallback) {
> +    if (!this._loginReputationEnabled || aElement.type !== "password") {
> +      // Don't query if login reputation isn't enabled or its not a

Maybe we should output a log message if it's not a password field?

::: toolkit/components/passwordmgr/nsLoginManager.js:566
(Diff revision 1)
> +        return;
> +      }
> +
> +      this._queryLoginReputationPromise = null;
> +
> +      log.debug("QueryLoginReputation invoked. Result is:", aResult);

nit: add a space after the colon:

    "QueryLoginReputation result: "

::: toolkit/components/reputationservice/ILoginReputation.idl:12
(Diff revision 1)
>  #include "nsISupports.idl"
>  
> +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> +interface ILoginReputationResult : nsISupports {
> +
> +  const unsigned short SAFE                 = 0;

There is also `VERDICT_TYPE_UNSPECIFIED`: https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/components/reputationservice/chromium/chrome/common/safe_browsing/csd.proto#254

If you add this one then our numbering will match the protobuf definition too.

::: toolkit/components/reputationservice/LoginReputation.cpp:56
(Diff revision 1)
> +    aQuery->GetActionUrl(actionUrl);
> +    LR_LOG(("Login reputation query parameters: formUrl(%s), actionUrl(%s)",
> +            formUrl.BeginReading(), actionUrl.BeginReading()));
> +  }
> +
> +  // XXX: Always return SAFE for now.

Maybe we should file a bug now for getting the verdicts from the real service and then link to it from here.

::: toolkit/components/satchel/nsFormFillController.cpp:922
(Diff revision 1)
>  }
>  
>  ////////////////////////////////////////////////////////////////////////
> +//// IQueryReputationObserver
> +
> +// XXX : The reputation query result for a password field should be

We could also file a bug here so that we have something concrete to link to.

::: toolkit/components/satchel/nsFormFillController.cpp:1456
(Diff revision 1)
>        mController->SetInput(nullptr);
>      }
>    }
>  
> +  if (mLoginManager) {
> +    mLoginManager->StopQueryLoginReputation();

When does `StopControllingInput()` get called? When the password field loses focus? When the form is submitted?

This seems like one of the corner cases you brought up in our last meeting.

I guess we could start by only cancelling the looking when the form is submitted. At that point, it's definitely too late.
Attachment #8923294 - Flags: review?(francois) → review-
(In reply to François Marier [:francois] from comment #4)
> That looks. I've got a few inline comments and one question.

That should have been "That looks good." :)
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review200298

Hello, I just saw this go by and have some drive-by comments:

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:370
(Diff revision 1)
> +    let messageData = { formOrigin,
> +                        actionOrigin
> +                      };

This should fail eslint as you should have a trailing comma in object and array literals. You should also move formOrigin to its own line.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:295
(Diff revision 1)
> +  doQueryLoginReputation({ formOrigin, actionOrigin, requestId
> +                         }, target) {

No need to wrap here at all (especially not in the middle of an object literal)

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:302
(Diff revision 1)
> +      actionUrl: actionOrigin
> +    }

Please run eslint on your change. This is missing a semicolon and trailing comma.

::: toolkit/components/passwordmgr/nsLoginManager.js:97
(Diff revision 1)
> +    this._queryLoginReputationPromise = null;
> +    this._loginReputationEnabled = Services.prefs.getBoolPref(LOGIN_REPUTATION_PREF);
> +    Services.prefs.addObserver(LOGIN_REPUTATION_PREF, this._observer);

Please put this in LoginHelper instead. We're trying to move away from the pref caching in nsLoginManager. See https://dxr.mozilla.org/mozilla-central/rev/083a9c84fbd09a6ff9bfecabbf773650842fe1c0/toolkit/components/passwordmgr/LoginHelper.jsm#39

::: toolkit/components/passwordmgr/nsLoginManager.js:549
(Diff revision 1)
> +  // XXX: Right now we didn't cache the request or result in content side, so if
> +  //      user focuses on a password field which we have already sent a query request
> +  //      for it before, we'll still trigger another query this time.
> +  queryLoginReputationAsync(aElement, aCallback) {
> +    if (!this._loginReputationEnabled || aElement.type !== "password") {
> +      // Don't query if login reputation isn't enabled or its not a

Nit: "it's"

::: toolkit/components/reputationservice/ILoginReputation.idl:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  
> +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> +interface ILoginReputationResult : nsISupports {

I've never heard we're dropping the "ns"/"moz" prefix on interfaces.

::: toolkit/components/reputationservice/ILoginReputation.idl:20
(Diff revision 1)
> +  readonly attribute ACString formUrl;
> +
> +  readonly attribute ACString actionUrl;

Both of these are only origins so I would recommend the names:
formOrigin and formActionOrigin like we've been moving to.

::: toolkit/components/satchel/IQueryReputationObserver.idl:8
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, function, uuid(d7e7ab50-13d4-4279-af2e-2762b172eb48)]
> +interface IQueryReputationObserver : nsISupports

It seems a bit unusual for a "IQueryReputationObserver" interface to be defined in satchel. It would probably be better in the reputation service dir.

::: toolkit/components/satchel/nsFormFillController.cpp:1093
(Diff revision 1)
> +  // Trigger an asynchronous login reputation query when user focuses on a
> +  // password field.
> +  if (isPwmgrInput && mLoginManager) {
> +    mLoginManager->QueryLoginReputationAsync(mFocusedInput, this);
> +  }

I'm not sure we should spread the logic out between pwmgr and satchel. Why not keep it all in pwmgr?
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review200252

> nit: add a space after the colon:
> 
>     "QueryLoginReputation result: "

The logging module automatically inserts a space between arguments so no space was correct.
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review200252

> "query the login reputation service"
> 
> Also, technically, we're querying URLs, not the password field itself, so maybe this?
> 
>     Query the login reputation service for the URLs associated with a password field.

You are right, thanks for helping correct this

> "Right now we don't cache the request or result in the content process, so if a user focuses a password field twice, we'll send a query each time."
> 
> Does it matter? I mean it's doing more IPC than necessary, but it will be very quick once we have an in-memory cache.
> 
> Also it's async and it requires user input to trigger. Maybe we don't need to worry about this.

I agree with that you we don't need to worry about this too much.
I put the TODO comment here because I am  thinking maybe a simple approach can avoid an unnecessary IPC call.
But I haven't thought how the "simple" approach should be, maybe it won't be easy.

> There is also `VERDICT_TYPE_UNSPECIFIED`: https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/components/reputationservice/chromium/chrome/common/safe_browsing/csd.proto#254
> 
> If you add this one then our numbering will match the protobuf definition too.

Great! Thanks for telling.

> Maybe we should file a bug now for getting the verdicts from the real service and then link to it from here.

Sorry I am not sure what exactly you would like me to do here.
We have to implement whitelisting query, remote lookup , verdict cache and Trust-based whitelisting heuristics here.

The next step is whitelisting query which will be implemented in Bug 1407879
Do you suggest putting a link to this bug first?

> We could also file a bug here so that we have something concrete to link to.

file Bug 1413389

> When does `StopControllingInput()` get called? When the password field loses focus? When the form is submitted?
> 
> This seems like one of the corner cases you brought up in our last meeting.
> 
> I guess we could start by only cancelling the looking when the form is submitted. At that point, it's definitely too late.

Yes, StopControllingInput() get called when password feild loses focus.

The |mLoginManager->StopQueryLoginReputation| doesn't really cancel a login reputation query.
It just prevents UI got the callback result for password fields are not focused anymore.

Its kind of how we want the UX behavior be, if we only want to show warning message for the "focused" password field, that means,
if a password field loses focus and then we got the result for it, we should ignore the result.

If we still want to show warning in that case, then I should do us you suggest only call this function when form is submitted.
The approach in this patch just imitates how "insecure login" pop UI works now.
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6)
> Comment on attachment 8923294 [details]
> Bug 1407878 - Add a trigger when a password field is focused.
> 
> https://reviewboard.mozilla.org/r/194474/#review200298
> 
> Hello, I just saw this go by and have some drive-by comments:
> 

Thank you Matthew, I was originally going to find someone to review the FormFillControl & LoginManager part after francois reviews my patch.
Thanks for jumping into this, now I know who should I ask :)
Hi Matthew,
Thanks for your comment, I'll address them in next patch.
I list some questions below.

Ni francois for the question about dropping "ns"/"moz" prefix.

> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:370
> (Diff revision 1)
> > +    let messageData = { formOrigin,
> > +                        actionOrigin
> > +                      };
> 
> This should fail eslint as you should have a trailing comma in object and
> array literals. You should also move formOrigin to its own line.
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I just want to confirm this coding style because I found this pattern occurs in other places in LoginManagerContent.jsm.

> >  
> > +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> > +interface ILoginReputationResult : nsISupports {
> 
> I've never heard we're dropping the "ns"/"moz" prefix on interfaces.

francois told me that we are dropping "ns" prefix, but not sure about "moz" prefix.
Ni francois for this question.

> It seems a bit unusual for a "IQueryReputationObserver" interface to be
> defined in satchel. It would probably be better in the reputation service
> dir.

Yes, I think you are right. I'll fix this.

> ::: toolkit/components/satchel/nsFormFillController.cpp:1093
> (Diff revision 1)
> > +  // Trigger an asynchronous login reputation query when user focuses on a
> > +  // password field.
> > +  if (isPwmgrInput && mLoginManager) {
> > +    mLoginManager->QueryLoginReputationAsync(mFocusedInput, this);
> > +  }
> 
> I'm not sure we should spread the logic out between pwmgr and satchel. Why
> not keep it all in pwmgr?

The login reputation query is triggered when user focuses on a password field.
I though this is kind of user behavior that leads LoginManager starts to do the query and this is the same
scenario as we trigger an autocomplete search in FormFillController.
If we do this in pwmgr, then pwmgr has to handle the focus/unfocus things, it's a little bit strange to me.

I am not very familiar the code here, so please feel free to correct me if anything wrong.
Flags: needinfo?(francois)
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review200252

> Sorry I am not sure what exactly you would like me to do here.
> We have to implement whitelisting query, remote lookup , verdict cache and Trust-based whitelisting heuristics here.
> 
> The next step is whitelisting query which will be implemented in Bug 1407879
> Do you suggest putting a link to this bug first?

I just filed bug 1413732. You could say:

    // Return SAFE until we add support for the remote service (bug 1413732).

> Yes, StopControllingInput() get called when password feild loses focus.
> 
> The |mLoginManager->StopQueryLoginReputation| doesn't really cancel a login reputation query.
> It just prevents UI got the callback result for password fields are not focused anymore.
> 
> Its kind of how we want the UX behavior be, if we only want to show warning message for the "focused" password field, that means,
> if a password field loses focus and then we got the result for it, we should ignore the result.
> 
> If we still want to show warning in that case, then I should do us you suggest only call this function when form is submitted.
> The approach in this patch just imitates how "insecure login" pop UI works now.

> Its kind of how we want the UX behavior be, if we only want to show warning message for the "focused" password field, that means, if a password field loses focus and then we got the result for it, we should ignore the result.

That works fine for the insecure password warning because we already know whether or not the page is safe.

For password phishing, the verdict could come late because of network latency. I think it's still worth trying to warn the user and to get them not to submit the form.
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6)
> ::: toolkit/components/reputationservice/ILoginReputation.idl:10
> (Diff revision 1)
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "nsISupports.idl"
> >  
> > +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> > +interface ILoginReputationResult : nsISupports {
> 
> I've never heard we're dropping the "ns"/"moz" prefix on interfaces.

Ah, so we're just dropping "ns" from new filenames and class names?
Flags: needinfo?(francois) → needinfo?(MattN+bmo)
(In reply to François Marier [:francois] from comment #11)
> For password phishing, the verdict could come late because of network
> latency. I think it's still worth trying to warn the user and to get them
> not to submit the form.

Agree! I'll update my patch, thanks!
Just an update that I'm working on Bug 1407879 now so I could discuss with francois about the threading model next week.
So I won't work on this one this week.
Hi Matthew,
Could you help review this? or you know someone could help check the patch.
Thanks!
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review208244

::: toolkit/components/passwordmgr/nsLoginManager.js:96
(Diff revisions 1 - 2)
>  
>      this._remember = this._prefBranch.getBoolPref("rememberSignons");
>      this._autoCompleteLookupPromise = null;
>  
> -    this._queryLoginReputationPromise = null;
> -    this._loginReputationEnabled = Services.prefs.getBoolPref(LOGIN_REPUTATION_PREF);
> +    // Cache ongoing queries to avoid requesting same query while one is
> +    // still on the fly.

"still in flight"

::: toolkit/components/passwordmgr/nsLoginManager.js:552
(Diff revisions 1 - 2)
> -        // results, don't bother reporting them.
> -        return;
> +      return;
> -      }
> +    }
>  
> -      this._queryLoginReputationPromise = null;
> +    if (this._queryLoginReputationElements.has(aElement)) {
> +      return;

It may be worth adding a comment like:

    // We already have a request in flight for this field.

::: toolkit/components/reputationservice/ILoginReputation.idl:14
(Diff revision 2)
> +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> +interface ILoginReputationResult : nsISupports {
> +
> +  // These should sync with 'VerdictType' defined in
> +  // LoginReputationClientResponse in csd.proto.
> +  const unsigned short VERDICT_TYPE_UNSPECIFIED    = 0;

Maybe the type here should match the one in `onQueryComplete()`? i.e. `uint16_t`

::: toolkit/components/satchel/nsFormFillController.cpp:937
(Diff revision 2)
>  }
>  
>  ////////////////////////////////////////////////////////////////////////
> +//// IQueryReputationObserver
> +
> +// XXX : Bug 1413389, the reputation query result for a password field should be

nit: I would move the XXX comment to after the `MOZ_LOG()` line inside the function body.

Here's a suggestion for simpler wording:

  // TODO: integrate with browser UI (bug 1413389)

::: toolkit/components/satchel/nsFormFillController.cpp:939
(Diff revision 2)
>  ////////////////////////////////////////////////////////////////////////
> +//// IQueryReputationObserver
> +
> +// XXX : Bug 1413389, the reputation query result for a password field should be
> +// passed here. Integrate with UI to show warning message accordingly.
> +// Note that it is possible that the element is not visible to the user when we

nit: I would move this note just under `MOZ_ASSERT(aElement)`.
Attachment #8923294 - Flags: review?(francois) → review+
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review208250

::: toolkit/components/reputationservice/ILoginReputation.idl:14
(Diff revision 2)
> +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> +interface ILoginReputationResult : nsISupports {
> +
> +  // These should sync with 'VerdictType' defined in
> +  // LoginReputationClientResponse in csd.proto.
> +  const unsigned short VERDICT_TYPE_UNSPECIFIED    = 0;

Why not just `UNSPECIFIED`?

That's what you have in bug 1407879.
Hi Matthew,
Could you help review this patch? 
Your suggestions in Comment 6 are either addressed or answered in Comment 10, thanks!
(In reply to François Marier [:francois] from comment #12)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #6)
> > ::: toolkit/components/reputationservice/ILoginReputation.idl:10
> > (Diff revision 1)
> > >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > >  
> > >  #include "nsISupports.idl"
> > >  
> > > +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> > > +interface ILoginReputationResult : nsISupports {
> > 
> > I've never heard we're dropping the "ns"/"moz" prefix on interfaces.
> 
> Ah, so we're just dropping "ns" from new filenames and class names?

I don't know… I haven't heard about that for code using XPCOM. JS code which isn't implementing an interface never used the prefixes.
Flags: needinfo?(MattN+bmo)
(In reply to Dimi Lee[:dimi][:dlee] from comment #10)
> > ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:370
> > (Diff revision 1)
> > > +    let messageData = { formOrigin,
> > > +                        actionOrigin
> > > +                      };
> > 
> > This should fail eslint as you should have a trailing comma in object and
> > array literals. You should also move formOrigin to its own line.
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I just want to confirm this coding style because I found this pattern occurs
> in other places in LoginManagerContent.jsm.

For objects with more than one property, each property should be on its own line so that adding a new property doesn't change blame of existing properties. The other places in that file are incorrect.

> > >  
> > > +[scriptable, uuid(6219f9da-297e-446d-8d47-ccdd8e72a1d5)]
> > > +interface ILoginReputationResult : nsISupports {
> > 
> > I've never heard we're dropping the "ns"/"moz" prefix on interfaces.
> 
> francois told me that we are dropping "ns" prefix, but not sure about "moz"
> prefix.
> Ni francois for this question.

That is news to me… I never saw it discussed on dev-platform and didn't see it on the coding style MDN page.

> > ::: toolkit/components/satchel/nsFormFillController.cpp:1093
> > (Diff revision 1)
> > > +  // Trigger an asynchronous login reputation query when user focuses on a
> > > +  // password field.
> > > +  if (isPwmgrInput && mLoginManager) {
> > > +    mLoginManager->QueryLoginReputationAsync(mFocusedInput, this);
> > > +  }
> > 
> > I'm not sure we should spread the logic out between pwmgr and satchel. Why
> > not keep it all in pwmgr?
> 
> The login reputation query is triggered when user focuses on a password
> field.
> I though this is kind of user behavior that leads LoginManager starts to do
> the query and this is the same
> scenario as we trigger an autocomplete search in FormFillController.
> If we do this in pwmgr, then pwmgr has to handle the focus/unfocus things,
> it's a little bit strange to me.

pwmgr already handles "focus" for username fields but now that I look at this again, you're only really using three helpers from pwmgr AFAICT:
* LoginUtils._getPasswordOrigin(doc.documentURI);
* LoginUtils._getActionOrigin(form);
* LoginFormFactory.createFromField

The first 2 can easily be moved to LoginHelper.jsm so they can be used from outside LoginManagerContent.jsm. The last one probably isn't useful if you only care about type=password. What are you supposed to send as the "form action" for <input type=password> not in a <form>?

Rather than spreading the logic across 3 modules and 3 pwmgr files, I think it would be cleaner to have nsFormFillController call directly into a JS-implemented reputation service. This will keep the details out of password manager (which is complex enough), make it clear that it's not code owner by password manager peers, and consolidate the logic in one place.
Comment on attachment 8923294 [details]
Bug 1407878 - Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/194474/#review210904

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1402
(Diff revision 4)
>  var LoginUtils = {
>    /**
>     * Get the parts of the URL we want for identification.
>     * Strip out things like the userPass portion
>     */
>    _getPasswordOrigin(uriString, allowJS) {

Move both these methods to LoginHelper.jsm so you can use them from your login reputation service that satchel calls.

::: toolkit/components/satchel/nsFormFillController.cpp:937
(Diff revision 4)
> +NS_IMETHODIMP
> +nsFormFillController::OnQueryReputationCompletion(nsIDOMHTMLInputElement* aElement,
> +                                                  uint16_t aResult)
> +{
> +  MOZ_ASSERT(aElement);
> +
> +  MOZ_LOG(sLogger, LogLevel::Verbose, ("OnQueryReputationCompletion (%d)", aResult));
> +
> +  // TODO: integrate with browser UI (bug 1413389)
> +  // Note that it is possible that the element is not visible to the user when we
> +  // receive the callback (for example, page is navigated).
> +
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +

Move this to a service in the reputationservice component.
Attachment #8923294 - Flags: review?(MattN+bmo)
Attachment #8923294 - Attachment is obsolete: true
Hi Francois,
Sorry I have to ask you to review again.
I rewrote the patch according to Matt's suggestion, right now everything is put in LoginReputationService.

I think this makes sense especially because we will only collect telemetry.
The patch contains two parts. One is IPDL for IPC; the other one is to support IPC
in LoginReputationService, it also removes some unnecessary code compare to the previous patch since we won't query remote server now (For example, not passing form action url).
Comment on attachment 8935296 [details]
Bug 1407878 - P1. Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/206194/#review211774


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/components/reputationservice/LoginReputation.cpp:32
(Diff revision 1)
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_ILOGINREPUTATIONQUERY
> +
> +  ReputationQueryParam(nsIURI* aURI)

Error: Bad implicit conversion constructor for 'reputationqueryparam' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8935296 [details]
Bug 1407878 - P1. Check URLs against the login reputation service when a password field is focused.

https://reviewboard.mozilla.org/r/206194/#review212202
Attachment #8935296 - Flags: review?(francois) → review+
Comment on attachment 8935297 [details]
Bug 1407878 - P2. Add PLoginReputation.ipdl for LoginReputationService.

https://reviewboard.mozilla.org/r/206196/#review212204

The URL Classifier bits look good.

It might be good to find a more knowledgeable person than me to review the IPC changes.
Attachment #8935297 - Flags: review?(francois) → review+
> It might be good to find a more knowledgeable person than me to review the
> IPC changes.

Thanks for review, I will dop that!
Attachment #8935297 - Flags: review?(wmccloskey)
Hi Bill,
Could you help take a look at the IPC part? Thanks!
Comment on attachment 8935297 [details]
Bug 1407878 - P2. Add PLoginReputation.ipdl for LoginReputationService.

https://reviewboard.mozilla.org/r/206196/#review212588

I'm kinda confused since this code doesn't seem to do anything yet. But I guess I can see that you plan to do something with it later.

::: dom/ipc/PLoginReputation.ipdl:13
(Diff revision 1)
> +include protocol PContent;
> +
> +namespace mozilla {
> +namespace dom {
> +
> +protocol PLoginReputation

You really need a comment describing the purpose of this thing. Especially since it doesn't do anything right now.

::: toolkit/components/reputationservice/LoginReputation.cpp:133
(Diff revision 1)
>  
>    if (XRE_IsContentProcess()) {
> +    using namespace mozilla::ipc;
> +
> +    ContentChild* content = ContentChild::GetSingleton();
> +    if (NS_WARN_IF(!content || content->IsShuttingDown())) {

No need for the null check.

::: toolkit/components/reputationservice/LoginReputation.cpp:140
(Diff revision 1)
> +    }
> +
> +    URIParams uri;
> +    SerializeURI(documentURI, uri);
> +
> +    if (!content->SendPLoginReputationConstructor(uri)) {

I guess eventually you're going to subclass PLoginReputationChild and attach the callback to the subclass? Then the dealloc method for the subclass will call the callback? It's hard to review code that's incomplete like this.

::: toolkit/components/reputationservice/LoginReputationIPC.cpp:1
(Diff revision 1)
> +

Blank line should be removed.

::: toolkit/components/reputationservice/LoginReputationIPC.cpp:25
(Diff revision 1)
> +
> +mozilla::ipc::IPCResult
> +LoginReputationParent::QueryReputation(nsIURI* aURI)
> +{
> +  nsresult rv;
> +  nsCOMPtr<ILoginReputationService> service =

ILoginRepuationService? The name makes it sound like it's part of MS COM. For interfaces, we use nsIFoo.

::: toolkit/components/reputationservice/LoginReputationIPC.cpp:34
(Diff revision 1)
> +    return IPC_OK();
> +  }
> +
> +  nsCOMPtr<ILoginReputationQuery> query =
> +    LoginReputationService::ConstructQueryParam(aURI);
> +  rv = service->QueryReputation(query, this);

Please make sure that QueryReputation keeps |this| alive until OnQueryComplete is called.

::: toolkit/components/reputationservice/LoginReputationIPC.cpp:48
(Diff revision 1)
> +LoginReputationParent::OnQueryComplete(uint16_t aResult)
> +{
> +  LR_LOG(("OnQueryComplete() [result=%d]", aResult));
> +
> +  if (mIPCOpen) {
> +    Unused << Send__delete__(this);

Shouldn't you send aResult as a parameter to the destructor?
Attachment #8935297 - Flags: review?(wmccloskey) → review+
Comment on attachment 8935297 [details]
Bug 1407878 - P2. Add PLoginReputation.ipdl for LoginReputationService.

https://reviewboard.mozilla.org/r/206196/#review212588

> You really need a comment describing the purpose of this thing. Especially since it doesn't do anything right now.

I'll add more description.

> I guess eventually you're going to subclass PLoginReputationChild and attach the callback to the subclass? Then the dealloc method for the subclass will call the callback? It's hard to review code that's incomplete like this.

Again, I am sorry I didn't make this clear before asking for review.

Our first step is send the url to parent first and record the telemetry result.
Based on the result, we will decide if should keep working on the login reputation check.
So we won't have the callback in the near future.
And yes, you are right, if we are going to notify child the result, it will be implemented as the way you suggest.

> ILoginRepuationService? The name makes it sound like it's part of MS COM. For interfaces, we use nsIFoo.

I heard that we are dropping 'ns' prefix, so that's why I removed it in the first place.
But now I am not sure, I'll double check this.

> Shouldn't you send aResult as a parameter to the destructor?

Will fix this in next patch, thanks for suggestion.
Comment on attachment 8935297 [details]
Bug 1407878 - P2. Add PLoginReputation.ipdl for LoginReputationService.

https://reviewboard.mozilla.org/r/206196/#review212588

Hi Bill,
Thanks for your review, I am sorry I didn't make this clear before asking for review.

You are right, this patch only sneds the url to parent when user focuses on a password field, that's all. The remaining part is implemented in Bug 1407879 and Bug 1422671.

To make it more clear, the whole scenario will be:
1. User focuses on a password field, this IPC call is triggered with url as parameter. (This patch)
2. Parent side (LoginReputationService) receives the IPC call and check the url against the local whitelist table (Bug 1407879).
3. Record the result in telemetry (Bug 1422671).

As first step, we will analyze the telemetry first, so there won't be any callback to child now.

I should really describe this before sending the review, sorry again for making it so hard to review.
Comment on attachment 8935297 [details]
Bug 1407878 - P2. Add PLoginReputation.ipdl for LoginReputationService.

https://reviewboard.mozilla.org/r/206196/#review212774
Comment on attachment 8935297 [details]
Bug 1407878 - P2. Add PLoginReputation.ipdl for LoginReputationService.

https://reviewboard.mozilla.org/r/206196/#review212588

> Will fix this in next patch, thanks for suggestion.

My previous comment was wrong.
As mentioned in the comment 35, send back the result to child is kind of future work.
We want to collect telemetry in parent first and then decide the next step.
Hi francois,
It seems "dropping nx prefix" confusing people, I can't find anyone has mentioned this in mail thread.
I guess I just use mozIxxx instead of Ixxx or nsIxxx. How do you think?
Flags: needinfo?(francois)
(In reply to Dimi Lee[:dimi][:dlee] from comment #38)
> It seems "dropping nx prefix" confusing people, I can't find anyone has
> mentioned this in mail thread.
> I guess I just use mozIxxx instead of Ixxx or nsIxxx. How do you think?

Olly is the one who pointed out to me (during a review) that new code should not have the "ns" prefix in class names or filenames. Maybe he knows of an announcement.
Flags: needinfo?(francois) → needinfo?(bugs)
(In reply to François Marier [:francois] from comment #39)
> Olly is the one

That should be "Olli". Sorry for misspelling your name.
Interfaces still tend to have ns prefix, or moz, but classes don't.
Coding style uses ns-prefix for interfaces.
Sorry, we're inconsistent here, but that is how things are now.

Luckily .idl interfaces are added rarely this days.



Another thing, if one does something like
for (var i = 0; i < 10000; ++i) {
  document.body.innerHTML = "<input type='password'>";
  document.body.firstChild.focus();
}
does the new code start to show up in the profiles badly?

(Speedometer does effectively something very similar to that but with some other type="...". Silly benchmark, but that is what it is)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (won't be in Austin) from comment #41)
> Interfaces still tend to have ns prefix, or moz, but classes don't.
> Coding style uses ns-prefix for interfaces.
> Sorry, we're inconsistent here, but that is how things are now.
> 
> Luckily .idl interfaces are added rarely this days.
> 
> 
Thanks for clarification, Olli, I'll update my patch : )

> 
> Another thing, if one does something like
> for (var i = 0; i < 10000; ++i) {
>   document.body.innerHTML = "<input type='password'>";
>   document.body.firstChild.focus();
> }
> does the new code start to show up in the profiles badly?
> 
> (Speedometer does effectively something very similar to that but with some
> other type="...". Silly benchmark, but that is what it is)

The old patch had this kind of check, but after we decide to just record telemetry, I removed that check.
Since we have this benchmark, I'll add some check to prevent this, thanks for pointing this out, Olli!

Hi Francois,
Olli's question reminds me a question about the telemetry data we want to record here.
Right now, If a user clicks the same password field (the same form URL) multiple times in a single session, we will record the result (in whitelist or not) multiple times. But I guess what we really want is the percentage of different URL user visited in whitelist.
If you agree with this, I'll add a check to skip 'QueryReputation' if the URL has been checked before.
Flags: needinfo?(francois)
(In reply to Dimi Lee[:dimi][:dlee] from comment #42)
> Olli's question reminds me a question about the telemetry data we want to
> record here.
> Right now, If a user clicks the same password field (the same form URL)
> multiple times in a single session, we will record the result (in whitelist
> or not) multiple times. But I guess what we really want is the percentage of
> different URL user visited in whitelist.
> If you agree with this, I'll add a check to skip 'QueryReputation' if the
> URL has been checked before.

Yes, that makes sense to me. We should only call it once per page load. So ideally we should find a way to cache it in a way that the cache gets blown away when the user navigates away from (or reloads) the page.
Flags: needinfo?(francois)
Blocks: 1425168
(In reply to François Marier [:francois] from comment #43)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #42)
> 
> Yes, that makes sense to me. We should only call it once per page load. So
> ideally we should find a way to cache it in a way that the cache gets blown
> away when the user navigates away from (or reloads) the page.

I don't want to block this bug, so file bug 1425168 to implement this.
Since this setting is default off, so the issue mentioned by Olli in Comment 41 won't happen now.
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d13ac1acae1
P1. Check URLs against the login reputation service when a password field is focused. r=francois
https://hg.mozilla.org/integration/autoland/rev/003d418a3516
P2. Add PLoginReputation.ipdl for LoginReputationService. r=billm,francois
So did someone actually profile this code? (see comment 41)

Also, do we really want nsIURI* documentURI = node->OwnerDoc()->GetDocumentURI();
or should we use the url from the principal?
aha, pref is off by default.
https://hg.mozilla.org/mozilla-central/rev/2d13ac1acae1
https://hg.mozilla.org/mozilla-central/rev/003d418a3516
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1425625
You need to log in before you can comment on or make changes to this bug.