fix: audit findings#406
Open
Jakubk15 wants to merge 16 commits into
Open
Conversation
PlaceBlockBlocker was subscribed twice, causing every BlockPlaceEvent to be handled twice and the "block placing blocked" notice to be sent twice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FightTask exited its entire run() on the first expired tag, so with more than one player in combat the remaining players were skipped every tick: combat-timer notifications became unreliable and untagging was delayed to at most one expiry per second. Use continue to process all players. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When headDropOnlyInCombat was true and the victim was not in combat, the guard fell through to the chance roll and heads could still drop. Also the in-combat case short-circuited to true, ignoring headDropChance. Now the setting blocks out-of-combat drops and the configured chance always applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FightUntagEvent fires on death, logout and admin untag, where the player may be offline and server.getPlayer returns null, causing an NPE. Remove the trident delay by UUID regardless and only reset the cooldown when the player is online. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A FightTagEvent can carry an offline player's UUID (e.g. an attacker tagged via crystal PvP who already disconnected). Throwing IllegalStateException logged a stack trace on every such tag. Return early instead, consistent with FlyingBlocker.onTag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The recurring particle task read entity state (isDead/isValid/getLocation) and called world.spawnParticle from an async scheduler thread, which is not thread-safe on Paper and throws on Folia. Use the sync scheduler instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Inventory#addItem returns items that did not fit, which was discarded, so kept items were silently destroyed when the respawn inventory was full. Drop any leftovers at the player's location. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Index off the working copy instead of the original list, guard the empty case, clamp the delete count to the total available amount (so a misconfig > 100% cannot spin forever), always remove at least one item per iteration to guarantee progress, and drop drained stacks so no zero-amount ItemStacks leak back into the drop list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isInCombat did containsKey followed by get; a concurrent untag (reachable from async PlaceholderAPI calls and the async reload command) between the two calls returned null and threw NPE. Use a single get. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Blacklisted commands could be run in combat via a namespaced form (/minecraft:tp) or with leading whitespace, since matching was a raw prefix check on the message. Normalize the label by trimming and stripping the namespace prefix before matching. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tagOuts was a plain HashMap that never removed expired entries, growing unbounded over long uptime. Use a Caffeine cache with per-entry expiry tied to the tag-out end time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The (int) cast bound tighter than the division, casting the long millis to int before dividing by 50, which could overflow for very large durations. Divide first, then cast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both targets were tagged before checking cancellation, so a cancelled second tag left the first player tagged with no rollback, and the trailing combined-cancel check was unreachable. Tag the second player only after the first succeeds and untag the first if the second is cancelled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several bug fixes, robustness enhancements, and performance improvements across the combat plugin, including a self-expiring cache for tag-outs, command normalization to prevent blacklist bypasses, and safer inventory item removal. The review feedback highlights three key issues: using the correct respawn location when dropping leftover items during respawn, avoiding unintended untagging of players who were already in combat during a command rollback, and adding null checks in the inventory utility to prevent potential null pointer exceptions from empty slots.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
During PlayerRespawnEvent the player has not been teleported yet, so player.getLocation() is still the death location. Use the event's respawn location (and its world) so leftover kept items land at the player's feet. Addresses PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rollback for a cancelled second tag unconditionally untagged the first target, which would wrongly remove a player who was already legitimately in combat before the command ran. Capture the prior state and only roll back tags the command itself created. Addresses PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bukkit inventory lists can contain null elements for empty slots, which would throw NPE on getAmount(). Skip nulls when summing and drop them from the working list. Addresses PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
I made Claude audit EternalCombat's codebase and it found a couple of findings. I will verify the claims and mark this PR as ready for review when I'm certain.
Naprawione findingi
Każdy fix to osobny commit. Legenda: 🔴 wysoki · 🟠 średni · 🟡 niski.
CombatPlugin.javaPlaceBlockBlockerzarejestrowany dwukrotnie — każdeBlockPlaceEventobsługiwane 2×, powiadomienie wysyłane 2×FightTask.javareturnzamiastcontinue— pierwszy wygasły tag przerywał cały tick, przy >1 graczu w walce powiadomienia gubiły się, a untag był opóźnionyreturn→continueDropController.javaheadDropOnlyInCombatnie egzekwowane — głowy wypadały poza walką, a w walce ignorowany byłheadDropChanceTridentController.javaonUntag—server.getPlayerzwracanulldla gracza offline (śmierć/wylogowanie)KnockbackRegionController.javaonTagrzucałIllegalStateExceptiondla offline gracza (crystal PvP) — stacktrace przy każdym takim tagureturnzamiast rzucania wyjątkuDeathFlareController.javaisDead/getLocation/spawnParticle) — race na Paper, wyjątek na FoliaDropController.javaaddItemignorowana, pełny ekwipunek = zniszczone itemyInventoryUtil.javaremoveRandomItems— kruchy indeks, marnowane iteracje, ryzyko pętli przy misconfig, zerowe stacki w dropieFightManagerImpl.javaisInCombat—containsKey+getprzy współbieżnym untagu (async PAPI/reload)getCommandsBlocker.java/minecraft:tp) i wiodące spacjeFightTagOutServiceImpl.javaHashMap— brak eksmisji wygasłych wpisówPearlServiceImpl.java(int)przed dzieleniem — możliwy overflow dla dużych czasówFightTagCommand.javatagMultiple— połowicznie nałożony tag przy anulowaniu drugiego + martwy kodNie ruszone (świadomie)
untagAllnie odpala eventów — dodanie ich uruchamiałoby side-effecty (reset trident/flight) per gracz; zmiana zamierzonego zachowania, zostawione do decyzji.Moduł kompiluje się czysto (
compileJava,EXIT=0) — tylko wcześniej istniejące ostrzeżenia deprecation.