1500748 - Date.parse accepts illegal non-zero-padded ISO8601 format

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

Date.parse("2018-1-7T00:00Z"); // no padded zero, should be 2018-01-07T00:00Z


Actual results:

1515283200000


Expected results:

NaN

Comment 1

[Tooru Fujisawa [:arai]]

https://tc39.github.io/ecma262/#sec-date-time-string-format
> 20.3.1.15 Date Time String Format
> ECMAScript defines a string interchange format for date-times based upon
> a simplification of the ISO 8601 calendar date extended format.
> The format is as follows: YYYY-MM-DDTHH:mm:ss.sssZ

https://tc39.github.io/ecma262/#sec-date.parse
> 20.3.3.2 Date.parse ( string )
> ...
> The function first attempts to parse the String according to the format
> described in Date Time String Format (20.3.1.15), including expanded years.
> If the String does not conform to that format the function may fall back to
> any implementation-specific heuristics or implementation-specific date
> formats.

implementation is allowed to accept any non-conforming string,
so at least this doesn't violate the current spec.
unless this is causing critical web-compat issue, the behavior won't be an immediate problem.

but Chrome, Safari, and Edge return NaN, so it might be better align them for this specific case.

bug 1274354 comment #27 may be related.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

Hmm. Can I assign myself? I'd like to make my first contribution and this seems simple enough to fix.

Looks like this is the relevant file, am I right? https://phabricator.services.mozilla.com/source/mozilla-central/browse/default/js/src/builtin/intl/DateTimeFormat.js

Comment 3

[Tooru Fujisawa [:arai]]

sure, contribution is welcome :)

this should be the related code, which says "MM   = one or two-digit month (01=January, etc.)"
https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/js/src/jsdate.cpp#1010

here's the documentation for building SpiderMonkey JS shell
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

jorendorff, do you think it's fine to reject the date format in comment #0 ?

Comment 4

[André Bargull [:anba]]

The code to accept one-digit inputs was added in bug 1205298, so we should ensure to not regress that bug. IIUC we probably could reject one-digit inputs iff the 'T' separator is present.

Comment 5

[Jason Orendorff [:jorendorff]]

We can try rejecting this format (when the 'T' is present), since V8 rejects it.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

I'm having a hard time understanding how to create a patch... Git based services let me fork the repository, change some files, open pull requests and get merged. How can I do that for Firefox?

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: bug1500748.patch

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Couldn't find a test about this type of formatting issue, where should I write one? (And hopefully how can I run it without running all others?)

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 9060617 [details] [diff] [review]
bug1500748.patch

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

::: js/src/jsdate.cpp
@@ +966,4 @@
>  
>  done_date:
>    if (PEEK('T') || PEEK(' ')) {
> +    if (i != 10) {

can you add some constant with descriptive name instead of raw `10` ?
also, please add detailed comment about the reason of the number `10`,
with the standard format described in the comment,
and how the check interacts with non-standard format

Comment 10

[Tooru Fujisawa [:arai]]

then, about the testcase, you can add to js/src/tests/non262/Date directory.
and you can run it by running jstests.py file like:

cd js/src/tests
./jstests.py PATH_TO_JS_SHELL js/src/tests/non262/Date/TEST_FILE_NAME

Comment 11

[André Bargull [:anba]]

This can't land as is because of comment #4.

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Apparently Chrome forces standard compliance when T exists as a time part marker (not when divided by a space), so I'll modify the patch to match that behavior.

Chrome also prevents time format like T1:1:1 (note T), should we also prevent it or should it be discussed separately?

Comment 13

[André Bargull [:anba]]

(In reply to saschanaz from comment #12)

Apparently Chrome forces standard compliance when T exists as a time part marker (not when divided by a space), so I'll modify the patch to match that behavior.

Great, thanks!

Chrome also prevents time format like T1:1:1 (note T), should we also prevent it or should it be discussed separately?

It's possible that there are some existing tests which expect that Date.parse("T01:01:01") is valid, because time-only formats were allowed in ES5, but got later removed in ES5.1. Apart from tests, I don't think removing support for time-only formats should result in any web-compatibility issues, because neither V8 nor JSC nor Chakra (still) support it. So it should be okay to remove it here, too, but maybe in an additional patch, though.

From ES5.1, Annex F, "Technically Significant Corrections and Clarifications in the 5.1 Edition":

15.9.1.15: Specified legal value ranges for fields that lacked them. Eliminated ―time-only‖ formats. Specified
default values for all optional fields.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Oops, I mean 1999-01-01T1:1:1, sorry to confuse you. (But wow, didn't know Firefox support T1:1:1 😲)

Comment 15

[Paul Bone [:pbone]]

Hi saschanaz,

Regarding creating patches for firefox we use Mercurial with phabricator and lando. It involves setting up some tools on your system that once setup work fairly smoothly:

Mercurial at mozilla: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html
Phabricator at mozilla: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

However, new contributors can't land code directly without first going through some reputation-building stuff and being "vouched for" and such. I got a message from :marcosc saying he's helping you with level 1 access so that you can use our "try servers". As for committing code I don't know how phabricator/lando work with level 1, I assume what happens is that you write the code, have it reviewed via phabricator and then once approved you can set the "checkin-needed" keyword on this bug and one of our sherifs will commit the code for you. Someone please correct me if I got this process wrong!

Welcome.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: 0001-Bug-1500748-Require-standard-compliance-when-a-time-.patch

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Paul Bone [:pbone] from comment #15)

Regarding creating patches for firefox we use Mercurial with phabricator and lando. It involves setting up some tools on your system that once setup work fairly smoothly:

Thanks for the guides! I didn't get my access yet so I'm waiting...

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 9060976 [details] [diff] [review]
0001-Bug-1500748-Require-standard-compliance-when-a-time-.patch

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

thanks again!

::: js/src/jsdate.cpp
@@ +918,4 @@
>    size_t tzHour = 0;
>    size_t tzMin = 0;
>  
> +  static const int standardDateLength = 10; // length of "1970-01-01"

this should be `size_t` type, given this is compared against `size_t i`.
also, this kind of constant should be all capitalized, like `STANDARD_DATE_LENGTH`

@@ +969,5 @@
>  
>  done_date:
> +  if (PEEK('T')) {
> +    if (i != standardDateLength) {
> +      // Require standard format "1970-01-01" if a time part marker "T" exists

This doesn't work for expanded years like `+001970-01-01T00:00'`:
https://tc39.github.io/ecma262/#sec-expanded-years

that corresponds to the following branch:
https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/js/src/jsdate.cpp#954-959

also, please add testcase for it.

Comment 19

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: 0001-Bug-1500748-Require-standard-compliance-when-a-time-.patch

To pass tests including 2018T10:10:10 I had to remove previous constant length comparison and instead add a flag named permissive.

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: 0001-Bug-1500748-Require-standard-compliance-when-a-time-.patch

Using bool and variable name consistency

Comment 21

[Kagami Rosylight [:saschanaz] (they/them)]

I'm constantly having trouble with try server, do anyone have an idea why this happens?

Bundling 1 changesets
ERROR sequence item 0: expected string, NoneType found
Run the command again with `git -c cinnabar.check=traceback <command>` to see the full traceback.
remote: ** Unknown exception encountered with possibly-broken third-party extension obsolescencehacks
remote: ** which supports versions 4.8 of Mercurial.
remote: ** Please disable obsolescencehacks and try your action again.
remote: ** If that fixes the bug please report it to the extension author.
remote: ** Python 2.7.5 (default, Apr 11 2018, 07:36:10) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]
remote: ** Mercurial Distributed SCM (version 4.9.1)
remote: ** Extensions loaded: blackbox, clonebundles, mozhooks, obsolescencehacks, pushlog, replicateowner, serverlog, readonly, vcsreplicator
remote: Traceback (most recent call last):
remote:   File "/var/hg/venv_pash/bin/hg", line 43, in <module>
remote:     dispatch.run()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 99, in run
remote:     status = dispatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 225, in dispatch
remote:     ret = _runcatch(req) or 0
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 376, in _runcatch
remote:     return _callcatch(ui, _runcatchfunc)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 384, in _callcatch
remote:     return scmutil.callcatch(ui, func)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/scmutil.py", line 165, in callcatch
remote:     return func()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 367, in _runcatchfunc
remote:     return _dispatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 1021, in _dispatch
remote:     cmdpats, cmdoptions)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 756, in runcommand
remote:     ret = _runcommand(ui, options, cmd, d)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 1030, in _runcommand
remote:     return cmdfunc()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 1018, in <lambda>
remote:     d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/util.py", line 1670, in check
remote:     return func(*args, **kwargs)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/commands.py", line 5270, in serve
remote:     s.serve_forever()
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 491, in serve_forever
remote:     return super(sshserverwrapped, self).serve_forever()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireprotoserver.py", line 794, in serve_forever
remote:     self.serveuntil(threading.Event())
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireprotoserver.py", line 800, in serveuntil
remote:     _runsshserver(self._ui, self._repo, self._fin, self._fout, ev)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireprotoserver.py", line 655, in _runsshserver
remote:     rsp = wireprotov1server.dispatch(repo, proto, request)
remote:   File "/var/hg/version-control-tools/pylib/vcsreplicator/vcsreplicator/hgext.py", line 546, in wireprotodispatch
remote:     return orig(repo, proto, command)
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 279, in wrappeddispatch
remote:     return orig(repo, proto, command)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireprotov1server.py", line 74, in dispatch
remote:     return func(repo, proto, *args)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireprotov1server.py", line 582, in unbundle
remote:     for p in payload:
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireprotoserver.py", line 534, in getpayload
remote:     count = int(self._fin.readline())
remote: ValueError: invalid literal for int() with base 10: ''
error: failed to push some refs to 'hg::ssh://hg.mozilla.org/try'

Comment 22

[Jean-Yves Avenard [:jya]]

(In reply to saschanaz from comment #21)

remote: ValueError: invalid literal for int() with base 10: ''
error: failed to push some refs to 'hg::ssh://hg.mozilla.org/try'

change your .git/config to use:
[remote "try"]
url = hg://hg.mozilla.org/try
pushurl = hg://hg.mozilla.org:ssh/try

And modify your ~/.ssh/config
to have an entry for hg.mozilla.org:
For example I have:
$ cat ~/.ssh/config
Host hg.mozilla.org
User [e-mail]

this works one both windows and mac/linux

You need to at least ssh to hg.mozilla.org to permanently approve the host signature and verify that it works:
e.g. you should see:
$ ssh hg.mozilla.org
A SSH connection has been successfully established.

Your account ([e-mail]) has privileges to access Mercurial over
SSH.

You are a member of the following LDAP groups that govern source control
access:

scm_level_1, scm_level_2, scm_level_3

This will give you write access to the following repos:

Firefox Repos (mozilla-central, releases/*), Project Repos (projects/), Try, User Repos (users/)

You will NOT have write access to the following repos:

Autoland (integration/autoland), Localization Repos (releases/l10n/*, others), Version Control Tools (hgcustom/version-control-tools)

You did not specify a command to run on the server. This server only
supports running specific commands. Since there is nothing to do, you
are being disconnected.
Connection to hg.mozilla.org closed.

Comment 23

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Jean-Yves Avenard [:jya] from comment #22)

(In reply to saschanaz from comment #21)

remote: ValueError: invalid literal for int() with base 10: ''
error: failed to push some refs to 'hg::ssh://hg.mozilla.org/try'

change your .git/config to use:
[remote "try"]
url = hg://hg.mozilla.org/try
pushurl = hg://hg.mozilla.org:ssh/try

And modify your ~/.ssh/config
to have an entry for hg.mozilla.org:

Thanks! I can already access hg.mozilla.org so I only added the git remote value, but the issue still occurs. Filed Bug 1549243.

Comment 24

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1500748 - Require standard compliance when a time part exists

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efacc855c356
Require standard compliance when a time part exists r=arai

Comment 26

[Natalia Csoregi [:nataliaCs]]

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

Comment 27

[Kohei Yoshino]

Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2019/date-parse-no-longer-accepts-illegal-iso-8601-date-time-formats/