diff --git a/.prettierignore b/.prettierignore index 9df5ea64de..44b8cbdcff 100644 --- a/.prettierignore +++ b/.prettierignore @@ -26,3 +26,11 @@ packages/civitai-db-schema/src/schema-drift/__tests__/fixtures/catalog-productio packages/civitai-db-schema/src/enums.ts packages/civitai-db-schema/src/models.ts packages/civitai-db-schema/src/kysely/ + +# πŸ”΄ THE FORMATTING IS THE FIXTURE. These modules exist so +# `src/server/services/__tests__/moderator-restriction-vocabulary.test.ts` can prove its reader sees a +# divergence in the moderator app's restriction vocabulary regardless of how that vocabulary is +# written β€” double-quoted entries, a trailing comma, a multi-line array, a comment containing a `]`. +# Prettier normalises exactly those differences away, which would silently collapse four distinct +# cases into one and leave the suite green while testing a third of what it claims to. +src/server/services/__tests__/__fixtures__/moderator-vocabulary/ diff --git a/apps/moderator/src/lib/__tests__/restriction-types.test.ts b/apps/moderator/src/lib/__tests__/restriction-types.test.ts new file mode 100644 index 0000000000..a083c767c1 --- /dev/null +++ b/apps/moderator/src/lib/__tests__/restriction-types.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest'; +import { + RESTRICTION_TYPE, + RESTRICTION_TYPES, + RESTRICTION_TYPE_LABELS, + RULINGS_WIRED_FOR, + unwiredRulingReason, +} from '$lib/restriction-types'; + +/** + * The one predicate three surfaces read: the audit queue's `resolve`/`ban` refusal, the audit queue's + * disabled ruling buttons, and the retool User Lookup panel's disabled ruling form. It was open-coded + * in the route before #4609's audit; a predicate spelled at N sites is wrong at Nβˆ’1 of them, and here + * the sites disagreeing means a live button whose only possible outcome is a rejected call. + * + * The refusal that MATTERS is enforced by the main app, inside `resolveUserRestriction` β€” this list is + * the same rule read forward so a form that cannot succeed is never offered. The two are pinned to + * each other by `src/server/services/__tests__/restriction-type-seam.test.ts`. + */ +describe('unwiredRulingReason', () => { + it('permits a ruling on every wired-for type', () => { + // Non-vacuous: there is at least one, and it is the queue's default. + expect(RULINGS_WIRED_FOR.length).toBeGreaterThan(0); + expect(RULINGS_WIRED_FOR).toContain(RESTRICTION_TYPE); + for (const type of RULINGS_WIRED_FOR) expect(unwiredRulingReason(type)).toBeNull(); + }); + + /** + * πŸ”΄ PINNED BY VALUE, not by "at least one type is refused". + * + * The test below used to lean on `unwired.length > 0` to stay non-vacuous, and that made it catch + * a widened `RULINGS_WIRED_FOR` only BY ACCIDENT β€” `bot-account` being the sole unwired type is + * the only reason wiring it in emptied the list. Add a third filed type and the accident is gone: + * `RULINGS_WIRED_FOR` could claim a verdict path for `bot-account` while `unwired` still holds the + * third type, so the length check passes and the loop passes and nothing here notices. + * + * A value pin does not decay that way. It is the mirror of the main app's own + * (`expect([...RULINGS_WIRED_FOR]).toEqual(['generation'])` in + * `src/server/__tests__/pending-review-mute.test.ts`), and widening this list on either side is + * supposed to be a deliberate act with the verdict path parameterised first. + */ + it('claims a verdict path for generation and for nothing else', () => { + expect([...RULINGS_WIRED_FOR]).toEqual(['generation']); + }); + + it('refuses every filed type that has no verdict path, naming it', () => { + const unwired = RESTRICTION_TYPES.filter((t) => !RULINGS_WIRED_FOR.includes(t)); + // Kept only as a vacuity guard. It is NOT what catches a widened `RULINGS_WIRED_FOR` any more β€” + // the value pin above is, and it does not decay as the vocabulary grows. + expect(unwired.length).toBeGreaterThan(0); + + for (const type of unwired) { + const reason = unwiredRulingReason(type); + // The type is named because a moderator has to be able to tell WHICH queue is review-only, and + // the message doubles as the audit route's `fail(400)` body. + expect(reason).toContain(`"${type}"`); + expect(reason).toContain('NOT resolved'); + } + }); + + it('refuses a type nobody has heard of, rather than defaulting it in', () => { + // The value reaching this can come off a database row, so it is not confined to the union. + for (const type of ['', 'GENERATION', 'generation ', 'nonsense']) + expect(unwiredRulingReason(type)).not.toBeNull(); + }); + + it('keeps a label for every filed type, so a refused queue can still be named on screen', () => { + expect(Object.keys(RESTRICTION_TYPE_LABELS).sort()).toEqual([...RESTRICTION_TYPES].sort()); + }); +}); diff --git a/apps/moderator/src/lib/restriction-types.ts b/apps/moderator/src/lib/restriction-types.ts new file mode 100644 index 0000000000..0b070f8f17 --- /dev/null +++ b/apps/moderator/src/lib/restriction-types.ts @@ -0,0 +1,69 @@ +/** + * The kinds of review that file into the moderator mute queue. + * + * `UserRestriction.type` is free text in the database with a `[type, status]` index, so a new kind of + * review costs no migration β€” it files into the same queue under a different type. The list is + * enumerated here rather than derived from the rows so the queue's filter cannot be driven to an + * arbitrary string from the URL, and so a type nobody built a view for cannot render an empty page + * that reads as "no work to do". + * + * πŸ”΄ This file is deliberately OUTSIDE `$lib/server/`. `RestrictionFilters.svelte` renders the type + * picker and therefore needs these as VALUES; SvelteKit rejects a value import of `$lib/server/*` from + * client-reachable code, and the sibling components only get away with importing from the service + * because theirs are `import type` and erase. Keep the list here and re-export it from the service. + * + * Mirrored for the main app in `src/server/services/user-restriction.service.ts`; the two lists are + * pinned to each other by `src/server/services/__tests__/restriction-type-seam.test.ts`, which + * imports and executes this module rather than reading it as text. + */ +export const RESTRICTION_TYPES = ['generation', 'bot-account'] as const; +export type RestrictionType = (typeof RESTRICTION_TYPES)[number]; + +/** What the queue shows when the URL names no type β€” the only type that existed before the seam. */ +export const RESTRICTION_TYPE: RestrictionType = 'generation'; + +export const RESTRICTION_TYPE_LABELS: Record = { + generation: 'Generation', + 'bot-account': 'Bot account', +}; + +/** + * The types a VERDICT can be handed to β€” narrower than `RESTRICTION_TYPES`, which is what may be + * filed and reviewed. + * + * The refusal itself is enforced by the main app, one level below every ruling surface, in + * `resolveUserRestriction` β€” this app's ruling forms all post through `/api/mod/restriction/resolve`. + * This list is the same rule read forward instead of backward: it is what lets a form that cannot + * possibly succeed be disabled rather than merely rejected, and what lets the audit queue refuse a + * ban BEFORE it bans (that action bans and then rules, so a late refusal would leave the account + * banned against a restriction nobody can close). + * + * πŸ”΄ Kept identical to the main app's `RULINGS_WIRED_FOR` by + * `src/server/services/__tests__/restriction-type-seam.test.ts`, which IMPORTS AND EXECUTES this + * module and compares the resulting values. The two apps are separate builds with no runtime import + * path between them, so a pinned copy is the strongest available form of "one rule, one place" β€” do + * not fork it by hand. + * + * πŸ”΄ That guard used to parse this file as TEXT, and passed green over a real divergence: a `]` in a + * trailing comment truncated its capture. Two preconditions come out of the replacement, and they are + * SEPARATE β€” the second is not implied by the first: + * + * 1. KEEP THIS MODULE IMPORT-FREE. It has no imports today, and that is the only reason the main + * app's Vitest project can load it across the app boundary; adding a `$lib/…` import here breaks + * the seam guard loudly rather than silently, but it does break it. + * 2. KEEP EVERY VALUE HERE ENVIRONMENT-INDEPENDENT β€” no `import.meta.env`, no `process.env`, and + * nothing derived from them. Neither needs an import statement, so rule 1 does not cover this. + * The guard EXECUTES this module inside the main app's test process: a list that branches on the + * environment is read under Vitest and never under this app's production build, so the two apps + * can ship different lists with every guard on both sides green. Assembling a value from + * constants declared in this file is fine β€” that is the same value everywhere. Reading one from + * the environment is not, and is refused by name. + */ +export const RULINGS_WIRED_FOR: readonly RestrictionType[] = ['generation']; + +/** Why a ruling may not be handed to a row of this type, or `null` when it may. */ +export function unwiredRulingReason(type: string): string | null { + return (RULINGS_WIRED_FOR as readonly string[]).includes(type) + ? null + : `Rulings are not yet available for "${type}" restrictions β€” the verdict path still sends generation-specific notices. This restriction was NOT resolved.`; +} diff --git a/apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts b/apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts new file mode 100644 index 0000000000..75b957e824 --- /dev/null +++ b/apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it, vi } from 'vitest'; + +/** + * `UserRestriction.type` is what separates one review queue from another. The rows carry no other + * marker β€” same table, same status vocabulary, same shape β€” so if the predicate is dropped, weakened, + * or bound to the wrong value, one queue simply renders another's cases and a moderator rules on them + * under the wrong assumptions. + * + * πŸ”΄ Asserted against the COMPILED SQL **and its bound parameters**. The text alone cannot see the bug + * this file exists for: `where('ur.type','=',x)` emits `"ur"."type" = $1` whatever `x` is, so a version + * that ignores its argument and always filters `generation` produces byte-identical SQL. The parameter + * is the only place the difference is visible. + * + * The count query is checked alongside the row query on purpose β€” they are separately compiled off a + * shared builder, and a filter that reaches one but not the other gives a pager whose total counts + * every type's rows. + */ + +const captured = vi.hoisted(() => [] as string[]); +const capturedParams = vi.hoisted(() => [] as unknown[][]); + +// Built inside the factory, not in `vi.hoisted`: hoisted blocks run before this file's own imports, so +// constructing the client there reads it before initialisation. +vi.mock('$lib/server/db', async () => { + const { capturingDb } = await import('../../../test/capture-sql'); + const db = capturingDb(captured, [], capturedParams); + return { dbRead: db, dbWrite: db }; +}); + +const { getGenerationRestrictions, RESTRICTION_TYPE } = await import('../user-restriction.service'); + +type Compiled = { sql: string; params: unknown[] }; + +const compile = async ( + query: Parameters[0] +): Promise => { + captured.length = 0; + capturedParams.length = 0; + await getGenerationRestrictions(query); + // πŸ”΄ Assert the COUNT, not just the contents. A chain that stops early leaves `captured` short while + // every assertion over what IS in it still passes β€” on a query that was never built. + expect(captured).toHaveLength(2); + return captured.map((sql, i) => ({ sql, params: capturedParams[i] })); +}; + +const base = { page: 1, limit: 20 } as const; + +/** The `ur.type` predicate's bound value, or `undefined` if the query emitted no such predicate. */ +const boundType = ({ sql, params }: Compiled): unknown => { + const match = /"ur"\."type" = \$(\d+)/.exec(sql); + return match ? params[Number(match[1]) - 1] : undefined; +}; + +describe('getGenerationRestrictions β€” type scoping', () => { + it('filters on the generation type when the caller names none', async () => { + const compiled = await compile({ ...base }); + + // Both statements, not just the first: the second is the pager's total. + for (const c of compiled) expect(boundType(c)).toBe('generation'); + expect(RESTRICTION_TYPE).toBe('generation'); + }); + + it('filters on the type it was given', async () => { + const compiled = await compile({ ...base, type: 'bot-account' }); + + for (const c of compiled) expect(boundType(c)).toBe('bot-account'); + }); + + // πŸ”΄ The leak, stated as a property rather than as one example: whatever type is asked for, no other + // type's rows can satisfy the query. A predicate bound to the requested value is what guarantees it. + it.each(['generation', 'bot-account'] as const)( + 'binds %s and no other type, in both the list and the count', + async (type) => { + const compiled = await compile({ ...base, type }); + + for (const c of compiled) { + expect(boundType(c)).toBe(type); + // Exactly one type PREDICATE β€” a second, differently-bound one would AND/OR in another queue. + // Matched with the `= $n` so this counts predicates and not the `ur.type` in the SELECT list. + expect(c.sql.match(/"ur"\."type" = \$\d+/g)).toHaveLength(1); + } + } + ); + + it('keeps the type predicate when other filters are also applied', async () => { + // The type predicate is unconditional while the rest are `$if`s. A refactor that folded it in with + // them is exactly how it would go missing, and only a query carrying both shapes can see that. + const compiled = await compile({ + ...base, + type: 'bot-account', + status: 'Pending', + username: 'someone', + }); + + for (const c of compiled) { + expect(boundType(c)).toBe('bot-account'); + expect(c.sql).toMatch(/"ur"\."status" = \$/); + } + }); + + /** + * The one caller allowed past the filter: a lookup by primary key. A form posts to `?/resolve`, which + * replaces the query string, so an action never learns which queue the moderator was in β€” filtering a + * by-id lookup by the DEFAULT type would 404 every row outside it. A primary key cannot be made more + * correct by a type predicate, so the predicate is dropped rather than guessed. + */ + it("drops the predicate only for the explicit 'any'", async () => { + const compiled = await compile({ ...base, type: 'any', restrictionId: 7 }); + + for (const c of compiled) { + expect(boundType(c)).toBeUndefined(); + // The PREDICATE is gone. `ur.type` still appears in the row query's SELECT list, which is what + // lets the caller read back the type it did not filter on. + expect(c.sql).not.toMatch(/"ur"\."type" = \$/); + expect(c.sql).toMatch(/"ur"\."id" = \$/); + } + }); + + it('selects the row type, so a caller can tell what it got back', async () => { + // `restrictionById` reads it to decide whether a ruling is wired for the row; without it in the + // SELECT that check silently compares against `undefined`. + const [list] = await compile({ ...base }); + + // πŸ”΄ Sliced to the SELECT list first. Asserted against the whole statement, this passes on the + // `"ur"."type" = $1` in the WHERE β€” which every pre-change version also emitted β€” so the guard + // would report coverage of a column that is not being selected at all. + const selectList = list.sql.slice(0, list.sql.indexOf(' from "UserRestriction"')); + expect(selectList).toContain('"ur"."type"'); + }); +}); diff --git a/apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts b/apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts new file mode 100644 index 0000000000..1fb0c8d629 --- /dev/null +++ b/apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it, vi } from 'vitest'; + +/** + * Which UserRestriction row the User Lookup header and its ruling form speak for. + * + * πŸ”΄ The panel shows ONE row, and until #4609 it picked "newest of any type" β€” `ORDER BY ur.id DESC + * LIMIT 1`, no type predicate. That was sound only while a user could hold at most one open row. + * Restrictions now dedupe PER TYPE, so two open cases can coexist, and the old ordering had a silent + * failure mode: a Pending generation case sitting behind a LATER Upheld bot-account row rendered as + * *no open restriction at all* β€” the account stays muted, the ruling form is never drawn, and nobody + * looking at the account can see there is an open case. + * + * Asserted against the COMPILED SQL. There is no database in this tier, and the ordering is the whole + * behaviour β€” it decides which of several rows the moderator is shown, and it typechecks and lints + * identically either way. Same instrument the sibling `restriction-type-filter.test.ts` uses, for the + * same reason. + */ + +const captured = vi.hoisted(() => [] as string[]); + +// Built inside the factory, not in `vi.hoisted`: hoisted blocks run before this file's own imports. +vi.mock('$lib/server/db', async () => { + const { capturingDb } = await import('../../../test/capture-sql'); + // One canned row, so `executeTakeFirst` resolves and the chain does not stop early. + const db = capturingDb(captured, [{ id: 1 }]); + return { dbRead: db, dbWrite: db }; +}); + +// The identity query is the only thing under test here; its module's other exports reach a second +// database and an HTTP service, and none of them is on this path. +const { getIdentity } = await import('../user-lookup.service'); + +const identitySql = async (): Promise => { + captured.length = 0; + await getIdentity(42); + // πŸ”΄ Count first. A chain that stopped early would leave `captured` empty while every assertion over + // its contents passed vacuously. + expect(captured).toHaveLength(1); + return captured[0].replace(/\s+/g, ' '); +}; + +/** + * The three correlated subqueries that read the account's restriction, sliced from the head of each + * `FROM "UserRestriction" ur` to its `LIMIT 1`. + */ +const restrictionSubqueries = (sql: string): string[] => + sql + .split('FROM "UserRestriction" ur') + .slice(1) + .map((chunk) => { + const end = chunk.indexOf('LIMIT 1'); + expect(end).toBeGreaterThan(-1); + return chunk.slice(0, end).trim(); + }); + +describe('user lookup β€” which restriction the panel speaks for', () => { + it('reads three restriction columns, which is what the panel needs to render a ruling form', async () => { + const sql = await identitySql(); + + // Positive control on the slicer, and the thing that makes every assertion below non-vacuous: a + // rename or a fourth column shows up here rather than silently reducing the loops to no-ops. + expect(restrictionSubqueries(sql)).toHaveLength(3); + for (const alias of ['restrictionStatus', 'restrictionType', 'restrictionId']) + expect(sql).toContain(`as "${alias}"`); + }); + + /** + * πŸ”΄ The regression. A Pending row outranks a merely newer one, so an open case cannot be hidden + * behind a later resolved one of another type. + */ + it('prefers a Pending row over a newer one, in every restriction column', async () => { + const sql = await identitySql(); + + for (const sub of restrictionSubqueries(sql)) + expect(sub).toContain(`ORDER BY (ur.status = 'Pending') DESC NULLS LAST, ur.id DESC`); + }); + + /** + * πŸ”΄ The null placement, asserted on its own so it cannot be dropped by someone tidying the + * ordering above back to a bare `DESC`. + * + * Postgres defaults `DESC` to `NULLS FIRST`. A NULL `(ur.status = 'Pending')` would therefore sort + * ABOVE a genuinely Pending row and hide the open case β€” exactly the failure the preference exists + * to prevent, arriving through the column's nullability rather than through the ordering. The + * column is a NOT NULL enum today, so this is an unstated precondition being made explicit rather + * than a live defect; spelled out, the ordering no longer depends on it. + */ + it('places nulls last, so the ordering does not depend on status being NOT NULL', async () => { + const sql = await identitySql(); + const subs = restrictionSubqueries(sql); + + // Non-vacuous: three subqueries exist to check. (The slicer's own control lives in the first + // test; this repeats the count because an empty list would make the loop below pass.) + expect(subs).toHaveLength(3); + for (const sub of subs) { + expect(sub).toContain(`'Pending') DESC NULLS LAST`); + // And not a bare `DESC` on that expression, which is what the default null placement is. + expect(sub).not.toMatch(/'Pending'\)\s+DESC\s*,/); + } + }); + + /** + * The tiebreak has to be TOTAL, or the three subqueries are free to resolve to different rows and + * the panel renders one row's status against another's id β€” a ruling form posting an id that does + * not belong to the case it is describing. + */ + it('breaks the tie on a unique column, so the three columns name one row', async () => { + const sql = await identitySql(); + const subs = restrictionSubqueries(sql); + + for (const sub of subs) expect(sub).toContain('ur.id DESC'); + // Identical apart from the column each selects β€” which is the only reason they agree. (An + // INVARIANT GUARD: true before this change too. Kept because the fix replaced three hand-written + // copies with one helper, and this is what stops them being hand-written again.) + const withoutSelectList = subs.map((s) => s.slice(s.indexOf('WHERE'))); + expect(new Set(withoutSelectList).size).toBe(1); + }); + + it('scopes each subquery to the account being looked up', async () => { + const sql = await identitySql(); + + for (const sub of restrictionSubqueries(sql)) expect(sub).toContain(`ur."userId" = u.id`); + }); +}); diff --git a/apps/moderator/src/lib/server/rest-error-reason.ts b/apps/moderator/src/lib/server/rest-error-reason.ts new file mode 100644 index 0000000000..dfc5476244 --- /dev/null +++ b/apps/moderator/src/lib/server/rest-error-reason.ts @@ -0,0 +1,49 @@ +/** + * The refusal a main-app endpoint wrote for the operator, read out of its JSON error body. + * + * πŸ”΄ TWO ENVELOPE SHAPES REACH THIS APP, and reading only one of them silently destroys the reason. + * `defineModeratorEndpoint` hands a throw to the main app's `handleEndpointError` + * (`src/server/utils/endpoint-helpers.ts`), and that helper emits: + * + * - `{ error, message, code }` β€” every `restErrorBody(...)` path, which is what the 5xx branch and + * the genericized-4xx branch use; + * - `{ message }` alone β€” the 4xx/503 PASS-THROUGH branch, i.e. exactly the statuses that carry a + * refusal somebody wrote FOR a human (`throwBadRequestError`, `throwNotFoundError`, …). + * + * Reading `error` only therefore came back `null` for the entire second group, and the caller fell + * back to `"