Skip to main content

max / makenotwork

mt: harden the image upload pipeline (F2, F5) F2 (orphan ordering): the handler uploaded to S3 first and inserted the DB row second, so a failed insert left an S3 object with no row — invisible to the m028 reconcile sweep (keyed on the images table), an unbounded leak. Insert the row first, then upload; on upload failure roll the row back. A failed/absent upload now leaves at most a sweepable row pointing at a missing object, never a silent S3 cost. Adds mutations::delete_image_row for the rollback. F5 (content validation): validate_image only cross-checked the filename extension against the multipart Content-Type — both attacker-controlled, so a content-spoofed file (HTML/SVG bytes named evil.png) passed and was served inline same-origin. Add a magic-byte sniff (PNG/JPEG/GIF/WebP) as the authoritative check, and set nosniff + Content-Disposition on serve. Ultra-fuzz Run #2 findings F2 and F5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-18 20:09 UTC
Commit: 8828d8700715b9e5431ee6008b222d5ca15d252f
Parent: a19e4df
3 files changed, +153 insertions, -22 deletions
M multithreaded/crates/mt-db/src/mutations.rs +12
@@ -1168,6 +1168,18 @@ pub async fn remove_image(
1168 1168 Ok(())
1169 1169 }
1170 1170
1171 + /// Hard-delete an image row. Used to roll back the row inserted just before a
1172 + /// failed S3 upload — the object never landed in the bucket, so there is
1173 + /// nothing to reconcile and the row should simply not exist.
1174 + #[tracing::instrument(skip_all)]
1175 + pub async fn delete_image_row(pool: &PgPool, image_id: Uuid) -> Result<(), sqlx::Error> {
1176 + sqlx::query("DELETE FROM images WHERE id = $1")
1177 + .bind(image_id)
1178 + .execute(pool)
1179 + .await?;
1180 + Ok(())
1181 + }
1182 +
1171 1183 /// Mark images whose backing S3 object has been deleted, so the reconcile sweep
1172 1184 /// never revisits them. Called inline after a successful best-effort delete and
1173 1185 /// in batch by the background sweep.
M multithreaded/src/routes/uploads.rs +27 -8
@@ -73,7 +73,7 @@ pub(super) async fn upload_image_handler(
73 73 (StatusCode::BAD_REQUEST, "Failed to read file.").into_response()
74 74 })?;
75 75
76 - let (ext, validated_ct) = storage::validate_image(&filename, &content_type, data.len())
76 + let (ext, validated_ct) = storage::validate_image(&filename, &content_type, &data)
77 77 .map_err(|msg| (StatusCode::UNPROCESSABLE_ENTITY, msg).into_response())?;
78 78
79 79 // Strip EXIF from JPEG
@@ -86,13 +86,12 @@ pub(super) async fn upload_image_handler(
86 86 let s3_key = storage::generate_image_key(&slug, ext);
87 87 let data_len = data.len() as i64;
88 88
89 - // Upload to S3
90 - s3.upload(&s3_key, validated_ct, data).await.map_err(|e| {
91 - tracing::error!(error = %e, "S3 upload failed");
92 - StatusCode::INTERNAL_SERVER_ERROR.into_response()
93 - })?;
94 -
95 - // Record in DB
89 + // Record the DB row BEFORE the S3 upload. If we uploaded first and the
90 + // insert then failed, the S3 object would have no row and the reconcile
91 + // sweep (keyed on `images`) could never find it — an unbounded orphan.
92 + // Insert-first inverts the failure mode: a failed/absent upload leaves at
93 + // most a row with no object, which is bounded, queryable, and cleaned up
94 + // below (or by the sweep), never a silent S3 cost.
96 95 let image_id = mt_db::mutations::insert_image(
97 96 &state.db,
98 97 user.user_id,
@@ -108,6 +107,18 @@ pub(super) async fn upload_image_handler(
108 107 StatusCode::INTERNAL_SERVER_ERROR.into_response()
109 108 })?;
110 109
110 + // Upload to S3. On failure, drop the row we just inserted so neither store
111 + // is left holding a dangling reference; if the cleanup delete itself fails,
112 + // the row simply remains pointing at a missing object — recoverable, not a
113 + // leaked S3 object.
114 + if let Err(e) = s3.upload(&s3_key, validated_ct, data).await {
115 + tracing::error!(error = %e, "S3 upload failed");
116 + if let Err(del) = mt_db::mutations::delete_image_row(&state.db, image_id).await {
117 + tracing::warn!(error = ?del, image_id = %image_id, "failed to roll back image row after S3 upload failure");
118 + }
119 + return Err(StatusCode::INTERNAL_SERVER_ERROR.into_response());
120 + }
121 +
111 122 // Return JSON with the image URL for markdown insertion
112 123 let url = format!("/uploads/{image_id}");
113 124 let markdown = format!("![{filename}]({url})");
@@ -166,10 +177,18 @@ pub(super) async fn serve_image_handler(
166 177 StatusCode::INTERNAL_SERVER_ERROR.into_response()
167 178 })?;
168 179
180 + // Serve hardening. `nosniff` pins the browser to the stored Content-Type
181 + // (which upload-time magic-byte validation guarantees is a real image), so
182 + // a content-spoofed file can never be reinterpreted as HTML/JS. Disposition
183 + // stays `inline` because these are embedded in post bodies via <img>;
184 + // `attachment` would force a download and break the feature. The byte-level
185 + // validation, not a disposition flag, is what makes inline serving safe.
169 186 Ok(Response::builder()
170 187 .status(StatusCode::OK)
171 188 .header(header::CONTENT_TYPE, content_type)
172 189 .header(header::CACHE_CONTROL, "private, max-age=86400, immutable")
190 + .header(header::X_CONTENT_TYPE_OPTIONS, "nosniff")
191 + .header(header::CONTENT_DISPOSITION, "inline")
173 192 .body(Body::from(data))
174 193 .unwrap())
175 194 }
M multithreaded/src/storage.rs +114 -14
@@ -75,12 +75,46 @@ pub fn generate_image_key(community_slug: &str, ext: &str) -> String {
75 75 format!("mt/{community_slug}/{id}.{ext}")
76 76 }
77 77
78 + /// Sniff the real image format from leading magic bytes, independent of the
79 + /// client-declared filename or Content-Type. Returns the canonical extension
80 + /// family (`png`/`jpg`/`gif`/`webp`) or `None` if the bytes are not one of the
81 + /// four allowed image formats. This is the only attacker-uncontrollable signal
82 + /// in the upload — the filename extension and the multipart Content-Type are
83 + /// both client-supplied, so a content check that only cross-references those
84 + /// two (as the old validator did) is trivially satisfied by a spoofed file.
85 + pub fn sniff_image_format(data: &[u8]) -> Option<&'static str> {
86 + // PNG: 89 50 4E 47 0D 0A 1A 0A
87 + if data.starts_with(&[0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]) {
88 + return Some("png");
89 + }
90 + // JPEG: starts with FF D8 FF
91 + if data.starts_with(&[0xFF, 0xD8, 0xFF]) {
92 + return Some("jpg");
93 + }
94 + // GIF: "GIF87a" or "GIF89a"
95 + if data.starts_with(b"GIF87a") || data.starts_with(b"GIF89a") {
96 + return Some("gif");
97 + }
98 + // WebP: "RIFF" .... "WEBP" (bytes 0-3 = RIFF, 8-11 = WEBP)
99 + if data.len() >= 12 && &data[0..4] == b"RIFF" && &data[8..12] == b"WEBP" {
100 + return Some("webp");
101 + }
102 + None
103 + }
104 +
78 105 /// Validate an uploaded file. Returns the sanitized extension and content type.
106 + ///
107 + /// Defence in depth: the filename extension and the multipart Content-Type must
108 + /// agree AND the actual file bytes must sniff to the same image format. A
109 + /// polyglot or content-spoofed file (HTML/SVG bytes named `evil.png` with a
110 + /// faked `image/png` Content-Type) fails the byte sniff even though it satisfies
111 + /// the metadata cross-check.
79 112 pub fn validate_image(
80 113 filename: &str,
81 114 content_type: &str,
82 - size: usize,
115 + data: &[u8],
83 116 ) -> Result<(&'static str, &'static str), &'static str> {
117 + let size = data.len();
84 118 if size > MAX_IMAGE_SIZE {
85 119 return Err("Image exceeds 5 MB limit.");
86 120 }
@@ -122,7 +156,12 @@ pub fn validate_image(
122 156 return Err("File extension does not match content type.");
123 157 }
124 158
125 - Ok((ext_str, ct))
159 + // Authoritative check: the bytes themselves must be the declared format.
160 + match sniff_image_format(data) {
161 + Some(sniffed) if sniffed == ext_str => Ok((ext_str, ct)),
162 + Some(_) => Err("File contents do not match the declared image type."),
163 + None => Err("File is not a valid PNG, JPEG, GIF, or WebP image."),
164 + }
126 165 }
127 166
128 167 /// Strip EXIF metadata from JPEG data.
@@ -189,44 +228,105 @@ pub fn strip_exif_jpeg(data: &[u8]) -> Vec<u8> {
189 228 mod tests {
190 229 use super::*;
191 230
231 + // Minimal byte fixtures carrying each format's real magic bytes.
232 + fn png_bytes() -> Vec<u8> {
233 + let mut v = vec![0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A];
234 + v.extend_from_slice(&[0u8; 64]);
235 + v
236 + }
237 + fn jpeg_bytes() -> Vec<u8> {
238 + let mut v = vec![0xFF, 0xD8, 0xFF, 0xE0];
239 + v.extend_from_slice(&[0u8; 64]);
240 + v
241 + }
242 + fn gif_bytes() -> Vec<u8> {
243 + let mut v = b"GIF89a".to_vec();
244 + v.extend_from_slice(&[0u8; 64]);
245 + v
246 + }
247 + fn webp_bytes() -> Vec<u8> {
248 + let mut v = b"RIFF\x00\x00\x00\x00WEBP".to_vec();
249 + v.extend_from_slice(&[0u8; 64]);
250 + v
251 + }
252 +
192 253 #[test]
193 254 fn validate_valid_png() {
194 - let (ext, ct) = validate_image("photo.png", "image/png", 1024).unwrap();
255 + let (ext, ct) = validate_image("photo.png", "image/png", &png_bytes()).unwrap();
195 256 assert_eq!(ext, "png");
196 257 assert_eq!(ct, "image/png");
197 258 }
198 259
199 260 #[test]
200 261 fn validate_valid_jpeg() {
201 - let (ext, _) = validate_image("photo.jpg", "image/jpeg", 1024).unwrap();
262 + let (ext, _) = validate_image("photo.jpg", "image/jpeg", &jpeg_bytes()).unwrap();
202 263 assert_eq!(ext, "jpg");
203 264 }
204 265
205 266 #[test]
206 267 fn validate_rejects_oversized() {
207 - let err = validate_image("big.png", "image/png", 6 * 1024 * 1024).unwrap_err();
268 + let mut big = png_bytes();
269 + big.resize(6 * 1024 * 1024, 0);
270 + let err = validate_image("big.png", "image/png", &big).unwrap_err();
208 271 assert!(err.contains("5 MB"));
209 272 }
210 273
211 274 #[test]
212 275 fn validate_rejects_bad_extension() {
213 - let err = validate_image("file.exe", "application/octet-stream", 1024).unwrap_err();
276 + let err = validate_image("file.exe", "application/octet-stream", &png_bytes()).unwrap_err();
214 277 assert!(err.contains("Allowed types"));
215 278 }
216 279
217 280 #[test]
218 281 fn validate_rejects_mismatched_type() {
219 - let err = validate_image("photo.png", "image/jpeg", 1024).unwrap_err();
282 + let err = validate_image("photo.png", "image/jpeg", &png_bytes()).unwrap_err();
220 283 assert!(err.contains("does not match"));
221 284 }
222 285
223 286 #[test]
224 287 fn validate_rejects_empty() {
225 - let err = validate_image("photo.png", "image/png", 0).unwrap_err();
288 + let err = validate_image("photo.png", "image/png", &[]).unwrap_err();
226 289 assert!(err.contains("Empty"));
227 290 }
228 291
229 292 #[test]
293 + fn validate_rejects_content_spoof() {
294 + // Metadata says PNG and they agree, but the bytes are HTML — the classic
295 + // polyglot/spoof the old metadata-only check waved through.
296 + let html = b"<!DOCTYPE html><script>alert(1)</script>";
297 + let err = validate_image("evil.png", "image/png", html).unwrap_err();
298 + assert!(err.contains("not a valid"), "got: {err}");
299 + }
300 +
301 + #[test]
302 + fn validate_rejects_format_mismatch_bytes() {
303 + // Declared PNG (ext + content-type agree) but the bytes are a real JPEG.
304 + let err = validate_image("photo.png", "image/png", &jpeg_bytes()).unwrap_err();
305 + assert!(err.contains("do not match the declared"), "got: {err}");
306 + }
307 +
308 + #[test]
309 + fn sniff_detects_each_format() {
310 + assert_eq!(sniff_image_format(&png_bytes()), Some("png"));
311 + assert_eq!(sniff_image_format(&jpeg_bytes()), Some("jpg"));
312 + assert_eq!(sniff_image_format(&gif_bytes()), Some("gif"));
313 + assert_eq!(sniff_image_format(&webp_bytes()), Some("webp"));
314 + }
315 +
316 + #[test]
317 + fn sniff_rejects_non_image() {
318 + assert_eq!(sniff_image_format(b"<svg xmlns=...>"), None);
319 + assert_eq!(sniff_image_format(b""), None);
320 + assert_eq!(sniff_image_format(b"RIFF\x00\x00\x00\x00AVI "), None); // RIFF but not WEBP
321 + }
322 +
323 + #[test]
324 + fn validate_accepts_gif_and_webp() {
325 + assert!(validate_image("a.gif", "image/gif", &gif_bytes()).is_ok());
326 + assert!(validate_image("a.webp", "image/webp", &webp_bytes()).is_ok());
327 + }
328 +
329 + #[test]
230 330 fn strip_exif_preserves_non_jpeg() {
231 331 let data = b"not a jpeg";
232 332 let result = strip_exif_jpeg(data);
@@ -268,14 +368,14 @@ mod tests {
268 368 // Verify our ALLOWED_EXTENSIONS list matches validate_image behavior
269 369 for ext in ALLOWED_EXTENSIONS {
270 370 let filename = format!("test.{ext}");
271 - let ct = match *ext {
272 - "png" => "image/png",
273 - "jpg" | "jpeg" => "image/jpeg",
274 - "gif" => "image/gif",
275 - "webp" => "image/webp",
371 + let (ct, bytes) = match *ext {
372 + "png" => ("image/png", png_bytes()),
373 + "jpg" | "jpeg" => ("image/jpeg", jpeg_bytes()),
374 + "gif" => ("image/gif", gif_bytes()),
375 + "webp" => ("image/webp", webp_bytes()),
276 376 _ => continue,
277 377 };
278 - assert!(validate_image(&filename, ct, 1024).is_ok(), "Extension {ext} should be valid");
378 + assert!(validate_image(&filename, ct, &bytes).is_ok(), "Extension {ext} should be valid");
279 379 }
280 380 }
281 381 }