Skip to main content

max / makenotwork

storage: cover liveness by bare s3_key; ranged read for media sniff Cover liveness was the one fragile, load-bearing string match: the deletion worker LIKE-suffix-matched a full cover_image_url with hand-rolled wildcard escaping and path-anchoring, while every other key class exact-matched a bare key. Give projects a cover_s3_key column (migration 152, with a backfill of both projects and legacy items from the canonical {cdn_base}/{key} URL), store it on both project-cover write paths, and switch the registry to exact matching. The S3KeyMatch enum, the UrlSuffix variant, and the escaping/anchoring code are deleted — forgetting the bare key is no longer how a cover is stored. Also add a ranged GetObject (download_head) to the storage backend and use it for the media-confirm 4 KB content sniff, so sniffing a 20 GB object no longer initiates a full-object GET. ultra-fuzz Run 5 M-Sto1 + M3 (deep). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-24 02:32 UTC
Commit: ea9eb66d2c5495a34c590b1af3e5267896d3810a
Parent: b84e6e9
10 files changed, +178 insertions, -71 deletions
A server/migrations/152_cover_s3_key_backfill.sql +33
@@ -0,0 +1,33 @@
1 + -- Cover liveness via a bare S3 key, replacing fragile URL-suffix matching.
2 + --
3 + -- The deletion worker's liveness check (`db::pending_s3_deletions::is_s3_key_live`)
4 + -- exact-matches a stored bare key for every S3 key class EXCEPT covers, which it
5 + -- had to LIKE-suffix-match against a full `cover_image_url`, with hand-rolled
6 + -- wildcard escaping and `/`-anchoring. `items.cover_s3_key` already exists
7 + -- (migration 053) and is written by the item-image confirm path; `projects`
8 + -- stored only the URL. Give projects a bare key column too, backfill both tables'
9 + -- legacy rows from the canonical `{cdn_base}/{s3_key}` URL form, and the registry
10 + -- switches both cover classes to exact matching (the UrlSuffix matcher and its
11 + -- escaping are deleted from the code in the same change).
12 +
13 + ALTER TABLE projects ADD COLUMN IF NOT EXISTS cover_s3_key TEXT;
14 +
15 + -- Backfill from the canonical URL form: strip scheme + host (and any query
16 + -- string) to recover the object key. Stored cover URLs are always
17 + -- `{cdn_base}/{s3_key}`, so the path after the host IS the key. Only touch rows
18 + -- that have a URL but no key yet; a URL that isn't an http(s) path with a
19 + -- non-empty path is left NULL (nothing for the worker to protect).
20 + UPDATE projects
21 + SET cover_s3_key = substring(cover_image_url from '^https?://[^/]+/([^?]*)')
22 + WHERE cover_image_url IS NOT NULL
23 + AND cover_s3_key IS NULL
24 + AND cover_image_url ~ '^https?://[^/]+/[^?]+';
25 +
26 + -- Legacy items predating migration 053 may carry a cover_image_url with a NULL
27 + -- cover_s3_key; backfill them the same way so dropping the items URL-suffix
28 + -- registry entry can't strand a still-live cover.
29 + UPDATE items
30 + SET cover_s3_key = substring(cover_image_url from '^https?://[^/]+/([^?]*)')
31 + WHERE cover_image_url IS NOT NULL
32 + AND cover_s3_key IS NULL
33 + AND cover_image_url ~ '^https?://[^/]+/[^?]+';
M server/src/db/pending_s3_deletions.rs +23 -49
@@ -86,47 +86,36 @@ pub async fn move_to_dead_letter(pool: &PgPool, ids: &[Uuid]) -> Result<u64> {
86 86 Ok(result.rows_affected())
87 87 }
88 88
89 - /// How a table column references an S3 object key.
90 - #[derive(Clone, Copy)]
91 - enum S3KeyMatch {
92 - /// `column = $1` — the column stores the bare s3_key.
93 - Exact,
94 - /// `column LIKE $2 ESCAPE '\'` — the column stores a full URL ending in the
95 - /// s3_key (e.g. a CDN cover-image URL). The bound value is `%{escaped_key}`.
96 - UrlSuffix,
97 - }
98 -
99 89 /// One place a live S3 object key can still be referenced. [`is_s3_key_live`] is
100 90 /// generated from this registry, so a new s3_key-bearing table is added in
101 91 /// exactly one place AND declares its own bucket — it can't be checked against
102 92 /// the wrong bucket (the OTA-key-checked-in-the-main-branch bug this replaced)
103 - /// or silently forgotten by one branch of a hand-written query.
93 + /// or silently forgotten by one branch of a hand-written query. Every registered
94 + /// column stores a bare s3_key, so the check is always `column = $1` — covers
95 + /// gained `cover_s3_key` in migration 152, retiring the old URL-suffix match.
104 96 struct S3KeyRef {
105 97 bucket: &'static str,
106 98 table: &'static str,
107 99 column: &'static str,
108 - matching: S3KeyMatch,
109 100 }
110 101
111 102 /// The registry. To add a table that stores an S3 key, add a row here.
112 103 const S3_KEY_REFS: &[S3KeyRef] = &[
113 104 // ── main bucket: creator content ──
114 - S3KeyRef { bucket: "main", table: "media_files", column: "s3_key", matching: S3KeyMatch::Exact },
115 - S3KeyRef { bucket: "main", table: "versions", column: "s3_key", matching: S3KeyMatch::Exact },
116 - S3KeyRef { bucket: "main", table: "items", column: "audio_s3_key", matching: S3KeyMatch::Exact },
117 - S3KeyRef { bucket: "main", table: "items", column: "cover_s3_key", matching: S3KeyMatch::Exact },
118 - S3KeyRef { bucket: "main", table: "items", column: "video_s3_key", matching: S3KeyMatch::Exact },
119 - // cover_image_url stores a full CDN URL, not a bare key; image keys live
120 - // under their own `.../image/` prefix so a suffix match is unambiguous.
121 - S3KeyRef { bucket: "main", table: "items", column: "cover_image_url", matching: S3KeyMatch::UrlSuffix },
122 - S3KeyRef { bucket: "main", table: "projects", column: "cover_image_url", matching: S3KeyMatch::UrlSuffix },
123 - // gallery images store the bare s3_key directly (not a URL) → exact match.
124 - S3KeyRef { bucket: "main", table: "item_images", column: "s3_key", matching: S3KeyMatch::Exact },
125 - S3KeyRef { bucket: "main", table: "project_images", column: "s3_key", matching: S3KeyMatch::Exact },
126 - S3KeyRef { bucket: "main", table: "content_insertions", column: "storage_key", matching: S3KeyMatch::Exact },
105 + S3KeyRef { bucket: "main", table: "media_files", column: "s3_key" },
106 + S3KeyRef { bucket: "main", table: "versions", column: "s3_key" },
107 + S3KeyRef { bucket: "main", table: "items", column: "audio_s3_key" },
108 + S3KeyRef { bucket: "main", table: "items", column: "cover_s3_key" },
109 + S3KeyRef { bucket: "main", table: "items", column: "video_s3_key" },
110 + // Covers store the bare key directly (migration 152 added projects.cover_s3_key;
111 + // items.cover_s3_key predates it).
112 + S3KeyRef { bucket: "main", table: "projects", column: "cover_s3_key" },
113 + S3KeyRef { bucket: "main", table: "item_images", column: "s3_key" },
114 + S3KeyRef { bucket: "main", table: "project_images", column: "s3_key" },
115 + S3KeyRef { bucket: "main", table: "content_insertions", column: "storage_key" },
127 116 // ── synckit bucket: SyncKit blobs + OTA artifacts (both deterministic keys) ──
128 - S3KeyRef { bucket: "synckit", table: "sync_blobs", column: "s3_key", matching: S3KeyMatch::Exact },
129 - S3KeyRef { bucket: "synckit", table: "ota_artifacts", column: "s3_key", matching: S3KeyMatch::Exact },
117 + S3KeyRef { bucket: "synckit", table: "sync_blobs", column: "s3_key" },
118 + S3KeyRef { bucket: "synckit", table: "ota_artifacts", column: "s3_key" },
130 119 ];
131 120
132 121 /// Returns true if any live row in `bucket` still references `s3_key`. Used by
@@ -156,33 +145,18 @@ pub async fn is_s3_key_live(pool: &PgPool, bucket: &str, s3_key: &str) -> Result
156 145 }
157 146
158 147 // Table/column names are compile-time constants from S3_KEY_REFS (never user
159 - // input); the key value is always a bound parameter ($1 exact, $2 url-suffix).
160 - let needs_url_suffix = refs.iter().any(|r| matches!(r.matching, S3KeyMatch::UrlSuffix));
148 + // input); the key value is always the bound parameter $1. Every class stores
149 + // a bare key, so liveness is a flat OR of equality checks.
161 150 let clauses: Vec<String> = refs
162 151 .iter()
163 - .map(|r| match r.matching {
164 - S3KeyMatch::Exact => format!("EXISTS(SELECT 1 FROM {} WHERE {} = $1)", r.table, r.column),
165 - S3KeyMatch::UrlSuffix => {
166 - format!("EXISTS(SELECT 1 FROM {} WHERE {} LIKE $2 ESCAPE '\\')", r.table, r.column)
167 - }
168 - })
152 + .map(|r| format!("EXISTS(SELECT 1 FROM {} WHERE {} = $1)", r.table, r.column))
169 153 .collect();
170 154 let sql = format!("SELECT {}", clauses.join(" OR "));
171 155
172 - let mut query = sqlx::query_scalar::<_, bool>(&sql).bind(s3_key);
173 - if needs_url_suffix {
174 - // Escape LIKE wildcards so a key with a literal `_`/`%` (from a user
175 - // folder name) can't false-positive against neighbouring rows.
176 - let escaped = s3_key.replace('\\', "\\\\").replace('%', "\\%").replace('_', "\\_");
177 - // Anchor on the leading path separator. Stored values are always
178 - // `{cdn_base}/{s3_key}`, so the key is preceded by `/` in every live
179 - // URL. Without the anchor, `LIKE '%u/1/cover.png'` also matches an
180 - // unrelated row ending `.../OTHERu/1/cover.png`, falsely marking a dead
181 - // object live and suppressing its deletion (orphan leak). Requiring the
182 - // `/` boundary makes the substring match a true path-suffix match.
183 - query = query.bind(format!("%/{escaped}"));
184 - }
185 - Ok(query.fetch_one(pool).await?)
156 + Ok(sqlx::query_scalar::<_, bool>(&sql)
157 + .bind(s3_key)
158 + .fetch_one(pool)
159 + .await?)
186 160 }
187 161
188 162 /// Fetch stale pending deletions (older than min_age, up to limit).
M server/src/db/projects.rs +11 -2
@@ -362,11 +362,16 @@ pub async fn update_project_image_url<'e>(
362 362 id: ProjectId,
363 363 user_id: UserId,
364 364 url: &str,
365 + cover_s3_key: Option<&str>,
365 366 ) -> Result<bool> {
366 - let result = sqlx::query("UPDATE projects SET cover_image_url = $1, updated_at = NOW() WHERE id = $2 AND user_id = $3")
367 + // Store the bare key alongside the URL so the deletion worker's liveness
368 + // check matches it exactly (no URL-suffix parsing). The wizard derives the
369 + // key from the CDN URL it just validated; `None` only when clearing.
370 + let result = sqlx::query("UPDATE projects SET cover_image_url = $1, cover_s3_key = $4, updated_at = NOW() WHERE id = $2 AND user_id = $3")
367 371 .bind(url)
368 372 .bind(id)
369 373 .bind(user_id)
374 + .bind(cover_s3_key)
370 375 .execute(executor)
371 376 .await?;
372 377
@@ -394,11 +399,14 @@ pub async fn update_project_cover_cas<'e>(
394 399 user_id: UserId,
395 400 expected_old_url: Option<&str>,
396 401 url: &str,
402 + cover_s3_key: &str,
397 403 file_size_bytes: i64,
398 404 ) -> Result<bool> {
405 + // Record the bare key (the confirm handler's `req.s3_key`) so deletion-worker
406 + // liveness is an exact key match, not a URL-suffix match.
399 407 let result = sqlx::query(
400 408 r#"UPDATE projects
401 - SET cover_image_url = $1, cover_image_size_bytes = $4, updated_at = NOW()
409 + SET cover_image_url = $1, cover_s3_key = $6, cover_image_size_bytes = $4, updated_at = NOW()
402 410 WHERE id = $2 AND user_id = $3
403 411 AND cover_image_url IS NOT DISTINCT FROM $5"#,
404 412 )
@@ -407,6 +415,7 @@ pub async fn update_project_cover_cas<'e>(
407 415 .bind(user_id)
408 416 .bind(file_size_bytes)
409 417 .bind(expected_old_url)
418 + .bind(cover_s3_key)
410 419 .execute(executor)
411 420 .await?;
412 421
M server/src/routes/pages/dashboard/wizards/project.rs +14 -1
@@ -275,7 +275,20 @@ async fn save_appearance(
275 275 {
276 276 return Err(AppError::validation("Invalid cover image URL"));
277 277 }
278 - db::projects::update_project_image_url(&state.db, project.id, project.user_id, image_url).await?;
278 + // Recover the bare s3_key from the canonical `{cdn_base}/{key}` URL so the
279 + // deletion worker can match the cover by exact key (migration 152). None
280 + // in dev/no-CDN (presigned URL) — acceptable, there is no live worker there.
281 + // The validated URL lives under the CDN base, so the CDN-prefix branch
282 + // recovers the key; bucket/endpoint (path-style fallback) aren't needed.
283 + let cover_s3_key = crate::storage::extract_s3_key_from_url(
284 + image_url,
285 + state.config.cdn_base_url.as_deref(),
286 + None,
287 + None,
288 + );
289 + db::projects::update_project_image_url(
290 + &state.db, project.id, project.user_id, image_url, cover_s3_key.as_deref(),
291 + ).await?;
279 292 }
280 293 Ok(())
281 294 }
M server/src/routes/storage/images.rs +1 -1
@@ -237,7 +237,7 @@ pub(super) async fn project_image_confirm(
237 237 ).await?;
238 238 let ok = db::projects::update_project_cover_cas(
239 239 &mut *tx, req.project_id, user.id,
240 - project.cover_image_url.as_deref(), &image_url, file_size_bytes,
240 + project.cover_image_url.as_deref(), &image_url, &req.s3_key, file_size_bytes,
241 241 ).await?;
242 242 if !ok {
243 243 return Ok(false);
M server/src/routes/storage/media.rs +3 -12
@@ -241,18 +241,9 @@ pub(super) async fn media_confirm(
241 241 // gate (Run #22 Storage MED). Sniff the object's leading bytes; if it is
242 242 // detectably a different audio/visual category than declared, reject + orphan.
243 243 {
244 - let mut stream = s3.download_stream(&req.s3_key).await?;
245 - let mut head: Vec<u8> = Vec::with_capacity(4096);
246 - loop {
247 - if head.len() >= 4096 {
248 - break;
249 - }
250 - match stream.try_next().await {
251 - Ok(Some(chunk)) => head.extend_from_slice(&chunk),
252 - Ok(None) => break,
253 - Err(e) => return Err(AppError::Storage(format!("read media header from S3: {e}"))),
254 - }
255 - }
244 + // Ranged read of just the header — a 4 KB sniff must not transfer the
245 + // whole (up to 20 GB) object. Production issues `Range: bytes=0-4095`.
246 + let head = s3.download_head(&req.s3_key, 4096).await?;
256 247 let detected = infer::get(&head).map(|kind| kind.matcher_type());
257 248 // `media_type` is only ever "image" or "video" (see `classify_media`).
258 249 let mismatch = match media_type {
M server/src/storage.rs +29
@@ -321,6 +321,23 @@ pub trait StorageBackend: Send + Sync {
321 321 /// drive the stream to disk (scanner spool) or to a layer that consumes
322 322 /// chunks directly.
323 323 async fn download_stream(&self, s3_key: &str) -> Result<s3_storage::ByteStream>;
324 + /// Read up to the first `len` bytes of an object. Production overrides this
325 + /// with a ranged `GetObject` so a content sniff transfers only the header,
326 + /// not the whole object. The default streams and stops early — correct, but
327 + /// it still initiates a full GET — which is fine for in-memory test backends.
328 + async fn download_head(&self, s3_key: &str, len: usize) -> Result<Vec<u8>> {
329 + let mut stream = self.download_stream(s3_key).await?;
330 + let mut head = Vec::with_capacity(len.min(64 * 1024));
331 + while head.len() < len {
332 + match stream.try_next().await {
333 + Ok(Some(chunk)) => head.extend_from_slice(&chunk),
334 + Ok(None) => break,
335 + Err(e) => return Err(AppError::Storage(format!("read object head from S3: {e}"))),
336 + }
337 + }
338 + head.truncate(len);
339 + Ok(head)
340 + }
324 341 async fn upload_object(&self, s3_key: &S3Key, content_type: &str, data: Vec<u8>, cache_control: Option<&str>) -> Result<()>;
325 342 /// Delete an object. Requires an [`S3DeleteAuthority`] — route handlers
326 343 /// cannot mint one, so they must enqueue through `pending_s3_deletions`
@@ -642,6 +659,14 @@ impl S3Client {
642 659 .map_err(AppError::Storage)
643 660 }
644 661
662 + /// Read the first `len` bytes via a ranged S3 GET (for content sniffing).
663 + pub async fn download_head(&self, s3_key: &str, len: usize) -> Result<Vec<u8>> {
664 + self.inner
665 + .download_head(s3_key, len)
666 + .await
667 + .map_err(AppError::Storage)
668 + }
669 +
645 670 /// Upload an object to S3 from bytes
646 671 pub async fn upload_object(
647 672 &self,
@@ -857,6 +882,10 @@ impl StorageBackend for S3Client {
857 882 self.download_stream(s3_key).await
858 883 }
859 884
885 + async fn download_head(&self, s3_key: &str, len: usize) -> Result<Vec<u8>> {
886 + self.download_head(s3_key, len).await
887 + }
888 +
860 889 async fn upload_object(&self, s3_key: &S3Key, content_type: &str, data: Vec<u8>, cache_control: Option<&str>) -> Result<()> {
861 890 self.upload_object(s3_key, content_type, data, cache_control).await
862 891 }
M server/tests/workflows/ota.rs +32
@@ -838,3 +838,35 @@ async fn is_s3_key_live_covers_main_bucket() {
838 838 "an unreferenced main-bucket key must not be reported live"
839 839 );
840 840 }
841 +
842 + #[tokio::test]
843 + async fn is_s3_key_live_matches_project_cover_by_bare_key() {
844 + // Migration 152: project covers store a bare `cover_s3_key` and liveness is
845 + // an exact key match (no URL-suffix matching). A project whose cover key is
846 + // set must report that key live, regardless of the cover_image_url value.
847 + let mut h = TestHarness::new().await;
848 + let user_id = h.signup("coverliveuser", "coverlive@example.com", "Password1!").await;
849 + let pid: uuid::Uuid = sqlx::query_scalar(
850 + "INSERT INTO projects (user_id, slug, title, cover_image_url, cover_s3_key) \
851 + VALUES ($1, 'cov', 'Cov', 'https://cdn.example/abc/projects/p/image/c.png', $2) RETURNING id",
852 + )
853 + .bind(user_id)
854 + .bind("abc/projects/p/image/c.png")
855 + .fetch_one(&h.db)
856 + .await
857 + .unwrap();
858 + let _ = pid;
859 +
860 + assert!(
861 + makenotwork::db::pending_s3_deletions::is_s3_key_live(&h.db, "main", "abc/projects/p/image/c.png")
862 + .await
863 + .unwrap(),
864 + "a project cover's bare s3_key must be reported live"
865 + );
866 + assert!(
867 + !makenotwork::db::pending_s3_deletions::is_s3_key_live(&h.db, "main", "abc/projects/p/image/OTHER.png")
868 + .await
869 + .unwrap(),
870 + "a non-matching cover key must not be reported live"
871 + );
872 + }
M server/tests/workflows/storage.rs +7 -6
@@ -998,34 +998,35 @@ async fn update_project_cover_cas_guards_against_stale_and_non_owner() {
998 998 let owner = UserId::from(uuid::Uuid::parse_str(&user_id).unwrap());
999 999
1000 1000 // cover_image_url is NULL. A stale expected url matches zero rows.
1001 - let ok = update_project_cover_cas(&h.db, pid, owner, Some("stale_url"), "url_v1", 1000)
1001 + let ok = update_project_cover_cas(&h.db, pid, owner, Some("stale_url"), "url_v1", "key_v1", 1000)
1002 1002 .await
1003 1003 .unwrap();
1004 1004 assert!(!ok, "a stale expected url must match zero rows");
1005 1005
1006 1006 // Correct CAS (expected None) commits.
1007 - let ok = update_project_cover_cas(&h.db, pid, owner, None, "url_v1", 1000).await.unwrap();
1007 + let ok = update_project_cover_cas(&h.db, pid, owner, None, "url_v1", "key_v1", 1000).await.unwrap();
1008 1008 assert!(ok);
1009 1009
1010 1010 // Stale-None loses now that the cover is set.
1011 - let ok = update_project_cover_cas(&h.db, pid, owner, None, "url_v2", 2000).await.unwrap();
1011 + let ok = update_project_cover_cas(&h.db, pid, owner, None, "url_v2", "key_v2", 2000).await.unwrap();
1012 1012 assert!(!ok, "stale-None confirm must lose once the cover is set");
1013 1013
1014 1014 // A non-owner cannot write, even with the correct expected url.
1015 1015 let stranger = UserId::new();
1016 - let ok = update_project_cover_cas(&h.db, pid, stranger, Some("url_v1"), "url_v3", 3000)
1016 + let ok = update_project_cover_cas(&h.db, pid, stranger, Some("url_v1"), "url_v3", "key_v3", 3000)
1017 1017 .await
1018 1018 .unwrap();
1019 1019 assert!(!ok, "a non-owner must not write the project cover");
1020 1020
1021 - let (url, sz): (Option<String>, Option<i64>) = sqlx::query_as(
1022 - "SELECT cover_image_url, cover_image_size_bytes FROM projects WHERE id = $1::uuid",
1021 + let (url, key, sz): (Option<String>, Option<String>, Option<i64>) = sqlx::query_as(
1022 + "SELECT cover_image_url, cover_s3_key, cover_image_size_bytes FROM projects WHERE id = $1::uuid",
1023 1023 )
1024 1024 .bind(&project_id)
1025 1025 .fetch_one(&h.db)
1026 1026 .await
1027 1027 .unwrap();
1028 1028 assert_eq!(url.as_deref(), Some("url_v1"));
1029 + assert_eq!(key.as_deref(), Some("key_v1"), "the bare cover key is persisted alongside the url");
1029 1030 assert_eq!(sz, Some(1000));
1030 1031 }
1031 1032
M shared/s3-storage/src/lib.rs +25
@@ -160,6 +160,31 @@ impl S3Client {
160 160 Ok(resp.body)
161 161 }
162 162
163 + /// Download only the first `len` bytes of an object via a ranged
164 + /// `GetObject` (`Range: bytes=0-{len-1}`). Used for content sniffing so a
165 + /// 4 KB header read doesn't initiate a transfer of the whole object. Returns
166 + /// fewer bytes if the object is smaller than `len`.
167 + pub async fn download_head(&self, key: &str, len: usize) -> Result<Vec<u8>, String> {
168 + if len == 0 {
169 + return Ok(Vec::new());
170 + }
171 + let resp = self
172 + .client
173 + .get_object()
174 + .bucket(&self.bucket)
175 + .key(key)
176 + .range(format!("bytes=0-{}", len - 1))
177 + .send()
178 + .await
179 + .map_err(|e| format!("S3 ranged download failed: {e}"))?;
180 + let data = resp
181 + .body
182 + .collect()
183 + .await
184 + .map_err(|e| format!("S3 ranged body read failed: {e}"))?;
185 + Ok(data.to_vec())
186 + }
187 +
163 188 /// Delete an object from S3.
164 189 pub async fn delete(&self, key: &str) -> Result<(), String> {
165 190 self.client