code review 6210067: net/http: distinguish between query string and body par...

net/http: distinguish between query string and body parameters, add a
body-only (POST/PUT) parameter accessor, and prioritize POST/PUT body
over the URL query string in FormValue.

Fixes issue 3630.

[pmylund, 2012-05-18 07:06:13]

Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org, rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[pmylund, 2012-05-18 07:08:51]

I'm a little split here:

 - req.FormValue("foo") still "just works" (prioritizing body values)
 - Adds the body-only value accessor req.BodyFormValue("foo") (necessary?)
 - BREAKS req.Form["foo"] where body == foo=bar, and adds req.BodyForm["foo"]
 - BREAKS req.Form["both"] where query?both=x and body == both=y
 - Doesn't incur double overhead for POST requests (with a lot of data)

Is it significant that you can't interchangeably use query and body params in
r.Form (when FormValue still works)? Should I try not to break it? If so, would
you prefer I turn it into a slice of pointers, or copy the contents (even for
large POST requests)?

[dsymonds, 2012-05-18 07:29:48]

On Fri, May 18, 2012 at 5:08 PM,  <patrick@patrickmn.com> wrote:

>  - BREAKS req.Form["foo"] where body == foo=bar, and adds
> req.BodyForm["foo"]
>  - BREAKS req.Form["both"] where query?both=x and body == both=y

You cannot do this. It violates the contract we made with Go 1
(http://golang.org/doc/go1compat.html). This is a new feature; it may
not change the behaviour of existing code.


Dave.

[pmylund, 2012-05-18 07:31:09]

I suspected. I'll change and resubmit. Thanks.

On Fri, May 18, 2012 at 9:29 AM, David Symonds <dsymonds@golang.org> wrote:
> On Fri, May 18, 2012 at 5:08 PM,  <patrick@patrickmn.com> wrote:
>
>>  - BREAKS req.Form["foo"] where body == foo=bar, and adds
>> req.BodyForm["foo"]
>>  - BREAKS req.Form["both"] where query?both=x and body == both=y
>
> You cannot do this. It violates the contract we made with Go 1
> (http://golang.org/doc/go1compat.html). This is a new feature; it may
> not change the behaviour of existing code.
>
>
> Dave.

[pmylund, 2012-05-18 08:34:20]

Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[pmylund, 2012-05-18 08:39:21]

This still changes the ordering of POST vs. GET values (see issue 3630.) This
arguably falls under both the unspecified behavior and security categories of
the compatibility contract. This _only_ breaks anything if somebody is actually
counting on getting a parameter with the same name from the query string before
the body.

Is it worth reinventing BodyForm's url.Values to hold pointers to strings,
making the behavior of Form vs. BodyForm somewhat inconsistent?

[pmylund, 2012-05-18 08:55:29]

Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[pmylund, 2012-05-18 09:14:27]

Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[rodrigo.moraes, 2012-05-18 13:55:54]

http://codereview.appspot.com/6210067/diff/11001/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

http://codereview.appspot.com/6210067/diff/11001/src/pkg/net/http/request.go#...
src/pkg/net/http/request.go:628: }
After this, add a "if r.BodyForm == nil { r.BodyForm, e =
url.ParseQuery(string(b)) [...] }"

(expect that BodyForm could be set already).

[rodrigo.moraes, 2012-05-18 14:26:00]

Quick take: http://codereview.appspot.com/6208080

[rsc, 2012-05-18 15:40:15]

Please call BodyForm PostForm.

Thanks.
Russ

[pmylund, 2012-05-18 17:18:25]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, rodrigo.moraes@gmail.com
(cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[pmylund, 2012-05-18 17:21:06]

On 2012/05/18 17:18:25, pmylund wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:dsymonds@golang.org,
mailto:rodrigo.moraes@gmail.com
> (cc: mailto:bradfitz@golang.org, mailto:golang-dev@googlegroups.com,
mailto:rsc@golang.org),
> 
> Please take another look.

FormValue will still return a query parameter value if parsing the body fails.
This is the current behavior, too. Should this fail?

I noticed a few fringe cases where FormValue would call ParseForm twice,
ParseForm wouldn't be idempotent, or ParseForm wouldn't always initialize
Form/PostForm (which is the behavior the docs hint at.) I added a test for these
(nil POST body and nil URL.)

[rodrigo.moraes, 2012-05-18 20:20:14]

On 2012/05/18 17:21:06, pmylund wrote:
> FormValue will still return a query parameter value if parsing the body fails.
> This is the current behavior, too. Should this fail?

No, that is fine. It loaded the stuff it could.

[pmylund, 2012-05-18 20:25:03]

On 2012/05/18 20:20:14, rodrigo.moraes wrote:
> On 2012/05/18 17:21:06, pmylund wrote:
> > FormValue will still return a query parameter value if parsing the body
fails.
> > This is the current behavior, too. Should this fail?
> 
> No, that is fine. It loaded the stuff it could.

Indeed, but it presents the same security issue as prioritizing query string
parameters over body ones did: an application might use a query string parameter
instead of the body one it normally uses in some requests.

This one seems quite unlikely though, and probably unlikely enough to not
warrant erroring out prematurely, I agree.

[pmylund, 2012-05-18 21:03:04]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, rodrigo.moraes@gmail.com
(cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[rodrigo.moraes, 2012-05-18 21:06:48]

On 2012/05/18 20:25:03, pmylund wrote:
> Indeed, but it presents the same security issue as prioritizing query string
> parameters over body ones did: an application might use a query string
parameter
> instead of the body one it normally uses in some requests.

No, because now PostForm will exist. :)

[pmylund, 2012-05-20 02:17:59]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, rodrigo.moraes@gmail.com
(cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[pmylund, 2012-05-20 02:18:56]

Sorry for the many submissions. Should've spotted everything now.

[pmylund, 2012-05-29 16:12:57]

Hey rsc,

Would you like me to revise this? Move less code around? Make the test neater?

P

[rsc, 2012-05-29 16:19:37]

It would be nice to have smaller diffs, yes. parsePostForm should at
least be moved down a function, so that the code is not moving as far.

Also, either you are changing the meaning of the Form map, which is
not something we can do, or the new code in the FormValue method is
unnecessary. I hope it is the latter, in which case please remove it.

Russ

[pmylund, 2012-05-29 16:30:22]

On 2012/05/29 16:19:37, rsc wrote:
> It would be nice to have smaller diffs, yes. parsePostForm should at
> least be moved down a function, so that the code is not moving as far.
> 
> Also, either you are changing the meaning of the Form map, which is
> not something we can do, or the new code in the FormValue method is
> unnecessary. I hope it is the latter, in which case please remove it.
> 
> Russ

The behavior does change slightly, but this arguably falls under both
"undefined" and "security". The new code ensures body/post values precede query
values. Could you clarify if this is desired/only a body-value accessor is
needed? (ref. issue 3630)

I'm already adding body values to r.Form before query values, so the FormValue
addition _is_ redundant in most cases. Nevertheless, the behavior does change.

[rsc, 2012-05-29 16:32:21]

On Tue, May 29, 2012 at 12:30 PM,  <patrick@patrickmn.com> wrote:
> The behavior does change slightly, but this arguably falls under both
> "undefined" and "security". The new code ensures body/post values
> precede query values. Could you clarify if this is desired/only a
> body-value accessor is needed? (ref. issue 3630)

It's okay that the order in r.Form changes (post first, then url).

> I'm already adding body values to r.Form before query values, so the
> FormValue addition _is_ redundant in most cases. Nevertheless, the
> behavior does change.

Given that r.Form changes, I don't understand why the FormValue edit is needed.

Russ

[pmylund, 2012-05-29 16:50:45]

On 2012/05/29 16:32:21, rsc wrote:
> On Tue, May 29, 2012 at 12:30 PM,  <mailto:patrick@patrickmn.com> wrote:
> > The behavior does change slightly, but this arguably falls under both
> > "undefined" and "security". The new code ensures body/post values
> > precede query values. Could you clarify if this is desired/only a
> > body-value accessor is needed? (ref. issue 3630)
> 
> It's okay that the order in r.Form changes (post first, then url).
> 

Ok.

> > I'm already adding body values to r.Form before query values, so the
> > FormValue addition _is_ redundant in most cases. Nevertheless, the
> > behavior does change.
> 
> Given that r.Form changes, I don't understand why the FormValue edit is
needed.
> 
> Russ

It's not. It was to ensure "body values precede query ones" in corner cases e.g.
someone initializes r.Form by themselves, and for clarity. But agreed, this is
unnecessary.

Will change to reflect. Thanks!

[pmylund, 2012-05-30 13:57:00]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, rodrigo.moraes@gmail.com
(cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[rsc, 2012-06-26 00:14:55]

I'm sorry for dropping this. I think it's in good shape, just a few small things
below.

http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go#...
src/pkg/net/http/request.go:603: func copyValues(src, dest url.Values) {
Copy functions in Go always take dst, src, just like the = statement. Please fix
this here and then update the call sites.

http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go#...
src/pkg/net/http/request.go:604: // TODO: make this smoother.
Please be more precise about what smoother means, or delete the comment.

http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go#...
src/pkg/net/http/request.go:736: // POST and PUT body parameters precede URL
query string values.
s/precede/take precedence over/

[pmylund, 2012-06-26 00:22:43]

On 2012/06/26 00:14:55, rsc wrote:
> I'm sorry for dropping this. I think it's in good shape, just a few small
things
> below.
> 
> http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go
> File src/pkg/net/http/request.go (right):
> 
>
http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go#...
> src/pkg/net/http/request.go:603: func copyValues(src, dest url.Values) {
> Copy functions in Go always take dst, src, just like the = statement. Please
fix
> this here and then update the call sites.
> 
>
http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go#...
> src/pkg/net/http/request.go:604: // TODO: make this smoother.
> Please be more precise about what smoother means, or delete the comment.
> 
>
http://codereview.appspot.com/6210067/diff/24002/src/pkg/net/http/request.go#...
> src/pkg/net/http/request.go:736: // POST and PUT body parameters precede URL
> query string values.
> s/precede/take precedence over/

No worries. I've been kind of pondering how to make the test and the copying
nicer, but couldn't come up with anything that wasn't more expensive, or less
intuitive.

I'll update. As for the smoother comment, I'm not totally sure either (it was
there before.) I guess the whole copying aspect isn't too "smooth", but alas.
Will remove unless Brad objects.

[pmylund, 2012-06-26 00:34:03]

Hello rsc@golang.org (cc: bradfitz@golang.org, dsymonds@golang.org,
golang-dev@googlegroups.com, rodrigo.moraes@gmail.com),

Please take another look.

[rsc, 2012-06-26 00:41:07]

LGTM

[rsc, 2012-06-26 00:41:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=0dac18e695f3 ***

net/http: provide access to POST-only form values

Fixes issue 3630.

R=rsc
CC=bradfitz, dsymonds, golang-dev, rodrigo.moraes
http://codereview.appspot.com/6210067

Committer: Russ Cox <rsc@golang.org>