Harden auth and logging in chat/download paths

⛔ closed · #10 · willchen96/mike ← abbyshekit/mike · opened 25d ago by abbyshekit · closed 25d ago · +87-11 across 5 files · ↗ on GitHub

From the PR description

Summary

Three small, themed security/operational fixes for the Express backend. All changes are surgical (one commit, +87/-11 across 5 files) and compile cleanly.

What changed

1. POST /chat/create enforces checkProjectAccess

Without this, any authenticated caller can attach a chat row to any project_id, even one they cannot read. The streaming POST /chat already enforces the same rule, so /chat/create should as well. Also validates project_id is a non-empty string before the check, so a malformed body returns 400 instead of falling through to the insert and producing a 500.

(This bug was independently caught and patched in ryanmcdonough/mike - credit there for spotting it. Reproduced and shipped here against latest main.)

2. Gate Claude raw-stream disk logging behind DEBUG_RAW_LLM_STREAM

backend/src/lib/llm/claude.ts was unconditionally appending every Claude SSE event to claude-raw-stream.log in the working directory on every chat. In production this grows without bound and persists model output (which for this app includes user-submitted legal documents) to disk.

The mirror is now opt-in. The env var is parsed against an explicit truthy allowlist (1 / true / yes / on, case-insensitive) so common "disabled" values like false or 0 don't accidentally enable it. When enabled, the log file is created with mode 0o600, and a code-comment notes the PII risk.

3. Drop "dev-secret" fallback in downloadTokens.ts

getSecret() previously fell back to a hardcoded "dev-secret" literal if neither DOWNLOAD_SIGNING_SECRET nor SUPABASE_SECRET_KEY was set - letting any caller mint download URLs for arbitrary keys. Now throws if no secret is configured.

To avoid a misconfigured deploy turning every saved download link into a 500, assertDownloadSigningConfigured() is exported and called once from backend/src/index.ts so the server fails fast at boot with a clear error.

Empty-string env values (DOWNLOAD_SIGNING_SECRET=) are trimmed and ignored so an unfilled env template still falls back to SUPABASE_SECRET_KEY rather than failing.

How to test

# 1. Build cleanly
npm run build --prefix backend

# 2. Boot guard fires when no signing secret is set
unset DOWNLOAD_SIGNING_SECRET SUPABASE_SECRET_KEY
npm run start --prefix backend
# → exits with: "Download signing secret is not configured: set DOWNLOAD_SIGNING_SECRET (preferred) or SUPABASE_SECRET_KEY."

# 3. Boot succeeds with a real secret set
export DOWNLOAD_SIGNING_SECRET="$(openssl rand -hex 32)"
# ... start backend, exercise /chat/create with and without project_id, with and without project access

# 4. Raw-stream logging stays off by default and on common false-y values
DEBUG_RAW_LLM_STREAM=false npm run dev --prefix backend  # no log file written
DEBUG_RAW_LLM_STREAM=1     npm run dev --prefix backend  # log file appears with mode 0600

Out of scope (deferred)

The following came up during review and are intentionally not in this PR - happy to do as follow-ups if useful:

  • TOCTOU race between the project access check and the chat insert. Real but very low probability; properly addressed by Supabase RLS / a DB-side insert RPC rather than route code.
  • Requiring DOWNLOAD_SIGNING_SECRET explicitly in production (i.e., disallowing the Supabase-key fallback when NODE_ENV=production). Useful but a maintainer call.
  • Backend test coverage. The repo currently has no backend tests / test runner, and bootstrapping one as a side effect of a security fix felt like scope creep. Worth a dedicated PR.
  • Email case-sensitivity in projects.ts sharing checks (separate bug, separate fix).
  • Express 5 / Multer 2 / Next.js patch bumps - covered well by other forks (see hreiten/mike).

Notes

  • Diff is one commit, +87/-11. No dependency changes, no schema changes, no formatting churn (existing files were not prettier-clean and were left untouched outside the changed regions).
  • License: AGPL-3.0-only (unchanged).

Our analysis

Three surgical security fixes for the Express backend — read the full analysis →

Think the analysis missed something the PR description covers?

Capture this PR into my fork

Download a Markdown prompt that tells Claude how to port every commit in this PR into your working tree. Run it via claude -p < capture-pull-10.md from inside the repo you want the changes in.

⬇ Download capture-pull-10.md