fix: enforce WS message size limit + add connection keepalive

[HimanshuPathak2725]

Fix for finding #3 from the private security report sent per SECURITY.md submitted as a separate PR per @VishalRaut2106's
direction (findings #1+#2 already merged in #20).

1. Neither readPump nor writePump ever called conn.SetReadLimit(). Gorilla's default is unlimited message size, so any connected client could send a single oversized WS frame and force the server to allocate memory proportional to that frame via ReadJSON() a memory-exhaustion DoS vector. Now capped at 32KB per message.
2. Added the standard gorilla ping/pong keepalive pattern (SetReadDeadline + SetPongHandler in readPump, a periodic-ping ticker in writePump). Without it, a silently dead connection (client sleeps, NAT drop, no clean TCP close) was never detected readPump blocked forever, leaking the goroutine and Client state.Different root cause from [BUG] Goroutine leak: guest readPump blocks forever on room.unregister when host disconnects with guests still connected #7/fix: prevent goroutine leak in readPump on room destroy #8 (that was a shutdow ordering bug; this is network-level dead-connection detection), fixed together here since both touch readPump/writePump.

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Docs / CI

Checklist

• go build ./... passes locally
• go vet ./... passes locally
• I've tested the change manually
• Added TestReadPumpEnforcesMessageSizeLimit
• README update — not needed, internal server hardening

[github-actions]

Feedback

Bugs and Security Issues

1. Message Size Limit Enforcement:

   • The SetReadLimit call in readPump is correctly implemented to enforce a 32KB message size limit. This addresses the memory-exhaustion DoS vector mentioned in the PR description.
   • The test TestReadPumpEnforcesMessageSizeLimit verifies this behavior by sending a message larger than the limit and checking if the connection is closed. This is a good addition.

2. Connection Keepalive:

   • The keepalive mechanism using ping/pong messages is correctly implemented in both readPump and writePump.
   • In readPump, SetReadDeadline and SetPongHandler are used to handle pong messages and reset the read deadline.
   • In writePump, a ticker sends periodic ping messages, and the write deadline is set appropriately.

Performance Bottlenecks

1. Ticker Management:

   • The ticker in writePump is properly stopped in the defer function, which is good practice to avoid goroutine leaks.

2. Deadline Management:

   • Both read and write deadlines are set appropriately, which helps in detecting dead connections and preventing indefinite blocking.

Idiomatic Go Conventions

1. Constants:

   • The constants maxMessageSize, writeWait, pongWait, and pingPeriod are defined at the package level, which is a good practice for configuration values.

2. Error Handling:

   • Error handling in both readPump and writePump is straightforward and follows Go conventions. The connection is closed on errors, and the goroutines terminate.

3. Defer Statements:

   • Defer statements are used appropriately to ensure resources are cleaned up, such as closing the connection and stopping the ticker.

Robust Error Handling

1. Connection Closure:

   • The connection is closed in the defer statements of both readPump and writePump, ensuring that resources are released even if an error occurs.

2. Ping/Pong Handling:

   • The pong handler in readPump correctly resets the read deadline, which is crucial for keepalive functionality.

3. Write Message Handling:

   • The writePump correctly handles the closure of the send channel by sending a close message, which is a good practice for WebSocket connections.

Suggestions for Improvement

1. Logging:

   • Consider adding logging for errors and connection closures to help with debugging and monitoring. This is especially important for server-side code.

2. Configuration:

   • The constants could be moved to a configuration file or environment variables to make them more flexible and easier to manage.

3. Testing:

   • The test TestReadPumpEnforcesMessageSizeLimit is comprehensive and covers the main functionality. Consider adding more tests to cover edge cases, such as very small messages and messages exactly at the limit.

4. Documentation:

   • Add comments to explain the purpose of the constants and the keepalive mechanism, especially for new developers who might not be familiar with the Gorilla WebSocket library.

Summary

The changes address the security and performance issues mentioned in the PR description effectively. The code is well-structured, follows idiomatic Go conventions, and includes robust error handling. The suggestions for improvement focus on adding logging, making the configuration more flexible, and enhancing test coverage. Overall, the changes are a significant improvement to the codebase.

[HimanshuPathak2725]

CI's Build & Lint failure isn't from this diff — I confirmed by running golangci-lint run ./... directly against upstream/master (no changes), and it already reports 8 pre-existing issues (7 errcheck on unchecked .Close() calls across cmd/cli/main.go/cmd/server/main.go/tests, 1 staticcheck suggestion on handleWebSocket's role switch), unrelated to this PR.

I did fix the 6 issues genuinely introduced by this diff (unchecked SetReadDeadline/SetWriteDeadline/WriteMessage on the new ping/pong code), in the latest commit.

Left the pre-existing .Close() pattern untouched since its repo-wide and predates this change fixing it here would be scope
creep and touch files unrelated to WS read limits. Happy to open a separate cleanup PR for that if useful, or if you'd rather scope golangci-lint to only new-from-rev issues in CI so future PRs aren't blocked by pre-existing debt, I can help with that config too.