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.

Sign up for GitHub

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

Jump to bottom

Use connection host if TLS options host not specified #179

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Use connection host if TLS options host not specified #179

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

await this.useTLS(secureOptions)
}
await this.login(options.user, options.password)
await this.useDefaultSettings()
Expand Down