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

set schema to smtps if MAIL_ENCRYPTION === tls #53749

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

danielrona
Copy link
Contributor

@danielrona danielrona commented Dec 4, 2024

This fixes the problem with the missing scheme in the config this Pull only added the validation but never added the config option

config/mail.php Outdated Show resolved Hide resolved
config/mail.php Outdated
@@ -38,6 +38,7 @@
'mailers' => [

'smtp' => [
'scheme' => env('MAIL_SCHEME', 'smtp'),
Copy link
Member

Choose a reason for hiding this comment

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

This will make scheme default to smtp instead of relying on port value as indicated in #53585

With this changes everyone using the following configuration:

MAIL_ENCRYPTION=tls
MAIL_PORT=465

Will resolved to stmp instead correctly using smtps and create more gotcha than what we correctly have with MAIL_ENCRYPTION.

Copy link
Contributor Author

@danielrona danielrona Dec 4, 2024

Choose a reason for hiding this comment

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

Right now everyone who has configured MAIL_ENCRYPTION=null has a broken configuration.

I also do not understand why we would be relying on a port to define a scheme that makes no sense.

Just because a port can be smtp or smtps doesn't mean it needs to be,
I can have port 25 be smpts or I can have port 587 be smtp this is totally up to the setup of the provider.

Yes the defaults would be smtp for 25 and smtps for 587 not to mention that 465 is a legacy default port for smtps nowadays it would/should be 587.

So right now this would still end up causing issues for most people and having a dedicated configuration parameter would resolve this.

You can setup a fresh laravel installation and it will fail due to not having the proper config entries.

MAIL_MAILER=smtp
MAIL_HOST=127.0.0.1
MAIL_PORT=9925
MAIL_USERNAME=null
MAIL_PASSWORD=null
MAIL_ENCRYPTION=null
MAIL_FROM_ADDRESS="[email protected]"
MAIL_FROM_NAME="${APP_NAME}"

TestMail is a empty view Mailabable

Mail::to('test@localhost')->send(new TestMail());

will fail with
The "" scheme is not supported; supported schemes for mailer "smtp" are: "smtp", "smtps".

Copy link
Member

Choose a reason for hiding this comment

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

I also do not understand why we would be relying on a port to define a scheme that makes no sense.

This is a requirement by Symfony Mailer. scheme here is only used to comply with Dsn instance but the result is then passed to EsmtpTransport differently.

https://github.com/symfony/mailer/blob/e4d358702fb66e4c8a2af08e90e7271a62de39cc/Transport/Smtp/EsmtpTransport.php#L56-L68

The "" scheme is not supported; supported schemes for mailer "smtp" are: "smtp", "smtps".

The fixed in #53585 has already fixed this and before the release can be done you should downgrade to 7.1 as suggested.

We will look into removing MAIL_ENCRYPTION and streamlining the configuration for next major version if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while #53585 might fix this for everyone using port 465 , this doesn't make much sense and it's still a breaking change because everyone with a port != 465 will have not smtps set on their configuration files while actually having MAIL_ENCRYPTION=tls

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/symfony/mailer/blob/7.2/CHANGELOG.md#440

encryption configuration was deprecated and removed in earlier symfony/mailer release. This probably being left out from swiftmailer era

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted my pull dropped the config changes and this would solve the issue, until MAIL_ENCRYPTION is removed and the configuration file streamlined with the next major version.

@danielrona danielrona changed the title add missing mailers.smtp.scheme to config/mail.php set schema to smtps if MAIL_ENCRYPTION === tls Dec 4, 2024
@taylorotwell taylorotwell merged commit c07f426 into laravel:11.x Dec 6, 2024
38 checks passed
browner12 pushed a commit to browner12/framework that referenced this pull request Dec 10, 2024
@vaclavekp
Copy link

vaclavekp commented Dec 10, 2024

hi, today I upgraded to v11.34.2, and this breaking change quite surprised me. Is this expected?

@danielrona
Copy link
Contributor Author

@vaclavekp if you just upgraded to v11.34.2 you still have the breaking change, this merge-pull fixed that.

You might want to consider upgrading to v.11.35.0 or apply this commit as a patch to v11.34.2 to fix your issue.

@eduarguz
Copy link

Got this error when trying to update using sendgrid.

Symfony\Component\Mailer\Exception\TransportException
Connection could not be established with host "ssl://smtp.sendgrid.net:587": stream_socket_client(): SSL operation failed with code 1. OpenSSL Error messages:
error:0A00010B:SSL routines::wrong version number

So, looks like I am working on default values which were in (laravel 10)

'mailers' => [
        'smtp' => [
                'port' => env('MAIL_PORT', 587),
                'encryption' => env('MAIL_ENCRYPTION', 'tls'),
        ],
]

Then, even though the default is tls, looks like the port is not what is used as default in laravel 11.

Will wait a little bit before upgrading, need to clarify if I should use tls or not, and what port should be used

@sgilberg
Copy link
Contributor

Same @eduarguz after updating to 11.35.0 I got the same error, looks like this is a breaking change for folks who have a mail port set to something other than 465 and mail encryption set to 'tls'. For now I've added 'scheme' => 'smtp' to my 'smtp' config and that seems to set it back the way it was.

@zeroesones-dev
Copy link

I also got an error with wrong version number for postmark. All I did for now, changing tls to starttls and it works now. But something tells me, this is only temporary fix.

@crynobone
Copy link
Member

I also got an error with wrong version number for postmark. All I did for now, changing tls to starttls and it works now. But something tells me, this is only temporary fix.

You mean MAIL_ENCRYPTION=starttls?

@danielrona
Copy link
Contributor Author

symfony/mailer will always use encryption when the server advertises STARTTLScapabilities

MAIL_ENCRYPTION=null would be the correct configuration in 99% of the use cases.

When the server does not advertise STARTTLS capabilities but you want the connection to be encrypted and the server does support TLS you would set MAIL_ENCRYPTION=tls
#53863 (comment)

In case anyone else stumbles upon this here.

@LogicSatinn
Copy link
Contributor

LogicSatinn commented Jan 2, 2025

symfony/mailer will always use encryption when the server advertises STARTTLScapabilities
MAIL_ENCRYPTION=null would be the correct configuration in 99% of the use cases.
When the server does not advertise STARTTLS capabilities but you want the connection to be encrypted and the server does support TLS you would set MAIL_ENCRYPTION=tls
#53863 (comment)

In case anyone else stumbles upon this here.

This still is a breaking change. I don't think this was well handled imo.

@jeffhuys
Copy link

jeffhuys commented Jan 9, 2025

This has also caused an outage for us - lots of registrations, password forgot, etc emails never sent.

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.

9 participants