Skip to content
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

bug: FTPFS incorrectly parses results of LIST command #506

Closed
adriangb opened this issue Dec 9, 2021 · 5 comments · Fixed by #507
Closed

bug: FTPFS incorrectly parses results of LIST command #506

adriangb opened this issue Dec 9, 2021 · 5 comments · Fixed by #507
Labels
Milestone

Comments

@adriangb
Copy link
Contributor

adriangb commented Dec 9, 2021

I'm connecting to a Linux FTP server that has usernames with a .. The current regex ([\w\-]+) doesn't allow this, and so no directories are listed (side note: would it make sense to error when none of the matchers match and the line isn't empty? It was very confusing at first).

According to the Ubuntu manpage for adduser:

adduser and addgroup enforce conformity to IEEE Std 1003.1-2001, which allows only the following characters to appear in group and user names: letters, digits, underscores, periods, at signs (@) and dashes. The name may no start with a dash. The "$" sign is allowed at the end of usernames (to conform to samba).

Now, some distros may have more stringent restrictions, but I suggest that the regex be expanded to at least accept the pattern mentioned in the manpage for Ubuntu. I think the regex would be [A-Za-z0-9][A-Za-z0-9\-\.\_\@]*[\$]{0,1}.

If you agree, I opened #507 to address this.

@adriangb adriangb changed the title FTFS incorrectly parses results of LIST command bug: FTPFS incorrectly parses results of LIST command Dec 9, 2021
@althonos
Copy link
Member

Hi Adrian, thanks for the bug report and the quick patch! I also think that the regex should be accepting a large set of characters rather than be conservative. The FTP protocol is really not standardized at all and there are plenty of strand FTP server in the wild, so I'll be happy to merge this patch.

would it make sense to error when none of the matchers match and the line isn't empty? It was very confusing at first.

Absolutely! I think we can probably create a new exception here, inheriting ValueError, and then chain it into an OperationFailed in the relevant part of the FTPFS code so that we can get a detailed error result in case it happens again.

@adriangb
Copy link
Contributor Author

I opened #508 to track raising an error (this issue will be closed by #507)

@adriangb
Copy link
Contributor Author

Thank you for the quick turnaround time @althonos! Is there any chance you could cut a patch release with these changes? Thanks!

@althonos
Copy link
Member

I'll make a new patch soon, but while we are fixing some issues with FTPFS I think I'll try to get #508 and #497 fixed before.

@adriangb
Copy link
Contributor Author

Awesome. I'll try to find time to work on #508 this week.

@althonos althonos added this to the v2.4.15 milestone Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants