Skip to content

better handling of ftp features #91

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

Merged
merged 5 commits into from
Oct 10, 2017
Merged

better handling of ftp features #91

merged 5 commits into from
Oct 10, 2017

Conversation

willmcgugan
Copy link
Member

@willmcgugan willmcgugan commented Oct 10, 2017

Work in Progress

Handle utf8 encoding for ftp servers without opening another connection.

Also ensures that features are requested after login.

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-1.0e-05%) to 99.949% when pulling fbd9b9b on ftp-features into d22fba6 on master.

else 'latin-1'
)
if not PY2:
_ftp.file = _ftp.sock.makefile(
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how much of a hack this is. The file attribute of FTP objects is the only thing that references the encoding, by re-creating it, we can modify the encoding based on features.

Copy link
Member

Choose a reason for hiding this comment

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

It should be fine, I don't think the ftplib implementation will change since it is part of the stdlib.

Isn't your linter complaining about the seemingly-useless self.ftp lines in other methods though ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be fine, I don't think the ftplib implementation will change since it is part of the stdlib.

That's my thinking.

Isn't your linter complaining about the seemingly-useless self.ftp lines in other methods though ?

It is, and a few more complaints besides. I'll see if I can clear some of those up.

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.95% when pulling 2353fc0 on ftp-features into d22fba6 on master.

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.95% when pulling 139a4fb on ftp-features into d22fba6 on master.

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+5.0e-05%) to 99.95% when pulling 61f7a67 on ftp-features into d22fba6 on master.

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+5.0e-05%) to 99.95% when pulling 61f7a67 on ftp-features into d22fba6 on master.

@willmcgugan willmcgugan merged commit d2da568 into master Oct 10, 2017
@willmcgugan willmcgugan deleted the ftp-features branch October 10, 2017 12:36
@muffl0n
Copy link
Contributor

muffl0n commented Oct 10, 2017

I just tested these changes with our proxy and now I hate our proxy even more. :)
The code clearly does what it should by calling "FEAT" after logging in:

*resp* '220 proxy FTP server (Version x.x.x.x) ready'
*cmd* 'USER [email protected]'
*resp* '331 (not authenticated): Enter server password'
*cmd* 'PASS *********'
*resp* '230-Connected to server. Logging in...\n230-220 ProFTPD 1.3.4a Server (Debian) [x.x.x.x]\n230-331 Password required for user\n230 230 User user logged in'
*cmd* 'FEAT'

But that does not help with my use case as the proxy still does not proxy the command to the target server:

*resp* "500 'FEAT': command unrecognized."

I'm still trying to figure out how ncftp or lftp handle this.

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.

4 participants