Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kellym
Copy link

@kellym kellym commented Mar 5, 2017

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
Copy link
Author

kellym commented Mar 8, 2017

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

@kellym kellym changed the title Use original socket to connect when using 'implicit' secure mode Bugfix: Use original socket to connect when using 'implicit' secure mode Mar 14, 2017
@kellym
Copy link
Author

kellym commented Jun 2, 2017

bump. is this repo not maintained anymore?

@Neullson
Copy link

Neullson commented Jul 11, 2017

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
Copy link

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
Copy link

gabema commented Oct 13, 2017

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
Copy link

pft commented Sep 3, 2018

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
Copy link

ValYouW commented Jun 23, 2019

@kellym Thanks, works great

@iisdan
Copy link

iisdan commented Apr 16, 2020

Can we get this merged in please

@doron2402
Copy link

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

miezzi added a commit to miezzi/node-ftp that referenced this pull request Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants