fix(sticker): prove authority before a confirmed pack delete

A /delpack confirmation outlived the record that authorised it. The under-lock re-check listed the states it would refuse - a pending record still naming this set - and fell through on the two that mattered: no record at all, and a record that had moved on to a different pack. Reachable with ordinary commands and no attacker: run /delpack without pressing, let the pack disappear from Telegram's side so a self-heal frees the name, let another user claim it, then press. DeleteStickerSet is keyed by set name, which Telegram authorises for every set this bot created, so the press destroys whoever holds the name at that moment. Invert the guard: delete only when a confirmed record still names this exact set. A check phrased as "which states do I refuse" cannot fail closed against a state nobody enumerated. Dropping a pack record now also clears any outstanding confirmation, so a dead prompt stops existing rather than merely being refused on use. This also stops the reservation leaking when a confirmed delete lands on a record that has moved on, since that case no longer reaches Telegram. Alongside: - Resuming an interrupted /newpack discarded a retyped title and reported success quoting the old one. - TestNewPack_DifferentSlugReplacesDeadIntent was named for releasing a dead name and never asserted it. - lockUser's comment justified the lock with cron and stats-hook contention that does not exist: the map is state-local and this module registers neither. The lock stays for the read-modify-write pattern; a wrong reason for a right guard misleads the next reader.
2026-09-04 04:18:08 +00:00 · 2026-08-25 16:49:37 +07:00
parent c4c8c3088e
commit c83bcfaebf
7 changed files with 447 additions and 13 deletions
@@ -171,17 +171,30 @@ func (s *state) handleDelPackCallback(ctx context.Context, b *bot.Bot, update *m

 	defer s.lockUser(action.OwnerID)()

-	// Re-check under the lock that the record still authorises a Telegram-side
-	// delete. handleDelPack refuses to prompt for a pending record, so this
-	// should be unreachable — but the guard belongs on the destructive
-	// operation rather than only on the path that normally reaches it.
-	if current, found, err := getPack(ctx, s.store, action.OwnerID); err != nil {
+	// Re-establish, under the lock, that the caller still holds this exact set.
+	//
+	// The authority to delete comes from the record, not from the prompt, and a
+	// prompt outlives the record: /delpack can sit unpressed for ten minutes
+	// while the pack disappears from Telegram's side, a self-heal frees the
+	// name, and somebody else claims it. DeleteStickerSet is keyed by set name
+	// and Telegram authorises it for every set this bot created, so a press
+	// then lands on whoever holds the name at that moment.
+	//
+	// Stated as an allowlist deliberately. The first version of this guard
+	// listed the states it would refuse — a pending record still naming this
+	// set — and fell through on the two that mattered: no record at all, and a
+	// record that had moved on to a different pack. Proving authority is the
+	// only formulation that fails closed against a state nobody thought of.
+	current, found, err := getPack(ctx, s.store, action.OwnerID)
+	if err != nil {
 		log.Error("sticker_delpack_recheck", "err", err)
 		return answerCallback(ctx, b, query.ID, "Could not confirm right now. Try /delpack again.")
-	} else if found && current.Pending && ownsSet(current, action.SetName) {
+	}
+	if !found || current.Pending || !ownsSet(current, action.SetName) {
 		s.dropPendingDelete(ctx, key)
 		clearButton(ctx, b, action.ChatID, action.MessageID)
-		return answerCallback(ctx, b, query.ID, "That attempt was never confirmed, so nothing was deleted at Telegram.")
+		return answerCallback(ctx, b, query.ID,
+			"This confirmation is out of date — that pack is no longer yours to delete. Run /delpack again if you still want to.")
 	}

 	// Consume the action *before* the destructive call, so a double press
@@ -256,7 +256,8 @@ func TestDelPackCallback_IgnoresNonCallbackUpdate(t *testing.T) {
 // pack and orphaning it permanently.
 func TestDelPackCallback_StalePressLeavesTheCurrentPackAlone(t *testing.T) {
 	rb := testutil.NewRecordingBot(t)
-	// The old set no longer exists: this is the deleted-then-recreated flow.
+	// Registered so that if the guard ever lets the call through, it is the
+	// assertion below that reports it rather than a confusing downstream error.
 	rb.FailMethodCode("deleteStickerSet", 400, "Bad Request: STICKERSET_INVALID")
 	s := newTestState()
 	ctx := context.Background()
@@ -277,6 +278,14 @@ func TestDelPackCallback_StalePressLeavesTheCurrentPackAlone(t *testing.T) {
 		t.Fatalf("callback: %v", err)
 	}

+	// Nothing may reach Telegram. The old name may since have been claimed by
+	// another user, and DeleteStickerSet is keyed by name alone — asserting only
+	// that this user's own record survived misses the cross-user damage
+	// entirely, which is how this went unnoticed.
+	if n := countMethod(rb, "deleteStickerSet"); n != 0 {
+		t.Errorf("deleteStickerSet calls = %d, want 0 — a stale confirmation reached Telegram", n)
+	}
+
 	pack, found := loadPack(t, s)
 	if !found {
 		t.Fatal("the live pack's record was deleted by a stale confirmation")
@@ -394,3 +403,51 @@ func TestDelPackCallback_ReleasesTheName(t *testing.T) {
 		t.Error("the name is still reserved after the pack was deleted")
 	}
 }
+
+// A confirmation must never outlive the authority it was issued under.
+//
+// Reachable with ordinary commands and no attacker: U runs /delpack and does
+// not press; U's pack then disappears from Telegram's side, so a self-heal
+// clears U's record and frees the name; V legitimately claims that name; U
+// finally presses. DeleteStickerSet is keyed by set name, which Telegram
+// authorises for every set this bot created, so the press landed on V's pack.
+//
+// The re-check that was supposed to stop this was written as a blocklist — it
+// refused only a *pending* record still naming this set, and fell through when
+// the record was missing or had moved on. Authority must be proven, not
+// disproven.
+func TestDelPackCallback_StalePressCannotDeleteTheNextHolder(t *testing.T) {
+	rb := testutil.NewRecordingBot(t)
+	s := newTestState()
+	ctx := context.Background()
+
+	// U has a confirmed pack and a live confirmation prompt for it.
+	seedPack(t, s, 3)
+	action := seedPendingDelete(t, s, nil)
+
+	// U's set vanishes at Telegram; a self-heal clears the record and the name.
+	s.dropPackRecord(ctx, testUser)
+
+	// V now legitimately holds that name, with a set behind it.
+	const victim = int64(99)
+	if err := s.slugs.Put(ctx, slugKey("mypack"),
+		SlugReservation{Slug: "mypack", OwnerID: victim, CreatedAt: fixedNow.UnixMilli()}); err != nil {
+		t.Fatalf("seed victim reservation: %v", err)
+	}
+	if err := s.store.Put(ctx, packKey(victim), Pack{
+		Slug: "mypack", Name: testSet, Title: "V's pack", OwnerID: victim, Count: 5,
+	}); err != nil {
+		t.Fatalf("seed victim pack: %v", err)
+	}
+
+	if err := s.handleDelPackCallback(ctx, rb.Bot, confirmPress(action, testUser)); err != nil {
+		t.Fatalf("callback: %v", err)
+	}
+
+	if n := countMethod(rb, "deleteStickerSet"); n != 0 {
+		t.Errorf("deleteStickerSet calls = %d, want 0 — the press destroyed whoever holds that name now", n)
+	}
+	if _, _, err := s.store.Get(ctx, packKey(victim)); err != nil {
+		t.Errorf("victim's pack record damaged: %v", err)
+	}
+}
@@ -250,8 +250,14 @@ func (s *state) claimSlug(ctx context.Context, b *bot.Bot, msg *models.Message,
 			existing.Slug, shareLink(existing.Name)))

 	case existing.Slug == slug:
-		// Our own interrupted attempt for this exact name: resume it.
-		return existing, false, nil
+		// Our own interrupted attempt for this exact name: resume it, but with
+		// the title from *this* command. Returning the stored record verbatim
+		// silently discarded a retyped title and then reported success using
+		// the old one — "/newpack mypack New Title" answering "Created Old."
+		resumed := existing
+		resumed.Title = intent.Title
+		resumed.Name = intent.Name
+		return resumed, false, nil

 	default:
 		// An earlier attempt was interrupted under a *different* name. Probe
@@ -513,6 +519,11 @@ func (s *state) dropPackRecord(ctx context.Context, ownerID int64) {
 		return
 	}

+	// Any outstanding /delpack confirmation was issued against the record just
+	// removed, so it no longer authorises anything. The callback re-checks too;
+	// this keeps a dead prompt from surviving to be pressed at all.
+	s.dropPendingDelete(commitCtx, pendingDeleteKey(ownerID))
+
 	if err == nil && found && pack.Slug != "" {
 		s.releaseSlug(commitCtx, ownerID, pack.Slug)
 	}
@@ -164,6 +164,12 @@ func TestNewPack_DifferentSlugReplacesDeadIntent(t *testing.T) {
 	if pack.Slug != "mypack" || pack.Pending {
 		t.Errorf("pack = %+v, want a confirmed mypack record", pack)
 	}
+	// The old name had nothing behind it, so it must return to the pool.
+	// Without this assertion the test was named for a behaviour it never
+	// checked: every abandoned attempt would quietly shrink the namespace.
+	if _, held, _ := getSlugReservation(context.Background(), s.slugs, "oldslug"); held {
+		t.Error("the dead name stayed reserved with no set behind it")
+	}
 }

 // An unclassifiable getStickerSet failure means "unknown". Guessing either way
@@ -850,3 +856,57 @@ func (c ctxHonouringSlugs) List(ctx context.Context, prefix string) ([]string, e
 	}
 	return c.inner.List(ctx, prefix)
 }
+
+// A record that goes away takes its outstanding confirmation with it.
+//
+// The callback re-checks authority anyway, so this is defence in depth — but an
+// unpinned guard is how the last one rotted into a blocklist unnoticed.
+func TestDropPackRecord_ClearsAnOutstandingConfirmation(t *testing.T) {
+	s := newTestState()
+	ctx := context.Background()
+
+	seedPack(t, s, 3)
+	seedPendingDelete(t, s, nil)
+
+	s.dropPackRecord(ctx, testUser)
+
+	if _, _, err := s.pending.Get(ctx, pendingDeleteKey(testUser)); err == nil {
+		t.Error("confirmation outlived the record that authorised it")
+	}
+}
+
+// Resuming an interrupted attempt must use the title from the command the user
+// just sent, not the one stored by the attempt that failed.
+//
+// The stored record was returned verbatim, so a retyped title was silently
+// discarded and the success message quoted the old one — "/newpack mypack New"
+// answering "Created Old."
+func TestNewPack_ResumeUsesTheTitleJustTyped(t *testing.T) {
+	rb := testutil.NewRecordingBot(t)
+	stubBotIdentity(rb)
+	setMissing(rb) // nothing was created last time, so this run creates it
+	s := newTestState()
+	ctx := context.Background()
+
+	seedInterrupted(t, s, "mypack", testSet) // stored title is "Old"
+
+	if err := s.handleNewPack(ctx, rb.Bot, stickerReply("/newpack mypack Brand New Title", otherSet)); err != nil {
+		t.Fatalf("handleNewPack: %v", err)
+	}
+
+	pack, found := loadPack(t, s)
+	if !found {
+		t.Fatal("no record after a resumed create")
+	}
+	if pack.Title != "Brand New Title" {
+		t.Errorf("stored title = %q, want the one just typed", pack.Title)
+	}
+	if got := rb.LastSent().Text(); !strings.Contains(got, "Brand New Title") {
+		t.Errorf("reply = %q, want it to quote the title just typed", got)
+	}
+	for _, call := range rb.Sent() {
+		if call.Method == "createNewStickerSet" && call.Form["title"] != "Brand New Title" {
+			t.Errorf("created with title %q, want the one just typed", call.Form["title"])
+		}
+	}
+}
@@ -79,9 +79,18 @@ func mediaContext(ctx context.Context) (context.Context, context.CancelFunc) {
 	return chathelper.FetchContext(ctx)
 }

-// lockUser serialises a user's mutations. Handlers run one at a time today, but
-// the cron scheduler and the detached per-command stats hook run concurrently
-// with them, so this is load-bearing rather than decorative.
+// lockUser serialises a user's mutations.
+//
+// Nothing contends for it today: the map is state-local to this module, the bot
+// dispatches inline with a single worker, and this module registers neither a
+// cron nor a command hook. An earlier version of this comment claimed the cron
+// scheduler and stats hook contended here — they do not, and a wrong reason for
+// a right guard is worse than none, because the next reader trusts it.
+//
+// It stays because every mutation here is a read-modify-write, which is wrong
+// the moment dispatch stops being serial, and an uncontended mutex costs
+// nothing. Note that releaseSlug's read-then-delete is not atomic under this
+// lock either; that is safe only while dispatch is serial.
 func (s *state) lockUser(ownerID int64) func() {
 	return s.locks.Acquire(strconv.FormatInt(ownerID, 10))
 }
@@ -299,6 +299,30 @@ per-user pack limit is one (former O2).

 ## Design Revisions

+### 2026-08-25 — /delpack confirmations must prove authority, not disprove it
+
+Removing adoption closed the takeover class, but the same DeleteStickerSet
+primitive stayed reachable through a stale confirmation. The under-lock re-check
+was written as a blocklist — it refused only a *pending* record still naming the
+set — and fell through on the two states that mattered: no record at all, and a
+record that had moved on. Reproduced with ordinary commands and no attacker:
+prompt, pack disappears at Telegram, self-heal frees the name, another user
+claims it, first user presses, their pack is destroyed.
+
+Inverted to an allowlist: delete only when a confirmed record still names this
+exact set. A guard phrased as "which states do I refuse" cannot fail closed
+against a state nobody enumerated. Dropping a pack record also clears any
+outstanding confirmation, so a dead prompt is gone rather than merely refused.
+
+This also closes the reservation leak on the confirmed-delete path, since the
+moved-on case no longer reaches Telegram at all.
+
+Fixed alongside: resuming an interrupted `/newpack` silently discarded a retyped
+title and reported success with the old one; a test named for freeing a dead
+name never asserted it; and `lockUser`'s comment justified the lock with cron
+and stats-hook contention that does not exist — the map is state-local and this
+module registers neither.
+
 ### 2026-08-25 — pack adoption removed

 Verification found the round-4 guard defeated by the module's own recovery
@@ -0,0 +1,260 @@
+# Sticker module — round 5 adversarial verification
+
+Commit `e81b1b7` ("fix(sticker): never adopt an existing pack"), branch
+`feature/sticker-pack-module`, Go 1.27, golangci-lint v2.13.1.
+
+**Verdict: DO_NOT_MERGE.** The adoption class is genuinely closed — every route
+to a committed `Pack` record naming a foreign set was enumerated and executed,
+and all of them refuse. But the round-5 pattern repeated in the *other*
+direction: the commit correctly identified that `DeleteStickerSet` is a
+cross-user primitive keyed by set name, added a guard for one way of reaching
+it, and left a second way open. A stale `/delpack` confirmation destroys
+whichever user holds that name at press time.
+
+---
+
+## C1 (Critical) — a stale `/delpack` confirmation deletes a re-issued name's pack
+
+`internal/modules/sticker/delpack_callback.go:177-185`
+
+```go
+if current, found, err := getPack(ctx, s.store, action.OwnerID); err != nil {
+    ...
+} else if found && current.Pending && ownsSet(current, action.SetName) {
+    // refuse
+}
+// falls through to DeleteStickerSet(action.SetName)
+```
+
+The re-check is **negative** — it blocks exactly one bad state (`Pending`) — where
+it needed to be **positive**: only proceed when the record still authorises this
+delete. `!found` and "record now names a different set" both fall through to the
+destructive call. `dropPackRecordIfSet` performs precisely the right check
+(`found && ownsSet`), but it runs *after* `DeleteStickerSet`, so it protects the
+local record and not the set.
+
+### Reproduction (executed; all steps are ordinary public commands)
+
+Test `TestProbe_StaleConfirmationDeletesReissuedName`, run against
+`internal/modules/sticker`:
+
+| # | Actor | Command | Effect |
+|---|-------|---------|--------|
+| 1 | U | `/newpack mypack Mine` | committed record + reservation `mypack` |
+| 2 | U | `/delpack`, **do not press** | `PendingDelete{SetName: mypack_by_testbot}` stored, TTL 10 min |
+| 3 | U | `/delsticker` down to zero, then any command | set gone at Telegram → `STICKERSET_INVALID` → `dropPackRecord` drops the record **and releases the reservation**. The unpressed confirmation is untouched — no `dropPackRecord` path clears `s.pending`. |
+| 4 | V | `/newpack mypack Victim Pack` | reservation free, `GetStickerSet` missing → V legitimately creates and owns `mypack_by_testbot` |
+| 5 | U | presses the button from step 2 | binding OK, not expired, re-check sees `found=false` → **`deleteStickerSet name=mypack_by_testbot`** |
+
+Observed output:
+
+```
+step3 self-heal: U record found=false, reservation held=false
+step3 U's unpressed confirmation SURVIVED the self-heal
+step5 V pack found=true {Slug:mypack Name:mypack_by_testbot ... OwnerID:4242 Pending:false}
+step6 methods = [deleteStickerSet editMessageReplyMarkup sendMessage answerCallbackQuery]
+HOLE CONFIRMED: U deleted "mypack_by_testbot" — V's pack, created after U's record was gone
+step6 V record still found=true (local record survives, set does not)
+step6 answer = "Pack deleted."
+```
+
+V is left with a committed record pointing at a destroyed set, and U is told the
+delete succeeded.
+
+**Reachability: production, not hypothetical.** Dispatch is serial
+(`internal/telegram/client.go:28` `WithNotAsyncHandlers`, single worker) — this is
+a plain sequential command sequence, no race. Steps 1-3 are fully under the
+attacker's control and take seconds; the only constraint is that step 4 lands
+inside the 10-minute `pendingDeleteTTL`. The same sequence also occurs with no
+attacker at all: run `/delpack`, get distracted, empty the pack, run one more
+command, and whoever takes the freed name loses it when you finally press.
+
+Second, milder variant, also executed
+(`TestProbe_StaleConfirmationAfterRecordMovedOn`): with the record moved to a
+different pack, the press still issues
+`deleteStickerSet name="mypack_by_testbot"` while the record names
+`other_by_testbot`. `dropPackRecordIfSet` correctly leaves the record alone —
+after the set is already gone.
+
+### Fix shape
+
+Invert the guard to a positive authorisation, matching `dropPackRecordIfSet`:
+
+```go
+current, found, err := getPack(ctx, s.store, action.OwnerID)
+if err != nil { ...transient answer... }
+if !found || current.Pending || !ownsSet(current, action.SetName) {
+    s.dropPendingDelete(ctx, key)
+    clearButton(ctx, b, action.ChatID, action.MessageID)
+    return answerCallback(ctx, b, query.ID, "That pack is no longer yours to delete; nothing was deleted at Telegram.")
+}
+```
+
+Additionally, every `dropPackRecord` / `dropPackRecordIfSet` should clear
+`pendingDeleteKey(ownerID)`. A record that no longer exists must not leave a live
+capability behind it; the TTL is the only thing bounding it today.
+
+---
+
+## H1 (High) — the round-5 re-check is not pinned by any test
+
+Mutation M2b: delete the entire `getPack` re-check block from
+`handleDelPackCallback` (delpack_callback.go:177-185).
+
+**SURVIVED.** Full suite `ok`. Item 4 of the commit description — "a defensive
+re-check was also added in `handleDelPackCallback` under the lock" — has zero
+test coverage. This is the same defect class the memory file records: a claim
+asserted in the commit text that no test exercises. It is also the exact guard
+whose incompleteness produces C1, so the gap and the bug are the same omission.
+
+---
+
+## Mutation results (full)
+
+Backed up to scratchpad, restored, `md5sum -c` all match, `git status --short`
+empty (verified after every mutation).
+
+| # | Mutation | Result | Killing test |
+|---|----------|--------|--------------|
+| M1 | `createPack` `err == nil` → `finishNewPack` (adoption restored) | **killed** | `TestNewPack_InterruptedAttemptWithLiveSetIsRefused`, `TestNewPack_WipedStoreCannotAdoptSurvivingPack`, `TestNewPack_InconclusiveProbeThenLiveSetCannotTakeOver` |
+| M2 | remove `/delpack` pending short-circuit | **killed** | `TestDelPack_PendingRecordDeletesNothingAtTelegram` |
+| **M2b** | **remove `handleDelPackCallback`'s under-lock re-check** | **SURVIVED** | — |
+| M3 | neutralise `releaseSlug` ownership check | **killed** | `TestReleaseSlug_RefusesANameHeldBySomeoneElse` |
+| M4 | re-attach `releaseSlug`'s ownership read to the request ctx | **killed** | `TestReleaseSlug_CompletesOnACancelledContext` |
+| M5b | `resolveStaleIntent` `err == nil` branch adopts the old set (R4's second path) | **killed** | `TestNewPack_DifferentSlugDoesNotAdoptExistingSet` |
+| M6 | release the slug unconditionally on claim bail (drop the `created` guard) | **killed** | `TestNewPack_ResumedReservationNotReleasedWhenClaimBails` |
+| **M7** | **`resolveStaleIntent` missing-branch no longer releases the dead name** | **SURVIVED** | — |
+| M8 | `createPack` unknown-lookup branch drops intent + reservation | **killed** | `TestNewPack_UnknownLookupErrorAborts` |
+| M9 | `createPack` refusal keeps the intent | **killed** | `TestNewPack_InterruptedAttemptWithLiveSetIsRefused` |
+
+M7 detail: `TestNewPack_DifferentSlugReplacesDeadIntent` (pack_handlers_test.go:149)
+is named for replacing a dead intent but asserts only the *new* pack. Nothing
+checks that `oldslug`'s reservation was freed, so the R1 name-burn class is
+unpinned. The code is correct today; only the regression barrier is missing.
+
+The four tests added by this commit are otherwise non-vacuous: M1 kills
+`TestNewPack_InconclusiveProbeThenLiveSetCannotTakeOver`, M2 kills
+`TestDelPack_PendingRecordDeletesNothingAtTelegram`, M3/M4 kill the two
+`TestReleaseSlug_*` tests. The `ctxHonouringSlugs` wrapper is load-bearing — its
+comment ("this assertion passed whether or not the code detached until the store
+was made to honour cancellation") is accurate.
+
+---
+
+## What the commit did close (verified by execution, not inspection)
+
+### No path to a committed record naming a foreign set
+
+`Pending = false` is written in exactly one place, `finishNewPack`
+(pack_handlers.go:352), reachable only after `CreateNewStickerSet` returns nil.
+`adjustCount` and `handleRenamePack` preserve/require the flag. Enumerated and
+executed in `TestProbe_PostWipeAttackSurface`, `TestProbe_PendingToCommittedSweep`,
+`TestProbe_CommittedRecordOnlyAfterCreate`:
+
+- post-wipe `/newpack victimslug` with the set live → `slugTaken`, record dropped,
+  reservation released, no `createNewStickerSet`
+- inconclusive probe, then a second `/newpack` with the set live → refused, cleaned
+- resume with a pending record naming the victim's set → refused, cleaned
+- `createNewStickerSet` refused (`PACK_SHORT_NAME_OCCUPIED`) → intent and
+  reservation both released
+- with a pending record naming the victim's set, all six other commands refuse:
+  `/addsticker` and `/renamepack` → `noPackYet`; `/delsticker`, `/editsticker`,
+  `/ordersticker`, `/setpackicon` → `notOwnedRefusal`. **Zero** Telegram
+  mutations in every case.
+
+### `/delpack` pending short-circuit cannot be aimed at another user
+
+`getPack(ctx, s.store, senderID(msg))` and `dropPackRecord(ctx, ownerID)` are
+owner-keyed throughout, and `releaseSlug` re-verifies the holder itself
+(M3 confirms). Executed: a caller can only free a reservation they hold
+(`sticker_release_slug_refused` logged otherwise). Freeing a name that still has
+a live set behind it is possible but not exploitable — the next claimant is
+refused by `createPack`'s occupancy probe (verified: third party gets
+`slugTaken`).
+
+### No wedge
+
+`TestProbe_WedgeAudit`, 4 leftover states × 3 escape routes = 12 runs. Every
+state escapes via `/newpack <other-slug>`; 11 of 12 also escape via the same
+slug or `/delpack`. The one refusal (`pending record + foreign reservation`,
+same slug) is correct and has two working escapes.
+
+---
+
+## M (Medium) — resuming an interrupted `/newpack` silently uses the old title
+
+`claimSlug`'s resume branch (pack_handlers.go:246) returns `existing`, discarding
+the freshly parsed `title`. Executed (`TestProbe_ResumeIgnoresNewTitle`): after a
+pending `mypack`/"Old" record, `/newpack mypack Brand New Title` calls
+`createNewStickerSet title="Old"` and answers `"Created Old."` — confirming a
+title the user did not type, with `/renamepack` the only fix. The same branch
+also reuses `existing.Name`, so after a BotFather rename every resume builds a
+set name with the stale `_by_<old>` suffix that Telegram will reject; the
+freshly computed `setName` is discarded.
+
+## L1 — `resolveStaleIntent` leaks the old reservation permanently
+
+The `err == nil` branch keeps the old reservation with no record pointing at it
+(`TestProbe_StaleIntentReservationLeak`: `reservation[oldslug] owner=42`, no pack
+record). Correct in intent — a set really occupies the name — but the entry is
+unreachable by any code path while the user holds another committed pack. Not
+weaponisable: reaching the branch requires a set to genuinely exist under the
+name, so it is not a cheap name-burn primitive.
+
+## L2 — confirmed delete leaks the reservation when the record moved on
+
+`TestProbe_ConfirmedDeleteLeaksReservationWhenRecordMoved`: `DeleteStickerSet`
+succeeds, `dropPackRecordIfSet` correctly declines to touch the moved record, and
+nobody releases `mypack` — a name whose set is now definitely gone stays reserved
+forever. Same code path as C1's second variant.
+
+## L3 — `releaseSlug` read-then-delete is not atomic
+
+`getSlugReservation` then `Delete` on the same key with no CAS. Under concurrent
+dispatch, a reservation re-claimed between the two calls would be deleted by the
+previous holder. **Not reachable today** (serial dispatch), but `state.go:83`
+documents the lock as "load-bearing rather than decorative" because of the cron
+scheduler and stats hook — worth a `PutVersioned`-style compare-and-delete or an
+explicit note that neither touches this store.
+
+---
+
+## Gates
+
+| Gate | Result |
+|------|--------|
+| `go build ./...` | pass |
+| `go vet ./...` | pass |
+| `gofmt -l .` | clean |
+| `golangci-lint run ./...` | 0 issues |
+| `go test ./...` | pass |
+| `go test -race ./...` | pass |
+| `go test -race -count=20 ./internal/modules/sticker/...` | pass, 86.5s, no flakes |
+| workspace restored | `md5sum -c` all match, `git status --short` empty |
+
+## Recommended actions
+
+1. **Blocking** — fix C1: invert the `handleDelPackCallback` re-check to positive
+   authorisation (`!found || Pending || !ownsSet` → refuse).
+2. **Blocking** — clear `pendingDeleteKey(ownerID)` in `dropPackRecord` and
+   `dropPackRecordIfSet`, so a dropped record cannot leave a live delete
+   capability behind.
+3. **Blocking** — add a test for the re-check (kill M2b) covering all three bad
+   states: `!found`, `Pending`, and record-names-another-set. The end-to-end
+   sequence in C1 is the right shape.
+4. High — extend `TestNewPack_DifferentSlugReplacesDeadIntent` to assert the old
+   reservation was released (kill M7).
+5. Medium — carry the new title (and freshly computed set name) through the
+   resume branch, or state in the reply that the original title was kept.
+6. Low — release the reservation in L2's branch; document or close L3.
+
+## Unresolved questions
+
+- Does Telegram in fact delete a sticker set when its last sticker is removed?
+  The module documents this as undocumented behaviour. C1's step 3 uses it as the
+  cheapest self-service way to make the set vanish, but the hole does not depend
+  on it — any external deletion, or any `STICKERSET_INVALID` self-heal, reaches
+  the same state.
+- Is `pendingDeleteTTL` (10 min) intended as a security bound? It is currently
+  the only thing limiting C1's exploitation window, and the comment justifies it
+  on irreversibility grounds rather than as an authorisation control.