Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for websocket errors

[Daniel P. Berrange]

On Wed, Sep 06, 2017 at 03:06:03PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
> 
> On 09/06/2017 07:40 AM, Daniel P. Berrange wrote:
> > When any error occurs while processing the websockets handshake,
> > QEMU just terminates the connection abruptly. This is in violation
> > of the HTTP specs and does not help the client understand what they
> > did wrong. This is particularly bad when the client gives the wrong
> > path, as a "404 Not Found" would be very helpful.
> > 
> > Refactor the handshake code so that it always sends a response to
> > the client unless there was an I/O error.
> > 
> > Fixes bug: #1715186
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >   io/channel-websock.c | 179 
> > ++++++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 134 insertions(+), 45 deletions(-)
> > 
> > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > index 5a3badbec2..b9cc5a1371 100644
> > --- a/io/channel-websock.c
> > +++ b/io/channel-websock.c
> > @@ -44,13 +44,39 @@
> >   #define QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE "Upgrade"
> >   #define QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET "websocket"
> > -#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE  \
> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
> > +    "Server: QEMU VNC\r\n"                       \
> > +    "Date: %s\r\n"
> 
> and
>         "Sec-WebSocket-Version: " \
>           QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "\r\n" \

This header is only supposed to be set in error responses,
not in the 101 response.

> or what about:
> 
> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(conn)   \
>           "Server: QEMU VNC\r\n"                         \
>           "Date: %s\r\n"                                 \
>           "Connection: " conn "\r\n"                     \
>           "Sec-WebSocket-Version: "                      \
>             QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "\r\n"

I'm not a fan of parameterizing this - I think it is clearer
to see the full connection header inline below.

> > +
> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK    \
> >       "HTTP/1.1 101 Switching Protocols\r\n"      \
> > +    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON    \
> >       "Upgrade: websocket\r\n"                    \
> >       "Connection: Upgrade\r\n"                   \
> >       "Sec-WebSocket-Accept: %s\r\n"              \
> >       "Sec-WebSocket-Protocol: binary\r\n"        \
> >       "\r\n"
> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND \
> > +    "HTTP/1.1 404 Not Found\r\n"                    \
> > +    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON        \
> > +    "Connection: close\r\n"                         \
> > +    "\r\n"
> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
> > +    "HTTP/1.1 400 Bad Request\r\n"                    \
> > +    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON          \
> > +    "Connection: close\r\n"                           \
> > +    "Sec-WebSocket-Version: 13\r\n"                   \
> 
> drop
> 
> > +    "\r\n"
> 
> or with previous macro:
> 
> #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
>     "HTTP/1.1 400 Bad Request\r\n"                    \
>     QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON("close") \
>     "\r\n"
> 
> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR \
> > +    "HTTP/1.1 500 Internal Server Error\r\n"         \
> > +    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
> > +    "Connection: close\r\n"                          \
> > +    "\r\n"
> > +#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE  \
> > +    "HTTP/1.1 403 Request Entity Too Large\r\n"      \
> > +    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
> > +    "Connection: close\r\n"                          \
> > +    "\r\n"
> >   #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
> >   #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
> >   #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|