mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
fix(server): close the background image body on a non-200 response
serveImage returned early on an unexpected status code without closing the response body, pinning the connection until the 5s client timeout. The nolint:bodyclose above the request suppressed the linter that would have caught it, and its justification only holds on the success path, where the body is handed to the CachedStream wrapper.
This commit is contained in:
parent
8c44f877b6
commit
a20338c45a
17
server/backgrounds/backgrounds_suite_test.go
Normal file
17
server/backgrounds/backgrounds_suite_test.go
Normal file
@ -0,0 +1,17 @@
|
||||
package backgrounds
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/navidrome/navidrome/log"
|
||||
"github.com/navidrome/navidrome/tests"
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
. "github.com/onsi/gomega"
|
||||
)
|
||||
|
||||
func TestBackgrounds(t *testing.T) {
|
||||
tests.Init(t, false)
|
||||
log.SetLevel(log.LevelFatal)
|
||||
RegisterFailHandler(Fail)
|
||||
RunSpecs(t, "Backgrounds Suite")
|
||||
}
|
||||
@ -80,7 +80,7 @@ func (h *Handler) serveImage(ctx context.Context, item cache.Item) (io.Reader, e
|
||||
}
|
||||
c := http.Client{Timeout: imageRequestTimeout}
|
||||
req, _ := http.NewRequestWithContext(ctx, http.MethodGet, imageURL(image), nil)
|
||||
resp, err := c.Do(req) //nolint:bodyclose,gosec // No need to close resp.Body, it will be closed via the CachedStream wrapper
|
||||
resp, err := c.Do(req) //nolint:bodyclose,gosec // On success the body is closed via the CachedStream wrapper
|
||||
if errors.Is(err, context.DeadlineExceeded) {
|
||||
defaultImage, _ := base64.StdEncoding.DecodeString(consts.DefaultUILoginBackgroundOffline)
|
||||
return strings.NewReader(string(defaultImage)), nil
|
||||
@ -89,6 +89,7 @@ func (h *Handler) serveImage(ctx context.Context, item cache.Item) (io.Reader, e
|
||||
return nil, fmt.Errorf("could not get background image from hosting service: %w", err)
|
||||
}
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
_ = resp.Body.Close()
|
||||
return nil, fmt.Errorf("unexpected status code getting background image from hosting service: %d", resp.StatusCode)
|
||||
}
|
||||
log.Debug(ctx, "Got background image from hosting service", "image", image, "elapsed", time.Since(start))
|
||||
|
||||
72
server/backgrounds/handler_test.go
Normal file
72
server/backgrounds/handler_test.go
Normal file
@ -0,0 +1,72 @@
|
||||
package backgrounds
|
||||
|
||||
import (
|
||||
"context"
|
||||
"io"
|
||||
"net/http"
|
||||
"strings"
|
||||
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
. "github.com/onsi/gomega"
|
||||
)
|
||||
|
||||
type testItem string
|
||||
|
||||
func (i testItem) Key() string { return string(i) }
|
||||
|
||||
type recordingBody struct {
|
||||
io.Reader
|
||||
closed *bool
|
||||
}
|
||||
|
||||
func (b recordingBody) Close() error {
|
||||
*b.closed = true
|
||||
return nil
|
||||
}
|
||||
|
||||
type stubTransport struct {
|
||||
statusCode int
|
||||
closed *bool
|
||||
}
|
||||
|
||||
func (t stubTransport) RoundTrip(*http.Request) (*http.Response, error) {
|
||||
return &http.Response{
|
||||
StatusCode: t.statusCode,
|
||||
Header: make(http.Header),
|
||||
Body: recordingBody{Reader: strings.NewReader("image-bytes"), closed: t.closed},
|
||||
}, nil
|
||||
}
|
||||
|
||||
var _ = Describe("serveImage", func() {
|
||||
var closed bool
|
||||
|
||||
BeforeEach(func() {
|
||||
closed = false
|
||||
})
|
||||
|
||||
stubStatus := func(statusCode int) {
|
||||
original := http.DefaultTransport
|
||||
http.DefaultTransport = stubTransport{statusCode: statusCode, closed: &closed}
|
||||
DeferCleanup(func() { http.DefaultTransport = original })
|
||||
}
|
||||
|
||||
It("closes the response body when the hosting service returns an error", func() {
|
||||
stubStatus(http.StatusNotFound)
|
||||
|
||||
_, err := (&Handler{}).serveImage(context.Background(), testItem("some-image.webp"))
|
||||
|
||||
Expect(err).To(MatchError(ContainSubstring("unexpected status code")))
|
||||
Expect(closed).To(BeTrue(), "response body was left open")
|
||||
})
|
||||
|
||||
It("hands the still-open body to the caller on success", func() {
|
||||
stubStatus(http.StatusOK)
|
||||
|
||||
reader, err := (&Handler{}).serveImage(context.Background(), testItem("some-image.webp"))
|
||||
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(closed).To(BeFalse(), "response body must stay open for the CachedStream wrapper")
|
||||
body, _ := io.ReadAll(reader)
|
||||
Expect(string(body)).To(Equal("image-bytes"))
|
||||
})
|
||||
})
|
||||
Loading…
x
Reference in New Issue
Block a user