From 0e9cd72c5d118728d3ac975797bed2cf44e9571c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 19:54:43 +0000 Subject: [PATCH 1/4] Accept MKV, and say why a file was refused instead of failing silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An .mkv upload had two ways to end badly. Small ones were refused for an extension nothing claimed, with "Unsupported file type" and no hint at what would have worked. Large ones never got that far: the whole body is sent before anything looks at it, so a clip over the request-body limit had its connection cut mid-send and reached the uploader as a bare "network error" — the same message a dropped connection gives, with nothing to suggest the size was the problem. Converting the clip to a smaller MP4 made both go away, which points at neither. Matroska needed nothing but the extension: it shares WebM's container, so the header check already recognised it and ffmpeg already takes frames from it. The refusals now happen in the browser, before a byte is sent. The handlers own the extension list and their own ceilings, so the page can hand the browser what the server takes and both stay in step; a file that doesn't fit is turned away as it's picked, named and measured against the limit it broke. The upload card says up front what does fit, the server repeats the same wording for anything that reaches it, and the XHR error case no longer pretends a severed upload was only ever the network. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RYoVr4rkVdeQn5THttcGRM --- README.md | 10 ++- src/Shoebox.Web/Pages/Pool/Gallery.cshtml | 6 +- src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs | 11 ++- src/Shoebox.Web/Services/IMediaHandler.cs | 63 +++++++++++++- src/Shoebox.Web/Services/MediaService.cs | 7 +- src/Shoebox.Web/Services/PhotoHandler.cs | 4 +- src/Shoebox.Web/Services/VideoHandler.cs | 8 +- src/Shoebox.Web/wwwroot/css/site.css | 15 ++++ src/Shoebox.Web/wwwroot/js/gallery.js | 57 ++++++++++++- tests/Shoebox.Tests.Web/CoreFlowTests.cs | 84 +++++++++++++++++++ .../ShoeboxWebApplicationFactory.cs | 13 ++- 11 files changed, 263 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 8437b11..dfe16b9 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ For everyone else: - **Fast gallery**: a WebP thumbnail grid plus a full-screen lightbox backed by a downscaled web-safe proxy, so viewing is sharp without sending a full-size original over the wire. Filter by uploader; photos sort by capture time (EXIF). - **HEIC / HEIF from phones**: decoded server-side, so iPhone photos get thumbnails and previews in every browser, not just Safari. - **GIFs that move**: an animated GIF (or animated WebP) plays in the lightbox and when you hover its tile. The grid itself holds still, so a box full of GIFs doesn't flicker at everyone at once. -- **Short videos, minimally**: MP4/MOV/WebM clips can be dropped in alongside the photos. Each gets a poster frame so it has a tile in the grid, and downloads as the original file. Nothing is transcoded and there is no in-browser playback. +- **Short videos, minimally**: MP4/MOV/MKV/WebM clips can be dropped in alongside the photos. Each gets a poster frame so it has a tile in the grid, and downloads as the original file. Nothing is transcoded and there is no in-browser playback. - **Flexible downloads**: a single photo, the whole box as a streamed ZIP, or "download others'": everything except your own uploads. - **Private admin link**: the creator can rename the box, change or remove the password, adjust expiry, delete individual photos, or delete the whole box. - **Auto-expiry**: a box can be set to delete itself a chosen number of days after the event. @@ -168,6 +168,13 @@ appear in the gallery everywhere. The original file is always stored unmodified Download button returns. Files of the wrong type, over `MaxFileSizeMb`, or that don't decode as a real image within the pixel limits are rejected at upload. +The browser checks the extension and the size against these limits before it sends anything, +so a file that wouldn't be accepted is refused in the moment it's picked, naming what the box +does take. That check matters most for the size: a file past the request-body limit has its +connection cut part-way through, which the browser can only report as a generic network +error, with nothing to say the size was the problem. The server re-checks everything +regardless — the browser-side check is there to save the wait, not to enforce anything. + ### Animations An animated GIF (or animated WebP) keeps its animation: the display proxy is written as an @@ -194,6 +201,7 @@ same box as the photos, and no more: | MP4 | `.mp4`, `.m4v` | | QuickTime | `.mov` | | WebM | `.webm` | +| Matroska | `.mkv` | A clip is stored untouched, appears in the grid as a still frame with a **Video** badge, and is included in ZIP downloads like anything else. There is **no in-browser playback and no diff --git a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml index e79ccb9..460d8c5 100644 --- a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml +++ b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml @@ -53,10 +53,14 @@
- + @* accept only steers the file picker; the drop zone ignores it, so gallery.js + checks every file against data-limits before it sends a byte of it. *@ +

or drop photos anywhere on this page

+

Takes @Model.UploadPolicy.Summary.

diff --git a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs index bc24ac7..3ea6f24 100644 --- a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs +++ b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using Shoebox.Web.Data; using Shoebox.Web.Services; using Microsoft.AspNetCore.Mvc; @@ -16,7 +17,8 @@ public class GalleryModel( PoolService pools, PoolAccessService access, UploaderIdentity identity, - ShareLinkService links) : PageModel + ShareLinkService links, + MediaHandlers handlers) : PageModel { public Data.Pool Pool { get; set; } = null!; public List Items { get; set; } = []; @@ -32,6 +34,13 @@ public class GalleryModel( // "everyone else's" download only offers itself when it would actually return files. public bool HasOthers { get; set; } + // What the server takes, handed to the page so the browser can turn a file away before + // sending it. A 400 MB clip that gets its connection cut for exceeding the request-body + // limit reaches the uploader as "network error" and nothing more. + public UploadPolicy UploadPolicy => handlers.Policy; + + public string UploadLimitsJson => JsonSerializer.Serialize(UploadPolicy.MaxBytesByExtension); + public async Task OnGetAsync(string code) { var pool = await pools.FindByCodeAsync(code); diff --git a/src/Shoebox.Web/Services/IMediaHandler.cs b/src/Shoebox.Web/Services/IMediaHandler.cs index 7339703..3431412 100644 --- a/src/Shoebox.Web/Services/IMediaHandler.cs +++ b/src/Shoebox.Web/Services/IMediaHandler.cs @@ -11,10 +11,14 @@ public interface IMediaHandler { MediaKind Kind { get; } + /// What to call this kind when telling someone their file didn't fit. + string Label { get; } + /// - /// The content type to store for this extension, or null when this handler doesn't take it. + /// Every extension this handler takes, mapped to the content type to store for it. Looked + /// up case-insensitively, so implementations build it with an ordinal-ignore-case comparer. /// - string? ContentTypeFor(string extension); + IReadOnlyDictionary ContentTypes { get; } /// Per-file upload ceiling for this kind. long MaxBytes { get; } @@ -40,6 +44,29 @@ public interface IMediaHandler string? RenderFailureReason { get; } } +/// +/// What the browser needs to turn a file away before sending it, plus the words to use when +/// something doesn't fit. A video can be hundreds of megabytes, and a file the server won't +/// take is worth saying no to in the moment it's picked — not after a long upload, and +/// certainly not by having the connection cut for exceeding the request-body limit, which +/// reaches the user as nothing more useful than "network error". +/// +/// Accepted extensions (lowercase, dotted) and their ceilings. +/// Value for the file input's accept attribute. +/// Plain-language "what fits", e.g. for the upload card and rejections. +public record UploadPolicy( + IReadOnlyDictionary MaxBytesByExtension, + string Accept, + string Summary) +{ + /// The reason to give for a file whose extension nothing here takes. + public string RejectionFor(string extension) => + $"Can't take {(string.IsNullOrWhiteSpace(extension) ? "files with no extension" : extension.ToLowerInvariant() + " files")} — {Summary}"; + + /// Sizes are only ever shown to people, so one decimal of MB is plenty. + public static string DescribeSize(long bytes) => $"{bytes / (1024.0 * 1024.0):0.#} MB"; +} + /// Finds the handler for an upload, by file extension or by stored kind. public class MediaHandlers(IEnumerable handlers) { @@ -53,7 +80,7 @@ public class MediaHandlers(IEnumerable handlers) { foreach (var handler in all) { - if (handler.ContentTypeFor(extension) is { } contentType) + if (handler.ContentTypes.GetValueOrDefault(extension) is { } contentType) { return (handler, contentType); } @@ -63,4 +90,34 @@ public class MediaHandlers(IEnumerable handlers) } public IMediaHandler For(MediaKind kind) => all.First(h => h.Kind == kind); + + /// + /// What this build accepts, assembled from the handlers so the browser, the upload card and + /// the rejection messages can never drift from what the server actually stores. + /// + public UploadPolicy Policy => new( + all.SelectMany(h => h.ContentTypes.Keys.Select(e => (Extension: e.ToLowerInvariant(), h.MaxBytes))) + .ToDictionary(x => x.Extension, x => x.MaxBytes, StringComparer.OrdinalIgnoreCase), + BuildAccept(), + string.Join(", ", all.Select(h => + $"{h.Label}s ({string.Join(", ", h.ContentTypes.Keys.Select(e => e.TrimStart('.')))}) " + + $"up to {UploadPolicy.DescribeSize(h.MaxBytes)}"))); + + /// + /// The wildcards (image/*, video/*) keep phone pickers showing the camera roll; the explicit + /// extensions cover the formats a picker may not map to one (HEIC and MKV in particular). + /// Nothing here is a security control — it only steers the picker, and the drop zone ignores + /// it entirely — so the real check is on the way in. + /// + private string BuildAccept() + { + var wildcards = all + .SelectMany(h => h.ContentTypes.Values) + .Select(type => type[..(type.IndexOf('/') + 1)] + "*") + .Distinct(StringComparer.OrdinalIgnoreCase); + var extensions = all + .SelectMany(h => h.ContentTypes.Keys.Select(e => e.ToLowerInvariant())) + .Distinct(StringComparer.Ordinal); + return string.Join(",", wildcards.Concat(extensions)); + } } diff --git a/src/Shoebox.Web/Services/MediaService.cs b/src/Shoebox.Web/Services/MediaService.cs index efe0e1c..0d1fb73 100644 --- a/src/Shoebox.Web/Services/MediaService.cs +++ b/src/Shoebox.Web/Services/MediaService.cs @@ -22,7 +22,8 @@ public async Task SaveAsync(Pool pool, IFormFile file, string uplo var match = handlers.For(extension); if (match is null) { - return UploadResult.Rejected(fileName, "Unsupported file type"); + // Say what does fit: "unsupported" on its own leaves the uploader guessing. + return UploadResult.Rejected(fileName, handlers.Policy.RejectionFor(extension)); } var (handler, contentType) = match.Value; @@ -34,7 +35,9 @@ public async Task SaveAsync(Pool pool, IFormFile file, string uplo if (file.Length > handler.MaxBytes) { - return UploadResult.Rejected(fileName, $"Larger than {handler.MaxBytes / (1024 * 1024)} MB"); + return UploadResult.Rejected(fileName, + $"Too big ({UploadPolicy.DescribeSize(file.Length)}) — {handler.Label}s can be up to " + + UploadPolicy.DescribeSize(handler.MaxBytes)); } Directory.CreateDirectory(paths.OriginalsDirectory(pool.Id)); diff --git a/src/Shoebox.Web/Services/PhotoHandler.cs b/src/Shoebox.Web/Services/PhotoHandler.cs index 54c1c51..825abe9 100644 --- a/src/Shoebox.Web/Services/PhotoHandler.cs +++ b/src/Shoebox.Web/Services/PhotoHandler.cs @@ -9,7 +9,7 @@ namespace Shoebox.Web.Services; /// public class PhotoHandler(ImageRenderer renderer, IOptions options) : IMediaHandler { - private static readonly Dictionary ContentTypes = new(StringComparer.OrdinalIgnoreCase) + public IReadOnlyDictionary ContentTypes { get; } = new Dictionary(StringComparer.OrdinalIgnoreCase) { [".jpg"] = "image/jpeg", [".jpeg"] = "image/jpeg", @@ -22,7 +22,7 @@ public class PhotoHandler(ImageRenderer renderer, IOptions optio public MediaKind Kind => MediaKind.Photo; - public string? ContentTypeFor(string extension) => ContentTypes.GetValueOrDefault(extension); + public string Label => "photo"; public long MaxBytes => options.Value.MaxFileSizeBytes; diff --git a/src/Shoebox.Web/Services/VideoHandler.cs b/src/Shoebox.Web/Services/VideoHandler.cs index 7e3acc1..38033cc 100644 --- a/src/Shoebox.Web/Services/VideoHandler.cs +++ b/src/Shoebox.Web/Services/VideoHandler.cs @@ -10,17 +10,21 @@ namespace Shoebox.Web.Services; /// public class VideoHandler(VideoRenderer renderer, IOptions options) : IMediaHandler { - private static readonly Dictionary ContentTypes = new(StringComparer.OrdinalIgnoreCase) + public IReadOnlyDictionary ContentTypes { get; } = new Dictionary(StringComparer.OrdinalIgnoreCase) { [".mp4"] = "video/mp4", [".m4v"] = "video/mp4", [".mov"] = "video/quicktime", [".webm"] = "video/webm", + // Matroska shares WebM's container, so the header check and ffmpeg already handle it; + // only the extension was missing, and a phone-sized .mkv would hit the request-body + // limit and die as a bare "network error" long before anything said it wasn't wanted. + [".mkv"] = "video/x-matroska", }; public MediaKind Kind => MediaKind.Video; - public string? ContentTypeFor(string extension) => ContentTypes.GetValueOrDefault(extension); + public string Label => "video"; public long MaxBytes => options.Value.MaxVideoFileSizeBytes; diff --git a/src/Shoebox.Web/wwwroot/css/site.css b/src/Shoebox.Web/wwwroot/css/site.css index 3eee90e..19c0c36 100644 --- a/src/Shoebox.Web/wwwroot/css/site.css +++ b/src/Shoebox.Web/wwwroot/css/site.css @@ -514,6 +514,13 @@ input[type="checkbox"] { accent-color: var(--ink); } color: var(--muted); } +/* What fits, said up front, so nobody finds out by watching a long upload fail. */ +.upload-formats { + margin: 0.25rem 0 0; + font-size: 0.8rem; + color: var(--muted); +} + .upload-progress { list-style: none; margin: 0.6rem 0 0; padding: 0; font-size: 0.85rem; } .upload-progress li { display: flex; @@ -522,6 +529,14 @@ input[type="checkbox"] { accent-color: var(--ink); } padding: 0.3rem 0.1rem; border-top: 1px dashed var(--line); } +/* A rejection needs room to say why, so the file name is what gives way. */ +.upload-progress li > span:first-child { + min-width: 4rem; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} +.upload-progress .status { text-align: right; } .upload-progress .ok { color: var(--ok); } .upload-progress .fail { color: var(--danger); } diff --git a/src/Shoebox.Web/wwwroot/js/gallery.js b/src/Shoebox.Web/wwwroot/js/gallery.js index 3e2a5a4..41d526e 100644 --- a/src/Shoebox.Web/wwwroot/js/gallery.js +++ b/src/Shoebox.Web/wwwroot/js/gallery.js @@ -12,6 +12,46 @@ // ---------- Upload ---------- + // What the server will take, rendered into the file input by the page: extension -> byte + // ceiling. The browser checks against it first, because everything past this point is + // expensive to get wrong — a file over the request-body limit has its connection cut + // mid-send and surfaces as a bare "network error", with nothing to tell the uploader that + // the size was the problem. + const limits = parseLimits(fileInput.dataset.limits); + const limitsSummary = fileInput.dataset.limitsSummary || ""; + + function parseLimits(json) { + try { + return JSON.parse(json || "{}"); + } catch { + // Without limits the pre-flight check simply stands down and the server decides. + return {}; + } + } + + // The reason this file can't be sent, or null when it's worth trying. Wording matches the + // server's own rejections, since either can be what lands in the progress list. + function whyNotUploadable(file) { + const dot = file.name.lastIndexOf("."); + const extension = dot > 0 ? file.name.slice(dot).toLowerCase() : ""; + if (!Object.keys(limits).length) return null; + + const max = limits[extension]; + if (max === undefined) { + const what = extension ? extension + " files" : "files with no extension"; + return `Can't take ${what} — ${limitsSummary}`; + } + if (file.size === 0) return "Empty file"; + if (file.size > max) { + return `Too big (${describeSize(file.size)}) — up to ${describeSize(max)}`; + } + return null; + } + + function describeSize(bytes) { + return (bytes / (1024 * 1024)).toFixed(1).replace(/\.0$/, "") + " MB"; + } + pickBtn.addEventListener("click", () => { if (!requireName()) return; fileInput.click(); @@ -63,6 +103,13 @@ progressList.appendChild(item); const status = item.querySelector(".status"); + const problem = whyNotUploadable(file); + if (problem) { + status.textContent = problem; + status.className = "status fail"; + continue; + } + try { const result = await uploadOne(file, (pct) => (status.textContent = pct + "%")); const r = result.results && result.results[0]; @@ -105,14 +152,20 @@ } else if (xhr.status === 401) { reject(new Error("box is locked; refresh the page")); } else if (xhr.status === 413) { - reject(new Error("file too large")); + reject(new Error(`file too large — ${limitsSummary || "try a smaller one"}`)); } else { let msg = "upload failed"; try { msg = JSON.parse(xhr.responseText).error || msg; } catch { /* keep default */ } reject(new Error(msg)); } }); - xhr.addEventListener("error", () => reject(new Error("network error"))); + // Anything the browser can't attribute to a response lands here, including a body the + // server cut off for being too large — hence the size hint alongside the connection one. + xhr.addEventListener("error", () => + reject(new Error("upload was cut off — connection lost, or too big for this server")) + ); + xhr.addEventListener("abort", () => reject(new Error("upload cancelled"))); + xhr.addEventListener("timeout", () => reject(new Error("upload timed out"))); xhr.send(form); }); } diff --git a/tests/Shoebox.Tests.Web/CoreFlowTests.cs b/tests/Shoebox.Tests.Web/CoreFlowTests.cs index 73d6dcd..dac4c39 100644 --- a/tests/Shoebox.Tests.Web/CoreFlowTests.cs +++ b/tests/Shoebox.Tests.Web/CoreFlowTests.cs @@ -22,6 +22,14 @@ public class CoreFlowTests (byte)'i', (byte)'s', (byte)'o', (byte)'m', (byte)'m', (byte)'p', (byte)'4', (byte)'2', ]; + // An EBML header, which is what a Matroska (.mkv) or WebM file starts with. Enough to clear + // the container check without needing a real clip, as with Mp4Header above. + private static readonly byte[] MatroskaHeader = + [ + 0x1A, 0x45, 0xDF, 0xA3, 0x01, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x1F, 0x42, 0x86, 0x81, 0x01, + ]; + [Fact] public async Task Animated_gif_gets_a_moving_proxy_and_a_still_thumbnail() { @@ -191,6 +199,82 @@ public async Task Real_clip_gets_a_poster_frame() return null; } + [Fact] + public async Task Matroska_clip_is_accepted_like_any_other_video() + { + using var factory = new ShoeboxWebApplicationFactory(); + using var owner = CreateClient(factory); + var code = await CreateBoxAsync(owner); + + var added = await UploadAsync(owner, code, "Alice", "clip.mkv", MatroskaHeader); + Assert.Equal("added", added.Status); + var mediaId = Assert.IsType(added.MediaId); + + var original = await owner.GetAsync($"/api/media/{mediaId}/original"); + Assert.Equal(HttpStatusCode.OK, original.StatusCode); + Assert.Equal("video/x-matroska", original.Content.Headers.ContentType?.MediaType); + Assert.Equal(MatroskaHeader, await original.Content.ReadAsByteArrayAsync()); + + var gallery = await owner.GetAsync($"/p/{code}"); + Assert.Contains("media-badge\">\u25B6 Video", await gallery.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task Rejected_file_type_says_what_the_box_does_take() + { + using var factory = new ShoeboxWebApplicationFactory(); + using var owner = CreateClient(factory); + var code = await CreateBoxAsync(owner); + + var result = await UploadAsync(owner, code, "Alice", "notes.txt", [1, 2, 3, 4]); + + Assert.Equal("rejected", result.Status); + Assert.Null(result.MediaId); + // "Unsupported" alone leaves the uploader guessing; the accepted formats and their + // ceilings have to be in the message itself. + Assert.Contains(".txt", result.Reason); + Assert.Contains("mkv", result.Reason); + Assert.Contains("jpg", result.Reason); + Assert.Contains("200 MB", result.Reason); + } + + [Fact] + public async Task Oversized_video_is_rejected_with_its_size_and_the_limit() + { + using var factory = new ShoeboxWebApplicationFactory(new Dictionary + { + ["Shoebox:MaxVideoFileSizeMb"] = "1", + }); + using var owner = CreateClient(factory); + var code = await CreateBoxAsync(owner); + + var oversized = new byte[3 * 1024 * 1024]; + MatroskaHeader.CopyTo(oversized, 0); + var result = await UploadAsync(owner, code, "Alice", "long.mkv", oversized); + + Assert.Equal("rejected", result.Status); + Assert.Null(result.MediaId); + Assert.Contains("Too big (3 MB)", result.Reason); + Assert.Contains("videos can be up to 1 MB", result.Reason); + } + + [Fact] + public async Task Gallery_page_tells_the_browser_what_it_may_send() + { + using var factory = new ShoeboxWebApplicationFactory(); + using var owner = CreateClient(factory); + var code = await CreateBoxAsync(owner); + + var html = await (await owner.GetAsync($"/p/{code}")).Content.ReadAsStringAsync(); + + // The pre-flight check in gallery.js reads these; without them a file the server + // would refuse is uploaded in full first, and an oversized one dies as "network error". + Assert.Contains("data-limits=", html); + Assert.Contains("".mkv":209715200", html); + Assert.Contains("".jpg":52428800", html); + Assert.Contains("accept=\"image/*,video/*,", html); + } + [Fact] public async Task File_that_is_not_really_a_video_is_rejected() { diff --git a/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs b/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs index 6e94097..6d4ecda 100644 --- a/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs +++ b/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs @@ -6,8 +6,15 @@ namespace Shoebox.Tests.Web; public sealed class ShoeboxWebApplicationFactory : WebApplicationFactory { - public ShoeboxWebApplicationFactory() + private readonly IReadOnlyDictionary settings; + + /// + /// Configuration overrides for this instance, e.g. a small size ceiling so the oversized + /// path can be exercised without a real 200 MB file. + /// + public ShoeboxWebApplicationFactory(IReadOnlyDictionary? settings = null) { + this.settings = settings ?? new Dictionary(); DataPath = Path.Combine( Path.GetTempPath(), $"shoebox-tests-{Guid.NewGuid():N}"); @@ -21,6 +28,10 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) builder.UseEnvironment("Testing"); builder.UseSetting("Shoebox:DataPath", DataPath); builder.UseSetting("Shoebox:CookieLifetimeDays", "1"); + foreach (var (key, value) in settings) + { + builder.UseSetting(key, value); + } } protected override void Dispose(bool disposing) From 6517f46951149f184f1134ab6d95159fc0c3669f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 20:05:51 +0000 Subject: [PATCH 2/4] Ask the server why an upload failed instead of reporting "network error" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correcting the previous commit's account of the cause: the size limit is not what refused a 150 MB clip, and an unsupported extension does not produce a network error by itself. Uploading a 150 MB .mkv to the old code returns a clean JSON rejection, 200 and all. What does produce it is any failure the browser can't attribute to a response. Reproduced two: a body past the request-body limit dies after ~4 MB of a 250 MB send with a 500 and a severed connection, and a box that is gone or locked answers before the body is read at all, cutting off an upload already in flight. All three of those — plus a proxy hanging up on a body it thinks too big, and a genuinely dropped connection — reach the page as one bare error event carrying nothing to act on, and the page was turning every one of them into the same three words. So it stops guessing and asks. A failed upload is followed by a request small enough to answer when a large one just died, and what comes back separates box gone from box locked from box fine — the last meaning it was that upload in particular the server would not take. The same probe now covers a response that isn't ours, which an error page from a proxy would be. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RYoVr4rkVdeQn5THttcGRM --- README.md | 14 ++++++--- src/Shoebox.Web/Api/MediaEndpoints.cs | 21 +++++++++++++ src/Shoebox.Web/wwwroot/js/gallery.js | 39 +++++++++++++++++++----- tests/Shoebox.Tests.Web/CoreFlowTests.cs | 19 ++++++++++++ 4 files changed, 82 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index dfe16b9..2d8fce9 100644 --- a/README.md +++ b/README.md @@ -170,10 +170,16 @@ a real image within the pixel limits are rejected at upload. The browser checks the extension and the size against these limits before it sends anything, so a file that wouldn't be accepted is refused in the moment it's picked, naming what the box -does take. That check matters most for the size: a file past the request-body limit has its -connection cut part-way through, which the browser can only report as a generic network -error, with nothing to say the size was the problem. The server re-checks everything -regardless — the browser-side check is there to save the wait, not to enforce anything. +does take. The server re-checks everything regardless — the browser-side check is there to +save the wait, not to enforce anything. + +An upload can also fail without the server ever answering: a request that exceeds the +body limit (the app's own, or a reverse proxy's `client_max_body_size`) is cut off part-way +through, and a box that locked itself again answers before the body is read. The browser +reports every one of these as the same bare error event, with nothing in it to act on, so the +page follows a failed upload with a small request to `GET /api/p/{code}/status` and reports +what that says instead: box gone, box locked, or box fine — in which case it was that +particular upload the server would not take. ### Animations diff --git a/src/Shoebox.Web/Api/MediaEndpoints.cs b/src/Shoebox.Web/Api/MediaEndpoints.cs index f1c9012..7dd56de 100644 --- a/src/Shoebox.Web/Api/MediaEndpoints.cs +++ b/src/Shoebox.Web/Api/MediaEndpoints.cs @@ -13,6 +13,7 @@ public static void MapMediaApi(this IEndpointRouteBuilder app) var api = app.MapGroup("/api"); api.MapPost("/p/{code}/media", UploadAsync).DisableAntiforgery(); + api.MapGet("/p/{code}/status", PoolStatusAsync); api.MapGet("/media/{id:guid}/thumb", ServeThumbAsync); api.MapGet("/media/{id:guid}/display", ServeDisplayAsync); api.MapGet("/media/{id:guid}/original", ServeOriginalAsync); @@ -72,6 +73,26 @@ private static async Task UploadAsync( return Results.Ok(new { results }); } + /// + /// Whether this box is still there and still open to the caller. Small and cheap on + /// purpose: an upload that dies without a response leaves the browser able to report + /// nothing but "network error", and this is what the page asks afterwards to turn that + /// into something the uploader can act on. + /// + private static async Task PoolStatusAsync( + string code, HttpContext context, PoolService pools, PoolAccessService access) + { + var pool = await pools.FindByCodeAsync(code); + if (pool is null) + { + return Results.NotFound(); + } + + return access.CanView(context, pool) + ? Results.Ok(new { open = true }) + : Results.Unauthorized(); + } + private static async Task ServeThumbAsync( Guid id, HttpContext context, AppDbContext db, PoolAccessService access, StoragePaths paths) { diff --git a/src/Shoebox.Web/wwwroot/js/gallery.js b/src/Shoebox.Web/wwwroot/js/gallery.js index 41d526e..bec6161 100644 --- a/src/Shoebox.Web/wwwroot/js/gallery.js +++ b/src/Shoebox.Web/wwwroot/js/gallery.js @@ -134,6 +134,22 @@ } } + // Turns a failure the browser couldn't explain into one the uploader can act on, by asking + // the server a question small enough to answer even when a large upload just died. + function failWithDiagnosis(reject, fallback) { + fetch(`/api/p/${poolCode}/status`, { cache: "no-store" }) + .then((res) => { + if (res.status === 401) return "the box locked again — refresh the page to re-enter the password"; + if (res.status === 404) return "this box is gone — it may have expired"; + // The box is fine and reachable, so it was this particular upload that was refused. + if (res.ok) return `${fallback} — it may be larger than the server accepts`; + return fallback; + }) + // The small request failed too, so the connection really is the problem. + .catch(() => "lost the connection to the server") + .then((reason) => reject(new Error(reason))); + } + function uploadOne(file, onProgress) { // XHR instead of fetch: fetch has no upload progress events. return new Promise((resolve, reject) => { @@ -148,21 +164,30 @@ }); xhr.addEventListener("load", () => { if (xhr.status >= 200 && xhr.status < 300) { - resolve(JSON.parse(xhr.responseText)); + try { + resolve(JSON.parse(xhr.responseText)); + } catch { + // A 200 that isn't ours — a proxy's interstitial, most likely. + failWithDiagnosis(reject, "the server sent back something unexpected"); + } } else if (xhr.status === 401) { reject(new Error("box is locked; refresh the page")); } else if (xhr.status === 413) { reject(new Error(`file too large — ${limitsSummary || "try a smaller one"}`)); } else { - let msg = "upload failed"; - try { msg = JSON.parse(xhr.responseText).error || msg; } catch { /* keep default */ } - reject(new Error(msg)); + // An error page rather than our JSON means whatever went wrong went wrong before + // the upload was ever looked at, so the server is worth asking about. + let msg = null; + try { msg = JSON.parse(xhr.responseText).error; } catch { /* not ours */ } + if (msg) reject(new Error(msg)); + else failWithDiagnosis(reject, `the server refused this upload (error ${xhr.status})`); } }); - // Anything the browser can't attribute to a response lands here, including a body the - // server cut off for being too large — hence the size hint alongside the connection one. + // A request that dies without a response lands here, and the event says nothing about + // why: a dropped connection, a box that locked itself again, and a proxy hanging up on + // a body it thought too big are one and the same to the browser. Don't guess — ask. xhr.addEventListener("error", () => - reject(new Error("upload was cut off — connection lost, or too big for this server")) + failWithDiagnosis(reject, "the upload was cut off before the server answered") ); xhr.addEventListener("abort", () => reject(new Error("upload cancelled"))); xhr.addEventListener("timeout", () => reject(new Error("upload timed out"))); diff --git a/tests/Shoebox.Tests.Web/CoreFlowTests.cs b/tests/Shoebox.Tests.Web/CoreFlowTests.cs index dac4c39..f3db2d2 100644 --- a/tests/Shoebox.Tests.Web/CoreFlowTests.cs +++ b/tests/Shoebox.Tests.Web/CoreFlowTests.cs @@ -275,6 +275,25 @@ public async Task Gallery_page_tells_the_browser_what_it_may_send() Assert.Contains("accept=\"image/*,video/*,", html); } + [Fact] + public async Task Pool_status_says_whether_the_box_is_reachable_and_open() + { + using var factory = new ShoeboxWebApplicationFactory(); + using var owner = CreateClient(factory); + var code = await CreateBoxAsync(owner, password: "festival-secret"); + + // This is what the page asks after an upload dies without a response, so it has to + // separate the three things the browser's error event cannot: box gone, box locked, + // and box fine (so it was that upload the server refused). + var open = await owner.GetAsync($"/api/p/{code}/status"); + Assert.Equal(HttpStatusCode.OK, open.StatusCode); + Assert.True((await open.Content.ReadFromJsonAsync()).GetProperty("open").GetBoolean()); + + using var guest = CreateClient(factory); + Assert.Equal(HttpStatusCode.Unauthorized, (await guest.GetAsync($"/api/p/{code}/status")).StatusCode); + Assert.Equal(HttpStatusCode.NotFound, (await owner.GetAsync("/api/p/NOSUCHBX/status")).StatusCode); + } + [Fact] public async Task File_that_is_not_really_a_video_is_rejected() { From 5936c12586967d2cff9cfc14c1280db7b3fb6613 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 01:24:35 +0000 Subject: [PATCH 3/4] Put the reason in the upload response; stop the page inventing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page was filling in its own explanation whenever the response didn't hand it one, and the explanation it reached for was the connection. That is how an unsupported format arrives as "network error": nothing on the wire was wrong, the client just had no reason to show and picked the wrong one. So the reason comes from the server now, on every path that can fail. A box that expired, a box that locked itself again, a malformed request and a body past the request limit all answer in the same shape as a success, with the reason in the body. The last of those never answered at all before — Kestrel throws on an over-limit body and the exception handler returns an HTML error page, so a 500 with no mention of size was all a 250 MB upload ever got back. Catching it turns that into a 413 that names the ceiling it broke. The page shows what it was told and nothing else. A status code with no body is reported as that status code, a reply that isn't ours is reported as one, and a request that got no reply says only that. No cause is asserted that wasn't given, which also removes the follow-up request added a commit ago: asking a second question is not how the first one should have been answered. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RYoVr4rkVdeQn5THttcGRM --- README.md | 13 +++-- src/Shoebox.Web/Api/MediaEndpoints.cs | 58 +++++++++++---------- src/Shoebox.Web/wwwroot/js/gallery.js | 64 +++++++++-------------- tests/Shoebox.Tests.Web/CoreFlowTests.cs | 66 ++++++++++++++++++------ 4 files changed, 113 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index 2d8fce9..2f6a968 100644 --- a/README.md +++ b/README.md @@ -173,13 +173,12 @@ so a file that wouldn't be accepted is refused in the moment it's picked, naming does take. The server re-checks everything regardless — the browser-side check is there to save the wait, not to enforce anything. -An upload can also fail without the server ever answering: a request that exceeds the -body limit (the app's own, or a reverse proxy's `client_max_body_size`) is cut off part-way -through, and a box that locked itself again answers before the body is read. The browser -reports every one of these as the same bare error event, with nothing in it to act on, so the -page follows a failed upload with a small request to `GET /api/p/{code}/status` and reports -what that says instead: box gone, box locked, or box fine — in which case it was that -particular upload the server would not take. +Every way an upload can fail answers with the reason in the body, in the same shape as a +success — a box that expired, a box that locked itself again, a body past the request limit +(which otherwise surfaces as an unhandled exception and an HTML error page saying nothing +about size). The page shows what the server said and never fills in a cause of its own: a +client guessing at a bare status code or a dead connection reliably blames the network for +something the server already knew and named. ### Animations diff --git a/src/Shoebox.Web/Api/MediaEndpoints.cs b/src/Shoebox.Web/Api/MediaEndpoints.cs index 7dd56de..8e2952f 100644 --- a/src/Shoebox.Web/Api/MediaEndpoints.cs +++ b/src/Shoebox.Web/Api/MediaEndpoints.cs @@ -13,7 +13,6 @@ public static void MapMediaApi(this IEndpointRouteBuilder app) var api = app.MapGroup("/api"); api.MapPost("/p/{code}/media", UploadAsync).DisableAntiforgery(); - api.MapGet("/p/{code}/status", PoolStatusAsync); api.MapGet("/media/{id:guid}/thumb", ServeThumbAsync); api.MapGet("/media/{id:guid}/display", ServeDisplayAsync); api.MapGet("/media/{id:guid}/original", ServeOriginalAsync); @@ -24,41 +23,64 @@ public static void MapMediaApi(this IEndpointRouteBuilder app) api.MapGet("/p/{code}/qr", QrCodeAsync); } + /// + /// Takes one upload. Every way this can fail answers with the reason in the body, in the + /// same shape as a success: whatever the page shows the uploader has to come from here, + /// because a client left to fill in the blank can only guess, and a guess reads as a + /// network problem when the file was simply not one this box takes. + /// private static async Task UploadAsync( string code, HttpRequest request, AppDbContext db, PoolService pools, MediaService media, + MediaHandlers handlers, PoolAccessService access, UploaderIdentity identity) { var pool = await pools.FindByCodeAsync(code); if (pool is null) { - return Results.NotFound(); + return UploadFailed(StatusCodes.Status404NotFound, + "This box no longer exists — it may have expired."); } if (!access.CanView(request.HttpContext, pool)) { - return Results.Unauthorized(); + return UploadFailed(StatusCodes.Status401Unauthorized, + "This box is locked. Refresh the page and enter the password again."); } if (!request.HasFormContentType) { - return Results.BadRequest(new { error = "Expected multipart form data." }); + return UploadFailed(StatusCodes.Status400BadRequest, "Expected multipart form data."); + } + + IFormCollection form; + try + { + form = await request.ReadFormAsync(); + } + catch (Exception ex) when (ex is BadHttpRequestException or InvalidDataException) + { + // The body ran past the request limit, so it was never read and nothing below has + // seen the file. Left alone this surfaces as an unhandled exception and an HTML + // error page, which says nothing at all about size. + return UploadFailed(StatusCodes.Status413PayloadTooLarge, + $"That file is larger than this server accepts — {handlers.Policy.Summary}."); } - var form = await request.ReadFormAsync(); var uploaderName = form["uploaderName"].ToString().Trim(); if (uploaderName.Length is 0 or > 80) { - return Results.BadRequest(new { error = "Please tell us who you are (1-80 characters)." }); + return UploadFailed(StatusCodes.Status400BadRequest, + "Please tell us who you are (1-80 characters)."); } if (form.Files.Count == 0) { - return Results.BadRequest(new { error = "No files in upload." }); + return UploadFailed(StatusCodes.Status400BadRequest, "No files in upload."); } var uid = identity.GetOrCreateUid(request.HttpContext); @@ -73,25 +95,9 @@ private static async Task UploadAsync( return Results.Ok(new { results }); } - /// - /// Whether this box is still there and still open to the caller. Small and cheap on - /// purpose: an upload that dies without a response leaves the browser able to report - /// nothing but "network error", and this is what the page asks afterwards to turn that - /// into something the uploader can act on. - /// - private static async Task PoolStatusAsync( - string code, HttpContext context, PoolService pools, PoolAccessService access) - { - var pool = await pools.FindByCodeAsync(code); - if (pool is null) - { - return Results.NotFound(); - } - - return access.CanView(context, pool) - ? Results.Ok(new { open = true }) - : Results.Unauthorized(); - } + /// An upload that failed outright, carrying why in the body the page reads. + private static IResult UploadFailed(int statusCode, string error) => + Results.Json(new { error }, statusCode: statusCode); private static async Task ServeThumbAsync( Guid id, HttpContext context, AppDbContext db, PoolAccessService access, StoragePaths paths) diff --git a/src/Shoebox.Web/wwwroot/js/gallery.js b/src/Shoebox.Web/wwwroot/js/gallery.js index bec6161..9c4bae3 100644 --- a/src/Shoebox.Web/wwwroot/js/gallery.js +++ b/src/Shoebox.Web/wwwroot/js/gallery.js @@ -21,11 +21,16 @@ const limitsSummary = fileInput.dataset.limitsSummary || ""; function parseLimits(json) { + return parseJson(json) || {}; + } + + // Null for anything that isn't JSON — an error page from a proxy, most likely, which is + // exactly the case where the page must not pretend to know what happened. + function parseJson(text) { try { - return JSON.parse(json || "{}"); + return JSON.parse(text); } catch { - // Without limits the pre-flight check simply stands down and the server decides. - return {}; + return null; } } @@ -134,22 +139,6 @@ } } - // Turns a failure the browser couldn't explain into one the uploader can act on, by asking - // the server a question small enough to answer even when a large upload just died. - function failWithDiagnosis(reject, fallback) { - fetch(`/api/p/${poolCode}/status`, { cache: "no-store" }) - .then((res) => { - if (res.status === 401) return "the box locked again — refresh the page to re-enter the password"; - if (res.status === 404) return "this box is gone — it may have expired"; - // The box is fine and reachable, so it was this particular upload that was refused. - if (res.ok) return `${fallback} — it may be larger than the server accepts`; - return fallback; - }) - // The small request failed too, so the connection really is the problem. - .catch(() => "lost the connection to the server") - .then((reason) => reject(new Error(reason))); - } - function uploadOne(file, onProgress) { // XHR instead of fetch: fetch has no upload progress events. return new Promise((resolve, reject) => { @@ -162,35 +151,30 @@ xhr.upload.addEventListener("progress", (e) => { if (e.lengthComputable) onProgress(Math.round((e.loaded / e.total) * 100)); }); + // Whatever went wrong, the reason is the server's to give — every failure it produces + // answers with one. Nothing here invents a cause from a bare status code, because the + // invented cause is always wrong in the same direction: it blames the connection for + // something the server already knew and said. xhr.addEventListener("load", () => { + const body = parseJson(xhr.responseText); if (xhr.status >= 200 && xhr.status < 300) { - try { - resolve(JSON.parse(xhr.responseText)); - } catch { - // A 200 that isn't ours — a proxy's interstitial, most likely. - failWithDiagnosis(reject, "the server sent back something unexpected"); - } - } else if (xhr.status === 401) { - reject(new Error("box is locked; refresh the page")); + if (body) resolve(body); + else reject(new Error("the server's reply wasn't in a form this page could read")); + } else if (body && body.error) { + reject(new Error(body.error)); } else if (xhr.status === 413) { - reject(new Error(`file too large — ${limitsSummary || "try a smaller one"}`)); + // No reason in the body means this 413 came from something in front of the app — + // a proxy with a smaller body limit of its own. The status still says what it is. + reject(new Error(`larger than this server accepts — ${limitsSummary}`)); } else { - // An error page rather than our JSON means whatever went wrong went wrong before - // the upload was ever looked at, so the server is worth asking about. - let msg = null; - try { msg = JSON.parse(xhr.responseText).error; } catch { /* not ours */ } - if (msg) reject(new Error(msg)); - else failWithDiagnosis(reject, `the server refused this upload (error ${xhr.status})`); + reject(new Error(`the server refused this upload (HTTP ${xhr.status})`)); } }); - // A request that dies without a response lands here, and the event says nothing about - // why: a dropped connection, a box that locked itself again, and a proxy hanging up on - // a body it thought too big are one and the same to the browser. Don't guess — ask. + // No response arrived, and that is the whole of what this event says. Report exactly + // that: a message naming a cause it doesn't know is worse than no cause at all. xhr.addEventListener("error", () => - failWithDiagnosis(reject, "the upload was cut off before the server answered") + reject(new Error("the upload didn't finish — the server sent no reply")) ); - xhr.addEventListener("abort", () => reject(new Error("upload cancelled"))); - xhr.addEventListener("timeout", () => reject(new Error("upload timed out"))); xhr.send(form); }); } diff --git a/tests/Shoebox.Tests.Web/CoreFlowTests.cs b/tests/Shoebox.Tests.Web/CoreFlowTests.cs index f3db2d2..0cce36c 100644 --- a/tests/Shoebox.Tests.Web/CoreFlowTests.cs +++ b/tests/Shoebox.Tests.Web/CoreFlowTests.cs @@ -275,23 +275,49 @@ public async Task Gallery_page_tells_the_browser_what_it_may_send() Assert.Contains("accept=\"image/*,video/*,", html); } - [Fact] - public async Task Pool_status_says_whether_the_box_is_reachable_and_open() + [Theory] + [InlineData("locked", HttpStatusCode.Unauthorized, "locked")] + [InlineData("missing", HttpStatusCode.NotFound, "no longer exists")] + public async Task Upload_that_fails_outright_still_answers_with_the_reason( + string scenario, HttpStatusCode expectedStatus, string expectedReason) { using var factory = new ShoeboxWebApplicationFactory(); using var owner = CreateClient(factory); var code = await CreateBoxAsync(owner, password: "festival-secret"); - // This is what the page asks after an upload dies without a response, so it has to - // separate the three things the browser's error event cannot: box gone, box locked, - // and box fine (so it was that upload the server refused). - var open = await owner.GetAsync($"/api/p/{code}/status"); - Assert.Equal(HttpStatusCode.OK, open.StatusCode); - Assert.True((await open.Content.ReadFromJsonAsync()).GetProperty("open").GetBoolean()); + // A client that has to fill in the blank itself can only guess, and the guess comes + // out as a connection problem however little the connection had to do with it. + using var caller = scenario == "locked" ? CreateClient(factory) : owner; + var target = scenario == "missing" ? "NOSUCHBX" : code; - using var guest = CreateClient(factory); - Assert.Equal(HttpStatusCode.Unauthorized, (await guest.GetAsync($"/api/p/{code}/status")).StatusCode); - Assert.Equal(HttpStatusCode.NotFound, (await owner.GetAsync("/api/p/NOSUCHBX/status")).StatusCode); + var response = await PostUploadAsync(caller, target, "Alice", "sample.png", FortyPixelPng); + + Assert.Equal(expectedStatus, response.StatusCode); + var error = (await response.Content.ReadFromJsonAsync()).GetProperty("error").GetString(); + Assert.Contains(expectedReason, error, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task Upload_past_the_request_body_limit_answers_413_saying_so() + { + // Small enough ceilings that the request limit itself (the larger of the two, plus + // form overhead) is reachable without moving hundreds of megabytes through the test. + using var factory = new ShoeboxWebApplicationFactory(new Dictionary + { + ["Shoebox:MaxFileSizeMb"] = "1", + ["Shoebox:MaxVideoFileSizeMb"] = "1", + }); + using var owner = CreateClient(factory); + var code = await CreateBoxAsync(owner); + + var response = await PostUploadAsync(owner, code, "Alice", "huge.mp4", new byte[3 * 1024 * 1024]); + + // Kestrel's own answer to an over-limit body is an unhandled exception and an HTML + // error page — a 500 that says nothing about size, and nothing a page can show. + Assert.Equal(HttpStatusCode.RequestEntityTooLarge, response.StatusCode); + var error = (await response.Content.ReadFromJsonAsync()).GetProperty("error").GetString(); + Assert.Contains("larger than this server accepts", error); + Assert.Contains("1 MB", error); } [Fact] @@ -438,6 +464,19 @@ private static async Task UploadAsync( string uploader, string fileName, byte[] bytes) + { + var response = await PostUploadAsync(client, code, uploader, fileName, bytes); + response.EnsureSuccessStatusCode(); + var body = await response.Content.ReadFromJsonAsync(); + return Assert.Single(Assert.IsType(body).Results); + } + + private static async Task PostUploadAsync( + HttpClient client, + string code, + string uploader, + string fileName, + byte[] bytes) { using var form = new MultipartFormDataContent(); form.Add(new StringContent(uploader), "uploaderName"); @@ -446,10 +485,7 @@ private static async Task UploadAsync( new System.Net.Http.Headers.MediaTypeHeaderValue("application/octet-stream"); form.Add(file, "files", fileName); - var response = await client.PostAsync($"/api/p/{code}/media", form); - response.EnsureSuccessStatusCode(); - var body = await response.Content.ReadFromJsonAsync(); - return Assert.Single(Assert.IsType(body).Results); + return await client.PostAsync($"/api/p/{code}/media", form); } private sealed record UploadEnvelope(UploadResponse[] Results); From 173afc282d7ecbcb2b237e7d5be323188f8cdc98 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 01:51:55 +0000 Subject: [PATCH 4/4] Cut the upload changes back to what the fix needs Drops the format hint under the upload card and its styling, the progress-row styling, the generated accept attribute (the existing one just gains .mkv), and the commentary that had grown longer than the code it sat above. Tests keep the two that would fail if this regressed: an .mkv upload being stored and served as a video, and an over-limit body answering 413 with the reason rather than 500 and an HTML page. The rest asserted message wording and page markup, which the code they cover already reads plainly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RYoVr4rkVdeQn5THttcGRM --- README.md | 15 +--- src/Shoebox.Web/Api/MediaEndpoints.cs | 13 ++- src/Shoebox.Web/Pages/Pool/Gallery.cshtml | 6 +- src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs | 4 +- src/Shoebox.Web/Services/IMediaHandler.cs | 44 +-------- src/Shoebox.Web/Services/MediaService.cs | 4 +- src/Shoebox.Web/Services/VideoHandler.cs | 4 +- src/Shoebox.Web/wwwroot/css/site.css | 15 ---- src/Shoebox.Web/wwwroot/js/gallery.js | 38 +++----- tests/Shoebox.Tests.Web/CoreFlowTests.cs | 89 ++----------------- .../ShoeboxWebApplicationFactory.cs | 5 +- 11 files changed, 36 insertions(+), 201 deletions(-) diff --git a/README.md b/README.md index 2f6a968..6336a4f 100644 --- a/README.md +++ b/README.md @@ -168,17 +168,10 @@ appear in the gallery everywhere. The original file is always stored unmodified Download button returns. Files of the wrong type, over `MaxFileSizeMb`, or that don't decode as a real image within the pixel limits are rejected at upload. -The browser checks the extension and the size against these limits before it sends anything, -so a file that wouldn't be accepted is refused in the moment it's picked, naming what the box -does take. The server re-checks everything regardless — the browser-side check is there to -save the wait, not to enforce anything. - -Every way an upload can fail answers with the reason in the body, in the same shape as a -success — a box that expired, a box that locked itself again, a body past the request limit -(which otherwise surfaces as an unhandled exception and an HTML error page saying nothing -about size). The page shows what the server said and never fills in a cause of its own: a -client guessing at a bare status code or a dead connection reliably blames the network for -something the server already knew and named. +The browser checks a file against these limits before sending it, so one that wouldn't be +accepted is refused as soon as it's picked; the server checks again regardless. Uploads that +fail outright answer with the reason in the body, and the page shows what it was told rather +than filling in a cause of its own. ### Animations diff --git a/src/Shoebox.Web/Api/MediaEndpoints.cs b/src/Shoebox.Web/Api/MediaEndpoints.cs index 8e2952f..8270551 100644 --- a/src/Shoebox.Web/Api/MediaEndpoints.cs +++ b/src/Shoebox.Web/Api/MediaEndpoints.cs @@ -24,10 +24,9 @@ public static void MapMediaApi(this IEndpointRouteBuilder app) } /// - /// Takes one upload. Every way this can fail answers with the reason in the body, in the - /// same shape as a success: whatever the page shows the uploader has to come from here, - /// because a client left to fill in the blank can only guess, and a guess reads as a - /// network problem when the file was simply not one this box takes. + /// Takes one upload. Every way this can fail answers with the reason in the body: what the + /// page shows has to come from here, since a client filling in the blank itself can only + /// guess, and its guess is always that the network was at fault. /// private static async Task UploadAsync( string code, @@ -64,9 +63,8 @@ private static async Task UploadAsync( } catch (Exception ex) when (ex is BadHttpRequestException or InvalidDataException) { - // The body ran past the request limit, so it was never read and nothing below has - // seen the file. Left alone this surfaces as an unhandled exception and an HTML - // error page, which says nothing at all about size. + // Past the request-body limit. Left alone this is an unhandled exception and an + // HTML error page, which says nothing about size. return UploadFailed(StatusCodes.Status413PayloadTooLarge, $"That file is larger than this server accepts — {handlers.Policy.Summary}."); } @@ -95,7 +93,6 @@ private static async Task UploadAsync( return Results.Ok(new { results }); } - /// An upload that failed outright, carrying why in the body the page reads. private static IResult UploadFailed(int statusCode, string error) => Results.Json(new { error }, statusCode: statusCode); diff --git a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml index 460d8c5..906707f 100644 --- a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml +++ b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml @@ -53,14 +53,12 @@
- @* accept only steers the file picker; the drop zone ignores it, so gallery.js - checks every file against data-limits before it sends a byte of it. *@ -

or drop photos anywhere on this page

-

Takes @Model.UploadPolicy.Summary.

    diff --git a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs index 3ea6f24..1369a88 100644 --- a/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs +++ b/src/Shoebox.Web/Pages/Pool/Gallery.cshtml.cs @@ -34,9 +34,7 @@ public class GalleryModel( // "everyone else's" download only offers itself when it would actually return files. public bool HasOthers { get; set; } - // What the server takes, handed to the page so the browser can turn a file away before - // sending it. A 400 MB clip that gets its connection cut for exceeding the request-body - // limit reaches the uploader as "network error" and nothing more. + // Handed to the page so the browser can turn a file away before sending it. public UploadPolicy UploadPolicy => handlers.Policy; public string UploadLimitsJson => JsonSerializer.Serialize(UploadPolicy.MaxBytesByExtension); diff --git a/src/Shoebox.Web/Services/IMediaHandler.cs b/src/Shoebox.Web/Services/IMediaHandler.cs index 3431412..bf05be4 100644 --- a/src/Shoebox.Web/Services/IMediaHandler.cs +++ b/src/Shoebox.Web/Services/IMediaHandler.cs @@ -45,25 +45,11 @@ public interface IMediaHandler } /// -/// What the browser needs to turn a file away before sending it, plus the words to use when -/// something doesn't fit. A video can be hundreds of megabytes, and a file the server won't -/// take is worth saying no to in the moment it's picked — not after a long upload, and -/// certainly not by having the connection cut for exceeding the request-body limit, which -/// reaches the user as nothing more useful than "network error". +/// What the server takes: the ceiling per extension, for the browser to check a file against +/// before sending it, and the same thing in words, for saying why one didn't fit. /// -/// Accepted extensions (lowercase, dotted) and their ceilings. -/// Value for the file input's accept attribute. -/// Plain-language "what fits", e.g. for the upload card and rejections. -public record UploadPolicy( - IReadOnlyDictionary MaxBytesByExtension, - string Accept, - string Summary) +public record UploadPolicy(IReadOnlyDictionary MaxBytesByExtension, string Summary) { - /// The reason to give for a file whose extension nothing here takes. - public string RejectionFor(string extension) => - $"Can't take {(string.IsNullOrWhiteSpace(extension) ? "files with no extension" : extension.ToLowerInvariant() + " files")} — {Summary}"; - - /// Sizes are only ever shown to people, so one decimal of MB is plenty. public static string DescribeSize(long bytes) => $"{bytes / (1024.0 * 1024.0):0.#} MB"; } @@ -91,33 +77,11 @@ public class MediaHandlers(IEnumerable handlers) public IMediaHandler For(MediaKind kind) => all.First(h => h.Kind == kind); - /// - /// What this build accepts, assembled from the handlers so the browser, the upload card and - /// the rejection messages can never drift from what the server actually stores. - /// + /// Assembled from the handlers, so nothing that quotes it can drift from them. public UploadPolicy Policy => new( all.SelectMany(h => h.ContentTypes.Keys.Select(e => (Extension: e.ToLowerInvariant(), h.MaxBytes))) .ToDictionary(x => x.Extension, x => x.MaxBytes, StringComparer.OrdinalIgnoreCase), - BuildAccept(), string.Join(", ", all.Select(h => $"{h.Label}s ({string.Join(", ", h.ContentTypes.Keys.Select(e => e.TrimStart('.')))}) " + $"up to {UploadPolicy.DescribeSize(h.MaxBytes)}"))); - - /// - /// The wildcards (image/*, video/*) keep phone pickers showing the camera roll; the explicit - /// extensions cover the formats a picker may not map to one (HEIC and MKV in particular). - /// Nothing here is a security control — it only steers the picker, and the drop zone ignores - /// it entirely — so the real check is on the way in. - /// - private string BuildAccept() - { - var wildcards = all - .SelectMany(h => h.ContentTypes.Values) - .Select(type => type[..(type.IndexOf('/') + 1)] + "*") - .Distinct(StringComparer.OrdinalIgnoreCase); - var extensions = all - .SelectMany(h => h.ContentTypes.Keys.Select(e => e.ToLowerInvariant())) - .Distinct(StringComparer.Ordinal); - return string.Join(",", wildcards.Concat(extensions)); - } } diff --git a/src/Shoebox.Web/Services/MediaService.cs b/src/Shoebox.Web/Services/MediaService.cs index 0d1fb73..bbbd138 100644 --- a/src/Shoebox.Web/Services/MediaService.cs +++ b/src/Shoebox.Web/Services/MediaService.cs @@ -22,8 +22,8 @@ public async Task SaveAsync(Pool pool, IFormFile file, string uplo var match = handlers.For(extension); if (match is null) { - // Say what does fit: "unsupported" on its own leaves the uploader guessing. - return UploadResult.Rejected(fileName, handlers.Policy.RejectionFor(extension)); + var what = extension.Length == 0 ? "files with no extension" : $"{extension.ToLowerInvariant()} files"; + return UploadResult.Rejected(fileName, $"Can't take {what} — {handlers.Policy.Summary}"); } var (handler, contentType) = match.Value; diff --git a/src/Shoebox.Web/Services/VideoHandler.cs b/src/Shoebox.Web/Services/VideoHandler.cs index 38033cc..1669d28 100644 --- a/src/Shoebox.Web/Services/VideoHandler.cs +++ b/src/Shoebox.Web/Services/VideoHandler.cs @@ -16,9 +16,7 @@ public class VideoHandler(VideoRenderer renderer, IOptions optio [".m4v"] = "video/mp4", [".mov"] = "video/quicktime", [".webm"] = "video/webm", - // Matroska shares WebM's container, so the header check and ffmpeg already handle it; - // only the extension was missing, and a phone-sized .mkv would hit the request-body - // limit and die as a bare "network error" long before anything said it wasn't wanted. + // Matroska shares WebM's container, so the header check and ffmpeg already handled it. [".mkv"] = "video/x-matroska", }; diff --git a/src/Shoebox.Web/wwwroot/css/site.css b/src/Shoebox.Web/wwwroot/css/site.css index 19c0c36..3eee90e 100644 --- a/src/Shoebox.Web/wwwroot/css/site.css +++ b/src/Shoebox.Web/wwwroot/css/site.css @@ -514,13 +514,6 @@ input[type="checkbox"] { accent-color: var(--ink); } color: var(--muted); } -/* What fits, said up front, so nobody finds out by watching a long upload fail. */ -.upload-formats { - margin: 0.25rem 0 0; - font-size: 0.8rem; - color: var(--muted); -} - .upload-progress { list-style: none; margin: 0.6rem 0 0; padding: 0; font-size: 0.85rem; } .upload-progress li { display: flex; @@ -529,14 +522,6 @@ input[type="checkbox"] { accent-color: var(--ink); } padding: 0.3rem 0.1rem; border-top: 1px dashed var(--line); } -/* A rejection needs room to say why, so the file name is what gives way. */ -.upload-progress li > span:first-child { - min-width: 4rem; - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; -} -.upload-progress .status { text-align: right; } .upload-progress .ok { color: var(--ok); } .upload-progress .fail { color: var(--danger); } diff --git a/src/Shoebox.Web/wwwroot/js/gallery.js b/src/Shoebox.Web/wwwroot/js/gallery.js index 9c4bae3..89e1044 100644 --- a/src/Shoebox.Web/wwwroot/js/gallery.js +++ b/src/Shoebox.Web/wwwroot/js/gallery.js @@ -12,20 +12,11 @@ // ---------- Upload ---------- - // What the server will take, rendered into the file input by the page: extension -> byte - // ceiling. The browser checks against it first, because everything past this point is - // expensive to get wrong — a file over the request-body limit has its connection cut - // mid-send and surfaces as a bare "network error", with nothing to tell the uploader that - // the size was the problem. - const limits = parseLimits(fileInput.dataset.limits); + // What the server takes, extension -> byte ceiling, so a file it would refuse is refused + // here instead of after a long upload. The server checks again regardless. + const limits = parseJson(fileInput.dataset.limits) || {}; const limitsSummary = fileInput.dataset.limitsSummary || ""; - function parseLimits(json) { - return parseJson(json) || {}; - } - - // Null for anything that isn't JSON — an error page from a proxy, most likely, which is - // exactly the case where the page must not pretend to know what happened. function parseJson(text) { try { return JSON.parse(text); @@ -34,22 +25,19 @@ } } - // The reason this file can't be sent, or null when it's worth trying. Wording matches the - // server's own rejections, since either can be what lands in the progress list. + // Why this file can't be sent, or null when it's worth trying. function whyNotUploadable(file) { - const dot = file.name.lastIndexOf("."); - const extension = dot > 0 ? file.name.slice(dot).toLowerCase() : ""; if (!Object.keys(limits).length) return null; + const dot = file.name.lastIndexOf("."); + const extension = dot > 0 ? file.name.slice(dot).toLowerCase() : ""; const max = limits[extension]; if (max === undefined) { const what = extension ? extension + " files" : "files with no extension"; return `Can't take ${what} — ${limitsSummary}`; } if (file.size === 0) return "Empty file"; - if (file.size > max) { - return `Too big (${describeSize(file.size)}) — up to ${describeSize(max)}`; - } + if (file.size > max) return `Too big (${describeSize(file.size)}) — up to ${describeSize(max)}`; return null; } @@ -151,10 +139,7 @@ xhr.upload.addEventListener("progress", (e) => { if (e.lengthComputable) onProgress(Math.round((e.loaded / e.total) * 100)); }); - // Whatever went wrong, the reason is the server's to give — every failure it produces - // answers with one. Nothing here invents a cause from a bare status code, because the - // invented cause is always wrong in the same direction: it blames the connection for - // something the server already knew and said. + // The reason is the server's to give; nothing here invents one it wasn't told. xhr.addEventListener("load", () => { const body = parseJson(xhr.responseText); if (xhr.status >= 200 && xhr.status < 300) { @@ -163,15 +148,14 @@ } else if (body && body.error) { reject(new Error(body.error)); } else if (xhr.status === 413) { - // No reason in the body means this 413 came from something in front of the app — - // a proxy with a smaller body limit of its own. The status still says what it is. + // A 413 with no reason in it came from something in front of the app, but the + // status alone still says what happened. reject(new Error(`larger than this server accepts — ${limitsSummary}`)); } else { reject(new Error(`the server refused this upload (HTTP ${xhr.status})`)); } }); - // No response arrived, and that is the whole of what this event says. Report exactly - // that: a message naming a cause it doesn't know is worse than no cause at all. + // No response arrived, which is the whole of what this event says. Say only that. xhr.addEventListener("error", () => reject(new Error("the upload didn't finish — the server sent no reply")) ); diff --git a/tests/Shoebox.Tests.Web/CoreFlowTests.cs b/tests/Shoebox.Tests.Web/CoreFlowTests.cs index 0cce36c..f030e50 100644 --- a/tests/Shoebox.Tests.Web/CoreFlowTests.cs +++ b/tests/Shoebox.Tests.Web/CoreFlowTests.cs @@ -22,8 +22,8 @@ public class CoreFlowTests (byte)'i', (byte)'s', (byte)'o', (byte)'m', (byte)'m', (byte)'p', (byte)'4', (byte)'2', ]; - // An EBML header, which is what a Matroska (.mkv) or WebM file starts with. Enough to clear - // the container check without needing a real clip, as with Mp4Header above. + // An EBML header: what a Matroska (.mkv) file starts with, enough to clear the container + // check without a real clip, as with Mp4Header above. private static readonly byte[] MatroskaHeader = [ 0x1A, 0x45, 0xDF, 0xA3, 0x01, 0x00, 0x00, 0x00, @@ -219,89 +219,11 @@ public async Task Matroska_clip_is_accepted_like_any_other_video() Assert.Contains("media-badge\">\u25B6 Video", await gallery.Content.ReadAsStringAsync()); } - [Fact] - public async Task Rejected_file_type_says_what_the_box_does_take() - { - using var factory = new ShoeboxWebApplicationFactory(); - using var owner = CreateClient(factory); - var code = await CreateBoxAsync(owner); - - var result = await UploadAsync(owner, code, "Alice", "notes.txt", [1, 2, 3, 4]); - - Assert.Equal("rejected", result.Status); - Assert.Null(result.MediaId); - // "Unsupported" alone leaves the uploader guessing; the accepted formats and their - // ceilings have to be in the message itself. - Assert.Contains(".txt", result.Reason); - Assert.Contains("mkv", result.Reason); - Assert.Contains("jpg", result.Reason); - Assert.Contains("200 MB", result.Reason); - } - - [Fact] - public async Task Oversized_video_is_rejected_with_its_size_and_the_limit() - { - using var factory = new ShoeboxWebApplicationFactory(new Dictionary - { - ["Shoebox:MaxVideoFileSizeMb"] = "1", - }); - using var owner = CreateClient(factory); - var code = await CreateBoxAsync(owner); - - var oversized = new byte[3 * 1024 * 1024]; - MatroskaHeader.CopyTo(oversized, 0); - var result = await UploadAsync(owner, code, "Alice", "long.mkv", oversized); - - Assert.Equal("rejected", result.Status); - Assert.Null(result.MediaId); - Assert.Contains("Too big (3 MB)", result.Reason); - Assert.Contains("videos can be up to 1 MB", result.Reason); - } - - [Fact] - public async Task Gallery_page_tells_the_browser_what_it_may_send() - { - using var factory = new ShoeboxWebApplicationFactory(); - using var owner = CreateClient(factory); - var code = await CreateBoxAsync(owner); - - var html = await (await owner.GetAsync($"/p/{code}")).Content.ReadAsStringAsync(); - - // The pre-flight check in gallery.js reads these; without them a file the server - // would refuse is uploaded in full first, and an oversized one dies as "network error". - Assert.Contains("data-limits=", html); - Assert.Contains("".mkv":209715200", html); - Assert.Contains("".jpg":52428800", html); - Assert.Contains("accept=\"image/*,video/*,", html); - } - - [Theory] - [InlineData("locked", HttpStatusCode.Unauthorized, "locked")] - [InlineData("missing", HttpStatusCode.NotFound, "no longer exists")] - public async Task Upload_that_fails_outright_still_answers_with_the_reason( - string scenario, HttpStatusCode expectedStatus, string expectedReason) - { - using var factory = new ShoeboxWebApplicationFactory(); - using var owner = CreateClient(factory); - var code = await CreateBoxAsync(owner, password: "festival-secret"); - - // A client that has to fill in the blank itself can only guess, and the guess comes - // out as a connection problem however little the connection had to do with it. - using var caller = scenario == "locked" ? CreateClient(factory) : owner; - var target = scenario == "missing" ? "NOSUCHBX" : code; - - var response = await PostUploadAsync(caller, target, "Alice", "sample.png", FortyPixelPng); - - Assert.Equal(expectedStatus, response.StatusCode); - var error = (await response.Content.ReadFromJsonAsync()).GetProperty("error").GetString(); - Assert.Contains(expectedReason, error, StringComparison.OrdinalIgnoreCase); - } - [Fact] public async Task Upload_past_the_request_body_limit_answers_413_saying_so() { - // Small enough ceilings that the request limit itself (the larger of the two, plus - // form overhead) is reachable without moving hundreds of megabytes through the test. + // Ceilings small enough to reach the request-body limit without moving hundreds of + // megabytes through the test. using var factory = new ShoeboxWebApplicationFactory(new Dictionary { ["Shoebox:MaxFileSizeMb"] = "1", @@ -312,8 +234,7 @@ public async Task Upload_past_the_request_body_limit_answers_413_saying_so() var response = await PostUploadAsync(owner, code, "Alice", "huge.mp4", new byte[3 * 1024 * 1024]); - // Kestrel's own answer to an over-limit body is an unhandled exception and an HTML - // error page — a 500 that says nothing about size, and nothing a page can show. + // Uncaught, an over-limit body is a 500 and an HTML error page never mentioning size. Assert.Equal(HttpStatusCode.RequestEntityTooLarge, response.StatusCode); var error = (await response.Content.ReadFromJsonAsync()).GetProperty("error").GetString(); Assert.Contains("larger than this server accepts", error); diff --git a/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs b/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs index 6d4ecda..a53c24e 100644 --- a/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs +++ b/tests/Shoebox.Tests.Web/ShoeboxWebApplicationFactory.cs @@ -8,10 +8,7 @@ public sealed class ShoeboxWebApplicationFactory : WebApplicationFactory settings; - /// - /// Configuration overrides for this instance, e.g. a small size ceiling so the oversized - /// path can be exercised without a real 200 MB file. - /// + /// Configuration overrides for this instance. public ShoeboxWebApplicationFactory(IReadOnlyDictionary? settings = null) { this.settings = settings ?? new Dictionary();