code review 5318072: websocket: return an error HTTP response for bad websoc...

websocket: return an error HTTP response for bad websocket request.

websocket spec had changed server-side requiements to return
an HTTP response with an appropriate error code (such as 400 Bad
Request) when it finds client did not send a handshake that matches
websocket protocol, rather than just closing connection.
It needs to flush out response before closing connection.
Fixes issues 2396.

[ukai, 2011-11-02 07:47:31]

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

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

[bradfitz, 2011-11-02 15:09:49]

LGTM

On Wed, Nov 2, 2011 at 12:47 AM, <ukai@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> websocket: return an error HTTP response for bad websocket request.
>
> websocket spec had changed server-side requiements to return
> an HTTP response with an appropriate error code (such as 400 Bad
> Request) when it finds client did not send a handshake that matches
> websocket protocol, rather than just closing connection.
> It needs to flush out response before closing connection.
>
> Please review this at
http://codereview.appspot.com/**5318072/<http://codereview.appspot.com/5318072/>
>
> Affected files:
>  M src/pkg/websocket/server.go
>  M src/pkg/websocket/websocket_**test.go
>
>
> Index: src/pkg/websocket/server.go
> ==============================**==============================**=======
> --- a/src/pkg/websocket/server.go
> +++ b/src/pkg/websocket/server.go
> @@ -20,6 +20,7 @@
>                fmt.Fprintf(buf, "Sec-WebSocket-Version: %s\r\n",
> SupportedProtocolVersion)
>                buf.WriteString("\r\n")
>                buf.WriteString(err.Error())
> +               buf.Flush()
>                return
>        }
>        if err != nil {
> @@ -34,12 +35,17 @@
>                fmt.Fprintf(buf, "HTTP/1.1 %03d %s\r\n", code,
> http.StatusText(code))
>                buf.WriteString("\r\n")
>                buf.WriteString(err.Error())
> +               buf.Flush()
>                return
>        }
>        config.Protocol = nil
>
>        err = hs.AcceptHandshake(buf.Writer)
>        if err != nil {
> +               code = http.StatusBadRequest
> +               fmt.Fprintf(buf, "HTTP/1.1 %03d %s\r\n", code,
> http.StatusText(code))
> +               buf.WriteString("\r\n")
> +               buf.Flush()
>                return
>        }
>        conn = hs.NewServerConn(buf, rwc, req)
> Index: src/pkg/websocket/websocket_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/websocket/websocket_**test.go
> +++ b/src/pkg/websocket/websocket_**test.go
> @@ -200,20 +200,19 @@
>        once.Do(startServer)
>
>        // If the client did not send a handshake that matches the protocol
> -       // specification, the server should abort the WebSocket connection.
> -       _, err := http.Get(fmt.Sprintf("http://%**s/echo", serverAddr))
> -       if err == nil {
> -               t.Error("Get: unexpected success")
> +       // specification, the server MUST return an HTTP respose with an
> +       // appropriate error code (such as 400 Bad Request)
> +       resp, err := http.Get(fmt.Sprintf("http://%**s/echo", serverAddr))
> +       if err != nil {
> +               t.Errorf("Get: error %#v", err)
>                return
>        }
> -       urlerr, ok := err.(*url.Error)
> -       if !ok {
> -               t.Errorf("Get: not url.Error %#v", err)
> +       if resp == nil {
> +               t.Error("Get: resp is null")
>                return
>        }
> -       if urlerr.Err != io.ErrUnexpectedEOF {
> -               t.Errorf("Get: error %#v", err)
> -               return
> +       if resp.StatusCode != http.StatusBadRequest {
> +               t.Errorf("Get: expected %q got %q", http.StatusBadRequest,
> resp.StatusCode)
>        }
>  }
>
>
>
>

[adg, 2011-11-03 03:13:45]

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

websocket: return an error HTTP response for bad websocket request.

websocket spec had changed server-side requiements to return
an HTTP response with an appropriate error code (such as 400 Bad
Request) when it finds client did not send a handshake that matches
websocket protocol, rather than just closing connection.
It needs to flush out response before closing connection.
Fixes issues 2396.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5318072

Committer: Andrew Gerrand <adg@golang.org>