From 1d8e29815ebfeee8d1604253ef14f82fa404cb7f Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 28 Apr 2026 19:09:27 +0200 Subject: [PATCH] fix(coderd/x/chatd/chatdebug): restore request body after capture (#24784) > Mux working on behalf of Mike. Debug recording could consume request bodies when a provider SDK returned the active body from `GetBody`, which left the upstream request with an empty body after capture. Reset the request body after debug capture and add coverage for shared `GetBody` readers so debug logging does not alter the bytes sent upstream. --- coderd/x/chatd/chatdebug/transport.go | 42 ++++- coderd/x/chatd/chatdebug/transport_test.go | 169 +++++++++++++++++++++ 2 files changed, 204 insertions(+), 7 deletions(-) diff --git a/coderd/x/chatd/chatdebug/transport.go b/coderd/x/chatd/chatdebug/transport.go index 102de968af..07cdb92568 100644 --- a/coderd/x/chatd/chatdebug/transport.go +++ b/coderd/x/chatd/chatdebug/transport.go @@ -12,6 +12,8 @@ import ( "strings" "sync" "time" + + "golang.org/x/xerrors" ) // attemptStatusCompleted is the status recorded when a response body @@ -160,14 +162,22 @@ func captureRequestBody(req *http.Request) ([]byte, error) { if req.GetBody != nil { clone, err := req.GetBody() if err == nil { - defer clone.Close() - limited, err := io.ReadAll(io.LimitReader(clone, maxRecordedRequestBodyBytes+1)) - if err == nil { - if len(limited) > maxRecordedRequestBodyBytes { - return []byte("[TRUNCATED]"), nil - } - return RedactJSONSecrets(limited), nil + limited, readErr := io.ReadAll(io.LimitReader(clone, maxRecordedRequestBodyBytes+1)) + _ = clone.Close() + // Some SDKs return the active body from GetBody instead of an + // independent reader. Restore the request body from GetBody so + // the upstream transport still receives the original bytes. + resetErr := resetRequestBody(req) + if resetErr != nil { + return nil, xerrors.Errorf("chatdebug: reset request body: %w", resetErr) } + if readErr != nil { + return nil, nil + } + if len(limited) > maxRecordedRequestBodyBytes { + return []byte("[TRUNCATED]"), nil + } + return RedactJSONSecrets(limited), nil } } @@ -178,6 +188,24 @@ func captureRequestBody(req *http.Request) ([]byte, error) { return nil, nil } +// resetRequestBody replaces req.Body with a fresh reader from req.GetBody. +// It closes the previous request body before installing the replacement. +// Callers must ensure req.GetBody is non-nil. +func resetRequestBody(req *http.Request) error { + body, err := req.GetBody() + if err != nil { + return err + } + if req.Body != nil { + if err := req.Body.Close(); err != nil { + _ = body.Close() + return err + } + } + req.Body = body + return nil +} + type recordingBody struct { inner io.ReadCloser contentLength int64 diff --git a/coderd/x/chatd/chatdebug/transport_test.go b/coderd/x/chatd/chatdebug/transport_test.go index 0b5b9448e8..369eacd7a2 100644 --- a/coderd/x/chatd/chatdebug/transport_test.go +++ b/coderd/x/chatd/chatdebug/transport_test.go @@ -1,6 +1,7 @@ package chatdebug //nolint:testpackage // Uses unexported recorder helpers. import ( + "bytes" "context" "encoding/json" "errors" @@ -56,6 +57,17 @@ func (*scriptedReadCloser) Close() error { return nil } +type closeTrackingReadCloser struct { + *bytes.Reader + closed bool + closeErr error +} + +func (c *closeTrackingReadCloser) Close() error { + c.closed = true + return c.closeErr +} + func TestRecordingTransport_NoSink(t *testing.T) { t.Parallel() @@ -139,6 +151,163 @@ func TestRecordingTransport_CaptureRequest(t *testing.T) { require.Equal(t, "Bearer top-secret", received.authorization) } +func TestRecordingTransport_CaptureRequestRestoresSharedGetBody(t *testing.T) { + t.Parallel() + + const requestBody = `{"message":"hello","api_key":"super-secret"}` + + gotRequest := make(chan []byte, 1) + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + body, err := io.ReadAll(req.Body) + require.NoError(t, err) + gotRequest <- body + _, _ = rw.Write([]byte(`{"ok":true}`)) + })) + defer server.Close() + + ctx, sink := newTestSinkContext(t) + client := &http.Client{ + Transport: &RecordingTransport{Base: server.Client().Transport}, + } + + reader := bytes.NewReader([]byte(requestBody)) + originalBody := &closeTrackingReadCloser{Reader: reader} + req, err := http.NewRequestWithContext( + ctx, + http.MethodPost, + server.URL, + originalBody, + ) + require.NoError(t, err) + req.ContentLength = int64(len(requestBody)) + req.GetBody = func() (io.ReadCloser, error) { + _, err := reader.Seek(0, io.SeekStart) + if err != nil { + return nil, err + } + return io.NopCloser(reader), nil + } + + resp, err := client.Do(req) + require.NoError(t, err) + _, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + require.JSONEq(t, requestBody, string(<-gotRequest)) + require.True(t, originalBody.closed) + attempts := sink.snapshot() + require.Len(t, attempts, 1) + require.Equal(t, attemptStatusCompleted, attempts[0].Status) + require.JSONEq(t, `{"message":"hello","api_key":"[REDACTED]"}`, string(attempts[0].RequestBody)) +} + +func TestRecordingTransport_CaptureRequestResetFailureFailsRequest(t *testing.T) { + t.Parallel() + + const requestBody = `{"message":"hello"}` + + gotRequest := make(chan struct{}, 1) + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + gotRequest <- struct{}{} + _, _ = rw.Write([]byte(`{"ok":true}`)) + })) + defer server.Close() + + ctx, sink := newTestSinkContext(t) + client := &http.Client{ + Transport: &RecordingTransport{Base: server.Client().Transport}, + } + + reader := bytes.NewReader([]byte(requestBody)) + req, err := http.NewRequestWithContext( + ctx, + http.MethodPost, + server.URL, + io.NopCloser(reader), + ) + require.NoError(t, err) + req.ContentLength = int64(len(requestBody)) + getBodyCalls := 0 + req.GetBody = func() (io.ReadCloser, error) { + getBodyCalls++ + if getBodyCalls == 2 { + return nil, xerrors.New("reset failed") + } + _, err := reader.Seek(0, io.SeekStart) + if err != nil { + return nil, err + } + return io.NopCloser(reader), nil + } + + resp, err := client.Do(req) + if resp != nil { + require.NoError(t, resp.Body.Close()) + } + require.ErrorContains(t, err, "chatdebug: reset request body: reset failed") + require.Nil(t, resp) + require.Empty(t, sink.snapshot()) + select { + case <-gotRequest: + t.Fatal("request should not be sent with a drained body") + default: + } +} + +func TestRecordingTransport_CaptureRequestBodyCloseFailureFailsRequest(t *testing.T) { + t.Parallel() + + const requestBody = `{"message":"hello"}` + + gotRequest := make(chan struct{}, 1) + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + gotRequest <- struct{}{} + _, _ = rw.Write([]byte(`{"ok":true}`)) + })) + defer server.Close() + + ctx, sink := newTestSinkContext(t) + client := &http.Client{ + Transport: &RecordingTransport{Base: server.Client().Transport}, + } + + reader := bytes.NewReader([]byte(requestBody)) + originalBody := &closeTrackingReadCloser{ + Reader: reader, + closeErr: xerrors.New("close failed"), + } + req, err := http.NewRequestWithContext( + ctx, + http.MethodPost, + server.URL, + originalBody, + ) + require.NoError(t, err) + req.ContentLength = int64(len(requestBody)) + req.GetBody = func() (io.ReadCloser, error) { + _, err := reader.Seek(0, io.SeekStart) + if err != nil { + return nil, err + } + return io.NopCloser(reader), nil + } + + resp, err := client.Do(req) + if resp != nil { + require.NoError(t, resp.Body.Close()) + } + require.ErrorContains(t, err, "chatdebug: reset request body: close failed") + require.Nil(t, resp) + require.True(t, originalBody.closed) + require.Empty(t, sink.snapshot()) + select { + case <-gotRequest: + t.Fatal("request should not be sent when the captured body cannot be closed") + default: + } +} + func TestRecordingTransport_RedactsSensitiveQueryParameters(t *testing.T) { t.Parallel()