Skip to content

fix: address the findings from a full-repo adversarial review - #519

Merged
thomasmny merged 12 commits into
masterfrom
fix/gauntlet-extreme-findings
Jul 30, 2026
Merged

fix: address the findings from a full-repo adversarial review#519
thomasmny merged 12 commits into
masterfrom
fix/gauntlet-extreme-findings

Conversation

@thomasmny

@thomasmny thomasmny commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Addresses the findings from a full-repo adversarial review (16 lenses, ~42 raw findings, refutation pass over the highest-stakes ones).

Heads-up on scope: this is large. Every finding that survived review is fixed here, plus the framework change and readability work that came out of it. Commits are kept separable so anything can be dropped or cherry-picked.

Data safety

  • Restore no longer destroys the world before validating the archive. BackupProfileImpl.applyRestore unloaded the world, recursively deleted its directory, and only then opened the zip — so any unreadable archive (truncated upload, disk error, hand-edited file) left nothing to restore and no rollback. Validation now happens first, via a new validateBackup. A failed delete also aborts instead of extracting over a half-deleted world.
  • SFTP uploads can no longer leave a truncated archive posing as the newest backup. storeBackup wrote directly to the final name with no failure handling; because the filename timestamp is assigned before the write, a partial .zip sorted first and BackupsMenu shows no size or integrity hint. Now writes to a temp name and renames on success, so a partial upload can never occupy the real filename.
  • Auto-backup failures are no longer silent. The scheduled driver discarded createBackup()'s future entirely — no whenComplete, no exceptionally — so on the default path a failure produced no log line at all. Only the manually-triggered helper logged.
  • S3Client.get cleans up a partially-written file when the transfer throws mid-stream (BodyHandlers.ofFile has already created and written it, so the existing cleanup branch was skipped).
  • YamlStore.save is now crash-atomic — temp file plus ATOMIC_MOVE, falling back to a non-atomic replace where the filesystem refuses. Previously an in-place truncating write; a crash mid-write left a truncated file that reload() logged and then proceeded past, silently losing world/player metadata.
  • AbstractYamlStorage routes through the ioLock. spawn.yml, setup.yml, status.yml and categories.yml bypassed the lock the cache-backed storages use, so two threads could write the same file.
  • onDisable cancels the periodic save before joining the shutdown save, not after — cancelling can't stop a tick already in flight.
  • Config migration backs up config.yml before mutating it, matching what the storage layer already did deliberately, and aborts rather than migrating unprotected if the copy fails.
  • createBackup() no longer calls World::save() on the caller's thread. It is public API returning a CompletableFuture, so an off-thread caller tripped the async catcher on their own thread instead of getting a failed future.
  • Credential-holding config records redact password/secretKey in toString(), so a future logger.info("settings: " + settings) can't leak them.

Authorization

  • Service layer no longer trusts the GUI. WorldServiceImpl.deleteWorld(Player, ...) had no permission check at all — the menu was the only enforcement point. The no-Player overload stays unchecked for internal/API callers.
  • Status assignment is world-scoped. Every other resource action follows X/X.self/X.other; the menu path gated only on flat nodes, so a moderator with edit.other + edit.status + setstatus.finished could set status on anyone's world while /worlds setstatus correctly refused them.
  • Authorization is no longer read back from the rendered icon. EditMenu's builders/visibility handlers and BuilderMenu's add button decided access by comparing the clicked item's Material to what was drawn at populate time. Remove-builder had no check at all.
  • The GUI add-builder path is gated like the command path — it called AddBuilderSubCommand.getAddBuilderInput directly, skipping the only place canPerformCommand ran.
  • Registries no longer resurrect deleted entries: persist() wrote unconditionally, so a stale open editor rewrote a status/category another admin had deleted.

New: MenuButton.usableBy / canClick

permission() could only express a single node, so every resource-scoped or creator-based check became a hand-rolled early return inside onClick — which is how the icon-material bug happened. Buttons can now declare usableBy(Predicate<Player>) alongside the node, and canClick(Player) is the single place the two combine and the only thing handleClick consults. The node stays introspectable, so the golden permissionBySlot() tests are unaffected.

Permissions

buildsystem.create.<categoryId> → buildsystem.create.category.<categoryId> and buildsystem.navigator.<categoryId> → buildsystem.navigator.category.<categoryId>. The old forms collided with the reserved create.folder / create.private / create.type.* / create.template.* / create.generator.* and navigator.item nodes whenever a category id matched one of those words — and with the create.<visibility>.<amount> world-limit nodes. Not a migration concern: v4.0.0 is unreleased (4.0.0-SNAPSHOT, latest tag 3.0.2), so these never shipped.

Also deleted the dead Bypassable capability — attached to three properties, read by nothing, with real enforcement re-typing the same literals elsewhere — plus the buildsystem.bypass.permission node it carried, which granted nothing. Permissions' class javadoc no longer claims the full node set is readable in one place; that was false for the runtime factory methods and is why the collision went unnoticed.

Correctness and performance

  • UpdateChecker: HttpURLConnection with no timeouts and a reader closed only on the happy path → HttpClient with an explicit 5s timeout, matching PlayerLookupService and S3Client. Its continuation ran on a ForkJoinPool thread and called player.sendMessage off the main thread, on every player join; now hops to the main thread and guards the exceptional path (it dereferenced result without checking the throwable).
  • buildModePlayers is a ConcurrentHashMap.newKeySet() — a plain HashSet mutated on the main thread but read by BuildModeCalculator, which LuckPerms invokes from arbitrary threads.
  • evaluateModify (hot path, every block break/place/interact) evaluated a LuckPerms-routed hasPermission before a Set.contains, and a bypass-permission lookup before the map read whose default makes it moot.
  • DisplayablesMenu no longer re-runs the full filter+sort on every page flip (~1,200 permission lookups per populate at 300 worlds); data collection moved to refreshData(), page arrows re-render the cached slice.
  • Auto-backup staggers to one world per tick instead of firing every due world's World.save() in a single tick.
  • ButtonMenu.register throws on a slot collision within a population pass. EditMenu draws its 54 slots from two independent catalogs, and a collision silently deleted a button.
  • zipDirectoryToMemory uses zip4j instead of hand-rolling a second zip codec beside the zip4j one in the same file.
  • /worlds setPermission and setProject warn when a folder override means the saved value is not the one in effect — previously they reported success unconditionally while enforcement kept using the folder's value.
  • newWorld's name validation now also applies to importWorld, which shared the same shape and enforced nothing.

Tests

418 → 421, and skipped 1 → 0.

  • PromptFlowTest's only test was skipped on a MockBukkit gap, so the reject → reprompt → complete contract had zero executed assertions while counting toward a green suite. It now runs.
  • New BackupProfileImplTest pins the retention boundaries (at-cap, under-cap, cap-lowered) with shuffled insertion order so a "delete first N" regression fails. The math was verified correct — this guards it, it does not fix it.
  • BuildSystemPluginSingletonGuardTest → BuildSystemPluginNoStaticSelfReferenceTest: it never enables the plugin, so the old name promised runtime double-enable protection it cannot give.

Readability

EditMenu 728 → 551 lines, with async head resolution moved into MenuItems (which already owned that concern) and the non-authorization render helpers extracted to EditMenuRenderer. The builders/visibility renders deliberately stayed in EditMenu so they cannot drift from the predicates that gate them.

Caveat worth reading

Of the review's findings, 24 were never adversarially verified — and every finding that was verified got downgraded or refuted outright (one critical → high, four high TOCTOUs → two mediums, one refuted entirely). The unverified fixes are implemented conservatively, but some may be addressing problems that aren't real.

Docs for the permission changes are a separate PR in buildsystem-docs (v4).

thomasmny added 12 commits July 30, 2026 09:48
@thomasmny
fix: stop a corrupt backup from destroying the world it restores over
ed59985 
applyRestore unloaded the world and recursively deleted its directory
before extractBackup ever opened the archive, so any unreadable zip left
nothing to restore and no rollback. Validation now runs first, and a
failed delete aborts instead of extracting over a half-deleted world.

The SFTP upload wrote straight to the final name with no failure
handling. Because the filename timestamp is assigned before the write, a
truncated archive sorted as the newest backup and the menu shows no size
or integrity hint. It now writes to a temp name and renames on success.

The scheduled auto-backup discarded createBackup()'s future entirely, so
a failure on the default path produced no log line at all. Backups also
stagger one world per tick rather than firing every due world's save in
the same tick, and createBackup() no longer runs World::save on the
caller's thread now that it is public API returning a future.

S3Client.get deletes the partially written target when the transfer
itself throws, not only when S3 returns a failure status.
@thomasmny
fix: make on-disk writes survive a crash and an interrupted migration
e21958e 
YamlStore.save truncated and rewrote in place, so a crash mid-write left
a truncated file that reload() logged and then proceeded past, silently
losing world and player metadata. It now writes a temp file and moves it
atomically, falling back to a plain replace where the filesystem refuses.

AbstractYamlStorage called reload/save directly, bypassing the ioLock the
cache-backed storages use, so spawn.yml, setup.yml, status.yml and
categories.yml could be written by two threads at once. onDisable also
cancelled the periodic save only after joining the shutdown save, which
cannot stop a tick already in flight.

Config migration rewrote config.yml with no backup, unlike the storage
layer that deliberately backs up first. It now copies the file aside and
aborts rather than migrating unprotected if that copy fails.

The SFTP and S3 settings records redact their secrets in toString(), so a
future log of the record cannot leak plaintext credentials.
@thomasmny
feat: let a menu button declare resource-scoped access, not just a node
5a73508 
permission() can only express a single permission node, so every check
that depends on the resource being acted on -- whether the player created
this world, whether they are under their world limit -- became a
hand-rolled early return inside onClick. That is how authorization came to
be decided by comparing a clicked item's Material to what was drawn at
populate time.

Buttons can now declare usableBy(Predicate<Player>) alongside the node.
canClick(Player) is the single place the two combine and the only thing
handleClick consults, so declaring either is enough to have it enforced.
The node stays introspectable, so the golden permissionBySlot tests are
unaffected.

register() also throws on a slot collision within one population pass.
EditMenu draws its 54 slots from two independent catalogs and a collision
silently deleted whichever button registered second.
@thomasmny
fix: enforce authorization below the GUI, not only inside it
6cbe310 
WorldServiceImpl.deleteWorld(Player, ...) checked existence and path
safety but never creator, admin or any permission, so the menu was the
only enforcement point for an irreversible action. It now applies the
same gate the command path uses; the no-Player overload stays unchecked
for internal and API callers, and says why.

The GUI add-builder flow called AddBuilderSubCommand.getAddBuilderInput
directly, skipping execute() and with it the only canPerformCommand call
in that path. Both entry points now share one gate.

newWorld validated names against reserved and path-escaping forms while
importWorld, four lines below and with the same shape, validated nothing
-- despite a comment that reads as class-wide policy. Both now call one
helper. This makes importWorld reject names it previously accepted.

The registries persisted unconditionally, so an editor left open on an
entry another admin deleted wrote it back to disk on save.

startWorldNameInput's two adjacent booleans are now a record; the
compiler would previously have accepted them transposed.
@thomasmny
fix: decide menu access from the predicate, not the rendered icon
e2b3c7e 
EditMenu's builders and visibility handlers, and BuilderMenu's add
button, inferred whether the player was allowed by comparing the clicked
item's Material to what was drawn at populate time. Remove-builder had no
check at all. Status assignment gated only on flat nodes, so a moderator
with edit.other, edit.status and setstatus.finished could set status on
anyone's world while the equivalent command correctly refused them.

These now declare permission() and usableBy() and let the framework
enforce them. BuilderMenu and CreateMenu override onPermissionDenied to
keep their existing outcomes -- return to the editor, and play the deny
sound without ejecting the player from the creation flow.

EditMenu also drops from 728 to 551 lines: the async head resolution
moved into MenuItems, which already owned that concern and no longer
needed a TaskScheduler and Logger threaded through EditMenu's
constructor, and the render helpers that carry no authorization moved to
EditMenuRenderer. The builders and visibility renders stayed put so they
cannot drift from the predicates that gate them.
@thomasmny
fix!: namespace category permission nodes and delete the dead Bypassable
2d06f01 
buildsystem.create.<categoryId> and buildsystem.navigator.<categoryId>
collided with the reserved create.folder, create.private, create.type.*,
create.template.*, create.generator.* and navigator.item nodes whenever a
category id matched one of those words, and with the
create.<visibility>.<amount> world-limit nodes. They are now
create.category.<id> and navigator.category.<id>, so a collision is
structurally impossible.

This is not a migration concern: 4.0.0 is unreleased, so neither form has
ever shipped.

Bypassable was attached to three properties and read by nothing --
getCapability is only ever called for Overridable -- while the real
enforcement re-typed the same permission literals elsewhere. Removed,
along with the buildsystem.bypass.permission node it carried, which
granted nothing. WorldSetting keeps its bypass literal in one place.

Permissions' class javadoc no longer claims the full node set is readable
in one place. That was false for the runtime factory methods, and is why
the collision above went unnoticed.
@thomasmny
fix: give the update check timeouts and keep Bukkit calls on the main…
66458f3 
… thread

UpdateChecker used HttpURLConnection with no connect or read timeout and
closed its reader only on the happy path, and was the only caller still on
that API. It now uses HttpClient with an explicit timeout, matching
PlayerLookupService and S3Client.

Its continuation ran on whichever ForkJoinPool thread completed the
future and then called player.sendMessage -- on every player join, since
the checker is enabled by default. It now hops to the main thread and
guards the exceptional path, which previously dereferenced the result
without checking the throwable. The message is built from Placeholders
rather than chained replace calls.

buildModePlayers was a plain HashSet mutated on the main thread but read
by BuildModeCalculator, which LuckPerms invokes from arbitrary threads; a
contains() racing an add() resize returns a wrong build-mode context.

evaluateModify runs on every block break, place and interact. It
evaluated a LuckPerms-routed hasPermission before a Set.contains, and a
bypass-permission lookup before the map read whose default makes it moot.

zipDirectoryToMemory uses zip4j rather than hand-rolling a second zip
codec beside the zip4j one in the same file.
@thomasmny
fix: stop reporting success for a value a folder override discards
d40d941 
ConfigurableProperty.get() honours an active Overridable provider while
set() only writes the underlying field, so /worlds setPermission and
setProject on a world inside an override-enabled folder reported success,
stored the value, and changed nothing that was enforced or displayed. The
world then silently changed to the operator's months-old value whenever
the override was later disabled. Both commands now say when the saved
value is not the one in effect.

DisplayablesMenu rebuilt cachedDisplayables on every populate, and the
page arrows call populate directly -- so each page flip re-ran the folder
filter, the per-world canEnter checks and the sort over every world. At
300 worlds in one category that is roughly 1,200 permission lookups per
flip, on the main thread inside the click handler. Collection moved to
refreshData(), called from open(); flipping a page re-renders the cached
slice.

The sort and filter controls are grouped into DisplayBar, and the two
create-button click paths share one method that documents the
player-head-versus-glass invariant they both depend on. CategoryWorldsMenu
stops redeclaring three slot literals the base class already owns.
@thomasmny
test: cover retention selection and make the prompt-flow test actuall…
1e5770c 
…y run

PromptFlowTest's only test was skipped on a MockBukkit gap, so the
reject-then-reprompt-then-complete contract it documents had zero
executed assertions while still counting toward a green suite. It now
drives the listener directly and runs.

BackupProfileImplTest is new. deleteExcess decides which archives are
deleted when a world exceeds its cap and had no test at all. The
arithmetic and comparator were verified correct, so this pins them rather
than fixing anything; insertion order is deliberately shuffled so a
"delete the first N" regression fails.

BuildSystemPluginSingletonGuardTest is renamed to
BuildSystemPluginNoStaticSelfReferenceTest. It never loads or enables the
plugin -- it is a bytecode guard against reintroducing a static singleton
accessor -- so the old name promised runtime double-enable protection it
cannot give. Assertions are unchanged.
@thomasmny
fix!: drop the unchecked buildsystem.create.private node
5255162 
It was declared in plugin.yml as a child of buildsystem.create with
default TRUE, but no hasPermission call anywhere referenced it, so
operators granting it to allow private world creation got nothing. The
node that actually gates this is buildsystem.create.category.private.

Registers buildsystem.create.category in its place, defaulted to OP to
match how its dynamic children resolve. Category ids come from the
registry at runtime so the individual nodes cannot be declared here; the
parent exists for discoverability, in the same spirit as the existing
create.template and create.generator entries.
@thomasmny
fix: enforce the world-count limit where worlds are created
8ab068d 
canCreateWorld was consulted in exactly two places and both were render
decisions: whether to draw the create button in CategoryWorldsMenu, and
which icon the visibility toggle shows in EditMenu. Nothing on the path
that actually creates a world ever asked, so the configured limit bound
only the GUI that happened to be the entry point and no other caller
would inherit it.

Checked in startWorldNameInput, the single funnel all three creation
flows pass through. Deliberately not checking createType here: template
and custom-generator creation are gated by create.template.<name> and
create.generator.<name>, so a blanket create.type.<name> check would
reject them.
@thomasmny
fix: make world.limits.* the per-player default it was always meant t…
3220576 
…o be

The key descends from world.default.settings.public-worlds/private-worlds,
renamed by MigrationV2ToV3 -- a per-player default, not a server capacity
cap. The implementation had drifted from that: it selected the limit by
visibility but compared it against the count of every build world on the
server, so world.limits.private: 5 stopped all world creation once the
server held five worlds of any kind, silently capping public worlds too.
An operator could not discover that without reading the source.

canCreateWorld now reads one ladder: the
buildsystem.create.<visibility>.<amount> node if the player holds one,
otherwise the configured default, otherwise unlimited. Counting is per
player and per visibility throughout, so a limit on one visibility no
longer blocks the other.

buildsystem.admin short-circuits first. The resolver returns -1 both for
"admin, unlimited" and for "no node held", so without that an admin would
inherit the configured fallback -- which a new test caught. This is a
behavior change from the old code, where the config cap applied to admins
too, and it matches how buildsystem.admin behaves everywhere else.

Out-of-the-box behavior is unchanged: the shipped default is -1/-1.
@thomasmny
thomasmny force-pushed the fix/gauntlet-extreme-findings branch from 3b8009d to 3220576 Compare July 30, 2026 08:31
@thomasmny
thomasmny merged commit a88f1e7 into master Jul 30, 2026
2 checks passed
@thomasmny
thomasmny deleted the fix/gauntlet-extreme-findings branch July 30, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@thomasmny