From 2798798563627f7dd6d3428ac3ed15bbb6a76544 Mon Sep 17 00:00:00 2001 From: lorentz Date: Sat, 11 Jul 2026 11:37:47 -0400 Subject: [PATCH] docs(13): add code review report 2 critical, 3 warning, 1 info. Critical findings: unguarded POST handler in /api/pax8/sync (no try/catch around the new integration_settings query or PAX8 client init), and a missing admin/role permission check on the same cost-incurring route (any authenticated session can trigger a sync, unlike the equivalent admin toggle route). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LHRgZqkzBHBbAbc3KHneuR --- .../13-scheduler-admin-toggle/13-REVIEW.md | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 .planning/phases/13-scheduler-admin-toggle/13-REVIEW.md diff --git a/.planning/phases/13-scheduler-admin-toggle/13-REVIEW.md b/.planning/phases/13-scheduler-admin-toggle/13-REVIEW.md new file mode 100644 index 0000000..c8ac096 --- /dev/null +++ b/.planning/phases/13-scheduler-admin-toggle/13-REVIEW.md @@ -0,0 +1,191 @@ +--- +phase: 13-scheduler-admin-toggle +reviewed: 2026-07-11T00:00:00Z +depth: standard +files_reviewed: 5 +files_reviewed_list: + - migrations/096_pax8_daily_schedule.sql + - lib/services/sync-scheduler.ts + - CLAUDE.md + - lib/services/integration-health.ts + - app/api/pax8/sync/route.ts +findings: + critical: 2 + warning: 3 + info: 1 + total: 6 +status: issues_found +--- + +# Phase 13: Code Review Report + +**Reviewed:** 2026-07-11 +**Depth:** standard +**Files Reviewed:** 5 +**Status:** issues_found + +## Summary + +Phase 13 wires `Pax8SyncService.fullSync()` into a daily cron schedule (migration 096 + +`sync-scheduler.ts`), registers PAX8 as a toggleable row on `/admin/integrations` +(`integration-health.ts`), and gates the existing manual-trigger route +(`app/api/pax8/sync/route.ts`) on the same `integration_settings.disabled` flag with a +403. The scheduler-side dual guard (`isPax8Configured()` + DB toggle) is implemented +correctly and was live-verified per 13-03's SUMMARY. `integration-health.ts`'s new +`checkConfigOnly('pax8', ...)` entry is a clean, low-risk addition consistent with the +existing config-only pattern, and the `CLAUDE.md` doc update accurately reflects the +new behavior. + +The route-level gate (`app/api/pax8/sync/route.ts`) is where the real problems are: the +POST handler has no `try/catch` at all (a pre-existing gap from phase 11 that phase +13-02 made worse by adding a second unguarded DB query), and there is no role/permission +check on what is a privileged, cost-incurring action — combined with an unresolved +inconsistency against the route's own stated design intent (to "match itglue/veeam +sync routes," which are registered as public in `middleware.ts`; `/api/pax8/sync` never +was). Migration 096 also reproduces a pre-existing systemic risk (also present in +089/090) where the seed INSERT targets a table that is only ever created by the Node +app at runtime, not by any migration — a genuine hazard on a truly fresh Postgres +volume, masked in this project's own testing because verification always runs against a +long-lived dev DB. + +## Critical Issues + +### CR-01: POST /api/pax8/sync has no error handling — any failure crashes with a raw 500 instead of the project's JSON error convention + +**File:** `app/api/pax8/sync/route.ts:5-30` +**Issue:** The entire `POST` handler is missing a `try/catch`. Two concrete failure paths are unguarded: +1. `postgresClient.query(...)` (lines 6-8, added by phase 13-02) can throw (transient DB error, connection hiccup) — this propagates as an unhandled rejection out of the route handler instead of the standard `NextResponse.json({ error, message }, { status })` shape mandated by `CLAUDE.md` ("Errors: try/catch, return NextResponse.json..."). +2. `getPax8SyncService()` (line 19) calls `getPax8Client()` internally (`lib/services/pax8-factory.ts:9-13`), which throws synchronously — `'PAX8 is not configured — set PAX8_CLIENT_ID and PAX8_CLIENT_SECRET'` — if credentials are missing. Every sibling sync route (`app/api/appgate/sync/route.ts:19-24`, similarly veeam/qbo) catches this class of error and returns a clean `503` with a descriptive message. This route instead lets it fall through as an unhandled exception → generic 500. + +This is a direct regression versus the established pattern in this exact commit family (13-02 added the query without adding the surrounding guard the rest of the codebase uses everywhere else for sync-trigger routes). + +**Fix:** +```ts +export async function POST(req: NextRequest) { + try { + if (!isPax8Configured()) { + return NextResponse.json( + { error: 'PAX8 not configured — set PAX8_CLIENT_ID/PAX8_CLIENT_SECRET' }, + { status: 503 } + ); + } + const disabledRes = await postgresClient.query<{ disabled: boolean }>( + `SELECT disabled FROM integration_settings WHERE key = 'pax8'` + ); + if (disabledRes.rows[0]?.disabled === true) { + return NextResponse.json( + { error: 'PAX8 is disabled', message: 'PAX8 sync is disabled via /admin/integrations' }, + { status: 403 } + ); + } + + const body = await req.json().catch(() => ({})); + const triggeredBy = body.triggeredBy || 'manual'; + + const svc = getPax8SyncService(); + if (svc.isSyncInProgress()) { + return NextResponse.json({ error: 'Sync already in progress' }, { status: 409 }); + } + + svc.fullSync(triggeredBy).catch(err => + console.error('[Pax8Sync] Background sync error:', err.message) + ); + + return NextResponse.json({ ok: true, message: 'PAX8 sync started' }); + } catch (err) { + console.error('[Pax8Sync] POST /api/pax8/sync failed:', err); + return NextResponse.json( + { error: err instanceof Error ? err.message : 'Failed to start PAX8 sync' }, + { status: 500 } + ); + } +} +``` + +### CR-02: No authorization/role check on a privileged, cost-incurring action — and the route's stated auth model doesn't match reality + +**File:** `app/api/pax8/sync/route.ts:5` (and `middleware.ts:6-44`, referenced for context — not itself in this phase's diff) +**Issue:** `POST /api/pax8/sync` triggers a full external PAX8 API sync (real API calls, ~30k row upserts per 13-03's live test) and now also enforces the new 403 disable-gate — but the handler performs **no permission/role check at all** (no `requireAuth()`/`requireAdmin()`/`requirePermission()` from `lib/auth-utils.ts`, unlike `app/api/admin/integrations/route.ts` which correctly calls `requirePermission('admin', 'access')` for the equivalent toggle action). + +Compounding this: the route's introducing commit (`ad992f3`, phase 11-02) states the route was deliberately left off `middleware.ts`'s public allowlist "to match itglue/veeam sync routes." That claim is false as written today — `middleware.ts:31-33` explicitly lists `/api/itglue/sync` and `/api/veeam/sync` (and `/api/qbo/sync`, `/api/appgate/sync`) as public, session-less, "webhook-style" entrypoints, but `/api/pax8/sync` is not in that list. 13-03's own live-verification SUMMARY independently confirms this at runtime ("This route requires an authenticated session — curl without a session cookie gets redirected to /auth/sign-in by middleware"). So today the route sits in an unintentional middle state: not public like its stated siblings, but also not admin-gated like the equivalent `/api/admin/integrations` toggle route. Net effect: **any authenticated user of any role** (not just admin/super-admin) can trigger a full PAX8 sync and can also currently observe whether it's disabled, sync history, and row counts via `GET` (also unguarded). + +There is currently no UI caller wired to this route (confirmed via repo-wide search), which limits present-day exploitability, but the API itself is reachable by any logged-in session today and this exposure will activate the moment a "trigger sync" button is added to any page a non-admin user can reach. + +**Fix:** Pick one deliberate model and make the code match it: +- If this is meant to be an admin-triggered action (consistent with "gating the manual sync route" being framed as an admin control surface in the phase goal), add the same guard `/api/admin/integrations` uses: +```ts +import { requirePermission } from '@/lib/auth-utils'; + +export async function POST(req: NextRequest) { + const { error } = await requirePermission('admin', 'access'); + if (error) return error; + // ... existing logic +} +``` +- If it's meant to be scheduler/webhook-style (matching the commit message's stated intent), add `/api/pax8/sync` to `middleware.ts`'s `publicRoutes` array alongside `/api/itglue/sync` / `/api/veeam/sync`, and rely on the 403 disabled-check + `isPax8Configured()` as the only gates (matching qbo/appgate/veeam). Given this route is also directly reachable by any logged-in session today, the admin-gated option is the safer default given the cost/side-effects of the action. + +## Warnings + +### WR-01: Migration 096 (and its 089/090 precedents) seed `sync_schedules` before any migration creates that table — will fail on a genuinely fresh Postgres volume + +**File:** `migrations/096_pax8_daily_schedule.sql:8-13` +**Issue:** `sync_schedules` is only ever created via `CREATE TABLE IF NOT EXISTS sync_schedules (...)` inside `SyncScheduler.createSchedulesTable()` (`lib/services/sync-scheduler.ts:140-163`), which runs as a side effect of the Next.js app importing `sync-scheduler.ts` at server startup. No SQL migration creates this table. `docker-compose.yml` mounts `./migrations` as `/docker-entrypoint-initdb.d` (read-only), which Postgres's official image executes with `psql -v ON_ERROR_STOP=1` **before the application container has ever run**, on a genuinely fresh volume. On such a volume, this `INSERT INTO sync_schedules ...` (and the equivalent inserts in migrations 089 and 090) will fail with `relation "sync_schedules" does not exist`, aborting that init file (and, per `ON_ERROR_STOP=1` + the entrypoint script's `set -e`, likely halting the rest of the init sequence too). + +This is a systemic, pre-existing issue (089/090 already carry it) that 096 faithfully reproduces rather than fixes — it wasn't caught in this phase's own testing because 13-01's SUMMARY explicitly says the migration was "applied to the running dev DB" (a long-lived volume where `sync_schedules` already exists), never against a fresh volume. Given `CLAUDE.md`'s own "Watch out for" section already flags migration-ordering fragility, this is worth fixing now rather than letting a fourth migration reproduce it. + +**Fix:** Either move `CREATE TABLE IF NOT EXISTS sync_schedules (...)` into an early numbered migration (so it exists by the time 089/090/096 run on a fresh volume), or guard the seed inserts: +```sql +INSERT INTO sync_schedules (id, name, description, cron_expression, sync_type, is_enabled) +SELECT 'pax8-daily', 'PAX8 Daily Sync', '...', '0 4 * * *', 'pax8-daily', false + WHERE to_regclass('sync_schedules') IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM sync_schedules WHERE name = 'PAX8 Daily Sync'); +``` + +### WR-02: Migration 096's idempotency guard checks `name`, not the primary key `id` — a mismatched pre-existing row would cause a hard failure instead of a no-op + +**File:** `migrations/096_pax8_daily_schedule.sql:8-13` +**Issue:** `sync_schedules.id` is `VARCHAR(50) PRIMARY KEY` (`lib/services/sync-scheduler.ts:143`). The migration's `WHERE NOT EXISTS` guard checks `name = 'PAX8 Daily Sync'`, not `id = 'pax8-daily'`. If a row with `id='pax8-daily'` already exists under a different `name` (e.g., hand-edited via the schedule admin API — `PATCH`/`POST` on `app/api/sync/schedules/*`), the `NOT EXISTS` check is satisfied (no row with that `name`), the `INSERT` proceeds, and it fails with a primary-key violation rather than being the idempotent no-op the migration's own comment promises. This mirrors the same latent gap in migration 089, not a new pattern invented here, but it's worth closing while touching this file. + +**Fix:** +```sql +INSERT INTO sync_schedules (id, name, description, cron_expression, sync_type, is_enabled) +SELECT 'pax8-daily', 'PAX8 Daily Sync', '...', '0 4 * * *', 'pax8-daily', false + WHERE NOT EXISTS (SELECT 1 FROM sync_schedules WHERE id = 'pax8-daily' OR name = 'PAX8 Daily Sync'); +``` + +### WR-03: Identical `integration_settings` disabled-check query duplicated verbatim across two files with no shared helper + +**File:** `lib/services/sync-scheduler.ts:469-471`, `app/api/pax8/sync/route.ts:6-8` +**Issue:** `SELECT disabled FROM integration_settings WHERE key = 'pax8'` (plus the `rows[0]?.disabled === true` check) is copy-pasted between the scheduler branch and the route handler. Per this phase's own PLAN/SUMMARY notes this is an explicit, documented decision (D-01: "no shared helper," deferred), so it's not an oversight — but it's still a real forward-maintenance risk: `integration-health.ts` already has its own, third, subtly different implementation of "is this key disabled" (`getDbDisabledKeys()`, which also merges the `INTEGRATIONS_DISABLED` env var and key aliases — neither of the two PAX8-specific call sites honor that env var at all). If PAX8 is ever added to `INTEGRATIONS_DISABLED`/alias handling, only the health-check display would respect it; the scheduler and manual route would silently keep running. Worth a small shared helper before a fourth call site appears. + +**Fix:** +```ts +// lib/services/integration-health.ts +export async function isIntegrationDisabledInDb(key: string): Promise { + const { default: postgresClient } = await import('@/lib/services/postgres-client'); + try { + const res = await postgresClient.query<{ disabled: boolean }>( + 'SELECT disabled FROM integration_settings WHERE key = $1', + [key], + ); + return res.rows[0]?.disabled === true; + } catch { + return false; + } +} +``` +Both `sync-scheduler.ts` and `route.ts` can then call `isIntegrationDisabledInDb('pax8')`. + +## Info + +### IN-01: GET /api/pax8/sync exposes sync history and row counts to any authenticated user with no role check + +**File:** `app/api/pax8/sync/route.ts:32-64` +**Issue:** Consistent with CR-02, `GET` has no permission check either. It's read-only and low-sensitivity (row counts, in-progress flag, last 10 sync_history rows), so this is informational rather than a blocker, but if CR-02 is fixed with a `requirePermission` gate on `POST`, consider applying the same gate to `GET` for consistency. +**Fix:** Add the same `requirePermission('admin', 'access')` check used for `POST` if PAX8 sync status is judged sensitive enough to restrict; otherwise leave as-is and note the decision. + +--- + +_Reviewed: 2026-07-11_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_