Feature/outbound hardening - #87
Merged
Merged
Conversation
added 8 commits
August 21, 2026 11:50
Two callers reach hosts we do not own - the Docker Hub version check and the
Companion import - and they had independently grown two different sets of holes.
This is the door they will both go through.
What a caller cannot opt out of: a named destination, HTTPS, a deadline that
covers the body as well as the connection, and a byte cap. The allowlist is an
exact hostname match rather than a suffix test, because endsWith('example.com')
is also true of evil-example.com. Redirects are not followed, since following
one leaves the allowlist behind entirely.
The transport is injected, so every failure is reachable in tests without a
network - including the one this was written for, a host that accepts the
connection and then says nothing.
fetchVersionTags now goes through net/outbound, which closes three things at once. It had no timeout at all - the five-page bound bounds pages, not time, so a host that accepted the connection and then said nothing hung the promise and the admin's request with it. It accumulated the body with no ceiling. And it took the hostname for the next page out of the response body, which let whoever answered choose where we knocked next; only the path survives now, and the host is ours. The route no longer repeats an upstream error string back to the client. An admin can only retry or wait either way, and those strings carry host paths and internal addresses we did not choose to publish. Tests inject a fetch stub rather than a fake https module, since the request itself now belongs to outbound. What is tested here is the walk over pages.
It already had a deadline and fixed reasons, which is why it was the better of the two callers - but it had no byte cap and nothing enforcing which host it was allowed to reach. Both now come from net/outbound rather than from this file remembering to do them. Test stubs answer with text() rather than json(), because that is what a real response does and what outbound reads: it measures the body against the cap before parsing. A json()-only stub would have been exercising a path production never takes, and accommodating it in the helper would have meant a route that skips the cap entirely.
…rom tests The import is the one open route that spends our outbound requests on a caller's say-so, which made it two things: a way to point our address at a third party as fast as you can ask, and an oracle for walking a six-character keyspace to read back other people's characters. Requiring a token would close both and take the feature away from open installs, where players have none - so the ceiling is on the rate instead of on who may knock. Ten in ten minutes: generous to someone who mistyped their code, useless to a script. req.ip was the nginx container's address for every request, so a per-caller limit would have counted the whole table as one. nginx now forwards X-Forwarded-For and express trusts one hop - the backend port is not published, so nginx is the only thing that can reach it. Separately: the updater's route tests had been talking to the real Docker Hub. The https spy stopped applying when the transport changed underneath it, and the suite kept passing because the real registry happened to agree with the assertions. That is the second time this has happened, so fetch now fails loudly in tests unless a test has stubbed it.
… per request That route is unauthenticated on purpose - a client has to keep reading it across the restart an update causes - and it shelled out to `docker info` every time it was asked. execSync holds the event loop until the daemon answers, so anyone who could reach the endpoint could stall the whole server by requesting it in a loop. It was the most exploitable thing in the outbound sweep and the cheapest to close: the socket is mounted at container start or it is not, so the answer belongs to the process rather than to the request. A no is remembered as firmly as a yes. Socket-less is a supported posture, not a transient failure, and retrying it per request would be the same stall for the installs most likely to be exposed. The probe also gained a timeout, for a socket that is present but not answering.
Version bumped in both package files. The changelog entry is written for someone deciding whether to update rather than for someone reading the diff: what could have happened, what changes for them, and - since this release changes no behaviour anyone asked for - why it was worth a release at all.
Docker installs need to do nothing - nginx.conf is built into the frontend image rather than mounted, so a pull carries it. The note is for people running their own reverse proxy, where a missing X-Forwarded-For makes the new per-player import limit count the whole table as one caller, and for anyone exposing the backend directly now that it trusts one forwarding hop. Also flags the changed update-check behaviour: a rate-limited Docker Hub now reports an error instead of "You're up to date".
Two gaps in the 1.9.4 entry, both found while verifying the updater against the live registry after the entry was written. A throttled Docker Hub refusal used to be read as an empty list of releases, and an empty list is indistinguishable from nothing newer being published - so the check reported "You're up to date" without having found out. That is the only user-visible change in the release and it was not mentioned at all. The entry also now says up front that Docker installs need do nothing, and points anyone running their own reverse proxy at UPGRADE.md.
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