code review 1669056: Fix client code to insert spaces into keys per new sp...

Fix client code to insert spaces into keys per new spec, eliminating
    condition of leading and trailing spaces that confuse the HTTP
    header parsing routines.

[Bill Neubauer, 2010-07-25 15:00:32]

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

I'd like you to review this change.

[Bill Neubauer, 2010-07-25 15:05:33]

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

Please take another look.

[rsc, 2010-07-25 21:40:20]

What if key1 or key2 contains a " character?
It looks like it can happen: basically every
printable character except digits and space
are in the set of possible inserted characters.

You have to wonder what the websocket
authors were taking.

Russ

[rsc, 2010-07-25 21:46:51]

adding ukai

On Sun, Jul 25, 2010 at 14:40, Russ Cox <rsc@golang.org> wrote:
> What if key1 or key2 contains a " character?
> It looks like it can happen: basically every
> printable character except digits and space
> are in the set of possible inserted characters.

it looks like strconv.Quote will do the right thing:
the only relevant characters in the string are " and \
and it will convert them to \" and \\ as expected by http.
i can't tell from the http rfc (2616) whether getting
"abc" and abc should be the same (i.e. whether the
http package should be the one escaping or unescaping
these things) but for now let's assume it is the app's job.

russ

[rsc, 2010-07-25 21:47:09]

actually adding ukai this time; not just talk.

On Sun, Jul 25, 2010 at 14:46, Russ Cox <rsc@golang.org> wrote:
> adding ukai
>
> On Sun, Jul 25, 2010 at 14:40, Russ Cox <rsc@golang.org> wrote:
>> What if key1 or key2 contains a " character?
>> It looks like it can happen: basically every
>> printable character except digits and space
>> are in the set of possible inserted characters.
>
> it looks like strconv.Quote will do the right thing:
> the only relevant characters in the string are " and \
> and it will convert them to \" and \\ as expected by http.
> i can't tell from the http rfc (2616) whether getting
> "abc" and abc should be the same (i.e. whether the
> http package should be the one escaping or unescaping
> these things) but for now let's assume it is the app's job.
>
> russ
>

[Bill Neubauer, 2010-07-25 22:19:00]

I think the issues are even more unclear with the websocket header. Since
the spec dictates that the entire key has to fit on a single line followed
by the CRLF, that is providing a contradicting definition on what RFC 2616
says is allowed. This could cause problems if the header went through a
proxy, and the proxy decided to rewrite the header using line continuation.
That is allowed within the scope of RFC 2616, but the proxy has no awareness
of websocket, but it would result in a header that is no longer websocket
compliant.


On Mon, Jul 26, 2010 at 6:47 AM, Russ Cox <rsc@golang.org> wrote:

> actually adding ukai this time; not just talk.
>
> On Sun, Jul 25, 2010 at 14:46, Russ Cox <rsc@golang.org> wrote:
> > adding ukai
> >
> > On Sun, Jul 25, 2010 at 14:40, Russ Cox <rsc@golang.org> wrote:
> >> What if key1 or key2 contains a " character?
> >> It looks like it can happen: basically every
> >> printable character except digits and space
> >> are in the set of possible inserted characters.
> >
> > it looks like strconv.Quote will do the right thing:
> > the only relevant characters in the string are " and \
> > and it will convert them to \" and \\ as expected by http.
> > i can't tell from the http rfc (2616) whether getting
> > "abc" and abc should be the same (i.e. whether the
> > http package should be the one escaping or unescaping
> > these things) but for now let's assume it is the app's job.
> >
> > russ
> >
>

[Bill Neubauer, 2010-07-26 02:00:43]

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

Please take another look.

[ukai, 2010-07-26 05:44:43]

Looks good go me.

[ukai, 2010-07-26 05:46:46]

http://codereview.appspot.com/1669056/diff/15001/16001
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/1669056/diff/15001/16001#newcode137
src/pkg/websocket/client.go:137: //     positions.
positions other than the start or end of the string.

http://codereview.appspot.com/1669056/diff/15001/16002
File src/pkg/websocket/websocket_test.go (right):

http://codereview.appspot.com/1669056/diff/15001/16002#newcode149
src/pkg/websocket/websocket_test.go:149: // The last runs of this create keys
with trailing spaces that need to be
.. trailing spaces that should not be generated by the client (?)

[rsc1, 2010-07-29 01:27:27]

LGTM with ukai's suggestions

[Bill Neubauer, 2010-07-29 14:30:35]

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

Please take another look.

[rsc1, 2010-07-30 03:39:09]

http://codereview.appspot.com/1669056/diff/21001/22001
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/1669056/diff/21001/22001#newcode128
src/pkg/websocket/client.go:128: //     positions other than the start or end of
the string.
i think you put this comment on the wrong code.
it belongs on step 22

[Bill Neubauer, 2010-07-30 04:17:53]

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

Please take another look.

[rsc, 2010-07-30 19:27:05]

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

websocket: fix bug involving spaces in header keys

R=rsc, ukai
CC=golang-dev
http://codereview.appspot.com/1669056

Committer: Russ Cox <rsc@golang.org>