diff --git a/.env.example b/.env.example index 614e288..f0203d9 100644 --- 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 diff --git a/backend/src/config.rs b/backend/src/config.rs index 88363fc..bcc2d01 100644 --- 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 '' | 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()); + } +} diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index f9ea788..7994c9a 100644 --- 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, RequireHost(_auth): RequireHost, ) -> Result { - 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, RequireHost(_auth): RequireHost, ) -> Result { - 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) } diff --git a/docker-compose.yml b/docker-compose.yml index 6919e38..57f1fa7 100644 --- 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: diff --git a/docs/SECURITY-BACKLOG.md b/docs/SECURITY-BACKLOG.md index f50685b..2282e1e 100644 --- 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 diff --git a/e2e/fixtures/api-client.ts b/e2e/fixtures/api-client.ts index 0bdde22..c86a61a 100644 --- 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('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('POST', '/host/event/close', { token, expectedStatus: [200, 204] }); } diff --git a/e2e/specs/04-host/pin-reset.spec.ts b/e2e/specs/04-host/pin-reset.spec.ts new file mode 100644 index 0000000..8feb3b6 --- /dev/null +++ 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] }); + }); +});