1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P5)

Description

[Luke Chang [:lchang] (inactive)]

Since we don't want the master password dialog to show up before any credit card doorhanger, currently we only use the last 4 digits of the credit card number to search if there's any duplicate record in the storage. It results in that no doorhanger shows up if the incoming record contains a number whose last 4 digits are identical to one of the records in the storage.

This issue can be mitigated by decrypting the number before comparing when we're sure no master password dialog will be involved.

Comment 1

[Luke Chang [:lchang] (inactive)]

On a second thought, I think maybe we shouldn't use the decrypted credit card number to search duplicate records when master password is enabled (even it's logged in already) as it might put the credit card number at risk. For example, when Firefox has logged in the master password, someone else can brute-force trying your credit card number by checking if doorhanger appears without having any idea of your master password.

Therefore, I changed the title to only use decrypted number to compare numbers when the master password is disabled.

Comment 2

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled.

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

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8931254 [details]
Bug 1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled.

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

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8931254 [details]
Bug 1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled.

https://reviewboard.mozilla.org/r/202392/#review208800

::: browser/extensions/formautofill/ProfileStorage.jsm:1665
(Diff revision 2)
>        let isDuplicate = this.VALID_FIELDS.every(field => {
>          if (!clonedTargetCreditCard[field]) {
>            return !creditCard[field];
>          }
> -        if (field == "cc-number") {
> +        if (field == "cc-number" && creditCard[field]) {
> +          if (MasterPassword.isEnabled) {

How about `if (MasterPassword.isEnabled && !MasterPassword.isLoggedIn)` so we can compare full number if it's logged in already?

::: browser/extensions/formautofill/test/unit/test_creditCardRecords.js:560
(Diff revision 2)
> +  let token = tokendb.getInternalKeyToken();
> +  token.reset();
> +  token.initPassword("password");
> +  do_check_eq(profileStorage.creditCards.getDuplicateGuid(record), guid);
> +
> +  // ... Even though the master password is enabled and the last 4 digits are the

I'm not sure why this test would work cause I didn't see we handle the invalid number in the `getDuplicateGuid`

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8931254 [details]
Bug 1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled.

https://reviewboard.mozilla.org/r/202392/#review208800

> How about `if (MasterPassword.isEnabled && !MasterPassword.isLoggedIn)` so we can compare full number if it's logged in already?

I'm on purpose not to use `!MasterPassword.isLoggedIn` to avoid potential risks. See comment 1 for details.

> I'm not sure why this test would work cause I didn't see we handle the invalid number in the `getDuplicateGuid`

Because the incoming record will be normalized and invalid numbers will be deleted. This test should pass without this patch but I want to ensure it won't be broken in the future.

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8931254 [details]
Bug 1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled.

https://reviewboard.mozilla.org/r/202392/#review208838

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8931254 [details]
Bug 1418223 - Use the decrypted credit card number to search duplicate records in the storage if master password is disabled.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5bc80ce386c
Use the decrypted credit card number to search duplicate records in the storage if master password is disabled. r=steveck

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c5bc80ce386c