diff --git a/TODO.md b/TODO.md index b85bf37..c046779 100644 --- a/TODO.md +++ b/TODO.md @@ -38,6 +38,15 @@ P2: security: per-origin rate limiting commented entry under `example.com`. Getting Started and "Running under upaas" in `README.md` say that every image on an allowlisted host is served without a signature, so an allowlisted host must be one the operator controls. +- 2026-10-15 a chunked upstream body of exactly `upstream_max_response_size` + bytes is accepted (closes #248): once the limit was used up, the fetcher's + next read refused the body without looking for its end. Go's HTTP client + returns the end of a body sent with a `Content-Length` together with its last + bytes, but the end of a chunked body can come in a later read, so such a body + was refused after it had been downloaded whole, and the image routes + answered 500. Once the limit is used up, the fetcher now reads one more byte: + if the body has ended it is accepted, and if there is more it is refused with + `ErrResponseTooLarge`, as before. - 2026-10-13 `fit=outside` no longer makes an output above 8192 on a side (closes #257): it scales the image to cover the requested size, and the side scaled past that size was held to no maximum, so `1x8192?fit=outside` of a diff --git a/internal/httpfetcher/fetch_internal_test.go b/internal/httpfetcher/fetch_internal_test.go index 83c78c0..c443987 100644 --- a/internal/httpfetcher/fetch_internal_test.go +++ b/internal/httpfetcher/fetch_internal_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "slices" + "strconv" "strings" "sync" "testing" @@ -297,6 +298,139 @@ func TestFetchMaxResponseSizeEnforced(t *testing.T) { } } +// fetchWithMaxResponseSize serves handler as the fake upstream and fetches +// its /image route with MaxResponseSize set to maxResponseSize. +func fetchWithMaxResponseSize( + t *testing.T, + maxResponseSize int64, + handler http.HandlerFunc, +) *FetchResult { + t.Helper() + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + cfg := DefaultConfig() + cfg.MaxResponseSize = maxResponseSize + + f, _ := newServerFetcher(t, srv, cfg) + + res := fetchImage(t, f, "/image") + t.Cleanup(func() { _ = res.Content.Close() }) + + return res +} + +// TestFetchContentLengthBodyAtMaxResponseSize checks that a body sent with a +// Content-Length is read whole at exactly MaxResponseSize bytes and refused +// at one byte more. +func TestFetchContentLengthBodyAtMaxResponseSize(t *testing.T) { + t.Parallel() + + const maxResponseSize = 8 + + for _, tc := range []struct { + size int + wantErr error + }{ + {maxResponseSize, nil}, + {maxResponseSize + 1, ErrResponseTooLarge}, + } { + t.Run(strconv.Itoa(tc.size)+" bytes", func(t *testing.T) { + t.Parallel() + + body := strings.Repeat("x", tc.size) + + res := fetchWithMaxResponseSize(t, maxResponseSize, + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", contentTypeJPEG) + w.Header().Set("Content-Length", strconv.Itoa(tc.size)) + _, _ = io.WriteString(w, body) + }) + + if res.ContentLength != int64(tc.size) { + t.Fatalf("ContentLength = %d, want %d", res.ContentLength, tc.size) + } + + data, err := io.ReadAll(res.Content) + t.Logf("body of %d bytes: read %d, error %v", tc.size, len(data), err) + + if !errors.Is(err, tc.wantErr) { + t.Fatalf("read error = %v, want %v", err, tc.wantErr) + } + + if err == nil && string(data) != body { + t.Errorf("read %q, want %q", data, body) + } + }) + } +} + +// TestFetchChunkedBodyAtMaxResponseSize checks that a chunked body is read +// whole at exactly MaxResponseSize bytes and refused at one byte more. The +// upstream sends the end of the body only once the test has read +// MaxResponseSize bytes, so the read after them comes before the end of the +// body has arrived, as it can from a real upstream. +func TestFetchChunkedBodyAtMaxResponseSize(t *testing.T) { + t.Parallel() + + const maxResponseSize = 8 + + for _, tc := range []struct { + size int + wantErr error + }{ + {maxResponseSize, nil}, + {maxResponseSize + 1, ErrResponseTooLarge}, + } { + t.Run(strconv.Itoa(tc.size)+" bytes", func(t *testing.T) { + t.Parallel() + + body := strings.Repeat("x", tc.size) + endBody := make(chan struct{}) + + res := fetchWithMaxResponseSize(t, maxResponseSize, + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", contentTypeJPEG) + _, _ = io.WriteString(w, body) + // With no Content-Length, flushing sends the body + // chunked; its end is sent when the handler returns. + _ = http.NewResponseController(w).Flush() + + select { + case <-endBody: + case <-r.Context().Done(): + } + }) + + if res.ContentLength != -1 { + t.Fatalf("ContentLength = %d, want -1 (chunked)", res.ContentLength) + } + + head := make([]byte, maxResponseSize) + + _, err := io.ReadFull(res.Content, head) + if err != nil { + t.Fatalf("read the first %d bytes: %v", maxResponseSize, err) + } + + close(endBody) + + rest, err := io.ReadAll(res.Content) + data := string(head) + string(rest) + t.Logf("body of %d bytes: read %d, error %v", tc.size, len(data), err) + + if !errors.Is(err, tc.wantErr) { + t.Fatalf("read error = %v, want %v", err, tc.wantErr) + } + + if err == nil && data != body { + t.Errorf("read %q, want %q", data, body) + } + }) + } +} + func TestFetchSemaphoreReleasedOnError(t *testing.T) { t.Parallel() diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 9d09295..1428040 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -660,14 +660,33 @@ func dialSSRFSafe( return nil, fmt.Errorf("%w to %s", errConnectFailed, host) } -// limitedReader wraps a reader and limits the number of bytes read. +// limitedReader wraps a reader and refuses a body longer than remaining +// bytes with ErrResponseTooLarge. remaining goes below 0 once the body is +// found to be longer. type limitedReader struct { reader io.Reader remaining int64 } +// Read reads at most the remaining bytes. Once they are read, it reads one +// more byte to tell the end of the body from more data: at the end it +// returns the wrapped reader's error, io.EOF; with more data it returns +// ErrResponseTooLarge, as does every later Read. func (r *limitedReader) Read(p []byte) (int, error) { - if r.remaining <= 0 { + if r.remaining < 0 { + return 0, ErrResponseTooLarge + } + + if r.remaining == 0 { + var extra [1]byte + + n, err := r.reader.Read(extra[:]) + r.remaining -= int64(n) + + if n == 0 { + return 0, err + } + return 0, ErrResponseTooLarge }