769379 - implement the value sanitizing algorithm for <input type=date>

Comment on attachment 638630 [details] [diff] [review]
patch

Review of attachment 638630 [details] [diff] [review]:
-----------------------------------------------------------------

I would have write the validation checker more like a parsing algorithm. It's a bit too late to have a deeper look on that code but you have already some changes to do so I will have a look later. Otherwise, we should discuss about this.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2622,5 @@
> +                                   PRUint32 &aMonth, 
> +                                   PRUint32 &aDay,
> +                                   nsAString &aValue) const
> +{
> +  PRInt32 ec;

nit: define that when you actually need it.

@@ +2625,5 @@
> +{
> +  PRInt32 ec;
> +  PRInt32 offset = aValue.FindChar('-', 0);
> +  // Check that the year length is 4 or more.
> +  if (offset < 4) {

I don't like that we artificially want the year to be above or equal 1000. I don't see how this could be explained technically. Maybe we should raise this to the specs?

@@ +2663,5 @@
> +  return NS_OK;
> +}
> +
> +PRUint32
> +nsHTMLInputElement::MaxDayInMonth(PRUint32 aYear, PRUint32 aMonth) const

I would have invert |aYear| and |aMonth| here because we want to know the number of days of the |aMonth|.
The method should also be named ::NumberOfDaysInMonth().

Also, it might be great to explain as a big comment at the beginning of the function, in real words, how this is working. Feel free to copy-paste the specs.

@@ +2667,5 @@
> +nsHTMLInputElement::MaxDayInMonth(PRUint32 aYear, PRUint32 aMonth) const
> +{
> +  const bool longMonths[] = { true, false, true, false, true, false, 
> +                              true, true, false, true, false, true };
> +  if (longMonths[aMonth-1]) {

Before that line, you yould do:
MOZ_ASSERT(aMonth <= 12 && aMonth > 0);

@@ +2670,5 @@
> +                              true, true, false, true, false, true };
> +  if (longMonths[aMonth-1]) {
> +    return 31;
> +  }
> +  if (aMonth != 2){

nit: empty line before that.

@@ +2673,5 @@
> +  }
> +  if (aMonth != 2){
> +    return 30;
> +  }
> +  if (aYear % 400 == 0 || (aYear % 100 != 0 && aYear % 4 == 0)) {

nit: empty line before.

::: content/html/content/src/nsHTMLInputElement.h
@@ +575,5 @@
> +   */
> +  PRInt32 GetValueAsDate(PRUint32 &aYear,
> +                         PRUint32 &aMonth,
> +                         PRUint32 &aDay,
> +                         nsAString &aValue) const;

Few things:
- I would prefer if the method was named as it is used instead of preparing it for a further patch.
- Usually input arguments are first, output arguments are last.

@@ +578,5 @@
> +                         PRUint32 &aDay,
> +                         nsAString &aValue) const;
> +
> +  /**
> +   * returns the maximum possible day in the month aMonth and  the year aYear

As said in the cpp file, this is not the maximum number of days but the number of days. January 2013 will have 31 days, it's not a maximum, it's a fact... unless the earth blows up before ;)

::: content/html/content/test/forms/test_input_date_sanitize.html
@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=765772
> +-->
> +<head>
> +  <title>Test for Bug 765772</title>

nit: put a real title.

@@ +6,5 @@
> +<head>
> +  <title>Test for Bug 765772</title>
> +  <script type="application/javascript" src="/MochiKit/packed.js"></script>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>

nit: no need for that.

@@ +18,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +var input = document.getElementById('i');

You can also simply do:
var input = document.createElement('input');

@@ +37,5 @@
> +  "2012-3-001",
> +  "2012-12-00",
> +  "2012-11-31",
> +  "2011-02-29",
> +  "2100-02-29",

What about:
"a2000-01-01",
"2000a-01-0'",
"20aa00-01-01",
"2000a2000-01-01",
"2000-1-1",
"2000-1-01",
"2000-01-1",
"2000-01-01 ",
"2000- 01-01",

Attachment #638630 - Flags: review?(mounir)

Raphael Catolino (:raphc)

Assignee

Comment 3

•

12 years ago

(In reply to Mounir Lamouri (:mounir) from comment #2)

> I would have write the validation checker more like a parsing algorithm.
> It's a bit too late to have a deeper look on that code but you have already
> some changes to do so I will have a look later. Otherwise, we should discuss
> about this.

I actually implemented it more that way at first. It looked like that http://hg.mozilla.org/users/rcatolino_mozilla.com/forms-patch-queue/file/1df1723ede73/input-date-value-sanitizing.patch

> @@ +2625,5 @@
> > +{
> > +  PRInt32 ec;
> > +  PRInt32 offset = aValue.FindChar('-', 0);
> > +  // Check that the year length is 4 or more.
> > +  if (offset < 4) {
> 
> I don't like that we artificially want the year to be above or equal 1000. I
> don't see how this could be explained technically. Maybe we should raise
> this to the specs?
> 

Hum the number of digit must be above 4 but it doesn't mean that the year should be above 1000. 0001 is valid for instance. I guess the spec asks for 4 digits to avoid confusion when reading something like 99-01-01 and wondering whether it means 1999-01-01 or 0099-01-01.

Raphael Catolino (:raphc)

Assignee

Comment 4

•

12 years ago

Attached patch patch (obsolete) (deleted) — Details — Splinter Review

Attachment #638630 - Attachment is obsolete: true

Attachment #639120 - Flags: review?(mounir)

Mounir Lamouri (:mounir)

Comment 5

•

12 years ago

Comment on attachment 639120 [details] [diff] [review]
patch

Review of attachment 639120 [details] [diff] [review]:
-----------------------------------------------------------------

It looks generally good but I would like to see the updated patch because there are a lot of small things to fix.

Maybe you could add your test inside:
content/html/content/test/test_bug549475.html
And rename the file to:
content/html/content/test/forms/test_input_sanitization.html

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2605,5 @@
>        }
>        break;
> +    case NS_FORM_INPUT_DATE:
> +      {
> +        if (!GetValueAsDate(aValue)) {

IsValidDate()

@@ +2629,5 @@
> +  PRUint32 month = 0;
> +  PRUint32 day = 0;
> +  PRInt32 fieldMaxSize = 0;
> +  PRInt32 fieldMinSize = 4;
> +  PRInt32 field = 0;

Could you use an enum?

@@ +2634,5 @@
> +  PRInt32 fieldSize = 0;
> +  PRInt32 ec;
> +  PRUint32 offset;
> +
> +  for (offset = 0; offset < aValue.Length(); offset++) {

nit: ++offset

@@ +2642,5 @@
> +      }
> +
> +      switch(field) {
> +        case 0:
> +          year = PromiseFlatString(StringHead(aValue, offset)).ToInteger(&ec);

Add:
NS_ENSURE_SUCCESS(ec, false);

@@ +2645,5 @@
> +        case 0:
> +          year = PromiseFlatString(StringHead(aValue, offset)).ToInteger(&ec);
> +          break;
> +        case 1:
> +          month = PromiseFlatString(Substring(aValue, offset-fieldSize, offset)).ToInteger(&ec);

ditto

@@ +2656,5 @@
> +      }
> +
> +      fieldSize = 0;
> +      fieldMaxSize = 2;
> +      fieldMinSize = 2;

Add a comment explaining that except the first field, they all follow that rule.

@@ +2673,5 @@
> +    fieldSize++;
> +  }
> +
> +  if (field != 2 || fieldSize != fieldMinSize) {
> +    return false;

I don't like that |day| is parsed after the main loop.
Instead of:
if(aValue[offset] == '-') {
you could do:
if (aValue[offset] == '-' || offsett == (aValue.Length() - 1)) {

IOW, we should consider the end of a field when we reach the end of the string.

But to make that work correctly, you should actually, make sure that aValue[offset] is a digit.

To make that happen, you should have something like:
for (PRUint32 offset = 0; offset < aValue.Length(); ++offset) {
  if (CurrentFieldIsBiggerThanTheMaxSize) {
    return false;
  }

  // Illegal character.
  if (aValue[offset] != '-' && !NS_IsAsciiDigit(aValue[offset]) {
    return false;
  }

  // This is not the end of a field.
  if (aValue[offset] != '-' && offset != (aValue.Length() - 1)) {
    continue;
  }

  // Now you can parse the field.
}

@@ +2686,5 @@
> +}
> +
> +PRUint32
> +nsHTMLInputElement::NumberOfDaysInMonth(PRUint32 aMonth, PRUint32 aYear) const
> +{

You need to add some comments here like:
/*
 * This methods returns the number of days for a given month (in a given year.).
 * Months that are |longMonths| always have 31 days.
 * Months that are not |longMonths| have 30 days except February (month 2).
 * February has 29 days during leap years which are years that are divisible by 400 or divisible by 100 and 4. February has 28 days otherwise.
 */

@@ +2688,5 @@
> +PRUint32
> +nsHTMLInputElement::NumberOfDaysInMonth(PRUint32 aMonth, PRUint32 aYear) const
> +{
> +  const bool longMonths[] = { true, false, true, false, true, false,
> +                              true, true, false, true, false, true };

Given that this method might be called a lot as soon as we have one <input type='date'>. Maybe we could have this array being static. That way, the first time it is used, it will be stored in memory and stay there until the application is closed. That would solve allocation/deallocation every time we call that function.

@@ +2690,5 @@
> +{
> +  const bool longMonths[] = { true, false, true, false, true, false,
> +                              true, true, false, true, false, true };
> +  MOZ_ASSERT(aMonth <= 12 && aMonth > 0);
> +  if (longMonths[aMonth-1]) {

nit: leave an empty line between MOZ_ASSERT() and if()

@@ +2699,5 @@
> +    return 30;
> +  }
> +
> +  if (aYear % 400 == 0 || (aYear % 100 != 0 && aYear % 4 == 0)) {
> +    return 29;

You could use that syntax sugar:
return (aYear % 400 == 0 || (aYear % 100 != 0 && aYear % 4 == 0)
         ? 29 : 28;

::: content/html/content/src/nsHTMLInputElement.h
@@ +570,5 @@
>    /**
> +   * Parse a date string of the form yyyy-mm-dd
> +   * @param the string to be parsed.
> +   * @return the year, month, day parsed.
> +   * @result wether the parsing was successful.

You should remove:
"@return the year, month, day parsed."
and s/@result/@return/

@@ +572,5 @@
> +   * @param the string to be parsed.
> +   * @return the year, month, day parsed.
> +   * @result wether the parsing was successful.
> +   */
> +  bool GetValueAsDate(nsAString &aValue) const;

Could you rename this "IsValidDate()"?

@@ +575,5 @@
> +   */
> +  bool GetValueAsDate(nsAString &aValue) const;
> +
> +  /**
> +   * returns the maximum possible day in the month aMonth and  the year aYear

nit: Returns [...] aYear.

::: content/html/content/test/forms/test_input_date_sanitize.html
@@ +48,5 @@
> +  "2000-1-01",
> +  "2000-01-1",
> +  "2000-01-01 ",
> +  "2000- 01-01",
> +  "-1970-01-01",

What about:
"0000-00-00",
"0001-00-00",
"1234 12 12",
"1234-12-42",

@@ +57,5 @@
> +  "1234-12-12",
> +  "1234567890-01-02",
> +  "2012-12-31",
> +  "2012-02-29",
> +  "2000-02-29",

Could you add:
"0000-01-01",

Attachment #639120 - Flags: review?(mounir)

Raphael Catolino (:raphc)

Assignee

Comment 6

•

12 years ago

Attached patch patch (obsolete) (deleted) — Details — Splinter Review

Attachment #639120 - Attachment is obsolete: true

Attachment #641778 - Flags: review?(mounir)

Mounir Lamouri (:mounir)

Comment 7

•

12 years ago

Comment on attachment 641778 [details] [diff] [review]
patch

Review of attachment 641778 [details] [diff] [review]:
-----------------------------------------------------------------

Use 'hg mv' to move the file. Don't 'hg rm' + 'hg add because that doesn't keep the history. And it's also harder to see what changes you did ;)

Sorry, I think I will need to look at this patch another time.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2673,5 @@
>        break;
> +    case NS_FORM_INPUT_DATE:
> +      {
> +        if (!IsValidDate(aValue)) {
> +          aValue.Truncate();

Check if the value isn't empty before checking if the value is valid:
if (!aValue.IsEmpty() && !IsValidDate(aValue)) {
  aValue.Truncate();
}

@@ +2684,5 @@
> +bool
> +nsHTMLInputElement::IsValidDate(nsAString &aValue) const
> +{
> +  /*
> +     parse the year, month, day values out a date string formatted as 'yyy-mm-dd'.

nit: "Parse [...]."

@@ +2701,5 @@
> +    YEAR, MONTH, DAY
> +  } field;
> +  PRInt32 fieldSize = 0;
> +  PRInt32 ec;
> +  PRUint32 offset;

Remove |offset| from here and declare it in the for loop.

@@ +2706,5 @@
> +
> +
> +  if (aValue.IsEmpty()) {
> +    return false;
> +  }

You can do that at the beginning, before initializing any variable if you want. Up to you.

@@ +2710,5 @@
> +  }
> +
> +  field = YEAR;
> +  for (offset = 0; offset < aValue.Length(); ++offset) {
> +

for (PRUint32 offset, [...]) {

and remove the blank line.

@@ +2711,5 @@
> +
> +  field = YEAR;
> +  for (offset = 0; offset < aValue.Length(); ++offset) {
> +
> +    // test if the fied size is superior to its maximum size.

nit: "// Test [...]."

@@ +2716,5 @@
> +    if (fieldMaxSize && fieldSize > fieldMaxSize) {
> +      return false;
> +    }
> +
> +    // Illegal char

nit: // Illegal char.

@@ +2727,5 @@
> +      fieldSize++;
> +      continue;
> +    }
> +
> +    // Parse the field

nit: // Parse the field.

@@ +2735,5 @@
> +
> +    switch(field) {
> +      case YEAR:
> +        year = PromiseFlatString(StringHead(aValue, offset)).ToInteger(&ec);
> +        NS_ENSURE_SUCCESS(ec, false);

According to the specs, year > 0. Please, check that and add a test checking that "0000-01-01" isn't valid.

@@ +2743,5 @@
> +        fieldMaxSize = 2;
> +        fieldMinSize = 2;
> +        break;
> +      case MONTH:
> +        month = PromiseFlatString(Substring(aValue, offset-fieldSize, offset)).ToInteger(&ec);

nit: 80 chars.

@@ +2756,5 @@
> +        fieldMinSize = 1;
> +        fieldMaxSize = 1;
> +        break;
> +      case DAY:
> +        day = PromiseFlatString(Substring(aValue, offset-fieldSize, offset + 1)).ToInteger(&ec);

nit: 80 chars.

@@ +2762,5 @@
> +
> +        if (day <  1 || day > NumberOfDaysInMonth(month, year)) {
> +          return false;
> +        }
> +        break;

Can you add a field type NONE and set it here. At the beginning of the for loop, check if the field type is NONE. If it is, return false.

@@ +2770,5 @@
> +
> +    fieldSize = 0;
> +  }
> +
> +  return true;

I don't understand how "1337" isn't valid when passed to that method. It will validate the year part and will leave the for loop, thus return 'true'. Is there something I'm missing.
If there is not, do: |return field == NONE;| given that NONE will return false if we go inside the loop. NONE when we are outside the loop is what we are expecting to have.

@@ +2777,5 @@
> +PRUint32
> +nsHTMLInputElement::NumberOfDaysInMonth(PRUint32 aMonth, PRUint32 aYear) const
> +{
> +  static const bool longMonths[] = { true, false, true, false, true, false,
> +                              true, true, false, true, false, true };

nit: indentation

::: content/html/content/src/nsHTMLInputElement.h
@@ +569,5 @@
>  
>    /**
> +   * Parse a date string of the form yyyy-mm-dd
> +   * @param the string to be parsed.
> +   * @return wether the is a valid date.

nit: whether

@@ +579,5 @@
> +   * This methods returns the number of days for a given month, in a given year.
> +   * Months that are |longMonths| always have 31 days.
> +   * Months that are not |longMonths| have 30 days except February (month 2).
> +   * February has 29 days during leap years which are years that are divisible by 400.
> +   * or divisible by 100 and 4. February has 28 days otherwise.

I was actually expecting a general comment in the header, not something as detailed. The details should go to the implementation so the readers can more easily understand what is happening and what was the goal. Like you did for |IsValidDate| actually.

For example, you should say here that |aMonth| has to be between 0 and 11.

::: content/html/content/test/forms/test_input_sanitization.html
@@ +74,5 @@
> +    "2000-01-1",
> +    "2000-01-01 ",
> +    "2000- 01-01",
> +    "-1970-01-01",
> +    "0000-00-00",

Add: "0000-01-01". I think your code doesn't handle that case.

Attachment #641778 - Flags: review?(mounir) → review-

Raphael Catolino (:raphc)

Assignee

Comment 8

•

12 years ago

Attached patch patch (obsolete) (deleted) — Details — Splinter Review

Attachment #641778 - Attachment is obsolete: true

Attachment #649255 - Flags: review?(mounir)

Raphael Catolino (:raphc)

Assignee

Comment 9

•

12 years ago

Attached patch patch (obsolete) (deleted) — Details — Splinter Review

Forgot a nit in the previous patch.

Attachment #649255 - Attachment is obsolete: true

Attachment #649255 - Flags: review?(mounir)

Attachment #649257 - Flags: review?(mounir)

Mounir Lamouri (:mounir)

Comment 10

•

12 years ago

Comment on attachment 649257 [details] [diff] [review]
patch

Review of attachment 649257 [details] [diff] [review]:
-----------------------------------------------------------------

General nit: coding style is "Type& name", not "Type &name".

r=me with the change below.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2672,5 @@
>        }
>        break;
> +    case NS_FORM_INPUT_DATE:
> +      {
> +        if (!IsValidDate(aValue)) {

if (!aValue.IsEmpty() && !IsValidDate(aValue)) {

Attachment #649257 - Flags: review?(mounir) → review+

Raphael Catolino (:raphc)

Assignee

Comment 11

•

12 years ago

Attached patch patch (deleted) — Details — Splinter Review

Attachment #649257 - Attachment is obsolete: true

Mounir Lamouri (:mounir)

Comment 12

•

12 years ago

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

Status: NEW → RESOLVED

Closed: 12 years ago

Flags: in-testsuite+

Resolution: --- → FIXED

Target Milestone: --- → mozilla20

Mounir Lamouri (:mounir)

Updated

•

12 years ago

Keywords: dev-doc-needed

Mounir Lamouri (:mounir)

Updated

•

12 years ago

Depends on: 828938

Mounir Lamouri (:mounir)

Updated

•

12 years ago

Depends on: 829210

Florian Scholz (Open Web Docs)

Comment 13

•

11 years ago

See bug 866440 for documentation.

Keywords: dev-doc-needed → doc-bug-filed

patch 12 years ago Raphael Catolino (:raphc) (deleted), patch		Details \| Diff \| Splinter Review
patch 12 years ago Raphael Catolino (:raphc) (deleted), patch		Details \| Diff \| Splinter Review
patch 12 years ago Raphael Catolino (:raphc) (deleted), patch	mounir : review-	Details \| Diff \| Splinter Review
patch 12 years ago Raphael Catolino (:raphc) (deleted), patch		Details \| Diff \| Splinter Review
patch 12 years ago Raphael Catolino (:raphc) (deleted), patch	mounir : review+	Details \| Diff \| Splinter Review
patch 12 years ago Raphael Catolino (:raphc) (deleted), patch		Details \| Diff \| Splinter Review