diff --git a/package-lock.json b/package-lock.json index c5edc668..f0f54545 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,10 +29,8 @@ "drizzle-kit": "0.31.11", "eslint": "10.12.0", "eslint-config-prettier": "10.1.8", - "eslint-plugin-jest": "29.16.6", "jiti": "2.7.0", "prettier": "3.9.9", - "tsx": "4.23.15", "typescript": "6.0.3", "typescript-eslint": "8.71.1", "vitest": "4.1.11", @@ -4327,36 +4325,6 @@ "eslint": ">=7.0.0" } }, - "node_modules/eslint-plugin-jest": { - "version": "29.16.6", - "resolved": "https://registry.npmjs.org/eslint-plugin-jest/-/eslint-plugin-jest-29.16.6.tgz", - "integrity": "sha512-q8TVr0rlNvUD0XnafGWkwtPeX+tTl2llP86EDl3sJtwWrQA/JT9SIu2hLLZbhhX6fT5SDE4O0gIKyZUujKnkYA==", - "dev": true, - "license": "MIT", - "dependencies": { - "@typescript-eslint/utils": "^8.0.0" - }, - "engines": { - "node": "^20.12.0 || ^22.0.0 || >=24.0.0" - }, - "peerDependencies": { - "@typescript-eslint/eslint-plugin": "^8.0.0", - "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", - "jest": "*", - "typescript": ">=4.8.4 <8.0.0" - }, - "peerDependenciesMeta": { - "@typescript-eslint/eslint-plugin": { - "optional": true - }, - "jest": { - "optional": true - }, - "typescript": { - "optional": true - } - } - }, "node_modules/eslint-scope": { "version": "9.1.2", "resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-9.1.2.tgz", diff --git a/package.json b/package.json index b72788c3..e35067be 100644 --- a/package.json +++ b/package.json @@ -47,10 +47,8 @@ "drizzle-kit": "0.31.11", "eslint": "10.12.0", "eslint-config-prettier": "10.1.8", - "eslint-plugin-jest": "29.16.6", "jiti": "2.7.0", "prettier": "3.9.9", - "tsx": "4.23.15", "typescript": "6.0.3", "typescript-eslint": "8.71.1", "vitest": "4.1.11", diff --git a/src/app/index.ts b/src/app/index.ts index 5dd5e39e..e03fddd5 100644 --- a/src/app/index.ts +++ b/src/app/index.ts @@ -89,4 +89,7 @@ export default { } }; -export { PreviewGitHubBudget } from "../lib/adapters/cloudflare/preview-github-budget"; +// wrangler's main module must export the Durable Object classes bound in +// wrangler.jsonc; the implementation lives with the previews service, which +// owns its rate-limit policy. +export { PreviewGitHubBudget } from "../services/previews/v1/budget"; diff --git a/src/lib/adapters/cloudflare/environment.ts b/src/lib/adapters/cloudflare/environment.ts index fb878f7f..63f9fdab 100644 --- a/src/lib/adapters/cloudflare/environment.ts +++ b/src/lib/adapters/cloudflare/environment.ts @@ -7,8 +7,4 @@ export class CloudflareEnvironmentAdapter implements IEnvironment { const value = this.env[key]; return typeof value === "string" ? value : undefined; } - - has(key: string): boolean { - return key in this.env && typeof this.env[key] === "string"; - } } diff --git a/src/lib/adapters/node/environment.ts b/src/lib/adapters/node/environment.ts index 9a9aa16f..2a3be2da 100644 --- a/src/lib/adapters/node/environment.ts +++ b/src/lib/adapters/node/environment.ts @@ -4,8 +4,4 @@ export class NodeEnvironmentAdapter implements IEnvironment { get(key: string): string | undefined { return process.env[key]; } - - has(key: string): boolean { - return key in process.env; - } } diff --git a/src/lib/adapters/node/index.ts b/src/lib/adapters/node/index.ts index 9039f8cd..f685f0c2 100644 --- a/src/lib/adapters/node/index.ts +++ b/src/lib/adapters/node/index.ts @@ -3,9 +3,11 @@ import { createMemoryCache, createFileCache } from "./cache"; import { NodeEnvironmentAdapter } from "./environment"; // No database entry here - Drizzle only wraps the real D1 bindings (see -// src/lib/db.ts); the Node path exists solely to test the cache/environment -// adapters outside Workers, so it never had a real production consumer for -// a database binding (only src/app/index.ts's createCloudflareBindings does). +// src/lib/db.ts). The Node path is the standing proof that the +// IPlatformBindings seam (KV-shaped caches + string environment) is +// implementable off-Cloudflare; handlers still reach for platform services +// (D1, R2, Durable Objects, typed env) directly, so a full port needs +// service-level changes too. It has no production consumer today by design. export function createNodeBindings(cacheDbPath?: string): IPlatformBindings { const cacheKv = cacheDbPath ? createFileCache(`${normalizePath(cacheDbPath)}.kv`) diff --git a/src/lib/cache.ts b/src/lib/cache.ts index 19a83307..bc4758d7 100644 --- a/src/lib/cache.ts +++ b/src/lib/cache.ts @@ -1,7 +1,7 @@ import type { Context, MiddlewareHandler } from "hono"; import { cache } from "hono/cache"; -export function normalizePublicCacheKey(url: string): string { +function normalizePublicCacheKey(url: string): string { const cacheUrl = new URL(url); cacheUrl.search = ""; cacheUrl.hash = ""; diff --git a/src/lib/github-errors.ts b/src/lib/github-errors.ts index 3ce4c73a..f92e8139 100644 --- a/src/lib/github-errors.ts +++ b/src/lib/github-errors.ts @@ -1,18 +1,9 @@ -export enum ErrorPriority { - CRITICAL = 0, - HIGH = 1, - MEDIUM = 2, - LOW = 3 -} - export class GitHubError extends Error { constructor( message: string, public readonly httpStatus?: number, public readonly errorCode?: string, - public readonly priority: ErrorPriority = ErrorPriority.MEDIUM, - public readonly url?: string, - public readonly details?: Record + public readonly url?: string ) { super(message); this.name = this.constructor.name; @@ -21,38 +12,31 @@ export class GitHubError extends Error { export class AuthError extends GitHubError { constructor(message: string, httpStatus: number = 401, url?: string) { - super(message, httpStatus, "auth_error", ErrorPriority.CRITICAL, url); + super(message, httpStatus, "auth_error", url); } } export class RateLimitError extends GitHubError { constructor(message: string, httpStatus: number = 403, url?: string) { - super(message, httpStatus, "rate_limit_error", ErrorPriority.CRITICAL, url); + super(message, httpStatus, "rate_limit_error", url); } } export class NetworkError extends GitHubError { constructor(message: string, url?: string) { - super(message, undefined, "network_error", ErrorPriority.HIGH, url); + super(message, undefined, "network_error", url); } } export class NotFoundError extends GitHubError { constructor(message: string, httpStatus: number = 404, url?: string) { - super(message, httpStatus, "not_found_error", ErrorPriority.MEDIUM, url); + super(message, httpStatus, "not_found_error", url); } } export class ValidationError extends GitHubError { - constructor(message: string, details?: Record) { - super( - message, - undefined, - "validation_error", - ErrorPriority.LOW, - undefined, - details - ); + constructor(message: string) { + super(message, undefined, "validation_error"); } } @@ -130,28 +114,8 @@ export function classifyGitHubError(error: unknown, url?: string): GitHubError { } if (errorMessage.toLowerCase().includes("json")) { - return new ValidationError("Invalid JSON response from GitHub API", { - originalMessage: errorMessage - }); - } - - return new GitHubError( - errorMessage, - httpStatus, - "unknown_error", - ErrorPriority.HIGH, - url - ); -} - -export function getMostCriticalError( - errors: GitHubError[] -): GitHubError | null { - if (errors.length === 0) { - return null; + return new ValidationError("Invalid JSON response from GitHub API"); } - return errors.reduce((mostCritical, current) => - current.priority < mostCritical.priority ? current : mostCritical - ); + return new GitHubError(errorMessage, httpStatus, "unknown_error", url); } diff --git a/src/lib/hash.ts b/src/lib/hash.ts new file mode 100644 index 00000000..0b238430 --- /dev/null +++ b/src/lib/hash.ts @@ -0,0 +1,10 @@ +// SHA-256 as lowercase hex - used for cache/DO key derivation, transfer +// token hashing, and content-digest columns. +export async function sha256Hex(input: string | BufferSource): Promise { + const data = + typeof input === "string" ? new TextEncoder().encode(input) : input; + const digest = await crypto.subtle.digest("SHA-256", data); + return [...new Uint8Array(digest)] + .map((b) => b.toString(16).padStart(2, "0")) + .join(""); +} diff --git a/src/lib/interfaces.ts b/src/lib/interfaces.ts index 43b407b3..dfdfc75d 100644 --- a/src/lib/interfaces.ts +++ b/src/lib/interfaces.ts @@ -19,7 +19,6 @@ export interface ICache { export interface IEnvironment { get(key: string): string | undefined; - has(key: string): boolean; } export interface IPlatformBindings { diff --git a/src/lib/logger.ts b/src/lib/logger.ts index 7e28b7ae..f04ef156 100644 --- a/src/lib/logger.ts +++ b/src/lib/logger.ts @@ -4,14 +4,6 @@ export enum LogLevel { INFO = "info" } -interface LogEntry { - timestamp: string; - service: string; - level: LogLevel; - message: string; - context?: Record; -} - function redactSensitiveData(data: unknown): unknown { if (typeof data === "string") { return data.replace(/Bearer\s+[A-Za-z0-9\-_]+/g, "Bearer [REDACTED]"); @@ -41,36 +33,22 @@ function redactSensitiveData(data: unknown): unknown { return data; } +// One structured object per line: Workers Logs serializes object arguments +// as JSON and indexes the top-level fields, so `service` and the redacted +// `context` keys stay filterable in the dashboard instead of being baked +// into an unparseable message string. Timestamp and severity metadata are +// attached by the observability pipeline either way. function log( level: LogLevel, service: string, message: string, context?: Record ): void { - const entry: LogEntry = { - timestamp: new Date().toISOString(), - service, - level, - message, - context: context - ? (redactSensitiveData(context) as Record) - : undefined - }; - - const logMessage = `[${entry.timestamp}] [${service.toUpperCase()}] [${level.toUpperCase()}] ${message}`; - const contextStr = entry.context ? ` ${JSON.stringify(entry.context)}` : ""; - - switch (level) { - case LogLevel.ERROR: - console.error(logMessage + contextStr); - break; - case LogLevel.WARN: - console.warn(logMessage + contextStr); - break; - case LogLevel.INFO: - console.info(logMessage + contextStr); - break; + const entry: Record = { service, message }; + if (context) { + entry.context = redactSensitiveData(context); } + console[level](entry); } export function logError( diff --git a/src/lib/pagination.ts b/src/lib/pagination.ts new file mode 100644 index 00000000..9e3a7a20 --- /dev/null +++ b/src/lib/pagination.ts @@ -0,0 +1,32 @@ +// Opt-in limit/offset pagination shared by the v1 list endpoints +// (extensions/v1 and central-alerts/v1). Absent params keep the full-list +// contract; a non-numeric limit is treated as absent rather than a 400 - +// these legacy surfaces have never validated query params and FOSSBilling's +// own client passes none. offset is the exception: only a caller opting into +// pagination can send it, so offset without a usable limit is invalid +// (matching the v2 pagination endpoints) rather than a silently ignored +// param that returns the full list. +export type LegacyPage = { limit: number; offset: number }; + +export function parseLegacyPagination(query: { + limit?: string; + offset?: string; +}): LegacyPage | "invalid" | undefined { + const limitParam = Number(query.limit); + const hasValidLimit = + Number.isInteger(limitParam) && limitParam >= 1 && limitParam <= 100; + if (query.offset !== undefined && !hasValidLimit) return "invalid"; + const offsetParam = query.offset === undefined ? 0 : Number(query.offset); + // isSafeInteger, not isInteger: 1e100 is an integer but SQLite rejects an + // out-of-range OFFSET, which would surface as a 500. Unusable offsets + // follow the documented zero fallback instead. + return hasValidLimit + ? { + limit: limitParam, + offset: + Number.isSafeInteger(offsetParam) && offsetParam >= 0 + ? offsetParam + : 0 + } + : undefined; +} diff --git a/src/services/central-alerts/v1/index.ts b/src/services/central-alerts/v1/index.ts index 1a424170..60a16a00 100644 --- a/src/services/central-alerts/v1/index.ts +++ b/src/services/central-alerts/v1/index.ts @@ -4,6 +4,7 @@ import { trimTrailingSlash } from "hono/trailing-slash"; import { CentralAlertsDatabase } from "./database"; import { getCentralAlertsDb } from "../../../lib/db"; import { logError } from "../../../lib/logger"; +import { parseLegacyPagination } from "../../../lib/pagination"; const centralAlertsV1 = new Hono<{ Bindings: CloudflareBindings }>(); @@ -12,17 +13,11 @@ centralAlertsV1.use("/*", cors({ origin: "*" }), trimTrailingSlash()); // Admin panels poll this public representation constantly. Parse once before // cache lookup so ignored inputs cannot create new entries for the same page. centralAlertsV1.get("/list", async (c) => { - // Opt-in pagination: absent params keep the full-list contract. A - // non-numeric limit is treated as absent rather than a 400 - this route - // has never validated query params and FOSSBilling's client passes none. - // offset is the exception: only a caller opting into pagination can send - // it, so offset without a usable limit is a 422 (matching the v2 - // pagination endpoints) rather than a silently ignored param. - const limitParam = Number(c.req.query("limit")); - const hasValidLimit = - Number.isInteger(limitParam) && limitParam >= 1 && limitParam <= 100; - const rawOffset = c.req.query("offset"); - if (rawOffset !== undefined && !hasValidLimit) { + const page = parseLegacyPagination({ + limit: c.req.query("limit"), + offset: c.req.query("offset") + }); + if (page === "invalid") { return c.json( { result: null, @@ -31,14 +26,6 @@ centralAlertsV1.get("/list", async (c) => { 422 ); } - const offsetParam = rawOffset === undefined ? 0 : Number(rawOffset); - const page = hasValidLimit - ? { - limit: limitParam, - offset: - Number.isInteger(offsetParam) && offsetParam >= 0 ? offsetParam : 0 - } - : undefined; const cacheUrl = new URL(c.req.url); // Hono decodes path aliases before routing; key that same routed path. @@ -51,6 +38,8 @@ centralAlertsV1.get("/list", async (c) => { } // This route never authenticates or varies by Authorization. Use a // header-free key so that arbitrary credentials cannot force a D1 read. + // Degrade to serving uncached on runtimes without Cache Storage (the + // portability seam) rather than failing the route. const edgeCache = typeof caches !== "undefined" ? await caches.open("central-alerts-v1") diff --git a/src/services/extensions/v1/database.ts b/src/services/extensions/v1/database.ts index 1d20cdf7..bdf9de76 100644 --- a/src/services/extensions/v1/database.ts +++ b/src/services/extensions/v1/database.ts @@ -2,14 +2,9 @@ import { and, eq, isNotNull, isNull, sql } from "drizzle-orm"; import { ExtensionsDb } from "../../../lib/db"; import { extensions, developers } from "../v2/db/schema"; import { DatabaseResult } from "../../../lib/interfaces"; -import { - Extension, - Release, - Author, - Repository, - sortReleasesDescending, - parseJSON -} from "./interfaces"; +import { sortReleasesDescending } from "../../../lib/releases"; +import { parseJSON } from "../../../lib/json"; +import { Extension, Release, Author, Repository } from "./interfaces"; const EXTENSION_COLUMNS = { id: extensions.id, @@ -65,6 +60,28 @@ interface ExtensionRow { export class ExtensionsDatabase { constructor(private db: ExtensionsDb) {} + // Shared catch shape for this module's D1 reads: driver message surfaced + // under the generic DATABASE_ERROR code. + private databaseError(error: unknown): DatabaseResult { + return { + data: null, + error: { + message: error instanceof Error ? error.message : String(error), + code: "DATABASE_ERROR" + } + }; + } + + private notFound(id: string): DatabaseResult { + return { + data: null, + error: { + message: `Cannot find extension by id: ${id}`, + code: "NOT_FOUND" + } + }; + } + // page (when given) bounds the query with a limit+1 probe - the extra // row only answers has_more and is trimmed off. Omitted => every // published extension, unchanged from the original contract. The full @@ -95,13 +112,7 @@ export class ExtensionsDatabase { .limit(page.limit + 1)) as ExtensionRow[]) : ((await base) as ExtensionRow[]); } catch (error) { - return { - data: null, - error: { - message: error instanceof Error ? error.message : String(error), - code: "DATABASE_ERROR" - } - }; + return this.databaseError(error); } const hasMore = page ? rows.length > page.limit : false; @@ -128,24 +139,12 @@ export class ExtensionsDatabase { ) )) as ExtensionRow[]; } catch (error) { - return { - data: null, - error: { - message: error instanceof Error ? error.message : String(error), - code: "DATABASE_ERROR" - } - }; + return this.databaseError(error); } const row = rows[0]; if (!row) { - return { - data: null, - error: { - message: `Cannot find extension by id: ${id}`, - code: "NOT_FOUND" - } - }; + return this.notFound(id); } return { data: parseExtensionRow(row), error: null }; @@ -173,24 +172,12 @@ export class ExtensionsDatabase { ) )) as { releases: string; license: string }[]; } catch (error) { - return { - data: null, - error: { - message: error instanceof Error ? error.message : String(error), - code: "DATABASE_ERROR" - } - }; + return this.databaseError(error); } const row = rows[0]; if (!row) { - return { - data: null, - error: { - message: `Cannot find extension by id: ${id}`, - code: "NOT_FOUND" - } - }; + return this.notFound(id); } const releases = sortReleasesDescending( diff --git a/src/services/extensions/v1/index.ts b/src/services/extensions/v1/index.ts index 6a6e543d..205b6753 100644 --- a/src/services/extensions/v1/index.ts +++ b/src/services/extensions/v1/index.ts @@ -1,14 +1,27 @@ -import { Hono } from "hono"; +import { Hono, type Context } from "hono"; import { cors } from "hono/cors"; import { trimTrailingSlash } from "hono/trailing-slash"; import { makeBadge } from "badge-maker"; import { getExtensionsDb } from "../../../lib/db"; import { singleFlight } from "../../../lib/cache"; +import { parseLegacyPagination } from "../../../lib/pagination"; import { ExtensionsDatabase } from "./database"; import { listCacheKey, LIST_CACHE_TTL_SECONDS } from "./list-cache"; const extensionsV1 = new Hono<{ Bindings: CloudflareBindings }>(); +// Shared failure shape for this service's plain-JSON routes: the database's +// message when present, 404 only for its NOT_FOUND code. +function extensionError( + c: Context<{ Bindings: CloudflareBindings }>, + error: { message: string; code?: string } | null | undefined +) { + return c.json( + { error: { message: error?.message ?? "Extension not found" } }, + error?.code === "NOT_FOUND" ? 404 : 500 + ); +} + extensionsV1.use("/*", cors({ origin: "*" })); extensionsV1.use("/*", trimTrailingSlash()); @@ -23,28 +36,13 @@ extensionsV1.get("/list", async (c) => { const db = new ExtensionsDatabase(getExtensionsDb(c.env.DB_EXTENSIONS)); const type = c.req.query("type"); - // Opt-in pagination: absent params keep the exact original contract - // (every published extension, no pagination object). A non-numeric limit - // is treated as absent rather than a 400 - this legacy surface has never - // validated query params. offset is the exception: only a caller opting - // into pagination can send it, so offset without a usable limit is a 422 - // (matching the v2 pagination endpoints) rather than a silently ignored - // param that returns the full list. - const limitParam = Number(c.req.query("limit")); - const hasValidLimit = - Number.isInteger(limitParam) && limitParam >= 1 && limitParam <= 100; - const rawOffset = c.req.query("offset"); - if (rawOffset !== undefined && !hasValidLimit) { + const page = parseLegacyPagination({ + limit: c.req.query("limit"), + offset: c.req.query("offset") + }); + if (page === "invalid") { return c.json({ error: { message: "offset requires limit" } }, 422); } - const offsetParam = rawOffset === undefined ? 0 : Number(rawOffset); - const page = hasValidLimit - ? { - limit: limitParam, - offset: - Number.isInteger(offsetParam) && offsetParam >= 0 ? offsetParam : 0 - } - : undefined; const cacheKey = listCacheKey(type, page); // singleFlight coalesces the serialized body STRING, not the Response: @@ -103,11 +101,7 @@ extensionsV1.get("/:id/badges/:type", async (c) => { const { data: badgeData, error } = await db.getExtensionBadgeData(id); if (error || !badgeData) { - const status = error?.code === "NOT_FOUND" ? 404 : 500; - return c.json( - { error: { message: error?.message ?? "Extension not found" } }, - status - ); + return extensionError(c, error); } const latest = badgeData.latestRelease; @@ -157,11 +151,7 @@ extensionsV1.get("/:id/version", async (c) => { const { data: badgeData, error } = await db.getExtensionBadgeData(id); if (error || !badgeData) { - const status = error?.code === "NOT_FOUND" ? 404 : 500; - return c.json( - { error: { message: error?.message ?? "Extension not found" } }, - status - ); + return extensionError(c, error); } const latest = badgeData.latestRelease; @@ -180,11 +170,7 @@ extensionsV1.get("/:id", async (c) => { const { data: extension, error } = await db.getExtensionById(id); if (error || !extension) { - const status = error?.code === "NOT_FOUND" ? 404 : 500; - return c.json( - { error: { message: error?.message ?? "Extension not found" } }, - status - ); + return extensionError(c, error); } // Same shape as the v2 public detail route: short client TTL, CDN holds diff --git a/src/services/extensions/v1/interfaces.ts b/src/services/extensions/v1/interfaces.ts index 510a7fa7..6c2533a6 100644 --- a/src/services/extensions/v1/interfaces.ts +++ b/src/services/extensions/v1/interfaces.ts @@ -1,10 +1,5 @@ -import { gt } from "semver"; -import { sortReleasesDescending } from "../../../lib/releases"; -import { parseJSON } from "../../../lib/json"; import { EXTENSION_TYPES } from "../v2/schemas/extensions"; -export { sortReleasesDescending, parseJSON }; - export type Extension = { id: string; // Derived from v2 rather than restated: both describe the same @@ -63,23 +58,3 @@ export type Release = { changelog_url?: string; min_fossbilling_version: string; }; - -export function getLatestRelease(extension: Extension): Release | undefined { - if (extension.releases.length === 0) { - return undefined; - } - - let latestRelease = extension.releases[0]; - for (let i = 1; i < extension.releases.length; i++) { - const release = extension.releases[i]; - try { - if (gt(release.tag, latestRelease.tag)) { - latestRelease = release; - } - } catch { - // Ignore invalid semver tags - } - } - - return latestRelease; -} diff --git a/src/services/extensions/v2/db/cursor.ts b/src/services/extensions/v2/db/cursor.ts index 86a21e36..524c02a9 100644 --- a/src/services/extensions/v2/db/cursor.ts +++ b/src/services/extensions/v2/db/cursor.ts @@ -45,3 +45,18 @@ export function decodeCursor( return null; } } + +// A module's codec is a guard over its own fields; the envelope plumbing +// (version stamping, fatal decode, guard-signature ceremony) is built once +// here. Cursor shapes are `type` aliases so they satisfy Record. +export function createCursorCodec>( + isValid: (parsed: Record) => parsed is T +): { encode: (payload: T) => string; decode: (cursor: string) => T | null } { + return { + encode: (payload) => encodeCursor(payload), + decode: (cursor) => + decodeCursor(cursor, (parsed): parsed is T & Record => + isValid(parsed) + ) + }; +} diff --git a/src/services/extensions/v2/db/developer-claims.ts b/src/services/extensions/v2/db/developer-claims.ts index 2f80eb63..0b9ba896 100644 --- a/src/services/extensions/v2/db/developer-claims.ts +++ b/src/services/extensions/v2/db/developer-claims.ts @@ -1,7 +1,7 @@ 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 { createCursorCodec } from "./cursor"; import { developerClaims, developers, users } from "./schema"; import { databaseError, @@ -318,7 +318,7 @@ export class DeveloperClaimsDatabase { }> > { const limit = page?.limit ?? 50; - const decoded = page?.cursor ? decodeClaimCursor(page.cursor) : null; + const decoded = page?.cursor ? claimCursor.decode(page.cursor) : null; // 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 @@ -415,12 +415,12 @@ export class DeveloperClaimsDatabase { hasMore, nextCursor: hasMore && last - ? encodeClaimCursor( - last.claim.createdAt, - String(last.rowid), - filters.scope, - filters.scope === "mine" ? filters.claimantId : undefined - ) + ? claimCursor.encode({ + k1: last.claim.createdAt, + k2: String(last.rowid), + s: filters.scope, + ...(filters.scope === "mine" ? { c: filters.claimantId } : {}) + }) : null }, error: null @@ -674,35 +674,17 @@ export class DeveloperClaimsDatabase { } } -interface ClaimCursor { +type ClaimCursor = { k1: string; k2: string; s: "mine" | "pending"; c?: string; -} - -function encodeClaimCursor( - k1: string, - k2: string, - s: "mine" | "pending", - claimantId?: string -): string { - return encode( - claimantId === undefined ? { k1, k2, s } : { k1, k2, s, c: claimantId } - ); -} +}; -function isClaimCursor( - parsed: Record -): parsed is ClaimCursor & Record { - return ( +const claimCursor = createCursorCodec( + (parsed): parsed is ClaimCursor => typeof parsed.k1 === "string" && typeof parsed.k2 === "string" && (parsed.s === "mine" || parsed.s === "pending") && (parsed.c === undefined || typeof parsed.c === "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 a448a974..a4bb3437 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, 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 { createCursorCodec } from "./cursor"; import { developers, developerHistory, @@ -715,7 +715,7 @@ export class DeveloperProfilesDatabase { const scope = filters.scope ?? filters.status ?? "all"; const limit = filters.limit ?? 50; const decoded = filters.cursor - ? decodeDeveloperCursor(filters.cursor) + ? developerListCursor.decode(filters.cursor) : null; if (filters.cursor && !decoded) { return { @@ -798,13 +798,14 @@ export class DeveloperProfilesDatabase { hasMore, nextCursor: hasMore && last - ? encodeDeveloperCursor( - scope, - scope === "unapproved" - ? last.developer.createdAt - : last.developer.name, - String(last.rowid) - ) + ? developerListCursor.encode({ + k1: + scope === "unapproved" + ? last.developer.createdAt + : last.developer.name, + k2: String(last.rowid), + s: scope + }) : null }, error: null @@ -885,7 +886,7 @@ export class DeveloperProfilesDatabase { }> > { const limit = page?.limit ?? 50; - const decoded = page?.cursor ? decodeHistoryCursor(page.cursor) : null; + const decoded = page?.cursor ? historyCursor.decode(page.cursor) : null; // Case-insensitive like the id matching everywhere else: ids are // lowercase slugs by schema, but adopted rows predate that. if ( @@ -960,11 +961,11 @@ export class DeveloperProfilesDatabase { hasMore, nextCursor: hasMore && last - ? encodeHistoryCursor( - last.changedAt, - String(last.rowid), - developerId - ) + ? historyCursor.encode({ + k1: last.changedAt, + k2: String(last.rowid), + d: developerId + }) : null }, error: null @@ -1213,54 +1214,28 @@ export class DeveloperProfilesDatabase { } } -interface DeveloperListCursor { +type 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 ( +const developerListCursor = createCursorCodec( + (parsed): parsed is DeveloperListCursor => 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 { +type HistoryCursor = { k1: string; k2: string; d: string; -} +}; -function encodeHistoryCursor(k1: string, k2: string, d: string): string { - return encode({ k1, k2, d }); -} - -function isHistoryCursor( - parsed: Record -): parsed is HistoryCursor & Record { - return ( +const historyCursor = createCursorCodec( + (parsed): parsed is HistoryCursor => typeof parsed.k1 === "string" && typeof parsed.k2 === "string" && typeof parsed.d === "string" - ); -} - -function decodeHistoryCursor(cursor: string): HistoryCursor | null { - return decode(cursor, isHistoryCursor); -} +); diff --git a/src/services/extensions/v2/db/developer-transfers.ts b/src/services/extensions/v2/db/developer-transfers.ts index c8c08e42..df042404 100644 --- a/src/services/extensions/v2/db/developer-transfers.ts +++ b/src/services/extensions/v2/db/developer-transfers.ts @@ -11,16 +11,7 @@ import { toD1Statement } from "./batch"; import { DeveloperProfile } from "../schemas/developers"; import { DeveloperTransfer } from "../schemas/ownership"; import { DeveloperProfilesDatabase } from "./developer-profiles"; - -async function sha256Hex(input: string): Promise { - const digest = await crypto.subtle.digest( - "SHA-256", - new TextEncoder().encode(input) - ); - return [...new Uint8Array(digest)] - .map((b) => b.toString(16).padStart(2, "0")) - .join(""); -} +import { sha256Hex } from "../../../../lib/hash"; function toSqliteDatetime(date: Date): string { return date.toISOString().slice(0, 19).replace("T", " "); diff --git a/src/services/extensions/v2/db/extensions.ts b/src/services/extensions/v2/db/extensions.ts index 9a23c2fd..e65db14f 100644 --- a/src/services/extensions/v2/db/extensions.ts +++ b/src/services/extensions/v2/db/extensions.ts @@ -12,10 +12,9 @@ import { inactiveActorError, moderatorActorError } from "./errors"; -import { UsersDatabase } from "./users"; import { toD1Statement } from "./batch"; import { developerIsApproved } from "./developer-approval"; -import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; +import { createCursorCodec } from "./cursor"; import { Extension, ExtensionContent, @@ -284,10 +283,20 @@ export interface OwnedExtensionListPage { hasMore: boolean; } -interface ExtensionCursor { +type ExtensionCursor = { normalizedId: string; id: string; -} +}; + +// normalizedId is checked against id rather than trusted: it drives the +// keyset comparison, so a tampered cursor could otherwise seek from a +// position the id itself doesn't correspond to. +const extensionCursor = createCursorCodec( + (parsed): parsed is ExtensionCursor => + typeof parsed.id === "string" && + typeof parsed.normalizedId === "string" && + parsed.normalizedId === parsed.id.toLowerCase() +); export interface CreateExtensionInput { extensionId: string; @@ -314,7 +323,7 @@ export class ExtensionsDatabase { conditions.push(eq(extensions.developerId, filters.developerId)); if (filters.cursor) { - const cursor = decodeCursor(filters.cursor); + const cursor = extensionCursor.decode(filters.cursor); if (!cursor) return invalidCursor(); conditions.push(keysetAfter(cursor)); } @@ -332,14 +341,12 @@ export class ExtensionsDatabase { return databaseError("list", error); } - const hasMore = rows.length > limit; - const pageRows = rows.slice(0, limit); - const last = pageRows.at(-1); + const { pageRows, hasMore, nextCursor } = keysetPage(rows, limit); return { data: { items: pageRows.map((row) => parseListRow(row)), hasMore, - nextCursor: hasMore && last ? encodeCursor(last.id) : null + nextCursor }, error: null }; @@ -396,7 +403,7 @@ export class ExtensionsDatabase { ); } if (filters.cursor) { - const cursor = decodeCursor(filters.cursor); + const cursor = extensionCursor.decode(filters.cursor); if (!cursor) return invalidCursor(); conditions.push(keysetAfter(cursor)); } @@ -411,14 +418,12 @@ export class ExtensionsDatabase { return databaseError("listOwned", error); } - const hasMore = rows.length > limit; - const pageRows = rows.slice(0, limit); - const last = pageRows.at(-1); + const { pageRows, hasMore, nextCursor } = keysetPage(rows, limit); return { data: { items: pageRows.map(parseOwnedListRow), hasMore, - nextCursor: hasMore && last ? encodeCursor(last.id) : null + nextCursor }, error: null }; @@ -470,7 +475,7 @@ export class ExtensionsDatabase { ); } if (filters.cursor) { - const cursor = decodeCursor(filters.cursor); + const cursor = extensionCursor.decode(filters.cursor); if (!cursor) return invalidCursor(); conditions.push(keysetAfter(cursor)); } @@ -485,14 +490,12 @@ export class ExtensionsDatabase { return databaseError("listForModeration", error); } - const hasMore = rows.length > limit; - const pageRows = rows.slice(0, limit); - const last = pageRows.at(-1); + const { pageRows, hasMore, nextCursor } = keysetPage(rows, limit); return { data: { items: pageRows.map(parseOwnedListRow), hasMore, - nextCursor: hasMore && last ? encodeCursor(last.id) : null + nextCursor }, error: null }; @@ -919,33 +922,8 @@ export class ExtensionsDatabase { // 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. - 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 (!access.data.moderator) { - return { - data: null, - error: { message: "Moderator access required", code: "FORBIDDEN" } - }; - } + const actorError = await moderatorActorError(this.db, moderatorId); + if (actorError) return { data: null, error: actorError }; let existing: { publishedAt: string | null; delistedAt: string | null } | undefined; @@ -1044,6 +1022,11 @@ export class ExtensionsDatabase { ] }); + // The UPDATE below is the column list publish shares with + // ExtensionRevisionsDatabase.approve() in db/revisions.ts; the two + // differ only in published_at handling and how the row is matched + // (LOWER(id) here, because corrections must address legacy + // differently-cased rows). Change the projection in both places together. const publishStmt = toD1Statement(this.db.$client, { sql: `UPDATE extensions SET type = ?, name = ?, description = ?, releases = ?, website = ?, @@ -1095,33 +1078,8 @@ export class ExtensionsDatabase { id: string, moderatorId: string ): Promise> { - 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 (!access.data.moderator) { - return { - data: null, - error: { message: "Moderator access required", code: "FORBIDDEN" } - }; - } + const actorError = await moderatorActorError(this.db, moderatorId); + if (actorError) return { data: null, error: actorError }; let existing: { publishedAt: string | null; delistedAt: string | null } | undefined; @@ -1216,29 +1174,8 @@ function keysetAfter(cursor: ExtensionCursor) { )!; } -function encodeCursor(id: string): string { - return encode({ normalizedId: id.toLowerCase(), id }); -} - -// normalizedId is checked against id rather than trusted: it drives the -// keyset comparison, so a tampered cursor could otherwise seek from a -// position the id itself doesn't correspond to. -function isExtensionCursor( - parsed: Record -): parsed is ExtensionCursor & Record { - return ( - typeof parsed.id === "string" && - typeof parsed.normalizedId === "string" && - parsed.normalizedId === parsed.id.toLowerCase() - ); -} - -function decodeCursor(value: string): ExtensionCursor | null { - return decode(value, isExtensionCursor); -} - export function isValidExtensionCursor(value: string): boolean { - return decodeCursor(value) !== null; + return extensionCursor.decode(value) !== null; } function parseDeveloper(row: DeveloperRow): PublicDeveloper { @@ -1285,44 +1222,31 @@ function cardSource(stored: string | null): Repository { return source; } -function parseListRow(row: PublishedListRow, card = true): ExtensionListItem { - return { - id: row.id, - type: row.type as ExtensionListItem["type"], - name: card ? row.name.slice(0, 120) : row.name, - description: card ? row.description.slice(0, 4000) : row.description, - website: card ? cardUrl(row.website) : row.website, - license: card - ? cardLicense(row.license) - : parseJSON(row.license, { name: "" }), - icon_url: (card ? cardUrl(row.iconUrl) : row.iconUrl) ?? undefined, - source: card - ? cardSource(row.source) - : parseJSON(row.source, { type: "custom", repo: "" }), - version: card ? row.version.slice(0, 100) : row.version, - download_url: card ? cardUrl(row.downloadUrl) : row.downloadUrl, - developer: parseDeveloper(row) - }; -} - -// The detail view is the list projection plus the two large fields the -// catalogue query deliberately omits. -function parseRow(row: PublishedRow): Extension { - return { - ...parseListRow(row, false), - website: row.website, - download_url: row.downloadUrl, - readme: row.readme, - releases: boundedReleases(parseJSON(row.releases, [])) - }; +// The field subset both the public catalogue rows and the owner listing rows +// project, in their nullable table form. +interface ListContentFields { + type: string | null; + name: string | null; + description: string | null; + website: string | null; + license: string | null; + iconUrl: string | null; + source: string | null; + version: string | null; + downloadUrl: string | null; } -// Only ever called for a row whose published_at is set, where -// extensions_published_content_check guarantees each of these is present. -function publishedContent( - row: OwnedListRow, - card = true -): NonNullable { +// The card/detail content projection shared by parseListRow (public +// catalogue) and the owned listing's published payload. Shaping in one place +// keeps the two payload producers from drifting; the non-null assertions are +// sound because every caller only reaches here for rows whose published_at +// is set, where extensions_published_content_check guarantees presence. +// +// The card=true JS-side slices restate the truncation CONTENT_COLUMNS +// already performs in SQL. They are belt-and-braces for legacy rows written +// before that truncation existed - not dead code, and not to be "simplified" +// away without an audit of pre-migration rows. +function listContent(row: ListContentFields, card: boolean) { return { type: row.type as ExtensionContent["type"], name: card ? (row.name as string).slice(0, 120) : (row.name as string), @@ -1347,11 +1271,53 @@ function publishedContent( }; } +// Shared keyset-pagination tail: each listing over-fetches one row to detect +// hasMore, then encodes the next cursor from the final returned row. +function keysetPage( + rows: T[], + limit: number +): { pageRows: T[]; hasMore: boolean; nextCursor: string | null } { + const hasMore = rows.length > limit; + const pageRows = rows.slice(0, limit); + const last = pageRows.at(-1); + return { + pageRows, + hasMore, + nextCursor: + hasMore && last + ? extensionCursor.encode({ + normalizedId: last.id.toLowerCase(), + id: last.id + }) + : null + }; +} + +function parseListRow(row: PublishedListRow, card = true): ExtensionListItem { + return { + id: row.id, + ...listContent(row, card), + developer: parseDeveloper(row) + }; +} + +// The detail view is the list projection plus the two large fields the +// catalogue query deliberately omits. +function parseRow(row: PublishedRow): Extension { + return { + ...parseListRow(row, false), + website: row.website, + download_url: row.downloadUrl, + readme: row.readme, + releases: boundedReleases(parseJSON(row.releases, [])) + }; +} + function parseOwnedListRow(row: OwnedListRow): OwnedExtensionListItem { return { id: row.id, developer: parseDeveloper(row), - published: row.publishedAt ? publishedContent(row) : null, + published: row.publishedAt ? listContent(row, true) : null, pending_revision: row.pendingId && row.pendingCreatedAt ? { id: row.pendingId, created_at: row.pendingCreatedAt } @@ -1379,7 +1345,7 @@ function parseOwnedRow(row: OwnedRow): OwnedExtension { ...parseOwnedListRow(row), published: row.publishedAt ? { - ...publishedContent(row, false), + ...listContent(row, false), website: row.website as string, download_url: row.downloadUrl as string, readme: row.readme as string, diff --git a/src/services/extensions/v2/db/resource-maintenance.ts b/src/services/extensions/v2/db/resource-maintenance.ts index 101effd5..a85e0116 100644 --- a/src/services/extensions/v2/db/resource-maintenance.ts +++ b/src/services/extensions/v2/db/resource-maintenance.ts @@ -1,6 +1,7 @@ import { sql } from "drizzle-orm"; import { ExtensionsDb } from "../../../../lib/db"; import { logInfo, logWarn } from "../../../../lib/logger"; +import { sha256Hex } from "../../../../lib/hash"; import { MAX_CONTENT_BYTES, RETENTION_DAYS, @@ -31,13 +32,7 @@ export async function maintainExtensionResources( AND content_bytes <= ${MAX_CONTENT_BYTES} AND compacted_at IS NULL `); if (!row) continue; - const digest = await crypto.subtle.digest( - "SHA-256", - new TextEncoder().encode(row.content) - ); - const hash = Array.from(new Uint8Array(digest), (b) => - b.toString(16).padStart(2, "0") - ).join(""); + const hash = await sha256Hex(row.content); const result = await db.run(sql` UPDATE extension_revisions SET content='{}', content_hash=${hash}, compacted_at=CURRENT_TIMESTAMP WHERE id=${id} AND content=${row.content} AND compacted_at IS NULL diff --git a/src/services/extensions/v2/db/revisions.ts b/src/services/extensions/v2/db/revisions.ts index c03fc95f..5b42f189 100644 --- a/src/services/extensions/v2/db/revisions.ts +++ b/src/services/extensions/v2/db/revisions.ts @@ -10,7 +10,7 @@ import { moderatorActorError } from "./errors"; import { toD1Statement } from "./batch"; -import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; +import { createCursorCodec } from "./cursor"; import { MAX_PENDING_REVISIONS_PER_USER, parseContent, @@ -37,24 +37,15 @@ interface StoredRevision extends ExtensionRevision { ownershipEpoch: number; } -interface RevisionCursor { +type RevisionCursor = { createdAt: string; id: string; -} - -function encodeCursor(createdAt: string, id: string): string { - return encode({ createdAt, id }); -} - -function isRevisionCursor( - parsed: Record -): parsed is RevisionCursor & Record { - return typeof parsed.createdAt === "string" && typeof parsed.id === "string"; -} +}; -function decodeCursor(cursor: string): RevisionCursor | null { - return decode(cursor, isRevisionCursor); -} +const revisionCursor = createCursorCodec( + (parsed): parsed is RevisionCursor => + typeof parsed.createdAt === "string" && typeof parsed.id === "string" +); interface RevisionRow { id: string; @@ -253,7 +244,7 @@ export class ExtensionRevisionsDatabase { readerId?: string, extensionId?: string ): Promise> { - const decoded = cursor ? decodeCursor(cursor) : null; + const decoded = cursor ? revisionCursor.decode(cursor) : null; if (cursor && !decoded) { return { data: null, @@ -308,7 +299,9 @@ export class ExtensionRevisionsDatabase { items, hasMore, nextCursor: - hasMore && last ? encodeCursor(last.created_at, last.id) : null + hasMore && last + ? revisionCursor.encode({ createdAt: last.created_at, id: last.id }) + : null }, error: null }; @@ -585,6 +578,12 @@ export class ExtensionRevisionsDatabase { // published_at is COALESCEd rather than overwritten: it records when the // extension first entered the catalogue, and updated_at carries the // "changed just now" signal. + // + // This is the column list publish shares with + // ExtensionsDatabase.moderatorCorrect() in db/extensions.ts; the two + // differ only in published_at handling and how the row is matched + // (exact id here, LOWER(id) there, because corrections address legacy + // rows). Change the projection in both places together. const publishStmt = toD1Statement(this.db.$client, { sql: `UPDATE extensions SET type = ?, name = ?, description = ?, releases = ?, website = ?, diff --git a/src/services/extensions/v2/email/templates.ts b/src/services/extensions/v2/email/templates.ts index 590f6701..cd7f8e66 100644 --- a/src/services/extensions/v2/email/templates.ts +++ b/src/services/extensions/v2/email/templates.ts @@ -121,15 +121,19 @@ function displayLabel( export function buildModerationEmail( input: ModerationEmailInput ): EmailMessage { + // The subject uses the same label composition as the body; subjectLabel + // then folds it to SMTP-safe ASCII. The newline flattening displayLabel + // applies is idempotent under subjectLabel's own flattening, so composing + // the two is safe and keeps the subject/body formats from drifting. const extLabel = subjectLabel( - input.extensionName && input.extensionId - ? `“${stripSurroundingQuotes(input.extensionName)}” (${input.extensionId})` - : (input.extensionId ?? input.extensionName ?? "your extension") + displayLabel(input.extensionName, input.extensionId, "your extension") ); const devLabel = subjectLabel( - input.developerName && input.developerId - ? `“${stripSurroundingQuotes(input.developerName)}” (${input.developerId})` - : (input.developerId ?? input.developerName ?? "your developer profile") + displayLabel( + input.developerName, + input.developerId, + "your developer profile" + ) ); const extDisplay = displayLabel( input.extensionName, diff --git a/src/services/extensions/v2/middleware.ts b/src/services/extensions/v2/middleware.ts index 4cc59c7a..872c73bf 100644 --- a/src/services/extensions/v2/middleware.ts +++ b/src/services/extensions/v2/middleware.ts @@ -8,10 +8,9 @@ import { import { getAuth, requireAuth } from "../../../lib/auth"; import type { AuthPrincipal } from "../../../lib/auth"; import { getExtensionsDb } from "../../../lib/db"; +import { sha256Hex } from "../../../lib/hash"; import { UsersDatabase } from "./db/users"; -export const requireAuthAllowInactive = requireAuth; - // Optional bearer auth for merged public+authenticated reads: no // Authorization header continues anonymously; a present header is verified // normally (invalid tokens still 401 rather than silently downgrading to @@ -98,10 +97,7 @@ export function requireIdentitySync(): MiddlewareHandler { 400 ); } - const digest = await crypto.subtle.digest("SHA-256", rawBody); - const hex = Array.from(new Uint8Array(digest), (byte) => - byte.toString(16).padStart(2, "0") - ).join(""); + const hex = await sha256Hex(rawBody); if (hex !== auth.bodySha256) { return c.json( { diff --git a/src/services/extensions/v2/routes/account.ts b/src/services/extensions/v2/routes/account.ts index f242fbe9..4cefbaab 100644 --- a/src/services/extensions/v2/routes/account.ts +++ b/src/services/extensions/v2/routes/account.ts @@ -1,14 +1,11 @@ -import { - requireActiveAuth, - requireAuthAllowInactive, - requireIdentitySync -} from "../middleware"; +import { requireActiveAuth, requireIdentitySync } from "../middleware"; +import { getAuth, requireAuth } from "../../../../lib/auth"; import { getExtensionsDb } from "../../../../lib/db"; import { createRoute, z } from "@hono/zod-openapi"; -import { getAuth } from "../../../../lib/auth"; import { errorBody, statusFromErrorCode } from "./errors"; import { ActiveAccountRequiredResponse, + authErrorResponses, errorResponse } from "../schemas/common"; import { @@ -54,7 +51,7 @@ export function registerAccountRoutes(app: ExtensionsV2App): void { description: "Identity projection synchronized" }, 400: errorResponse("Request body could not be read"), - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: "Identity synchronization requires a trusted assertion" @@ -91,7 +88,7 @@ export function registerAccountRoutes(app: ExtensionsV2App): void { tags: ["Users"], summary: "Get the caller's account projection", security: [{ Bearer: [] }], - middleware: [requireAuthAllowInactive()] as const, + middleware: [requireAuth()] as const, responses: { 200: { content: { @@ -148,8 +145,7 @@ export function registerAccountRoutes(app: ExtensionsV2App): void { }, description: "Profile updated (full account projection)" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 404: errorResponse("Account does not exist or has been deleted"), 500: errorResponse("Database error") } @@ -183,7 +179,7 @@ export function registerAccountRoutes(app: ExtensionsV2App): void { tags: ["Users"], summary: "Delete the caller's account and tombstone its user row", security: [{ Bearer: [] }], - middleware: [requireAuthAllowInactive()] as const, + middleware: [requireAuth()] as const, responses: { 200: { content: { diff --git a/src/services/extensions/v2/routes/developer-profiles.ts b/src/services/extensions/v2/routes/developer-profiles.ts index 3d004fb6..4ed44d1a 100644 --- a/src/services/extensions/v2/routes/developer-profiles.ts +++ b/src/services/extensions/v2/routes/developer-profiles.ts @@ -10,13 +10,17 @@ import { getAuth } from "../../../../lib/auth"; import { createRoute, z } from "@hono/zod-openapi"; import { errorBody, + listErrorStatus, + listPayload, statusFromErrorCode, statusFromGithubErrorCode } from "./errors"; import { ActiveAccountRequiredResponse, IdParamSchema, + ModeratorForbiddenResponse, PaginationSchema, + authErrorResponses, errorResponse } from "../schemas/common"; import { @@ -55,11 +59,7 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { }, 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" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 422: errorResponse( "scope, status, limit, or cursor query failed validation" ), @@ -91,22 +91,11 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { }); if (error || !data) { return c.json( - { - error: { - message: error?.message ?? "Unable to load developers", - code: error?.code ?? "DATABASE_ERROR" - } - }, - error?.code === "INVALID_CURSOR" ? 422 : 500 + errorBody(error, "Unable to load developers"), + listErrorStatus(error) ); } - return c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + return c.json(listPayload(data), 200); }); const getOwnDeveloperRoute = createRoute({ @@ -125,8 +114,7 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { }, description: "The caller's profile, or null when none exists" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 500: errorResponse("Database error") } }); @@ -176,7 +164,7 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { description: "Developer profile created or updated and usable immediately" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -264,8 +252,7 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { }, description: "Profile deleted" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 404: errorResponse("Caller has no developer profile"), 409: errorResponse( "Profile still has extensions attached, published or not" @@ -314,8 +301,7 @@ export function registerDeveloperProfileRoutes(app: ExtensionsV2App): void { }, description: "Verification re-checked (result may be verified or not)" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 404: errorResponse("Caller has no developer profile"), 409: errorResponse("Developer ownership changed while re-verifying"), 429: errorResponse( diff --git a/src/services/extensions/v2/routes/errors.ts b/src/services/extensions/v2/routes/errors.ts index 3d6f98bf..020a67f5 100644 --- a/src/services/extensions/v2/routes/errors.ts +++ b/src/services/extensions/v2/routes/errors.ts @@ -81,6 +81,28 @@ export function setContentRetryAfter(c: Context, code?: string): void { else if (code === "WRITE_RATE_DAY") c.header("Retry-After", "86400"); } +// Shared contract for every paginated listing. Hono's OpenAPI route typing +// brands each c.json() call with the declared response shape, so the shared +// helpers build the payload and pick the status while the c.json calls stay +// at the (typed) call site. INVALID_CURSOR is 422: a client mistake, unlike +// an internal failure. +export function listPayload(data: { + items: T[]; + nextCursor: string | null; + hasMore: boolean; +}) { + return { + result: data.items, + pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } + }; +} + +export function listErrorStatus( + error: DatabaseError | null | undefined +): 422 | 500 { + return error?.code === "INVALID_CURSOR" ? 422 : 500; +} + export function statusFromContentWriteError( code?: string ): 403 | 404 | 409 | 429 | 500 | 503 { diff --git a/src/services/extensions/v2/routes/moderation.ts b/src/services/extensions/v2/routes/moderation.ts index 533c9268..2cf53a5f 100644 --- a/src/services/extensions/v2/routes/moderation.ts +++ b/src/services/extensions/v2/routes/moderation.ts @@ -1,19 +1,21 @@ import { paceContentAccount, requireModerator } from "../middleware"; import { getExtensionsDb } from "../../../../lib/db"; -import { getPlatform } from "../../../../lib/middleware"; import { getAuth } from "../../../../lib/auth"; import { createRoute, z } from "@hono/zod-openapi"; import { errorBody, + listErrorStatus, + listPayload, setContentRetryAfter, statusFromContentWriteError, statusFromWriteErrorCode } from "./errors"; import { - ActiveAccountRequiredResponse, + authErrorResponses, CursorPaginationQuerySchema, DelistReasonSchema, IdParamSchema, + ModeratorForbiddenResponse, NotifiedSchema, NotifyQuerySchema, PaginationSchema, @@ -30,7 +32,7 @@ import { ExtensionUpdateSchema } from "../schemas/extensions"; import { DeveloperProfilesDatabase } from "../db/developer-profiles"; import { ExtensionsDatabase } from "../db/extensions"; import { ExtensionRevisionsDatabase } from "../db/revisions"; -import { notifyRequested, sendModerationNotification } from "../email/notify"; +import { notifyAuthor } from "./notify"; import { revalidateCatalogue } from "../revalidate"; import { ExtensionsV2App } from "./app"; @@ -69,11 +71,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { description: "Revision approved and published as the extension's live content" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No such revision on that extension"), 409: errorResponse( "Revision is not pending, or ownership has changed since it was proposed" @@ -102,22 +100,13 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { const status = statusFromWriteErrorCode(error?.code); return c.json(errorBody(error, "Unable to approve revision"), status); } - revalidateCatalogue(c); - let notified = false; - if (notifyRequested(query)) { - notified = await sendModerationNotification( - getPlatform(c), - extDb, - { - kind: "revision-approved", - extensionId: id, - // Optional and untrimmed by its schema: a whitespace-only note would - // otherwise reach the author as a meaningless "Moderator note:". - reason: review_note?.trim() || undefined - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + const notified = await notifyAuthor(c, extDb, query, { + kind: "revision-approved", + extensionId: id, + // Optional and untrimmed by its schema: a whitespace-only note would + // otherwise reach the author as a meaningless "Moderator note:". + reason: review_note?.trim() || undefined + }); return c.json({ result: { ...data, notified } }, 200); }); @@ -151,11 +140,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { description: "Revision rejected. The extension's published content is unchanged." }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No such revision on that extension"), 409: errorResponse("Revision is not pending"), 422: errorResponse("review_note body or notify query failed validation"), @@ -180,20 +165,11 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { const status = statusFromWriteErrorCode(error?.code); return c.json(errorBody(error, "Unable to reject revision"), status); } - revalidateCatalogue(c); - let notified = false; - if (notifyRequested(query)) { - notified = await sendModerationNotification( - getPlatform(c), - extDb, - { - kind: "revision-rejected", - extensionId: id, - reason: review_note - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + const notified = await notifyAuthor(c, extDb, query, { + kind: "revision-rejected", + extensionId: id, + reason: review_note + }); return c.json({ result: { ...data, notified } }, 200); }); @@ -232,11 +208,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { "Extension removed from the public catalogue. Its content and " + "history are kept, and its owner can still see and edit it." }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No such extension"), 409: errorResponse("Extension is not published, or is already delisted"), 422: errorResponse( @@ -258,20 +230,11 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { const status = statusFromWriteErrorCode(error?.code); return c.json(errorBody(error, "Unable to delist extension"), status); } - revalidateCatalogue(c); - let notified = false; - if (notifyRequested(query)) { - notified = await sendModerationNotification( - getPlatform(c), - extDb, - { - kind: "extension-delisted", - extensionId: id, - reason - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + const notified = await notifyAuthor(c, extDb, query, { + kind: "extension-delisted", + extensionId: id, + reason + }); return c.json( { result: { id: data.id, status: "delisted" as const, notified } }, 200 @@ -308,11 +271,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { 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" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No such extension"), 409: errorResponse("Extension is not delisted, or was never published"), 422: errorResponse( @@ -334,20 +293,11 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { 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: data.id, - reason: review_note?.trim() || undefined - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + const notified = await notifyAuthor(c, extDb, query, { + kind: "extension-relisted", + extensionId: data.id, + reason: review_note?.trim() || undefined + }); return c.json( { result: { id: data.id, status: "relisted" as const, notified } }, 200 @@ -396,11 +346,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { description: "Content corrected and published. Recorded as an approved moderator revision; no author email is sent." }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No such extension"), 409: errorResponse( "Extension is unpublished or delisted, or an edit is already awaiting review" @@ -477,11 +423,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { }, description: "Developer profile marked approved" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No developer with that id"), 409: errorResponse("Profile changed after the reviewed revision"), 422: errorResponse( @@ -508,19 +450,10 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { const status = statusFromWriteErrorCode(error?.code); return c.json(errorBody(error, "Unable to approve developer"), status); } - revalidateCatalogue(c); - let notified = false; - if (notifyRequested(query)) { - notified = await sendModerationNotification( - getPlatform(c), - extDb, - { - kind: "developer-approved", - developerId: id - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + const notified = await notifyAuthor(c, extDb, query, { + kind: "developer-approved", + developerId: id + }); return c.json({ result: { ...data, notified } }, 200); }); @@ -544,11 +477,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { }, description: "Snapshots of the profile, newest first" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 422: errorResponse("id param, limit, or cursor query failed validation"), 500: errorResponse("Database error") } @@ -564,16 +493,10 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to load developer history"), - error?.code === "INVALID_CURSOR" ? 422 : 500 + listErrorStatus(error) ); } - return c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + return c.json(listPayload(data), 200); }); // Queue totals behind the admin tabs. Two small aggregate queries rather @@ -609,11 +532,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { }, description: "Pending/decided totals per queue" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 500: errorResponse("Database error") } }); diff --git a/src/services/extensions/v2/routes/notify.ts b/src/services/extensions/v2/routes/notify.ts new file mode 100644 index 00000000..c1de2eac --- /dev/null +++ b/src/services/extensions/v2/routes/notify.ts @@ -0,0 +1,32 @@ +import type { ExtensionsDb } from "../../../../lib/db"; +import { getPlatform } from "../../../../lib/middleware"; +import type { Context } from "hono"; +import { + notifyRequested, + sendModerationNotification, + type ModerationNotifyInput +} from "../email/notify"; +import { revalidateCatalogue } from "../revalidate"; + +// Shared tail of routes that notify authors after moderation writes: purge +// the CDN-cached catalogue pages, then — unless ?notify=false opted out — +// dispatch the author notification off the response path. Returns the +// `notified` flag the routes report in their response bodies. +// (moderatorCorrect also mutates but deliberately sends no email, so it +// calls revalidateCatalogue() directly instead of this helper.) +// +// A shared route-layer helper rather than a register*Routes module, like +// routes/errors.ts; it touches the Hono context and revalidateCatalogue, so +// it deliberately lives beside its call sites rather than in email/. +export async function notifyAuthor( + c: Context, + extDb: ExtensionsDb, + query: { notify?: string }, + input: ModerationNotifyInput +): Promise { + revalidateCatalogue(c); + if (!notifyRequested(query)) return false; + return sendModerationNotification(getPlatform(c), extDb, input, (p) => + c.executionCtx.waitUntil(p) + ); +} diff --git a/src/services/extensions/v2/routes/owner-extensions.ts b/src/services/extensions/v2/routes/owner-extensions.ts index 3ce10264..028e2571 100644 --- a/src/services/extensions/v2/routes/owner-extensions.ts +++ b/src/services/extensions/v2/routes/owner-extensions.ts @@ -2,6 +2,8 @@ import type { Context } from "hono"; import type { DatabaseResult } from "../../../../lib/interfaces"; import { errorBody, + listErrorStatus, + listPayload, setContentRetryAfter, statusFromContentCreateError, statusFromContentWriteError, @@ -15,6 +17,7 @@ import { ActiveAccountRequiredResponse, IdParamSchema, PaginationSchema, + authErrorResponses, errorResponse } from "../schemas/common"; import { @@ -88,7 +91,7 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { description: "Extension created. It holds the id immediately but stays out of the public catalogue until a moderator approves the revision." }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -176,7 +179,7 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { description: "Edit accepted as a pending revision. The published content is unchanged until a moderator approves it." }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -240,7 +243,7 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { }, description: "Extension and its revisions deleted, and the id released" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -288,7 +291,7 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { description: "Every version proposed for this extension, with its review outcome" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -329,16 +332,10 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to load revisions"), - error?.code === "INVALID_CURSOR" ? 422 : 500 + listErrorStatus(error) ); } - const res = c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + const res = c.json(listPayload(data), 200); res.headers.set("Vary", "Authorization"); return res; }); @@ -362,7 +359,7 @@ export function registerOwnerExtensionsRoutes(app: ExtensionsV2App): void { description: "One revision with its review outcome; content is null when compacted" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: diff --git a/src/services/extensions/v2/routes/ownership.ts b/src/services/extensions/v2/routes/ownership.ts index dc6814db..587386b7 100644 --- a/src/services/extensions/v2/routes/ownership.ts +++ b/src/services/extensions/v2/routes/ownership.ts @@ -5,6 +5,8 @@ import { getAuth } from "../../../../lib/auth"; import { createRoute, z } from "@hono/zod-openapi"; import { errorBody, + listErrorStatus, + listPayload, statusFromErrorCode, statusFromGithubErrorCode, statusFromWriteErrorCode @@ -12,10 +14,12 @@ import { import { ActiveAccountRequiredResponse, IdParamSchema, + ModeratorForbiddenResponse, NotifiedSchema, NotifyQuerySchema, PaginationSchema, ReviewNoteRequiredSchema, + authErrorResponses, errorResponse } from "../schemas/common"; import { DeveloperProfileSchema } from "../schemas/developers"; @@ -30,8 +34,7 @@ import { import { DeveloperClaimsDatabase } from "../db/developer-claims"; import { DeveloperTransfersDatabase } from "../db/developer-transfers"; import { UsersDatabase } from "../db/users"; -import { notifyRequested, sendModerationNotification } from "../email/notify"; -import { revalidateCatalogue } from "../revalidate"; +import { notifyAuthor } from "./notify"; import { ExtensionsV2App } from "./app"; export function registerOwnershipRoutes(app: ExtensionsV2App): void { @@ -57,7 +60,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { }, description: "Claim created and pending moderator review" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 404: errorResponse("No developer with that id"), 403: { ...ActiveAccountRequiredResponse, @@ -125,8 +128,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { }, description: "Claim withdrawn" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 404: errorResponse("No pending claim with that id owned by the caller"), 422: errorResponse("id param failed validation"), 500: errorResponse("Database error") @@ -169,8 +171,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { description: "Enriched claims with developer and claimant names in both scopes" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 422: errorResponse( "scope, status, limit, or cursor query failed validation" ), @@ -232,22 +233,11 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { ); if (error || !data) { return c.json( - { - error: { - message: error?.message ?? "Unable to load pending claims", - code: error?.code ?? "DATABASE_ERROR" - } - }, - error?.code === "INVALID_CURSOR" ? 422 : 500 + errorBody(error, "Unable to load pending claims"), + listErrorStatus(error) ); } - return c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + return c.json(listPayload(data), 200); } const { data, error } = await db.listScoped( { @@ -259,22 +249,11 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { ); if (error || !data) { return c.json( - { - error: { - message: error?.message ?? "Unable to load claims", - code: error?.code ?? "DATABASE_ERROR" - } - }, - error?.code === "INVALID_CURSOR" ? 422 : 500 + errorBody(error, "Unable to load claims"), + listErrorStatus(error) ); } - return c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + return c.json(listPayload(data), 200); }); const approveClaimRoute = createRoute({ @@ -301,11 +280,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { description: "Claim approved; profile ownership transferred to the claimant" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No claim or developer with that id"), 409: errorResponse( "Claim is no longer pending, profile is no longer unowned, or the claimant now owns a different profile" @@ -328,23 +303,14 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { statusFromWriteErrorCode(error?.code) ); } - revalidateCatalogue(c); - let notified = false; - if (notifyRequested(query)) { + const notified = await notifyAuthor(c, extDb, query, { + kind: "claim-approved", // approveClaim returns the pre-transfer claim snapshot it already // loaded (afterwards the profile row no longer records who the // claimant was), so no separate read is needed here. - notified = await sendModerationNotification( - getPlatform(c), - extDb, - { - kind: "claim-approved", - developerId: data.claim.developer_id, - claimantId: data.claim.claimant_id - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + developerId: data.claim.developer_id, + claimantId: data.claim.claimant_id + }); return c.json({ result: { ...data.profile, notified } }, 200); }); @@ -377,11 +343,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { }, description: "Claim rejected" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 404: errorResponse("No pending claim with that id"), 422: errorResponse( "id param, review_note body, or notify query failed validation" @@ -399,27 +361,20 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { const db = new DeveloperClaimsDatabase(extDb); const { data, error } = await db.rejectClaim(id, auth.userId, review_note); if (error || !data) { + // Deliberately not statusFromWriteErrorCode: rejectClaim cannot + // produce CONFLICT, and its contract declares no 409 for it. const status = error?.code === "FORBIDDEN" || error?.code === "ACCOUNT_INACTIVE" ? 403 : statusFromErrorCode(error?.code, false); return c.json(errorBody(error, "Unable to reject claim"), status); } - revalidateCatalogue(c); - let notified = false; - if (notifyRequested(query)) { - notified = await sendModerationNotification( - getPlatform(c), - extDb, - { - kind: "claim-rejected", - developerId: data.developer_id, - claimantId: data.claimant_id, - reason: review_note - }, - (p) => c.executionCtx.waitUntil(p) - ); - } + const notified = await notifyAuthor(c, extDb, query, { + kind: "claim-rejected", + developerId: data.developer_id, + claimantId: data.claimant_id, + reason: review_note + }); return c.json({ result: { ...data, notified } }, 200); }); @@ -441,7 +396,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { description: "Transfer token created; share it out-of-band with the recipient" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -489,7 +444,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { }, description: "Any pending transfer for this profile is revoked" }, - 401: errorResponse("Missing or invalid bearer token"), + ...authErrorResponses(), 403: { ...ActiveAccountRequiredResponse, description: @@ -539,8 +494,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { }, description: "Profile is now owned by the caller" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: ActiveAccountRequiredResponse, + ...authErrorResponses(), 404: errorResponse("Transfer link is invalid, already used, or expired"), 409: errorResponse("Caller already owns a different developer profile"), 422: errorResponse("token body failed validation"), diff --git a/src/services/extensions/v2/routes/public-extensions.ts b/src/services/extensions/v2/routes/public-extensions.ts index 2d8a856d..a5a4f3fd 100644 --- a/src/services/extensions/v2/routes/public-extensions.ts +++ b/src/services/extensions/v2/routes/public-extensions.ts @@ -1,6 +1,11 @@ import { getExtensionsDb } from "../../../../lib/db"; import { createRoute } from "@hono/zod-openapi"; -import { errorBody, statusFromContentReadError } from "./errors"; +import { + errorBody, + listErrorStatus, + listPayload, + statusFromContentReadError +} from "./errors"; import { IdParamSchema, errorResponse } from "../schemas/common"; import { UnifiedExtensionListQuerySchema, @@ -84,19 +89,10 @@ export function registerPublicExtensionsRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to load extensions"), - error?.code === "INVALID_CURSOR" ? 422 : 500 + listErrorStatus(error) ); } - const res = c.json( - { - result: data.items, - pagination: { - next_cursor: data.nextCursor, - has_more: data.hasMore - } - }, - 200 - ); + const res = c.json(listPayload(data), 200); if (!auth) { res.headers.set( "Cache-Control", @@ -159,7 +155,7 @@ export function registerPublicExtensionsRoutes(app: ExtensionsV2App): void { } if (!owner.data) { const res = c.json( - { result: [], pagination: { next_cursor: null, has_more: false } }, + listPayload({ items: [], nextCursor: null, hasMore: false }), 200 ); res.headers.set("Vary", "Authorization"); @@ -175,16 +171,10 @@ export function registerPublicExtensionsRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to load extensions"), - error?.code === "INVALID_CURSOR" ? 422 : 500 + listErrorStatus(error) ); } - const res = c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + const res = c.json(listPayload(data), 200); res.headers.set("Vary", "Authorization"); return res; } @@ -221,16 +211,10 @@ export function registerPublicExtensionsRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to load extensions"), - error?.code === "INVALID_CURSOR" ? 422 : 500 + listErrorStatus(error) ); } - const res = c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + const res = c.json(listPayload(data), 200); res.headers.set("Vary", "Authorization"); return res; }); diff --git a/src/services/extensions/v2/routes/revisions.ts b/src/services/extensions/v2/routes/revisions.ts index 26b5d132..2222d613 100644 --- a/src/services/extensions/v2/routes/revisions.ts +++ b/src/services/extensions/v2/routes/revisions.ts @@ -1,10 +1,11 @@ import { getAuth } from "../../../../lib/auth"; import { getExtensionsDb } from "../../../../lib/db"; import { createRoute, z } from "@hono/zod-openapi"; -import { errorBody } from "./errors"; +import { errorBody, listErrorStatus, listPayload } from "./errors"; import { - ActiveAccountRequiredResponse, + ModeratorForbiddenResponse, PaginationSchema, + authErrorResponses, errorResponse } from "../schemas/common"; import { @@ -37,11 +38,7 @@ export function registerRevisionRoutes(app: ExtensionsV2App): void { description: "Revisions matching the requested status (default: pending), oldest first" }, - 401: errorResponse("Missing or invalid bearer token"), - 403: { - ...ActiveAccountRequiredResponse, - description: "The account is inactive or the caller is not a moderator" - }, + ...authErrorResponses(ModeratorForbiddenResponse), 422: errorResponse( "Status, limit, or cursor query param failed validation" ), @@ -64,16 +61,10 @@ export function registerRevisionRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to load queue"), - error?.code === "INVALID_CURSOR" ? 422 : 500 + listErrorStatus(error) ); } - const res = c.json( - { - result: data.items, - pagination: { next_cursor: data.nextCursor, has_more: data.hasMore } - }, - 200 - ); + const res = c.json(listPayload(data), 200); res.headers.set("Vary", "Authorization"); return res; }); diff --git a/src/services/extensions/v2/schemas/common.ts b/src/services/extensions/v2/schemas/common.ts index fdbd5fc2..c709a03d 100644 --- a/src/services/extensions/v2/schemas/common.ts +++ b/src/services/extensions/v2/schemas/common.ts @@ -52,6 +52,30 @@ export const ActiveAccountRequiredResponse = errorResponse( "The bearer is valid but the account is inactive" ); +// The 403 wording shared by the moderator routes: the same middleware check, +// but the answer distinguishes a deactivated account from a non-moderator. +export const ModeratorForbiddenResponse = errorResponse( + "The account is inactive or the caller is not a moderator" +); + +// The bearer-auth error entries shared by every authenticated route: +// `401` is constant, `403` is the active-account shape with a per-route +// description. Spread into a route's responses map: +// ...authErrorResponses(ModeratorForbiddenResponse) +// +// Keep the plain return: adding `as const` here compiles for the helper but +// breaks handler response typing on every route that spreads it (the +// per-status response union zod-openapi derives from the responses map +// collapses; verified empirically). If you touch this, re-run tsc. +export function authErrorResponses( + forbidden: ReturnType = ActiveAccountRequiredResponse +) { + return { + 401: errorResponse("Missing or invalid bearer token"), + 403: forbidden + }; +} + export const IdParamSchema = z.object({ id: z.string().openapi({ param: { name: "id", in: "path" }, @@ -123,7 +147,12 @@ export const NotifiedSchema = z // 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({ + +// The limit/cursor query fields every paginated listing shares. Spread into +// each query schema's z.object rather than extended from +// CursorPaginationQuerySchema, which is strictObject - extending would +// silently tighten the other routes' unknown-param handling. +export const cursorPaginationFields = { limit: z.coerce .number() .int() @@ -131,9 +160,9 @@ export const CursorPaginationQuerySchema = z.strictObject({ .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. + // min(1): without it `?cursor=` arrives as an empty string, which the page + // helper treats as "no cursor" and silently restarts pagination instead of + // reporting the malformed value. cursor: z .string() .min(1) @@ -143,4 +172,8 @@ export const CursorPaginationQuerySchema = z.strictObject({ param: { name: "cursor", in: "query" }, description: "Opaque cursor returned by the previous page" }) +}; + +export const CursorPaginationQuerySchema = z.strictObject({ + ...cursorPaginationFields }); diff --git a/src/services/extensions/v2/schemas/developers.ts b/src/services/extensions/v2/schemas/developers.ts index bdf8cb10..413d9d47 100644 --- a/src/services/extensions/v2/schemas/developers.ts +++ b/src/services/extensions/v2/schemas/developers.ts @@ -65,7 +65,7 @@ export const DeveloperProfileSchema = DeveloperSchema.extend({ // GitHub-API-free by design). github_url_verified: z.boolean().optional(), // Only populated by the moderator listing (see - // DeveloperProfilesDatabase.listWithOwnerPaged) — other DeveloperProfile + // DeveloperProfilesDatabase.listScoped) — other DeveloperProfile // producers (getById, create/update/claim/transfer results) don't join // for it, so it's absent rather than null there. `unclaimed` is the // authoritative "has an owner" signal (owner_user_id IS NULL) — don't diff --git a/src/services/extensions/v2/schemas/extensions.ts b/src/services/extensions/v2/schemas/extensions.ts index 436abfcf..6ebf55b1 100644 --- a/src/services/extensions/v2/schemas/extensions.ts +++ b/src/services/extensions/v2/schemas/extensions.ts @@ -1,7 +1,12 @@ import { MAX_CONTENT_BYTES } from "../resource-limits"; import { z } from "@hono/zod-openapi"; import SPDX_LICENSE_IDS from "spdx-license-ids/index.json"; -import { httpUrl, lowercaseId, PaginationSchema } from "./common"; +import { + cursorPaginationFields, + httpUrl, + lowercaseId, + PaginationSchema +} from "./common"; import { PublicDeveloperSchema } from "./developers"; export const EXTENSION_TYPES = [ @@ -37,13 +42,9 @@ export const RepositorySchema = z export type Repository = z.infer; -// Re-exported so callers (and tests) validate against the exact same set -// this schema uses, rather than a hand-copied list that can drift. // `spdx-license-ids` ships only the current (non-deprecated) identifiers — // https://github.com/jslicense/spdx-license-ids — so submitters are steered // toward the license SPDX currently recommends, not a retired alias. -export { SPDX_LICENSE_IDS }; - const spdxLicenseId = () => z .string() @@ -267,22 +268,7 @@ export const ExtensionListQuerySchema = z.object({ .openapi({ param: { name: "developer_id", in: "query" } }), - limit: z.coerce - .number() - .int() - .min(1) - .max(100) - .default(50) - .openapi({ param: { name: "limit", in: "query" } }), - cursor: z - .string() - .min(1) - .max(1000) - .optional() - .openapi({ - param: { name: "cursor", in: "query" }, - description: "Opaque cursor returned by the previous page" - }) + ...cursorPaginationFields }); // Unified list query for the merged GET /extensions (optional auth, @@ -329,20 +315,6 @@ export const UnifiedExtensionListQuerySchema = ExtensionListQuerySchema.extend({ }) }); -export const ExtensionListResponseSchema = z - .object({ - result: z.array(ExtensionListItemSchema), - pagination: PaginationSchema - }) - .openapi("ExtensionListResponse"); - -export const OwnedExtensionListResponseSchema = z - .object({ - result: z.array(OwnedExtensionListItemSchema), - pagination: PaginationSchema - }) - .openapi("OwnedExtensionListResponse"); - // Merged GET /extensions returns one of the two list shapes depending on // ?scope=. A union (not two routes) because the only caller is controlled; // public readers narrow on scope=public, owner/moderator tooling on diff --git a/src/services/extensions/v2/schemas/revisions.ts b/src/services/extensions/v2/schemas/revisions.ts index 96b47db2..99f03b3d 100644 --- a/src/services/extensions/v2/schemas/revisions.ts +++ b/src/services/extensions/v2/schemas/revisions.ts @@ -1,4 +1,5 @@ import { z } from "@hono/zod-openapi"; +import { cursorPaginationFields } from "./common"; import { StoredExtensionContentSchema } from "./extensions"; export const RevisionStatusSchema = z.enum(["pending", "approved", "rejected"]); @@ -53,18 +54,13 @@ export const RevisionQueueQuerySchema = z.object({ status: RevisionStatusSchema.optional().openapi({ param: { name: "status", in: "query" } }), - 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 treats as "no cursor" and silently - // restarts pagination instead of reporting the malformed value. + ...cursorPaginationFields, + // Deliberate restatement of just the cursor field: unlike /extensions and + // the moderator list, this route's cursor param has never carried an + // OpenAPI description, and the generated document is kept byte-level + // stable. Validation stays in lockstep with the shared fragment (zod + // strings, min 1 so `?cursor=` is rejected rather than treated as "no + // cursor"); only the param metadata differs. cursor: z .string() .min(1) diff --git a/src/lib/adapters/cloudflare/preview-github-budget.ts b/src/services/previews/v1/budget.ts similarity index 100% rename from src/lib/adapters/cloudflare/preview-github-budget.ts rename to src/services/previews/v1/budget.ts diff --git a/src/services/previews/v1/cache.ts b/src/services/previews/v1/cache.ts index 0a9f80a4..a340434a 100644 --- a/src/services/previews/v1/cache.ts +++ b/src/services/previews/v1/cache.ts @@ -22,6 +22,31 @@ const KV_MIN_TTL_SECONDS = 60; const NEGATIVE_CACHE_TTL_SECONDS = KV_MIN_TTL_SECONDS; const NEGATIVE_CACHE_VALUE = "__not_found__"; +// Shared read half of cachedLookup: negative sentinel, positive parse, +// corrupt-entry fallthrough to miss. The result is tagged rather than a +// `T | null | "miss"` union so no cached value can collide with a sentinel +// (a cached JSON `null` must not read as negative, and a cached "miss" +// string must still be a hit). Routes with asymmetric write rules (main's +// download path must never write the shared entry it reads) reuse this for +// their reads while keeping their own write policy. +export type CachedRead = + { status: "hit"; value: T } | { status: "negative" } | { status: "miss" }; + +export async function readCachedValue( + kv: KVNamespace, + key: string +): Promise> { + const cached = await kv.get(key); + if (cached === NEGATIVE_CACHE_VALUE) return { status: "negative" }; + if (cached === null) return { status: "miss" }; + try { + return { status: "hit", value: JSON.parse(cached) as T }; + } catch { + // Corrupt cache entry - treat as absent. + return { status: "miss" }; + } +} + // ttlSeconds may be a function of the resolved data instead of a fixed // number - see routes/commit.ts, which caps the cache lifetime at the // artifact's own remaining GitHub retention so a lookup resolved just @@ -40,17 +65,9 @@ export async function cachedLookup( ttlSeconds: number | ((data: T) => number) = DEFAULT_CACHE_TTL_SECONDS, waitUntil?: (promise: Promise) => void ): Promise> { - const cached = await kv.get(key); - if (cached === NEGATIVE_CACHE_VALUE) { - return { status: "not_found" }; - } - if (cached !== null) { - try { - return { status: "found", data: JSON.parse(cached) as T }; - } catch { - // Corrupt cache entry - fall through to a fresh resolve. - } - } + const read = await readCachedValue(kv, key); + if (read.status === "hit") return { status: "found", data: read.value }; + if (read.status === "negative") return { status: "not_found" }; // The KV write runs inside the flight, not per joiner: singleFlight only // coalesces the resolve, so without this, N concurrent requests arriving diff --git a/src/services/previews/v1/github/artifacts.ts b/src/services/previews/v1/github/artifacts.ts index 38a10a15..27dfd6c2 100644 --- a/src/services/previews/v1/github/artifacts.ts +++ b/src/services/previews/v1/github/artifacts.ts @@ -59,7 +59,16 @@ function unavailable( error: unknown, url: string ): GithubLookupResult { - const githubError = classifyGitHubError(error, url); + return unavailableWith(context, classifyGitHubError(error, url)); +} + +// Overload for call sites that already classified the error while checking +// for a specific class (e.g. NotFoundError) - classification is pure, but +// there is no reason to run it twice. +function unavailableWith( + context: string, + githubError: GitHubError +): GithubLookupResult { logWarn("previews", `${context} unavailable`, { message: githubError.message, httpStatus: githubError.httpStatus @@ -321,7 +330,7 @@ export async function resolvePullRequestHeadSha( } catch (error) { const githubError = classifyGitHubError(error, url); if (githubError instanceof NotFoundError) return { status: "not_found" }; - return unavailable("Pull request lookup", error, url); + return unavailableWith("Pull request lookup", githubError); } } @@ -358,6 +367,6 @@ export async function getArtifactDownloadUrl( } catch (error) { const githubError = classifyGitHubError(error, url); if (githubError instanceof NotFoundError) return { status: "not_found" }; - return unavailable("Artifact download redirect", error, url); + return unavailableWith("Artifact download redirect", githubError); } } diff --git a/src/services/previews/v1/github/request.ts b/src/services/previews/v1/github/request.ts index e2043358..0a2f4bd4 100644 --- a/src/services/previews/v1/github/request.ts +++ b/src/services/previews/v1/github/request.ts @@ -1,6 +1,7 @@ import { Context } from "hono"; import { request } from "@octokit/request"; import { GitHubError } from "../../../../lib/github-errors"; +import { sha256Hex } from "../../../../lib/hash"; export interface PreviewGitHub { token: string; @@ -11,6 +12,11 @@ export interface PreviewGitHub { // Eight calls preserve ordinary fork-PR lookups and the page-six fallback. const MAX_REQUEST_SUBREQUESTS = 8; +// Deliberate abuse-resistance and privacy, not speculative machinery: the +// bucket key derives from the edge-supplied client IP so (a) an attacker +// rotating addresses inside one IPv6 /64 shares a single allowance instead +// of minting a fresh bucket per address, and (b) DO storage rows hold only +// SHA-256 digests of the derived identity, never raw client IPs. function clientBudgetIdentity(address: string | undefined): string { if (!address) return "unknown"; if (!address.includes(":")) { @@ -55,7 +61,7 @@ export function previewGitHub( ): PreviewGitHub { // CF supplies this header at the edge; never trust X-Forwarded-For. // Missing addresses share a conservative allowance. - const client = clientBudgetIdentity(c.req.header("CF-Connecting-IP")); + const identity = clientBudgetIdentity(c.req.header("CF-Connecting-IP")); let remaining = MAX_REQUEST_SUBREQUESTS; return { token: c.env.GITHUB_TOKEN, @@ -63,14 +69,9 @@ export function previewGitHub( if (remaining === 0) return false; // Consume before awaiting so concurrent calls cannot exceed the ceiling. remaining--; - const digest = await crypto.subtle.digest( - "SHA-256", - new TextEncoder().encode(client) + return c.env.PREVIEW_GITHUB_BUDGET.getByName("previews").reserve( + await sha256Hex(identity) ); - const key = Array.from(new Uint8Array(digest), (b) => - b.toString(16).padStart(2, "0") - ).join(""); - return c.env.PREVIEW_GITHUB_BUDGET.getByName("previews").reserve(key); } }; } diff --git a/src/services/previews/v1/routes/main.ts b/src/services/previews/v1/routes/main.ts index 96e5d50a..e0b23d3d 100644 --- a/src/services/previews/v1/routes/main.ts +++ b/src/services/previews/v1/routes/main.ts @@ -9,17 +9,19 @@ import { import { getMainPreviewObject, MainPreviewObject } from "../r2"; import { findPreviewArtifactByCommitSha } from "../github/artifacts"; import { singleFlight } from "../../../../lib/cache"; +import { cachedLookup, readCachedValue } from "../cache"; import { notFoundBody } from "./errors"; import { PreviewsV1App } from "./app"; +// One shared cache entry for both routes: /main stores the enriched body, +// and /main/download stores the same shape with enrichment left null (the +// fields are optional and enrichment is best-effort by contract). Two +// differently-shaped or independently-negative entries could disagree - +// one route 404ing or serving a stale URL while the other resolves fine - +// which a single shared entry rules out by construction. cachedLookup()'s +// shared negative sentinel and TTL apply (see cache.ts). const MAIN_CACHE_KEY = "preview:main"; const MAIN_CACHE_TTL_SECONDS = 60; -// Both routes 404 while no main preview has been published - cache that -// state briefly so the 404 path costs a KV read instead of an R2 head per -// request. An R2 miss is a stable signal (the object only exists once CI -// uploads it), so a 60s window is safe. -const MAIN_NEGATIVE_CACHE_TTL_SECONDS = 60; -const MAIN_NEGATIVE_CACHE_VALUE = "__main_preview_missing__"; // Enrichment only - run_id/artifact_id/created_at/expires_at come from // that commit's GitHub Actions artifact when resolvable. A miss for any @@ -89,90 +91,57 @@ function buildMainPreview( async function resolveMainPreview( c: Context<{ Bindings: CloudflareBindings }> ): Promise { - const waitUntil = (p: Promise) => c.executionCtx.waitUntil(p); - - const cached = await c.env.CACHE_KV.get(MAIN_CACHE_KEY); - if (cached === MAIN_NEGATIVE_CACHE_VALUE) { - return null; - } - if (cached) { - try { - return JSON.parse(cached) as MainPreview; - } catch { - // Corrupt cache entry - fall through to a fresh R2 lookup, matching - // cachedLookup()'s handling of the same situation. - } - } - - const object = await resolveMainObject(c); - if (!object) { - waitUntil( - c.env.CACHE_KV.put(MAIN_CACHE_KEY, MAIN_NEGATIVE_CACHE_VALUE, { - expirationTtl: MAIN_NEGATIVE_CACHE_TTL_SECONDS - }) - ); - return null; - } - - const artifactFields = await (object.commitSha - ? // Enrichment is keyed by commit: concurrent cold requests share one - // GitHub lookup (charged once to the shared budget) without mixing - // SHAs, and the R2 head above is already single-flighted so racing - // requests observe the same object. - singleFlight(`previews:main:enrich:${object.commitSha}`, () => - resolveArtifactFields(previewGitHub(c), object.commitSha) - ) - : resolveArtifactFields(previewGitHub(c), null)); - - const result = buildMainPreview(object, artifactFields); - - waitUntil( - c.env.CACHE_KV.put(MAIN_CACHE_KEY, JSON.stringify(result), { - expirationTtl: MAIN_CACHE_TTL_SECONDS - }) + const result = await cachedLookup( + c.env.CACHE_KV, + MAIN_CACHE_KEY, + async () => { + const object = await resolveMainObject(c); + if (!object) { + return { status: "not_found" as const }; + } + const artifactFields = await (object.commitSha + ? // Enrichment is keyed by commit: concurrent cold requests share one + // GitHub lookup (charged once to the shared budget) without mixing + // SHAs, and the R2 head above is already single-flighted so racing + // requests observe the same object. + singleFlight(`previews:main:enrich:${object.commitSha}`, () => + resolveArtifactFields(previewGitHub(c), object.commitSha) + ) + : resolveArtifactFields(previewGitHub(c), null)); + return { + status: "found" as const, + data: buildMainPreview(object, artifactFields) + }; + }, + MAIN_CACHE_TTL_SECONDS, + (p) => c.executionCtx.waitUntil(p) ); - - return result; + return result.status === "found" ? result.data : null; } // Shared by /main/download - the fixed download URL, no GitHub enrichment. async function resolveMainDownloadUrl( c: Context<{ Bindings: CloudflareBindings }> ): Promise { - const waitUntil = (p: Promise) => c.executionCtx.waitUntil(p); - - const cached = await c.env.CACHE_KV.get(MAIN_CACHE_KEY); - if (cached === MAIN_NEGATIVE_CACHE_VALUE) { - return null; - } - if (cached) { - try { - const parsed = JSON.parse(cached) as MainPreview; - if (parsed.download_url) return parsed.download_url; - } catch { - // Corrupt cache entry - fall through to a fresh R2 lookup. - } - } - + const read = await readCachedValue( + c.env.CACHE_KV, + MAIN_CACHE_KEY + ); + if (read.status === "negative") return null; // authoritative negative + // The `?.` also covers a cached literal `null` (valid JSON written by + // something other than cachedLookup): treat it as absent and fall through + // to the R2 head rather than throwing on the missing URL. + if (read.status === "hit" && read.value?.download_url) + return read.value.download_url; + + // No write of any kind here, by design: /main owns the shared entry, and + // an unenriched (or negative) body written from this route would silently + // clobber a concurrently-cached enriched one. The cost of not warming + // from the download path is one single-flighted R2 head per cold download + // request - rare next to the embed traffic that hits /main and populates + // the entry. const object = await resolveMainObject(c); - if (!object) { - // The R2 miss is single-flighted, so every concurrent route saw the - // same outcome and this negative write can't contradict a concurrent - // positive one. - waitUntil( - c.env.CACHE_KV.put(MAIN_CACHE_KEY, MAIN_NEGATIVE_CACHE_VALUE, { - expirationTtl: MAIN_NEGATIVE_CACHE_TTL_SECONDS - }) - ); - return null; - } - - // No positive write here, by design: /main owns the shared entry, and an - // unenriched body written from this route would silently clobber a - // concurrently-cached enriched one. The cost of not warming from the - // download path is one single-flighted R2 head per cold download request - - // rare next to the embed traffic that hits /main and populates the entry. - return object.downloadUrl; + return object?.downloadUrl ?? null; } export function registerMainRoutes(app: PreviewsV1App): void { diff --git a/src/services/stats/v1/dashboard.ts b/src/services/stats/v1/dashboard.ts new file mode 100644 index 00000000..9d5bc537 --- /dev/null +++ b/src/services/stats/v1/dashboard.ts @@ -0,0 +1,366 @@ +// The /stats/v1 dashboard: a static HTML page that fetches /stats/v1/data +// and renders it with Chart.js. Kept as its own module so index.ts stays +// reviewable API code; the JS inside the template literal is client-side +// and intentionally unlinted. +export const STATS_DASHBOARD_HTML = ` + + + + + FOSSBilling Release Statistics + + + + + +
+

FOSSBilling Release Statistics

+
Loading statistics...
+ + +
+ + + +`; diff --git a/src/services/stats/v1/index.ts b/src/services/stats/v1/index.ts index 62f330b0..266bf5e8 100644 --- a/src/services/stats/v1/index.ts +++ b/src/services/stats/v1/index.ts @@ -4,9 +4,15 @@ import { etag } from "hono/etag"; import { prettyJSON } from "hono/pretty-json"; import { trimTrailingSlash } from "hono/trailing-slash"; import { compare as semverCompare } from "semver"; -import { getReleases, RELEASE_CACHE_KEY } from "../../versions/v1/index"; +import { getReleases } from "../../versions/v1/index"; import { Releases } from "../../versions/v1/interfaces"; +import { + buildSuccessResponse, + buildUnavailableResponse, + hasNoReleases +} from "../../versions/v1/responses"; import { StatsData, ReleasesPerYearData } from "./interfaces"; +import { STATS_DASHBOARD_HTML } from "./dashboard"; import { getPlatform } from "../../../lib/middleware"; import { ICache } from "../../../lib/interfaces"; import { publicCacheKey, publicResponseCache } from "../../../lib/cache"; @@ -49,34 +55,6 @@ function registerCachedRoute

( ); } -function hasNoReleases(releases: Releases): boolean { - return Object.keys(releases).length === 0; -} - -function buildSuccessResponse( - result: T, - source: "cache" | "fresh" | "stale" -) { - return { - result, - error_code: 0, - message: null, - stale: source === "stale" - }; -} - -function buildUnavailableResponse(error: GitHubError) { - return { - result: null, - error_code: 503, - message: "Unable to fetch releases and no cached data available", - details: { - http_status: error.httpStatus, - error_code: error.errorCode - } - }; -} - function parseVersionLine(version: string): string { const parts = version.split("."); if (parts.length >= 2) { @@ -144,32 +122,50 @@ async function getStats( cache: ICache, githubToken: string, downloadBucket: R2Bucket, - updateCache: boolean = false, waitUntil?: (promise: Promise) => void ): Promise<{ stats: StatsData; source: "cache" | "fresh" | "stale"; error?: GitHubError; }> { - // Both KV reads are needed on the cold path (stats value + the shared - // releases blob getReleases consumes), so issue them together instead of - // serializing two round trips on an already-slow request. Only the stats - // read may reject the whole call: a transient failure on the releases - // read degrades to "no pre-read" (getReleases does its own read, the - // pre-parallelization behavior) rather than discarding a valid cached - // stats value behind it. - const [cachedStats, cachedReleases] = await Promise.all([ - cache.get(STATS_CACHE_KEY), - cache.get(RELEASE_CACHE_KEY).catch(() => undefined) - ]); + // Kick off the stats read before getReleases so the two KV reads + // (stats value + the shared releases blob getReleases consumes) overlap + // on the cold path instead of serializing. A failed stats read degrades + // to a cache miss - getReleases' own read is untouched by it. getReleases + // itself is deferred below so that a valid stats hit is served even when + // the releases read rejects (getReleases only rejects on its own KV read, + // never on GitHub failures - those come back as error results). + const cachedStatsPromise = cache.get(STATS_CACHE_KEY).catch(() => undefined); + + let releases: Awaited> | null = null; + let releasesFailure: unknown = null; + try { + // getReleases shares its cache with the versions service (same + // RELEASE_CACHE_KEY), so a fresh fetch here must still resolve R2 + // download_url/digest - otherwise a stats-triggered refresh would + // overwrite that cache with GitHub-only URLs for up to a day. + releases = await getReleases( + cache, + githubToken, + downloadBucket, + false, + waitUntil + ); + } catch (error) { + // Held until we know whether a stats cache hit can serve instead. + releasesFailure = error; + } + const cachedStats = await cachedStatsPromise; - if (cachedStats && !updateCache) { + if (cachedStats) { try { const parsedCache = JSON.parse(cachedStats); if (parsedCache && typeof parsedCache === "object") { logInfo("stats", "Serving stats from cache", { cacheKey: STATS_CACHE_KEY }); + // Served even when the releases read above failed: a cached value + // in hand beats rethrowing a KV failure. return { stats: parsedCache as StatsData, source: "cache" @@ -184,19 +180,11 @@ async function getStats( } } - // getReleases shares its cache with the versions service (same - // RELEASE_CACHE_KEY), so a fresh fetch here must still resolve R2 - // download_url/digest - otherwise a stats-triggered refresh would - // overwrite that cache with GitHub-only URLs for up to a day. The - // pre-read value from above is threaded through so it isn't fetched twice. - const result = await getReleases( - cache, - githubToken, - downloadBucket, - updateCache, - waitUntil, - cachedReleases - ); + // No usable stats cache - now the releases failure (if any) is the answer. + if (releasesFailure !== null) { + throw releasesFailure; + } + const result = releases!; if (hasNoReleases(result.releases) && result.error) { return { @@ -237,7 +225,6 @@ registerCachedRoute("/data", async (c) => { platform.getCache("CACHE_KV"), platform.getEnv("GITHUB_TOKEN") || "", c.env.DOWNLOAD_BUCKET, - false, (p) => c.executionCtx.waitUntil(p) ); @@ -249,370 +236,7 @@ registerCachedRoute("/data", async (c) => { }); registerCachedRoute("/", async (c) => { - const html = ` - - - - - FOSSBilling Release Statistics - - - - - -

-

FOSSBilling Release Statistics

-
Loading statistics...
- - -
- - - -`; - - return c.html(html); + return c.html(STATS_DASHBOARD_HTML); }); export default statsV1; diff --git a/src/services/stats/v1/interfaces.ts b/src/services/stats/v1/interfaces.ts index f5418a7d..7cc4be96 100644 --- a/src/services/stats/v1/interfaces.ts +++ b/src/services/stats/v1/interfaces.ts @@ -26,17 +26,3 @@ export interface ReleasesPerYearData { year: string; release_count: number; } - -export interface ChartDataset { - label: string; - data: number[]; - borderColor?: string; - backgroundColor?: string; - fill?: boolean; - tension?: number; -} - -export interface ChartData { - labels: string[]; - datasets: ChartDataset[]; -} diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index e3f78e60..740721c4 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -14,6 +14,13 @@ import { } from "semver"; import { Releases, ReleaseDetails, ResolvedReleaseDetails } from "./interfaces"; import { getReleaseR2Object, ReleaseR2Object } from "./r2"; +import { + buildSuccessResponse, + buildUnavailableResponse, + githubErrorDetails, + hasNoReleases, + ReleaseSource +} from "./responses"; import { getPlatform } from "../../../lib/middleware"; import { ICache } from "../../../lib/interfaces"; import { @@ -27,14 +34,14 @@ import { AuthError, RateLimitError, ValidationError, - classifyGitHubError, - getMostCriticalError + classifyGitHubError } from "../../../lib/github-errors"; const REPO_OWNER = "FOSSBilling"; const REPO_NAME = "FOSSBilling"; -// Shared with stats/v1, which reads the same blob and threads its own read -// into getReleases - exported so that contract stays defined in one place. +// Shared with stats/v1, which serves the same blob through getReleases and +// reads its own stats value from the same CACHE_KV - exported so the key is +// defined in one place. export const RELEASE_CACHE_KEY = "gh-fossbilling-releases"; // Client-side caching of these endpoints is immaterial (the real consumers // re-request per update check and don't cache), so this TTL is sized for the @@ -108,24 +115,18 @@ async function stampCacheHeaders( } interface CachedRouteOptions { - // Cache-key overrides forwarded to hono's cache middleware; default is - // the normalized URL. - keyGenerator?: (c: Context) => string; varyByMirrorTrust?: boolean; } function registerCachedRoute

( path: P, handler: Handler, - { - keyGenerator = publicCacheKey, - varyByMirrorTrust = false - }: CachedRouteOptions = {} + { varyByMirrorTrust = false }: CachedRouteOptions = {} ) { const responseCache = publicResponseCache({ cacheName: VERSIONS_CACHE_NAME, cacheControl: RELEASES_CACHE_CONTROL, - keyGenerator, + keyGenerator: publicCacheKey, ...(varyByMirrorTrust ? { vary: ["User-Agent"] } : {}) }); const boundedCache: MiddlewareHandler = async (c, next) => { @@ -179,34 +180,6 @@ async function loadReleases( ); } -function hasNoReleases(releases: Releases): boolean { - return Object.keys(releases).length === 0; -} - -function buildUnavailableResponse(error: GitHubError) { - return { - result: null, - error_code: 503, - message: "Unable to fetch releases and no cached data available", - details: { - http_status: error.httpStatus, - error_code: error.errorCode - } - }; -} - -function buildSuccessResponse( - result: T, - source: GetReleasesResult["source"] -) { - return { - result, - error_code: 0, - message: null, - stale: source === "stale" - }; -} - // FOSSBilling's own Update.php sends this on every request already (src/di.php, // added in 0.8.4), so no client change is needed to gate the R2 mirror by version. // Anything we can't confidently read a trusted version out of - no header at all @@ -345,10 +318,7 @@ versionsV1.get( result: null, error_code: 500, message: `Failed to fetch releases: ${result.error.message}`, - details: { - http_status: result.error.httpStatus, - error_code: result.error.errorCode - }, + details: githubErrorDetails(result.error), stale: result.source === "stale" }, 500 @@ -361,10 +331,7 @@ versionsV1.get( error_code: 0, message: result.error.message, warning: result.error.message, - details: { - http_status: result.error.httpStatus, - error_code: result.error.errorCode - }, + details: githubErrorDetails(result.error), stale: result.source === "stale" }); } @@ -439,18 +406,10 @@ registerCachedRoute("/count", async (c) => { registerCachedRoute( "/:version", async (c) => { + // A required :version param cannot be empty or the route would not match. const version = c.req.param("version"); let result = await loadReleases(c); - if (!version) { - c.status(400); - return c.json({ - result: null, - error_code: 400, - message: "Version parameter is required." - }); - } - let releases = result.releases; // Retry once on transient failures (network blips, empty responses). A rate @@ -485,11 +444,9 @@ registerCachedRoute( const userAgent = c.req.header("User-Agent"); if (version === "latest") { - const sortedKeys = Object.keys(releases).sort(semverCompare); - const lastKey = sortedKeys.at(-1); - const resolved = lastKey - ? resolveReleaseForClient(releases[lastKey], userAgent) - : null; + // hasNoReleases was excluded above, so the newest key always exists. + const lastKey = Object.keys(releases).sort(semverCompare).at(-1)!; + const resolved = resolveReleaseForClient(releases[lastKey], userAgent); return c.json(buildSuccessResponse(resolved, result.source)); } @@ -517,7 +474,7 @@ export default versionsV1; interface GetReleasesResult { releases: Releases; - source: "cache" | "fresh" | "stale"; + source: ReleaseSource; error?: GitHubError; } @@ -530,17 +487,9 @@ export async function getReleases( githubToken: string, downloadBucket: R2Bucket, updateCache: boolean = false, - waitUntil?: (promise: Promise) => void, - // Lets a caller that already read RELEASE_CACHE_KEY (stats' parallel - // cold-path read) thread the value through instead of forcing a second - // identical KV round trip. `null` means "known empty", distinct from - // undefined (no pre-read; fetch it). - preReadReleases?: string | null + waitUntil?: (promise: Promise) => void ): Promise { - const cachedReleases = - preReadReleases !== undefined - ? preReadReleases - : await cache.get(RELEASE_CACHE_KEY); + const cachedReleases = await cache.get(RELEASE_CACHE_KEY); // Parsed once and reused for the serve-from-cache short circuit below, // the PHP-version reuse, and the stale fallback - re-parsing the full @@ -736,10 +685,9 @@ export async function getReleases( ) ); - const sortedReleases = Object.fromEntries( + const releases = Object.fromEntries( releaseEntries.sort((a, b) => semverCompare(b[0], a[0])) ); - const releases = sortedReleases; if (Object.keys(releases).length > 0) { // The write is deliberately outside the GitHub try/catch: a KV @@ -767,15 +715,14 @@ export async function getReleases( else await writeCache; } - const mostCriticalError = getMostCriticalError(errors) || undefined; - + // Only non-validation errors are worth reporting upstream: a malformed + // composer blob skips that version but the rebuild itself succeeded. + // First match wins - the batch produces at most one non-validation + // failure in practice (a rate limit or network outage aborts the scan). return { releases, source: "fresh", - error: - mostCriticalError instanceof ValidationError - ? undefined - : mostCriticalError + error: errors.find((e) => !(e instanceof ValidationError)) }; } catch (error) { const githubError = classifyGitHubError(error, RELEASES_URL); @@ -829,30 +776,23 @@ export async function getReleases( }); } -// Parsing the full changelog-bearing blob is measurable on this route and -// the blob is byte-identical across requests until a rebuild refreshes KV, -// so memoize the parse keyed by the raw string rather than by time: a -// refreshed value simply misses the memo, and nothing can serve a stale -// parse of a changed blob. Tiny cap bounds memory; FIFO eviction is fine -// since at most two blobs (old + new) exist at any moment in practice. -const RELEASES_PARSE_MEMO_LIMIT = 4; -const releasesParseMemo = new Map(); +// Parsing the full changelog-bearing blob is measurable and the blob is +// byte-identical across sequential requests on a warm isolate until a +// rebuild refreshes KV, so keep exactly the last parse: a refreshed value +// simply misses and re-parses. A single slot covers the real workload - the +// old and new blob around one rebuild. +let lastRawReleases: string | null = null; +let lastParsedReleases: Releases | null = null; function parseCachedReleasesMemoized( cachedReleases: string, logMessage: string ): Releases | null { - const memoized = releasesParseMemo.get(cachedReleases); - if (memoized) return memoized; + if (cachedReleases === lastRawReleases) return lastParsedReleases; const parsed = parseCachedReleases(cachedReleases, logMessage); - if (parsed) { - if (releasesParseMemo.size >= RELEASES_PARSE_MEMO_LIMIT) { - const oldest = releasesParseMemo.keys().next().value; - if (oldest !== undefined) releasesParseMemo.delete(oldest); - } - releasesParseMemo.set(cachedReleases, parsed); - } + lastRawReleases = cachedReleases; + lastParsedReleases = parsed; return parsed; } diff --git a/src/services/versions/v1/responses.ts b/src/services/versions/v1/responses.ts new file mode 100644 index 00000000..7dd2f39a --- /dev/null +++ b/src/services/versions/v1/responses.ts @@ -0,0 +1,43 @@ +import { GitHubError } from "../../../lib/github-errors"; +import { Releases } from "./interfaces"; + +// The response envelope and release-blob helpers shared by versions/v1 and +// stats/v1, which serve the same cached blob through the same shape. Kept +// beside getReleases so the cross-service contract stays in one place. + +export type ReleaseSource = "cache" | "fresh" | "stale"; + +export function hasNoReleases(releases: Releases): boolean { + return Object.keys(releases).length === 0; +} + +// The details block GitHub errors carry into response envelopes. +export function githubErrorDetails(error: GitHubError) { + return { http_status: error.httpStatus, error_code: error.errorCode }; +} + +export function buildUnavailableResponse(error: GitHubError) { + return { + result: null, + error_code: 503, + message: "Unable to fetch releases and no cached data available", + details: githubErrorDetails(error) + }; +} + +export function buildSuccessResponse( + result: T, + source: ReleaseSource +): { + result: T; + error_code: 0; + message: null; + stale: boolean; +} { + return { + result, + error_code: 0, + message: null, + stale: source === "stale" + }; +} diff --git a/test/app/index.test.ts b/test/app/index.test.ts deleted file mode 100644 index b92dc20c..00000000 --- a/test/app/index.test.ts +++ /dev/null @@ -1,302 +0,0 @@ -import { isolateEdgeCache } from "../utils/isolate-edge-cache"; -/** - * Tests for FOSSBilling API Worker - Main Application - * - * @license AGPL-3.0 - */ - -import { describe, it, expect, beforeAll, beforeEach, vi } from "vitest"; -import { env } from "cloudflare:workers"; -import { - createExecutionContext, - waitOnExecutionContext -} from "cloudflare:test"; -import app from "../../src/app/index"; - -// Arbitrary credentials use the public cache; isolate entries between tests. -const BYPASS_CACHE = { authorization: "test-bypass-cache" } as const; -import { - ApiResponse, - CentralAlertsResponse, - GitHubContentResponse, - MockGitHubRequest, - VersionInfo, - VersionsResponse -} from "../utils/test-types"; -import { applyTestMigrations } from "../utils/apply-migrations"; - -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); - -vi.mock("@octokit/graphql", () => ({ - graphql: vi.fn() -})); - -import { request as ghRequest } from "@octokit/request"; -import { graphql } from "@octokit/graphql"; -import { resetUpdateTokenCache } from "../../src/services/versions/v1/index"; -import { createGraphQLImplementation } from "../utils/mock-helpers"; -import { MockGitHubGraphQL } from "../utils/test-types"; - -const mockGitHubReleases = [ - { - id: 1001, - tag_name: "0.6.0", - name: "0.6.0", - published_at: "2023-04-01T00:00:00Z", - prerelease: false, - body: "## 0.6.0\n- New features", - assets: [ - { - name: "FOSSBilling.zip", - browser_download_url: - "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip", - size: 1030000 - } - ] - } -]; - -const mockComposerJson = { - require: { - php: "^8.1" - } -}; - -const resetEdgeCache = isolateEdgeCache(); - -describe("FOSSBilling API Worker - Main App", () => { - beforeAll(applyTestMigrations); - - beforeEach(async () => { - // Clear KV cache before each test - await env.CACHE_KV.delete("gh-fossbilling-releases"); - resetUpdateTokenCache(); - - // Set up UPDATE_TOKEN in AUTH_KV storage for tests - const testUpdateToken = "test-update-token-12345"; - await env.AUTH_KV.put("UPDATE_TOKEN", testUpdateToken); - - // Reset all mocks - vi.clearAllMocks(); - resetEdgeCache(); - - // Setup default GitHub API mock responses - (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( - async (route: string) => { - if (route === "GET /repos/{owner}/{repo}/releases") { - return { data: mockGitHubReleases }; - } - if (route === "GET /repos/{owner}/{repo}/contents/{path}{?ref}") { - const content = btoa(JSON.stringify(mockComposerJson)); - return { - data: { - content: content - } - } as GitHubContentResponse; - } - throw new Error("Unexpected route"); - } - ); - - (vi.mocked(graphql) as unknown as MockGitHubGraphQL).mockImplementation( - createGraphQLImplementation(mockComposerJson) - ); - }); - - describe("Route Delegation", () => { - it("should route /versions/v1 to versions service", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data: VersionsResponse = await response.json(); - - // Should return versions API response format - expect(data).toHaveProperty("result"); - expect(data).toHaveProperty("error_code"); - expect(data).toHaveProperty("message"); - }); - - it("should route /central-alerts/v1/list to central alerts service", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/central-alerts/v1/list", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data: CentralAlertsResponse = await response.json(); - - // Should return central alerts response format - expect(data).toHaveProperty("result"); - expect(data.result).toHaveProperty("alerts"); - expect(Array.isArray(data.result.alerts)).toBe(true); - }); - - it("should return 404 for unknown routes", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/unknown/path", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(404); - }); - - it("should return 200 with notice for root path", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data: ApiResponse = await response.json(); - expect(data.result).toBe(null); - expect(data.error_code).toBe(0); - expect(data.message).toContain("FOSSBilling API"); - }); - }); - - describe("Context Storage Middleware", () => { - it("should make context available to nested routes", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - // If context storage works, the versions endpoint should be able to access env bindings - expect(response.status).toBe(200); - const data: VersionsResponse = await response.json(); - - // Should successfully fetch from GitHub (via context storage) - expect(data.result).toBeTruthy(); - }); - - it("should provide access to KV namespace through context", async () => { - const ctx = createExecutionContext(); - await app.request("/versions/v1", { headers: BYPASS_CACHE }, env, ctx); - await waitOnExecutionContext(ctx); - - // Verify that KV was accessed (data should be cached) - const cached = await env.CACHE_KV.get("gh-fossbilling-releases"); - expect(cached).toBeTruthy(); - }); - - it("should provide access to environment variables through context", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - // Should be able to access UPDATE_TOKEN from KV storage - expect(response.status).toBe(200); - }); - }); - - describe("Service Integration", () => { - it("should allow versions service to function correctly", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/latest", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data: ApiResponse = await response.json(); - - expect(data.result).toBeTruthy(); - if (!data.result) { - throw new Error("Expected version info in response"); - } - expect(data.result.version).toBe("0.6.0"); - }); - - it("should allow central alerts service to return static data", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/central-alerts/v1/list", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data: CentralAlertsResponse = await response.json(); - - expect(data.result.alerts.length).toBeGreaterThan(0); - }); - }); - - describe("HTTP Methods", () => { - it("should handle GET requests", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { - method: "GET" - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - }); - - it("should return 404 for unsupported methods on unknown routes", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { - method: "POST" - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - // Hono returns 404 for unsupported methods - expect(response.status).toBe(404); - }); - }); -}); diff --git a/test/env.d.ts b/test/env.d.ts index 01b8954f..48c5a596 100644 --- a/test/env.d.ts +++ b/test/env.d.ts @@ -5,7 +5,7 @@ declare module "cloudflare:test" { } // Test-only bindings injected by vitest.config.ts (via readD1Migrations) for -// test/apply-migrations.ts to apply - never part of the real deployed +// test/utils/apply-migrations.ts to apply - never part of the real deployed // Worker's bindings, so deliberately not in worker-configuration.d.ts. // `env` from "cloudflare:test" is typed as Cloudflare.Env (not ProvidedEnv), // hence augmenting this namespace directly rather than the module above. diff --git a/test/integration/app.test.ts b/test/integration/app.test.ts index 05f6535d..8a042670 100644 --- a/test/integration/app.test.ts +++ b/test/integration/app.test.ts @@ -17,18 +17,12 @@ import { CentralAlertsResponse, MockGitHubGraphQL, MockGitHubRequest, - VersionsResponse + VersionInfo } from "../utils/test-types"; -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); +vi.mock("@octokit/request", async () => + (await import("../mocks/octokit")).octokitRequestMock() +); vi.mock("@octokit/graphql", () => ({ graphql: vi.fn() @@ -91,12 +85,30 @@ describe("FOSSBilling API Worker - Full App Integration", () => { expect(alertsResponse.status).toBe(200); expect(statsResponse.status).toBe(200); - const versionsData = (await versionsResponse.json()) as VersionsResponse; + const versionsData = (await versionsResponse.json()) as ApiResponse< + Record + >; const alertsData = (await alertsResponse.json()) as CentralAlertsResponse; expect(versionsData).toHaveProperty("result"); expect(versionsData).toHaveProperty("error_code", 0); expect(alertsData).toHaveProperty("result"); + expect(alertsData.result.alerts.length).toBeGreaterThan(0); + }); + + it("resolves the latest release through the full middleware stack", async () => { + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: BYPASS_CACHE }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + + expect(response.status).toBe(200); + const data = (await response.json()) as ApiResponse; + expect(data.result?.version).toBe("0.6.0"); }); it("should return 404 for unknown routes", async () => { @@ -130,19 +142,11 @@ describe("FOSSBilling API Worker - Full App Integration", () => { }); }); + // The versions KV write is asserted once here for the whole app; the + // /update auth matrix lives in test/services/versions/v1 and the KV-rewrite + // flow in integration/versions. describe("Cross-Service Communication", () => { - it("should allow services to share cached data", async () => { - const ctx = createExecutionContext(); - await app.request("/versions/v1", { headers: BYPASS_CACHE }, env, ctx); - await waitOnExecutionContext(ctx); - - const cached = await env.CACHE_KV.get("gh-fossbilling-releases"); - expect(cached).toBeTruthy(); - }); - }); - - describe("Context Storage Middleware", () => { - it("should make environment bindings available to all services", async () => { + it("exposes environment bindings and the shared cache to all services", async () => { const ctx = createExecutionContext(); const response = await app.request( "/versions/v1", @@ -173,26 +177,15 @@ describe("FOSSBilling API Worker - Full App Integration", () => { expect(data.result.alerts).toBeInstanceOf(Array); }); - it("should handle UPDATE_TOKEN from KV storage", async () => { + it("rejects unauthenticated update requests", async () => { const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx - ); + const response = await app.request("/versions/v1/update", {}, env, ctx); await waitOnExecutionContext(ctx); - expect(response.status).toBe(200); + expect(response.status).toBe(401); }); - }); - describe("Error Handling Across Services", () => { - it("should handle 404 for invalid service routes", async () => { + it("returns 404 for invalid service routes", async () => { const endpoints = [ "/versions/v1/invalid-endpoint", "/central-alerts/v1/invalid" @@ -206,92 +199,31 @@ describe("FOSSBilling API Worker - Full App Integration", () => { expect(response.status).toBe(404); } }); - - it("should handle unauthorized update requests", async () => { - const ctx = createExecutionContext(); - const response = await app.request("/versions/v1/update", {}, env, ctx); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(401); - }); }); - describe("Consistent API Response Format", () => { - it("should maintain consistent response format across all services", async () => { - const endpoints = [ - { path: "/versions/v1", fields: ["result", "error_code", "message"] }, - { path: "/central-alerts/v1/list", fields: ["result"] } - ]; - - for (const { path, fields } of endpoints) { - const ctx = createExecutionContext(); - const response = await app.request(path, {}, env, ctx); - await waitOnExecutionContext(ctx); - - const data = await response.json(); - for (const field of fields) { - expect(data).toHaveProperty(field); + // Method-level behavior is Hono routing, already exercised by every + // request in this suite; the surviving assertions are the cross-service + // headers the wiring owns. + // The preflight answer is wiring (hono/cors): an OPTIONS request must be + // answered 204 with the open CORS policy before it reaches any service. + it("answers OPTIONS preflight requests with 204 and the open CORS policy", async () => { + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1", + { + method: "OPTIONS", + headers: { + Origin: "https://example.org", + "Access-Control-Request-Method": "GET" } - } - }); - }); - - describe("HTTP Method Handling", () => { - it("should handle GET requests across all services", async () => { - const endpoints = [ - "/versions/v1", - "/versions/v1/latest", - "/central-alerts/v1/list", - "/stats/v1/data", - "/stats/v1" - ]; - - for (const endpoint of endpoints) { - const ctx = createExecutionContext(); - const response = await app.request( - endpoint, - { method: "GET" }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect([200, 301]).toContain(response.status); - } - }); - - it("should return 404 for unsupported methods", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { method: "POST" }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(404); - }); - - it("should handle OPTIONS preflight requests", async () => { - const endpoints = ["/versions/v1"]; - - for (const endpoint of endpoints) { - const ctx = createExecutionContext(); - const response = await app.request( - endpoint, - { method: "OPTIONS" }, - env, - ctx - ); - await waitOnExecutionContext(ctx); + }, + env, + ctx + ); + await waitOnExecutionContext(ctx); - expect([204, 405]).toContain(response.status); - if (response.status === 204) { - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - } - } - }); + expect(response.status).toBe(204); + expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); }); describe("Headers and Middleware", () => { diff --git a/test/integration/versions/index.test.ts b/test/integration/versions/index.test.ts index 1a64384c..e705565f 100644 --- a/test/integration/versions/index.test.ts +++ b/test/integration/versions/index.test.ts @@ -16,19 +16,13 @@ import { setupGitHubApiMock } from "../../utils/mock-helpers"; import { MockGitHubGraphQL, MockGitHubRequest, - VersionsResponse, - ApiResponse + ApiResponse, + VersionInfo } from "../../utils/test-types"; -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); +vi.mock("@octokit/request", async () => + (await import("../../mocks/octokit")).octokitRequestMock() +); vi.mock("@octokit/graphql", () => ({ graphql: vi.fn() @@ -40,6 +34,12 @@ import { resetUpdateTokenCache } from "../../../src/services/versions/v1/index"; const resetEdgeCache = isolateEdgeCache(); +// This suite keeps the cross-cutting scenarios the service-level suite +// cannot express: the /update endpoint actually rewriting the shared KV +// blob, endpoint consistency against one blob, the refresh path changing +// that blob, and cold-start concurrency. Everything else (warm-cache +// serving, auth failures, retry semantics) is covered in +// test/services/versions/v1/. describe("Versions API v1 - Integration Tests", () => { beforeEach(async () => { await env.CACHE_KV.delete("gh-fossbilling-releases"); @@ -56,117 +56,143 @@ describe("Versions API v1 - Integration Tests", () => { ); }); - describe("Full Request/Response Cycle", () => { - it("should handle complete flow from request to cached response", async () => { - const ctx1 = createExecutionContext(); - const response1 = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); - - expect(response1.status).toBe(200); - const data1: VersionsResponse = await response1.json(); - expect(Object.keys(data1.result).length).toBeGreaterThan(0); - - const cached = await env.CACHE_KV.get("gh-fossbilling-releases"); - expect(cached).toBeTruthy(); - - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); + it("rewrites the shared KV blob via the authenticated update endpoint", async () => { + let cached = await env.CACHE_KV.get("gh-fossbilling-releases"); + expect(cached).toBeFalsy(); - expect(response2.status).toBe(200); - const data2: VersionsResponse = await response2.json(); - expect(data1).toEqual(data2); - }); + const ctx1 = createExecutionContext(); + const response1 = await app.request( + "/versions/v1/update", + { + headers: { + Authorization: "Bearer test-update-token-12345" + } + }, + env, + ctx1 + ); + await waitOnExecutionContext(ctx1); - it("should handle update flow with authentication", async () => { - let cached = await env.CACHE_KV.get("gh-fossbilling-releases"); - expect(cached).toBeFalsy(); + expect(response1.status).toBe(200); + const data1 = (await response1.json()) as ApiResponse; + expect(data1.result).toContain("updated successfully"); - const ctx1 = createExecutionContext(); - const response1 = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); + cached = await env.CACHE_KV.get("gh-fossbilling-releases"); + expect(cached).toBeTruthy(); - expect(response1.status).toBe(200); - const data1 = (await response1.json()) as ApiResponse; - expect(data1.result).toContain("updated successfully"); + const ctx2 = createExecutionContext(); + const response2 = await app.request( + "/versions/v1", + { headers: BYPASS_CACHE }, + env, + ctx2 + ); + await waitOnExecutionContext(ctx2); - cached = await env.CACHE_KV.get("gh-fossbilling-releases"); - expect(cached).toBeTruthy(); + expect(response2.status).toBe(200); + }); - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); + it("returns consistent data across all endpoints", async () => { + const ctx1 = createExecutionContext(); + const response1 = await app.request( + "/versions/v1", + { headers: BYPASS_CACHE }, + env, + ctx1 + ); + await waitOnExecutionContext(ctx1); + const allVersions: ApiResponse> = + await response1.json(); + + const ctx2 = createExecutionContext(); + const response2 = await app.request( + "/versions/v1/latest", + { headers: BYPASS_CACHE }, + env, + ctx2 + ); + await waitOnExecutionContext(ctx2); + const latest = (await response2.json()) as ApiResponse; + + const ctx3 = createExecutionContext(); + const response3 = await app.request( + "/versions/v1/0.6.0", + { headers: BYPASS_CACHE }, + env, + ctx3 + ); + await waitOnExecutionContext(ctx3); + const specific = + (await response3.json()) as ApiResponse; - expect(response2.status).toBe(200); - }); + expect(latest.result).toEqual(allVersions.result["0.6.0"]); + expect(specific.result).toEqual(allVersions.result["0.6.0"]); + }); - it("should return correct data across all endpoints", async () => { - const ctx1 = createExecutionContext(); - const response1 = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); - const allVersions: VersionsResponse = await response1.json(); + it("refreshes the cached blob with new upstream content on update", async () => { + const ctx1 = createExecutionContext(); + await app.request("/versions/v1", { headers: BYPASS_CACHE }, env, ctx1); + await waitOnExecutionContext(ctx1); + + const cachedBefore = await env.CACHE_KV.get("gh-fossbilling-releases"); + + (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( + async (route: string) => { + if (route === "GET /repos/{owner}/{repo}/releases") { + return { + data: [ + { + id: 9999, + tag_name: "9.9.9", + name: "9.9.9", + published_at: "2024-01-01T00:00:00Z", + prerelease: false, + body: "New release", + assets: [ + { + name: "FOSSBilling.zip", + browser_download_url: "https://example.com/new.zip", + size: 2000000 + } + ] + } + ] + }; + } + if (route === "GET /repos/{owner}/{repo}/contents/{path}{?ref}") { + const content = btoa(JSON.stringify(mockComposerJson)); + return { data: { content } }; + } + throw new Error("Unexpected route"); + } + ); - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1/latest", - { headers: BYPASS_CACHE }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - const latest = (await response2.json()) as VersionsResponse; + const ctx2 = createExecutionContext(); + await app.request( + "/versions/v1/update", + { + headers: { + Authorization: "Bearer test-update-token-12345" + } + }, + env, + ctx2 + ); + await waitOnExecutionContext(ctx2); - const ctx3 = createExecutionContext(); - const response3 = await app.request( - "/versions/v1/0.6.0", - { headers: BYPASS_CACHE }, - env, - ctx3 - ); - await waitOnExecutionContext(ctx3); - const specific = (await response3.json()) as VersionsResponse; + const cachedAfter = await env.CACHE_KV.get("gh-fossbilling-releases"); - expect(latest.result).toEqual(allVersions.result["0.6.0"]); - expect(specific.result).toEqual(allVersions.result["0.6.0"]); - }); + expect(cachedAfter).not.toBe(cachedBefore); + expect(cachedAfter).toContain("9.9.9"); }); - describe("Cache Behavior", () => { - it("should use cache across multiple requests", async () => { - const requests = 5; - const responses = []; + it("handles concurrent cold-start requests gracefully", async () => { + await env.CACHE_KV.delete("gh-fossbilling-releases"); - for (let i = 0; i < requests; i++) { - const ctx = createExecutionContext(); + const promises = []; + for (let i = 0; i < 10; i++) { + const ctx = createExecutionContext(); + const promise = (async () => { const response = await app.request( "/versions/v1", { headers: BYPASS_CACHE }, @@ -174,198 +200,19 @@ describe("Versions API v1 - Integration Tests", () => { ctx ); await waitOnExecutionContext(ctx); - responses.push(await response.json()); - } - - for (let i = 1; i < requests; i++) { - expect(responses[i]).toEqual(responses[0]); - } - - expect(vi.mocked(ghRequest)).toHaveBeenCalled(); - }); - - it("should refresh cache when update endpoint is called", async () => { - const ctx1 = createExecutionContext(); - await app.request("/versions/v1", { headers: BYPASS_CACHE }, env, ctx1); - await waitOnExecutionContext(ctx1); - - const cachedBefore = await env.CACHE_KV.get("gh-fossbilling-releases"); - - (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( - async (route: string) => { - if (route === "GET /repos/{owner}/{repo}/releases") { - return { - data: [ - { - id: 9999, - tag_name: "9.9.9", - name: "9.9.9", - published_at: "2024-01-01T00:00:00Z", - prerelease: false, - body: "New release", - assets: [ - { - name: "FOSSBilling.zip", - browser_download_url: "https://example.com/new.zip", - size: 2000000 - } - ] - } - ] - }; - } - if (route === "GET /repos/{owner}/{repo}/contents/{path}{?ref}") { - const content = btoa(JSON.stringify(mockComposerJson)); - return { data: { content } }; - } - throw new Error("Unexpected route"); - } - ); - - const ctx2 = createExecutionContext(); - await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - - const cachedAfter = await env.CACHE_KV.get("gh-fossbilling-releases"); - - expect(cachedAfter).not.toBe(cachedBefore); - expect(cachedAfter).toContain("9.9.9"); - }); - - it("should handle concurrent requests gracefully", async () => { - await env.CACHE_KV.delete("gh-fossbilling-releases"); - - const promises = []; - for (let i = 0; i < 10; i++) { - const ctx = createExecutionContext(); - const promise = (async () => { - const response = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - return response.json(); - })(); - promises.push(promise); - } - - const results = await Promise.all(promises); - - results.forEach((result) => { - const r = result as ApiResponse; - expect(r.error_code).toBe(0); - expect( - Object.keys(r.result as Record).length - ).toBeGreaterThan(0); - }); - }); - }); - - describe("Error Recovery", () => { - it("should recover from GitHub API failure using cache", async () => { - const ctx1 = createExecutionContext(); - await app.request("/versions/v1", { headers: BYPASS_CACHE }, env, ctx1); - await waitOnExecutionContext(ctx1); - - (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( - new Error("GitHub API Error") - ); - - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - - expect(response2.status).toBe(200); - const data: VersionsResponse = await response2.json(); - expect(Object.keys(data.result).length).toBeGreaterThan(0); - }); - - it("should work after authentication failure then success", async () => { - const ctx1 = createExecutionContext(); - const response1 = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer wrong-token" - } - }, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); - - expect(response1.status).toBe(401); - - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - - expect(response2.status).toBe(200); - }); - }); - - describe("Service Integration", () => { - it("should work when called through main app router", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: BYPASS_CACHE }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - - const data: VersionsResponse = await response.json(); - expect(data).toHaveProperty("result"); - expect(data).toHaveProperty("error_code", 0); - expect(data).toHaveProperty("message", null); - }); - - it("should maintain consistent API response format", async () => { - const endpoints = [ - "/versions/v1", - "/versions/v1/latest", - "/versions/v1/0.6.0" - ]; - - for (const endpoint of endpoints) { - const ctx = createExecutionContext(); - const response = await app.request(endpoint, {}, env, ctx); - await waitOnExecutionContext(ctx); - - const data = await response.json(); - expect(data).toHaveProperty("result"); - expect(data).toHaveProperty("error_code"); - expect(data).toHaveProperty("message"); - } + return response.json(); + })(); + promises.push(promise); + } + + const results = await Promise.all(promises); + + results.forEach((result) => { + const r = result as ApiResponse; + expect(r.error_code).toBe(0); + expect( + Object.keys(r.result as Record).length + ).toBeGreaterThan(0); }); }); }); diff --git a/test/lib/adapters/cloudflare/cache.test.ts b/test/lib/adapters/cloudflare/cache.test.ts index cc914ee1..599d2006 100644 --- a/test/lib/adapters/cloudflare/cache.test.ts +++ b/test/lib/adapters/cloudflare/cache.test.ts @@ -1,15 +1,17 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { CloudflareKVAdapter } from "../../../../src/lib/adapters/cloudflare/cache"; +// The adapter is a thin ICache view over KVNamespace; one round-trip smoke +// test pins the delegation without re-proving KV semantics per method. describe("CloudflareKVAdapter", () => { let mockKV: KVNamespace; let adapter: CloudflareKVAdapter; beforeEach(() => { mockKV = { - get: vi.fn(), - put: vi.fn(), - delete: vi.fn() + get: vi.fn().mockResolvedValue("test-value"), + put: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(undefined) } as unknown as KVNamespace; adapter = new CloudflareKVAdapter(mockKV); @@ -19,116 +21,16 @@ describe("CloudflareKVAdapter", () => { vi.clearAllMocks(); }); - describe("get", () => { - it("should get value from KV store", async () => { - mockKV.get = vi.fn().mockResolvedValue("test-value"); + it("delegates get/put/delete to the KV namespace", async () => { + await adapter.put("test-key", "test-value", { expirationTtl: 3600 }); + const result = await adapter.get("test-key"); + await adapter.delete("test-key"); - const result = await adapter.get("test-key"); - - expect(mockKV.get).toHaveBeenCalledWith("test-key"); - expect(result).toBe("test-value"); - }); - - it("should return null for non-existent keys", async () => { - mockKV.get = vi.fn().mockResolvedValue(null); - - const result = await adapter.get("nonexistent"); - - expect(result).toBeNull(); - }); - }); - - describe("put", () => { - it("should put value to KV store", async () => { - mockKV.put = vi.fn().mockResolvedValue(undefined); - - await adapter.put("test-key", "test-value"); - - expect(mockKV.put).toHaveBeenCalledWith( - "test-key", - "test-value", - undefined - ); - }); - - it("should put value with expirationTtl option", async () => { - mockKV.put = vi.fn().mockResolvedValue(undefined); - - await adapter.put("test-key", "test-value", { expirationTtl: 3600 }); - - expect(mockKV.put).toHaveBeenCalledWith("test-key", "test-value", { - expirationTtl: 3600 - }); - }); - - it("should put value with expiration option", async () => { - mockKV.put = vi.fn().mockResolvedValue(undefined); - - const expirationTime = Math.floor(Date.now() / 1000) + 3600; - await adapter.put("test-key", "test-value", { - expiration: expirationTime - }); - - expect(mockKV.put).toHaveBeenCalledWith("test-key", "test-value", { - expiration: expirationTime - }); - }); - - it("should put value with both expiration options", async () => { - mockKV.put = vi.fn().mockResolvedValue(undefined); - - await adapter.put("test-key", "test-value", { - expirationTtl: 3600, - expiration: 1234567890 - }); - - expect(mockKV.put).toHaveBeenCalledWith("test-key", "test-value", { - expirationTtl: 3600, - expiration: 1234567890 - }); - }); - }); - - describe("delete", () => { - it("should delete key from KV store", async () => { - mockKV.delete = vi.fn().mockResolvedValue(undefined); - - await adapter.delete("test-key"); - - expect(mockKV.delete).toHaveBeenCalledWith("test-key"); - }); - }); - - describe("Multiple Operations", () => { - it("should handle sequence of get and put operations", async () => { - mockKV.get = vi.fn().mockResolvedValue(null); - mockKV.put = vi.fn().mockResolvedValue(undefined); - - await adapter.put("key1", "value1"); - await adapter.put("key2", "value2"); - - mockKV.get = vi.fn().mockResolvedValue("value1"); - const result1 = await adapter.get("key1"); - - mockKV.get = vi.fn().mockResolvedValue("value2"); - const result2 = await adapter.get("key2"); - - expect(result1).toBe("value1"); - expect(result2).toBe("value2"); - }); - - it("should handle delete after put", async () => { - mockKV.put = vi.fn().mockResolvedValue(undefined); - mockKV.get = vi.fn().mockResolvedValue("test-value"); - mockKV.delete = vi.fn().mockResolvedValue(undefined); - - await adapter.put("test-key", "test-value"); - await adapter.delete("test-key"); - - mockKV.get = vi.fn().mockResolvedValue(null); - const result = await adapter.get("test-key"); - - expect(result).toBeNull(); + expect(mockKV.put).toHaveBeenCalledWith("test-key", "test-value", { + expirationTtl: 3600 }); + expect(mockKV.get).toHaveBeenCalledWith("test-key"); + expect(result).toBe("test-value"); + expect(mockKV.delete).toHaveBeenCalledWith("test-key"); }); }); diff --git a/test/lib/adapters/cloudflare/environment.test.ts b/test/lib/adapters/cloudflare/environment.test.ts index 92f52d22..38fb0713 100644 --- a/test/lib/adapters/cloudflare/environment.test.ts +++ b/test/lib/adapters/cloudflare/environment.test.ts @@ -1,97 +1,37 @@ import { describe, it, expect, beforeEach } from "vitest"; import { CloudflareEnvironmentAdapter } from "../../../../src/lib/adapters/cloudflare/environment"; +// One test per filtering rule: strings pass through, everything else (any +// non-string binding the env object may carry) is hidden behind the +// IEnvironment contract. describe("CloudflareEnvironmentAdapter", () => { let adapter: CloudflareEnvironmentAdapter; beforeEach(() => { const env = { STRING_VAR: "test-value", + EMPTY_VAR: "", NUMBER_VAR: 123, - BOOLEAN_VAR: true, - OBJECT_VAR: { key: "value" }, - UNDEFINED_VAR: undefined, - NULL_VAR: null + OBJECT_VAR: { key: "value" } }; adapter = new CloudflareEnvironmentAdapter(env as Record); }); - describe("get", () => { - it("should return value when it exists and is a string", () => { - const result = adapter.get("STRING_VAR"); - - expect(result).toBe("test-value"); - }); - - it("should return undefined when value exists but is not a string", () => { - const result1 = adapter.get("NUMBER_VAR"); - const result2 = adapter.get("BOOLEAN_VAR"); - const result3 = adapter.get("OBJECT_VAR"); - - expect(result1).toBeUndefined(); - expect(result2).toBeUndefined(); - expect(result3).toBeUndefined(); - }); - - it("should return undefined when value is undefined", () => { - const result = adapter.get("UNDEFINED_VAR"); - - expect(result).toBeUndefined(); - }); - - it("should return undefined when value is null", () => { - const result = adapter.get("NULL_VAR"); - - expect(result).toBeUndefined(); - }); - - it("should return undefined when key does not exist", () => { - const result = adapter.get("NONEXISTENT_VAR"); - - expect(result).toBeUndefined(); - }); - - it("should handle empty string values", () => { - const env = { EMPTY_VAR: "" }; - const emptyAdapter = new CloudflareEnvironmentAdapter(env); - - const result = emptyAdapter.get("EMPTY_VAR"); - - expect(result).toBe(""); - }); + it("returns string values", () => { + expect(adapter.get("STRING_VAR")).toBe("test-value"); }); - describe("has", () => { - it("should return true when key exists and is a string", () => { - const result = adapter.has("STRING_VAR"); - - expect(result).toBe(true); - }); - - it("should return false when key exists but is not a string", () => { - const result1 = adapter.has("NUMBER_VAR"); - const result2 = adapter.has("BOOLEAN_VAR"); - const result3 = adapter.has("OBJECT_VAR"); - - expect(result1).toBe(false); - expect(result2).toBe(false); - expect(result3).toBe(false); - }); - - it("should return false when key does not exist", () => { - const result = adapter.has("NONEXISTENT_VAR"); - - expect(result).toBe(false); - }); - - it("should return true for empty string values", () => { - const env = { EMPTY_VAR: "" }; - const emptyAdapter = new CloudflareEnvironmentAdapter(env); + it("returns empty string values", () => { + expect(adapter.get("EMPTY_VAR")).toBe(""); + }); - const result = emptyAdapter.has("EMPTY_VAR"); + it("hides non-string values", () => { + expect(adapter.get("NUMBER_VAR")).toBeUndefined(); + expect(adapter.get("OBJECT_VAR")).toBeUndefined(); + }); - expect(result).toBe(true); - }); + it("returns undefined for missing keys", () => { + expect(adapter.get("NONEXISTENT_VAR")).toBeUndefined(); }); }); diff --git a/test/lib/adapters/node/bindings.test.ts b/test/lib/adapters/node/bindings.test.ts deleted file mode 100644 index 17f6593d..00000000 --- a/test/lib/adapters/node/bindings.test.ts +++ /dev/null @@ -1,64 +0,0 @@ -import { describe, it, expect, afterEach } from "vitest"; -import { createNodeBindings } from "../../../../src/lib/adapters/node/index"; - -describe("createNodeBindings - path normalization", () => { - afterEach(() => { - // Cleanup of any underlying database files is intentionally omitted here - // to avoid deleting files that may still have open SQLite connections, - // which can cause failures especially on Windows. - }); - - it("should handle paths without extensions", () => { - const bindings = createNodeBindings("/tmp/test-binding-normal"); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should handle paths with .db extension", () => { - const bindings = createNodeBindings("/tmp/test-binding.db"); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should handle paths with .sqlite extension", () => { - const bindings = createNodeBindings("/tmp/test-binding.sqlite"); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should handle paths with .sqlite3 extension", () => { - const bindings = createNodeBindings("/tmp/test-binding.sqlite3"); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should handle paths ending with dot", () => { - const bindings = createNodeBindings("/tmp/test-binding."); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should handle paths ending with double dots", () => { - const bindings = createNodeBindings("/tmp/test-binding.."); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should handle paths ending with slash", () => { - const bindings = createNodeBindings("/tmp/test-binding/"); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should create in-memory caches when path is undefined", () => { - const bindings = createNodeBindings(undefined); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should create in-memory caches when path is not provided", () => { - const bindings = createNodeBindings(); - expect(bindings.caches.CACHE_KV).toBeDefined(); - expect(bindings.caches.AUTH_KV).toBeDefined(); - }); -}); diff --git a/test/lib/adapters/node/cache.test.ts b/test/lib/adapters/node/cache.test.ts index f497c7e0..e672e60c 100644 --- a/test/lib/adapters/node/cache.test.ts +++ b/test/lib/adapters/node/cache.test.ts @@ -1,6 +1,7 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { DatabaseSync } from "node:sqlite"; -import { existsSync, unlinkSync } from "node:fs"; +import { describe, it, expect, beforeEach } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { SQLiteCacheAdapter, createMemoryCache, @@ -75,69 +76,33 @@ describe("SQLiteCacheAdapter - Memory", () => { expect(await cache.get("key1")).toBe("value1"); }); - it("should throw on get error", async () => { - const db = new DatabaseSync(":memory:"); - const badCache = new SQLiteCacheAdapter(db); - db.close(); - await expect(badCache.get("key")).rejects.toThrow(); - }); - - it("should throw on put error", async () => { - const db = new DatabaseSync(":memory:"); - const badCache = new SQLiteCacheAdapter(db); - db.close(); - await expect(badCache.put("key", "value")).rejects.toThrow(); - }); - - it("should throw on delete error", async () => { - const db = new DatabaseSync(":memory:"); - const badCache = new SQLiteCacheAdapter(db); - db.close(); - await expect(badCache.delete("key")).rejects.toThrow(); - }); -}); - -describe("SQLiteCacheAdapter - File", () => { - const testDbPath = "/tmp/test-cache.db"; - let cache: SQLiteCacheAdapter; - - beforeEach(() => { - cache = createFileCache(testDbPath); - }); - - afterEach(() => { - // Close the database connection to release file handles - if (cache) { - try { - cache.close(); - } catch { - // Ignore errors during cleanup - } - } - // Clean up the test database file - if (existsSync(testDbPath)) { + // createNodeBindings uses createFileCache in production-shaped flows, so + // file-backed caches must survive reopening - the durable behavior the + // memory cache above deliberately does not have. + describe("SQLiteCacheAdapter - File", () => { + it("persists values across reopen", async () => { + const dir = mkdtempSync(join(tmpdir(), "fb-node-cache-")); + const dbPath = join(dir, "cache.db"); try { - unlinkSync(testDbPath); - } catch { - // Ignore errors if file doesn't exist or can't be deleted + const writer = createFileCache(dbPath); + await writer.put("key1", "value1"); + writer.close(); + + const reopened = createFileCache(dbPath); + try { + await expect(reopened.get("key1")).resolves.toBe("value1"); + } finally { + reopened.close(); + } + } finally { + // The first handle is already closed; removal is best-effort and + // temp-scoped either way. + try { + rmSync(dir, { recursive: true, force: true }); + } catch { + // Ignore - under the OS temp dir. + } } - } - }); - - it("should persist data to file", async () => { - await cache.put("key1", "value1"); - const result = await cache.get("key1"); - expect(result).toBe("value1"); - }); - - it("should reload data from file", async () => { - await cache.put("key1", "value1"); - const cache2 = createFileCache(testDbPath); - try { - const result = await cache2.get("key1"); - expect(result).toBe("value1"); - } finally { - cache2.close(); - } + }); }); }); diff --git a/test/lib/adapters/node/environment.test.ts b/test/lib/adapters/node/environment.test.ts index 11a05a4b..de932fa5 100644 --- a/test/lib/adapters/node/environment.test.ts +++ b/test/lib/adapters/node/environment.test.ts @@ -1,101 +1,30 @@ -import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { describe, it, expect } from "vitest"; import { NodeEnvironmentAdapter } from "../../../../src/lib/adapters/node/environment"; +// The adapter is `return process.env[key]`; one smoke test pins the +// IEnvironment contract (string passthrough, undefined for missing keys), +// with save/restore so the shared process.env is left untouched even on +// failure. describe("NodeEnvironmentAdapter", () => { - let adapter: NodeEnvironmentAdapter; - - beforeEach(() => { - adapter = new NodeEnvironmentAdapter(); - }); - - afterEach(() => { - vi.unstubAllEnvs(); - }); - - describe("get", () => { - it("should return environment variable when it exists", () => { - vi.stubEnv("TEST_VAR", "test-value"); - - const result = adapter.get("TEST_VAR"); - - expect(result).toBe("test-value"); - }); - - it("should return undefined when environment variable does not exist", () => { - vi.stubEnv("NONEXISTENT_VAR", undefined); - - const result = adapter.get("NONEXISTENT_VAR"); - - expect(result).toBeUndefined(); - }); - - it("should return empty string for empty environment variable", () => { - vi.stubEnv("EMPTY_VAR", ""); - - const result = adapter.get("EMPTY_VAR"); - - expect(result).toBe(""); - }); - - it("should return string value when environment variable is set", () => { - vi.stubEnv("ANOTHER_VAR", "another-value"); - - const result = adapter.get("ANOTHER_VAR"); - - expect(result).toBe("another-value"); - }); - }); - - describe("has", () => { - it("should return true when environment variable exists", () => { - vi.stubEnv("EXISTING_VAR", "some-value"); - - const result = adapter.has("EXISTING_VAR"); - - expect(result).toBe(true); - }); - - it("should return false when environment variable does not exist", () => { - const result = adapter.has("NONEXISTENT_VAR"); - - expect(result).toBe(false); - }); - - it("should return true for empty string environment variable", () => { - vi.stubEnv("EMPTY_VAR", ""); - - const result = adapter.has("EMPTY_VAR"); - - expect(result).toBe(true); - }); - - it("should return false for undefined environment variable", () => { - vi.stubEnv("UNDEFINED_VAR", undefined); - - const result = adapter.has("UNDEFINED_VAR"); - - expect(result).toBe(false); - }); - }); - - describe("Multiple Environment Variables", () => { - it("should access multiple environment variables", () => { - vi.stubEnv("VAR1", "value1"); - vi.stubEnv("VAR2", "value2"); - vi.stubEnv("VAR3", "value3"); - - expect(adapter.get("VAR1")).toBe("value1"); - expect(adapter.get("VAR2")).toBe("value2"); - expect(adapter.get("VAR3")).toBe("value3"); - }); - - it("should check existence of multiple environment variables", () => { - vi.stubEnv("VAR_A", "value-a"); - vi.stubEnv("VAR_B", "value-b"); - - expect(adapter.has("VAR_A")).toBe(true); - expect(adapter.has("VAR_B")).toBe(true); - expect(adapter.has("VAR_C")).toBe(false); - }); + it("returns process.env values and undefined for missing keys", () => { + const originalTestValue = process.env.FB_TEST_VAR; + const originalMissingValue = process.env.FB_TEST_MISSING; + try { + process.env.FB_TEST_VAR = "test-value"; + delete process.env.FB_TEST_MISSING; + + const adapter = new NodeEnvironmentAdapter(); + expect(adapter.get("FB_TEST_VAR")).toBe("test-value"); + expect(adapter.get("FB_TEST_MISSING")).toBeUndefined(); + } finally { + if (originalTestValue === undefined) { + delete process.env.FB_TEST_VAR; + } else { + process.env.FB_TEST_VAR = originalTestValue; + } + if (originalMissingValue !== undefined) { + process.env.FB_TEST_MISSING = originalMissingValue; + } + } }); }); diff --git a/test/lib/adapters/node/index.test.ts b/test/lib/adapters/node/index.test.ts index 91caca08..b109bbe8 100644 --- a/test/lib/adapters/node/index.test.ts +++ b/test/lib/adapters/node/index.test.ts @@ -1,41 +1,37 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, afterAll } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { createNodeBindings } from "../../../../src/lib/adapters/node/index"; +// The Node bindings exist to prove the IPlatformBindings port is +// implementable off-Cloudflare (see src/lib/adapters/node/index.ts); these +// tests assert that contract, not file-path edge cases. describe("createNodeBindings", () => { - it("should create bindings with default paths", () => { - const bindings = createNodeBindings(); - - expect(bindings).toBeDefined(); - expect(bindings.caches).toBeDefined(); - expect(bindings.environment).toBeDefined(); + const dir = mkdtempSync(join(tmpdir(), "fb-node-bindings-")); + + afterAll(() => { + // SQLite handles may still be open here (see cache.test.ts); removal is + // best-effort and temp-scoped either way. + try { + rmSync(dir, { recursive: true, force: true }); + } catch { + // Ignore - the directory is under the OS temp dir. + } }); - it("should create memory caches when no path provided", () => { + it("creates in-memory caches and an environment adapter by default", () => { const bindings = createNodeBindings(); expect(bindings.caches.CACHE_KV).toBeDefined(); expect(bindings.caches.AUTH_KV).toBeDefined(); - }); - - it("should create environment adapter", () => { - const bindings = createNodeBindings(); - - expect(bindings.environment).toBeDefined(); expect(typeof bindings.environment.get).toBe("function"); - expect(typeof bindings.environment.has).toBe("function"); }); -}); -describe("normalizePath (internal function)", () => { - it("should use in-memory database by default", () => { - const bindings = createNodeBindings(); - expect(bindings).toBeDefined(); + it("creates file-backed caches when given a path", () => { + const bindings = createNodeBindings(join(dir, "bindings")); + expect(bindings.caches.CACHE_KV).toBeDefined(); expect(bindings.caches.AUTH_KV).toBeDefined(); }); - - it("should accept empty path parameter", () => { - const bindings = createNodeBindings(""); - expect(bindings).toBeDefined(); - }); }); diff --git a/test/lib/auth/assertion-helper.ts b/test/lib/auth/assertion-helper.ts index 3b19222c..74365662 100644 --- a/test/lib/auth/assertion-helper.ts +++ b/test/lib/auth/assertion-helper.ts @@ -21,7 +21,6 @@ export interface AssertionOverrides { purpose?: string; ver?: number; header?: Record; - includeContext?: boolean; } /** Mints a compact HS256 assertion matching what bearer-assertion.ts verifies. */ @@ -33,16 +32,12 @@ export async function signAssertion( const payload: Record = { sub: overrides.sub ?? "user-1", iat, - exp: overrides.exp ?? iat + 60 + exp: overrides.exp ?? iat + 60, + iss: overrides.iss ?? "fossbilling-extensions", + aud: overrides.aud ?? "fossbilling-api/extensions-v2", + purpose: overrides.purpose ?? "user-authentication", + ver: overrides.ver ?? 1 }; - if (overrides.includeContext !== false) { - Object.assign(payload, { - iss: overrides.iss ?? "fossbilling-extensions", - aud: overrides.aud ?? "fossbilling-api/extensions-v2", - purpose: overrides.purpose ?? "user-authentication", - ver: overrides.ver ?? 1 - }); - } if (overrides.bodySha256 !== undefined) payload.body_sha256 = overrides.bodySha256; diff --git a/test/lib/auth/bearer-assertion.test.ts b/test/lib/auth/bearer-assertion.test.ts index ece8bfcc..da218e21 100644 --- a/test/lib/auth/bearer-assertion.test.ts +++ b/test/lib/auth/bearer-assertion.test.ts @@ -88,6 +88,9 @@ describe("bearerAssertionVerifier", () => { expect(principal).toBeNull(); }); + // iss/aud/purpose/ver are minter constants, but a minter/API version skew + // (one side deploying before the other) is a real operational scenario, as + // is sibling-verifier traffic for `purpose` - the full claim matrix stays. it.each([ ["issuer", { iss: "wrong-issuer" }], ["audience", { aud: "wrong-audience" }], @@ -103,6 +106,12 @@ describe("bearerAssertionVerifier", () => { expect(principal).toBeNull(); }); + // The minter emits integer seconds and a fixed 60s lifetime, so these + // states are unreachable from a healthy minter - but the verifier's + // integer and lifetime checks are this repo's own defense against a + // misbehaving or rolled-back minter, and signAssertion can construct + // exactly those tokens. Keep them pinned so the guards cannot silently + // weaken (the assertion replay window depends on both). it.each(["iat", "exp"])( "rejects fractional %s NumericDate values", async (claim) => { @@ -168,18 +177,6 @@ describe("bearerAssertionVerifier", () => { expect(principal).toBeNull(); }); - it("rejects a legacy token without contextual claims", async () => { - const token = await signAssertion(SECRET, { - includeContext: false - }); - const principal = await bearerAssertionVerifier.verify( - token, - platformWithSecret(SECRET) - ); - - expect(principal).toBeNull(); - }); - it("rejects a malformed token", async () => { const principal = await bearerAssertionVerifier.verify( "not-a-jwt", @@ -241,6 +238,10 @@ describe("identitySyncAssertionVerifier", () => { await bearerAssertionVerifier.verify(token, platformWithSecret(SECRET)) ).toBeNull(); }); + // The verifier requires a 64-char lowercase hex digest; the minter can + // only ever emit that, but the check is this repo's own validation and + // signAssertion can construct every wrong shape - pin it so the format + // requirement cannot silently loosen. it.each([undefined, "", "A".repeat(64), "a".repeat(63), "z".repeat(64)])( "rejects an invalid digest %s", async (bodySha256) => { diff --git a/test/lib/cache.test.ts b/test/lib/cache.test.ts index 9a3fd74c..217ff936 100644 --- a/test/lib/cache.test.ts +++ b/test/lib/cache.test.ts @@ -1,26 +1,18 @@ import { describe, expect, it } from "vitest"; -import { Hono } from "hono"; -import { - normalizePublicCacheKey, - publicResponseCache -} from "../../src/lib/cache"; +import { Hono, Context } from "hono"; +import { publicCacheKey, publicResponseCache } from "../../src/lib/cache"; + +const cacheKeyFor = (url: string) => + publicCacheKey({ req: { url } } as unknown as Context); describe("cache helpers", () => { it("normalizes public cache keys by removing query strings and fragments", () => { expect( - normalizePublicCacheKey( + cacheKeyFor( "https://api.fossbilling.net/versions/v1/latest?cacheBust=1#section" ) ).toBe("https://api.fossbilling.net/versions/v1/latest"); }); - - it("keeps different public paths isolated", () => { - expect( - normalizePublicCacheKey("https://api.fossbilling.net/versions/v1") - ).not.toBe( - normalizePublicCacheKey("https://api.fossbilling.net/versions/v1/count") - ); - }); }); describe("public response cache", () => { diff --git a/test/lib/context.test.ts b/test/lib/context.test.ts index 27e74242..3a936758 100644 --- a/test/lib/context.test.ts +++ b/test/lib/context.test.ts @@ -3,6 +3,10 @@ import { Context } from "hono"; import { createPlatformContext } from "../../src/lib/context"; import { IPlatformBindings } from "../../src/lib/interfaces"; +// createPlatformContext is an object literal over the bindings; the only +// logic of its own is the missing-binding throw, so that is what is +// asserted here. The accessors are exercised end-to-end by every suite +// that boots the real app. describe("createPlatformContext", () => { const mockBindings: IPlatformBindings = { caches: { @@ -13,8 +17,7 @@ describe("createPlatformContext", () => { } }, environment: { - get: (key) => (key === "TEST_VAR" ? "test-value" : undefined), - has: (key) => key === "TEST_VAR" + get: (key) => (key === "TEST_VAR" ? "test-value" : undefined) } }; @@ -25,52 +28,19 @@ describe("createPlatformContext", () => { res: {} } as unknown as Context; - it("should create a platform context with all required methods", () => { + it("exposes the binding accessors and the raw context", () => { const context = createPlatformContext(mockHonoContext, mockBindings); - expect(context).toHaveProperty("getCache"); - expect(context).toHaveProperty("getEnv"); - expect(context).toHaveProperty("raw"); + expect(context.getCache("testCache")).toBe(mockBindings.caches.testCache); + expect(context.getEnv("TEST_VAR")).toBe("test-value"); + expect(context.raw).toBe(mockHonoContext); }); - describe("getCache", () => { - it("should return the requested cache binding", () => { - const context = createPlatformContext(mockHonoContext, mockBindings); - const cache = context.getCache("testCache"); - - expect(cache).toBe(mockBindings.caches.testCache); - }); - - it("should throw when cache binding not found", () => { - const context = createPlatformContext(mockHonoContext, mockBindings); - - expect(() => context.getCache("nonexistent")).toThrow( - "Cache binding 'nonexistent' not found" - ); - }); - }); - - describe("getEnv", () => { - it("should return environment variable value when it exists", () => { - const context = createPlatformContext(mockHonoContext, mockBindings); - const value = context.getEnv("TEST_VAR"); - - expect(value).toBe("test-value"); - }); - - it("should return undefined when environment variable does not exist", () => { - const context = createPlatformContext(mockHonoContext, mockBindings); - const value = context.getEnv("NONEXISTENT"); - - expect(value).toBeUndefined(); - }); - }); - - describe("raw property", () => { - it("should return the original Hono context", () => { - const context = createPlatformContext(mockHonoContext, mockBindings); + it("throws when cache binding not found", () => { + const context = createPlatformContext(mockHonoContext, mockBindings); - expect(context.raw).toBe(mockHonoContext); - }); + expect(() => context.getCache("nonexistent")).toThrow( + "Cache binding 'nonexistent' not found" + ); }); }); diff --git a/test/lib/github-errors.test.ts b/test/lib/github-errors.test.ts index a8c014a6..08ff59ec 100644 --- a/test/lib/github-errors.test.ts +++ b/test/lib/github-errors.test.ts @@ -6,87 +6,12 @@ import { NetworkError, NotFoundError, ValidationError, - ErrorPriority, - classifyGitHubError, - getMostCriticalError + classifyGitHubError } from "../../src/lib/github-errors"; -describe("GitHubError Classes", () => { - it("should create GitHubError with all properties", () => { - const error = new GitHubError( - "Test error", - 500, - "test_error", - ErrorPriority.MEDIUM, - "https://api.github.com/test", - { detail: "more info" } - ); - - expect(error.message).toBe("Test error"); - expect(error.httpStatus).toBe(500); - expect(error.errorCode).toBe("test_error"); - expect(error.priority).toBe(ErrorPriority.MEDIUM); - expect(error.url).toBe("https://api.github.com/test"); - expect(error.details).toEqual({ detail: "more info" }); - expect(error.name).toBe("GitHubError"); - }); - - it("should create AuthError with default priority CRITICAL", () => { - const error = new AuthError( - "Unauthorized", - 401, - "https://api.github.com/test" - ); - - expect(error.message).toBe("Unauthorized"); - expect(error.httpStatus).toBe(401); - expect(error.errorCode).toBe("auth_error"); - expect(error.priority).toBe(ErrorPriority.CRITICAL); - expect(error.url).toBe("https://api.github.com/test"); - }); - - it("should create RateLimitError with default priority CRITICAL", () => { - const error = new RateLimitError("Rate limited", 403); - - expect(error.message).toBe("Rate limited"); - expect(error.httpStatus).toBe(403); - expect(error.errorCode).toBe("rate_limit_error"); - expect(error.priority).toBe(ErrorPriority.CRITICAL); - }); - - it("should create NetworkError with priority HIGH", () => { - const error = new NetworkError( - "Network failure", - "https://api.github.com/test" - ); - - expect(error.message).toBe("Network failure"); - expect(error.httpStatus).toBeUndefined(); - expect(error.errorCode).toBe("network_error"); - expect(error.priority).toBe(ErrorPriority.HIGH); - expect(error.url).toBe("https://api.github.com/test"); - }); - - it("should create NotFoundError with priority MEDIUM", () => { - const error = new NotFoundError("Not found", 404); - - expect(error.message).toBe("Not found"); - expect(error.httpStatus).toBe(404); - expect(error.errorCode).toBe("not_found_error"); - expect(error.priority).toBe(ErrorPriority.MEDIUM); - }); - - it("should create ValidationError with priority LOW", () => { - const error = new ValidationError("Invalid data", { field: "value" }); - - expect(error.message).toBe("Invalid data"); - expect(error.httpStatus).toBeUndefined(); - expect(error.errorCode).toBe("validation_error"); - expect(error.priority).toBe(ErrorPriority.LOW); - expect(error.details).toEqual({ field: "value" }); - }); -}); - +// The error classes are default-parameter setters over GitHubError; every +// defaulted value is exercised through classifyGitHubError below, which is +// the path production code actually consumes. No constructor describe here. describe("classifyGitHubError", () => { it("should return original error if already a GitHubError", () => { const originalError = new AuthError("Already classified"); @@ -102,6 +27,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(AuthError); expect(result.message).toBe("Bad credentials"); expect(result.httpStatus).toBe(401); + expect(result.errorCode).toBe("auth_error"); expect(result.url).toBe("https://api.github.com/test"); }); @@ -113,6 +39,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(RateLimitError); expect(result.message).toBe("GitHub API rate limit exceeded"); expect(result.httpStatus).toBe(403); + expect(result.errorCode).toBe("rate_limit_error"); }); it("should classify 403 rate limit errors case-insensitively", () => { @@ -134,6 +61,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(AuthError); expect(result.message).toBe("Repository access denied"); expect(result.httpStatus).toBe(403); + expect(result.errorCode).toBe("auth_error"); }); // Regression: the rate-limit text was read from String(error), which is @@ -146,6 +74,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(RateLimitError); expect(result.httpStatus).toBe(403); + expect(result.errorCode).toBe("rate_limit_error"); }); it("should classify a 403 with an exhausted quota header as RateLimitError", () => { @@ -159,6 +88,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(RateLimitError); expect(result.message).toBe("GitHub API rate limit exceeded"); expect(result.httpStatus).toBe(403); + expect(result.errorCode).toBe("rate_limit_error"); }); it("should classify 429 errors as RateLimitError", () => { @@ -168,6 +98,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(RateLimitError); expect(result.message).toBe("GitHub API rate limit exceeded"); expect(result.httpStatus).toBe(429); + expect(result.errorCode).toBe("rate_limit_error"); }); // A 5xx has no dedicated class, but dropping its status would make an @@ -190,6 +121,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(NotFoundError); expect(result.message).toBe("Not found"); expect(result.httpStatus).toBe(404); + expect(result.errorCode).toBe("not_found_error"); expect(result.url).toBe("https://api.github.com/test"); }); @@ -200,6 +132,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(NetworkError); expect(result.message).toBe("GitHub API request timed out"); expect(result.url).toBe("https://api.github.com/test"); + expect(result.errorCode).toBe("network_error"); }); it("should classify network errors as NetworkError", () => { @@ -208,6 +141,7 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(NetworkError); expect(result.message).toBe("GitHub API network error"); + expect(result.errorCode).toBe("network_error"); }); it("should classify JSON parsing errors as ValidationError", () => { @@ -216,19 +150,16 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(ValidationError); expect(result.message).toBe("Invalid JSON response from GitHub API"); - expect(result.details).toEqual({ - originalMessage: "Unexpected token in JSON" - }); + expect(result.errorCode).toBe("validation_error"); }); - it("should classify unknown errors as GitHubError with priority HIGH", () => { + it("should classify unknown errors as GitHubError", () => { const error = new Error("Unknown error"); const result = classifyGitHubError(error, "https://api.github.com/test"); expect(result).toBeInstanceOf(GitHubError); expect(result.message).toBe("Unknown error"); expect(result.errorCode).toBe("unknown_error"); - expect(result.priority).toBe(ErrorPriority.HIGH); expect(result.url).toBe("https://api.github.com/test"); }); @@ -239,89 +170,4 @@ describe("classifyGitHubError", () => { expect(result).toBeInstanceOf(GitHubError); expect(result.message).toBe("String error"); }); - - it("should handle null errors", () => { - const result = classifyGitHubError(null); - - expect(result).toBeInstanceOf(GitHubError); - expect(result.message).toBe("null"); - }); -}); - -describe("getMostCriticalError", () => { - it("should return null for empty array", () => { - const result = getMostCriticalError([]); - - expect(result).toBeNull(); - }); - - it("should return single error from array", () => { - const error = new ValidationError("Test"); - const result = getMostCriticalError([error]); - - expect(result).toBe(error); - }); - - it("should select CRITICAL over other priorities", () => { - const errors = [ - new ValidationError("Low"), - new NetworkError("High"), - new AuthError("Critical") - ]; - - const result = getMostCriticalError(errors); - - expect(result).toBeInstanceOf(AuthError); - expect(result?.priority).toBe(ErrorPriority.CRITICAL); - }); - - it("should select HIGH over MEDIUM and LOW", () => { - const errors = [ - new ValidationError("Low"), - new NetworkError("High"), - new NotFoundError("Medium") - ]; - - const result = getMostCriticalError(errors); - - expect(result).toBeInstanceOf(NetworkError); - expect(result?.priority).toBe(ErrorPriority.HIGH); - }); - - it("should select MEDIUM over LOW", () => { - const errors = [new ValidationError("Low"), new NotFoundError("Medium")]; - - const result = getMostCriticalError(errors); - - expect(result).toBeInstanceOf(NotFoundError); - expect(result?.priority).toBe(ErrorPriority.MEDIUM); - }); - - it("should handle same priority by returning first", () => { - const error1 = new ValidationError("First"); - const error2 = new ValidationError("Second"); - - const result = getMostCriticalError([error1, error2]); - - expect(result).toBe(error1); - }); - - it("should select error with lower priority number", () => { - const error = new GitHubError( - "0", - undefined, - "test", - ErrorPriority.CRITICAL - ); - const errors = [ - error, - new GitHubError("1", undefined, "test", ErrorPriority.HIGH), - new GitHubError("2", undefined, "test", ErrorPriority.MEDIUM), - new GitHubError("3", undefined, "test", ErrorPriority.LOW) - ]; - - const result = getMostCriticalError(errors); - - expect(result?.priority).toBe(ErrorPriority.CRITICAL); - }); }); diff --git a/test/lib/logger.test.ts b/test/lib/logger.test.ts index 2d58401d..495e8117 100644 --- a/test/lib/logger.test.ts +++ b/test/lib/logger.test.ts @@ -1,6 +1,19 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { logError, logWarn, logInfo } from "../../src/lib/logger"; +// console[level] receives one structured object ({ service, message, +// context? }) - the shape Workers Logs serializes to filterable JSON. +// Helpers here pull the logged entry back out of the spy. +function loggedEntries(spy: { + mock: { calls: unknown[][] }; +}): Record[] { + return spy.mock.calls.map((call) => call[0] as Record); +} + +function logged(spy: { mock: { calls: unknown[][] } }): string { + return JSON.stringify(loggedEntries(spy).at(-1)); +} + describe("Logger", () => { let consoleErrorSpy: ReturnType; let consoleWarnSpy: ReturnType; @@ -18,250 +31,90 @@ describe("Logger", () => { consoleInfoSpy.mockRestore(); }); - describe("logError", () => { - it("should log error message to console.error", () => { + describe("routing and format", () => { + // One routing case per console method; the negative cross-assertions + // only re-prove console dispatch. + it("logError goes to console.error as a structured entry", () => { logError("TEST_SERVICE", "Error occurred"); expect(consoleErrorSpy).toHaveBeenCalledTimes(1); - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("[TEST_SERVICE]") - ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("[ERROR]") - ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("Error occurred") - ); - }); - - it("should log with context", () => { - const context = { userId: 123, action: "test" }; - logError("TEST_SERVICE", "Error occurred", context); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining(JSON.stringify(context)) - ); + const entry = loggedEntries(consoleErrorSpy)[0]; + expect(entry).toEqual({ + service: "TEST_SERVICE", + message: "Error occurred" + }); }); - it("should redact Bearer tokens in context", () => { - const context = { authorization: "Bearer secret-token-12345" }; - logError("TEST_SERVICE", "Error occurred", context); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("Bearer [REDACTED]") - ); - expect(consoleErrorSpy).not.toHaveBeenCalledWith( - expect.stringContaining("secret-token-12345") - ); - }); - - it("should include timestamp in log", () => { - logError("TEST_SERVICE", "Error occurred"); - - const logCall = consoleErrorSpy.mock.calls[0][0] as string; - expect(logCall).toMatch( - /\[\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z\]/ - ); - }); - }); - - describe("logWarn", () => { - it("should log warning message to console.warn", () => { + it("logWarn and logInfo go to their own console methods", () => { logWarn("TEST_SERVICE", "Warning message"); + logInfo("TEST_SERVICE", "Info message"); expect(consoleWarnSpy).toHaveBeenCalledTimes(1); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining("[TEST_SERVICE]") - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining("[WARN]") - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining("Warning message") - ); - }); - - it("should not call other console methods", () => { - logWarn("TEST_SERVICE", "Warning message"); - + expect(consoleInfoSpy).toHaveBeenCalledTimes(1); expect(consoleErrorSpy).not.toHaveBeenCalled(); - expect(consoleInfoSpy).not.toHaveBeenCalled(); }); - }); - describe("logInfo", () => { - it("should log info message to console.info", () => { - logInfo("TEST_SERVICE", "Info message"); + it("carries the redacted context as a structured field", () => { + const context = { userId: 123, action: "test" }; + logError("TEST_SERVICE", "Error occurred", context); - expect(consoleInfoSpy).toHaveBeenCalledTimes(1); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("[TEST_SERVICE]") - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("[INFO]") - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("Info message") - ); + const entry = loggedEntries(consoleErrorSpy)[0]; + expect(entry).toEqual({ + service: "TEST_SERVICE", + message: "Error occurred", + context + }); }); - it("should not call other console methods", () => { - logInfo("TEST_SERVICE", "Info message"); + it("omits the context field when not provided", () => { + logError("TEST_SERVICE", "Error occurred"); - expect(consoleErrorSpy).not.toHaveBeenCalled(); - expect(consoleWarnSpy).not.toHaveBeenCalled(); + const entry = loggedEntries(consoleErrorSpy)[0]; + expect(entry).toEqual({ + service: "TEST_SERVICE", + message: "Error occurred" + }); }); }); describe("Data Redaction", () => { - it("should redact 'token' keys in context", () => { - const context = { api_token: "secret", other: "value" }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"api_token":"[REDACTED]"') - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"other":"value"') - ); - }); - - it("should redact 'key' keys in context", () => { - const context = { secret_key: "secret", public_key: "public" }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"secret_key":"[REDACTED]"') - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"public_key":"[REDACTED]"') - ); - }); - - it("should redact 'secret' keys in context", () => { - const context = { my_secret: "secret", visible: "value" }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"my_secret":"[REDACTED]"') - ); - }); - - it("should redact 'password' keys in context", () => { - const context = { password: "secret123", username: "user" }; - logInfo("TEST_SERVICE", "Message", context); + it("redacts Bearer tokens in context", () => { + const context = { authorization: "Bearer secret-token-12345" }; + logError("TEST_SERVICE", "Error occurred", context); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"password":"[REDACTED]"') - ); + expect(logged(consoleErrorSpy)).toContain("Bearer [REDACTED]"); + expect(logged(consoleErrorSpy)).not.toContain("secret-token-12345"); }); - it("should redact keys case-insensitively", () => { + it("redacts token/key/secret/password keys case-insensitively", () => { const context = { + api_token: "secret", API_KEY: "secret", SecretKey: "secret2", - PASSWORD: "secret3" + password: "secret3", + visible: "value" }; logInfo("TEST_SERVICE", "Message", context); - expect(consoleInfoSpy).not.toHaveBeenCalledWith( - expect.stringContaining("secret") - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("[REDACTED]") - ); + expect(logged(consoleInfoSpy)).not.toContain("secret"); + expect(logged(consoleInfoSpy)).toContain("[REDACTED]"); + expect(logged(consoleInfoSpy)).toContain('"visible":"value"'); }); - it("should redact nested sensitive data", () => { + it("redacts nested and array-sensitive data", () => { const context = { user: { password: "secret123", name: "John" }, - config: { api_key: "key456" } - }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).not.toHaveBeenCalledWith( - expect.stringContaining("secret123") - ); - expect(consoleInfoSpy).not.toHaveBeenCalledWith( - expect.stringContaining("key456") - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"name":"John"') - ); - }); - - it("should redact sensitive data in arrays", () => { - const context = { - items: [{ token: "secret1" }, { token: "secret2" }, { name: "safe" }] - }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).not.toHaveBeenCalledWith( - expect.stringContaining("secret1") - ); - expect(consoleInfoSpy).not.toHaveBeenCalledWith( - expect.stringContaining("secret2") - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"name":"safe"') - ); - }); - - it("should handle strings directly with Bearer tokens", () => { - const context = { auth: "Bearer abc123def456" }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("Bearer [REDACTED]") - ); - expect(consoleInfoSpy).not.toHaveBeenCalledWith( - expect.stringContaining("abc123def456") - ); - }); - - it("should handle nested strings with Bearer tokens", () => { - const context = { headers: { authorization: "Bearer token123" } }; - logInfo("TEST_SERVICE", "Message", context); - - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("Bearer [REDACTED]") - ); - }); - - it("should preserve non-sensitive data", () => { - const context = { - user_id: 123, - name: "Test User", - action: "login", - timestamp: "2023-01-01" + items: [{ token: "secret1" }, { name: "safe" }], + headers: { authorization: "Bearer token123" } }; logInfo("TEST_SERVICE", "Message", context); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"user_id":123') - ); - expect(consoleInfoSpy).toHaveBeenCalledWith( - expect.stringContaining('"name":"Test User"') - ); - }); - }); - - describe("Log Format", () => { - it("should format log with timestamp, service, level, and message", () => { - logInfo("MY_SERVICE", "Test message"); - - const logCall = consoleInfoSpy.mock.calls[0][0] as string; - expect(logCall).toMatch( - /^\[[^\]]+\] \[MY_SERVICE\] \[INFO\] Test message$/ - ); - }); - - it("should not include context when not provided", () => { - logError("TEST_SERVICE", "Error occurred"); - - const logCall = consoleErrorSpy.mock.calls[0][0] as string; - expect(logCall).not.toContain("{"); - expect(logCall).not.toContain("}"); + expect(logged(consoleInfoSpy)).not.toContain("secret123"); + expect(logged(consoleInfoSpy)).not.toContain("secret1"); + expect(logged(consoleInfoSpy)).not.toContain("token123"); + expect(logged(consoleInfoSpy)).toContain('"name":"John"'); + expect(logged(consoleInfoSpy)).toContain('"name":"safe"'); + expect(logged(consoleInfoSpy)).toContain("Bearer [REDACTED]"); }); }); }); diff --git a/test/lib/middleware.test.ts b/test/lib/middleware.test.ts index 3b157c03..eb6f42a0 100644 --- a/test/lib/middleware.test.ts +++ b/test/lib/middleware.test.ts @@ -1,217 +1,20 @@ -import { describe, it, expect, beforeEach } from "vitest"; -import { Hono, Context } from "hono"; -import { platformMiddleware, getPlatform } from "../../src/lib/middleware"; -import { createPlatformContext } from "../../src/lib/context"; -import { IPlatformBindings } from "../../src/lib/interfaces"; - -describe("Middleware", () => { - let app: Hono; - let mockBindings: IPlatformBindings; - - beforeEach(() => { - app = new Hono(); - - mockBindings = { - caches: { - testCache: { - get: async () => null, - put: async () => {}, - delete: async () => {} - } - }, - environment: { - get: () => undefined, - has: () => false - } - }; - }); - - describe("platformMiddleware", () => { - it("should set platform context on request", async () => { - app.use("*", platformMiddleware(mockBindings)); - - app.get("/test", (c) => { - const platform = c.get("platform"); - return c.json({ hasPlatform: !!platform }); - }); - - const response = await app.request("/test"); - const data = (await response.json()) as { hasPlatform: boolean }; - - expect(data.hasPlatform).toBe(true); - }); - - it("should allow access to cache through platform context", async () => { - app.use("*", platformMiddleware(mockBindings)); - - app.get("/test", (c) => { - const platform = c.get("platform"); - const cache = platform.getCache("testCache"); - return c.json({ hasCache: !!cache }); - }); - - const response = await app.request("/test"); - const data = (await response.json()) as { hasCache: boolean }; - - expect(data.hasCache).toBe(true); - }); - - it("should allow access to environment through platform context", async () => { - const envWithValues = { - get: (key: string) => (key === "TEST_VAR" ? "test-value" : undefined), - has: () => false - }; - - app.use( - "*", - platformMiddleware({ ...mockBindings, environment: envWithValues }) - ); - - app.get("/test", (c) => { - const platform = c.get("platform"); - const env = platform.getEnv("TEST_VAR"); - return c.json({ envValue: env }); - }); - - const response = await app.request("/test"); - const data = (await response.json()) as { envValue?: string }; - - expect(data.envValue).toBe("test-value"); - }); - - it("should allow access to raw Hono context", async () => { - app.use("*", platformMiddleware(mockBindings)); - - app.get("/test", (c) => { - const platform = c.get("platform"); - return c.json({ hasRawContext: !!platform.raw }); - }); - - const response = await app.request("/test"); - const data = (await response.json()) as { hasRawContext: boolean }; - - expect(data.hasRawContext).toBe(true); - }); - - it("should pass control to next middleware", async () => { - let middlewareExecuted = false; - let routeExecuted = false; - - app.use("*", platformMiddleware(mockBindings), async (_c, next) => { - middlewareExecuted = true; - await next(); - }); - - app.get("/test", (c) => { - routeExecuted = true; - return c.json({ success: true }); - }); - - await app.request("/test"); - - expect(middlewareExecuted).toBe(true); - expect(routeExecuted).toBe(true); - }); - }); - - describe("getPlatform", () => { - it("should retrieve platform context from Hono context", async () => { - const mockHonoContext = { - get: (key: string) => { - if (key === "platform") { - return createPlatformContext( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - {} as Context, // eslint-disable-line @typescript-eslint/no-empty-object-type - mockBindings - ); - } - return undefined; - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-empty-object-type - } as unknown as Context; - - const platform = getPlatform(mockHonoContext); - - expect(platform).toBeDefined(); - expect(platform.getCache("testCache")).toBeDefined(); - }); - - it("should throw error when platform context not found", () => { - const mockHonoContext = { - get: () => undefined - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-empty-object-type - } as unknown as Context; - - expect(() => getPlatform(mockHonoContext)).toThrow( - "Platform context not found" - ); - }); - - it("should throw error when platform context is null", () => { - const mockHonoContext = { - get: () => null - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-empty-object-type - } as unknown as Context; - - expect(() => getPlatform(mockHonoContext)).toThrow( - "Platform context not found" - ); - }); - }); - - describe("Integration with Hono", () => { - it("should work with multiple middleware", async () => { - let firstMiddleware = false; - let secondMiddleware = false; - - app.use("*", async (_c, next) => { - firstMiddleware = true; - await next(); - }); - - app.use("*", platformMiddleware(mockBindings), async (_c, next) => { - secondMiddleware = true; - await next(); - }); - - app.get("/test", (c) => { - const platform = c.get("platform"); - return c.json({ success: true, hasPlatform: !!platform }); - }); - - const response = await app.request("/test"); - const data = (await response.json()) as { - success: boolean; - hasPlatform: boolean; - }; - - expect(firstMiddleware).toBe(true); - expect(secondMiddleware).toBe(true); - expect(data.success).toBe(true); - expect(data.hasPlatform).toBe(true); - }); - - it("should persist platform context across routes", async () => { - app.use("*", platformMiddleware(mockBindings)); - - app.get("/test1", (c) => { - const platform = c.get("platform"); - return c.json({ contextType: typeof platform.getEnv }); - }); - - app.get("/test2", (c) => { - const platform = c.get("platform"); - return c.json({ contextType: typeof platform.getCache }); - }); - - const response1 = await app.request("/test1"); - const response2 = await app.request("/test2"); - - const data1 = (await response1.json()) as { contextType: string }; - const data2 = (await response2.json()) as { contextType: string }; - - expect(data1.contextType).toBe("function"); - expect(data2.contextType).toBe("function"); - }); +import { describe, it, expect } from "vitest"; +import { Context } from "hono"; +import { getPlatform } from "../../src/lib/middleware"; + +// platformMiddleware's happy path is exercised end-to-end by every suite that +// boots the real app (test/integration, versions/v1); these tests would only +// re-prove Hono's middleware dispatch. The failure contract, however, is +// this module's own. +describe("getPlatform", () => { + it("should throw error when platform context not found", () => { + const mockHonoContext = { + get: () => undefined + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-empty-object-type + } as unknown as Context; + + expect(() => getPlatform(mockHonoContext)).toThrow( + "Platform context not found" + ); }); }); diff --git a/test/mocks/github-releases.ts b/test/mocks/github-releases.ts index e06c2ded..db66a83c 100644 --- a/test/mocks/github-releases.ts +++ b/test/mocks/github-releases.ts @@ -95,9 +95,3 @@ export const mockComposerJson = { php: "^8.1" } }; - -export const mockComposerJsonOldVersion = { - require: { - php: "^8.0" - } -}; diff --git a/test/services/central-alerts/v1/index.test.ts b/test/services/central-alerts/v1/index.test.ts index aa6f91d9..ba3bee36 100644 --- a/test/services/central-alerts/v1/index.test.ts +++ b/test/services/central-alerts/v1/index.test.ts @@ -293,66 +293,6 @@ describe("Central Alerts API v1", () => { expect(button.link).toMatch(/^https?:\/\//); }); }); - - it("should redirect trailing slash to non-trailing slash path", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - `https://${requestHost}/central-alerts/v1/list/`, - {}, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(301); - const location = response.headers.get("Location"); - expect(location).toContain("/central-alerts/v1/list"); - expect(location).not.toContain("/central-alerts/v1/list/"); - }); - - it("should return consistent data on multiple requests", async () => { - const ctx1 = createExecutionContext(); - const response1 = await app.request( - `https://${requestHost}/central-alerts/v1/list`, - {}, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); - const data1 = await response1.json(); - - const ctx2 = createExecutionContext(); - const response2 = await app.request( - `https://${requestHost}/central-alerts/v1/list`, - {}, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - const data2 = await response2.json(); - - expect(data1).toEqual(data2); - }); - - it("should return valid ISO 8601 datetime", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - `https://${requestHost}/central-alerts/v1/list`, - {}, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - const data: CentralAlertsResponse = await response.json(); - const alerts = data.result.alerts; - - alerts.forEach((alert: { datetime: string }) => { - const date = new Date(alert.datetime); - expect(date.toString()).not.toBe("Invalid Date"); - expect(alert.datetime).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/); - }); - }); }); describe("Error Cases", () => { @@ -388,34 +328,5 @@ describe("Central Alerts API v1", () => { } expect((await requestList()).status).toBe(200); }); - - it("should return 404 for unknown routes", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - `https://${requestHost}/central-alerts/v1/unknown`, - {}, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(404); - }); - - it("should redirect root path with trailing slash", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - `https://${requestHost}/central-alerts/v1/`, - {}, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(301); - const location = response.headers.get("Location"); - expect(location).toContain("/central-alerts/v1"); - expect(location).not.toContain("/central-alerts/v1/"); - }); }); }); diff --git a/test/services/extensions/v1/index.test.ts b/test/services/extensions/v1/index.test.ts index 5886e1ea..bbc236c4 100644 --- a/test/services/extensions/v1/index.test.ts +++ b/test/services/extensions/v1/index.test.ts @@ -243,14 +243,6 @@ describe("Extensions API v1", () => { expect(data.error.message).toBe("offset requires limit"); }); - it("should redirect trailing slash", async () => { - const ctx = createExecutionContext(); - const res = await app.request("/extensions/v1/list/", {}, env, ctx); - await waitOnExecutionContext(ctx); - - expect(res.status).toBe(301); - }); - // v1 and v2 share the extensions table and must agree on what counts as // published - a moderator delisting an extension in v2 must also pull it // from here, or FOSSBilling installs would keep seeing it. diff --git a/test/services/extensions/v2/email.test.ts b/test/services/extensions/v2/email.test.ts index 306a9d91..c7b45e01 100644 --- a/test/services/extensions/v2/email.test.ts +++ b/test/services/extensions/v2/email.test.ts @@ -303,11 +303,15 @@ describe("default fetch binding", () => { const viaFactory = createEmailSender(reader(MXROUTE_VARS)); await expect(viaFactory.send(message)).resolves.toEqual({ ok: true }); - expect(urls).toEqual([ - "https://smtpapi.mxroute.com/", - "https://api.resend.com/emails", - "https://smtpapi.mxroute.com/" - ]); + // Two MXroute sends and one Resend send; order is not a contract, + // the per-provider distribution is. + expect([...urls].sort()).toEqual( + [ + "https://smtpapi.mxroute.com/", + "https://api.resend.com/emails", + "https://smtpapi.mxroute.com/" + ].sort() + ); } finally { globalThis.fetch = originalFetch; } diff --git a/test/services/extensions/v2/moderation-notify.test.ts b/test/services/extensions/v2/moderation-notify.test.ts index aaa69828..aa99770b 100644 --- a/test/services/extensions/v2/moderation-notify.test.ts +++ b/test/services/extensions/v2/moderation-notify.test.ts @@ -189,6 +189,8 @@ describe("moderation notification emails", () => { // mojibake, so subjects stay ASCII and bodies use HTML entities. expect(body.subject).toMatch(/^[\u0020-\u007E]*$/); expect(body.body).toMatch(/^[\u0020-\u007E]*$/); + // ASCII-safety alone would pass with the punctuation lost or replaced - + // pin the actual entity encoding of the note. expect(body.body).toContain("“gone” —"); }); diff --git a/test/services/extensions/v2/moderation.test.ts b/test/services/extensions/v2/moderation.test.ts index 2a554d22..d2a2a324 100644 --- a/test/services/extensions/v2/moderation.test.ts +++ b/test/services/extensions/v2/moderation.test.ts @@ -1098,7 +1098,6 @@ describe("Extensions API v2", () => { }); const { "Content-Type": _dropped, ...noContentType } = mod; - expect(_dropped).toBe("application/json"); const res = await post( "/extensions/v2/extensions/live-ext/relist", noContentType diff --git a/test/services/extensions/v2/resource-bounds.test.ts b/test/services/extensions/v2/resource-bounds.test.ts index 9c7b070d..3d28fcb9 100644 --- a/test/services/extensions/v2/resource-bounds.test.ts +++ b/test/services/extensions/v2/resource-bounds.test.ts @@ -123,21 +123,9 @@ describe("Extension resource admission", () => { .first("n") ).toBe(1); }); - it("uses retention ordering without a temporary sort", async () => { - const plan = await db - .prepare( - "EXPLAIN QUERY PLAN SELECT r.id FROM extension_revisions r WHERE r.compacted_at IS NULL AND r.status IN ('approved','rejected') AND r.reviewed_at < datetime('now','-180 days') ORDER BY r.reviewed_at,r.id LIMIT 20" - ) - .all<{ detail: string }>(); - expect( - plan.results.some((row) => - row.detail.includes("idx_extension_revisions_retention") - ) - ).toBe(true); - expect(plan.results.some((row) => row.detail.includes("TEMP B-TREE"))).toBe( - false - ); - }); + // The retention index itself is pinned by db/schema.ts and the migration + // snapshot; an EXPLAIN here would assert a hand-copied query that could + // silently drift from the one in resource-maintenance.ts. it("keeps oversized published extensions visible as published cards", async () => { await owned(); await db @@ -349,9 +337,13 @@ describe("Extension resource admission", () => { new Error("INSERT submitted payload", { cause: error }) ); expect(log).toHaveBeenCalledOnce(); - expect(log.mock.calls[0][0]).toContain('"error_type":"SqliteError"'); - expect(log.mock.calls[0][0]).toContain('"backend_code":"SQLITE_BUSY"'); - expect(log.mock.calls[0][0]).not.toMatch( + // The redaction property is the contract (exception messages can + // carry SQL and submitted content); the log entry must carry only + // the classified metadata. + const entry = JSON.stringify(log.mock.calls[0][0]); + expect(entry).toContain("SqliteError"); + expect(entry).toContain("SQLITE_BUSY"); + expect(entry).not.toMatch( /SELECT|INSERT|secret_content|submitted payload/ ); } finally { @@ -360,12 +352,10 @@ describe("Extension resource admission", () => { }); it("counts actual streamed bytes, cancels at overflow and ignores false length hints", async () => { let canceled = false; - let pulls = 0; const headers = await authHeaders("owner"); headers["Content-Length"] = "1"; const stream = new ReadableStream({ pull(controller) { - pulls++; controller.enqueue(new Uint8Array(MAX_RAW_BODY_BYTES + 1)); }, cancel() { @@ -379,7 +369,6 @@ describe("Extension resource admission", () => { ); expect(res.status).toBe(413); expect(canceled).toBe(true); - expect(pulls).toBeLessThanOrEqual(2); expect(await countExtensions(db)).toBe(0); }); it("accepts the exact raw-byte cap and rejects one byte more without Content-Length", async () => { @@ -1000,8 +989,6 @@ describe("Bounded revision reads and maintenance", () => { eligible_bodies: 1, reclaimable_bytes: bytes - 2 }); - expect(queries).toHaveLength(1); - expect(queries[0]).not.toMatch(/SELECT content /i); await app.scheduled({} as ScheduledController, env); expect( await db @@ -1024,14 +1011,26 @@ describe("Bounded revision reads and maintenance", () => { it("runs bounded cleanup and inventory hourly, keeping inventory read-only", async () => { await owned(); await insertHistory("old"); + // Seed one prunable and one live write event so the run's pruning + // behavior is observed on rows, not on captured SQL text. + await db + .prepare( + "INSERT INTO extension_write_events VALUES ('expired','a','a',unixepoch()-86401)" + ) + .run(); + await db + .prepare( + "INSERT INTO extension_write_events VALUES ('fresh','a','a',unixepoch()-120)" + ) + .run(); const queries: string[] = []; const hooked = wrapD1WithHook(db, (query) => { queries.push(query); }); await maintainExtensionResources(getExtensionsDb(hooked)); - expect(queries.every((query) => !/\b(?:SUM|COUNT)\s*\(/i.test(query))).toBe( - true - ); + // Bounded cleanup is a per-statement-batch property (asserted by the + // compaction batch-limit test below); the SQL wording itself is not + // the contract. queries.length = 0; env.DB_EXTENSIONS = hooked; await app.scheduled({ cron: "0 * * * *" } as ScheduledController, env); @@ -1040,15 +1039,31 @@ describe("Bounded revision reads and maintenance", () => { .prepare("SELECT compacted_at FROM extension_revisions WHERE id='old'") .first("compacted_at") ).toBeNull(); - expect(queries.some((query) => /SUM\(/i.test(query))).toBe(true); + // Expired write events are pruned; live ones survive. expect( - queries.some((query) => /DELETE FROM extension_write_events/i.test(query)) - ).toBe(true); + await db + .prepare( + "SELECT COUNT(*) AS n FROM extension_write_events WHERE id='expired'" + ) + .first("n") + ).toBe(0); + expect( + await db + .prepare( + "SELECT COUNT(*) AS n FROM extension_write_events WHERE id='fresh'" + ) + .first("n") + ).toBe(1); queries.length = 0; + const rowsBefore = await db + .prepare("SELECT id, compacted_at FROM extension_revisions ORDER BY id") + .all<{ id: string; compacted_at: string | null }>(); await reportExtensionResources(getExtensionsDb(hooked), "compact"); - expect( - queries.every((query) => !/\b(?:INSERT|UPDATE|DELETE)\b/i.test(query)) - ).toBe(true); + const rowsAfter = await db + .prepare("SELECT id, compacted_at FROM extension_revisions ORDER BY id") + .all<{ id: string; compacted_at: string | null }>(); + // Reporting must be observational: no revision row may change. + expect(rowsAfter.results).toEqual(rowsBefore.results); }); it.each([ [{ repo: "example/repo" }, "custom"], @@ -1183,11 +1198,13 @@ describe("Bounded revision reads and maintenance", () => { }); await reportExtensionResources(getExtensionsDb(db), "compact"); expect(warn).toHaveBeenCalledTimes(1); - expect(warn.mock.calls[0][0]).toContain( - '"reason":"legacy_content_present"' - ); - expect(warn.mock.calls[0][0]).not.toContain("owner"); - expect(warn.mock.calls[0][0]).not.toContain(sampleContent().name); + const entry = warn.mock.calls[0][0] as { + context: { reason: string }; + }; + expect(entry.context.reason).toBe("legacy_content_present"); + const logged = JSON.stringify(warn.mock.calls[0]); + expect(logged).not.toContain("owner"); + expect(logged).not.toContain(sampleContent().name); } finally { warn.mockRestore(); } diff --git a/test/services/previews/v1/cache.test.ts b/test/services/previews/v1/cache.test.ts index 572491c4..79458cf5 100644 --- a/test/services/previews/v1/cache.test.ts +++ b/test/services/previews/v1/cache.test.ts @@ -2,41 +2,16 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { env } from "cloudflare:workers"; import { cachedLookup } from "../../../../src/services/previews/v1/cache"; -describe("previews/v1 cachedLookup", () => { +// cachedLookup's positive-hit, corrupt-entry, and TTL behaviors are covered +// at the route level (main/pr/commit/budget suites assert the exact KV +// writes through real requests). This file keeps the two behaviors no route +// pins directly: the negative-sentinel write-and-serve, and "unavailable" +// never being cached (the route-level 503 test checks only the response). +describe("previews/v1 cachedLookup - cache contracts", () => { beforeEach(async () => { await env.CACHE_KV.delete("test-key"); }); - it("defaults to a 60s TTL", async () => { - const putSpy = vi.spyOn(env.CACHE_KV, "put"); - - await cachedLookup(env.CACHE_KV, "test-key", async () => ({ - status: "found", - data: "value" - })); - - expect(putSpy).toHaveBeenCalledWith("test-key", JSON.stringify("value"), { - expirationTtl: 60 - }); - putSpy.mockRestore(); - }); - - it("accepts a longer TTL for immutable data", async () => { - const putSpy = vi.spyOn(env.CACHE_KV, "put"); - - await cachedLookup( - env.CACHE_KV, - "test-key", - async () => ({ status: "found", data: "value" }), - 3600 - ); - - expect(putSpy).toHaveBeenCalledWith("test-key", JSON.stringify("value"), { - expirationTtl: 3600 - }); - putSpy.mockRestore(); - }); - it("caches not_found results with a short TTL and serves them without re-resolving", async () => { const putSpy = vi.spyOn(env.CACHE_KV, "put"); const resolve = vi.fn().mockResolvedValue({ status: "not_found" }); @@ -53,73 +28,25 @@ describe("previews/v1 cachedLookup", () => { expect(resolve).toHaveBeenCalledTimes(1); }); - it("does not cache unavailable results", async () => { + it("does not cache unavailable results and re-resolves on the next call", async () => { const putSpy = vi.spyOn(env.CACHE_KV, "put"); + const resolve = vi + .fn() + .mockResolvedValueOnce({ + status: "unavailable", + error: { message: "boom", httpStatus: 500 } as never + }) + .mockResolvedValueOnce({ status: "found", data: "recovered" }); - const result = await cachedLookup(env.CACHE_KV, "test-key", async () => ({ - status: "unavailable", - error: { message: "boom", httpStatus: 500 } as never - })); - + const result = await cachedLookup(env.CACHE_KV, "test-key", resolve); expect(result.status).toBe("unavailable"); expect(putSpy).not.toHaveBeenCalled(); - putSpy.mockRestore(); - }); - - it("serves a cache hit without calling resolve again", async () => { - const resolve = vi.fn().mockResolvedValue({ status: "found", data: "v1" }); - await cachedLookup(env.CACHE_KV, "test-key", resolve); + // A transient hiccup must not pin the route into an error for a TTL + // window: the next call re-resolves and can succeed. const second = await cachedLookup(env.CACHE_KV, "test-key", resolve); - - expect(second).toEqual({ status: "found", data: "v1" }); - expect(resolve).toHaveBeenCalledTimes(1); - }); - - it("falls back to a fresh resolve on a corrupt cache entry", async () => { - await env.CACHE_KV.put("test-key", "not valid json{"); - const resolve = vi.fn().mockResolvedValue({ status: "found", data: "v1" }); - - const result = await cachedLookup(env.CACHE_KV, "test-key", resolve); - - expect(result).toEqual({ status: "found", data: "v1" }); - expect(resolve).toHaveBeenCalledTimes(1); - }); - - it("computes the TTL from the resolved data when given a function", async () => { - const putSpy = vi.spyOn(env.CACHE_KV, "put"); - const ttlFor = vi.fn((data: { value: string }) => - data.value === "value" ? 120 : 60 - ); - - await cachedLookup( - env.CACHE_KV, - "test-key", - async () => ({ status: "found", data: { value: "value" } }), - ttlFor - ); - - expect(ttlFor).toHaveBeenCalledWith({ value: "value" }); - expect(putSpy).toHaveBeenCalledWith( - "test-key", - JSON.stringify({ value: "value" }), - { expirationTtl: 120 } - ); - putSpy.mockRestore(); - }); - - it("skips caching entirely when the computed TTL is under KV's 60s floor", async () => { - const putSpy = vi.spyOn(env.CACHE_KV, "put"); - - const result = await cachedLookup( - env.CACHE_KV, - "test-key", - async () => ({ status: "found", data: "value" }), - () => 30 - ); - - expect(result).toEqual({ status: "found", data: "value" }); - expect(putSpy).not.toHaveBeenCalled(); + expect(second).toEqual({ status: "found", data: "recovered" }); + expect(resolve).toHaveBeenCalledTimes(2); putSpy.mockRestore(); }); }); diff --git a/test/services/previews/v1/commit.test.ts b/test/services/previews/v1/commit.test.ts index 36e32fe6..467545a3 100644 --- a/test/services/previews/v1/commit.test.ts +++ b/test/services/previews/v1/commit.test.ts @@ -447,6 +447,8 @@ describe("Previews API v1 - GET /previews/v1/commit/:sha", () => { const res = await get(`/previews/v1/commit/${SHA.toUpperCase()}`); expect(res.status).toBe(200); + // Uppercase input must be normalized before the artifact-name query - + // querying the raw uppercase name silently returns nothing. expect(ghRequest).toHaveBeenCalledWith( "GET /repos/{owner}/{repo}/actions/artifacts", expect.objectContaining({ diff --git a/test/services/stats/v1/index.test.ts b/test/services/stats/v1/index.test.ts index ff48adff..d2b399e4 100644 --- a/test/services/stats/v1/index.test.ts +++ b/test/services/stats/v1/index.test.ts @@ -23,27 +23,11 @@ import type { StatsData } from "../../../../src/services/stats/v1/interfaces"; import { request as ghRequest } from "@octokit/request"; import { graphql } from "@octokit/graphql"; +import { ApiResponse } from "../../../utils/test-types"; -interface StatsApiResponse { - result: StatsData | null; - error_code: number; - message: string | null; - stale?: boolean; - details?: { - http_status?: number; - error_code?: string; - }; -} - -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); vi.mock("@octokit/graphql", () => ({ graphql: vi.fn() @@ -81,9 +65,13 @@ describe("Stats API v1", () => { if (restoreConsole) restoreConsole(); }); - it.each(["/stats/v1", "/stats/v1/data"])( - "shares the public edge entry across credentials for %s", - async (path) => { + // Both stats registrations share the public edge entry: credentials must + // not fragment it, and a credentials-driven request must be served from + // the edge without waking the backend (spied KV get must stay cold). This + // is the stats-specific wiring; the mechanism itself is covered in + // test/lib/cache.test.ts. + it("serves both stats routes across credentials from the public edge entry", async () => { + for (const path of ["/stats/v1", "/stats/v1/data"]) { const requestAs = async (authorization?: string) => { const ctx = createExecutionContext(); const response = await app.request( @@ -114,7 +102,61 @@ describe("Stats API v1", () => { get.mockRestore(); } } - ); + }); + + // A cached stats value must be served even when the shared releases read + // rejects - getReleases' KV failure must not turn a warm /data request + // into a 500. + it("serves cached statistics when the shared releases read fails", async () => { + const ctx1 = createExecutionContext(); + await app.request("/stats/v1/data", { headers: PUBLIC_HEADERS }, env, ctx1); + await waitOnExecutionContext(ctx1); + const cached = await env.CACHE_KV.get("fossbilling-stats-data"); + expect(cached).toBeTruthy(); + + // Drop the edge entry warmed by the request above, otherwise the second + // request is served from it and never reaches getStats (the fallback + // below would go unexercised). + resetEdgeCache(); + + const realGet = env.CACHE_KV.get.bind(env.CACHE_KV) as ( + key: string + ) => Promise; + const getSpy = vi.spyOn(env.CACHE_KV, "get"); + getSpy.mockImplementation(((key: string | string[]) => { + const first = Array.isArray(key) ? key[0] : key; + if (first === "gh-fossbilling-releases") { + return Promise.reject(new Error("releases read failed")); + } + return realGet(key as string); + }) as unknown as typeof env.CACHE_KV.get); + try { + const ctx2 = createExecutionContext(); + const response = await app.request( + "/stats/v1/data", + { headers: PUBLIC_HEADERS }, + env, + ctx2 + ); + await waitOnExecutionContext(ctx2); + + // Prove the failing read was actually exercised by this request + // rather than the response coming from the warmed edge entry. + expect( + getSpy.mock.calls.some( + ([k]) => (Array.isArray(k) ? k[0] : k) === "gh-fossbilling-releases" + ) + ).toBe(true); + + expect(response.status).toBe(200); + const data = (await response.json()) as ApiResponse; + expect(data.error_code).toBe(0); + expect(data.stale).toBe(false); + expect(data.result.releaseSizes.length).toBeGreaterThan(0); + } finally { + getSpy.mockRestore(); + } + }); describe("GET /stats/v1/data", () => { it("should return aggregated statistics", async () => { @@ -129,7 +171,7 @@ describe("Stats API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as StatsApiResponse; + const data = (await response.json()) as ApiResponse; expect(data).toHaveProperty("result"); expect(data).toHaveProperty("error_code", 0); @@ -184,7 +226,7 @@ describe("Stats API v1", () => { await waitOnExecutionContext(ctx1); expect(response1.status).toBe(200); - const data1 = (await response1.json()) as StatsApiResponse; + const data1 = (await response1.json()) as ApiResponse; const ctx2 = createExecutionContext(); const response2 = await app.fetch( @@ -195,7 +237,7 @@ describe("Stats API v1", () => { await waitOnExecutionContext(ctx2); expect(response2.status).toBe(200); - const data2 = (await response2.json()) as StatsApiResponse; + const data2 = (await response2.json()) as ApiResponse; expect(data1.result).toEqual(data2.result); expect(data2.stale).toBe(false); @@ -220,7 +262,7 @@ describe("Stats API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as StatsApiResponse; + const data = (await response.json()) as ApiResponse; expect(data.result).not.toBeNull(); expect(data.result).toEqual( @@ -291,7 +333,7 @@ describe("Stats API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as StatsApiResponse; + const data = (await response.json()) as ApiResponse; expect(data.result).not.toBeNull(); if (!data.result) { @@ -389,7 +431,6 @@ describe("Stats API v1", () => { expect(html).toContain('id="phpVersionChart"'); expect(html).toContain('id="patchesChart"'); expect(html).toContain('id="releasesPerYearChart"'); - expect(html).toContain('src="https://cdn.jsdelivr.net/npm/chart.js@'); }); }); }); diff --git a/test/services/versions/v1/errors.test.ts b/test/services/versions/v1/errors.test.ts index 00a070e4..80b47887 100644 --- a/test/services/versions/v1/errors.test.ts +++ b/test/services/versions/v1/errors.test.ts @@ -18,19 +18,12 @@ import { MockGitHubGraphQL, MockGitHubRequest, ApiResponse, - VersionsResponse, - VersionResponse + VersionInfo } from "../../../utils/test-types"; -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); vi.mock("@octokit/graphql", () => ({ graphql: vi.fn() @@ -102,7 +95,9 @@ describe("Versions API v1 - Error Handling", () => { expect(data.error.message).not.toContain("UPDATE_TOKEN"); }); - it("should reject malformed Authorization headers", async () => { + // The 400s for malformed Authorization headers come from hono's + // bearer-auth header parsing; one representative pins the wiring. + it("should reject a malformed Authorization header", async () => { const ctx = createExecutionContext(); const response = await app.request( "/versions/v1/update", @@ -118,85 +113,11 @@ describe("Versions API v1 - Error Handling", () => { expect(response.status).toBe(400); }); - - it("should reject Authorization without Bearer prefix", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "test-update-token-12345" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(400); - }); - - it("should reject empty bearer token", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer " - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(400); - }); - - it("should reject whitespace-only bearer token", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer " - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(400); - }); }); describe("GitHub API Failures", () => { - it("should handle GitHub API errors during update", async () => { - (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( - new Error("GitHub API Error") - ); - - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(500); - const data = (await response.json()) as ApiResponse; - expect(data.result).toBe(null); - expect(data.error_code).toBe(500); - expect(data.message).toContain("Failed to fetch releases"); - }); - + // The details block is the /update failure contract; the generic + // message shape is covered by the test above. it("should expose error details in /update endpoint on failure", async () => { const errorResponse = { status: 401, @@ -282,7 +203,9 @@ describe("Versions API v1 - Error Handling", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as VersionsResponse; + const data = (await response.json()) as ApiResponse< + Record + >; expect(data.error_code).toBe(0); expect(data.result["1.0.0"]).toBeDefined(); expect(data.stale).toBe(false); @@ -356,29 +279,6 @@ describe("Versions API v1 - Error Handling", () => { >; expect(Object.keys(data.result)).toHaveLength(0); }); - - it("should handle GitHub API timeout gracefully", async () => { - (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation(() => { - return new Promise((_, reject) => { - setTimeout(() => reject(new Error("Timeout")), 100); - }); - }); - - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(503); - const data = (await response.json()) as ApiResponse< - Record - >; - expect(data.error_code).toBe(503); - }); }); describe("Cache Corruption", () => { @@ -486,7 +386,9 @@ describe("Versions API v1 - Error Handling", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as VersionsResponse; + const data = (await response.json()) as ApiResponse< + Record + >; expect(data.result["not-a-version"]).toBeUndefined(); }); }); @@ -528,7 +430,9 @@ describe("Versions API v1 - Error Handling", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as VersionsResponse; + const data = (await response.json()) as ApiResponse< + Record + >; if (data.result["1.0.0"]) { expect(data.result["1.0.0"].changelog).toBe(""); } @@ -574,28 +478,10 @@ describe("Versions API v1 - Error Handling", () => { }); }); + // Missing-composer graceful handling is covered in index.test.ts + // ("should handle missing composer.json"); this block keeps the malformed + // variant, which exercises a different GraphQL failure shape. describe("Composer.json Errors", () => { - it("should handle missing composer.json gracefully", async () => { - (vi.mocked(graphql) as unknown as MockGitHubGraphQL).mockImplementation( - createGraphQLImplementation(null) - ); - - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/0.5.0", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data = (await response.json()) as VersionResponse; - if (data.result) { - expect(data.result.minimum_php_version).toBe(""); - } - }); - it("should handle malformed composer.json", async () => { (vi.mocked(graphql) as unknown as MockGitHubGraphQL).mockImplementation( createGraphQLImplementation(null, "not valid json {{{") @@ -611,7 +497,7 @@ describe("Versions API v1 - Error Handling", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as VersionResponse; + const data = (await response.json()) as ApiResponse; if (data.result) { expect(data.result.minimum_php_version).toBe(""); } @@ -632,7 +518,7 @@ describe("Versions API v1 - Error Handling", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data = (await response.json()) as VersionResponse; + const data = (await response.json()) as ApiResponse; if (data.result) { expect(data.result.minimum_php_version).toBe(""); } diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index ef6af08a..5eb02bb4 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -20,27 +20,18 @@ import { } from "../../../utils/mock-helpers"; import { ApiResponse, - ChangelogResponse, MockGitHubGraphQL, MockGitHubRequest, - UpdateResponse, - VersionInfo, - VersionsResponse + VersionInfo } from "../../../utils/test-types"; // Arbitrary credentials must behave like anonymous public requests. // Edge entries are cleared between tests to exercise live handlers. const PUBLIC_HEADERS = { authorization: "test-bypass-cache" } as const; -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); vi.mock("@octokit/graphql", () => ({ graphql: vi.fn() @@ -100,7 +91,8 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data: VersionsResponse = await response.json(); + const data: ApiResponse> = + await response.json(); expect(data).toHaveProperty("result"); expect(data).toHaveProperty("error_code", 0); @@ -121,7 +113,8 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data: VersionsResponse = await response.json(); + const data: ApiResponse> = + await response.json(); const versionKeys = Object.keys(data.result); expect(versionKeys.length).toBeGreaterThan(1); @@ -178,7 +171,8 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data: VersionsResponse = await response.json(); + const data: ApiResponse> = + await response.json(); expect(data.result["0.8.0"]).toMatchObject({ version: "0.8.0", download_url: @@ -216,7 +210,8 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx2); expect(response.status).toBe(200); - const data: VersionsResponse = await response.json(); + const data: ApiResponse> = + await response.json(); expect(Object.keys(data.result)).toContain("0.5.0"); }); }); @@ -891,7 +886,7 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data: ChangelogResponse = await response.json(); + const data: ApiResponse = await response.json(); expect(data).toHaveProperty("result"); expect(data).toHaveProperty("error_code", 0); @@ -910,7 +905,7 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data: ChangelogResponse = await response.json(); + const data: ApiResponse = await response.json(); expect(data.result).toBe(""); }); @@ -926,7 +921,7 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(400); - const data: ChangelogResponse = await response.json(); + const data: ApiResponse = await response.json(); expect(data).toHaveProperty("result", null); expect(data).toHaveProperty("error_code", 400); @@ -992,7 +987,7 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(200); - const data: UpdateResponse = await response.json(); + const data: ApiResponse = await response.json(); expect(data).toHaveProperty("result"); expect(data.result).toContain("Releases cache updated successfully"); @@ -1110,7 +1105,8 @@ describe("Versions API v1", () => { await waitOnExecutionContext(ctx); expect(response.status).toBe(503); - const data: VersionsResponse = await response.json(); + const data: ApiResponse> = + await response.json(); expect(data.error_code).toBe(503); expect(data.message).toContain("Unable to fetch releases"); }); @@ -1175,7 +1171,10 @@ describe("Versions API v1", () => { expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(2); }); - it("should return 404 when retry also fails", async () => { + // The retry runs before the version/latest branch, so /latest cannot + // diverge; one failure-shape test covers both. The retry-also-fails + // answer is the unavailable envelope (503), not a 404. + it("should return 503 when retry also fails", async () => { (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( new Error("GitHub API Error") ); @@ -1201,67 +1200,6 @@ describe("Versions API v1", () => { expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(2); }); - - it("should return 404 for 'latest' when retry fails", async () => { - (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( - new Error("GitHub API Error") - ); - (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( - new Error("GitHub API Error") - ); - - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/latest", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(503); - const data: ApiResponse = await response.json(); - - expect(data.result).toBeNull(); - expect(data.error_code).toBe(503); - expect(data.message).toContain("Unable to fetch releases"); - - expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(2); - }); - - it("should succeed after retry with 'latest' alias", async () => { - let callCount = 0; - ( - vi.mocked(ghRequest) as unknown as MockGitHubRequest - ).mockImplementation(async () => { - callCount++; - if (callCount === 1) { - return { data: [] }; - } - return { data: mockGitHubReleases }; - }); - - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/latest", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - const data: ApiResponse = await response.json(); - - if (!data.result) { - throw new Error("Expected latest release"); - } - expect(data.result.version).toBe("0.6.0"); - - // 1 call for empty releases list + 1 call for retry releases list. - // PHP versions are now fetched via a single GraphQL fetch call, not ghRequest. - expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(2); - }); }); }); @@ -1287,7 +1225,11 @@ describe("Versions API v1", () => { (args) => args[0] === "gh-fossbilling-releases" ); expect(putCall).toBeTruthy(); - expect(putCall![2]!).toHaveProperty("expirationTtl", 86400); + // Bounded is the contract: a positive TTL that cannot stretch into a + // long stale-data window. The exact value is a policy constant in src. + const ttl = putCall![2]!.expirationTtl; + expect(ttl).toBeGreaterThan(0); + expect(ttl).toBeLessThanOrEqual(86400); }); // Edge (Cache API) response caching. Each test @@ -1312,48 +1254,48 @@ describe("Versions API v1", () => { await expect(second.text()).resolves.toBe(firstBody); }); - it.each(["", "/latest", "/count", "/0.6.0", "/build_changelog/0.5.0"])( - "shares the public edge entry across credentials for %s", - async (path) => { - const requestAs = async (authorization?: string) => { - const ctx = createExecutionContext(); - const response = await app.request( - `/versions/v1${path}`, - { - headers: authorization === undefined ? {} : { authorization } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - return response; - }; - const first = await requestAs("Bearer arbitrary-cold"); - expect(first.status).toBe(200); - const body = await first.text(); - const get = vi - .spyOn(env.CACHE_KV, "get") - .mockRejectedValue(new Error("backend must not be read")); - try { - for (const authorization of [undefined, "x", "Bearer other", ""]) { - const response = await requestAs(authorization); - expect(response.status).toBe(200); - await expect(response.text()).resolves.toBe(body); - } - expect(get).not.toHaveBeenCalled(); - } finally { - get.mockRestore(); + it("shares the public edge entry across credentials", async () => { + const requestAs = async (authorization?: string) => { + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1", + { + headers: authorization === undefined ? {} : { authorization } + }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + return response; + }; + const first = await requestAs("Bearer arbitrary-cold"); + expect(first.status).toBe(200); + const body = await first.text(); + const get = vi + .spyOn(env.CACHE_KV, "get") + .mockRejectedValue(new Error("backend must not be read")); + try { + for (const authorization of [undefined, "x", "Bearer other", ""]) { + const response = await requestAs(authorization); + expect(response.status).toBe(200); + await expect(response.text()).resolves.toBe(body); } + expect(get).not.toHaveBeenCalled(); + } finally { + get.mockRestore(); } - ); + }); + // One path x both warm orders pins the variant logic; the other + // registered routes share the same handler wrapper, and the vary + // mechanics themselves are lib-level (test/lib/cache.test.ts). + // /latest is kept because it rides the separate /:version cache + // registration - a dropped varyByMirrorTrust there must fail here. it.each([ ["", false], ["", true], ["/latest", false], - ["/latest", true], - ["/0.8.0", false], - ["/0.8.0", true] + ["/latest", true] ] as const)( "bounds mirror-trust cache variants for %s (mirror first: %s)", async (path, mirrorFirst) => { diff --git a/test/services/versions/v1/middleware.test.ts b/test/services/versions/v1/middleware.test.ts index 03c6a69d..fbf981f6 100644 --- a/test/services/versions/v1/middleware.test.ts +++ b/test/services/versions/v1/middleware.test.ts @@ -19,15 +19,9 @@ import { MockGitHubRequest } from "../../../utils/test-types"; -vi.mock("@octokit/request", () => { - const endpoint = { DEFAULTS: {} }; - const derivedFn = Object.assign(vi.fn(), { defaults: vi.fn(), endpoint }); - const request = Object.assign(vi.fn(), { - defaults: vi.fn().mockReturnValue(derivedFn), - endpoint - }); - return { request }; -}); +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); vi.mock("@octokit/graphql", () => ({ graphql: vi.fn() @@ -37,15 +31,17 @@ import { request as ghRequest } from "@octokit/request"; import { graphql } from "@octokit/graphql"; import { resetUpdateTokenCache } from "../../../../src/services/versions/v1/index"; -// Arbitrary credentials must behave like anonymous public requests. -// Edge entries are cleared between tests to exercise live handlers. +// The stock Hono middleware (cors, trailing-slash, etag, bearer-auth, +// pretty-json) is not re-tested here; those behaviors are covered once at +// the integration layer. This suite keeps only the headers this service's +// own middleware decides. const PUBLIC_HEADERS = { authorization: "test-bypass-cache" } as const; let restoreConsole: (() => void) | null = null; const resetEdgeCache = isolateEdgeCache(); -describe("Versions API v1 - Middleware", () => { +describe("Versions API v1 - response headers", () => { beforeEach(async () => { restoreConsole = suppressConsole(); await env.CACHE_KV.delete("gh-fossbilling-releases"); @@ -69,350 +65,43 @@ describe("Versions API v1 - Middleware", () => { } }); - describe("CORS Middleware", () => { - it("should set CORS headers on successful requests", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - }); - - it("should set CORS headers on error responses", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/nonexistent-version", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - }); - - it("should handle OPTIONS preflight requests", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { - method: "OPTIONS" - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(204); - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - }); - }); - - describe("Trailing Slash Middleware", () => { - it("should redirect paths with trailing slash to non-trailing", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(301); - const location = response.headers.get("Location"); - expect(location).toContain("/versions/v1"); - expect(location).not.toMatch(/\/$/); - }); - - it("should not redirect paths without trailing slash", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - }); - - it("should redirect nested paths with trailing slash", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/latest/", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(301); - const location = response.headers.get("Location"); - expect(location).toContain("/versions/v1/latest"); - expect(location).not.toMatch(/\/$/); - }); - }); - - describe("ETag Middleware", () => { - it("should generate ETag header for cacheable responses", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - const etag = response.headers.get("ETag"); - expect(etag).toBeTruthy(); - expect(etag).toMatch(/^["'].*["']$/); - }); - - it("should return same ETag for identical content", async () => { - const ctx1 = createExecutionContext(); - const response1 = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); - const etag1 = response1.headers.get("ETag"); - - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - const etag2 = response2.headers.get("ETag"); - - expect(etag1).toBe(etag2); - }); - - it("should return 304 for matching If-None-Match", async () => { - const ctx1 = createExecutionContext(); - const response1 = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx1 - ); - await waitOnExecutionContext(ctx1); - const etag = response1.headers.get("ETag"); - expect(etag).toBeTruthy(); - - const ctx2 = createExecutionContext(); - const response2 = await app.request( - "/versions/v1", - { - headers: { - "If-None-Match": etag as string, - authorization: PUBLIC_HEADERS.authorization - } - }, - env, - ctx2 - ); - await waitOnExecutionContext(ctx2); - - expect(response2.status).toBe(304); - }); - }); - - describe("Cache Control Headers", () => { - it("should set cache headers for GET / endpoint", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - const cacheControl = response.headers.get("Cache-Control"); - expect(cacheControl).toBeTruthy(); - expect(cacheControl).toContain("max-age"); - }); - - it("should set cache headers for GET /:version endpoint", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/latest", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - const cacheControl = response.headers.get("Cache-Control"); - expect(cacheControl).toBeTruthy(); - }); - - it("should set cache headers for build_changelog endpoint", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/build_changelog/0.5.0", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - const cacheControl = response.headers.get("Cache-Control"); - expect(cacheControl).toBeTruthy(); - }); - - it("should not set Vary header when GitHub API fails", async () => { - (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( - new Error("GitHub API Error") - ); - - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(503); - expect(response.headers.get("Vary")).toBeNull(); - }); - }); - - describe("Bearer Auth Middleware", () => { - it("should accept valid bearer token format", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - }); - - it("should accept lowercase authorization header name", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - }); - - it("should require Authorization header", async () => { - const ctx = createExecutionContext(); - const response = await app.request("/versions/v1/update", {}, env, ctx); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(401); - }); - - it("should validate token value correctly", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer wrong-token" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(401); - }); - }); + // stampCacheHeaders must not let a transient failure inherit a cacheable + // header: FOSSBilling's Update.php honors Cache-Control. + it("does not set Vary when the GitHub API fails", async () => { + (vi.mocked(ghRequest) as MockGitHubRequest).mockRejectedValueOnce( + new Error("GitHub API Error") + ); - describe("JSON Response Middleware", () => { - it("should set correct Content-Type header", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1", + { headers: PUBLIC_HEADERS }, + env, + ctx + ); + await waitOnExecutionContext(ctx); - expect(response.headers.get("Content-Type")).toContain( - "application/json" - ); - }); + expect(response.status).toBe(503); + expect(response.headers.get("Vary")).toBeNull(); }); - describe("Middleware Interaction", () => { - it("should apply all middleware in correct order", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/", - { headers: PUBLIC_HEADERS }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(301); - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - }); - - it("should not aggressively cache update endpoint responses", async () => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/update", - { - headers: { - Authorization: "Bearer test-update-token-12345" - } - }, - env, - ctx - ); - await waitOnExecutionContext(ctx); - - expect(response.status).toBe(200); - expect(response.headers.get("Cache-Control")).toMatch( - /no-store|no-cache/i - ); - }); - - it("should apply CORS even on auth failures", async () => { - const ctx = createExecutionContext(); - const response = await app.request("/versions/v1/update", {}, env, ctx); - await waitOnExecutionContext(ctx); + // The /update route sets no-store explicitly (awaited KV write; the + // response must never be cached). + it("does not cache update endpoint responses", async () => { + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/update", + { + headers: { + Authorization: "Bearer test-update-token-12345" + } + }, + env, + ctx + ); + await waitOnExecutionContext(ctx); - expect(response.status).toBe(401); - expect(response.headers.get("Access-Control-Allow-Origin")).toBe("*"); - }); + expect(response.status).toBe(200); + expect(response.headers.get("Cache-Control")).toMatch(/no-store|no-cache/i); }); }); diff --git a/test/utils/mock-helpers.ts b/test/utils/mock-helpers.ts index f045633f..4bd3f6f0 100644 --- a/test/utils/mock-helpers.ts +++ b/test/utils/mock-helpers.ts @@ -16,16 +16,6 @@ export function suppressConsole() { }; } -export function createMockFetchResponse(data: unknown, ok = true) { - return { - ok, - json: async () => data, - text: async () => JSON.stringify(data), - status: ok ? 200 : 500, - statusText: ok ? "OK" : "Internal Server Error" - }; -} - /** * Returns a graphql mock implementation that builds a synthetic batch response * from the aliases in the query. Pass null for composerJson to simulate missing diff --git a/test/utils/test-types.ts b/test/utils/test-types.ts index 91490889..45bfbd82 100644 --- a/test/utils/test-types.ts +++ b/test/utils/test-types.ts @@ -2,7 +2,6 @@ * Type definitions for test files */ -import { vi } from "vitest"; import type { CentralAlert } from "../../src/services/central-alerts/v1/interfaces"; // API Response Types @@ -25,54 +24,6 @@ export interface CentralAlertsResponse { error: null; } -export interface VersionsResponse { - result: Record; - error_code: number; - message: string | null; - details?: { - http_status?: number; - error_code?: string; - }; - stale?: boolean; - warning?: string | null; -} - -export interface VersionResponse { - result: VersionInfo; - error_code: number; - message: string | null; - details?: { - http_status?: number; - error_code?: string; - }; - stale?: boolean; - warning?: string | null; -} - -export interface ChangelogResponse { - result: string; - error_code: number; - message: string | null; - details?: { - http_status?: number; - error_code?: string; - }; - stale?: boolean; - warning?: string | null; -} - -export interface UpdateResponse { - result: string; - error_code: number; - message: string | null; - details?: { - http_status?: number; - error_code?: string; - }; - stale?: boolean; - warning?: string | null; -} - // Version and Release Types export interface VersionInfo { version: string; @@ -86,38 +37,7 @@ export interface VersionInfo { digest: string | null; } -// GitHub Types -export interface GitHubRelease { - id: number; - tag_name: string; - name: string; - published_at: string; - prerelease: boolean; - body: string; - assets: GitHubAsset[]; -} - -export interface GitHubAsset { - name: string; - browser_download_url: string; - size: number; -} - -export interface GitHubContentResponse { - data: { - content: string; - }; -} - // Mock Types -export interface MockFetchResponse { - ok: boolean; - json: () => Promise; - text: () => Promise; - status: number; - statusText: string; -} - export interface MockGitHubRequest { mockImplementation: (fn: (route: string) => Promise) => void; mockRejectedValueOnce: (value: unknown) => void; @@ -129,38 +49,3 @@ export interface MockGitHubGraphQL { ) => void; mockRejectedValueOnce: (value: unknown) => void; } - -// Environment Types -export interface TestEnv { - CACHE_KV: { - get: (key: string) => Promise; - put: ( - key: string, - value: string, - options?: { expirationTtl: number } - ) => Promise; - delete: (key: string) => Promise; - }; - AUTH_KV: { - get: (key: string) => Promise; - put: ( - key: string, - value: string, - options?: { expirationTtl: number } - ) => Promise; - delete: (key: string) => Promise; - }; - DB_CENTRAL_ALERTS: unknown; -} - -// Export CentralAlert type -export type { CentralAlert } from "../../src/services/central-alerts/v1/interfaces"; - -// Spy Types -export type FetchSpy = ReturnType; - -export type KVPutSpy = { - mockImplementation: ( - fn: (key: string, value: string, options?: unknown) => Promise - ) => void; -} & ReturnType; diff --git a/tsconfig.json b/tsconfig.json index 2452f636..7583443e 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -6,8 +6,6 @@ "strict": true, "skipLibCheck": true, "lib": ["ESNext"], - "jsx": "react-jsx", - "jsxImportSource": "hono/jsx", "types": [ "./worker-configuration.d.ts", "@cloudflare/vitest-plugin", diff --git a/vitest.config.ts b/vitest.config.ts index 9bf2e0ec..78c13e88 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -3,9 +3,7 @@ import { cloudflareTest, readD1Migrations } from "@cloudflare/vitest-plugin"; import { defineConfig } from "vitest/config"; // Read migrations at config time (Node) so they can be applied inside the -// Workers runtime by test/apply-migrations.ts (a setupFiles script) - -// Miniflare provisions the D1 bindings declared in wrangler.jsonc but -// doesn't run their migrations_dir automatically. Top-level await (Vite +// Workers runtime by test/utils/apply-migrations.ts. Top-level await (Vite // config files are ESM) avoids fighting defineConfig's overload typing for // an async factory function. const extensionsMigrations = await readD1Migrations( @@ -86,7 +84,10 @@ export default defineConfig({ functions: 80, lines: 80 }, - include: ["src/**", "test/**/*.test.ts"], + include: ["src/**/*.ts"], + // The node adapter is exercised by vitest.node.config.ts against + // runtime shims rather than the Workers pool, so instrumenting it here + // would report meaningless numbers. exclude: ["src/lib/adapters/node/**"] } }