JS Date type mixed with document.cookie considered harmful

VERIFIED FIXED in mozilla0.9.8

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
16 years ago
12 years ago

People

(Reporter: blizzard, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla0.9.8
js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 63608 [details]
test page

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

Comment 2

16 years ago
IE 5.5 uses a format like so:

Sat Jan 4 20:28:40 EST 2003
(Reporter)

Comment 3

16 years ago
Netscape 4.79 (Windows) looks like this:

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

Comment 4

16 years ago
Netscape 4.78 (Linux) looks like this:

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

Comment 5

16 years ago
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?
(Reporter)

Comment 6

16 years ago
Mozilla 0.9.7 on win32:

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

Comment 7

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

Comment 8

16 years ago
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.
(Reporter)

Comment 10

16 years ago
unchanging summary since that's not the issue - we might still yet change the
parser.
Summary: date format from GMT to localtime zone → JS Date type mixed with document.cookie considered harmful
(Reporter)

Comment 11

16 years ago
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

Comment 13

16 years ago
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

16 years ago
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

16 years ago
...or teach PR_ParseTimeString() to treat everything in parens as whitespace.
(Reporter)

Comment 16

16 years ago
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

16 years ago
Reassigning to Kenton - 
Assignee: rogerl → khanson
(Assignee)

Comment 18

16 years ago
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
(Reporter)

Comment 19

16 years ago
Created attachment 63707 [details]
test that sets a cookie as well
(Reporter)

Comment 20

16 years ago
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

16 years ago
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.
(Assignee)

Comment 22

16 years ago
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
(Reporter)

Comment 23

16 years ago
If you follow PR_ParseTimeString it ends up with the year 500 or something
similar.  It's not parsing it correctly at all.
(Assignee)

Comment 24

16 years ago
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
Assignee: khanson → brendan
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla0.9.8
Priority: -- → P1
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 25

16 years ago
Created attachment 63778 [details] [diff] [review]
proposed fix

Looking for fast review for 0.9.8.

/be
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?
Attachment #63778 - Flags: superreview+
(Assignee)

Comment 27

16 years ago
Created attachment 63779 [details] [diff] [review]
oops, avoid trailing space if no (PST) or whatever

Review this!
Attachment #63778 - Attachment is obsolete: true
Comment on attachment 63779 [details] [diff] [review]
oops, avoid trailing space if no (PST) or whatever

sr=shaver.

You're so picky!
Attachment #63779 - Flags: superreview+

Comment 29

16 years ago
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+
(Assignee)

Comment 30

16 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 31

16 years ago
Have filed bug 118636 against Rhino for this issue -
(Reporter)

Comment 32

16 years ago
Yeah, ifilm works now.  Cool.
Status: RESOLVED → VERIFIED

Comment 33

16 years ago
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 -

Updated

12 years ago
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.