Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
Mirrored from https://github.com/Slazee/caddy.git
Synced Nov 11, 02:15 UTC
reverseproxy: close backend body and hijacked socket on upgrade setup failures (#8269)
* reverseproxy: close backend body and hijacked socket on upgrade setup failures When an upstream response indicates an HTTP upgrade (101 Switching Protocols), handleUpgradeResponse takes ownership of res.Body and subsequently hijacks the client connection. However, if any setup check fails prior to starting handleUpgradeTunnel (such as mismatched upgrade headers, non-writable body assertion failure, hijack unsupported, or handshake buffer flush errors), the function returned early without closing res.Body or the hijacked client socket. Because calling Hijack() permanently disables net/http's automatic connection cleanup, and raw TCP sockets are not closed by the garbage collector, these early failure exits leaked file descriptors into CLOSE_WAIT/ESTABLISHED. Fix this by: 1. Adding a deferred cleanup guard that closes res.Body and any hijacked front connection unless the tunnel successfully takes ownership. 2. Wrapping connections with onceCloseConn to ensure idempotent closure while preserving optional CloseWrite (TCP half-close) support. 3. Adding TestUpgradeResponseCleanupOnFailure to verify cleanup. * reverseproxy: test post-hijack cleanup and remove once-close wrappers Cover handshake flush and buffered backend-write failures after successful hijacking, checking that both connections close. Keep setup-failure cleanup while using the original connections directly. Code and tests were AI-assisted using an OpenAI coding assistant in Pi.
Rob Martin committed
Oct 24, 2026 at 16:19 UTC
62bc6064335af61914e68ad2197e93e6d37a5efc
2 files changed
+165
-1
modules/caddyhttp/reverseproxy/streaming.go
+14
-1
@@ -60,6 +60,19 @@ func (rwc extendedConnectReadWriteCloser) Write(p []byte) (n int, err error) {
60
}
61
62
func (h *Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWriter, req *http.Request, res *http.Response, upstreamAddr string) {
63
+ // Until the tunnel takes ownership, every setup failure must close the
64
+ // backend body and any hijacked client connection. net/http no longer
65
+ // owns the client socket after a successful hijack.
66
+ var conn io.ReadWriteCloser
67
+ tunnelOwnsConnections := false
68
+ defer func() {
69
+ if !tunnelOwnsConnections {
70
+ res.Body.Close()
71
+ if conn != nil {
72
+ conn.Close()
73
+ }
74
+ }
75
+ }()
76
reqUpType := upgradeType(req.Header)
77
resUpType := upgradeType(res.Header)
78
@@ -100,7 +113,6 @@ func (h *Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWrit
113
streamTimeout := time.Duration(h.StreamTimeout)
114
115
var (
103
- conn io.ReadWriteCloser
116
brw *bufio.ReadWriter
117
detached bool
118
)
@@ -219,6 +231,7 @@ func (h *Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWrit
231
232
start := time.Now()
233
234
+ tunnelOwnsConnections = true
235
if !detached {
236
handleUpgradeTunnel(
237
streamLogger,
modules/caddyhttp/reverseproxy/streaming_test.go
+151
@@ -17,6 +17,7 @@ import (
17
"time"
18
19
"github.com/prometheus/client_golang/prometheus"
20
+ "go.uber.org/zap"
21
22
"github.com/caddyserver/caddy/v2"
23
"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
@@ -781,3 +782,153 @@ func TestUpgradeDetachmentRequiresWriterSupport(t *testing.T) {
782
})
783
}
784
}
785
+
786
+func TestUpgradeResponseCleanupOnFailure(t *testing.T) {
787
+ body := &closeTrackingBody{Buffer: bytes.NewBufferString("test body")}
788
+
789
+ res := &http.Response{
790
+ StatusCode: http.StatusSwitchingProtocols,
791
+ Header: make(http.Header),
792
+ Body: body,
793
+ }
794
+ res.Header.Set("Upgrade", "websocket")
795
+ res.Header.Set("Connection", "Upgrade")
796
+
797
+ // Request with mismatched upgrade type -> should fail check and close body
798
+ req, err := http.NewRequest("GET", "http://example.com", nil)
799
+ if err != nil {
800
+ t.Fatal(err)
801
+ }
802
+ req.Header.Set("Upgrade", "something-else")
803
+ req.Header.Set("Connection", "Upgrade")
804
+
805
+ recorder := httptest.NewRecorder()
806
+ h := &Handler{}
807
+ logger := zap.NewNop()
808
+
809
+ h.handleUpgradeResponse(logger, recorder, req, res, "127.0.0.1:8080")
810
+
811
+ if !body.closed {
812
+ t.Errorf("expected backend response body to be closed on upgrade protocol mismatch, but it was not")
813
+ }
814
+}
815
+
816
+func TestUpgradeResponseCleanupAfterHijack(t *testing.T) {
817
+ for _, tc := range []struct {
818
+ name string
819
+ flushFailure bool
820
+ }{
821
+ {name: "handshake flush failure", flushFailure: true},
822
+ {name: "buffered backend write failure"},
823
+ } {
824
+ t.Run(tc.name, func(t *testing.T) {
825
+ front, client := net.Pipe()
826
+ back, backend := net.Pipe()
827
+ for _, conn := range []net.Conn{front, client, back, backend} {
828
+ t.Cleanup(func() { conn.Close() })
829
+ }
830
+
831
+ writeFailure := &upgradeWriteErrorConn{writeErr: errors.New("upgrade setup write failed")}
832
+ reader := bufio.NewReader(strings.NewReader(""))
833
+ writer := bufio.NewWriter(front)
834
+ var body io.ReadWriteCloser = back
835
+ var wantWrite string
836
+ if tc.flushFailure {
837
+ writeFailure.Conn = front
838
+ writer = bufio.NewWriter(writeFailure)
839
+ wantWrite = "HTTP/1.1 101 Switching Protocols\r\n\r\n"
840
+ // Flush must have pending bytes; flushing an empty writer
841
+ // never reaches the failing connection's Write method.
842
+ if _, err := writer.WriteString(wantWrite); err != nil {
843
+ t.Fatal(err)
844
+ }
845
+ } else {
846
+ writeFailure.Conn = back
847
+ body = writeFailure
848
+ wantWrite = "buffered client data"
849
+ reader = bufio.NewReader(strings.NewReader(wantWrite))
850
+ // Buffered() must be nonzero before the handler checks it.
851
+ if _, err := reader.Peek(len(wantWrite)); err != nil {
852
+ t.Fatal(err)
853
+ }
854
+ }
855
+
856
+ rw := &upgradeFailureHijacker{
857
+ ResponseRecorder: httptest.NewRecorder(),
858
+ conn: front,
859
+ brw: bufio.NewReadWriter(reader, writer),
860
+ }
861
+ req := prepareTestRequest(httptest.NewRequest(http.MethodGet, "/upgrade", nil))
862
+ req.Header.Set("Connection", "Upgrade")
863
+ req.Header.Set("Upgrade", "test-stream")
864
+ res := &http.Response{StatusCode: http.StatusSwitchingProtocols, Header: req.Header.Clone(), Body: body}
865
+ logger := zap.NewNop()
866
+ h := &Handler{logger: logger, tunnelTracker: newTunnelTracker(logger, 0)}
867
+ for _, peer := range []net.Conn{client, backend} {
868
+ if err := peer.SetReadDeadline(time.Now().Add(time.Second)); err != nil {
869
+ t.Fatal(err)
870
+ }
871
+ }
872
+
873
+ h.handleUpgradeResponse(logger, rw, req, res, "cleanup-test")
874
+
875
+ if !rw.hijacked {
876
+ t.Fatal("setup failed before successful client hijack")
877
+ }
878
+ if writeFailure.writeCalls != 1 || string(writeFailure.written) != wantWrite {
879
+ t.Fatalf("failure injection: got %d writes of %q, want one write of %q",
880
+ writeFailure.writeCalls, writeFailure.written, wantWrite)
881
+ }
882
+ for _, peer := range []struct {
883
+ name string
884
+ conn net.Conn
885
+ }{
886
+ {name: "client", conn: client},
887
+ {name: "backend", conn: backend},
888
+ } {
889
+ var data [1]byte
890
+ if n, err := peer.conn.Read(data[:]); n != 0 || !errors.Is(err, io.EOF) {
891
+ t.Errorf("%s peer read = (%d, %v), want (0, EOF) after setup failure", peer.name, n, err)
892
+ }
893
+ }
894
+ if remaining := len(h.tunnelTracker.connections); remaining != 0 {
895
+ t.Errorf("setup failure registered %d tunnel connections", remaining)
896
+ }
897
+ })
898
+ }
899
+}
900
+
901
+type upgradeFailureHijacker struct {
902
+ *httptest.ResponseRecorder
903
+ conn net.Conn
904
+ brw *bufio.ReadWriter
905
+ hijacked bool
906
+}
907
+
908
+func (w *upgradeFailureHijacker) Hijack() (net.Conn, *bufio.ReadWriter, error) {
909
+ w.hijacked = true
910
+ return w.conn, w.brw, nil
911
+}
912
+
913
+type upgradeWriteErrorConn struct {
914
+ net.Conn
915
+ writeErr error
916
+ writeCalls int
917
+ written []byte
918
+}
919
+
920
+func (c *upgradeWriteErrorConn) Write(p []byte) (int, error) {
921
+ c.writeCalls++
922
+ c.written = append(c.written, p...)
923
+ return 0, c.writeErr
924
+}
925
+
926
+type closeTrackingBody struct {
927
+ *bytes.Buffer
928
+ closed bool
929
+}
930
+
931
+func (c *closeTrackingBody) Close() error {
932
+ c.closed = true
933
+ return nil
934
+}