fix(thumbnails): bound declared image dimensions before decoding - #3457
Merged
Conversation
dschmidt
force-pushed
the
fix/thumbnails-dimension-guard
branch
from
September 3, 2026 12:16
da512aa to
1a104b5
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
🟢 Coverage 76.71% diff coverage · +0.06% coverage variation
Metric Results Coverage variation ✅ +0.06% coverage variation (-1.00%) Diff coverage ✅ 76.71% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (61037cc) 88237 20687 23.44% Head commit (0d9565f) 88300 (+63) 20751 (+64) 23.50% (+0.06%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3457) 73 56 76.71% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
dschmidt
force-pushed
the
fix/thumbnails-dimension-guard
branch
from
September 3, 2026 12:19
1a104b5 to
bff00be
Compare
The imaging build decodes the full pixel buffer from the header-declared dimensions before the existing MaxInputWidth/MaxInputHeight guard runs, so a tiny crafted file whose header declares huge dimensions forces a multi-GB allocation and can OOM the worker. Read the header with DecodeConfig and reject oversized sources before the decode allocates, in both the imaging and vips builds, and thread the limit through the audio cover-art and geogebra decoders that decode a second attacker-controlled image.
dschmidt
force-pushed
the
fix/thumbnails-dimension-guard
branch
from
September 3, 2026 12:21
bff00be to
50c38bf
Compare
dschmidt
marked this pull request as ready for review
September 3, 2026 12:23
There was a problem hiding this comment.
🟡 Changes recommended
The vips rejection path leaks resources, and oversized conversion errors return the wrong API status.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds pre-decode dimension validation to prevent crafted images from exhausting thumbnail-worker memory.
Changes:
- Propagates configured input limits through preprocessors and nested decoders.
- Guards imaging and vips decoding before pixel allocation.
- Adds regression tests for oversized images.
File summaries
| File | Review |
|---|---|
services/thumbnails/pkg/service/grpc/v0/service.go |
Passes limits to preprocessors. Moderate: ErrImageTooLarge from conversion is incorrectly mapped to NotFound instead of Forbidden at lines 174 and 272. |
services/thumbnails/pkg/preprocessor/preprocessor.go |
Implements shared guards and nested limit propagation. |
services/thumbnails/pkg/preprocessor/preprocessor_vips.go |
Checks dimensions before processing. Moderate: close rejected ImageRef values to avoid retaining Go and native memory. |
services/thumbnails/pkg/preprocessor/preprocessor_test.go |
Updates decoder construction for limits. |
services/thumbnails/pkg/preprocessor/preprocessor_imaging.go |
Checks header dimensions before pixel decoding. |
services/thumbnails/pkg/preprocessor/dimensionguard_test.go |
Tests oversized-image rejection and recursive propagation. |
Review details
Suppressed comments (1)
services/thumbnails/pkg/service/grpc/v0/service.go:273
- The newly enabled preprocessor limit can return
ErrImageTooLarge, but this path currently converts that sentinel to NotFound, unlike the Forbidden responses used for the same error from the source and manager. Preserve the established oversized-image mapping before handling generic conversion failures.
"maxInputWidth": g.preprocessorOpts.MaxInputWidth,
"maxInputHeight": g.preprocessorOpts.MaxInputHeight,
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+26
to
+28
| if v.limit.exceeded(img.Width(), img.Height()) { | ||
| return nil, thumbnailerErrors.ErrImageTooLarge | ||
| } |
Comment on lines
+174
to
+175
| "maxInputWidth": g.preprocessorOpts.MaxInputWidth, | ||
| "maxInputHeight": g.preprocessorOpts.MaxInputHeight, |
This was referenced Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The non-vips (imaging) build decodes the full pixel buffer from the header declared dimensions before the existing
MaxInputWidth/MaxInputHeightguard runs. That guard sits inSimpleManager.Generate, at which pointpreprocessor.Converthas already decoded the image. The Go stdlib backend allocatesimage.NewGray/image.NewYCbCrfor the declared width*height at the SOS marker, before it fails on the mismatched scan data, so a tiny crafted file forces a large allocation and can exhaust the memory of the thumbnails worker.This reads only the header with
DecodeConfig, which allocates no pixels, and rejects oversized sources before the decode happens. The limit is threaded through the decoders that recurse into a second attacker controlled image: audio cover art (mp3/flac/ogg) and the geogebra decoders. The vips build gets the same check on the header lazyvips.NewImageFromReader, beforeThumbnailWithSizematerializes anything.The service now passes the configured
MaxInputWidth/MaxInputHeightinto the preprocessor options, so a zero limit keeps the previous behaviour and an unconfigured decoder stays unrestricted.Impact is availability only, and the official container images are not affected because they build with
ENABLE_VIPS=true. Affected are the non-vips builds produced bymake release-linux/release-darwin, so development builds and the published standalone binaries. Reachable by anyone who can have a preview generated, which means authenticated upload or a public share.Related Issue
Reported privately to security@opencloud.eu on 2026-08-18 and cleared for public handling, no public issue exists.
Motivation and Context
A 375 byte grayscale JPEG whose SOF0 declares large dimensions while the pixel data stays 8x8 drives the allocation in
ImageDecoder.Convert. Peak liveHeapAlloc: 20000x20000 gives roughly 381 MB, 65535x65535 gives roughly 4 GB. With the guard in place the same files stay at roughly 17 KB.How Has This Been Tested?
go test ./services/thumbnails/..., plus a running container to check the vips pathErrImageTooLargebefore the decode, verified by the new specs and by peak heap measurementTypes of changes
Checklist: