From 41ac742af81897f62af988be810d06cd81b40b27 Mon Sep 17 00:00:00 2001 From: fabi Date: Thu, 2 Jul 2026 07:14:33 +0200 Subject: [PATCH] test+docs: tighten H1 assertions, cover secret/XFF edges, document boot & SSE-ban MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review follow-ups (non-blocking quality items): Tests: - moderation H1 specs now assert exact `→ 403` instead of bare .rejects.toThrow(), so a spurious 500 can no longer masquerade as "revocation worked". - config: added case-insensitivity (upper/mixed-case placeholder) and the len==32/31 boundary cases for validate_secrets. - rate_limiter: added the trailing-comma empty-entry case for client_ip (must fall back, not return ""). (Backend unit tests: 35 pass.) Docs: - README: note that with APP_ENV=production a placeholder .env makes the app refuse to boot and Caddy wait unhealthy — reason is in `docker compose logs app`. - SECURITY-BACKLOG + sse.rs comment: document that a mid-session ban does not tear down an already-open SSE stream (new tickets are blocked; low blast radius). Co-Authored-By: Claude Opus 4.8 --- README.md | 2 ++ backend/src/config.rs | 19 +++++++++++++++++++ backend/src/handlers/sse.rs | 4 ++++ backend/src/services/rate_limiter.rs | 10 ++++++++++ docs/SECURITY-BACKLOG.md | 6 ++++++ e2e/specs/04-host/moderation.spec.ts | 12 +++++++----- 6 files changed, 48 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a3db1fe..0300098 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,8 @@ docker compose up -d Caddy automatically obtains a Let's Encrypt certificate on first start. The app is live at `https://DOMAIN` within ~30 seconds. +> **If the site never comes up:** with `APP_ENV=production` the backend **refuses to boot** while `JWT_SECRET`/`ADMIN_PASSWORD_HASH` still hold the `.env.example` placeholders (this is deliberate — a publicly-known signing key is worse than downtime). Caddy then waits on the unhealthy `app` container and never serves. Check `docker compose logs app` — a "Refusing to start … placeholder …" line means you skipped step 2. Rotate the secrets (see below) and restart. + > **Production note:** `docker compose up -d` does **not** expose the database — Postgres is reachable only on the internal Docker network. For local development where you need host access to Postgres, opt into the dev overlay explicitly: > ```bash > docker compose -f docker-compose.yml -f docker-compose.dev.yml up diff --git a/backend/src/config.rs b/backend/src/config.rs index bcc2d01..d35e8a7 100644 --- a/backend/src/config.rs +++ b/backend/src/config.rs @@ -135,4 +135,23 @@ mod tests { fn non_prod_still_rejects_short_non_sentinel_secret() { assert!(validate_secrets(false, "tooshort", "").is_err()); } + + #[test] + fn placeholder_detection_is_case_insensitive() { + // looks_placeholder lowercases before matching — an upper/mixed-case + // placeholder must still be rejected in prod. + assert!(validate_secrets(true, "CHANGE_ME_TO_A_RANDOM_64_BYTE_HEX_STRING", REAL_HASH).is_err()); + assert!(validate_secrets(true, REAL_SECRET, "$2Y$12$PLACEHOLDER_replace_me").is_err()); + } + + #[test] + fn prod_len_boundary_at_32() { + // Exactly 32 non-placeholder chars is the minimum accepted; 31 is rejected. + const LEN_32: &str = "abcdefghijklmnopqrstuvwxyz012345"; + const LEN_31: &str = "abcdefghijklmnopqrstuvwxyz01234"; + assert_eq!(LEN_32.len(), 32); + assert_eq!(LEN_31.len(), 31); + assert!(validate_secrets(true, LEN_32, REAL_HASH).is_ok()); + assert!(validate_secrets(true, LEN_31, REAL_HASH).is_err()); + } } diff --git a/backend/src/handlers/sse.rs b/backend/src/handlers/sse.rs index d4a0971..ed63d05 100644 --- a/backend/src/handlers/sse.rs +++ b/backend/src/handlers/sse.rs @@ -48,6 +48,10 @@ pub async fn stream( .consume(&q.ticket) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; + // NOTE: this authenticates via ticket→session, not the `AuthUser` extractor, so + // it does not re-check `is_banned`. A user banned mid-stream keeps receiving + // events until the connection drops; they cannot reconnect (issue_ticket uses + // AuthUser, which rejects banned users). Documented in docs/SECURITY-BACKLOG.md. Session::find_by_token_hash(&state.pool, &token_hash) .await .map_err(|e| AppError::Internal(e.into()))? diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index cb9a041..91f0d73 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -167,4 +167,14 @@ mod tests { fn client_ip_falls_back_when_header_absent() { assert_eq!(client_ip(&HeaderMap::new(), "127.0.0.1"), "127.0.0.1"); } + + #[test] + fn client_ip_falls_back_on_trailing_comma_empty_entry() { + // A trailing comma leaves an empty right-most segment after trimming; the + // `.filter(!is_empty)` must reject it and fall through to the fallback + // rather than returning "" (which would collapse callers into one bucket). + let mut h = HeaderMap::new(); + h.insert("x-forwarded-for", "203.0.113.7, ".parse().unwrap()); + assert_eq!(client_ip(&h, "127.0.0.1"), "127.0.0.1"); + } } diff --git a/docs/SECURITY-BACKLOG.md b/docs/SECURITY-BACKLOG.md index 2282e1e..cbb8ab4 100644 --- a/docs/SECURITY-BACKLOG.md +++ b/docs/SECURITY-BACKLOG.md @@ -81,6 +81,12 @@ tenancy model changes. - Performance micro-indexes (`idx_like_user_upload`, comment pagination index) — current queries are sub-ms at this row count. - Optimistic-like in-flight guard, ownership-snapshot-at-mount, assorted copy tweaks — UX polish. +- **Mid-session ban does not tear down an already-open SSE stream.** The live-ban check lives in the + `AuthUser` extractor, but the stream authenticates via ticket→session (`handlers/sse.rs::stream`), + so a user banned while connected keeps receiving broadcast events until their stream drops. New + tickets *are* blocked (`issue_ticket` uses `AuthUser`), so they cannot reconnect. Low blast radius + (read-only feed events, no re-subscribe); tearing live streams down would need a per-session + broadcast filter. Revisit only if bans must take effect within seconds. ## By-design notes (audit branch's signed-media model — see DECISION-media-auth.md) diff --git a/e2e/specs/04-host/moderation.spec.ts b/e2e/specs/04-host/moderation.spec.ts index 153160b..13e0f74 100644 --- a/e2e/specs/04-host/moderation.spec.ts +++ b/e2e/specs/04-host/moderation.spec.ts @@ -71,8 +71,10 @@ test.describe('Host — live role/ban revocation (H1)', () => { // Demote via admin — the host does NOT re-login. await api.setRole(adminToken, u.userId, 'guest'); - // Same token is now rejected because the middleware trusts the DB role. - await expect(api.listUsers(hostJwt)).rejects.toThrow(); + // Same token is now rejected with exactly 403 (RequireHost sees the DB role, + // not the JWT claim). Asserting the status guards against a spurious 500 + // masquerading as "revoked". + await expect(api.listUsers(hostJwt)).rejects.toThrow(/→ 403/); }); test('a banned host is locked out immediately on their existing session', async ({ @@ -85,14 +87,14 @@ test.describe('Host — live role/ban revocation (H1)', () => { await api.banUser(adminToken, host.userId, false); - // Banned users are rejected by the auth extractor before any handler runs. - await expect(api.listUsers(host.jwt)).rejects.toThrow(); + // Banned users are rejected with 403 by the auth extractor before any handler runs. + await expect(api.listUsers(host.jwt)).rejects.toThrow(/→ 403/); }); test('a banned host cannot unban themselves', async ({ api, adminToken, host }) => { await api.banUser(adminToken, host.userId, false); await expect( api.unbanUser(host.jwt, host.userId, { expectedStatus: [204] }) - ).rejects.toThrow(); + ).rejects.toThrow(/→ 403/); }); });