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

Log the contents of the failed mail and attachment(s). #69

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

Conversation

wgresshoff
Copy link
Contributor

❤️ Thank you for your contribution!

Description

When the retry mechanism finally fails the contents of the mail and the attachments will be logged so admins get informed.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

mail_log.logger.debug(msg.recipients)
mail_log.logger.debug(msg.subject)
mail_log.logger.debug(msg.body)
for attachment in msg.attachments:
Copy link
Member

@Samk13 Samk13 Jan 15, 2025

Choose a reason for hiding this comment

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

Logging attachment data, the full message body, and recipients might expose sensitive information, don't you think?
Would it be better to e.g. redact the body and only log the number of attachments instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a very special use case here in Münster so this is done under just some special circumstances:

  • the retries have come to an end
  • the log is at debug level and has to be switched to this by configuration

We only send mails when data is archived and a certificate for the funders is needed (which is in the attachment) about it. And made sure there is no sensible information included.

@wgresshoff wgresshoff requested a review from ntarocco January 30, 2025 07:28
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.

2 participants