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/http: document that ParseForm consumes Request.Body #35620

Open
mipnw opened this issue Nov 15, 2019 · 3 comments

Comments

@mipnw
Copy link

@mipnw mipnw commented Nov 15, 2019

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

go version go1.11.13 darwin/amd64
but looking at the code, it's clear the bug exists in master

Does this issue reproduce with the latest release?

yes it would

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

Irrelevant. Looking at net/http/request.go we can see the bug. In parsePostForm(), the body of the http.Request is read but not restored. Therefore one cannot read from the request body again after calling this API, which either is intentional in which case it's a documentation bug as the comment on the function does not indicate callers should not call this unless they're done with the body, or it's not intentional in which case it's a functional bug.

One should be able to call ParseForm() and still read the body of an http.Request later.

What did you do?

package http_test

import (
	"bytes"
	"io"
	"io/ioutil"
	"net/http"
	"strings"
	"testing"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

// Demonstration of bug in 
// https://github.com/golang/go/blob/3409ce39bfd7584523b7a8c150a310cea92d879d/src/net/http/request.go
// specifically the http.Request.ParseForm() API
//
func TestProofOfNetHttpBug(t *testing.T) {
	input := `form-data; name="key" value`
	destroyedInput := ""

	// create an http POST request with a Content-Type of application/x-www-form-urlencoded
	req, err := http.NewRequest(
		"POST",
		"https://doesntmatter.com",
		ioutil.NopCloser(bytes.NewBufferString(input)))
	require.Nil(t, err)
	req.Header = map[string][]string {
		"Content-Type": {"application/x-www-form-urlencoded"},
	}

	// We've set a body on the request, we show we the body is as expected
	body, err := snoop(&req.Body)
	assert.Nil(t, err)
	assert.Equal(t, input, body) 

	// For the skeptics, we demonstrate snoop is idempotent, we simply snoop again and show the body is still there
	body, err = snoop(&req.Body)
	assert.Nil(t, err)
	assert.Equal(t, input, body)

	// Now let's call the problem API
	assert.Nil(t, req.ParseForm())

	// Kaboom, the call to parseForm destroyed the body of the request
	body, err = snoop(&req.Body)
	assert.Nil(t, err)
	assert.Equal(t, destroyedInput, body)
}

// This test also shows the proper way to read twice from an io.ReadCloser.
func TestReadCloser(t *testing.T) {
	input := "abcdef"
	destroyedInput := ""

	// What not to do: copying a readcloser
	var reader1 io.ReadCloser = ioutil.NopCloser(strings.NewReader(input))
	var reader2 io.Reader = reader1
	b2, err := ioutil.ReadAll(reader2)
	assert.Nil(t, err)
	assert.Equal(t, input, string(b2))
	b1, err := ioutil.ReadAll(reader1)
	assert.Nil(t, err)
	reader1.Close()
	assert.Equal(t, destroyedInput, string(b1))
	// Kaboom: the input was destroyed

	// What to do instead: restore the reader from the buffer you read with ioutil.ReadAll
	reader1 = ioutil.NopCloser(strings.NewReader(input))
	b1, err = ioutil.ReadAll(reader1)
	assert.Nil(t, err)
	reader1.Close()
	reader1 = ioutil.NopCloser(bytes.NewBuffer(b1))
	b1, err = ioutil.ReadAll(reader1) // Now we prove this works by reading a second time
	assert.Nil(t, err)
	reader1.Close()
	assert.Equal(t, input, string(b1)) 
	// Tada: the input is read a second time, this works!
}


// snoop reads from an io.ReadCloser and restores it so it can be read again
func snoop(body *io.ReadCloser) (string, error) {
	if body != nil && *body != nil {
		result, err := ioutil.ReadAll(*body)
		(*body).Close()

		if err != nil {
			return "", err
		}
		
		*body = ioutil.NopCloser(bytes.NewReader(result))
		return string(result), nil
	}
	return "", nil
}

What did you expect to see?

I expected the first test to fail.

What did you see instead?

The first test passed, showing there's a bug.

@bradfitz bradfitz changed the title Bug in net/http/Request when calling ParseForm net/http: document that ParseForm consumes Request.Body Nov 15, 2019
@bradfitz bradfitz added this to the Backlog milestone Nov 15, 2019
@andybons andybons added the NeedsFix label Nov 15, 2019
@golang golang deleted a comment Nov 17, 2019
@marti1125

This comment has been minimized.

Copy link

@marti1125 marti1125 commented Nov 18, 2019

Is it a good first issue? I never contribute with golang 🤔

@marti1125

This comment has been minimized.

Copy link

@marti1125 marti1125 commented Nov 20, 2019

Hi @mipnw your example Is it like this?
https://astaxie.gitbooks.io/build-web-application-with-golang/en/04.1.html
ParseForm() from handler, could you tell more in which context happened?

@mipnw

This comment has been minimized.

Copy link
Author

@mipnw mipnw commented Nov 22, 2019

@marti1125 My example is very close to the code pasted in this bug's description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.