Closed Bug 118266 Opened 23 years ago Closed 23 years ago

JS Date type mixed with document.cookie considered harmful


(Core :: JavaScript Engine, defect, P1)






(Reporter: blizzard, Assigned: brendan)




(Keywords: js1.5)


(3 files, 1 obsolete file)

Build is from Jan 04, 2002.

I was browsing around 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.
Attached file test page
Test page that shows what the date string comes out as.
IE 5.5 uses a format like so:

Sat Jan 4 20:28:40 EST 2003
Netscape 4.79 (Windows) looks like this:

Sat Jan 04 20:36:15 GMT-0500 (Eastern Standard Time) 2003
Netscape 4.78 (Linux) looks like this:

Sat Jan 04 20:27:51 GMT-0500 (EST) 2003
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?
Mozilla 0.9.7 on win32:

Sat Jan 04 20:52:53 GMT-0500 (Eastern Standard Time) 2003
this is somewhat related to the bug 116598 comment # 3 by johnny.
changing summary accordingly
Summary: JS Date type mixed with document.cookie considered harmful → date format from GMT to localtime zone
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.
unchanging summary since that's not the issue - we might still yet change the
Summary: date format from GMT to localtime zone → JS Date type mixed with document.cookie considered harmful
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
Not a DOM HTML bug, Date is a JS engine type, reassigning.
Assignee: jst → rogerl
Component: DOM HTML → Javascript Engine
OS: Linux → All
QA Contact: stummala → pschwartau
Hardware: PC → All
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?
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*.
...or teach PR_ParseTimeString() to treat everything in parens as whitespace.
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.
Reassigning to Kenton - 
Assignee: rogerl → khanson
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.

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

If you follow PR_ParseTimeString it ends up with the year 500 or something
similar.  It's not parsing it correctly at all.
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.

Assignee: khanson → brendan
Keywords: js1.5, mozilla0.9.8
Priority: -- → P1
Target Milestone: --- → mozilla0.9.8
Attached patch proposed fix (obsolete) — Splinter Review
Looking for fast review for 0.9.8.

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


Can we get some folks to test this on the other platforms, against 
blizzard's test cases and ifilm?
Attachment #63778 - Flags: superreview+
Review this!
Attachment #63778 - Attachment is obsolete: true
Comment on attachment 63779 [details] [diff] [review]
oops, avoid trailing space if no (PST) or whatever


You're so picky!
Attachment #63779 - Flags: superreview+
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)
Attachment #63779 - Flags: review+
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 and 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.

Closed: 23 years ago
Resolution: --- → FIXED
Have filed bug 118636 against Rhino for this issue -
Yeah, ifilm works now.  Cool.
As Brendan pointed out in Comment #30, the following testcases needed
to be repaired to take the new date format into account:


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 -
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.