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

Support multipart messages without content-type in subparts. (GH: #14) #28

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wolfsage
Copy link
Contributor

Per RFC 1341, section 7.2

https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

A body part is NOT to be interpreted as actually being an RFC 822 message.
To begin with, NO header fields are actually required in body parts. A
body part that starts with a blank line, therefore, is allowed and is a
body part for which all default values are to be assumed. In such a case,
the absence of a Content-Type header field implies that the encapsulation
is plain US-ASCII text.

@rjbs
Copy link
Owner

rjbs commented Aug 1, 2016

Thanks for waiting so patiently on this… I think the idea here is on the right track. I'm not sure about this line;

+ if ($bit =~ /^([\r\n][\r\n])/) {

If the input is CRLF delimited lines, won't this match a single CRLF, rather than two line-delimiting sequences?

Maybe this should be using mycrlf?

…rts.

Per RFC 1341, section 7.2

  https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

  A body part is NOT to be interpreted as actually being an RFC 822 message.
  To begin with, NO header fields are actually required in body parts. A
  body part that starts with a blank line, therefore, is allowed and is a
  body part for which all default values are to be assumed. In such a case,
  the absence of a Content-Type header field implies that the encapsulation
  is plain US-ASCII text.
@wolfsage wolfsage force-pushed the topic/gh-14-multipart-implicit-headers branch from 2b9d213 to 981d820 Compare August 2, 2016 02:33
@wolfsage
Copy link
Contributor Author

wolfsage commented Aug 2, 2016

Ah, whoops. Nice catch. Updated PR to add test for CRLF emails and include associated fix.

@rjbs
Copy link
Owner

rjbs commented Aug 2, 2016

I'm still reviewing this, I think I'm seeing more fundamental problems with the pre-existing code. But in the meantime: $no_header seems to be declared and never used.

@rjbs
Copy link
Owner

rjbs commented Aug 3, 2016

Basically, Email::MIME is not great. When we look for /\Q$boundary\E\s*?/ I think we really mean \h, and I know we can't use that because 5.8, but what if there's a stray CR before the CRLF, what does that even mean? Who knows! I continue to think about whether I want to move my own workings to Courriel or just look at a Email::MrMIME to replace this library.

Next problem (I think): You're adding a header to each MIME part that lacks one. This means the message won't round-trip. That means that DKIM signatures (RFC 4871) will break if you read in a message without that property and then reinject $email->as_string. Right? I have only inspected the code, not run it.

When we come across a part that has no specific content-type, we inject one
to satisfy RFC 1341, section 7.2. We must inject a header because Email::MIME
body parts are just Email::Simple objects -- which expect messages that have at
least one header.

After we've generated the part, we can then remove the added header, and be
on our merry way. This is awful, but works.
@wolfsage
Copy link
Contributor Author

As discussed, solved the possible DKIM signature breaking change by adding a header temporarily and then removing it after we're done.

As to the first part (CR before CRLF).. I'm not sure what to do here.

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