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
…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 |
|
|
||
|
|
||
| @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.
|
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 |
2 participants
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.
@soreavisconfirmed 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 itcalls the automatic
_rset()cleanup, an error from the now-deadconnection during that
rset()could mask the originalSMTPDataError— the caller would see a confusing connection errorinstead 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 ofsendmail()'s error paths through_rset()consistently. Separately,ssl.SSLErrorhas subclassedOSErrorsince Python 3.3, sosend()/getreply()'s existingexcept OSError:handlers alreadynormalize a dead-connection error into
SMTPServerDisconnectedbefore_rset()'s ownexcept SMTPServerDisconnected: passever 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
DATAwith a rejection code directly, whichmakes
smtplib.data()raiseSMTPDataErrorimmediately, beforesendmail()ever reaches its ownself._rset()call. That path can'tdemonstrate the masking at all, since
_rset()is never invoked. Theactual vulnerable path only exists when the server accepts
DATAwith354, receives the full message, and then rejects on the finalresponse — that's the branch in
sendmail()that callsself._rset()before raising
SMTPDataError. Fixed the test to use that scenario.Validated the test is meaningful, not a tautology: temporarily
narrowed
send()'s andgetreply()'sexcept OSError:handlers tosimulate the pre-fix world. The test correctly fails — a
ConnectionResetErrorleaks out and replaces theSMTPDataError—confirming this test would have caught the original bug were it still
present.
No
Misc/NEWS.dentry: test-only change per the devguide's ownexceptions list,
patchcheckagrees.Validation
main.test_smtplib.py: 86 tests, 0 failures.test_ssl: 211 tests, 0 failures (also newly buildable in thisenvironment — OpenSSL wasn't available here at all until now).
Tools/patchcheck/patchcheck.py: clean.