fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(security): high — live authz, XFF, SSE buffering, atomicity, pagination

Browse Source 
H1: auth extractor re-reads the live user row — trusts the DB role (not the
    JWT claim) and rejects banned users mid-session. e2e proves a demoted
    host loses powers and a banned host is locked out and can't self-unban.
H2: client_ip takes the right-most XFF hop (the one Caddy appends); spoofed
    left-most entries are ignored, restoring IP-based throttles.
H3: Caddy excludes /api/v1/stream from `encode` so SSE isn't buffered.
H4: upload — quota increment + row insert + hashtag links now one txn.
H5: feed keyset pagination tiebroken on (created_at, id) + composite index
    (migration 010); feed_delta bounded with LIMIT.
H7: LightboxModal keys its comment load off upload.id, ending the
    SSE-driven refetch storm.
H8: CSP via SvelteKit kit.csp (svelte.config.js) hardens the localStorage
    token model against injection.

Migration 010 also adds the latent comment/comment_hashtag indexes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
fabi
2026-07-01 21:25:50 +02:00
parent 498b4e256b
commit f08d858281
12 changed files with 248 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
Caddyfile
View File

@@ -1,5 +1,8 @@
{$DOMAIN} {
encode zstd gzip
# Compress everything EXCEPT the SSE stream — gzip buffering delays
# "real-time" likes/comments until the ~30s keep-alive tick.
@compressible not path /api/v1/stream
encode @compressible zstd gzip
# Site-wide security headers (defense-in-depth). HSTS is free since Caddy
# already terminates TLS. nosniff also covers all of /media/*.

3
backend/migrations/010_review_indexes.down.sql Normal file
View File

@@ -0,0 +1,3 @@
DROP INDEX IF EXISTS idx_comment_hashtag_hashtag;
DROP INDEX IF EXISTS idx_comment_user;
DROP INDEX IF EXISTS idx_upload_event_created_id;

17
backend/migrations/010_review_indexes.up.sql Normal file
View File

@@ -0,0 +1,17 @@
-- Composite feed index with an id tiebreaker so keyset pagination
-- (ORDER BY created_at DESC, id DESC) stays index-covered and stable when
-- multiple uploads share a created_at timestamp.
CREATE INDEX idx_upload_event_created_id
ON upload(event_id, created_at DESC, id DESC)
WHERE deleted_at IS NULL;
-- A user's own comments (moderation, "who commented"). The sibling
-- idx_upload_user already exists for uploads; comment(user_id) was missing.
CREATE INDEX idx_comment_user
ON comment(user_id)
WHERE deleted_at IS NULL;
-- Hashtag filtering over comments — mirrors idx_upload_hashtag_hashtag, which
-- only covered upload_hashtag.
CREATE INDEX idx_comment_hashtag_hashtag
ON comment_hashtag(hashtag_id);

20
backend/src/auth/middleware.rs
View File

@@ -1,4 +1,4 @@
use axum::extract::{FromRequestParts, State};
use axum::extract::FromRequestParts;
use axum::http::request::Parts;
use uuid::Uuid;
@@ -43,6 +43,18 @@ impl FromRequestParts<AppState> for AuthUser {
.map_err(|e| AppError::Internal(e.into()))?
.ok_or_else(|| AppError::Unauthorized("Sitzung nicht gefunden oder abgelaufen.".into()))?;
// Trust *live* DB state, not the JWT: a role/ban stored in the token would
// survive a demote/ban for the full session lifetime (up to 30d). Re-read
// the user row so a demoted host loses host powers and a banned user is
// locked out immediately on their next request.
let user = crate::models::user::User::find_by_id(&state.pool, claims.sub)
.await
.map_err(|e| AppError::Internal(e.into()))?
.ok_or_else(|| AppError::Unauthorized("Benutzer nicht gefunden.".into()))?;
if user.is_banned {
return Err(AppError::Forbidden("Dein Zugang wurde gesperrt.".into()));
}
// Update last_seen_at in the background (fire-and-forget). Failures are
// non-fatal but worth surfacing — silent swallowing hides DB connection
// pressure that would otherwise be the first symptom of a real problem.
@@ -55,9 +67,9 @@ impl FromRequestParts<AppState> for AuthUser {
});
Ok(Self {
user_id: claims.sub,
event_id: claims.event_id,
role: claims.role,
user_id: user.id,
event_id: user.event_id,
role: user.role,
token_hash,
})
}

60
backend/src/handlers/feed.rs
View File

@@ -79,6 +79,17 @@ pub async fn feed(
let limit = q.limit.unwrap_or(20).min(100);
// Resolve the cursor to a (created_at, id) position. The pair is compared as a
// tuple so ties on created_at break on id — keyset pagination on created_at
// alone would silently drop rows sharing a timestamp across a page boundary.
let (cursor_time, cursor_id) = match q.cursor {
Some(c) => match get_cursor_pos(&state.pool, c).await {
Some((t, id)) => (Some(t), Some(id)),
None => (None, None),
},
None => (None, None),
};
let rows = if let Some(hashtag) = &q.hashtag {
let tag = hashtag.trim().trim_start_matches('#').to_lowercase();
sqlx::query_as::<_, FeedRow>(
@@ -88,19 +99,14 @@ pub async fn feed(
JOIN upload_hashtag uh ON uh.upload_id = v.id
JOIN hashtag h ON h.id = uh.hashtag_id AND h.tag = $1
WHERE v.event_id = $2
AND ($3::timestamptz IS NULL OR v.created_at < $3)
ORDER BY v.created_at DESC
LIMIT $4",
AND ($3::timestamptz IS NULL OR (v.created_at, v.id) < ($3, $4))
ORDER BY v.created_at DESC, v.id DESC
LIMIT $5",
)
.bind(&tag)
.bind(auth.event_id)
.bind(
if let Some(cursor) = q.cursor {
get_cursor_time(&state.pool, cursor).await
} else {
None
},
)
.bind(cursor_time)
.bind(cursor_id)
.bind(limit + 1)
.fetch_all(&state.pool)
.await?
@@ -110,18 +116,13 @@ pub async fn feed(
mime_type, caption, like_count, comment_count, created_at
FROM v_feed
WHERE event_id = $1
AND ($2::timestamptz IS NULL OR created_at < $2)
ORDER BY created_at DESC
LIMIT $3",
AND ($2::timestamptz IS NULL OR (created_at, id) < ($2, $3))
ORDER BY created_at DESC, id DESC
LIMIT $4",
)
.bind(auth.event_id)
.bind(
if let Some(cursor) = q.cursor {
get_cursor_time(&state.pool, cursor).await
} else {
None
},
)
.bind(cursor_time)
.bind(cursor_id)
.bind(limit + 1)
.fetch_all(&state.pool)
.await?
@@ -178,15 +179,21 @@ pub async fn feed_delta(
auth: AuthUser,
Query(q): Query<DeltaQuery>,
) -> Result<Json<DeltaResponse>, AppError> {
// Bounded like the paginated feed: a stale `since` could otherwise pull the
// entire event's uploads in one response. If a client hits the cap it should
// fall back to a full feed refresh rather than another delta.
const DELTA_LIMIT: i64 = 200;
let rows = sqlx::query_as::<_, FeedRow>(
"SELECT id, user_id, uploader_name, preview_path, thumbnail_path,
mime_type, caption, like_count, comment_count, created_at
FROM v_feed
WHERE event_id = $1 AND created_at > $2
ORDER BY created_at DESC",
ORDER BY created_at DESC, id DESC
LIMIT $3",
)
.bind(auth.event_id)
.bind(q.since)
.bind(DELTA_LIMIT)
.fetch_all(&state.pool)
.await?;
@@ -249,14 +256,17 @@ pub async fn hashtags(
))
}
async fn get_cursor_time(pool: &sqlx::PgPool, cursor_id: Uuid) -> Option<DateTime<Utc>> {
let row: Option<(DateTime<Utc>,)> =
sqlx::query_as("SELECT created_at FROM upload WHERE id = $1")
/// Resolve a cursor id to its `(created_at, id)` position. Both are needed:
/// `created_at` alone isn't unique, so pagination must break ties on `id` to
/// avoid silently dropping rows that share a timestamp across a page boundary.
async fn get_cursor_pos(pool: &sqlx::PgPool, cursor_id: Uuid) -> Option<(DateTime<Utc>, Uuid)> {
let row: Option<(DateTime<Utc>, Uuid)> =
sqlx::query_as("SELECT created_at, id FROM upload WHERE id = $1")
.bind(cursor_id)
.fetch_optional(pool)
.await
.ok()?;
row.map(|r| r.0)
row
}
async fn get_liked_set(

56
backend/src/handlers/upload.rs
View File

@@ -190,25 +190,6 @@ pub async fn upload(
}
tokio::fs::write(&absolute_path, &data).await.map_err(|e| AppError::Internal(e.into()))?;
// Update user's total upload bytes
sqlx::query("UPDATE \"user\" SET total_upload_bytes = total_upload_bytes + $2 WHERE id = $1")
.bind(auth.user_id)
.bind(size)
.execute(&state.pool)
.await?;
// Insert upload record
let upload = Upload::create(
&state.pool,
auth.event_id,
auth.user_id,
&relative_path,
&mime,
size,
caption.as_deref(),
)
.await?;
// Process hashtags from caption and explicit CSV
let mut tags: Vec<String> = Vec::new();
if let Some(ref cap) = caption {
@@ -225,10 +206,30 @@ pub async fn upload(
tags.sort();
tags.dedup();
// Quota accounting, the upload row, and its hashtag links must be atomic: a
// crash between the bytes increment and the insert would permanently charge
// bytes with no row to reclaim them (silent quota erosion / spurious lockout).
let mut tx = state.pool.begin().await?;
sqlx::query("UPDATE \"user\" SET total_upload_bytes = total_upload_bytes + $2 WHERE id = $1")
.bind(auth.user_id)
.bind(size)
.execute(&mut *tx)
.await?;
let upload = Upload::create(
&mut *tx,
auth.event_id,
auth.user_id,
&relative_path,
&mime,
size,
caption.as_deref(),
)
.await?;
for tag in &tags {
let h = Hashtag::upsert(&state.pool, auth.event_id, tag).await?;
Hashtag::link_to_upload(&state.pool, upload.id, h.id).await?;
let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?;
Hashtag::link_to_upload(&mut *tx, upload.id, h.id).await?;
}
tx.commit().await?;
// Spawn compression task
state
@@ -279,17 +280,20 @@ pub async fn edit_upload(
return Err(AppError::Forbidden("Nur eigene Uploads bearbeiten.".into()));
}
// Caption update + hashtag wipe-then-relink in one transaction, so a crash
// mid-relink can't leave the upload with its hashtags stripped.
let mut tx = state.pool.begin().await?;
if let Some(ref caption) = body.caption {
Upload::update_caption(&state.pool, upload_id, Some(caption)).await?;
Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?;
}
if let Some(ref hashtags) = body.hashtags {
Hashtag::unlink_all_from_upload(&state.pool, upload_id).await?;
Hashtag::unlink_all_from_upload(&mut *tx, upload_id).await?;
for tag in hashtags {
let h = Hashtag::upsert(&state.pool, auth.event_id, tag).await?;
Hashtag::link_to_upload(&state.pool, upload_id, h.id).await?;
let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?;
Hashtag::link_to_upload(&mut *tx, upload_id, h.id).await?;
}
}
tx.commit().await?;
Ok(StatusCode::OK)
}

32
backend/src/models/hashtag.rs
View File

@@ -10,7 +10,13 @@ pub struct Hashtag {
impl Hashtag {
/// Upsert a hashtag (insert if not exists, return existing if it does).
pub async fn upsert(pool: &PgPool, event_id: Uuid, tag: &str) -> Result<Self, sqlx::Error> {
///
/// Takes any executor so callers can run it inside a transaction (atomic
/// upload/comment writes) or standalone against the pool.
pub async fn upsert<'e, E>(executor: E, event_id: Uuid, tag: &str) -> Result<Self, sqlx::Error>
where
E: sqlx::PgExecutor<'e>,
{
let normalized = tag.trim().trim_start_matches('#').to_lowercase();
sqlx::query_as::<_, Self>(
"INSERT INTO hashtag (event_id, tag) VALUES ($1, $2)
@@ -19,33 +25,39 @@ impl Hashtag {
)
.bind(event_id)
.bind(&normalized)
.fetch_one(pool)
.fetch_one(executor)
.await
}
pub async fn link_to_upload(
pool: &PgPool,
pub async fn link_to_upload<'e, E>(
executor: E,
upload_id: Uuid,
hashtag_id: Uuid,
) -> Result<(), sqlx::Error> {
) -> Result<(), sqlx::Error>
where
E: sqlx::PgExecutor<'e>,
{
sqlx::query(
"INSERT INTO upload_hashtag (upload_id, hashtag_id) VALUES ($1, $2)
ON CONFLICT DO NOTHING",
)
.bind(upload_id)
.bind(hashtag_id)
.execute(pool)
.execute(executor)
.await?;
Ok(())
}
pub async fn unlink_all_from_upload(
pool: &PgPool,
pub async fn unlink_all_from_upload<'e, E>(
executor: E,
upload_id: Uuid,
) -> Result<(), sqlx::Error> {
) -> Result<(), sqlx::Error>
where
E: sqlx::PgExecutor<'e>,
{
sqlx::query("DELETE FROM upload_hashtag WHERE upload_id = $1")
.bind(upload_id)
.execute(pool)
.execute(executor)
.await?;
Ok(())
}

24
backend/src/models/upload.rs
View File

@@ -36,15 +36,20 @@ pub struct UploadDto {
}
impl Upload {
pub async fn create(
pool: &PgPool,
/// Takes any executor so the caller can run it inside a transaction (atomic
/// quota + insert) or standalone against the pool.
pub async fn create<'e, E>(
executor: E,
event_id: Uuid,
user_id: Uuid,
original_path: &str,
mime_type: &str,
original_size_bytes: i64,
caption: Option<&str>,
) -> Result<Self, sqlx::Error> {
) -> Result<Self, sqlx::Error>
where
E: sqlx::PgExecutor<'e>,
{
sqlx::query_as::<_, Self>(
"INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption)
VALUES ($1, $2, $3, $4, $5, $6)
@@ -56,7 +61,7 @@ impl Upload {
.bind(mime_type)
.bind(original_size_bytes)
.bind(caption)
.fetch_one(pool)
.fetch_one(executor)
.await
}
@@ -182,15 +187,18 @@ impl Upload {
Ok(deleted)
}
pub async fn update_caption(
pool: &PgPool,
pub async fn update_caption<'e, E>(
executor: E,
id: Uuid,
caption: Option<&str>,
) -> Result<(), sqlx::Error> {
) -> Result<(), sqlx::Error>
where
E: sqlx::PgExecutor<'e>,
{
sqlx::query("UPDATE upload SET caption = $2 WHERE id = $1")
.bind(id)
.bind(caption)
.execute(pool)
.execute(executor)
.await?;
Ok(())
}

26
backend/src/services/rate_limiter.rs
View File

@@ -71,14 +71,21 @@ impl RateLimiter {
}
}
/// Extract the client IP from X-Forwarded-For (Caddy sets this) or fall back
/// to a provided socket address string.
/// Extract the client IP from X-Forwarded-For or fall back to a provided socket
/// address string.
///
/// We take the **right-most** entry, not the left-most. Caddy is the sole ingress
/// and the app port is only `expose`d (never published), so the last hop Caddy
/// appends is the real client. A client can prepend arbitrary spoofed values to
/// the left of XFF to dodge throttles — those are ignored here. This assumes
/// exactly one trusted proxy (Caddy); revisit if that changes.
pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String {
headers
.get("x-forwarded-for")
.and_then(|v| v.to_str().ok())
.and_then(|s| s.split(',').next())
.and_then(|s| s.rsplit(',').next())
.map(|s| s.trim().to_owned())
.filter(|s| !s.is_empty())
.unwrap_or_else(|| fallback.to_owned())
}
@@ -134,9 +141,18 @@ mod tests {
}
#[test]
fn client_ip_prefers_first_forwarded_for_entry() {
fn client_ip_takes_rightmost_forwarded_for_entry() {
// The right-most entry is the hop our trusted proxy (Caddy) appended.
let mut h = HeaderMap::new();
h.insert("x-forwarded-for", "203.0.113.7, 10.0.0.1".parse().unwrap());
h.insert("x-forwarded-for", "10.0.0.1, 203.0.113.7".parse().unwrap());
assert_eq!(client_ip(&h, "fallback"), "203.0.113.7");
}
#[test]
fn client_ip_ignores_spoofed_leftmost_entry() {
// A client prepending a fake IP to dodge throttles must not win.
let mut h = HeaderMap::new();
h.insert("x-forwarded-for", "1.2.3.4, 9.9.9.9, 203.0.113.7".parse().unwrap());
assert_eq!(client_ip(&h, "fallback"), "203.0.113.7");
}

51
e2e/specs/04-host/moderation.spec.ts
View File

@@ -45,3 +45,54 @@ test.describe('Host — moderation API', () => {
expect(row?.role).toBe('host');
});
});
/**
* Regression for the review's H1: role & ban used to be trusted from the JWT and
* never re-checked against the DB, so a demoted/banned host kept full powers for
* the life of their token (up to 30d) and a banned host could even unban
* themselves. The auth extractor now re-reads the live user row, so these take
* effect on the *existing* session with no re-login.
*/
test.describe('Host — live role/ban revocation (H1)', () => {
test('a demoted host loses host powers on their existing session', async ({
api,
adminToken,
guest,
}) => {
const u = await guest('DemoteMidSession');
await api.setRole(adminToken, u.userId, 'host');
// Fresh host token (role is encoded at mint time).
const { body } = await api.recover(u.displayName, u.pin);
const hostJwt = body.jwt;
// Sanity: the token currently has host powers.
await api.listUsers(hostJwt);
// 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();
});
test('a banned host is locked out immediately on their existing session', async ({
api,
adminToken,
host,
}) => {
// Sanity: host token works.
await api.listUsers(host.jwt);
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();
});
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();
});
});

14
frontend/src/lib/components/LightboxModal.svelte
View File

@@ -7,6 +7,7 @@
import { doubletap } from '$lib/actions/doubletap';
import { focusTrap } from '$lib/actions/focus-trap';
import { scrollLock } from '$lib/actions/scroll-lock';
import { modalInert } from '$lib/actions/modal-inert';
import { toastError } from '$lib/toast-store';
import { vibrate } from '$lib/haptics';
import HeartBurst from './HeartBurst.svelte';
@@ -51,13 +52,19 @@
if (burstTimer) clearTimeout(burstTimer);
});
// Only refetch when a *different* upload is shown. The feed reassigns the
// `upload` prop object on every SSE like/comment count update; keying the
// effect off the memoized id avoids a refetch storm that would also clobber
// a just-posted optimistic comment.
const uploadId = $derived(upload.id);
$effect(() => {
loadComments();
loadComments(uploadId);
});
async function loadComments() {
async function loadComments(id: string) {
try {
comments = await api.get<CommentDto[]>(`/upload/${upload.id}/comments`);
comments = await api.get<CommentDto[]>(`/upload/${id}/comments`);
} catch {
// Background fetch — failure leaves the panel empty; reopening the lightbox retries.
}
@@ -106,6 +113,7 @@
aria-labelledby="lightbox-title"
use:focusTrap={{ onclose }}
use:scrollLock
use:modalInert
>
<div class="flex max-h-[90vh] w-full max-w-2xl flex-col overflow-hidden rounded-xl bg-white dark:bg-gray-900">
<!-- Media -->

23
frontend/svelte.config.js
View File

@@ -6,7 +6,28 @@ const config = {
runes: true
},
kit: {
adapter: adapter()
adapter: adapter(),
// Content-Security-Policy — the highest-value header for this UGC app and the
// cheapest hardening of the localStorage-based auth (the whole model rests on
// never having an XSS). `mode: 'auto'` lets SvelteKit nonce/hash its own inline
// hydration script, so script-src stays free of 'unsafe-inline'.
csp: {
mode: 'auto',
directives: {
'default-src': ['self'],
'script-src': ['self'],
// Svelte emits inline style attributes (style: directives); allow them.
'style-src': ['self', 'unsafe-inline'],
'img-src': ['self', 'data:', 'blob:'],
'media-src': ['self', 'blob:'],
'font-src': ['self'],
'connect-src': ['self'],
'object-src': ['none'],
'base-uri': ['self'],
'form-action': ['self'],
'frame-ancestors': ['none']
}
}
}
};