99designs / 99designs/gqlgen

Attempt to overwrite response headers when gorilla/websocket upgrade fails

Abierto
#3,529 0 comentarios 0 reacciones 0 asignados Ver en GitHub
websocket
Lenguaje dominante
Go
Estrellas
10.8k
Forks
1.3k
Merge medio
2 d 36 min
PR fusionados (30 d)
26

Descripción

In transport/websocket.go, when `Upgrader.Upgrade()` returns an error, [we try to send a `http.StatusBadRequest`](https://github.com/99designs/gqlgen/blob/master/graphql/handler/transport/websocket.go#L96C2-L101C3)

```go
func (t Websocket) Do(w http.ResponseWriter, r *http.Request, exec graphql.GraphExecutor) {
t.injectGraphQLWSSubprotocols()
ws, err := t.Upgrader.Upgrade(w, r, http.Header{}) // <-- already writes errors
if err != nil {
// error to request already written
log.Printf("unable to upgrade %T to websocket %s: ", w, err.Error())
SendErrorf(w, http.StatusBadRequest, "unable to upgrade") // <-- overwriting headers. Seems unnecessary?
return
}
```

but the [gorilla Upgrade func](https://github.com/gorilla/websocket/blob/main/server.go#L124) itself already writes HTTP errors if `Upgrader.Upgrade()` returns an error, quoting the source of v1.5.0:

``` go
// If the upgrade fails, then Upgrade replies to the client with an HTTP error
// response.
func (u *Upgrader) Upgrade(w http.ResponseWriter, r *http.Request, responseHeader http.Header) (*Conn, error)
```
so we end up attempting to overwrite response headers every time an upgrade fails (e.g. when `CheckOrigin` returns `false`, a HTTP 403 Forbidden is written in Upgrader.Upgrade(), but we attempt to overwrite it with 400 Bad Request).

It's nothing dramatic, but my console is full of:

```
[GIN-debug] [WARNING] Headers were already written. Wanted to override status code 403 with 400
```

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.