214405 - Unable to ftp virtual folder

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Lee]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030717

ftp via mozilla is OK when the virtual directory is root (/). however, if the 
ftp server (on win32) set the virtual directory as something, e.g TEMP, mozilla 
(linux and win32) refused/unable to access. Message "Error 425 Download 
failed." appeared. IE 6.0, GFTP, and command line are OK.

Reproducible: Always

Steps to Reproduce:
1.Setup GuildFTPd server on win32
2.Create 2 path ("/" and "/temp")
3.ftp://user:pwd@ftp_server/ (should be OK)
4.ftp://user:pwd@ftp_server/temp (not ok)

Actual Results:  
For Step 4. "425 Download Failed." error message appeared.

Expected Results:  
Try other client, e.g. IE6, command line, gFTP. It should be able to list files.

The problem should be tested for other ftp daemons (e.g pureftp, proftp) 
sitting on other platform (linux, win32, solaris). Not sure whether it is 
GuildFTPd specific. Mozilla should be from multiple platforms too.

Comment 1

[Josh Birnbaum]

*** Bug 214404 has been marked as a duplicate of this bug. ***

Comment 2

[Josh Birnbaum]

Is this the same issue as bug 168093?

Comment 3

[Lee]

Bug 168093 solved first part of test (when the virtual directory is root (/)). 
That's why Step 3 doesn't produce any error. The issue now is when the virtual 
folder is something other than root, e.g. TEMP, as in Step 4.

Comment 4

[Josh Birnbaum]

Can you capture some FTP logs? 
http://www.mozilla.org/projects/netlib/http/http-debugging.html has instructions
(the very bottom gives info on FTP instead of HTTP logging).  The logs hopefully
will explain what is happening in the two cases.

Comment 5

[Lee]

Attachment: ftp log file

Comment 6

[Lee]

I tried to setup similiar logging in Linux but unsuccessful. I just noticed 
that I can't even access the subdirectory of the '/' virtual directory setting. 
It just lists the '/' files and directories. Upon clicking the listed 
subdirectory, same error appeared (425, blah, blah). This could be same issue 
as the when the virtual root is something (due the fact they have same error 
message).

On Linux, Konquerer has no issue at all. Both the '/' and non-'/' virtual 
directory setting, it can list and descended into the subdirectory. Perfect!

Just download GuildFTPd or whatever FTP daemon, install and test it out!

Comment 7

[Josh Birnbaum]

Can you do the logging one more time, but use NSPR_LOG_MODULES=nsFTPProtocol:5
(with the :5 at the end)?  The web page could be clearer--the default logging
level doesn't capture much.

Comment 8

[Lee]

Attachment: ftp log file with 5

Here it is, with "NSPR_LOG_MODULES=nsFTPProtocol:5" command

Comment 9

[Lee]

I noticed that for Mozilla to ftp into vsftpd running on Linux, no issue at all 
(to list and descend into subdirectory). Never tried to set the virtual 
directory on vsftpd other than '/' and test ftp from mozilla. Should test it!

Comment 10

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 11

[Quinkink]

Hello, Thanks for all the hard work,
I get the same error "425 download failed" with firefox 1.5 beta 1 and GuildFTP
0.999.13 Error does not occure with IE. Cause seems to be absence of last "\" on
folder paths. Unpracticle work around is to add every single folder path to
GuildFTP user paths.

Comment 12

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 13

[Chris]

Still does not work in Guidftpd.

http://forums.guildftpd.com/viewtopic.php?t=96

When you descend below the setup virtual paths after you try this workaround you get the error.

Normal FTP clients and IE7 work fine without workaround.

Firefox seems to be issuing a \ which result in two \\ given for the path

There is a suggestion that "Firefox doesnt follow the FTP RFC" as far as directories are concerned.

http://www.w3.org/Protocols/rfc959/

Comment 14

[Matthias Versen [:Matti]]

new based on last comment

Comment 15

[Michal Novotny [:michal]]

Attachment: patch v1

It is a question if it is a bug in firefox or in GuildFTPd. Firefox always tries to download URL first as a file and when it fails immediately with code 5xx then firefox treat URL as a directory. GuildFTPd's response to "RETR /somedirectory" is "150 Opening binary ..." followed by "425 Download failed." Error code 425 doesn't make sense to me in this situation according to RFC959, other FTP servers return usually immediately code 550. That's IMHO bug in GuildFTPd.

Anyway Firefox isn't able to list directory in case when positive preliminary reply 1xy is sent by server before error code 550. The empty data connection causes closing FTP channel so there is no listener for the second try.

The patch changes generation of the directory indexes so that directories have trailing slash and FTP protocol doesn't issue RETR command when URL have a trailing slash. It works but I'm afraid of regressions due to added trailing slash.

Comment 16

[Boris Zbarsky [:bzbarsky]]

There's no way I'm qualified to review this, but I think we've had issues with the trailing slash stuff before.... not sure about ftp.  dougt, darin, or biesi might know.

I'm loath to change this code without a reasonable ftp regression test suite in place.

Comment 17

[Doug Turner (:dougt)]

@michal could you address boris's questions in #15.

Doug

Comment 18

[Michal Novotny [:michal]]

(In reply to comment #17)
> @michal could you address boris's questions in #15.

Actually I see no question. The whole fix is based on the added slash, so if I really can't add it, then there is nothing to review. And the unit test can't be written until bug #479179 is fixed.

Comment 19

[Doug Turner (:dougt)]

> GuildFTPd's response to "RETR /somedirectory" is "150 Opening binary ..." followed by "425 Download failed." Error code 425 doesn't make sense to me in this situation according to RFC959, other FTP servers return usually immediately code 550. That's IMHO bug in GuildFTPd.

I suppose i would have considered this a question -- is GuildFTPd busted, and if so, can we just fix our handling for this server.  (@bz don't cry, a RFC 959 FTP client can't ftp to many sites.)

Comment 20

[Michal Novotny [:michal]]

I'm lost a little bit. That's not boris's question, but my comment. All I wanted to say was that GuildFTPd's behavior is nonstandard but we can handle it in our code.

Comment 21

[Doug Turner (:dougt)]

doh.  sorry about that.  Michal, i am worry about changing our behavior for just one server.  Can we do this continually using the mServerType?

Comment 22

[Michal Novotny [:michal]]

I don't think so. The problematic part is IMHO adding the slash and not skipping RETR command. The slash is added in nsIndexedToHTML::OnIndexAvailable() in source netwerk/streamconv/converters/nsIndexedToHTML.cpp where we don't know anything about mServerType.

Btw mServerType is FTP_UNIX_TYPE since it is set according to response to SYST command. We would need to parse inviting 220 reply to know that it is GuildGFTPd.

Comment 23

[Jason Duell]

Comment on attachment 365264 [details] [diff] [review]
patch v1

Boris knows the FTP stuff better than I do, so I'm inclined to go with him.  I.e. In the absence of a good deal of testing on lots of different FTP servers, I'm wary of adding '/' for all servers just to work around what appears to be buggy behavior by GuildFTP.  

Switching reviewer to biesi, in case he's got more of a sense of whether this would be safe.

Comment 24

[Jason Duell]

Just to be clear:  if Michal (or someone else) has time to test this on enough different FTP servers to be comfortable that nothing's broken, we could probably move this along.  Does anyone have any idea of what sorts of edge cases (if any) might be triggered by adding the '/'?

P.S.  mServerType is currently planned to to turn into an "isVMS" flag once bug 202730 lands.   But if you could use it somehow to identify GuildFTP, that could be a way to limit the '/' fix to just the server that needs it.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm not sure how this patch could break anything? Surely the only thing it can break is the case when someone enters a URL ending in /, if that URL is actually a file. right?

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 365264 [details] [diff] [review]
patch v1

+    // Should we add the trailing slash only in case of FTP? (bug #214405)

I think this is fine for all protocols. But you should update the comment at line 914.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #22)
> I don't think so. The problematic part is IMHO adding the slash and not
> skipping RETR command.

That would already be a problem if a user directly enters an ftp://foo/bar/ URL though, right?

Comment 28

[Michal Novotny [:michal]]

(In reply to comment #27)
> That would already be a problem if a user directly enters an ftp://foo/bar/ URL
> though, right?

Yes. So what is the result of the review? Would the patch be acceptable after changing the comment and doing some tests against several FTP servers?

Comment 29

[Jason Duell]

> Would the patch be acceptable after
> changing the comment and doing some tests against several FTP servers?

In my opinion, yes.  I'm guessing it's a "yes" for biesi too.  Obviously, the more FTP servers tested (esp the most common ones) the better.

Comment 30

[Michal Novotny [:michal]]

I've tested this patch against several ftp daemons (vsftpd, proftpd, pure-ftpd, IIS, Crushftp, FreeBSD ftpd 6.00LS, Cerberus, FileZilla and of course GuildFTPd).

I've tried:
 - entering URL of file/directory with and without trailing slash
 - following link to file/directory in the listing
 - entering URL of hidden file/directory (only on some ftp servers)
 - entering URL of file/directory when directory listing is prohibited (only on some ftp servers)

So far I didn't notice any problem. Apart from fixing the problem with GuildFTPd this patch fixes also another bug I've noticed. Without this patch it is possible to download a file from some servers (proftpd, IIS, Cerberus, FileZilla) when the URL contains the trailing slash.


> I think this is fine for all protocols. But you should update the comment at
> line 914.

Hmm, how this patch affects the comment at line 914? http://hg.mozilla.org/mozilla-central/annotate/628f4b819a3d/netwerk/streamconv/converters/nsIndexedToHTML.cpp#l914

Comment 31

[Jason Duell]

Comment on attachment 365264 [details] [diff] [review]
patch v1

This seems like enough testing to push this patch into the tree.  I'm not sure we need sr on this, but I'm adding it for now to give christian a little while to explain what he meant about the request to change the comment on line 914.  If he's too busy to reply soon, I say we just push this.

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 365264 [details] [diff] [review]
patch v1

The reason to update that comment is because with this change, all protocols have a trailing slash for directories.

Also you should remove or rephrase this comment, because questions targeted at nobody in particular in code don't really make sense:
+    // Should we add the trailing slash only in case of FTP? (bug #214405)

Comment 33

[Michal Novotny [:michal]]

Attachment: patch v2

I've changed both comments. Please have a look if it make sense now.

Comment 34

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/ef994fbb7ca3