Opened 9 years ago

Closed 17 months ago

Last modified 17 months ago

#1333 enhancement closed fixed (fixed)

Survey existing FTP servers and clients to figure out how LIST works

Reported by: exarkun Owned by:
Priority: low Milestone:
Component: ftp Keywords:
Cc: exarkun, spiv, itamarst, jknight, jesstess, adi@… Branch: branches/ftp-list-flags-1333-2
(diff, github, buildbot, log)
Author: spiv, adiroiban Launchpad Bug:

Description


Attachments (1)

1333-1.diff (2.0 KB) - added by adiroiban 17 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by exarkun

Currently we special case a few options in ftp_LIST.  This sucks and we should
do something better.

comment:2 Changed 4 years ago by jesstess

  • Cc jesstess added
  • Owner changed from exarkun to jesstess

comment:3 Changed 3 years ago by <automation>

  • Owner jesstess deleted

comment:4 follow-up: Changed 2 years ago by adiroiban

  • Cc adi@… added

Beside the current code:

        # bug in konqueror
        if path == "-a":
            path = ''
        # bug in gFTP 2.0.15
        if path == "-aL":
            path = ''
        # bug in Nautilus 2.10.0
        if path == "-L":
            path = ''
        # bug in ange-ftp
        if path == "-la":
            path = ''

I have also discovered the following clients:

        # bug in FireFTP
        if path == "-al":
            path = ''

        # bug in BareFTP
        if path == "-La":
            path = ''

Maybe we can use a regex or something simple like:

if path.lower() in ['-a', '-l', '-la', '-al']:
    path = ''

comment:5 Changed 2 years ago by adiroiban

  • Keywords review added

comment:6 in reply to: ↑ 4 Changed 2 years ago by spiv

  • Keywords review removed
  • Owner set to spiv
  • Status changed from new to assigned

Replying to adiroiban:

Maybe we can use a regex or something simple like:

if path.lower() in ['-a', '-l', '-la', '-al']:
    path = ''

I like this approach. Obviously it needs tests etc. I'll make a patch, if I don't fall asleep or otherwise get distracted first :)

comment:7 Changed 2 years ago by spiv

  • Author set to spiv
  • Branch set to branches/ftp-list-flags-1333

(In [34787]) Branch to ftp-list-flags-1333

comment:8 Changed 2 years ago by spiv

  • Keywords review added
  • Owner spiv deleted
  • Status changed from assigned to new

comment:9 Changed 2 years ago by therve

  • Keywords review removed
  • Owner set to spiv
  1. Please name the test test_LISTWithBinLsFlags and add a docstring.
  1. This needs a NEWS fragment.

Thanks, merge once those points are fixed!

comment:10 Changed 2 years ago by therve

#5791 has been closed a duplicate of this bug.

comment:11 Changed 17 months ago by adiroiban

  • Keywords review added
  • Owner spiv deleted

I have attached a patch on top of branch 1333

I have renamed the test and add docstring. Also added docstring and renaming for neighbor tests.

It looks like the branch already has a news file.


Please switch to git/github. Working with SVN is so slow, Trac code navigation is slow and uploading patched to tickets is not fun.

Changed 17 months ago by adiroiban

comment:12 Changed 17 months ago by tomprince

  • Author changed from spiv to spiv, tomprince
  • Branch changed from branches/ftp-list-flags-1333 to branches/ftp-list-flags-1333-2

(In [37410]) Branching to ftp-list-flags-1333-2.

comment:13 Changed 17 months ago by tomprince

  • Resolution set to fixed
  • Status changed from new to closed

(In [37413]) Merge ftp-list-flags-1333-2: Handle bogus options to ftp's LIST command.

Author: spiv, adiroiban
Reviewers: therve, tom.prince
Fixes: #1333

The twisted.protocols.ftp.FTP server now treats "LIST -La", "LIST -al", and all
other combinations of ordering and case of the "-l" and "-a" flags the same: by
ignoring them rather than treating them as a pathname.

comment:14 Changed 17 months ago by tom.prince

  • Author changed from spiv, tomprince to spiv, adiroiban
  • Keywords review removed

(I accidentally put my review on #5411 by mistake:)

Minor change: Prefer assertIn to assertTrue(... in ...).

I've fixed this up and merged forward.

Will merge once build results are in.

Note: See TracTickets for help on using tickets.