mirror of
https://github.com/tiennm99/tiennm99bot.git
synced 2026-10-13 22:16:33 +00:00
fix(sticker): never adopt an existing pack
Two ordinary /newpack commands could take over a stranger's pack. The first probe returns an inconclusive error, which correctly keeps the reservation so the user can retry - but that turned a fresh claim into a resumed one and defeated the guard that made adoption conditional. resolveStaleIntent had a second adopt path that never consulted the guard at all. Both are reproduced by tests added here. This is the fourth failure of the same mechanism, and it is structural. Adoption must prove "this set is mine to finish" from local state, and local state is what a restart on the in-memory backend erases while the packs at Telegram survive. With the proof gone, a genuine interrupted attempt and a stranger naming a public share link are indistinguishable. Remove adoption entirely. /newpack refuses any name a set already occupies, and leaves no intent or reservation behind when it does. A pending record is not evidence of ownership either: anyone can make one naming any set, and DeleteStickerSet is keyed by set name, which Telegram authorises for every set this bot created. /delpack therefore clears a pending record locally and contacts Telegram only for a confirmed one. The cost is that a crash between creating a set and recording it strands that set. That is documented rather than mitigated - every mitigation available is the mechanism that just failed. Also drop a test whose name claimed to pin the resumed-reservation distinction but bailed past the code that implements it, rename a delpack test after the guard that actually stops a foreign presser, and pin releaseSlug's ownership check and detached read - the latter needed a context-honouring store, since the in-memory one ignores cancellation and made the first version of that test vacuous.
This commit is contained in:
tiennm99 committed
1 parent
a077b75d04
commit
1810c1ee47
9 files changed
+854
-144
No files matched your search
+26
-13
@@ -97,9 +97,8 @@ never says *who* holds a name. Refusals about *managing* a pack are deliberately
|
||||
uniform for the opposite reason — see below.
|
||||
|
||||
A name is claimed *before* the bot calls Telegram, not after the pack exists.
|
||||
That ordering is the point: the claim is what proves, on a later re-run, that an
|
||||
existing set under that name is yours to finish rather than someone else's to
|
||||
take.
|
||||
That ordering is what keeps two users from racing for the same name: the first
|
||||
claimant wins it and everyone else is refused before any set is created.
|
||||
|
||||
The claim is given up again whenever the bot has positive evidence that no pack
|
||||
stands behind it — Telegram refusing the creation outright, `/delpack`, or a
|
||||
@@ -107,6 +106,9 @@ later command finding the set already gone. A `/newpack` that never got as far
|
||||
as claiming, or that is refused before Telegram is contacted, leaves nothing
|
||||
behind.
|
||||
|
||||
The claim is deliberately **not** treated as proof of ownership over a set that
|
||||
already exists. See "The bot never takes over an existing pack" below.
|
||||
|
||||
Telegram may keep a deleted short name reserved on its own side, so a freed name
|
||||
is not guaranteed to be usable again by anyone, including its previous owner.
|
||||
|
||||
@@ -114,17 +116,28 @@ is not guaranteed to be usable again by anyone, including its previous owner.
|
||||
"that sticker isn't from your pack" produce the exact same reply. Distinct
|
||||
wording would let anyone probe which sets exist under this bot.
|
||||
|
||||
**An interrupted `/newpack` can be finished.** The bot records your intent
|
||||
before calling Telegram, so if a deploy or crash lands mid-creation, re-running
|
||||
the same `/newpack` command completes it instead of reporting the name taken.
|
||||
`/mypack` marks an unfinished attempt so it is visible rather than mysterious.
|
||||
**The bot never takes over an existing pack.** If a set already exists under the
|
||||
name you ask for, `/newpack` refuses — always, for everyone, whatever the bot's
|
||||
records say about it.
|
||||
|
||||
This depends on the bot still holding your claim to the name. If its storage has
|
||||
been wiped since — which is what a restart does when no database is configured —
|
||||
the claim is gone while the pack at Telegram is not, and `/newpack` reports the
|
||||
name as taken rather than adopting a set it can no longer prove is yours.
|
||||
Recovering a pack in that state needs operator help. Run this module against a
|
||||
real database, not the in-memory backend.
|
||||
Earlier versions adopted such a set when local records suggested it came from
|
||||
your own interrupted attempt. That was wrong in a way no amount of checking
|
||||
fixes: every fact the bot could use to prove "this set is yours" lives in the
|
||||
same storage a restart erases, while the packs at Telegram survive. Once the
|
||||
proof is gone, a genuine interrupted attempt and a stranger naming your pack's
|
||||
public link present the bot with identical evidence. The feature was the hole,
|
||||
so the feature is gone.
|
||||
|
||||
The cost is real and worth stating plainly: if the bot crashes between creating
|
||||
your set at Telegram and recording it, the set is stranded. It exists, it is
|
||||
linkable, and no command in this bot can manage or delete it. `/mypack` marks
|
||||
the unfinished attempt, and `/delpack` clears the leftover record so you can
|
||||
create a pack under a different name — but it will not delete anything at
|
||||
Telegram, because an unfinished record is not evidence that the bot made that
|
||||
set for you. Anyone can produce such a record for any name.
|
||||
|
||||
Run this module against a real database. On the in-memory backend every restart
|
||||
strands every pack.
|
||||
|
||||
**Deleting the last sticker may delete the pack.** Telegram's behaviour here is
|
||||
undocumented, so the bot does not guess: it will not remove your pack record on
|
||||
|
||||
@@ -38,6 +38,20 @@ func (s *state) handleDelPack(ctx context.Context, b *bot.Bot, update *models.Up
|
||||
return reply(ctx, b, msg, noPackYet)
|
||||
}
|
||||
|
||||
// A pending record is bookkeeping, not proof that this bot created a set
|
||||
// under that name on this user's behalf — /newpack writes it before
|
||||
// Telegram is called, and anyone can make one naming any set. Deleting by
|
||||
// set name is authorised by Telegram for every set this bot created, so
|
||||
// confirming a delete from a pending record would let one user destroy
|
||||
// another's pack. Clear the local record only, and touch nothing upstream.
|
||||
if pack.Pending {
|
||||
defer s.lockUser(ownerID)()
|
||||
s.dropPackRecord(ctx, ownerID)
|
||||
return reply(ctx, b, msg, fmt.Sprintf(
|
||||
"Cleared an unfinished attempt at %s and freed the name. Nothing was deleted at Telegram; if that attempt did create a pack, it is no longer reachable through this bot.",
|
||||
pack.Slug))
|
||||
}
|
||||
|
||||
id, err := newActionID()
|
||||
if err != nil {
|
||||
log.Error("sticker_delpack_id", "err", err)
|
||||
@@ -115,6 +129,9 @@ func (s *state) handleDelPackCallback(ctx context.Context, b *bot.Bot, update *m
|
||||
return answerCallback(ctx, b, query.ID, "Could not load this confirmation. Try /delpack again.")
|
||||
}
|
||||
|
||||
// Unreachable by construction — the action was loaded under this presser's
|
||||
// own key, so a foreign press already returned above. Kept as defence in
|
||||
// depth against a future change to how actions are addressed.
|
||||
if query.From.ID != action.OwnerID {
|
||||
return answerCallback(ctx, b, query.ID, "Only the user who ran /delpack can confirm it.")
|
||||
}
|
||||
@@ -154,6 +171,19 @@ 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 {
|
||||
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) {
|
||||
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.")
|
||||
}
|
||||
|
||||
// Consume the action *before* the destructive call, so a double press
|
||||
// cannot delete twice or race a second confirmation.
|
||||
if err := s.pending.Delete(ctx, key); err != nil {
|
||||
|
||||
@@ -106,7 +106,14 @@ func TestDelPackCallback_HappyPath(t *testing.T) {
|
||||
|
||||
// Identity comes from From.ID, never from the payload — the payload is
|
||||
// client-controlled.
|
||||
func TestDelPackCallback_RejectsOtherUser(t *testing.T) {
|
||||
// A foreign presser gets nothing. The mechanism is the key, not the owner
|
||||
// comparison: the action is loaded by the presser's own id, so someone else's
|
||||
// press finds no action at all and returns before the owner check is reached.
|
||||
//
|
||||
// Named for that, because the previous name claimed to exercise the owner
|
||||
// comparison at delpack_callback.go and did not — that branch is unreachable
|
||||
// by construction, and is kept only as defence in depth.
|
||||
func TestDelPackCallback_ForeignPresserResolvesToNothing(t *testing.T) {
|
||||
rb := testutil.NewRecordingBot(t)
|
||||
s := newTestState()
|
||||
seedPack(t, s, 3)
|
||||
|
||||
@@ -38,12 +38,16 @@ type PackStore = storage.DocStore[Pack]
|
||||
// have a pack" — they cannot answer "who holds this name". Without that second
|
||||
// question the module cannot tell its own interrupted attempt from a set
|
||||
// belonging to someone else, because both look identical from the caller's
|
||||
// side: a pending record naming a set that exists. Adopting on that evidence
|
||||
// alone let any user take over any pack whose public link they could guess.
|
||||
// side: a pending record naming a set that exists.
|
||||
//
|
||||
// The reservation is the missing half. It is claimed with a create-only write
|
||||
// before Telegram is touched, so the first claimant of a name is the only user
|
||||
// who can ever adopt a set under it.
|
||||
// The reservation answers the question the pack record cannot: who is entitled
|
||||
// to create under a given name. It is claimed with a create-only write before
|
||||
// Telegram is touched, so the first claimant wins the name and everyone else is
|
||||
// refused before any set exists.
|
||||
//
|
||||
// It is NOT ownership of whatever set may already sit under that name. Nothing
|
||||
// in this module grants that, because nothing stored here survives the wipe
|
||||
// that would make the grant necessary.
|
||||
type SlugReservation struct {
|
||||
Slug string `bson:"slug"`
|
||||
OwnerID int64 `bson:"ownerId"`
|
||||
|
||||
@@ -39,9 +39,10 @@ const (
|
||||
// - an owner-keyed pending Pack — "does this user have a pack?" (claimSlug)
|
||||
//
|
||||
// Both are needed. The pending record alone proves only that this caller *asked
|
||||
// for* the name, which a user naming someone else's pack also does; treating it
|
||||
// as proof of ownership was a pack-takeover hole. The reservation is what makes
|
||||
// "this set is mine to adopt" a fact rather than an assumption.
|
||||
// for* the name, which a user naming someone else's pack also does. The
|
||||
// reservation settles who gets to create under a name; neither of them, nor the
|
||||
// two together, is ever treated as permission to take over a set that already
|
||||
// exists — see createPack.
|
||||
func (s *state) handleNewPack(ctx context.Context, b *bot.Bot, update *models.Update) error {
|
||||
ctx, cancel := handlerContext(ctx)
|
||||
defer cancel()
|
||||
@@ -122,22 +123,21 @@ func (s *state) handleNewPack(ctx context.Context, b *bot.Bot, update *models.Up
|
||||
return err
|
||||
}
|
||||
|
||||
return s.createOrAdopt(ctx, b, msg, claimed, source, created)
|
||||
return s.createPack(ctx, b, msg, claimed, source)
|
||||
}
|
||||
|
||||
// reserveSlug claims a pack name for ownerID, globally and permanently.
|
||||
//
|
||||
// Pack records are keyed by owner, so they answer "does this user have a pack"
|
||||
// and nothing else. That is not enough to make adoption safe: a user with no
|
||||
// pack who names someone else's slug produces exactly the same evidence as a
|
||||
// user resuming their own interrupted attempt — a pending record naming a set
|
||||
// that exists. Adopting on that evidence let anyone take over any pack whose
|
||||
// public link they could guess, and every share link is public.
|
||||
// and nothing else — a user naming someone else's slug produces exactly the
|
||||
// same record as one resuming their own interrupted attempt. The reservation
|
||||
// adds the missing fact about the *name*: who is entitled to create under it.
|
||||
//
|
||||
// A create-only write on the name itself supplies the missing fact. The first
|
||||
// claimant is the only user who can ever adopt a set under that name, so by the
|
||||
// time createOrAdopt sees an existing set, "it is ours" has actually been
|
||||
// proven rather than assumed.
|
||||
// A create-only write on the name itself is what keeps two users from racing
|
||||
// for the same name: the first claimant wins it, and everyone else is refused
|
||||
// before any set is created. It is deliberately NOT treated as proof of
|
||||
// ownership over a set that already exists — see createPack for why no local
|
||||
// fact can carry that weight.
|
||||
//
|
||||
// created reports whether this call wrote the reservation, so a caller that
|
||||
// bails can release exactly what it made and never a reservation it merely
|
||||
@@ -254,11 +254,9 @@ func (s *state) claimSlug(ctx context.Context, b *bot.Bot, msg *models.Message,
|
||||
return existing, false, nil
|
||||
|
||||
default:
|
||||
// An earlier attempt was interrupted under a *different* name. Probing
|
||||
// first is what makes this safe: if that set was already created,
|
||||
// overwriting the record here would orphan it permanently — the set
|
||||
// would exist, owned by this user, with adoption keyed on a slug the
|
||||
// record no longer holds.
|
||||
// An earlier attempt was interrupted under a *different* name. Probe
|
||||
// before overwriting, so a set that attempt did create is logged as
|
||||
// stranded rather than silently forgotten.
|
||||
return s.resolveStaleIntent(ctx, b, msg, existing, intent)
|
||||
}
|
||||
}
|
||||
@@ -266,19 +264,19 @@ func (s *state) claimSlug(ctx context.Context, b *bot.Bot, msg *models.Message,
|
||||
// resolveStaleIntent decides what to do with a pending record for a slug the
|
||||
// caller is no longer asking for.
|
||||
func (s *state) resolveStaleIntent(ctx context.Context, b *bot.Bot, msg *models.Message, existing, intent Pack) (Pack, bool, error) {
|
||||
// The old name is only adoptable if this caller reserved it too. They
|
||||
// normally did — the pending record came from their own earlier run through
|
||||
// reserveSlug — but adoption is the dangerous operation in this module, so
|
||||
// it is re-proven rather than inferred from the pending record.
|
||||
// Establish whether this caller still holds the old name before touching
|
||||
// anything under it. They normally do — the pending record came from their
|
||||
// own earlier run through reserveSlug — but it is re-read rather than
|
||||
// inferred from the pending record.
|
||||
held, found, err := getSlugReservation(ctx, s.slugs, existing.Slug)
|
||||
if err != nil {
|
||||
log.Error("sticker_newpack_stale_reservation", "err", err)
|
||||
return Pack{}, true, reply(ctx, b, msg, genericFailure)
|
||||
}
|
||||
if !found || held.OwnerID != intent.OwnerID {
|
||||
// Someone else holds the old name, or it was released. Either way this
|
||||
// caller cannot adopt under it; drop the dead intent and let them
|
||||
// proceed with the name they actually asked for.
|
||||
// Someone else holds the old name, or it was released. Either way the
|
||||
// old intent is dead; replace it and let the caller proceed with the
|
||||
// name they actually asked for.
|
||||
if putErr := s.store.Put(ctx, packKey(intent.OwnerID), intent); putErr != nil {
|
||||
log.Error("sticker_newpack_replace_intent", "err", putErr)
|
||||
return Pack{}, true, reply(ctx, b, msg, genericFailure)
|
||||
@@ -289,19 +287,21 @@ func (s *state) resolveStaleIntent(ctx context.Context, b *bot.Bot, msg *models.
|
||||
_, err = b.GetStickerSet(ctx, &bot.GetStickerSetParams{Name: existing.Name})
|
||||
switch {
|
||||
case err == nil:
|
||||
// The old set exists — adopt it rather than stranding it.
|
||||
adopted := existing
|
||||
adopted.Pending = false
|
||||
if adopted.Count == 0 {
|
||||
adopted.Count = 1
|
||||
}
|
||||
if commitErr := s.commitPack(ctx, adopted); commitErr != nil {
|
||||
log.Error("sticker_newpack_adopt_commit", "err", commitErr)
|
||||
// The old name is occupied. This branch used to adopt, on the strength
|
||||
// of a reservation it re-read here — and it never consulted the newer
|
||||
// per-invocation guard at all, so it stayed a takeover route after that
|
||||
// guard was added. No adoption happens anywhere in this module now.
|
||||
//
|
||||
// The old reservation stays: a set genuinely exists under that name, so
|
||||
// the name is not free, and releasing it would only send the next
|
||||
// caller down this same refusal. The dead intent is replaced so the
|
||||
// caller can get on with the name they actually asked for.
|
||||
if putErr := s.store.Put(ctx, packKey(intent.OwnerID), intent); putErr != nil {
|
||||
log.Error("sticker_newpack_replace_intent", "err", putErr)
|
||||
return Pack{}, true, reply(ctx, b, msg, genericFailure)
|
||||
}
|
||||
return Pack{}, true, reply(ctx, b, msg, fmt.Sprintf(
|
||||
"You already have a pack (%s) from an earlier attempt — it has been restored.\n%s\nUse /delpack first if you want a different name.",
|
||||
adopted.Slug, shareLink(adopted.Name)))
|
||||
log.Error("sticker_newpack_stranded_set", "slug", existing.Slug, "owner", existing.OwnerID)
|
||||
return intent, false, nil
|
||||
|
||||
case isStickerSetMissing(err):
|
||||
// Nothing was created under the old name; take over the record. A
|
||||
@@ -322,45 +322,29 @@ func (s *state) resolveStaleIntent(ctx context.Context, b *bot.Bot, msg *models.
|
||||
}
|
||||
}
|
||||
|
||||
// createOrAdopt performs the Telegram-side creation for a claimed intent, or
|
||||
// adopts a set an interrupted attempt already created.
|
||||
// createPack performs the Telegram-side creation for a claimed intent.
|
||||
//
|
||||
// freshReservation reports that *this* invocation first claimed the name, which
|
||||
// is what disqualifies adoption: see the err == nil branch.
|
||||
func (s *state) createOrAdopt(ctx context.Context, b *bot.Bot, msg *models.Message, pack Pack, source stickerSource, freshReservation bool) error {
|
||||
// It never takes over a set that already exists, for anybody, under any local
|
||||
// evidence. Adoption was the source of four consecutive takeover holes, and the
|
||||
// reason is structural rather than a bug that can be patched: every fact this
|
||||
// module could use to prove "that set is mine to finish" lives in the same
|
||||
// store that a restart on the in-memory backend erases, while the packs at
|
||||
// Telegram survive. Once the proof is gone, a real interrupted attempt and an
|
||||
// attacker naming a victim's public slug present identical evidence.
|
||||
//
|
||||
// Removing the branch removes the class. Nothing local can be forged into
|
||||
// rights over an existing set, because no local fact grants them.
|
||||
func (s *state) createPack(ctx context.Context, b *bot.Bot, msg *models.Message, pack Pack, source stickerSource) error {
|
||||
_, err := b.GetStickerSet(ctx, &bot.GetStickerSetParams{Name: pack.Name})
|
||||
switch {
|
||||
case err == nil:
|
||||
// A set exists under this name. Adopting it grants full control -
|
||||
// DeleteStickerSet, DeleteStickerFromSet and SetStickerSetTitle are all
|
||||
// keyed by set name alone, with no owner scoping - so this branch must
|
||||
// prove the set is the caller's own interrupted attempt, not merely
|
||||
// assume it.
|
||||
//
|
||||
// The reservation proves it only if it outlived the set. It does not
|
||||
// always: the reservation lives in our store and the set lives at
|
||||
// Telegram, and the store can be wiped (a restart on the in-memory
|
||||
// backend does exactly that) while every pack survives. Then the
|
||||
// evidence a takeover produces and the evidence a real resume produces
|
||||
// are once again identical.
|
||||
//
|
||||
// freshReservation separates them without any new state. A genuine
|
||||
// interrupted attempt reserved the name *before* creating the set, so it
|
||||
// re-enters here having found its own reservation, never having made
|
||||
// one. Claiming the name for the first time therefore proves the set
|
||||
// under it is somebody else's.
|
||||
if freshReservation {
|
||||
log.Error("sticker_newpack_adopt_refused", "slug", pack.Slug, "owner", pack.OwnerID)
|
||||
// Leave nothing behind. The intent records an attempt that is not
|
||||
// going to happen, and the reservation names a set this caller has
|
||||
// just been refused — holding either would deny the name to the
|
||||
// set's real owner, who after a store wipe has to re-register it
|
||||
// exactly as this caller tried to.
|
||||
s.dropIntent(ctx, pack.OwnerID)
|
||||
s.releaseSlug(ctx, pack.OwnerID, pack.Slug)
|
||||
return reply(ctx, b, msg, slugTaken)
|
||||
}
|
||||
return s.finishNewPack(ctx, b, msg, pack, true)
|
||||
// Occupied. Leave nothing behind: a pending record naming a set this
|
||||
// caller does not own is itself a weapon, because /delpack deletes by
|
||||
// set name and Telegram authorises that on any set this bot created —
|
||||
// including the real owner's.
|
||||
s.dropIntent(ctx, pack.OwnerID)
|
||||
s.releaseSlug(ctx, pack.OwnerID, pack.Slug)
|
||||
return reply(ctx, b, msg, slugTaken)
|
||||
|
||||
case isStickerSetMissing(err):
|
||||
// Free: create it.
|
||||
@@ -390,18 +374,19 @@ func (s *state) createOrAdopt(ctx context.Context, b *bot.Bot, msg *models.Messa
|
||||
if err != nil {
|
||||
// Only a refusal that proves nothing was created lets us undo the
|
||||
// claim. On anything else the create may have succeeded server-side, so
|
||||
// both the intent and the reservation stay and a re-run adopts the set.
|
||||
// both the intent and the reservation stay — a re-run then reports the
|
||||
// name as taken rather than guessing, which is the safe direction.
|
||||
if createRefused(err) {
|
||||
s.dropIntent(ctx, pack.OwnerID)
|
||||
s.releaseSlug(ctx, pack.OwnerID, pack.Slug)
|
||||
}
|
||||
return replyAPIError(ctx, b, msg, "sticker_newpack_create", err)
|
||||
}
|
||||
return s.finishNewPack(ctx, b, msg, pack, false)
|
||||
return s.finishNewPack(ctx, b, msg, pack)
|
||||
}
|
||||
|
||||
// finishNewPack commits the confirmed record and replies with the share link.
|
||||
func (s *state) finishNewPack(ctx context.Context, b *bot.Bot, msg *models.Message, pack Pack, adopted bool) error {
|
||||
func (s *state) finishNewPack(ctx context.Context, b *bot.Bot, msg *models.Message, pack Pack) error {
|
||||
pack.Pending = false
|
||||
if pack.Count == 0 {
|
||||
pack.Count = 1
|
||||
@@ -410,12 +395,8 @@ func (s *state) finishNewPack(ctx context.Context, b *bot.Bot, msg *models.Messa
|
||||
log.Error("sticker_newpack_commit", "err", err)
|
||||
return reply(ctx, b, msg, genericFailure)
|
||||
}
|
||||
prefix := "Created"
|
||||
if adopted {
|
||||
prefix = "Finished an earlier attempt at"
|
||||
}
|
||||
return reply(ctx, b, msg, fmt.Sprintf("%s %s.\n%s\n\nAdd more with /addsticker while replying to a sticker.",
|
||||
prefix, pack.Title, shareLink(pack.Name)))
|
||||
return reply(ctx, b, msg, fmt.Sprintf("Created %s.\n%s\n\nAdd more with /addsticker while replying to a sticker.",
|
||||
pack.Title, shareLink(pack.Name)))
|
||||
}
|
||||
|
||||
// dropIntent removes a write-ahead record whose creation never happened, so the
|
||||
|
||||
@@ -80,58 +80,67 @@ func TestNewPack_SecondPackRefusedWithoutAPICall(t *testing.T) {
|
||||
// Re-running the same command after an interruption must complete the pack,
|
||||
// not report the slug taken. This is what keeps a crash from stranding a
|
||||
// permanent URL.
|
||||
func TestNewPack_ResumesInterruptedAttempt(t *testing.T) {
|
||||
// An interrupted attempt whose set DOES exist is refused, not resumed.
|
||||
//
|
||||
// This used to adopt. Adoption is gone: no local fact can prove a set belongs
|
||||
// to the caller, because the store holding that fact is exactly what a restart
|
||||
// on the in-memory backend erases while the packs survive. The cost is that a
|
||||
// crash between creation and commit strands the set; the benefit is that the
|
||||
// same evidence cannot be manufactured by an attacker.
|
||||
func TestNewPack_InterruptedAttemptWithLiveSetIsRefused(t *testing.T) {
|
||||
rb := testutil.NewRecordingBot(t)
|
||||
stubBotIdentity(rb)
|
||||
setExists(rb)
|
||||
s := newTestState()
|
||||
ctx := context.Background()
|
||||
|
||||
seedInterrupted(t, s, "mypack", testSet)
|
||||
|
||||
if err := s.handleNewPack(context.Background(), rb.Bot, stickerReply("/newpack mypack My Pack", otherSet)); err != nil {
|
||||
if err := s.handleNewPack(ctx, rb.Bot, stickerReply("/newpack mypack My Pack", otherSet)); err != nil {
|
||||
t.Fatalf("handleNewPack: %v", err)
|
||||
}
|
||||
|
||||
// The set already exists, so it is adopted rather than recreated.
|
||||
if countMethod(rb, "createNewStickerSet") != 0 {
|
||||
t.Errorf("methods = %v, want no create — the set already existed", methodsSent(rb))
|
||||
t.Errorf("methods = %v, want no create — a set already exists under that name", methodsSent(rb))
|
||||
}
|
||||
pack, _ := loadPack(t, s)
|
||||
if pack.Pending {
|
||||
t.Error("record still Pending after the resumed attempt")
|
||||
if got := rb.LastSent().Text(); !strings.Contains(got, slugTaken) {
|
||||
t.Errorf("reply = %q, want the name-taken refusal", got)
|
||||
}
|
||||
if strings.Contains(rb.LastSent().Text(), "taken") {
|
||||
t.Errorf("reply = %q, want a success message", rb.LastSent().Text())
|
||||
if pack, found := loadPack(t, s); found && !pack.Pending {
|
||||
t.Errorf("adopted the existing set: %+v", pack)
|
||||
}
|
||||
// Nothing left behind: a pending record naming a set the caller may not own
|
||||
// is a delete primitive, since /delpack deletes by set name.
|
||||
if _, found := loadPack(t, s); found {
|
||||
t.Error("refusal kept the intent — /delpack could then aim it at that set")
|
||||
}
|
||||
if _, held, _ := getSlugReservation(ctx, s.slugs, "mypack"); held {
|
||||
t.Error("refusal kept the reservation")
|
||||
}
|
||||
}
|
||||
|
||||
// A pending record under a *different* slug whose set exists must be adopted,
|
||||
// not overwritten: overwriting orphans that set permanently, because adoption
|
||||
// keys on the slug matching.
|
||||
func TestNewPack_DifferentSlugAdoptsExistingSet(t *testing.T) {
|
||||
// A pending record under a *different* slug whose set exists is likewise not
|
||||
// adopted; the caller proceeds under the name they asked for, and the stranded
|
||||
// name stays reserved because a set really does occupy it.
|
||||
func TestNewPack_DifferentSlugDoesNotAdoptExistingSet(t *testing.T) {
|
||||
rb := testutil.NewRecordingBot(t)
|
||||
stubBotIdentity(rb)
|
||||
setExists(rb)
|
||||
s := newTestState()
|
||||
ctx := context.Background()
|
||||
|
||||
seedInterrupted(t, s, "oldslug", testSet)
|
||||
|
||||
if err := s.handleNewPack(context.Background(), rb.Bot, stickerReply("/newpack newslug New", otherSet)); err != nil {
|
||||
if err := s.handleNewPack(ctx, rb.Bot, stickerReply("/newpack newslug New", otherSet)); err != nil {
|
||||
t.Fatalf("handleNewPack: %v", err)
|
||||
}
|
||||
|
||||
if countMethod(rb, "createNewStickerSet") != 0 {
|
||||
t.Errorf("methods = %v, want no create", methodsSent(rb))
|
||||
if pack, found := loadPack(t, s); found && pack.Slug == "oldslug" && !pack.Pending {
|
||||
t.Errorf("adopted the old set: %+v", pack)
|
||||
}
|
||||
pack, _ := loadPack(t, s)
|
||||
if pack.Slug != "oldslug" {
|
||||
t.Errorf("slug = %q, want the adopted oldslug — the old set must not be orphaned", pack.Slug)
|
||||
}
|
||||
if pack.Pending {
|
||||
t.Error("adopted record still Pending")
|
||||
}
|
||||
if !strings.Contains(rb.LastSent().Text(), "oldslug") {
|
||||
t.Errorf("reply = %q, want it to name the adopted pack", rb.LastSent().Text())
|
||||
// The old name stays claimed: a set exists under it, so it is not free.
|
||||
if _, held, _ := getSlugReservation(ctx, s.slugs, "oldslug"); !held {
|
||||
t.Error("released a name that still has a set behind it")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -568,26 +577,12 @@ func TestNewPack_ReservationReleasedWhenClaimFails(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// A reservation the caller merely resumed must not be released when a later
|
||||
// step bails — it predates this command.
|
||||
func TestNewPack_ResumedReservationSurvivesABail(t *testing.T) {
|
||||
rb := testutil.NewRecordingBot(t)
|
||||
stubBotIdentity(rb)
|
||||
rb.FailMethod("getStickerSet", 500, `{"ok":false,"description":"upstream is unhappy"}`)
|
||||
s := newTestState()
|
||||
ctx := context.Background()
|
||||
|
||||
seedInterrupted(t, s, "mypack", testSet)
|
||||
|
||||
if err := s.handleNewPack(ctx, rb.Bot, stickerReply("/newpack mypack My Pack", otherSet)); err != nil {
|
||||
t.Fatalf("handleNewPack: %v", err)
|
||||
}
|
||||
if _, held, _ := getSlugReservation(ctx, s.slugs, "mypack"); !held {
|
||||
t.Error("a resumed reservation was released on an unknown error; the name is now claimable by others while the set may exist")
|
||||
}
|
||||
}
|
||||
|
||||
// The two tests below are the only coverage of the `created` flag itself.
|
||||
// These two are the only coverage of the `created` flag itself.
|
||||
//
|
||||
// A predecessor named ...ResumedReservationSurvivesABail sat here and did not
|
||||
// test its name: its bail happened after claimSlug, which never consults the
|
||||
// flag, so it passed with the whole distinction deleted. It duplicated
|
||||
// TestNewPack_UnknownLookupErrorAborts and has been removed.
|
||||
//
|
||||
// Both drive a bail *inside* claimSlug, which is the single place handleNewPack
|
||||
// consults `created`. The other reservation tests bail later — in createOrAdopt
|
||||
@@ -689,3 +684,169 @@ func TestNewPack_WipedStoreCannotAdoptSurvivingPack(t *testing.T) {
|
||||
t.Error("the refused attempt kept the reservation, denying the name to the set's actual owner")
|
||||
}
|
||||
}
|
||||
|
||||
// The two-command takeover: no crash, no store error, two ordinary /newpack
|
||||
// calls, and the attacker used to end up owning a stranger's pack.
|
||||
//
|
||||
// The first command's GetStickerSet is inconclusive (429, 5xx, a deadline), so
|
||||
// the module correctly keeps the intent and the reservation — re-running is how
|
||||
// a real user recovers. But that turned the attacker's *fresh* reservation into
|
||||
// a *resumed* one, which defeated the per-invocation guard that was supposed to
|
||||
// make adoption safe. The second identical command then adopted.
|
||||
//
|
||||
// The guard is gone; refusing outright is what closes this. The starting state
|
||||
// is an empty store, which is what a restart on the in-memory backend leaves
|
||||
// behind while every pack at Telegram survives.
|
||||
func TestNewPack_InconclusiveProbeThenLiveSetCannotTakeOver(t *testing.T) {
|
||||
s := newTestState()
|
||||
ctx := context.Background()
|
||||
|
||||
rb1 := testutil.NewRecordingBot(t)
|
||||
stubBotIdentity(rb1)
|
||||
rb1.FailMethod("getStickerSet", 500, `{"ok":false,"description":"upstream is unhappy"}`)
|
||||
if err := s.handleNewPack(ctx, rb1.Bot, stickerReply("/newpack mypack Mine", otherSet)); err != nil {
|
||||
t.Fatalf("first /newpack: %v", err)
|
||||
}
|
||||
|
||||
// Fresh bot: Reset() deliberately keeps registered failures.
|
||||
rb2 := testutil.NewRecordingBot(t)
|
||||
stubBotIdentity(rb2)
|
||||
setExists(rb2)
|
||||
if err := s.handleNewPack(ctx, rb2.Bot, stickerReply("/newpack mypack Mine", otherSet)); err != nil {
|
||||
t.Fatalf("second /newpack: %v", err)
|
||||
}
|
||||
|
||||
if pack, found := loadPack(t, s); found && !pack.Pending {
|
||||
t.Errorf("TAKEOVER: caller now owns %+v and can /delpack a set they never created", pack)
|
||||
}
|
||||
if got := rb2.LastSent().Text(); !strings.Contains(got, slugTaken) {
|
||||
t.Errorf("reply = %q, want the name-taken refusal", got)
|
||||
}
|
||||
}
|
||||
|
||||
// A pending record is not authority to delete a set.
|
||||
//
|
||||
// /newpack writes its intent before Telegram is called, so anyone can produce a
|
||||
// pending record naming any set. DeleteStickerSet is keyed by set name and
|
||||
// Telegram authorises it for every set this bot created — so confirming a
|
||||
// delete from a pending record would let one user destroy another's pack, with
|
||||
// no adoption needed at all.
|
||||
func TestDelPack_PendingRecordDeletesNothingAtTelegram(t *testing.T) {
|
||||
rb := testutil.NewRecordingBot(t)
|
||||
s := newTestState()
|
||||
ctx := context.Background()
|
||||
|
||||
// What a post-wipe /newpack against a stranger's slug leaves behind.
|
||||
seedInterrupted(t, s, "mypack", testSet)
|
||||
|
||||
if err := s.handleDelPack(ctx, rb.Bot, testutil.NewPrivateMessage(testUser, "/delpack")); err != nil {
|
||||
t.Fatalf("handleDelPack: %v", err)
|
||||
}
|
||||
|
||||
if n := countMethod(rb, "deleteStickerSet"); n != 0 {
|
||||
t.Errorf("deleteStickerSet calls = %d, want 0 — an unconfirmed record must not reach Telegram", n)
|
||||
}
|
||||
// It should still clean up locally, so the user is not wedged.
|
||||
if _, found := loadPack(t, s); found {
|
||||
t.Error("local record survived, so /newpack stays blocked")
|
||||
}
|
||||
if _, held, _ := getSlugReservation(ctx, s.slugs, "mypack"); held {
|
||||
t.Error("name stayed reserved with nothing behind it")
|
||||
}
|
||||
}
|
||||
|
||||
// releaseSlug verifies the holder itself rather than trusting its callers.
|
||||
//
|
||||
// A bare delete-by-name is a cross-user primitive: it is reached from seven
|
||||
// call sites, and one of them getting the owner wrong would hand a live name
|
||||
// away while the set still exists.
|
||||
func TestReleaseSlug_RefusesANameHeldBySomeoneElse(t *testing.T) {
|
||||
s := newTestState()
|
||||
ctx := context.Background()
|
||||
const holder, caller = int64(1), int64(2)
|
||||
|
||||
if err := s.slugs.Put(ctx, slugKey("mypack"),
|
||||
SlugReservation{Slug: "mypack", OwnerID: holder, CreatedAt: fixedNow.UnixMilli()}); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
s.releaseSlug(ctx, caller, "mypack")
|
||||
|
||||
held, found, err := getSlugReservation(ctx, s.slugs, "mypack")
|
||||
if err != nil {
|
||||
t.Fatalf("read back: %v", err)
|
||||
}
|
||||
if !found || held.OwnerID != holder {
|
||||
t.Error("a non-holder released the name; the holder's set is still live and the name is now claimable")
|
||||
}
|
||||
}
|
||||
|
||||
// The release must survive a cancelled request context.
|
||||
//
|
||||
// Both the ownership read and the delete run on a detached context. When only
|
||||
// the delete was detached, a shutdown mid-handler failed the read and returned
|
||||
// early — leaving a reservation with no pack and no set behind it, which no
|
||||
// code path can reach again.
|
||||
func TestReleaseSlug_CompletesOnACancelledContext(t *testing.T) {
|
||||
s := newTestState()
|
||||
seed := context.Background()
|
||||
|
||||
if err := s.slugs.Put(seed, slugKey("mypack"),
|
||||
SlugReservation{Slug: "mypack", OwnerID: testUser, CreatedAt: fixedNow.UnixMilli()}); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
// The in-memory backend ignores context entirely, so cancelling one proves
|
||||
// nothing against it — this assertion passed whether or not the code
|
||||
// detached until the store was made to honour cancellation.
|
||||
s.slugs = ctxHonouringSlugs{inner: s.slugs}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cancel() // as if SIGTERM landed mid-handler
|
||||
|
||||
s.releaseSlug(ctx, testUser, "mypack")
|
||||
|
||||
if _, held, _ := getSlugReservation(seed, s.slugs, "mypack"); held {
|
||||
t.Error("reservation survived a cancelled release; the name is stranded permanently")
|
||||
}
|
||||
}
|
||||
|
||||
// ctxHonouringSlugs makes a SlugStore respect context cancellation, which the
|
||||
// in-memory backend does not. Needed to test anything about detached contexts:
|
||||
// against the bare memory store the operation completes either way.
|
||||
type ctxHonouringSlugs struct{ inner SlugStore }
|
||||
|
||||
func (c ctxHonouringSlugs) Get(ctx context.Context, id string) (SlugReservation, int64, error) {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return SlugReservation{}, 0, err
|
||||
}
|
||||
return c.inner.Get(ctx, id)
|
||||
}
|
||||
|
||||
func (c ctxHonouringSlugs) Put(ctx context.Context, id string, val SlugReservation) error {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
return c.inner.Put(ctx, id, val)
|
||||
}
|
||||
|
||||
func (c ctxHonouringSlugs) PutVersioned(ctx context.Context, id string, expectedVersion int64, val SlugReservation) error {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
return c.inner.PutVersioned(ctx, id, expectedVersion, val)
|
||||
}
|
||||
|
||||
func (c ctxHonouringSlugs) Delete(ctx context.Context, id string) error {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
return c.inner.Delete(ctx, id)
|
||||
}
|
||||
|
||||
func (c ctxHonouringSlugs) List(ctx context.Context, prefix string) ([]string, error) {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return c.inner.List(ctx, prefix)
|
||||
}
|
||||
@@ -299,6 +299,34 @@ per-user pack limit is one (former O2).
|
||||
|
||||
## Design Revisions
|
||||
|
||||
### 2026-08-25 — pack adoption removed
|
||||
|
||||
Verification found the round-4 guard defeated by the module's own recovery
|
||||
branch: an inconclusive `GetStickerSet` keeps the reservation, which turns a
|
||||
*fresh* claim into a *resumed* one, so two ordinary `/newpack` commands took
|
||||
over a stranger's pack. `resolveStaleIntent` had a second adopt path that never
|
||||
consulted the guard at all. Reproduced against the real handlers.
|
||||
|
||||
That is the fourth consecutive failure of the same mechanism, and the reason is
|
||||
structural rather than a bug that can be patched. Adoption needs to prove "this
|
||||
set is mine to finish" from local state, and local state is exactly what a
|
||||
restart on the in-memory backend erases while the packs at Telegram survive.
|
||||
Once the proof is gone the honest and the malicious case are indistinguishable.
|
||||
|
||||
Adoption is therefore removed entirely — both branches. `/newpack` refuses any
|
||||
name a set already occupies, for everyone, whatever the records say.
|
||||
|
||||
Implementing it surfaced a second hole the option did not cover: a *pending*
|
||||
record is not evidence either, and `/delpack` deletes by set name, which
|
||||
Telegram authorises for every set this bot created. Keeping a refused intent so
|
||||
the user could `/delpack` it would hand over a way to destroy the pack we had
|
||||
just refused to adopt. `/delpack` now clears a pending record locally and calls
|
||||
Telegram only for a confirmed one.
|
||||
|
||||
Accepted cost: a crash between creating a set and recording it strands that set
|
||||
permanently. Documented rather than mitigated, because every mitigation is the
|
||||
mechanism that just failed four times.
|
||||
|
||||
### 2026-08-25 — three-lens review pass (security, correctness, tests)
|
||||
|
||||
Three independent reviewers ran against the finished module. What they changed:
|
||||
|
||||
@@ -0,0 +1,227 @@
|
||||
# Adversarial verification — sticker packs, round 4
|
||||
|
||||
Scope: round-4 fixes on `feature/sticker-pack-module` (whole module lives in one
|
||||
commit `d831747`, so round-3→4 cannot be isolated by git; current state reviewed).
|
||||
Method: source read + four constructed attacks executed against the real handlers
|
||||
in a throwaway copy of the repo, plus a 45s fuzz of the emoji splitter and an
|
||||
image-pipeline probe. `go vet`, `go test ./...` (25 pkgs), `golangci-lint run` all
|
||||
clean on the branch as committed. No repo file was modified.
|
||||
|
||||
Verdict: **DO_NOT_MERGE** — change 1 does not close the takeover it was written for.
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL 1 — cross-user pack takeover still reachable; `freshReservation` is a per-invocation fact, not durable proof
|
||||
|
||||
`internal/modules/sticker/pack_handlers.go:352` (guard), `:365-374` (the branch that
|
||||
defeats it), `:289-305` (a second adopt branch that never consults the guard).
|
||||
|
||||
The guard's premise (`:329`, `:340-351`) is "a genuine interrupted attempt always
|
||||
re-enters having found its own reservation, never having made one". True. The
|
||||
converse it relies on — "an attacker naming a foreign set can only ever be the one
|
||||
who made the reservation" — is false, because the module deliberately **keeps** a
|
||||
fresh reservation whenever the lookup is inconclusive:
|
||||
|
||||
```go
|
||||
default:
|
||||
// Unknown. Keep both the intent and the reservation: the set may exist,
|
||||
// and re-running is how the user recovers.
|
||||
log.Error("sticker_newpack_lookup", "err", err)
|
||||
```
|
||||
|
||||
One inconclusive `GetStickerSet` converts the attacker's fresh reservation into a
|
||||
resumed one. The next invocation has `freshReservation == false` and adopts.
|
||||
|
||||
**Precondition (all routes):** the victim's slug reservation is absent while the set
|
||||
lives at Telegram. This is the exact state `docs/sticker-packs.md:122-127` documents
|
||||
("what a restart does when no database is configured") and promises is safe:
|
||||
"`/newpack` reports the name as taken rather than adopting a set it can no longer
|
||||
prove is yours." With `KV_PROVIDER` auto-detect (`cmd/server/main.go:259-279`), any
|
||||
deploy without `MONGO_URL` re-enters this state on **every restart**.
|
||||
|
||||
**Route A — two ordinary commands, no crash, no store error** (executed, reproduced):
|
||||
|
||||
1. Attacker sends `/newpack <victimslug> X`. `getStickerSet` answers 429 / 5xx /
|
||||
deadline-exceeded — not `STICKERSET_INVALID` — so the `default` branch keeps the
|
||||
attacker's intent *and* reservation. User sees "Something went wrong."
|
||||
2. Attacker sends the identical command again. `reserveSlug` conflicts, holder is the
|
||||
attacker → `created=false` → `createOrAdopt(..., freshReservation=false)` → set
|
||||
exists → **adopted**.
|
||||
|
||||
Observed reply: `Finished an earlier attempt at Mine. https://t.me/addstickers/mypack_by_testbot`,
|
||||
record `{Slug:mypack Name:mypack_by_testbot OwnerID:999 Pending:false Count:1}`.
|
||||
The attacker now holds `/delpack` (irreversible `DeleteStickerSet`), `/renamepack`,
|
||||
`/setpackicon`, `/delsticker` over the victim's set — all keyed by set name with no
|
||||
owner scoping.
|
||||
|
||||
Step 1 is attacker-inducible, not luck: Telegram 429s are per-bot and any user can
|
||||
provoke them, and after changes 3+4 the tail budget left for `GetStickerSet` is
|
||||
~3 s (`FetchContext` reserve), so a slow media leg alone produces
|
||||
`context deadline exceeded` → same `default` branch.
|
||||
|
||||
**Route B — `resolveStaleIntent` never checks the guard at all** (executed, reproduced).
|
||||
With intent + reservation for `<victimslug>` surviving (e.g. a crash at the refusal
|
||||
point, before `dropIntent`), the attacker runs `/newpack <anyothername>`;
|
||||
`claimSlug` → `resolveStaleIntent` re-proves only *who holds* the old reservation,
|
||||
finds the victim's set, and adopts at `:292-304`. Reply: "You already have a pack
|
||||
(mypack) from an earlier attempt — it has been restored."
|
||||
|
||||
**Route C — partial refusal cleanup** (executed, reproduced). `:359-361` is two
|
||||
independent, error-swallowing writes. If `releaseSlug` fails or the process dies
|
||||
between them, the attacker keeps the reservation and the next attempt adopts (Route A
|
||||
step 2 without needing step 1).
|
||||
|
||||
**Not exploitable (checked):** `dropIntent` is always keyed to `pack.OwnerID`, which
|
||||
is always the caller (`claimSlug` builds the intent, or `getPack(caller)` returns it);
|
||||
`releaseSlug` re-verifies the holder at `:204`. **User A cannot drop user B's intent
|
||||
or reservation.** That part of round 4 holds.
|
||||
|
||||
**Fix (prototyped and verified).** Replace the per-invocation inference with durable
|
||||
positive evidence: a `Probed bool` on `SlugReservation`, set only when
|
||||
`GetStickerSet` positively answers `STICKERSET_INVALID` for a reservation this owner
|
||||
holds, written *before* `CreateNewStickerSet`, and required by **both** adopt
|
||||
branches. Fails closed: if the flag write fails, abort before creating.
|
||||
|
||||
```go
|
||||
// createOrAdopt, err == nil branch
|
||||
if freshReservation || !s.probedClear(ctx, pack.OwnerID, pack.Slug) { ...refuse... }
|
||||
|
||||
// createOrAdopt, isStickerSetMissing branch, before CreateNewStickerSet
|
||||
if !s.markProbed(ctx, pack.OwnerID, pack.Slug) { return reply(ctx, b, msg, genericFailure) }
|
||||
|
||||
// resolveStaleIntent
|
||||
case err == nil && !held.Probed: // refuse: release old reservation, take the new intent
|
||||
```
|
||||
|
||||
With this applied, all four attack routes refuse and the entire existing suite stays
|
||||
green — one fixture needs updating (`seedInterrupted` in `pack_handlers_test.go:394`
|
||||
must seed `Probed: true`, since it models a post-probe interrupted attempt).
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM 2 — the slug-ownership check still runs *after* the media pipeline
|
||||
|
||||
`pack_handlers.go:94` (`resolveSource`) precedes `:109` (`reserveSlug`).
|
||||
|
||||
Change 3's comment (`:81-84`) says making a user who cannot create a pack pay for the
|
||||
full media pipeline "was free work for anyone who wanted to spend the bot's CPU" —
|
||||
but that is still exactly what happens when the slug belongs to someone else.
|
||||
Executed: `/newpack <slug-held-by-another-user>` replying to a photo issued `getFile`
|
||||
and the file download before any slug check; on a decodable image it also resamples
|
||||
and `UploadStickerFile`s. Methods recorded: `[getFile x.jpg sendMessage]`.
|
||||
|
||||
Impact, single dispatch worker (`WithNotAsyncHandlers`, one worker): each such message
|
||||
occupies the bot for the whole leg, and the attacker never acquires a pack so the new
|
||||
`Pending` precheck never starts refusing them — the loop is unbounded. Measured
|
||||
`toStickerPNG` cost on the single worker: 0.71 s for a flat 4096×4096 source (360 KB
|
||||
on the wire) and 3.7 s for a noisy one (the ladder rungs). `mediaContext` does not
|
||||
bound this — `toStickerPNG` takes no context and cannot be interrupted. Wasted
|
||||
`UploadStickerFile` calls also burn the bot's API quota, which is the 429 that
|
||||
Finding 1 step 1 needs.
|
||||
|
||||
Fix: a read-only `getSlugReservation` before `resolveSource` — refuse when held by
|
||||
another user. Read-only, so it does not reintroduce the round-1 name-burning DoS
|
||||
(the create-only `reserveSlug` write stays where it is).
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM 3 — detached commit contexts are not actually protected at shutdown
|
||||
|
||||
`state.go:53-61`, `cmd/server/main.go:220-227`, `:125`.
|
||||
|
||||
Change 2 extends `context.WithoutCancel` to the reads, on the stated grounds that "a
|
||||
commit that records a completed Telegram-side action must not be lost because the
|
||||
process is shutting down". `main` does not honour that: on SIGTERM it cancels
|
||||
`rootCtx`, calls `srv.Shutdown` on the health server (returns in ms with no live
|
||||
connections), then returns — running `defer closeProvider()`, which disconnects Mongo.
|
||||
Nothing waits for the in-flight inline handler. The detached context survives
|
||||
cancellation but the process does not wait for the write, so a deploy can still lose
|
||||
the commit that these comments promise is safe.
|
||||
|
||||
Fix: track in-flight dispatch with a `sync.WaitGroup` (or a drain deadline) before
|
||||
`closeProvider`, or downgrade the comments to "best effort".
|
||||
|
||||
---
|
||||
|
||||
## LOW 4 — `downloadTimeout` is now dead
|
||||
|
||||
`download.go:25,39` vs `state.go:78-80`. `FetchContext` reserves 3 s of a 10 s
|
||||
handler, so `mediaCtx` is ≤ ~7 s and the 8 s `http.Client.Timeout` can never bind.
|
||||
The comment still calls it "this module's own ceiling". Either lower it to match the
|
||||
real budget or say it is a backstop for a caller with no deadline.
|
||||
|
||||
## LOW 5 — `lockUser`'s stated rationale is not true for this module
|
||||
|
||||
`state.go:82-84` claims "the cron scheduler and the detached per-command stats hook
|
||||
run concurrently with them, so this is load-bearing". Grepped: `keylock` here is
|
||||
`state`-local, the sticker module registers no cron jobs (`sticker.go:21-88`), and the
|
||||
stats hook (`dispatcher.go:82-90`) touches only the stats collection. Nothing else
|
||||
acquires these keys, and all sticker handlers run inline on the single dispatch
|
||||
goroutine. Keep the lock (cheap, correct if dispatch ever goes async) but fix the
|
||||
claim. Corollary: change 3 cannot deadlock or block the worker — verified, see below.
|
||||
|
||||
## LOW 6 — nested `commitContext` re-arms the budget
|
||||
|
||||
`pack_handlers.go:483` passes an already-detached ctx into `dropPackRecord:520`, which
|
||||
derives another. `context.WithoutCancel` drops the parent deadline, so the inner
|
||||
helper gets a fresh 5 s, and `dropPackRecord` → `releaseSlug` adds a third. A
|
||||
`/delpack` confirm can therefore spend ~15 s in detached cleanup. No leak (every
|
||||
`cancel` is deferred) and every op is bounded; just not the 5 s the constant implies.
|
||||
|
||||
## LOW 7 — recording bot truncates instead of rejecting oversized bodies
|
||||
|
||||
`testutil/recording_bot.go:195` reads through `io.LimitReader(r.Body, 8<<20)`; a body
|
||||
above the cap is silently truncated and then fails `ParseMultipartForm` as a confusing
|
||||
"bad multipart form: unexpected EOF" rather than a size error. No current test is near
|
||||
the cap. Reading one extra byte and reporting "body too large" would match the
|
||||
module's own `downloadFile:75` pattern.
|
||||
|
||||
## LOW 8 — duplicated refusal text
|
||||
|
||||
`pack_handlers.go:89-91` and `:248-250` build the same "You already have a pack" reply
|
||||
independently. One helper; they will drift.
|
||||
|
||||
---
|
||||
|
||||
## Attacked and held
|
||||
|
||||
- **Cross-user destruction of state.** `releaseSlug` re-reads and compares the holder
|
||||
(`:204`) and `dropIntent` is always keyed by the caller's own id. Probed both adopt
|
||||
refusal paths: user A cannot drop user B's intent or reservation. Held.
|
||||
- **Deadlock / worker starvation from change 3.** `WithNotAsyncHandlers` +
|
||||
one worker (`internal/telegram/client.go:26-30`) means all sticker handlers run
|
||||
serially on one goroutine; the `keylock.Map` is `state`-local; no cron, no hook, no
|
||||
detached goroutine touches it; no handler nests a second `lockUser`. Held (the lock
|
||||
is uncontended today).
|
||||
- **`Pending` record behaviour after moving the precheck.** The precheck refuses only
|
||||
`found && !existing.Pending`, so a pending user still falls through to the same
|
||||
`claimSlug` resume path as before. No behaviour change. Held.
|
||||
- **Change 5, extreme aspect ratios.** Executed 4096×1, 1×4096, 4096×3, 1×1,
|
||||
4096×4096: outputs 512×1, 1×512, 512×1, 512×512, 512×512 — no zero dimension, long
|
||||
edge exactly 512, and the ladder's targets still derive from the *original* bounds
|
||||
so the aspect is identical to the one-step version. Thumbnail path stays 100×100.
|
||||
Held.
|
||||
- **Change 6, emoji clustering.** 45 s / 526k-exec fuzz over valid UTF-8: no panic, no
|
||||
infinite loop, no empty cluster, and nothing dropped except leading/trailing ZWJ and
|
||||
whitespace. Spot-checked the singleton table against the standard non-block emoji
|
||||
set (©, ®, ‼, ⁉, ™, ℹ, Ⓜ, ⤴, ⤵, 〰, 〽, ㊗, ㊙) — complete. Held.
|
||||
- **Change 7, other modules' tests.** 28 files reference `NewRecordingBot`; full
|
||||
`go test ./...` green. The library always sends `multipart/form-data` with a
|
||||
boundary header and only omits the body for nil-param methods
|
||||
(`raw_request.go:29-72`), so "empty body ⇒ skip parse, non-empty ⇒ must parse" is
|
||||
the correct split, and replacing `r.Body` after buffering works because the boundary
|
||||
comes from the unchanged header. Held.
|
||||
- **`/delpack` callback authorisation.** Lookup keyed by presser, owner compared,
|
||||
chat+message binding before any side effect, action consumed before the destructive
|
||||
call. Held.
|
||||
- **Token leakage.** `classify` inspects error *types* only; `errDownloadFailed`
|
||||
replaces every download error; `replyErr` echoes only `userError`. Held.
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
1. Is production on Mongo or on the auto-detected memory backend? Finding 1 is routine
|
||||
after any restart on memory, and needs data loss on Mongo. It should be fixed either
|
||||
way, but this decides whether it blocks the deploy or only the config.
|
||||
2. `docs/sticker-packs.md:122-127` states the wiped-store refusal as a guarantee. It is
|
||||
currently only true for the first attempt; the doc needs no change once Finding 1 is
|
||||
fixed, but it should not ship as-is.
|
||||
@@ -0,0 +1,259 @@
|
||||
# Round-4 Verification — sticker module (adversarial, mutation-driven)
|
||||
|
||||
Date: 2026-08-25 · Branch `feature/sticker-pack-module` @ `30fa3b3` (identical to
|
||||
`origin/feature/sticker-pack-module`) · Go 1.27.0 linux/arm64 · golangci-lint v2.13.1
|
||||
|
||||
Method: every claim tested by mutating source and re-running the package, not by
|
||||
reading. 34 mutations applied and reverted. All files restored byte-identical
|
||||
(md5 verified, `git status --short` empty).
|
||||
|
||||
## Verdict
|
||||
|
||||
**All six round-4 claims verified.** No claim refuted. But three *previously
|
||||
claimed* guarantees are unpinned and one test name is still misleading (the same
|
||||
class of defect as round 3's `...SurvivesABail`). None of this is a new production
|
||||
defect; it is test-coverage overstatement.
|
||||
|
||||
## 1. Mutation results
|
||||
|
||||
### Round-4 claims under test
|
||||
|
||||
| # | Mutation | Result | Killing test |
|
||||
|---|---|---|---|
|
||||
| M1a | `handleNewPack`: delete `if created { s.releaseSlug(...) }` | **KILLED** | `TestNewPack_FreshReservationReleasedWhenClaimBails` |
|
||||
| M1b | `handleNewPack`: make the release unconditional | **KILLED** | `TestNewPack_ResumedReservationNotReleasedWhenClaimBails` |
|
||||
| M2 | `createOrAdopt`: remove the `if freshReservation` adoption gate | **KILLED** | `TestNewPack_WipedStoreCannotAdoptSurvivingPack` |
|
||||
| M3 | `handleAddSticker`: early `return nil` (handler is a no-op) | **KILLED** | `TestAddSticker_EmojiPrecedence` (+both subtests), `TestAddSticker_FallsBackToDefaultEmoji`, +6 others |
|
||||
| M3b | invert precedence: source emoji beats explicit args | **KILLED** | `TestAddSticker_EmojiPrecedence/explicit_wins` |
|
||||
| M3c | drop source-emoji inheritance | **KILLED** | `TestAddSticker_EmojiPrecedence/inherits_from_replied_sticker` |
|
||||
| M4 | remove `"sticker": sticker.New` + import from `factories()` | **KILLED** | `TestCommandDiscovery_AllPublicCommandsHaveSafeMetadata` — and by the **reverse check specifically**: 9 lines `command_menu_test.go:133: /<cmd> is expected but not registered` |
|
||||
| M5 | `recording_bot.handle`: tolerate any multipart parse failure (`if err == nil`) | **KILLED** | `TestRecordingBot_RejectsMalformedMultipart` |
|
||||
| M6a | `isBinding`: disable tag-block (`E0020..E007F`) binding | **KILLED** | `TestParseEmoji_ClusterEdgeCases/tag_sequence_flag` |
|
||||
| M6b | `splitClusters`: ZWJ absorbs the next rune unconditionally | **KILLED** | `.../joiner_before_a_flag` |
|
||||
| M6c | `trimJoiners` → identity | **KILLED** | `.../joiner_before_a_flag`, `.../trailing_joiner` |
|
||||
| M6d | `isEmojiCluster`: accept a lone regional indicator | **KILLED** | `.../lone_regional_indicator`, `.../odd_regional_indicator_count` |
|
||||
| M6e | drop each of the 9 new `emojiSingletons` entries, one at a time | **9/9 KILLED** | `.../copyright`, `/registered`, `/arrow_curving_up`, `/arrow_curving_down`, `/circled_m`, `/wavy_dash`, `/part_alternation`, `/japanese_congratulations`, `/japanese_secret` |
|
||||
|
||||
Claims 1-6: **verified, all directions.** Claim 1's two-direction pinning is real —
|
||||
M1a and M1b are killed by *different* tests, and by only one test each.
|
||||
|
||||
### Additional adversarial mutations (not claimed, run to find gaps)
|
||||
|
||||
| # | Mutation | Result | Killing test |
|
||||
|---|---|---|---|
|
||||
| M8 | `createOrAdopt` unknown-error branch releases the slug | KILLED | `TestNewPack_UnknownLookupErrorAborts`, `TestNewPack_ResumedReservationSurvivesABail` |
|
||||
| M10 | `releaseSlug`: drop its **own** ownership check | **SURVIVED** | — |
|
||||
| M11b | `resolveStaleIntent`: skip the reservation-owner re-proof | KILLED | `TestNewPack_StaleIntentCannotAdoptForeignName` |
|
||||
| M12b | `reserveSlug`: treat another user's reservation as resumable | KILLED | `TestNewPack_CannotSeizeAnotherUsersPack`, `TestNewPack_ForeignReservationRefusedBeforeAnyAPICall` |
|
||||
| M13 | refused adoption leaves intent + reservation behind | KILLED | `TestNewPack_WipedStoreCannotAdoptSurvivingPack` |
|
||||
| M14 | `releaseSlug`: read reservation on request ctx, not `commitContext` | **SURVIVED** | — |
|
||||
| M16b | delpack callback: drop `query.From.ID != action.OwnerID` | **SURVIVED** | — |
|
||||
| M17 | delpack callback: drop the message-binding check | KILLED | `TestDelPackCallback_RejectsWrongBinding` (+subtests), `TestDelPackCallback_BystanderCannotTouchAnotherUsersPrompt` |
|
||||
| M18 | delpack callback: drop the expiry check | KILLED | `TestDelPackCallback_RejectsExpired` |
|
||||
| M19b | delpack callback: drop the `action.ID != id` nonce check | **SURVIVED** | — |
|
||||
| M6e' | drop each pre-existing singleton `203C`, `2049`, `2122`, `2139` | **4× SURVIVED** | — |
|
||||
| M7 | drop each `emojiRanges` entry, one at a time | 3 KILLED (`1F300`, `2600`, `2B00`) / **5 SURVIVED** (`1F000`, `2190`, `2300`, `25A0`, `1F1E6`) | — |
|
||||
|
||||
**Score: 29 killed / 34 mutation slots, 12 survivors across 5 distinct sites.**
|
||||
|
||||
## 2. Emoji differential probe (full rune space)
|
||||
|
||||
Temp in-package test walked `0x0..0x10FFFF` comparing `isEmojiRune` against a
|
||||
reconstructed round-3 switch (same 8 blocks + the 4 pre-existing singletons).
|
||||
`emoji.go` exists in exactly one commit (`d831747`) on this branch and nowhere in
|
||||
history/reflog/other branches, so the old switch could not be recovered verbatim
|
||||
— it was reconstructed from the range table plus the round-3 correctness report
|
||||
(`correctness-review-260825-1515-sticker-module.md:116-131`, which enumerates the
|
||||
9 refused codepoints and names `2122`/`2139` as already special-cased).
|
||||
|
||||
```
|
||||
deliberate additions observed: 9 of 9 -> [U+00A9 U+00AE U+24C2 U+2934 U+2935 U+3030 U+303D U+3297 U+3299]
|
||||
UNEXPECTED classification changes: 0
|
||||
```
|
||||
|
||||
**Result: zero unexpected classification changes.** The switch→array+map
|
||||
restructure is behaviour-preserving modulo exactly the 9 intended additions.
|
||||
|
||||
Second, non-circular probe against authoritative Unicode data
|
||||
(`unicode.org/Public/UCD/latest/ucd/emoji/emoji-data.txt`, 1288 lines):
|
||||
|
||||
```
|
||||
accepted-but-not Emoji/Extended_Pictographic: 2140 (e.g. U+2190..U+21FF arrows, U+25A0.. shapes)
|
||||
Emoji-property runes rejected: 12 -> [U+0023 U+002A U+0030..U+0039]
|
||||
```
|
||||
|
||||
The 12 rejections are the keycap bases, handled by the `keycapCombining` branch in
|
||||
`isEmojiCluster` — not a defect. The 2140 false positives are the documented
|
||||
block-approximation trade-off (`emoji.go` comment: "Deliberately ranges rather
|
||||
than a property lookup"), pre-existing and low impact: Telegram answers
|
||||
`STICKER_EMOJI_INVALID` and `apiRefusal` renders a sane message. Not a round-4
|
||||
regression. Informational only.
|
||||
|
||||
## 3. New / still-wrong tests found
|
||||
|
||||
### F-1 (MEDIUM) — `TestNewPack_ResumedReservationSurvivesABail` still does not test what its name and comment claim
|
||||
|
||||
`pack_handlers_test.go:573`. Its comment says *"A reservation the caller merely
|
||||
resumed must not be released when a later step bails — it predates this command."*
|
||||
That is the `created`-flag distinction. Proven false by mutation:
|
||||
|
||||
- under **M1b** (release made unconditional — i.e. the resumed/fresh distinction
|
||||
deleted outright) this test still passes: `ok ... 0.019s`. Only
|
||||
`TestNewPack_ResumedReservationNotReleasedWhenClaimBails` catches M1b.
|
||||
- what it *actually* pins is M8: `createOrAdopt`'s unknown-error branch must not
|
||||
release. So does `TestNewPack_UnknownLookupErrorAborts`, which M8 also killed.
|
||||
|
||||
It is a duplicate of `TestNewPack_UnknownLookupErrorAborts` wearing the name of the
|
||||
round-4 test that replaced it. Round 4 correctly added the two real tests but left
|
||||
the misnamed one in place. Rename to `TestNewPack_UnknownLookupErrorKeepsTheReservation`
|
||||
or delete it.
|
||||
|
||||
### F-2 (MEDIUM) — `TestDelPackCallback_RejectsOtherUser` does not exercise the owner check
|
||||
|
||||
`delpack_callback_test.go`. **M16b survived**: deleting
|
||||
`if query.From.ID != action.OwnerID` from `delpack_callback.go:118` breaks no test.
|
||||
Reason: the record is fetched with `key := pendingDeleteKey(query.From.ID)` (`:108`),
|
||||
so a foreign presser has no record at all and exits three lines earlier via
|
||||
`ErrNotFound` → *"This confirmation expired or was already used."* The owner check
|
||||
is structurally unreachable; the test named for it passes through a different
|
||||
branch. Authorization is genuinely enforced (by the key), so this is a test-naming
|
||||
and dead-code issue, not a hole. Fix: assert the *reply text*, or drop the
|
||||
unreachable branch.
|
||||
|
||||
### F-3 (LOW) — `releaseSlug`'s own ownership check is unpinned
|
||||
|
||||
**M10 survived.** The code comment (`pack_handlers.go:176-181`) states the check
|
||||
exists precisely because *"this module has already been bitten once by an ownership
|
||||
check that lived in the caller instead of the operation."* Nothing tests it. Every
|
||||
current call site happens to pass the correct owner, so the check is presently
|
||||
redundant — which is exactly why a future call site could silently regress it.
|
||||
|
||||
### F-4 (LOW) — round-3's `commitContext` fix in `releaseSlug` is unpinned
|
||||
|
||||
**M14 survived.** Moving the ownership read back onto the request context breaks
|
||||
nothing: there is no cancelled-context test in the package (`grep context.WithCancel
|
||||
internal/modules/sticker/*_test.go` → 0 hits). The comment at `:183-190` describes a
|
||||
concrete failure mode (SIGTERM mid-release leaves an unreachable reservation) with
|
||||
no test behind it.
|
||||
|
||||
### F-5 (LOW) — delpack nonce check unpinned
|
||||
|
||||
**M19b survived.** Acknowledged in-code as defence-in-depth subsumed by the
|
||||
message binding (`:141-143`), so lower priority than F-2, but it is untested
|
||||
dead weight.
|
||||
|
||||
### F-6 (LOW) — dead `emojiRanges` entry with a load-bearing-sounding comment
|
||||
|
||||
`{0x1F1E6, 0x1F1FF}` is fully contained in `{0x1F000, 0x1F2FF}` — **M7 confirms both
|
||||
survive individual deletion**. Its comment ("regional indicators; isEmojiCluster
|
||||
requires a pair") reads as though it is required. It is not. Same for the untested
|
||||
`2190`, `2300`, `25A0`, `1F000` entries and the four pre-existing singletons
|
||||
(`203C`, `2049`, `2122`, `2139`) — all silently deletable.
|
||||
|
||||
### No vacuous-loop assertions remain
|
||||
|
||||
Swept every `for _, call := range rb.Sent()` in the changed packages. The four hits
|
||||
are either negative assertions (correct as loops), counting helpers, or already
|
||||
gated by a preceding `countMethod(...) != 1` fatal. Round 4's `addedStickerPayload`
|
||||
helper is genuine — M3 proves it. Also scanned all 89 sticker tests + the new
|
||||
dispatcher/testutil/cmd tests for zero-assertion bodies: 4 heuristic hits, all
|
||||
false positives (the assertion is a `Fatalf` on `err == nil`).
|
||||
|
||||
## 4. Regression sweep — `internal/testutil/recording_bot.go`
|
||||
|
||||
26 test files across 18 packages construct a `RecordingBot`. All pass.
|
||||
|
||||
The behaviour change (non-empty body that fails multipart parse → 400 instead of
|
||||
200-with-empty-form) is scoped correctly:
|
||||
|
||||
- `go-telegram/bot@v1.20.0 raw_request.go:28-71` always sends multipart; when
|
||||
`params == nil` (parameterless methods) it skips both `buildRequestForm` and
|
||||
`form.Close()`, so the body is **zero bytes** — the `len(body) > 0` gate is the
|
||||
right discriminator. `TestRecordingBot_ServesParameterlessCall` confirms getMe.
|
||||
- The exact hazard the comment warns about exists in the repo:
|
||||
`internal/modules/stock/dividend_flow_test.go:115` and `:139` assert
|
||||
`Form["reply_markup"] != ""` is false. Both still pass — the change *protects*
|
||||
them rather than breaking them.
|
||||
- No caller asserts on a field only present under a file part.
|
||||
|
||||
`go test -race -count=1 ./...` — clean, no races, exit 0.
|
||||
`go test -count=20 ./internal/modules/sticker/` — `ok ... 7.964s`, no flakes.
|
||||
|
||||
Nit (non-blocking): `recording_bot.go handle()` carries two overlapping comment
|
||||
paragraphs saying the same thing — a stale round-3 paragraph left above the
|
||||
round-4 one.
|
||||
|
||||
## 5. Gates
|
||||
|
||||
```
|
||||
golangci-lint run ./... → 0 issues.
|
||||
gofmt -l . (minus third_party) → (empty)
|
||||
go test -race -count=1 ./... → clean
|
||||
```
|
||||
|
||||
## 6. Final state (verbatim)
|
||||
|
||||
```
|
||||
$ git status --short
|
||||
$
|
||||
```
|
||||
(empty)
|
||||
|
||||
md5 of every tracked file diffed against the pre-review baseline: **MD5 IDENTICAL**.
|
||||
|
||||
```
|
||||
$ go test ./...
|
||||
ok github.com/tiennm99/miti99bot/cmd/server (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/cron (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/deploynotify (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/keylock (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/log (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/metrics (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/amlich (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/coin (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/gold (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/lol (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/loldle (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/misc (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/monkeyd (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/stats (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/sticker (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/stock (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/util (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/util/chathelper (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/modules/wordle (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/server (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/storage (cached)
|
||||
? github.com/tiennm99/miti99bot/internal/systemstate [no test files]
|
||||
ok github.com/tiennm99/miti99bot/internal/telegram (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/testutil (cached)
|
||||
ok github.com/tiennm99/miti99bot/internal/testutil/mongotest (cached)
|
||||
```
|
||||
|
||||
## 7. Recommended actions
|
||||
|
||||
1. **F-1** — rename or delete `TestNewPack_ResumedReservationSurvivesABail`. Third
|
||||
round running that a test in this file claims more than it proves; the name is
|
||||
what misled round 3.
|
||||
2. **F-2** — assert the reply text in `TestDelPackCallback_RejectsOtherUser`, or
|
||||
remove the unreachable `query.From.ID != action.OwnerID` branch.
|
||||
3. **F-3 / F-4** — add two small tests: a cross-owner `releaseSlug` call, and a
|
||||
`releaseSlug` under a cancelled parent context. Both are ~10 lines and pin
|
||||
fixes whose comments describe real past incidents.
|
||||
4. **F-6** — delete `{0x1F1E6, 0x1F1FF}` from `emojiRanges` or fix its comment.
|
||||
5. Drop the duplicated comment paragraph in `recording_bot.go handle()`.
|
||||
|
||||
None of 1-5 blocks merge. All are test/comment hygiene against real production
|
||||
behaviour that is correct today.
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
- The pre-round-4 `isEmojiRune` switch is unrecoverable from git (single squashed
|
||||
commit). The differential probe is therefore partly circular *for the singleton
|
||||
set* — it is fully independent for the eight ranges. If the round-3 source is
|
||||
available elsewhere, re-running the probe against the verbatim original would
|
||||
close the last gap.
|
||||
- Are the four pre-existing singletons (`203C ‼`, `2049 ⁉`, `2122 ™`, `2139 ℹ`)
|
||||
intentionally unlisted in `TestParseEmoji_ClusterEdgeCases`, or an oversight when
|
||||
round 4 added the nine?
|
||||
Reference in new issue
Block a user