Bug 59951

Created attachment 91951 [details] [PATCH] Add Date+Time Value Sanitization The actual WebCore diff is very small, thanks to the well designed classes.

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()?

(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.

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.

(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.

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.

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

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.

(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?

(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.

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.

(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.

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

(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.

Comment on attachment 91997 [details] [PATCH] Add Date+Time Value Sanitization How long are we going to hold off on landing this?

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.

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

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

Comment on attachment 91997 [details] [PATCH] Add Date+Time Value Sanitization Clearing r+ flag. We should block this bug on another.

(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.

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

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.

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

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