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

Use connection host if TLS options host not specified #179

Merged
merged 1 commit into from Mar 24, 2021

Conversation

alandoherty
Copy link
Contributor

Potential fix for

#166

@@ -263,7 +263,9 @@ export class Client {
welcome = await this.connect(options.host, options.port)
}
if (useExplicitTLS) {
await this.useTLS(options.secureOptions)
let secureOptions = options.secureOptions || {}
secureOptions.host = secureOptions.host || options.host
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm willing to merge this if this really fixed things in your case. It did, right? There is one problem here we should be aware of: FTP uses two socket connections, one for commands (control) and one created each time for a file transfer or directory listing (data). The error you're trying to fix here occurred with the data socket connection, not the control connection which is established initially, when using access. (I still don't know why this happens.) That secureOptions will be used later for setting up data socket connections. Now, the original FTP spec says that in theory an FTP server can make a client use an entirely different server for data connections than for the control one. With this change, we would probably no longer support that because we ourselves set the host... This is used very rarely today, though. Have to think about it but we could probably go ahead with this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, the error in the original issue is not showing up after the useTLS below. Only much later when an actual data transfer happens.

@patrickjuchli patrickjuchli merged commit 5c272fb into patrickjuchli:master Mar 24, 2021
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.

None yet

2 participants