code review 8731044: go.net/websocket: allow server configurable

go.net/websocket: allow server configurable

Add websocket.Server to configure WebSocket server handler.

- Config.Header is additional headers to send, so you can use it
  to send cookies or so.
  To read cookies, you can use Conn.Request().Header.
- factor out Handshake.
  You can set func to check origin, subprotocol etc.
  Handler checks origin by default.

Fixes issue 4198.
Fixes issue 5178.

[ukai, 2013-04-13 05:29:37]

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

I'd like you to review this change to
https://code.google.com/p/go.net

[mikio, 2013-04-20 06:53:31]

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go
File websocket/hybi.go (right):

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go#newcode531
websocket/hybi.go:531: func GetOrigin(config *Config, req *http.Request)
(*url.URL, error) {
just Origin; no redundant Get prefix for getter looks go idiomatic.

https://codereview.appspot.com/8731044/diff/5001/websocket/server.go
File websocket/server.go (right):

https://codereview.appspot.com/8731044/diff/5001/websocket/server.go#newcode62
websocket/server.go:62: // Server is a server of a WebSocket.
Server represents a ...

[crobin, 2013-04-23 14:27:58]

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go
File websocket/hybi.go (right):

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go#newcode531
websocket/hybi.go:531: func GetOrigin(config *Config, req *http.Request)
(*url.URL, error) {
Also the header is not mandatory, the function shouldn't fail but return nil if
the header is not here. Same thing if the Origin is the string "null", as it is
an allowed value according to the RFC but ParseRequestURI will fail on simple
strings with no scheme and port

[ukai, 2013-05-01 07:31:19]

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go
File websocket/hybi.go (right):

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go#newcode531
websocket/hybi.go:531: func GetOrigin(config *Config, req *http.Request)
(*url.URL, error) {
On 2013/04/20 06:53:31, mikio wrote:
> just Origin; no redundant Get prefix for getter looks go idiomatic.

Done.

https://codereview.appspot.com/8731044/diff/5001/websocket/hybi.go#newcode531
websocket/hybi.go:531: func GetOrigin(config *Config, req *http.Request)
(*url.URL, error) {
On 2013/04/23 14:27:58, crobin wrote:
> Also the header is not mandatory, the function shouldn't fail but return nil
if
> the header is not here. Same thing if the Origin is the string "null", as it
is
> an allowed value according to the RFC but ParseRequestURI will fail on simple
> strings with no scheme and port

If you don't need to origin check, please use websocket.Server rather than
websocket.Handler.

https://codereview.appspot.com/8731044/diff/5001/websocket/server.go
File websocket/server.go (right):

https://codereview.appspot.com/8731044/diff/5001/websocket/server.go#newcode62
websocket/server.go:62: // Server is a server of a WebSocket.
On 2013/04/20 06:53:31, mikio wrote:
> Server represents a ...

Done.

[ukai, 2013-05-01 07:31:28]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, crobin@nekoo.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2013-05-10 10:39:10]

LGTM

sorry for the late response.

not sure IssueTracker can handle multiple issue numbers.
perhaps two-lines might be safe.
Fixes issue NNNN.
Fixes issue NNNN.

[ukai, 2013-05-10 13:53:07]

On 2013/05/10 10:39:10, mikio wrote:
> LGTM

Thanks!

 > sorry for the late response.
> 
> not sure IssueTracker can handle multiple issue numbers.
> perhaps two-lines might be safe.
> Fixes issue NNNN.
> Fixes issue NNNN.

Updated the description.

[mikio, 2013-05-12 04:50:18]

*** Submitted as
https://code.google.com/p/go/source/detail?r=1e65ca1b2499&repo=net ***

go.net/websocket: allow server configurable

Add websocket.Server to configure WebSocket server handler.

- Config.Header is additional headers to send, so you can use it
  to send cookies or so.
  To read cookies, you can use Conn.Request().Header.
- factor out Handshake.
  You can set func to check origin, subprotocol etc.
  Handler checks origin by default.

Fixes issue 4198.
Fixes issue 5178.

R=golang-dev, mikioh.mikioh, crobin
CC=golang-dev
https://codereview.appspot.com/8731044

Committer: Mikio Hara <mikioh.mikioh@gmail.com>

[everton.marques, 2013-05-13 20:56:22]

I think this commit affected javascript websocket client code running in Google
Chrome.

My javascript websocket client is now issuing:

WebSocket connection to 'ws://127.0.0.3:8888/' failed: Invalid UTF-8 sequence in
header value 

I've also found this:

http://stackoverflow.com/questions/16507595/websocket-output-received-by-brow...

I got new go websocket code with:

go get code.google.com/p/go.net/websocket

How can I "go get" an older version of code.google.com/p/go.net/websocket to
confirm?

[johan.bilien, 2013-05-14 01:15:53]

On 2013/05/13 20:56:22, everton.marques wrote:
> I think this commit affected javascript websocket client code running in
Google
> Chrome.
> 
> My javascript websocket client is now issuing:
> 
> WebSocket connection to 'ws://127.0.0.3:8888/' failed: Invalid UTF-8 sequence
in
> header value 
> 
> I've also found this:
> 
>
http://stackoverflow.com/questions/16507595/websocket-output-received-by-brow...
> 
> I got new go websocket code with:
> 
> go get code.google.com/p/go.net/websocket
> 
> How can I "go get" an older version of code.google.com/p/go.net/websocket to
> confirm?

I don't know how to do it with go get but you can revert manually with  hg
update -c 48:9c2295dac419 from ${GOPATH}//src/code.google.com/p/go.net

I was having the exact same problem, and reverting fixed it for me.

[rog, 2013-05-15 14:38:30]

This CL broke some of our existing code.

I wonder if the default behaviour should not check
origin (as was the case previously, I believe).
Just checking if the URL parses isn't actually very useful
and some websocket clients don't send a well formatted URL
by default.


On 12 May 2013 05:50,  <mikioh.mikioh@gmail.com> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=1e65ca1b2499&repo=net ***
>
> go.net/websocket: allow server configurable
>
> Add websocket.Server to configure WebSocket server handler.
>
> - Config.Header is additional headers to send, so you can use it
>   to send cookies or so.
>   To read cookies, you can use Conn.Request().Header.
> - factor out Handshake.
>   You can set func to check origin, subprotocol etc.
>   Handler checks origin by default.
>
> Fixes issue 4198.
> Fixes issue 5178.
>
> R=golang-dev, mikioh.mikioh, crobin
> CC=golang-dev
> https://codereview.appspot.com/8731044
>
> Committer: Mikio Hara <mikioh.mikioh@gmail.com>
>
>
>
> https://codereview.appspot.com/8731044/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[mikio, 2013-05-15 15:10:27]

On Wed, May 15, 2013 at 11:38 PM, roger peppe <rogpeppe@gmail.com> wrote:

> This CL broke some of our existing code.

Sorry about that. But...

> I wonder if the default behaviour should not check
> origin (as was the case previously, I believe).

Just reread section 10 of RFC 6455 and still seem that it's hard
to deal w/ security considerations whether opt-out is better or not.

[rog, 2013-05-15 18:14:15]

What security issues does parsing the Origin header address?
On May 15, 2013 4:10 PM, "Mikio Hara" <mikioh.mikioh@gmail.com> wrote:

> On Wed, May 15, 2013 at 11:38 PM, roger peppe <rogpeppe@gmail.com> wrote:
>
> > This CL broke some of our existing code.
>
> Sorry about that. But...
>
> > I wonder if the default behaviour should not check
> > origin (as was the case previously, I believe).
>
> Just reread section 10 of RFC 6455 and still seem that it's hard
> to deal w/ security considerations whether opt-out is better or not.
>

[mikio, 2013-05-16 01:12:57]

On Thu, May 16, 2013 at 3:14 AM, roger peppe <rogpeppe@gmail.com> wrote:

> What security issues does parsing the Origin header address?

I'm not sure what's the previous behavior you are saying, but perhaps you don't
want origin validation at checkOrigin when apps use websocket.Handler instead
of websocket.Server instead? Right? Hm, I'm sure I'm missing something
important.

Well, can you please file an issue? Thanks.

Ah, you are saying drop
>
> On May 15, 2013 4:10 PM, "Mikio Hara" <mikioh.mikioh@gmail.com> wrote:
>>
>> On Wed, May 15, 2013 at 11:38 PM, roger peppe <rogpeppe@gmail.com> wrote:
>>
>> > This CL broke some of our existing code.
>>
>> Sorry about that. But...
>>
>> > I wonder if the default behaviour should not check
>> > origin (as was the case previously, I believe).
>>
>> Just reread section 10 of RFC 6455 and still seem that it's hard
>> to deal w/ security considerations whether opt-out is better or not.

[ukai, 2013-05-16 02:23:43]

On Wed, May 15, 2013 at 11:38 PM, roger peppe <rogpeppe@gmail.com> wrote:

> This CL broke some of our existing code.


> I wonder if the default behaviour should not check
> origin (as was the case previously, I believe).
> Just checking if the URL parses isn't actually very useful
> and some websocket clients don't send a well formatted URL
> by default.
>

 How did this CL break existing code with origin check?

Before this CL, there is no way to skip origin check. (it was done in
ReadHandshake).
This CL makes enable to create websocket server without origin check.
Use websocket.Server without Handshake if you don't need to check origin,
like
   http.Handle("/ws", websocket.Server{Handler: wsHandler})

Anyway, why do you use websocket? I think major client of websocket is
webbrowser, which should send proper origin.
what websocket client do you use?

Thanks,
ukai



>
> On 12 May 2013 05:50,  <mikioh.mikioh@gmail.com> wrote:
> > *** Submitted as
> > https://code.google.com/p/go/source/detail?r=1e65ca1b2499&repo=net ***
> >
> > go.net/websocket: allow server configurable
> >
> > Add websocket.Server to configure WebSocket server handler.
> >
> > - Config.Header is additional headers to send, so you can use it
> >   to send cookies or so.
> >   To read cookies, you can use Conn.Request().Header.
> > - factor out Handshake.
> >   You can set func to check origin, subprotocol etc.
> >   Handler checks origin by default.
> >
> > Fixes issue 4198.
> > Fixes issue 5178.
> >
> > R=golang-dev, mikioh.mikioh, crobin
> > CC=golang-dev
> > https://codereview.appspot.com/8731044
> >
> > Committer: Mikio Hara <mikioh.mikioh@gmail.com>
> >
> >
> >
> > https://codereview.appspot.com/8731044/
> >
> > --
> >
> > ---You received this message because you are subscribed to the Google
> Groups
> > "golang-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to golang-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >
>

[shurcooL, 2015-03-01 23:15:38]

I spotted a minor issue with the comment formatting, I'd like to point it out.

https://codereview.appspot.com/8731044/diff/16001/websocket/server.go
File websocket/server.go (right):

https://codereview.appspot.com/8731044/diff/16001/websocket/server.go#newcode107
websocket/server.go:107: //. that doesn't check origin in its Handshake.
There seems to be some problem with the comment content/formatting here.

"//. that doesn't"

There should not be a period after the two forward slashes. But it also feels
like some words are missing between "you could use Server" and "that doesn't",
maybe?

[songofacandy, 2015-03-02 01:19:47]

https://codereview.appspot.com/8731044/diff/16001/websocket/hybi.go
File websocket/hybi.go (right):

https://codereview.appspot.com/8731044/diff/16001/websocket/hybi.go#newcode540
websocket/hybi.go:540: if origin == "null" {
RFC 6455 1.3 says "for non-browser clients, this header field may be sent if..."
So I think "" should be nil origin, too.

[mikio, 2015-03-07 09:52:39]

thanks. in next time, please open a new issue.

On Mon, Mar 2, 2015 at 8:15 AM,  <shurcooL@gmail.com> wrote:
> I spotted a minor issue with the comment formatting, I'd like to point
> it out.
>
>
> https://codereview.appspot.com/8731044/diff/16001/websocket/server.go
> File websocket/server.go (right):
>
>
https://codereview.appspot.com/8731044/diff/16001/websocket/server.go#newcode107
> websocket/server.go:107: //. that doesn't check origin in its Handshake.
> There seems to be some problem with the comment content/formatting here.
>
> "//. that doesn't"
>
> There should not be a period after the two forward slashes. But it also
> feels like some words are missing between "you could use Server" and
> "that doesn't", maybe?
>
> https://codereview.appspot.com/8731044/

[mikio, 2015-03-07 09:53:23]

thanks, filed golang.org/issue/10102. in next time, please open a new issue.

On Mon, Mar 2, 2015 at 10:19 AM,  <songofacandy@gmail.com> wrote:
>
> https://codereview.appspot.com/8731044/diff/16001/websocket/hybi.go
> File websocket/hybi.go (right):
>
>
https://codereview.appspot.com/8731044/diff/16001/websocket/hybi.go#newcode540
> websocket/hybi.go:540: if origin == "null" {
> RFC 6455 1.3 says "for non-browser clients, this header field may be
> sent if..."
> So I think "" should be nil origin, too.
>
> https://codereview.appspot.com/8731044/