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: ParseDate incorrectly prunes out RFC 5322 date with a capital "T" when dealing with an obsolete timezone #39260

Open
sywesk opened this issue May 26, 2020 · 3 comments
Labels
Milestone

Comments

@sywesk
Copy link

@sywesk sywesk commented May 26, 2020

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

$ go version
go version go1.14.3 linux/amd64

Does this issue reproduce with the latest release?

The version i'm using is the latest at the time of writing.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home//.cache/go-build"
GOENV="/home//.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home//go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/c/Users//Documents/projects//backend/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build302313865=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/KEedWiDqkyz

package main

import (
	"net/mail"
)

func main() {
	_, err := mail.ParseDate("Thu, 14 May 2020 11:44:40 GMT")
	if err != nil {
		panic(err)
	} 
	
	println("ok")
}

What did you expect to see?

I expect the time string "Thu, 14 May 2020 11:44:40 GMT" to be parsed correctly without error.

What did you see instead?

mail: header could not be parsed


Research

Before posting here I did spend some time researching the problem. Here is a plaground link with the full ParseDate code extracted and isolated to be able to investigate.

I did at first regenerate all the time layouts using buildDateLayouts(), and my specific time string did match with two of the generated layouts. So that's a good start.

But looking at the ParseDate(date string) (time.Time, error) we can see that the following code seems to rewrite the date string sequence leaving only the letter T:

        else if ind := strings.Index(p.s, "T"); ind != -1 && len(p.s) >= ind+1 {
		// The last letter T of the obsolete time zone is checked when no standard time zone is found.
		// If T is misplaced, the date to parse is garbage.
		date = p.s[:ind+1]
		p.s = p.s[ind+1:]
	}

It seems that this particular time string triggers this come into thinking that the T from Thu is about an obsolete timezone. Changing the time string to "Mon, 14 May 2020 11:44:40 GMT" for example solves the problem.

I hope my report is complete, and it may help you identify the logic bug causing this problem. If can help with anything, let me know. With some explanations about the logic the problematic code section I may even be able to make a pull request :D

Thanks for your time,

@sywesk sywesk changed the title ParseDate corner case error net/mail ParseDate() Corner case error May 26, 2020
@odeke-em odeke-em changed the title net/mail ParseDate() Corner case error net/mail: ParseDate incorrectly assumes that any RFC 5322 date with a capital "T" is dealing with an obsolete timezone May 26, 2020
@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 26, 2020

Thank you for catching this interesting bug @sywesk, for the investigation, but also welcome to the Go project, it is great to receive your report!

This issue was introduced almost 2 years ago (September 2018 in Go1.12) in the bug fix for #22661 in CL https://go-review.googlesource.com/c/go/+/117596. As you noticed, the presence of "T" anywhere in the string will choke it and return an error, so other days of the week will work except for "Tue" and "Thur". I believe a fix for this will include:
a) Invoking strings.LastIndex instead of strings.Index
b) Ensuring that the found index+1 <= len(p.s)

which will hopefully be a trivial fix and we'll also need more tests to catch the bug and prevent regressions.

Kindly paging @iWdGo @ianlancetaylor.

@odeke-em odeke-em added this to the Go1.16 milestone May 26, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 26, 2020

Change https://golang.org/cl/235200 mentions this issue: net/mail: fix ParseDate obsolete timezone "T" index check

@odeke-em odeke-em changed the title net/mail: ParseDate incorrectly assumes that any RFC 5322 date with a capital "T" is dealing with an obsolete timezone net/mail: ParseDate incorrectly prunes out RFC 5322 date with a capital "T" when dealing with an obsolete timezone May 26, 2020
@sywesk
Copy link
Author

@sywesk sywesk commented May 27, 2020

Hello @odeke-em ! Thank you for your kind & quick message, plus the quick CL 👍 I went through your modifications and saw you did a wonderful job correcting other problematic cases ;)

@andybons andybons added the NeedsFix label May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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