fix(websocket): don't panic on a plain HTTP request to a WebSocket route

[om7057]

Summary

Fixes #3862.

App.WebSocket registered its handler with an unsafe type assertion on WSConnectionKey. A plain HTTP GET to a WebSocket route (no Connection: Upgrade / Upgrade: websocket headers) never gets that key set on the request context by the upgrade middleware, so the assertion ctx.Request.Context().Value(websocket.WSConnectionKey).(string) panicked with interface conversion: interface {} is nil, not string.

GetWebsocketConnection also returns a nil *Connection for an unknown connection ID, so even a safe type assertion would still hit a nil pointer dereference at the following conn.Conn == nil check.

Both call sites now check ok/nil before dereferencing and return the existing websocket.ErrorConnection instead of panicking, matching the fix direction suggested in the issue.

Test plan

• go build ./...
• go vet ./...
• go test ./pkg/gofr/...
• New test: Test_WebSocket_PlainHTTPRequestDoesNotPanic sends a plain GET to a route registered via app.WebSocket and asserts the response carries websocket.ErrorConnection's message rather than the generic panic-recovery message (both map to HTTP 500, so the response body is the only way to distinguish "handled cleanly" from "panicked and got recovered"). Verified this test fails with the exact panic from the issue when the fix is reverted, and passes with it applied.

[om7057]

@Umang01-hash @coolwednesday a small reminder on this!

[aryanmehrotra]

Sorry for the wait, and for missing your ping — thirteen days on a remotely-triggerable panic is too
long.

Bug is real, fix is right. Ran both builds in containers:

development   500  Internal Server Error   + panic: interface conversion: interface {} is nil, not string
this branch   500  couldn't establish connection to web socket   + no panics

Both guards are needed — GetWebsocketConnection does return a nil *Connection for an unknown ID.

One change: the status code on the first return site.

The issue asked for 4xx and this keeps the pre-existing 500. Not a regression, but RFC 6455 §4.2.1
says a failed handshake gets "an appropriate error code (such as 400 Bad Request)" — and we already
do that twelve lines away, in middleware/web_socket.go:21, when an upgrade is attempted and fails.
A request that never attempted one should not be treated as more serious than one that tried.

Only the first site though — the two are different failures:

if !ok {
    return nil, websocket.ErrorConnection   // plain GET, no upgrade -> client error, 400
}
if conn == nil || conn.Conn == nil {
    return nil, websocket.ErrorConnection   // upgrade ran, connection missing -> server fault, keep 500
}

So please return a distinct error at the first site rather than putting StatusCode() on the shared
ErrorConnection — folding both into a 4xx would hide a genuine bug class. ErrorConnection has no
other references in the repo and only your new test pins a status, so it is a one-line assertion
change.

(426 looks tempting but is wrong here — RFC 6455 reserves it for version negotiation in §4.2.2.)

Also: this branch predates #4051 so it does not build standalone against current development.
Merges cleanly — I tested the merged result — just worth a rebase.

[om7057]

Sorry for the wait, and for missing your ping — thirteen days on a remotely-triggerable panic is too long.

Bug is real, fix is right. Ran both builds in containers:

development   500  Internal Server Error   + panic: interface conversion: interface {} is nil, not string
this branch   500  couldn't establish connection to web socket   + no panics

Both guards are needed — GetWebsocketConnection does return a nil *Connection for an unknown ID.

One change: the status code on the first return site.

The issue asked for 4xx and this keeps the pre-existing 500. Not a regression, but RFC 6455 §4.2.1 says a failed handshake gets "an appropriate error code (such as 400 Bad Request)" — and we already do that twelve lines away, in middleware/web_socket.go:21, when an upgrade is attempted and fails. A request that never attempted one should not be treated as more serious than one that tried.

Only the first site though — the two are different failures:

if !ok {
    return nil, websocket.ErrorConnection   // plain GET, no upgrade -> client error, 400
}
if conn == nil || conn.Conn == nil {
    return nil, websocket.ErrorConnection   // upgrade ran, connection missing -> server fault, keep 500
}

So please return a distinct error at the first site rather than putting StatusCode() on the shared ErrorConnection — folding both into a 4xx would hide a genuine bug class. ErrorConnection has no other references in the repo and only your new test pins a status, so it is a one-line assertion change.

(426 looks tempting but is wrong here — RFC 6455 reserves it for version negotiation in §4.2.2.)

Also: this branch predates #4051 so it does not build standalone against current development. Merges cleanly — I tested the merged result — just worth a rebase.

Sure, no worries @aryanmehrotra.
Split the two error cases as suggested: added websocket.ErrorNotWebSocketUpgrade{} with StatusCode() int returning 400 for the "never attempted an upgrade" path, kept ErrorConnection as-is (falls through to 500) for the "upgrade ran, connection missing" path. Updated the test to assert the 400 and the new message.

Also rebased onto current development, builds standalone now.