harden: low-severity bucket — unspoofable IP, admin floor, log/SSE hygiene
The cheap, real LOWs from the review (bucket 🅰); 🅱 items and won't-fix
decisions are recorded in docs/SECURITY-BACKLOG.md.
- XFF spoofing (highest-value): client_ip now prefers an unspoofable X-Real-IP
(Caddy `header_up X-Real-IP {remote_host}`) and otherwise takes the *rightmost*
X-Forwarded-For token, never the client-controlled leftmost. The join cap,
recover throttle, and admin-login floor all keyed on this. Unit-tested.
Caddyfile also drops the dead /media/previews|originals cache matchers (the
gateway sets its own Cache-Control) and the false "only host can download"
comment.
- admin_login: add a hard, non-disableable rate floor (30/5min/IP) so the DB
rate-limit master toggle can't remove brute-force protection.
- SSE: the broadcast upload-error payload no longer includes the raw error
(absolute filesystem paths) — generic message to clients, details logged
server-side only.
- Access logs: the request span logs the path only, never the query string, so
the replayable media `?sig=` capability isn't written to logs.
- Reaper TOCTOU: reap_deleted now deletes rows by the ids it actually unlinked
(DELETE … WHERE id = ANY) instead of re-evaluating the time predicate, so a
row crossing the grace line mid-sweep can't be row-deleted without its files
unlinked.
- Dockerfile: run the backend as a non-root user; create /media owned by `app`
so a fresh volume is writable.
- a11y: aria-labels on the upload caption and feed search inputs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -18,10 +18,18 @@ RUN touch src/main.rs && cargo build --release
|
||||
# --- Runtime stage ---
|
||||
FROM alpine:3.21
|
||||
|
||||
RUN apk add --no-cache ca-certificates ffmpeg
|
||||
# Run as an unprivileged user (defense-in-depth). The media volume mounts at
|
||||
# /media; creating it here as `app` means a fresh named volume inherits app
|
||||
# ownership and is writable without running as root. (An existing root-owned
|
||||
# volume from a prior deploy needs a one-time `chown -R app:app` — see deploy notes.)
|
||||
RUN apk add --no-cache ca-certificates ffmpeg \
|
||||
&& addgroup -S app && adduser -S -G app app \
|
||||
&& mkdir -p /media && chown app:app /media
|
||||
|
||||
WORKDIR /app
|
||||
COPY --from=builder /app/target/release/eventsnap-backend ./
|
||||
RUN chown -R app:app /app
|
||||
|
||||
USER app
|
||||
EXPOSE 3000
|
||||
CMD ["./eventsnap-backend"]
|
||||
|
||||
@@ -271,6 +271,22 @@ pub async fn admin_login(
|
||||
// a long-running guess campaign. 5 attempts / minute / IP is plenty for
|
||||
// honest typos.
|
||||
let ip = client_ip(&headers, "unknown");
|
||||
|
||||
// Hard, non-disableable floor: admin_login is the one credential endpoint
|
||||
// whose brute-force protection must survive the DB rate-limit master toggle
|
||||
// being turned off. 30 attempts / 5 min / IP is generous for typos but caps
|
||||
// sustained guessing regardless of config.
|
||||
if !state.rate_limiter.check(
|
||||
format!("admin_login_floor:{ip}"),
|
||||
30,
|
||||
Duration::from_secs(300),
|
||||
) {
|
||||
return Err(AppError::TooManyRequests(
|
||||
"Zu viele Anmeldeversuche. Bitte warte kurz und versuche es erneut.".into(),
|
||||
None,
|
||||
));
|
||||
}
|
||||
|
||||
let rate_limits_on = config::get_bool(&state.pool, "rate_limits_enabled", true).await;
|
||||
let admin_rate_on = config::get_bool(&state.pool, "admin_login_rate_enabled", true).await;
|
||||
if rate_limits_on && admin_rate_on
|
||||
|
||||
@@ -131,11 +131,24 @@ async fn main() -> Result<()> {
|
||||
// there is no raw static mount. The gateway verifies an HMAC signature and
|
||||
// consults the DB (deleted / ban-hidden / type), so private artifacts and
|
||||
// the export archives are never reachable by guessing a path.
|
||||
// Trace spans log the request *path* only, never the query string — signed
|
||||
// media URLs carry a replayable `?sig=` capability that must not land in
|
||||
// access logs.
|
||||
let trace_layer = TraceLayer::new_for_http().make_span_with(
|
||||
|req: &axum::http::Request<axum::body::Body>| {
|
||||
tracing::info_span!(
|
||||
"request",
|
||||
method = %req.method(),
|
||||
path = %req.uri().path(),
|
||||
)
|
||||
},
|
||||
);
|
||||
|
||||
let router = Router::new()
|
||||
.route("/health", get(|| async { "ok" }))
|
||||
.merge(api)
|
||||
.route("/media/{kind}/{id}", get(handlers::media::serve))
|
||||
.layer(TraceLayer::new_for_http())
|
||||
.layer(trace_layer)
|
||||
.with_state(state);
|
||||
|
||||
let listener = tokio::net::TcpListener::bind(("0.0.0.0", config.app_port)).await?;
|
||||
|
||||
@@ -54,10 +54,16 @@ impl CompressionWorker {
|
||||
});
|
||||
}
|
||||
Err(e) => {
|
||||
// Log the detailed error (incl. paths) server-side only; the
|
||||
// SSE channel is broadcast to every client, so send a generic
|
||||
// message — never leak absolute filesystem paths.
|
||||
tracing::error!("compression failed for upload {upload_id}: {e:#}");
|
||||
let _ = worker.sse_tx.send(SseEvent {
|
||||
event_type: "upload-error".to_string(),
|
||||
data: serde_json::json!({ "upload_id": upload_id, "error": e.to_string() }).to_string(),
|
||||
data: serde_json::json!({
|
||||
"upload_id": upload_id,
|
||||
"error": "Verarbeitung fehlgeschlagen."
|
||||
}).to_string(),
|
||||
});
|
||||
let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await;
|
||||
}
|
||||
|
||||
@@ -37,8 +37,13 @@ pub async fn unlink_media(media_path: &Path, paths: &DeletedPaths) {
|
||||
/// DB-row-driven — it never walks the filesystem, so it can never remove a file
|
||||
/// belonging to a live upload.
|
||||
pub async fn reap_deleted(pool: &PgPool, media_path: &Path) {
|
||||
let rows: Vec<DeletedPaths> = match sqlx::query_as::<_, DeletedPaths>(
|
||||
"SELECT original_path AS original, preview_path AS preview, thumbnail_path AS thumbnail
|
||||
// Snapshot the exact rows (id + paths) we are about to unlink, and delete by
|
||||
// those ids — NOT by re-evaluating the `deleted_at < now - 1 day` predicate.
|
||||
// A row that crosses the 1-day line *during* the unlink loop would otherwise
|
||||
// be hard-deleted by the second predicate without ever having its files
|
||||
// unlinked → a permanent orphan.
|
||||
let rows: Vec<(uuid::Uuid, String, Option<String>, Option<String>)> = match sqlx::query_as(
|
||||
"SELECT id, original_path, preview_path, thumbnail_path
|
||||
FROM upload
|
||||
WHERE deleted_at IS NOT NULL AND deleted_at < NOW() - INTERVAL '1 day'",
|
||||
)
|
||||
@@ -56,16 +61,21 @@ pub async fn reap_deleted(pool: &PgPool, media_path: &Path) {
|
||||
return;
|
||||
}
|
||||
|
||||
for paths in &rows {
|
||||
unlink_media(media_path, paths).await;
|
||||
let mut ids = Vec::with_capacity(rows.len());
|
||||
for (id, original, preview, thumbnail) in &rows {
|
||||
unlink_media(media_path, &DeletedPaths {
|
||||
original: original.clone(),
|
||||
preview: preview.clone(),
|
||||
thumbnail: thumbnail.clone(),
|
||||
})
|
||||
.await;
|
||||
ids.push(*id);
|
||||
}
|
||||
|
||||
match sqlx::query(
|
||||
"DELETE FROM upload
|
||||
WHERE deleted_at IS NOT NULL AND deleted_at < NOW() - INTERVAL '1 day'",
|
||||
)
|
||||
.execute(pool)
|
||||
.await
|
||||
match sqlx::query("DELETE FROM upload WHERE id = ANY($1)")
|
||||
.bind(&ids)
|
||||
.execute(pool)
|
||||
.await
|
||||
{
|
||||
Ok(r) => tracing::info!("media reaper: removed {} deleted upload(s)", r.rows_affected()),
|
||||
Err(e) => tracing::warn!(error = ?e, "media reaper: row delete failed"),
|
||||
|
||||
@@ -71,13 +71,55 @@ 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 for rate-limit keys.
|
||||
///
|
||||
/// Prefers `X-Real-IP`, which our reverse proxy (Caddy) overwrites with the real
|
||||
/// TCP peer (`header_up X-Real-IP {remote_host}`) — a client cannot spoof it.
|
||||
/// Falls back to the **rightmost** `X-Forwarded-For` token (the entry the proxy
|
||||
/// appended), never the leftmost: the leftmost is fully client-controlled, and
|
||||
/// trusting it let an attacker rotate the key to bypass the join cap, the
|
||||
/// recover throttle, and the admin-login floor.
|
||||
pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String {
|
||||
if let Some(ip) = headers
|
||||
.get("x-real-ip")
|
||||
.and_then(|v| v.to_str().ok())
|
||||
.map(|s| s.trim())
|
||||
.filter(|s| !s.is_empty())
|
||||
{
|
||||
return ip.to_owned();
|
||||
}
|
||||
headers
|
||||
.get("x-forwarded-for")
|
||||
.and_then(|v| v.to_str().ok())
|
||||
.and_then(|s| s.split(',').next())
|
||||
.and_then(|s| s.split(',').next_back())
|
||||
.map(|s| s.trim().to_owned())
|
||||
.filter(|s| !s.is_empty())
|
||||
.unwrap_or_else(|| fallback.to_owned())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::client_ip;
|
||||
use axum::http::HeaderMap;
|
||||
|
||||
#[test]
|
||||
fn prefers_x_real_ip() {
|
||||
let mut h = HeaderMap::new();
|
||||
h.insert("x-real-ip", "9.9.9.9".parse().unwrap());
|
||||
h.insert("x-forwarded-for", "1.1.1.1, 9.9.9.9".parse().unwrap());
|
||||
assert_eq!(client_ip(&h, "fb"), "9.9.9.9");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn xff_uses_rightmost_not_spoofable_leftmost() {
|
||||
// Attacker prepends a forged entry; the proxy-appended real peer is last.
|
||||
let mut h = HeaderMap::new();
|
||||
h.insert("x-forwarded-for", "1.2.3.4, 203.0.113.7".parse().unwrap());
|
||||
assert_eq!(client_ip(&h, "fb"), "203.0.113.7");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn falls_back_when_no_headers() {
|
||||
assert_eq!(client_ip(&HeaderMap::new(), "fb"), "fb");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user