Author zvyn
Recipients barry, jesstess, pitrou, r.david.murray, zvyn
Date 2014-06-13.23:45:34
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <539B8D1C.4020205@oberkirch.org>
In-reply-to <1402608692.35.0.591417369561.issue21725@psf.upfronthosting.co.za>
Content
Thanks a lot for your feedback!

> Your modifications to the handling of max_command_size are technically buggy (and clearly there's no test for it): when we aren't in extended smtp mode, the limit should be the hardcoded value, as in the original code.  My reading of the RFC (specifically 4.1.1.5) indicates that it is legal to send a new HELO or EHLO command without issuing a QUIT, meaning a single SMTPChannel instance might have to handle both.  This isn't a likely scenario except in testing...but testing is probably one of the main uses of smtpd, so I think we ought to support it.

I agree that Section 4.1.1 should be interpreted that way and that
should probably get fixed. The old implementation is very strict about
EHLO/HELO being allowed exactly once. And there are tests to verify this
bug exists.
We could allow it for SMTPUTF8 since we do not need to care about bug
compatibility if someone uses enable_SMTPUTF8 and we document the change.

My intention was to leave the command_size_limits untouched on HELO so
it will return 512 on every request (as max_command_size does then). And
the existing implementation guaranteed me there is only one of HELO or EHLO.

> I think it would be clearer to use an unconditional set of the value based on the command_size_limit constant, which you'll have to restore to address my previous comment, instead of the pop (though the pop is a nice trick :).
I agree on replacing the .pop() with command_size_limits['MAIL'] =
command_size_limit. I see no easier/shorter way to add up the values
right now however.

> There is one piece missing here: providing a way for process_message to know that the client claimed the message was using SMTPUTF8 (we know it might lie, but we need to transmit what the client said).  Since process_message is an 'override in subclass' API, we can't just extend it's call signature.  So I think we need to provide a new process_smtputf8_message method that will be called for such messages.
Thanks for pointing that out, I totally missed that!

> Note that you also need to reset the 'this message is SMTPUTF8 flag' on RSET, HELO/EHLO, and at the completion of the call to the new process_message API.
Agreed.

I already implemented the things that are clear but am not sure how to
handle the first point about multiple HELO/EHLO.

- Milan
History
Date User Action Args
2014-06-13 23:45:34zvynsetrecipients: + zvyn, barry, pitrou, r.david.murray, jesstess
2014-06-13 23:45:34zvynlinkissue21725 messages
2014-06-13 23:45:34zvyncreate