mscdex / node-ftp

An FTP client module for node.js
MIT License
1.13k stars 243 forks source link

Bugfix: Use original socket to connect when using 'implicit' secure mode #171

Open kellym opened 7 years ago

kellym commented 7 years ago

Currently, implicit FTPS mode is broken.

When using implicit FTPS, the current code assigns the returned socket from tls.connect to this._socket, which is used to make the initial connection.

The problem is that the TLSSocket expects the original socket passed in to make the connection. Since it doesn't, the socket never actually connects and it always ends in a timeout.

I ran into this with a client this past week, and there's a StackOverflow question that is unanswered regarding this same problem. It appears there are no tests for connecting, but I've verified that I'm able to connect to our client's implicit FTPS server with this change.

kellym commented 7 years ago

FYI this is a bug fix, not a feature request.

kellym commented 7 years ago

bump. is this repo not maintained anymore?

Neullson commented 7 years ago

Hi @kellym ,

Hope you are doing fine. I use your connection.js for this issue. And it works! Connection to FTP using implicit is successfully established. But I got this problem when do a directory listing using 'list' command.

Error: Data channel timed out due to not meeting the minimum bandwidth requirement

When using FTP Client such as FileZilla, it can do the listing, put file, etc. Here is my config in node : "host": "xxx.xxx.xxx.xxx", "port": 990, "user": "xxxxxxx", "password": "xxxxxxxxxx", "secure": "implicit", "secureOptions": { "rejectUnauthorized": false }

This is piece of my simple code to test directory listing.

`this._FTPClient = new FTP(); this._FTPClient.connect(ftpConfig); Logging("ftpConfig : " + JSON.stringify(ftpConfig));

            this._FTPClient.on('ready', ()=> {
                Logging("Connection SUCCESSFULLY established");
                this._FTPClient.list((err:Error, list:any) => {
                    Logging("Reading input folder...")
                    if (err) {
                        Logging("Got Promise error");
                        ErrorHandlingService.throwPromiseError(pReject, 70000, "[API] FTP promise error : " +err.toString());
                        pReject(err.toString());
                    }
                    Logging("Reading input folder completed");
                    let res:any = list;
                    Logging("File Data : ");
                    Logging(JSON.stringify(res));
                    this._FTPClient.end();
                    pResolve(res);
                });
            });`

Is my config is wrong ? Is there any other thing that I need to check ?

rcmedeiros commented 7 years ago

Thanks @kellym. Although your fix didn't suffice to make it work with Filezilla Server (it bumps in other issues), it did make a successfull connection to trs/ftp-srv (which has explicit FTPS broken). I'm developing server and client altogether, FTPS mandatory, so for anyone trying this fix and getting errors, it may be worth giving a try with ftp-srv.

gabema commented 7 years ago

Thank you so much @kellym. This needs to get in. Confirmed this PR addressed my issue as well. Is @mscdex abandoning this project? It's a shame because other great projects (like promise-ftp) depend on this. Would @mscdex be willing to add additional maintainers to this project? ✋

pft commented 6 years ago

This PR works for me with FileZilla server up until now. I can both .list() a directory and put() a file.

My knowledge of the module and FTPS does not suffice to say whether the PR poses any problems.

ValYouW commented 5 years ago

@kellym Thanks, works great

iisdan commented 4 years ago

Can we get this merged in please

doron2402 commented 4 years ago

@kellym Can you publish your fork to npm? you should call it node-ftp-reloaded :)