1407879 - Check password field url against the local whitelist

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Dimi Lee [:dimi]]

We need to check local whitelist table (goog-passwordwhite-proto) for password url

Comment 1

[Dimi Lee [:dimi]]

Attachment: WIP patch

Comment 2

[François Marier [:francois]]

Are you sure this needs the updated csd.proto file? The whitelist is already being downloaded successfully on central.

Comment 3

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #2)
> Are you sure this needs the updated csd.proto file? The whitelist is already
> being downloaded successfully on central.

As discussed offline, the dependency to bug 1385461 is set because we need the dummy login reputation service component in that bug.

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1407879 - Check password field url against the local whitelist.

Review commit: https://reviewboard.mozilla.org/r/197592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197592/

Comment 5

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/1-2/

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review203252


C/C++ static analysis found 3 defects in this patch.

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


::: toolkit/components/reputationservice/LoginReputation.cpp:29
(Diff revision 1)
> +  }
> +
> +  RefPtr<QueryReputationPromise> QueryTrustBasedWhitelisting(QueryRequest* aRequest);
> +
> +private:
> +  ~TrustBasedWhitelisting()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

::: toolkit/components/reputationservice/LoginReputation.cpp:166
(Diff revision 1)
> +
> +nsresult
> +LoginReputationService::QueryReputationInternal(QueryRequest* aRequest)
> +{
> +  //// We do these in parallel
> +  auto localPromise = DoQueryLocalWhitelistTable(aRequest);

Error: Unused "kungfudeathgrip" 'refptr<mozilla::mozpromise<bool, bool, false> >' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

::: toolkit/components/reputationservice/LoginReputation.cpp:167
(Diff revision 1)
> +nsresult
> +LoginReputationService::QueryReputationInternal(QueryRequest* aRequest)
> +{
> +  //// We do these in parallel
> +  auto localPromise = DoQueryLocalWhitelistTable(aRequest);
> +  auto trustPromise = mTrust->QueryTrustBasedWhitelisting(aRequest);

Error: Unused "kungfudeathgrip" 'refptr<mozilla::mozpromise<bool, bool, false> >' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review203268


C/C++ static analysis found 5 defects in this patch.

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


::: toolkit/components/reputationservice/LoginReputation.cpp:77
(Diff revision 2)
> +  }
> +
> +  RefPtr<QueryReputationPromise> QueryRemoteLookup(QueryRequest* aRequest);
> +
> +private:
> +  ~RemoteLookup()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

::: toolkit/components/reputationservice/LoginReputation.cpp:97
(Diff revision 2)
> +
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  return InvokeAsync(mThread, __func__, [this, aRequest]() {
> +    DimiLog("Remote lookup call invoke async");
> +    return QueryInternal(aRequest);

Error: Refcounted variable 'this' of type 'remotelookup' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: toolkit/components/reputationservice/LoginReputation.cpp:127
(Diff revision 2)
> +  }
> +
> +  RefPtr<QueryReputationPromise> QueryTrustBasedWhitelisting(QueryRequest* aRequest);
> +
> +private:
> +  ~TrustBasedWhitelisting()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

::: toolkit/components/reputationservice/LoginReputation.cpp:147
(Diff revision 2)
> +
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  return InvokeAsync(mThread, __func__, [this, aRequest]() {
> +    DimiLog("QueryTrustBasedWhitelisting inside invoke async");
> +    return QueryInternal(aRequest);

Error: Refcounted variable 'this' of type 'trustbasedwhitelisting' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: toolkit/components/reputationservice/LoginReputation.cpp:422
(Diff revision 2)
> +    SplitTables(aLists, lists);
> +
> +    nsAutoCString whitelist;
> +    GetWhiteListTables(whitelist);
> +    
> +    for (auto list : lists) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

    for (auto list : lists) {
              ^
         const  &

Comment 8

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/2-3/

Comment 9

[François Marier [:francois]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review204428

I can think of a few useful telemetry probes to add to this patch:

- time for doing a lookup against the passwordwhite list
- total time for doing the full check
- result of the full check

We can add other probes when we add the other pieces.

::: modules/libpref/init/all.js:5414
(Diff revision 3)
>  // Tables for tracking protection
>  pref("urlclassifier.trackingTable", "test-track-simple,base-track-digest256");
>  pref("urlclassifier.trackingWhitelistTable", "test-trackwhite-simple,mozstd-trackwhite-digest256");
>  
>  // These tables will never trigger a gethash call.
>  pref("urlclassifier.disallow_completions", "test-malware-simple,test-harmful-simple,test-phish-simple,test-unwanted-simple,test-track-simple,test-trackwhite-simple,test-block-simple,goog-downloadwhite-digest256,base-track-digest256,mozstd-trackwhite-digest256,content-track-digest256,mozplugin-block-digest256,mozplugin2-block-digest256,block-flash-digest256,except-flash-digest256,allow-flashallow-digest256,except-flashallow-digest256,block-flashsubdoc-digest256,except-flashsubdoc-digest256,except-flashinfobar-digest256");

We should add the password whitelist table to this pref.

::: modules/libpref/init/all.js:5460
(Diff revision 3)
>  pref("browser.safebrowsing.downloads.remote.block_dangerous_host",       true);
>  pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", true);
>  pref("browser.safebrowsing.downloads.remote.block_uncommon",             true);
>  
>  // Password protection
> -pref("browser.safebrowsing.passwords.enabled", false);
> +pref("browser.safebrowsing.passwords.enabled", true);

We should probably keep that turned off until the feature is ready to ship.

To get telemetry data on Nightly though, we can set it to `true` inside an `#ifdef NIGHTLY_BUILD`.

Let's add a second pref for the remote lookups:  `browser.safebrowsing.passwords.remote.enabled`. It will default to `false` on all channels until we have updated our privacy policy.

::: toolkit/components/passwordmgr/nsLoginManager.js:561
(Diff revision 3)
>        }
>  
>        this._queryLoginReputationPromise = null;
>  
> -      log.debug("QueryLoginReputation invoked. Result is:", aResult);
> +      log.debug("QueryLoginReputation invoked. Result is:" + aResult);
> +      dump("QueryLoginReputation invoked. Result is:" + aResult + "\n");

Did you intend to log this twice?

::: toolkit/components/reputationservice/LoginReputation.h:15
(Diff revision 3)
> +#include "nsIURIClassifier.h"
> +#include "nsIObserver.h"
>  #include "mozilla/Logging.h"
> +#include "mozilla/MozPromise.h"
>  
> -class LoginReputationService final : public ILoginReputationService
> +class TrustBasedWhitelisting;

We should use a noun here to match the other ones: `TrustBasedWhitelist`.

::: toolkit/components/reputationservice/LoginReputation.h:17
(Diff revision 3)
>  #include "mozilla/Logging.h"
> +#include "mozilla/MozPromise.h"
>  
> -class LoginReputationService final : public ILoginReputationService
> +class TrustBasedWhitelisting;
> +class RemoteLookup;
> +class CSDQuery;

I think this is a "legacy" name that comes from the fact that the whitelist is the same as the one used for the "client-side phishing detection" that Chrome has.

We could call this `LoginWhitelist` maybe.

::: toolkit/components/reputationservice/LoginReputation.cpp:70
(Diff revision 3)
> +
> +nsresult
> +RemoteLookup::Init()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

This should check that `browser.safebrowsing.passwords.remote.enabled == true` before creating the new thread.

::: toolkit/components/reputationservice/LoginReputation.cpp:77
(Diff revision 3)
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to create remote lookup thread for login reputation");
> +  }
> +
> +  // TODO : Bug 1416647 - Support verdict cache for password phishing.
> +  //        Load verdict cache when Init, but task should be post to worke

"posted to the worker thread"

::: toolkit/components/reputationservice/LoginReputation.cpp:100
(Diff revision 3)
> +
> +RefPtr<ReputationPromise>
> +RemoteLookup::QueryRemoteLookup(ILoginReputationQuery* aParam)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

We should return an error if `mThread == nullptr` since that means that remote lookups are disabled.

::: toolkit/components/reputationservice/LoginReputation.cpp:191
(Diff revision 3)
> +
> +RefPtr<ReputationPromise>
> +TrustBasedWhitelisting::QueryTrustBasedWhitelist(ILoginReputationQuery* aParam)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

Add a check for `mThread == nullptr`.

::: toolkit/components/reputationservice/LoginReputation.cpp:246
(Diff revision 3)
> +  CSDQuery() = default;
> +
> +private:
> +  ~CSDQuery() = default;
> +
> +  void GetWhiteListTables(nsACString& aTables);

nit: `GetWhitelistTables` (lowercase `l`)

::: toolkit/components/reputationservice/LoginReputation.cpp:301
(Diff revision 3)
> +}
> +
> +nsresult
> +CSDQuery::OnClassifyComplete(nsresult aErrorCode,
> +                             const nsACString& aLists,
> +                             const nsACString& aPrnovider,

typo: `aProvider`

::: toolkit/components/reputationservice/LoginReputation.cpp:325
(Diff revision 3)
> +}
> +
> +void
> +CSDQuery::GetWhiteListTables(nsACString& aTables)
> +{
> +  aTables.AssignLiteral("goog-passwordwhite-proto");

That should be read from `urlclassifier.passwordAllowTable`.

::: toolkit/components/reputationservice/LoginReputation.cpp:331
(Diff revision 3)
> +}
> +
> +// -------------------------------------------------------------------------
> +// LoginReputationService
> +//
> +// TODO : Add description

Or just remove the comment :)

::: toolkit/components/reputationservice/LoginReputation.cpp:366
(Diff revision 3)
>  
>  NS_IMETHODIMP
> +LoginReputationService::Init()
> +{
> +  LR_LOG(("Init login reputation service"));
> +

Should we add a check here for `browser.safebrowsing.passwords.enabled` and exit early if the feature is disabled?

::: toolkit/components/reputationservice/LoginReputation.cpp:393
(Diff revision 3)
> +  return NS_OK;
> +}
> +
> +/**
> + * The QueryReputation includes three steps:
> + * 1. Check CSD whitelist

I would say "check a Google-provided whitelist".

::: toolkit/components/reputationservice/LoginReputation.cpp:394
(Diff revision 3)
> +}
> +
> +/**
> + * The QueryReputation includes three steps:
> + * 1. Check CSD whitelist
> + * 2. Check trust-based white

"Check for signs that the site is trusted by the user."

::: toolkit/components/reputationservice/LoginReputation.cpp:399
(Diff revision 3)
> + * 2. Check trust-based white
> + * 3. Check verdict cache and issue a remote lookup if url isn't found in cache
> + *
> + * Each step has its thread and once a thread finishes its task it delivers
> + * it back to main-thread first so main-thread could decide if the task should
> + * proceed to next step (Tasks may finish earlier when whitelist return SAFE).

nit: "tasks may..." (lowercase `t`)

::: toolkit/components/reputationservice/LoginReputation.cpp:402
(Diff revision 3)
> + * Each step has its thread and once a thread finishes its task it delivers
> + * it back to main-thread first so main-thread could decide if the task should
> + * proceed to next step (Tasks may finish earlier when whitelist return SAFE).
> + *
> + * Query reputation tasks could be processed in parallel, for example, when a task
> + * is in step3 and another task comes in, then we would have one task is handled

nit: "step 3" (space)

::: toolkit/components/reputationservice/LoginReputation.cpp:403
(Diff revision 3)
> + * it back to main-thread first so main-thread could decide if the task should
> + * proceed to next step (Tasks may finish earlier when whitelist return SAFE).
> + *
> + * Query reputation tasks could be processed in parallel, for example, when a task
> + * is in step3 and another task comes in, then we would have one task is handled
> + * by CSD whitelist thread (step1) and one task is handled by remote lookup thread

ditto

::: toolkit/components/reputationservice/LoginReputation.cpp:404
(Diff revision 3)
> + * proceed to next step (Tasks may finish earlier when whitelist return SAFE).
> + *
> + * Query reputation tasks could be processed in parallel, for example, when a task
> + * is in step3 and another task comes in, then we would have one task is handled
> + * by CSD whitelist thread (step1) and one task is handled by remote lookup thread
> + * (step3). Also noted that tasks may not be finished in the same order as they

ditto

::: toolkit/components/reputationservice/LoginReputation.cpp:440
(Diff revision 3)
> -  aCallback->OnQueryComplete(ILoginReputationResult::SAFE);
> +  // We could ensure the |QueryRequest| is always valid until Finish() is
> +  // called or LoginReputationService is shutdown.
> +  auto* request =
> +    mQueryRequests.AppendElement(MakeUnique<QueryRequest>(aQuery, aCallback));
> +
> +  // Check whitelist first so we don't have to perform a remote lookup for every

"Check whitelists first" (plural)

::: toolkit/components/reputationservice/LoginReputation.cpp:572
(Diff revision 3)
> +    },
> +    [self, aRequest](nsresult rv) -> void {
> +      // Promise is rejected only if there is an error.
> +      MOZ_ASSERT(NS_FAILED(rv));
> +
> +      LR_LOG(("QueryRemoteLookup return fail 0x%.8x", rv));

Personally, I like to use `ErrorNames` to display the name of the NS_ERROR in a human-readable way: https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/toolkit/components/reputationservice/ApplicationReputation.cpp#301-304

::: toolkit/components/reputationservice/LoginReputation.cpp:596
(Diff revision 3)
> +  if (LR_LOG_ENABLED()) {
> +    LR_LOG(("Query login reputation finished, result is %s",
> +      ReputationResultToString(aResult).get()));
> +  }
> +
> +  // TODO : Add a testcase to make sure enum(ReputationResult) is sync with definition

Could part of this be done as a static assert?

I'm thinking, as a first step, of comparing the size of these enums. That way we could detect new verdict types at compile time.

Comment 10

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/3-4/

Comment 11

[François Marier [:francois]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review208252

That's looking really good. My next review should be quick if you don't rebase in between ;)

::: toolkit/components/reputationservice/LoginReputation.cpp:40
(Diff revision 4)
> +{
> +  switch(aResult) {
> +    case ReputationResult::LOW_REPUTATION:  return nsCString("Low Reputation");
> +    case ReputationResult::SAFE:            return nsCString("Safe");
> +    case ReputationResult::PHISHING:        return nsCString("Phishing");
> +    default:                                return nsCString("Unspecified");

Since "unspecified" is a valid value we could get from the server, maybe it's a good idea to have a fifth case too?

     ...
     case ReputationResult::UNSPECIFIED:   return "Unspecified";
     default:                              return "Invalid";

::: toolkit/components/reputationservice/LoginReputation.cpp:51
(Diff revision 4)
> +//
> +// For non-whitelisted sites, we will do a remote lookup against a google
> +// service and receive one of the following verdicts:
> +// 1. safe
> +// 2. low-reputation
> +// 3. phishing

I guess there's also the possibility that we'll get "unspecified" too. TBC with Google.

Maybe we should just say "one of the verdicts in ReputationResult" to avoid having to update this comment if it ever changes.

::: toolkit/components/reputationservice/LoginReputation.cpp:86
(Diff revision 4)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsresult rv = NS_OK;
> +
> +  if (!mThread) {

Given that we will re-init when the `remote.enabled` pref is toggled, we need to make sure that nothing is run if this is already enabled.

So I would suggest replacing this `if` statement with:

    if (mThread) {
        return NS_OK; // already initialized
    }

::: toolkit/components/reputationservice/LoginReputation.cpp:94
(Diff revision 4)
> +      NS_WARNING("Failed to create remote lookup thread for login reputation");
> +    }
> +  }
> +
> +  // TODO : Bug 1416647 - Support verdict cache for password phishing.
> +  //        Load verdict cache when Init, but task should be post to worker

s/post to/sent to/

::: toolkit/components/reputationservice/LoginReputation.cpp:310
(Diff revision 4)
> +  // Return rejected promise while there is an error.
> +  auto fail = MakeScopeExit([&] () {
> +    holder->Reject(rv, __func__);
> +  });
> +
> +  // Get uri and white list table to check.

nit: this comment is probably not necessary

::: toolkit/components/reputationservice/LoginReputation.cpp:327
(Diff revision 4)
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return p;
> +  }
> +
> +  // AsyncClassifyLocalWithTables API won't trigger a gethash request
> +  // when match a full-length prefix, so this API call should only

s/when match a full-length prefix/on a full-length match/

::: toolkit/components/reputationservice/LoginReputation.cpp:485
(Diff revision 4)
> +  Unused << NS_WARN_IF(NS_FAILED(rv));
> +
> +  mRemoteLookup->Uninit();
> +  Unused << NS_WARN_IF(NS_FAILED(rv));
> +
> +  mQueryRequests.Clear();

Maybe we should clear the requests at the start of the function before uninitializing anything?

::: toolkit/components/reputationservice/LoginReputation.cpp:565
(Diff revision 4)
>      aQuery->GetActionOrigin(actionOrigin);
> -    LR_LOG(("Login reputation query parameters: formOrigin(%s), actionOrigin(%s)",
> -            formOrigin.BeginReading(), actionOrigin.BeginReading()));
> +    LR_LOG(("QueryReputation() started [request = %p, formOrigin = %s, actionOrigin = %s]",
> +            request->get(), formOrigin.BeginReading(), actionOrigin.BeginReading()));
> +  }
> +
> +  // Check whitelists first so we don't have to perform a remote lookup for every

I think this comment is redundant given the large one just before the function.

::: toolkit/components/reputationservice/LoginReputation.cpp:658
(Diff revision 4)
> +          LR_LOG(("Query trust-based whitelist cannot find the URL [request = %p]",
> +                  aRequest));
> +        }
> +      }
> +
> +      // Both whitelisting methods fail to find url, query remote server.

"failed to"

::: toolkit/components/reputationservice/LoginReputation.cpp:709
(Diff revision 4)
> +
> +nsresult
> +LoginReputationService::Finish(const QueryRequest* aRequest, ReputationResult aResult)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRequest);

`NS_ENSURE_ARG_POINTER()`

Comment 12

[François Marier [:francois]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review208258

::: toolkit/components/reputationservice/LoginReputation.cpp:577
(Diff revision 4)
> +
> +nsresult
> +LoginReputationService::QueryLoginWhitelist(QueryRequest* aRequest)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRequest);

`NS_ENSURE_ARG_POINTER`

::: toolkit/components/reputationservice/LoginReputation.cpp:623
(Diff revision 4)
> +
> +nsresult
> +LoginReputationService::QueryTrustBasedWhitelist(QueryRequest* aRequest)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aRequest);

`NS_ENSURE_ARG_POINTER`

Comment 13

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/4-5/

Comment 14

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review208252

Thanks for your review!
I forget to add the static_assert fix in previous patch, I'll add it in next one.

> Maybe we should clear the requests at the start of the function before uninitializing anything?

This has to be put after those Unint calls since mQueryRequests will still be accessed while main-thread is blocking at mThread->shutdown() in Uninit calls.

The detail order of execution flow is:
1. Main thread calls Uninit and then is blocked in mThread->shutdown()
2. Worker thread executes it's task.
3. Worker thread finish it's task, task is then send back to main thread (MozPromise resolve or reject function).
4. Main thread execute MozPromise resolve/reject function (still access mQueryRequest at this moment)
5. Worker thread is killed because of shutdown call in Uninit (step 1)
6. Main thread is resume from mThread->shutdown()

Comment 15

[Code Review Bot [:reviewbot]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review208742


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:96
(Diff revision 5)
> +  nsresult rv;
> +
> +  if (mThread) {
> +    // Already initialized.
> +    return NS_OK;
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 16

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

I would like to ask for review together with testcase, so cancel the current one.

Comment 17

[Dimi Lee [:dimi]]

Attachment: Bug 1407879 - Add testcase for login reputation service.

Review commit: https://reviewboard.mozilla.org/r/204482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204482/

Comment 18

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/5-6/

Comment 19

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #11)
> That's looking really good. My next review should be quick if you don't
> rebase in between ;)
> 

I also refine the code to sync with ApplicationReputation and add the error status for testcase.
The architecture is the same as the previous patch.

Comment 20

[Code Review Bot [:reviewbot]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review210018


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:87
(Diff revision 6)
> +  nsresult rv;
> +
> +  if (mThread) {
> +    // Already initialized.
> +    return NS_OK;
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 21

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review210462

::: toolkit/components/reputationservice/ILoginReputation.idl:23
(Diff revision 6)
> -  const unsigned short LOW_REPUTATION              = 2;
> -  const unsigned short PHISHING                    = 3;
> +  const unsigned long LOW_REPUTATION   = 2;
> +  const unsigned long PHISHING         = 3;
> +
> +  // This is used by static_assert to ensure this is sync with
> +  // ReputationResult in LoginReputation.h
> +  const unsigned long MAX_VALUE        = 4;

"MAX_VALUE" should be removed in next patch since it is no longer required

Comment 22

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/6-7/

Comment 23

[Dimi Lee [:dimi]]

Comment on attachment 8933541 [details]
Bug 1407879 - Add testcase for login reputation service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204482/diff/1-2/

Comment 24

[François Marier [:francois]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

https://reviewboard.mozilla.org/r/197592/#review214328

::: commit-message-9cc69:3
(Diff revision 7)
> +Bug 1407879 - Check password field url against the local whitelist. r?francois
> +
> +Support check local whitelist table when querying login reputation. The local

I would remove the word "Support" and start the sentence at "Check local whitelist...".

Alternatively, you could also just remove the long description since the short one is fairly descriptive.

::: toolkit/components/reputationservice/LoginReputation.cpp:158
(Diff revision 7)
> +  return p;
> +}
> +
> +nsresult
> +LoginWhitelist::OnClassifyComplete(nsresult aErrorCode,
> +                             const nsACString& aLists,

nit: these three lines are not indented properly

::: toolkit/components/reputationservice/LoginReputation.cpp:240
(Diff revision 7)
>    switch (XRE_GetProcessType()) {
>    case GeckoProcessType_Default:
>      LR_LOG(("Init login reputation service in parent"));
>      break;
>    case GeckoProcessType_Content:
>      LR_LOG(("Init login reputation service in child"));

Since we're no longer initializing in the child, we should remove this log message.

::: toolkit/components/reputationservice/LoginReputation.cpp:384
(Diff revision 7)
>  
>    LR_LOG(("QueryReputation() [this=%p]", this));
>  
> -  if (!sPasswordProtectionEnabled) {
> -    return NS_ERROR_FAILURE;
> +  if (gShuttingDown || !sPasswordProtectionEnabled) {
> +    LR_LOG(("QueryReputation() abort [this=%p]", this));
> +      aCallback->OnComplete(NS_ERROR_ABORT,

nit: indentation

::: toolkit/components/reputationservice/LoginReputation.cpp:390
(Diff revision 7)
> +      nsILoginReputationVerdictType::UNSPECIFIED);
> +    return NS_OK;
> +  }
> +
> +  // mQueryRequests is an array used to maintain the ownership of |QueryRequest|.
> +  // We could ensure that |QueryRequest| is always valid until Finish() is

I'm a little confused by "we could ensure". Do you mean "We ensure that ... is valid"?

::: toolkit/components/reputationservice/LoginReputation.cpp:454
(Diff revision 7)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  NS_ENSURE_ARG_POINTER(aRequest);
> +
> +  // gShuttingDown shouldn be false here because we disconnect all MozPromiseRequests

Is that because the MozPromiseRequests are disconnected automatically by the MozPromiseHolder when mQueryRequests is cleared?

::: toolkit/components/reputationservice/LoginReputation.cpp:472
(Diff revision 7)
> +    if (mQueryRequests[idx].get() == aRequest) {
> +      break;
> +    }
> +  }
> +
> +  MOZ_ASSERT(idx < mQueryRequests.Length());

That seems like it should probably be a runtime check otherwise we'll remove a bogus element from the array.

Comment 25

[François Marier [:francois]]

Comment on attachment 8933541 [details]
Bug 1407879 - Add testcase for login reputation service.

https://reviewboard.mozilla.org/r/204482/#review214600

::: toolkit/components/reputationservice/test/unit/test_login_rep.js:51
(Diff revision 2)
> +    Services.prefs.clearUserPref("urlclassifier.passwordAllowTable");
> +  });
> +});
> +
> +add_test(function test_setup_local_whitelist() {
> +  // Setup the http server for SafeBrowsing update, so we can sace SAFE entries

spelling: "so we can _save_"

Comment 26

[Dimi Lee [:dimi]]

Comment on attachment 8926308 [details]
Bug 1407879 - Check password field url against the local whitelist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197592/diff/7-8/

Comment 27

[Dimi Lee [:dimi]]

Comment on attachment 8933541 [details]
Bug 1407879 - Add testcase for login reputation service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204482/diff/2-3/

Comment 28

[Dimi Lee [:dimi]]

Comment on attachment 8933541 [details]
Bug 1407879 - Add testcase for login reputation service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204482/diff/3-4/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb521dc27954
Check password field url against the local whitelist. r=francois
https://hg.mozilla.org/integration/autoland/rev/b3f26bc2b5a9
Add testcase for login reputation service. r=francois

Comment 30

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/bb521dc27954
https://hg.mozilla.org/mozilla-central/rev/b3f26bc2b5a9