fix: codebase analysis findings 2026-05-23

[LiukScot]

Summary

• SECURITY: Fix broken actions/checkout@v6 tag → @v4 (workflow failing at checkout step — v6 does not exist)
• SECURITY: Add authMiddleware to POST /logout (belt-and-suspenders CSRF protection; cookie is SameSite=Strict but same-site top-level navigations bypass it)
• SECURITY: Add Strict-Transport-Security and Referrer-Policy headers (additive, no regression risk)
• SECURITY BUG: Add depth guard (max 100) to archiveRecursive — unbounded recursion crashed server inside live DB transaction on deep/cyclic trees
• DEPS CVE: hono 4.12.8 → 4.12.22 (GHSA-qp7p-654g-cw7p cross-user cache leak + 10 other advisories)
• DEPS CVE: vite 6.4.1 → 6.4.2 (GHSA-p9ff-h696-f583 arbitrary file read via dev server WebSocket, GHSA-4w7w-66w2-5vf9 path traversal)
• DEPS CVE: postcss 8.5.8 → 8.5.15 (GHSA-qx2v-qp2m-jg93 XSS via unescaped </style> in CSS)
• DEPS CVE: turbo 2.8.19 → 2.9.14 (GHSA-hcf7-66rw-9f5r login callback CSRF/session fixation, GHSA-3qcw-2rhx-2726 local code execution)
• BUG: updatePageSchema.title lacked .min(1) — PATCH /api/pages/:id { "title": "" } silently stored empty title in DB
• QUALITY: Remove dead ReorderPagesInput type export (zero imports across entire repo)
• QUALITY: Extract AUTOSAVE_DEBOUNCE_MS = 1000 constant in Editor.tsx (magic number)
• PERF: Add composite index idx_pages_sort_order(created_by, archived_at, parent_page_id, sort_order) — GET /pages ORDER BY was doing a filesort after the existing index scan
• BUG (LOW): Finalize session cleanup prepared statement on startup (prevent GC leak warning in some bun versions)
• INFRA: Align packageManager field to bun@1.3.11 (CI uses 1.3.11, field said 1.3.10)
• INFRA: Add *.pem *.key *.p12 *.p8 to .gitignore

Findings (deferred — implementabili in run dedicata)

Deferred — File upload magic-byte validation

File: apps/server/src/routes/pages.ts:220-278
Severità: MEDIUM
Soluzione proposta: Install file-type npm package; read first bytes of uploaded file and compare magic bytes to allowed image types before saving. Client-supplied Content-Type and file extension are fully attacker-controlled.
Patch indicativa:

import { fileTypeFromBuffer } from "file-type";
// inside cover upload handler:
const buffer = await file.arrayBuffer();
const detected = await fileTypeFromBuffer(buffer);
if (!detected || !["image/jpeg","image/png","image/webp","image/gif"].includes(detected.mime)) {
  return c.json({ error: "Invalid image file" }, 422);
}

Decision points: Which MIME types to allow; whether to re-encode images server-side (eliminates embedded payloads).
Trade-off: file-type is a new dep; without re-encoding, a valid JPEG container could embed a malicious payload for exploits that parse JPEG metadata (rare but possible).
Test richiesto: Upload a .jpg file with Content-Type: image/jpeg that is actually a JS file; expect 422.

---

Deferred — Authorization bypass via ancestor-cycle (PATCH /pages/:id parentPageId)

File: apps/server/src/routes/pages.ts:199-201
Severità: HIGH
Soluzione proposta: Walk ancestor chain from input.parentPageId up to root before accepting reparent; reject with 400 if the walk reaches id.
Patch indicativa:

async function wouldCreateCycle(pageId: string, newParentId: string): Promise<boolean> {
  let current: string | null = newParentId;
  while (current) {
    if (current === pageId) return true;
    const row = db.select({ parentPageId: pages.parentPageId }).from(pages)
      .where(eq(pages.id, current)).get();
    current = row?.parentPageId ?? null;
  }
  return false;
}
// before applying reparent:
if (await wouldCreateCycle(id, input.parentPageId)) {
  return c.json({ error: "Cannot create circular parent chain" }, 400);
}

Decision points: Whether to return 400 (bad request) or 422 (unprocessable); whether to expose the reason to the client.
Trade-off: Adds O(depth) DB reads per reparent call. For typical sidebar depth (≤10) this is negligible.
Test richiesto: Create page A → B → C; PATCH A with parentPageId: C; expect 400.

---

Deferred — sortOrder race condition (POST /pages and POST /databases)

File: apps/server/src/routes/pages.ts:113, apps/server/src/routes/databases.ts:104-135
Severità: HIGH
Soluzione proposta: Move getNextSortOrder() call inside the db.transaction() block so the read and write are atomic.
Patch indicativa:

db.transaction(() => {
  const sortOrder = getNextSortOrder(parentPageId, user.id); // ← move inside tx
  db.insert(pages).values({ ..., sortOrder }).run();
});

Decision points: None — pure correctness fix. SQLite WAL serialises writes so no data corruption occurs, but concurrent creates get duplicate sortOrder values and ordering becomes non-deterministic.
Trade-off: Low risk. No API change.
Test richiesto: Two concurrent POST /pages requests with same parentPageId; assert resulting sortOrder values are distinct.

---

Deferred — Cell upsert lacks composite primary key (PUT /databases/:id/rows/:rowId/cells/:propId)

File: apps/server/src/routes/databases.ts:503-525, apps/server/src/db/schema.ts:79-87
Severità: MEDIUM
Soluzione proposta: Add composite PK (row_id, property_id) to database_cell_values in schema + new Drizzle migration; replace DELETE+INSERT with onConflictDoUpdate.
Patch indicativa:

// schema.ts
export const databaseCellValues = sqliteTable("database_cell_values", {
  rowId: text("row_id").notNull().references(…),
  propertyId: text("property_id").notNull().references(…),
  value: text("value", { mode: "json" }),
}, (t) => ({ pk: primaryKey({ columns: [t.rowId, t.propertyId] }) }));

// route
db.insert(databaseCellValues).values({ rowId, propertyId, value })
  .onConflictDoUpdate({ target: [databaseCellValues.rowId, databaseCellValues.propertyId], set: { value } })
  .run();

Decision points: Migration must include CREATE UNIQUE INDEX on existing data with no duplicates — verify first.
Trade-off: Schema migration on live table. Run SELECT row_id, property_id, COUNT(*) FROM database_cell_values GROUP BY 1,2 HAVING COUNT(*) > 1 to confirm no duplicates before applying.
Test richiesto: Two concurrent PUT cell requests for same (rowId, propertyId); assert only one row exists after both complete.

---

Deferred — CORS allows all non-browser origins unconditionally

File: apps/server/src/index.ts:27-28
Severità: MEDIUM
Soluzione proposta: Return null (deny) instead of the falsy origin to block server-side relay with credentials.
Patch indicativa:

origin: (origin) => {
  if (!origin) return null; // ← deny non-browser requests for credentialed CORS
  if (allowedOrigins.includes(origin)) return origin;
  return null;
},

Decision points: Will this break any server-side clients (cron, health check, internal proxy) that call the API with credentials? List all internal callers before applying.
Trade-off: If the API is also used by non-browser clients that rely on CORS credentials passthrough, those clients will break.
Test richiesto: curl -H 'Cookie: session=...' http://localhost:3000/api/auth/me should return 401/CORS rejection after fix.

---

Deferred — N+1 queries in archiveRecursive (replace with recursive CTE)

File: apps/server/src/routes/pages.ts:328-349
Severità: HIGH (performance)
Soluzione proposta: Replace recursive function with a single Drizzle SQL recursive CTE that archives all descendants in one query.
Patch indicativa:

import { sql } from "drizzle-orm";
db.run(sql`
  WITH RECURSIVE cte(id) AS (
    SELECT ${id}
    UNION ALL
    SELECT p.id FROM pages p
    INNER JOIN cte ON p.parent_page_id = cte.id
    WHERE p.created_by = ${user.id} AND p.archived_at IS NULL
  )
  UPDATE pages SET archived_at = ${now}, updated_at = ${now}
  WHERE id IN (SELECT id FROM cte) AND created_by = ${user.id}
`);

Decision points: Verify bun-sqlite supports recursive CTEs (SQLite 3.8.3+; bun ships with SQLite 3.46+ — safe).
Trade-off: More complex SQL, harder to unit-test in isolation. The depth guard added in this PR mitigates the immediate crash risk.
Test richiesto: Create 50-page deep tree; archive root; verify all 50 descendants archived in one DB round-trip (use SQLite EXPLAIN QUERY PLAN).

---

Deferred — N+1 UPDATE loops in reorder endpoints

File: apps/server/src/routes/pages.ts:167-174, apps/server/src/routes/databases.ts:344-356
Severità: HIGH (performance)
Soluzione proposta: Replace per-item UPDATE loop with a single bulk CASE-expression UPDATE.
Patch indicativa:

// Instead of forEach loop, use drizzle sql template:
import { sql, inArray } from "drizzle-orm";
const ids = input.orderedPageIds;
db.run(sql`
  UPDATE pages SET sort_order = CASE id
    ${sql.join(ids.map((id, i) => sql`WHEN ${id} THEN ${i}`), sql` `)}
    ELSE sort_order END,
    updated_at = ${now}
  WHERE id IN (${sql.join(ids.map(id => sql`${id}`), sql`,`)})
  AND created_by = ${user.id}
`);

Decision points: Drizzle raw SQL syntax may need adjustment for bun-sqlite driver. Test with 3+ items.
Trade-off: More complex SQL, but eliminates 100x network round-trips for large sidebars.
Test richiesto: Reorder 10 pages; assert DB state matches requested order with 1 write statement.

---

Deferred — Unbounded SELECT on pages (missing pagination)

File: apps/server/src/routes/pages.ts:65-88
Severità: MEDIUM (performance)
Soluzione proposta: Add pagination (cursor-based or offset) or a hard cap (e.g., 2000 pages max) with a warning header.
Decision points: Cursor vs. offset; how the frontend sidebar handles pagination (requires frontend changes too).
Trade-off: Breaking change for clients; sidebar tree-building logic must also change.
Test richiesto: Insert 5000 pages; assert response time stays under 200ms and payload under 1MB.

---

Deferred — Dead links table (schema + indexes with no route)

File: apps/server/src/db/schema.ts:89-100, apps/server/src/db/index.ts:82-87
Severità: LOW (dead code)
Soluzione proposta: Either implement the links feature or drop the table via a new migration.
Patch indicativa:

-- migration 0002_drop_links.sql
DROP TABLE IF EXISTS links;

Decision points: Is this feature planned? If yes, leave the schema. If no, migrate it out.
Trade-off: DROP TABLE migration is irreversible. Confirm links table is empty in production before applying.
Test richiesto: Run migration on staging; verify no foreign-key errors.

---

Deferred — @emoji-mart/data static import (1.4 MB on every page load)

File: apps/web/src/components/page/PageChrome.tsx
Severità: MEDIUM (performance)
Soluzione proposta: Lazy-load @emoji-mart/data and @emoji-mart/react inside a React.lazy / dynamic import() triggered only when the icon picker opens.
Patch indicativa:

const EmojiPicker = React.lazy(() => import("./EmojiPickerLazy"));
// EmojiPickerLazy.tsx:
import data from "@emoji-mart/data";
import Picker from "@emoji-mart/react";
export default function EmojiPickerLazy(props) { return <Picker data={data} {...props} />; }

Decision points: Show a loading spinner or skeleton while the picker chunk downloads (~400ms on slow 3G).
Trade-off: Adds a Suspense boundary; first open has a visible delay.
Test richiesto: Lighthouse bundle analysis before/after; initial JS parse time should drop by ~300ms.

---

Deferred — Test coverage gaps (multiple paths)

Severità: MEDIUM
Decision points: Whether to use Bun test (server) or Vitest+RTL (frontend) for each gap.

Key uncovered paths (highest risk first):

1. DELETE /api/pages/:id/cover — success, 404, 423 locked
2. PATCH /api/pages/:id self-parent guard → 400
3. PATCH /api/pages/:id cross-user parentPageId → 404
4. PUT /api/pages/reorder with locked sibling → 423
5. /app route beforeLoad auth guard — unauthenticated redirect
6. POST /api/pages/:id/cover-upload locked page → 423
7. updatePropertySchema — empty name, unknown type
8. signupSchema name/password upper-bound (100, 1024 chars)

Sub-analyst reports

• Security: (1) Broken CI action @v6 caused workflow failure; (2) logout missing auth middleware (CSRF risk); (3) file upload validates only client-supplied MIME+extension — no magic-byte check
• Quality: (1) actions/checkout@v6 broken tag; (2) N+1 in archiveRecursive; (3) duplicate getNextSortOrder / getLockedError across pages.ts and databases.ts
• Bugs: (1) Ancestor-cycle bypass in reparent (depth ≥ 2); (2) sortOrder race condition; (3) unbounded archiveRecursive stack overflow
• Tests: (1) DELETE /cover endpoint zero tests; (2) self-parent guard path untested; (3) cross-user parentPageId reparent untested
• Deps: (1) hono 4.12.8 (10+ CVEs → 4.12.22); (2) vite 6.4.1 (2 CVEs → 6.4.2); (3) turbo 2.8.19 (2 CVEs → 2.9.14)
• Performance: (1) N+1 recursive archive; (2) N+1 reorder UPDATE loops (pages + properties); (3) 1.4 MB emoji-mart loaded on every page

---

Generated automatically by Codebase Analyst — 2026-05-23

[coderabbitai]

Warning

Review limit reached

@LiukScot, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 1 review/hour. Refill in 21 minutes and 49 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 592fd8e3-fb64-4a46-9731-8626bd6aa6a3

📥 Commits

Reviewing files that changed from the base of the PR and between 9b52484 and 28b25f6.

⛔ Files ignored due to path filters (1)

• bun.lock is excluded by !**/*.lock

📒 Files selected for processing (10)

• .github/workflows/claude-pr-review.yml
• .gitignore
• apps/server/package.json
• apps/server/src/db/index.ts
• apps/server/src/index.ts
• apps/server/src/routes/pages.ts
• apps/web/package.json
• apps/web/src/components/editor/Editor.tsx
• package.json
• packages/shared/src/validators.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

TEST_ONLY_DO_NOT_POST

[claude]

Sintesi

PR ben strutturata con fix concreti. Trovati 4 problemi da correggere prima del merge.

Findings

[MEDIUM] 1. Off-by-one nel depth guard — pages.ts:331

Con MAX_ARCHIVE_DEPTH = 100, la condizione depth > 100 e falsa a depth=100 — il 101-esimo livello dalla radice viene processato. Se il nome implica max 100 livelli (profondita 0-99), cambiare in depth >= MAX_ARCHIVE_DEPTH.

[MEDIUM] 2. Version constraint hono troppo larga per CVE — server/package.json:18

^4.12.18 ammette versioni da 4.12.18 a 4.12.21 potenzialmente ancora vulnerabili (GHSA-qp7p-654g-cw7p). Il lockfile risolve a 4.12.22, ma una fresh install senza lockfile potrebbe portare una versione pre-fix. Cambiare in ^4.12.22.

[MEDIUM] 3. Version constraint postcss troppo larga per CVE — web/package.json:45

Analogo: ^8.5.10 ammette versioni fino a 8.5.14 vulnerabili (GHSA-qx2v-qp2m-jg93). Lockfile risolve a 8.5.15. Cambiare in ^8.5.15.

[LOW] 4. authMiddleware su /logout — regressione UX — auth.ts:101

Con authMiddleware, un utente con sessione gia scaduta riceve 401 prima del handler — il cookie non viene cancellato. Se il frontend gestisce 401 su /logout con clear del cookie, il rischio e nullo; altrimenti verificare prima del merge.

[claude]

Potenziale regressione UX. Con authMiddleware, un utente la cui sessione e gia scaduta o invalidata server-side riceve 401 prima che il handler venga eseguito — il cookie non viene cancellato. Lx utente non riesce a fare logout finche il cookie non scade naturalmente.

Verificare che il frontend gestisca esplicitamente 401 su /logout (clear del cookie e dello stato locale) prima del merge.

[claude]

Sintesi

PR risolve CVE multipli (hono, vite, postcss, turbo), aggiunge authMiddleware su logout, depth guard su archiveRecursive, fix validazione title. Due problemi bloccanti:

1. CI failing — authMiddleware su logout rompe il test POST /api/auth/logout > returns ok without cookie present (atteso 200, ricevuto 401). Fix: aggiornare auth.test.ts:172 per aspettarsi 401.
2. CVE hono incompleto — bun.lock risolve hono@4.12.18 ma package.json richiede ^4.12.22 (>=4.12.22). Eseguire bun install e committare il lockfile aggiornato.

Findings

Nr	Severita	File	Issue
1	CRITICAL	apps/server/src/routes/auth.ts:101	Test regression logout — CI in errore
2	HIGH	bun.lock:783	hono risolto a 4.12.18, non soddisfa ^4.12.22
3	MEDIUM	apps/server/src/routes/pages.ts:332	Errore depth guard propagato come 500 con messaggio esposto

[claude]

[CRITICAL - CI failing] Aggiungere authMiddleware rompe il test esistente:

(fail) POST /api/auth/logout > returns ok without cookie present
Expected: 200
Received: 401
  at apps/server/src/routes/auth.test.ts:172

La modifica di sicurezza e corretta; il test va aggiornato. In auth.test.ts:172:

- expect(res.status).toBe(200);
+ expect(res.status).toBe(401);

Considerare di rinominare il test in "returns 401 without valid session".

[claude]

[LOW - bun.lock workspace specifier mismatch] La versione risolta nel lockfile e corretta (hono@4.12.22, CVE patchato). Tuttavia la sezione workspace di bun.lock registra il specifier come ^4.12.18 mentre apps/server/package.json richiede ^4.12.22. Questo mismatch causa il fallimento di bun install --frozen-lockfile. Ri-eseguire bun install per allineare il lockfile.

[claude]

[MEDIUM - error message exposure] Questa eccezione viene gettata dentro db.transaction() e non e catturata nel route handler. Hono restituisce un 500 con il messaggio interno "Page tree exceeds maximum depth of 100" visibile al client.

Catturare nel handler e restituire un 422: wrappare db.transaction(...) in try/catch, rilevare questo errore via e instanceof Error && e.message.startsWith("Page tree exceeds"), poi rispondere con c.json(...) e status 422. Alternativamente definire una classe ArchiveDepthError per un instanceof piu preciso.