Full-mod audit hardening: alias hot-reload, recursion guard, lifecycle state, atomic config writes#2
Merged
Merged
Conversation
High severity: - /customperm reload now applies aliases.json changes to the live dispatcher (additions, removals with shadowed-node restoration, and edited steps - the execution closure captured the step list at registration time and kept running stale steps with op-4 elevation). - Guard alias execution against recursive alias chains (self-invoking alias or cycle) with a max nesting depth of 8 instead of recursing to StackOverflowError. - Clear static dispatcher state (ORIGINAL_ROOTS, WRAPPED_NODES, SHADOWED_ORIGINALS, REGISTERED_ALIASES) on every RegisterCommandsEvent and on server stop: no more old-tree memory leaks across /reload and same-JVM restarts, and no restoration of stale shadowed nodes from a previous server instance. Medium severity: - Subscribe to RegisterCommandsEvent at EventPriority.LOWEST and run a catch-up repair() at ServerStartedEvent so commands registered by later mod handlers are still wrapped at boot. - Write config files atomically (temp file + ATOMIC_MOVE) so a crash mid-save can no longer truncate them. - Re-wrap a restored shadowed command immediately after alias removal. - Key direct command exposure on the selected backend instead of mere LuckPerms presence, so the internal fallback can gate commands when LP is present but outdated/broken. Low severity: - Strip a single leading slash in alias steps (WorldEdit-style //wand). - Emit the shadowing warning when alias addstep creates a new alias. - Trim permission nodes in grade addperm/removeperm. - Rethrow fatal JVM Errors from alias steps (D1 policy).
Collaborator
Author
|
Local validation: |
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
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
Follow-up to the LuckPerms audit (#1): a full-mod bug hunt covering the command-tree rewriter, alias engine, config manager, and lifecycle handling. This PR fixes 3 high-severity and 4 medium-severity bugs plus several small hardening items. All fixes are listed in
CHANGELOG.mdunder Unreleased.High severity
H1 -
/customperm reloaddid not apply aliases.json changesonConfigReloadonly repaired command wrappers;AliasManagerwas never invoked. Aliases added to the file never appeared, removed aliases stayed executable, and - worse - edited steps were ignored because the execution closure captures the step list at registration time, so a step removed for being dangerous kept running with op-4 elevation.AliasManager.applyConfig()now re-registers the union of configured and currently-registered aliases on reload (restoring shadowed commands for deletions), followed by arepair()pass.H2 - No recursion guard on aliases
/customperm alias add boom boom(or a cycle a -> b -> a) recursed infinitely with an op-4 elevated source untilStackOverflowError, which was then swallowed level-by-level by thecatch (Throwable)in the step loop, spamming thousands of failure messages. Alias execution now tracks nesting depth in aThreadLocaland aborts beyond depth 8 with a clear message.H3 - Static dispatcher state leaked across server lifecycles
ORIGINAL_ROOTS/WRAPPED_NODES(CommandTreeRewriter) andSHADOWED_ORIGINALS/REGISTERED_ALIASES(AliasManager) were never cleared. Consequences: old command trees retained in memory after every/reloadand embedded restart, and - after a same-JVM server restart - removing a shadowing alias restored the stale node from the previous server instance. State is now cleared at the top of everyRegisterCommandsEvent(fresh dispatcher) and onServerStoppedEvent.Medium severity
ServerStartedEventdid no catch-up. Mods registering commands after CustomPerm were never wrapped at boot. Now:EventPriority.LOWEST+ arepair()safety net at server start.save()used directFiles.writeString(truncate-then-write); a crash mid-save corrupted the file - andload()callssave(), so every reload was a corruption window. Writes now go through temp file +ATOMIC_MOVE(with non-atomic move fallback).refreshAliasnow runsrepair().luckPermsFallbackMode=internal, the internal backend was active yet exposure stayed disabled, making grade command nodes useless.isDirectCommandExposureEnabled()now checks whether the LP backend was actually selected. (CI/no-LP and LP-active behaviours are unchanged.)Low severity
//wandsteps now work).alias addstepemits the same shadowing warning asalias addwhen it creates the alias.grade addperm|removepermare trimmed.Errors thrown by alias steps are rethrown (matches the D1 policy already applied inLuckPermsService).Known issues documented but intentionally not fixed
/tpvs/teleportexposure remains asymmetric.word()argument type prevents debugging/exposing namespaced command roots containing:.load()reports failure when the post-loadsave()fails even though the snapshot was applied.Validation
./gradlew compileJava compileGameTestJava test- passed../gradlew runGameTestServer- see PR checks / comment below.Related: #1 (LuckPerms-specific audit). Note for the merge: both PRs add a
ServerStoppedEventlistener inCustomPerm.javaand an Unreleased CHANGELOG section - trivial conflicts expected whichever lands second.