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

net/mail: ParseAddress/String corrupt address #11292

Closed
dvyukov opened this issue Jun 19, 2015 · 5 comments
Closed

net/mail: ParseAddress/String corrupt address #11292

dvyukov opened this issue Jun 19, 2015 · 5 comments
Assignees
Milestone

Comments

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Jun 19, 2015

The following program fails the panic:

package main

import (
    "fmt"
    "net/mail"
)

func main() {
    data := "\"\\\x1f,\"<0@0>"
    addr, err := mail.ParseAddress(data)
    if err != nil {
        return
    }
    _, err = mail.ParseAddress(addr.String())
    if err != nil {
        fmt.Printf("failed to parse addr: %q -> %q\n", data, addr)
        panic(err)
    }
}
failed to parse addr: "\"\\\x1f,\"<0@0>" -> "=?utf-8?q?=1F,?= <0@0>"
panic: mail: no angle-addr

ParseAddress must handle result of Address.String, or else first ParseAddress must fail.

go version devel +514014c Thu Jun 18 15:54:35 2015 +0200 linux/amd64

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jun 19, 2015
@alexcesaro
Copy link
Contributor

@alexcesaro alexcesaro commented Jun 24, 2015

Nice catch. I overlooked section 5.3 of RFC 2047 in mime.WordEncoder.Encode (used in addr.String()).

Since this is a new Go 1.5 method, should I fix it now?

cc @bradfitz

@dsymonds
Copy link
Member

@dsymonds dsymonds commented Jun 26, 2015

@alexcesaro: Go ahead and prepare a fix. We should fix bugs in new code.

@alexcesaro
Copy link
Contributor

@alexcesaro alexcesaro commented Jun 26, 2015

I think to fix that we should:

  1. Add a WordEncoder to handle Q-encoding in a 'phrase' context.
  2. Use that new WordEncoder in net/mail instead of mime.QEncoding.

The first point concerns the new code while the second point concerns existing code and is not a regression: http://play.golang.org/p/nKIl4bflnE

Should I fix point 1 only ? both ? none ?

@alexcesaro
Copy link
Contributor

@alexcesaro alexcesaro commented Oct 19, 2015

I see two ways to fix that issue:

  1. As I said in the previous message, add a new WordEncoder to Q-encode in a "phrase" context (ie. names in address fields).
  2. Use base64 encoding when the names in addresses use the additional characters that need to be encoded.

Solution 1 code will feel a bit hacky since WordEncoder underlying type is byte. We will need to add a new const like QPhraseEncoder = WordEncoder('Q').
Solution 2 is simpler, adds no new API and this is what Gmail do for example.

@bradfitz is solution 2 ok?

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 20, 2015

CL https://golang.org/cl/16012 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.