Fixing proxy-problems in _parsePassiveModeReply

[SteveOswald]

FTPClient in passive mode didn't worked, when using a proxy, cause the FTPClient tried to open a data-connection to the proxy-host instead the real passive-host. This was caused, cause the passiveHost was ALWAYS overwritten by the socket-address (wich is the address of the proxy-host, when using a proxy). The passive-host should ALWAYS be parsed from the pasvResponse and only be overwritten, when the pasvResponse is a "look lie IP address" (0,0,0,0).

[garydgregory]

Hello @SteveOswald
This PR needs a test to prove the PR fixes something and to avoid regressions.

[SteveOswald]

Hi @garydgregory ,

we have tested it in our test environment as well as in our production environment. It works with and without proxy.

Also, it does not comply with the RFC to simply have the pasvHost overwritten with the socket address every time (as is currently the case). The server specifies an address in the PASV response to connect to. This address should also be used. The only exception should be if the server sends an invalid address ala "0,0,0,0". This case is already covered in the code, so it is not necessary to always use the socket address - as is currently the case.

A test case for this is factually hard, as it would require a working HTTP proxy. However, it follows from pure logic that the current behavior is wrong.

[schmied]

This seems to fix my problem with squid and FTPS. By now, as a workaround, I set org.apache.commons.net.ftp.ipAddressFromPasvResponse = true.

[garydgregory]

Still missing tests

[marwenlahmar]

We are facing the same problem connecting to FTP servers behind a squid proxy, any chance for a new release ?

[garydgregory]

We are facing the same problem connecting to FTP servers behind a squid proxy, any chance for a new release ?

Hi @marwenlahmar,
Finally coming back to these parts...

I am not comfortable merging this PR unless there is a test that shows that it fixes something. No matter what IRL testing may have taken place.

IOW, a test should fail without any change to the main tree. Then, when the main changes are applied, the test passes.

This new test can be set up in any way you want, using plain unit testing, using mock objects, or even Docker (there is a Maven Docker plugin).

HTH,
G

[DazzaL]

This 3.9 behaviour also breaks all FTP servers that sit behind F5 load balancers when passive mode is used which this patch fixes too as the client tries to connect to the F5 address Vs the serving pool member (connection refused error or read timeouts occur). Having said that isn't the resultant code now odd?
As 3.9 added this isIpAddressFromPassiveResponse but now it's now set from the passive response anyway (and not overwritten from the control channel anymore). The only difference is that this setting still acts as a control switch for enabling / disabling the nat workaround strategy behaviour. So is this switch now not redundant here? Also what was the purpose of the switch, ie what's the problem with trusting the servers reply that resulted in a breaking change in 3.9.0 anyway?