Closed
Bug 119071
Opened 23 years ago
Closed 23 years ago
Can not ftp files from / on Cobalt web server
Categories
(Core Graveyard :: Networking: FTP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla0.9.9
People
(Reporter: mozilla, Assigned: andreas.otte)
References
Details
Attachments
(2 files)
2.46 KB,
patch
|
bbaetz
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
5.64 KB,
patch
|
bbaetz
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.7+) Gecko/20020108 BuildID: 2002010803 My web site is located on a Cobalt rackmounted server. To use ftp to access the log files I have to use the following instructions; ****************** Site Administrator FTP instructions ****************** This session begins in your user home directory at /users/dmccormack Your personal web directory is /users/dmccormack/web, accessible at the URL http:///~dmccormack/ As a site administrator, you can access the site home directory at / The site web directory is /web, accessible at the URL http:/// To upload to your personal web directory, type: "cd web", then "put ", where is the full file name of the document on your local computer. To upload to the site web directory, type: "cd /web", then "put " where is the full file name of the document on your local computer. ************************************************************************* Mozilla does not allow me to move higher than the initial starting folder though. Reproducible: Always Steps to Reproduce: Entering the address ftp://dmccormack:<password>@ftp.zamang.co.uk/../../logs in the address text box. Actual Results: I get the error message '/users/dmccormack/logs: No such file or directory'. Expected Results: I would have expected this to login (once I've entered the correct password), to have the current folder as /users/dmccormack then cd ../ twice up to /. It should then list the main folders in the root folder. It appears that Mozilla is ignoring the ../../ and preventing me from using Mozilla to access any of the admistrator files. I've tried this address in MSIE 6 and it is accepted. Though it might be broken behaviour on IE's part.
Comment 1•23 years ago
|
||
andreas - is this related to yuor fix for bug 84242?
Assignee: bbaetz → andreas.otte
Assignee | ||
Comment 2•23 years ago
|
||
Hm ... I don't think so. It's more a problem that we normalize the url while parsing it and part of that is to get rid of ../.. and similar stuff. One special thing: We don't go above the starting point, anything doing that will get dropped and that seems to happen here. What we do is okay for http, but maybe wrong for ftp. David: What url is displayed in the adress bar when you start with ftp://dmccormack:<password>@ftp.zamang.co.uk/../../logs after the error message ?
Reporter | ||
Comment 3•23 years ago
|
||
Andreas: After the error message is displayed the address stays the same as entered, 'ftp://dmccormack:<password>@ftp.zamang.co.uk/../../logs'.
Assignee | ||
Comment 4•23 years ago
|
||
Okay, I was able to reproduce it myself. The url in the adressbar seems not to be updated. I'm now pretty sure the problem is as I described above. The problem really seems to be that we normalize absolute as well as relative urls. For relative urls RFC 2396 says: If the resulting buffer string still begins with one or more complete path segments of "..", then the reference is considered to be in error. Implementations may handle this error by retaining these components in the resolved path (i.e., treating them as part of the final URI), by removing them from the resolved path (i.e., discarding relative levels above the root), or by avoiding traversal of the reference. and also: Parsers must be careful in handling the case where there are more relative path ".." segments than there are hierarchical levels in the base URI's path. Note that the ".." syntax cannot be used to change the authority component of a URI. ... In practice, some implementations strip leading relative symbolic elements (".", "..") after applying a relative URI calculation, based on the theory that compensating for obvious author errors is better than allowing the request to fail. Thus, the above two references will be interpreted as "http://a/g" by some implementations. Mozilla discards relative levels above the root. However we may be in error to also subject absolute URLs to this kind of normalisation. I could not find anything in RFC 2396 that says that absolute urls are also subject to this part of the algorithm to resolve relative urls. ccing darin on this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•23 years ago
|
||
David: as a quick workaround you can probably set up a soft link to / in your home dir... that way you can still use mozilla to access files outside your home directory. andreas: seems to me that "ftp://user:pass@foo.com/../../bar" should be valid. only the FTP server knows otherwise. our coalescing logic needs to be smarter to not collapse ../'s when it would change the resource being referenced. a change like this would probably be ok for http, though i can't imagine it being useful in practice. another thing is to consider security issues... are there any we need to consider here? nothing comes to mind, but we should be careful.
Assignee | ||
Comment 6•23 years ago
|
||
A security problem with http might arise when the webserver would allow ../../ GETs to move to a level above the root of the webserver. But that would be a bug with the webserver ...
Assignee | ||
Comment 7•23 years ago
|
||
This patch to CoalesceDirs does not drop to much /.. instead it leaves them in the path. Darin could you take a look? This works now with the travesal variable which counts the hierachies.
Comment 8•23 years ago
|
||
Comment on attachment 64667 [details] [diff] [review] patch to fix the problem sr=darin
Attachment #64667 -
Flags: superreview+
Assignee | ||
Comment 9•23 years ago
|
||
Bradley, want to give a r=, already have sr= from darin
Comment 10•23 years ago
|
||
Comment on attachment 64667 [details] [diff] [review] patch to fix the problem This looks fine. PRUint32 instead of int, and use --traversal intsead of traversal--, (and ++foo intead of foo += 1). r=bbaetz if you've tested.
Attachment #64667 -
Flags: review+
Assignee | ||
Comment 11•23 years ago
|
||
incorporated bbaetz suggestions, tested again with standard urltest tests and some special cases. fix checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I think the fix for this bug broke the relative links (the dates) on the right side of http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey . I'd be surprised if it didn't break other sites as well. Could it be limited to FTP? What do the URI and URL RFCs say?
...or, now that I read the bug more carefully, perhaps it could be limited to absolute URLs only?
Comment 14•23 years ago
|
||
ok, i backed out the patch. we need to figure out how relative paths are getting into CoallesceDirs and determine if they either need to be stopped from doing so, or if CoallesceDirs needs to also know how to deal with them :-(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•23 years ago
|
||
Hmmm ... sorry I had to sleep for three hours ... it should be okay for both absolute and relative urls. The problems seems to be if the absolute url does not end with an / but the last part of the path is *meant* as part of the directory (but parsed as filname instead). Did not catch this case with urltest and also not with the other tests I did and I couldn't because it's correct what happens. My guess is we had this problem in the old version of CoalesceDirs too but dropping the ../ saved it. I could make tinderbox a case for tech evangelism. ;-)
Assignee | ||
Comment 16•23 years ago
|
||
Further looking into this ... it's not exactly what I thought first, the html generated by the tinderbox script seems to be bogus, please correct me if I'm wrong here. The base-url is: http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey The directory part is /. The generated html contains then: <tt> <a HREF='showlog.cgi?log=SeaMonkey/1011167160.20268.gz' onclick="return log(event,0,'1011167160.20268.gz');">L</a> <a href=../bonsai/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1011165060&maxdate=1011167159>C</a></tt> or <td> <a href='../registry/who.cgi?email=law%25netscape.com&d=MozillaTinderboxAll|HEAD|/cvsroot|1011162660|1011162840' onclick="return who(event);">law</a> </td> Both hrefs are not correct since they reference stuff above the root level of the http server. Simply dropping the /.. in CoalesceDirs fixed the html in this page only by accident. If nobody objects I will file a tech evangelism bug against mozilla.org shortly, I always wanted to do that ... ;-)
Comment 17•23 years ago
|
||
showbuilds.cgi?tree=SeaMonkey is the same as http://tinderbox.mozilla.org/Seamonkey/, I think, or something similar. Doesn't the HTTP spec say something about these forms of urls?
Assignee | ||
Comment 18•23 years ago
|
||
That looks very much like the first URL I saw at home: http://tinderbox.mozilla.org/Seamonkey Note, it is without the / at the end, so my first comment after the backout would apply with the same result: wrong HTML. See comment 4 for info what the spec says. We simply changed from dropping to leaving.
Assignee | ||
Comment 19•23 years ago
|
||
The problem seems to be that there are different access paths to tinderbox information: http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey and http://tinderbox.mozilla.org/SeaMonkey/ The second one would work fine with the patch since it puts the base url at the right level. The first one seems to be the problem, the generated relative urls do not fit the base url. Previously we silently ignored this error ...
But wouldn't you expect that some other sites have the same problem, since it seems that all browsers handle such URLs the same way?
Assignee | ||
Comment 21•23 years ago
|
||
I don't expect it to see this in very much cases, since this clearly is a very visible error, base url and relative urls simply do not fit together. All this sites are cases for tech evangelism. I would like to do the following: 1. Get tinderbox fixed 2. Check in the patch again shortly after the 0.9.8 branch 3. See which (if any!) sites else have problems. What we do is clearly within the bounderies of RFC 2396: If the resulting buffer string still begins with one or more complete path segments of "..", then the reference is considered to be in error. Implementations may handle this error by retaining these components in the resolved path (i.e., treating them as part of the final URI), by removing them from the resolved path (i.e., discarding relative levels above the root), or by avoiding traversal of the reference.
Assignee | ||
Comment 22•23 years ago
|
||
I looked again and both versions are off by one level ... time to file another bug.
Assignee | ||
Comment 23•23 years ago
|
||
As a quick solution we could add CoalesceDirsAbs to nsURLHelper which works like the CoalesceDirs in the patch. We would call that version while normalizing absoulte urls and call the original CoalesceDirs while resolving relative urls.
Assignee | ||
Comment 24•23 years ago
|
||
Comment 25•23 years ago
|
||
i agree... this sounds like a good plan to me. i do wish the code could be shared a bit more, but perhaps that would just result in something less efficient?? andreas: your comment is that this might be a good quick solution... what do you have in mind long term? sr=darin
Updated•23 years ago
|
Attachment #65454 -
Flags: superreview+
Assignee | ||
Comment 26•23 years ago
|
||
The right solution in my opinion is of course the first patch. There is nothing wrong with it, it just found a big bug in the pages generated by tinderbox, because we are no longer so forgiving about ill formed relative urls. The second patch is only forgiving when doing resolutions of relative urls. My intention of course is to get in the first patch *after* tinderbox is fixed.
Assignee | ||
Comment 27•23 years ago
|
||
bbaetz, want to r= again?
Comment 29•23 years ago
|
||
andreas: actually, if tinderbox is/was abusing this then i suspect other web sites probably do as well. so, i'm not sure it is worth it to try to land the original patch. what is wrong with the latest patch?
Assignee | ||
Comment 30•23 years ago
|
||
There is nothing wrong with it. I just think it is doing uri-fixup (by accident) that it should not do. Embedded links in a html page (as opposed to something the user types into the urlbar) should follow a certain minimal standard, and such (relative) urls are simply wrong. Doing this kind of "fixup" violates in my opinion all that mozilla stands for.
Assignee | ||
Comment 31•23 years ago
|
||
Doug, want to r= the second patch?
Comment 32•23 years ago
|
||
well, try this argument: ~$ cd /home /home$ cd / /$ cd ../../../../home /home$ cd /../../../home /home$ The filesystem does it, so why shouldn't uris? Anyway, the second patch is OK, I think. I'm a bit concerned about the #ifdef XP_WIN stuff, though - does that handle all cases? I don't know anything about windows charset handling.
Assignee | ||
Comment 33•23 years ago
|
||
I did not change the #ifdef XP_WIN stuff. So it should be okay if it is okay in the old CoalesceDirs. RFC 2396 clearly states that such an URI is wrong. Allowing it (and silently modifying it to something that *may be* correct) would just promote lazyness. I simply don't like it. For now I will go with the second patch and if it is checked in I will close this bug, but I will open a new one on the issue, ask around in the netlib newsgroup, ....
Comment 34•23 years ago
|
||
let bradley review the second since he reviewed the first.
Comment 35•23 years ago
|
||
Comment on attachment 65454 [details] [diff] [review] another patch as described above, working around the wrong urls in tinderbox OK. r=bbaetz
Attachment #65454 -
Flags: review+
Assignee | ||
Comment 36•23 years ago
|
||
fix checked in
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Updated•3 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•