feat: storage, thumbnails, image inspection, and ZIP extraction #3
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/storage-and-thumbnails"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements the storage, thumbnail, image inspection, and ZIP extraction infrastructure layer — no REST endpoints yet.
What's included
ApplicationCore — 5 new ports
IImageInspectorIImageStorageIThumbnailGeneratorIThumbnailStorageIZipExtractorInfrastructure — 5 implementations
SkiaSharpImageInspectorSkiaSharpThumbnailGeneratorFilesystemImageStorage{imageDir}/{shard}/{uuid}.extwith auto-mkdirFilesystemThumbnailStorage{thumbnailDir}/{shard}/{uuid}.webpZipExtractorZipArchive-based,IAsyncEnumerable,[EnumeratorCancellation], sorts by path, ignores non-imagesSkiaSharp package strategy (from OpenRouter.Net)
Following the proven pattern from
TeamAI/OpenRouter.Net:SkiaSharp(managed wrapper only)SkiaSharp.NativeAssets.Linux.NoDependencies(provideslibSkiaSharp.so)SkiaSharp.NativeAssets.Linux.NoDependencies(so tests run on Linux)Server DI registration
All services registered as singletons with env-var configurable paths:
DOUJIN_MANAGER_IMAGE_DIR(default:/app/data/images)DOUJIN_MANAGER_THUMBNAIL_DIR(default:/app/data/thumbnails)Tests (24 new, 47 total, all passing)
FilesystemImageStorageTestsFilesystemThumbnailStorageTestsSkiaSharpImageInspectorTestsSkiaSharpThumbnailGeneratorTestsZipExtractorTestsVerification
dotnet builddotnet testgit diff --checkRelated ADRs
ApplicationCore ports: - IImageInspector: decode image metadata (dimensions, mediaType, sha256) - IImageStorage: save/read/delete/exists for UUID-sharded image files - IThumbnailGenerator: generate WebP thumbnails from image stream - IThumbnailStorage: save/read/delete/exists for WebP thumbnails - IZipExtractor: extract images from ZIP sorted by path, ignore non-images Infrastructure implementations: - SkiaSharpImageInspector: SkiaSharp decode + magic-byte format detection (JPEG/PNG/GIF/WebP/BMP) + SHA-256 hashing - SkiaSharpThumbnailGenerator: Mitchell cubic resize preserving aspect ratio, WebP encode at quality 80, no upscaling - FilesystemImageStorage: {imageDir}/{shard}/{uuid}.ext with auto-mkdir - FilesystemThumbnailStorage: {thumbnailDir}/{shard}/{uuid}.webp - ZipExtractor: ZipArchive-based, IAsyncEnumerable, EnumeratorCancellation SkiaSharp package strategy (following OpenRouter.Net pattern): - Infrastructure: SkiaSharp (managed wrapper only) - Server: SkiaSharp.NativeAssets.Linux.NoDependencies (libSkiaSharp.so) - Infrastructure.Tests: SkiaSharp.NativeAssets.Linux.NoDependencies - All pinned at 3.119.4 via CPM Server DI registration: - IImageInspector, IThumbnailGenerator, IZipExtractor as singletons - IImageStorage, IThumbnailStorage with env-var paths (DOUJIN_MANAGER_IMAGE_DIR, DOUJIN_MANAGER_THUMBNAIL_DIR) Tests (24 new, 47 total, all passing): - FilesystemImageStorage: 5 tests (save, read, exists, delete, missing file) - FilesystemThumbnailStorage: 4 tests (save, exists, read, delete) - SkiaSharpImageInspector: 5 tests (jpeg, png, sha256, different hashes, invalid) - SkiaSharpThumbnailGenerator: 5 tests (webp output, aspect ratio, no upscale, square, invalid) - ZipExtractor: 5 tests (sorted extraction, ignore non-images, content, empty, nested dirs)@ -0,0 +10,4 @@/// </summary>public class SkiaSharpImageInspector : IImageInspector{private static readonly string[] SupportedExtensions = [".jpg", ".jpeg", ".png", ".gif", ".webp", ".bmp"];I would like you to centralize the supported file formats: Single source of truth. Both this class and ZipExtractor have the exact same list of items. Considering that it is SkiaSharpImageInspector who defines the supported formats, ZipExtractor shouldn't know this information at all, but at best retrieve it.
🤖 Hermes automated review: changes requested
Independent review of this PR (head
b9dd78f7, 21 files, +919). Local checks ran clean:dotnet build→ 0 errors, 0 warnings ✅dotnet test→ 47/47 pass (24 new tests inInfrastructure.Tests) ✅Two blocking findings before this should merge. Details with
file:linereferences and suggested fixes below.🔴 Major
1.
backend/src/DoujinManager.Infrastructure/Archives/ZipExtractor.cs:38— Unbounded memory allocation from untrusted ZIP inputEach entry is copied entirely into a
MemoryStreambefore being yielded:A malicious "decompression bomb" (tiny compressed file → gigabytes decompressed) or a large legitimate archive will OOM the process. Since ZIPs come from user uploads, this is the highest-risk issue.
Suggested fix: Check
entry.Lengthagainst a configurable max (e.g. 100 MB) before copying, and pass a bounded buffer toCopyToAsync. Consider streaming instead of buffering when the caller can consume incrementally.2.
backend/src/DoujinManager.Infrastructure/Archives/ZipExtractor.cs:16— Archive disposal on partial/abandoned enumerationusing var archive = new ZipArchive(zipStream, ZipArchiveMode.Read, leaveOpen: false)disposes only when the iterator completes normally. If a caller breaks out ofawait foreachearly or an exception propagates, disposal depends on the callerDisposeAsync-ing the enumerator (whichawait foreachdoes, but only if the loop is exited viabreak/exception, not if the enumerator is abandoned). Also,leaveOpen: falsedisposes the underlyingzipStream— surprising if the caller owns that stream.Suggested fix: Move the archive into a
try/finallyinside the iterator, or switchleaveOpentotrueand document stream ownership. At minimum, add a doc comment stating the caller must fully consume orawait DisposeAsyncthe enumerator.🟡 Minor (non-blocking)
3.
backend/src/DoujinManager.Infrastructure/Storage/FilesystemImageStorage.cs:47-50— No validation ofextensionparameterextensionis concatenated into the path with no check.ImageFileIdis aGuid(safe), but a caller passingextension: "../../../etc/passwd"would traverse. Current callers are internal/trusted, but defense-in-depth is cheap here.Suggested fix: Validate
extensionagainst the sameImageExtensionswhitelist used inZipExtractor, or assert it matches^\.[a-z0-9]+$.4.
backend/src/DoujinManager.Infrastructure/Images/SkiaSharpImageInspector.cs:16-18— Entire stream loaded into memory with no size capstream.CopyToAsync(ms)with no limit. A huge uploaded image causes OOM, same class of issue as finding #1.Suggested fix: Enforce a max size (check
stream.Lengthif seekable, or use a boundedCopyToAsyncvariant).5.
backend/src/DoujinManager.Infrastructure/Images/SkiaSharpThumbnailGenerator.cs:34-36— Cancellation only checked onceThrowIfCancellationRequestedruns at the start ofTask.Run, but the CPU-bound decode/resize/encode cannot be interrupted mid-operation. Cancellation is delayed for large images. Not a correctness bug; note for future.6.
FilesystemImageStorage.cs:35/FilesystemThumbnailStorage.cs:40— Unnecessary TOCTOU on deleteif (File.Exists(path)) File.Delete(path)has a benign race.File.Deleteis a no-op on a missing file on .NET, so theExistsguard is misleading and unnecessary — just callFile.Delete(path)directly.✅ What looks good
ApplicationCore, implementations inInfrastructure.ImageFileIdis a strongly-typedGuid— no path traversal via the ID.ZipExtractoryieldsentry.Name(basename) +MemoryStream, never writes archive paths to disk.OpenRouter.Net) is sound.Automated daily review. I never merge PRs. This is a conversation comment, not a formal Forgejo review approval — the current MCP integration cannot create review approval states.
@hermes-agent wrote in #3 (comment):
The limit should be a bit higher than that. Valid large uploads of multiple 100 images with decent quality images can easily be reaching 500MB
Review feedback addressed — commit
1f1ef60All blocking and non-blocking findings have been fixed. 54 tests pass (7 new).
🔴 Major findings
#1 — ZipExtractor unbounded memory (decompression bomb)
maxEntrySizeBytesparameter toIZipExtractor.ExtractAsync(default: 500 MB per @bjoern's feedback)MemoryStreampre-allocated fromentry.Lengthto avoid over-allocationExtractAsync_Skips_Entries_Exceeding_MaxSize#2 — ZipExtractor archive disposal / stream ownership
leaveOpenfromfalsetotrue— caller owns the streamExtractAsync_Does_Not_Dispose_Caller_Stream🟡 Minor findings
#3 — FilesystemImageStorage extension validation
^\.[a-z0-9]+$regex validation on all public methods../../../etc/passwdis now rejectedSaveAsync_Throws_For_Invalid_Extension,SaveAsync_Throws_For_Empty_Extension#4 — SkiaSharpImageInspector size cap
maxSizeBytesparameter toIImageInspector.InspectAsync(default: 500 MB)stream.Lengthif seekable; bounded read loop for non-seekable streamsInspectAsync_Throws_When_Stream_Exceeds_MaxSize#5 — ThumbnailGenerator cancellation
#6 — File.Delete TOCTOU
File.Existsguard. However,File.Deleteon .NET throwsDirectoryNotFoundExceptionwhen the parent directory doesn't exist (contrary to the review's assumption that it's a no-op). Added acatch (DirectoryNotFoundException)to handle this cleanly.DeleteAsync_Does_Not_Throw_When_File_Does_Not_Exist(both storage classes)Single source of truth — commit
31c3563Fixed. The supported extensions list now lives in one place.
Changes:
IImageInspectornow exposesIReadOnlySet<string> SupportedExtensions— the inspector is the single source of truth for which image formats are supportedSkiaSharpImageInspectorpublishes its extensions set as a public propertyZipExtractortakesIReadOnlySet<string>via constructor injection — it no longer has any knowledge of which formats are supported, it just filters using whatever set it's givenServer/Program.cscreates the inspector first, then passesinspector.SupportedExtensionsto theZipExtractorconstructorTests added:
SkiaSharpImageInspectorTests.SupportedExtensions_Contains_Expected_Formats— verifies the inspector exposes all 6 formatsZipExtractorTests.ExtractAsync_Uses_Provided_Extensions_Set— verifies the extractor respects the injected set (creates an extractor with only.png, confirms.jpgentries are filtered out)56 tests pass, 0 warnings.
🤖 Hermes automated review: changes requested
Reviewed the updated head
31c3563after the earlier fixes. Local checks passed:git diff --check→ clean ✅dotnet build --no-restore backend/DoujinManager.slnx→ 0 warnings, 0 errors ✅dotnet test --no-restore --no-build backend/DoujinManager.slnx→ 56/56 tests passed ✅The prior ZIP-size/stream-ownership/extension-source findings look addressed. I found one remaining blocking DoS risk before this should merge.
🔴 Major
1.
backend/src/DoujinManager.Infrastructure/Images/SkiaSharpImageInspector.cs:51andbackend/src/DoujinManager.Infrastructure/Images/SkiaSharpThumbnailGenerator.cs:21— compressed-size limits do not protect against image pixel bombsThe new 500 MB byte limit is useful, but both image paths still fully decode attacker-controlled image data into an
SKBitmap:A malicious PNG/WebP/JPEG can be small on disk but declare extremely large dimensions. That stays under
maxSizeBytes, thenSKBitmap.Decode(...)attempts to allocate memory based on decoded pixels, potentially exhausting process memory. The current tests cover compressed byte size, but not decoded pixel dimensions.Suggested fix: inspect image metadata before full bitmap decode and enforce a maximum decoded pixel area/dimension, e.g. via
SKCodec.Create(...)/codec.Info(or equivalent bounds-only decode) before allocating anSKBitmap. Apply the same limit inSkiaSharpThumbnailGenerator, or make thumbnail generation require/consume a previously validatedImageInspection. Add tests for a small-byte image with dimensions above the allowed pixel cap.🟡 Minor (non-blocking)
2.
backend/src/DoujinManager.Infrastructure/Archives/ZipExtractor.cs:43-44— oversized ZIP entries are silently droppedSkipping entries above
maxEntrySizeBytesavoids OOM, but it can also produce incomplete imports without any signal to the caller/user. This is especially easy to miss later when REST endpoints are added.Suggested fix: consider returning skipped-entry metadata or throwing a domain exception for oversized image entries so upload/import code can show an explicit error instead of silently losing pages.
Automated daily review. I never merge PRs. This is a conversation comment, not a formal Forgejo review approval — the current MCP integration cannot create review approval states.
Second-round review feedback addressed — commit
dc91cc7Both findings from comment #46 are fixed. 58 tests pass.
🔴 #1 — Pixel bomb protection
Inspector:
IImageInspector.InspectAsyncnow takesmaxPixelDimensionparameter (default: 30,000)SkiaSharpImageInspectorusesSKCodecto read declared dimensions before allocating a fullSKBitmapArgumentExceptionimmediately — no pixel allocationThumbnail generator:
MaxPixelDimension = 30,000guardSKCodecto check dimensions before full decode (secondary defense)✅ Test:
InspectAsync_Throws_For_Pixel_Bomb_Small_File_Huge_Dimensions— a 100×100 image is rejected whenmaxPixelDimensionis set to 50🟡 #2 — Oversized ZIP entries now throw
ZipExtractornow throwsInvalidDataExceptionwith the entry name and size instead of silently skipping✅ Test:
ExtractAsync_Throws_For_Entries_Exceeding_MaxSize(updated from skip-expectation to throw-expectation)