1360694 - [Form Autofill] Cap the length of profile fields saved in storage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P5)

Description

[Matthew N. [:MattN]]

Similar to bug 488796, we shouldn't save huge fields in autofill storage since we don't ask to save each profile.

Comment 1

[Ray Lin[:ralin]]

Unassign myself as MattN has a ready-made patch for this :D

Comment 2

[Luke Chang [:lchang] (inactive)]

Hi Matt,

I'd like to take over this bug if you don't mind.

Comment 3

[Matthew N. [:MattN]]

Attachment: WIP patch

Comment 4

[Luke Chang [:lchang] (inactive)]

Matt, Thanks.

Comment 5

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1360694 - [Form Autofill] Cap the length of profile fields saved in storage.

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

Comment 6

[Luke Chang [:lchang] (inactive)]

Ray, Would you mind taking a look at this patch (since you once studied it)? Thanks.

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8919993 [details]
Bug 1360694 - [Form Autofill] Cap the length of profile fields saved in storage.

https://reviewboard.mozilla.org/r/190942/#review196142

LGTM, thanks.  few minor comments, but not big deal.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:691
(Diff revision 1)
>        log.debug("No address record saving since there are only",
>                       Object.keys(data.address.record).length,
>                       "usable fields");

maybe we can make these lines in the same indent level

::: browser/extensions/formautofill/FormAutofillUtils.jsm:31
(Diff revision 1)
>    "givenName", "additionalName", "familyName", "organization", "streetAddress",
>    "state", "province", "city", "country", "zip", "postalCode", "email", "tel",
>  ];
>  const MANAGE_CREDITCARDS_KEYWORDS = ["manageCreditCardsTitle", "addNewCreditCardTitle", "showCreditCardsBtnLabel"];
>  const EDIT_CREDITCARD_KEYWORDS = ["cardNumber", "nameOnCard", "cardExpires"];
> +const MAX_FIELD_VALUE_LENGTH = 200;

A comment would be a plus to clarify its usage.

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8919993 [details]
Bug 1360694 - [Form Autofill] Cap the length of profile fields saved in storage.

https://reviewboard.mozilla.org/r/190942/#review196142

> A comment would be a plus to clarify its usage.

Good point. Thanks.

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8919993 [details]
Bug 1360694 - [Form Autofill] Cap the length of profile fields saved in storage.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72cce53ae7fd
[Form Autofill] Cap the length of profile fields saved in storage. r=ralin

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/72cce53ae7fd

Comment 12

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8919993 [details]
Bug 1360694 - [Form Autofill] Cap the length of profile fields saved in storage.

Approval Request Comment
[Feature/Bug causing the regression]: Feature.
[User impact if declined]: A potential risk that users' disks might be filled by malicious data.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Verified locally.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just added a check for string length.
[String changes made/needed]: N/A

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919993 [details]
Bug 1360694 - [Form Autofill] Cap the length of profile fields saved in storage.

Must fix, Beta57+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6273b5da0f81

Comment 15

[Andrei Vaida [:avaida]]

(In reply to Luke Chang [:lchang] from comment #12)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Verified locally.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Luke's assessment on manual testing needs and the fact that this fix has automated coverage.