This is a security hardening measure against HTTP request smuggling.
Thank you to ZeddYu for reporting this issue.
We weren't parsing things correctly anyway, allowing "identity" to be
combined with "chunked", and ignoring any Transfer-Encoding header past
the first. This is a delicate security surface that already broke
before, just be strict and don't add complexity to support cases not
observed in the wild (nginx removed "identity" support [1] and multiple
TE header support [2]) and removed by RFC 7230 (see page 81).
It'd probably be good to also drop support for anything other than
"chunked" in outbound TE headers, as "identity" is not a thing anymore,
and we are probably off-spec for anything other than "chunked", but it
should not be a security concern, so leaving it for now. See #38867.
[1]: https://hg.nginx.org/nginx/rev/fe5976aae0e3
[2]: https://hg.nginx.org/nginx/rev/aca005d232ff
Change-Id: If17d0827f9c6167a0b19a158e2bc5844ec803288
Reviewed-on: https://go-review.googlesource.com/c/go/+/231418
Reviewed-by: Katie Hockman <katie@golang.org>
Some were never used, and some haven't been used for years.
One exception is net/http's readerAndCloser, which was only used in a
test. Move it to a test file.
While at it, remove a check in regexp that could never fire; the field
is an uint32, so it can never be negative.
Change-Id: Ia2200f6afa106bae4034045ea8233b452f38747b
Reviewed-on: https://go-review.googlesource.com/c/go/+/192621
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
A handful of packages were reimplementing IsExported, so use
token.IsExported instead. This caused the deps test to fail for net/rpc.
However, net/rpc deals with Go types, and go/token is light and fairly
low-level in terms of Go tooling packages, so that's okay.
While at it, replace all uses of ast.IsExported with token.IsExported.
This is more consistent, and also means that the import graphs are
leaner. A couple of files no longer need to import go/ast, for example.
We can't get rid of cmd/compile/internal/types.IsExported, as the
compiler can only depend on go/token as of Go 1.4. However, gc used
different implementations in a couple of places, so consolidate the use
of types.IsExported there.
Finally, we can't get rid of the copied IsExported implementation in
encoding/gob, as go/token depends on it as part of a test. That test
can't be an external test either, so there's no easy way to break the
import cycle.
Overall, this removes about forty lines of unnecessary code.
Change-Id: I86a475b7614261e6a7b0b153d5ca02b9f64a7b2d
Reviewed-on: https://go-review.googlesource.com/c/go/+/172037
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
There are cases where HTTP message specifies the Trailer header
but not the Transfer-Encoding = chunked. The existing
implementation would return an error in those cases, without
returning also the message itself.
Instead, it would be preferable to let the library user decide when
the message is valid or not.
This change makes the fixTrailer() function not to return an error
and to keep the Trailer value in the Response.Header map but not
populate Response.Trailer.
Fixes#27197
Change-Id: Ic1e96791fde97f31dc5ecb8de05c8e4f49465c2c
Reviewed-on: https://go-review.googlesource.com/c/145398
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Replace references to the obsoleted RFC 2616 with references to RFC
7230 through 7235, to avoid unnecessary confusion.
Obvious inconsistencies are marked with todo comments.
Updates #21974
Change-Id: I8fb4fcdd1333fc5193b93a2f09598f18c45e7a00
Reviewed-on: https://go-review.googlesource.com/94095
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Previously, golang.org/cl/75350 updated ReadMIMEHeader to ignore the
first header line when it begins with a leading space, as in the
following example:
GET / HTTP/1.1
Host: foo.com
Accept-Encoding: gzip
However, golang.org/cl/75350 changed ReadMIMEHeader's behavior for the
following example: before the CL it returned an error, but after the
CL it ignored the first line.
GET / HTTP/1.1
Host foo.com
Accept-Encoding: gzip
This change updates ReadMIMEHeader to always fail when the first header
line starts with a space. During the discussion for golang.org/cl/75350,
we realized we had three competing needs:
1. HTTP clients should accept malformed response headers when possible
(ignoring the malformed lines).
2. HTTP servers should reject all malformed request headers.
3. The net/textproto package is used by multiple protocols (most notably,
HTTP and SMTP) which have slightly different parsing semantics. This
complicates changes to net/textproto.
We weren't sure how to best fix net/textproto without an API change, but
it is too late for API changes in Go 1.10. We decided to ignore initial
lines that begin with spaces, thinking that would have the least impact on
existing users -- malformed headers would continue to parse, but the
initial lines would be ignored. Instead, golang.org/cl/75350 actually
changed ReadMIMEHeader to succeed in cases where it previously failed
(as in the above example).
Reconsidering the above two examples, there does not seem to be a good
argument to silently ignore ` Host: foo.com` but fail on ` Host foo.com`.
Hence, this change fails for *all* headers where the initial line begins
with a space.
Updates #22464
Change-Id: I68d3d190489c350b0bc1549735bf6593fe11a94c
Reviewed-on: https://go-review.googlesource.com/80055
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
A header line with leading whitespaces is not valid in HTTP as per
RFC7230. This change ignores these invalid lines in ReadMIMEHeader.
Updates #22464
Change-Id: Iff9f00380d28a9617a55ff7888a76fba82001402
Reviewed-on: https://go-review.googlesource.com/75350
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When writing the 'Connection: close' header based on response Close
attribute we also check if it is already in the headers scheduled
to be written and skip if necessary.
Fixes#19499
Change-Id: I92357344a37ae385454ec8006114fa4cfa585810
Reviewed-on: https://go-review.googlesource.com/38076
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reading a response with a status line like "HTTP/1.0 401 Unauthorized"
(with two spaces after the version) has been returning an error. Now the
extra space will be ignored.
Fixes#19989
Change-Id: I0c88a6ef7562ba80e2e2635be2070dd1b5b671a7
Reviewed-on: https://go-review.googlesource.com/40933
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Before: 8.9 seconds for go test -short
After: 2.8 seconds
There are still 250 tests without t.Parallel, but I got the important
onces using a script:
$ go test -short -v 2>&1 | go run ~/slowtests.go
Where slowtests.go is https://play.golang.org/p/9mh5Wg1nLN
The remaining 250 (the output lines from slowtests.go) all have a
reported duration of 0ms, except one 50ms test which has to be serial.
Where tests can't be parallel, I left a comment at the top saying why,
so people don't add t.Parallel later and get surprised at failures.
Updates #17751
Change-Id: Icbe32cbe2b996e23c89f1af6339287fa22af5115
Reviewed-on: https://go-review.googlesource.com/32684
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
The Transport's automatic gzip uncompression lost information in the
process (the compressed Content-Length, if known). Normally that's
okay, but it's not okay for reverse proxies which have to be able to
generate a valid HTTP response from the Transport's provided
*Response.
Reverse proxies should normally be disabling compression anyway and
just piping the compressed pipes though and not wasting CPU cycles
decompressing them. So also document that on the new Uncompressed
field.
Then, using the new field, fix Response.Write to not inject a bogus
"Connection: close" header when it doesn't see a transfer encoding or
content-length.
Updates #15366 (the http2 side remains, once this is submitted)
Change-Id: I476f40aa14cfa7aa7b3bf99021bebba4639f9640
Reviewed-on: https://go-review.googlesource.com/22671
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently only used by the client. The server is not yet wired up. A
TODO remains to document how it works server-side, once implemented.
Updates #14660
Change-Id: I27c2e74198872b2720995fa8271d91de200e23d5
Reviewed-on: https://go-review.googlesource.com/21496
Reviewed-by: Andrew Gerrand <adg@golang.org>
This is a subset of https://golang.org/cl/20022 with only the copyright
header lines, so the next CL will be smaller and more reviewable.
Go policy has been single space after periods in comments for some time.
The copyright header template at:
https://golang.org/doc/contribute.html#copyright
also uses a single space.
Make them all consistent.
Change-Id: Icc26c6b8495c3820da6b171ca96a74701b4a01b0
Reviewed-on: https://go-review.googlesource.com/20111
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The http package by default adds "Accept-Encoding: gzip" to outgoing
requests, unless it's a bad idea, or the user requested otherwise.
Only when the http package adds its own implicit Accept-Encoding header
does the http package also transparently un-gzip the response.
If the user requested part of a document (e.g. bytes 40 to 50), it appears
that Github/Varnish send:
range(gzip(content), 40, 50)
And not:
gzip(range(content, 40, 50))
The RFC 2616 set of replacements (with the purpose of
clarifying ambiguities since 1999) has an RFC about Range
requests (http://tools.ietf.org/html/rfc7233) but does not
mention the interaction with encodings.
Regardless of whether range(gzip(content)) or gzip(range(content)) is
correct, this change prevents the Go package from asking for gzip
in requests if we're also asking for Range, avoiding the issue.
If the user cared, they can do it themselves. But Go transparently
un-gzipping a fragment of gzip is never useful.
Fixes#8923
LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/155420044
