Release/1.10.0 - #88
Merged
Merged
Conversation
added 18 commits
August 21, 2026 13:50
Three faults on the privileged path, all on the same code. runUpdate spawned unconditionally, so a double-click ran two `compose pull` processes and then two privileged helper containers against the same stack, each recreating containers the other was recreating. The second press also cleared the first run's error, so a failure could vanish before anyone read it. A second press is now refused rather than queued - two updates are not twice an update - and the guard lives in runUpdate as well as the route, since a guard that only lives in the caller is one refactor from being gone. A run that hangs releases after fifteen minutes, or the button would stay dead for the life of the process. GET /update/status is unauthenticated on purpose: the client has to keep reading it across the restart, when nobody is logged in. It was answering with the last forty lines of update.log - compose output, host paths, absolute directories - and no client has ever displayed them. The payload is now a phase, a stable code and a sentence of ours; the detail goes to the log file where it already went. The helper command was assembled into an `sh -c` string with the working directory interpolated in double quotes, which reads safe and is not: double quotes stop word-splitting, not `$(...)` or backticks. That path comes from a compose label, so a project directory named with a command substitution would have run it inside a container holding the Docker socket. It is argv now, so nothing is parsed as shell at all - which also fixes a working directory with a space in it.
The preflight refusal offered only "add the socket", which reads as a fault to repair rather than a choice already made. Running without it is a reasonable stance for an install facing the internet - the socket is root on the host - so the message now offers updating from the host as an equal option, and UPGRADE.md says how and what it costs. UPGRADE.md also now states what pressing Update actually trusts: images pulled from a Docker Hub account and run as root with the host socket, by tag rather than by digest. That is inherent to any auto-updater and belongs written down next to the two ways to opt out, rather than left implicit. Version bumped in both package files.
…n failures Two things found reviewing the branch rather than writing it. The fifteen-minute stale release was a hole I had just introduced. Elapsed time cannot tell a hung pull from a slow one - two images over a poor line outlast any threshold worth setting - so releasing on the clock alone would let a second press start a competing update against a stack the first was still changing, which is the whole thing this branch exists to prevent. A child process we can still see now holds the lock however long it takes. The cost is that a pull which truly never returns holds the button until the backend restarts; that is recoverable from the host, and it is the safer direction for an operation running as root. The elapsed check stays for the case where the child is gone and its events never arrived, where nothing is running and nothing else would let go. The helper leg had an error handler but no close handler, so a helper whose `compose up` failed left the phase on 'restarting' for ever and the client polling for a restart that was never coming. That is precisely the failure this module was written to fix, still alive in its second half. On success it usually never fires, since `up -d` replaces the container watching - which is why the helper exists, and why this only fires on the path that matters.
Battle maps checked nothing at all, and music checked req.file.mimetype - the Content-Type the uploader wrote into the multipart body, a claim about the file rather than a reading of it. So a file called payload.html sent as audio/mpeg was written as payload.html into a directory served with no auth, where the extension is what sets the Content-Type on the way back out. Stored HTML on the app's own origin, from an admin token. Both now check the extension, since that is the field that decides how the file is served. The lists are wider than the pickers offer on purpose - every image format including SVG and TIFF, every audio format a browser will play including opus, aac and webm. The old mimetype list turned opus away for having an unusual Content-Type rather than for being unplayable, which is not a reason. What makes a wide list safe is the serving, not the list: /uploads now sets a sandbox CSP and nosniff on every file. A document opened from there lands in an opaque origin with nothing of ours to reach, which also covers whatever is already on disk from before any of this existed. It is why SVG can stay - script in an SVG cannot run inside an <img>, only when the file is opened directly, and that is the case the header closes.
Prompted by asking why webm was not on it. Maps are drawn with THREE.TextureLoader, which decodes through an <img>, so there is no video path at all - a .webm would upload perfectly and then render as nothing. Animated map loops are a real thing people buy; this is a missing feature rather than a refused format, and it needs a VideoTexture before the extension means anything. Checking that turned up a mistake in the previous commit. I had added .tif/.tiff in the name of being generous, and no browser displays TIFF in an <img> - so a GM uploading one would have got a successful upload and a blank battle map instead of a sentence telling them to convert it. Being permissive past what the renderer can show is not kindness, it is a silent failure. Removed, with a test naming TIFF as the specific trap and video as the deliberate omission.
A .webm, .mp4 or .m4v plays under the tokens. The animated map packs people buy work as they are, with nothing to convert and nothing to configure. The scene is two components now rather than one with a branch in it. useLoader suspends and there is no Suspense boundary anywhere above the battle map, so the video path must not take it - and hooks cannot be called conditionally, which settles the shape. A still map goes through exactly the code it always did. Everything awkward about the video is browser policy rather than rendering, and all of it fails silently: unmuted never autoplays, iOS takes a video without playsInline full-screen so it is not on the map at all, a hidden tab keeps decoding until something pauses it, and pausing alone leaves the buffers held so a GM stepping through floors accumulates one loaded video per map. Each of those is a test. BattleMapScene had no coverage before this - the app smoke test mocks it away - so which loader a map goes to is now pinned directly.
It had its own shorter list - png, jpg, webp, gif - so a map in any other accepted format could be uploaded, used on the location it was uploaded to, and then never found again from anywhere else. Animated maps made it visible, but it was already true of avif, bmp and svg. It now reads the list the upload route enforces, so the two cannot drift again. Also records what a browser actually does with the sandbox header added in 1.9.5, having checked rather than reasoned about it. Both halves hold: a PNG and an SVG still load normally as <img> through that mount, so battle maps and portraits are untouched - and a stored HTML file does not merely land in an opaque origin, its script is refused outright.
Each rejection had been written where it was needed, so each said a different amount. The size limit said nothing at all - it was never handled, so an oversized file reached multer's own error, fell through to Express's default handler, and came back as an HTML page the client then tried to parse as JSON. What a person saw for uploading a large map was a syntax error about an unexpected '<'. One message shape now answers the three questions somebody staring at a rejected upload actually has: which file, what was wrong with it, and what would have worked. multer aborts before any handler runs and its error carries only the form field, so the filename is recorded on the way in - without that, "too large" names nothing. The cap was written out three times in one component and once in the backend, with nothing keeping them in step, which is how you tell someone a file is fine and then reject it. It is one constant each side now, and a test reads the frontend's copy from source and asserts it matches the server's. Also: animated maps are listed in SELECT EXISTING since the previous commit, and that gallery draws every entry as an <img>, so a loop showed as a broken icon.
25MB made animated maps decorative. An animated map is a video and the loops people actually buy run past that regularly, so the feature only worked for files small enough not to be worth animating. The cap could not simply be raised: multer buffered the whole upload in memory and the route hashed the complete buffer, so the ceiling was a RAM budget on a machine that is often somebody's home server. Uploads now stream to a temporary file and are hashed in chunks, then renamed to their content hash - memory stays flat whatever the size, and the ceiling costs disk instead. Deduplication by hash is what keeps that honest: the same map on a dozen locations is one file. Every exit from the handler now cleans up after itself, since the upload is on disk before the format is checked. Tested: a 40MB map through the real route, deduplicated on second upload, and nothing left in the temporary directory when a map is refused. The size-limit tests use a small stand-in limit rather than allocating a quarter of a gigabyte to prove a point about not allocating a quarter of a gigabyte.
The security work and the animated maps were on stacked branches and were going to ship as 1.9.5 and 1.10.0. They are one release: making animated maps usable meant reworking how uploads are stored and checked, which is where the upload half of the security pass already was, so splitting them would have put related changes either side of a version boundary. The changelog entries are merged rather than rewritten - every bullet from both survives, reorganised under Added / Security / Changed / Fixed.
Caught auditing the project structure, not by any test. nginx caps request bodies at its own limit and everything reaches the backend through it in a Docker install, so it sat at 25M while battle maps moved to 250MB - a large animated map would have been refused by the proxy before the app ever saw it, with a bare 413 naming neither the file nor the reason. The feature would not have worked in the only deployment that matters, with 996 tests passing. Nothing caught it because every test here talks to Express directly and never sees the proxy. A test now reads nginx.conf and asserts its ceiling is at least the largest limit in LIMITS, which fails if either side moves again. Also refreshes the README entries for the files this release changed - the battle map route, music, server.js and nginx.conf all described behaviour they no longer have - and splits the upgrade notes, which had picked up a 1.10.0 paragraph under the 1.9.4 heading. The X-Forwarded-For note stays with 1.9.4, where it shipped; only the size ceiling belongs to this release.
Nothing in this repository touched nginx. Every test mounts an Express router directly, so the proxy was untested infrastructure by construction - and two faults in one release lived exactly there: a body ceiling smaller than the app's own, and a missing X-Forwarded-For that made every request look like it came from the proxy. Both were one line of config and both were invisible to a thousand passing tests. Two layers, because they catch different things. nginx_config.test.js reads the config and asserts what the application code assumes of it: a body ceiling at least the largest upload limit, the forwarded address the rate limiter depends on, the two headers the socket upgrade needs, a location block for every path the backend serves, and upstreams that name a service the compose file actually defines. Static, so it runs everywhere and costs nothing. The Nginx Proxy Behaviour CI job runs the real thing - nginx:alpine against this repository's config and a stub upstream that reports what reached it - and checks a 40MB body arrives whole, the caller's address is forwarded, and the socket path offers an upgrade. That covers what a grep cannot judge: whether the config as written does what reading it suggests.
…en down The CI job was written without ever having executed. It has now been run by hand against nginx:alpine and this repository's own config: a 40MB body arrives whole with status 200, X-Forwarded-For reaches the upstream, the socket path carries its upgrade headers, and /uploads proxies. The counterfactual matters more than the result. The same 40MB body against the previous 25M ceiling returns 413 from nginx, which means the probe fails when the config is wrong rather than passing whatever it finds.
The one claim in this work that no test could reach. jsdom has neither a media stack nor WebGL, so the tests here cover how the video element is configured and nothing about whether a frame ever reaches the screen. Checked against a real VP9 file generated for the purpose: it decodes, muted autoplay starts with no gesture, THREE.VideoTexture uploads frames to the GPU with its version incrementing per render, and six successive rendered frames have six different checksums. The file was served through the same sandboxed /uploads mount the app uses, so those headers do not interfere with a video texture either. Recorded in the hook because one result looked like a fault and was not: a tab the compositor has stopped painting fires neither requestAnimationFrame nor requestVideoFrameCallback, so the texture never marks itself stale and the plane renders black while currentTime keeps advancing. That is the same mechanism the hook uses to pause on hidden tabs - a map frozen in a background window is working as intended, and anyone debugging that later should not go looking for a bug in the texture path.
`video.loop = true` is set and unit-tested, but nothing had observed the wrap. Seeking to just before the end of a 5s clip and sampling: 4.62, 4.88, 5.00, 0.21, 0.48 ... still playing, and `ended` never fires. A loop rather than a clip that stops on the last frame.
Every path out of the upload handler cleans up after itself, but a process killed mid-transfer gets no say, and what it leaves is a partial file of up to 250MB that nothing else will ever look at again. Swept once at startup, which is the only moment an orphan can be told apart from an upload happening right now - anything older than an hour is certainly abandoned, since no upload this accepts can legitimately take that long. Once at startup rather than on a timer: uploads are rare, orphans rarer, and a background interval is a thing to reason about forever in exchange for nothing. The tests write into the real temp directory, so they clean up in afterEach - the first version leaked a file when an assertion failed and the next test tripped over it, which is the same class of mistake as the code being fixed.
Found checking the release rather than by writing it. Both are user-visible and both were only in commit messages. A failed update could leave the app waiting for a restart that was never coming: the pull reported a bad exit but the helper that recreates the containers did not, so a failure there sat on "restarting" indefinitely. And an upload interrupted halfway used to leave its partial file behind for ever - up to the full size limit each, swept at startup now.
Prompted by the only question that matters when upgrading: does any of this delete what people have already put in. The sweep deletes files, runs automatically at startup, and lives one directory below everybody's maps, so the boundary between "abandoned fragment" and "someone's battle map" deserves a test rather than a careful reading of the loop. A year-old stored map, with the sweep run at its most aggressive setting, survives. It reads the temp directory and only the temp directory.
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
1 participant
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.
Summary
Test plan
Pre-merge checklist
Code quality:
Version & Release:
frontend/package.jsonversiondocker-compose.ymlAPP_VERSIONCHANGELOG.mdwith release notesBefore merging to main:
Related issues