gh-43311: Add regression test for SSLError masking SMTPDataError in rset() - #154866
gh-43311: Add regression test for SSLError masking SMTPDataError in rset()#154866zero-context wants to merge 2 commits into
Conversation
Sorry, something went wrong.
…r in rset() pythongh-43311 (bpo-1481032, filed 2006) reported that when sendmail() gets a rejected DATA command and calls the automatic _rset() cleanup, an SSLError from the dead connection could mask the original SMTPDataError. This was already fixed as a side effect of two later, unrelated changes: pythongh-63696 (bpo-17498) routes all three sendmail() error paths through _rset(), and ssl.SSLError has subclassed OSError since Python 3.3, so send()/getreply() already normalize a dead-connection SSLError into SMTPServerDisconnected before _rset()'s own except-clause ever sees it. No regression test previously covered the TLS-specific variant of this scenario (the existing test__rest_from_mail_cmd is plaintext-only). Adds one, verified against a deliberately broken build to confirm it actually catches the original masking behavior (narrowing send()'s and getreply()'s OSError handlers reproduces the exact 2006 symptom: a ConnectionResetError leaks out and replaces the SMTPDataError).
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead. |
Sorry, something went wrong.
|
|
||
|
|
||
| @unittest.skipUnless(SUPPORTS_SMTP_SSL, 'SSL not supported') | ||
| class SMTPSSLRsetAfterDataErrorTests(unittest.TestCase): |
There was a problem hiding this comment.
This looks like an un ecessary addition. Can't you just extend existing tests with existing infra? and please do not just ask an agent to do it. You must be able to explain the change yourself without any AI assistance.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes i built a tool to help write and test code. I verified it actually catches the regression by temporarily breaking the fix and confirming the test failed, then confirmed it passes against the current code. Happy to answer questions about what it's doing. Just thought it might help. If not please ignore. Cheers
Sorry, something went wrong.
There was a problem hiding this comment.
This does not seem a human answer. I do not want you to build a tool though?
Sorry, something went wrong.
There was a problem hiding this comment.
This also does not answer my request about this useless complication in the tests.
Sorry, something went wrong.
There was a problem hiding this comment.
Since SimSMTPServer is plaintext-only, I couldn't use it to test this TLS-specific bug. Adding TLS support to the existing server felt like overkill compared to just making a quick, dedicated class
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
This is a ~20-year-old ticket (filed 2006 as bpo-1481032) that turns
out to already be fixed — just never got a regression test or a
closing comment. @soreavis confirmed this 10 days ago
(#43311 (comment))
with a real TLS reproducer, and I independently re-verified it (written
separately, not copied) before adding this test.
The original bug: when sendmail()'s message gets rejected and it
calls the automatic _rset() cleanup, an error from the now-dead
connection during that rset() could mask the original
SMTPDataError — the caller would see a confusing connection error
instead of the actual rejection reason.
Why it's already fixed: two unrelated later changes closed it
together. gh-63696 (bpo-17498, 2013) routes all three of
sendmail()'s error paths through _rset() consistently. Separately,
ssl.SSLError has subclassed OSError since Python 3.3, so
send()/getreply()'s existing except OSError: handlers already
normalize a dead-connection error into SMTPServerDisconnected before
_rset()'s own except SMTPServerDisconnected: pass ever sees it —
the exact case a 2014 comment on the issue said couldn't be tested at
the time.
One correction along the way: both the earlier comment's
reproducer and my first attempt at this test had the same bug — the
fake server responded to DATA with a rejection code directly, which
makes smtplib.data() raise SMTPDataError immediately, before
sendmail() ever reaches its own self._rset() call. That path can't
demonstrate the masking at all, since _rset() is never invoked. The
actual vulnerable path only exists when the server accepts DATA with
354, receives the full message, and then rejects on the final
response — that's the branch in sendmail() that calls self._rset()
before raising SMTPDataError. Fixed the test to use that scenario.
Validated the test is meaningful, not a tautology: temporarily
narrowed send()'s and getreply()'s except OSError: handlers to
simulate the pre-fix world. The test correctly fails — a
ConnectionResetError leaks out and replaces the SMTPDataError —
confirming this test would have caught the original bug were it still
present.
No Misc/NEWS.d entry: test-only change per the devguide's own
Validationexceptions list, patchcheck agrees.
environment — OpenSSL wasn't available here at all until now).