mirror of
https://github.com/prosolis/gogobee.git
synced 2026-07-15 00:32:40 +00:00
N4/E2 review fixes: close a gift-enabled equip dupe; harden vault writes
A high-effort code review of the gifting/vault work turned up three issues: 1. Item duplication (real exploit). The masterwork/arena equip confirmation captures an item at prompt time and equips it on "yes" without re-checking ownership. Since MasterworkGear/ArenaGear are now giftable, a player could !give the item away in the window, then confirm — minting a copy onto themselves while the recipient kept theirs. Fixed by taking the user lock (making gift-vs-confirm atomic) and re-loading the inventory to refuse an item that is no longer ours. Magic items are exempt (not giftable); other delete paths load-and-remove within one locked invocation. 2. clearAdvInventory read the vault-filtered list but its DELETE wiped every row including vaulted ones — no live caller today, but it would break the vault's "safe from !sell all" promise the instant sell-all routed through it. Delete now matches the read (in_vault = 0). 3. vault store/take took no per-user lock, so two near-simultaneous stores could both pass the capacity check and overfill the 10-slot vault. Now serialized on the same user lock the gift path uses. go test ./... green; golden byte-identical. Claude-Session: https://claude.ai/code/session_017mEwUmmS7aQTP2NQXj6rUa
This commit is contained in:
@@ -613,7 +613,10 @@ func clearAdvInventory(userID id.UserID) ([]AdvItem, error) {
|
|||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
d := db.Get()
|
d := db.Get()
|
||||||
_, err = d.Exec(`DELETE FROM adventure_inventory WHERE user_id = ?`, string(userID))
|
// Delete only what loadAdvInventory returned — vaulted rows are out of play
|
||||||
|
// and must survive a bulk clear, or the vault's whole promise (safe from
|
||||||
|
// !sell all) breaks the moment any caller routes sell-all through here.
|
||||||
|
_, err = d.Exec(`DELETE FROM adventure_inventory WHERE user_id = ? AND in_vault = 0`, string(userID))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -492,8 +492,33 @@ func (p *AdventurePlugin) handleMasterworkEquipConfirm(ctx MessageContext, inter
|
|||||||
return p.SendDM(ctx.Sender, "Equip cancelled.")
|
return p.SendDM(ctx.Sender, "Equip cancelled.")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Serialize against the sender's own !give: MasterworkGear/ArenaGear are
|
||||||
|
// giftable (N4/E2), so the item captured at prompt time may have changed
|
||||||
|
// hands before this confirmation. The lock makes gift-vs-confirm atomic,
|
||||||
|
// and the re-load below refuses an item that is no longer ours — otherwise
|
||||||
|
// equipping the captured copy would mint a duplicate the gift already
|
||||||
|
// handed to the recipient.
|
||||||
|
userMu := p.advUserLock(ctx.Sender)
|
||||||
|
userMu.Lock()
|
||||||
|
defer userMu.Unlock()
|
||||||
|
|
||||||
selected := data.Item
|
selected := data.Item
|
||||||
|
|
||||||
|
inv, err := loadAdvInventory(ctx.Sender)
|
||||||
|
if err != nil {
|
||||||
|
return p.SendDM(ctx.Sender, "Failed to load your inventory.")
|
||||||
|
}
|
||||||
|
stillOwned := false
|
||||||
|
for _, it := range inv {
|
||||||
|
if it.ID == selected.ID {
|
||||||
|
stillOwned = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !stillOwned {
|
||||||
|
return p.SendDM(ctx.Sender, fmt.Sprintf("**%s** isn't in your inventory anymore. Equip cancelled.", selected.Name))
|
||||||
|
}
|
||||||
|
|
||||||
equip, err := loadAdvEquipment(ctx.Sender)
|
equip, err := loadAdvEquipment(ctx.Sender)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return p.SendDM(ctx.Sender, "Failed to load equipment.")
|
return p.SendDM(ctx.Sender, "Failed to load equipment.")
|
||||||
|
|||||||
@@ -48,9 +48,17 @@ func (p *AdventurePlugin) handleVaultCmd(ctx MessageContext, args string) error
|
|||||||
case lower == "" || lower == "list":
|
case lower == "" || lower == "list":
|
||||||
reply = renderVault(ctx.Sender)
|
reply = renderVault(ctx.Sender)
|
||||||
case strings.HasPrefix(lower, "store "):
|
case strings.HasPrefix(lower, "store "):
|
||||||
|
// Serialize a player's own store/take so two near-simultaneous stores
|
||||||
|
// can't both pass the capacity check and overfill the vault.
|
||||||
|
userMu := p.advUserLock(ctx.Sender)
|
||||||
|
userMu.Lock()
|
||||||
reply = vaultStoreItem(ctx.Sender, strings.TrimSpace(args[len("store "):]))
|
reply = vaultStoreItem(ctx.Sender, strings.TrimSpace(args[len("store "):]))
|
||||||
|
userMu.Unlock()
|
||||||
case strings.HasPrefix(lower, "take "):
|
case strings.HasPrefix(lower, "take "):
|
||||||
|
userMu := p.advUserLock(ctx.Sender)
|
||||||
|
userMu.Lock()
|
||||||
reply = vaultTakeItem(ctx.Sender, strings.TrimSpace(args[len("take "):]))
|
reply = vaultTakeItem(ctx.Sender, strings.TrimSpace(args[len("take "):]))
|
||||||
|
userMu.Unlock()
|
||||||
default:
|
default:
|
||||||
reply = "Usage: `!adventure vault` to view · `!adventure vault store <item>` · `!adventure vault take <item>`."
|
reply = "Usage: `!adventure vault` to view · `!adventure vault store <item>` · `!adventure vault take <item>`."
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user