120959 - Use of ".." in address causes links to relative URL's to fail

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Matt Roper]

I find that if an address containing ".." is entered in the URL bar, links to
relative URL's on the target page can not be followed properly.

To reproduce:
* Go to http://www.mozillazine.org/build_comments/
* Add ".." to the end of the URL and press ENTER
* hover mouse over one of the "## responses" links on the page and note the URL
that Mozilla displays in status bar (it's missing a couple of characters)
* try to click on the link

I've had the same results trying this on other sites as well.

Comment 1

[timeless]

wfm nc4.78w32

Comment 2

[Boris Zbarsky [:bzbarsky]]

Interesting....

Load "http://www.mozillazine.org/build_comments/..".  The URL bar says
"http://www.mozillazine.org/" but the base url for the document is
"http://www.mozillazine.or./"  Note "." instead of "g".

Confirming, linux build 2002-01-18-08.

Comment 3

[Andreas Otte]

Testing:

urltest http://www.mozillazine.org/build_comments/..

gives this result:

http,,,www.mozillazine.org,-1,,g,,,,,http://www.mozillazine.org/

should be:

http,,,www.mozillazine.org,-1,/,,,,,,http://www.mozillazine.org/


This is not related to CoalesceDirs, checked it, must be somewhere else in the
parser ...

Comment 4

[Andreas Otte]

The problem can be found in 

nsBaseURLParser::ParseFilePath(const char *filepath, PRInt32 filepathLen,
                               PRUint32 *directoryPos, PRInt32 *directoryLen,
                               PRUint32 *basenamePos, PRInt32 *basenameLen,
                               PRUint32 *extensionPos, PRInt32 *extensionLen)

where we search backwards for a filename, in this case getting .. as filename.
With the old urlparser we did a CoalesceDirs before doing something similar. We
should put that back in ...

Comment 5

[Andreas Otte]

raising severity ... should be fixed for 0.9.8

Comment 6

[benc]

-> Networking, +helpwanted
andreas, do you want to take this?

Comment 7

[Andreas Otte]

I will give it a shot, on second thought, I would go for special casing .. and .
not doing a call to CoalesceDirs.

taking ...

Comment 8

[Andreas Otte]

cc-ing darin, who wrote that stuff, for input on a better idea. 

Comment 9

[Andreas Otte]

Attachment: patch to fix the problem

catch /.. and /. when searching for filename

Comment 10

[Andreas Otte]

Anyone want to give it an r=?

Comment 11

[Andreas Otte]

nominating for 0.9.8

Comment 12

[Darin Fisher]

andreas: your patch looks good to me.  sr=darin

Comment 13

[Andreas Otte]

Doug, want to r= this patch?

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 65909 [details] [diff] [review]
patch to fix the problem

looks good.  Incredible work again.

Comment 15

[Andreas Otte]

fix checked in

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 65909 [details] [diff] [review]
patch to fix the problem

>Index: mozilla/netwerk/base/src/nsURLParsers.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/src/nsURLParsers.cpp,v
>retrieving revision 1.7
>diff -u -r1.7 nsURLParsers.cpp
>--- mozilla/netwerk/base/src/nsURLParsers.cpp	29 Nov 2001 23:28:34 -0000	1.7
>+++ mozilla/netwerk/base/src/nsURLParsers.cpp	21 Jan 2002 19:39:29 -0000
>@@ -325,6 +325,10 @@
>     for (p = end - 1; *p != '/' && p > filepath; --p)
>         ;
>     if (*p == '/') {
>+        // catch /.. and /.
>+        if ((p+1 < end && *(p+1) == '.') && 
>+           (p+2 == end || (p+2 < end && *(p+2) == '.' && p+3 == end)))

Nit: if (p+1 < end) then (p+2 <= end), so if !(p+2 == end) [which must be true
for the || operator's right operand to be evaluated), then (p+2 < end) -- so
there is no need to test (p+2 < end) in the first clause of the final
conjunction.

>+            p = end - 1;
>         // filepath = <directory><filename>.<extension>
>         SET_RESULT(directory, 0, p - filepath + 1);
>         ParseFileName(p + 1, end - (p + 1),

Comment 17

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.8

Comment 18

[Andreas Otte]

Attachment: patch to the patch based on brendan's comment

Brendan is right, the additional clause is not necessary.

Comment 19

[Asa Dotzler [:asa]]

Andreas, can you check this into the 0.9.8 branch. It looks like it landed on
the trunk after we branched and never made it to the branch. Thanks.

Comment 20

[Darin Fisher]

asa: i'm checking it right now :-)

Comment 21

[Darin Fisher]

fixed on the 0.9.8 branch

andreas: can you please land your be-revision on the trunk?  thx!

Comment 22

[Andreas Otte]

Sure, I assume I have a r/sr= on this one?

Comment 23

[Darin Fisher]

r/sr=brendan,darin

Comment 24

[Andreas Otte]

revised patch checked in