empty cookie path treated incorrectly

VERIFIED FIXED in mozilla1.0.1

Status

()

P3
normal
VERIFIED FIXED
18 years ago
16 years ago

People

(Reporter: st.n, Assigned: morse)

Tracking

Trunk
mozilla1.0.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need info][fixed-trunk] [Needs refresh of a=])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

18 years ago
I would expect that a cookie without a path would be treated like a cookie with
the path '/' (coming from Set-Cookie: ...; path=/). But Mozilla stores these
as two different cookies, which can be verified by opening the cookie manager.
Additionally, Mozilla sends this cookie twice to the server (like this:
Cookie: max=50; max=200), where the second one is the one with the path '/'.

And what's more annoying: it is impossible to remove the cookie without a path
with a Set-Cookie:-request, at least not with an expiration time in the past and
without a specified path. Unfortunately, IE handles all this like I would
expect. :-(

I could provide a PHP script as a test case, if that would help. The essential
part is that I used to set a cookie with

	setcookie( 'max', $max, time() + 90*24*60*60 );

and now that I included a path in a newer version like this:

	setcookie( 'max', $max, time() + 90*24*60*60, '/' );

I would like to remove the old cookie like this:

	setcookie( 'max', '', 1 );

You can find the description of the setcookie function at
http://php.net/setcookie .

I'm using nightly build 2000112620 in NT4 at the moment, but I don't think that
matters, so I'm setting Platform and OS to all.
(Assignee)

Comment 1

18 years ago
A complete testcase (html file containing javascript set-cookies) would be 
helpful here.

How does 4.x treat this?
(Reporter)

Comment 2

18 years ago
Unfortunately I'm not very experienced with Javascript yet, so I can't write a
test case before next week. Could someone give my some hints on how to do this,
or links to documentation? As I said, I wrote this in PHP.

And I haven't tried it with Nav4.x yet, but I'm also going to report here as
soon as I do. But it's Friday evening here in Germany already, so again: next
week. :-)

Comment 3

18 years ago
Will make testcase a.s.a.p.
Friendly, HJ.
Netscape Nav triage team: this is not a Netscape beta stopper.
Keywords: nsbeta1-
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2

Comment 5

17 years ago
OK, I ran into the same problem and here are more details on this bug.

According to the spec at 
http://www.netscape.com/newsref/std/cookie_spec.html

"If the path is not specified, it as assumed to be the same path as the document
being described by the header which contains the cookie."

The questions is: What is a "path" in this context?

The script that i was accessing had an URL like
http://www.example.com/332/login.php?target=http://www.example.info/idx_menu.php&username=a&password=b

This script's output contained a set-cookie header without a path option.
The path of the cookie was set to

path=/332/login.php?target=http://www.example.info

I believe it should have been set to

path=/332/login.php

This would be in accordance with what javascript thinks is a path of a URL
(location.path).
It seems that the code that determines the path looks for the last slash in the
URL, but it should stop at the first question mark it encounters.

This bug is somewhat complicated to demonstrate with a javascript testcase since
it depends on the URL of the page that is served.

If necessary, I can however provide a test setup that you can access remotely
(with a browser) that shows this bug.

For reference:

It seems both Netscape 4.x and MS IE 6 don't put the query string (the part
after the "?" including the "?" itself) into the cookie's path.

Comment 6

17 years ago
More info:

It seems the RFC 2109 regarding cookies is more recent than Netscape's
specification.
It states at
http://www.cis.ohio-state.edu/cgi-bin/rfc/rfc2109.html#sec-4.3.1

 Path     Defaults to the path of the request URL that generated the
          Set-Cookie response, up to, but not including, the
          right-most /.


This is exactly what mozilla does at the moment, but I still think it's a bug
because the RFC authors didn't consider the query string (the part following the
"?") and the "/"'s it might contain.
I am CC'ing them, maybe they can confirm my theory?

Once again:

for the URL

http://example.com/dir/script?param1=abc&param2=de/fg&param3=gh
I think the path should default to
http://example.com/dir
and not to
http://example.com/dir/script?param1=abc&param2=de

David and Lou, please comment.
(Assignee)

Comment 7

17 years ago
If there was a slash in the query string, it would probably need to be escaped.

Comment 8

17 years ago
Created attachment 74330 [details] [diff] [review]
Use path up to first "?", then try last "/"

This patch is to the 0.9.9 source. I couldn't test it because I don't have a
build environment ready.
It should strip down everything after the first question mark to get the path.
If there is no question mark, the old behaviour is used.

Comment 9

17 years ago
Well, for one the "slash" character is considered a reserved character in the 
query string and needs to be escaped.  THerefore, it should NEVER appear 
unescaped in the query string.  


See W3C doc - http://www.w3.org/Addressing/URL/uri-spec.html
Section: BNF of generic URI syntax


reserved 
= | ; | / | # | ? | : | space 


Therefore the spec is correct and the URL that fails does so because it is an 
inproperly formed URL (and should fail).


Comment 10

17 years ago
I'm not sure if it needs to be escaped. Does RFC 1630 page 7 "QUERY STRING" or
RFC 2396 section 3.4 apply?
RFC 2396 does not claim to supersede or update RFC 1630.

A page with an unescaped "/" in the query string passes through the "HTML 4.01
strict" validator at the W3C.

There are lots of pages that don't encode "&" (as "&") and "/" properly in
the query string. It would be good of mozilla would tolerate such errors in this
simple case.

Comment 11

17 years ago
Email to both authors of RFC 2109 bounces.

If an unescaped "/" is illegal in the query part of the URL, i think mozilla
should ignore it in its attempt to determine the path. This is what my patch does.

Comment 12

17 years ago
There is an issue with my patch.

For the URL
http://example.org/a/b?c
the path will be
http://example.org/a/b
but it should be
http://example.org/a

Could someone come up with a better patch?
It should find the last slash in the URL, but not past the first question mark.

Comment 13

17 years ago
Why not just use this algorithm:

1. Parse the URL on the "?" into an object (like an array)
2. Take the first element of the object, get the position of the last instance 
of the "/" character
3. Using that position, get the substring from 0 --> position
4. There's your path :)

Comment 14

17 years ago
Created attachment 74346 [details] [diff] [review]
Find last "/" but not past first "?"

This one works like before with query strings that don't have "/" in them.

It obsoletes my attachment 74330 [details] [diff] [review] (i'm not authorized to check the box.. bleh).

Comment 15

17 years ago
Comment on attachment 74330 [details] [diff] [review]
Use path up to first "?", then try last "/"

buggy. obsoleted by attachment 74346 [details] [diff] [review]
Attachment #74330 - Attachment is obsolete: true
(Assignee)

Comment 16

17 years ago
I'm not convinced there is a problem here.  If the query string contains an 
unescaped /, then the query string is incorrectly formed (comment 7 and comment 
9) and we have an evangelism issue.  However if IE is tolerant enough to accept 
this malformed query, then perhaps we should be too.

That said, the patch that you are proposing is indeed the correct way to handle 
this.  My only criticism is the use of the variable name "slash" since you are 
using it when you look for the questionmark too.  Change that to a more generic 
name, such as pathEndChar or something equivalent.

With that change, r=morse.  Now you need to get yourself an sr.
(Assignee)

Comment 17

17 years ago
*** Bug 121355 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 18

17 years ago
OK, there are starting to be dups on this one, and I'm afraid there are going to 
be many sites that will fail because of this problem.  Even though it's an 
evangelism issue for those sites, we are the ones that are going to get the 
blame.

Therefore I'm going to recommend that this one be considered for nsbeta1.  
Especially since we already have a patch for it and it's probably of low risk.
Keywords: nsbeta1
Target Milestone: mozilla1.2alpha → mozilla1.0

Comment 19

17 years ago
Nav triage team needs info: What is the real impact here? Does this affect any
top sites?
Whiteboard: [need info]
(Assignee)

Comment 20

17 years ago
I have no idea whether any top sites are affected.  We won't know the answer to 
that until after we release the product and start getting complaints.  This is a 
case where we can avoid the problem completely with a relatively safe fix that 
already exists (thanks to a web contributor).  So why should we not use it?

Comment 21

17 years ago
Impact in bug 121355: Unable to access paid content on a site where verification
uses cookies.

Even though the underlying problem lies with the site, this works in Netscape
3,4.x,6.0-6.2, Mozilla <0.9.6, IE 4-6, Opera 5-6, and Konqueror, so users that
run into a similar problem will very likely blame Mozilla. 

Evangelism is of course an option, but the problem with the respective sites can
be rather difficult to diagnose. Infact, I tried to look into the source-code of
the non-working site, but the incriminated unescaped slash is hidden in a
variable that is set by a php-script and normally not seen by the user. Most
people lack morse's skills and dedication, so often we won't even know we should
evangelize the affected sites. Much more probable, the user will just turn away
from Mozilla/Netscape (especially if the browser does not show "my" *paid* content).

Comment 22

17 years ago
sr=jag provided Morse's comment about the variable name is addressed. A common
name for such variables is |iter|.
(Assignee)

Updated

17 years ago
Keywords: adt1.0.0

Comment 23

17 years ago
Comment on attachment 74346 [details] [diff] [review]
Find last "/" but not past first "?"

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #74346 - Flags: approval+
(Assignee)

Comment 24

17 years ago
Checked in on trunk.  Can't check in on branch due to lack of adt approval.

Comment 25

17 years ago
Created attachment 79405 [details] [diff] [review]
Same as attachment 74346 [details] [diff] [review], but with better variable name

Here's the patch again with the requested name change. Thanks for considering
it for 1.0.
Attachment #74346 - Attachment is obsolete: true
(Assignee)

Comment 26

17 years ago
Sven, thanks for the updated patch.  It's exactly what I've already checked in 
on the trunk before you made your latest posting (I had made those same changes 
based on my comments and jag's).

Comment 27

17 years ago
Just curious - is there a "contributers" file or something I get added to for
fixing this problem? :-)

Comment 28

17 years ago
Thank you Steve, Sven! This fixed the problem in bug 121355. Hopefully we will
get  approval for the branch before release. 

Comment 29

17 years ago
tever, please test the bug fix on the trunk and add your comments here.  The adt 
needs to know before the fix is checked into the branch. thanks.

Comment 30

17 years ago
Nav triage team: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 31

17 years ago
Samir, what's the reason for the nsbeta-? Please check morse's comment 18 as
well as comment 21. 
Since the patch is checked into the trunk (and fixes the problem with the site),
should this be renominated nsbeta after a few days of baking?

Comment 32

17 years ago
adt1.0.0- on behalf of the adt. Not needed for MachV.

Updated

17 years ago
Keywords: adt1.0.0 → adt1.0.0-

Updated

17 years ago
Whiteboard: [need info] → [need info][fixed-trunk]

Comment 33

17 years ago
This bug is still present in 1.0rc2 it seems. Does this mean it won't make it
into 1.0? I can't access a site with mozilla because of this. It works fine in
all other major browsers.
(Assignee)

Comment 34

17 years ago
Sven, see comments 24, 30, and 32.  Unless it gets adt1.0.0+ it can't go into 
the branch.
(Assignee)

Comment 35

17 years ago
*** Bug 144148 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 36

17 years ago
removing minus to get back on ADT radar.
Keywords: adt1.0.0- → adt1.0.0
(Assignee)

Comment 37

17 years ago
Being marked fixed since it is checked in on trunk, even though it is still not 
fixed on the branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Keywords: nsbeta1- → nsbeta1
Resolution: --- → FIXED
(Assignee)

Updated

17 years ago
Keywords: mozilla1.0

Comment 38

17 years ago
Please get an nsbeta1+ from the nav-triage team before seeking ADT approval.

Comment 39

17 years ago
nsbeta1+ per Nav triage team, since minimal Nav team work is required.
Keywords: nsbeta1 → nsbeta1+

Comment 40

17 years ago
adt1.0.0+ (on ADT's behalf) for approval to checkin to the 1.0 branch, pending
Driver's re-approval (e.g. a= is older than 3 days).  After, checking in, please
add the fixed1.0 keyword.
Whiteboard: [need info][fixed-trunk] → [need info][fixed-trunk] [Needs refresh of a=]

Updated

17 years ago
Keywords: adt1.0.0 → adt1.0.0+
(Assignee)

Updated

17 years ago
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 41

17 years ago
changing to adt1.0.1+ for adt approval for checkin to the 1.01 milestone. 
Please get drivers approval before checking in.
Keywords: adt1.0.0+ → adt1.0.1+
Comment on attachment 79405 [details] [diff] [review]
Same as attachment 74346 [details] [diff] [review], but with better variable name

re-approval for 1.0.1 branch.

please check into the 1.0.1 branch ASAP. once landed remove the
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword
Attachment #79405 - Flags: superreview+
Attachment #79405 - Flags: review+
Attachment #79405 - Flags: approval+

Updated

17 years ago
Keywords: mozilla1.0 → mozilla1.0.1+
(Assignee)

Updated

17 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1
(Assignee)

Comment 43

17 years ago
See bug 135932 which is a consequence of this bug.
(Assignee)

Comment 44

17 years ago
*** Bug 150953 has been marked as a duplicate of this bug. ***

Comment 45

17 years ago
I realize I'm wandering into this discussion rather late, and it's good to know
that this bug is being fixed.  I think that this is a parsing issue more than
anything--in this case, how to parse certain invalid URLs.

<a href="http://www.w3.org/Addressing/rfc1808.txt">RFC 1808</a> has some good
guidelines on how to parse URLs.  Obviously, mozilla itself doesn't mistake a
URL of the form "http://foo.com/bar?moby/quux" as referring to the path
"/bar?moby", because then it wouldn't be parsing the query correctly.

Whether or not the query is invalid or contains a '/' is really a separate
issue--the path for the cookie is only supposed to concern the URL's path, not
its query.  Sure, the query is incorrectly formed, but that shouldn't be
relevant to the path at all, any more than an incorrectly formed hostname or
fragment would be.

And if these URLs really are invalid, why doesn't Mozilla give me an error?  My
guess would be because its URL parsing works something like the method specified
in RFC 1808, and therefore is rather forgiving in what it accepts.  The problem
here is that Mozilla silently sets the wrong path for the cookie; a warning or
an error would have been much easier to diagnose.  :)

But it's great to hear that this has been fixed; I just wish it could have made
it into Netscape 7.0PR1, since people are starting to use it now.  I'll download
a nightly build of Mozilla and test it again...

Comment 46

17 years ago
verified1.0.1
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1

Comment 47

16 years ago
verified galeon 1.2.6 + Mozilla 1.1
You need to log in before you can comment on or make changes to this bug.