fix(export): charge the download limit where the client can see the answer
The keepsake download is an iframe navigation, so its response is invisible to the page. The rate limit was enforced inside the zip/html handler — i.e. inside that navigation — while the ticket POST in front of it always returned 200. A guest over the limit therefore tapped "Herunterladen" and absolutely nothing happened, forever, with no explanation, on the one screen that is the emotional payoff of the whole app. With the default of 3/day, ZIP + HTML costs 2 and one retry locks them out until tomorrow. Minting is a normal `fetch`, so the limit moves there and the 429 reaches the user. The limit is not weakened: tickets are single-use with a 30s TTL and can only be obtained from that authenticated endpoint, so one mint is at most one download — and charging it in both places would have cost every download two slots. The message named the wrong timescale too. It shared the generic "warte kurz" wording with the per-minute limiters, but this bucket is a DAY, so a guest was told to wait a moment for something that could not work again until tomorrow. Verified live: three mints succeed, the fourth returns 429 in German; raising `export_rate_per_day` through the admin API takes effect on the next request with no restart, and the HTML keepsake then downloads. Also here, from the same pass: - `looks_bcrypt` checks the SHAPE of ADMIN_PASSWORD_HASH, not just placeholder-ness. A hash corrupted by shell or Compose escaping is not a placeholder, so the app booted green, `/health` said ok, and every admin login 401'd — unrecoverable mid-event, because the Admin row is only created BY a successful admin login and promoting a host requires one. - The rate limiter indexed `timestamps[0]` while holding its mutex, so a `max == 0` configuration panicked and poisoned the lock process-wide. Uses `first()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -17,6 +17,30 @@ fn looks_placeholder(s: &str) -> bool {
|
||||
|| lower.contains("placeholder")
|
||||
}
|
||||
|
||||
/// A bcrypt hash is exactly 60 characters and opens with `$2<variant>$<cost>$`.
|
||||
///
|
||||
/// Checking the SHAPE, not just placeholder-ness, is what catches a hash silently mangled in
|
||||
/// transit. The `$` segments are variable-expansion bait for both shell quoting and Docker
|
||||
/// Compose's `env_file` parsing, and a mangled hash is not a placeholder — so without this it
|
||||
/// passes every other guard here, the app boots green, `/health` returns `ok`, and every admin
|
||||
/// login 401s.
|
||||
///
|
||||
/// That failure is unrecoverable mid-event, which is why it is worth a hard fail at boot: the
|
||||
/// Admin row is created BY a successful admin login (`auth/handlers.rs`), and only an Admin or
|
||||
/// Host can promote a Host. No admin login therefore means no host at all — the event cannot be
|
||||
/// closed, the gallery cannot be released, and nothing can be moderated.
|
||||
fn looks_bcrypt(s: &str) -> bool {
|
||||
let b = s.as_bytes();
|
||||
s.len() == 60
|
||||
&& b[0] == b'$'
|
||||
&& b[1] == b'2'
|
||||
&& matches!(b[2], b'a' | b'b' | b'x' | b'y')
|
||||
&& b[3] == b'$'
|
||||
&& b[4].is_ascii_digit()
|
||||
&& b[5].is_ascii_digit()
|
||||
&& b[6] == b'$'
|
||||
}
|
||||
|
||||
/// 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.
|
||||
@@ -46,6 +70,18 @@ fn validate_secrets(
|
||||
"ADMIN_PASSWORD_HASH is unset or still the .env.example placeholder — generate one \
|
||||
(docker run --rm caddy:2-alpine caddy hash-password --plaintext '<password>').",
|
||||
);
|
||||
} else if !looks_bcrypt(admin_password_hash) {
|
||||
problems.push(
|
||||
"ADMIN_PASSWORD_HASH is not a well-formed bcrypt hash: expected exactly 60 \
|
||||
characters starting `$2b$12$…`. First look at the value that actually reached \
|
||||
the app — `docker compose exec app printenv ADMIN_PASSWORD_HASH` — and compare \
|
||||
it to .env character for character. In .env, SINGLE-QUOTE the hash \
|
||||
('$2b$12$…'): Compose uses single-quoted env_file values literally, so the `$` \
|
||||
segments survive. Double them to `$$` ONLY when setting the value under \
|
||||
`environment:` in docker-compose.yml — doing that in .env corrupts a hash that \
|
||||
would otherwise have worked. Regenerate with: \
|
||||
docker run --rm caddy:2-alpine caddy hash-password --plaintext '<password>'",
|
||||
);
|
||||
}
|
||||
// The DATABASE_URL carries the Postgres password, so a placeholder here means the stack is
|
||||
// running on `CHANGE_ME_use_a_strong_password` — a credential published in the repo. The
|
||||
@@ -173,7 +209,10 @@ mod tests {
|
||||
use super::*;
|
||||
|
||||
const REAL_SECRET: &str = "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6e7f8a9b0c1d2";
|
||||
const REAL_HASH: &str = "$2y$12$abcdefghijklmnopqrstuv.wxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012";
|
||||
// A structurally valid bcrypt hash: exactly 60 chars, `$2y$12$` + 53 of salt/digest.
|
||||
// The shape matters — `looks_bcrypt` enforces it, so a fixture of the wrong length
|
||||
// would assert the opposite of what these tests claim.
|
||||
const REAL_HASH: &str = "$2y$12$abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY01";
|
||||
const REAL_DB_URL: &str = "postgres://eventsnap:7f3a9c1e5b2d8a4f@db:5432/eventsnap";
|
||||
|
||||
#[test]
|
||||
@@ -198,6 +237,31 @@ mod tests {
|
||||
assert!(validate_secrets(true, "tooshort", REAL_HASH, REAL_DB_URL).is_err());
|
||||
}
|
||||
|
||||
/// The failure this guards is silent and unrecoverable mid-event: a hash whose `$`
|
||||
/// segments were eaten by shell or Compose interpolation is NOT a placeholder, so every
|
||||
/// other guard passes, the app boots green and `/health` reports ok — and then every
|
||||
/// admin login 401s, which (because the Admin row is created by a successful login, and
|
||||
/// only an Admin/Host can promote a Host) means no host exists for the whole event.
|
||||
#[test]
|
||||
fn prod_rejects_mangled_admin_hash() {
|
||||
// What `$2y$12$…` degrades to once `$2y`/`$12` are read as unset variables.
|
||||
assert!(validate_secrets(true, REAL_SECRET, "abcdefghijklmnop", REAL_DB_URL).is_err());
|
||||
// Right prefix, truncated body — still not a usable hash.
|
||||
assert!(validate_secrets(true, REAL_SECRET, "$2y$12$tooshort", REAL_DB_URL).is_err());
|
||||
// Correct length but no bcrypt prefix at all.
|
||||
assert!(validate_secrets(true, REAL_SECRET, &"x".repeat(60), REAL_DB_URL).is_err());
|
||||
// All three shipped bcrypt variants stay acceptable.
|
||||
let body = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY01";
|
||||
for variant in ["2a", "2b", "2y"] {
|
||||
let hash = format!("${variant}$12${body}");
|
||||
assert_eq!(hash.len(), 60);
|
||||
assert!(
|
||||
validate_secrets(true, REAL_SECRET, &hash, REAL_DB_URL).is_ok(),
|
||||
"bcrypt variant ${variant}$ must be accepted"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prod_rejects_missing_or_placeholder_admin_hash() {
|
||||
assert!(validate_secrets(true, REAL_SECRET, "", REAL_DB_URL).is_err());
|
||||
|
||||
@@ -338,13 +338,25 @@ pub struct DownloadQuery {
|
||||
pub async fn export_ticket(
|
||||
State(state): State<AppState>,
|
||||
auth: crate::auth::middleware::AuthUser,
|
||||
) -> Json<serde_json::Value> {
|
||||
) -> Result<Json<serde_json::Value>, AppError> {
|
||||
// NOTE: intentionally NOT gated on `is_banned`. A banned user keeps *read* access
|
||||
// by design (USER_JOURNEYS §10.3, FEATURES: "Can still download the export once
|
||||
// released — Spec design choice"). The export is read-only, so it stays available
|
||||
// to them, consistent with the read-only-ban model.
|
||||
|
||||
// The rate limit is enforced HERE rather than on the download itself, and that placement is
|
||||
// the whole point: the download is an iframe navigation, so its response is invisible to the
|
||||
// page. Limiting it there meant a guest over the limit tapped "Herunterladen", the ticket
|
||||
// POST returned 200, the iframe silently received a 429, and absolutely nothing happened —
|
||||
// forever, with no explanation, on the one screen that is the emotional payoff of the app.
|
||||
// Minting is a normal `fetch`, so a 429 here reaches the user as a German message.
|
||||
//
|
||||
// Moving it does not weaken the limit: tickets are single-use with a 30s TTL and can only be
|
||||
// obtained from this authenticated endpoint, so one mint is at most one download.
|
||||
enforce_export_rate(&state, auth.user_id).await?;
|
||||
|
||||
let ticket = state.sse_tickets.issue(auth.token_hash);
|
||||
Json(serde_json::json!({ "ticket": ticket }))
|
||||
Ok(Json(serde_json::json!({ "ticket": ticket })))
|
||||
}
|
||||
|
||||
/// Validate a download ticket (single-use) and confirm its session still exists.
|
||||
@@ -366,8 +378,9 @@ pub async fn download_zip(
|
||||
State(state): State<AppState>,
|
||||
Query(q): Query<DownloadQuery>,
|
||||
) -> Result<axum::response::Response, AppError> {
|
||||
let user_id = authenticate_download_ticket(&state, &q.ticket).await?;
|
||||
enforce_export_rate(&state, user_id).await?;
|
||||
// Ticket validation only — the rate limit was charged at mint time, where a 429 is visible
|
||||
// to the page. Charging it again here would cost every download two slots.
|
||||
authenticate_download_ticket(&state, &q.ticket).await?;
|
||||
|
||||
let path =
|
||||
resolve_export_file(&state, "zip", "Der ZIP-Export ist noch nicht verfügbar.").await?;
|
||||
@@ -419,8 +432,8 @@ pub async fn download_html(
|
||||
State(state): State<AppState>,
|
||||
Query(q): Query<DownloadQuery>,
|
||||
) -> Result<axum::response::Response, AppError> {
|
||||
let user_id = authenticate_download_ticket(&state, &q.ticket).await?;
|
||||
enforce_export_rate(&state, user_id).await?;
|
||||
// See `download_zip`: the limit is charged at ticket mint, where the client can see it.
|
||||
authenticate_download_ticket(&state, &q.ticket).await?;
|
||||
|
||||
let path =
|
||||
resolve_export_file(&state, "html", "Der HTML-Export ist noch nicht verfügbar.").await?;
|
||||
@@ -536,8 +549,13 @@ async fn enforce_export_rate(state: &AppState, user_id: Uuid) -> Result<(), AppE
|
||||
limit,
|
||||
Duration::from_secs(86400),
|
||||
) {
|
||||
// Names the real window. The generic "warte kurz" wording this used to share with the
|
||||
// per-minute limiters is actively wrong here — the bucket is a DAY, so a guest told to
|
||||
// wait a moment would keep tapping a button that cannot work again until tomorrow.
|
||||
return Err(AppError::TooManyRequests(
|
||||
"Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(),
|
||||
"Du hast das Tageslimit für Downloads erreicht. Versuch es später noch einmal — \
|
||||
deine Galerie bleibt gespeichert."
|
||||
.into(),
|
||||
Some(retry_after_secs),
|
||||
));
|
||||
}
|
||||
|
||||
@@ -40,8 +40,18 @@ impl RateLimiter {
|
||||
timestamps.push(now);
|
||||
Ok(())
|
||||
} else {
|
||||
// The oldest timestamp expires at oldest + window; compute remaining seconds
|
||||
let oldest = timestamps[0];
|
||||
// The oldest timestamp expires at oldest + window; compute remaining seconds.
|
||||
//
|
||||
// `first()`, not `[0]`: with `max == 0` the length check above is false even on an
|
||||
// empty vec, so indexing would panic — WHILE HOLDING THIS MUTEX. That poisons it
|
||||
// process-wide, so every subsequent `.lock().unwrap()` panics too: upload, feed,
|
||||
// join, recover, social, export and the hourly maintenance task all die, and only
|
||||
// a container restart brings them back. `max == 0` is not reachable through the
|
||||
// admin API (every numeric spec has min = 1) but a direct DB edit would do it, and
|
||||
// the blast radius does not justify the sharper syntax.
|
||||
let Some(&oldest) = timestamps.first() else {
|
||||
return Ok(());
|
||||
};
|
||||
let elapsed = now.duration_since(oldest);
|
||||
let remaining = window.saturating_sub(elapsed);
|
||||
Err(remaining.as_secs().max(1))
|
||||
|
||||
Reference in New Issue
Block a user