Security hardening + GPL-3.0 license (audit 2026-07-04)
Applied fixes from a full security/bug audit (per-finding detail in the maintainer's audit report): dupe/exploit/injection/thread-safety and input-validation fixes as applicable to this plugin, plus build-portability fixes (JDK path / dependency pins / path casing) where present. Added GPL-3.0 LICENSE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -394,6 +394,30 @@ public class DatabaseManager {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically transitions a listing to a new status ONLY if it is still ACTIVE.
|
||||
* Returns true only for the caller that actually changed the row, so it can be
|
||||
* used as a compare-and-swap guard to prevent double-processing races such as
|
||||
* cancel-vs-purchase or expire-vs-purchase (which would otherwise duplicate the item).
|
||||
*/
|
||||
public CompletableFuture<Boolean> updateListingStatusIfActive(int listingId, Listing.ListingStatus status) {
|
||||
return CompletableFuture.supplyAsync(() -> {
|
||||
String sql = "UPDATE listings SET status = ? WHERE id = ? AND status = 'ACTIVE'";
|
||||
|
||||
try (Connection conn = getConnection();
|
||||
PreparedStatement stmt = conn.prepareStatement(sql)) {
|
||||
|
||||
stmt.setString(1, status.name());
|
||||
stmt.setInt(2, listingId);
|
||||
|
||||
return stmt.executeUpdate() > 0;
|
||||
} catch (Exception e) {
|
||||
plugin.getLogger().log(Level.WARNING, "Failed to update listing status (conditional)", e);
|
||||
}
|
||||
return false;
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets expired active listings.
|
||||
*/
|
||||
@@ -666,6 +690,31 @@ public class DatabaseManager {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically transitions an auction to a new status ONLY if it is still ACTIVE.
|
||||
* Returns true only for the caller that actually changed the row. Used as a
|
||||
* compare-and-swap guard so that ending, buying-out or cancelling an auction can
|
||||
* never be processed twice (which would otherwise duplicate seller earnings and
|
||||
* the item delivered to the winner/seller).
|
||||
*/
|
||||
public CompletableFuture<Boolean> updateAuctionStatusIfActive(int auctionId, Auction.AuctionStatus status) {
|
||||
return CompletableFuture.supplyAsync(() -> {
|
||||
String sql = "UPDATE auctions SET status = ? WHERE id = ? AND status = 'ACTIVE'";
|
||||
|
||||
try (Connection conn = getConnection();
|
||||
PreparedStatement stmt = conn.prepareStatement(sql)) {
|
||||
|
||||
stmt.setString(1, status.name());
|
||||
stmt.setInt(2, auctionId);
|
||||
|
||||
return stmt.executeUpdate() > 0;
|
||||
} catch (Exception e) {
|
||||
plugin.getLogger().log(Level.WARNING, "Failed to update auction status (conditional)", e);
|
||||
}
|
||||
return false;
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets auctions that have ended but are still active.
|
||||
*/
|
||||
|
||||
@@ -45,6 +45,10 @@ public class CreateAuctionGui implements MarketGui {
|
||||
private Double buyoutPrice = null;
|
||||
private int durationHours;
|
||||
|
||||
// Prevents double-submission (e.g. rapid double-click on confirm) which would
|
||||
// otherwise create two auctions while only one set of items is removed = item/money dupe.
|
||||
private boolean processing = false;
|
||||
|
||||
// ==================== LAYOUT CONSTANTS ====================
|
||||
private static final int INFO_SLOT = 4; // Top center
|
||||
private static final int ITEM_DISPLAY_SLOT = 13; // Center
|
||||
@@ -314,6 +318,11 @@ public class CreateAuctionGui implements MarketGui {
|
||||
}
|
||||
|
||||
private void confirmAuction(Player player) {
|
||||
// Guard against double-submission: a confirm is already being processed.
|
||||
if (processing) {
|
||||
return;
|
||||
}
|
||||
|
||||
var msgManager = plugin.getMessageManager();
|
||||
|
||||
// Verify item still exists in player's inventory with sufficient quantity
|
||||
@@ -341,10 +350,14 @@ public class CreateAuctionGui implements MarketGui {
|
||||
// Create the auction
|
||||
ItemStack auctionItem = selectedItem.clone();
|
||||
|
||||
// From here an async creation is in flight; block further confirms until it finishes.
|
||||
processing = true;
|
||||
|
||||
plugin.getTransactionService().createAuctionTransaction(
|
||||
player, auctionItem, startPrice, buyoutPrice, durationHours
|
||||
).thenAccept(result -> {
|
||||
Bukkit.getScheduler().runTask(plugin, () -> {
|
||||
processing = false;
|
||||
if (result.isSuccess()) {
|
||||
// Remove items from inventory AFTER successful creation
|
||||
InventoryUtil.removeItem(player, auctionItem, auctionItem.getAmount());
|
||||
|
||||
@@ -44,6 +44,10 @@ public class CreateListingGui implements MarketGui {
|
||||
private double price;
|
||||
private int durationHours;
|
||||
|
||||
// Prevents double-submission (e.g. rapid double-click on confirm) which would
|
||||
// otherwise create two listings while only one set of items is removed = item/money dupe.
|
||||
private boolean processing = false;
|
||||
|
||||
// ==================== LAYOUT CONSTANTS ====================
|
||||
private static final int INFO_SLOT = 4; // Top center
|
||||
private static final int ITEM_DISPLAY_SLOT = 13; // Center row 1
|
||||
@@ -258,6 +262,11 @@ public class CreateListingGui implements MarketGui {
|
||||
}
|
||||
|
||||
private void confirmListing(Player player) {
|
||||
// Guard against double-submission: a confirm is already being processed.
|
||||
if (processing) {
|
||||
return;
|
||||
}
|
||||
|
||||
var msgManager = plugin.getMessageManager();
|
||||
|
||||
// Verify item still exists in player's inventory with sufficient quantity
|
||||
@@ -286,10 +295,14 @@ public class CreateListingGui implements MarketGui {
|
||||
int amount = selectedItem.getAmount();
|
||||
ItemStack listItem = selectedItem.clone();
|
||||
|
||||
// From here an async creation is in flight; block further confirms until it finishes.
|
||||
processing = true;
|
||||
|
||||
plugin.getTransactionService().createListingTransaction(
|
||||
player, listItem, amount, price, durationHours
|
||||
).thenAccept(result -> {
|
||||
Bukkit.getScheduler().runTask(plugin, () -> {
|
||||
processing = false;
|
||||
if (result.isSuccess()) {
|
||||
// Remove items from inventory AFTER successful creation
|
||||
InventoryUtil.removeItem(player, listItem, amount);
|
||||
|
||||
@@ -229,9 +229,9 @@ public class ItemSelectionGui implements MarketGui {
|
||||
itemWithQuantity.setAmount(quantity);
|
||||
|
||||
if (mode == SelectionMode.LISTING) {
|
||||
guiManager.getCreateListingGui().openWithItem(player, playerInvSlot, itemWithQuantity);
|
||||
new CreateListingGui(plugin, guiManager).openWithItem(player, playerInvSlot, itemWithQuantity);
|
||||
} else {
|
||||
guiManager.getCreateAuctionGui().openWithItem(player, playerInvSlot, itemWithQuantity);
|
||||
new CreateAuctionGui(plugin, guiManager).openWithItem(player, playerInvSlot, itemWithQuantity);
|
||||
}
|
||||
} else {
|
||||
// User cancelled - go back to item selection
|
||||
@@ -244,9 +244,9 @@ public class ItemSelectionGui implements MarketGui {
|
||||
singleItem.setAmount(1);
|
||||
|
||||
if (mode == SelectionMode.LISTING) {
|
||||
guiManager.getCreateListingGui().openWithItem(player, playerInvSlot, singleItem);
|
||||
new CreateListingGui(plugin, guiManager).openWithItem(player, playerInvSlot, singleItem);
|
||||
} else {
|
||||
guiManager.getCreateAuctionGui().openWithItem(player, playerInvSlot, singleItem);
|
||||
new CreateAuctionGui(plugin, guiManager).openWithItem(player, playerInvSlot, singleItem);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -305,27 +305,31 @@ public class AuctionService {
|
||||
return CompletableFuture.completedFuture(CancelResult.HAS_BIDS);
|
||||
}
|
||||
|
||||
// If admin cancelling with bids, refund highest bidder
|
||||
if (isAdmin && auction.getBidCount() > 0 && auction.getHighestBidderUuid() != null) {
|
||||
plugin.getEconomyManager().deposit(auction.getHighestBidderUuid(), auction.getCurrentBid());
|
||||
}
|
||||
|
||||
// Update status
|
||||
return plugin.getDatabaseManager().updateAuctionStatus(auctionId, Auction.AuctionStatus.CANCELLED)
|
||||
// Atomically flip to CANCELLED only if still ACTIVE. This guarantees the
|
||||
// refund + item return below run exactly once and never race with the
|
||||
// periodic end-processing task (which would otherwise duplicate the item
|
||||
// or hand the highest bidder both the item and a refund).
|
||||
return plugin.getDatabaseManager().updateAuctionStatusIfActive(auctionId, Auction.AuctionStatus.CANCELLED)
|
||||
.thenApply(success -> {
|
||||
if (success) {
|
||||
// Return item to claim storage
|
||||
ClaimItem claimItem = new ClaimItem(
|
||||
auction.getSellerUuid(),
|
||||
auction.getItem().clone(),
|
||||
ClaimItem.ClaimReason.CANCELLED_AUCTION,
|
||||
"Auction #" + auctionId
|
||||
);
|
||||
plugin.getDatabaseManager().addClaimItem(claimItem);
|
||||
invalidateCache();
|
||||
return CancelResult.SUCCESS;
|
||||
if (!success) {
|
||||
return CancelResult.FAILED;
|
||||
}
|
||||
return CancelResult.FAILED;
|
||||
|
||||
// If admin cancelling with bids, refund highest bidder
|
||||
if (isAdmin && auction.getBidCount() > 0 && auction.getHighestBidderUuid() != null) {
|
||||
plugin.getEconomyManager().deposit(auction.getHighestBidderUuid(), auction.getCurrentBid());
|
||||
}
|
||||
|
||||
// Return item to claim storage
|
||||
ClaimItem claimItem = new ClaimItem(
|
||||
auction.getSellerUuid(),
|
||||
auction.getItem().clone(),
|
||||
ClaimItem.ClaimReason.CANCELLED_AUCTION,
|
||||
"Auction #" + auctionId
|
||||
);
|
||||
plugin.getDatabaseManager().addClaimItem(claimItem);
|
||||
invalidateCache();
|
||||
return CancelResult.SUCCESS;
|
||||
});
|
||||
});
|
||||
}
|
||||
@@ -350,17 +354,23 @@ public class AuctionService {
|
||||
*/
|
||||
private CompletableFuture<Void> processAuctionEnd(int auctionId) {
|
||||
return getAuction(auctionId)
|
||||
.thenAccept(optAuction -> {
|
||||
if (optAuction.isEmpty()) return;
|
||||
.thenCompose(optAuction -> {
|
||||
if (optAuction.isEmpty()) return CompletableFuture.<Void>completedFuture(null);
|
||||
|
||||
Auction auction = optAuction.get();
|
||||
|
||||
// Update status first
|
||||
// Determine terminal status
|
||||
Auction.AuctionStatus newStatus = auction.getBidCount() > 0
|
||||
? Auction.AuctionStatus.SOLD
|
||||
: Auction.AuctionStatus.EXPIRED;
|
||||
|
||||
plugin.getDatabaseManager().updateAuctionStatus(auctionId, newStatus);
|
||||
// Atomically claim the auction end: only the caller that flips the row
|
||||
// away from ACTIVE proceeds to credit earnings / deliver the item. This
|
||||
// makes end-processing idempotent and prevents duplication when the
|
||||
// periodic task and a buyout (or two task runs) race on the same auction.
|
||||
return plugin.getDatabaseManager().updateAuctionStatusIfActive(auctionId, newStatus)
|
||||
.thenAccept(claimed -> {
|
||||
if (!claimed) return;
|
||||
|
||||
if (auction.getBidCount() > 0 && auction.getHighestBidderUuid() != null) {
|
||||
// Auction has a winner
|
||||
@@ -432,6 +442,7 @@ public class AuctionService {
|
||||
}
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -116,8 +116,19 @@ public class ClaimService {
|
||||
if (removed) {
|
||||
// Give item on main thread
|
||||
ItemStack itemToGive = item.getItem();
|
||||
ClaimItem source = item;
|
||||
Bukkit.getScheduler().runTask(plugin, () -> {
|
||||
InventoryUtil.giveItem(player, itemToGive);
|
||||
ItemStack leftover = InventoryUtil.giveItem(player, itemToGive);
|
||||
if (leftover != null) {
|
||||
// Inventory filled up mid-claim: put the remainder back into
|
||||
// claim storage instead of silently destroying the items.
|
||||
plugin.getDatabaseManager().addClaimItem(new ClaimItem(
|
||||
player.getUniqueId(),
|
||||
leftover,
|
||||
source.getReason(),
|
||||
source.getSourceInfo()
|
||||
));
|
||||
}
|
||||
});
|
||||
claimed++;
|
||||
}
|
||||
|
||||
@@ -291,8 +291,9 @@ public class ListingService {
|
||||
return CompletableFuture.completedFuture(false);
|
||||
}
|
||||
|
||||
// Update status
|
||||
return plugin.getDatabaseManager().updateListingStatus(listingId, Listing.ListingStatus.CANCELLED)
|
||||
// Update status atomically (only if still ACTIVE) so a concurrent
|
||||
// purchase/expiry can never both deliver the item AND return it here.
|
||||
return plugin.getDatabaseManager().updateListingStatusIfActive(listingId, Listing.ListingStatus.CANCELLED)
|
||||
.thenApply(success -> {
|
||||
if (success) {
|
||||
// Return item to claim storage
|
||||
@@ -319,8 +320,9 @@ public class ListingService {
|
||||
return plugin.getDatabaseManager().getExpiredListings()
|
||||
.thenAccept(listings -> {
|
||||
for (Listing listing : listings) {
|
||||
// Update status to expired
|
||||
plugin.getDatabaseManager().updateListingStatus(listing.getId(), Listing.ListingStatus.EXPIRED)
|
||||
// Update status to expired atomically (only if still ACTIVE) so a
|
||||
// concurrent purchase can't both sell the item and return it to claim.
|
||||
plugin.getDatabaseManager().updateListingStatusIfActive(listing.getId(), Listing.ListingStatus.EXPIRED)
|
||||
.thenAccept(success -> {
|
||||
if (success) {
|
||||
// Return item to claim storage
|
||||
|
||||
Reference in New Issue
Block a user