Repository navigation
Support validated website WebSockets and owned TCP shutdown - #42
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Header-filtering CPU amplification remains unresolved, and protocol handoff and concurrent shutdown require final human validation.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds validated fixed-origin website WebSockets and preserves server ownership of upgraded TCP connections.
Changes:
- Validates H1.1 WebSocket handshakes while retaining header filtering.
- Closes upgraded sockets during shutdown without closing shared transports.
- Adds regression tests and documents compatibility boundaries.
| File | Description |
|---|---|
| README.md | Introduces source-build WebSocket support. |
| internal/tunnel/websocket_lifecycle_test.go | Tests upgraded connection shutdown and capacity release. |
| internal/tunnel/websocket_combined_test.go | Tests combined-server WebSockets across TLS versions. |
| internal/tunnel/web_tcp_owner.go | Tracks and closes owned TCP sockets. |
| internal/tunnel/web_tcp_owner_test.go | Tests ownership and late acceptance. |
| internal/tunnel/web_limits.go | Adds ownership registration and once-only closure. |
| internal/tunnel/web_h2.go | Integrates socket ownership into server shutdown. |
| internal/cover/websocket.go | Implements handshake validation and body ownership. |
| internal/cover/websocket_test.go | Tests validation and cleanup boundaries. |
| internal/cover/websocket_integration_test.go | Tests wire exchanges and rejection paths. |
| internal/cover/handler.go | Integrates validated upgrades and header filtering. |
| docs/WEB_COVER.md | Defines WebSocket support and limitations. |
| docs/PROTOCOL.md | Clarifies separation from tunnel protocols. |
| docs/DEPLOYMENT.md | Documents deployment compatibility. |
| docs/ARCHITECTURE.md | Explains upgrade ownership and response boundaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Final source / local verification
Tested/pushed head:
870b57eea8bec01a63dd4b4cbb68ffb7e4261fe2; treea9020b16c8d2c5ed6d1edf5323ef717963168212.Full Go/docs/module/README manifest: 209 paths, SHA-256
652195884757ed340a14b84b8cc6c72707417ebd58f870c7fc916bb34e9a008a.make checkandmake race: passed on cached Go1.25.13 Darwin/arm64, offline.Actions / review
Prior head2107b93 passed CI/CodeQL/netem, then automated review found one actionable nomination-scan finding; repaired in c86066b. Its CI attempt1 passed10/11 jobs but exposed the delayed-Accept fixture synchronization issue in Go1.27. Full original failures/receipts are retained; no blind CI retry was requested. These earlier runs are not final-head gates.
Final-head CI37059387372 (all11 jobs), CodeQL37059387261 and netem37059387314 all completed successfully at exact head870b57e, pull_request/attempt1. Actual Docker integration, dual-platform OCI export, netem completion log/artifact and synthetic checkout tree/parents were checked. Final official review snapshot found zero unresolved/new actionable inline threads; the sole original COMMENTED review remains historical, not a new approval. Its source-backed nomination finding was repaired and the thread resolved/outdated.
Merge and main verification
Squash-merged to main commit
0d42cc2f825aae217c802650a65443f3ce2bda6f. Official/local treea9020b16c8d2c5ed6d1edf5323ef717963168212exactly equals the tested final head tree; parent is the unchanged base. Local main is fast-forwarded and clean; before/after209-source manifest matches. Postmerge all-new race ×3 passed again:20 tops /60 top executions,180 terminal leaves /540 leaf executions,582 RUN/PASS.Separate exact-main push/attempt1 CI37060011602 (all11 jobs), CodeQL37060011645 and netem37060011711 completed successfully. Main OCI checkout is the actual merge commit; real Docker integration and amd64/arm64 manifest/tarball export completed. No release/tag or deployment is included.
Limits
This is fixed-website compatibility and lifecycle correctness, not browser identity or traffic-classification parity. No browser/PCAP campaign, remote SSH, release/tag or production deployment was performed. Close aborts sockets rather than negotiating WebSocket close frames. Origin/subprotocol/extensions/access policy remain the website/client's responsibility; raw hijacked101 is not promised the bound Alt-Svc override. The native Go ordinary-response
Connection: closeinformation-loss boundary is documented and unchanged. Existing impaired netem evidence covers native direct/quic/tls only, not web H2/H3.