From 562e43974fc03f87b097d45a1a5155597cd6cb36 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Tue, 25 Aug 2026 17:09:19 +0700 Subject: [PATCH] test(sticker): pin the delpack authority guard, keep the stored set name The previous commit's regression test never reached the guard it was named for. It broke the pack record with dropPackRecord, which now also clears the confirmation, so the callback returned at the pending.Get miss long before the allowlist. The test passed with the entire guard reverted - shipping the fix with its own detector inoperative, which is the defect that let five earlier rounds report a false clean. Replace it with a table that leaves the confirmation intact and breaks the record three ways, one per disjunct: record gone, record unconfirmed, record moved on. Reverting the guard now fails two cases; each disjunct was mutated individually. The !found disjunct is an equivalent mutant: ownsSet already returns false for a zero-value record's empty Name, so no test can kill it. Kept and commented, because that redundancy is an accident of ownsSet's empty-string guard rather than something this check should rely on. Also revert the set-name half of the previous commit's resume change. Carrying the retyped title is right; re-deriving Pack.Name was not. The name comes from the bot username, which can change at BotFather, and the stored one identifies the set the interrupted attempt may already have created - refreshing it orphaned that set and aimed later commands at a different name, contradicting ownsSet's own documented rule. Pinned. --- internal/modules/sticker/delpack_callback.go | 4 + .../modules/sticker/delpack_callback_test.go | 112 ++++--- internal/modules/sticker/pack_handlers.go | 5 +- .../modules/sticker/pack_handlers_test.go | 31 ++ .../verify-260825-1700-sticker-round6.md | 310 ++++++++++++++++++ 5 files changed, 423 insertions(+), 39 deletions(-) create mode 100644 plans/reports/verify-260825-1700-sticker-round6.md diff --git a/internal/modules/sticker/delpack_callback.go b/internal/modules/sticker/delpack_callback.go index 3688f16..10756de 100644 --- a/internal/modules/sticker/delpack_callback.go +++ b/internal/modules/sticker/delpack_callback.go @@ -190,6 +190,10 @@ func (s *state) handleDelPackCallback(ctx context.Context, b *bot.Bot, update *m log.Error("sticker_delpack_recheck", "err", err) return answerCallback(ctx, b, query.ID, "Could not confirm right now. Try /delpack again.") } + // !found is stated explicitly even though ownsSet already returns false for + // a zero-value record's empty Name — mutation testing shows it is currently + // redundant. It stays because that redundancy is an accident of ownsSet's + // empty-string guard, not something this check should depend on. if !found || current.Pending || !ownsSet(current, action.SetName) { s.dropPendingDelete(ctx, key) clearButton(ctx, b, action.ChatID, action.MessageID) diff --git a/internal/modules/sticker/delpack_callback_test.go b/internal/modules/sticker/delpack_callback_test.go index 673c687..6397941 100644 --- a/internal/modules/sticker/delpack_callback_test.go +++ b/internal/modules/sticker/delpack_callback_test.go @@ -406,48 +406,84 @@ func TestDelPackCallback_ReleasesTheName(t *testing.T) { // 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. +// Each case leaves the PendingDelete intact and breaks the pack record a +// different way, so the press actually reaches the under-lock allowlist. An +// earlier version of this test called dropPackRecord, which now clears the +// confirmation too — so the callback returned at the pending.Get miss ~50 lines +// before the guard, and the test passed with the whole guard reverted. Every +// disjunct is exercised here on purpose. // -// 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. +// The damage is cross-user: DeleteStickerSet is keyed by set name, which +// Telegram authorises for every set this bot created, so a press with stale +// authority destroys whoever holds that name at press time. +func TestDelPackCallback_StaleAuthorityNeverReachesTelegram(t *testing.T) { 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) + + cases := []struct { + name string + break_ func(t *testing.T, s *state, ctx context.Context) + }{ + { + // !found — a self-heal removed the record but the prompt survived. + name: "record gone", + break_: func(t *testing.T, s *state, ctx context.Context) { + if err := s.store.Delete(ctx, packKey(testUser)); err != nil { + t.Fatalf("delete record: %v", err) + } + }, + }, + { + // current.Pending — the record is an unconfirmed attempt, which is + // no evidence this bot made that set for this user. + name: "record is unconfirmed", + break_: func(t *testing.T, s *state, ctx context.Context) { + pack, _ := loadPack(t, s) + pack.Pending = true + if err := s.store.Put(ctx, packKey(testUser), pack); err != nil { + t.Fatalf("mark pending: %v", err) + } + }, + }, + { + // !ownsSet — the record has moved on to a different pack. + name: "record moved on", + break_: func(t *testing.T, s *state, ctx context.Context) { + if err := s.store.Put(ctx, packKey(testUser), Pack{ + Slug: "newslug", Name: "newslug_by_testbot", Title: "New", OwnerID: testUser, Count: 7, + }); err != nil { + t.Fatalf("move record: %v", err) + } + }, + }, } - if err := s.handleDelPackCallback(ctx, rb.Bot, confirmPress(action, testUser)); err != nil { - t.Fatalf("callback: %v", err) - } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rb := testutil.NewRecordingBot(t) + s := newTestState() + ctx := context.Background() - 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) + seedPack(t, s, 3) + action := seedPendingDelete(t, s, nil) + tc.break_(t, s, ctx) + + // Someone else now holds that name, with a live set behind it. + 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: %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 — a stale confirmation destroyed the current holder's pack", 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 0d6a7ff..6cfaa53 100644 --- a/internal/modules/sticker/pack_handlers.go +++ b/internal/modules/sticker/pack_handlers.go @@ -256,7 +256,10 @@ func (s *state) claimSlug(ctx context.Context, b *bot.Bot, msg *models.Message, // the old one — "/newpack mypack New Title" answering "Created Old." resumed := existing resumed.Title = intent.Title - resumed.Name = intent.Name + // Name is deliberately NOT refreshed. It is derived from the bot's + // username, which can change at BotFather; the stored one names the set + // the interrupted attempt may already have created, and repointing it + // would orphan that set and aim later commands at a different name. return resumed, false, nil default: diff --git a/internal/modules/sticker/pack_handlers_test.go b/internal/modules/sticker/pack_handlers_test.go index 08d782f..eaa63bc 100644 --- a/internal/modules/sticker/pack_handlers_test.go +++ b/internal/modules/sticker/pack_handlers_test.go @@ -910,3 +910,34 @@ func TestNewPack_ResumeUsesTheTitleJustTyped(t *testing.T) { } } } + +// Resuming must not re-derive the set name. +// +// Pack.Name is built from the bot's username, which can change at BotFather. +// The stored name identifies the set the interrupted attempt may already have +// created; refreshing it from the current username would repoint the record at +// a name nothing exists under, orphaning that set and aiming every later +// command at the wrong one. ownsSet documents the same rule. +func TestNewPack_ResumeKeepsTheStoredSetName(t *testing.T) { + rb := testutil.NewRecordingBot(t) + stubBotIdentity(rb) // resolves to "testbot" + setMissing(rb) + s := newTestState() + ctx := context.Background() + + // The earlier attempt ran while the bot was called something else. + const legacySet = "mypack_by_oldbot" + seedInterrupted(t, s, "mypack", legacySet) + + if err := s.handleNewPack(ctx, rb.Bot, stickerReply("/newpack mypack My Pack", otherSet)); err != nil { + t.Fatalf("handleNewPack: %v", err) + } + + pack, found := loadPack(t, s) + if !found { + t.Fatal("no record after resume") + } + if pack.Name != legacySet { + t.Errorf("set name = %q, want the stored %q — the earlier attempt's set is now orphaned", pack.Name, legacySet) + } +} diff --git a/plans/reports/verify-260825-1700-sticker-round6.md b/plans/reports/verify-260825-1700-sticker-round6.md new file mode 100644 index 0000000..9f262f7 --- /dev/null +++ b/plans/reports/verify-260825-1700-sticker-round6.md @@ -0,0 +1,310 @@ +# Adversarial verification — sticker module, round 6 + +- Commit under review: `b7803ce` ("fix(sticker): prove authority before a confirmed pack delete") +- Branch: `feature/sticker-pack-module`, Go 1.27, golangci-lint v2.13.1 +- Method: static enumeration + driven end-to-end probes + mutation testing. + All source mutations were backed up, restored, and verified byte-identical + (`git status --short` empty, md5sums match baseline). + +## Verdict + +**The security fix is correct.** I could not reach any of the seven +owner-unscoped Telegram mutations with authority the caller does not hold, +under serial dispatch or under forced concurrency. The R5 hole +(`DeleteStickerSet` via a stale confirmation) is closed twice over, and I +confirmed by driven probe that the allowlist alone still holds in the one +production state where change 2 fails. + +**The recurring pattern did repeat, one level down.** The flagship new test is +vacuous with respect to the guard it is named for, and the two disjuncts of the +new allowlist that the commit message itself identifies as the R5 bug are +completely unpinned. Nothing in the suite stops this fix from regressing back +into exactly the blocklist it replaced. + +That is a test-integrity defect, not a live exploit. See "Merge position". + +--- + +## 1. Enumeration of every owner-unscoped Telegram mutation + +`grep` over `internal/modules/sticker/*.go` (non-test) yields exactly these +mutating calls. For each: what proves ownership at the moment of the call, and +whether that proof can go stale or be manufactured. + +| Call | Site | Proof of authority at call time | Can it go stale / be forged? | +|---|---|---|---| +| `DeleteStickerSet` | `delpack_callback.go:210` | Under `lockUser`, immediately before the call: `found && !current.Pending && ownsSet(current, action.SetName)` re-read from the store | **No.** Non-`Pending` records are written only by `finishNewPack` (after a successful `CreateNewStickerSet`) and by `commitPack` from `adjustCount`/`handleRenamePack`, both of which copy an existing record's `Name`. So a non-`Pending` record proves this owner created that set. Gap between check and call is one store `Delete` on the pending key, no dispatch point. | +| `CreateNewStickerSet` | `pack_handlers.go:370` | `GetStickerSet(pack.Name)` must positively return `STICKERSET_INVALID` in the same handler | No adoption path remains; `err == nil` (occupied) drops the intent and releases the reservation. Verified by probe C below. | +| `SetStickerSetTitle` | `pack_handlers.go:591` | `getPack(ownerID)` found and `!Pending`; `Name` taken from that record | Owner-keyed read, same handler. Read happens *before* `lockUser` — hypothetical-concurrency only (see L2). | +| `SetStickerSetThumbnail` | `setpackicon.go:44` | `resolveOwned` → `ownsSet(pack, replied.Sticker.SetName)`; `Name` from the caller's own record | Slow media leg sits between check and call, but no dispatch point under serial dispatch. | +| `AddStickerToSet` | `sticker_handlers.go:69` | `getPack(ownerID)` found and `!Pending`; `UserID` is always the caller, `Name` from the caller's record | Same shape. | +| `DeleteStickerFromSet` | `sticker_handlers.go:122` | `resolveOwned` → `ownsSet(pack, st.SetName)` on the replied sticker | `st.SetName` and `st.FileID` come from the same Telegram-rendered `Sticker`; not client-forgeable. Old scrollback stickers from a deleted-then-reclaimed pack are stopped because the record is dropped alongside the set. | +| `SetStickerEmojiList` | `sticker_handlers.go:175` | `resolveOwned` | Same. | +| `SetStickerPositionInSet` | `sticker_handlers.go:214` | `resolveOwned` | Same. | + +**No deferred capability exists anywhere except `PendingDelete`.** Every other +command resolves authority and consumes it inside the same handler invocation, +so the stale-authority shape found at `/delpack` has no sibling at +`/addsticker`, `/delsticker`, `/editsticker`, `/ordersticker`, `/setpackicon` +or `/renamepack`. I drove `/addsticker` and `/delsticker` end to end against a +record that had moved on; both refuse at `resolveOwned`/`getPack`. + +## 2. Attacks driven end to end (probe results) + +Probes were written as a temporary test file, run, and removed. + +| Probe | Setup | Result | +|---|---|---| +| **A** — record gone, confirmation alive | non-`Pending` pack + live `PendingDelete`, record deleted out from under it, victim seeded holding `mypack_by_testbot` | `methods = [editMessageReplyMarkup answerCallbackQuery]`. **0 `deleteStickerSet`.** Victim record intact. Allowlist holds. | +| **A2** — record moved to `Pending`, confirmation alive | same, record replaced with a `Pending` intent naming the same set | **0 `deleteStickerSet`.** | +| **C** — `/delpack` on a `Pending` record frees a name with a live set behind it, next claimant attacks | Bob's interrupted attempt created the set; Bob `/delpack` (frees `mypack`); Alice `/newpack mypack` | Alice: `[getMe getStickerSet sendMessage]`, reply `"That pack name is taken."` No record, no adoption. Alice's follow-up `/delpack`: `"You don't have a pack yet."`, 0 API calls. **`createPack`'s occupancy probe is the wall and it holds.** | +| **E** — `dropPackRecord` with a failing pending store | `pending.Delete` returns an error | Record deleted, reservation released, **confirmation survives**. This is the state that makes the allowlist's `!found` disjunct load-bearing in production. | +| **G** — can a `Pending` record coexist with a live confirmation via handlers alone? | `/delpack` (prompt live) then `/newpack second Two` | Refused at the precheck: `"You already have a pack (mypack)."` Not reachable through handlers — but *is* reachable after an E-style failed clear. | +| **Concurrency** — 3 goroutines (`handleDelPackCallback` + `handleDelPack` + `handleAddSticker`) × 50 iterations × 3 runs, `-race` | | No races, never more than one `deleteStickerSet`. | + +Dispatch model re-confirmed serial: `internal/telegram/client.go:27-28` +(`WithSkipGetMe`, `WithNotAsyncHandlers`), no `WithWorkers` anywhere, so the +library default of one worker applies. All concurrency observations below are +labelled hypothetical. + +## 3. Mutation testing + +Backup → mutate → `go test ./internal/modules/sticker/` → restore. + +| # | Mutation | Outcome | Killing test | +|---|---|---|---| +| 1 | Allowlist reverted to the R5 blocklist (`found && current.Pending && ownsSet(...)`) | **KILLED** | `TestDelPackCallback_StalePressLeavesTheCurrentPackAlone` (`delpack_callback_test.go:286`) — *only* this one | +| 2 | `dropPendingDelete` removed from `dropPackRecord` | **KILLED** | `TestDropPackRecord_ClearsAnOutstandingConfirmation` (`pack_handlers_test.go:874`) — *only* this one | +| 3 | Resume returns `existing` verbatim (both carry-overs removed) | **KILLED** | `TestNewPack_ResumeUsesTheTitleJustTyped` | +| 3b | Only `resumed.Name = intent.Name` removed | **SURVIVED** | — | +| 4 | `releaseSlug` removed from `resolveStaleIntent`'s `isStickerSetMissing` branch | **KILLED** | `TestNewPack_DifferentSlugReplacesDeadIntent` (`pack_handlers_test.go:171`) | +| 5 | Allowlist disjunct `!found` dropped (`_ = found`) | **SURVIVED** | — | +| 6 | Allowlist disjunct `current.Pending` dropped | **SURVIVED** | — | +| 7 | Allowlist disjunct `!ownsSet(current, action.SetName)` dropped | **KILLED** | `TestDelPackCallback_StalePressLeavesTheCurrentPackAlone` | +| 8 | Mutations 1 **and** 2 together | **KILLED** | all three of `StalePressLeavesTheCurrentPackAlone`, `StalePressCannotDeleteTheNextHolder`, `DropPackRecord_ClearsAnOutstandingConfirmation` | + +Gates: `go build ./...` OK · `go test ./... -race -count=20` OK · +`golangci-lint run ./...` → `0 issues.` · `gofmt -l .` → clean. + +--- + +## Findings + +### H1 — The flagship round-6 test does not exercise the round-6 guard (High, test integrity) + +`TestDelPackCallback_StalePressCannotDeleteTheNextHolder` +(`internal/modules/sticker/delpack_callback_test.go:411-453`) is documented as +the regression test for the allowlist. It is not. + +Reproduction (mutation 1, in isolation): + +``` +$ # revert the allowlist to the R5 blocklist, nothing else +$ go test ./internal/modules/sticker/ -run TestDelPackCallback_StalePressCannotDeleteTheNextHolder -v +--- PASS: TestDelPackCallback_StalePressCannotDeleteTheNextHolder (0.00s) +``` + +Cause: at line 434 the test calls + +```go + // U's set vanishes at Telegram; a self-heal clears the record and the name. + s.dropPackRecord(ctx, testUser) +``` + +`dropPackRecord` now (change 2) deletes the `PendingDelete` as well, so the +callback returns at `delpack_callback.go:124` +(`pending.Get` → `storage.ErrNotFound` → `"This confirmation expired or was +already used."`) roughly fifty lines before the allowlist at line 193. The test +proves change 2, and only change 2 — which is already proven by +`TestDropPackRecord_ClearsAnOutstandingConfirmation`. + +Mutation 2 in isolation also leaves this test **passing** (the allowlist then +catches it). Only the double revert (mutation 8) fails it. A test that requires +both defences to be removed before it fires cannot detect either one +regressing. + +This is the fourth consecutive round in which a test was named for a behaviour +a structurally earlier guard prevents it from reaching. + +**Fix:** seed the state directly instead of routing through `dropPackRecord` — +`s.store.Delete(ctx, packKey(testUser))` and leave the confirmation in place — +so the press actually arrives at the under-lock re-check. Probe A above is a +working version of that test; it passes on `HEAD` and fails under mutation 1. + +### H2 — The `!found` disjunct is unpinned, and the state it guards is production-reachable (High) + +Mutation 5 (`_ = found; if current.Pending || !ownsSet(current, action.SetName)`) +survives the entire suite. That disjunct is the exact half of the R5 bug the +commit message calls out first ("fell through on the two that mattered: no +record at all"). + +It is not dead code. `dropPackRecord` logs and continues when the pending +delete cannot be removed: + +```go +func (s *state) dropPendingDelete(ctx context.Context, key string) { + commitCtx, cancel := commitContext(ctx) + defer cancel() + if err := s.pending.Delete(commitCtx, key); err != nil && !errors.Is(err, storage.ErrNotFound) { + log.Error("sticker_drop_pending_delete", "err", err) + } +} +``` + +Probe E confirms the resulting state on `HEAD`: pack record gone, reservation +released, confirmation still live and pressable. Probe A confirms `!found` is +what refuses the press in that state, and that without it the press lands on +whoever holds the name now. A single Mongo write failure is enough to enter it. + +**Fix:** add the probe-A test (record deleted directly, confirmation left +alive, victim seeded under the same name, assert zero `deleteStickerSet`). + +### H3 — The `current.Pending` disjunct is unpinned (Medium-High) + +Mutation 6 survives. Reachable in production by composing H2 with a normal +`/newpack`: once a failed `dropPendingDelete` has left a confirmation alive +with no record, `/newpack` passes the precheck and writes a fresh `Pending` +intent. A `Pending` record is bookkeeping written *before* Telegram is called — +`handleDelPack` and `TestDelPack_PendingRecordDeletesNothingAtTelegram` both +say so explicitly — so it must never authorise a delete. Probe A2 shows the +guard works today; nothing pins it. + +**Fix:** add probe A2 as a test. + +### M1 — `resumed.Name = intent.Name` is unpinned and repoints the record at a different set under a bot rename (Medium) + +`pack_handlers.go`, `claimSlug` resume branch: + +```go + resumed := existing + resumed.Title = intent.Title + resumed.Name = intent.Name + return resumed, false, nil +``` + +Mutation 3b (removing only the `Name` line) survives the whole suite — the +title carry-over is the only half the new test covers, despite the `Name` line +being the only one of the two that changes *which set* is touched. + +Probe B, driven end to end: seed an interrupted attempt with +`Name = "mypack_by_oldbot"` (bot renamed in BotFather since), stub `getMe` → +`testbot`, re-run `/newpack mypack Title`: + +``` +probed name = "mypack_by_testbot" +created name = "mypack_by_testbot" +stored pack = {Slug:mypack Name:mypack_by_testbot ... Pending:false} +``` + +Before `b7803ce` the probe targeted `mypack_by_oldbot`. If the interrupted +attempt did create that set, the old behaviour answered `slugTaken` and cleaned +up; the new behaviour creates a second set and orphans the first with no local +record pointing at it and no route to reach it through the bot. The `mypack` +reservation stays held (same slug), so no cross-user damage — this is a +resource leak and a behaviour regression, not a security defect. + +It also directly contradicts the invariant `ownsSet`'s own doc comment states +(`setname.go:73-79`): "It deliberately does not re-derive the name from the +live bot username. Renaming the bot in BotFather is supported and leaves +existing set names untouched." The resume branch now re-derives it. + +**Fix:** either drop the `Name` carry-over (the title fix is what the commit +message describes; the `Name` line is unexplained scope), or keep it and add a +test that pins the intent under a changed username. As written it is an +unexplained, untested line inside a security-sensitive commit. + +### L1 — `ownsSet` uses Unicode case folding on a security comparison (Low, informational) + +`strings.EqualFold` applies simple Unicode folding, so +`ownsSet(Pack{Name: "mypack_by_testbot"}, "mypacKk_by_testbot")` (U+212A +KELVIN SIGN) returns **true** — verified by probe F. Not exploitable: Telegram +constrains sticker-set short names to `[A-Za-z0-9_]`, `validateSlug` forces +`^[a-z][a-z0-9_]{2,39}$`, and the only two inputs are a stored record name and +a Telegram-rendered `Sticker.SetName`. Recording it because the comment +justifies `EqualFold` on casing grounds alone and does not note the folding +surface it brings along. `strings.ToLower` comparison would be equally correct +and narrower. + +### L2 — Read-modify-write outside the lock in four handlers (Low, hypothetical concurrency) + +`state.go`'s corrected `lockUser` comment says the lock "stays because every +mutation here is a read-modify-write, which is wrong the moment dispatch stops +being serial." Four handlers do not honour that: + +- `handleDelPack` — reads the pack, then writes `s.pending`, with **no lock at + all** on the prompt path (the lock is taken only inside the `pack.Pending` + branch). +- `handleAddSticker`, `handleDelSticker`, `handleRenamePack` — `getPack` runs + *before* `defer s.lockUser(ownerID)()`, so the record they act on was read + outside the critical section. + +Only `handleNewPack` takes the lock first. Moot under +`WithNotAsyncHandlers` + one worker; flagged because the comment asserts a +property the code does not have, which is precisely the class of defect change +6 was written to fix. + +### L3 — `internal/keylock` package doc contradicts the dispatch model (Low, out of scope) + +`internal/keylock/keylock.go:6-8`: "The bot dispatcher runs each Telegram update +in its own goroutine". `internal/telegram/client.go:18-22` and +`internal/modules/dispatcher.go:124-126` both say the opposite, and change 6 +corrected `state.go` to match. Same wrong-reason-for-a-right-guard shape, one +package over. Not this commit's responsibility; worth a follow-up. + +--- + +## Previously closed classes — re-confirmed still closed + +| Class | Evidence | +|---|---| +| Post-wipe adoption | No adoption branch remains (`createPack` has only `occupied → refuse` / `missing → create` / `unknown → abort`). `TestNewPack_WipedStoreCannotAdoptSurvivingPack` passes; mutation of the occupancy branch is out of scope but the branch is asserted on directly. | +| Inconclusive probe then live set | `TestNewPack_InconclusiveProbeThenLiveSetCannotTakeOver` passes; the guard it defeated no longer exists (refusal is unconditional). | +| Pending record as delete authority | `handleDelPack` refuses to prompt for a `Pending` record and drops it locally; `TestDelPack_PendingRecordDeletesNothingAtTelegram` passes; the callback's `current.Pending` disjunct is a second wall (probe A2). | +| Name-burning DoS | Precheck ordering (record read before `reserveSlug`) intact; `TestNewPack_RefusedRunsClaimNoNames` and `TestNewPack_FreshReservationReleasedWhenClaimBails` cover it. | +| Cross-user `releaseSlug` | Ownership verified inside the operation, not the caller; `TestReleaseSlug_RefusesANameHeldBySomeoneElse` passes. | + +## Change 2 audit (`dropPackRecord` now writes `s.pending`) + +Every call site passes an owner the caller already owns — no cross-user aim is +possible: + +| Call site | `ownerID` source | +|---|---| +| `handleDelPack` (pending branch) | `senderID(msg)` | +| `handleRenamePack` (`isStickerSetMissing`) | `senderID(msg)` | +| `handleAddSticker` / `handleDelSticker` / `handleEditSticker` / `handleOrderSticker` / `handleSetPackIcon` (`isStickerSetMissing`) | `senderID(msg)` | +| `dropPackRecordIfSet` ← delpack callback success | `action.OwnerID`, and the callback already proved `query.From.ID == action.OwnerID` and loaded the action under the presser's own key | + +`senderID` additionally rejects bots, anonymous group admins +(`SenderChat != nil`) and `From.ID == 0`, so `pendingDeleteKey` can never be +built from the shared `GroupAnonymousBot` identity. Failure of the added write +is logged and non-fatal, leaving the record deleted and the confirmation alive +— the H2 state, which the allowlist covers. + +## Merge position + +`b7803ce` is a genuine, correct security fix and I would not block it on +correctness. What I do block on is the test claim: the commit ships a test +named for the guard it introduces, that guard can be fully reverted with the +test still green, and two of the guard's three load-bearing disjuncts have zero +coverage. Given five prior rounds where a false-clean was produced by exactly +this — a same-named test that never reaches the branch — the fix should not +land with its own regression detector inoperative. + +Blocking work is small and mechanical: replace `s.dropPackRecord(ctx, testUser)` +in `StalePressCannotDeleteTheNextHolder` with a direct `s.store.Delete`, and add +the probe-A2 variant. Both are ten-line changes and both fail on `HEAD` under +the corresponding mutation. + +M1 (`resumed.Name`) should be resolved before merge too — decided either way, +but not left as an untested, undescribed line in a commit about proving +authority. + +## Unresolved questions + +1. Is `resumed.Name = intent.Name` intentional, and if so what should happen to + a set stranded under the pre-rename name? The commit message describes only + the title fix. +2. Does Telegram reserve a deleted sticker set's short name? Plan note R11 + still marks this unverified, and `dropPackRecord`'s release-the-name + behaviour is documented as a no-op if it does. Unchanged by this commit.