exposing handle creation functions and fixed issue in reading multi line response - #2
exposing handle creation functions and fixed issue in reading multi line response #2pucsdian wants to merge 3 commits into
Conversation
|
how do you test this? |
telser
left a comment
telser
left a comment
There was a problem hiding this comment.
Overall seems okay, but again second the testing, though we don't have a suite today.
| 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 |
There was a problem hiding this comment.
This seems reasonable on the face to me, but I do really wish we had a test suite for these kinds of things.
There was a problem hiding this comment.
I don't have test suite now, but I faced this issue with one FTP server where welcome banner is like provided example.
There was a problem hiding this comment.
what is the hostname and port of the FTP server?
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ')
There was a problem hiding this comment.
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.
Follows bounded-text, beeline, shrubbery, orb, rollbar-haskell and haskell-non-empty-text, which all carry a manual ci flag holding the strict ghc-options. Two deviations from that list: no henforcer plugin, since this repo has no henforcer.toml and its open imports would fail immediately, and no -Wmissing-import-lists, which would rewrite thirteen imports in the file PR #2 also edits. The flag is manual and defaults off, so nothing reaches Hackage consumers. It is enabled for all three packages in stack-base.yaml, so every rung of the matrix builds with -Werror. Libraries get no ghc-options at all when the flag is off, as before -- an -O2 in the else branch would have imposed it on downstream users, and cabal check says so. Roughly fifty warnings across the four source files. Most were mechanical: unused imports and do-binds, shadowed names, missing local signatures. Three were not: The record update at connectTLS was incomplete because TLSSettings is a sum type and settingDisableCertificateValidation only exists on TLSSettingsSimple. Rebuilding just that constructor keeps it working on crypton-connection 0.3, which lacks the settingClientSupported field a positional call would need. Producer and Consumer are deprecated conduit synonyms, five of them in exported signatures. They expand to `forall i. ConduitT i o m ()` and `forall o. ConduitT i o m r`, not to ConduitT () o m () and ConduitT i Void m r -- those are Source and Sink. Rewriting them with the type variables left free keeps the exported types identical; pinning them to ()/Void would have narrowed the API and broken the internal fusion in mlsd and stor. MonadResource is in five exported signatures but is re-exported by Conduit, so the direct resourcet import counted as unused and -Wunused-packages then rejected the dependency. Taking the name from resourcet explicitly and hiding it from the Conduit import keeps the dependency and its version bound honest. transformers comes out of ftp-client and containers out of example; neither was imported anywhere. Deriving Typeable is dropped -- a no-op since GHC 7.10 that only GHC 9.12 warns about -- and acct, pbsz, prot, ccc and auth are now exported rather than sitting unused, matching every other command wrapper.
3 participants
$1. Exposed below 3 functions to have better control on ftp handle's life-cycle.
$2. Fixed issue in reading multi line response
eg : for response like below
'220-First Line
220-Second Line
220 Third Line'
Expected all 3 lines but getting only 2 as code checks for only code of 2 lines.
But the check should follow https://datatracker.ietf.org/doc/html/rfc959#page-36
Thus the format for multi-line replies is that the first line will begin with the exact required reply code, followed immediately by a Hyphen, "-" (also known as Minus), followed by text. The last line will begin with the same code, followed immediately by Space , optionally some text, and the Telnet end-of-line code.