Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with smtp and STARTTLS/SMTPS #494

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

markkrj
Copy link

@markkrj markkrj commented Jul 3, 2019

Fixes #519

We used starttls by setting mail.smtp.starttls.enable=true in advanced settings. After 579c6ab it started failing.
I wen't on and checked the new code. I'm not a programmer, but I think that with this piece of code, the advanced settings that are behind this try block will never run when using plain text SMTP.

        try
        {
            if ( smtpServerType == SmtpServerType.SMTP )
            {
                return properties;
            }

I noticed also that if it was set to use START_TLS, it would also set SSL properties, which is wrong also.
It must be SSL (usually on port 465) or StartTLS (usually port 587). Never both.

As I'm not a programmer, I'm not sure that the small changes that I did are the correct approach for this issue but I tested and it worked for every scenarios that I thought would be more common as:

Plain SMTP with advanced mail.smtp.starttls.enable=true (As I was using before)
Plain SMTP with no starttls (usual no encryption, port 25, with auth).
SMTPS (with auth)
START_TLS (with auth)

Only scenarios that I didn't test are without authentication. But my patch didn't touch that.

SMTPS and STARTTLS doesn't work together. So this will test for the
selected config (SMTPS or STARTTLS) and put the correct mail.smtp properties.
@markkrj
Copy link
Author

markkrj commented Oct 21, 2019

@jrivard any comments on this? Not the proper solution? I tested every scenario and it actually fixed the bug. The current implementation is wrong as:

  • If using SMTP, advanced settings are not set.
  • If using STARTTLS, SSL properties are also set.
    Only scenario that works with current behavior is SSL (SMTPS).

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.

Email bug in StartTLS and advanced settings
1 participant