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

[Mailer] Remove line breaks in email attachment content with Sendgrid API #33672

Merged
merged 1 commit into from Jan 4, 2020

Conversation

fyfey
Copy link

@fyfey fyfey commented Sep 23, 2019

Line breaks are not allowed in attachment content when sending over the
API.

Q A
Branch? 4.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #33671, Closes #32645
License MIT
Doc PR

This is a fix for #33671. Send grid's API throws a 400 error when sending email attachments with default base64 encoding.
Removing the line breaks resolves this issue.

$email = new Email();
$email->from('foo@example.com')
->to('bar@example.com')
->attach('Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod', 'lorem.txt');
Copy link
Member

Choose a reason for hiding this comment

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

There is no line-break in this content, so this is not testing your patch.

Copy link
Member

Choose a reason for hiding this comment

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

@stof it does, line-breaks are added by the MIME-compliant base64 encoder:

return $firstLine.trim(chunk_split($encodedString, $maxLineLength, "\r\n"));

Copy link
Member

Choose a reason for hiding this comment

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

This is so confusing that we should add a comment before the ->attach() method. Something like:

// even it content doesn't include new lines, the Base64 encoding performed later may add them
->attach('...)

@chalasr chalasr added this to the 4.3 milestone Sep 23, 2019
$email = new Email();
$email->from('foo@example.com')
->to('bar@example.com')
->attach('Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod', 'lorem.txt');
Copy link
Member

Choose a reason for hiding this comment

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

This is so confusing that we should add a comment before the ->attach() method. Something like:

// even it content doesn't include new lines, the Base64 encoding performed later may add them
->attach('...)

@stof
Copy link
Member

stof commented Sep 24, 2019

Maybe the actual issue is the usage of bodyToString on the DataPart, relying on the base64 encoding built for SMTP while this is sent to an HTTP API.

Btw, this may also impact other API providers.

@fabpot
Copy link
Member

fabpot commented Sep 24, 2019

@stof is right. We had the same kind of issue with SmtpEnvelope which was optimized for SMTP. We fixed that in 4.4 by renaming it to Envelope but more importantly moved the SMTP logic to the SMTP classes. Maybe something we need to do here as well. I haven't looked at the code to have an idea on how to do it properly though.

@chalasr
Copy link
Member

chalasr commented Sep 24, 2019

The SendGrid HTTP API requires attachments to be base64 encoded though (but with "strict" base64 i.e. no line-break). But API transports could indeed bypass the encoding system and use base64_encode() directly if needed, that's what I propose in #32645 for the very same bug.

@nicolas-grekas nicolas-grekas changed the title Remove line breaks in email attachment content. [Mailer] Remove line breaks in email attachment content. Jan 4, 2020
@nicolas-grekas nicolas-grekas changed the title [Mailer] Remove line breaks in email attachment content. [Mailer] Remove line breaks in email attachment content Jan 4, 2020
@nicolas-grekas nicolas-grekas changed the title [Mailer] Remove line breaks in email attachment content [Mailer] Remove line breaks in email attachment content to Sendgrid API Jan 4, 2020
@nicolas-grekas nicolas-grekas changed the title [Mailer] Remove line breaks in email attachment content to Sendgrid API [Mailer] Remove line breaks in email attachment content with Sendgrid API Jan 4, 2020
@nicolas-grekas
Copy link
Member

Thank you @fyfey.

nicolas-grekas added a commit that referenced this pull request Jan 4, 2020
…tuart Fyfe)

This PR was squashed before being merged into the 4.3 branch.

Discussion
----------

[Mailer] Remove line breaks in email attachment content

Line breaks are not allowed in attachment content when sending over the
API.

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #33671, Closes #32645
| License       | MIT
| Doc PR        |

This is a fix for #33671. Send grid's API throws a 400 error when sending email attachments with default base64 encoding.
Removing the line breaks resolves this issue.

Commits
-------

a28a7f9 [Mailer] Remove line breaks in email attachment content
@nicolas-grekas nicolas-grekas merged commit a28a7f9 into symfony:4.3 Jan 4, 2020
This was referenced Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants