Opened 3 years ago

Closed 3 years ago

#4909 enhancement closed fixed (fixed)

FTP.ftp_STOR() should pass FTPCmdError errors back to the client

Reported by: bigjools Owned by: exarkun
Priority: normal Milestone:
Component: ftp Keywords:
Cc: itamarst Branch: branches/ftp-stor-error-reporting-4909
(diff, github, buildbot, log)
Author: bigjools Launchpad Bug:

Description

ftp_STOR() has an errorBack, ebSent(), which ignores the error. It would be better if it could pass that error back to the client (as opposed to the default "internal server error")

Attachments (2)

fix-stor-errors.patch (443 bytes) - added by bigjools 3 years ago.
diff.patch (2.3 KB) - added by bigjools 3 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc itamarst added

Changed 3 years ago by bigjools

comment:2 Changed 3 years ago by bigjools

  • Keywords review added

This change is to support a Launchpad change that I am making which will send errors back to 'dput' when people upload packages without a valid GPG signature on their .changes file. It is the source of many questions when their uploads go missing!

comment:3 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to bigjools

Thanks! This sounds like a great feature to add. The approach of handling the particular exception type specially seems right, too.

  1. Can you also add a unit test that verifies the code and message are sent back in this case?
  2. What would you link about not logging the error in this case?
  3. Can you write a news fragment describing this new feature?

Thanks again!

comment:4 Changed 3 years ago by bigjools

  1. I've not contributed to Twisted before. Can you point me at the right place to add a test for this please? (I want to make sure I don't do it wrong)
  1. Not logging the error - possibly, but I think it's more useful to log it for audit reasons. At least, I'd like it logged in my ftp server.
  1. Okay. Jeez, and I thought our Launchpad landing process was tough :)

comment:5 Changed 3 years ago by exarkun

Can you point me at the right place to add a test for this please?

The tests for FTP are in twisted/test/test_ftp.py (you can see this module named in the test-case-name definition at the top of ftp.py - many, but not all, Twisted files use this to point to their tests). It looks like BasicFTPServerTestCase in that file might be a good test case to add the new test to.

it's more useful to log it for audit reasons

Okay, that's fine. I think we can probably provide a better logging experience for audit purposes than reporting a full traceback, but that's clearly a separate feature, so don't worry about it.

and I thought our Launchpad landing process was tough

:)

Changed 3 years ago by bigjools

comment:6 Changed 3 years ago by bigjools

  • Keywords review added
  • Owner bigjools deleted

Latest patch attached, with thanks to therve and jml for helping me fix my test.

comment:7 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:8 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/ftp-stor-error-reporting-4909

(In [30840]) Branching to 'ftp-stor-error-reporting-4909'

comment:9 Changed 3 years ago by exarkun

  • Author changed from exarkun to bigjools
  • Keywords review removed

Thanks very much for adding the unit test. This makes it clear that my original review missed the fact that the error comes out formatted rather strangely. :) Trying to understand why the message comes out as:

550 Cannot rmd, ('blah',) is not a directory

instead of:

550 Cannot rmd, blah is not a directory

led me first to the seemingly bizarre signature of FTPCmdError.__init__, which takes *args and just saves them away. So IsNotADirectoryError("foo") gives an errorMessage attribute of ("foo",). Not a very nice "message". But then I realized this is why there's the FTPCmdError.response method, which will spit out a properly formatted string.

However, calling this in ebSent is no good, because it returns the complete response line, and ebSent needs to return a tuple giving a response code and arguments to interpolate into the message for that code.

Fortunately, it seems all is not lost. For the very next errback in the chain after ebSent is the nested function processFailed in FTP.lineReceived. And that errback checks for FTPCmdError failures and writes out the result of calling their FTPCmdError.response methods. Just what's wanted here. So I think this tweak to the patch is in order:

  • twisted/protocols/ftp.py

     
    11121112        def ebSent(err): 
    11131113            log.msg("Unexpected error receiving file from client:") 
    11141114            log.err(err) 
     1115            if err.check(FTPCmdError): 
     1116                return err 
    11151117            return (CNX_CLOSED_TXFR_ABORTED,) 
    11161118 
    11171119        def cbConsumer(cons): 

That is, just pass on the failure if it is wrapping an FTPCmdError exception and let the next errback deal with it.

As I was figuring all this out, I noticed there is another nested ebSent method in the implementation of RETR which has more or less the exact same issue as this one! That probably merits a bit of fixing as well (separate ticket), and maybe the code can be refactored to remove the duplication as well.

With that overly long exposition out of the way, I think we can get on to the action. I'll check your latest into a branch, make my tweak to improve the message rendering, make sure it works everywhere, and then land it. Thanks!

comment:10 Changed 3 years ago by exarkun

(In [30841]) Apply latest patch

refs #4909

comment:11 Changed 3 years ago by exarkun

(In [30842]) Get rid of the extra tupleyness of the error message reported back to the client.

refs #4909

comment:12 Changed 3 years ago by bigjools

Great! Thanks for fixing up and accepting. When's the next Twisted release expected?

comment:13 Changed 3 years ago by exarkun

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

(In [30844]) Merge ftp-stor-error-reporting-4909

Author: bigjools
Reviewer: exarkun
Fixes: #4909

If an FTPCmdError is encountered while writing a file when handling
a STOR command in twisted.protocols.ftp.FTP, allow the details
of that exception to be sent back to the client, rather than flattening
them into a generic CNX_CLOSED_TXFR_ABORTED.

Note: See TracTickets for help on using tickets.