several fixes and features (rfc3659) #19

rvicentiu · 2016-09-13T10:12:41Z

UTF8 proper support activation
MLST facts configuration (don't count on server defaults)
LIST DOS/Windows "dir"-style response processing
CWD command support (required instead of LIST/MLSD path argument on some servers)
several fixes

changes tested on about 200 anonymous ftp servers

* some servers require it to enable features (such as UTF8 support)

* while some servers use UTF8 by default if supported, others require activation (and CLNT) * without UTF8 enabled you cannot access files/paths with non-english names as the server will treat your input as ASCII

* request only information we can process in parseMLST

* support for filenames containing "; " * fix for broken servers sending type "dir" with name "." or ".."

* some servers send a 426 when the data connection fails during transfer * this response remains in the queue breaking all commands sent after it

* just write to the debug log

* no point in spamming it before every LIST * helps when accessing high latency servers

* some servers return "not found" with "MLST path" or "LIST path" * running "CWD path" and "LIST" or "MLST" works instead

muirmanders

Wow, thanks for all the work! Seems like you have a lot more experience with FTP than me.

muirmanders · 2016-09-20T04:01:39Z

build_test_server.sh

@@ -84,7 +84,7 @@ VpOorURz8ETlfAA=
 -----END CERTIFICATE-----
 CERT

-curl -O http://download.pureftpd.org/pub/pure-ftpd/releases/obsolete/pure-ftpd-1.0.36.tar.gz
+curl -k -O https://download.pureftpd.org/pub/pure-ftpd/releases/obsolete/pure-ftpd-1.0.36.tar.gz


Still not sure why why need -k. What are you developing on that doesn't have a CA bundle?

I do have the CA bundle... but from experience I've also ended up working with servers that do not have it or it is outdated (not my choice). If you think it is unsafe I can remove it as it is not really need, I just added it for compatibility.

I think we should remove it.

muirmanders · 2016-09-20T04:06:17Z

persistent_connection.go

+}
+
+func (pconn *persistentConn) setUnicode() error {
+        code, msg, err := pconn.sendCommand("OPTS UTF8 ON")


Specifying UTF8 when available makes sense, but for my own edification, when is this required? Are there servers that won't send UTF-8 path names without this command?

I had an issue with several servers that treat UTF8 in client commands (like path in LIST / MLSD) as ASCII, thus replying with "not found", although they sent proper UTF-8 paths in replies

Again this is a feature not required in RFCs as UTF8 should be enabled by default if listed in FEAT but needed to work around broken servers (and there are quite a few of them).

muirmanders · 2016-09-20T04:12:27Z

file_system.go

@@ -112,6 +112,27 @@ func (c *Client) Getwd() (string, error) {
 	return dir, nil
 }

+// Setwd changes the current working directory.
+func (c *Client) Setwd(path string) (error) {


I didn't include this because it isn't compatible with the connection pool (would end up with connections in different directories). I considered making it a client-wide setting that gets applied whenever you get a connection (i.e. switches to the desired directory), but that could get complicated, too.

I know(and there is a comment on the commit with this :) ) but it seems it is used as a workaround by ftp clients when LIST or MLSD fails with path argument. I ran into the issue while testing and using CWD first before LIST (without path) worked.

I was considering the following:

renaming all functions that call getIdleConn() to *Ex() and replace them with a wrapper.

the Ex() versions would get the idx of the pconn as a param

the Ex() versions would call getIdleConn() only if idx was -1 otherwise use the idx pconn / fail.

SetWd() returns the pconn idx

some locking might be needed if the user starts several go routines using the same handle

as for me I really need this but I could move it to a separate branch if you do not

What if each connection remembers what dir it is in, and when a new connection is gotten it automatically does the cwd if it isn't in the correct dir (user's desired dir is stored on client object when they set via setwd())?

muirmanders · 2016-09-20T04:16:29Z

file_system.go

-	if err != nil {
-		if !commandNotSupporterdError(err) {
+	if pconn.mlsdNotSupported || err != nil {
+		if !pconn.mlsdNotSupported && !commandNotSupporterdError(err) {


Can this commandNotSupported check be moved up directly after the dataStringList() call? It's a little weird having the nested checks if mlsdNotSupported.

right... missed that, I'll commit a fix

muirmanders · 2016-09-20T04:20:30Z

file_system.go

@@ -148,7 +180,9 @@ func (c *Client) ReadDir(path string) ([]os.FileInfo, error) {
 		info, err := parser(entry, true)
 		if err != nil {
 			c.debug("error in ReadDir: %s", err)
-			return nil, err
+			if !strings.Contains(err.Error(), "incomplete") {


Do you have examples of incomplete MLSD/LIST output? Is it safe to give the user back incomplete results (i.e. maybe we just failed parsing)?

"............" is text utf8, this is a wireshark dump

220 Serv-U FTP Server v10.4 ready...
....
Type=dir;Modify=20160910211543.432;Perm=el; music
Type=dir;Modify=20160917224516.936;Perm=el; ............
....
Type=dir;Modify=20151128035821.241;Perm=el; video
Type=dir;Perm=el; ..........
Type=dir;Modify=20160911053801.535;Perm=el; www
...

"Type=dir;Perm=el; .........." is a directory that is no longer accesible and the server does not send any other information about it but we can still use the received list

muirmanders · 2016-09-20T04:40:56Z

persistent_connection.go

+}
+
+func (pconn *persistentConn) setMLST() error {
+        code, msg, err := pconn.sendCommand("OPTS MLST type;size;modify;perm;unix.mode;")


Should this be UNIX.mode or does case not matter?

from RFC 3659:
"Fact names are case-insensitive. Size, size, SIZE, and SiZe are the same fact."

I've made some protocol dumps from a few ftp clients and they seem to use lower case so I kept it as such, though the RFC continues with:
"Further operating system specific keywords could be specified by
using the IANA operating system name as a prefix (examples only):

OS/2.ea -- OS/2 extended attributes MACOS.rf -- MacIntosh resource forks UNIX.mode -- Unix file modes (permissions)"

muirmanders · 2016-09-20T04:47:41Z

file_system.go

+        if strings.Contains(matches[2], "DIR") {
+                mode |= os.ModeDir
+        } else {
+            size, err = strconv.ParseUint(matches[2], 10, 64)


Is ParseUint going to work on sizes with a comma like in your example above?

No, it will fail with err (size will be 0) but I have not found a single server that returns commas yet. Only running "dir" in local "command prompt" does that.

muirmanders · 2016-09-20T04:48:47Z

file_system.go

@@ -321,6 +348,66 @@ func (f *ftpFile) Sys() interface{} {
 	return f.raw
 }

+var dirRegex = regexp.MustCompile(`^\s*(\S+\s+\S+\s{0,1}\S{2})\s+(\S+)\s+(.*)`)
+var dirTimeFormats = []string{
+        "01-02-06  03:04",


Should this be 15:04 for parsing of 24hr clock value without am/pm?

muirmanders · 2016-09-20T04:51:42Z

file_system.go

+
+        var mtime time.Time
+        for _, format := range dirTimeFormats {
+                if len(entry) >= len(format) {


Is this check needed? Are you certain time strings can never be shorter than the formats (eg. "2006-01-02 2:04" is shorter than "2006-01-02 15:04")?

I needed a way to speed up processing large lists (thousands of files) and avoid false matches.
I have not yet found a server that doesn't pad with zero's but it might be possible.

muirmanders · 2016-09-20T04:53:00Z

file_system.go

+// 08/07/2016  17:20             1,135 file-name
+// or
+// 02-10-16  12:10PM           1067784 file.exe
+func parseDirLIST(entry string, loc *time.Location, skipSelfParent bool) (os.FileInfo, error) {


What server returns this format?

220 Microsoft FTP Service
...
STAT
211-Microsoft FTP Service status:
Logged in user: anonymous
TYPE: ASCII; FORM: NONPRINT; STRUcture: FILE; transfer MODE: STREAM
Data connection: none
211 End of status.
SYST
215 Windows_NT`

LIST output looks like this:
02-10-16 12:10PM 8067784 OneDriveSetup.exe
03-03-16 05:52PM
OS
06-20-16 03:43PM 164559 print.pdf

the main issue is the date format so I tried to support the most common formats I have found

muirmanders · 2016-09-20T05:10:03Z

Sorry, read your checkin comments and updated a few of by PR comments.

muirmanders · 2017-09-17T04:00:16Z

Hi - you have a bunch of good stuff in, but we still have some open discussions. If you don't have time to spend on this, do you want me to cherry pick the non-controversial parts and merge those?

tesharp · 2018-08-15T12:50:16Z

Any plan on merging this pull request? Would like to use this ftp client, but require support for Windows FTP Servers and this fixes those issues.

muirmanders · 2018-09-22T19:30:59Z

@tesharp do you know which fixes are needed for your use case? This branch has a lot of changes and unresolved conversations so I can't merge it as is. The easiest thing is to pull out the minimal fixes to a separate branch.

rvicentiu added 14 commits September 12, 2016 18:32

add CLNT feature support

690b188

* some servers require it to enable features (such as UTF8 support)

add UTF8 feature support

eddfb12

* while some servers use UTF8 by default if supported, others require activation (and CLNT) * without UTF8 enabled you cannot access files/paths with non-english names as the server will treat your input as ASCII

add MLST feature support (rfc3659)

a121710

* request only information we can process in parseMLST

fix panic when server send empty line in FEAT response

86961ae

fix parseMLST issues

89378c2

* support for filenames containing "; " * fix for broken servers sending type "dir" with name "." or ".."

add "dir"-style LIST response support

adaab2c

fix unexpected "426 Data connection closed..."

0812101

* some servers send a 426 when the data connection fails during transfer * this response remains in the queue breaking all commands sent after it

ignore incomplete MLSD / LIST entries without failing ReadDir

59f0223

* just write to the debug log

remember if MLSD is not supported

b1ca06d

* no point in spamming it before every LIST * helps when accessing high latency servers

add CWD command support (Setwd function)

dec280e

* some servers return "not found" with "MLST path" or "LIST path" * running "CWD path" and "LIST" or "MLST" works instead

fix tests - pureftp moved to https

5e0f0e8

clarify documentation

abfd949

cleanup spaces

1ac9e7f

cleanup spaces

abe3739

muirmanders reviewed Sep 20, 2016

View reviewed changes

muirmanders mentioned this pull request Jul 20, 2019

Cannot read directory error if filename contains ; char crazy-max/ftpgrab#55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

several fixes and features (rfc3659) #19

several fixes and features (rfc3659) #19

rvicentiu commented Sep 13, 2016 •

edited

Loading

muirmanders left a comment

muirmanders Sep 20, 2016

rvicentiu Sep 20, 2016

muirmanders Oct 2, 2016

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Oct 2, 2016

muirmanders Sep 20, 2016

rvicentiu Sep 20, 2016

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016

muirmanders Sep 20, 2016

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016

muirmanders Sep 20, 2016

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders commented Sep 20, 2016 •

edited

Loading

muirmanders commented Sep 17, 2017

tesharp commented Aug 15, 2018

muirmanders commented Sep 22, 2018

several fixes and features (rfc3659) #19

Are you sure you want to change the base?

several fixes and features (rfc3659) #19

Conversation

rvicentiu commented Sep 13, 2016 • edited Loading

muirmanders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muirmanders Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muirmanders Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

muirmanders Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

muirmanders Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvicentiu Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

muirmanders commented Sep 20, 2016 • edited Loading

muirmanders commented Sep 17, 2017

tesharp commented Aug 15, 2018

muirmanders commented Sep 22, 2018

rvicentiu commented Sep 13, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

rvicentiu Sep 20, 2016 •

edited

Loading

muirmanders commented Sep 20, 2016 •

edited

Loading