Repository navigation
Conversation
Ftp.java read the connect args at the wrong indexes (JS sends host, port, username, password, connectionMode, securityType, encoding), so active mode could never be enabled, and securityType was read but never used, so every connection was plain FTP. Use explicit FTPSClient (AUTH TLS) when securityType is "ftps" and send PBSZ 0 / PROT P after login so the data channel is encrypted too. Fixes Acode-Foundation#2979
Contributor
 [Medium impact] The PR appears safe to merge; the previous cached-connection issue is fixed. SummaryFixes FTP connection arguments and enables explicit FTPS.
Elitex07 explicitly leaves certificate validation, TLS session reuse, and implicit FTPS for possible follow-ups. Reviews (2) · Last reviewed commit: "fix(ftp): don't reuse cached client acro..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The profile id ignores security type and editing a profile does not disconnect, so switching FTP to FTPS could keep returning the old plain client. Only reuse the cached client when its type matches.
Contributor
Author
|
@greptile review again |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2979
What
Ftp.javaread theconnectargs at the wrong indexes. JS sendshost, port, username, password, connectionMode, securityType, encoding, but Java readconnectionModefrom index 6 and never usedsecurityType. Result: FTPS was ignored (always plain FTP) and active mode could never be enabled.Fix
FTPSClient(false)(explicit FTPS, AUTH TLS) whensecurityTypeisftps, otherwiseFTPClient.PBSZ 0+PROT Pso the data channel is encrypted too.No new dependency (
FTPSClientships with commons-net 3.12.0) and no JS change.Testing
On an emulator against a local pyftpdlib server requiring TLS on control + data:
550 SSL/TLS required on the control channel).PORT(full transfer not checked, emulator networking can't accept the back-connection).Not covered (possible follow-ups)
require_ssl_reuse=YES).