(opus-4.8 high) Pre-Launch System Review

[kilemensi]

Description

Findings are grouped by severity. Critical = must fix before launch (data breach / cross-tenant compromise / broken deploy pipeline). High = fix before or immediately after launch. Medium/Low = should be tracked, not launch-blocking.

---

Critical

1. Any logged-in user can read and overwrite every AI/Airtable/Meedan API key for the whole platform

• Files: src/globals/Settings/index.ts:12-15, src/globals/Settings/tabs/AITab.ts:487-502, AirtableTab.ts:21, MeedanTab.ts:21, src/globals/Settings/tabs/MaskedApiKeyField.tsx
• The settings global stores providerCredentials[].apiKey, airtableAPIKey, and meedanAPIKey as plain type: "text" fields. Access control is read/update: ({ req }) => Boolean(req.user) — any authenticated user, regardless of role or tenant.
• MaskedApiKeyField.tsx only masks the value in the admin React UI (getMaskedApiKeyPreview); it wraps a plain text field with no access.read restriction at the field or collection level. A logged-in user hitting GET /api/globals/settings (REST) or the equivalent GraphQL query gets every key back in plaintext — the masking provides zero server-side protection.
• Failure scenario: a volunteer/editor account created just to manage one tenant's political-entity pages calls /api/globals/settings and exfiltrates the OpenAI/Anthropic/Google/etc. keys and the Airtable/Meedan tokens for the entire platform, or overwrites them (denial of service, billing abuse, data exfiltration via a swapped Airtable key).
• Fix: add a field-level access: { read: () => false } (or an admin-only role check) on every credential field, and/or move these to server-only env vars entirely (the app already supports env-var fallback per .env.example), removing them from the editable/readable Payload schema.

2. Multi-tenant isolation is fully disabled — every authenticated user has cross-tenant access

• Files: src/plugins/index.ts:64 (userHasAccessToAllTenants: () => true), src/collections/Users/index.ts (fields: [], no access)
• The multi-tenant plugin's own default for userHasAccessToAllTenants is () => false (verified in node_modules/@payloadcms/plugin-multi-tenant/dist/index.js); this app overrides it to unconditionally true. That function gates the tenant-scoping access wrapper (addCollectionAccess/withTenantAccess) applied to pages, site-settings, political-entities, tenants, and to user management — with it hardcoded true, the tenant constraint is skipped for every authenticated user, and access falls back to each collection's own (unrestricted) access function.
• Compounding this: Users has no roles field and no access block at all — there is currently no way to distinguish an admin from a low-privilege tenant editor. Payload's own default access when none is defined is Boolean(user) (node_modules/payload/dist/auth/defaultAccess.js), so every logged-in user can read/create/update/delete every other user account, and (per the above) every tenant's Pages, PoliticalEntities, and per-tenant SiteSettings.
• Failure scenario: a Kenya-tenant editor account logs in and reads/edits Uganda's political entities, pages, and site settings, or promotes their own account by editing another user record — there is no server-side check preventing any of this today.
• Fix: implement a real role/super-admin model (e.g. a roles field on Users and a tenant-membership array), make userHasAccessToAllTenants check that role instead of returning true, and add explicit access functions to Users, Pages, PoliticalEntities, and PromiseStatus that check both authentication and tenant membership.

3. No production deploy pipeline, and the only existing pipeline deploys unconditionally

• File: .github/workflows/deploy-dev.yml (only Dokku-dev workflow; .github/workflows/ contains just this and claude.yml)
• The workflow runs on every push to main: checkout → Docker build → push to Docker Hub → deploy to Dokku, with no lint, typecheck, or test step anywhere in the job (needs: is unused; there is exactly one job).
• There is no deploy-prod.yml or tag/release-gated workflow at all.
• Failure scenario: a broken PR merges to main and is live on (dev today, prod tomorrow if this file is copied under time pressure) within minutes, with no automated gate having run.
• Fix: add a test job (lint + tsc --noEmit + pnpm test:int) that deploy depends on via needs:, and add a separate, approval-gated production workflow before launch.

4. Database migrations are never run automatically

• Verified via grep across Dockerfile, docker/entrypoint.sh, and .github/workflows/deploy-dev.yml: payload migrate (defined only in package.json:16) is never invoked by the container entrypoint or by CI/CD.
• Failure scenario: a PR changes a collection schema; the deploy succeeds; the running app now expects a schema the database doesn't have. This is a silent, easy-to-forget manual step at exactly the moment (launch, then ongoing feature work) where it matters most.
• Fix: run pnpm migrate as a release-phase/pre-boot step in docker/entrypoint.sh before the app starts serving traffic.

---

High

5. DATABASE_URI has no fail-fast validation (inconsistent with PAYLOAD_SECRET)

• File: src/payload.config.ts:32-34 throws a clear error if PAYLOAD_SECRET is missing, but line 74 does url: process.env.DATABASE_URI || "" with no equivalent guard.
• Fix: mirror the PAYLOAD_SECRET guard for DATABASE_URI.

6. No health-check endpoint / container health probe

• No /api/health (or similar) route exists under src/app/; no HEALTHCHECK in Dockerfile or docker-compose.yml. The entrypoint script does check Tika readiness internally (docker/entrypoint.sh:70-92) but doesn't expose that state externally.
• Fix: add a cheap /api/health route (DB ping) and wire it into a Dokku/Docker health check.

7. AIExtractions.extractions[].uniqueId is not required, breaking the Meedan upload matching logic

• File: src/collections/AIExtractions/index.ts:102-110; matched against in src/tasks/uploadToMeedan.ts (matches by uniqueId).
• An extraction row saved without a uniqueId will never match during the upload-status update, leaving it permanently "pending" even after a successful upload.
• Fix: mark uniqueId required: true, or backfill/generate it in a beforeChange hook if it's meant to be system-generated.

8. No per-run concurrency guard on the AI extraction workflow/cron

• Files: src/workflows/airtableWorkflow.ts, job queue config in src/payload.config.ts (default PAYLOAD_JOBS_CRON_SCHEDULE="* * * * *")
• The exportRows job already got a "stop pileup, make transaction-safe" fix (per recent commit a0422fc5); extractPromises/airtableWorkflow don't appear to have the same guard. If a run takes longer than the 1-minute cron interval, a second run starts against the same unprocessed documents.
• Failure scenario: a slow AI provider makes one extractPromises run take 90s; the next cron tick starts a second run over the same documents, producing duplicate AI extractions.
• Fix: apply the same "already queued/running" check used for exportRows to extractPromises/airtableWorkflow.

9. No timeout on AI provider calls, and no document-size cap before chunking

• File: src/tasks/extractPromises.ts (generateText() calls around the chunk-processing loop; 14,000-char chunking with no upstream size limit)
• A hung provider call blocks the task indefinitely (recovered only by the 2-hour stuck-job cleanup); an oversized or malicious document generates an unbounded number of chunks/API calls with no cost ceiling.
• Fix: add a request-level timeout on generateText() calls, and a hard max-document-size check before chunking begins (reject/flag oversized documents instead of processing them).

10. Missing error.tsx / not-found.tsx at the frontend root

• Verified: find "src/app/(frontend)" shows only layout.tsx and page.tsx at the top level — no error.tsx, no not-found.tsx anywhere under (frontend).
• Failure scenario: an unhandled error on any public page (promise detail, entity page) surfaces Next.js's default error UI instead of a branded, graceful page; unmatched routes may not 404 cleanly.
• Fix: add src/app/(frontend)/error.tsx and not-found.tsx.

11. Sequential (non-parallel) data fetching on hot public pages

• File: src/app/(frontend)/[entitySlug]/promises/[promiseId]/page.tsx — getTenantNavigation() → getPoliticalEntityBySlug() → getPromiseById() → getPromiseUpdateEmbed() → getTenantSiteSettings() → resolveMedia() are awaited one after another; several of these don't depend on each other's output and could run concurrently via Promise.all.
• Fix: parallelize the independent lookups; this directly affects TTFB on the highest-traffic page type at launch.

---

Medium

12. Webhook auth in meedan-sync uses non-constant-time comparison

• File: src/app/api/meedan-sync/route.ts:310 — providedSecret !== configuredSecret. Contrast with the airtable-upload route, which uses crypto.timingSafeEqual.
• Fix: use timingSafeEqual here too (pad/length-check first to avoid throwing on length mismatch).

13. Deleting a PromiseStatus or PoliticalEntity leaves denormalized export-row data stale

• File: src/collections/PromiseStatus/hooks/index.ts:27-44 queues a re-sync on status delete but doesn't clear export rows that reference the deleted status first, so rows can carry a statusId pointing at nothing with a stale statusLabel.
• Fix: delete/null out the affected export-row fields before (or as part of) queuing the re-sync.

14. AI provider credential resolved once per task, not re-checked if Settings change mid-run

• File: src/tasks/extractPromises.ts — credentials are resolved at task start; if an admin changes the provider/key while a long run is in progress, the task keeps using the stale credential rather than failing fast or picking up the change.
• Fix: acceptable to leave as-is if runs are short; otherwise re-resolve per document/batch.

15. No retry backoff on AI provider calls

• File: src/tasks/extractPromises.ts (maxRetries: 3/4 on generateText()), no visible backoff configuration.
• Failure scenario: a rate-limited or briefly-outaged provider gets hit with immediate retries across many documents/chunks in parallel, amplifying the outage and burning quota/cost.
• Fix: confirm the ai SDK's built-in retry behavior includes backoff; if not, wrap with exponential backoff.

16. No application-level security headers

• File: next.config.mjs — no headers() config for CSP, X-Frame-Options, Referrer-Policy, etc.
• Fix: add a baseline security-headers config (Next.js headers() function), at minimum clickjacking (X-Frame-Options/frame-ancestors) and a reasonable CSP given third-party embeds (share widgets, media).

17. Sentry configured at 100% trace sampling with no PII scrubbing

• Files: sentry.server.config.ts, sentry.edge.config.ts — tracesSampleRate: 1, enableLogs: true, no beforeSend. meedan-sync/route.ts:190 logs clientIp/userAgent, which could flow into Sentry breadcrumbs unfiltered.
• Fix: lower tracesSampleRate for production via env, add a beforeSend hook to scrub IP/PII fields before events are sent.

18. Backfill migration isn't safely re-runnable if interrupted

• File: migrations/20260423_000000_backfill_ai_extraction_export_rows.ts — deletes existing export rows for a batch, then insertMany(..., { ordered: false }). If interrupted between delete and insert, or if insert partially fails, re-running can leave a partial/inconsistent batch.
• Fix: wrap the delete+insert per batch in a transaction, or add a dedupe/reconciliation check.

---

Low

• Footer logo missing meaningful alt text — src/components/Footer/index.tsx:72 (secondary logo alt=""); should carry the org/site name for screen readers.
• Promise status indicator relies on color alone — src/components/PromiseStatus/PromiseStatus.tsx; consider an icon/pattern alongside color for colorblind users.
• Hardcoded English fallback strings in Hero (src/components/Hero/index.tsx:396-406) will show through on the French locale if a block's config omits copy — move defaults into locale-aware config.
• ESLint: 0 errors, 39 warnings (mostly no-explicit-any / unused vars), concentrated in src/tasks/createPoliticalEntity.ts and src/tasks/downloadDocuments.ts — non-blocking cleanup.
• No dead-letter/failure escalation for the job queue — failed jobs (hasError=true) aren't surfaced to Sentry; an operator has to check the Payload admin Jobs list manually to notice a stuck sync.
• Stuck-job cleanup at a fixed 2-hour threshold (src/tasks/cleanupFailedJobs.ts) could delete a legitimately long-running large-document extraction rather than just abandoned jobs; consider a longer threshold or archiving instead of deleting.

---

Verified test/lint status (as of this review)

• pnpm exec vitest run (int suite): 7 passed, 1 failed — the 1 failure (tests/int/api.int.spec.ts) requires a live MongoDB Atlas connection unreachable from this sandbox; not a code defect. 32 tests passed, 1 skipped.
• pnpm lint: 0 errors, 39 warnings.
• Dockerfile: multi-stage build, runs as non-root (nextjs, uid 1001), uses BuildKit --mount=type=secret correctly for database_uri/payload_secret/Sentry tokens (not baked into image layers), .dockerignore excludes .git/node_modules/.env*/tests — this part of the release story is solid.

---

Checked and ruled out (do not re-flag)

• generateMetadata "missing await" on SEO calls — an initial pass flagged return buildSeoMetadata(...) (no await) inside generateMetadata in the promise-detail and catch-all pages as breaking SEO metadata. This is not a bug: generateMetadata is itself async, and return <promise> inside an async function is automatically flattened/awaited by the JS runtime before the function's own promise resolves — behaviorally identical to return await <promise>. No fix needed.
• Public read: () => true on Documents, Promises, Media, AIExtractions, Partners — flagged by an initial pass as "unauthenticated data exposure." Given PromiseTracker's purpose (public accountability/transparency of promises and their evidence), public read access to this content is very likely intentional product behavior, not a defect. Worth a one-line confirmation with product ownership before launch, but not treated as a vulnerability here — the real problem is finding Fix pie chart events on hovering #1 above (writable/readable credentials), not readable public content.

---

Suggested fix order before launch

1. Lock down globals/Settings credential fields (finding Fix pie chart events on hovering #1) and fix the tenant-access bypass + add real roles (finding Update Readme #2) — these two together mean the app currently has no meaningful access control boundary.
2. Add a CI gate (lint/typecheck/test) to the deploy workflow and wire payload migrate into the release path (findings Fix filters history + indicators #3, Add Issue and PR templates #4).
3. Add DATABASE_URI validation, a health-check endpoint, and error.tsx/not-found.tsx (findings Filters not updating on history pop #5, Homepage contribute section #6, Add about us page #10) — cheap, high-value production-hardening.
4. Everything else in High/Medium can reasonably follow in the first post-launch iteration.