Closed Bug 214405 Opened 20 years ago Closed 13 years ago

Unable to ftp virtual folder

Categories

(Core :: Networking: FTP, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: lx3hf, Assigned: michal)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
*** Bug 214404 has been marked as a duplicate of this bug. ***
Is this the same issue as bug 168093?
Assignee: harishd → dougt
Component: Web Services → Networking: FTP
QA Contact: ashishbhatt → benc
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.
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.
Attached file ftp log file
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!
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.
Attached file ftp log file with 5
Here it is, with "NSPR_LOG_MODULES=nsFTPProtocol:5" command
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!
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/
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.
mass reassigning to nobody.
Assignee: dougt → nobody
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/



new based on last comment
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: benc → networking.ftp
Assignee: nobody → michal
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #365264 - Flags: review?(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.
Depends on: 479179
Attachment #365264 - Flags: review?(bzbarsky) → review?(doug.turner)
@michal could you address boris's questions in #15.

Doug
(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.
> 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.)
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.
doh.  sorry about that.  Michal, i am worry about changing our behavior for just one server.  Can we do this continually using the mServerType?
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.
Attachment #365264 - Flags: review?(doug.turner) → review?(jduell.mcbugs)
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.
Attachment #365264 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
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.
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 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.
Attachment #365264 - Flags: review?(cbiesinger) → review-
(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?
(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?
> 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.
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 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.
Attachment #365264 - Flags: superreview?(cbiesinger)
Attachment #365264 - Flags: review-
Attachment #365264 - Flags: review+
Attachment #365264 - Flags: superreview?(cbiesinger) → superreview+
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)
Attached patch patch v2Splinter Review
I've changed both comments. Please have a look if it make sense now.
Attachment #365264 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ef994fbb7ca3
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.