Added CR to CRLF conversion support for ASCII downloads

[andrewshulgin]

As per https://github.com/giampaolo/pyftpdlib/pull/492#issuecomment-477135219

[andrewshulgin]

Please check if this acceptable, and I'll update CHANGES.rst if true.

[giampaolo]

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.

[coveralls]

Coverage remained the same at ?% when pulling 0676df522a126eb9e7a32fe1b5a655ba1f9fb6d7 on andrew-shulgin:ascii_cr_to_crlf_conversion into 3514646b6607eecf5ebe0ff74587d7e3e3f24154 on giampaolo:master.

[giampaolo]

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'

[andrewshulgin]

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.

[andrewshulgin]

According to Wikipedia, CR line endings are native to:

• Commodore 8-bit machines (C64, C128)
• Acorn BBC
• ZX Spectrum
• TRS-80
• Apple II family
• Oberon
• classic Mac OS
• MIT Lisp Machine
• OS-9

Thus, any file created on these platforms has CR line endings. To be honest, it doesn't seem that any of these platforms are widely used nowadays.

[giampaolo]

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.

But we cannot know that, nor make assumptions. It's RFC-959 which dictates to convert os.sep to \r\n when sending a file in ASCII mode.

Also, one question: you modified FileProducer (meaning RETR, aka file going from server to client). What about the other way around? Does the same problem affect DTPHandler._posix_ascii_data_wrapper?

[andrewshulgin]

DTPHandler._posix_ascii_data_wrapper works correctly, because the client must always send CRLF-separated data if ASCII transfer mode is being used.

[andrewshulgin]

But we cannot know that

We can just detect and convert CR or LF line endings on-the-fly; this exact behaviour is implemented in this PR.

[giampaolo]

DTPHandler._posix_ascii_data_wrapper works correctly, because the client must always send CRLF-separated data if ASCII transfer mode is being used.

Oh you're right.

We can just detect and convert CR or LF line endings on-the-fly; this exact behaviour is implemented in this PR.

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.

[andrewshulgin]

It would be interesting to know what proftpd and vsftpd do in this regard.

I agree, going to check it soon.

[andrewshulgin]

I checked that and can conclude: both ProFTPd and vsftpd do not convert stand-alone CRs into CRLFs. On Linux, at least.

[lurch]

CR line endings are native to: ZX Spectrum

Oh noes, you mean I can't run pyftpdlib on my Speccy?! :scream_cat: :rainbow: :keyboard: :cold_sweat: :rofl:

[lurch]

WRT the unit tests, I wonder if it might make sense to also have tests with \r or \n as the very first character of the string, to make sure any edge-cases get caught? :man_shrugging:

Note that these review-comments don't imply that I "endorse" this change; that's entirely up to @giampaolo :slightly_smiling_face:

[giampaolo]

I checked that and can conclude: both ProFTPd and vsftpd do not convert stand-alone CRs into CRLFs. On Linux, at least.

OK, I'm gonna close this out then. Thanks.