fix: resolve image proxy returning empty responses for SVGs#946
fix: resolve image proxy returning empty responses for SVGs#946Soner (shyim) wants to merge 3 commits intomainfrom
Conversation
The responseCapture wrapper around http.ResponseWriter broke the http.Flusher interface chain between the gzip handler and the reverse proxy. This caused SVG responses (text-based, often served with chunked transfer encoding) to arrive empty, while binary images with fixed Content-Length worked fine. Replace responseCapture with proxy.ModifyResponse to cache response bodies without wrapping the ResponseWriter. Also remove binary image formats (JPEG, PNG, GIF, WebP) from gzip content types since they are already compressed. Fixes #945
- Check error return values (errcheck) - Use http.NewRequestWithContext instead of http.NewRequest (noctx) - Use http.DefaultClient.Do instead of http.Get (noctx)
There was a problem hiding this comment.
Pull request overview
This PR fixes SVGs returning empty bodies through project image-proxy by removing the http.ResponseWriter wrapper that broke the http.Flusher interface chain used by the gzip handler, and instead capturing upstream responses via httputil.ReverseProxy.ModifyResponse. It also avoids gzip-compressing already-compressed binary image formats.
Changes:
- Replace
responseCaptureResponseWriter-wrapping cache logic withReverseProxy.ModifyResponsebody capture. - Remove JPEG/PNG/GIF/WebP from gzip content types (keep text-based +
image/svg+xml). - Add test coverage for SVG proxying (gzip/no-gzip, chunked responses, cache hits, browser-like headers) and ensure PNG isn’t gzipped.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/project/project_image_proxy.go | Moves caching to ModifyResponse to preserve http.Flusher; adjusts gzip content types to exclude binary image formats. |
| cmd/project/project_image_proxy_test.go | Adds integration-style tests covering SVG proxying behavior, gzip wrapping, chunked upstream responses, and cache hits. |
Comments suppressed due to low confidence (1)
cmd/project/project_image_proxy.go:265
- The code/tests rely on
gziphandler’s default minimum-size behavior (small SVGs are expected to stay uncompressed). If this behavior is important for the CLI, consider settingMinSizeexplicitly in the gzip options so behavior stays deterministic across library updates.
// Enable gzip compression for text-based content types.
// Binary image formats (JPEG, PNG, GIF, WebP) are excluded because
// they are already compressed and gzip provides no benefit.
gzipWrapper, _ := gziphandler.GzipHandlerWithOpts(
gziphandler.ContentTypes([]string{
"text/html",
"text/css",
"text/javascript",
"application/javascript",
"application/json",
"application/vnd.api+json",
"image/svg+xml",
"text/xml",
"application/xml",
"text/plain",
}),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/project/project_image_proxy.go
Outdated
| body, err := io.ReadAll(res.Body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| res.Body.Close() | ||
| res.Body = io.NopCloser(bytes.NewReader(body)) |
There was a problem hiding this comment.
ModifyResponse reads the entire upstream response into memory (io.ReadAll) before the reverse proxy can write anything to the client. This removes streaming/chunked delivery and increases time-to-first-byte for large responses (and can spike memory for large images). Consider streaming while capturing (e.g., tee the response body to the cache as it is read) rather than fully buffering up-front.
cmd/project/project_image_proxy.go
Outdated
| cleanPath := filepath.Clean(res.Request.URL.Path) | ||
| contentType := res.Header.Get("Content-Type") | ||
|
|
||
| body, err := io.ReadAll(res.Body) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Cache metadata only persists Content-Type. If the upstream response includes a non-identity Content-Encoding (e.g., gzip), the cached bytes will later be served without that header and may also be gzip-compressed again by the outer gziphandler, corrupting cache hits. A robust fix is to normalize upstream responses to identity encoding before caching (e.g., strip Accept-Encoding on the proxied request so Go transparently decompresses) or store/restore Content-Encoding alongside the cached body.
Address review feedback: - Use io.TeeReader to stream responses to the client while simultaneously capturing bytes for caching, instead of buffering the entire response in memory before writing. - Strip Accept-Encoding from the outgoing proxy request so Go's Transport transparently decompresses upstream responses. This ensures the cache always stores identity-encoded content, preventing double-compression or serving stale Content-Encoding on cache hits.
Summary
Fixes #945
The
responseCapturewrapper aroundhttp.ResponseWriterbroke thehttp.Flusherinterface chain between the gzip handler and the reverse proxy. This caused SVG responses (text-based, often served with chunked transfer encoding) to arrive with empty bodies, while binary images (which have fixedContent-Lengthand don't rely on flushing) worked fine.responseCapturewithproxy.ModifyResponse— caches response bodies without wrapping the ResponseWriter, keeping the full interface chain intact for the gzip handlerTest plan
go test ./...)