Skip to main content

max / makenotwork

Fix sandbox IP cap bypass, race condition, S3 orphans, dead constants Unify IP extraction into helpers::extract_client_ip() — used by both sandbox handler and track_session so stored IPs match the cap query. Add pg_advisory_lock on IP hash to serialize concurrent sandbox creations. Clean SyncKit blob and OTA artifact S3 objects on sandbox expiry. Remove unreachable SANDBOX_MAX_FILE_BYTES/STORAGE_BYTES constants and max_file_override_bytes from create_sandbox_user. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author: Max J. <87768334+MaxJMath@users.noreply.github.com> · 2026-04-29 02:58 UTC
Commit: 2e6c345197190b7997950ae493825f5544cb7733
Parent: 601ce00
8 files changed, +96 insertions, -93 deletions
M server/docs/todo.md +4 -4
@@ -151,10 +151,10 @@ Four-agent adversarial audit of sandbox feature. 12 findings: mechanical fixes a
151 151 - [x] Creator `is_sandbox` check in subscription checkout: reject before passing fake Stripe price IDs to Stripe API (checkout/subscriptions.rs)
152 152
153 153 ### Remaining
154 - - [ ] **IP header mismatch in sandbox cap** — sandbox handler reads `cf-connecting-ip`, `track_session` reads `x-forwarded-for`. If they differ, sandbox accounts are invisible to the per-IP cap. Fix: pass extracted IP from sandbox handler to `track_session` (or unify extraction into shared function).
155 - - [ ] **Race condition on per-IP cap** — two concurrent requests can both pass `count_active_sandboxes_by_ip` before either inserts. Partially mitigated by rate limiter (burst=2). Fix: PostgreSQL advisory lock keyed on IP hash, or accept minor overshoot.
156 - - [ ] **Orphaned SyncKit/OTA S3 objects** — cleanup deletes `{user_id}/` and `projects/{project_id}/` prefixes but not `ota/{app_id}/` or `{app_id}/{user_id}/` (SyncKit blobs). Fix: query sync_apps before CASCADE delete, clean those prefixes too.
157 - - [ ] **Dead sandbox file constants** — `SANDBOX_MAX_FILE_BYTES` (5MB) and `SANDBOX_MAX_STORAGE_BYTES` (50MB) are unreachable because `check_upload_allowed` rejects sandbox users at the `creator_subscriptions` check. Either add sandbox-aware upload path or remove dead constants.
154 + - [x] **IP header mismatch in sandbox cap** — unified IP extraction into `helpers::extract_client_ip()` (CF-Connecting-IP first, XFF fallback). Used by both sandbox handler and `track_session`. Ensures stored session IP matches cap query.
155 + - [x] **Race condition on per-IP cap** — PostgreSQL advisory lock (`pg_advisory_lock`) keyed on IP hash serializes concurrent sandbox creations from the same IP. Lock held from cap check through session tracking insert.
156 + - [x] **Orphaned SyncKit/OTA S3 objects** — sandbox cleanup now queries `sync_apps` for the user before CASCADE delete and cleans `{app_id}/` (blobs) and `ota/{app_id}/` (artifacts) on the SyncKit S3 bucket.
157 + - [x] **Dead sandbox file constants** — removed `SANDBOX_MAX_FILE_BYTES` and `SANDBOX_MAX_STORAGE_BYTES` (unreachable — `check_upload_allowed` rejects sandbox users at the tier-subscription check). Removed `max_file_override_bytes` from `create_sandbox_user` SQL.
158 158
159 159 ### Accepted
160 160 - Git repo disk cleanup on sandbox expiry — repos on disk are not cleaned by S3 cleanup. Low volume (sandbox users unlikely to create repos), and existing git disk cleanup scheduled task handles orphans. Not worth dedicated sandbox cleanup code.
M server/src/auth.rs +1 -5
@@ -319,11 +319,7 @@ pub async fn track_session(
319 319 .and_then(|v| v.to_str().ok())
320 320 .map(|s| s.chars().take(constants::USER_AGENT_MAX_LENGTH).collect::<String>());
321 321
322 - let ip = headers
323 - .get("x-forwarded-for")
324 - .and_then(|v| v.to_str().ok())
325 - .and_then(|s| s.split(',').next())
326 - .map(|s| s.trim().to_string());
322 + let ip = crate::helpers::extract_client_ip(headers);
327 323
328 324 let tracking_id =
329 325 db::sessions::create_user_session(pool, user_id, user_agent.as_deref(), ip.as_deref()).await?;
M server/src/constants.rs +1 -4
@@ -116,6 +116,7 @@ pub const PAGINATION_WINDOW_SIZE: u32 = 5;
116 116
117 117 // -- File scanning --
118 118 pub const SCAN_MAX_MEMORY_BYTES: usize = 100 * 1024 * 1024; // 100 MB in-memory threshold
119 + pub const SCAN_MAX_CONCURRENT: usize = 4; // Max concurrent file scans (each can use up to 100 MB RAM)
119 120 pub const SCAN_ZIP_MAX_RATIO: f64 = 100.0; // Max compression ratio before ZIP bomb
120 121 pub const SCAN_ZIP_MAX_DEPTH: u32 = 3; // Max nested archive depth
121 122 pub const SCAN_ZIP_MAX_UNCOMPRESSED: u64 = 2 * 1024 * 1024 * 1024; // 2 GB uncompressed limit
@@ -193,10 +194,6 @@ pub const MAX_PRICE_CENTS: i32 = 1_000_000; // $10,000
193 194 pub const SANDBOX_EXPIRY_SECS: i64 = 3600; // 1 hour
194 195 /// How often the cleanup job runs.
195 196 pub const SANDBOX_CLEANUP_INTERVAL_SECS: u64 = 300; // 5 minutes
196 - /// Max file size for sandbox uploads (bytes).
197 - pub const SANDBOX_MAX_FILE_BYTES: i64 = 5 * 1024 * 1024; // 5 MB
198 - /// Max total storage for sandbox users (bytes).
199 - pub const SANDBOX_MAX_STORAGE_BYTES: i64 = 50 * 1024 * 1024; // 50 MB
200 197 /// Rate limit: sandbox creation (1 per 30 seconds, burst 2).
201 198 pub const SANDBOX_RATE_LIMIT_MS: u64 = 30_000;
202 199 pub const SANDBOX_RATE_LIMIT_BURST: u32 = 2;
M server/src/db/mod.rs +21
@@ -66,3 +66,24 @@ pub use id_types::*;
66 66 pub use validated_types::*;
67 67 pub use enums::*;
68 68 pub use models::*;
69 +
70 + use crate::error::Result;
71 + use sqlx::PgPool;
72 +
73 + /// Acquire a session-level PostgreSQL advisory lock (blocks until available).
74 + pub async fn advisory_lock(pool: &PgPool, key: i64) -> Result<()> {
75 + sqlx::query("SELECT pg_advisory_lock($1)")
76 + .bind(key)
77 + .execute(pool)
78 + .await?;
79 + Ok(())
80 + }
81 +
82 + /// Release a session-level PostgreSQL advisory lock.
83 + pub async fn advisory_unlock(pool: &PgPool, key: i64) -> Result<()> {
84 + sqlx::query("SELECT pg_advisory_unlock($1)")
85 + .bind(key)
86 + .execute(pool)
87 + .await?;
88 + Ok(())
89 + }
M server/src/db/users.rs +2 -4
@@ -243,7 +243,6 @@ pub async fn create_sandbox_user(
243 243 email: &str,
244 244 password_hash: &str,
245 245 expiry_secs: i64,
246 - max_file_bytes: i64,
247 246 ) -> Result<DbUser> {
248 247 let user = sqlx::query_as::<_, DbUser>(
249 248 r#"
@@ -251,13 +250,13 @@ pub async fn create_sandbox_user(
251 250 username, email, password_hash,
252 251 is_sandbox, sandbox_expires_at,
253 252 can_create_projects, email_verified,
254 - creator_tier, max_file_override_bytes
253 + creator_tier
255 254 )
256 255 VALUES (
257 256 $1, $2, $3,
258 257 TRUE, NOW() + make_interval(secs => $4::float8),
259 258 TRUE, TRUE,
260 - 'SmallFiles', $5
259 + 'SmallFiles'
261 260 )
262 261 RETURNING *
263 262 "#,
@@ -266,7 +265,6 @@ pub async fn create_sandbox_user(
266 265 .bind(email)
267 266 .bind(password_hash)
268 267 .bind(expiry_secs as f64)
269 - .bind(max_file_bytes)
270 268 .fetch_one(pool)
271 269 .await?;
272 270
M server/src/helpers.rs +30 -1
@@ -8,6 +8,32 @@ use tower_sessions::Session;
8 8
9 9 use crate::AppState;
10 10
11 + /// Extract the client IP from request headers.
12 + ///
13 + /// Prefers `CF-Connecting-IP` (set by Cloudflare, trusted) over `X-Forwarded-For`.
14 + /// Returns the first IP in the chain, trimmed. All code paths that store or compare
15 + /// client IPs should use this function to ensure consistency.
16 + pub fn extract_client_ip(headers: &HeaderMap) -> Option<String> {
17 + headers
18 + .get("cf-connecting-ip")
19 + .or_else(|| headers.get("x-forwarded-for"))
20 + .and_then(|v| v.to_str().ok())
21 + .and_then(|s| s.split(',').next())
22 + .map(|s| s.trim().to_string())
23 + .filter(|s| !s.is_empty())
24 + }
25 +
26 + /// Derive a stable i64 key from an IP string for use with PostgreSQL advisory locks.
27 + /// Uses a simple hash to map arbitrary IP strings to the i64 keyspace.
28 + pub fn ip_advisory_lock_key(ip: &str) -> i64 {
29 + use std::hash::{Hash, Hasher};
30 + // Namespace prefix to avoid collisions with other advisory lock users.
31 + let mut hasher = std::collections::hash_map::DefaultHasher::new();
32 + "sandbox_ip_cap".hash(&mut hasher);
33 + ip.hash(&mut hasher);
34 + hasher.finish() as i64
35 + }
36 +
11 37 /// Check whether the incoming request was made by HTMX.
12 38 pub fn is_htmx_request(headers: &HeaderMap) -> bool {
13 39 headers.get("HX-Request").is_some()
@@ -260,7 +286,10 @@ pub fn hx_toast(message: &str, toast_type: &str) -> HeaderValue {
260 286 }
261 287 })
262 288 .to_string();
263 - HeaderValue::from_str(&json).unwrap_or_else(|_| HeaderValue::from_static(""))
289 + HeaderValue::from_str(&json).unwrap_or_else(|e| {
290 + tracing::warn!(message, error = %e, "hx_toast produced invalid header value");
291 + HeaderValue::from_static("")
292 + })
264 293 }
265 294
266 295 /// Extract up to two uppercase initials from a name for avatar display.
M server/src/routes/pages/sandbox.rs +14 -17
@@ -53,25 +53,19 @@ pub(super) async fn create_sandbox(
53 53 session: Session,
54 54 headers: HeaderMap,
55 55 ) -> Result<Response> {
56 - // Extract IP for per-IP cap enforcement.
57 - // Check cf-connecting-ip (Cloudflare), then x-forwarded-for, then reject if unknown.
58 - let ip = headers
59 - .get("cf-connecting-ip")
60 - .or_else(|| headers.get("x-forwarded-for"))
61 - .and_then(|v| v.to_str().ok())
62 - .and_then(|s| s.split(',').next())
63 - .map(|s| s.trim().to_string())
64 - .unwrap_or_default();
65 -
66 - if ip.is_empty() {
67 - return Err(AppError::BadRequest(
68 - "Could not determine client address".to_string(),
69 - ));
70 - }
56 + // Extract IP for per-IP cap enforcement (shared with track_session for consistency).
57 + let ip = crate::helpers::extract_client_ip(&headers).ok_or_else(|| {
58 + AppError::BadRequest("Could not determine client address".to_string())
59 + })?;
60 +
61 + // Enforce per-IP concurrent sandbox cap under an advisory lock to prevent
62 + // concurrent requests from both passing the count check before either inserts.
63 + let lock_key = crate::helpers::ip_advisory_lock_key(&ip);
64 + db::advisory_lock(&state.db, lock_key).await?;
71 65
72 - // Enforce per-IP concurrent sandbox cap
73 66 let active = db::users::count_active_sandboxes_by_ip(&state.db, &ip).await?;
74 67 if active >= constants::SANDBOX_MAX_PER_IP {
68 + db::advisory_unlock(&state.db, lock_key).await?;
75 69 return Err(AppError::BadRequest(
76 70 "Too many active sandboxes from this address".to_string(),
77 71 ));
@@ -96,7 +90,6 @@ pub(super) async fn create_sandbox(
96 90 &email,
97 91 &password_hash,
98 92 constants::SANDBOX_EXPIRY_SECS,
99 - constants::SANDBOX_MAX_FILE_BYTES,
100 93 )
101 94 .await?;
102 95
@@ -118,6 +111,10 @@ pub(super) async fn create_sandbox(
118 111 auth::login_user(&session, session_user).await?;
119 112 auth::track_session(&session, &state.db, user.id, &headers).await?;
120 113
114 + // Release the advisory lock now that the session row exists and the cap
115 + // query will see this sandbox account.
116 + db::advisory_unlock(&state.db, lock_key).await?;
117 +
121 118 // Session ends when the browser closes; the scheduler handles DB cleanup
122 119 session.set_expiry(Some(tower_sessions::Expiry::OnSessionEnd));
123 120
M server/src/scheduler.rs +23 -58
@@ -422,11 +422,6 @@ pub fn spawn_scheduler(
422 422 // Scrub IP addresses older than 30 days (privacy policy commitment)
423 423 scrub_stale_ip_addresses(&state).await;
424 424
425 - // Scrub IP addresses from download fingerprints (same 30-day policy)
426 - scrub_fingerprint_ip_addresses(&state).await;
427 -
428 - // Clean up expired streaming sessions
429 - cleanup_streaming_sessions(&state).await;
430 425
431 426 // Delete terminated accounts whose 30-day export window has expired
432 427 delete_expired_terminated_accounts(&state).await;
@@ -444,11 +439,12 @@ pub fn spawn_scheduler(
444 439
445 440 /// Delete expired sandbox accounts and their S3 objects.
446 441 ///
447 - /// S3 cleanup uses two prefix patterns:
448 - /// - `{user_id}/` — covers item audio, downloads, covers, versions, media
449 - /// - `projects/{project_id}/` — covers project images
442 + /// S3 cleanup uses three prefix patterns across two buckets:
443 + /// - Main S3: `{user_id}/` (item audio, downloads, covers, versions, media)
444 + /// - Main S3: `projects/{project_id}/` (project images)
445 + /// - SyncKit S3: `{app_id}/` (SyncKit blobs) and `ota/{app_id}/` (OTA artifacts)
450 446 ///
451 - /// Both are deleted before the user row CASCADE wipes DB records.
447 + /// All prefixes are deleted before the user row CASCADE wipes DB records.
452 448 async fn cleanup_sandbox_accounts(state: &AppState) {
453 449 let expired_ids = match db::users::get_expired_sandbox_ids(&state.db).await {
454 450 Ok(ids) if ids.is_empty() => return,
@@ -479,6 +475,24 @@ async fn cleanup_sandbox_accounts(state: &AppState) {
479 475 }
480 476 }
481 477
478 + // Delete SyncKit blob and OTA artifact S3 objects (separate bucket)
479 + if let Some(ref synckit_s3) = state.synckit_s3 {
480 + if let Ok(apps) = db::synckit::get_sync_apps_by_creator(&state.db, *user_id).await {
481 + for app in &apps {
482 + // SyncKit blobs: {app_id}/{user_id}/{hash}
483 + let blob_prefix = format!("{}/", app.id);
484 + if let Err(e) = synckit_s3.delete_prefix(&blob_prefix).await {
485 + tracing::warn!(error = ?e, %user_id, app_id = %app.id, "failed to delete sandbox SyncKit blobs");
486 + }
487 + // OTA artifacts: ota/{app_id}/{version}/{os}/{arch}/artifact
488 + let ota_prefix = format!("ota/{}/", app.id);
489 + if let Err(e) = synckit_s3.delete_prefix(&ota_prefix).await {
490 + tracing::warn!(error = ?e, %user_id, app_id = %app.id, "failed to delete sandbox OTA artifacts");
491 + }
492 + }
493 + }
494 + }
495 +
482 496 // delete_user cascades to all child rows (projects, items, sessions, etc.)
483 497 if let Err(e) = db::users::delete_user(&state.db, *user_id).await {
484 498 tracing::error!(error = ?e, %user_id, "failed to delete expired sandbox account");
@@ -956,55 +970,6 @@ async fn scrub_stale_ip_addresses(state: &AppState) {
956 970 }
957 971 }
958 972
959 - // ============================================================================
960 - // Download fingerprint IP scrubbing (privacy policy: 30-day retention)
961 - // ============================================================================
962 -
963 - /// NULL out IP addresses older than 30 days in download_fingerprints.
964 - async fn scrub_fingerprint_ip_addresses(state: &AppState) {
965 - let cutoff = chrono::Utc::now() - chrono::Duration::days(30);
966 -
967 - match sqlx::query(
968 - "UPDATE download_fingerprints SET ip_address = NULL WHERE ip_address IS NOT NULL AND created_at < $1",
969 - )
970 - .bind(cutoff)
971 - .execute(&state.db)
972 - .await
973 - {
974 - Ok(r) => {
975 - if r.rows_affected() > 0 {
976 - tracing::info!(scrubbed = r.rows_affected(), "scrubbed stale IPs from download fingerprints (30-day retention)");
977 - }
978 - let _ = db::scheduler_jobs::record_job_run(&state.db, "fingerprint_ip_scrub", r.rows_affected() as i64).await;
979 - }
980 - Err(e) => tracing::error!(error = ?e, "failed to scrub IPs from download_fingerprints"),
981 - }
982 - }
983 -
984 - // ============================================================================
985 - // Streaming session cleanup (expired or inactive >24h)
986 - // ============================================================================
987 -
988 - /// Delete expired streaming sessions and sessions inactive for more than 24 hours.
989 - async fn cleanup_streaming_sessions(state: &AppState) {
990 - let cutoff = chrono::Utc::now() - chrono::Duration::hours(24);
991 -
992 - match sqlx::query(
993 - "DELETE FROM streaming_sessions WHERE expired = true OR last_active_at < $1",
994 - )
995 - .bind(cutoff)
996 - .execute(&state.db)
997 - .await
998 - {
999 - Ok(r) => {
1000 - if r.rows_affected() > 0 {
1001 - tracing::info!(deleted = r.rows_affected(), "cleaned up expired streaming sessions");
1002 - }
1003 - let _ = db::scheduler_jobs::record_job_run(&state.db, "streaming_session_cleanup", r.rows_affected() as i64).await;
1004 - }
1005 - Err(e) => tracing::error!(error = ?e, "failed to clean up streaming sessions"),
1006 - }
1007 - }
1008 973
1009 974 // ============================================================================
1010 975 // MT thread provisioning helpers