fix: classify connect-stage SMTP errors with bounce rules - #497
Merged
Conversation
_onError() preset err.category='network' on every connect-stage failure, which
makes handleResponseError() take the preset-category branch and never call
bounces.check(). The bounce ruleset has therefore been dead at connect stage,
including the rule that names this exact response:
# 554 5.7.1 You are not allowed to connect
^554[ \-].* You are not allowed to connect,defer,blacklist,Sender IP blacklisted
Leave permanent (5xx) connect failures uncategorized so the rules classify them.
bounces.txt already ends in "^5\d\d,reject,other", so an unrecognized 5xx still
rejects per RFC 5321 4.2.1 without hardcoding that policy in JS, while the
specific rules above it apply -- including the 85 blacklist rules, which feed
disabledAddresses and rotate the source IP for the domain.
Transient failures keep category='network' and defer exactly as before. The 4xx
rules in bounces.txt answer for a response to an envelope, where "452 over quota"
is a final verdict; at connect time it is not one.
Refines #496, which routed every connect-stage 5xx to a hardcoded reject and so
skipped the blacklist rules, reimplementing the ruleset's own terminal rule in JS.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GFF2LP1xQR2yV9DB37wPyD
dragoangel
reviewed
Jul 17, 2026
| status: false | ||
| }; | ||
| } else if (!err.responseCode) { | ||
| } else if (!err.responseCode && !/^\d{3}\b/.test(err.response || err.message)) { |
Contributor
There was a problem hiding this comment.
@andris9 Hi, wanted to understand just for myself, is there any way we can get err.response || err.message with code and not get err.responseCode? Also err.message seems like usually have more details - more human readable thing - not original response from upstream/etc :)
Contributor
|
Thanks for getting this in right direction 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #496, which correctly diagnosed a real bug but fixed it one layer too high.
The problem
_onErrorpreseterr.category = 'network'on every connect-stage failure.handleResponseErrorshort-circuits on adns/network/policycategory, sobounces.check()has never run for connect-stage errors — the whole ruleset has been dead there, including the rule that names the exact response from #496 verbatim:That also explains both of the "catches" in #496: no
err.categoryfitted because the category was never supposed to be chosen in code, and the IP auto-switch "didn't work at this stage" becausecategory: 'blacklist'is exactly what drives it.The fix
Leave permanent (5xx) connect failures uncategorized so the rules classify them.
config/bounces.txtalready ends in:so an unrecognized 5xx still rejects per RFC 5321 §4.2.1 — #496's goal, kept — without hardcoding that policy in JS, while the 85 blacklist rules above it apply and rotate the source IP via
disabledAddresses.Transient failures keep
category: 'network'and defer as before. That boundary is deliberate: the 4xx rules answer for a response to an envelope, where452 over quotais final. At connect time it isn't, and 7 of 8 realistic greeting-stage 4xx responses would flip action if routed through them.Net effect at greeting:
554 ... You are not allowed to connectAlso reverts #496's
handleResponseErrorhunk: nodemailer already setserr.responseCodein_formatErrorbefore_onErrorruns, nothing downstream read the backfilled value, and the rewritten condition was equivalent to the original. The net production diff vs 3.10.17 is one hunk in_onError.Verified
handleResponseError:554 ... not allowed to connect→DEFERRED[blacklist]withcategory: blacklist+addressset, passing thesending-zone.js:350guard → IP rotates. Unrecognized 5xx →REJECTED[other].421→DEFERRED[network].err.responseCode/err.responsefrom the banner before_onErrorsees it.test/bounces-test.js(first coverage forbounces.check) pins the three rules this routing depends on. Suite + eslint green.Notes
_onErroralso covers HELO/LHLO, EHLO-with-requireTLS and STARTTLS, so a 5xx there routes through the rules too. Consistent with the intent.enforceTLSthe banner arrives before STARTTLS, so an on-path attacker can inject a 5xx that blacklists a pool IP for that domain forblacklist.ttl, where before it becamepolicy/reject. Bounded and per-domain (DomainConfig.getdeep-clones, so shared defaults can't be poisoned), and such an attacker can already fail the delivery outright — noting it so the tradeoff is deliberate.Thanks @dragoangel for the diagnosis and the production trace — the bug and its cause were both yours.
🤖 Generated with Claude Code
https://claude.ai/code/session_01GFF2LP1xQR2yV9DB37wPyD