fix(security): cap error response body reads in remote client
Use io.LimitReader(resp.Body, 1024) for all error-path reads in the remote client (parseResponse, requestWithRetry, streaming setup). Previously, unbounded io.ReadAll on error responses could cause OOM if the server returned very large error bodies.
- commit
- 7a412ec4de340ad2ab2ff2f3b05db20972dabf1a
- author
- brice · · 2026-03-14 22:18 UTC
- parent
- ff97ba9
- signature
- unsigned
2 files · +62 -3
M internal/adapters/in/cli/remote/client.go +5 -3
| @@ -33,6 +33,8 @@ var ( | |||
| 33 | 33 | retryBaseDelay = 250 * time.Millisecond | |
| 34 | 34 | ) | |
| 35 | 35 | ||
| 36 | + | const maxErrorBodySize int64 = 1024 // cap error response reads at 1 KB | |
| 37 | + | ||
| 36 | 38 | // ClientOption configures the Client. | |
| 37 | 39 | type ClientOption func(*Client) | |
| 38 | 40 | ||
| @@ -188,7 +190,7 @@ func parseResponse(resp *http.Response, target any) error { | |||
| 188 | 190 | defer resp.Body.Close() | |
| 189 | 191 | ||
| 190 | 192 | if resp.StatusCode >= 400 { | |
| 191 | - | body, _ := io.ReadAll(resp.Body) | |
| 193 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) | |
| 192 | 194 | return parseErrorResponse(resp, body) | |
| 193 | 195 | } | |
| 194 | 196 | ||
| @@ -245,7 +247,7 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, body | |||
| 245 | 247 | return resp, nil | |
| 246 | 248 | } | |
| 247 | 249 | ||
| 248 | - | respBody, _ := io.ReadAll(resp.Body) | |
| 250 | + | respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) | |
| 249 | 251 | _ = resp.Body.Close() | |
| 250 | 252 | lastErr = parseErrorResponse(resp, respBody) | |
| 251 | 253 | if attempt == retryMaxAttempts { | |
| @@ -972,7 +974,7 @@ func (c *Client) streamLogs(ctx context.Context, path string) (<-chan string, er | |||
| 972 | 974 | c.observeTokenRotation(resp) | |
| 973 | 975 | ||
| 974 | 976 | if resp.StatusCode >= 400 { | |
| 975 | - | body, _ := io.ReadAll(resp.Body) | |
| 977 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) | |
| 976 | 978 | resp.Body.Close() | |
| 977 | 979 | return nil, fmt.Errorf("%s: %s", resp.Status, string(body)) | |
| 978 | 980 | } | |
| @@ -33,6 +33,8 @@ var ( | |||||
| 33 | retryBaseDelay = 250 * time.Millisecond | 33 | retryBaseDelay = 250 * time.Millisecond | ||
| 34 | ) | 34 | ) | ||
| 35 | 35 | ||||
| 36 | + | const maxErrorBodySize int64 = 1024 // cap error response reads at 1 KB | |||
| 37 | + | ||||
| 36 | // ClientOption configures the Client. | 38 | // ClientOption configures the Client. | ||
| 37 | type ClientOption func(*Client) | 39 | type ClientOption func(*Client) | ||
| 38 | 40 | ||||
| @@ -188,7 +190,7 @@ func parseResponse(resp *http.Response, target any) error { | |||||
| 188 | defer resp.Body.Close() | 190 | defer resp.Body.Close() | ||
| 189 | 191 | ||||
| 190 | if resp.StatusCode >= 400 { | 192 | if resp.StatusCode >= 400 { | ||
| 191 | - | body, _ := io.ReadAll(resp.Body) | 193 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) |
| 192 | return parseErrorResponse(resp, body) | 194 | return parseErrorResponse(resp, body) | ||
| 193 | } | 195 | } | ||
| 194 | 196 | ||||
| @@ -245,7 +247,7 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, body | |||||
| 245 | return resp, nil | 247 | return resp, nil | ||
| 246 | } | 248 | } | ||
| 247 | 249 | ||||
| 248 | - | respBody, _ := io.ReadAll(resp.Body) | 250 | + | respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) |
| 249 | _ = resp.Body.Close() | 251 | _ = resp.Body.Close() | ||
| 250 | lastErr = parseErrorResponse(resp, respBody) | 252 | lastErr = parseErrorResponse(resp, respBody) | ||
| 251 | if attempt == retryMaxAttempts { | 253 | if attempt == retryMaxAttempts { | ||
| @@ -972,7 +974,7 @@ func (c *Client) streamLogs(ctx context.Context, path string) (<-chan string, er | |||||
| 972 | c.observeTokenRotation(resp) | 974 | c.observeTokenRotation(resp) | ||
| 973 | 975 | ||||
| 974 | if resp.StatusCode >= 400 { | 976 | if resp.StatusCode >= 400 { | ||
| 975 | - | body, _ := io.ReadAll(resp.Body) | 977 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) |
| 976 | resp.Body.Close() | 978 | resp.Body.Close() | ||
| 977 | return nil, fmt.Errorf("%s: %s", resp.Status, string(body)) | 979 | return nil, fmt.Errorf("%s: %s", resp.Status, string(body)) | ||
| 978 | } | 980 | } | ||
M internal/adapters/in/cli/remote/client_test.go +57 -0
| @@ -3,8 +3,10 @@ package remote | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "encoding/json" | |
| 6 | + | "io" | |
| 6 | 7 | "net/http" | |
| 7 | 8 | "net/http/httptest" | |
| 9 | + | "strings" | |
| 8 | 10 | "sync/atomic" | |
| 9 | 11 | "testing" | |
| 10 | 12 | "time" | |
| @@ -193,3 +195,58 @@ func TestClientFindAttachmentTargetsByImage_WithSlashContainingImage(t *testing. | |||
| 193 | 195 | require.NoError(t, err) | |
| 194 | 196 | assert.Equal(t, []string{"workers"}, targets) | |
| 195 | 197 | } | |
| 198 | + | ||
| 199 | + | func TestParseResponse_CapsErrorBodySize(t *testing.T) { | |
| 200 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |
| 201 | + | resp := &http.Response{ | |
| 202 | + | StatusCode: 500, | |
| 203 | + | Status: "500 Internal Server Error", | |
| 204 | + | Body: io.NopCloser(strings.NewReader(largeBody)), | |
| 205 | + | Header: make(http.Header), | |
| 206 | + | } | |
| 207 | + | ||
| 208 | + | err := parseResponse(resp, nil) | |
| 209 | + | ||
| 210 | + | require.Error(t, err) | |
| 211 | + | assert.Less(t, len(err.Error()), 2*1024, "error body should be capped to ~1KB") | |
| 212 | + | } | |
| 213 | + | ||
| 214 | + | func TestStreamLogs_CapsErrorBodySize(t *testing.T) { | |
| 215 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |
| 216 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 217 | + | w.WriteHeader(http.StatusInternalServerError) | |
| 218 | + | _, _ = w.Write([]byte(largeBody)) | |
| 219 | + | })) | |
| 220 | + | defer srv.Close() | |
| 221 | + | ||
| 222 | + | c := &Client{ | |
| 223 | + | baseURL: srv.URL, | |
| 224 | + | httpClient: srv.Client(), | |
| 225 | + | } | |
| 226 | + | ||
| 227 | + | _, err := c.StreamProcessLogs(context.Background(), 100) | |
| 228 | + | ||
| 229 | + | require.Error(t, err) | |
| 230 | + | assert.Less(t, len(err.Error()), 2*1024, "streaming error body should be capped to ~1KB") | |
| 231 | + | } | |
| 232 | + | ||
| 233 | + | func TestRequestWithRetry_CapsErrorBodySize(t *testing.T) { | |
| 234 | + | withFastRetry(t) | |
| 235 | + | ||
| 236 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |
| 237 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 238 | + | w.WriteHeader(http.StatusBadGateway) | |
| 239 | + | _, _ = w.Write([]byte(largeBody)) | |
| 240 | + | })) | |
| 241 | + | defer srv.Close() | |
| 242 | + | ||
| 243 | + | c := &Client{ | |
| 244 | + | baseURL: srv.URL, | |
| 245 | + | httpClient: srv.Client(), | |
| 246 | + | } | |
| 247 | + | ||
| 248 | + | _, err := c.requestWithRetry(context.Background(), http.MethodPost, "/test", nil) | |
| 249 | + | ||
| 250 | + | require.Error(t, err) | |
| 251 | + | assert.Less(t, len(err.Error()), 2*1024, "retry error body should be capped to ~1KB") | |
| 252 | + | } | |
| @@ -3,8 +3,10 @@ package remote | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "encoding/json" | 5 | "encoding/json" | ||
| 6 | + | "io" | |||
| 6 | "net/http" | 7 | "net/http" | ||
| 7 | "net/http/httptest" | 8 | "net/http/httptest" | ||
| 9 | + | "strings" | |||
| 8 | "sync/atomic" | 10 | "sync/atomic" | ||
| 9 | "testing" | 11 | "testing" | ||
| 10 | "time" | 12 | "time" | ||
| @@ -193,3 +195,58 @@ func TestClientFindAttachmentTargetsByImage_WithSlashContainingImage(t *testing. | |||||
| 193 | require.NoError(t, err) | 195 | require.NoError(t, err) | ||
| 194 | assert.Equal(t, []string{"workers"}, targets) | 196 | assert.Equal(t, []string{"workers"}, targets) | ||
| 195 | } | 197 | } | ||
| 198 | + | ||||
| 199 | + | func TestParseResponse_CapsErrorBodySize(t *testing.T) { | |||
| 200 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |||
| 201 | + | resp := &http.Response{ | |||
| 202 | + | StatusCode: 500, | |||
| 203 | + | Status: "500 Internal Server Error", | |||
| 204 | + | Body: io.NopCloser(strings.NewReader(largeBody)), | |||
| 205 | + | Header: make(http.Header), | |||
| 206 | + | } | |||
| 207 | + | ||||
| 208 | + | err := parseResponse(resp, nil) | |||
| 209 | + | ||||
| 210 | + | require.Error(t, err) | |||
| 211 | + | assert.Less(t, len(err.Error()), 2*1024, "error body should be capped to ~1KB") | |||
| 212 | + | } | |||
| 213 | + | ||||
| 214 | + | func TestStreamLogs_CapsErrorBodySize(t *testing.T) { | |||
| 215 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |||
| 216 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 217 | + | w.WriteHeader(http.StatusInternalServerError) | |||
| 218 | + | _, _ = w.Write([]byte(largeBody)) | |||
| 219 | + | })) | |||
| 220 | + | defer srv.Close() | |||
| 221 | + | ||||
| 222 | + | c := &Client{ | |||
| 223 | + | baseURL: srv.URL, | |||
| 224 | + | httpClient: srv.Client(), | |||
| 225 | + | } | |||
| 226 | + | ||||
| 227 | + | _, err := c.StreamProcessLogs(context.Background(), 100) | |||
| 228 | + | ||||
| 229 | + | require.Error(t, err) | |||
| 230 | + | assert.Less(t, len(err.Error()), 2*1024, "streaming error body should be capped to ~1KB") | |||
| 231 | + | } | |||
| 232 | + | ||||
| 233 | + | func TestRequestWithRetry_CapsErrorBodySize(t *testing.T) { | |||
| 234 | + | withFastRetry(t) | |||
| 235 | + | ||||
| 236 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |||
| 237 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 238 | + | w.WriteHeader(http.StatusBadGateway) | |||
| 239 | + | _, _ = w.Write([]byte(largeBody)) | |||
| 240 | + | })) | |||
| 241 | + | defer srv.Close() | |||
| 242 | + | ||||
| 243 | + | c := &Client{ | |||
| 244 | + | baseURL: srv.URL, | |||
| 245 | + | httpClient: srv.Client(), | |||
| 246 | + | } | |||
| 247 | + | ||||
| 248 | + | _, err := c.requestWithRetry(context.Background(), http.MethodPost, "/test", nil) | |||
| 249 | + | ||||
| 250 | + | require.Error(t, err) | |||
| 251 | + | assert.Less(t, len(err.Error()), 2*1024, "retry error body should be capped to ~1KB") | |||
| 252 | + | } | |||