Bug 59951 – Implement Date and Time Input Value Sanitization

• Component: Forms
• Status: RESOLVED FIXED

[Joseph Pecoraro]

A part of: <http://webkit.org/b/37024> Implement value sanitization algorithms

Note that adding value sanitization makes ValidityState.typeMismatch much less
useful, since there should no longer be invalid values.

[Joseph Pecoraro]

Created attachment 91951 [details]
[PATCH] Add Date+Time Value Sanitization

The actual WebCore diff is very small, thanks to the well designed classes.

[Joseph Pecoraro]

I think a typeMismatch can still happen for a localized string.
So, say a localized string implementation accepts "today" as
a valid string. This would not sanitize that value, but
typeMismatch might say it is invalid. I'll test this out. If that
is the case, maybe I should just change typeMismatch to be
what I have in sanitize()?

[Joseph Pecoraro]

(In reply to comment #2)
> I think a typeMismatch can still happen for a localized string.
> So, say a localized string implementation accepts "today" as
> a valid string. This would not sanitize that value, but
> typeMismatch might say it is invalid. I'll test this out.

Yes, this is the case with elem.value = 'today', but not the
case when the user types in "today", and convertVisibleValue
runs, and sets the internal value correctly. I think this is fine.

However, this pointed out to me that we can still get type
mismatches if a user types into the date field, because it
is a text field. I should update the tests to make sure that
if, instead of elem.value = '...', the user types '...' that there
is or is not a type mismatch.

Also, this may be a performance issue, since validity state
checking seems to run sanitization on each value change,
causing this to be run 7 times, which could be costly.
I'll post a follow-up.

[Darin Adler]

Comment on attachment 91951 [details]
[PATCH] Add Date+Time Value Sanitization

View in context: https://bugs.webkit.org/attachment.cgi?id=91951&action=review

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:221
> +    double parsedValue = parseLocalizedDate(proposedValue, dateType());
> +    if (isfinite(parsedValue))

I don’t think you need the local variable, nor do I think the local variable’s name adds much clarity. We can put this all on one line.

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:227
> +    if (!parseToDateComponents(proposedValue, 0))
> +        return String();
> +
> +    return proposedValue;

I know that often we do early return, but this function seems to return early in each case where it decides the value is good, except for this last case, where it does the opposite. I suggest reversing the sense to make the function overall read better.

Another way to do it would be to factor this into a function that returns a boolean and then this function would just call that and then return the appropriate string. It’s a little strange that the code in this function is not sanitizing, just rejecting values that can’t be parsed.

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 91951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91951&action=review
> 
> > Source/WebCore/html/BaseDateAndTimeInputType.cpp:221
> > +    double parsedValue = parseLocalizedDate(proposedValue, dateType());
> > +    if (isfinite(parsedValue))
> 
> I don’t think you need the local variable, nor do I think the local variable’s name
> adds much clarity. We can put this all on one line.

Done. This was the style of the rest of the file. But I agree, in this case it doesn't matter.


> > Source/WebCore/html/BaseDateAndTimeInputType.cpp:227
> > +    if (!parseToDateComponents(proposedValue, 0))
> > +        return String();
> > +
> > +    return proposedValue;
> 
> I know that often we do early return, but this function seems to return early in each
> case where it decides the value is good, except for this last case, where it does the
> opposite. I suggest reversing the sense to make the function overall read better.

Done. I literally just did that 30 seconds ago, heh =)


> However, this pointed out to me that we can still get type
> mismatches if a user types into the date field, because it
> is a text field. I should update the tests to make sure that
> if, instead of elem.value = '...', the user types '...' that there
> is or is not a type mismatch.

This turned out not to be true. Because the JS elem.value access
will still get the sanitized value, even if the user has typed "foo"
into the text field. So this was a false alarm. The only wacky case
is an accepted localized is not sanitized but would cause a
type mismatch.

Anyone have any thoughts on this? I think I should remove
the localized string check in sanitize. It is already an unlikely
scenario (no port currently implements LocalizedDate
convertFromVisibleValue). Also it means a non-standard
string could be assigned to "value".


> Also, this may be a performance issue, since validity state
> checking seems to run sanitization on each value change,
> causing this to be run 7 times, which could be costly.
> I'll post a follow-up.

Even though Darin r+'d this, I'd like to take a look at this
issue before landing to see if it can be improved. I'll probably
end up opening a separate bug on this.

[Joseph Pecoraro]

Created attachment 91997 [details]
[PATCH] Add Date+Time Value Sanitization

I decided sanitizeValue should actually sanitize HTML5 values,
and not include localized dates as valid. It makes no send
for a page to do elem.value = 'today' and have it actually work.

This also includes some JavaScript cleanup to the last patch.
Darin, are you okay with the change?

I will be filing a separate bug on improving ValidityState::valid,
which accesses input->value() 7 times, each of which might
re-sanitize the value without caching. Seems like there should
be some improvements there.

[Joseph Pecoraro]

Filed: <http://webkit.org/b/59975> ValidityState::valid causes value sanitization 7 times

[Kent Tamura]

Comment on attachment 91997 [details]
[PATCH] Add Date+Time Value Sanitization

This change is ok.  But could you hold on landing it please?

Many developers don't like the WebKit text field implementation of date/time types and want rich UI like Opera.  Some JavaScript libraries detect the text field implementation by having no value sanitization.
So I thought value sanitization should be implemented after rich UI like calendar picker was implemented.

[Joseph Pecoraro]

(In reply to comment #8)
> (From update of attachment 91997 [details])
> This change is ok.  But could you hold on landing it please?
> 
> Many developers don't like the WebKit text field implementation of date/time
> types and want rich UI like Opera.  Some JavaScript libraries detect the text
> field implementation by having no value sanitization.
>
> So I thought value sanitization should be implemented after rich UI like
> calendar picker was implemented.

Yep, I understand. Its would be irresponsible to think that users will
type in "2011-05-02T23:45:03.648Z" in a datetime field or else
they get the empty string sent.

The actual change is quite small, and could be hidden behind a flag
if we really wanted it to land early. But I don't think there is a rush.

How is the UI coming?

[Kent Tamura]

(In reply to comment #9)
> How is the UI coming?

I have a working code for type=date (Bug 53961), and no other implementations yet.
I think it takes some months to complete UI.

[Eric Seidel (no email)]

Comment on attachment 91997 [details]
[PATCH] Add Date+Time Value Sanitization

I'm surprised we don't auto-detect sanitization in the tests by just checking what the JS object returns after we set it.  Or maybe that wouldn't work.

[Joseph Pecoraro]

(In reply to comment #11)
> (From update of attachment 91997 [details])
> I'm surprised we don't auto-detect sanitization in the tests by just checking
> what the JS object returns after we set it.  Or maybe that wouldn't work.

Unless I'm misunderstanding you, I think that is what we do. If we expect the
value to be sanitized, we expect its value to be the empty string after we set it:

  - PASS The value "foo" doesn't underflow the minimum value "2011-W01".
  + PASS The value "" sanitized from "foo" doesn't underflow the minimum value "2011-W01".

Before this change, date inputs were treated like text fields, and you could
set inputElem.value to a bad value and that would be allowed. Web content
could detect that with inputElem.validity. After this change, that would
not be allowed, and you get:

  inputElem.value = "bad";
  inputElem.value === ""; // true

However, because right now there are no UIs to manage putting valid date
strings into input fields, we feel this change shouldn't land. If it landed we
would be requiring users to type in absolutely valid strings, otherwise it
would sanitize out their bad input.

[Joseph Pecoraro]

Of course he wasn't CC'd =). I'll send the comment to him directly.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 91997 [details] [details])
> > I'm surprised we don't auto-detect sanitization in the tests by just checking
> > what the JS object returns after we set it.  Or maybe that wouldn't work.
> 
> Unless I'm misunderstanding you, I think that is what we do. If we expect the
> value to be sanitized, we expect its value to be the empty string after we set it:

I don't think my comment makes sense anymore.

[Darin Adler]

Comment on attachment 91997 [details]
[PATCH] Add Date+Time Value Sanitization

How long are we going to hold off on landing this?

[Eric Seidel (no email)]

Comment on attachment 91951 [details]
[PATCH] Add Date+Time Value Sanitization

Cleared Darin Adler's review+ from obsolete attachment 91951 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Attachment 91997 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.

[Kent Tamura]

Joseph,
Please hold on landing the patch because of Comment #8, or let's make these input types optional by ENABLE_ flags.

[Joseph Pecoraro]

Comment on attachment 91997 [details]
[PATCH] Add Date+Time Value Sanitization

Clearing r+ flag. We should block this bug on another.

[Kent Tamura]

(In reply to comment #18)
> Joseph,
> Please hold on landing the patch because of Comment #8, or let's make these input types optional by ENABLE_ flags.

ENABLE_INPUT_TYPE_* was introduced and these types were disabled by default.
Please land the patch.

[Joseph Pecoraro]

Created attachment 120417 [details]
[PATCH] Rebased (For Bots)

Updated the patch:

  - updated WebCore changes to use "const" and "OVERRIDE"
  - updated test to their new paths

[Joseph Pecoraro]

Comment on attachment 120417 [details]
[PATCH] Rebased (For Bots)

View in context: https://bugs.webkit.org/attachment.cgi?id=120417&action=review

Minor fixes I'll need to make before landing.

> LayoutTests/fast/forms/datetime/ValidityState-rangeOverflow-datetime-expected.txt:12
> -PASS The value "foo" doesn't overflow the maximum value "2011-01-26T12:34Z".
> +PASS The value "" doesn't overflow the maximum value "2011-01-26T12:34Z".

Typo will cause this line to change.

> LayoutTests/fast/forms/datetime/ValidityState-rangeOverflow-datetime.html:29
> +function checkNotOverflow(value, max, disabled, sanitized, sanitized)

I'll fix this typo.

[Kent Tamura]

Comment on attachment 120417 [details]
[PATCH] Rebased (For Bots)

Thanks!

[Joseph Pecoraro]

Landed in r103957 <http://trac.webkit.org/changeset/103957>.