diff --git a/CLAUDE.md b/CLAUDE.md index db431f6..d777737 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -269,6 +269,15 @@ inconsistency here without checking the client first. every non-avatar drop with `AvatarItemType: null` for this reason; `boxAvatarItemType` applies it on every surface, so don't coalesce it with `?? 0` anywhere a box is built. This holds for every worker that mints a box, not only `econ`. +- A ban's `Duration` (`api`: `/api/PlayerReporting/v1/moderationBlockDetails`, and the + `ModerationKick` frame `www`'s staff ban sends) is the seconds LEFT as of the response — the + client counts it down from receipt. It is NOT the ban's full span paired with + `TimeoutStartedAt` (start + duration): served that way, the screen read correctly only in + the kick frame, sent at the instant the span and the remainder coincide, and every later + sign-in showed the whole ban still to run. `TimeoutStartedAt` stays null for a ban; where + other implementations set it at all, it names the start of a voice-chat timeout. Other + implementations with timed bans keep their set-time private and serve the remainder on + every read; `banned_at` here is the staff panel's audit trail, not the client's. - Accessibility is sent as the `RoomAccessibility` enum NAME on `rooms` `PUT /rooms/:id/subrooms/:sid/accessibility` (`accessibility=Private`), not the ordinal the room-level `/rooms/:id/accessibility` takes. The enum has five members diff --git a/apps/api/src/openapi.ts b/apps/api/src/openapi.ts index 884e8dd..ca70428 100644 --- a/apps/api/src/openapi.ts +++ b/apps/api/src/openapi.ts @@ -1370,8 +1370,9 @@ export const VoteToKickReason = z.object({ * `GET|POST /api/PlayerReporting/v1/moderationBlockDetails` — the caller's block. With an * account-wide ban in force (a `report` row with `banned` set) it describes that ban: * `IsBan` true, the report's `ReportCategory`, a fixed `Message` of "Rule violation", and - * its span as `TimeoutStartedAt` (the report's `created_at`) plus `Duration` (seconds to - * `ban_expires`; int32 max for a permanent ban). Otherwise it is the "not blocked" answer, mirroring the reference server's stub `ReturnModerationBlockDetails()`: + * `Duration` as the seconds LEFT as of the request (`ban_expires` minus now; int32 max for + * a permanent ban) — the client counts it down from receipt, so it is never the full span. + * `TimeoutStartedAt` stays null for a ban. Otherwise it is the "not blocked" answer, mirroring the reference server's stub `ReturnModerationBlockDetails()`: * `ReportCategory` is `Unknown` (-1) rather than 0, which is a real category, and * `Message` is null — the client distinguishes "no message" from a blank one, so we send * null where the reference sends an empty string. `IsVoiceModAutoban`/`TimeoutStartedAt` @@ -1391,7 +1392,7 @@ export const ModerationBlockDetails = z.object({ Duration: z .int() .describe( - 'Length of the block in seconds from `TimeoutStartedAt`; 2147483647 (int32 max) for a permanent ban; 0 when not blocked' + 'Seconds LEFT on the block as of this request (the client counts it down from receipt); 2147483647 (int32 max) for a permanent ban; 0 when not blocked' ), GameSessionId: z.int(), IsHostKick: z.boolean().describe('Always false — no host kick is ever recorded here'), @@ -1411,7 +1412,7 @@ export const ModerationBlockDetails = z.object({ .string() .nullable() .describe( - 'When the block began — the ban’s report `created_at` (ISO-8601 UTC); `Duration` runs from it. Null when not blocked' + 'Always null — the client does not pair it with `Duration` for a ban (it names the start of a voice-chat timeout, which this server never hands out)' ), AssociatedAccountUsername: z.string().nullable().describe('Always null'), ShowCreatorCodeOfConduct: z.boolean().describe('Always false'), diff --git a/apps/api/src/reports-db.ts b/apps/api/src/reports-db.ts index 948774f..f9e151a 100644 --- a/apps/api/src/reports-db.ts +++ b/apps/api/src/reports-db.ts @@ -125,9 +125,10 @@ export interface ReportRow { banned_by_player_id: number | null /** * ISO-8601 UTC instant the ban was HANDED DOWN — distinct from `created_at`, which is - * when the report was filed, and the two can be months apart. This is what the client's - * block screen counts the ban from (see `banBlockDetails`); rows written before - * 0020_report_ban_audit.sql carry NULL and fall back to `created_at`. + * when the report was filed, and the two can be months apart. The staff panel's audit + * trail and its ordering (`getBansInForce`); the client's block screen does NOT count + * from it (see `banBlockDetails`). Rows written before 0020_report_ban_audit.sql carry + * NULL and fall back to `created_at` where a date is needed. */ banned_at: string | null /** @@ -271,8 +272,8 @@ export async function isPlayerBanned( * report itself intact. * * `bannedBy` is the acting moderator, and is recorded alongside `banned_at` — the instant - * the ban was handed down, which is NOT the report's `created_at` and is what the client's - * block screen counts from. A lift clears all four columns together: an unbanned row must + * the ban was handed down, which is NOT the report's `created_at`; the staff panel shows + * and sorts by it. A lift clears all four columns together: an unbanned row must * not keep an audit trail saying a ban runs from somewhere, and `banned = 0` with a * `banned_at` still set would read as a ban to anything checking the timestamp. * @@ -507,9 +508,9 @@ export async function getBansInForce( // ---- Block details ---------------------------------------------------------- /** - * `Duration` on a permanent ban. The client's field is a 32-bit int of seconds that PAIRS - * with `TimeoutStartedAt` — start + duration is the end of the block — so a ban with no - * end gets the largest value the field holds, 68 years past its start. + * `Duration` on a permanent ban. The client's field is a 32-bit int of seconds LEFT, so a + * ban with no end gets the largest value the field holds — 68 years, which other + * implementations also send and the client's screen renders as permanent. */ export const PERMANENT_BAN_DURATION = 2_147_483_647 @@ -547,18 +548,24 @@ export const NOT_BLOCKED = { * (the screen at sign-in) and `www`'s live `ModerationKick` frame (the screen when the ban is * handed down mid-session). Built once here so both screens describe the same ban. * - * `Duration` and `TimeoutStartedAt` are a PAIR in the client: the block runs from the - * start for the duration. The start is `banned_at`, the instant the ban was handed down - * (see 0020_report_ban_audit.sql), and the duration is the seconds from there to - * `ban_expires`, so the two sum to the expiry; or `PERMANENT_BAN_DURATION` when there is - * none. + * `Duration` is the seconds LEFT on the ban as of `now` — the time from the request to + * `ban_expires`, rounded up so a ban with a second to run never reads 0 — or + * `PERMANENT_BAN_DURATION` when there is no expiry. The client counts it down from the + * moment it receives it, with no reference to when the ban began: other implementations + * with timed bans keep the instant they set the block PRIVATE and serve `Duration` minus + * the elapsed time on every read. + * This once sent the FULL span from `banned_at` instead, on the theory that the client + * pairs it with `TimeoutStartedAt` (start + duration = end). It does not: the screen read + * correctly only at the instant the ban was handed down — when the full span IS the + * remainder, so the mid-session `ModerationKick` looked right — and every later sign-in + * showed the whole ban still to run. * - * It falls back to the report's `created_at` for a row banned before that column existed. - * The two can be months apart, and using `created_at` as the start — which is what this - * did before there was anything else to use — misreports both halves of the pair: a 7-day - * ban applied to a 30-day-old report told the player their block began a month ago and - * ended three weeks ago. Every pre-migration row still reads exactly as it used to, which - * is the point of the fallback rather than a coalesce to now. + * `TimeoutStartedAt` stays null. Other implementations don't send it for a ban; where it is + * set at all it names the start of a voice-chat timeout, which is what the name is about. + * `banned_at` is the audit trail and the staff panel's, not the client's. + * + * `now` is a parameter so the caller can pass the same instant it resolved the ban with + * (`getActiveBan`), and a ban that was in force at the read can't come out as 0 left. * * The category is the one the report was * filed under, so the client's ban screen names the reason. `Message` is a fixed "Rule @@ -568,19 +575,17 @@ export const NOT_BLOCKED = { * reported them. Everything else keeps its `NOT_BLOCKED` value: the other block kinds and * screen dressings, none of which this server hands out. */ -export function banBlockDetails(ban: ReportRow) { - const startedAtIso = ban.banned_at ?? ban.created_at - const startedAt = Date.parse(startedAtIso) +export function banBlockDetails(ban: ReportRow, now: Date = new Date()) { const duration = ban.ban_expires === null ? PERMANENT_BAN_DURATION - : Math.max(1, Math.ceil((Date.parse(ban.ban_expires) - startedAt) / 1000)) + : Math.max(1, Math.ceil((Date.parse(ban.ban_expires) - now.getTime()) / 1000)) return { ...NOT_BLOCKED, ReportCategory: ban.report_category, Duration: duration, IsBan: true, Message: 'Rule violation', - TimeoutStartedAt: startedAtIso, + TimeoutStartedAt: null, } } diff --git a/apps/api/src/routes/moderation.ts b/apps/api/src/routes/moderation.ts index a21d2c3..631eb10 100644 --- a/apps/api/src/routes/moderation.ts +++ b/apps/api/src/routes/moderation.ts @@ -368,12 +368,11 @@ export const moderationRoutes = new Hono({ strict: false }) 'matchmake refuses on (login still issues a token, so the client can reach this ' + 'screen) — so a caller with one in force gets ' + '`IsBan: true`, the `ReportCategory` the report was filed under, the fixed ' + - '`Message` “Rule violation”, and the block’s span as the pair the client reads ' + - 'them as: `TimeoutStartedAt` is when the ban was handed down (`banned_at`, ' + - 'falling back to the report’s `created_at` for a ban set before that column ' + - 'existed) and `Duration` the ' + - 'seconds from there to `ban_expires` (2147483647, the int32 max, for a permanent ' + - 'ban). ' + + '`Message` “Rule violation”, and `Duration` as the seconds LEFT on the ban as of ' + + 'this request — the client counts it down from when it receives it, so it is ' + + '`ban_expires` minus now, rounded up, never the ban’s full span (2147483647, the ' + + 'int32 max, for a permanent ban). `TimeoutStartedAt` stays null: the client does ' + + 'not pair it with `Duration` for a ban, and no reference server sends it. ' + '`PlayerIdReporter` stays null: it names a kicking host, and the reporter is not ' + 'shown to the player they reported. Only the caller’s own account is consulted, ' + 'not the ban-evasion arms.\n\n' + @@ -396,8 +395,11 @@ export const moderationRoutes = new Hono({ strict: false }) async (c) => { const id = await authedId(c) if (id === null) return unauthorized(c) - const ban = await getActiveBan(c.env.DB, id) - return c.json(ban ? banBlockDetails(ban) : NOT_BLOCKED) + // One instant for both: the ban resolved as in force at `now` is described as of + // `now`, so it can't come out with 0 left. + const now = new Date() + const ban = await getActiveBan(c.env.DB, id, now) + return c.json(ban ? banBlockDetails(ban, now) : NOT_BLOCKED) } ) // The reasons the client offers when a player starts a vote-to-kick. Order matters — diff --git a/apps/api/src/test/integration/api.test.ts b/apps/api/src/test/integration/api.test.ts index 97581ff..477e56c 100644 --- a/apps/api/src/test/integration/api.test.ts +++ b/apps/api/src/test/integration/api.test.ts @@ -6245,17 +6245,16 @@ describe('player reports', () => { expect(res.status).toBe(401) }) - // Duration and TimeoutStartedAt are a pair in the client — the block runs from the - // start for the duration. The start is `banned_at`, the instant the ban was handed - // down (see 0020_report_ban_audit.sql), NOT the report's created_at: the two can be - // months apart. A permanent ban runs for the largest span the int holds. + // `Duration` is the seconds LEFT — the client counts it down from receipt. A permanent + // ban runs for the largest span the int holds, and `TimeoutStartedAt` stays null: the + // client does not pair it with `Duration` for a ban. test('describes a permanent ban', async () => { await submit( { PlayerIdReported: '221', ReportCategory: '102', Details: 'slurs' }, await bearer() ) const [row] = await getReportsAgainst(env.DB, 221) - const banned = await banFromReport(env.DB, row!.id) + await banFromReport(env.DB, row!.id) expect(await details('POST', '221')).toEqual({ ...NOT_BLOCKED, @@ -6266,21 +6265,15 @@ describe('player reports', () => { // the reporter is not shown to the player they reported (PlayerIdReporter // stays null). Message: 'Rule violation', - TimeoutStartedAt: banned!.banned_at, }) }) - // A timed ban's Duration is the seconds from the start to the expiry, so the pair - // sums to `ban_expires` — not the seconds left as of the request. - test('describes a timed ban as its banned_at plus the span to expiry', async () => { + // A fresh timed ban: the seconds left are (within the write's latency) the whole ban. + test('describes a timed ban as the seconds left as of the request', async () => { await submit({ PlayerIdReported: '222', ReportCategory: '103' }, await bearer()) const [row] = await getReportsAgainst(env.DB, 222) - // The expiry is set relative to NOW, which is what a moderator handing down a - // one-hour ban means — and now is `banned_at`, not the report's created_at. const banExpires = new Date(Date.now() + 3600 * 1000) - const banned = await banFromReport(env.DB, row!.id, { - banExpires: banExpires.toISOString(), - }) + await banFromReport(env.DB, row!.id, { banExpires: banExpires.toISOString() }) // `Duration` is asserted as a range below, so it is left out of the object match // — NOT_BLOCKED carries a 0 for it, which would win over the real value. @@ -6291,59 +6284,59 @@ describe('player reports', () => { ReportCategory: 103, IsBan: true, Message: 'Rule violation', - TimeoutStartedAt: banned!.banned_at, + TimeoutStartedAt: null, }) - // Within a second of the hour: `banned_at` is stamped by the write, so the span - // is the hour minus however long the write took. + // Within a second of the hour: however long the write took has already elapsed. expect(body.Duration).toBeGreaterThan(3595) expect(body.Duration).toBeLessThanOrEqual(3600) }) - // The whole point of `banned_at`: a ban applied to an OLD report used to be - // described as having started when the report was filed, so a 7-day ban on a - // month-old report told the player their block began a month ago — and the duration, - // measured from there, came out as already served. Both halves of the pair now - // start from the ban. - test('counts a ban from when it was handed down, not from when the report was filed', async () => { + // The case the bug lived in: a player signing in AFTER the ban was handed down. The + // client counts `Duration` down from the moment it receives it, so it must be what is + // left NOW — not the ban's full span from `banned_at`, which read as the whole ban + // still to run at every sign-in (and only looked right in the mid-session kick frame, + // sent at the instant the span and the remainder are the same). + test('counts down: a ban handed down a day ago has a day less to run', async () => { await submit({ PlayerIdReported: '224', ReportCategory: '102' }, await bearer()) const [row] = await getReportsAgainst(env.DB, 224) - // Backdate the report a month, as if it had sat in the queue. - const filed = new Date(Date.now() - 30 * 86_400_000).toISOString() - await env.DB.prepare('UPDATE report SET created_at = ?2 WHERE id = ?1') - .bind(row!.id, filed) + // A seven-day ban handed down yesterday: six days left. + const banned = await banFromReport(env.DB, row!.id, { + banExpires: new Date(Date.now() + 6 * 86_400_000).toISOString(), + }) + await env.DB.prepare('UPDATE report SET banned_at = ?2 WHERE id = ?1') + .bind(row!.id, new Date(Date.now() - 86_400_000).toISOString()) .run() - const banned = await banFromReport(env.DB, row!.id, { - banExpires: new Date(Date.now() + 7 * 86_400_000).toISOString(), - }) const body = (await details('GET', '224')) as Record - expect(body.TimeoutStartedAt).toBe(banned!.banned_at) - expect(body.TimeoutStartedAt).not.toBe(filed) - // Seven days, not the negative-then-clamped span the report's date would give. - expect(body.Duration).toBeGreaterThan(7 * 86_400 - 5) - expect(body.Duration).toBeLessThanOrEqual(7 * 86_400) + expect(body.IsBan).toBe(true) + expect(body.Duration).toBeGreaterThan(6 * 86_400 - 5) + expect(body.Duration).toBeLessThanOrEqual(6 * 86_400) + // The start is not on the wire at all: null, not `banned_at`. + expect(body.TimeoutStartedAt).toBeNull() + expect(banned!.banned_at).not.toBeNull() }) - // A row banned BEFORE 0020 added the column carries no `banned_at`, and must keep - // reading exactly as it used to rather than falling back to "now" — which would - // silently restart every standing ban the moment this shipped. - test('falls back to created_at for a ban with no recorded banned_at', async () => { + // A row banned BEFORE 0020 added `banned_at` reads the same as any other: the + // remainder needs only `ban_expires`. + test('reads a ban with no recorded banned_at the same way', async () => { await submit({ PlayerIdReported: '225', ReportCategory: '101' }, await bearer()) const [row] = await getReportsAgainst(env.DB, 225) await env.DB.prepare( `UPDATE report SET banned = 1, ban_expires = ?2, banned_at = NULL WHERE id = ?1` ) - .bind(row!.id, new Date(Date.parse(row!.created_at) + 3600 * 1000).toISOString()) + .bind(row!.id, new Date(Date.now() + 3600 * 1000).toISOString()) .run() - expect(await details('GET', '225')).toEqual({ - ...NOT_BLOCKED, + const { Duration: _duration, ...unblocked } = NOT_BLOCKED + const body = (await details('GET', '225')) as Record + expect(body).toMatchObject({ + ...unblocked, ReportCategory: 101, - Duration: 3600, IsBan: true, Message: 'Rule violation', - TimeoutStartedAt: row!.created_at, }) + expect(body.Duration).toBeGreaterThan(3595) + expect(body.Duration).toBeLessThanOrEqual(3600) }) // A ban that has served its time is not a block, even though the row still says diff --git a/apps/www/src/staff.ts b/apps/www/src/staff.ts index 6d8e8f0..e9850c0 100644 --- a/apps/www/src/staff.ts +++ b/apps/www/src/staff.ts @@ -216,10 +216,11 @@ function banExpiry( * ROOM ban must never send it (`rooms` sends `IsBan: false` and is enforced by refusing the * room), because the client cannot tell the two apart and shows a game-wide ban screen. * - * Its contents are {@link banBlockDetails} — the same `ReportCategory`, `Message` and the - * `Duration`/`TimeoutStartedAt` pair `moderationBlockDetails` answers for this ban — so the - * screen a player sees mid-session is the screen they see when they next sign in, not a - * generic "banned" with no length. + * Its contents are {@link banBlockDetails} — the same `ReportCategory`, `Message` and + * `Duration` `moderationBlockDetails` answers for this ban — so the screen a player sees + * mid-session is the screen they see when they next sign in, not a generic "banned" with no + * length. `Duration` is the seconds LEFT, and this frame goes out the instant the ban is + * handed down, so here that is the ban's whole span; every later sign-in gets less. * * Sent to anyone ONLINE, not only to a player standing in an instance: someone in a menu is * as banned as someone in a room. `GameSessionId` is their instance, or 0 when they are in diff --git a/apps/www/src/test/integration/api.test.ts b/apps/www/src/test/integration/api.test.ts index f8121b0..c3fb185 100644 --- a/apps/www/src/test/integration/api.test.ts +++ b/apps/www/src/test/integration/api.test.ts @@ -1280,14 +1280,16 @@ it('throws a banned player out of the instance they are standing in', async () = expect(frames[0].data.GameSessionId).toBe(77001) // The SAME ban the sign-in screen describes, not a generic one: its category, message and - // the Duration/TimeoutStartedAt pair, three days from when it was handed down. + // `Duration` — the seconds LEFT, which at the instant the ban is handed down is the whole + // three days. `TimeoutStartedAt` is not paired with it by the client and stays null. const banned = await getReportById(env.DB, report.id) expect(frames[0].data).toMatchObject({ ReportCategory: banned!.report_category, Message: 'Rule violation', - TimeoutStartedAt: banned!.banned_at, - Duration: 3 * 86_400, + TimeoutStartedAt: null, }) + expect(frames[0].data.Duration).toBeGreaterThan(3 * 86_400 - 5) + expect(frames[0].data.Duration).toBeLessThanOrEqual(3 * 86_400) // And they are out of that instance — moved into their own DORM, not deleted: `match` // lets a banned player matchmake there and nowhere else, so it is where they read the