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

Feature Request: Log failed messages somewhere #347

Closed
knpwrs opened this issue Aug 11, 2024 · 5 comments
Closed

Feature Request: Log failed messages somewhere #347

knpwrs opened this issue Aug 11, 2024 · 5 comments

Comments

@knpwrs
Copy link

knpwrs commented Aug 11, 2024

I was attempting to send some emails to Mailpit and it turned out they were malformed. Mailpit was having issues parsing the headers. I would see these errors in the log output, but no information was available in the web UI. It would be great if the raw email content could be stored somewhere for examination in the web UI.

@axllent
Copy link
Owner

axllent commented Aug 11, 2024

This is an interesting request @knpwrs. What was malformed in your messages exactly? Surely you were getting an SMTP error (rejection) while sending the malformed message, and if so, what error was reported there? What error message was logged in Mailpit's log? I ask because I am trying to understand the context of the error you experienced, and what was the cause and result.

I am reluctant to add a this feature for a couple of reasons:

  1. Handling message rejection (for whatever reason) should be handled via the SMTP client/application. When Mailpit fails it should return the error. Mailpit is designed to display the captured data, but you're asking for it to also store the data that was not captured. This is somewhat out of scope for the purpose of the application.
  2. Errors can be caused by many things and don't necessarily occur because of just bad message headers. There are plenty of invalid SMTP combinations such as bad username/password combinations, or too many recipients, wrong TLS protocol etc. Your proposal to store the raw (bad) message data creates a lot of technical overhead to provide insight into a small subsection of potential errors. Again, without knowing the reason your messages were malformed and rejected it's difficult for me to guess.

One potential option could be to display a temporary error notification in Mailpit's UI, displayed to any connected browser (similar to when a message is received). The issue with this approach is that it's more of a "hey a message just got rejected for XYZ", but then again, if someone has Mailpit exposed to the internet and a script kiddy is hammering away trying to authenticate (brute force), then this would flood the web UI with error messages too, so it's not ideal either. This also requires the user to be connected and see the error in a few seconds it is displayed.

I'm keen to hear more about your error though.

@knpwrs
Copy link
Author

knpwrs commented Aug 12, 2024

I did indeed get an SMTP rejection error, and I saw that there was a malformed header in the container logs for mailpit. The thing that was tricky for me was that I was using a go server, a Faktory queue with a go worker calling out to net/smtp, and mailpit. This is my first time using both Go and Faktory, and so it took me a little bit longer to find the bug than if I had been using a more familiar stack.

So this is really just a “would have been neat to see” idea. I was watching the mailpit UI and didn’t see anything, and so I assumed the message never reached mailpit, something along the way must have errored out. I started tracing from the beginning and came to find that it did indeed reach mailpit, but the message was rejected.

The issue was that I had an email roughly like this:

From: sender@example.com
To: recipient@example.com
Subject: Multipart Email Example
multipart/alternative; boundary="boundary-string"

--your-boundary
Content-Type: text/plain; charset="utf-8"

Hello, World!

--boundary-string
Content-Type: text/html; charset="utf-8"

<h1>Hello, World!</h1>

--boundary-string

As you can see, there is a value for Content-Type, but Content-Type itself is missing. This is quite the error on my part!

So really any way of surfacing these errors to the mailpit UI would be nice to have since I was watching the mailpit UI anyway. It doesn’t even have to be in the message list. Maybe a separate tab that’s like an audit log, or even like you suggested, some sort of temporal error message for connected clients.

@axllent
Copy link
Owner

axllent commented Aug 13, 2024

Thanks for the information, this is both useful for me to try understand the situation, and so make a more informed decision. Whilst storing rejected messages together with the reason sounds ideal, I don't think it is a feasible option as it introduces a rather large level of complexity because those messages need to also be stored somewhere, and these are likely only a fraction of the potential errors themselves. I believe that in your case a simple "alert" in the UI that a message had been rejected (with whatever reason you got from SMTP) would have been sufficient to help you diagnose the issue.

The easiest solution here would be to fire-and-forget a warning message to all connected browsers, meaning no need for any storage or special UI elements (other than what is already there), however that is possibly a bit too short-sighted in the scheme of things. This approach would require very few moving parts and wouldn't take too much to implement.

The other potential solution would be to both display a temporary notification (just like when a new message is received) as well as "store" a certain number of error messages (eg: up to 100 or something). The UI would need some sort of "error logs" section where one could then view these. This is more complex than the easy solution mentioned above, but would cater for when a browser isn't connected (or a user isn't watching) as well as, within reason, historical errors.

I need to give this a lot more thought as to how I approach this, and what the best way forward is. If you have any more thoughts then please feel free to share them in the meantime.

@axllent
Copy link
Owner

axllent commented Aug 17, 2024

@knpwrs I have added temporary (5 second) web UI error notifications in v1.20.2 after deciding that a whole UI logging system is just not feasible. There are so many different types of potential errors when it comes to the Mailpit ecosystem (it's a lot more than just SMTP), which makes a UI logging system a very complicated "nice to have" task. Client errors should always be handled properly client-side anyway.

Hopefully this helps you identify similar issues though, and I hope you understand the reasoning not adding the logging system to the UI?

@knpwrs
Copy link
Author

knpwrs commented Aug 17, 2024

That's very good, thank you @axllent! The only thing I'd suggest is maybe a persistent dismissible notification, but a five-second notification probably could have helped a lot.

Thank you again!

@knpwrs knpwrs closed this as completed Aug 17, 2024
tmeijn pushed a commit to tmeijn/dotfiles that referenced this issue Aug 20, 2024
This MR contains the following updates:

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

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.20.2`](https://github.com/axllent/mailpit/blob/HEAD/CHANGELOG.md#v1202)

[Compare Source](axllent/mailpit@v1.20.1...v1.20.2)

##### Feature

-   Web UI notifications of smtpd & POP3 errors ([#&#8203;347](axllent/mailpit#347))

##### Chore

-   Update Go dependencies
-   Update node dependencies
-   Add debug database storage logging
-   Add smtpd server logging in the CLI ([#&#8203;347](axllent/mailpit#347))

</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