fix(security): critical — repair PIN reset + enforce real prod secrets

C1: reset_user_pin wrote to a non-existent column (pin_failed_attempts); the real column is failed_pin_attempts, so every PIN reset 500'd. Fixed the column name; new e2e (pin-reset.spec.ts) proves a reset returns a usable PIN and the target can recover with it. C2: config.rs::validate_secrets now rejects placeholder-ish secrets (change_me/dev_secret/placeholder), enforces len>=32 in prod, and requires a real ADMIN_PASSWORD_HASH. docker-compose.yml sets APP_ENV=production so the guard actually runs. Corrected the false "fixed" claim in SECURITY-BACKLOG.md. .env.example documents the rule. Riders in these files (documented here since git can't split hunks): - host.rs also carries the event-scoped ban_user fix and the close_event/open_event no-op broadcast guard (medium). - docker-compose.yml also adds ORIGIN (H6), app/frontend healthchecks with Caddy waiting on health, and per-service memory limits (medium). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-01 21:25:39 +02:00
parent 5d21b3eefd
commit 498b4e256b
7 changed files with 227 additions and 31 deletions
--- a/.env.example
+++ b/.env.example
@@ -4,6 +4,10 @@ DOMAIN=my-event.example.com

 # ── App server ────────────────────────────────────────────────────────────────
 APP_PORT=3000
+# Set to `production` in real deployments. This activates the secret guard that
+# refuses to boot with placeholder JWT_SECRET / ADMIN_PASSWORD_HASH values.
+# (docker-compose.yml already sets APP_ENV=production for the app service.)
+APP_ENV=production

 # ── Database ──────────────────────────────────────────────────────────────────
 # Set a strong password and keep it in sync between DATABASE_URL and
--- a/backend/src/config.rs
+++ b/backend/src/config.rs
@@ -6,6 +6,47 @@ use anyhow::{anyhow, Context, Result};
 /// we refuse to start with this value; otherwise we warn loudly.
 const DEV_JWT_SECRET_SENTINEL: &str = "dev_secret_do_not_use_in_production_32byteslong_aaaa";

+/// A secret is "placeholder-ish" if it's the shipped dev sentinel or still carries
+/// the tell-tale scaffolding substrings from `.env.example`. Length alone is not
+/// enough — the shipped `change_me_...` placeholder is >32 chars.
+fn looks_placeholder(s: &str) -> bool {
+    let lower = s.to_ascii_lowercase();
+    s == DEV_JWT_SECRET_SENTINEL
+        || lower.contains("change_me")
+        || lower.contains("dev_secret")
+        || lower.contains("placeholder")
+}
+
+/// Enforce secret hygiene. In production every guard is hard-fail: a booting app
+/// with a publicly-known signing key is worse than one that refuses to start.
+/// Outside production the dev sentinel is tolerated (warned) so local dev is frictionless.
+fn validate_secrets(is_prod: bool, jwt_secret: &str, admin_password_hash: &str) -> Result<()> {
+    if is_prod {
+        if looks_placeholder(jwt_secret) {
+            return Err(anyhow!(
+                "Refusing to start in production with a placeholder JWT_SECRET — \
+                 rotate it (openssl rand -hex 64)."
+            ));
+        }
+        if jwt_secret.len() < 32 {
+            return Err(anyhow!("JWT_SECRET must be at least 32 characters."));
+        }
+        if admin_password_hash.is_empty() || looks_placeholder(admin_password_hash) {
+            return Err(anyhow!(
+                "Refusing to start in production without a real ADMIN_PASSWORD_HASH — \
+                 generate one (htpasswd -bnBC 12 '' <password> | tr -d ':\\n')."
+            ));
+        }
+    } else if jwt_secret == DEV_JWT_SECRET_SENTINEL {
+        tracing::warn!(
+            "JWT_SECRET is the dev sentinel — fine for local development, NEVER ship this."
+        );
+    } else if jwt_secret.len() < 32 {
+        return Err(anyhow!("JWT_SECRET must be at least 32 characters."));
+    }
+    Ok(())
+}
+
 #[derive(Clone, Debug)]
 pub struct AppConfig {
    pub database_url: String,
@@ -25,19 +66,9 @@ impl AppConfig {
        let is_prod = app_env.eq_ignore_ascii_case("production");

        let jwt_secret = std::env::var("JWT_SECRET").context("JWT_SECRET must be set")?;
-        if jwt_secret == DEV_JWT_SECRET_SENTINEL {
-            if is_prod {
-                return Err(anyhow!(
-                    "Refusing to start in production with the well-known dev JWT_SECRET — \
-                     rotate it (openssl rand -hex 64)."
-                ));
-            }
-            tracing::warn!(
-                "JWT_SECRET is the dev sentinel — fine for local development, NEVER ship this."
-            );
-        } else if jwt_secret.len() < 32 {
-            return Err(anyhow!("JWT_SECRET must be at least 32 characters."));
-        }
+        let admin_password_hash = std::env::var("ADMIN_PASSWORD_HASH").unwrap_or_default();
+
+        validate_secrets(is_prod, &jwt_secret, &admin_password_hash)?;

        Ok(Self {
            database_url: std::env::var("DATABASE_URL")
@@ -47,8 +78,7 @@ impl AppConfig {
                .unwrap_or_else(|_| "30".to_string())
                .parse()
                .context("SESSION_EXPIRY_DAYS must be a number")?,
-            admin_password_hash: std::env::var("ADMIN_PASSWORD_HASH")
-                .unwrap_or_default(),
+            admin_password_hash,
            event_name: std::env::var("EVENT_NAME")
                .unwrap_or_else(|_| "EventSnap".to_string()),
            event_slug: std::env::var("EVENT_SLUG")
@@ -63,3 +93,46 @@ impl AppConfig {
        })
    }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    const REAL_SECRET: &str = "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6e7f8a9b0c1d2";
+    const REAL_HASH: &str = "$2y$12$abcdefghijklmnopqrstuv.wxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012";
+
+    #[test]
+    fn prod_rejects_shipped_placeholder_secret() {
+        // The exact string shipped in `.env` — >32 chars, so it must be caught by
+        // the substring guard, not the length check.
+        let err = validate_secrets(true, "change_me_to_a_random_64_byte_hex_string", REAL_HASH);
+        assert!(err.is_err(), "placeholder JWT_SECRET must be rejected in prod");
+    }
+
+    #[test]
+    fn prod_rejects_dev_sentinel_and_short_secret() {
+        assert!(validate_secrets(true, DEV_JWT_SECRET_SENTINEL, REAL_HASH).is_err());
+        assert!(validate_secrets(true, "tooshort", REAL_HASH).is_err());
+    }
+
+    #[test]
+    fn prod_rejects_missing_or_placeholder_admin_hash() {
+        assert!(validate_secrets(true, REAL_SECRET, "").is_err());
+        assert!(validate_secrets(true, REAL_SECRET, "$2y$12$placeholder_replace_me").is_err());
+    }
+
+    #[test]
+    fn prod_accepts_real_secrets() {
+        assert!(validate_secrets(true, REAL_SECRET, REAL_HASH).is_ok());
+    }
+
+    #[test]
+    fn non_prod_tolerates_dev_sentinel() {
+        assert!(validate_secrets(false, DEV_JWT_SECRET_SENTINEL, "").is_ok());
+    }
+
+    #[test]
+    fn non_prod_still_rejects_short_non_sentinel_secret() {
+        assert!(validate_secrets(false, "tooshort", "").is_err());
+    }
+}
--- a/backend/src/handlers/host.rs
+++ b/backend/src/handlers/host.rs
@@ -113,10 +113,11 @@ pub async fn ban_user(
    }

    sqlx::query(
-        "UPDATE \"user\" SET is_banned = TRUE, uploads_hidden = $2 WHERE id = $1",
+        "UPDATE \"user\" SET is_banned = TRUE, uploads_hidden = $2 WHERE id = $1 AND event_id = $3",
    )
    .bind(user_id)
    .bind(body.hide_uploads)
+    .bind(auth.event_id)
    .execute(&state.pool)
    .await?;

@@ -263,7 +264,7 @@ pub async fn reset_user_pin(
    sqlx::query(
        "UPDATE \"user\"
         SET recovery_pin_hash = $1,
-             pin_failed_attempts = 0,
+             failed_pin_attempts = 0,
             pin_locked_until = NULL
         WHERE id = $2",
    )
@@ -341,14 +342,18 @@ pub async fn close_event(
    State(state): State<AppState>,
    RequireHost(_auth): RequireHost,
 ) -> Result<StatusCode, AppError> {
-    sqlx::query(
+    let result = sqlx::query(
        "UPDATE event SET uploads_locked_at = NOW() WHERE slug = $1 AND uploads_locked_at IS NULL",
    )
    .bind(&state.config.event_slug)
    .execute(&state.pool)
    .await?;

-    let _ = state.sse_tx.send(SseEvent::new("event-closed", "{}"));
+    // Only broadcast when this call actually flipped the lock — closing an
+    // already-closed event is a no-op and shouldn't spam listeners.
+    if result.rows_affected() > 0 {
+        let _ = state.sse_tx.send(SseEvent::new("event-closed", "{}"));
+    }

    Ok(StatusCode::NO_CONTENT)
 }
@@ -357,14 +362,16 @@ pub async fn open_event(
    State(state): State<AppState>,
    RequireHost(_auth): RequireHost,
 ) -> Result<StatusCode, AppError> {
-    sqlx::query(
-        "UPDATE event SET uploads_locked_at = NULL WHERE slug = $1",
+    let result = sqlx::query(
+        "UPDATE event SET uploads_locked_at = NULL WHERE slug = $1 AND uploads_locked_at IS NOT NULL",
    )
    .bind(&state.config.event_slug)
    .execute(&state.pool)
    .await?;

-    let _ = state.sse_tx.send(SseEvent::new("event-opened", "{}"));
+    if result.rows_affected() > 0 {
+        let _ = state.sse_tx.send(SseEvent::new("event-opened", "{}"));
+    }

    Ok(StatusCode::NO_CONTENT)
 }
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -14,6 +14,10 @@ services:
      interval: 5s
      timeout: 5s
      retries: 10
+    deploy:
+      resources:
+        limits:
+          memory: 512M

  app:
    build:
@@ -21,6 +25,10 @@ services:
      dockerfile: Dockerfile
    restart: unless-stopped
    env_file: .env
+    environment:
+      # Activates the production secret guard in config.rs — refuses to boot with
+      # placeholder JWT_SECRET / ADMIN_PASSWORD_HASH.
+      APP_ENV: production
    depends_on:
      db:
        condition: service_healthy
@@ -28,6 +36,18 @@ services:
      - media_data:/media
    expose:
      - "3000"
+    healthcheck:
+      test: ["CMD-SHELL", "wget -q -O- http://localhost:3000/health || exit 1"]
+      interval: 10s
+      timeout: 5s
+      retries: 5
+      start_period: 20s
+    deploy:
+      resources:
+        limits:
+          # Bounds a runaway ffmpeg transcode (large uploads, 2 workers) so it can't
+          # OOM the single box and take down Postgres.
+          memory: 1G

  frontend:
    build:
@@ -35,10 +55,24 @@ services:
      dockerfile: Dockerfile
    restart: unless-stopped
    env_file: .env
+    environment:
+      # adapter-node behind Caddy TLS needs the public origin for CSRF checks on
+      # POST form actions — without it they fail only in production.
+      ORIGIN: "https://${DOMAIN}"
    depends_on:
      - app
    expose:
      - "3001"
+    healthcheck:
+      test: ["CMD-SHELL", "wget -q -O- http://localhost:3001/ >/dev/null 2>&1 || exit 1"]
+      interval: 10s
+      timeout: 5s
+      retries: 5
+      start_period: 15s
+    deploy:
+      resources:
+        limits:
+          memory: 256M

  caddy:
    image: caddy:2-alpine
@@ -50,8 +84,14 @@ services:
      - ./Caddyfile:/etc/caddy/Caddyfile:ro
      - caddy_data:/data
    depends_on:
-      - app
-      - frontend
+      app:
+        condition: service_healthy
+      frontend:
+        condition: service_healthy
+    deploy:
+      resources:
+        limits:
+          memory: 256M

 volumes:
  postgres_data:
--- a/docs/SECURITY-BACKLOG.md
+++ b/docs/SECURITY-BACKLOG.md
@@ -51,15 +51,25 @@ item below is tagged with its **current status in `main`**:

 ## ✅ Fixed in main since the audit (for the record)

-These audit findings are present in `main` today (verified 2026-06-30): event-scoped social
-handlers (cross-event authz), server-side MIME/ext allowlist on upload, recovery-PIN lockout
-backoff, unspoofable client IP in the rate limiter, effective JWT production-secret guard, DB port
-no longer publicly exposed, container healthchecks, bcrypt offloaded via `spawn_blocking`,
-bounded compression concurrency (semaphore), bounded feed queries (`LIMIT ≤ 100`), and the
-viewport-fit / reduced-motion / aria a11y pass. **New since the audit:** an image-decode
-decompression-bomb cap (`image::Limits` 12000×12000 / 256 MiB) ported into
+These audit findings are present in `main` today (verified 2026-06-30): server-side MIME/ext
+allowlist on upload, recovery-PIN lockout backoff, DB port no longer publicly exposed, bcrypt
+offloaded via `spawn_blocking`, bounded compression concurrency (semaphore), bounded feed queries
+(`LIMIT ≤ 100`), and the viewport-fit / reduced-motion / aria a11y pass. An image-decode
+decompression-bomb cap (`image::Limits` 12000×12000 / 256 MiB) lives in
 `backend/src/services/compression.rs`.

+**Fixed in the 2026-07 review pass** (previously *claimed* fixed here but were not — corrected):
+- **Effective JWT production-secret guard** — `APP_ENV=production` is now set for the app service,
+  and `config.rs::validate_secrets` rejects any placeholder-ish `JWT_SECRET`/`ADMIN_PASSWORD_HASH`
+  (not just the exact dev sentinel) and enforces `len ≥ 32` unconditionally in prod.
+- **Unspoofable client IP in the rate limiter** — `client_ip` now takes the right-most
+  `X-Forwarded-For` entry (the hop Caddy appends), so a client-supplied left-most value is ignored.
+- **Live role/ban re-check** — the auth extractor re-reads the user row and trusts the DB role and
+  `is_banned` flag rather than the JWT claim, revoking demoted/banned sessions immediately.
+- **Container healthchecks** — `app` and `frontend` now have healthchecks and Caddy waits on
+  `service_healthy`.
+- **Event-scoped `ban_user`** — the ban UPDATE is now scoped by `event_id` like its siblings.
+
 ## 🅲 Consciously won't-fix at ~100-guest single-box scale

 Diminishing returns vs. the deployment's actual threat model. Revisit only if the scale or
--- a/e2e/fixtures/api-client.ts
+++ b/e2e/fixtures/api-client.ts
@@ -117,6 +117,25 @@ export class ApiClient {
    });
  }

+  async unbanUser(token: string, userId: string, opts: { expectedStatus?: number | number[] } = {}) {
+    return this.request<void>('POST', `/host/users/${userId}/unban`, {
+      token,
+      expectedStatus: opts.expectedStatus ?? [200, 204],
+    });
+  }
+
+  /** Reset another user's PIN. Returns the plaintext PIN the host must relay once. */
+  async resetUserPin(
+    token: string,
+    userId: string,
+    opts: { expectedStatus?: number | number[] } = {}
+  ): Promise<{ status: number; body: { pin?: string } }> {
+    return this.request<{ pin?: string }>('POST', `/host/users/${userId}/pin-reset`, {
+      token,
+      expectedStatus: opts.expectedStatus ?? [200],
+    });
+  }
+
  async closeEvent(token: string) {
    return this.request<void>('POST', '/host/event/close', { token, expectedStatus: [200, 204] });
  }
--- a/e2e/specs/04-host/pin-reset.spec.ts
+++ b/e2e/specs/04-host/pin-reset.spec.ts
@@ -0,0 +1,43 @@
+/**
+ * Regression for the review's C1: `reset_user_pin` wrote to a non-existent
+ * column (`pin_failed_attempts` vs the real `failed_pin_attempts`), so the
+ * endpoint 500'd every time and never returned a PIN — the feature had never
+ * worked against a real DB and no test caught it. These specs pin the contract:
+ * a successful reset returns a fresh 4-digit PIN, and the target can recover
+ * with it.
+ */
+import { test, expect } from '../../fixtures/test';
+
+test.describe('Host — reset guest PIN (C1)', () => {
+  test('resetting a guest PIN returns a fresh 4-digit PIN', async ({ api, host, guest }) => {
+    const target = await guest('ResetMe');
+
+    const { status, body } = await api.resetUserPin(host.jwt, target.userId);
+
+    expect(status).toBe(200);
+    expect(body.pin).toMatch(/^\d{4}$/);
+  });
+
+  test('the target can recover with the newly reset PIN (and not the old one)', async ({
+    api,
+    host,
+    guest,
+  }) => {
+    const target = await guest('RecoverWithNewPin');
+
+    const { body } = await api.resetUserPin(host.jwt, target.userId);
+    const newPin = body.pin!;
+
+    // New PIN works.
+    await api.recover(target.displayName, newPin, { expectedStatus: [200] });
+
+    // Old PIN no longer works (overwritten). 401 = wrong PIN.
+    if (target.pin !== newPin) {
+      await api.recover(target.displayName, target.pin, { expectedStatus: [401] });
+    }
+  });
+
+  test('a host cannot reset their own PIN via this endpoint', async ({ api, host }) => {
+    await api.resetUserPin(host.jwt, host.userId, { expectedStatus: [400] });
+  });
+});