From f317b9f23e528b853a1208f9b201eca294290bf3 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Wed, 23 Sep 2026 22:12:15 +0100 Subject: [PATCH 1/6] Normalize Extensions v2 contract per RFC 0001 --- AGENTS.md | 2 +- src/services/extensions/v2/README.md | 33 ++- .../extensions/v2/RFC-0001-contract-tidy.md | 85 ++++++ .../extensions/v2/db/developer-claims.ts | 156 +++++++--- .../extensions/v2/db/developer-profiles.ts | 269 ++++++++++++++---- src/services/extensions/v2/db/extensions.ts | 84 ++++++ src/services/extensions/v2/email/templates.ts | 10 + .../v2/routes/developer-profiles.ts | 43 ++- src/services/extensions/v2/routes/errors.ts | 6 - .../extensions/v2/routes/moderation.ts | 127 ++++++--- .../extensions/v2/routes/ownership.ts | 69 +++-- src/services/extensions/v2/schemas/common.ts | 103 +++---- .../extensions/v2/schemas/developers.ts | 56 ++-- .../extensions/v2/schemas/ownership.ts | 35 +-- test/services/extensions/v2/contract.test.ts | 105 +++++++ .../services/extensions/v2/moderation.test.ts | 167 ++++++++++- test/services/extensions/v2/ownership.test.ts | 16 +- 17 files changed, 1035 insertions(+), 331 deletions(-) create mode 100644 src/services/extensions/v2/RFC-0001-contract-tidy.md create mode 100644 test/services/extensions/v2/contract.test.ts diff --git a/AGENTS.md b/AGENTS.md index aa2927b5..5c78f03b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,7 +89,7 @@ apply when modifying the code. ## Cache Revalidation Any endpoint that mutates catalogue-visible content (revision approve/reject, -delist, developer approve, developer profile upsert via `PUT /developers/me`, +delist/relist, developer approve, developer profile upsert via `PUT /developers/me`, profile deletion via `DELETE /developers/me`, claim approve/reject, extension withdraw) must call `revalidateCatalogue(c)` from `src/services/extensions/v2/revalidate.ts` after a successful write. Skipping it diff --git a/src/services/extensions/v2/README.md b/src/services/extensions/v2/README.md index 4e47d6c1..81fe86fa 100644 --- a/src/services/extensions/v2/README.md +++ b/src/services/extensions/v2/README.md @@ -34,8 +34,9 @@ against it. the public catalogue for cause (its upstream source disappearing, for example). Moderator-only, and the inverse of neither `approve` nor `reject`: content and history are kept, so the owner can still see and edit - the extension, and a moderator can re-list it by hand later. There is no - `relist` endpoint yet - see `ExtensionsDatabase.delist()`. + the extension. `POST /extensions/{id}/relist` restores it (optional + `review_note`, same `?notify` opt-out); see `ExtensionsDatabase.delist()` + and `relist()`. - `GET /extensions/{id}` is role-aware: anonymous and unrelated callers get the published projection (or 404, which hides existence for drafts and delisted rows); the owner and moderators get the full `OwnedExtension`, @@ -52,8 +53,11 @@ against it. ### Moderation Notification Emails -Revision approve/reject, delist, developer approve, and claim approve/reject -email the affected author unless the moderator opts out with `?notify=false`. +Revision approve/reject, delist/relist, developer approve, and claim +approve/reject email the affected author unless the moderator opts out with +`?notify=false`. Automatic decisions by the FOSSBilling Bot account use the +same routes and mail path; they are identified by the `[auto policy=…]` +`review_note` prefix (no schema change). The recipient is the developer's `contact_email`, falling back to the owning account's `email`; claim decisions go to the claimant's account email. Sending is best-effort and never fails the write: the result carries @@ -110,20 +114,23 @@ newest first, for its owner or any moderator. `GET /revisions` is the global review queue (moderator only, `?status=` defaulting to `pending`, oldest first). -`GET /developers?status=` (`all` default, `unapproved` for the review queue) -replaces `GET /developers/unapproved`. `GET /developers/claims?scope=mine` -(the caller's claims) and `?scope=pending` (moderator queue) replace -`GET /developers/claims/mine` and `GET /developers/claims`; both return the -enriched pending shape. `GET /developers/{id}` is role-aware like +`GET /developers?scope=` (`all` default, `unapproved` for the review queue; +`?status=` remains as a deprecated alias during the coordinated migration and +422s when it disagrees with `?scope=`) replaces `GET /developers/unapproved`. +`GET /developers/claims?scope=mine` (the caller's claims) and `?scope=pending` +(moderator queue) replace `GET /developers/claims/mine` and +`GET /developers/claims`; both return the enriched pending shape, and a +`status` filter disagreeing with `scope=pending` is rejected with 422 rather +than silently ignored. `GET /developers/{id}` is role-aware like `GET /extensions/{id}`: public view anonymously, full view for the owner or a moderator. `PATCH /users/me` returns the full account projection, like `GET /users/me`. `GET /developers`, `GET /developers/claims`, and `GET /developers/{id}/history` -support offset pagination via `?limit=` (1-100) and `?offset=`. `offset` -without `limit` is rejected with 422; with no params at all the routes apply -a bounded default window (100 rows) instead of streaming every row, and the -response always carries `pagination: {limit, offset, has_more}`. +page by opaque keyset cursor like every other v2 list: `?limit=` (1-100, +default 50) with `?cursor=` carried from the previous page's +`pagination.next_cursor`. An invalid cursor is rejected with `INVALID_CURSOR` +(422); the response envelope is always `pagination: {next_cursor, has_more}`. ## Authentication diff --git a/src/services/extensions/v2/RFC-0001-contract-tidy.md b/src/services/extensions/v2/RFC-0001-contract-tidy.md new file mode 100644 index 00000000..5e8aa1c3 --- /dev/null +++ b/src/services/extensions/v2/RFC-0001-contract-tidy.md @@ -0,0 +1,85 @@ +# RFC 0001 — Extensions v2 contract tidy (breaking, still v2) + +- Status: **implemented** (API + site co-migrated; see contract.test.ts). +- Scope: contract-only. No D1 schema change in this RFC unless flagged `migration?`. +- Constraint: only caller is the `extensions` site; API + site land in one coordinated release. Base path stays `/extensions/v2`. No v3. +- Style: **action-oriented, normalized** — not REST-ified into generic `PATCH {status}` (see §0). + +## 0. Style decision (locked) + +Resource CRUD stays REST (`POST/GET /extensions`, `PUT /extensions/{id}`, `DELETE` withdraw, `PUT /developers/me`, `PATCH /users/me`). Workflow transitions stay explicit verbs (`POST …/approve|reject|delist|relist|claim|cancel|transfer|revoke|accept|reverify`). Rationale: each verb owns distinct auth, validation, atomicity, and side-effects (mail + `revalidateCatalogue()`); merging into one `PATCH` forces role-conditional bodies and a branched handler that is harder to document, generate clients for, and review. A unified `PATCH` was evaluated and rejected. + +## 1. Inventory (current, 29 operations) + +Extensions: + +- `GET /extensions?scope=public|mine|all` (+`type,developer_id|status,q,limit,cursor`) — cursor `{next_cursor,has_more}` +- `GET /extensions/{id}` — role-aware union (published vs `OwnedExtension`) +- `POST /extensions` → 201 pending — `ExtensionCreateSchema` +- `PUT /extensions/{id}` → 202 pending — `ExtensionUpdateSchema`, owner-only +- `DELETE /extensions/{id}` — withdraw unpublished only +- `GET /extensions/{id}/revisions` — owner-or-moderator history, cursor, newest-first +- `GET /revisions?status` — moderator queue, cursor, oldest-first +- `POST /extensions/{id}/revisions/{revisionId}/approve` (note optional) / `reject` (note required) — `?notify=false` opt-out, `{…, notified}` +- `POST /extensions/{id}/delist` (`{reason}` required) — no inverse (gap) +- `GET /moderation/counts` + +Developers / ownership / users: + +- `GET /developers?status=all|unapproved` — **offset** `{limit,offset,has_more}`, tag `Moderation` +- `GET /developers/me`, `PUT /developers/me`, `DELETE /developers/me`, `POST /developers/me/reverify?check_url`, `GET /developers/{id}` (role-aware) +- `POST /developers/{id}/claim`, `POST /developers/claims/{id}/cancel`, `GET /developers/claims?scope=mine|pending` — **offset**; `scope=pending` ignores `status` filter +- `POST /developers/claims/{id}/approve|reject`, `POST /developers/{id}/approve`, `GET /developers/{id}/history` — **offset** +- `POST /developers/{id}/transfer`, `POST /developers/{id}/transfer/revoke`, `POST /developers/transfers/accept {token}` +- `PUT /users/me/identity` (assertion scope only), `GET /users/me`, `PATCH /users/me`, `DELETE /users/me` + +Inconsistencies: two pagination envelopes; three `scope`/`status` vocabularies; `cancel`/`revoke` as `POST` (kept deliberately, see §3); tags mixed by role vs resource (`listDevelopers` under `Moderation`); `?notify` + `notified` copy-pasted per handler; four error mappers (`statusFromErrorCode`, `statusFromWriteErrorCode`, `statusFromOwnershipErrorCode`, `statusFromGithubErrorCode`); no `relist`; reservation rules in comments + migration `0020` only. + +## 2. Pagination — cursor everywhere + +- Retire offset envelope (`OffsetPaginationSchema`, `offsetPageFromQuery`, `offsetPaginationFrom`) for v2 lists. Single envelope `PaginationSchema {next_cursor, has_more}` + `limit (1-100, default 50)` + opaque `cursor`. +- Applies to: `GET /developers`, `GET /developers/claims`, `GET /developers/{id}/history`. Queue ordering preserved (admin lists: newest-first default unless queue semantics say oldest-first). +- Frontend replaces `fetchWholeList` offset walker (`extensions/src/lib/api/client.ts`) with cursor walker (same shape as `listMyExtensions`). +- `migration?` No. + +## 3. Verbs — canonical `POST /{resources}/{id}/{verb}` + +- Keep `POST` for all transitions, including `cancel` and `revoke`. `DELETE` means "row gone" (`DELETE /extensions/{id}` withdraw, `DELETE /developers/me`, `DELETE /users/me`) — `cancel`/`revoke` leave history rows, so `POST …/cancel|revoke` is correct, not an accident. +- No renames except additions below. `transfers/accept` stays `POST /developers/transfers/accept {token}` (token is the address, not an id — nesting under `{id}` would leak existence). +- **Add `POST /extensions/{id}/relist`** (moderator-only): inverse of `delist`, requires `{note?}` (optional, trimmed, max 2000), clears `delisted_at/delist_reason`, `revalidateCatalogue()`, same `?notify` + `notified` shape, 409 when not delisted / unpublished. `migration?` No. +- `migration?` No. + +## 4. Scope — one pattern + +- `?scope` selects projection; `?status` narrows within it; mismatched combos 422 (extend the rule already enforced in `routes/public-extensions.ts`). +- `GET /developers?scope=all|unapproved` (replaces `?status=`; `status` param removed). Default `all`. +- `GET /developers/claims?scope=mine|pending` kept; `status` narrows `mine` only, ignored-with-422 on `pending` (today silently ignored — tighten to 422). +- `GET /extensions?scope=public|mine|all` unchanged. +- `migration?` No. + +## 5. Notify + result envelope — one helper, same wire + +- Keep `?notify=false` query (checkbox-checked default sends). Do not move to body — avoids touching every form handler in `extensions/src/pages/account/admin/**`. +- Add a shared `NotifiedSchema` fragment for the `notified: boolean` field ("recipient resolved and send dispatched, delivery async") and use it in all seven moderation transitions in `routes/moderation.ts` + `routes/ownership.ts` instead of the hand-restated descriptions. +- `migration?` No. + +## 6. Errors + tags — one mapper, tags by resource + +- Collapse `routes/errors.ts` to `statusFromErrorCode` (reads: 404/500 + github 422/429/503) + `statusFromWriteErrorCode` (writes: 403/404/409/500). Delete `statusFromOwnershipErrorCode` (fold into write mapper). No per-handler ternaries beyond github codes. +- Tags by resource: `Extensions` (all `/extensions*`, `/revisions`, `/moderation/counts` moves to `Extensions`? keep `Moderation` only for `/moderation/counts` + queue reads — **decision:** `GET /revisions`, `GET /moderation/counts` stay `Moderation`; everything else by resource: `listDevelopers` moves `Moderation` → `Developers`; claim approve/reject move `Moderation` → `Developers`). Scalar grouping then matches paths. +- Document status-code rule: `201` created (claim, extension create), `202` accepted-pending (propose edit), else `200`. `422` = zod/contract failure via `defaultHook`; `409` = guarded-write no-op with diagnosed reason. +- `migration?` No. + +## 7. Path reservations + registration order — contract test + +- Reserve `me`, `claims`, `transfers` under `/developers`; `mine` needs no reservation (ordinary id now). Keep registration order: static before `/{id}` (`index.ts` comments become test assertions). +- Add `test/services/extensions/v2/contract.test.ts`: reserved ids unreachable as data ids (mirrors migration `0020` logic), static routes win over param routes, `?scope` misuse matrix 422s, pagination envelope shape per route. +- `migration?` No. + +## Annex A — Reviewer identity (bot, no special treatment) + +`FOSSBilling Bot` is an existing account row with `is_moderator=1`. No reserved id, no new verifier, no migration. Auto-worker mints standard 60s HS256 assertions (`sub=`, existing `bearerAssertionVerifier`) and calls canonical approve/reject. Machine-readable audit via note prefix `[auto policy=/ score=<0-1>] …` (fits existing `ReviewNote*` max 2000). Presentation maps bot `sub` → "FOSSBilling Bot (automatic)"; ops rule: never `DELETE /users/me` as the bot (avoids `display_name` nulling via `deleteAccount`). Shadow-mode logging (`would_approve`) precedes live auto-approve; rollout limited to readme-only diffs first (classifiable via `revision-diff`). + +## Annex B — Deferred follow-up: `moderator-correct` (preview, not in this epic) + +Built after this RFC lands, on canonical verbs: `POST /extensions/{id}/moderator-correct` (moderator-only, `ExtensionUpdateSchema.strict()` + `{correction_note: trim min(1) max(2000)}`, no `?notify`/mail, 404 unknown, 409 pending-exists/unpublished/delisted, single `batch()` inserting `status='approved'` row with `submitted_by=reviewer_id=moderator` + publishing + `published_revision_id`, `revalidateCatalogue()`). Frontend `account/admin/extensions/[id]/edit.astro` reusing `ExtensionForm` + preview, blocked-state when pending, `purgeCatalogue()` + flash (no-mail copy). diff --git a/src/services/extensions/v2/db/developer-claims.ts b/src/services/extensions/v2/db/developer-claims.ts index 96825301..4ac13faf 100644 --- a/src/services/extensions/v2/db/developer-claims.ts +++ b/src/services/extensions/v2/db/developer-claims.ts @@ -1,6 +1,7 @@ -import { and, asc, desc, eq, isNull, sql } from "drizzle-orm"; +import { and, asc, desc, eq, gt, isNull, lt, or, sql, SQL } from "drizzle-orm"; import { DatabaseResult } from "../../../../lib/interfaces"; import { ExtensionsDb } from "../../../../lib/db"; +import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; import { developerClaims, developers, users } from "./schema"; import { databaseError, @@ -272,10 +273,16 @@ export class DeveloperClaimsDatabase { // Unified reader for the merged GET /developers/claims?scope=. Always // returns the enriched Pending shape so mine and pending share one // contract; scope=mine is caller-filtered (any status unless narrowed), - // scope=pending is moderator-wide (pending by default). The filter is a - // discriminated union so scope=mine cannot be called without the caller's - // id, which would otherwise drop the ownership predicate and return every - // claim in the table. + // scope=pending is moderator-wide (pending only — the route rejects any + // other status filter with 422). The filter is a discriminated union so + // scope=mine cannot be called without the caller's id, which would + // otherwise drop the ownership predicate and return every claim in the + // table. + // + // Keyset (cursor) pagination: mine orders newest-first, pending + // oldest-first (same keys, opposite directions — the cursor comparison + // flips like ExtensionRevisionsDatabase.page). Ties on created_at are + // broken by rowid (insertion order), as the offset implementation did. async listScoped( filters: | { @@ -287,14 +294,33 @@ export class DeveloperClaimsDatabase { scope: "pending"; status?: "pending" | "approved" | "rejected" | "all"; }, - page?: { limit: number; offset: number } + page?: { limit?: number; cursor?: string } ): Promise< - DatabaseResult<{ items: PendingDeveloperClaim[]; hasMore: boolean }> + DatabaseResult<{ + items: PendingDeveloperClaim[]; + nextCursor: string | null; + hasMore: boolean; + }> > { + const limit = page?.limit ?? 50; + const decoded = page?.cursor ? decodeClaimCursor(page.cursor) : null; + if (page?.cursor && !decoded) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } + const afterRowid = decoded ? Number(decoded.k2) : NaN; + if (decoded && !Number.isInteger(afterRowid)) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } const status = filters.status ?? "all"; - let rows; + const newestFirst = filters.scope === "mine"; try { - const conditions = []; + const conditions: SQL[] = []; if (filters.scope === "mine") { conditions.push(eq(developerClaims.claimantId, filters.claimantId)); } @@ -303,55 +329,72 @@ export class DeveloperClaimsDatabase { } else if (status !== "all") { conditions.push(eq(developerClaims.status, status)); } - const base = this.db + if (decoded) { + conditions.push( + newestFirst + ? or( + lt(developerClaims.createdAt, decoded.k1), + and( + eq(developerClaims.createdAt, decoded.k1), + sql`"developer_claims".rowid < ${afterRowid}` + ) + )! + : or( + gt(developerClaims.createdAt, decoded.k1), + and( + eq(developerClaims.createdAt, decoded.k1), + sql`"developer_claims".rowid > ${afterRowid}` + ) + )! + ); + } + const rows = await this.db .select({ claim: developerClaims, developerName: developers.name, developerType: developers.type, claimantName: users.name, - claimantGithubLogin: users.githubLogin + claimantGithubLogin: users.githubLogin, + rowid: sql`"developer_claims".rowid` }) .from(developerClaims) .innerJoin(developers, eq(developers.id, developerClaims.developerId)) - .leftJoin(users, eq(users.id, developerClaims.claimantId)); - const filtered = conditions.length - ? base.where(and(...conditions)) - : base; - // Offset pagination needs a deterministic total order: created_at - // ties are broken by rowid (insertion order), matching listHistory. - // limit+1 probe - see DeveloperProfilesDatabase.listWithOwnerPaged. - const ordered = filtered.orderBy( - filters.scope === "mine" - ? desc(developerClaims.createdAt) - : asc(developerClaims.createdAt), - filters.scope === "mine" - ? sql`"developer_claims".rowid DESC` - : sql`"developer_claims".rowid ASC` - ); - rows = page - ? await ordered.offset(page.offset).limit(page.limit + 1) - : await ordered; + .leftJoin(users, eq(users.id, developerClaims.claimantId)) + .where(conditions.length ? and(...conditions)! : undefined) + .orderBy( + newestFirst + ? desc(developerClaims.createdAt) + : asc(developerClaims.createdAt), + newestFirst + ? sql`"developer_claims".rowid DESC` + : sql`"developer_claims".rowid ASC` + ) + .limit(limit + 1); + + const hasMore = rows.length > limit; + const pageRows = rows.slice(0, limit); + const last = pageRows.at(-1); + return { + data: { + items: pageRows.map((row) => ({ + ...parseClaimRow(row.claim), + developer_name: row.developerName, + developer_type: + row.developerType as PendingDeveloperClaim["developer_type"], + claimant_name: row.claimantName, + claimant_github_login: row.claimantGithubLogin + })), + hasMore, + nextCursor: + hasMore && last + ? encodeClaimCursor(last.claim.createdAt, String(last.rowid)) + : null + }, + error: null + }; } catch (error) { return databaseError("listScoped", error); } - - const hasMore = page ? rows.length > page.limit : false; - const trimmed = page && hasMore ? rows.slice(0, page.limit) : rows; - - return { - data: { - items: trimmed.map((row) => ({ - ...parseClaimRow(row.claim), - developer_name: row.developerName, - developer_type: - row.developerType as PendingDeveloperClaim["developer_type"], - claimant_name: row.claimantName, - claimant_github_login: row.claimantGithubLogin - })), - hasMore - }, - error: null - }; } private async explainClaimApprovalNoOp( @@ -589,3 +632,22 @@ export class DeveloperClaimsDatabase { return this.getClaimById(claimId); } } + +interface ClaimCursor { + k1: string; + k2: string; +} + +function encodeClaimCursor(k1: string, k2: string): string { + return encode({ k1, k2 }); +} + +function isClaimCursor( + parsed: Record +): parsed is ClaimCursor & Record { + return typeof parsed.k1 === "string" && typeof parsed.k2 === "string"; +} + +function decodeClaimCursor(cursor: string): ClaimCursor | null { + return decode(cursor, isClaimCursor); +} diff --git a/src/services/extensions/v2/db/developer-profiles.ts b/src/services/extensions/v2/db/developer-profiles.ts index 35557216..1e87bf01 100644 --- a/src/services/extensions/v2/db/developer-profiles.ts +++ b/src/services/extensions/v2/db/developer-profiles.ts @@ -1,7 +1,7 @@ -import { and, asc, desc, eq, isNull, or, sql, SQL } from "drizzle-orm"; -import { SQLiteColumn } from "drizzle-orm/sqlite-core"; +import { and, asc, desc, eq, gt, isNull, lt, or, sql, SQL } from "drizzle-orm"; import { DatabaseResult } from "../../../../lib/interfaces"; import { ExtensionsDb } from "../../../../lib/db"; +import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; import { developers, developerHistory, @@ -635,66 +635,126 @@ export class DeveloperProfilesDatabase { // the only readers that join users for owner_name/owner_github_login, which // is why DeveloperProfile treats those fields as optional. // - // page (when given) bounds the query with a limit+1 probe - the extra row - // only answers has_more and is trimmed off - so an admin UI can walk the - // table instead of unconditionally streaming all of it. - private async listWithOwnerPaged( - context: string, - where: SQL | undefined, - orderBy: SQL | SQLiteColumn, - page?: { limit: number; offset: number } - ): Promise> { + // Keyset (cursor) pagination, matching GET /extensions and GET /revisions: + // limit+1 probe answers has_more and is trimmed off. Ties are broken by + // rowid (insertion order) — the order keys alone (name, created_at) are + // not unique and timestamps are second-granular, so same-second ties are + // the common case, not the exception. The opaque cursor carries rowid; + // it is never exposed outside the cursor. + // The cursor tags its scope (`s`): `all` orders by name, `unapproved` by + // created_at, so a cursor from one scope must 422 in the other rather than + // compare names against timestamps. + async listScoped(filters: { + scope?: "all" | "unapproved"; + status?: "all" | "unapproved"; + limit?: number; + cursor?: string; + }): Promise< + DatabaseResult<{ + items: DeveloperProfile[]; + nextCursor: string | null; + hasMore: boolean; + }> + > { + const scope = filters.scope ?? filters.status ?? "all"; + const limit = filters.limit ?? 50; + const decoded = filters.cursor + ? decodeDeveloperCursor(filters.cursor) + : null; + if (filters.cursor && !decoded) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } + if (decoded && decoded.s !== scope) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } + const afterRowid = decoded ? Number(decoded.k2) : NaN; + if (decoded && !Number.isInteger(afterRowid)) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } + + const conditions: SQL[] = []; + let orderBy; + if (scope === "unapproved") { + conditions.push(isNull(developers.approvedAt)); + if (decoded) { + conditions.push( + or( + gt(developers.createdAt, decoded.k1), + and( + eq(developers.createdAt, decoded.k1), + sql`"developers".rowid > ${afterRowid}` + ) + )! + ); + } + orderBy = [ + asc(developers.createdAt), + sql`"developers".rowid ASC` + ] as const; + } else { + if (decoded) { + conditions.push( + or( + gt(developers.name, decoded.k1), + and( + eq(developers.name, decoded.k1), + sql`"developers".rowid > ${afterRowid}` + ) + )! + ); + } + orderBy = [asc(developers.name), sql`"developers".rowid ASC`] as const; + } + let rows; try { - // Offset pagination needs a deterministic total order: the orderBy - // keys (name, created_at) are not unique, so rowid breaks ties the - // same way listHistory does. - const base = this.db + rows = await this.db .select({ developer: developers, ownerName: users.name, - ownerGithubLogin: users.githubLogin + ownerGithubLogin: users.githubLogin, + rowid: sql`"developers".rowid` }) .from(developers) .leftJoin(users, eq(users.id, developers.ownerUserId)) - .where(where) - .orderBy(orderBy, sql`"developers".rowid ASC`); - rows = page - ? await base.offset(page.offset).limit(page.limit + 1) - : await base; + .where(conditions.length ? and(...conditions)! : undefined) + .orderBy(...orderBy) + .limit(limit + 1); } catch (error) { - return databaseError(context, error); + return databaseError("listScoped", error); } - const hasMore = page ? rows.length > page.limit : false; - const trimmed = page && hasMore ? rows.slice(0, page.limit) : rows; + const hasMore = rows.length > limit; + const pageRows = rows.slice(0, limit); + const last = pageRows.at(-1); return { - data: { items: trimmed.map(parseDeveloperRowWithOwner), hasMore }, + data: { + items: pageRows.map(parseDeveloperRowWithOwner), + hasMore, + nextCursor: + hasMore && last + ? encodeDeveloperCursor( + scope, + scope === "unapproved" + ? last.developer.createdAt + : last.developer.name, + String(last.rowid) + ) + : null + }, error: null }; } - // Unified reader for the merged moderator GET /developers?status=. - async listScoped(filters: { - status?: "all" | "unapproved"; - page?: { limit: number; offset: number }; - }): Promise> { - if (filters.status === "unapproved") { - return this.listWithOwnerPaged( - "listUnapproved", - isNull(developers.approvedAt), - asc(developers.createdAt), - filters.page - ); - } - return this.listWithOwnerPaged( - "listAll", - undefined, - asc(developers.name), - filters.page - ); - } - async approve( id: string, expectedRevision: number, @@ -764,15 +824,52 @@ export class DeveloperProfilesDatabase { return { data: { id, approved: true }, error: null }; } + // Newest-first keyset pages of one developer's audit history. + // CURRENT_TIMESTAMP has only second resolution, so two writes in the same + // second tie on changed_at; rowid (insertion order) breaks the tie so + // "newest first" is never ambiguous, exactly as the offset implementation + // did. The opaque cursor carries rowid. async listHistory( developerId: string, - page?: { limit: number; offset: number } + page?: { limit?: number; cursor?: string } ): Promise< - DatabaseResult<{ items: DeveloperHistoryEntry[]; hasMore: boolean }> + DatabaseResult<{ + items: DeveloperHistoryEntry[]; + nextCursor: string | null; + hasMore: boolean; + }> > { + const limit = page?.limit ?? 50; + const decoded = page?.cursor ? decodeHistoryCursor(page.cursor) : null; + if (page?.cursor && !decoded) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } + const afterRowid = decoded ? Number(decoded.k2) : NaN; + if (decoded && !Number.isInteger(afterRowid)) { + return { + data: null, + error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } + }; + } + let rows; try { - const base = this.db + const conditions = [eq(developerHistory.developerId, developerId)]; + if (decoded) { + conditions.push( + or( + lt(developerHistory.changedAt, decoded.k1), + and( + eq(developerHistory.changedAt, decoded.k1), + sql`"developer_history".rowid < ${afterRowid}` + ) + )! + ); + } + rows = await this.db .select({ developerId: developerHistory.developerId, type: developerHistory.type, @@ -780,29 +877,24 @@ export class DeveloperProfilesDatabase { url: developerHistory.url, changedBy: developerHistory.changedBy, changedByName: users.name, - changedAt: developerHistory.changedAt + changedAt: developerHistory.changedAt, + rowid: sql`"developer_history".rowid` }) .from(developerHistory) .leftJoin(users, eq(users.id, developerHistory.changedBy)) - .where(eq(developerHistory.developerId, developerId)) - // CURRENT_TIMESTAMP has only second resolution, so two writes in - // the same second tie on changed_at; rowid (insertion order, - // implicit - not a declared schema column) breaks the tie so - // "newest first" is never ambiguous. + .where(and(...conditions)) .orderBy( desc(developerHistory.changedAt), sql`"developer_history".rowid DESC` - ); - // limit+1 probe - see listWithOwnerPaged. - rows = page - ? await base.offset(page.offset).limit(page.limit + 1) - : await base; + ) + .limit(limit + 1); } catch (error) { return databaseError("listHistory", error); } - const hasMore = page ? rows.length > page.limit : false; - const trimmed = page && hasMore ? rows.slice(0, page.limit) : rows; + const hasMore = rows.length > limit; + const trimmed = rows.slice(0, limit); + const last = trimmed.at(-1); return { data: { @@ -815,7 +907,11 @@ export class DeveloperProfilesDatabase { changed_by_name: row.changedByName, changed_at: row.changedAt })), - hasMore + hasMore, + nextCursor: + hasMore && last + ? encodeHistoryCursor(last.changedAt, String(last.rowid)) + : null }, error: null }; @@ -1062,3 +1158,50 @@ export class DeveloperProfilesDatabase { } } } + +interface DeveloperListCursor { + k1: string; + k2: string; + s: "all" | "unapproved"; +} + +function encodeDeveloperCursor( + scope: "all" | "unapproved", + k1: string, + k2: string +): string { + return encode({ k1, k2, s: scope }); +} + +function isDeveloperListCursor( + parsed: Record +): parsed is DeveloperListCursor & Record { + return ( + typeof parsed.k1 === "string" && + typeof parsed.k2 === "string" && + (parsed.s === "all" || parsed.s === "unapproved") + ); +} + +function decodeDeveloperCursor(cursor: string): DeveloperListCursor | null { + return decode(cursor, isDeveloperListCursor); +} + +interface HistoryCursor { + k1: string; + k2: string; +} + +function encodeHistoryCursor(k1: string, k2: string): string { + return encode({ k1, k2 }); +} + +function isHistoryCursor( + parsed: Record +): parsed is HistoryCursor & Record { + return typeof parsed.k1 === "string" && typeof parsed.k2 === "string"; +} + +function decodeHistoryCursor(cursor: string): HistoryCursor | null { + return decode(cursor, isHistoryCursor); +} diff --git a/src/services/extensions/v2/db/extensions.ts b/src/services/extensions/v2/db/extensions.ts index c359bef8..a80b000c 100644 --- a/src/services/extensions/v2/db/extensions.ts +++ b/src/services/extensions/v2/db/extensions.ts @@ -764,6 +764,90 @@ export class ExtensionsDatabase { error: { message: "Extension could not be delisted", code: "CONFLICT" } }; } + + // Inverse of delist(): restores a delisted-but-published extension to the + // catalogue. Content and history are untouched; only the delist markers + // are cleared. `AND delisted_at IS NOT NULL` makes this a single atomic + // check-and-set mirroring delist(). + async relist( + id: string, + moderatorId: string + ): Promise> { + let result; + try { + result = await this.db + .update(extensions) + .set({ + delistedAt: null, + delistReason: null, + updatedAt: sql`CURRENT_TIMESTAMP` + }) + .where( + and( + sql`LOWER(${extensions.id}) = LOWER(${id})`, + isNotNull(extensions.publishedAt), + isNotNull(extensions.delistedAt), + sql`EXISTS ( + SELECT 1 FROM ${users} + WHERE ${users.id} = ${moderatorId} AND ${users.deletedAt} IS NULL + )` + ) + ); + } catch (error) { + return databaseError("relist", error); + } + + if (!result.meta?.changes) { + return this.relistBlockedError(id, moderatorId); + } + + return { data: { id }, error: null }; + } + + private async relistBlockedError( + id: string, + moderatorId: string + ): Promise> { + const inactive = await inactiveActorError(this.db, moderatorId); + if (inactive) return { data: null, error: inactive }; + + let existing: + { publishedAt: string | null; delistedAt: string | null } | undefined; + try { + [existing] = await this.db + .select({ + publishedAt: extensions.publishedAt, + delistedAt: extensions.delistedAt + }) + .from(extensions) + .where(sql`LOWER(${extensions.id}) = LOWER(${id})`); + } catch (error) { + return databaseError("relist", error); + } + if (!existing) return notFound(id); + if (!existing.publishedAt) { + return { + data: null, + error: { + message: "Only a published extension can be relisted", + code: "CONFLICT" + } + }; + } + if (!existing.delistedAt) { + return { + data: null, + error: { + message: "This extension is not delisted", + code: "CONFLICT" + } + }; + } + return { + data: null, + error: { message: "Extension could not be relisted", code: "CONFLICT" } + }; + } } // Escapes SQLite LIKE metacharacters in a caller-supplied search term so a diff --git a/src/services/extensions/v2/email/templates.ts b/src/services/extensions/v2/email/templates.ts index f7e4fdde..590f6701 100644 --- a/src/services/extensions/v2/email/templates.ts +++ b/src/services/extensions/v2/email/templates.ts @@ -2,6 +2,7 @@ import type { EmailMessage } from "./types"; export type ModerationEmailKind = | "extension-delisted" + | "extension-relisted" | "revision-approved" | "revision-rejected" | "developer-approved" @@ -155,6 +156,15 @@ export function buildModerationEmail( `View it here: ${DASHBOARD_URL}` ]; break; + case "extension-relisted": + subject = `${extLabel} restored to the FOSSBilling directory`; + title = "Your extension is back in the directory"; + paragraphs = [ + `${extDisplay} has been restored to the public FOSSBilling extension directory by a moderator.`, + ...(input.reason ? [`Moderator note: ${input.reason}`] : []), + `View it here: ${DASHBOARD_URL}` + ]; + break; case "revision-approved": subject = `${extLabel} update approved`; title = "Your extension update was approved"; diff --git a/src/services/extensions/v2/routes/developer-profiles.ts b/src/services/extensions/v2/routes/developer-profiles.ts index be0482bf..f3243196 100644 --- a/src/services/extensions/v2/routes/developer-profiles.ts +++ b/src/services/extensions/v2/routes/developer-profiles.ts @@ -16,10 +16,8 @@ import { import { ActiveAccountRequiredResponse, IdParamSchema, - errorResponse, - offsetPageFromQuery, - offsetPaginationFrom, - OffsetPaginationSchema + PaginationSchema, + errorResponse } from "../schemas/common"; import { DeveloperDetailResponseSchema, @@ -39,9 +37,9 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { const listDevelopersRoute = createRoute({ method: "get", path: "/developers", - tags: ["Moderation"], + tags: ["Developers"], summary: - "List developer profiles: every profile (status=all) or awaiting review (status=unapproved)", + "List developer profiles: every profile (scope=all) or awaiting review (scope=unapproved)", security: [{ Bearer: [] }], middleware: [requireModerator()] as const, request: { query: DeveloperListQuerySchema }, @@ -51,44 +49,59 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { "application/json": { schema: z.object({ result: z.array(DeveloperProfileSchema), - pagination: OffsetPaginationSchema + pagination: PaginationSchema }) } }, - description: "Developer profiles matching the status filter" + description: "Developer profiles matching the scope filter" }, 401: errorResponse("Missing or invalid bearer token"), 403: { ...ActiveAccountRequiredResponse, description: "The account is inactive or the caller is not a moderator" }, - 422: errorResponse("status query failed validation"), + 422: errorResponse("scope, limit, or cursor query failed validation"), 500: errorResponse("Database error") } }); app.openapi(listDevelopersRoute, async (c) => { - const { status, limit, offset } = c.req.valid("query"); + const { scope, status, limit, cursor } = c.req.valid("query"); + if (scope !== undefined && status !== undefined && scope !== status) { + return c.json( + { + error: { + message: "scope and status must agree", + code: "VALIDATION_ERROR" + } + }, + 422 + ); + } + const effective = scope ?? status ?? "all"; const db = new DeveloperProfilesDatabase( getExtensionsDb(c.env.DB_EXTENSIONS) ); - const page = offsetPageFromQuery({ limit, offset }); - const { data, error } = await db.listScoped({ status, page }); + const { data, error } = await db.listScoped({ + scope: effective, + limit, + cursor + }); if (error || !data) { return c.json( { error: { message: error?.message ?? "Unable to load developers", - code: "DATABASE_ERROR" + code: error?.code ?? "DATABASE_ERROR" } }, - 500 + error?.code === "INVALID_CURSOR" ? 422 : 500 ); } return c.json( { result: data.items, - pagination: offsetPaginationFrom(page, data.hasMore) + pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } }, 200 ); diff --git a/src/services/extensions/v2/routes/errors.ts b/src/services/extensions/v2/routes/errors.ts index 01e18468..ff4b42c7 100644 --- a/src/services/extensions/v2/routes/errors.ts +++ b/src/services/extensions/v2/routes/errors.ts @@ -52,12 +52,6 @@ export function statusFromWriteErrorCode( return statusFromErrorCode(code); } -export function statusFromOwnershipErrorCode(code?: string): 403 | 404 | 500 { - if (code === "NOT_FOUND") return 404; - if (code === "FORBIDDEN" || code === "ACCOUNT_INACTIVE") return 403; - return 500; -} - // Every handler reports a failed DatabaseResult the same way: the database's // own message and code when it supplied one, a route-specific fallback and // DATABASE_ERROR when it did not. The status stays at the call site, since diff --git a/src/services/extensions/v2/routes/moderation.ts b/src/services/extensions/v2/routes/moderation.ts index 54faa035..624d6d3a 100644 --- a/src/services/extensions/v2/routes/moderation.ts +++ b/src/services/extensions/v2/routes/moderation.ts @@ -10,16 +10,15 @@ import { } from "./errors"; import { ActiveAccountRequiredResponse, + CursorPaginationQuerySchema, DelistReasonSchema, IdParamSchema, - ListPaginationQuerySchema, + NotifiedSchema, NotifyQuerySchema, - OffsetPaginationSchema, + PaginationSchema, ReviewNoteOptionalSchema, ReviewNoteRequiredSchema, - errorResponse, - offsetPageFromQuery, - offsetPaginationFrom + errorResponse } from "../schemas/common"; import { DeveloperApprovalSchema, @@ -60,11 +59,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { result: z.object({ id: z.string(), status: z.literal("approved"), - notified: z - .boolean() - .describe( - "Whether a notification email was dispatched - delivery itself is asynchronous" - ) + notified: NotifiedSchema }) }) } @@ -146,11 +141,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { result: z.object({ id: z.string(), status: z.literal("rejected"), - notified: z - .boolean() - .describe( - "Whether a notification email was dispatched - delivery itself is asynchronous" - ) + notified: NotifiedSchema }) }) } @@ -230,11 +221,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { result: z.object({ id: z.string(), status: z.literal("delisted"), - notified: z - .boolean() - .describe( - "Whether a notification email was dispatched - delivery itself is asynchronous" - ) + notified: NotifiedSchema }) }) } @@ -289,6 +276,82 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { ); }); + const relistRoute = createRoute({ + method: "post", + path: "/extensions/{id}/relist", + tags: ["Moderation"], + summary: "Restore a delisted extension to the public catalogue", + security: [{ Bearer: [] }], + middleware: [requireModerator()] as const, + request: { + params: IdParamSchema, + query: NotifyQuerySchema, + body: { + content: { "application/json": { schema: ReviewNoteOptionalSchema } } + } + }, + responses: { + 200: { + content: { + "application/json": { + schema: z.object({ + result: z.object({ + id: z.string(), + status: z.literal("relisted"), + notified: NotifiedSchema + }) + }) + } + }, + description: + "Extension restored to the public catalogue. Content and history unchanged." + }, + 401: errorResponse("Missing or invalid bearer token"), + 403: { + ...ActiveAccountRequiredResponse, + description: "The account is inactive or the caller is not a moderator" + }, + 404: errorResponse("No such extension"), + 409: errorResponse("Extension is not delisted, or was never published"), + 422: errorResponse( + "Path params, review_note body, or notify query failed validation" + ), + 500: errorResponse("Database error") + } + }); + + app.openapi(relistRoute, async (c) => { + const auth = getAuth(c); + const { id } = c.req.valid("param"); + const { review_note } = c.req.valid("json"); + const query = c.req.valid("query"); + const extDb = getExtensionsDb(c.env.DB_EXTENSIONS); + const db = new ExtensionsDatabase(extDb); + const { data, error } = await db.relist(id, auth.userId); + if (error || !data) { + const status = statusFromWriteErrorCode(error?.code); + return c.json(errorBody(error, "Unable to relist extension"), status); + } + revalidateCatalogue(c); + let notified = false; + if (notifyRequested(query)) { + notified = await sendModerationNotification( + getPlatform(c), + extDb, + { + kind: "extension-relisted", + extensionId: id, + reason: review_note?.trim() || undefined + }, + (p) => c.executionCtx.waitUntil(p) + ); + } + return c.json( + { result: { id: data.id, status: "relisted" as const, notified } }, + 200 + ); + }); + const approveDeveloperRoute = createRoute({ method: "post", path: "/developers/{id}/approve", @@ -311,11 +374,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { result: z.object({ id: z.string(), approved: z.literal(true), - notified: z - .boolean() - .describe( - "Whether a notification email was dispatched - delivery itself is asynchronous" - ) + notified: NotifiedSchema }) }) } @@ -376,14 +435,14 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { summary: "List the write history of a developer profile", security: [{ Bearer: [] }], middleware: [requireModerator()] as const, - request: { params: IdParamSchema, query: ListPaginationQuerySchema }, + request: { params: IdParamSchema, query: CursorPaginationQuerySchema }, responses: { 200: { content: { "application/json": { schema: z.object({ result: z.array(DeveloperHistoryEntrySchema), - pagination: OffsetPaginationSchema + pagination: PaginationSchema }) } }, @@ -394,26 +453,28 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { ...ActiveAccountRequiredResponse, description: "The account is inactive or the caller is not a moderator" }, - 422: errorResponse("id param or pagination query failed validation"), + 422: errorResponse("id param, limit, or cursor query failed validation"), 500: errorResponse("Database error") } }); app.openapi(developerHistoryRoute, async (c) => { const { id } = c.req.valid("param"); - const { limit, offset } = c.req.valid("query"); - const page = offsetPageFromQuery({ limit, offset }); + const { limit, cursor } = c.req.valid("query"); const db = new DeveloperProfilesDatabase( getExtensionsDb(c.env.DB_EXTENSIONS) ); - const { data, error } = await db.listHistory(id, page); + const { data, error } = await db.listHistory(id, { limit, cursor }); if (error || !data) { - return c.json(errorBody(error, "Unable to load developer history"), 500); + return c.json( + errorBody(error, "Unable to load developer history"), + error?.code === "INVALID_CURSOR" ? 422 : 500 + ); } return c.json( { result: data.items, - pagination: offsetPaginationFrom(page, data.hasMore) + pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } }, 200 ); diff --git a/src/services/extensions/v2/routes/ownership.ts b/src/services/extensions/v2/routes/ownership.ts index acc51989..14133eeb 100644 --- a/src/services/extensions/v2/routes/ownership.ts +++ b/src/services/extensions/v2/routes/ownership.ts @@ -7,17 +7,16 @@ import { errorBody, statusFromErrorCode, statusFromGithubErrorCode, - statusFromOwnershipErrorCode + statusFromWriteErrorCode } from "./errors"; import { ActiveAccountRequiredResponse, IdParamSchema, + NotifiedSchema, NotifyQuerySchema, - OffsetPaginationSchema, + PaginationSchema, ReviewNoteRequiredSchema, - errorResponse, - offsetPageFromQuery, - offsetPaginationFrom + errorResponse } from "../schemas/common"; import { DeveloperProfileSchema } from "../schemas/developers"; import { @@ -161,7 +160,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { "application/json": { schema: z.object({ result: z.array(PendingDeveloperClaimSchema), - pagination: OffsetPaginationSchema + pagination: PaginationSchema }) } }, @@ -170,18 +169,34 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { }, 401: errorResponse("Missing or invalid bearer token"), 403: ActiveAccountRequiredResponse, - 422: errorResponse("scope query failed validation"), + 422: errorResponse( + "scope, status, limit, or cursor query failed validation" + ), 500: errorResponse("Database error") } }); app.openapi(listClaimsRoute, async (c) => { const auth = getAuth(c); - const { scope, status, limit, offset } = c.req.valid("query"); + const { scope, status, limit, cursor } = c.req.valid("query"); const extDb = getExtensionsDb(c.env.DB_EXTENSIONS); const db = new DeveloperClaimsDatabase(extDb); - const page = offsetPageFromQuery({ limit, offset }); + const page = { limit, cursor }; if (scope === "pending") { + // The pending scope is the moderator review queue: always pending. + // A status filter that disagrees is rejected rather than silently + // ignored so a caller cannot mistake one projection for another. + if (status !== "all" && status !== "pending") { + return c.json( + { + error: { + message: "status is only valid with scope=mine", + code: "VALIDATION_ERROR" + } + }, + 422 + ); + } const users = new UsersDatabase(extDb); const access = await users.moderatorAccess(auth.userId); if (access.error) { @@ -206,8 +221,6 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { 403 ); } - // The pending scope is the moderator review queue: always pending, - // regardless of any status filter (which only narrows scope=mine). const { data, error } = await db.listScoped( { scope: "pending", @@ -220,16 +233,16 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { { error: { message: error?.message ?? "Unable to load pending claims", - code: "DATABASE_ERROR" + code: error?.code ?? "DATABASE_ERROR" } }, - 500 + error?.code === "INVALID_CURSOR" ? 422 : 500 ); } return c.json( { result: data.items, - pagination: offsetPaginationFrom(page, data.hasMore) + pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } }, 200 ); @@ -247,16 +260,16 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { { error: { message: error?.message ?? "Unable to load claims", - code: "DATABASE_ERROR" + code: error?.code ?? "DATABASE_ERROR" } }, - 500 + error?.code === "INVALID_CURSOR" ? 422 : 500 ); } return c.json( { result: data.items, - pagination: offsetPaginationFrom(page, data.hasMore) + pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } }, 200 ); @@ -265,7 +278,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { const approveClaimRoute = createRoute({ method: "post", path: "/developers/claims/{id}/approve", - tags: ["Moderation"], + tags: ["Developers"], summary: "Approve a pending profile claim", security: [{ Bearer: [] }], middleware: [requireModerator()] as const, @@ -277,11 +290,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { schema: z.object({ result: DeveloperProfileSchema.and( z.object({ - notified: z - .boolean() - .describe( - "Whether a notification email was dispatched - delivery itself is asynchronous" - ) + notified: NotifiedSchema }) ) }) @@ -340,7 +349,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { const rejectClaimRoute = createRoute({ method: "post", path: "/developers/claims/{id}/reject", - tags: ["Moderation"], + tags: ["Developers"], summary: "Reject a pending profile claim", security: [{ Bearer: [] }], middleware: [requireModerator()] as const, @@ -358,11 +367,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { schema: z.object({ result: DeveloperClaimSchema.and( z.object({ - notified: z - .boolean() - .describe( - "Whether a notification email was dispatched - delivery itself is asynchronous" - ) + notified: NotifiedSchema }) ) }) @@ -438,6 +443,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { "The account is inactive or the caller does not own this profile" }, 404: errorResponse("No developer with that id"), + 409: errorResponse("Ownership conflict"), 422: errorResponse("id param failed validation"), 500: errorResponse("Database error") } @@ -453,7 +459,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to create transfer"), - statusFromOwnershipErrorCode(error?.code) + statusFromWriteErrorCode(error?.code) ); } return c.json({ result: data }, 200); @@ -485,6 +491,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { "The account is inactive or the caller does not own this profile" }, 404: errorResponse("No developer with that id"), + 409: errorResponse("Ownership conflict"), 422: errorResponse("id param failed validation"), 500: errorResponse("Database error") } @@ -500,7 +507,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to revoke transfer"), - statusFromOwnershipErrorCode(error?.code) + statusFromWriteErrorCode(error?.code) ); } return c.json({ result: data }, 200); diff --git a/src/services/extensions/v2/schemas/common.ts b/src/services/extensions/v2/schemas/common.ts index 6d3df2cf..74ddd768 100644 --- a/src/services/extensions/v2/schemas/common.ts +++ b/src/services/extensions/v2/schemas/common.ts @@ -104,68 +104,41 @@ export const PaginationSchema = z }) .openapi("Pagination"); -// Opt-in offset pagination for the moderator/audit list endpoints: offset -// without limit is rejected (422) rather than silently ignored, since the -// generated schema would otherwise advertise a param the routes drop. -// Params entirely omitted fall back to a bounded default window - these -// lists are moderator-only, developer_history is append-only, and -// "no params" previously meant streaming every row. Callers that want -// everything page through with limit=100; the response envelope reports -// has_more either way. -export const offsetRequiresLimit = (query: { - limit?: number; - offset?: number; -}): boolean => query.limit !== undefined || query.offset === undefined; - -export const ListPaginationQuerySchema = z - .object({ - limit: z.coerce - .number() - .int() - .min(1) - .max(100) - .optional() - .openapi({ param: { name: "limit", in: "query" } }), - offset: z.coerce - .number() - .int() - .min(0) - .optional() - .openapi({ param: { name: "offset", in: "query" } }) - }) - .refine(offsetRequiresLimit, { - message: "offset requires limit" - }); - -export const OffsetPaginationSchema = z - .object({ - limit: z.number().int(), - offset: z.number().int(), - has_more: z.boolean() - }) - .openapi("OffsetPagination"); - -// Normalises the validated pagination query into the shape the database -// readers take: offset defaults to 0, and params entirely omitted fall back -// to a bounded default window (see the contract note above) instead of an -// unbounded read. -const DEFAULT_OFFSET_PAGE = { limit: 100, offset: 0 }; - -export function offsetPageFromQuery(query: { - limit?: number; - offset?: number; -}): { limit: number; offset: number } { - return { - limit: query.limit ?? DEFAULT_OFFSET_PAGE.limit, - offset: query.offset ?? 0 - }; -} - -// The response half of the same deal: the OffsetPagination envelope, -// reporting the applied window and whether more rows follow it. -export function offsetPaginationFrom( - page: { limit: number; offset: number }, - hasMore: boolean -): { limit: number; offset: number; has_more: boolean } { - return { ...page, has_more: hasMore }; -} +// Shared moderation-write envelope fragment: whether a notification email +// was dispatched (delivery itself is asynchronous). Declared once so every +// moderation transition reports `notified` identically. +export const NotifiedSchema = z + .boolean() + .describe( + "Whether a notification email was dispatched - delivery itself is asynchronous" + ) + .openapi({ example: true }); + +// Cursor pagination for moderator/audit lists (RFC 0001): every v2 list +// now pages by opaque keyset cursor, matching GET /extensions and +// GET /revisions. `limit` bounds the window (default 50, max 100); +// `cursor` is the `next_cursor` of the previous page. An invalid cursor is +// rejected with INVALID_CURSOR (422) rather than restarting pagination. +// Callers that want everything page through with limit=100; the response +// envelope (PaginationSchema) reports has_more either way. +export const CursorPaginationQuerySchema = z.object({ + limit: z.coerce + .number() + .int() + .min(1) + .max(100) + .default(50) + .openapi({ param: { name: "limit", in: "query" } }), + // min(1) matches ExtensionListQuerySchema: without it `?cursor=` arrives + // as an empty string, which the page helper would treat as "no cursor" + // and silently restart pagination instead of reporting the malformed value. + cursor: z + .string() + .min(1) + .max(1000) + .optional() + .openapi({ + param: { name: "cursor", in: "query" }, + description: "Opaque cursor returned by the previous page" + }) +}); diff --git a/src/services/extensions/v2/schemas/developers.ts b/src/services/extensions/v2/schemas/developers.ts index f7614d23..c20f88ec 100644 --- a/src/services/extensions/v2/schemas/developers.ts +++ b/src/services/extensions/v2/schemas/developers.ts @@ -1,12 +1,7 @@ import { z } from "@hono/zod-openapi"; -import { - httpUrl, - lowercaseId, - ListPaginationQuerySchema, - offsetRequiresLimit -} from "./common"; - -// GET /developers/unapproved has been merged into GET /developers?status=. +import { httpUrl, lowercaseId, CursorPaginationQuerySchema } from "./common"; + +// GET /developers/unapproved has been merged into GET /developers?scope=. // "unapproved" is therefore no longer a shadowed static route, but stays // reserved so an adopted row can never collide with a future static segment. // "claims" and "me" are still live static routes under /developers/*. @@ -145,25 +140,32 @@ export const DeveloperApprovalSchema = z .strict() .openapi("DeveloperApproval"); -// Merged moderator GET /developers: status=all (default) lists every profile, -// status=unapproved lists only profiles awaiting review. Replaces the former -// GET /developers/unapproved sibling route. -export const DeveloperListQuerySchema = z - .object({ - status: z - .enum(["all", "unapproved"]) - .default("all") - .openapi({ - param: { name: "status", in: "query" }, - description: - "all: every profile. unapproved: only profiles awaiting review." - }), - limit: ListPaginationQuerySchema.shape.limit, - offset: ListPaginationQuerySchema.shape.offset - }) - .refine(offsetRequiresLimit, { - message: "offset requires limit" - }); +// Moderator GET /developers: scope=all (default) lists every profile, +// scope=unapproved lists only profiles awaiting review. Replaces the former +// GET /developers/unapproved sibling route. `scope` (not `status`) selects +// the projection, matching GET /extensions?scope= and +// GET /developers/claims?scope=. +export const DeveloperListQuerySchema = CursorPaginationQuerySchema.extend({ + scope: z + .enum(["all", "unapproved"]) + .optional() + .openapi({ + param: { name: "scope", in: "query" }, + description: + "all: every profile (default). unapproved: only profiles awaiting review." + }), + // Deprecated alias for the coordinated migration: ?status=all|unapproved + // behaves like ?scope=. Explicit conflict (?scope=X&status=Y, X!=Y) is + // rejected with 422 in the route so a caller cannot mistake one + // projection for another. Remove once the site migrates to ?scope=. + status: z + .enum(["all", "unapproved"]) + .optional() + .openapi({ + param: { name: "status", in: "query" }, + description: "Deprecated alias for scope." + }) +}); // Merged GET /developers/{id} (optional auth, role-aware): anonymous callers // get the sanitized PublicDeveloper, the owning caller gets their full Owned diff --git a/src/services/extensions/v2/schemas/ownership.ts b/src/services/extensions/v2/schemas/ownership.ts index 421af36d..1ec453a8 100644 --- a/src/services/extensions/v2/schemas/ownership.ts +++ b/src/services/extensions/v2/schemas/ownership.ts @@ -1,5 +1,5 @@ import { z } from "@hono/zod-openapi"; -import { ListPaginationQuerySchema, offsetRequiresLimit } from "./common"; +import { CursorPaginationQuerySchema } from "./common"; export const TransferAcceptanceSchema = z .object({ token: z.string().min(64).max(128) }) @@ -67,23 +67,18 @@ export const ClaimNoteSchema = z // share one contract; status optionally narrows the mine view. export const ClaimsScopeSchema = z.enum(["mine", "pending"]); -export const UnifiedClaimsQuerySchema = z - .object({ - scope: ClaimsScopeSchema.openapi({ - param: { name: "scope", in: "query" }, +export const UnifiedClaimsQuerySchema = CursorPaginationQuerySchema.extend({ + scope: ClaimsScopeSchema.openapi({ + param: { name: "scope", in: "query" }, + description: + "mine: the caller's own claims. pending: claims awaiting review (moderator only)." + }), + status: z + .enum(["pending", "approved", "rejected", "all"]) + .default("all") + .openapi({ + param: { name: "status", in: "query" }, description: - "mine: the caller's own claims. pending: claims awaiting review (moderator only)." - }), - status: z - .enum(["pending", "approved", "rejected", "all"]) - .default("all") - .openapi({ - param: { name: "status", in: "query" }, - description: "Filter claims by status (default: all)" - }), - limit: ListPaginationQuerySchema.shape.limit, - offset: ListPaginationQuerySchema.shape.offset - }) - .refine(offsetRequiresLimit, { - message: "offset requires limit" - }); + "Filter claims by status (default: all). Only valid with scope=mine." + }) +}); diff --git a/test/services/extensions/v2/contract.test.ts b/test/services/extensions/v2/contract.test.ts new file mode 100644 index 00000000..7b15780a --- /dev/null +++ b/test/services/extensions/v2/contract.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect, vi } from "vitest"; +import { + setupExtensionsV2Tests, + db, + authHeaders, + get, + put, + sampleDeveloper, + seedDeveloper +} from "./harness"; +import { insertUser } from "./db-fixtures"; + +// Hoisted so no v2 suite can make a real GitHub call. +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); + +setupExtensionsV2Tests(); + +// RFC 0001 contract invariants: static routes beat param routes, reserved +// ids stay unreachable as data, scope misuse 422s instead of silently +// returning the wrong projection, and every list answers the cursor +// envelope. +describe("Extensions API v2 contract", () => { + it("serves static developer routes ahead of /developers/{id}", async () => { + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper() + ); + + const me = await get( + "/extensions/v2/developers/me", + await authHeaders("user-1") + ); + expect(me.status).toBe(200); + await expect(me.json()).resolves.toMatchObject({ + result: { id: "dev-developer" } + }); + }); + + it("rejects reserved developer ids", async () => { + for (const reserved of ["me", "claims", "unapproved"]) { + const res = await put( + "/extensions/v2/developers/me", + await authHeaders(`user-${reserved}`), + sampleDeveloper({ id: reserved }) + ); + expect(res.status).toBe(422); + } + }); + + it("rejects scope misuse instead of returning the wrong projection", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + // developer_id is public-scope only. + const scoped = await get( + "/extensions/v2/extensions?scope=mine&developer_id=dev-developer", + mod + ); + expect(scoped.status).toBe(422); + + // status/q are all-scope only. + const queued = await get("/extensions/v2/extensions?status=published", mod); + expect(queued.status).toBe(422); + }); + + it("answers every list with the cursor envelope", async () => { + await seedDeveloper("new-developer", "user-1"); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + for (const path of [ + "/extensions/v2/extensions", + "/extensions/v2/revisions", + "/extensions/v2/developers", + "/extensions/v2/developers/claims?scope=pending", + "/extensions/v2/developers/new-developer/history" + ]) { + const res = await get(path, mod); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toMatchObject({ + pagination: { next_cursor: null, has_more: false } + }); + } + }); + + it("rejects invalid cursors on every list", async () => { + await seedDeveloper("new-developer", "user-1"); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + for (const path of [ + "/extensions/v2/extensions?cursor=nope", + "/extensions/v2/revisions?cursor=nope", + "/extensions/v2/developers?cursor=nope", + "/extensions/v2/developers/claims?scope=pending&cursor=nope", + "/extensions/v2/developers/new-developer/history?cursor=nope" + ]) { + const res = await get(path, mod); + expect(res.status).toBe(422); + } + }); +}); diff --git a/test/services/extensions/v2/moderation.test.ts b/test/services/extensions/v2/moderation.test.ts index c410b942..6c536dd3 100644 --- a/test/services/extensions/v2/moderation.test.ts +++ b/test/services/extensions/v2/moderation.test.ts @@ -1038,6 +1038,82 @@ describe("Extensions API v2", () => { }); }); + describe("POST /extensions/{id}/relist", () => { + it("requires moderator access", async () => { + await seedDeveloper("new-developer", "user-1"); + await insertExtension(db, { + id: "live-ext", + developer_id: "new-developer" + }); + + const res = await post( + "/extensions/v2/extensions/live-ext/relist", + await authHeaders("user-1"), + {} + ); + expect(res.status).toBe(403); + }); + + it("404s for an unknown extension", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + + const res = await post( + "/extensions/v2/extensions/no-such-extension/relist", + await authHeaders("mod-1"), + {} + ); + expect(res.status).toBe(404); + }); + + it("409s for an extension that is not delisted", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + await seedDeveloper("new-developer", "user-1"); + await insertExtension(db, { + id: "live-ext", + developer_id: "new-developer" + }); + + const res = await post( + "/extensions/v2/extensions/live-ext/relist", + await authHeaders("mod-1"), + {} + ); + expect(res.status).toBe(409); + }); + + it("restores a delisted extension to the public catalogue", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + await seedDeveloper("new-developer", "user-1"); + await insertExtension(db, { + id: "live-ext", + developer_id: "new-developer" + }); + const mod = await authHeaders("mod-1"); + await post("/extensions/v2/extensions/live-ext/delist", mod, { + reason: "Upstream source removed" + }); + expect((await get("/extensions/v2/extensions", {})).status).toBe(200); + await expect( + (await get("/extensions/v2/extensions", {})).json() + ).resolves.toMatchObject({ result: [] }); + + const res = await post("/extensions/v2/extensions/live-ext/relist", mod, { + review_note: "Upstream is back" + }); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ + result: { id: "live-ext", status: "relisted", notified: false } + }); + + expect(await getExtension(db, "live-ext")).toMatchObject({ + delisted_at: null + }); + await expect( + (await get("/extensions/v2/extensions", {})).json() + ).resolves.toMatchObject({ result: [{ id: "live-ext" }] }); + }); + }); + describe("developer moderation", () => { it("binds approval to the exact profile revision reviewed", async () => { await put( @@ -1233,6 +1309,78 @@ describe("Extensions API v2", () => { expect(res.status).toBe(403); }); + it("lists by scope and rejects a conflicting scope/status pair", async () => { + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper() + ); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + const scoped = await get( + "/extensions/v2/developers?scope=unapproved", + mod + ); + expect(scoped.status).toBe(200); + await expect(scoped.json()).resolves.toMatchObject({ + result: [{ id: "dev-developer" }], + pagination: { next_cursor: null, has_more: false } + }); + + const conflict = await get( + "/extensions/v2/developers?scope=all&status=unapproved", + mod + ); + expect(conflict.status).toBe(422); + }); + + it("walks the developer list by cursor", async () => { + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper({ id: "aaa-developer", name: "Aaa Developer" }) + ); + await put( + "/extensions/v2/developers/me", + await authHeaders("user-2"), + sampleDeveloper({ id: "zzz-developer", name: "Zzz Developer" }) + ); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + const first = await get("/extensions/v2/developers?limit=1", mod); + expect(first.status).toBe(200); + const firstBody = (await first.json()) as { + result: Array<{ id: string }>; + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(firstBody.result.map((d) => d.id)).toEqual(["aaa-developer"]); + expect(firstBody.pagination.has_more).toBe(true); + expect(firstBody.pagination.next_cursor).toBeTruthy(); + + const second = await get( + `/extensions/v2/developers?limit=1&cursor=${encodeURIComponent(firstBody.pagination.next_cursor!)}`, + mod + ); + expect(second.status).toBe(200); + const secondBody = (await second.json()) as { + result: Array<{ id: string }>; + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(secondBody.result.map((d) => d.id)).toEqual(["zzz-developer"]); + expect(secondBody.pagination).toEqual({ + next_cursor: null, + has_more: false + }); + + const bad = await get( + "/extensions/v2/developers?cursor=not-a-cursor", + mod + ); + expect(bad.status).toBe(422); + }); + it("blocks non-moderators from approving developers", async () => { await put( "/extensions/v2/developers/me", @@ -1277,9 +1425,7 @@ describe("Extensions API v2", () => { expect(data.result[0].changed_by).toBe("user-1"); }); - // Params omitted used to mean "stream every row" - developer_history is - // append-only, so the default is now a bounded window (100) with the - // pagination envelope reporting what was applied. + // Params omitted fall back to a bounded cursor window (default limit 50). it("applies the default pagination window when params are omitted", async () => { await put( "/extensions/v2/developers/me", @@ -1296,15 +1442,24 @@ describe("Extensions API v2", () => { expect(res.status).toBe(200); const data = (await res.json()) as { result: unknown[]; - pagination: { limit: number; offset: number; has_more: boolean }; + pagination: { next_cursor: string | null; has_more: boolean }; }; expect(data.pagination).toEqual({ - limit: 100, - offset: 0, + next_cursor: null, has_more: false }); }); + it("rejects an invalid cursor with 422", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + + const res = await get( + "/extensions/v2/developers/dev-developer/history?cursor=not-a-cursor", + await authHeaders("mod-1") + ); + expect(res.status).toBe(422); + }); + it("orders entries newest-first and snapshots each write", async () => { await put( "/extensions/v2/developers/me", diff --git a/test/services/extensions/v2/ownership.test.ts b/test/services/extensions/v2/ownership.test.ts index 8f51abaa..98243b59 100644 --- a/test/services/extensions/v2/ownership.test.ts +++ b/test/services/extensions/v2/ownership.test.ts @@ -634,7 +634,7 @@ describe("Extensions API v2", () => { expect(pendingData.result[0].developer_name).toBe("Legacy Developer"); }); - it("keeps the pending queue to pending claims even with a status filter", async () => { + it("rejects a status filter that disagrees with the pending queue", async () => { await seedUnownedDeveloper("legacy-developer"); await post( "/extensions/v2/developers/legacy-developer/claim", @@ -643,12 +643,20 @@ describe("Extensions API v2", () => { ); await insertUser(db, { id: "mod-1", is_moderator: 1 }); - const res = await get( + // status only narrows scope=mine; on scope=pending anything but + // pending/all is a 422 rather than a silently ignored filter. + const rejected = await get( "/extensions/v2/developers/claims?scope=pending&status=approved", await authHeaders("mod-1") ); - expect(res.status).toBe(200); - await expect(res.json()).resolves.toMatchObject({ + expect(rejected.status).toBe(422); + + const pending = await get( + "/extensions/v2/developers/claims?scope=pending&status=pending", + await authHeaders("mod-1") + ); + expect(pending.status).toBe(200); + await expect(pending.json()).resolves.toMatchObject({ result: [{ status: "pending" }] }); }); From 0147a22e7d3dfd8c7191ea83f8041e221cd2b510 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Wed, 23 Sep 2026 22:16:57 +0100 Subject: [PATCH 2/6] Remove temporary RFC doc --- .../extensions/v2/RFC-0001-contract-tidy.md | 85 ------------------- 1 file changed, 85 deletions(-) delete mode 100644 src/services/extensions/v2/RFC-0001-contract-tidy.md diff --git a/src/services/extensions/v2/RFC-0001-contract-tidy.md b/src/services/extensions/v2/RFC-0001-contract-tidy.md deleted file mode 100644 index 5e8aa1c3..00000000 --- a/src/services/extensions/v2/RFC-0001-contract-tidy.md +++ /dev/null @@ -1,85 +0,0 @@ -# RFC 0001 — Extensions v2 contract tidy (breaking, still v2) - -- Status: **implemented** (API + site co-migrated; see contract.test.ts). -- Scope: contract-only. No D1 schema change in this RFC unless flagged `migration?`. -- Constraint: only caller is the `extensions` site; API + site land in one coordinated release. Base path stays `/extensions/v2`. No v3. -- Style: **action-oriented, normalized** — not REST-ified into generic `PATCH {status}` (see §0). - -## 0. Style decision (locked) - -Resource CRUD stays REST (`POST/GET /extensions`, `PUT /extensions/{id}`, `DELETE` withdraw, `PUT /developers/me`, `PATCH /users/me`). Workflow transitions stay explicit verbs (`POST …/approve|reject|delist|relist|claim|cancel|transfer|revoke|accept|reverify`). Rationale: each verb owns distinct auth, validation, atomicity, and side-effects (mail + `revalidateCatalogue()`); merging into one `PATCH` forces role-conditional bodies and a branched handler that is harder to document, generate clients for, and review. A unified `PATCH` was evaluated and rejected. - -## 1. Inventory (current, 29 operations) - -Extensions: - -- `GET /extensions?scope=public|mine|all` (+`type,developer_id|status,q,limit,cursor`) — cursor `{next_cursor,has_more}` -- `GET /extensions/{id}` — role-aware union (published vs `OwnedExtension`) -- `POST /extensions` → 201 pending — `ExtensionCreateSchema` -- `PUT /extensions/{id}` → 202 pending — `ExtensionUpdateSchema`, owner-only -- `DELETE /extensions/{id}` — withdraw unpublished only -- `GET /extensions/{id}/revisions` — owner-or-moderator history, cursor, newest-first -- `GET /revisions?status` — moderator queue, cursor, oldest-first -- `POST /extensions/{id}/revisions/{revisionId}/approve` (note optional) / `reject` (note required) — `?notify=false` opt-out, `{…, notified}` -- `POST /extensions/{id}/delist` (`{reason}` required) — no inverse (gap) -- `GET /moderation/counts` - -Developers / ownership / users: - -- `GET /developers?status=all|unapproved` — **offset** `{limit,offset,has_more}`, tag `Moderation` -- `GET /developers/me`, `PUT /developers/me`, `DELETE /developers/me`, `POST /developers/me/reverify?check_url`, `GET /developers/{id}` (role-aware) -- `POST /developers/{id}/claim`, `POST /developers/claims/{id}/cancel`, `GET /developers/claims?scope=mine|pending` — **offset**; `scope=pending` ignores `status` filter -- `POST /developers/claims/{id}/approve|reject`, `POST /developers/{id}/approve`, `GET /developers/{id}/history` — **offset** -- `POST /developers/{id}/transfer`, `POST /developers/{id}/transfer/revoke`, `POST /developers/transfers/accept {token}` -- `PUT /users/me/identity` (assertion scope only), `GET /users/me`, `PATCH /users/me`, `DELETE /users/me` - -Inconsistencies: two pagination envelopes; three `scope`/`status` vocabularies; `cancel`/`revoke` as `POST` (kept deliberately, see §3); tags mixed by role vs resource (`listDevelopers` under `Moderation`); `?notify` + `notified` copy-pasted per handler; four error mappers (`statusFromErrorCode`, `statusFromWriteErrorCode`, `statusFromOwnershipErrorCode`, `statusFromGithubErrorCode`); no `relist`; reservation rules in comments + migration `0020` only. - -## 2. Pagination — cursor everywhere - -- Retire offset envelope (`OffsetPaginationSchema`, `offsetPageFromQuery`, `offsetPaginationFrom`) for v2 lists. Single envelope `PaginationSchema {next_cursor, has_more}` + `limit (1-100, default 50)` + opaque `cursor`. -- Applies to: `GET /developers`, `GET /developers/claims`, `GET /developers/{id}/history`. Queue ordering preserved (admin lists: newest-first default unless queue semantics say oldest-first). -- Frontend replaces `fetchWholeList` offset walker (`extensions/src/lib/api/client.ts`) with cursor walker (same shape as `listMyExtensions`). -- `migration?` No. - -## 3. Verbs — canonical `POST /{resources}/{id}/{verb}` - -- Keep `POST` for all transitions, including `cancel` and `revoke`. `DELETE` means "row gone" (`DELETE /extensions/{id}` withdraw, `DELETE /developers/me`, `DELETE /users/me`) — `cancel`/`revoke` leave history rows, so `POST …/cancel|revoke` is correct, not an accident. -- No renames except additions below. `transfers/accept` stays `POST /developers/transfers/accept {token}` (token is the address, not an id — nesting under `{id}` would leak existence). -- **Add `POST /extensions/{id}/relist`** (moderator-only): inverse of `delist`, requires `{note?}` (optional, trimmed, max 2000), clears `delisted_at/delist_reason`, `revalidateCatalogue()`, same `?notify` + `notified` shape, 409 when not delisted / unpublished. `migration?` No. -- `migration?` No. - -## 4. Scope — one pattern - -- `?scope` selects projection; `?status` narrows within it; mismatched combos 422 (extend the rule already enforced in `routes/public-extensions.ts`). -- `GET /developers?scope=all|unapproved` (replaces `?status=`; `status` param removed). Default `all`. -- `GET /developers/claims?scope=mine|pending` kept; `status` narrows `mine` only, ignored-with-422 on `pending` (today silently ignored — tighten to 422). -- `GET /extensions?scope=public|mine|all` unchanged. -- `migration?` No. - -## 5. Notify + result envelope — one helper, same wire - -- Keep `?notify=false` query (checkbox-checked default sends). Do not move to body — avoids touching every form handler in `extensions/src/pages/account/admin/**`. -- Add a shared `NotifiedSchema` fragment for the `notified: boolean` field ("recipient resolved and send dispatched, delivery async") and use it in all seven moderation transitions in `routes/moderation.ts` + `routes/ownership.ts` instead of the hand-restated descriptions. -- `migration?` No. - -## 6. Errors + tags — one mapper, tags by resource - -- Collapse `routes/errors.ts` to `statusFromErrorCode` (reads: 404/500 + github 422/429/503) + `statusFromWriteErrorCode` (writes: 403/404/409/500). Delete `statusFromOwnershipErrorCode` (fold into write mapper). No per-handler ternaries beyond github codes. -- Tags by resource: `Extensions` (all `/extensions*`, `/revisions`, `/moderation/counts` moves to `Extensions`? keep `Moderation` only for `/moderation/counts` + queue reads — **decision:** `GET /revisions`, `GET /moderation/counts` stay `Moderation`; everything else by resource: `listDevelopers` moves `Moderation` → `Developers`; claim approve/reject move `Moderation` → `Developers`). Scalar grouping then matches paths. -- Document status-code rule: `201` created (claim, extension create), `202` accepted-pending (propose edit), else `200`. `422` = zod/contract failure via `defaultHook`; `409` = guarded-write no-op with diagnosed reason. -- `migration?` No. - -## 7. Path reservations + registration order — contract test - -- Reserve `me`, `claims`, `transfers` under `/developers`; `mine` needs no reservation (ordinary id now). Keep registration order: static before `/{id}` (`index.ts` comments become test assertions). -- Add `test/services/extensions/v2/contract.test.ts`: reserved ids unreachable as data ids (mirrors migration `0020` logic), static routes win over param routes, `?scope` misuse matrix 422s, pagination envelope shape per route. -- `migration?` No. - -## Annex A — Reviewer identity (bot, no special treatment) - -`FOSSBilling Bot` is an existing account row with `is_moderator=1`. No reserved id, no new verifier, no migration. Auto-worker mints standard 60s HS256 assertions (`sub=`, existing `bearerAssertionVerifier`) and calls canonical approve/reject. Machine-readable audit via note prefix `[auto policy=/ score=<0-1>] …` (fits existing `ReviewNote*` max 2000). Presentation maps bot `sub` → "FOSSBilling Bot (automatic)"; ops rule: never `DELETE /users/me` as the bot (avoids `display_name` nulling via `deleteAccount`). Shadow-mode logging (`would_approve`) precedes live auto-approve; rollout limited to readme-only diffs first (classifiable via `revision-diff`). - -## Annex B — Deferred follow-up: `moderator-correct` (preview, not in this epic) - -Built after this RFC lands, on canonical verbs: `POST /extensions/{id}/moderator-correct` (moderator-only, `ExtensionUpdateSchema.strict()` + `{correction_note: trim min(1) max(2000)}`, no `?notify`/mail, 404 unknown, 409 pending-exists/unpublished/delisted, single `batch()` inserting `status='approved'` row with `submitted_by=reviewer_id=moderator` + publishing + `published_revision_id`, `revalidateCatalogue()`). Frontend `account/admin/extensions/[id]/edit.astro` reusing `ExtensionForm` + preview, blocked-state when pending, `purgeCatalogue()` + flash (no-mail copy). From 809d7827650d53ac4afe8b892d74bab478330d20 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Wed, 23 Sep 2026 22:17:06 +0100 Subject: [PATCH 3/6] Drop RFC references from comments --- src/services/extensions/v2/schemas/common.ts | 5 ++--- test/services/extensions/v2/contract.test.ts | 7 +++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/services/extensions/v2/schemas/common.ts b/src/services/extensions/v2/schemas/common.ts index 74ddd768..c2367c56 100644 --- a/src/services/extensions/v2/schemas/common.ts +++ b/src/services/extensions/v2/schemas/common.ts @@ -114,9 +114,8 @@ export const NotifiedSchema = z ) .openapi({ example: true }); -// Cursor pagination for moderator/audit lists (RFC 0001): every v2 list -// now pages by opaque keyset cursor, matching GET /extensions and -// GET /revisions. `limit` bounds the window (default 50, max 100); +// Cursor pagination for moderator/audit lists: every v2 list pages by +// opaque keyset cursor, matching GET /extensions and GET /revisions. `limit` bounds the window (default 50, max 100); // `cursor` is the `next_cursor` of the previous page. An invalid cursor is // rejected with INVALID_CURSOR (422) rather than restarting pagination. // Callers that want everything page through with limit=100; the response diff --git a/test/services/extensions/v2/contract.test.ts b/test/services/extensions/v2/contract.test.ts index 7b15780a..00d76ffe 100644 --- a/test/services/extensions/v2/contract.test.ts +++ b/test/services/extensions/v2/contract.test.ts @@ -17,10 +17,9 @@ vi.mock("@octokit/request", async () => setupExtensionsV2Tests(); -// RFC 0001 contract invariants: static routes beat param routes, reserved -// ids stay unreachable as data, scope misuse 422s instead of silently -// returning the wrong projection, and every list answers the cursor -// envelope. +// Contract invariants: static routes beat param routes, reserved ids stay +// unreachable as data, scope misuse 422s instead of silently returning the +// wrong projection, and every list answers the cursor envelope. describe("Extensions API v2 contract", () => { it("serves static developer routes ahead of /developers/{id}", async () => { await put( From 5da8d631af93caa1cc4d5b5ec6facc8627bb8a63 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Wed, 23 Sep 2026 22:32:50 +0100 Subject: [PATCH 4/6] Address review comments on contract tidy --- .../extensions/v2/db/developer-claims.ts | 28 +++- .../extensions/v2/db/developer-profiles.ts | 28 +++- .../v2/routes/developer-profiles.ts | 4 +- src/services/extensions/v2/schemas/common.ts | 5 +- .../extensions/v2/schemas/developers.ts | 2 +- test/services/extensions/v2/contract.test.ts | 130 +++++++++++++++++- .../extensions/v2/moderation-notify.test.ts | 31 +++++ .../v2/moderation-revalidate.test.ts | 23 ++++ .../services/extensions/v2/moderation.test.ts | 71 ++++++++++ 9 files changed, 306 insertions(+), 16 deletions(-) diff --git a/src/services/extensions/v2/db/developer-claims.ts b/src/services/extensions/v2/db/developer-claims.ts index 4ac13faf..295efa16 100644 --- a/src/services/extensions/v2/db/developer-claims.ts +++ b/src/services/extensions/v2/db/developer-claims.ts @@ -304,7 +304,12 @@ export class DeveloperClaimsDatabase { > { const limit = page?.limit ?? 50; const decoded = page?.cursor ? decodeClaimCursor(page.cursor) : null; - if (page?.cursor && !decoded) { + // Scopes order oppositely (mine newest-first, pending oldest-first), + // so a cursor from one scope would seek from the wrong key boundary in + // the other: reject it rather than return a silently wrong page. (The + // status filter within scope=mine shares the same ordering, so it needs + // no tag.) + if (page?.cursor && (!decoded || decoded.s !== filters.scope)) { return { data: null, error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } @@ -387,7 +392,11 @@ export class DeveloperClaimsDatabase { hasMore, nextCursor: hasMore && last - ? encodeClaimCursor(last.claim.createdAt, String(last.rowid)) + ? encodeClaimCursor( + last.claim.createdAt, + String(last.rowid), + filters.scope + ) : null }, error: null @@ -636,16 +645,25 @@ export class DeveloperClaimsDatabase { interface ClaimCursor { k1: string; k2: string; + s: "mine" | "pending"; } -function encodeClaimCursor(k1: string, k2: string): string { - return encode({ k1, k2 }); +function encodeClaimCursor( + k1: string, + k2: string, + s: "mine" | "pending" +): string { + return encode({ k1, k2, s }); } function isClaimCursor( parsed: Record ): parsed is ClaimCursor & Record { - return typeof parsed.k1 === "string" && typeof parsed.k2 === "string"; + return ( + typeof parsed.k1 === "string" && + typeof parsed.k2 === "string" && + (parsed.s === "mine" || parsed.s === "pending") + ); } function decodeClaimCursor(cursor: string): ClaimCursor | null { diff --git a/src/services/extensions/v2/db/developer-profiles.ts b/src/services/extensions/v2/db/developer-profiles.ts index 1e87bf01..7c4deadb 100644 --- a/src/services/extensions/v2/db/developer-profiles.ts +++ b/src/services/extensions/v2/db/developer-profiles.ts @@ -828,7 +828,9 @@ export class DeveloperProfilesDatabase { // CURRENT_TIMESTAMP has only second resolution, so two writes in the same // second tie on changed_at; rowid (insertion order) breaks the tie so // "newest first" is never ambiguous, exactly as the offset implementation - // did. The opaque cursor carries rowid. + // did. The opaque cursor carries rowid and is bound to this developer: a + // cursor from another developer (or another list) is rejected rather than + // seeking from the wrong key boundary. async listHistory( developerId: string, page?: { limit?: number; cursor?: string } @@ -841,7 +843,12 @@ export class DeveloperProfilesDatabase { > { const limit = page?.limit ?? 50; const decoded = page?.cursor ? decodeHistoryCursor(page.cursor) : null; - if (page?.cursor && !decoded) { + // Case-insensitive like the id matching everywhere else: ids are + // lowercase slugs by schema, but adopted rows predate that. + if ( + page?.cursor && + (!decoded || decoded.d.toLowerCase() !== developerId.toLowerCase()) + ) { return { data: null, error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } @@ -910,7 +917,11 @@ export class DeveloperProfilesDatabase { hasMore, nextCursor: hasMore && last - ? encodeHistoryCursor(last.changedAt, String(last.rowid)) + ? encodeHistoryCursor( + last.changedAt, + String(last.rowid), + developerId + ) : null }, error: null @@ -1190,16 +1201,21 @@ function decodeDeveloperCursor(cursor: string): DeveloperListCursor | null { interface HistoryCursor { k1: string; k2: string; + d: string; } -function encodeHistoryCursor(k1: string, k2: string): string { - return encode({ k1, k2 }); +function encodeHistoryCursor(k1: string, k2: string, d: string): string { + return encode({ k1, k2, d }); } function isHistoryCursor( parsed: Record ): parsed is HistoryCursor & Record { - return typeof parsed.k1 === "string" && typeof parsed.k2 === "string"; + return ( + typeof parsed.k1 === "string" && + typeof parsed.k2 === "string" && + typeof parsed.d === "string" + ); } function decodeHistoryCursor(cursor: string): HistoryCursor | null { diff --git a/src/services/extensions/v2/routes/developer-profiles.ts b/src/services/extensions/v2/routes/developer-profiles.ts index f3243196..b41bc2ce 100644 --- a/src/services/extensions/v2/routes/developer-profiles.ts +++ b/src/services/extensions/v2/routes/developer-profiles.ts @@ -60,7 +60,9 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { ...ActiveAccountRequiredResponse, description: "The account is inactive or the caller is not a moderator" }, - 422: errorResponse("scope, limit, or cursor query failed validation"), + 422: errorResponse( + "scope, status, limit, or cursor query failed validation" + ), 500: errorResponse("Database error") } }); diff --git a/src/services/extensions/v2/schemas/common.ts b/src/services/extensions/v2/schemas/common.ts index c2367c56..fdbd5fc2 100644 --- a/src/services/extensions/v2/schemas/common.ts +++ b/src/services/extensions/v2/schemas/common.ts @@ -120,7 +120,10 @@ export const NotifiedSchema = z // rejected with INVALID_CURSOR (422) rather than restarting pagination. // Callers that want everything page through with limit=100; the response // envelope (PaginationSchema) reports has_more either way. -export const CursorPaginationQuerySchema = z.object({ +// Strict: unknown query params (notably the retired `offset`) are rejected +// with 422 rather than silently stripped, so a caller paginating the old +// way gets an error instead of page one on repeat. +export const CursorPaginationQuerySchema = z.strictObject({ limit: z.coerce .number() .int() diff --git a/src/services/extensions/v2/schemas/developers.ts b/src/services/extensions/v2/schemas/developers.ts index c20f88ec..df8e2819 100644 --- a/src/services/extensions/v2/schemas/developers.ts +++ b/src/services/extensions/v2/schemas/developers.ts @@ -162,7 +162,7 @@ export const DeveloperListQuerySchema = CursorPaginationQuerySchema.extend({ .enum(["all", "unapproved"]) .optional() .openapi({ - param: { name: "status", in: "query" }, + param: { name: "status", in: "query", deprecated: true }, description: "Deprecated alias for scope." }) }); diff --git a/test/services/extensions/v2/contract.test.ts b/test/services/extensions/v2/contract.test.ts index 00d76ffe..098d2b52 100644 --- a/test/services/extensions/v2/contract.test.ts +++ b/test/services/extensions/v2/contract.test.ts @@ -4,11 +4,13 @@ import { db, authHeaders, get, + post, put, sampleDeveloper, - seedDeveloper + seedDeveloper, + seedUnownedDeveloper } from "./harness"; -import { insertUser } from "./db-fixtures"; +import { insertDeveloper, insertUser } from "./db-fixtures"; // Hoisted so no v2 suite can make a real GitHub call. vi.mock("@octokit/request", async () => @@ -85,6 +87,130 @@ describe("Extensions API v2 contract", () => { } }); + it("walks past the first page when rows exceed the limit", async () => { + await insertDeveloper(db, { + id: "aaa-developer", + type: "user", + name: "Aaa Developer", + url: null, + owner_user_id: "user-1" + }); + await insertDeveloper(db, { + id: "zzz-developer", + type: "user", + name: "Zzz Developer", + url: null, + owner_user_id: "user-2" + }); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + const first = await get("/extensions/v2/developers?limit=1", mod); + expect(first.status).toBe(200); + const firstBody = (await first.json()) as { + result: Array<{ id: string }>; + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(firstBody.result.map((d) => d.id)).toEqual(["aaa-developer"]); + expect(firstBody.pagination.has_more).toBe(true); + expect(firstBody.pagination.next_cursor).toBeTruthy(); + + const second = await get( + `/extensions/v2/developers?limit=1&cursor=${encodeURIComponent(firstBody.pagination.next_cursor as string)}`, + mod + ); + expect(second.status).toBe(200); + const secondBody = (await second.json()) as { + result: Array<{ id: string }>; + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(secondBody.result.map((d) => d.id)).toEqual(["zzz-developer"]); + expect(secondBody.pagination).toEqual({ + next_cursor: null, + has_more: false + }); + }); + + it("rejects a legacy offset parameter instead of ignoring it", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + for (const path of [ + "/extensions/v2/developers?limit=10&offset=10", + "/extensions/v2/developers/claims?scope=pending&limit=10&offset=10" + ]) { + const res = await get(path, mod); + expect(res.status).toBe(422); + } + }); + + it("rejects a claims cursor reused across scopes", async () => { + await seedUnownedDeveloper("legacy-a"); + await seedUnownedDeveloper("legacy-b"); + await post( + "/extensions/v2/developers/legacy-a/claim", + await authHeaders("user-1"), + {} + ); + await post( + "/extensions/v2/developers/legacy-b/claim", + await authHeaders("user-1"), + {} + ); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + const mine = await get( + "/extensions/v2/developers/claims?scope=mine&limit=1", + await authHeaders("user-1") + ); + const mineBody = (await mine.json()) as { + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(mineBody.pagination.has_more).toBe(true); + + const crossed = await get( + `/extensions/v2/developers/claims?scope=pending&cursor=${encodeURIComponent(mineBody.pagination.next_cursor as string)}`, + mod + ); + expect(crossed.status).toBe(422); + }); + + it("rejects a history cursor from another developer", async () => { + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper({ id: "dev-a", name: "Dev A" }) + ); + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper({ id: "dev-a", name: "Dev A Edited" }) + ); + await put( + "/extensions/v2/developers/me", + await authHeaders("user-2"), + sampleDeveloper({ id: "dev-b", name: "Dev B" }) + ); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + const first = await get( + "/extensions/v2/developers/dev-a/history?limit=1", + mod + ); + const firstBody = (await first.json()) as { + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(firstBody.pagination.has_more).toBe(true); + + const crossed = await get( + `/extensions/v2/developers/dev-b/history?cursor=${encodeURIComponent(firstBody.pagination.next_cursor as string)}`, + mod + ); + expect(crossed.status).toBe(422); + }); + it("rejects invalid cursors on every list", async () => { await seedDeveloper("new-developer", "user-1"); await insertUser(db, { id: "mod-1", is_moderator: 1 }); diff --git a/test/services/extensions/v2/moderation-notify.test.ts b/test/services/extensions/v2/moderation-notify.test.ts index 1acaabf2..aaa69828 100644 --- a/test/services/extensions/v2/moderation-notify.test.ts +++ b/test/services/extensions/v2/moderation-notify.test.ts @@ -114,6 +114,37 @@ describe("moderation notification emails", () => { expect(body.body).toContain("Upstream source removed"); }); + it("emails the author on relist with the moderator note", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + await seedLiveExtension(); + await post( + "/extensions/v2/extensions/live-ext/delist?notify=false", + await authHeaders("mod-1"), + { reason: "Upstream source removed" } + ); + setEmailEnv(); + const calls = stubSmtpApi(); + + const res = await post( + "/extensions/v2/extensions/live-ext/relist", + await authHeaders("mod-1"), + { review_note: "Upstream is back" } + ); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ + result: { id: "live-ext", status: "relisted", notified: true } + }); + + expect(calls).toHaveLength(1); + const body = JSON.parse(String(calls[0].init.body)); + expect(body).toMatchObject({ + to: "author@example.com", + from: "extensions@fossbilling.org" + }); + expect(body.subject).toContain("live-ext"); + expect(body.body).toContain("Upstream is back"); + }); + // Moderation routes pass the raw URL param, and ids are matched // case-insensitively everywhere else - the recipient lookup must not // regress to a case-sensitive match or mixed-case ids silently skip diff --git a/test/services/extensions/v2/moderation-revalidate.test.ts b/test/services/extensions/v2/moderation-revalidate.test.ts index 41d748aa..3973fed1 100644 --- a/test/services/extensions/v2/moderation-revalidate.test.ts +++ b/test/services/extensions/v2/moderation-revalidate.test.ts @@ -103,6 +103,29 @@ describe("CDN cache revalidation on catalogue mutations", () => { }); }); + it("purges the catalogue tags after a successful relist", async () => { + await seedModAndExtension(); + expect((await delist("mod-1")).status).toBe(200); + const fetcher = stubFrontend(); + + const res = await post( + "/extensions/v2/extensions/live-ext/relist?notify=false", + await authHeaders("mod-1"), + {} + ); + expect(res.status).toBe(200); + + expect(fetchCalls(fetcher)).toHaveLength(1); + const [url, init] = fetchCalls(fetcher)[0]; + expect(String(url)).toBe( + "https://extensions.fossbilling.org/api/revalidate" + ); + expect(init.method).toBe("POST"); + expect(JSON.parse(String(init.body))).toEqual({ + tags: ["catalogue", "developers"] + }); + }); + it("purges after a revision approval", async () => { await seedModAndExtension(); const created = await post( diff --git a/test/services/extensions/v2/moderation.test.ts b/test/services/extensions/v2/moderation.test.ts index 6c536dd3..c375088b 100644 --- a/test/services/extensions/v2/moderation.test.ts +++ b/test/services/extensions/v2/moderation.test.ts @@ -1081,6 +1081,35 @@ describe("Extensions API v2", () => { expect(res.status).toBe(409); }); + // A body-less call sends no Content-Type in production (browser fetch + // and the generated client omit it when there is no body), and the + // validator defaults that to {} — the same as the approve route. (The + // harness always sets Content-Type, so it is stripped here to exercise + // the production shape.) + it("accepts a body-less relist (note is optional)", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + await seedDeveloper("new-developer", "user-1"); + await insertExtension(db, { + id: "live-ext", + developer_id: "new-developer" + }); + const mod = await authHeaders("mod-1"); + await post("/extensions/v2/extensions/live-ext/delist", mod, { + reason: "Upstream source removed" + }); + + const { "Content-Type": _dropped, ...noContentType } = mod; + expect(_dropped).toBe("application/json"); + const res = await post( + "/extensions/v2/extensions/live-ext/relist", + noContentType + ); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ + result: { id: "live-ext", status: "relisted", notified: false } + }); + }); + it("restores a delisted extension to the public catalogue", async () => { await insertUser(db, { id: "mod-1", is_moderator: 1 }); await seedDeveloper("new-developer", "user-1"); @@ -1450,6 +1479,48 @@ describe("Extensions API v2", () => { }); }); + it("bounds history pages to the requested limit", async () => { + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper({ name: "Original Name" }) + ); + await put( + "/extensions/v2/developers/me", + await authHeaders("user-1"), + sampleDeveloper({ name: "Edited Name" }) + ); + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + const mod = await authHeaders("mod-1"); + + const first = await get( + "/extensions/v2/developers/dev-developer/history?limit=1", + mod + ); + expect(first.status).toBe(200); + const firstBody = (await first.json()) as { + result: Array<{ name: string }>; + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(firstBody.result.map((e) => e.name)).toEqual(["Edited Name"]); + expect(firstBody.pagination.has_more).toBe(true); + expect(firstBody.pagination.next_cursor).toBeTruthy(); + + const second = await get( + `/extensions/v2/developers/dev-developer/history?limit=1&cursor=${encodeURIComponent(firstBody.pagination.next_cursor as string)}`, + mod + ); + const secondBody = (await second.json()) as { + result: Array<{ name: string }>; + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(secondBody.result.map((e) => e.name)).toEqual(["Original Name"]); + expect(secondBody.pagination).toEqual({ + next_cursor: null, + has_more: false + }); + }); + it("rejects an invalid cursor with 422", async () => { await insertUser(db, { id: "mod-1", is_moderator: 1 }); From cda2750ccbb675aaad2c4bed489b3c5d909325dd Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Wed, 23 Sep 2026 22:42:39 +0100 Subject: [PATCH 5/6] Address second review round on contract tidy --- .../extensions/v2/db/developer-claims.ts | 26 +++++++--- src/services/extensions/v2/db/extensions.ts | 41 ++++++++++++--- .../extensions/v2/routes/moderation.ts | 2 +- .../extensions/v2/schemas/ownership.ts | 2 +- test/services/extensions/v2/contract.test.ts | 36 +++++++++++++ .../services/extensions/v2/moderation.test.ts | 51 +++++++++++++++++++ 6 files changed, 143 insertions(+), 15 deletions(-) diff --git a/src/services/extensions/v2/db/developer-claims.ts b/src/services/extensions/v2/db/developer-claims.ts index 295efa16..c4dce3f5 100644 --- a/src/services/extensions/v2/db/developer-claims.ts +++ b/src/services/extensions/v2/db/developer-claims.ts @@ -308,8 +308,16 @@ export class DeveloperClaimsDatabase { // so a cursor from one scope would seek from the wrong key boundary in // the other: reject it rather than return a silently wrong page. (The // status filter within scope=mine shares the same ordering, so it needs - // no tag.) - if (page?.cursor && (!decoded || decoded.s !== filters.scope)) { + // no tag.) Mine cursors are additionally bound to the claimant, like + // history cursors to their developer: claimantId is the primary filter + // of that projection, and another user's cursor would otherwise skip + // this caller's newest claims. + if ( + page?.cursor && + (!decoded || + decoded.s !== filters.scope || + (filters.scope === "mine" && decoded.c !== filters.claimantId)) + ) { return { data: null, error: { message: "Invalid pagination cursor", code: "INVALID_CURSOR" } @@ -395,7 +403,8 @@ export class DeveloperClaimsDatabase { ? encodeClaimCursor( last.claim.createdAt, String(last.rowid), - filters.scope + filters.scope, + filters.scope === "mine" ? filters.claimantId : undefined ) : null }, @@ -646,14 +655,18 @@ interface ClaimCursor { k1: string; k2: string; s: "mine" | "pending"; + c?: string; } function encodeClaimCursor( k1: string, k2: string, - s: "mine" | "pending" + s: "mine" | "pending", + claimantId?: string ): string { - return encode({ k1, k2, s }); + return encode( + claimantId === undefined ? { k1, k2, s } : { k1, k2, s, c: claimantId } + ); } function isClaimCursor( @@ -662,7 +675,8 @@ function isClaimCursor( return ( typeof parsed.k1 === "string" && typeof parsed.k2 === "string" && - (parsed.s === "mine" || parsed.s === "pending") + (parsed.s === "mine" || parsed.s === "pending") && + (parsed.c === undefined || typeof parsed.c === "string") ); } diff --git a/src/services/extensions/v2/db/extensions.ts b/src/services/extensions/v2/db/extensions.ts index a80b000c..1e06181e 100644 --- a/src/services/extensions/v2/db/extensions.ts +++ b/src/services/extensions/v2/db/extensions.ts @@ -768,14 +768,18 @@ export class ExtensionsDatabase { // Inverse of delist(): restores a delisted-but-published extension to the // catalogue. Content and history are untouched; only the delist markers // are cleared. `AND delisted_at IS NOT NULL` makes this a single atomic - // check-and-set mirroring delist(). + // check-and-set mirroring delist(). The actor guard re-checks moderator + // status inside the statement (not just activity): requireModerator() + // can only reject before the write, and a role revoked in between must + // still fail the write itself. RETURNING hands back the stored canonical + // id, which can differ in case from the path param. async relist( id: string, moderatorId: string ): Promise> { - let result; + let rows; try { - result = await this.db + rows = await this.db .update(extensions) .set({ delistedAt: null, @@ -789,19 +793,23 @@ export class ExtensionsDatabase { isNotNull(extensions.delistedAt), sql`EXISTS ( SELECT 1 FROM ${users} - WHERE ${users.id} = ${moderatorId} AND ${users.deletedAt} IS NULL + WHERE ${users.id} = ${moderatorId} + AND ${users.deletedAt} IS NULL + AND ${users.isModerator} = 1 )` ) - ); + ) + .returning({ id: extensions.id }); } catch (error) { return databaseError("relist", error); } - if (!result.meta?.changes) { + const [row] = rows; + if (!row) { return this.relistBlockedError(id, moderatorId); } - return { data: { id }, error: null }; + return { data: { id: row.id }, error: null }; } private async relistBlockedError( @@ -811,6 +819,25 @@ export class ExtensionsDatabase { const inactive = await inactiveActorError(this.db, moderatorId); if (inactive) return { data: null, error: inactive }; + // Matches the in-statement guard above: a moderator demoted after + // requireModerator() ran fails the write, and must be told so (403) + // rather than misreported as a state conflict. + let actor: { isModerator: number | null } | undefined; + try { + [actor] = await this.db + .select({ isModerator: users.isModerator }) + .from(users) + .where(eq(users.id, moderatorId)); + } catch (error) { + return databaseError("relist", error); + } + if (!actor || actor.isModerator !== 1) { + return { + data: null, + error: { message: "Moderator access required", code: "FORBIDDEN" } + }; + } + let existing: { publishedAt: string | null; delistedAt: string | null } | undefined; try { diff --git a/src/services/extensions/v2/routes/moderation.ts b/src/services/extensions/v2/routes/moderation.ts index 624d6d3a..6ee36033 100644 --- a/src/services/extensions/v2/routes/moderation.ts +++ b/src/services/extensions/v2/routes/moderation.ts @@ -340,7 +340,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { extDb, { kind: "extension-relisted", - extensionId: id, + extensionId: data.id, reason: review_note?.trim() || undefined }, (p) => c.executionCtx.waitUntil(p) diff --git a/src/services/extensions/v2/schemas/ownership.ts b/src/services/extensions/v2/schemas/ownership.ts index 1ec453a8..0d49efcb 100644 --- a/src/services/extensions/v2/schemas/ownership.ts +++ b/src/services/extensions/v2/schemas/ownership.ts @@ -79,6 +79,6 @@ export const UnifiedClaimsQuerySchema = CursorPaginationQuerySchema.extend({ .openapi({ param: { name: "status", in: "query" }, description: - "Filter claims by status (default: all). Only valid with scope=mine." + "Filter claims by status (default: all). With scope=mine, narrows claims; with scope=pending, only all or pending is allowed." }) }); diff --git a/test/services/extensions/v2/contract.test.ts b/test/services/extensions/v2/contract.test.ts index 098d2b52..b031b82c 100644 --- a/test/services/extensions/v2/contract.test.ts +++ b/test/services/extensions/v2/contract.test.ts @@ -176,6 +176,42 @@ describe("Extensions API v2 contract", () => { expect(crossed.status).toBe(422); }); + it("rejects a mine cursor reused by another claimant", async () => { + await seedUnownedDeveloper("legacy-a"); + await seedUnownedDeveloper("legacy-b"); + await seedUnownedDeveloper("legacy-c"); + await post( + "/extensions/v2/developers/legacy-a/claim", + await authHeaders("user-1"), + {} + ); + await post( + "/extensions/v2/developers/legacy-b/claim", + await authHeaders("user-1"), + {} + ); + await post( + "/extensions/v2/developers/legacy-c/claim", + await authHeaders("user-2"), + {} + ); + + const mine = await get( + "/extensions/v2/developers/claims?scope=mine&limit=1", + await authHeaders("user-1") + ); + const mineBody = (await mine.json()) as { + pagination: { next_cursor: string | null; has_more: boolean }; + }; + expect(mineBody.pagination.has_more).toBe(true); + + const crossed = await get( + `/extensions/v2/developers/claims?scope=mine&cursor=${encodeURIComponent(mineBody.pagination.next_cursor as string)}`, + await authHeaders("user-2") + ); + expect(crossed.status).toBe(422); + }); + it("rejects a history cursor from another developer", async () => { await put( "/extensions/v2/developers/me", diff --git a/test/services/extensions/v2/moderation.test.ts b/test/services/extensions/v2/moderation.test.ts index c375088b..18c8ef76 100644 --- a/test/services/extensions/v2/moderation.test.ts +++ b/test/services/extensions/v2/moderation.test.ts @@ -1141,6 +1141,57 @@ describe("Extensions API v2", () => { (await get("/extensions/v2/extensions", {})).json() ).resolves.toMatchObject({ result: [{ id: "live-ext" }] }); }); + + it("answers with the stored canonical id for a mixed-case path", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + await seedDeveloper("new-developer", "user-1"); + await insertExtension(db, { + id: "LIVE-ext", + developer_id: "new-developer" + }); + const mod = await authHeaders("mod-1"); + await post("/extensions/v2/extensions/live-ext/delist", mod, { + reason: "Upstream source removed" + }); + + const res = await post( + "/extensions/v2/extensions/Live-EXT/relist", + mod, + {} + ); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ + result: { id: "LIVE-ext", status: "relisted", notified: false } + }); + }); + + it("403s for a moderator demoted after authenticating", async () => { + await insertUser(db, { id: "mod-1", is_moderator: 1 }); + await seedDeveloper("new-developer", "user-1"); + await insertExtension(db, { + id: "live-ext", + developer_id: "new-developer" + }); + const mod = await authHeaders("mod-1"); + await post("/extensions/v2/extensions/live-ext/delist", mod, { + reason: "Upstream source removed" + }); + + await db + .prepare("UPDATE users SET is_moderator = 0 WHERE id = ?") + .bind("mod-1") + .run(); + + const res = await post( + "/extensions/v2/extensions/live-ext/relist", + mod, + {} + ); + expect(res.status).toBe(403); + expect(await getExtension(db, "live-ext")).toMatchObject({ + delisted_at: expect.any(String) + }); + }); }); describe("developer moderation", () => { From 5f3404a51b4508b9c400d68efd69202a20d37b06 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Wed, 23 Sep 2026 22:53:51 +0100 Subject: [PATCH 6/6] Use moderatorAccess in relist blocked-error path --- src/services/extensions/v2/db/extensions.ts | 37 +++++++++++++-------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/services/extensions/v2/db/extensions.ts b/src/services/extensions/v2/db/extensions.ts index 1e06181e..3577d2e4 100644 --- a/src/services/extensions/v2/db/extensions.ts +++ b/src/services/extensions/v2/db/extensions.ts @@ -6,6 +6,7 @@ import { sortReleasesDescending } from "../../../../lib/releases"; import { parseJSON } from "../../../../lib/json"; import { extensions, extensionRevisions, developers, users } from "./schema"; import { databaseError, inactiveActorError } from "./errors"; +import { UsersDatabase } from "./users"; import { toD1Statement } from "./batch"; import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; import { @@ -816,22 +817,32 @@ export class ExtensionsDatabase { id: string, moderatorId: string ): Promise> { - const inactive = await inactiveActorError(this.db, moderatorId); - if (inactive) return { data: null, error: inactive }; - - // Matches the in-statement guard above: a moderator demoted after + // One row answers both halves of the actor check, matching the + // in-statement guard above: a moderator deactivated or demoted after // requireModerator() ran fails the write, and must be told so (403) // rather than misreported as a state conflict. - let actor: { isModerator: number | null } | undefined; - try { - [actor] = await this.db - .select({ isModerator: users.isModerator }) - .from(users) - .where(eq(users.id, moderatorId)); - } catch (error) { - return databaseError("relist", error); + const access = await new UsersDatabase(this.db).moderatorAccess( + moderatorId + ); + if (access.error || !access.data) { + return { + data: null, + error: access.error ?? { + message: "Active account required", + code: "ACCOUNT_INACTIVE" + } + }; + } + if (!access.data.active) { + return { + data: null, + error: { + message: "Active account required", + code: "ACCOUNT_INACTIVE" + } + }; } - if (!actor || actor.isModerator !== 1) { + if (!access.data.moderator) { return { data: null, error: { message: "Moderator access required", code: "FORBIDDEN" }