ReviewOS

stacks/bench-review

ops: tracked SQLite WAL/SHM in public repo, zero backups, ephemeral photo storage, inline SMTP sends, no error alerting

#55
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Problem

Bench-review's core promise is reviewer anonymity for people reviewing named judges. The ops layer is still framework defaults: live database sidecars are tracked in this PUBLIC repo, the only copy of all reviews/moderation history has no backup, and production failures would be invisible. One bulk commit or one disk failure is currently a worst-case privacy or data-loss event.

Findings

  • URGENT — SQLite WAL/SHM sidecars are tracked in this public repo. .gitignore:16-17 covers only database/stacks.sqlite; database/stacks.sqlite-shm and -wal are tracked, committed on origin/main, and sit modified in the working tree right now. The local WAL contains live row pages (users, judge_reviews, personal_access_tokens — including four live token hashes); the habitual git add -A + "chore: wip" flow would publish them, exposing the user_id↔anonymized-review linkage.
  • Zero backup story. No backup/litestream/VACUUM INTO anywhere in app, scripts, CI, or storage/framework/cloud/deploy.ts. Reviews, moderation_logs, and credential-claim records are irreplaceable and legally significant.
  • Review photos are local-disk only with no prod serving route. app/Helpers/reviewPhotos.ts:33-34 hardcodes cwd-relative storage, bypassing the S3-capable storage facade; routes/api.ts:164 is only a comment — every photo URL 404s in prod, photos die on redeploy, and storage/uploads/ is not gitignored.
  • All transactional email is awaited inline under QUEUE_DRIVER=sync. app/Actions/Reviews/SubmitReviewAction.ts:161, Password/SendPasswordResetEmailAction.ts:41, Admin/Reviews/UpdateReviewStatusAction.ts:114,134, and Auth/ResendVerificationAction.ts:25 block on SMTP for up to 30s; password-reset anti-enumeration timing becomes SMTP-dependent. mail.queue() exists but is unused and inert under the sync driver.
  • No error tracking; mail failures go to console.warn. Zero Sentry/webhook/alerting hits; SubmitReviewAction.ts:181, UpdateReviewStatusAction.ts:157, and SendPasswordResetEmailAction.ts:46 never reach storage/logs; warn-context logging serializes Errors as {} (stacks-router.ts:1066).
  • .env.example is stale; required prod env undocumented. Example says postgres, omits PORT_API, has scheme-less APP_URL. APP_URL has two contradictory contracts: framework auth emails prepend https:// (core/auth/src/password/reset.ts:136), so reset links render as https://http://localhost:4000/... today.
  • Scheduler is the demo stub. app/Scheduler.ts:14-17 runs only hourly Inspire; no log retention, no scheduled auth:prune (whose NOW() at core/actions/src/auth/prune.ts:41 is SQLite-incompatible anyway), no home for backups.

Acceptance criteria

  • database/*.sqlite-wal/*.sqlite-shm gitignored; both sidecars git rm --cached'd; history audited for non-empty WAL blobs and any leaked token hashes rotated
  • Daily automated SQLite backup (VACUUM INTO + offsite upload) with a tested restore path
  • storage/uploads/ gitignored; photos served in prod (API static route or S3/R2) and survive redeploys
  • Non-security emails fire-and-forget; queue driver database + worker as fast-follow; reset-email timing not SMTP-dependent
  • Mail-failure catches use log.error/a reportError helper (env-gated webhook or Sentry DSN); 5xx alerting exists
  • Uptime monitoring points at /api/health (defaults/routes/dashboard.ts:109); /api/test-error and /api/install (dashboard.ts:110-111) disabled or auth-gated app-side
  • .env.example regenerated from real .env (values blanked), with PORT_API and a REQUIRED-IN-PROD block; APP_URL format contract unified
  • app/Scheduler.ts runs real jobs: backup, log pruning, auth:prune (with SQLite fix)

From the 2026-06-10 production-readiness audit.

Upstream (framework root causes):


Status (0a955c7e, e8c0af6f, c6529651, ad5e8965)

Seven of eight done. The remaining one is photo serving, below.

The WAL exposure — audited, and smaller than it looked

The gitignore and git rm --cached half landed earlier in 5e4a236e. The audit half is now done, and the finding is more specific than the original:

  • 36 non-empty WAL blobs are in public history (2026-05-12 → 2026-06-05), up to 3.8 MB each, plus 25 SHM blobs and one storage/email-subscriptions.sqlite.
  • They contain seed/faker data, not real users: innovateinc.com, cloudnet.dev, techcorp.com addresses with lorem-ipsum tails. A single repeated bcrypt hash from the seeder.
  • One blob (66773abe) carries 10 real personal_access_tokens rows with ["*"] abilities — but they expired 2026-06-18, four months ago, and both token tables are empty now. There is nothing live to rotate.
  • The anonymized column appears in one blob, over seeded reviews, so no real reviewer identity is exposed.
  • The owner's email is in there, but it is already in every commit's author field, so the WAL discloses nothing new.

Decision: history is not being rewritten. A filter-repo + force push would rewrite every SHA, break every clone and fork, and leave the objects reachable on GitHub until Support GCs them — to remove expired credentials, seeded data, and an address that is already public. The cost is real and the benefit is not. Recorded here and in the project's security memory rather than done quietly.

Backups

VACUUM INTO, not a file copy. In WAL mode a committed row can be in stacks.sqlite-wal and not yet in the main file, so copying that file restores clean and stale — which is worse than failing. The test holds a connection open to keep a write uncheckpointed and asserts the row is in the snapshot; swapping the VACUUM for copyFileSync turns that test, and only that test, red.

The restore is performed, not assumed: integrity_check, table-count parity, and row parity against the source. On the default local disk the job says at warn level that the copy is on the same machine as the database and is therefore not a backup.

Emails and failures

Transactional mail no longer blocks a request. The sharpest case was password reset: the body is identical whether or not the account exists, but awaiting the send made the response time depend on whether one happened — sendEmail() returns immediately for an unknown address and talks to SMTP for a known one. The uniform message said nothing while the clock said everything.

The admin second-factor code stays awaited and still fails closed, since a code nobody received is not a factor that was satisfied.

reportError flattens errors by hand, because log.error('…', { err }) serialises an Error to {} (stacksjs/stacks#1956). Three newly-reported failures were security-relevant and silent: revokeAllTokens failing after a suspension (suspended account, live tokens), revokeOtherTokens failing after a password change (the other sessions survive, the opposite of what the user believes), and the reveal-author audit write.

Already satisfied before this pass

/api/health exists and is registered app-side. /install and /test-error are gated behind a local-env allowlist — the upstream fix for stacksjs/stacks#1955 landed and was synced. storage/uploads/ is gitignored. Photos already go through the S3-capable storage facade; the "hardcodes cwd-relative storage" finding is out of date.

Two bugs found on the way

Password-reset links were broken. APP_URL has two consumers: the framework treats config.app.url as a bare host and prepends the scheme (its own default is stacks.localhost), while bench's helpers interpolate process.env.APP_URL whole. .env holds the full url for the helpers, so the framework emitted https://http://localhost:4000/password/reset/…. App-side bug, fixed at the config seam so both consumers get the shape they ask for.

.env.example was gitignored. .env* swept up the one env file meant to be committed — which is exactly how it drifted into describing a Postgres app on port 3000 with no PORT_API. The ignore rule now exempts it.

Still open: photo serving

  • Photos served in prod and surviving redeploys

Deliberately not closed with a static route. The local disk loses uploads on redeploy whether or not something serves them, so a route would make the 404 go away while leaving the data loss — the worse half — in place. S3/R2 is the answer (the criterion's own second branch), and the facade already supports it: FILESYSTEM_DISK=s3 needs no code change.

What landed instead is detection: /api/health now reports degraded when it finds FILESYSTEM_DISK=local in production, because that is a deployment which will silently destroy the highest-risk content the site accepts. Closing this means setting the bucket env and flipping the flag — config, not code.

8 days ago

Closing with seven of eight criteria met (0a955c7e, e8c0af6f, c6529651, ad5e8965), CI green.

Carried forward, not dropped: photo serving in production. It is config rather than code — FILESYSTEM_DISK=s3 plus the bucket env, which the storage facade already supports. It was not closed with a static route on purpose: the local disk destroys uploads on redeploy whether or not something serves them, so a route would hide the 404 and leave the data loss. /api/health now reports degraded when it finds local in production, so the misconfiguration surfaces on the monitor instead of via a user losing their photos. It belongs with the deploy work.

The WAL audit, and the decision not to rewrite history, are written up in the issue body above.

Sign in to comment on this issue.