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
Added CR to CRLF conversion support for ASCII downloads #494
Added CR to CRLF conversion support for ASCII downloads #494
Conversation
Please check if this acceptable, and I'll update CHANGES.rst if true. |
Hm... this adds a lot of complexity. If os.sep is "\r" on OSX 9 only I would say it's better to leave this out. |
Here's what I would do instead: --- a/pyftpdlib/handlers.py
+++ b/pyftpdlib/handlers.py
@@ -2835,8 +2835,12 @@ class FTPHandler(AsyncChat):
"""Set current type data type to binary/ascii"""
type = line.upper().replace(' ', '')
if type in ("A", "L7"):
- self.respond("200 Type set to: ASCII.")
- self._current_type = 'a'
+ if os.sep in ('\n', '\r\n'):
+ self.respond("200 Type set to: ASCII.")
+ self._current_type = 'a'
+ else:
+ self.respond('504 ASCII mode not supported on this platform')
+
elif type in ("I", "L8"):
self.respond("200 Type set to: Binary.")
self._current_type = 'i' |
As I stated in the prevoius PR, the platform-native separator doesn't matter much, because any file can have CR line ending on any platform, e.g. downloaded from the web. |
According to Wikipedia, CR line endings are native to:
Thus, any file created on these platforms has CR line endings. |
But we cannot know that, nor make assumptions. It's RFC-959 which dictates to convert Also, one question: you modified |
|
We can just detect and convert CR or LF line endings on-the-fly; this exact behaviour is implemented in this PR. |
Oh you're right.
Mm.. but is this really desirable? Couldn't "\r" be legitimate (aka it should not be converted)? It would be interesting to know what proftpd and vsftpd do in this regard. |
I agree, going to check it soon. |
I checked that and can conclude: both ProFTPd and vsftpd do not convert stand-alone CRs into CRLFs. On Linux, at least. |
Oh noes, you mean I can't run pyftpdlib on my Speccy?! 🙀 🌈 ⌨️ 😰 🤣 |
WRT the unit tests, I wonder if it might make sense to also have tests with Note that these review-comments don't imply that I "endorse" this change; that's entirely up to @giampaolo 🙂 |
OK, I'm gonna close this out then. Thanks. |
As per #492 (comment)