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

Update attachment headers #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NeonDaniel
Copy link

Refactor attachment MIME object to match other (working) providers per ProtonMail support
Closes #228

I validated this change by sending an email to my gmail address ([email protected]) and to a newly-created ProtonMail account. Both emails contained attachments as ecpected. A sample new header is included below.

Content-Type: text/plain; name="skills_log.txt"
MIME-Version: 1.0
Content-Disposition: attachment; filename="skills_log.txt"; name="skills_log.txt"
Content-Transfer-Encoding: base64

@kootenpv
Copy link
Owner

Thank you! Could you provide an example in which it does not work, and for which it will now work? I'd love to support it, but it is hard for me to verify at the moment that it will improve the situation.

@NeonDaniel
Copy link
Author

From the linked issue, protonmail recipient accounts would discard attachments sent with yagmail, but with this change they now show up as expected. I tested this by creating a new Protonmail email account and checking attachments in their email web app.

The old header looked like:

Content-Type: text/plain; name*=utf-8''voice_log.txt
MIME-Version: 1.0
Content-Transfer-Encoding: base64

and the new header:

Content-Type: text/plain; name="skills_log.txt"
MIME-Version: 1.0
Content-Disposition: attachment; filename="skills_log.txt"; name="skills_log.txt"
Content-Transfer-Encoding: base64

A sample call might look like:

        with yagmail.SMTP("[email protected]", password, "smtp.gmail.com", '465') as yag:
            yag.send(to="[email protected]", subject="Test Attachments, contents="See attached files.",
                     attachments=["/tmp/test_attach.log"])

@NeonDaniel
Copy link
Author

@kootenpv Anything else I can contribute here to help get this merged?

@mchubby
Copy link

mchubby commented Dec 11, 2024

Hello,
The ASVS Working Group at OWASP discussed ways to properly encode the Content-Disposition header. It is important to avoid the filename being misused to manipulate other headers. I kinda trust them more to navigate the flow of numerous RFCs around this subject.
OWASP/ASVS#1390

You may also refer to https://blog.nodemailer.com/2017/01/27/the-mess-that-is-attachment-filenames/ who claim you can also set Content-Type... name as a fallback value for legacy MUAs.

EDIT: Sorry, RFC 6266 is standard for HTTP. The relevant one for MTA is still RFC 2231 AFAICT. See CVE-2024-39929 for an example on how the header could be abused to send attachments with malicious names.

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.

Sending attachment fails
3 participants