488796 - form history should not save extremely large values

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect)

Description

[Justin Dolske [:Dolske]]

Satchel currently just blindly stores the input name and value from a form field. We should limit both to some upper limit... Gigantic values could cause performance issues, and are not really useful for autocompleting.

It would be interesting to gather some data from existing history DBs, but I'd guess that reasonable limits would be 50 characters for the fieldname and 200 characters for the field value.

Comment 1

[Simon Bünzli]

(In reply to comment #0)
> 50 characters for the fieldname

Looking at my main profile's form history, that seems too few. The longest remembered field name is 146 characters and the longest field with more than one remembered value is 62 characters long. Most of these long field names come from ASP.net apps (ctl00$ContentPlaceHolderApplication...).

Comment 2

[Matthew N. [:MattN]]

Attachment: Limit field name and values to 200

Only store form history of form submission if name and value fields are not longer than 200 characters.

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 375884 [details] [diff] [review]
Limit field name and values to 200

Just a couple of tiny nits...

>+// Limit the length of names and values stored in form history
>+#define MAX_HISTORY_NAME_LEN    200
>+#define MAX_HISTORY_VALUE_LEN    200

No tabs!

>-          if (!name.IsEmpty())
>+          if (!name.IsEmpty() && name.Length() <= MAX_HISTORY_NAME_LEN && value.Length() <= MAX_HISTORY_VALUE_LEN)

I think it would be a little cleaner to read if this had the length checks in a separate if-clause from the isempty check, having each bail out with a |continue| if there's a problem.

Comment 4

[Matthew N. [:MattN]]

Attachment: removed tabs and separated if clauses

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 375903 [details] [diff] [review]
removed tabs and separated if clauses

>+          if(name.Length() > MAX_HISTORY_NAME_LEN 
>+               || value.Length() > MAX_HISTORY_VALUE_LEN)
>+            continue;

The conditional operator should be at the end of the line, like:

    if(name.Length() > MAX_HISTORY_NAME_LEN ||
       value.Length() > MAX_HISTORY_VALUE_LEN)

Otherwise r+, I'll just fix this upon commit so no need for a new patch (unless gavin has additional comments).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 375903 [details] [diff] [review]
removed tabs and separated if clauses

"if (" (with a space), too :)

Comment 7

[Simon Bünzli]

(In reply to comment #0)
> Gigantic values could cause performance issues, and are not really useful
> for autocompleting.

Has this ever been a real-world issue? I.e. have we ever actually observed (malicious) sites use forms with huge names/values? And are there really no use cases where a value longer than 200 characters might be reasonable?

To me, this mainly looks like a potential future dataloss issue (and a subtle one at that)...

> It would be interesting to gather some data from existing history DBs

Has this already happened or will this ever happen before the patch lands?

Comment 8

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0ba276249f03

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #7)

> Has this ever been a real-world issue? I.e. have we ever actually observed
> (malicious) sites use forms with huge names/values?

That shouldn't matter -- it's a potential issue that could happen, so we should close it off.

> And are there really no use
> cases where a value longer than 200 characters might be reasonable?

I think it's rather unlikely, and is an edge case that we shouldn't design for. [That said, we can certainly tweak the value, as it's a somewhat arbitrary limit.] Form history UI really isn't designed to work with large values -- it's crammed into an autocomplete popup, and doesn't do substring matches.

FWIW, looking at the form history in my daily-use profile, all the long values are basically useless. Some data: image URIs, a number of fields from one site with a bunch of HTML stuck in them (?!), a few bugzilla fields for with enormous CC lists or bug dependency lists, and a few tinyurl(?) values where I had entered gigantic URLs. All of these values were only used once (the initial entry).

The query, in case anyone is interested, was:

SELECT length(value),timesUsed,fieldname,value FROM moz_formhistory ORDER BY length(value);

> To me, this mainly looks like a potential future dataloss issue (and a subtle
> one at that)...

Calling that dataloss is a bit of a stretch, IMO.

> > It would be interesting to gather some data from existing history DBs
> 
> Has this already happened or will this ever happen before the patch lands?

I've got a half-written version of a blog post I've been meaning to post, talking about form expiration and containing a crude version of what the Places Stats Project did (basically, cut'n'paste some code into the JS console, get a window showing some form history stats). I gathered some data a couple of months ago, although not specifically string lengths.

Comment 10

[Simon Bünzli]

(In reply to comment #9)
> That shouldn't matter -- it's a potential issue that could happen

Is an academic concern is enough to take this without Beta exposure, though?

> Calling that dataloss is a bit of a stretch, IMO.

About as stretchy as calling this an "issue"? ;-) Anyway, who knows what creative use extensions have for form history which might reasonably need more than 200 characters. Maybe that value should be upped to a number of characters you can't see all at once in a very long text field (say 512) - just to be on the safe side?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> Anyway, who knows what creative use extensions have for form history which
> might reasonably need more than 200 characters.

For what it's worth, the limit only applies to what form history itself will save in response to form submissions. It doesn't apply to extensions that use nsIFormHistory directly.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 375903 [details] [diff] [review]
removed tabs and separated if clauses

a191=beltzner

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b94a7b691d8e

Comment 14

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b1pre) Gecko/20090925 Namoroka/3.6b1pre