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

mime/multipart: add (*Reader).NextRawPart to avoid quoted-printable decoding #26803

Closed
gbbr opened this issue Aug 4, 2018 · 8 comments

Comments

@gbbr
Copy link
Member

commented Aug 4, 2018

Based on documentation, (*Reader).NextPart should return the unmodified next part in a multipart message. Instead, it removes the Content-Transfer-Encoding header and decodes the body automatically when its value is quoted-printable.

This seems rather uncalled for, given that:

  • RFC 2045 Section 6.7 indicates: "[...] encoded in Quoted-Printable to ensure
    the integrity of the data [...]". The current behavior does not ensure integrity.
  • Users of this package could access the Content-Transfer-Encoding header themselves and use the already existent mime/quotedprintable package.

Because data integrity is breached, this pretty much renders it impossible to use the mime/multipart package to implement a reliable IMAP server.

The proposal suggests removing this functionality and preserving the unmodified header and body. Users will be able to decode such bodies themselves when necessary by reading the Content-Transfer-Encoding header and using the mime/quotedprintable package, while also having access to the original message.

@gbbr gbbr added the Proposal label Aug 4, 2018

@gbbr

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2018

Unsure if my issue classifies as a bug or a proposal. Possibly more towards the latter since it breaks already existing code. Go2?

@ianlancetaylor ianlancetaylor changed the title mime/multipart: (*Reader).NextPart does not preserve message integrity proposal: mime/multipart: preserve message integrity in (*Reader).NextPart Aug 5, 2018

@gopherbot gopherbot added this to the Proposal milestone Aug 5, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2018

This proposal would be easier to adopt if you could propose a clean and simple way to avoid this decoding, rather than changing the existing behavior. Thanks.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

We can't change the existing behavior. We'd have to add something like NextRawPart.

But can you elaborate on why IMAP needs the encoded version of the part? In general it would seem like there are multiple ways to do quoted-printable encoding so if you wanted a canonical form for signatures and the like you'd use the decoded form.

@gbbr

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

But can you elaborate on why IMAP needs the encoded version of the part?

I'm not an expert on IMAP but going through RFC 3501 the idea I get is that IMAP should serve unmodified messages as they are in the mailbox. This assumption is even more so validated when consulting with Gmail's IMAP server, which also returns the body structure as "quoted-printable" and reports the size in bytes and lines of the unencoded message. You can try this yourself by running a . FETCH <x> BODY command on any message containing "quoted-printable" parts. Here is an example fetching part 1 of one of my email messages from Gmail:

. fetch 6 body[1]
* 6 FETCH (BODY[1] {56199}
--0000000000004cf36d056d78e562
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

...

The part body is returned as is. This is what makes me think automatic decoding is uncalled for. The whole purpose of "quoted-printable" is to preserve data integrity.

@gbbr

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

How about adding something like (*Reader).KeepQuoted to configure the behavior? (think (encoding/json).Decoder.UseNumber as an analogue to changing decoder behaviour)

// KeepQuoted will determine the body readers returned by NextPart to not decode
// content encoded with "quoted-printable".
func (r *Reader) KeepQuoted() {
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

A method like NextRawPart seems strictly better than a configuration knob.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

Like Ian said, global (even global to one Reader) configuration knobs are discouraged. Code calling NextPart should expect not to find quoted printable. It should not need to figure out the configuration first.

In contrast, json.Decoder's UseNumber is unforunately necessary because the actual decision being controlled is far removed from the actual API call. (The decoder encounters and tries to fill in some interface{} somewhere during decoding.)

@rsc rsc changed the title proposal: mime/multipart: preserve message integrity in (*Reader).NextPart mime/multipart: add (*Reader).NextRawPart to avoid quoted-printable decoding Aug 13, 2018

@rsc rsc modified the milestones: Proposal, Go1.12 Aug 13, 2018

@gbbr

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

I am not convinced we should do this. Even with this method added, an IMAP server would have to have access to the raw header of a part as well, for the same reasons. Know this, adding NextRawPart wouldn't change much with regards to how usable this package would be for an IMAP server implementation. There is no easy access to the raw header.

I think the intention behind this package was more from a client standpoint, rather than a server one. For now, I'd close this. If I can think of a better proposal which would give access to both raw header & body of a part I will open a new issue.

Thank you both for giving me you input and the opportunity to contribute a (first) feature to Go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.