proposal: net/mail: allow AddressParser helper code to signal invalid character set #41625

tt · 2020-09-25T08:01:01Z

What version of Go are you using (`go version`)?

$ go version
go version go1.15.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tt/Library/Caches/go-build"
GOENV="/Users/tt/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/tt/pkg/mod"
GONOPROXY="github.com/tt"
GONOSUMDB="github.com/tt"
GOOS="darwin"
GOPATH="/Users/tt:/Users/tt/libraries"
GOPRIVATE="github.com/tt"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/tt/src/github.com/tt/mail/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v3/slr36r597rn0rrfzq_my6nv00000gn/T/go-build296585987=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I assigned a mime.WordDecoder to mail.AddressParser in order to be able to support more charsets.

This changes the behavior of the parsing:

(&mail.AddressParser{}).Parse("=?foo?Q?=00?= <foo@example.com>") returns no value and the error (&errors.errorString{s:"mail: missing word in phrase: charset not supported: \"foo\""})
(&mail.AddressParser{WordDecoder: &mime.WordDecoder{}}).Parse("=?foo?Q?=00?= <foo@example.com>") returns the value &mail.Address{Name:"=?foo?Q?=00?=", Address:"foo@example.com"} and no error

Assigning the decoder's CharsetReader makes no difference.

See this Go playground for an example.

What did you expect to see?

I would expect to be able to still error when encountering an unsupported charset and maybe generally to know whether any error occurred.

What did you see instead?

The error was ignored and the name was set to the decoded word.

When mail.AddressParser was first implemented in 1defd22, it would return any error.

That changed in 6491496 and since only the unexported charsetError is treated as an error:

go/src/net/mail/message.go

Lines 733 to 750 in 3e636ab

    
           func (p *addrParser) decodeRFC2047Word(s string) (word string, isEncoded bool, err error) { 
        
           	if p.dec != nil { 
        
           		word, err = p.dec.Decode(s) 
        
           	} else { 
        
           		word, err = rfc2047Decoder.Decode(s) 
        
           	} 
        
           	if err == nil { 
        
           		return word, true, nil 
        
           	} 
        
           	if _, ok := err.(charsetError); ok { 
        
           		return s, true, err 
        
           	} 
        
           	// Ignore invalid RFC 2047 encoded-word errors. 
        
           	return s, false, nil 
        
           }

However, it feels like an unintended consequence of that commit.

I think one approach to solving this would be moving charsetError to the mime package (where mime.WordDecoder itself could use it) and exporting it.

The text was updated successfully, but these errors were encountered:

gopherbot · 2020-09-25T09:18:47Z

Change https://golang.org/cl/257398 mentions this issue: mime: define CharsetError

ianlancetaylor · 2020-09-26T00:03:37Z

I think the change in error handling happened in https://golang.org/cl/7890 aka 2b03610

odeke-em · 2020-10-25T09:15:51Z

Thank you for filing this bug @tt! As @ianlancetaylor mentioned on your CL https://go-review.googlesource.com/c/go/+/257398, exporting new errors requires a proposal review firstly. Could you please convert this issue into a proposal and I can help you tweak the title but please help tweak the body to make it a concrete proposal. Thank you!

odeke-em · 2020-10-25T09:25:32Z

Given the current rush for time and the fact that holding this up presents a potential bug, we can apply a pragmatic approach with either:
a) Keep the error in net/mail unexpected as is, and then do a string.HasPrefix check with a comment and link to the proposal link

	if if _, ok := err.(charsetError); ok {
		return s, true, err
	} else if strings.HasPrefix(err.Error(), "mime: unhandled charset") {
                 // Unfortunately before we have an exported cross-package error,
                 // we have to rely on string matching. TODO: replace this check with
                 // errors.Is/As whenever  https://golang.org/issue/41625 is accepted.
		return s, true, err
         }

b) Create an internal package called customerrors or so, and in it define that error that can then
be freely moved around or later when we the proposal for mime.CharsetError is accepted, just create a type alias

type CharsetError = customerrors.CharsetError

rsc · 2020-10-28T17:50:53Z

It doesn't seem like we should change anything for Go 1.16.

For the future, why does mime need to export the CharsetError? Why not net/mail?

odeke-em · 2020-10-28T17:53:09Z

The parsing starts from mime which is where it is necessary. However I proposed 2 alternatives to begin with: 1. Internal package 2. Sub string match.

…

On Wed, Oct 28, 2020 at 10:51 AM Russ Cox ***@***.***> wrote: It doesn't seem like we should change anything for Go 1.16. For the future, why does mime need to export the CharsetError? Why not net/mail? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#41625 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V7HKT6HIIDC4YC4UHDSNBKY5ANCNFSM4RZHM35Q> .

rsc · 2020-11-11T18:52:31Z

It looks like if WordDecoder.CharsetReader returns any error, that should be taken to mean "character set not supported".
It's unclear why we'd need to export a specific error when every error result should mean that.

So maybe some code in mime or net/mail needs to be more careful about doing that, and there is just a bug here.

rsc · 2021-01-13T15:55:39Z

I finally looked at this and mailed https://go-review.googlesource.com/c/go/+/283632.
It's just a bug, not something that needs new API.

gopherbot · 2021-01-13T15:55:48Z

Change https://golang.org/cl/283632 mentions this issue: net/mail: improve detection of charset errors

rsc · 2021-01-13T19:22:59Z

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

rsc · 2021-01-20T19:39:17Z

No change in consensus, so declined.
— rsc for the proposal review group

The detection of the "unknown charset" case was too tailored to one specific address parser. Make it generalize, so that custom address parsers behave the same way as the default one for character sets they do not handle. Fixes #41625. Change-Id: I347d4bb6844d0a1f23e908b776d21e8be5af3874 Reviewed-on: https://go-review.googlesource.com/c/go/+/283632 Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Russ Cox <rsc@golang.org> Run-TryBot: Russ Cox <rsc@golang.org> Auto-Submit: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>

tt changed the title ~~net/mail~~ net/mail: AddressParser ignores errors Sep 25, 2020

tt changed the title ~~net/mail: AddressParser ignores errors~~ net/mail: AddressParser ignores errors Sep 25, 2020

tt mentioned this issue Sep 25, 2020

mime: define CharsetError #41627

Closed

ianlancetaylor added the NeedsInvestigation label Sep 25, 2020

ianlancetaylor added this to the Backlog milestone Sep 25, 2020

odeke-em changed the title ~~net/mail: AddressParser ignores errors~~ proposal: mime should export a CharsetError to allow net/mail.AddressParser to catch errors associated with bad character sets instead of presently ignoring them since they can't be passed back Oct 25, 2020

gopherbot added the Proposal label Oct 25, 2020

rsc mentioned this issue Oct 29, 2020

proposal: review meeting minutes #33502

Open

rsc added the Proposal-FinalCommentPeriod label Jan 13, 2021

rsc removed the Proposal-FinalCommentPeriod label Jan 20, 2021

rsc closed this as completed Jan 20, 2021

golang locked and limited conversation to collaborators Jan 20, 2022

gopherbot added the FrozenDueToAge label Jan 20, 2022

rsc moved this to Declined in Proposals Aug 10, 2022

rsc added this to Proposals Aug 10, 2022

rsc removed this from Proposals Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: net/mail: allow AddressParser helper code to signal invalid character set #41625

proposal: net/mail: allow AddressParser helper code to signal invalid character set #41625

tt commented Sep 25, 2020

gopherbot commented Sep 25, 2020

ianlancetaylor commented Sep 26, 2020

odeke-em commented Oct 25, 2020

odeke-em commented Oct 25, 2020

rsc commented Oct 28, 2020

odeke-em commented Oct 28, 2020 via email

rsc commented Nov 11, 2020

rsc commented Jan 13, 2021

gopherbot commented Jan 13, 2021

rsc commented Jan 13, 2021

rsc commented Jan 20, 2021

proposal: net/mail: allow AddressParser helper code to signal invalid character set #41625

proposal: net/mail: allow AddressParser helper code to signal invalid character set #41625

Comments

tt commented Sep 25, 2020

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

gopherbot commented Sep 25, 2020

ianlancetaylor commented Sep 26, 2020

odeke-em commented Oct 25, 2020

odeke-em commented Oct 25, 2020

rsc commented Oct 28, 2020

odeke-em commented Oct 28, 2020 via email

rsc commented Nov 11, 2020

rsc commented Jan 13, 2021

gopherbot commented Jan 13, 2021

rsc commented Jan 13, 2021

rsc commented Jan 20, 2021

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?