From bf57d4d5a889e9bce413c788802cc2cba7ffe963 Mon Sep 17 00:00:00 2001 From: Draxx Date: Tue, 22 Sep 2026 17:51:59 -0300 Subject: [PATCH] fix(monetization): reject malformed gift recipients --- docgen/guides/log-codes.md | 1 + src/Server/Monetization.luau | 55 ++++++++++++++++++++-- src/Types.luau | 1 + test/Specs/gifting/GiftIntentNet.spec.luau | 48 +++++++------------ 4 files changed, 68 insertions(+), 37 deletions(-) diff --git a/docgen/guides/log-codes.md b/docgen/guides/log-codes.md index 13e92b8..e964b28 100644 --- a/docgen/guides/log-codes.md +++ b/docgen/guides/log-codes.md @@ -255,6 +255,7 @@ Each section heading below **is** the entry's `Category` value, so a row's secti | `GIFT_CREDIT_ISSUED` | Warn | A giftable perk was bought with no gift intent while the buyer already owns it, under `NoGiftIntentPolicy = "GrantOrCredit"`, so a re-aimable credit was written instead of a no-op grant. Also fires `OnGiftCredit`. | | `GIFT_CREDIT_UNCONFIRMED` | Warn | A gift bought with a paid credit could neither be confirmed delivered nor proved undelivered, so the credit was deliberately NOT refunded: the gift may already be queued, and handing the credit back would let one payment grant twice under a fresh id. It names the buyer, the product and the recipient so the rare genuine loss can be compensated by hand. | | `GIFT_DELIVERY_RETRY` | Warn | Cross-server gift delivery failed, so the receipt returns `NotProcessedYet` and Roblox will retry. Repeated hits point at a DataStore messaging problem. | +| `GIFT_INVALID_RECIPIENT` | Error | A persisted gift intent named a non-positive, non-integer, non-finite, or otherwise invalid recipient, so the receipt is held for repair instead of sending the paid gift to an invalid DataStore key. | | `GIFT_INTENT_EXPIRED` | Warn | A stored gift intent outlived the intent TTL, which is an abandoned prompt. It is cleared and any incoming receipt falls through to the no-intent policy. Normal cleanup. | | `GIFT_NO_INTENT` | Warn | A giftable perk was bought with no matching intent and the buyer does not already own it, so the perk was granted to the buyer. An expected fallback. | | `GIFT_RECIPIENT_ALREADY_OWNS` | Warn | Between prompt and receipt the recipient acquired the perk anyway, so the purchase became a re-aimable credit for the buyer rather than a wasted grant. Also fires `OnGiftCredit`. | diff --git a/src/Server/Monetization.luau b/src/Server/Monetization.luau index fa36e1b..e5d9781 100644 --- a/src/Server/Monetization.luau +++ b/src/Server/Monetization.luau @@ -392,10 +392,25 @@ function Monetization.new(ctx: any) return if fromAim then GIFT_AIMS else PENDING_GIFTS end + local function validRecipientId(value: any): number? + local recipientId = tonumber(value) + if + recipientId == nil + or recipientId ~= recipientId + or recipientId == math.huge + or recipientId == -math.huge + or recipientId <= 0 + or recipientId % 1 ~= 0 + then + return nil + end + return recipientId + end + --- The stored record for this product, or nil unless it is complete and belongs to --- this product. A differing Product name means a retired product reused this Id, so - --- the recipient was aimed at something else; a missing RecipientId is the phantom - --- shape a blind `Ts` write leaves behind, and honouring one gifts to userId 0. + --- the recipient was aimed at something else; malformed recipient values are rejected + --- before the receipt path can attempt a delivery. local function readGiftRecord( reserved: { [string]: any }, fromAim: boolean, @@ -410,7 +425,7 @@ function Monetization.new(ctx: any) if record.Product ~= nil and record.Product ~= product.Name then return nil end - if (tonumber(record.RecipientId) or 0) <= 0 or record.GiftId == nil then + if validRecipientId(record.RecipientId) == nil or record.GiftId == nil then return nil end -- No age check here on purpose: the horizon bounds what a record may be PRESUMED @@ -1314,7 +1329,22 @@ function Monetization.new(ctx: any) fromAim: boolean ): Enum.ProductPurchaseDecision local purchaseId = tostring(receiptInfo.PurchaseId) - local recipientId = tonumber(intent.RecipientId) or 0 + local recipientId = validRecipientId(intent.RecipientId) + if recipientId == nil then + Log.Error( + "Gifting", + "GIFT_INVALID_RECIPIENT", + "gift intent has an invalid recipient; holding the receipt for repair", + { + Player = entry.Player, + Product = product.Name, + PurchaseId = purchaseId, + RecipientId = intent.RecipientId, + } + ) + Metrics.Add("ReceiptsRetried") + return NOT_PROCESSED + end local giftId = tostring(intent.GiftId) local productIdStr = tostring(product.Id) @@ -1587,7 +1617,22 @@ function Monetization.new(ctx: any) fromAim: boolean ): Enum.ProductPurchaseDecision local purchaseId = tostring(receiptInfo.PurchaseId) - local recipientId = tonumber(record.RecipientId) or 0 + local recipientId = validRecipientId(record.RecipientId) + if recipientId == nil then + Log.Error( + "Gifting", + "GIFT_INVALID_RECIPIENT", + "gift record has an invalid recipient; holding the receipt for repair", + { + UserId = userId, + Product = product.Name, + PurchaseId = purchaseId, + RecipientId = record.RecipientId, + } + ) + Metrics.Add("ReceiptsRetried") + return NOT_PROCESSED + end local giftId = tostring(record.GiftId) local productIdStr = tostring(product.Id) diff --git a/src/Types.luau b/src/Types.luau index 86c570b..a8b9a72 100644 --- a/src/Types.luau +++ b/src/Types.luau @@ -180,6 +180,7 @@ export type LogCode = | "GIFT_CREDIT_UNKNOWN_PRODUCT" | "GIFT_CREDIT_USED" | "GIFT_DELIVERY_RETRY" + | "GIFT_INVALID_RECIPIENT" | "GIFT_INTENT_EXPIRED" | "GIFT_INTENT_WRITE_FAIL" | "GIFT_NO_INTENT" diff --git a/test/Specs/gifting/GiftIntentNet.spec.luau b/test/Specs/gifting/GiftIntentNet.spec.luau index 802a1d4..476132a 100644 --- a/test/Specs/gifting/GiftIntentNet.spec.luau +++ b/test/Specs/gifting/GiftIntentNet.spec.luau @@ -71,9 +71,8 @@ return function() end --- Every slot that exists must carry BOTH halves of an intent's identity. A - --- slot missing either is a phantom: `tonumber(intent.RecipientId) or 0` - --- resolves it to userId 0 and deliverGift MessageAsyncs a paid gift to the - --- DataStore key for user 0, which answers true. + --- slot missing either is a phantom. The receipt path must reject it before + --- `deliverGift` can turn a missing recipient into the userId 0 DataStore key. local function describePending(entry): string local keys = {} for productIdStr, intent in pending(entry) do @@ -221,8 +220,7 @@ return function() -- The trap a one-line "just refresh the timestamp" fix falls into. -- PendingGifts is a free-form dynamic node, so `PendingGifts[id].Ts.Set(x)` -- on an ABSENT slot creates `{Ts = n}` -- no RecipientId, no GiftId. A later - -- retry reads `tonumber(intent.RecipientId) or 0` and MessageAsyncs a paid - -- gift to the DataStore key for userId 0, which answers true. + -- retry must reject it before any paid delivery is attempted. withClock(1700000000, function(advance) local harness = makeHarness() local buyer, entry = harness.Join(80121) @@ -673,48 +671,34 @@ return function() end) end) - describe("DEFECT CHARACTERISATION: nothing re-validates the recipient on the receipt path", function() - it("MessageAsyncs a paid gift to the DataStore key for userId 0", function() - -- The consequence of a phantom, and the reason the guarded-refresh specs - -- above matter. PromptGift validates the recipient at its own entry - -- (`recipientUserId <= 0` is refused); NOTHING re-validates it afterwards. - -- handleGiftReceipt reads `tonumber(intent.RecipientId) or 0` and hands the - -- result straight to deliverGift, which MessageAsyncs to the recipient's - -- key. There is no live entry for user 0, so the send goes to the mailbox - -- for PLAYER_0 -- and MessageAsync answers true, which is a durable - -- delivery as far as every layer above it is concerned. The receipt then - -- answers PurchaseGranted and Roblox consumes the Robux. - -- - -- The intent below is planted directly rather than produced by a bug, - -- because it has to be: with the guard at the failure arm intact there is - -- no reachable way to MAKE a phantom today. That is exactly what the - -- phantom specs above protect, and this block records what it costs if one - -- ever gets through. - -- - -- THIS BLOCK MUST FLIP IF A `recipientUserId > 0` GUARD IS ADDED to the - -- receipt path. That guard is a scope call, not part of F2 or F3 -- flag - -- it, do not smuggle it in. + describe("invalid recipient is held before delivery", function() + it("does not send a paid gift to the DataStore key for userId 0", function() + -- PromptGift validates the recipient at its own entry, but persisted intent + -- data can still be malformed after a partial write, migration, or future + -- caller. The receipt path must fail closed rather than coerce nil to userId 0. withClock(1700000000, function() local harness = makeHarness() local buyer, entry = harness.Join(81101) + local invalid, unsubscribe = codeCounter("GIFT_INVALID_RECIPIENT") -- A phantom: the shape a blind `PendingGifts[id].Ts.Set(x)` produces. entry.Tree.Reserved.PendingGifts["222"].Set({ Ts = Clock.Now() }) local decision = harness.Data.HandleReceipt(receipt(81101, 222, "phantom-delivery")) + task.wait() + unsubscribe() local zeroKey = "Default/" .. harness.Ctx.Persistence.KeyFor(0) local zeroBox = harness.Fake.Mailboxes[zeroKey] or {} local sent = harness.Data.GetPurchases(buyer, { Kind = "Robux" }) - local line = sent[#sent] expect( - ("d=%s toUserZero=%d loggedTo=%s type=%s"):format( - tostring(decision == GRANTED), + ("d=%s toUserZero=%d logged=%d errors=%d"):format( + tostring(decision == NOT_PROCESSED), #zeroBox, - tostring(line.From.ToUserId), - tostring(line.From.Type) + #sent, + invalid() ) - ).to.equal("d=true toUserZero=1 loggedTo=0 type=GiftSent") + ).to.equal("d=true toUserZero=0 logged=0 errors=1") harness.Cleanup() end)