Closed
Bug 118266
Opened 23 years ago
Closed 23 years ago
JS Date type mixed with document.cookie considered harmful
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla0.9.8
People
(Reporter: blizzard, Assigned: brendan)
References
()
Details
(Keywords: js1.5)
Attachments
(3 files, 1 obsolete file)
371 bytes,
text/html
|
Details | |
463 bytes,
text/html
|
Details | |
1.40 KB,
patch
|
khanson
:
review+
shaver
:
superreview+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Test page that shows what the date string comes out as.
Reporter | ||
Comment 2•23 years ago
|
||
IE 5.5 uses a format like so: Sat Jan 4 20:28:40 EST 2003
Reporter | ||
Comment 3•23 years ago
|
||
Netscape 4.79 (Windows) looks like this: Sat Jan 04 20:36:15 GMT-0500 (Eastern Standard Time) 2003
Reporter | ||
Comment 4•23 years ago
|
||
Netscape 4.78 (Linux) looks like this: Sat Jan 04 20:27:51 GMT-0500 (EST) 2003
Reporter | ||
Comment 5•23 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•23 years ago
|
||
Mozilla 0.9.7 on win32: Sat Jan 04 20:52:53 GMT-0500 (Eastern Standard Time) 2003
Comment 7•23 years ago
|
||
this is somewhat related to the bug 116598 comment # 3 by johnny.
Comment 8•23 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•23 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•23 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
Comment 12•23 years ago
|
||
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•23 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•23 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•23 years ago
|
||
...or teach PR_ParseTimeString() to treat everything in parens as whitespace.
Reporter | ||
Comment 16•23 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.
Assignee | ||
Comment 18•23 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•23 years ago
|
||
Reporter | ||
Comment 20•23 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•23 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•23 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•23 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•23 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•23 years ago
|
Status: NEW → ASSIGNED
Keywords: js1.5,
mozilla0.9.8
Priority: -- → P1
Target Milestone: --- → mozilla0.9.8
Assignee | ||
Comment 25•23 years ago
|
||
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+
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•23 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•23 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
Closed: 23 years ago
Resolution: --- → FIXED
Comment 31•23 years ago
|
||
Have filed bug 118636 against Rhino for this issue -
Comment 33•23 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•19 years ago
|
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•