Fix and test for miss matched transfer encoding (header says it's quotedprintable when it's not) #23
Conversation
There was a problem hiding this comment.
Thanks for the contribution Chris, but I don't think this code is doing what you think it is; see my code comments for why. I think we are going to have to take a different strategy to solve this problem, probably creating a reader that detects invalid bytes and qp encodes them on the fly. It could then wrap contentReader at https://github.com/jhillyerd/enmime/blob/master/part.go#L89
Also, I would prefer it if PRs were submitted against the develop branch - I'm going to add a tl;dr to the contributing guide because I realize it's not clear for people that aren't familiar with git-flow.
| if ioerr != nil { | ||
|
|
||
| // quoted-printable was not quoted. try raw | ||
| if strings.Contains(ioerr.Error(), "quotedprintable: invalid unescaped byte") { |
There was a problem hiding this comment.
In general, it's best to avoid inspecting to the contents of an error string. In this case I don't see an easy way around it though.
https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully (see Never inspect the output of error.Error)
|
|
||
| // quoted-printable was not quoted. try raw | ||
| if strings.Contains(ioerr.Error(), "quotedprintable: invalid unescaped byte") { | ||
| reader, err := newCharsetReader("utf-8", p.rawReader) |
There was a problem hiding this comment.
I may be wrong, but my understanding is that at this point, p.rawReader has already been partially or completely consumed. The plain text portion of the message is lost. Your unit test below seems to confirm that; I will continue my comment there.
There was a problem hiding this comment.
Yes you are right. I totally know better.
| t.Fatal("Failed to parse MIME:", err) | ||
| } | ||
|
|
||
| if e.Text != "Stuff" { |
There was a problem hiding this comment.
"Stuff" is not the expected text portion of this message, it should be the ~60 lines of text from the Content-Type: text/plain; charset="utf-8" MIME part.
What's happening here is enmime saw that envelope.Text was empty, and down-converted the HTML section to text. That's not what we want.
| @@ -917,3 +917,51 @@ func TestEnvelopeHeaders(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestMissMatchedTransferEncoding(t *testing.T) { | |||
|
I'm going to take a crack at the reader thing, since I've had to implement a couple io.Readers for enmime already... |
|
Thank you for entertaining my rushed, poorly thought out PR. I will test your changes and let you know if anything crops up. Cheers! |
No description provided.