Skip to main content

max / makenotwork

security: owner-scope blog/item mutations, polyglot + moderation tests Drive Security A- -> A+ (ultra-fuzz Run 7 --deep). Owner-scope the two genuine guard-dependent mutations (Sec-M2 defense in depth, mirroring media_files::delete): delete_blog_post and set_item_listed now enforce ownership IN the SQL (project_id IN (SELECT id FROM projects WHERE user_id = $N)) and return whether an owned row matched, so a missing upstream ownership check can no longer touch another user's post/item. Reconciliation findings (verified against the tree, not patched): - bump_cache_generation is a cache-counter increment, not a security boundary -- the OwnedResource-witness premise was a false positive, so it is left as-is (would be churn across 55 cache-invalidation call sites). - scanning/archive.rs walk_compressed already enforces the per-stream ratio cap (counted > compressed*MAX_RATIO) with an early stop at that cap, structurally mirroring walk_zip; no asymmetry to unify. content_type polyglot/markup tests: script/SVG payloads, BOM- and whitespace-prefixed markup, markup pushed past the sniff window, and the acknowledged binary-magic-prefixed blind spot (pins the documented defense-in-depth boundary so a future tightening doesn't misread it). Moderation coverage for the previously untested admin handlers: admin_remove_item/admin_restore_item and admin_decide_appeal (denied). Gate: cargo clippy --all-targets clean, cargo test --lib (1770), cargo test --test integration (987). sqlx offline unchanged (runtime queries only). No version bump, no deploy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-25 00:42 UTC
Commit: 63e6a806cb95631487f66e639e0d29bb70a7b2d5
Parent: b412e48
10 files changed, +283 insertions, -23 deletions
M server/src/db/blog_posts.rs +17 -7
@@ -247,15 +247,25 @@ pub async fn publish_scheduled_blog_posts(pool: &PgPool) -> Result<Vec<DbBlogPos
247 247 Ok(posts)
248 248 }
249 249
250 - /// Permanently delete a blog post by ID.
250 + /// Permanently delete a blog post owned by `owner_id`.
251 + ///
252 + /// Ownership is scoped IN the SQL (`project_id IN (SELECT id FROM projects WHERE
253 + /// user_id = $2)`) so the delete can't remove another user's post even if a
254 + /// caller skips the upstream ownership check (Sec-M2 defense in depth, mirroring
255 + /// `media_files::delete`). Returns `true` if a post was deleted, `false` if none
256 + /// matched for `owner_id`.
251 257 #[tracing::instrument(skip_all)]
252 - pub async fn delete_blog_post(pool: &PgPool, id: BlogPostId) -> Result<()> {
253 - sqlx::query("DELETE FROM blog_posts WHERE id = $1")
254 - .bind(id)
255 - .execute(pool)
256 - .await?;
258 + pub async fn delete_blog_post(pool: &PgPool, id: BlogPostId, owner_id: UserId) -> Result<bool> {
259 + let res = sqlx::query(
260 + "DELETE FROM blog_posts \
261 + WHERE id = $1 AND project_id IN (SELECT id FROM projects WHERE user_id = $2)",
262 + )
263 + .bind(id)
264 + .bind(owner_id)
265 + .execute(pool)
266 + .await?;
257 267
258 - Ok(())
268 + Ok(res.rows_affected() > 0)
259 269 }
260 270
261 271 /// Set the linked MT thread ID for a blog post.
M server/src/db/bundles.rs +21 -10
@@ -302,18 +302,29 @@ pub async fn is_bundle_member(pool: &PgPool, bundle_id: ItemId, child_id: ItemId
302 302 }
303 303
304 304 /// Set the `listed` flag on an item. UNSCOPED: takes no owner and updates by id
305 - /// alone, so every caller MUST have already verified the item belongs to the
306 - /// acting user (the bundle/project ownership check upstream) before calling — the
307 - /// flag write itself enforces no ownership (Sec-M2).
305 + /// alone. Ownership is scoped IN the SQL (`project_id IN (SELECT id FROM projects
306 + /// WHERE user_id = $3)`) so the toggle can't reach another user's item even if a
307 + /// caller skips the upstream bundle/project ownership check (Sec-M2 defense in
308 + /// depth, mirroring `media_files::delete`). Returns `true` if an owned item was
309 + /// updated, `false` if none matched for `owner_id`.
308 310 #[tracing::instrument(skip_all, fields(%item_id, listed))]
309 - pub async fn set_item_listed(pool: &PgPool, item_id: ItemId, listed: bool) -> Result<()> {
310 - sqlx::query("UPDATE items SET listed = $2 WHERE id = $1")
311 - .bind(item_id)
312 - .bind(listed)
313 - .execute(pool)
314 - .await?;
311 + pub async fn set_item_listed(
312 + pool: &PgPool,
313 + item_id: ItemId,
314 + listed: bool,
315 + owner_id: UserId,
316 + ) -> Result<bool> {
317 + let res = sqlx::query(
318 + "UPDATE items SET listed = $2 \
319 + WHERE id = $1 AND project_id IN (SELECT id FROM projects WHERE user_id = $3)",
320 + )
321 + .bind(item_id)
322 + .bind(listed)
323 + .bind(owner_id)
324 + .execute(pool)
325 + .await?;
315 326
316 - Ok(())
327 + Ok(res.rows_affected() > 0)
317 328 }
318 329
319 330 #[cfg(test)]
M server/src/db/mod.rs +1 -1
@@ -22,7 +22,7 @@ pub(crate) mod discover;
22 22 pub(crate) mod custom_links;
23 23 pub(crate) mod auth;
24 24 pub mod waitlist;
25 - pub(crate) mod blog_posts;
25 + pub mod blog_posts;
26 26 pub mod license_keys;
27 27 pub mod synckit; // pub so the integration test crate can exercise compaction directly
28 28 pub mod synckit_billing;
M server/src/routes/api/blog.rs +1 -1
@@ -259,7 +259,7 @@ pub(super) async fn delete_blog_post(
259 259 user.check_not_suspended()?;
260 260 let post = verify_blog_post_ownership(&state, id, user.id).await?;
261 261
262 - db::blog_posts::delete_blog_post(&state.db, id).await?;
262 + db::blog_posts::delete_blog_post(&state.db, id, user.id).await?;
263 263 db::projects::bump_cache_generation(&state.db, post.project_id).await?;
264 264
265 265 Ok(htmx_toast_response("Blog post deleted", "success"))
M server/src/routes/api/internal/content.rs +1 -1
@@ -197,7 +197,7 @@ pub(super) async fn delete_blog_post(
197 197 return Err(AppError::Forbidden);
198 198 }
199 199
200 - db::blog_posts::delete_blog_post(&state.db, post_id).await?;
200 + db::blog_posts::delete_blog_post(&state.db, post_id, query.user_id).await?;
201 201
202 202 tracing::info!(user = %query.user_id, post = %post_id, "blog post deleted via CLI");
203 203
M server/src/routes/api/items/bundles.rs +2 -2
@@ -87,7 +87,7 @@ pub async fn bundle_toggle_listed(
87 87 if !db::bundles::is_bundle_member(&state.db, bundle_id, child_id).await? {
88 88 return Err(AppError::NotFound);
89 89 }
90 - db::bundles::set_item_listed(&state.db, child_id, req.listed).await?;
90 + db::bundles::set_item_listed(&state.db, child_id, req.listed, user.id).await?;
91 91 Ok(StatusCode::OK)
92 92 }
93 93
@@ -137,7 +137,7 @@ pub async fn bundle_create_child(
137 137 // Add to bundle and set unlisted
138 138 let count = db::bundles::get_bundle_item_count(&state.db, bundle_id).await?;
139 139 db::bundles::add_item_to_bundle(&state.db, bundle_id, child.id, count as i32).await?;
140 - db::bundles::set_item_listed(&state.db, child.id, false).await?;
140 + db::bundles::set_item_listed(&state.db, child.id, false, user.id).await?;
141 141
142 142 // Publish the child so it's downloadable via the bundle
143 143 db::items::bulk_publish(&state.db, &[child.id], bundle.project_id, user.id).await?;
M server/src/routes/pages/dashboard/wizards/item/save.rs +1 -1
@@ -141,7 +141,7 @@ pub(super) async fn save_content(
141 141 let should_be_unlisted = unlisted_ids.contains(&bi.id);
142 142 if bi.listed == should_be_unlisted {
143 143 // listed=true but should be unlisted, or listed=false but shouldn't be
144 - db::bundles::set_item_listed(&state.db, bi.id, !should_be_unlisted).await?;
144 + db::bundles::set_item_listed(&state.db, bi.id, !should_be_unlisted, user_id).await?;
145 145 }
146 146 }
147 147 }
M server/src/scanning/content_type.rs +64
@@ -425,4 +425,68 @@ mod tests {
425 425 let path_based = verify_content_type_path(tmp.path(), FileType::Audio);
426 426 assert_eq!(buffered.verdict, path_based.verdict);
427 427 }
428 +
429 + // -- Markup / polyglot sniffing (sniff_html_like) --
430 + //
431 + // For every non-Download type, markup is caught up front regardless of the
432 + // per-type logic, and `infer`-unclassifiable text fails the media arms. The
433 + // one acknowledged gap is the binary-magic-prefixed polyglot (documented at
434 + // the module top): `infer` classifies it by its leading magic, so this layer
435 + // passes it and layers 2-5 are what catch it. These pin both behaviors.
436 +
437 + #[test]
438 + fn script_payload_fails_as_audio() {
439 + let data = b"<script>alert(1)</script>";
440 + assert_eq!(verify_content_type(data, FileType::Audio).verdict, LayerVerdict::Fail);
441 + }
442 +
443 + #[test]
444 + fn html_after_utf8_bom_fails_as_cover() {
445 + // BOM-prefixed markup must still sniff as markup (the BOM is skipped).
446 + let mut data = vec![0xEF, 0xBB, 0xBF];
447 + data.extend_from_slice(b"<html><body>x</body></html>");
448 + assert_eq!(verify_content_type(&data, FileType::Cover).verdict, LayerVerdict::Fail);
449 + }
450 +
451 + #[test]
452 + fn html_after_leading_whitespace_fails_as_video() {
453 + // Leading whitespace is trimmed before the window scan, so padding the
454 + // front with newlines/spaces does not evade the markup sniff.
455 + let data = b"\n\n\t <html><head></head></html>";
456 + assert_eq!(verify_content_type(data, FileType::Video).verdict, LayerVerdict::Fail);
457 + }
458 +
459 + #[test]
460 + fn svg_payload_fails_as_image() {
461 + let data = b"<svg xmlns=\"http://www.w3.org/2000/svg\"><script>x</script></svg>";
462 + assert_eq!(verify_content_type(data, FileType::MediaImage).verdict, LayerVerdict::Fail);
463 + }
464 +
465 + #[test]
466 + fn text_with_markup_past_sniff_window_still_fails_media() {
467 + // Non-whitespace padding pushes the <script> past the 256-byte sniff
468 + // window, so the markup sniff misses it — but the payload is plain text,
469 + // so `infer` returns None and the audio arm rejects it anyway. The file
470 + // never passes as media regardless of which guard fires.
471 + let mut data = vec![b'A'; 300];
472 + data.extend_from_slice(b"<script>alert(1)</script>");
473 + assert_eq!(verify_content_type(&data, FileType::Audio).verdict, LayerVerdict::Fail);
474 + }
475 +
476 + #[test]
477 + fn png_prefixed_polyglot_past_head_passes_content_type_layer() {
478 + // The acknowledged blind spot: valid PNG magic, then enough binary
479 + // padding to push the script tail past the 1024-byte head the sniff
480 + // inspects. `infer` classifies the file as image/png by its leading
481 + // magic and the sniff never sees the markup, so THIS layer passes it.
482 + // The malicious tail is caught by the structural/archive/YARA/ClamAV
483 + // layers, not here. Pinning this documents the boundary so a future
484 + // change that "tightens" content_type doesn't mistakenly assume it is
485 + // the polyglot defense. (Markup WITHIN the head is caught — see
486 + // `script_payload_fails_as_audio`.)
487 + let mut data = vec![0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A];
488 + data.extend(std::iter::repeat_n(0u8, 1200));
489 + data.extend_from_slice(b"<script>alert(1)</script>");
490 + assert_eq!(verify_content_type(&data, FileType::Cover).verdict, LayerVerdict::Pass);
491 + }
428 492 }
M server/tests/workflows/mod.rs +1
@@ -96,3 +96,4 @@ mod cart;
96 96 mod bundles;
97 97 mod idempotency;
98 98 mod db_payments_layer;
99 + mod security_idor_moderation;
A server/tests/workflows/security_idor_moderation.rs +174
@@ -0,0 +1,174 @@
1 + //! Security A+ (ultra-fuzz Run 7): owner-scoping (Sec-M2 defense in depth) on
2 + //! the guard-dependent mutating helpers, plus coverage for the previously
3 + //! untested admin moderation handlers.
4 +
5 + use crate::harness::TestHarness;
6 + use makenotwork::db;
7 +
8 + // ── Owner-scoped mutations (Sec-M2): the SQL itself rejects cross-owner writes ──
9 +
10 + async fn creator_with_project(h: &TestHarness, tag: &str) -> (db::UserId, db::ProjectId) {
11 + let user: db::UserId = sqlx::query_scalar(
12 + "INSERT INTO users (username, email, password_hash, email_verified) \
13 + VALUES ($1, $2, 'x', true) RETURNING id",
14 + )
15 + .bind(format!("idor_{tag}"))
16 + .bind(format!("idor_{tag}@test.com"))
17 + .fetch_one(&h.db)
18 + .await
19 + .unwrap();
20 + let project: db::ProjectId = sqlx::query_scalar(
21 + "INSERT INTO projects (user_id, slug, title) VALUES ($1, $2, 'P') RETURNING id",
22 + )
23 + .bind(user)
24 + .bind(format!("idorproj_{tag}"))
25 + .fetch_one(&h.db)
26 + .await
27 + .unwrap();
28 + (user, project)
29 + }
30 +
31 + #[tokio::test]
32 + async fn delete_blog_post_is_owner_scoped() {
33 + let h = TestHarness::new().await;
34 + let (owner_a, _proj_a) = creator_with_project(&h, "blog_a").await;
35 + let (owner_b, proj_b) = creator_with_project(&h, "blog_b").await;
36 +
37 + let post_b: db::BlogPostId = sqlx::query_scalar(
38 + "INSERT INTO blog_posts (project_id, author_id, title, slug) \
39 + VALUES ($1, $2, 'B post', 'b-post') RETURNING id",
40 + )
41 + .bind(proj_b)
42 + .bind(owner_b)
43 + .fetch_one(&h.db)
44 + .await
45 + .unwrap();
46 +
47 + // Owner A cannot delete owner B's post — the SQL scope matches no row.
48 + let cross = db::blog_posts::delete_blog_post(&h.db, post_b, owner_a).await.unwrap();
49 + assert!(!cross, "a cross-owner delete must report no row deleted");
50 + let still_there: i64 =
51 + sqlx::query_scalar("SELECT COUNT(*) FROM blog_posts WHERE id = $1")
52 + .bind(post_b)
53 + .fetch_one(&h.db)
54 + .await
55 + .unwrap();
56 + assert_eq!(still_there, 1, "the post must survive a cross-owner delete attempt");
57 +
58 + // The real owner deletes it.
59 + let owned = db::blog_posts::delete_blog_post(&h.db, post_b, owner_b).await.unwrap();
60 + assert!(owned, "the owner's delete must succeed");
61 + }
62 +
63 + #[tokio::test]
64 + async fn set_item_listed_is_owner_scoped() {
65 + let h = TestHarness::new().await;
66 + let (owner_a, _proj_a) = creator_with_project(&h, "item_a").await;
67 + let (owner_b, proj_b) = creator_with_project(&h, "item_b").await;
68 +
69 + let item_b: db::ItemId = sqlx::query_scalar(
70 + "INSERT INTO items (project_id, title, item_type, price_cents, slug, listed) \
71 + VALUES ($1, 'B item', 'digital', 0, 'b-item', true) RETURNING id",
72 + )
73 + .bind(proj_b)
74 + .fetch_one(&h.db)
75 + .await
76 + .unwrap();
77 +
78 + // Owner A cannot toggle owner B's item.
79 + let cross = db::bundles::set_item_listed(&h.db, item_b, false, owner_a).await.unwrap();
80 + assert!(!cross, "a cross-owner listed-toggle must report no row updated");
81 + let listed: bool = sqlx::query_scalar("SELECT listed FROM items WHERE id = $1")
82 + .bind(item_b)
83 + .fetch_one(&h.db)
84 + .await
85 + .unwrap();
86 + assert!(listed, "the item's listed flag must be unchanged by a cross-owner attempt");
87 +
88 + // The real owner can.
89 + let owned = db::bundles::set_item_listed(&h.db, item_b, false, owner_b).await.unwrap();
90 + assert!(owned, "the owner's toggle must succeed");
91 + }
92 +
93 + // ── Admin moderation handlers (previously uncovered) ──
94 +
95 + #[tokio::test]
96 + async fn admin_remove_and_restore_item() {
97 + let (mut h, _admin) = TestHarness::with_admin().await;
98 + let setup = h.create_creator_with_item("modtarget", "text", 0).await;
99 + h.publish_project_and_item(&setup.project_id, &setup.item_id).await;
100 +
101 + h.client.post_form("/logout", "").await;
102 + h.login("admin", "password123").await;
103 +
104 + let resp = h
105 + .client
106 + .post_form(
107 + &format!("/api/admin/items/{}/remove", setup.item_id),
108 + "reason=Infringing+content",
109 + )
110 + .await;
111 + assert!(resp.status.is_success(), "remove failed: {} {}", resp.status, resp.text);
112 +
113 + let (removed, public): (bool, bool) =
114 + sqlx::query_as("SELECT removed_by_admin, is_public FROM items WHERE id = $1::uuid")
115 + .bind(&setup.item_id)
116 + .fetch_one(&h.db)
117 + .await
118 + .unwrap();
119 + assert!(removed, "item must be flagged removed_by_admin");
120 + assert!(!public, "removed item must be hidden");
121 +
122 + let resp = h
123 + .client
124 + .post_form(&format!("/api/admin/items/{}/restore", setup.item_id), "")
125 + .await;
126 + assert!(resp.status.is_success(), "restore failed: {} {}", resp.status, resp.text);
127 +
128 + let removed_after: bool =
129 + sqlx::query_scalar("SELECT removed_by_admin FROM items WHERE id = $1::uuid")
130 + .bind(&setup.item_id)
131 + .fetch_one(&h.db)
132 + .await
133 + .unwrap();
134 + assert!(!removed_after, "restore must clear removed_by_admin");
135 + }
136 +
137 + #[tokio::test]
138 + async fn admin_decide_appeal_denied_records_decision() {
139 + let (mut h, _admin) = TestHarness::with_admin().await;
140 + let target = h.signup("appellant", "appellant@test.com", "password123").await;
141 +
142 + // Suspend the user and have them submit an appeal.
143 + sqlx::query("UPDATE users SET suspended_at = NOW(), suspension_reason = 'spam' WHERE id = $1")
144 + .bind(target)
145 + .execute(&h.db)
146 + .await
147 + .unwrap();
148 + db::users::submit_appeal(&h.db, target, "Please reconsider").await.unwrap();
149 +
150 + let pending = db::users::get_pending_appeals(&h.db).await.unwrap();
151 + assert!(pending.iter().any(|u| u.id == target), "appeal must be pending before decision");
152 +
153 + h.client.post_form("/logout", "").await;
154 + h.login("admin", "password123").await;
155 + let resp = h
156 + .client
157 + .post_form(
158 + &format!("/api/admin/appeals/{target}/decide"),
159 + "decision=denied&response=Upheld+after+review",
160 + )
161 + .await;
162 + assert!(resp.status.is_success(), "decide failed: {} {}", resp.status, resp.text);
163 +
164 + // Decided appeals drop out of the pending queue; denial keeps the suspension.
165 + let pending_after = db::users::get_pending_appeals(&h.db).await.unwrap();
166 + assert!(!pending_after.iter().any(|u| u.id == target), "decided appeal must leave the queue");
167 + let still_suspended: bool =
168 + sqlx::query_scalar("SELECT suspended_at IS NOT NULL FROM users WHERE id = $1")
169 + .bind(target)
170 + .fetch_one(&h.db)
171 + .await
172 + .unwrap();
173 + assert!(still_suspended, "a denied appeal must keep the suspension");
174 + }