|
|
Created:
12 years ago by pmylund Modified:
11 years, 10 months ago Reviewers:
CC:
rsc, bradfitz, dsymonds, golang-dev, rodrigo.moraes Visibility:
Public. |
Descriptionnet/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.
Patch Set 1 #Patch Set 2 : diff -r 85e153815747 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 85e153815747 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 85e153815747 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 85e153815747 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 85e153815747 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 7 : diff -r 85e153815747 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r 85e153815747 https://go.googlecode.com/hg/ #Patch Set 9 : diff -r 3c702dae5f50 https://go.googlecode.com/hg/ #Patch Set 10 : diff -r 1195a9780464 https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 11 : diff -r c88692a626e9 https://go.googlecode.com/hg/ #
MessagesTotal messages: 30
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/
Sign in to reply to this message.
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)?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
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?
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
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).
Sign in to reply to this message.
Quick take: http://codereview.appspot.com/6208080
Sign in to reply to this message.
Please call BodyForm PostForm. Thanks. Russ
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.)
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. :)
Sign in to reply to this message.
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.
Sign in to reply to this message.
Sorry for the many submissions. Should've spotted everything now.
Sign in to reply to this message.
Hey rsc, Would you like me to revise this? Move less code around? Make the test neater? P
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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!
Sign in to reply to this message.
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.
Sign in to reply to this message.
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/
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello rsc@golang.org (cc: bradfitz@golang.org, dsymonds@golang.org, golang-dev@googlegroups.com, rodrigo.moraes@gmail.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|