118266 - JS Date type mixed with document.cookie considered harmful

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christopher Blizzard (:blizzard)]

Build is from Jan 04, 2002.

I was browsing around www.ifilm.com and I kept getting a warning about using an
unsupported browser for every page load.  The warning on the page says:

"One last thing: If you get this "Recommended Browsers" screen over and over
again, it is possible that your cookies are turned off. Please turn on your
cookies in your preferences menu and this problem should be resolved."

They have some code that sets a cookie from JS that is checked during all page
loads to check the browser version.  However, that cookie isn't being set
because the date that is set as part of setting the cookie isn't parsed properly
by PR_ParseTimeString() and comes out with about the year 500.  The string looks
like this:

Sat Jan 04 20:15:44 GMT-0500 (EST) 2003

And it comes from the date type.  I'm looking at other browsers to see what they
spit out.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: test page

Test page that shows what the date string comes out as.

Comment 2

[Christopher Blizzard (:blizzard)]

IE 5.5 uses a format like so:

Sat Jan 4 20:28:40 EST 2003

Comment 3

[Christopher Blizzard (:blizzard)]

Netscape 4.79 (Windows) looks like this:

Sat Jan 04 20:36:15 GMT-0500 (Eastern Standard Time) 2003

Comment 4

[Christopher Blizzard (:blizzard)]

Netscape 4.78 (Linux) looks like this:

Sat Jan 04 20:27:51 GMT-0500 (EST) 2003

Comment 5

[Christopher Blizzard (:blizzard)]

OK, here is my question.  Do we want to change this format or not so that it
uses the IE format?  If so, is there any impact?  It looks like people are using
this as the argument to expires= before setting document.cookie which breaks
things pretty badly.

Is there any harm in changing it?

Comment 6

[Christopher Blizzard (:blizzard)]

Mozilla 0.9.7 on win32:

Sat Jan 04 20:52:53 GMT-0500 (Eastern Standard Time) 2003

Comment 7

[Sivakiran Tummala]

this is somewhat related to the bug 116598 comment # 3 by johnny.

Comment 8

[Sivakiran Tummala]

changing summary accordingly

Comment 9

[Mike Shaver (:shaver emeritus)]

So what does the new summary mean?  That we should change from GMT to localtime?
 (Probably not, because we already provide local time, as we always have.)  That
we should change from localtime to GMT?

It's not clear to me that this is a GMT-vs-local issue.

Comment 10

[Christopher Blizzard (:blizzard)]

unchanging summary since that's not the issue - we might still yet change the
parser.

Comment 11

[Christopher Blizzard (:blizzard)]

Yet another format for the mac:


[21:18:07] <smfr> 4.x mac does:
[21:18:07] <smfr> Sat Jan 04 18:17:59 GMT-0800 2003
[21:19:31] <smfr> NS 6.2 does
[21:19:31] <smfr> Sat Jan 04 18:19:28 GMT-0800 2003
[21:22:19] <smfr> IE says:
[21:22:19] <smfr> Sat Jan 4 18:22:17 PST 2003

Comment 12

[Johnny Stenback (:jst)]

Not a DOM HTML bug, Date is a JS engine type, reassigning.

Comment 13

[Robert Ginda]

Looks to me that date_format (at jsdate.c#1530) is supposed to ignore any
timesone description with spaces (or other nasty characters) in it.

Could it somehow be broken on windows?

Comment 14

[Robert Ginda]

Oh, duh.

for (i = 0; i < tzlen; i++) {
   jschar c = tzbuf[i];
   if (c > 127 ||
       !(isalpha(c) || isdigit(c) ||
       c == ' ' || c == '(' || c == ')')) {
       usetz = JS_FALSE;
   }
}

we allow *only* alpha, digit, space, and parens.

strftime returns "The time zone or name or abbreviation" for "%Z", according to
/my/ man page.  I think we should either accept *only* alpha in tzbuf, or just
not use it at all.  ie. don't put "(EST)" *or* "(Eastern Standard Time)" in the
return string, *ever*.

Comment 15

[Robert Ginda]

...or teach PR_ParseTimeString() to treat everything in parens as whitespace.

Comment 16

[Christopher Blizzard (:blizzard)]

I'd rather not solve this in a way that adds yet another time/date format to
what web developers have to deal with.  We should either:

o Keep the current format and change PR_ParseTimeString() so that it can parse
the format for the Mac, win32 and Linux ( that would actually be adding three
new types. )

o Change the format that JS emits so that it's just like the IE format.  That
way we don't have to change PR_ParseTimeString() but we might end up breaking
hand rolled parsers on the Net.

Comment 17

[Phil Schwartau]

Reassigning to Kenton - 

Comment 18

[Brendan Eich [:brendan]]

ECMA permits the format mccabe and I agreed on, which was a compromise that
seemed to work best at the time.  Blizzard, what does 4.7x do, given that its JS
engine produces the same sort of date?  If it can parse such formats, then so
should Mozilla, which argues for your first proposed fix.

/be

Comment 19

[Christopher Blizzard (:blizzard)]

Attachment: test that sets a cookie as well

Comment 20

[Christopher Blizzard (:blizzard)]

It appears that Netscape doesn't save the cookie in the cookies file if you use
the sample I uploaded.  Netscape does, however, warn you about the cookie being
set.  My guess without having the source code is that the date isn't parsed
there either.

In the case of this site, the browser is listed as a supported browser so the
code in question isn't ever executed.

Comment 21

[Stephen P. Morse]

If n4 is accepting the cookie (as evidenced by the fact that it gives the 
warning dialog) but does not write it to the cookies.txt file, that means that 
n4 considered it to be a session cookie.  That would occur if, for examle, there 
was no date string.  So it looks like n4 rejected the date string in this case.

Comment 22

[Brendan Eich [:brendan]]

If I feed "Sun Jan 06 13:26:47 GMT-0800 (PST) 2002" or a similar string to
PR_ParseTimeString, it does not fail (it returns PR_SUCCESS), but the time it
computes is -2144963648 (for the given string above).  I'm not sure what Nav4's
code did.  This says that we need to undo mccabe's portable-timezone scheme used
by Date.prototype.toString, alas.

Kenton, can you target this bug?

/be

Comment 23

[Christopher Blizzard (:blizzard)]

If you follow PR_ParseTimeString it ends up with the year 500 or something
similar.  It's not parsing it correctly at all.

Comment 24

[Brendan Eich [:brendan]]

Eureka!  PR_ParseTimeString skips the -0800 "year" after successfully parsing
"GMT" because of the "-" in conjunction with the fact that no valid year has yet
been parsed.  If we put the 2002 after the day-in-month and before the GMT+0800
or whatever, all works fine.  That makes for an easily-hacked jsdate.c patch
that preserves mccabe's universal timezones.  I'll attach one later tonight.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: proposed fix

Looking for fast review for 0.9.8.

/be

Comment 26

[Mike Shaver (:shaver emeritus)]

Comment on attachment 63778 [details] [diff] [review]
proposed fix

sr=shaver.

Can we get some folks to test this on the other platforms, against 
blizzard's test cases and ifilm?

Comment 27

[Brendan Eich [:brendan]]

Attachment: oops, avoid trailing space if no (PST) or whatever

Review this!

Comment 28

[Mike Shaver (:shaver emeritus)]

Comment on attachment 63779 [details] [diff] [review]
oops, avoid trailing space if no (PST) or whatever

sr=shaver.

You're so picky!

Comment 29

[Kenton Hanson (gone)]

Comment on attachment 63779 [details] [diff] [review]
oops, avoid trailing space if no (PST) or whatever

May I suggest that the comment preceeding the format reflect the new format
(extra picky)

Comment 30

[Brendan Eich [:brendan]]

khanson: duh, I meant to do that, then forgot.  Thanks, fixed in my latest patch
(no point in attaching).

True confession: I made another change, to group date fields (day, month, date,
year) as FORMATSPEC_DATE does, then time fields (FORMATSPEC_TIME), when printing
FORMATSPEC_FULL.  This seems, now that I've thought for three seconds in a row,
like clearly the right thing.  This format is still correctly parsed by JS and
NSPR code.

Phil, the tests undef ecma_3/Date for 15.9.5.4 and 15.9.5.7 now fail, but they
were depending on the particular implementation-specific (per ECMA-262 Ed 3)
format of Date.prototype.toString's result.  Can you tweak them, or otherwise
make them more general (so they could be used to test any conforming ECMA-262
implementation, not just SpiderMonkey's)?  Also, does Rhino need a bug like this
one?  Thanks.

/be

Comment 31

[Phil Schwartau]

Have filed bug 118636 against Rhino for this issue -

Comment 32

[Christopher Blizzard (:blizzard)]

Yeah, ifilm works now.  Cool.

Comment 33

[Phil Schwartau]

As Brendan pointed out in Comment #30, the following testcases needed
to be repaired to take the new date format into account:

           mozilla/js/tests/ecma_3/Date/15.9.5.4
           mozilla/js/tests/ecma_3/Date/15.9.5.7

This has now been done. Both tests are passing again in the debug
and optimized JS shells on WinNT and Linux. On Mac9.1, only the
first test passes. This is to be expected; there is an outstanding
date bug 61184 on Mac which always causes the second testcase to fail -