Skip to content

feat: allow pass hostname in docker env#6

Open
tomerqodo wants to merge 2 commits intoclaude_claude_vs_qodo_base_feat_allow_pass_hostname_in_docker_env_pr6from
claude_claude_vs_qodo_head_feat_allow_pass_hostname_in_docker_env_pr6
Open

feat: allow pass hostname in docker env#6
tomerqodo wants to merge 2 commits intoclaude_claude_vs_qodo_base_feat_allow_pass_hostname_in_docker_env_pr6from
claude_claude_vs_qodo_head_feat_allow_pass_hostname_in_docker_env_pr6

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#6

Comment thread api/libs/smtp.py
Comment on lines +33 to +36
if self.use_tls and self.opportunistic_tls:
smtp.ehlo(local_host)
smtp.starttls()
smtp.ehlo(local_host)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 EHLO behavioral regression: when SMTP_LOCAL_HOSTNAME is not set (the default), the EHLO name changes from the SMTP server address (e.g., "smtp.gmail.com") to an empty string, which causes Python smtplib to fall back to socket.getfqdn(). In Docker containers, getfqdn() typically returns an unhelpful container hostname that SMTP servers may reject. Consider defaulting local_host to self.server (preserving the old behavior) when SMTP_LOCAL_HOSTNAME is not configured.

Extended reasoning...

What changed

The old code in api/libs/smtp.py explicitly passed self.server (the SMTP server address) as the EHLO name during opportunistic TLS:

smtp.ehlo(self.server)
smtp.starttls()
smtp.ehlo(self.server)

This was an intentional design choice, as the old code even had a comment: "Send EHLO command with the HELO domain name as the server address."

The new code replaces this with:

local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""
...
smtp.ehlo(local_host)
smtp.starttls()
smtp.ehlo(local_host)

How the regression manifests

When SMTP_LOCAL_HOSTNAME is not configured (the default for all existing users), dify_config.SMTP_LOCAL_HOSTNAME is None, so local_host becomes "". When "" is passed to smtp.ehlo(), Python's smtplib treats it as falsy and falls back to self.local_hostname, which is auto-detected via socket.getfqdn().

Step-by-step proof

  1. User upgrades Dify without setting SMTP_LOCAL_HOSTNAME (which is the default — the env var defaults to None in MailConfig).
  2. local_host = dify_config.SMTP_LOCAL_HOSTNAME or "" evaluates to "".
  3. smtp.ehlo("") is called. In Python's smtplib.SMTP.ehlo(), the implementation checks if name: self.ehlo_resp = ... name ... — since "" is falsy, it falls through to use self.local_hostname.
  4. self.local_hostname was set during SMTP.__init__() via local_hostname=local_host or None — since "" is also falsy, this passed None, causing smtplib to auto-detect via socket.getfqdn().
  5. In Docker containers, socket.getfqdn() typically returns the container ID or an unhelpful hostname like "buildkitsandbox" or "localhost".
  6. The SMTP server receives an EHLO with a container hostname instead of the expected server address, and may reject the connection.

Impact

This is a breaking behavioral change for all existing users who use opportunistic TLS and have not set SMTP_LOCAL_HOSTNAME. The old behavior sent the SMTP server's own address as the EHLO name (e.g., "smtp.gmail.com"), which servers universally accept. The new behavior sends whatever socket.getfqdn() returns, which in containerized environments is often an invalid or unresolvable hostname that SMTP servers may reject. This can silently break email delivery after upgrading.

Note that the local_hostname parameter is also passed to the SMTP/SMTP_SSL constructor on line 33 (local_hostname=local_host or None), which similarly changes behavior for the initial connection greeting — though the EHLO calls on lines 37-39 are the most impactful since they occur after STARTTLS.

Suggested fix

Default local_host to self.server when SMTP_LOCAL_HOSTNAME is not configured, preserving the old behavior:

local_host = dify_config.SMTP_LOCAL_HOSTNAME or self.server

This way, users who explicitly set SMTP_LOCAL_HOSTNAME get their custom value, while existing users retain the previous behavior without any configuration changes.

Comment thread api/libs/smtp.py
def send(self, mail: dict):
smtp = None
smtp: smtplib.SMTP | None = None
local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SMTPClient.send() reads dify_config.SMTP_LOCAL_HOSTNAME directly from global config (line 25) instead of receiving it as a constructor parameter. Every other SMTP setting (server, port, username, password, use_tls, opportunistic_tls) is read by ext_mail.py and passed to SMTPClient.__init__, but SMTP_LOCAL_HOSTNAME bypasses this pattern. This reduces testability (tests must mock dify_config or use ANY matchers) and prevents per-instance hostname configuration.

Extended reasoning...

What the bug is

The SMTPClient class in api/libs/smtp.py is designed as a library class that receives all its configuration through constructor parameters. The caller, extensions/ext_mail.py, reads config values from dify_config and passes them to the constructor: server, port, username, password, _from, use_tls, and opportunistic_tls (lines 49-57 of ext_mail.py).

However, the newly added SMTP_LOCAL_HOSTNAME setting breaks this pattern. Instead of being passed as a constructor parameter, it is read directly from dify_config inside the send() method at line 25 of smtp.py: local_host = dify_config.SMTP_LOCAL_HOSTNAME or "".

Why this is problematic

This creates a tight coupling between a library class (libs/smtp.py) and the global configuration system. The SMTPClient class now imports dify_config at the module level (line 6), which was not needed before this change. All other SMTP settings flow through the constructor, making the class self-contained and easy to test or reuse.

Step-by-step proof

  1. Look at ext_mail.py lines 49-57: SMTPClient is instantiated with server=dify_config.SMTP_SERVER, port=dify_config.SMTP_PORT, username=dify_config.SMTP_USERNAME, etc.
  2. Look at smtp.py line 25: local_host = dify_config.SMTP_LOCAL_HOSTNAME or "" — this is the only config value read directly inside the class rather than via a parameter.
  3. Look at the test changes: the tests had to switch from asserting exact local_hostname values to using ANY matchers (e.g., local_hostname=ANY), which is a direct consequence of the value being pulled from global config rather than controlled through the constructor.

Impact

The code works correctly at runtime — emails will be sent with the right hostname. However:

  • Tests cannot control local_hostname without mocking dify_config, reducing test precision (evidenced by the ANY matchers in the test diff).
  • SMTPClient cannot be reused outside of the Dify config system.
  • Per-instance hostname configuration is impossible if multiple SMTPClient instances are needed with different hostnames.

How to fix

Add local_hostname: str | None = None as a parameter to SMTPClient.__init__, store it as self.local_hostname, use self.local_hostname in send(), and have ext_mail.py pass dify_config.SMTP_LOCAL_HOSTNAME when constructing the client. Remove the from configs import dify_config import from smtp.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants