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/http: still return the content on malformed MIME header line #21290

Open
joaolsilva opened this Issue Aug 3, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@joaolsilva

joaolsilva commented Aug 3, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8.3 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jsilva/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_n/q8bzrfk93ss0nf_0fbk2ns8c0000gn/T/go-build250479046=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Fetch "http://107-182-231-168.webnow.net.br/status2.xsl?mount=/98fm.aac" or a similar URL from another Icecast server using http.Client.

Source: https://gist.github.com/joaolsilva/d24b7bf5602f0d6615eb5d385fd0ffdc
https://play.golang.org/p/MiokU7tQgI

What did you expect to see?

The content of the URL

What did you see instead?

err: Get http://107-182-231-168.webnow.net.br/status2.xsl?mount=/98fm.aac: malformed MIME header line: Content-Disposition = attachment; filename=file.asc

The MIME header might indeed be malformed, but other http clients such as cURL, browsers, etc. work (they seem to ignore the malformed line) and return the content.

@ALTree ALTree changed the title from malformed MIME header line: Content-Disposition = attachment; filename=file.asc to net/http: still return the content on malformed MIME header Aug 3, 2017

@ALTree ALTree added the NeedsDecision label Aug 3, 2017

@ALTree ALTree changed the title from net/http: still return the content on malformed MIME header to net/http: still return the content on malformed MIME header line Aug 3, 2017

@ALTree ALTree added this to the Go1.10 milestone Aug 3, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Aug 14, 2017

@tombergan

This comment has been minimized.

Contributor

tombergan commented Aug 14, 2017

Sigh.

On one hand, other browsers accepting this response is evidence that we should accept it as well.

On the other hand, the error comes from the net/textproto library and that "malformed MIME header line" error has been there since the initial implementation in 2010. The net/textproto library is widely used beyond net/http and I'm concerned that removing the error will break other users of net/textproto. There are many other uses of that library, including uses inside and outside the standard library. Also, this, although that ship has sailed.

This will require some thought.

Edit: Maybe this isn't so bad. I suppose we could add an IgnoreMalformedHeaderLines bool to textproto.Reader which defaults to false and is set to true by net/http.

@tombergan tombergan self-assigned this Aug 14, 2017

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 6, 2017

ping @tombergan. should we do this for Go 1.10?

@tombergan

This comment has been minimized.

Contributor

tombergan commented Nov 6, 2017

We're talking about this on golang.org/cl/75350. I think we should fix it but I'll leave it up to Brad on that CL if it's too late for 1.10.

@tombergan

This comment has been minimized.

Contributor

tombergan commented Nov 9, 2017

This is not going to happen for 1.10 because it's too late in the release cycle to resolve the open API questions.

@tombergan tombergan modified the milestones: Go1.10, Go1.11 Nov 9, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jun 22, 2018

Hello there @joaolsilva thank you for this bug report!

So currently with Go tip(pre-Go1.11) I can't produce this bug, and I also tried with Go1.8 but no dice, so perhaps the server might have fixed the header?
However, I have made a simple self contained snippet https://play.golang.org/p/whxnvo9-ZlF or inlined below, that can be run end-to-end without relying on an external server and I have tweaked the various parts of the "Content-Disposition" header but can't produce the problem either

package main

import (
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
)

func main() {
    log.SetFlags(0)
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		switch r.URL.Query().Get("k") {
		case "":
			w.Header().Set("Content-Disposition", `attachment; filename="file.txt"`)
		case "unq":
			w.Header().Set("Content-Disposition", `attachment; filename=file.txt`)
		case "lq":
			w.Header().Set("Content-Disposition", `attachment; filename="file.txt`)
		case "rq":
			w.Header().Set("Content-Disposition", `attachment; filename=file.txt"`)
		}
		w.Write([]byte(`Hello, World!`))
	}))
	defer cst.Close()

	for _, sym := range []string{"", "unq", "lq", "rq"} {
		resp, err := http.Get(cst.URL + "?k=" + sym)
		if err != nil {
			log.Printf("Failed to perform plain get: %v", err)
			continue
		}
		blob, _ := ioutil.ReadAll(resp.Body)
		_ = resp.Body.Close()
		log.Printf("sym=%-5q blob:%q\n", sym, blob)
	}
}

which just gives me

$ go run repro.go 
sym=""    blob:"Hello, World!"
sym="unq" blob:"Hello, World!"
sym="lq"  blob:"Hello, World!"
sym="rq"  blob:"Hello, World!"

that is all the data successfully parsed. Could you please take a look and tweak as possible?

I also tried reverting some changes to net/textproto as per https://github.com/golang/go/commits/master/src/net/textproto/reader.go but hit a point where my Go recompilation was crashing so figured I'd make the repro and hand it over for someone else to take a look. Also /cc @bradfitz

@joaolsilva

This comment has been minimized.

joaolsilva commented Jun 22, 2018

The original URL fixed the header. The following URL still has the same error:

http://ruby.streamguys.com:8120/status2.xsl?mount=/live

err: Get http://ruby.streamguys.com:8120/status2.xsl?mount=/live: net/http: HTTP/1.x transport connection broken: malformed MIME header line: Content-Disposition = attachment; filename=file.asc

I'm able to reproduce this issue with both go version go1.10.3 darwin/amd64 and go version devel +75fdeaa801 Fri Jun 22 13:28:19 2018 +0000 darwin/amd64

The headers are:

curl -I -XGET 'http://ruby.streamguys.com:8120/status2.xsl?mount=/live'

HTTP/1.0 200 OK
Content-Type: text/plain
Content-Length: 207
Content-Disposition = attachment; filename=file.asc
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: Origin, Accept, X-Requested-With, Content-Type
Access-Control-Allow-Methods: GET, OPTIONS, HEAD

Please note the Content-Disposition line, and how it's missing the ':'

I've updated the gist at https://gist.github.com/joaolsilva/d24b7bf5602f0d6615eb5d385fd0ffdc with the new URL.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 22, 2018

I'm inclined to do nothing here.

That's a pretty blatant spec violation, even if browsers accept it. But browsers accepting too much has led to security problems in the past, so I'm not eager to go down that road.

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jun 22, 2018

@joaolsilva

This comment has been minimized.

joaolsilva commented Jun 22, 2018

@bradfitz , what can a Go program do to override this behavior and still use net/http? All other http libraries for Go that I've tried had significant issues (not thread-safe, etc.).

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jun 22, 2018

The original URL fixed the header. The following URL still has the same error:

@joaolsilva thanks for the new URL. Perhaps might you have noticed in the self-contained repro in #21290 (comment)
screen shot 2018-06-22 at 1 44 33 pm
if possible, for posterity please update that code to reproduce your bug so that in the future when someone is trying to fix it they don't rely on a server that can fix its response as the first URL did.

@joaolsilva

This comment has been minimized.

joaolsilva commented Jun 22, 2018

if possible, for posterity please update that code to reproduce your bug so that in the future when someone is trying to fix it they don't rely on a server that can fix its response as the first URL did.

I can't (or at least I don't know how). The error originates from net/textproto/reader.go. The ReadMIMEHeaderfunction fails if it can't find a ':' in the header, however setting a header using w.Header().Set() will write well formed headers.

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jun 23, 2018

Perfect, thank you for the prescription @joaolsilva! Here is the self contained repro for posterity

package main

import (
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
)

func main() {
	log.SetFlags(0)
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		conn, bufw, _ := w.(http.Hijacker).Hijack()
		conn.Write([]byte(
`HTTP/1.0 200 OK
Content-Length: 13
Content-Disposition = attachment; filename=file.asc


Hello, World!`))
                bufw.Flush()
                conn.Close()
	}))
	defer cst.Close()

	resp, err := http.Get(cst.URL)
	if err != nil {
		log.Fatalf("Failed to perform plain get: %v", err)
	}
	_, _ = ioutil.ReadAll(resp.Body)
	_ = resp.Body.Close()
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment