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

Fix Transmit client treating a successful PASV response as failure #85

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

Conversation

michalc
Copy link
Contributor

@michalc michalc commented Nov 1, 2018

This fixes PASV connections on the Transmit client, that doesn't seem to handle the multiline response. An example transcript is below.

**********

Date/Time: 2018-11-01 16:37:39 +0000

1: Transmit 5.2.1 (x86_64) Session Transcript [Version 10.13.5 (Build 17F77)] (01/11/2018, 16:37)
1: LibNcFTP 3.2.3 (July 23, 2009) compiled for UNIX
1: 220: welcome
1: Connected to 127.0.01.
1: Cmd: USER myu
1: 331: password required
1: Cmd: PASS xxxxxxxx
1: 230: normal login
1: Cmd: TYPE A
1: 200: 
1: Logged in to 127.0.01 as myu.
1: Cmd: SYST
1: 215: UNIX Type: L8
1: Cmd: FEAT
1: 502: 'feat' not implemented
1: Cmd: PWD
1: 257: "/"
1: Cmd: PASV
1: Cannot parse PASV response: listen socket created
1: 227: listen socket created
1:      (0,0,0,0,15,161)
1: Passive mode refused.
1: Connection falling back to port (PORT) mode.
1: Cmd: PORT 127,0,0,1,243,198
1: 502: 'port' not implemented
1: Cmd: PORT 127,0,0,1,243,199
1: 502: 'port' not implemented
1: Cmd: PORT 127,0,0,1,243,200
1: 502: 'port' not implemented
1: Cmd: PORT 127,0,0,1,243,201
1: 502: 'port' not implemented
1: Canceling…
1: Disconnecting from server…
1: Cmd: QUIT
1: 221: bye

@pohmelie
Copy link
Collaborator

pohmelie commented Nov 1, 2018

I don't get what this solves and why multiline is a problem?

@michalc
Copy link
Contributor Author

michalc commented Nov 1, 2018

@pohmelie I have amended the PR description with more details

@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #85 into master will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   94.04%   93.98%   -0.06%     
==========================================
  Files           7        7              
  Lines        1695     1695              
==========================================
- Hits         1594     1593       -1     
- Misses        101      102       +1
Impacted Files Coverage Δ
aioftp/server.py 96.92% <66.66%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80089a6...9b294bd. Read the comment docs.

@pohmelie
Copy link
Collaborator

pohmelie commented Nov 1, 2018

I think at first you should try to get feedback from Transmit devs via e-mail (https://library.panic.com/transmit/). Sadly, their GOLD STANDARD app have no bugtracker.

227-listen socket created
227 (127,0,0,1,154,21)

Response is valid and works fine with all tools I tried.

@michalc
Copy link
Contributor Author

michalc commented Nov 1, 2018

I have now emailed them, thanks.

@pohmelie
Copy link
Collaborator

@michalc any news?

@tomharvey
Copy link

I'm getting the same error - but not just in Transmit (also seeing an error while using Cyberduck). Also, the PR from @michalc doesn't seem to fix the issue; still Cannot parse PASV response: listen socket created.

Maybe this should be an issue instead of in here?

I am using 0.0.0.0 as the bind IP, and then connecting to 127.0.0.1 - is this maybe related to passive not liking the local IP addresses? I can investigate more next weekend..

@pohmelie
Copy link
Collaborator

pohmelie commented Mar 1, 2020

I can investigate more next weekend

It will be good if you have time of course.

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.

3 participants