fix(monetization): hold gifts with invalid recipients

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`. |

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)
 

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"

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)