Skip to content

Operations API: moderation, worlds and backups over HTTP - #91

Merged
CaYatur merged 2 commits into
mainfrom
feat/api-ops
Jul 28, 2026
Merged

Operations API: moderation, worlds and backups over HTTP#91
CaYatur merged 2 commits into
mainfrom
feat/api-ops

Conversation

@CaYatur

@CaYatur CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Part of #53 — the three operation groups the issue lists first. Files, server
config, mods and Java stay IPC-only and are named as such in the docs, so the
issue stays open for part 2.

New reference doc: docs/api-operations.md.

The real risk in this slice: command injection

When a server is running, moderation is console commands, and sendCommand
writes the string plus a newline to the server's stdin:

mp.child.stdin.write(command.endsWith('\n') ? command : command + '\n')

So POST /players/op { player: "Steve\nstop" } would be two commands, the second
running as the server operator. The desktop app and the web panel are safe by
accident
— they pass names from a roster the server itself reported. An HTTP
caller passes whatever it likes.

shared/ops.ts validates before anything runs:

  • player against an allowlist — ^[A-Za-z0-9_]{3,16}$
  • a free-text reason stripped of every control character (C0, DEL, NEL,
    U+2028/U+2029) and capped

Allowlist rather than denylist on the name because a denylist of "characters that
break a command" has to be right about all of them and only has to be wrong once.
The reason genuinely is free text — "griefing spawn, 3rd warning" is a real ban
reason — so there it is a strip, not a reject.

World names get the path-component treatment: no separators, no ./.., no
drive letters, no Windows reserved device names.

A new worlds scope

Not folded into players or settings. Deleting or resetting a world destroys
data that no backup outside MSMS knows about, and an integration that only needs
to read the world list should not have to be trusted with erasing one. Of the
built-in roles only Operator carries it.

Confirmation on the four destructive calls

backup.restore, backup.delete, world.delete, world.reset need
confirm: true (or ?confirm=true on a DELETE) on top of the scope.

Explicitly not a security boundary — a caller with the scope can always pass
the flag. It is there because these are the calls an integration makes by
accident: a retry loop, a mis-set variable, an example copied without reading it.

Deliberately not exposed

  • World export/import — both take a local filesystem path, and a zip
    upload/download is a different shape of endpoint than the rest of this surface.
    Better tracked separately than half-done here.
  • BackupOptions.destDir — an arbitrary filesystem path. A backups-scoped
    caller able to write a zip anywhere on the host is a different privilege from
    being able to back up a world.

Endpoints

GET  /players                                  view
POST /players/{op,deop,ban,pardon,kick,
      whitelist-add,whitelist-remove,gamemode} players

GET    /worlds                                 view
POST   /worlds/{activate,rename,clone,reset}   worlds
DELETE /worlds?name=&confirm=true              worlds

GET    /backups                                view
POST   /backups                                backups
POST   /backups/restore                        backups
DELETE /backups?backupId=&confirm=true         backups

Own path matcher rather than widening the existing single-segment one: that regex
is (?:\/(\w+))?$, so a nested path silently falls through to the generic 404 —
the trap already documented on the alert routes — and widening it would also have
swallowed the /store/... block.

Verification

All twelve smoke gates pass.

Asserted: the pure validators against injection and traversal inputs; a key
scoped to one group refused on the other two; a newline in a player name refused
and the refusal audited; an unknown action 404; offline op reporting 409
rather than pretending to work; an offline whitelist-add actually reaching
whitelist.json by uuid; a real clone-then-delete world round trip; each
destructive call refused without its confirmation; a backup id from elsewhere
404; and every operation entry attributed to the key with its server.

Three bugs the smoke caught, all mine

  1. The world validator trimmed before applying its own trailing-space rule, so
    that rule could never fire — and the route then used the untrimmed name, which
    is the exact hazard the rule was written for. It no longer trims.
  2. World DELETE read confirm from a body a DELETE does not have, so
    ?confirm=true was ignored and world deletion was unreachable.
  3. The world assertions were wrapped in if (worldNames.length) — on a fixture
    with no world they silently tested nothing. The gate now seeds a world and
    fails if it does not register as one.

Part of #53. Covers the three groups the issue lists first; files, server
config, mods and Java remain IPC-only and are named as such in the docs.

Thin HTTP handlers over the existing core/* functions, the pattern the panel
already proves. Own path matcher rather than widening the single-segment one:
that regex is (?:\/(\w+))?$, so a nested path silently falls through to the
generic 404 - the trap already documented on the alert routes - and widening it
would also have swallowed the /store/... block.

**A new `worlds` scope**, not folded into `players` or `settings`. Deleting or
resetting a world destroys data no backup outside MSMS knows about, and an
integration that only needs to read the world list should not have to be trusted
with erasing one. Of the built-in roles only Operator carries it.

**Command injection was the real risk here.** When a server is running,
moderation is console commands, and sendCommand writes the string plus a newline
to stdin - so a player name of "Steve\nstop" is two commands, the second running
as the server operator. The desktop and the panel are safe by accident: they
pass names from a roster the server itself reported. An HTTP caller passes
whatever it likes. shared/ops.ts validates a name against an allowlist
(^[A-Za-z0-9_]{3,16}$) and strips every control character from a free-text
reason, before anything runs. A denylist of "characters that break a command"
has to be right about all of them and only has to be wrong once.

World names get the same treatment as path components: no separators, no
dot-segments, no drive letters, no Windows reserved device names. Checked
untrimmed on purpose - Windows drops a trailing dot or space, so `world ` and
`world` name the same directory while looking like different worlds, and
trimming inside the validator would both accept one string while using another
and make the trailing-space rule unable to fire.

**Confirmation on the four destructive calls** (backup restore/delete, world
delete/reset). Not a security boundary - a caller with the scope can always pass
the flag - but these are the calls an integration makes by accident, and a retry
loop should not be able to erase a world.

Two things deliberately not exposed: world export/import, because both take a
local filesystem path and a zip upload is a different shape of endpoint; and
BackupOptions.destDir, because a backups-scoped caller writing a zip anywhere on
the host is a different privilege from backing up a world.

Every call is audited, refusals included, attributed to the session username or
key:<label> with the server it acted on.

Verified with all twelve smoke gates. New coverage: the pure validators against
injection and traversal inputs, a key scoped to one group refused on the others,
a newline in a player name refused with the refusal audited, an unknown action
404, offline op reporting a conflict, an offline whitelist-add actually reaching
whitelist.json, a real clone-then-delete round trip, and each destructive call
refused without its confirmation.

Three bugs found by the smoke while writing it, all mine: the world validator
trimmed before applying its own trailing-space rule so that rule could never
fire; the world DELETE read `confirm` from a body that a DELETE does not have,
making world deletion unreachable; and the world assertions were wrapped in
`if (worldNames.length)`, so on a fixture with no world they silently tested
nothing - the gate now seeds one and fails if it does not register.
Copilot AI review requested due to automatic review settings July 28, 2026 07:06
@CaYatur CaYatur added enhancement New feature or request area:api External integration API (REST/WebSocket) labels Jul 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

**restoreBackup could run against a running server.** core/worlds.ts calls
assertStopped() before every destructive operation; restoreBackup never did.
Extracting over a live world corrupts it - the server holds region files open
and writes its in-memory state back on its own schedule, so a restore part-way
through leaves a mix of old and new chunks and is then overwritten by the save.

The only thing standing in the way was a sentence in the desktop dialog: "This
overwrites current files with the backup. Stop the server first." Advice, not a
check - and an API caller never reads it. This PR made restore reachable over
HTTP, which turns advisory into nothing at all.

Guarded in core, where the other destructive ops guard, so both surfaces get it
and the route's existing error mapping already answers 409. The desktop's advice
is now enforced rather than suggested.

Proved in MSMS_SMOKE, which is the only gate with a genuinely running server:
create a backup, attempt a restore, require 'server-running'. That sits next to
the existing world-delete guard assertion, which exists for the same reason.

**A dead route.** The ops matcher had its own `GET /players`, but
/api/servers/:id/players is a single path segment, so the matcher above claims it
and returns first. A second handler that never runs is worse than no handler: it
reads like the authoritative one.

All twelve smoke gates pass.
@CaYatur

CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Self review

Two findings, one of them a data-loss hazard this PR would have widened. Both
fixed in d21c71b.

1. restoreBackup had no running-server guard

core/worlds.ts calls assertStopped() before every destructive operation:

function assertStopped(id: string): void {
  // Windows keeps handles open on a running world, and a delete part-way
  // through a region write corrupts what survives.
  if (processManager.isRunning(id)) throw new Error('server-running')
}

restoreBackup never did. It goes straight to zip.extractAllTo(server.path, true) — over a live world. The server holds region files open and writes its
in-memory state back on its own schedule, so a restore part-way through leaves a
mix of old and new chunks and is then overwritten by the next save.

The only thing standing in the way was a sentence in the desktop dialog:

restoreBody: 'This overwrites current files with the backup. Stop the server first.'

Advice, not a check. And this PR made restore reachable over HTTP, where nobody
reads the dialog — so advisory became nothing at all. That is the part that makes
it mine rather than pre-existing.

Guarded in core, next to the other destructive ops, so both surfaces get it
and the route's existing error mapping already answers 409. The desktop's
advice is now enforced instead of suggested — a behaviour change, but in exactly
the direction its own UI text asks for.

Proved in MSMS_SMOKE, the only gate with a genuinely running server: create a
backup, attempt a restore, require server-running. It sits directly beside the
existing world-delete guard assertion, which exists for the same reason.

2. A dead route

The ops matcher had its own GET /players. But /api/servers/:id/players is a
single path segment, so the matcher above claims it and returns first — my
handler could never run.

Removed. A handler that never executes is worse than no handler: it reads like
the authoritative one, and the next person to change the roster response would
change the wrong copy.

(Checked the other two: /worlds and /backups have no handler in that block,
and it falls through without returning, so those do reach the ops matcher. That
fallthrough is load-bearing and now has a comment saying so.)

Reviewed and deliberately left alone

  • getPlayers(id) is called on every moderation request to resolve the
    roster entry. It reads a few small json files; the alternative is a cache that
    can be stale about who is opped, which is worse than the read.
  • The generic api.post entry now duplicates the specific operation entry
    for key-driven calls. Kept: it is the net for the many routes that still do not
    audit themselves, and it records the path rather than the semantics, so it is
    not redundant so much as coarser. The smoke asserts both are present.
  • confirmed() audits with an empty target on a missing confirmation. The
    name is in the request but not yet validated at that point in the world path;
    recording an unvalidated string as a target is worse than recording none.
  • Moderation on a stopped server needs a uuid and so fails for a player who
    has never joined. That is core/players.ts behaviour, not something to paper
    over in a route — reported as 409 with the real reason so the caller can act
    (start the server, or use a name that has joined).

Verification

All twelve smoke gates pass, including the new restore-guard assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:api External integration API (REST/WebSocket) enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants