Bound imageprocessor.Process input read to prevent unbounded memory use (#37)
All checks were successful
check / check (push) Successful in 4s
All checks were successful
check / check (push) Successful in 4s
closes #31 ## Problem `ImageProcessor.Process` used `io.ReadAll(input)` without any size limit, allowing arbitrarily large inputs to exhaust all available memory. This is a DoS vector — even though the upstream fetcher has a `MaxResponseSize` limit (50 MiB), the processor interface accepts any `io.Reader` and should defend itself independently. Additionally, the service layer's `processFromSourceOrFetch` read cached source content with `io.ReadAll` without a bound, so an unexpectedly large cached file could also cause unbounded memory consumption. ## Changes ### Processor (`processor.go`) - Added `maxInputBytes` field to `ImageProcessor` (configurable, defaults to 50 MiB via `DefaultMaxInputBytes`) - `NewImageProcessor` now accepts a `maxInputBytes` parameter (0 or negative uses the default) - `Process` now wraps the input reader with `io.LimitReader` and rejects inputs exceeding the limit with `ErrInputDataTooLarge` - Added `DefaultMaxInputBytes` and `ErrInputDataTooLarge` exported constants/errors ### Service (`service.go`) - `NewService` now wires the fetcher's `MaxResponseSize` through to the processor - Extracted `loadCachedSource` helper method to flatten nesting in `processFromSourceOrFetch` - Cached source reads are now bounded by `maxResponseSize` — oversized cached files are discarded and re-fetched ### Tests (`processor_test.go`) - `TestImageProcessor_RejectsOversizedInputData` — verifies that inputs exceeding `maxInputBytes` are rejected with `ErrInputDataTooLarge` - `TestImageProcessor_AcceptsInputWithinLimit` — verifies that inputs within the limit are processed normally - `TestImageProcessor_DefaultMaxInputBytes` — verifies that 0 and negative values use the default - All existing tests updated to use `NewImageProcessor(0)` (default limit) Co-authored-by: user <user@Mac.lan guest wan> Co-authored-by: clawbot <clawbot@eeqj.de> Reviewed-on: #37 Co-authored-by: clawbot <clawbot@noreply.example.org> Co-committed-by: clawbot <clawbot@noreply.example.org>
This commit was merged in pull request #37.
This commit is contained in:
@@ -11,17 +11,19 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/dustin/go-humanize"
|
||||
"sneak.berlin/go/pixa/internal/imageprocessor"
|
||||
)
|
||||
|
||||
// Service implements the ImageCache interface, orchestrating cache, fetcher, and processor.
|
||||
type Service struct {
|
||||
cache *Cache
|
||||
fetcher Fetcher
|
||||
processor Processor
|
||||
signer *Signer
|
||||
whitelist *HostWhitelist
|
||||
log *slog.Logger
|
||||
allowHTTP bool
|
||||
cache *Cache
|
||||
fetcher Fetcher
|
||||
processor *imageprocessor.ImageProcessor
|
||||
signer *Signer
|
||||
whitelist *HostWhitelist
|
||||
log *slog.Logger
|
||||
allowHTTP bool
|
||||
maxResponseSize int64
|
||||
}
|
||||
|
||||
// ServiceConfig holds configuration for the image service.
|
||||
@@ -50,15 +52,17 @@ func NewService(cfg *ServiceConfig) (*Service, error) {
|
||||
return nil, errors.New("signing key is required")
|
||||
}
|
||||
|
||||
// Resolve fetcher config for defaults
|
||||
fetcherCfg := cfg.FetcherConfig
|
||||
if fetcherCfg == nil {
|
||||
fetcherCfg = DefaultFetcherConfig()
|
||||
}
|
||||
|
||||
// Use custom fetcher if provided, otherwise create HTTP fetcher
|
||||
var fetcher Fetcher
|
||||
if cfg.Fetcher != nil {
|
||||
fetcher = cfg.Fetcher
|
||||
} else {
|
||||
fetcherCfg := cfg.FetcherConfig
|
||||
if fetcherCfg == nil {
|
||||
fetcherCfg = DefaultFetcherConfig()
|
||||
}
|
||||
fetcher = NewHTTPFetcher(fetcherCfg)
|
||||
}
|
||||
|
||||
@@ -74,14 +78,17 @@ func NewService(cfg *ServiceConfig) (*Service, error) {
|
||||
allowHTTP = cfg.FetcherConfig.AllowHTTP
|
||||
}
|
||||
|
||||
maxResponseSize := fetcherCfg.MaxResponseSize
|
||||
|
||||
return &Service{
|
||||
cache: cfg.Cache,
|
||||
fetcher: fetcher,
|
||||
processor: NewImageProcessor(),
|
||||
signer: signer,
|
||||
whitelist: NewHostWhitelist(cfg.Whitelist),
|
||||
log: log,
|
||||
allowHTTP: allowHTTP,
|
||||
cache: cfg.Cache,
|
||||
fetcher: fetcher,
|
||||
processor: imageprocessor.New(imageprocessor.Params{MaxInputBytes: maxResponseSize}),
|
||||
signer: signer,
|
||||
whitelist: NewHostWhitelist(cfg.Whitelist),
|
||||
log: log,
|
||||
allowHTTP: allowHTTP,
|
||||
maxResponseSize: maxResponseSize,
|
||||
}, nil
|
||||
}
|
||||
|
||||
@@ -146,6 +153,40 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e
|
||||
return response, nil
|
||||
}
|
||||
|
||||
// loadCachedSource attempts to load source content from cache, returning nil
|
||||
// if the cached data is unavailable or exceeds maxResponseSize.
|
||||
func (s *Service) loadCachedSource(contentHash ContentHash) []byte {
|
||||
reader, err := s.cache.GetSourceContent(contentHash)
|
||||
if err != nil {
|
||||
s.log.Warn("failed to load cached source, fetching", "error", err)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// Bound the read to maxResponseSize to prevent unbounded memory use
|
||||
// from unexpectedly large cached files.
|
||||
limited := io.LimitReader(reader, s.maxResponseSize+1)
|
||||
data, err := io.ReadAll(limited)
|
||||
_ = reader.Close()
|
||||
|
||||
if err != nil {
|
||||
s.log.Warn("failed to read cached source, fetching", "error", err)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
if int64(len(data)) > s.maxResponseSize {
|
||||
s.log.Warn("cached source exceeds max response size, discarding",
|
||||
"hash", contentHash,
|
||||
"max_bytes", s.maxResponseSize,
|
||||
)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
return data
|
||||
}
|
||||
|
||||
// processFromSourceOrFetch processes an image, using cached source content if available.
|
||||
func (s *Service) processFromSourceOrFetch(
|
||||
ctx context.Context,
|
||||
@@ -162,22 +203,8 @@ func (s *Service) processFromSourceOrFetch(
|
||||
var fetchBytes int64
|
||||
|
||||
if contentHash != "" {
|
||||
// We have cached source - load it
|
||||
s.log.Debug("using cached source", "hash", contentHash)
|
||||
|
||||
reader, err := s.cache.GetSourceContent(contentHash)
|
||||
if err != nil {
|
||||
s.log.Warn("failed to load cached source, fetching", "error", err)
|
||||
// Fall through to fetch
|
||||
} else {
|
||||
sourceData, err = io.ReadAll(reader)
|
||||
_ = reader.Close()
|
||||
|
||||
if err != nil {
|
||||
s.log.Warn("failed to read cached source, fetching", "error", err)
|
||||
// Fall through to fetch
|
||||
}
|
||||
}
|
||||
sourceData = s.loadCachedSource(contentHash)
|
||||
}
|
||||
|
||||
// Fetch from upstream if we don't have source data or it's empty
|
||||
@@ -274,7 +301,14 @@ func (s *Service) processAndStore(
|
||||
// Process the image
|
||||
processStart := time.Now()
|
||||
|
||||
processResult, err := s.processor.Process(ctx, bytes.NewReader(sourceData), req)
|
||||
processReq := &imageprocessor.Request{
|
||||
Size: imageprocessor.Size{Width: req.Size.Width, Height: req.Size.Height},
|
||||
Format: imageprocessor.Format(req.Format),
|
||||
Quality: req.Quality,
|
||||
FitMode: imageprocessor.FitMode(req.FitMode),
|
||||
}
|
||||
|
||||
processResult, err := s.processor.Process(ctx, bytes.NewReader(sourceData), processReq)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("image processing failed: %w", err)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user