Skip to content

feat: allow pass hostname in docker env#144

Open
tomerqodo wants to merge 2 commits intoqodo_action_req_1_base_feat_allow_pass_hostname_in_docker_env_pr6from
qodo_action_req_1_head_feat_allow_pass_hostname_in_docker_env_pr6
Open

feat: allow pass hostname in docker env#144
tomerqodo wants to merge 2 commits intoqodo_action_req_1_base_feat_allow_pass_hostname_in_docker_env_pr6from
qodo_action_req_1_head_feat_allow_pass_hostname_in_docker_env_pr6

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#6

@qodo-code-review
Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Unvalidated SMTP_LOCAL_HOSTNAME used 📘 Rule violation ⛨ Security
Description
SMTP_LOCAL_HOSTNAME is treated as an arbitrary external input and is used directly for EHLO
  without any validation/sanitization.
• When unset, local_host becomes an empty string and is still passed to smtp.ehlo(local_host),
  which can trigger SMTP protocol errors depending on server behavior.
• This violates the requirement to handle edge cases and validate external inputs before use.
Code

api/libs/smtp.py[R25-36]

+        local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""
        try:
-            if self.use_tls:
-                if self.opportunistic_tls:
-                    smtp = smtplib.SMTP(self.server, self.port, timeout=10)
-                    # Send EHLO command with the HELO domain name as the server address
-                    smtp.ehlo(self.server)
-                    smtp.starttls()
-                    # Resend EHLO command to identify the TLS session
-                    smtp.ehlo(self.server)
-                else:
-                    smtp = smtplib.SMTP_SSL(self.server, self.port, timeout=10)
-            else:
-                smtp = smtplib.SMTP(self.server, self.port, timeout=10)
+            # Use ternary to select SMTP class based on TLS mode
+            smtp = (smtplib.SMTP_SSL if (self.use_tls and not self.opportunistic_tls) else smtplib.SMTP)(
+                self.server, self.port, timeout=10, local_hostname=local_host or None
+            )
+
+            assert smtp is not None
+            if self.use_tls and self.opportunistic_tls:
+                smtp.ehlo(local_host)
+                smtp.starttls()
+                smtp.ehlo(local_host)
Evidence
The checklist requires explicit validation/sanitization of external inputs and explicit handling of
null/empty edge cases. The new code reads SMTP_LOCAL_HOSTNAME and uses it directly for ehlo()
while allowing the empty-string case.

Rule 6: Generic: Security-First Input Validation and Data Handling
Rule 3: Generic: Robust Error Handling and Edge Case Management
api/libs/smtp.py[23-36]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SMTP_LOCAL_HOSTNAME` (external config input) is used directly for `EHLO` and can be empty/invalid, causing SMTP handshake errors and failing the input validation requirement.

## Issue Context
Current behavior sets `local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""` and then calls `smtp.ehlo(local_host)` even when it is empty.

## Fix Focus Areas
- api/libs/smtp.py[23-36]
- api/configs/feature/__init__.py[952-956]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



ⓘ The new review experience is currently in Beta. Learn more

Qodo Logo

Comment on lines +25 to +36
local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""
try:
if self.use_tls:
if self.opportunistic_tls:
smtp = smtplib.SMTP(self.server, self.port, timeout=10)
# Send EHLO command with the HELO domain name as the server address
smtp.ehlo(self.server)
smtp.starttls()
# Resend EHLO command to identify the TLS session
smtp.ehlo(self.server)
else:
smtp = smtplib.SMTP_SSL(self.server, self.port, timeout=10)
else:
smtp = smtplib.SMTP(self.server, self.port, timeout=10)
# Use ternary to select SMTP class based on TLS mode
smtp = (smtplib.SMTP_SSL if (self.use_tls and not self.opportunistic_tls) else smtplib.SMTP)(
self.server, self.port, timeout=10, local_hostname=local_host or None
)

assert smtp is not None
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.

Action required

1. Unvalidated smtp_local_hostname used 📘 Rule violation ⛨ Security

SMTP_LOCAL_HOSTNAME is treated as an arbitrary external input and is used directly for EHLO
  without any validation/sanitization.
• When unset, local_host becomes an empty string and is still passed to smtp.ehlo(local_host),
  which can trigger SMTP protocol errors depending on server behavior.
• This violates the requirement to handle edge cases and validate external inputs before use.
Agent prompt
## Issue description
`SMTP_LOCAL_HOSTNAME` (external config input) is used directly for `EHLO` and can be empty/invalid, causing SMTP handshake errors and failing the input validation requirement.

## Issue Context
Current behavior sets `local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""` and then calls `smtp.ehlo(local_host)` even when it is empty.

## Fix Focus Areas
- api/libs/smtp.py[23-36]
- api/configs/feature/__init__.py[952-956]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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