diff --git a/.env.example b/.env.example index a1fa421..e2f57f4 100644 --- a/.env.example +++ b/.env.example @@ -50,3 +50,9 @@ LOG_LEVEL=info # --- CORS --- # CORS_ORIGINS=https://example.com,https://staging.example.com + +# --- Webhooks --- +# Outbound webhooks are blocked from targeting private/loopback/link-local +# hosts (SSRF guard). Set to true ONLY if you intentionally deliver to internal +# hosts and trust every configured target. +# WEBHOOK_ALLOW_PRIVATE_HOSTS=false diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0ca1411..931198c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -69,7 +69,7 @@ jobs: with: node-version: '24' cache: npm - cache-dependency-path: ${{ steps.meta.outputs.dir }}/package-lock.json + cache-dependency-path: package-lock.json - name: Log toolchain run: | @@ -86,9 +86,18 @@ jobs: exit 1 fi - - name: Install - working-directory: ${{ steps.meta.outputs.dir }} - run: npm ci || npm install --no-audit --no-fund + # Install from the workspace ROOT — the root package-lock.json is the + # single authoritative lockfile (per-package lockfiles were removed; they + # drifted out of sync and silently forced a non-deterministic `npm install`). + - name: Install (workspace root) + run: npm ci --include-workspace-root --workspaces + + # Publish gate: don't ship if the packages' own tests are red. These live + # in the root tests/unit suite (cms-client.test.ts / site-kit.test.ts). + - name: Unit tests (publish gate) + env: + SESSION_SECRET: ci_release_secret_ci_release_secret_ci_release_secret_ci + run: npm run test:unit - name: Build working-directory: ${{ steps.meta.outputs.dir }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bcfbddc..aa38cdc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -51,6 +51,12 @@ jobs: (cd packages/cms-client && npm run typecheck) (cd packages/site-kit && npm run typecheck) + - name: Build (compile the shipped dist/ — NodeNext catches ESM issues) + run: | + npm run build + (cd packages/cms-client && npm run build) + (cd packages/site-kit && npm run build) + - name: Unit tests run: npm run test:unit @@ -62,4 +68,35 @@ jobs: DB_PASSWORD: '' DB_NAME: skelpo_test SESSION_SECRET: ci_test_secret_ci_test_secret_ci_test_secret_ci_test - run: npm run test:integration + run: | + set -o pipefail + npm run test:integration 2>&1 | tee /tmp/itest.log + # Tripwire: fail if the suite ran zero tests, or skipped ALL of them + # (e.g. the MySQL service was unreachable) — otherwise CI is green + # having verified nothing. + total=$(grep -oE 'tests [0-9]+' /tmp/itest.log | tail -1 | grep -oE '[0-9]+' || echo 0) + skipped=$(grep -oE 'skipped [0-9]+' /tmp/itest.log | tail -1 | grep -oE '[0-9]+' || echo 0) + if [ "${total:-0}" -eq 0 ]; then echo "::error::integration suite ran 0 tests"; exit 1; fi + if [ "${skipped:-0}" -eq "${total:-0}" ]; then echo "::error::all ${total} integration tests were skipped (MySQL unreachable?)"; exit 1; fi + + - name: Boot smoke (compiled dist/server.js serves /healthz) + env: + DB_HOST: 127.0.0.1 + DB_PORT: '3306' + DB_USER: root + DB_PASSWORD: '' + DB_NAME: skelpo_test + SESSION_SECRET: ci_test_secret_ci_test_secret_ci_test_secret_ci_test + PORT: '3137' + HOST: 127.0.0.1 + run: | + node dist/server.js & + pid=$! + ok=0 + for i in $(seq 1 30); do + if curl -fsS "http://127.0.0.1:3137/healthz" >/dev/null 2>&1; then ok=1; break; fi + sleep 1 + done + kill "$pid" 2>/dev/null || true + if [ "$ok" -ne 1 ]; then echo "::error::compiled dist/server.js did not serve /healthz"; exit 1; fi + echo "boot smoke OK" diff --git a/.gitignore b/.gitignore index 81b015b..fd2fb1a 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,7 @@ __perry_js_bundle.js # Runtime data uploads/ +uploads-*/ .cache/ *.skelpo-backup diff --git a/CLAUDE.md b/CLAUDE.md index cb2fc17..179e9d9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -148,19 +148,74 @@ implementing the interface — every byte path already routes through it. ## Perry-native facts -- **`Bun.serve` is not implemented in Perry** as of Perry 0.5.1019. A - `Bun` sentinel object exists but `Bun.serve` is `undefined`, and it's - not on `globalThis`. The CMS server's Bun-detect fallback path - therefore does NOT yet boot natively — verify before claiming it does. -- For a Perry-native HTTP server today, use **Fastify** (Perry has a - native Rust impl in `perry-stdlib`) — it compiles and runs cleanly. - `node:http.createServer` compiles but the response body/headers don't - propagate (returns `content-length: 0`); don't rely on it. -- `@skelpo/cms-client` and `@skelpo/site-kit` **lack a `"perry": - "./src/index.ts"` exports entry** (and don't ship `src/`), so the - customer site can't cross-compile against them yet. `@perryts/mysql` - is the reference for the right shape (`perry` export + `src/` in - `files`). Adding this is the open blocker for compiling perry.land. +Status on **Perry 0.5.1039**: the CMS **compiles to a native binary and +boots** — migrate, seed, job worker, MySQL, and the HTTP listener bind are +all native and verified. **HTTP request handling does NOT yet work**: every +request throws `Symbol()` inside `app.fetch` and returns 500 (see the open +runtime bugs below). So "compiles + boots + listens" ✅, "serves real +responses" ❌ — not production-usable on Perry yet. + +Build it with **`npm run build:perry`** (do not call `perry` directly — see +the invocation pitfall below). The old CLI subcommand was `perry build`; +it's now **`perry compile`**. + +- **JS-only deps must be AOT-compiled (V8 runtime was removed).** Perry + can no longer evaluate JS at runtime, so any npm dep shipped as compiled + JS must be listed in **`perry.compilePackages`** (and + `perry.allow.compilePackages`) in `package.json`. We do this for + `hono` and `bcryptjs`. Without it: "JavaScript runtime (V8) support has + been removed." +- **Invoke perry from its REAL path, not the `~/.cargo/bin/perry` + symlink.** The compiler finds its workspace (and the on-demand + "auto-optimize" step that builds + links the per-feature ext libs, + incl. the node:http server lib `libperry_ext_http.a`) by walking up + from `current_exe()` for `crates/perry-runtime`. Through the symlink + that resolves to `~/.cargo/bin` → workspace not found → only + `libperry_runtime.a` + `libperry_stdlib.a` get linked → node:http + server symbols are unresolved → the binary dies at the HTTP bind with + `TypeError: value is not a function`. **`PERRY_RUNTIME_DIR` does NOT + fix this.** `scripts/build-perry.sh` resolves the symlink for us. +- **node:http server partially works.** `createServer`/`server.listen`/ + `res.end`/`res.write` (sync + async) and the listener bind all work + natively. `IncomingMessage`: `req.method`/`req.url` work, **`req.on('data')`/ + `req.on('end')` fire** (attach them **synchronously** in the createServer + callback — Perry fires them eagerly, so a deferred/`await`-ed registration + misses them and hangs). The old "body/headers don't propagate" note is + obsolete for the basics. +- **OPEN Perry runtime bugs blocking serve** (file/track upstream; all + reproduced on 0.5.1039 in an isolated worktree build): + 1. **`req.headers` is `undefined`** on `IncomingMessage`. Workaround: + read **`req.rawHeaders`** (flat `[k,v,k,v,…]`, populated correctly) + and rebuild a `Headers`. `rawHeaders` is portable to Node too. + 2. **request body chunks arrive as `string`, not `Buffer`** — so + `Buffer.concat(chunks)` throws "list[0] … must be Buffer/Uint8Array". + Coerce each chunk (`Buffer.from(chunk)`) before concat. + 3. **THE blocker: `app.fetch` throws a bare `Symbol()` on every request** + (even header-less `GET /healthz`, a pure `c.json`), so all routes 500. + A trivial 1-route Hono app served through the *same* inline adapter + works (200), and `c.json`/`c.text`/`getCookie`/`c.req.header` all work + in isolation — so the trigger is somewhere in the CMS's full + middleware/route graph under compilation, not yet isolated. This is + the thing to chase next. +- **Do NOT use `@hono/node-server`** under Perry. Its `serve()` now *binds* + (Perry #2533 fixed), but its request path throws and then crashes inside + its own catch handler (`e.name` on an undefined caught value). `server.ts` + should serve via an inline `node:http` adapter bridging + `(req,res)` ⇄ `app.fetch` instead — but note that adapter is **not yet + working end-to-end** because of bug 3 above. `@hono/node-server` is no + longer in `perry.compilePackages`. +- **No `foo!++`** — a non-null assertion on an update expression trips + `U006` ("Update expression only supports identifiers and member + expressions"). Drop the `!` (or use `+= 1`). +- **`Bun.serve` is still unimplemented** (a `Bun` sentinel exists, but + `Bun.serve` is `undefined` and not on `globalThis`) — the intended + Perry path is node:http, not Bun. +- `@skelpo/cms-client` / `@skelpo/site-kit` still **lack a `"perry"` + exports entry** and don't ship `src/`. With `compilePackages` a + consumer can now compile their published JS directly, so this is no + longer a hard blocker — but cross-compiling the customer site against + them this way is **unverified**. `@perryts/mysql` remains the + reference shape (`perry` export + `src/` in `files`). ## Customer site (separate repo) @@ -168,10 +223,11 @@ perry.land's Perry-native rewrite lives at `~/projects/perry-landing-skelpo` → pushed to `PerryTS/perryts.com` branch **`perry-native`**. Hono + JSX + Tailwind v4, depends on the two `@skelpo/*` packages from npm. Deployed at **beta.perryts.com** via `deploy.sh`: cross-compile on a -Linux worker (`root@84.32.98.120`, Perry 0.5.1018) → relay binary → +Linux worker (`root@builder.perryts.com`, Perry 0.5.1018) → relay binary → `root@webserver.skelpo.net` → pm2/nginx. Currently runs the -`--node-fallback` (tsx) path because the Perry compile is blocked on -the missing `perry:` exports above. +`--node-fallback` (tsx) path; a native Perry compile of the customer +site hasn't been re-attempted since the CMS-side findings above +(`compilePackages` + real-binary invocation + inline node:http adapter). ## Benchmarks diff --git a/docs/media-pipeline.md b/docs/media-pipeline.md new file mode 100644 index 0000000..5c3c918 --- /dev/null +++ b/docs/media-pipeline.md @@ -0,0 +1,287 @@ +# Responsive media pipeline — design + +Status: **proposal** (2026-06-19). Owner: media subsystem. + +Goal: serve every image **at (near-)exactly the size and format the requesting +device needs**, with **caching that is correct by construction**, fully +integrated into the CMS origin — no generic pre-baked thumbnails, no reliance on +a half-baked CDN image resizer. Because the CMS is (or will be) the native +origin, it can do this better than a bolt-on CDN: it knows the source bytes, the +focal point, *and* the layout. + +This doc is runtime-agnostic in its URL/cache/markup design; only the codec +implementation differs between Node (today) and Perry-native (later). + +--- + +## 1. Where we are today + +- Media rows already store `width`, `height`, `focalPoint` ({x,y} JSON), `sizeBytes`, + `mimeType`, and **mandatory `altText`** (`migrations/0001_initial.sql`, + `src/media/store.ts`). +- `GET /api/v1/media/:id/raw` → original bytes (`Cache-Control: immutable`), or + 302 to a backend public URL via `mediaPublicUrl` (`src/routes/api/media.ts`). +- `GET /api/v1/media/:id/url?w&h&format&quality&fit&gravity=focal` → a signed + **imgproxy** URL (external service; currently offline). +- Site `Picture` (`verrano/site/src/ui.tsx`): with imgproxy off it points + `` straight at `/raw` — i.e. **ships the full original to every device** + (bad LCP/bandwidth/CWV). With imgproxy on it uses `@skelpo/site-kit` + `buildResponsiveImage` → imgproxy srcset. + +**Decision:** `/raw` becomes the **canonical original / fallback**. Page markup +must reference sized, format-negotiated derivatives. Retire imgproxy as the +responsive path; replace with an integrated derivative endpoint. + +--- + +## 2. The core tension: "exact size" vs "cacheable" + +The server cannot know an image's *rendered* size unless the client tells it. +Two mechanisms exist, neither complete alone: + +1. **`srcset`/`sizes` (universal).** Server offers a *menu* of widths; the + browser picks the smallest candidate satisfying layout × DPR. Works in every + browser. Rounds up to the nearest offered width (not pixel-exact). +2. **Client Hints (`Sec-CH-Width`, `Sec-CH-DPR`).** Browser sends the exact + computed layout width + DPR in request headers → server returns the precise + size. This is the true "exactly what's needed" path — **Chromium only** + (Safari/Firefox do not send these). An *enhancement*, never the whole answer. + +**The "exact" trap:** honoring arbitrary requested widths makes every viewport a +distinct derivative → unbounded CPU/disk and a trivial DoS (`?w=1,2,3,…`). The +fix is **quantization**: snap any requested width *up* to a fixed ladder, clamp +to source width and a max. Visually indistinguishable from exact; cache-bounded +and safe. This single rule separates a robust system from a toy. + +Ladder (initial): `[320, 420, 540, 640, 768, 960, 1080, 1280, 1440, 1680, 1920, +2240, 2560, 3200, 3840]`, clamped to `min(sourceWidth, 3840)`. Tune later +(possibly perceptually spaced). DPR handled by the browser via `srcset` +(picks a 2× candidate) or folded into the client-hint computation. + +--- + +## 3. Architecture + +### 3.1 Content-addressed, immutable derivative URLs + +Every transform param — including a short hash of the source bytes — lives in +the path, so the URL fully determines the bytes: + +``` +/api/v1/media/{id}/{srcHash8}/{w}x{h}-{fit}-{focal}-q{q}.{avif|webp|jpeg} +# examples +/api/v1/media/5/9f3a1c7e/824x0-cover-c50_40-q72.avif # focal crop, AVIF +/api/v1/media/5/9f3a1c7e/640x0-fit-q80.webp # plain fit, WebP +``` + +- `srcHash8` = first 8 hex of SHA-256 of the source bytes (stored on the row). +- `w`/`h`: target box; `0` = derive from the other + aspect. Always quantized. +- `fit`: `cover` (crop) | `fit` (contain, no crop). +- `focal`: `c{xx}_{yy}` (focal point ×100) when `fit=cover`, else omitted. +- `q`: quality (or a `t{ssim}` target — see §3.6). +- extension = output format. + +Because the URL is a pure function of the output bytes: + +``` +Cache-Control: public, max-age=31536000, immutable +ETag: "{derivativeHash}" # strong; enables 304 +# NO Vary needed +``` + +Re-edit/replace the image → new `srcHash` → new URLs → automatic cache-bust, no +purge API. **`Vary`-free is the whole point** — it's where generic CDN image +resizers (Vary-on-Width / Vary-on-Accept) cache poorly in shared caches. + +### 3.2 First-request generation + persistent derivative cache + +1. Request hits the derivative URL. +2. Look up the derivative by key in the cache (disk/S3, behind the existing + `MediaStorage` interface — new prefix, e.g. `derivatives/`). +3. Hit → stream it (static-file fast path). +4. Miss → decode source, transform, encode, **persist**, then stream. +5. Concurrent misses for the same key coalesce (single-flight lock) so a cold + popular image isn't transformed N times. + +Derivatives are disposable — cheap to regenerate, optional LRU/size-cap eviction. + +### 3.3 Format negotiation via `` (not `Accept`+Vary) + +Emit ``, then `image/webp`, then a JPEG `` +fallback. The browser declares its choice by *selecting a source*, so format is +**in the URL** → still no `Vary`. AVIF is typically 30–50% smaller than JPEG at +equal quality → direct LCP win. (Server still validates/limits which formats it +will emit.) + +### 3.4 Focal-point art-direction — the structural advantage + +We already store `focalPoint`. The endpoint crops to the **exact aspect ratio +the layout asks for**, centered on the focal point, at every breakpoint. A +generic CDN cannot do this — it has no subject metadata. This is what makes +output look hand-cropped everywhere. Default focal `{0.5,0.5}` when unset. + +### 3.5 `site-kit` markup generator + +Replace `buildResponsiveImage`'s imgproxy target with derivative URLs. It emits: + +- `` with AVIF/WebP/JPEG ``s, +- a quantized `srcset` width ladder (clamped to source width), +- a correct `sizes` per placement (caller-provided; CMS can compute from layout), +- `width`/`height` attrs (kills CLS), +- `loading=lazy` below the fold; `fetchpriority=high` + `` for the LCP image, +- the LQIP placeholder as inline background (§3.7), +- the mandatory `alt`. + +Public API sketch: + +```ts +buildResponsiveImage({ + cmsBase, mediaId, srcHash, sourceWidth, + aspectRatio?, // forces a focal cover-crop to this ratio + sizes, // e.g. "(max-width:768px) 100vw, 640px" + formats?: ['avif','webp','jpeg'], + quality?, ladder?, // overrides +}) => ResponsiveImage +imageHtml(img, { alt, loading, fetchPriority, className }) => string +``` + +### 3.6 Quality targeting (optional, phase 3+) + +Instead of a fixed `q`, target a perceptual quality (SSIM/butteraugli) or a +target byte-size per derivative, per format. Yields smaller files at equal +perceived quality. Cache key uses the *resolved* `q` so URLs stay immutable. + +### 3.7 LQIP / BlurHash + +At upload, generate a tiny blur placeholder (BlurHash string or a ~20px inline +data-URI) and store it on the media row. Inline as the element background → +instant first paint, zero layout shift while the real image loads. + +### 3.8 Optional Client-Hints "exact" upgrade (phase 3) + +Opt in with `Accept-CH: Sec-CH-Width, Sec-CH-DPR` (+ `Critical-CH`). On browsers +that send them, a bare `GET /api/v1/media/:id` (or `/:id/auto`) computes +`ceil(width × dpr)`, **quantizes**, picks format from ``/Accept, and +**302-redirects to the immutable derivative URL**. Best of both: device-exact +sizing *and* perfect downstream caching. Non-supporting browsers ignore this and +use the `srcset` menu. The redirect response itself: short/no cache, `Vary: +Sec-CH-Width, Sec-CH-DPR, Accept` (only on this thin redirect, never on bytes). + +--- + +## 4. Schema changes + +Add to the `media` table (new migration): + +| column | type | purpose | +|--------|------|---------| +| `srcHash` | `CHAR(64)` | SHA-256 of source bytes → derivative URL hash + cache-bust | +| `blurhash` | `VARCHAR(64)` NULL | LQIP placeholder | +| `dominantColor` | `CHAR(7)` NULL | optional bg before blur paints | + +Backfill `srcHash`/`blurhash` for existing rows via a one-off job (read bytes, +hash, blur). New uploads compute them inline. + +--- + +## 5. `MediaTransformer` interface (runtime-agnostic) + +Mirror the storage-backend pattern (`MEDIA_BACKEND`). Routes/markup/cache never +change between implementations. + +```ts +interface TransformRequest { + source: Uint8Array; + width: number; height: number; fit: 'cover' | 'fit'; + focal?: { x: number; y: number }; + format: 'avif' | 'webp' | 'jpeg'; + quality: number; +} +interface MediaTransformer { + transform(req: TransformRequest): Promise; + probe(bytes: Uint8Array): Promise<{ width: number; height: number; mime: string }>; + blurhash(bytes: Uint8Array): Promise; +} +``` + +- **Node impl (now):** `sharp` (libvips) — decode/resize/encode AVIF/WebP/JPEG, + focal crop, blurhash. Ships today on the current Node deployment. +- **Perry-native impl (later):** a `@perryts/image`-style lib wrapping a Rust + stack — `fast_image_resize` + `ravif`/`rav1e` (AVIF), `webp`, `mozjpeg`, + decoders via `image`/`zune-image`. This is the real engineering cost of "fully + native"; `sharp` is a native Node addon Perry can't run. The interface lets us + defer it without blocking the rest. + +Selected via `MEDIA_TRANSFORMER=sharp|perry` (default `sharp`). + +--- + +## 6. Caching, correctness, security + +- Derivative bytes: `immutable` + strong `ETag`; honor `If-None-Match` → 304. +- No `Vary` on byte responses (format/size are in the URL). `Vary` only on the + thin client-hint redirect. +- **Anti-flood:** only quantized ladder widths accepted; clamp to source width + + hard max (3840) and max megapixels; allowlist formats; cap quality range. + Reject/normalize off-ladder params (302 to nearest, or 400). Optionally sign + derivative URLs (HMAC) so only CMS-emitted variants generate — prevents a + derivative-cache DoS. +- Single-flight generation lock per key. +- A dumb CDN or any HTTP cache in front "just works" because URLs are immutable — + the native origin does the smart part, the edge does distribution. + +--- + +## 7. SEO + +- Stable, crawlable derivative + canonical original URLs. +- Correct `Content-Type`; `width`/`height` attrs (CLS); modern formats (LCP). +- Eager + `preload` the LCP image; `loading=lazy` the rest. +- Mandatory descriptive `alt` (already enforced). +- Image sitemap entries. All emitted by the one `site-kit` renderer → consistent. + +--- + +## 8. Why integrated/native beats a CDN here + +- **One hop:** origin = transformer; no origin→CDN→resizer indirection. +- **Metadata-aware:** focal point + layout `sizes` live in the CMS; a generic CDN + resizer has neither, so its crops/sizes are guesses. +- **Markup + bytes from one place:** `srcset`/`sizes`/``/preload/LQIP + stay consistent with what the endpoint can actually produce. +- **Immutable URLs** make every downstream cache correct for free. +- **Policy control:** format/quality/quantization decided centrally, per image. + +The genuinely "further than everyone else" parts: **focal-exact art-direction at +every breakpoint**, **content-addressed immutability**, and the +**client-hints→immutable redirect** — all clean *only because* we own the origin. + +--- + +## 9. Phased plan + +1. **Derivative endpoint + disk cache + quantization** (Node/`sharp`): + content-addressed immutable URLs, focal-aware crops, single-flight, 304s. + Add `srcHash`/`blurhash` columns + backfill job. CLI/API parity per repo rule. +2. **`site-kit` ``/srcset generator** wired to it; flip site `Picture` + off `/raw`. → **~95% of the win, on Node, today.** +3. **Client-Hints exact upgrade + LQIP inlining + LCP preload** (+ quality + targeting, optional). +4. **Perry-native transformer** (`@perryts/image`) behind `MediaTransformer` — + drop-in once the codec lib exists. + +Steps 1–2 are high-leverage, low-risk, and shippable on the current Node prod. +Step 3 is an incremental enhancement, not a prerequisite. Step 4 unblocks +"fully native" and is gated on a Perry-linkable image codec stack. + +--- + +## 10. Open questions + +- Derivative storage location/eviction policy (disk vs S3; size cap?). +- Sign derivative URLs (HMAC) vs allowlist-only? (DoS posture.) +- Exact width ladder + whether to space it perceptually. +- Client-Hints: enable globally or per high-traffic template first. +- Backfill strategy for the existing ~140 media rows (one-off job vs lazy on + first transform). diff --git a/fable-audit.md b/fable-audit.md new file mode 100644 index 0000000..ec70332 --- /dev/null +++ b/fable-audit.md @@ -0,0 +1,438 @@ +# Fable Audit — Skelpo CMS + +**Date:** 2026-07-04 +**Branch:** `fix/perry-runtime-compat` (with uncommitted working-tree changes) +**Head:** `45989c4` (`fix(auth): lower bcrypt cost 12 → 10 for the perry runtime`) +**Scope:** Full repository — `src/` (~16k LOC), `packages/` (`@skelpo/cms-client`, `@skelpo/site-kit`), `migrations/`, `tests/`, `.github/`, `scripts/`, `docs/`, config, and the current working diff. +**Method:** Manual reading of the auth/authz/injection core plus six parallel deep-dive passes (auth & sessions; injection/XSS/SSRF/upload; authorization; correctness/concurrency; CI/build/deps/release; tests/docs/hygiene). Every finding was verified against the code; load-bearing claims (Perry fail-open path, cursor/`MyDateTime` serialization, CI exit-code masking, capability escalation, gitignore gap) were reproduced or re-read firsthand. Typecheck and the DB-free unit suite were executed. + +--- + +## Executive summary + +Skelpo CMS is a well-architected, genuinely thoughtful codebase: the SQL layer is **uniformly parameterized** (no SQL injection found anywhere), media path handling is **safe by construction**, session/token/invite secrets use a CSPRNG, API tokens are stored hashed, there is **no default admin credential**, the permission core (`can()`) is carefully written, and the schema is well-indexed. It typechecks clean and its 57 unit tests pass. + +However, the audit surfaced a set of serious issues concentrated in five areas: + +1. **Output encoding / stored XSS.** Content bodies are rendered to HTML with no sanitization (`marked` and the TipTap renderer), reachable by low-privilege authors — a live stored-XSS on the public site. +2. **Authorization holes.** `manage*` capabilities collapse into full admin (no allowlist on role/capability/user-role assignment); several read paths and the content `status` write skip the intended capability, exposing drafts, submission PII, and settings — some of it **unauthenticated**. +3. **Caching correctness.** `invalidate()` is repeatedly called with cache keys instead of dependency keys (a silent no-op), the dependency graph leaks on eviction, and there is no TTL — so several surfaces serve **permanently stale** data. +4. **CI integrity.** The integration test step can neither fail (an `&&/||` shell-chain masks the exit code) nor even detect a missing DB — so a green pipeline can verify almost nothing. +5. **Data hygiene.** ~78 MB of a real, named customer's product photography (`uploads-verrano/`) sits untracked but **not** covered by `.gitignore` in what the README presents as an MIT public repo. + +Counts: **3 Critical, ~22 High, ~18 Medium, ~15 Low/Info.** + +### Fix these first (in order) + +1. **Add `uploads-verrano/` (and `uploads*/`) to `.gitignore` now** — a `git add -A` permanently publishes customer photos. (§11.1) +2. **Sanitize rendered content HTML** in `@skelpo/site-kit` (`renderMarkdown`, `renderTipTap`) and block `javascript:` hrefs. (§3.1) +3. **Restrict capability/role/user-role assignment** — allowlist caps, forbid granting `*` or caps the actor lacks, block editing built-in roles, block self-role escalation. (§2.1) +4. **Fix the `test:integration` script** (exit code masking) and add a CI tripwire when the DB is unreachable. (§8.1) +5. **Bump `hono` to ≥4.12.25** (free, in-range CVE fix) and refresh the lockfile. (§8.2) +6. **Fix `invalidate()` dependency-key usage** and add a cache TTL. (§4.1–4.3) +7. **Gate the leaky read paths** — admin content/detail, `/admin/forms/:slug`, and `GET /api/v1/settings`. (§2.3–2.5) +8. **Disable the fake TOTP branch** (fail closed) until real verification ships. (§1.1) + +--- + +## Severity legend + +- **Critical** — severe and reachable in a realistic configuration; fix before further production use. +- **High** — serious; exploitable/triggerable under plausible conditions, or a correctness break that loses/leaks data. +- **Medium** — real risk requiring specific conditions or elevated privilege. +- **Low / Info** — hardening, hygiene, defense-in-depth, or documentation. + +Reachability is stated per finding. "Live" = affects the Node/Bun runtime as deployed today. "Latent" = not currently reachable but will activate under a stated condition (a Perry serve path, a custom role, an enrolled feature). + +--- + +## 1. Authentication & session security + +**1.1 — HIGH (latent) — TOTP/2FA is non-functional and bypassable on both login paths.** +`src/routes/api/auth.ts:80-91` gates on `user.totpVerified === 1` and then only checks `/^\d{6}$/.test(totpCode)` — **any six digits pass** (`000000` works); the code is never compared to `users.totpSecret` (the TODO admits it). The admin login (`src/admin/routes.tsx:101-129`) checks **no** TOTP at all. No code path currently sets `totpVerified = 1` (no enrollment route/CLI), so it is latent — but the docs present 2FA as a working feature, so an operator flipping the flag by hand turns 2FA into pure theater. `toPublicUser` even reports `totpEnabled` from that flag, so the UI would claim protection that doesn't exist. +**Fix:** fail **closed** — block login with a hard error when `totpVerified = 1` until `src/auth/totp.ts` (HMAC-SHA1, ±1 step window, one-time step-reuse guard) exists; enforce it on the admin path too; add a test that a wrong code is rejected. + +**1.2 — HIGH — Session cookie `Secure` flag is off behind a TLS-terminating proxy; no HSTS.** +`src/admin/routes.tsx:120` sets `secure: c.req.url.startsWith('https://')`. The inline `node:http` adapter always builds the URL as `http://…` (`src/server.ts`), and behind the documented nginx→pm2 deploy the app sees plaintext — so the admin `skelpoSession` cookie is issued **without `Secure`** and can leak over any http request. No `Strict-Transport-Security` header is set anywhere (`src/app.ts` adds only `X-Skelpo-Version`). The API login (`auth.ts:101`) is better (also honors `x-forwarded-proto`), but `/auth/refresh` (`auth.ts:172`) and the lang cookie (`routes.tsx:145`) share the weak check. +**Fix:** derive scheme from `x-forwarded-proto` / a `TRUST_PROXY`/`config.siteUrl` setting for `Secure` on all auth cookies; emit HSTS. + +**1.3 — MEDIUM — `clientIp` trusts spoofable `X-Forwarded-For` with no trusted-proxy config.** +`src/routes/api/_helpers.ts:19-25` takes the first `x-forwarded-for` / `x-real-ip` verbatim, falling back to the literal `'0.0.0.0'`; the socket address is never used. Consequences: the per-IP login limit (10/15 min, `ratelimit.ts`) is bypassed by rotating the header (password spraying); with no proxy every client shares the `'0.0.0.0'` bucket; the per-email limit (5/15 min, keyed on attacker-supplied email) lets an unauthenticated party **lock out a known admin email on demand** and flood `loginAttempts`; and `sessions.ip` / `formSubmissions.ip` audit fields are attacker-controlled. +**Fix:** key rate-limiting on the real connection IP; only honor `X-Forwarded-For` from configured proxy hops. + +**1.4 — MEDIUM — Login timing enables user enumeration.** +`auth.ts:65-77` and `routes.tsx:113`: an unknown email returns immediately (no bcrypt), a known email runs `bcrypt.compare` (~100-250 ms on Node, seconds on Perry). Response bodies match, but the timing gap distinguishes valid accounts. +**Fix:** run a dummy bcrypt compare against a constant hash on the unknown-user path. + +**1.5 — MEDIUM — Session tokens stored in plaintext as the primary key.** +`src/auth/sessions.ts:30,45` inserts and looks up the 256-bit token verbatim — unlike API tokens, which store `sha256(token)` (`tokens.ts`). Any read-only DB exposure (backup leak, replica, SQLi elsewhere) yields **live, replayable** session tokens. +**Fix:** store `sha256(token)` as the PK and hash on lookup, matching the API-token design. + +**1.6 — MEDIUM — Password change does not invalidate other sessions; "log out everywhere" is dead code.** +`src/routes/api/users.ts:114` updates `passwordHash` but never deletes sessions or revokes tokens; `deleteAllSessionsForUser` (`sessions.ts:55`) has **no caller** anywhere. After a suspected-compromise password reset, stolen cookies/tokens stay valid up to 30 days. +**Fix:** call `deleteAllSessionsForUser` (and revoke tokens) on password change, preserving the current session if desired. + +**1.7 — MEDIUM — API token scopes are stored but never enforced.** +`lookupToken` returns `scopes` and `middleware.ts:43` puts them on `auth.token.scopes`, but `can()` consults only role capabilities — no code reads `auth.token.scopes` (confirmed by grep). A "read-only" token wields the full privileges of its owner's role. +**Fix:** intersect role capabilities with token scopes in the auth/permission path, or remove the scopes UI until enforced. + +**1.8 — LOW — 30-day absolute session TTL, no idle timeout, no rotation; `/auth/refresh` never revokes the old session.** +`sessions.ts:6`, `auth.ts:165-176`. Sessions accumulate and a stolen cookie is usable for up to a month; refresh extends indefinitely without invalidating the prior token. +**Fix:** add an idle timeout, lower the admin absolute lifetime, and delete the prior session on refresh. + +**1.9 — LOW — Weak password policy; bcrypt silently truncates at 72 bytes.** +`src/auth/password.ts:16-21`: minimum 8, no complexity/breach check; the 200-char max is moot because bcrypt truncates at 72 bytes with no pre-hash, so long-passphrase entropy is lost. +**Fix:** raise the admin minimum (≥12), add a breached-password check, and pre-hash (e.g. base64(sha256)) before bcrypt if long passphrases should count. + +**1.10 — LOW — `SESSION_SECRET` is required at boot but never used.** +`src/config.ts:69` calls `required('SESSION_SECRET')`, but nothing in `src/` consumes it (cookies are unsigned bearer tokens; security rests on the random DB token, which is fine). The shipped placeholder passes the non-empty check, so operators may believe cookies are signed when they aren't. +**Fix:** remove the unused secret, or enforce a real length/entropy check and document that cookies are unsigned. + +**1.11 — INFO — bcrypt cost 10 is acceptable but was lowered for all runtimes.** +`password.ts:13` — cost 10 meets the OWASP floor; the Perry-perf rationale is sound. Note the change weakens Node/Bun hashes too (where it wasn't needed). Track raising back to ≥12 once Perry's bcrypt nears native speed. Existing hashes keep their embedded cost. + +**1.12 — INFO — CSRF rests solely on `SameSite=Lax`; logout is a state-changing GET.** +No CSRF tokens or Origin/Referer checks exist (grep-confirmed). `Lax` does block cross-site POST, so the POST admin mutations are reasonably covered — but `GET /admin/logout` (`routes.tsx:131`) is a top-level-navigable forced-logout CSRF, and there's no defense-in-depth. +**Fix:** make logout a POST; add a per-session CSRF token or explicit same-origin check to admin mutations. + +--- + +## 2. Authorization & access control + +**2.1 — CRITICAL — `manage*` capabilities collapse into full admin; no allowlist on capability/role/user-role assignment.** +`roleRoutes.patch('/:slug')` (`src/routes/api/users.ts:182-200`) writes `body.capabilities` verbatim with **no allowlist and no `isBuiltin` guard** (only DELETE checks `isBuiltin`). A holder of `manageRoles` can PATCH its **own** role to `{"global":["*"]}` (superadmin) or rewrite the built-in `admin`/`viewer` roles. Separately, a `manageUsers` holder can assign any `roleId`/`roleSlug` including `admin` on create (`users.ts:45-99`) or edit (`users.ts:101-124`), and can **reset any user's password** (`users.ts:114`) — i.e. mint or take over an admin account. `can()` grants everything for `global:['*']` (`check.ts:73,79`). +Default seed only gives `admin` these caps, so it is **not exploitable out of the box** — but the product's custom-role feature makes `manage*` effectively equivalent to `*` for any delegated role, which is a total escalation. +**Fix:** validate submitted capabilities against an allowlist; forbid granting `'*'` or any cap the actor lacks; block editing built-in roles and assigning a role ≥ the actor's privilege; forbid a user editing its own role's capabilities. + +**2.2 — HIGH — Content `status` is mass-assignable → publish without the `publish` capability, bypassing validation.** +`PATCH /api/v1/content/:id` (`content.ts:291-292`) forwards the raw body into `updateContent`, which applies `patch.status` / `publishedAt` / `scheduledAt` (`writer.ts:237-239`); `POST /content` similarly honors `body.status`. Both gate only on `update`/`create`, never `publish`. A **contributor** (seeded `post:['read','create','updateOwn']`, no publish) can `PATCH {"status":"published"}` on its own post and push it live — skipping `validateFields`/`validateSeoForPublish` and leaving `publishedAt = NULL` (so it's excluded by date filters and mis-sorted). The admin writer (`routes.tsx:733-735`) whitelists fields and checks publish separately, so the API is strictly weaker. +**Fix:** strip `status`/`publishedAt`/`scheduledAt` from the create/update field set; require an explicit `publish` check for any transition into `published`, running publish validation + `publishedAt` logic. + +**2.3 — HIGH — Admin content list & detail enforce only authentication, not `read`/`readDrafts`.** +`GET /admin/content/:type` (`routes.tsx:441-471`) lists all statuses with `includeDrafts:true`, and `GET /admin/content/:type/:id` (`:611-618`) loads with drafts, both behind only `gate(c)`. Any authenticated user — a **viewer** (no `readDrafts`) or an **author** (no cap at all on the `doc` type) — can read every type's drafts and other users' unpublished rows. The API by-id path checks `readDrafts`; the admin path does not. +**Fix:** check `read` and, for non-published rows, `readDrafts`/`readOthersDrafts` in both admin handlers. + +**2.4 — HIGH — Form-submission detail page has no capability check (PII exposure).** +`GET /admin/forms/:slug` (`screens.tsx:762-855`) renders all submissions — names, emails, IPs — behind only `gate(c)`. The index page `/admin/forms` correctly gates on `viewSubmissions || manageForms`; the detail page does not. Any authenticated user (viewer/author/contributor) can enumerate `/admin/forms/` and read submitted PII. +**Fix:** add the same `viewSubmissions || manageForms` gate the index uses. + +**2.5 — HIGH — `GET /api/v1/settings` and `/settings/:key` are unauthenticated (world-readable).** +`src/routes/api/settings.ts:15-33` has no `requireAuth`/`can` gate; `getAllSettings()` returns every key. Anonymous clients can dump all settings — including `site.previewToken` (the maintenance-mode bypass, set at `routes.tsx:381`) and any secret an operator stored in the flat KV (SMTP creds, API keys, analytics tokens). Strictly weaker than the admin equivalent, which sits behind login. +**Fix:** require auth, or maintain an explicit public-settings allowlist and expose only those keys. + +**2.6 — MEDIUM — `readOthersDrafts` is never enforced; `readDrafts` reads everyone's drafts.** +`can()` resolves `readDrafts` via the generic `includes(action)` (`check.ts:95`) and ignores `ownerId`; the `readOthersDrafts` action is defined and seeded on admin but referenced by **no** route and no branch of `can()`. The API draft reads pass `row.authorId` but discard it. An **author** (has `readDrafts`, not `readOthersDrafts`) can read every other author's unpublished drafts via `GET /content?status=draft` and the by-id/slug/path endpoints. +**Fix:** when `row.authorId !== userId`, require `readOthersDrafts`; make `can()` consult `ownerId` for these actions; scope the list query to the caller unless they hold `readOthersDrafts`. + +**2.7 — MEDIUM — Unauthenticated media item/raw/URL + IDOR enumeration.** +`GET /media/:id`, `/media/:id/raw`, `/media/:id/url` (`media.ts:53-102`) perform no auth check (only the list at `:40` requires auth). IDs are sequential; there is no per-asset ACL. Anyone can enumerate `/api/v1/media//raw` and download every uploaded asset, including images attached to unpublished content. +**Fix:** if media is private, require auth on item/raw/url; if public-CDN, use unguessable keys and document the public contract. At minimum make gating consistent. + +**2.8 — LOW — `viewSubmissions` is dead on the API; seeded `editor` cannot moderate.** +The submissions API (`forms.ts:129,150,162`) gates on `manageForms`, never `viewSubmissions`; the admin moderation POST (`screens.tsx:860`) does the same. The seeded `editor` carries `viewSubmissions` (not `manageForms`) and CLAUDE.md says editors moderate submissions — so the mark-spam/delete actions silently no-op for the exact role designed to use them. +**Fix:** accept `viewSubmissions` for read + moderation; reserve `manageForms` for definition CRUD. + +**2.9 — INFO — Per-type caps shadow the `'*'` type entry instead of merging.** +`check.ts:81`: `types[typeSlug] ?? types['*']` — a specific entry fully replaces the wildcard (no union). Intentional for the `editor.form` restriction, but a footgun: `{'*':['read'], post:['create']}` silently loses `read` on `post`. Over-denies (safe), not over-allows. + +**2.10 — INFO — Content-type schemas are served publicly.** +`GET /types` and `/types/:slug` (`types.ts:19-28`) are unauthenticated, revealing every type's full field schema. Acceptable if intended for the frontend; noted for completeness. + +**Positive:** the ownership core is correct — `can()` checks the base action before the `*Own` short-circuit (`check.ts:86-92`), so holding `updateOwn` does not grant edit on arbitrary rows, and the content write routes pass `row.authorId`. The wildcard `*` handling is sound. + +--- + +## 3. Injection, output encoding & SSRF + +**3.1 — CRITICAL — Content bodies rendered to HTML with no sanitization (stored XSS on the public site).** +`@skelpo/site-kit`'s `renderMarkdown()` (`packages/site-kit/src/markdown.ts`) calls `marked.parse()` with no sanitizer (site-kit's only dependency is `marked`, which passes raw inline HTML through). Content bodies are writable by non-admin roles (**editor** has create/update/publish on all non-form types; **author**/**contributor** author posts). An author publishing a body containing `` yields stored XSS for every public visitor of the consuming site. The TipTap renderer compounds it: `renderTipTap` (`packages/site-kit/src/richtext.ts:38-41`) emits `` where `attrEsc` only escapes `&`/`"` and does **not** validate the scheme — a `javascript:` link href renders a clickable XSS. +**Fix:** sanitize the rendered HTML server-side (DOMPurify/`sanitize-html`) with a strict allowlist, or disable raw-HTML passthrough in `marked`; allowlist URL schemes (`http`/`https`/`mailto`/relative) and drop `javascript:`/`data:`/`vbscript:`. + +**3.2 — HIGH — Media upload trusts client `Content-Type`, served inline same-origin (stored XSS / arbitrary hosting).** +Upload (`media.ts:106-148`) stores `file.type` verbatim with no MIME allowlist and no magic-byte check; `isImage` uses `startsWith('image/')`, accepting `image/svg+xml`. `GET /api/v1/media/:id/raw` (`:61-76`) is **unauthenticated**, streams with `Content-Type: m.mimeType` (client-chosen), no `Content-Disposition`, `Cache-Control: immutable`. With the default `MEDIA_BACKEND=local` (`publicUrl()===null`) it is served **same-origin as the admin**, so an SVG/HTML upload executes JS on the CMS origin and hosts arbitrary active content on the trusted domain. (Needs `manageMedia` — admin by default, but grantable to custom roles, and the unauthenticated hosting is dangerous regardless.) +**Fix:** enforce a server-side MIME/extension allowlist validated against sniffed magic bytes; never echo the client MIME for risky types; send `Content-Disposition: attachment` + `X-Content-Type-Options: nosniff` on `/raw`; store/serve SVG as `text/plain` or sanitize. + +**3.3 — HIGH — Webhook delivery is an SSRF with response exfiltration and no timeout.** +`deliverWebhookJob` (`src/webhooks/dispatch.ts:111`) does `fetch(hook.url)` where `hook.url` is stored unvalidated by `createWebhook`/`updateWebhook` — no scheme check, no block for `127.0.0.1` / `169.254.169.254` / RFC-1918 / `.internal`. A `manageSettings` holder can point a webhook at cloud metadata or internal services (fired by creating content that emits a subscribed event), and the **response status + body (first 2000 chars) is persisted** (`dispatch.ts:120-136`) and readable via `GET /api/v1/webhooks/:id/deliveries` — turning blind SSRF into full read. There is also **no `fetch` timeout**, so a slow endpoint ties up a job worker indefinitely. +**Fix:** validate the URL is `https?://` resolving to a public IP (re-check on redirect) or restrict to an operator allowlist; add `AbortSignal.timeout(...)`. + +**3.4 — MEDIUM — Unescaped user data in notification-email HTML (HTML/email injection).** +`forms.ts:87` builds `submissionHtml` as `

${k}: ${String(v)}

` from raw **public** submitter values, and `interpolate()` (`src/email/adapter.ts:86-88`) does no escaping. A public form submitter injects arbitrary HTML (phishing, tracking pixels) into the admin notification email. The same unescaped `interpolate` also injects `displayName` into invite/reset templates. +**Fix:** HTML-escape each key/value when composing HTML fragments; escape interpolation values by default. + +**3.5 — MEDIUM — No upload size cap / no global body limit (memory-exhaustion DoS).** +Uploads do `new Uint8Array(await file.arrayBuffer())` (`media.ts:139`, also `screens.tsx:590`) with no size check; `app.ts` mounts no `bodyLimit`; and the `node:http` adapter buffers the entire request body into memory before dispatch (`server.ts`). A large body is fully buffered. +**Fix:** add a `bodyLimit`/size guard and reject oversized files before buffering. + +**3.6 — LOW — Client-side `innerHTML` from content-type repeater sub-field labels (DOM XSS across privilege boundary).** +`skelpoRepeaterAdd` (`contentEditor.tsx:182,190,192`) sets `card.innerHTML` from `sf.label||sf.name`; `validateFieldsSchema` (`typeWriter.ts:206-225`) validates only top-level `f.name` and never recurses into repeater sub-fields or labels. A `manageTypes` (developer) user can set a sub-field label to `` that runs in an editor's session on "+ Add". +**Fix:** build rows with `createElement`/`textContent`; validate/escape sub-field names and labels. + +**3.7 — LOW — Redirect `destination` stored/served unvalidated.** +`redirects.ts:53-56 / 100-109` accepts any string; a `manageRedirects` user can store `javascript:` or off-site destinations that become open-redirect/XSS on the consuming frontend. +**Fix:** validate `destination` scheme (relative or `https?:`) at write time. + +**3.8 — LOW — `safeAdminReturn` is same-origin-only but unnormalized.** +`routes.tsx:154-156` correctly blocks `//host` cross-origin redirects but passes any raw `/admin*` value to `c.redirect()` without normalization or CR/LF stripping. +**Fix:** match a known-route allowlist; strip control characters. + +**3.9 — LOW — `cli/backup.ts` restore uses backup-derived identifiers and media keys.** +`backup.ts:118,136,147,157-159` builds `INSERT INTO \`${t}\` (\`${c}\`…)` from backup column names and writes media to `join(localPath, key)` from backup keys (a `../` key escapes the uploads dir). Table names come from a fixed list and this is a local operator op, so risk is low — but restoring an untrusted backup allows arbitrary-path writes / identifier injection. +**Fix:** validate keys stay within the media root; restrict column names to a known set. + +**3.10 — INFO — i18n bag injected into `` would break out. Source is developer-authored bundles today. +**Fix (defensive):** escape `<`,`>`,`&`,`U+2028/9` or use a safe serializer. + +**Positives (verified firsthand and via full sweep):** +- **No SQL injection.** Every `query()`/`execute()` passes user values as bound `?` params; dynamic `SET`/`WHERE` fragments use hardcoded column literals; `IN (…)` uses placeholder arrays; `ORDER BY` sort is allowlisted (`content.ts:297-303`); `LIMIT/OFFSET` are `Number()`-coerced and clamped; backup table names come from a constant. +- **Media path traversal is not exploitable.** `storageKey` is server-generated `${yyyymm}/${randomHex}-${sanitize(filename)}`; `sanitize()` strips `/` and non-`[A-Za-z0-9._-]`; the key is a single path segment and is never user-settable on read or update. +- Admin JSX auto-escapes text/attribute values; the `dangerouslySetInnerHTML` blocks are static CSS/JS (aside from the i18n note). +- imgproxy source URL is fixed to the site's own `/raw` and HMAC-signed — not an SSRF vector (only the unvalidated `format` param is a minor nit). + +--- + +## 4. Correctness — caching & invalidation + +**4.1 — HIGH — `invalidate()` is called with cache keys, not dependency keys → permanent staleness.** +`invalidate()` (`src/cache/deps.ts:81`) only consults the depKey→cacheKey reverse index. `GET /menus` (`menus.ts:27`) registers **no** deps; `POST /menus` invalidates `['GET:/menus']` — a cache key that was never a dep key, so 0 entries are removed. With no TTL the cached list is stale forever. Same pattern in `settings.ts:51,71` (new setting keys never appear in the cached `GET:/settings`) and the menu-create/404-caching flow. +**Fix:** register umbrella dep keys in the list functions (`deps.add('menus')`, `deps.add('settings')`) and invalidate those; or add and use an intentional `cacheDeletePrefix(cacheKey)`. + +**4.2 — HIGH — Dependency graph leaks on LRU eviction → unbounded memory growth.** +`LruMap.set` (`cache/lru.ts:29-37`) evicts the oldest entry with no callback; `cacheSet`/`cacheDelete` clean dep sets only for keys they explicitly touch (`deps.ts:41-64`). Every evicted entry leaves its cacheKey in each dep Set forever, and empty dep entries are never removed. Cache keys include full query strings (every `cursor=` value), so a busy site churns keys and leaks one string + N set memberships per eviction → creeping RSS/OOM; `invalidate()` counts get inflated by dead keys. +**Fix:** give `LruMap` an `onEvict` hook that runs the `cacheDelete` dep-cleanup; delete dep entries when their set empties. + +**4.3 — HIGH — Read/compute vs invalidate race caches stale data forever; TTL never enforced.** +`cache/respond.ts:37-58`: on a miss, `fn(deps)` reads the DB, then several awaits (content inflation, `computeEtag`) run before `cacheSet`. A write landing in that window runs `invalidate()` while the cache is empty → the entry then stored holds pre-write data. `storedAt` is written but **never checked** (no TTL), so it serves until the next write to the same dep. Trigger: a by-slug GET (miss) racing a PATCH of the same row → readers see the old title indefinitely. +**Fix:** snapshot an invalidation generation/epoch before `fn` and skip `cacheSet` if any of the entry's deps were invalidated since; add a real TTL check on `storedAt`. + +**4.4 — MEDIUM — Schema/urlPattern/default-locale changes never invalidate the response cache.** +`updateType`/`deleteType` (`typeWriter.ts:171-172`, `types.ts:85`) call only `invalidateTypeRegistry()`/`clearRevisionCache()`; neither calls `invalidate()`. The dep key `schema:` documented in `deps.ts:19` is emitted nowhere, and cached content embeds `url` computed from `urlPattern` + `site.defaultLocale` (`content.ts:63-73`) without registering `setting:site.defaultLocale`. Changing a urlPattern or the default locale leaves every cached content/list response with the old `url` (and pre-migration field names) forever. +**Fix:** emit `schema:` and `setting:site.defaultLocale` as deps on cached content responses; invalidate them (prefix) from `updateType`/`deleteType` and the settings writer. + +**4.5 — LOW — Per-route `cacheControl` override is lost on cache hit.** +`respond.ts:39-41`: the hit path calls `respondFromEntry` without the override, and `CacheEntry` doesn't store it — so only the miss response carries a custom `Cache-Control`; all later hits revert to the default `s-maxage=300`. +**Fix:** persist `cacheControl` in the entry. + +**4.6 — LOW — `If-None-Match` ignores weak validators (never 304 behind gzip proxies).** +`cache/etag.ts:17-23`: `tags.includes(etag)` never matches `W/"abc"` against a stored `"abc"`. RFC 9110 mandates weak comparison for `If-None-Match`, and nginx gzip hands clients weak forms. +**Fix:** strip a leading `W/` before comparing. + +**4.7 — LOW — In-flight registry load can clobber a newer invalidation.** +`content/types.ts:62-70` (and `settings/store.ts:18-22`): `loadTypeRegistry` has no epoch guard, so a slow SELECT that started before an `updateType`+invalidate can assign its stale result after the cache was cleared → old schema served until the next edit. +**Fix:** capture a generation counter before the query; discard the result if invalidated meanwhile. + +--- + +## 5. Correctness — jobs, scheduling & datetime + +**5.1 — HIGH — Cursor pagination never advances (infinite loop).** +`content.ts:268-293` builds `nextCursor` from `JSON.stringify({ publishedAt: last.publishedAt, id })`. `@perryts/mysql` returns TIMESTAMP columns as `MyDateTime` objects with **no `toJSON`** (verified: `node_modules/@perryts/mysql/dist/types/datetime.d.ts` — only `Decimal` has `toJSON`), so the cursor encodes an object. On decode, the guard `typeof decoded.publishedAt === 'string'` (`:269`) fails → the cursor clause is silently skipped → page 2 returns page 1 with a fresh non-null cursor and `hasMore:true` forever. The keyset clause is also hard-coded to `publishedAt DESC` and ignores `sort` (duplicated/skipped rows for any other sort), and `publishedAt = NULL` draft rows also fail the string check. +**Fix:** encode `dateToIso(last.publishedAt)`+id; make the keyset clause match the active sort column/direction (or restrict cursoring to the default sort); fall back to id-only keyset when `publishedAt` is NULL. + +**5.2 — HIGH — Scheduled publishing has no producer — it silently never happens.** +`scheduledPublish` appears only in the `JobKind` union and the handler; nothing enqueues it and nothing scans `content.scheduledAt` (the `idx_content_scheduled` index is unused). An editor sets `scheduledAt` (stored, returned by the API) → the date passes → the post never publishes. A manually enqueued job would also skip cache `invalidate()` and `fireEvent('content.published')`. +**Fix:** a periodic tick that claims `status='draft' AND scheduledAt <= NOW()`, publishes, invalidates, and fires the event. + +**5.3 — HIGH — Job lease recovery can resurrect a still-running job → concurrent double execution.** +`recoverStuckJobs` (`queue.ts:135-142`) flips any `running` row with `lockedAt < NOW()-10min` back to `pending` without checking liveness; handlers have no time bound (`worker.ts:59-72`). A `sendEmail`/webhook handler that blocks 11 min gets re-claimed and runs twice (duplicate email/webhook); the first run's `markDone` then stomps the second's state. (The claim itself is correct — `SELECT … FOR UPDATE SKIP LOCKED` inside a transaction is atomic.) +**Fix:** bound handler runtime below the lease (`Promise.race` timeout); make recovery conditional on a heartbeat/attempts guard; retry failed `markDone`. + +**5.4 — HIGH — Mixed UTC-JS-string vs SQL `NOW()` timezone conventions.** +No session `time_zone` is set (pool config, `.env.example`). `dateToIso` (`datetime.ts:24-35`) appends `Z` to session-local TIMESTAMP text; `enqueue` stores `runAt` as a UTC wall-time string but the claim compares `runAt <= NOW()` (session tz); `createContent` writes `publishedAt` via JS `toISOString()` (UTC) while `publishContent` uses `NOW()`. On a non-UTC MySQL server, queued jobs run hours early/late, `publishedAt` values disagree by the offset, and every API timestamp is a local time mislabeled `Z`. It coincidentally works on a UTC server — which is why local/CI pass. (This also affects session expiry in `sessions.ts`.) +**Fix:** set session `time_zone = '+00:00'` at pool init, or use `UTC_TIMESTAMP()` everywhere and never mix JS datetime strings with `NOW()`. + +**5.5 — MEDIUM — Recurring maintenance jobs run once per boot; several kinds have no producer.** +`startWorker` (`worker.ts:100-103`) seeds `pruneSessions`/`pruneLoginAttempts` once; the handlers don't re-enqueue and there's no scheduler → they run once per process lifetime, so `sessions`/`loginAttempts` grow unbounded on a long-lived process. `pruneContentRevisions` is never enqueued (autosave revisions accumulate), and completed `jobs` rows are never pruned. +**Fix:** re-enqueue from the handler (or a scheduler tick); enqueue the revisions prune. + +**5.6 — MEDIUM — Fire-and-forget promises without `.catch` → unhandled rejection can kill the process.** +`void recoverStuckJobs()` (`worker.ts:97`), boot `void enqueue(...)` (`:101-102`), and every `void fireEvent(...)` (`content.ts:270,305,327,348,368`; `routes.tsx:657`) have no rejection handler and hit the DB. A transient MySQL error → `unhandledRejection` → Node 22 default terminates the process mid-request. (`void tick()` is safe — it has its own try/catch.) +**Fix:** append `.catch(err => console.error(...))` at each site; consider a process-level `unhandledRejection` handler. + +**5.7 — LOW — Non-Error throw breaks `markFailed`, stranding the job.** +`worker.ts:70` reads `(err as Error).message` (undefined for a thrown string), and `markFailed` (`queue.ts:97`) then calls `error.slice(...)` → TypeError inside the catch → the job stays `running` until 10-min recovery, real error lost. +**Fix:** `String((err as Error)?.message ?? err)`. + +--- + +## 6. Correctness — content, schema evolution & migrations + +**6.1 — HIGH — Field rename via schema diff = remove + add → values vanish from every row.** +`diffSchema` (`typeWriter.ts` / `schemaEvolution.ts:70-77`) never populates `renamed` (it can't detect renames), and PATCH `/types/:slug` computes the diff whenever `changes` is omitted (`typeWriter.ts:142`). A rename becomes `removed:[old]+added:[new]`; lazy migration moves each row's value into `fields._legacy.` and the new field reads default/null. Trigger: rename `subtitle`→`tagline` on `post` → all posts show an empty `tagline`, recoverable only by hand from `_legacy`. Diff-generated `retyped` entries also carry no `transform`, so a text→number retype leaves `"abc"` in a number field (and `NaN < min` is false, so it can publish). +**Fix:** require explicit `changes` when the diff has simultaneous adds+removes (or add rename hints in the admin flow); default retypes to a sensible transform and validate `Number.isFinite`. + +**6.2 — MEDIUM — `publishContent` validates unmigrated fields; `schemaRevision` never advances.** +`writer.ts:273-279` parses `existing.fields` raw (no `migrateFields`) and validates against the **current** schema → a draft written at rev N fails a bogus "required" error after a rename at rev N+1 even though a read would show it migrated. Root cause: `updateContent` (`writer.ts:230-242`) never sets `schemaRevision = currentRevision`, so rows lag forever and migrations re-run on every read (contradicting the `schemaEvolution.ts:4` "row rewritten on next save" design note). +**Fix:** migrate fields before validating in `publishContent`; persist migrated fields + bump `schemaRevision` in `updateContent`. + +**6.3 — MEDIUM — Concurrent updates lose revision snapshots and return the other writer's data.** +`writer.ts:242-247,316`: `revision = revision + 1` is atomic, but the follow-up unkeyed `SELECT *` can observe the other updater's later state — A and B both bump (r+1, r+2), both re-SELECT r+2, both `saveRevision(id, r+2)`; the `(contentId, revision)` unique key + `INSERT IGNORE` drops one snapshot and **no snapshot exists for r+1**; A's HTTP response contains B's changes. No optimistic locking. +**Fix:** return the row via `WHERE id=? AND revision=`; replace `INSERT IGNORE` with a real insert that surfaces conflicts; accept `If-Match`/expectedRevision. + +**6.4 — MEDIUM — Migration statement splitter corrupts string literals; no migration lock.** +`migrate.ts:75-85,117-124`: `line.replace(/--.*$/, '')` strips `--` **inside** quoted literals and the `;\s*\n` split fires on semicolons inside literals. A seed like `INSERT INTO settings VALUES ('sep','a--b');` is truncated → syntax error mid-file; DDL autocommits and the version row is only written at the end, leaving a half-migrated DB that re-applies earlier statements on retry. Also, two instances booting concurrently both migrate (no `GET_LOCK`) → duplicate `schemaMigrations` PK crash. +**Fix:** a quote-aware splitter (or one-statement-per-file); an advisory lock around `runMigrations`. + +**6.5 — LOW — `translationGroupId` two-step write is non-atomic; slug-dup race returns 500 not 409.** +`writer.ts:172-198`: INSERT with placeholder `0` then UPDATE to `id` — a crash between leaves `translationGroupId=0` permanently (sibling joins break). Separately, the SELECT-then-INSERT dup check races: two simultaneous creates of the same (type,slug,locale) both pass, and `uq_content_slug` makes the second INSERT throw an uncaught `ER_DUP_ENTRY` → 500 instead of the intended 409. +**Fix:** single-statement/transactional group-id assignment; catch duplicate-key errors and map to the validation shape. + +--- + +## 7. The Perry-compat branch (working diff review) + +This branch's purpose is Perry-runtime compatibility. The committed change (bcrypt 12→10) and the working diff (`server.ts` inline `node:http` adapter, `notAuth()` guard, `seed.ts` counter typing, `package.json` `perry` block) are individually reasonable, but the branch is **incompletely applied** in one security-relevant way. + +**7.1 — HIGH (latent, Perry target) — The `instanceof Response` → `notAuth()` fix was applied to the admin only; the entire `/api/v1` surface still uses `instanceof Response`, which is a fail-open under Perry.** +CLAUDE.md documents that under Perry `x instanceof Response` is **always false** for the native fetch handle. The working diff replaces every admin guard with `notAuth()` (field-based discriminator) — correct — but **54 `instanceof Response` checks remain across 11 API route files** (`auth, content, forms, jobs, media, menus, redirects, settings, types, users, webhooks`). Under Perry: +- Inline-guard routes (menus, content, users, …) dereference `auth.user.id` immediately after the check, so an unauthenticated request **crashes** (500) — fail closed, but broken. +- **Guard-helper routes fail *open*.** In `webhooks.ts`, `guard()` returns a `Response` for an authenticated-but-unauthorized user, and the route bodies (`GET/POST/PATCH/DELETE /webhooks`) never touch `g.user` after `if (g instanceof Response) return g;`. Under Perry that check is skipped and the mutation runs — **any logged-in user can manage webhooks** (including the SSRF-capable create). This is exactly the footgun CLAUDE.md warns about. +Not exploitable today (on Node/Bun `instanceof` works; Perry can't serve any request yet per the open `app.fetch` `Symbol()` bug), but it will activate the moment Perry serving lands, and it's an inconsistency that should be closed now while the context is fresh. +**Fix:** replace **all** `instanceof Response` auth guards repo-wide with a single shared discriminator (`isResponse()`/`notAuth()`), and never rely on a post-guard `.user` dereference for safety. + +**7.2 — Notes on the inline `node:http` adapter (`server.ts`).** The header rebuild from `req.rawHeaders`, the text/binary body coercion, and synchronous `data`/`end` registration are correct and portable. Two observations: the adapter **buffers the entire request body and the entire response** in memory (ties into §3.5 — add a size cap), and the startup warmup `app.fetch('/healthz')` swallows all errors (inert on Node, intended for Perry id-counter advancement — fine, but document its removal condition, already noted in-code). No Node/Bun regression. + +**7.3 — INFO — `package.json` ends without a trailing newline** after the added `perry` block; harmless but worth a POSIX newline. + +**7.4 — INFO — bcrypt cost applies to all runtimes** — see §1.11. + +--- + +## 8. Build, CI, dependencies & release + +**8.1 — CRITICAL — CI integration tests can neither fail nor detect a missing DB (the safety net verifies nothing).** +Two independent defects combine: +- `package.json:26` — `test:integration` is `ls … && node --test … || echo 'no integration tests'`. In an `A && B || C` chain, if `B` (the test run) **fails**, bash falls into `|| C` and the whole line **exits 0** (reproduced with a deliberately failing test). Integration tests (`admin.test.ts`, `api.test.ts`) cannot fail CI regardless of assertions. +- Each integration test self-skips via `mysqlAvailable()` → `test(name, { skip: … })` (`tests/helpers/db.ts`). If CI's MySQL is ever unreachable, all 34 tests report "skipped," exit 0, and CI is green having verified nothing. `test.yml` doesn't assert the DB is reachable. +Given CLAUDE.md's invariant "CI must be green," a green pipeline that verifies nothing is a critical process gap — every finding in this report could have been introduced undetected. +**Fix:** replace the `&&/||` one-liner with explicit `if/then/else` so the runner's exit code propagates; add a CI-only tripwire that fails when `HAS_DB` is false (assert reachability before running, or grep the output for `tests 0`). + +**8.2 — HIGH — `hono@4.12.21` carries known CVEs; the fix is free and in-range.** +`npm audit` reports a high-severity advisory (CORS middleware reflecting any Origin with credentials, GHSA-88fw-hqm2-52qc) plus moderate ones (serve-static path traversal, header-merging). Fixed in 4.12.25+; latest `4.12.27` is inside the declared `^4.10.0` range. `hono/cors` and `hono/serve-static` aren't imported today, limiting blast radius — but there's no reason to carry known CVEs. +**Fix:** `npm update hono` (or `npm audit fix`) and commit the refreshed lockfile. + +**8.3 — HIGH — `packages/site-kit/package-lock.json` is stale, silently degrading release reproducibility.** +`site-kit/package.json` declares `marked ^18.0.4`, but its lockfile has **no `marked` entry** (only devDependencies). `npm ci` from `working-directory: packages/site-kit` in `release.yml` therefore always fails the lock/manifest sync check and falls through to `npm install` — so the published tarball's `marked` version is resolved fresh at publish time and is never the exact version the tests exercised. The `npm ci || npm install` fallback masks this. +**Fix:** regenerate `site-kit`'s lockfile, or (better) drop per-package lockfiles and have `release.yml` install from the workspace root before building/publishing. + +**8.4 — MEDIUM — No test-execution gate before `npm publish`.** +`release.yml` runs version-match → install → `npm run build` (tsc only) → `npm pack --dry-run` → `npm publish --provenance`, never re-running the packages' behavioral tests. A `workflow_dispatch` run or a tag on a commit that raced ahead of a red `main` publishes with only a typecheck-level guarantee. +**Fix:** run `npm run test:unit` before publish, or require the tag's commit to have a passing `test.yml` via branch protection. + +**8.5 — MEDIUM — GitHub Actions pinned to floating tags, not commit SHAs.** +`test.yml` and `release.yml` use `actions/checkout@v4` / `actions/setup-node@v4`. Floating majors can be repointed with no diff here — materially worse in `release.yml`, which holds `id-token: write` (OIDC) and publish rights. +**Fix:** pin to commit SHAs (`@ # v4.x`) and update via Dependabot/Renovate. + +**8.6 — MEDIUM — `@hono/node-server` is a declared runtime dependency but dead code.** +`package.json:29` lists it, but `grep` finds zero imports (only an explanatory comment in `server.ts:112`); `server.ts` serves via the inline adapter, and CLAUDE.md says it's removed from the Perry path. +**Fix:** remove it from `dependencies` (and the lockfile) unless a fallback still needs it. + +**8.7 — MEDIUM — `moduleResolution: "Bundler"` doesn't enforce ESM `.js` extensions, and CI never builds/boots `dist/`.** +CLAUDE.md documents at length that Node 22 strict ESM requires `.js` on relative imports in compiled output. `Bundler` resolution is lenient about this. Source is 100% disciplined today (229/229 relative imports carry `.js`), so no live bug — but there's no guardrail against regression, and `test.yml` only runs `tsc --noEmit` + `tsx` (both bundler-like); the actual artifacts (`dist/server.js`, `dist/cli/main.js`) are **never compiled or smoke-run** by CI. +**Fix:** switch root + both packages to `"module":"NodeNext","moduleResolution":"NodeNext"` (a missing extension becomes a compile error); add a CI step that `npm run build`s and boots `dist/server.js` to `/healthz` on ≥1 Node version. + +**8.8 — MEDIUM — `scripts/build-perry.sh` symlink-resolution premise doesn't hold universally.** +The script exists to resolve the `~/.cargo/bin/perry` symlink to its real workspace path (CLAUDE.md: invoking via the symlink breaks native `node:http` linking). On this machine `~/.cargo/bin/perry` is a **regular file**, not a symlink, so `readlink` returns empty, the guard is false, and the script execs the symlink-equivalent path directly — the exact invocation CLAUDE.md says fails. CLAUDE.md's "compiles + boots" verification was "in an isolated worktree build," possibly a different install layout. +**Fix:** verify empirically (`npm run build:perry` here, confirm the binary actually binds `node:http`); resolve the workspace via a marker file/`PERRY_WORKSPACE` env rather than relying on symlink-walking; loop `readlink` and assert the final path is a real file. + +**8.9 — LOW — `perry.config.json` and `package.json` both use `compilePackages` with different contents and no cross-reference.** +`perry.config.json` lists `@perryts/mysql, hono, bcryptjs`; `package.json`'s `perry.compilePackages` lists only `hono, bcryptjs` (correct — `@perryts/mysql` ships a `perry` export and doesn't need JS-AOT). Two mechanisms sharing a key name with nothing documenting the distinction. +**Fix:** add a one-line comment in each cross-referencing the other. + +**8.10 — INFO — `exactOptionalPropertyTypes: false` is explicitly disabled** amid otherwise-strict options (`strict`, `noUncheckedIndexedAccess`, `noImplicitOverride`). No live bug; confirm it's deliberate (likely JSX/Hono type noise). + +**8.11 — INFO — Transitive `esbuild` low-severity advisory via `tsx`** — dev-only, no exploitation path; clears on a `tsx` bump. + +**Positives:** `test.yml` correctly uses the single `npm ci --include-workspace-root --workspaces` install (avoiding the documented workspace-prune pitfall) and a Node 22+24 matrix with a `mysql:8.4` service; `release.yml` OIDC is correct (`id-token: write`, no `registry-url`, `--provenance`, version-vs-tag check). Root tsconfig is otherwise strict. + +--- + +## 9. Tests + +Unit tests are strong exactly where they're cheap and pure; everything stateful is reachable only through the integration suite, which (per §8.1) can't fail CI. + +- **9.1 — HIGH — Bearer-token auth path has zero coverage.** No test in `tests/` references `Bearer`, `apiTokens`, `createToken`, or `lookupToken`, yet the API spec says session and bearer auth are equally supported. A full, security-relevant path (`middleware.ts:33-45`) ships unverified — including whether revoked/expired tokens are rejected. **Fix:** add an integration test: create a token, authenticate with `Authorization: Bearer`, revoke, confirm 401. +- **9.2 — MEDIUM — Schema-evolution transforms untested.** `migrateFields()` handles `added`/`renamed`/`removed→_legacy`/`retyped`; only the "added" path is exercised (integration, needs DB). Given §6.1's rename data-loss bug, this is the code that most needs unit tests. **Fix:** add `tests/unit/schema-evolution.test.ts` covering each transform and multi-revision migration. +- **9.3 — MEDIUM — Job-queue concurrency/retry/dead-letter untested.** No test proves `FOR UPDATE SKIP LOCKED` prevents double-claim, or exercises backoff / `dead` transition / `recoverStuckJobs`. **Fix:** race two `claimNext` calls on one job; force a handler throw and assert backoff/dead-letter. +- **9.4 — MEDIUM — Media `sanitize()` path-traversal defense unverified.** It's safe on inspection but not exported and not asserted against adversarial filenames (`../../etc/passwd`, absolute paths, null bytes) — a refactor could silently reintroduce traversal. **Fix:** export it and unit-test that payloads never yield a `/` or escape `root` after `join()`. + +**Measured health (this audit):** `tsc --noEmit` → exit 0 (clean). `test:unit` → **57/57 pass** (~430 ms). Integration not run here (needs MySQL). + +**Positive:** real, meaningful unit coverage exists for permissions (`can()` ownership/wildcard cases), cache/ETag, datetime normalization, password hashing, content-writer validation, admin i18n, and the two published packages. + +--- + +## 10. Documentation accuracy + +- **10.1 — HIGH — Documented auth endpoints don't exist, and there is no password-reset path at all.** `docs/api-spec.md:142-174` documents `POST /auth/totp/setup|verify`, `DELETE /auth/totp`, `POST /auth/password-reset/request|confirm`. None are registered (`auth.ts` has only `/login`,`/logout`,`/me`,`/refresh`,`/tokens*`). The `passwordResets` table exists but is used nowhere — **users have no way to recover a forgotten password** via API, admin, or CLI. A real product gap, compounding §1.1. **Fix:** implement the flows or strike them and mark "not implemented"; prioritize password recovery. +- **10.2 — MEDIUM — README/`.env.example` present SMTP/Postmark/SES as available; all three are throw-stubs.** `adapter.ts:56-72` — only `log` and `resend` work; the rest throw "not yet implemented." A deployer choosing SMTP gets every queued email failing to `dead` with no alert path. **Fix:** caveat the docs to match the honest code comment. +- **10.3 — MEDIUM — Documented content-list filters don't exist and fail silently.** `api-spec.md:207-214` documents `q`, `tag`, `category`, custom-field, and `fields` params; `ListContentOptions` and the route support none of them, and Hono ignores unknown params — so a consumer's search box gets silently unfiltered results. **Fix:** implement them or strike them; consider 400 on unknown filter params. +- **10.4 — MEDIUM — `docs/perry-landing-integration.md` is stale and contradicts CLAUDE.md.** It describes a Next.js 16 static-export + Perry-Fastify cutover; CLAUDE.md describes the actual Hono + JSX + Tailwind rewrite in a separate repo. Two different stacks, no reconciliation. **Fix:** add a "superseded — see CLAUDE.md" banner or delete it. +- **10.5 — LOW — `docs/media-pipeline.md` proposes `sharp` (native C++) against README's non-negotiable "zero native deps."** Not yet in `package.json`, so a planning conflict, not a violation — but it should be flagged/resolved before implementation. +- **10.6 — LOW — README's global-capability list omits `viewSubmissions`** (12 vs the code's 13 in `GLOBAL_ACTIONS`). +- **10.7 — LOW — Leftover planning framing.** README opens "v0.1… end-to-end verified" but closes (`:824`) "**Next step:** approve this plan, then start scaffolding…"; `api-spec.md:3-6` still headers itself "Draft… lock changes before touching code" despite being substantially built and diverged. **Fix:** delete the stale lines. +- **10.8 — LOW — README test counts are stale** ("47 unit / 81 total" vs actual 57 / 91). + +--- + +## 11. Repository & data hygiene + +- **11.1 — HIGH — Real customer photos (`uploads-verrano/`, ~78 MB, 142 files) are untracked but NOT gitignored.** `.gitignore` ignores `uploads/` (exact literal), which does **not** match the sibling `uploads-verrano/` (confirmed: `git check-ignore uploads-verrano` → not ignored). The files are genuine product photography of a named catering business ("Verrano" — antipasti platters, carpaccio, Jausenplatte), and `docs/media-pipeline.md:27` references a real `verrano/site/src/ui.tsx` path. The README presents this as an MIT public repo. One `git add -A` commits a named customer's (possibly licensed) photography into public history permanently. **Fix:** add `uploads-verrano/` and a generic `uploads*/` to `.gitignore` **now**; investigate why a customer's live upload dir exists in the checkout (local dev pointed `MEDIA_LOCAL_PATH` at real customer uploads — should be an isolated fixture path). +- **11.2 — LOW — Internal deployment hostnames + root-SSH convention in a public-repo file.** `CLAUDE.md:220-230` names `root@builder.perryts.com` and `root@webserver.skelpo.net` and states the deploy logs in as `root`. Not a credential leak, but unnecessary topology disclosure in an MIT repo. **Fix:** move ops specifics to a private doc. + +**Positives:** `.env` is **not** tracked; `dist/` and `node_modules/` are properly ignored; `.proof/`, `.testuploads/`, and `uploads/` are ignored; a `git grep` for secrets/keys/tokens across tracked files found no real credentials (only placeholders in `.env.example`); no large committed binaries. + +--- + +## 12. What's done well + +- **SQL layer is uniformly parameterized** — no injection found across the entire surface. +- **Media path handling is safe by construction** (server-generated, sanitized, single-segment keys; not user-settable on read). +- **No default admin credential** — the first admin is created via CLI, avoiding the classic `admin/admin` seed. +- **Secrets use a CSPRNG** (`crypto.getRandomValues`) for sessions, tokens, invites, webhook secrets; **API tokens are stored hashed** (`sha256`); a fresh session token is issued per login (no fixation). +- **Permission core is careful** — ownership checked before the `*Own` short-circuit, correct wildcard handling, unit-tested. +- **Rate-limiting exists** on both login paths with per-email and per-IP windows and success-reset. +- **Webhook signing** uses HMAC-SHA256 with a timestamp (good replay-mitigation shape). +- **Schema is well-indexed** (unique keys on emails/slugs/token-hashes, composite content indexes, FULLTEXT). +- **CI avoids the documented workspace-prune pitfall**; the release workflow's OIDC/provenance setup is correct. +- **Storage-agnostic media** and a clean Hono/JSX/HTMX architecture; strict TypeScript; real unit tests where they're cheap. +- The **Perry-compat working is genuinely careful** and well-documented in-code (rawHeaders, body coercion, sync listener registration) — §7.1 is an incompleteness, not a wrong approach. + +--- + +## 13. Prioritized remediation roadmap + +**Do immediately (hours):** +1. `.gitignore` `uploads-verrano/` + `uploads*/` (§11.1). +2. Bump `hono` ≥4.12.25 (§8.2). +3. Fix the `test:integration` exit-code masking + DB tripwire (§8.1). +4. Disable the fake-TOTP branch (fail closed) (§1.1). +5. Gate `GET /api/v1/settings` (§2.5). + +**This week (correctness + authz):** +6. Sanitize `renderMarkdown`/`renderTipTap` output; block `javascript:` hrefs (§3.1). +7. Allowlist capability/role/user-role assignment; block built-in-role edits (§2.1). +8. Strip `status`/`publishedAt` from content create/update; require `publish` (§2.2). +9. Add `read`/`readDrafts` checks to admin content list/detail and the form-submission page (§2.3, §2.4). +10. Fix `invalidate()` to use dep keys; add a cache TTL; add an `onEvict` dep-cleanup (§4.1–4.3). +11. Fix cursor pagination (`MyDateTime` serialization + sort-aware keyset) (§5.1). +12. Add webhook URL validation (SSRF) + `fetch` timeout; add an upload size cap (§3.3, §3.5). + +**This month (robustness + Perry):** +13. Replace all `instanceof Response` guards repo-wide with a shared discriminator (§7.1). +14. Set the DB session `time_zone` to UTC (or standardize on `UTC_TIMESTAMP()`) (§5.4). +15. Implement `scheduledPublish` producer; bound handler runtime under the lease (§5.2, §5.3). +16. Require explicit `changes` for schema renames; migrate-before-validate + advance `schemaRevision` (§6.1, §6.2). +17. Harden cookies (`Secure` via `x-forwarded-proto` + HSTS), real client-IP for rate-limiting, session-token hashing, invalidate-on-password-change, enforce token scopes (§1.2–1.7). +18. Add `.js`-extension-enforcing `moduleResolution` + a CI build/boot step; pin Actions to SHAs; add a pre-publish test gate (§8.5–8.7). +19. Fill the highest-value test gaps: bearer auth, schema evolution, job concurrency, media sanitize (§9). +20. Reconcile the docs (auth endpoints, email backends, list filters, stale integration doc) (§10). + +--- + +## Appendix — coverage & method + +Reviewed firsthand: `app.ts`, all of `auth/*`, `permissions/check.ts`, `config.ts`, `routes/api/{auth,users,menus,webhooks,media,forms,settings,content(read)}.ts`, `media/{local,store}.ts`, `webhooks/dispatch.ts`, `email/adapter.ts`, `db/seed.ts` (roles), `migrations/0001_initial.sql` (keys), `.env.example`, and the full working diff. Six parallel deep-dive passes covered the remainder (auth, injection, authz, correctness/concurrency, CI/build/deps, tests/docs/hygiene); all their claims were cross-checked against the code, and the load-bearing ones (Perry fail-open, `MyDateTime.toJSON`, CI exit masking, capability escalation, gitignore gap, `hono` version) were re-verified directly. Runtime facts about Perry are taken from CLAUDE.md and the branch's own notes; the CMS does not yet serve requests under Perry, so all Perry-specific findings are latent by definition. + +*Generated by an automated audit on 2026-07-04. Severities reflect impact on the CMS as deployed today (Node/Bun); revisit once the Perry serve path lands.* diff --git a/migrations/0002_hash_session_tokens.sql b/migrations/0002_hash_session_tokens.sql new file mode 100644 index 0000000..f709863 --- /dev/null +++ b/migrations/0002_hash_session_tokens.sql @@ -0,0 +1,5 @@ +-- Session tokens are now stored hashed (sha256) instead of in plaintext, so a +-- DB leak can't be replayed to hijack a session. Existing rows hold plaintext +-- tokens that can never match a hashed lookup, so clear them; every user simply +-- signs in again once. (apiTokens were already hashed, so they're unaffected.) +DELETE FROM `sessions`; diff --git a/package-lock.json b/package-lock.json index 64d6db4..bc03240 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,10 +12,9 @@ "packages/*" ], "dependencies": { - "@hono/node-server": "^1.13.0", "@perryts/mysql": "^0.1.4", "bcryptjs": "^3.0.2", - "hono": "^4.10.0" + "hono": "^4.12.25" }, "bin": { "skelpo-cms": "dist/cli/main.js" @@ -472,18 +471,6 @@ "node": ">=18" } }, - "node_modules/@hono/node-server": { - "version": "1.19.14", - "resolved": "https://registry.npmjs.org/@hono/node-server/-/node-server-1.19.14.tgz", - "integrity": "sha512-GwtvgtXxnWsucXvbQXkRgqksiH2Qed37H9xHZocE5sA3N8O8O8/8FA3uclQXxXVzc9XBZuEOMK7+r02FmSpHtw==", - "license": "MIT", - "engines": { - "node": ">=18.14.1" - }, - "peerDependencies": { - "hono": "^4" - } - }, "node_modules/@mixmark-io/domino": { "version": "2.2.0", "resolved": "https://registry.npmjs.org/@mixmark-io/domino/-/domino-2.2.0.tgz", @@ -585,9 +572,9 @@ } }, "node_modules/hono": { - "version": "4.12.21", - "resolved": "https://registry.npmjs.org/hono/-/hono-4.12.21.tgz", - "integrity": "sha512-uV63apnb0kyPtAUwoWgaGh9HyIFcv8lgmzPZSiTBQAFOFGIzka5EZ1dZocmGnn0XdX0+XTqJ6Tqv7selMuGLRQ==", + "version": "4.12.27", + "resolved": "https://registry.npmjs.org/hono/-/hono-4.12.27.tgz", + "integrity": "sha512-1yrb/+w6HWQJrUCLkJ2IF5jNIPvvFkblV5RNOYl6bV+OA6p9GLcMpHFFGTosSvHvcAUibuUukRqhlYI4z32C7Q==", "license": "MIT", "engines": { "node": ">=16.9.0" diff --git a/package.json b/package.json index 1e66a1a..99e2b55 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ }, "scripts": { "build": "tsc -p tsconfig.json", - "build:perry": "perry build src/server.ts -o dist/skelpo-cms", + "build:perry": "bash scripts/build-perry.sh", "start": "node dist/server.js", "start:bun": "bun src/server.ts", "start:perry": "./dist/skelpo-cms", @@ -23,13 +23,12 @@ "migrate": "tsx src/cli/main.ts migrate", "test": "npm run test:unit && npm run test:integration", "test:unit": "SESSION_SECRET=unit_test_secret_unit_test_secret_unit_test_secret_unit node --import tsx --test 'tests/unit/*.test.ts'", - "test:integration": "ls tests/integration/*.test.ts >/dev/null 2>&1 && node --import tsx --test --test-concurrency=1 'tests/integration/*.test.ts' || echo 'no integration tests (set up a test DB + add tests/integration/*.test.ts)'" + "test:integration": "if ls tests/integration/*.test.ts >/dev/null 2>&1; then node --import tsx --test --test-concurrency=1 'tests/integration/*.test.ts'; else echo 'no integration tests (set up a test DB + add tests/integration/*.test.ts)'; fi" }, "dependencies": { - "@hono/node-server": "^1.13.0", "@perryts/mysql": "^0.1.4", "bcryptjs": "^3.0.2", - "hono": "^4.10.0" + "hono": "^4.12.25" }, "devDependencies": { "@types/node": "^22.0.0", @@ -59,5 +58,17 @@ ], "workspaces": [ "packages/*" - ] + ], + "perry": { + "compilePackages": [ + "hono", + "bcryptjs" + ], + "allow": { + "compilePackages": [ + "hono", + "bcryptjs" + ] + } + } } diff --git a/packages/cms-client/package-lock.json b/packages/cms-client/package-lock.json deleted file mode 100644 index 1105378..0000000 --- a/packages/cms-client/package-lock.json +++ /dev/null @@ -1,51 +0,0 @@ -{ - "name": "@skelpo/cms-client", - "version": "0.1.3", - "lockfileVersion": 3, - "requires": true, - "packages": { - "": { - "name": "@skelpo/cms-client", - "version": "0.1.3", - "license": "UNLICENSED", - "devDependencies": { - "@types/node": "^22.0.0", - "typescript": "^5.6.0" - }, - "engines": { - "node": ">=22" - } - }, - "node_modules/@types/node": { - "version": "22.19.19", - "resolved": "https://registry.npmjs.org/@types/node/-/node-22.19.19.tgz", - "integrity": "sha512-dyh/xO2Fh5bYrfWaaqGrRQQGkNdmYw6AmaAUvYeUMNTWQtvb796ikLdmTchRmOlOiIJ1TDXfWgVx1QkUlQ6Hew==", - "dev": true, - "license": "MIT", - "dependencies": { - "undici-types": "~6.21.0" - } - }, - "node_modules/typescript": { - "version": "5.9.3", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.9.3.tgz", - "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", - "dev": true, - "license": "Apache-2.0", - "bin": { - "tsc": "bin/tsc", - "tsserver": "bin/tsserver" - }, - "engines": { - "node": ">=14.17" - } - }, - "node_modules/undici-types": { - "version": "6.21.0", - "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.21.0.tgz", - "integrity": "sha512-iwDZqg0QAGrg9Rav5H4n0M64c3mkR59cJ6wQp+7C4nI0gsmExaedaYLNO44eT4AtBBwjbTiGPMlt2Md0T9H9JQ==", - "dev": true, - "license": "MIT" - } - } -} diff --git a/packages/cms-client/tsconfig.json b/packages/cms-client/tsconfig.json index 27ab11a..1c97fe4 100644 --- a/packages/cms-client/tsconfig.json +++ b/packages/cms-client/tsconfig.json @@ -1,8 +1,8 @@ { "compilerOptions": { "target": "ES2022", - "module": "ESNext", - "moduleResolution": "Bundler", + "module": "NodeNext", + "moduleResolution": "NodeNext", "lib": ["ES2023", "DOM"], "rootDir": "src", "outDir": "dist", diff --git a/packages/site-kit/package-lock.json b/packages/site-kit/package-lock.json deleted file mode 100644 index 8791390..0000000 --- a/packages/site-kit/package-lock.json +++ /dev/null @@ -1,51 +0,0 @@ -{ - "name": "@skelpo/site-kit", - "version": "0.1.2", - "lockfileVersion": 3, - "requires": true, - "packages": { - "": { - "name": "@skelpo/site-kit", - "version": "0.1.2", - "license": "UNLICENSED", - "devDependencies": { - "@types/node": "^22.0.0", - "typescript": "^5.6.0" - }, - "engines": { - "node": ">=22" - } - }, - "node_modules/@types/node": { - "version": "22.19.19", - "resolved": "https://registry.npmjs.org/@types/node/-/node-22.19.19.tgz", - "integrity": "sha512-dyh/xO2Fh5bYrfWaaqGrRQQGkNdmYw6AmaAUvYeUMNTWQtvb796ikLdmTchRmOlOiIJ1TDXfWgVx1QkUlQ6Hew==", - "dev": true, - "license": "MIT", - "dependencies": { - "undici-types": "~6.21.0" - } - }, - "node_modules/typescript": { - "version": "5.9.3", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.9.3.tgz", - "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", - "dev": true, - "license": "Apache-2.0", - "bin": { - "tsc": "bin/tsc", - "tsserver": "bin/tsserver" - }, - "engines": { - "node": ">=14.17" - } - }, - "node_modules/undici-types": { - "version": "6.21.0", - "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.21.0.tgz", - "integrity": "sha512-iwDZqg0QAGrg9Rav5H4n0M64c3mkR59cJ6wQp+7C4nI0gsmExaedaYLNO44eT4AtBBwjbTiGPMlt2Md0T9H9JQ==", - "dev": true, - "license": "MIT" - } - } -} diff --git a/packages/site-kit/src/markdown.ts b/packages/site-kit/src/markdown.ts index df11eef..39f20f3 100644 --- a/packages/site-kit/src/markdown.ts +++ b/packages/site-kit/src/markdown.ts @@ -1,15 +1,25 @@ // Markdown renderer. Thin wrapper around `marked` that: // - configures sensible defaults (GFM tables/strikethrough, line-break // handling matching CommonMark) -// - returns plain HTML strings (no DOM, no sanitization layer — the -// CMS controls what reaches this function, content authors are -// trusted users) +// - SANITIZES output: raw inline/block HTML is escaped (never passed through +// live), and link/image URLs are scheme-checked. Content authors are CMS +// users (editor/author/contributor) — not fully trusted — so an unsanitized +// `marked.parse` would be a stored-XSS sink on the public site. // -// Use renderMarkdown(md) for new markdown-stored content. Old TipTap -// JSON content keeps rendering via renderTipTap; auto-detect with +// Use renderMarkdown(md) for new markdown-stored content. Old TipTap JSON +// content keeps rendering via renderTipTap; auto-detect with // `typeof body === 'string'` upstream. -import { marked } from 'marked'; +import { marked, type Tokens } from 'marked'; +import { safeHref, safeSrc } from './url.js'; + +function esc(s: string): string { + return s + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"'); +} marked.setOptions({ gfm: true, @@ -17,7 +27,29 @@ marked.setOptions({ pedantic: false, }); -/** Render a Markdown string to an HTML string. Empty input → empty string. */ +// Override the renderers that can emit attacker-controlled markup. +marked.use({ + renderer: { + // Raw HTML in the source is rendered as visible, escaped text — never as + // live markup. This neutralizes ` there'); + assert.ok(!/