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/