fix: validate SMTP port before connection to prevent 500/CommandError… - #9487
Open
sakthimahalakshmi-vc wants to merge 1 commit into
Open
fix: validate SMTP port before connection to prevent 500/CommandError…#9487sakthimahalakshmi-vc wants to merge 1 commit into
sakthimahalakshmi-vc wants to merge 1 commit into
Conversation
sakthimahalakshmi-vc
requested review from
dheeru0198 and
pablohashescobar
as code owners
July 28, 2026 10:14
|
|
Contributor
📝 WalkthroughWalkthroughSMTP port parsing and range validation were added to both email testing paths. Invalid or non-integer ports now produce explicit command errors or HTTP 400 responses before SMTP connection creation. ChangesSMTP port validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/db/management/commands/test_email.py`:
- Around line 29-34: Reject empty or unset SMTP port values in both validation
paths instead of defaulting them to 587. In
apps/api/plane/db/management/commands/test_email.py lines 29-34, parse
EMAIL_PORT unconditionally so empty values reach the existing CommandError
handler; apply the same validation in
apps/api/plane/license/api/views/configuration.py lines 107-115 so empty values
produce HTTP 400.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3b86ebe-f4b5-4ee7-801d-cdff668278fa
📒 Files selected for processing (2)
apps/api/plane/db/management/commands/test_email.pyapps/api/plane/license/api/views/configuration.py
Comment on lines
+29
to
+34
| try: | ||
| port = int(EMAIL_PORT) if EMAIL_PORT else 587 | ||
| if not (1 <= port <= 65535): | ||
| raise ValueError() | ||
| except (ValueError, TypeError): | ||
| raise CommandError("SMTP port is invalid. Please configure a valid SMTP port.") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject empty and unsaved SMTP ports in both paths.
Both implementations treat falsey values as the valid default 587, allowing an empty configuration to proceed instead of producing CommandError or HTTP 400.
apps/api/plane/db/management/commands/test_email.py#L29-L34: parseEMAIL_PORTunconditionally so empty values reach the existing error handler.apps/api/plane/license/api/views/configuration.py#L107-L115: apply the same validation so empty values return HTTP 400.
📍 Affects 2 files
apps/api/plane/db/management/commands/test_email.py#L29-L34(this comment)apps/api/plane/license/api/views/configuration.py#L107-L115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/db/management/commands/test_email.py` around lines 29 - 34,
Reject empty or unset SMTP port values in both validation paths instead of
defaulting them to 587. In apps/api/plane/db/management/commands/test_email.py
lines 29-34, parse EMAIL_PORT unconditionally so empty values reach the existing
CommandError handler; apply the same validation in
apps/api/plane/license/api/views/configuration.py lines 107-115 so empty values
produce HTTP 400.
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.
Description
This PR fixes an unhandled crash in the SMTP test-email flow.
In
EmailCredentialCheckEndpoint.post(apps/api/plane/license/api/views/configuration.py)and in the
test_emailmanagement command(apps/api/plane/db/management/commands/test_email.py), the SMTP port
was passed directly to
int(EMAIL_PORT)with no validation. IfEMAIL_PORT was empty or not yet saved, this raised an unhandled
ValueError, causing a 500 Internal Server Error (or an uncaught
exception in the management command) instead of a clear error message.
This PR adds validation that the port is a valid integer between
1 and 65535 before establishing the SMTP connection. If invalid,
the API now returns a 400 Bad Request with a descriptive message,
and the management command raises a CommandError instead of crashing.
Note: this PR is backend-only. The frontend email-config-form.tsx
may have a related UX issue (allowing "Send test email" before the
config is saved) that is out of scope here and can be tracked separately.
Type of Change
Screenshots and Media (if applicable)
N/A — backend-only fix, no UI changes.
Test Scenarios
with EMAIL_PORT unset/empty — previously returned a 500 error,
now returns a 400 with a clear validation message.
port — previously raised an unhandled exception, now raises a
CommandError with a clear message.
test email successfully, unaffected by this change.
References
Fixes #9472
Summary by CodeRabbit