Files and server config over HTTP

[CaYatur]

#53 part 2. Files, server.properties, Java args and favourite. Mods, Java
install, metrics config and server creation stay IPC-only and are named as such
in the docs, so #53 remains open for part 3.

Docs extended: docs/api-operations.md.

Reading files needs files, not view

server.properties holds the RCON password, and whatever else an operator has
pasted into a config. A status glance should not include that.

What the traversal guard does and does not cover

core/serverFiles.ts already refuses to leave the server root — every entry
point runs the path through the same safe() check — so a traversal comes back
400 path-escape without reading anything. Asserted for ../../secrets,
..\..\secrets and /etc/passwd.

What it does not do is stop a caller reaching the files that decide what
runs. Replacing server.jar is code execution on the next start; eula.txt and
server.properties change what the server is.

That is not a reason to block them — an operator edits these constantly, and a
file editor that refuses to edit the interesting files is not a file editor. It
is the reason files is its own scope rather than part of settings, and the
reason every write is audited with its path.

Deleting requires ?confirm=true. Nothing inside MSMS brings the file back.

Properties: newlines are the whole risk

A property value containing a newline smuggles in a second key —
motd=hi\nmax-players=999 would quietly change the player cap. Refused with
400 newline-in-value, and the refusal is audited.

updates merges, so a targeted write leaves the rest of the file alone (asserted:
writing motd does not disturb enable-rcon). raw replaces it wholesale for
callers that want that.

A real pre-existing bug, exposed by the first partial patch anyone sent it

Object.assign(s, patch, { id: s.id })
if (patch.java) s.java = { ...s.java, ...patch.java }

Object.assign has already replaced s.java with the patch's partial by the
time the merge line runs. So the spread merges the partial into itself and drops
every key the caller did not mention:

result: {"maxMemoryMB":3072}   <- preset, flags, jarFile all gone

Invisible for as long as the only caller was the desktop args editor, which
always sends a complete config. POST /config/java sends a partial, and the
preset vanished. Fixed by capturing s.java before the assign — the Java patch
now merges, which is what the API documents.

Two hardenings from the same failure

buildJvmFlags crashed on an incomplete config. cfg.extraFlags.trim() on a
hand-editable JSON file throws undefined.trim() from inside start(), which
surfaces to the user as "the server never started" with no hint why. Defaulted.

The smoke could poison its siblings. The failed assertion that found the
merge bug returned before its inline java restore, leaving the shared dev-root
fixture with a partial config — and then every other gate failed to start a
server for reasons unrelated to what it tests. The restore is in finally now,
with a properties snapshot beside it. A test that can damage the fixture for its
siblings is worse than the bug it caught.

This is the second time that shape has bitten this suite (the first is recorded
in the setr_t/nosee_t leftovers fix), which is why it got a structural fix
rather than a one-line restore.

Verification

All twelve gates pass — with the WEB gate deliberately run first in the loop
to prove it no longer leaves the fixture damaged for the ones after it, and the
fixture's java config verified intact afterwards.

New coverage: the scope split both ways (a settings key cannot read files, a
files key cannot read config), traversal refused, write→read-back→delete with
the delete gated on confirmation, the write audited with its path, a newline in a
property value refused and audited, a targeted property write not disturbing
another key, and a partial Java patch keeping both the preset and extraFlags —
the second because losing that key is what broke the launch.

[Copilot]

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

[CaYatur]

Self review

One finding, and it is the sharpest in this PR: the change turned the
settings scope into arbitrary code execution. Fixed in 55f7cd3.

POST /config/java accepted the fields that decide what binary runs

const b = (await readBody(req)) as Partial<JavaArgsConfig>
const updated = registry.updateServer(id, { java: b as JavaArgsConfig })

Three of those fields are not settings, they are the command line:

// processManager.start()
const javaInfo = await detectJava(server.java.javaPath || getConfig().defaults.javaPath)
const javaBin  = javaInfo?.path ?? javaExecutable(server.java.javaPath)

• javaPath is spawned as the process binary. Point it anywhere and the next
  server start runs that instead of Java.
• customArgs is the whole command line when preset is custom.
• extraFlags is appended to the real one.

So a settings-scoped API key — a scope whose UI label is literally "Edit
settings" — was remote code execution as the MSMS process, waiting for the next
restart.

Over IPC this was never a problem, which is exactly why it existed. There the
caller is the operator sitting at the machine, who already has full filesystem
access; offering them a text box grants nothing they did not have. What my PR did
was expose the same fields to a remote scope, and that is the whole difference.
It is worth naming that shape because it is going to recur: "safe because the
caller is local" stops being true the moment an HTTP route reuses the function.

Now refused with 403 local-only-field, audited, and still editable in the
desktop app.

Two deliberate details:

• A mixed patch is refused whole. { minMemoryMB: 512, javaPath: "..." }
  applies neither. A caller should not have to work out which half of their
  request landed.
• Denylist, not an allowlist of safe fields. The safe set is memory numbers
  and booleans that will grow; a new safe field being accidentally blocked is an
  annoyance, a new dangerous one being accidentally allowed is this bug again.

Checked and clean

• Large-file reads are not a DoS. readTextFile returns
  { content: '', binary: true } past MAX_TEXT before reading, so a caller
  cannot pull a multi-gigabyte region file into a JSON response.
• Writes are bounded by readBody's 256 KB cap.
• config GET returns the absolute server path to a settings-scoped
  caller. Left in: it is the same string the desktop shows, and every operation
  in this surface is already scoped to that one server.

Reviewed and deliberately left alone

• files can replace server.jar. Not blockable without making the file
  editor useless — an operator edits jars, eula.txt and server.properties
  constantly. It is why files is its own scope and why every write is audited
  with its path.
• Reading needs files rather than view. Deliberately stricter:
  server.properties holds the RCON password.
• POST /config/properties accepts raw, replacing the file wholesale. Same
  reasoning as file writes, and the newline check would be pointless there — the
  caller is explicitly supplying a whole file.

Verification

All twelve gates, WEB run first to prove it leaves the fixture intact.