Skip to main content

max / makenotwork

Fix OTA quarantine deleting from the wrong S3 bucket The scan worker selected the download client by target kind (the SyncKit bucket for OTA artifacts) but the quarantine purge used ctx.s3 (main) and the fallback enqueue hardcoded "main". S3 DeleteObject is idempotent, so deleting an OTA key from the main bucket succeeded and the worker reported a clean quarantine while the malicious artifact persisted in the SyncKit bucket. Add ScanTargetKind::storage_bucket() as the single source of truth and drive the download client, the delete, and the enqueue bucket from it. Regression tests pin OTA->Synckit and cover the OTA kind the purge-object test had silently omitted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-06 13:17 UTC
Commit: 703a7ec8465eec96c6e052e6174a1afdfe567dde
Parent: d7d4254
2 files changed, +41 insertions, -6 deletions
M server/src/db/scan_jobs.rs +30
@@ -64,6 +64,19 @@ impl ScanTargetKind {
64 64 })
65 65 }
66 66
67 + /// The S3 bucket this kind's objects live in. Single source of truth for
68 + /// the scan worker's client selection, quarantine delete, and durable-delete
69 + /// enqueue — all three MUST agree, or a quarantine deletes from the wrong
70 + /// bucket and idempotently "succeeds" while the malware persists (ultra-fuzz
71 + /// Run #24 Storage CRITICAL). OTA artifacts are the only synckit-bucket kind
72 + /// today; this mirrors the `S3_KEY_REFS` registry in `pending_s3_deletions`.
73 + pub fn storage_bucket(&self) -> crate::storage::S3Bucket {
74 + match self {
75 + ScanTargetKind::OtaArtifact => crate::storage::S3Bucket::Synckit,
76 + _ => crate::storage::S3Bucket::Main,
77 + }
78 + }
79 +
67 80 /// Whether this kind carries no `scan_status` column and is rendered
68 81 /// straight from `cdn.makenot.work/{key}`, so a `Quarantined` verdict can
69 82 /// only be enforced by deleting the *DB row* (which stops the URL from
@@ -456,12 +469,29 @@ mod tests {
456 469 ScanTargetKind::ItemImage,
457 470 ScanTargetKind::GalleryImage,
458 471 ScanTargetKind::ContentInsertion,
472 + ScanTargetKind::OtaArtifact,
459 473 ] {
460 474 assert!(kind.quarantine_purges_object(), "{} must purge object", kind.as_str());
461 475 }
462 476 }
463 477
464 478 #[test]
479 + fn storage_bucket_matches_download_and_delete_path() {
480 + // Regression for ultra-fuzz Run #24 Storage CRITICAL: the quarantine
481 + // worker deleted OTA objects from the main bucket (idempotent success)
482 + // while the object persisted in the synckit bucket. `storage_bucket()`
483 + // is now the one source of truth the worker uses for the download
484 + // client, the delete, and the durable-delete enqueue — pin it here so a
485 + // new synckit-bucket kind can't silently default to Main.
486 + use crate::storage::S3Bucket;
487 + use ScanTargetKind::*;
488 + assert_eq!(OtaArtifact.storage_bucket(), S3Bucket::Synckit);
489 + for kind in [Item, Version, Media, ProjectImage, ItemImage, GalleryImage, ContentInsertion] {
490 + assert_eq!(kind.storage_bucket(), S3Bucket::Main, "{} is a main-bucket kind", kind.as_str());
491 + }
492 + }
493 +
494 + #[test]
465 495 fn only_gateless_image_kinds_delete_their_row() {
466 496 // The DB-row deletion is the gate-less-image-only half of enforcement.
467 497 use ScanTargetKind::*;
M server/src/scanning/worker.rs +11 -6
@@ -307,14 +307,19 @@ async fn run_pipeline_and_decide(
307 307 // Holding it across the GET serializes downloads at SCAN_MAX_CONCURRENT
308 308 // and lets a scan backlog starve the DB pool.
309 309 // OTA artifacts live in the SyncKit bucket; everything else in the main
310 - // bucket. Pick the backend to download from by target kind.
311 - let backend: &Arc<dyn StorageBackend> = match kind {
312 - ScanTargetKind::OtaArtifact => ctx.synckit_s3.as_ref().ok_or_else(|| {
310 + // bucket. `kind.storage_bucket()` is the single source of truth: the same
311 + // value drives the download client here, the quarantine delete, and the
312 + // durable-delete enqueue below, so they can't diverge (ultra-fuzz Run #24
313 + // Storage CRITICAL was a wrong-bucket quarantine delete from exactly that
314 + // divergence).
315 + let bucket = kind.storage_bucket();
316 + let backend: &Arc<dyn StorageBackend> = match bucket {
317 + crate::storage::S3Bucket::Synckit => ctx.synckit_s3.as_ref().ok_or_else(|| {
313 318 Box::<dyn std::error::Error + Send + Sync>::from(
314 319 "SyncKit storage not configured; cannot scan OTA artifact",
315 320 )
316 321 })?,
317 - _ => &ctx.s3,
322 + crate::storage::S3Bucket::Main => &ctx.s3,
318 323 };
319 324
320 325 let result: ScanResult = if job.file_size_bytes as u64 > constants::SCAN_SPOOL_MAX_BYTES {
@@ -429,7 +434,7 @@ async fn run_pipeline_and_decide(
429 434 // still-referenced gated key). Mint the authority the sealed delete
430 435 // API requires.
431 436 let auth = crate::storage::S3DeleteAuthority::new();
432 - match ctx.s3.delete_object(&auth, &crate::storage::S3Key::from_stored(&job.s3_key)).await {
437 + match backend.delete_object(&auth, &crate::storage::S3Key::from_stored(&job.s3_key)).await {
433 438 Ok(()) => {
434 439 tracing::warn!(
435 440 s3_key = %job.s3_key, target_kind = %kind.as_str(),
@@ -452,7 +457,7 @@ async fn run_pipeline_and_decide(
452 457 if row_deleted {
453 458 match db::pending_s3_deletions::enqueue_deletions(
454 459 &ctx.db,
455 - &[(job.s3_key.clone(), "main".to_string())],
460 + &[(job.s3_key.clone(), bucket.as_str().to_string())],
456 461 "malware_quarantine",
457 462 )
458 463 .await