fix: upload size limit 30MB to 100MB, 413 instead of 500 on oversized requests #41
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/upload-size-limit"
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?
Problem
Batch image uploads (27 images, ~57MB) fail with
500 Internal Server Error. Two root causes:1. Kestrel 30MB default limit
Kestrel's default
MaxRequestBodySizeis 30,000,000 bytes (~28.6 MiB). The chapter upload of 27 images in one multipart POST exceeds this limit. Kestrel throwsBadHttpRequestException: Request body too largebefore the endpoint code even runs.Fix: Added
RequestSizeLimitAttribute(100_000_000)(100 MB) to both upload endpoints:POST /api/variants/{id}/pages(batch image upload)POST /api/variants/{id}/pages/zip(ZIP upload)2. 500 instead of 413 on oversized requests
GlobalExceptionMiddlewarecaughtBadHttpRequestExceptionand returned a generic500 Internal Server Errorwith"An unexpected error occurred.". This is misleading — the client has no idea what went wrong or what the actual limit is.Fix: Added a specific
catchforBadHttpRequestExceptionwith"Request body too large"in the message. Returns:413 Payload Too Large(not 500)"The request body exceeds the maximum allowed size of 100,000,000 bytes (100.0 MB)."Warninglevel (notError— this is a client error, not a server fault)Response shape (413)
Build succeeds. GlobalExceptionMiddleware tests pass (2/2).
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 85.2%
DoujinManager.Infrastructure - 93%
pshot
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
DoujinManager.RestAdapter - 85.2%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 21.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ A small, surgical fix! Just two files, one attribute, one new catch block. This is exactly the kind of tidy little patch I love to see — clear problem statement, clear solution, no scope creep. The 413-instead-of-500 behavior is the correct HTTP semantic (a client error should never masquerade as a server fault!), and returning the actual byte limit in the problem detail is chef's kiss for client developers. ♡ Let me peek under the hood though, fufu~
Verdict: ✅ Looks good to me~
No security issues, no blocking logic errors. Static scan came back spotless (no secrets, no injection, no unsafe patterns). The
RequestSizeLimitAttribute(100_000_000)correctly targets the two upload endpoints that actually need it (batch images + ZIP), rather than globally relaxing Kestrel's limit — good defense-in-depth instinct. And mappingBadHttpRequestExceptionto 413 withapplication/problem+json+ RFC-7807ErrorResponsekeeps the envelope contract consistent with your existing 500 path. ♪💡 Little ideas (non-blocking)~
[
GlobalExceptionMiddleware.cs— new 413 catch block] — The new branch readsIHttpMaxRequestBodySizeFeature'sMaxRequestBodySize, which is the configured limit (here 100 MB). That's correct and helpful! But CI coverage onGlobalExceptionMiddlewaresits at 50% line / 37.5% branch — meaning this shiny new 413 path isn't exercised by a test. The author's PR body says "GlobalExceptionMiddleware tests pass (2/2)" — those 2 tests cover the existing 500 path, not the new 413 branch. It would be lovely to add one test that posts an oversized body (or throwsBadHttpRequestExceptionwith a "Request body too large" message through the middleware) and asserts the 413 + problem detail + actual byte count. Not blocking — the logic is simple and correct — but right now a future refactor could silently break this and nothing would catch it. ♡[
GlobalExceptionMiddleware.cs—StringComparison.OrdinalIgnoreCasematch] — Clever usingContains("Request body too large", OrdinalIgnoreCase)as the discriminator rather than checking the exception's status code. Kestrel's message wording is stable across versions, but if ASP.NET Core ever rephrases it, this catch would fall through to the generic 500 handler and the 413 regression would be silent. A more robust (but heavier) alternative isex.StatusCode == 413—BadHttpRequestExceptionexposes it since .NET 8. Totally optional — the current approach is readable and fine for a single-deployment app. Just something to keep in your wings~ ♪Automated review by Jibril · 2026-07-01
CI/CD: passed for head
e1cabf8(forgejo-actions coverage #654, line 85.2%, branch 50.4%) · Local checks: skipped per CI-evidence policyStatic scan: clean (no secrets, shell injection, eval/exec, pickle, or SQL injection in added lines)
I don't like magic number here. How about you extract this into a configuration file, potentially IOptions, and allow the server docker config to set the upper limit. Large chapters might go up to 500MB potentially. The exception should then use the value for the error. Of course that also requires updated deployment doco