Skip to content

exposing handle creation functions and fixed issue in reading multi line response #2

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.

Sign up for GitHub

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

Open
pucsdian wants to merge 3 commits into
base: main
Choose a base branch
from
Open

exposing handle creation functions and fixed issue in reading multi line response #2

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
660b4cb
expose createSIOHandle and createTLSConnection
pucsdian Feb 28, 2025
afd7a65
fix issue in reading multi line response
pucsdian Apr 1, 2025
0386eb7
expose connectTLS
pucsdian Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ftp-client/src/Network/FTP/Client.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ module Network.FTP.Client (
Handle(..),
-- * Exceptions
FTPException(..),
-- * System Handle Creation
createSIOHandle,
createTLSConnection,
connectTLS,
-- * Handle Implementations
sIOHandleImpl,
tlsHandleImpl,
Expand Down Expand Up @@ -269,8 +273,8 @@ loopMultiLine
loopMultiLine h code lines = do
nextLine <- liftIO $ getLineResp h
let newLines = lines <> [C.dropWhile (== ' ') nextLine]
nextCode = C.take 3 nextLine
if nextCode == code
isLastLine = C.isPrefixOf (code <> " ") nextLine -- Ref for reading multiline response : https://datatracker.ietf.org/doc/html/rfc959#page-36

@telser telser Apr 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable on the face to me, but I do really wish we had a test suite for these kinds of things.

@pucsdian pucsdian Apr 2, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have test suite now, but I faced this issue with one FTP server where welcome banner is like provided example.

@ysangkok ysangkok Apr 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the hostname and port of the FTP server?

@pucsdian pucsdian Apr 16, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't share the details. But you can use below python ftp server to test it,

you need to add pyftpdlib

from pyftpdlib.authorizers import DummyAuthorizer
from pyftpdlib.handlers import FTPHandler
from pyftpdlib.servers import FTPServer

authorizer = DummyAuthorizer()
authorizer.add_user("user", "12345", ".", perm="elr") 

class CustomHandler(FTPHandler):
    def on_connect(self):
        self.banner = (
            "220-Welcome to the Python FTP Server!\n"
            "220-This is a test server with a multiline banner.\n"
            "220-Feel free to browse or upload files.\n"
            "220 Have fun!"
        )
        super().on_connect()

handler = CustomHandler
handler.authorizer = authorizer

server = FTPServer(("127.0.0.1", 2121), handler)
print("FTP Server running on port 2121...")
server.serve_forever()

@ysangkok ysangkok May 7, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the snippet, I tried it with pyftpdlib version 2.0.1. I think the 220 on the first line needs to be removed, otherwise it is duplicated.

So I tested with

            "Welcome to the Python FTP Server!\n"
            "220-This is a test server with a multiline banner.\n"
            "220-Feel free to browse or upload files.\n"
            "220 Have fun!"

This emits

220-Welcome to the Python FTP Server!
220-This is a test server with a multiline banner.
220-Feel free to browse or upload files.
220 Have fun!
220

when connected to with nc localhost 2121.

I tried testing with the program

module Main where

import Network.FTP.Client

main :: IO ()
main = withFTP "127.0.0.1" 2121 $ \h welcome -> do
    putStrLn "Connected, printing welcome"
    print welcome
    putStrLn "Now doing login"
    login h "user" "12345"
    putStrLn "Did login"

On main, this progam surprisingly succeeds, with the output

Connected, printing welcome
220 Welcome to the Python FTP Server!
220-This is a test server with a multiline banner.
Now doing login
Did login

I think it mistakes the additional lines for successful login responses. On this PR, it gives the following output:

Connected, printing welcome
220 Welcome to the Python FTP Server!
220-This is a test server with a multiline banner.
220-Feel free to browse or upload files.
220 Have fun!
Now doing login
example-exe: Uncaught exception ftp-client-0.5.1.6-inplace:Network.FTP.Client.FTPException:

UnsuccessfulException 331 Username ok, send password.

While handling UnsuccessfulException 331 Username ok, send password.

So it seems that ftp-client doesn't understand the type of exchange in section 7 of the RFC.

Did you also encounter this issue? I am surprised that you didn't hit this issue, because you said you had tested with this program.

@pucsdian pucsdian May 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I faced same issue. bcoz python server by default sending 200<space> after banner lines so we should not add 200<space> in our banner at beginning of last line.

Please try with below snippet. (without changing pytpdlib)

"Welcome to the Python FTP Server!\n"
"220-This is a test server with a multiline banner.\n"
"220-Feel free to browse or upload files. Have fun!\n"

I am surprised that you didn't hit this issue, because you said you had tested with this program => I forgot to tell you that I have made some changes in the python library so that it sends the banner as we have provided there, sorry for that.
Please check this
lib/python3.8/site-packages/pyftpdlib/handlers.py

if not self._closed and not self._closing:
          if len(self.banner) <= 75:
                self.respond(f"220 {self.banner!s}")
            else:
                self.push(f'220-{self.banner!s}\r\n')
              #  self.respond('220 ')

@ysangkok ysangkok Jun 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel comfortable merging this, since it seems like it might provoke other issues, like the missing handling of the 331 code. Also, I am not comfortable with changing other libraries to test this library.

But I think the idea is ok, it seems like multi-line banners are common and should be supported. It's just that if we merge this, we should also make sure that the example scenario I posted above (with no modifications to pyftpdlib) is handled in this library. That way, at least we know that this basic use case keeps working.

Currently, since we don't have a patch for code 331 support, I think we're blocked on that.

if isLastLine
then return newLines
else loopMultiLine h code newLines

Expand Down