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

Content headers missing for inlined images #399

Closed
ndousson opened this issue Dec 3, 2024 · 10 comments
Closed

Content headers missing for inlined images #399

ndousson opened this issue Dec 3, 2024 · 10 comments

Comments

@ndousson
Copy link

ndousson commented Dec 3, 2024

Hi, we recently moved from Maildev to Mailpit in order to keep the nicest tool on our local stack and for our review app environments.

After switching, everything was working nicely and as expected BUT one things was broken:

Inlined image rendering

After a quick analysis, we discovered that Content headers related to inlined images in the raw email was missing.

Here is a screenshot of the issue:

image

Here is the raw email from maildev and mailpit to compare:

maildev.txt
mailpit.txt

Let me know if you need more information.

@axllent
Copy link
Owner

axllent commented Dec 3, 2024

Hi @ndousson. These are two very different emails - the mailpit.txt is missing all attachments in the raw email (ie: no attachments) whereas the maildev.txt is not. This means the inline images were not included by whatever generated that email before sending to the SMTP server (Mailpit). I don't think it has anything to do with Mailpit at all because if I send the contents of maildev.txt to my mailpit it works as expected (inline images show).

What are you using to generate these emails, and why would it be behaving different when sending to Mailpit?

@ndousson
Copy link
Author

ndousson commented Dec 4, 2024

Hi @axllent,

After reading your answer, I double-checked how we were pushing messages to Maildev and confirmed that it was via SMTP.
Since we've implemented an ApiTransport, we're now sending messages via API to Mailpit.

I dug into the code with some debugging and found that the issue was at the AbstractTransport level, where the headers were missing. So you're absolutely right — this isn't related to Mailpit.

I apologize for the confusion and the noise. Thank you again for your response; it was incredibly helpful! 🙏🏻

@ndousson ndousson closed this as completed Dec 4, 2024
@ndousson ndousson reopened this Dec 4, 2024
@ndousson
Copy link
Author

ndousson commented Dec 4, 2024

Hi @axllent,

I'm back 🙈

After a deep dive with a colleague, we discovered that sending emails through the Mailpit API doesn't seem to support inline attachments when the attachment is base64-encoded.

Here’s the payload of a fake template we use in our tests, which should work but doesn’t:

fake_email.json

We also reviewed the Mailpit code to check if we missed something in the documentation about this behavior but didn’t find anything.

Do you agree that the API doesn’t (yet) support inline attachments with base64-encoded values?

If so, do you have an alternative approach that would allow us to continue using Mailpit? We can’t use SMTP as we rely on webhook functionality and the ID returned by the API call.

@ndousson
Copy link
Author

ndousson commented Dec 4, 2024

@axllent
Copy link
Owner

axllent commented Dec 5, 2024

@ndousson Thanks for the investigation. Firstly there are a few things wrong with your JSON file (for use with Mailpit), and I'm not sure where you are getting your JSON structure from. Have you seen the docs for sending via the Mailpit HTTP API?

  1. You need to use Attachments, not inlinedAttachments
  2. Attachments have a Content field, not base64Content
  3. Mailpit does not (currently) support specifying contentType for attachments, they are automatically detected based on the binary data extracted from the base64-encoded Content
  4. Your attachment filenames in your JSON all lack any file extension, so "filename": "greyLogo" should be "filename": "greyLogo.png". It's not that it won't work, it is just bad practice not to use file extensions because your mileage will vary with email program support.

Based on your feedback, I like the idea of being able to optionally set the Content-Id to determine if an attachment is inline or not, and makes this backwards compatible with existing implementations. As you noted, Mailpit currently only supports regular attachments. That being said, many email programs do support "inlining" attachments simply by using their filename, so <img src="test.gif" /> tends to work if the email has a test.gif attachment. I'm definitely not saying this is best practice though!

I am now testing the option to optionally set both the Content-Type and Content-Id via the API.

  1. If ContentType is left empty (or not set) then it is auto-detected
  2. If ContentID is set, then the attachment is inline, otherwise just a regular attachment

Does this make sense to you, and do you agree that this would provide you with the necessary functionality to test your application?

@ndousson
Copy link
Author

ndousson commented Dec 5, 2024

Thank you, @axllent, for your feedback.

You're absolutely right about the implementation. While I wasn't the one who originally implemented the payload, I agree that it doesn't align with the Mailpit API contract.

I just updated it a few minutes ago to match the expected behavior. 👍🏻

I also appreciate your suggestion regarding the Content-Type and Content-ID.

If the final result aligns with the example below, it should work perfectly. ✨

Current data part on Mailpit with current API contract:

Content-Type: image/gif; name=avatar.gif
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=avatar.gif

<based_64_encoded>

Expected (example from Maildev):

Content-ID: <cd19d7feae616eca67de89a4dc5229b9@symfony>
Content-Type: image/gif; name="cd19d7feae616eca67de89a4dc5229b9@symfony"
Content-Transfer-Encoding: base64
Content-Disposition: inline; name="cd19d7feae616eca67de89a4dc5229b9@symfony"; filename=avatar

<based_64_encoded>

If you have a pre-release, I'm ready to test it 👍🏻

FYI, the payload that was used is the same than the one for Mailjet.

axllent added a commit that referenced this issue Dec 5, 2024
Optional settings for Attachment ContentID & ContentType
@axllent
Copy link
Owner

axllent commented Dec 5, 2024

Excellent! Assuming you are running Mailpit's Docker environment you can use the axllent/mailpit:edge build which has just been updated with this change. I've also taken the liberty to modify your original payload: fake_email.json. I have added the correct casing to the JSON keys, although this isn't required as it will accept any casing (eg: attachments is the same as AttaCHMenTs) :)

As for the payload format vs: Mailjet - I originally did a comparison between several popular HTTP API services and every single one did it differently to each other. In the end I went with what felt was a good combination.

Content-Disposition: inline; filename=icon-love.gif
Content-Id: <icon-love>
Content-Transfer-Encoding: base64
Content-Type: image/gif; name=icon-love.gif

Please let me know if this resolves your issue?

@ndousson
Copy link
Author

ndousson commented Dec 5, 2024

I just tested the edge release of the Docker image, and it works perfectly! ✨

Everything is now resolved on our side. Huge thanks for your responsiveness and help with this issue. 🙏🏻

We'll wait for the next tagged release to upgrade. In the meantime, I'll use the edge release in our non-production environment to unblock our developers with the images.

@axllent
Copy link
Owner

axllent commented Dec 5, 2024

Awesome, thanks for the quick feedback. I will be releasing a new version within the next 2-3 days (by the end of the weekend). I'll let you know on this issue when I do 👍

@axllent
Copy link
Owner

axllent commented Dec 8, 2024

This feature has now been officially released in v1.21.6. Enjoy!

@axllent axllent closed this as completed Dec 8, 2024
tmeijn pushed a commit to tmeijn/dotfiles that referenced this issue Dec 9, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [axllent/mailpit](https://github.com/axllent/mailpit) | patch | `v1.21.5` -> `v1.21.6` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>axllent/mailpit (axllent/mailpit)</summary>

### [`v1.21.6`](https://github.com/axllent/mailpit/blob/HEAD/CHANGELOG.md#v1216)

[Compare Source](axllent/mailpit@v1.21.5...v1.21.6)

##### Feature

-   Include Mailpit label (if set) in webhook HTTP header ([#&#8203;400](axllent/mailpit#400))
-   Add support for sending inline attachments via HTTP API ([#&#8203;399](axllent/mailpit#399))

##### Chore

-   Update caniemail database
-   Update node dependencies
-   Update Go dependencies

##### Fix

-   Message view not updating when deleting messages from search ([#&#8203;395](axllent/mailpit#395))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
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

No branches or pull requests

2 participants