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.
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. |
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 |
| 8 | Mutations 1 **and** 2 together | **KILLED** | all three of `StalePressLeavesTheCurrentPackAlone`, `StalePressCannotDeleteTheNextHolder`, `DropPackRecord_ClearsAnOutstandingConfirmation` |
Gates: `go build ./...` OK · `go test ./... -race -count=20` OK ·
*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
| `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.
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.