Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to overwrite response headers when gorilla/websocket upgrade fails #3529

Open
maxeth opened this issue Feb 11, 2025 · 0 comments
Open

Comments

@maxeth
Copy link

maxeth commented Feb 11, 2025

In transport/websocket.go, when Upgrader.Upgrade() returns an error, we try to send a http.StatusBadRequest

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 itself already writes HTTP errors if Upgrader.Upgrade() returns an error, quoting the source of v1.5.0:

// 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants