diff --git a/internal/modules/sticker/delpack_callback.go b/internal/modules/sticker/delpack_callback.go index 08363a3..3688f16 100644 --- a/internal/modules/sticker/delpack_callback.go +++ b/internal/modules/sticker/delpack_callback.go @@ -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 diff --git a/internal/modules/sticker/delpack_callback_test.go b/internal/modules/sticker/delpack_callback_test.go index fedf070..673c687 100644 --- a/internal/modules/sticker/delpack_callback_test.go +++ b/internal/modules/sticker/delpack_callback_test.go @@ -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) + } +} diff --git a/internal/modules/sticker/pack_handlers.go b/internal/modules/sticker/pack_handlers.go index 27faaba..0d6a7ff 100644 --- a/internal/modules/sticker/pack_handlers.go +++ b/internal/modules/sticker/pack_handlers.go @@ -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) } diff --git a/internal/modules/sticker/pack_handlers_test.go b/internal/modules/sticker/pack_handlers_test.go index cda1199..08d782f 100644 --- a/internal/modules/sticker/pack_handlers_test.go +++ b/internal/modules/sticker/pack_handlers_test.go @@ -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"]) + } + } +} diff --git a/internal/modules/sticker/state.go b/internal/modules/sticker/state.go index 8c66d08..eb1901e 100644 --- a/internal/modules/sticker/state.go +++ b/internal/modules/sticker/state.go @@ -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)) } diff --git a/plans/260824-1051-sticker-pack-module/plan.md b/plans/260824-1051-sticker-pack-module/plan.md index 04536e4..c4e9d5a 100644 --- a/plans/260824-1051-sticker-pack-module/plan.md +++ b/plans/260824-1051-sticker-pack-module/plan.md @@ -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 diff --git a/plans/reports/verify-260825-1630-sticker-round5.md b/plans/reports/verify-260825-1630-sticker-round5.md new file mode 100644 index 0000000..e1b7131 --- /dev/null +++ b/plans/reports/verify-260825-1630-sticker-round5.md @@ -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 `; 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_` 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.