Add HTTP CONNECT proxy support to SMTP service#125
Open
mithunbharadwaj wants to merge 7 commits intomainfrom
Open
Add HTTP CONNECT proxy support to SMTP service#125mithunbharadwaj wants to merge 7 commits intomainfrom
mithunbharadwaj wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HTTP CONNECT proxy support and related config; introduces ProxyConnectError; refactors recipient normalization and header construction; updates send() signature defaults; implements proxy-aware SMTP send flow with helper methods, retries, and cleanup. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as EmailOutputService
participant Proxy as HTTP_PROXY
participant SMTP as SMTP_SERVER
Client->>Service: send(email_to, body, ...)
Service->>Service: validate config, normalize recipients
alt proxy configured
Service->>Proxy: TCP connect to proxy_host:proxy_port
Service->>Proxy: HTTP CONNECT target_host:target_port (+ Proxy-Authorization)
Proxy-->>Service: 200 Connection Established
Service->>Service: wrap tunneled socket (optional TLS)
Service->>SMTP: SMTP handshake/auth over tunneled socket
else direct
Service->>SMTP: connect to smtp_host:smtp_port (TLS/STARTTLS as needed)
end
Service->>SMTP: MAIL FROM / RCPT TO (normalized recipients)
Service->>SMTP: DATA (message)
SMTP-->>Service: 250 OK
Service->>SMTP: QUIT
Service-->>Client: send result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
asabiris/output/smtp/service.py (3)
257-264: Add exception chaining for better debugging context.When re-raising as
ASABIrisError, the originalProxyConnectErrortraceback is lost. Useraise ... from eto preserve the exception chain.♻️ Proposed fix
- raise ASABIrisError( + raise ASABIrisError( ErrorCode.SMTP_CONNECTION_ERROR, tech_message="SMTP proxy connection failed: {}.".format(str(e)), error_i18n_key="Could not connect to SMTP for host '{{host}}'.", error_dict={ "host": self.Host, } - ) + ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 257 - 264, The ASABIrisError raised in the SMTP proxy connection failure block (using ASABIrisError and ErrorCode.SMTP_CONNECTION_ERROR) currently discards the original ProxyConnectError traceback; update the raise to chain the original exception by re-raising the ASABIrisError using "raise ... from e" so the original exception (variable e) is preserved with its traceback while keeping the same tech_message, error_i18n_key, and error_dict (including self.Host).
396-396: Consider adding IPv6 support for proxy connections.Using
socket.AF_INETlimits proxy connections to IPv4 only. If the proxy host resolves to an IPv6 address, this will fail.♻️ Proposed fix using getaddrinfo for dual-stack support
- sock_obj = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock_obj.setblocking(False) - loop = asyncio.get_running_loop() - try: + # Resolve address to support both IPv4 and IPv6 + addr_info = socket.getaddrinfo(self.ProxyHost, proxy_port, socket.AF_UNSPEC, socket.SOCK_STREAM) + if not addr_info: + raise ProxyConnectError("Could not resolve proxy host '{}'".format(self.ProxyHost)) + family, socktype, proto, _, sockaddr = addr_info[0] + sock_obj = socket.socket(family, socktype, proto) + sock_obj.setblocking(False) + loop = asyncio.get_running_loop() await asyncio.wait_for( - loop.sock_connect(sock_obj, (self.ProxyHost, proxy_port)), + loop.sock_connect(sock_obj, sockaddr), timeout=self.ProxyConnectTimeout )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` at line 396, The socket is created with socket.AF_INET which forces IPv4; update the proxy connection code (where sock_obj is created) to use socket.getaddrinfo(host, port, family=socket.AF_UNSPEC, type=socket.SOCK_STREAM) and iterate over returned address tuples, creating each socket with the returned ai_family (instead of socket.AF_INET), attempting connect and breaking on success, and ensuring failed sockets are closed and exceptions propagated; this enables IPv6/IPv4 dual-stack support while preserving existing connect logic around sock_obj.
414-426: Add exception chaining to preserve debugging context.Multiple
raise ProxyConnectError(...)statements withinexceptblocks lose the original exception traceback. Useraise ... from e(orfrom Noneif intentionally suppressing) to preserve context for debugging.♻️ Proposed fixes
except asyncio.TimeoutError: sock_obj.close() - raise ProxyConnectError("Timeout while connecting to proxy {}:{}".format(self.ProxyHost, proxy_port)) + raise ProxyConnectError("Timeout while connecting to proxy {}:{}".format(self.ProxyHost, proxy_port)) from None except Exception as e: sock_obj.close() - raise ProxyConnectError(str(e)) + raise ProxyConnectError(str(e)) from e try: status_line = header_bytes.split(b"\r\n", 1)[0].decode("iso-8859-1") status_code = int(status_line.split(" ", 2)[1]) except Exception as e: sock_obj.close() - raise ProxyConnectError("Malformed proxy CONNECT response: {}".format(str(e))) + raise ProxyConnectError("Malformed proxy CONNECT response: {}".format(str(e))) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 414 - 426, The except blocks around the proxy connection and response parsing should preserve original tracebacks by chaining exceptions: change "except asyncio.TimeoutError:" to "except asyncio.TimeoutError as e:" and raise ProxyConnectError(... ) from e; update the first "except Exception as e:" raise to "raise ProxyConnectError(str(e)) from e"; and update the response-parsing "except Exception as e:" raise to "raise ProxyConnectError('Malformed proxy CONNECT response: {}'.format(str(e))) from e" while keeping the sock_obj.close() calls and maintaining existing messages; reference the ProxyConnectError class, sock_obj close calls, header_bytes/status_line parsing and the status_code extraction in your edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asabiris/output/smtp/service.py`:
- Around line 177-185: The Cc header is being set even when email_cc is an empty
list (because earlier code normalizes None to []), causing msg['Cc'] = '' —
update the logic in the block handling email_cc (the variables email_cc,
formatted_email_cc, cc_recipients and the call to self.format_sender_info) so
that you only set msg['Cc'] when there are actual addresses: build
formatted_email_cc as you do, append addresses to cc_recipients, and then check
that formatted_email_cc (or cc_recipients) is non-empty before assigning
msg['Cc'] (i.e., use a truthy check like if formatted_email_cc: to avoid
creating an empty header).
- Around line 235-244: When a proxy socket is used (self.ProxyHost true and
socket created by _connect_via_http_proxy) the code currently sets
send_kwargs["hostname"] = None which breaks TLS SNI; change the proxy branch so
send_kwargs["sock"] = proxy_socket, send_kwargs["hostname"] = self.Host (not
None) and keep send_kwargs["port"] = None before calling aiosmtplib.send(msg,
**send_kwargs) so TLS certificate negotiation uses the target host.
---
Nitpick comments:
In `@asabiris/output/smtp/service.py`:
- Around line 257-264: The ASABIrisError raised in the SMTP proxy connection
failure block (using ASABIrisError and ErrorCode.SMTP_CONNECTION_ERROR)
currently discards the original ProxyConnectError traceback; update the raise to
chain the original exception by re-raising the ASABIrisError using "raise ...
from e" so the original exception (variable e) is preserved with its traceback
while keeping the same tech_message, error_i18n_key, and error_dict (including
self.Host).
- Line 396: The socket is created with socket.AF_INET which forces IPv4; update
the proxy connection code (where sock_obj is created) to use
socket.getaddrinfo(host, port, family=socket.AF_UNSPEC, type=socket.SOCK_STREAM)
and iterate over returned address tuples, creating each socket with the returned
ai_family (instead of socket.AF_INET), attempting connect and breaking on
success, and ensuring failed sockets are closed and exceptions propagated; this
enables IPv6/IPv4 dual-stack support while preserving existing connect logic
around sock_obj.
- Around line 414-426: The except blocks around the proxy connection and
response parsing should preserve original tracebacks by chaining exceptions:
change "except asyncio.TimeoutError:" to "except asyncio.TimeoutError as e:" and
raise ProxyConnectError(... ) from e; update the first "except Exception as e:"
raise to "raise ProxyConnectError(str(e)) from e"; and update the
response-parsing "except Exception as e:" raise to "raise
ProxyConnectError('Malformed proxy CONNECT response: {}'.format(str(e))) from e"
while keeping the sock_obj.close() calls and maintaining existing messages;
reference the ProxyConnectError class, sock_obj close calls,
header_bytes/status_line parsing and the status_code extraction in your edits.
asabiris/output/smtp/service.py
Outdated
Comment on lines
+235
to
+244
| if self.ProxyHost: | ||
| proxy_socket = await self._connect_via_http_proxy() | ||
| send_kwargs["sock"] = proxy_socket | ||
| send_kwargs["hostname"] = None | ||
| send_kwargs["port"] = None | ||
| else: | ||
| send_kwargs["hostname"] = self.Host | ||
| send_kwargs["port"] = int(self.Port) if self.Port != "" else None | ||
|
|
||
| result = await aiosmtplib.send(msg, **send_kwargs) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
aiosmtplib send sock parameter hostname port documentation
💡 Result:
aiosmtplib.send() accepts both hostname/port and a sock parameter, but sock is mutually exclusive with port (and socket_path).
- Signature (current docs):
aiosmtplib.send(..., hostname='localhost', port=None, ..., socket_path=None, sock=None)[1] sockmeaning: “An existing, connected socket object… Passing a socket object will transfer control of it to the asyncio connection, and it will be closed when the client disconnects.” Also: not compatible withportorsocket_path. [1]- The same rule applies to the lower-level
SMTP(..., sock=...)/SMTP.connect(sock=...): ifsockis given, you should not providehostname,port, orsocket_path(andsocket_pathis also incompatible withport/sock). [2]
Sources:
[1] aiosmtplib API reference (send coroutine parameters) (aiosmtplib.readthedocs.io)
[2] aiosmtplib smtp.py module docs/source (mutual exclusivity and connect behavior) (aiosmtplib.readthedocs.io)
Citations:
- 1: https://aiosmtplib.readthedocs.io/en/stable/reference.html?utm_source=openai
- 2: https://aiosmtplib.readthedocs.io/en/stable/_modules/aiosmtplib/smtp.html?utm_source=openai
🏁 Script executed:
# Search for aiosmtplib import and send() usage in the repository
fd -e py -e txt | head -20 | xargs -I {} sh -c 'grep -l "aiosmtplib" {} 2>/dev/null'Repository: TeskaLabs/asab-iris
Length of output: 47
🏁 Script executed:
# Check the actual file to see how send_kwargs is built and used
cat -n "asabiris/output/smtp/service.py" | sed -n '200,250p'Repository: TeskaLabs/asab-iris
Length of output: 2016
🏁 Script executed:
# Search for aiosmtplib version requirements
fd -E node_modules \( -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "poetry.lock" -o -name "Pipfile" \) -type fRepository: TeskaLabs/asab-iris
Length of output: 293
🏁 Script executed:
# Try to find aiosmtplib version or requirements
find . -type f \( -name "*.txt" -o -name "*.toml" -o -name "*.cfg" -o -name "*.py" \) | xargs grep -l "aiosmtplib" 2>/dev/null | head -5Repository: TeskaLabs/asab-iris
Length of output: 115
🌐 Web query:
aiosmtplib send None port hostname values behavior mutually exclusive
💡 Result:
- In
aiosmtplib.send(...), the documented defaults arehostname="localhost"andport=None(whereportthen defaults to 465 ifuse_tls=True, 587 ifstart_tls=True, otherwise 25). [1] - If you explicitly pass
hostname=Noneand you are not usingsockorsocket_path, the client will still replace it with"localhost"duringconnect(). [2] - If you explicitly pass
port=Noneand you are not usingsockorsocket_path, the client will pick the default port (465/587/25) duringconnect(). [2] - Mutual-exclusion / incompatibility rules relevant to
hostname/port:sockandsocket_pathare mutually exclusive. [3]- If
sockorsocket_pathis used, thenportmust beNone(i.e., don’t provide a port). [3] - If
sock/socket_pathis used withuse_tls=True, thenhostnamemust not beNone(it’s required for TLS SNI/cert handling). [3] - (
senddocs also statesocket_path/sockare “not compatible with port”.) [1]
Sources: aiosmtplib API reference for send [1], and the SMTP.connect() / _validate_config() implementation in the docs-hosted source [2][3].
🏁 Script executed:
cat requirements.txt | grep aiosmtplibRepository: TeskaLabs/asab-iris
Length of output: 83
🏁 Script executed:
# Check the SSL/TLS context and what hostname should be for proxy + TLS
cat -n "asabiris/output/smtp/service.py" | sed -n '220,245p'Repository: TeskaLabs/asab-iris
Length of output: 1095
🏁 Script executed:
# Look for how self.Host and self.SSL are defined or documented
rg -A 3 -B 3 "(self\.SSL|self\.Host|self\.ProxyHost)" "asabiris/output/smtp/service.py" | head -40Repository: TeskaLabs/asab-iris
Length of output: 1950
Set hostname to self.Host when using proxy with TLS enabled.
When using a pre-connected socket via HTTP proxy with TLS/SSL enabled (use_tls=True), aiosmtplib requires hostname to be set for TLS SNI (Server Name Indication) certificate negotiation. Currently, the code unconditionally sets hostname=None in the proxy path, which will fail at runtime when both proxy and SSL are configured.
The fix is to set hostname to the target SMTP server (self.Host) even when using a proxy socket:
if self.ProxyHost:
proxy_socket = await self._connect_via_http_proxy()
send_kwargs["sock"] = proxy_socket
send_kwargs["hostname"] = self.Host # Required for TLS SNI with proxy
send_kwargs["port"] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/output/smtp/service.py` around lines 235 - 244, When a proxy socket
is used (self.ProxyHost true and socket created by _connect_via_http_proxy) the
code currently sets send_kwargs["hostname"] = None which breaks TLS SNI; change
the proxy branch so send_kwargs["sock"] = proxy_socket, send_kwargs["hostname"]
= self.Host (not None) and keep send_kwargs["port"] = None before calling
aiosmtplib.send(msg, **send_kwargs) so TLS certificate negotiation uses the
target host.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
asabiris/output/smtp/service.py (1)
177-185:⚠️ Potential issue | 🟡 MinorAvoid emitting an empty
Ccheader.At Line 177,
email_cchas already been normalized to[], soif email_cc is not None:is always true and can producemsg['Cc'] = ''.Proposed fix
- if email_cc is not None: + if email_cc: assert isinstance(email_cc, list) formatted_email_cc = [] for email_address in email_cc: _, sender_email = self.format_sender_info(str(email_address)) if sender_email: formatted_email_cc.append(sender_email) cc_recipients.append(sender_email) - msg['Cc'] = ', '.join(formatted_email_cc) + if formatted_email_cc: + msg['Cc'] = ', '.join(formatted_email_cc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 177 - 185, The Cc header can be set to an empty string because email_cc is normalized to [] and the code uses "if email_cc is not None:"; update the logic in the block that builds formatted_email_cc (around variables email_cc, formatted_email_cc, cc_recipients and the call to self.format_sender_info) to only assign msg['Cc'] when formatted_email_cc is non-empty (e.g., check if formatted_email_cc before doing msg['Cc'] = ', '.join(...)), and keep populating cc_recipients as before.
🧹 Nitpick comments (2)
asabiris/output/smtp/service.py (2)
222-223: Remove deadproxy_socketcleanup in retry loop.
proxy_socketis initialized but never assigned in this scope; cleanup here is unreachable after moving proxy handling into_send_via_proxy_smtp_client().Proposed cleanup
- for attempt in range(retry_attempts): - proxy_socket = None + for attempt in range(retry_attempts): try: if self.ProxyHost: result = await self._send_via_proxy_smtp_client( msg=msg, sender=sender, recipients=to_recipients + cc_recipients + bcc_recipients ) ... - finally: - if proxy_socket is not None: - try: - proxy_socket.close() - except OSError: - passAlso applies to: 348-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 222 - 223, Remove the dead proxy_socket variable and its cleanup inside the retry loop: delete the proxy_socket = None initialization and any finally/cleanup block that checks or closes proxy_socket within the retry logic, since proxy handling was moved into _send_via_proxy_smtp_client(); instead rely on _send_via_proxy_smtp_client() to open/close proxy sockets and ensure no leftover references to proxy_socket remain in the retry loop (also remove the duplicate cleanup occurrence that mirrors this pattern elsewhere in the same function).
387-395: Don't silently swallow cleanup failures.At Lines 387-395, broad
except Exception: passhides operational signals during quit/close cleanup. Prefer scoped exceptions and debug logging.Proposed refactor
finally: if client is not None: try: await client.quit() - except Exception: - pass + except aiosmtplib.errors.SMTPException: + L.debug("Ignoring SMTP client quit failure", exc_info=True) try: proxy_socket.close() - except Exception: - pass + except OSError: + L.debug("Ignoring proxy socket close failure", exc_info=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 387 - 395, The cleanup code currently swallows all exceptions from client.quit() and proxy_socket.close() with bare "except Exception: pass"; change these to catch only appropriate exceptions (e.g., smtplib.SMTPException or OSError/IOError) and avoid swallowing asyncio.CancelledError/KeyboardInterrupt (re-raise them), and log any caught error at debug or warning level using the module/class logger (referencing client.quit() and proxy_socket.close()) so failures are visible during debugging while non-operational exceptions propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asabiris/output/smtp/service.py`:
- Around line 421-428: The current CONNECT-header reader (uses buffer,
loop.sock_recv, ProxyConnectError, max_headers_size) can over-read and drop SMTP
bytes arriving in the same recv; change the function so after detecting
b"\r\n\r\n" it splits buffer = buffer.split(b"\r\n\r\n", 1) and returns both the
header part and the leftover bytes (instead of only headers), or store the
leftover on the socket wrapper so callers (the SMTP handshake reader) can
consume the leftover bytes; update the function signature and its callers to
accept/handle (headers, remainder) so no SMTP bytes are discarded.
---
Duplicate comments:
In `@asabiris/output/smtp/service.py`:
- Around line 177-185: The Cc header can be set to an empty string because
email_cc is normalized to [] and the code uses "if email_cc is not None:";
update the logic in the block that builds formatted_email_cc (around variables
email_cc, formatted_email_cc, cc_recipients and the call to
self.format_sender_info) to only assign msg['Cc'] when formatted_email_cc is
non-empty (e.g., check if formatted_email_cc before doing msg['Cc'] = ',
'.join(...)), and keep populating cc_recipients as before.
---
Nitpick comments:
In `@asabiris/output/smtp/service.py`:
- Around line 222-223: Remove the dead proxy_socket variable and its cleanup
inside the retry loop: delete the proxy_socket = None initialization and any
finally/cleanup block that checks or closes proxy_socket within the retry logic,
since proxy handling was moved into _send_via_proxy_smtp_client(); instead rely
on _send_via_proxy_smtp_client() to open/close proxy sockets and ensure no
leftover references to proxy_socket remain in the retry loop (also remove the
duplicate cleanup occurrence that mirrors this pattern elsewhere in the same
function).
- Around line 387-395: The cleanup code currently swallows all exceptions from
client.quit() and proxy_socket.close() with bare "except Exception: pass";
change these to catch only appropriate exceptions (e.g., smtplib.SMTPException
or OSError/IOError) and avoid swallowing
asyncio.CancelledError/KeyboardInterrupt (re-raise them), and log any caught
error at debug or warning level using the module/class logger (referencing
client.quit() and proxy_socket.close()) so failures are visible during debugging
while non-operational exceptions propagate.
Comment on lines
+421
to
+428
| while b"\r\n\r\n" not in buffer: | ||
| chunk = await loop.sock_recv(sock_obj, 4096) | ||
| if not chunk: | ||
| raise ProxyConnectError("Proxy closed connection before CONNECT response") | ||
| buffer += chunk | ||
| if len(buffer) > max_headers_size: | ||
| raise ProxyConnectError("Proxy CONNECT response headers are too large") | ||
| return buffer.split(b"\r\n\r\n", 1)[0] |
There was a problem hiding this comment.
Proxy CONNECT header reader can drop SMTP bytes.
At Lines 421-428, reading in 4096-byte chunks and returning only the part before \r\n\r\n can discard bytes already belonging to the SMTP stream (if they arrive in the same recv). That can cause intermittent handshake/read failures.
Proposed fix (avoid over-read)
async def _read_http_headers(self, sock_obj):
loop = asyncio.get_running_loop()
buffer = b""
max_headers_size = 65536
- while b"\r\n\r\n" not in buffer:
- chunk = await loop.sock_recv(sock_obj, 4096)
+ header_terminator = b"\r\n\r\n"
+ while not buffer.endswith(header_terminator):
+ chunk = await loop.sock_recv(sock_obj, 1)
if not chunk:
raise ProxyConnectError("Proxy closed connection before CONNECT response")
buffer += chunk
if len(buffer) > max_headers_size:
raise ProxyConnectError("Proxy CONNECT response headers are too large")
- return buffer.split(b"\r\n\r\n", 1)[0]
+ return buffer[:-len(header_terminator)]🧰 Tools
🪛 Ruff (0.15.2)
[warning] 424-424: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 427-427: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/output/smtp/service.py` around lines 421 - 428, The current
CONNECT-header reader (uses buffer, loop.sock_recv, ProxyConnectError,
max_headers_size) can over-read and drop SMTP bytes arriving in the same recv;
change the function so after detecting b"\r\n\r\n" it splits buffer =
buffer.split(b"\r\n\r\n", 1) and returns both the header part and the leftover
bytes (instead of only headers), or store the leftover on the socket wrapper so
callers (the SMTP handshake reader) can consume the leftover bytes; update the
function signature and its callers to accept/handle (headers, remainder) so no
SMTP bytes are discarded.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
asabiris/output/smtp/service.py (2)
389-396: Avoid silent failures in proxy cleanup paths.Swallowing exceptions without logging here makes intermittent socket/client shutdown issues hard to diagnose.
🔎 Suggested improvement
finally: if client is not None: try: await client.quit() - except Exception: - pass + except Exception as e: + L.debug("Failed to quit SMTP proxy client: %s", e) try: proxy_socket.close() - except Exception: - pass + except Exception as e: + L.debug("Failed to close SMTP proxy socket: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 389 - 396, The cleanup block currently swallows exceptions from client.quit() and proxy_socket.close(); instead catch Exception as e and log the failure so shutdown issues are visible. Update the try/except around client.quit() and proxy_socket.close() to log the exception (e.g., logger.exception(...) or self.logger.error(..., exc_info=True)) including context like "error quitting SMTP client" and "error closing proxy_socket", preserving the existing control flow after logging. Ensure you reference the same logger used in this module/class (e.g., logger or self.logger) so logs are consistent and searchable.
223-223: Remove deadproxy_socketcleanup insend().
proxy_socketis initialized but never assigned in this scope, so thefinallyclose block is effectively dead code and misleading.🧹 Suggested cleanup
for attempt in range(retry_attempts): - proxy_socket = None try: if self.ProxyHost: result = await self._send_via_proxy_smtp_client( msg=msg, sender=sender, recipients=to_recipients + cc_recipients + bcc_recipients ) @@ - finally: - if proxy_socket is not None: - try: - proxy_socket.close() - except OSError: - passAlso applies to: 349-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` at line 223, In send() the local variable proxy_socket is initialized but never assigned, and the finally block that attempts to close it is dead/misleading; remove the unused proxy_socket initialization and the finally/cleanup code that closes proxy_socket (or, if proxy_socket is supposed to be created elsewhere, move its creation into this scope and keep the cleanup). Apply the same change to the other similar block referenced (around lines 349-354) so no unreachable proxy_socket cleanup remains; look for proxy_socket references in send() and the other send-related method and either remove the unused variable and its close call or properly scope creation and cleanup together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asabiris/output/smtp/service.py`:
- Around line 72-77: The constructor currently only checks presence of
ProxyHost/ProxyPort but not their validity; update the initialization logic
around ProxyHost, ProxyPort and ProxyConnectTimeout (symbols: ProxyHost,
ProxyPort, ProxyConnectTimeout) to parse and validate proxy_port as an integer
in the valid TCP port range (1–65535) and ensure ProxyConnectTimeout is a
positive integer (>0); if parsing fails or values are out of range, raise
ValueError with a clear message so the service fails fast at startup. Apply the
same validation to the corresponding alternative initialization block referenced
around the other occurrence (lines ~438-440) to keep both code paths consistent.
---
Nitpick comments:
In `@asabiris/output/smtp/service.py`:
- Around line 389-396: The cleanup block currently swallows exceptions from
client.quit() and proxy_socket.close(); instead catch Exception as e and log the
failure so shutdown issues are visible. Update the try/except around
client.quit() and proxy_socket.close() to log the exception (e.g.,
logger.exception(...) or self.logger.error(..., exc_info=True)) including
context like "error quitting SMTP client" and "error closing proxy_socket",
preserving the existing control flow after logging. Ensure you reference the
same logger used in this module/class (e.g., logger or self.logger) so logs are
consistent and searchable.
- Line 223: In send() the local variable proxy_socket is initialized but never
assigned, and the finally block that attempts to close it is dead/misleading;
remove the unused proxy_socket initialization and the finally/cleanup code that
closes proxy_socket (or, if proxy_socket is supposed to be created elsewhere,
move its creation into this scope and keep the cleanup). Apply the same change
to the other similar block referenced (around lines 349-354) so no unreachable
proxy_socket cleanup remains; look for proxy_socket references in send() and the
other send-related method and either remove the unused variable and its close
call or properly scope creation and cleanup together.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
asabiris/output/smtp/service.py (1)
367-389:⚠️ Potential issue | 🟠 MajorProxy implicit-TLS path is still misconfigured for SNI/cert validation.
At Line 382,
hostname=Noneis used with a pre-connected proxy socket. Combined withuse_tls=self.SSL(Line 384), this can break TLS hostname/SNI behavior and is why secure config is being hard-blocked at Lines 367-375. Sethostname=self.Hostin the SMTP client config and remove the forced insecure fallback.🔧 Proposed fix
async def _send_via_proxy_smtp_client(self, *, msg, sender, recipients): - if self.SSL and self.ValidateCerts: - raise ASABIrisError( - ErrorCode.INVALID_SERVICE_CONFIGURATION, - tech_message=( - "Proxy with implicit TLS and certificate validation is not supported with current SMTP client " - "version. Use STARTTLS or disable certificate validation." - ), - error_i18n_key="Unsupported SMTP proxy TLS configuration." - ) - proxy_socket = await self._connect_via_http_proxy() client = None try: client = aiosmtplib.SMTP( sock=proxy_socket, - hostname=None, + hostname=self.Host, port=None, use_tls=self.SSL, start_tls=False, validate_certs=self.ValidateCerts, cert_bundle=self.Cert or None, timeout=self.ProxyConnectTimeout )In the latest aiosmtplib docs, for SMTP(sock=<connected socket>, use_tls=True), is hostname required/non-None for TLS/SNI and certificate validation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 367 - 389, The SMTP client is created with hostname=None which breaks TLS SNI and certificate validation when using a pre-connected proxy socket; in the block that calls self._connect_via_http_proxy() and constructs aiosmtplib.SMTP (currently passing hostname=None and use_tls=self.SSL), replace hostname=None with hostname=self.Host so the TLS handshake uses the real host for SNI/cert checks (leave use_tls=self.SSL, validate_certs=self.ValidateCerts and cert_bundle=self.Cert as-is); ensure this change is applied in the code that constructs the SMTP client (the aiosmtplib.SMTP call) so the earlier strict configuration check for implicit TLS will no longer be required.
🧹 Nitpick comments (3)
asabiris/output/smtp/service.py (3)
233-234:proxy_socketinsend()is dead cleanup state.
proxy_socketis initialized in Line 233 but never assigned in this scope; thefinallyclose block (Lines 359-364) is effectively no-op and can be removed for clarity.Also applies to: 359-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 233 - 234, In send() the local variable proxy_socket is initialized but never assigned or used, so the final cleanup block that attempts to close proxy_socket is dead code; remove the unused proxy_socket declaration and the corresponding finally/close block (the finally that references proxy_socket) to simplify the function, or if proxy behavior was intended, instead ensure proxy_socket is assigned where the proxy is created (and closed in the finally), referencing the same proxy_socket symbol consistently.
400-408: Cleanup failures are silently swallowed.Lines 403-404 and 407-408 use broad
except Exception: pass, which hides teardown issues during proxy SMTP failures.🛠️ Suggested change
finally: if client is not None: try: await client.quit() - except Exception: - pass + except Exception as e: + L.debug("SMTP proxy client quit failed: %s", e) try: proxy_socket.close() - except Exception: - pass + except Exception as e: + L.debug("Proxy socket close failed: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/output/smtp/service.py` around lines 400 - 408, The cleanup blocks currently swallow all exceptions for client.quit() and proxy_socket.close(); replace the bare "except Exception: pass" with explicit exception capture and logging so teardown failures are visible. In the block handling client (where client is not None and await client.quit() is called) catch Exception as e and call the module/class logger (e.g., logger.exception(...) or processLogger.error(...) with the exception) to record the error and stacktrace; do the same for proxy_socket.close() (catch Exception as e and log it). Ensure you reference the existing logger variable used elsewhere in this module (or use logging.getLogger(__name__) if none) and do not change the control flow beyond logging (no silencing).
100-109:send()signature does not match theOutputABCcontract, but actual callers have been updated.The method violates the abstract base class (line 17 of
asabiris/output_abc.pydefinessend(self, to, body_html, attachment=None, cc=None, bcc=None, subject=None)), yet all found callsites inasabiris/orchestration/sendemail.pyalready use the new keyword-only parameters (email_to,body,email_cc, etc.). No breaking positional calls or uses of old parameter names were found.This signature mismatch is a design concern if
SmtpServiceis used polymorphically through theOutputABCinterface, but the practical breaking change risk is low since all existing callers have been updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@asabiris/output/smtp/service.py`:
- Around line 367-389: The SMTP client is created with hostname=None which
breaks TLS SNI and certificate validation when using a pre-connected proxy
socket; in the block that calls self._connect_via_http_proxy() and constructs
aiosmtplib.SMTP (currently passing hostname=None and use_tls=self.SSL), replace
hostname=None with hostname=self.Host so the TLS handshake uses the real host
for SNI/cert checks (leave use_tls=self.SSL, validate_certs=self.ValidateCerts
and cert_bundle=self.Cert as-is); ensure this change is applied in the code that
constructs the SMTP client (the aiosmtplib.SMTP call) so the earlier strict
configuration check for implicit TLS will no longer be required.
---
Nitpick comments:
In `@asabiris/output/smtp/service.py`:
- Around line 233-234: In send() the local variable proxy_socket is initialized
but never assigned or used, so the final cleanup block that attempts to close
proxy_socket is dead code; remove the unused proxy_socket declaration and the
corresponding finally/close block (the finally that references proxy_socket) to
simplify the function, or if proxy behavior was intended, instead ensure
proxy_socket is assigned where the proxy is created (and closed in the finally),
referencing the same proxy_socket symbol consistently.
- Around line 400-408: The cleanup blocks currently swallow all exceptions for
client.quit() and proxy_socket.close(); replace the bare "except Exception:
pass" with explicit exception capture and logging so teardown failures are
visible. In the block handling client (where client is not None and await
client.quit() is called) catch Exception as e and call the module/class logger
(e.g., logger.exception(...) or processLogger.error(...) with the exception) to
record the error and stacktrace; do the same for proxy_socket.close() (catch
Exception as e and log it). Ensure you reference the existing logger variable
used elsewhere in this module (or use logging.getLogger(__name__) if none) and
do not change the control flow beyond logging (no silencing).
asabiris/output/smtp/service.py
Outdated
| struct_data={"proxy_host": self.ProxyHost, "proxy_port": self.ProxyPort, "host": self.Host} | ||
| ) | ||
| if attempt < retry_attempts - 1: | ||
| L.info("Retrying email send after proxy connection failure, attempt {}".format(attempt + 1)) |
There was a problem hiding this comment.
Please use L.log(asab.LOG_NOTICE... instead of info
| "host": self.Host, | ||
| } | ||
| ) | ||
| except ASABIrisError: |
| error_i18n_key="Unsupported SMTP proxy TLS configuration." | ||
| ) | ||
|
|
||
| proxy_socket = await self._connect_via_http_proxy() |
There was a problem hiding this comment.
Please add comments here so people now that in order to use proxy a new socket needs to be created
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation