max / makenotwork
5 files changed,
+254 insertions,
-47 deletions
| @@ -34,6 +34,36 @@ pub async fn get_by_id(pool: &PgPool, section_id: ItemSectionId) -> Result<Optio | |||
| 34 | 34 | ||
| 35 | 35 | /// Insert a new section for an item. | |
| 36 | 36 | #[tracing::instrument(skip_all)] | |
| 37 | + | /// Whether another section on the same item already uses `slug`. | |
| 38 | + | /// | |
| 39 | + | /// `exclude_id` lets an update skip the row being edited (so renaming a section | |
| 40 | + | /// to a slug it already owns isn't a self-conflict). Backs the slug-dedup loop | |
| 41 | + | /// in the create/update handlers; the `UNIQUE(item_id, slug)` index remains the | |
| 42 | + | /// race-safe source of truth. | |
| 43 | + | #[tracing::instrument(skip_all)] | |
| 44 | + | pub async fn slug_conflicts( | |
| 45 | + | pool: &PgPool, | |
| 46 | + | item_id: ItemId, | |
| 47 | + | slug: &str, | |
| 48 | + | exclude_id: Option<ItemSectionId>, | |
| 49 | + | ) -> Result<bool> { | |
| 50 | + | let exists: bool = sqlx::query_scalar( | |
| 51 | + | r#" | |
| 52 | + | SELECT EXISTS( | |
| 53 | + | SELECT 1 FROM item_sections | |
| 54 | + | WHERE item_id = $1 AND slug = $2 | |
| 55 | + | AND ($3::uuid IS NULL OR id <> $3) | |
| 56 | + | ) | |
| 57 | + | "#, | |
| 58 | + | ) | |
| 59 | + | .bind(item_id) | |
| 60 | + | .bind(slug) | |
| 61 | + | .bind(exclude_id) | |
| 62 | + | .fetch_one(pool) | |
| 63 | + | .await?; | |
| 64 | + | Ok(exists) | |
| 65 | + | } | |
| 66 | + | ||
| 37 | 67 | pub async fn create( | |
| 38 | 68 | pool: &PgPool, | |
| 39 | 69 | item_id: ItemId, |
| @@ -32,6 +32,35 @@ pub async fn get_by_id(pool: &PgPool, section_id: ProjectSectionId) -> Result<Op | |||
| 32 | 32 | Ok(section) | |
| 33 | 33 | } | |
| 34 | 34 | ||
| 35 | + | /// Whether another section on the same project already uses `slug`. | |
| 36 | + | /// | |
| 37 | + | /// `exclude_id` lets an update skip the row being edited. Backs the slug-dedup | |
| 38 | + | /// loop in the create/update handlers; the `UNIQUE(project_id, slug)` index | |
| 39 | + | /// remains the race-safe source of truth. | |
| 40 | + | #[tracing::instrument(skip_all)] | |
| 41 | + | pub async fn slug_conflicts( | |
| 42 | + | pool: &PgPool, | |
| 43 | + | project_id: ProjectId, | |
| 44 | + | slug: &str, | |
| 45 | + | exclude_id: Option<ProjectSectionId>, | |
| 46 | + | ) -> Result<bool> { | |
| 47 | + | let exists: bool = sqlx::query_scalar( | |
| 48 | + | r#" | |
| 49 | + | SELECT EXISTS( | |
| 50 | + | SELECT 1 FROM project_sections | |
| 51 | + | WHERE project_id = $1 AND slug = $2 | |
| 52 | + | AND ($3::uuid IS NULL OR id <> $3) | |
| 53 | + | ) | |
| 54 | + | "#, | |
| 55 | + | ) | |
| 56 | + | .bind(project_id) | |
| 57 | + | .bind(slug) | |
| 58 | + | .bind(exclude_id) | |
| 59 | + | .fetch_one(pool) | |
| 60 | + | .await?; | |
| 61 | + | Ok(exists) | |
| 62 | + | } | |
| 63 | + | ||
| 35 | 64 | /// Insert a new section for a project. | |
| 36 | 65 | #[tracing::instrument(skip_all)] | |
| 37 | 66 | pub async fn create( |
| @@ -113,11 +113,34 @@ pub fn slugify(title: &str) -> crate::db::Slug { | |||
| 113 | 113 | } | |
| 114 | 114 | } | |
| 115 | 115 | if result.len() < 2 { | |
| 116 | - | result = "post".to_string(); | |
| 116 | + | // The ASCII-only pass kept nothing usable — typically an all-non-Latin | |
| 117 | + | // title (CJK/Cyrillic/Arabic/emoji). A constant "post" fallback made | |
| 118 | + | // every such title collide on one slug, so a creator working in a | |
| 119 | + | // non-Latin script hit a unique-violation on their second section and | |
| 120 | + | // every blog post URL became `/post`. Derive a stable suffix from the | |
| 121 | + | // title content so distinct titles get distinct slugs (hash-fallback; | |
| 122 | + | // no transliteration dependency). | |
| 123 | + | result = format!("post-{}", title_hash_suffix(title)); | |
| 117 | 124 | } | |
| 118 | 125 | crate::db::Slug::from_trusted(result) | |
| 119 | 126 | } | |
| 120 | 127 | ||
| 128 | + | /// Short, stable, URL-safe suffix derived from a title's content. Used as the | |
| 129 | + | /// slug fallback when ASCII slugification yields nothing usable, so two | |
| 130 | + | /// different non-Latin titles produce two different slugs instead of colliding | |
| 131 | + | /// on a constant. Deterministic: the same title always yields the same suffix. | |
| 132 | + | fn title_hash_suffix(title: &str) -> String { | |
| 133 | + | use sha2::{Digest, Sha256}; | |
| 134 | + | let digest = Sha256::digest(title.trim().as_bytes()); | |
| 135 | + | // 4 bytes -> 8 lowercase hex chars; ample to separate a creator's titles, | |
| 136 | + | // and the section/blog dedup loop handles the astronomically rare clash. | |
| 137 | + | let mut suffix = String::with_capacity(8); | |
| 138 | + | for b in &digest[..4] { | |
| 139 | + | suffix.push_str(&format!("{:02x}", b)); | |
| 140 | + | } | |
| 141 | + | suffix | |
| 142 | + | } | |
| 143 | + | ||
| 121 | 144 | /// Sanitize a string for use as a CSV cell value. | |
| 122 | 145 | /// | |
| 123 | 146 | /// Prevents CSV injection by quoting cells and escaping values that start | |
| @@ -376,9 +399,31 @@ mod tests { | |||
| 376 | 399 | ||
| 377 | 400 | #[test] | |
| 378 | 401 | fn slugify_too_short_falls_back() { | |
| 379 | - | assert_eq!(slugify("a").as_str(), "post"); | |
| 380 | - | assert_eq!(slugify("").as_str(), "post"); | |
| 381 | - | assert_eq!(slugify("---").as_str(), "post"); | |
| 402 | + | // Fallback is now `post-<hash>` (was a constant "post"): still a valid | |
| 403 | + | // slug, deterministic per input, and distinct across inputs. | |
| 404 | + | for input in ["a", "", "---"] { | |
| 405 | + | let s = slugify(input); | |
| 406 | + | assert!(s.as_str().starts_with("post-"), "got {}", s.as_str()); | |
| 407 | + | assert!(crate::validation::validate_slug(s.as_str()).is_ok()); | |
| 408 | + | // Deterministic. | |
| 409 | + | assert_eq!(slugify(input).as_str(), s.as_str()); | |
| 410 | + | } | |
| 411 | + | // Distinct inputs -> distinct fallback slugs (the bug this fixes). | |
| 412 | + | assert_ne!(slugify("a").as_str(), slugify("---").as_str()); | |
| 413 | + | } | |
| 414 | + | ||
| 415 | + | #[test] | |
| 416 | + | fn slugify_non_latin_titles_are_distinct() { | |
| 417 | + | // The whole point of the hash-fallback: two different non-Latin titles | |
| 418 | + | // must not collide on one slug (which caused a unique-violation 500 for | |
| 419 | + | // the second section a non-Latin creator added). | |
| 420 | + | let a = slugify("日本語の記事"); | |
| 421 | + | let b = slugify("Статья на русском"); | |
| 422 | + | assert!(a.as_str().starts_with("post-")); | |
| 423 | + | assert!(b.as_str().starts_with("post-")); | |
| 424 | + | assert_ne!(a.as_str(), b.as_str()); | |
| 425 | + | assert!(crate::validation::validate_slug(a.as_str()).is_ok()); | |
| 426 | + | assert!(crate::validation::validate_slug(b.as_str()).is_ok()); | |
| 382 | 427 | } | |
| 383 | 428 | ||
| 384 | 429 | #[test] | |
| @@ -388,12 +433,16 @@ mod tests { | |||
| 388 | 433 | ||
| 389 | 434 | #[test] | |
| 390 | 435 | fn slugify_all_special_chars() { | |
| 391 | - | assert_eq!(slugify("!@#$%^&*()").as_str(), "post"); | |
| 436 | + | let s = slugify("!@#$%^&*()"); | |
| 437 | + | assert!(s.as_str().starts_with("post-")); | |
| 438 | + | assert!(crate::validation::validate_slug(s.as_str()).is_ok()); | |
| 392 | 439 | } | |
| 393 | 440 | ||
| 394 | 441 | #[test] | |
| 395 | 442 | fn slugify_single_valid_char() { | |
| 396 | - | assert_eq!(slugify("x").as_str(), "post"); | |
| 443 | + | let s = slugify("x"); | |
| 444 | + | assert!(s.as_str().starts_with("post-")); | |
| 445 | + | assert!(crate::validation::validate_slug(s.as_str()).is_ok()); | |
| 397 | 446 | } | |
| 398 | 447 | ||
| 399 | 448 | #[test] | |
| @@ -517,10 +566,11 @@ mod tests { | |||
| 517 | 566 | ||
| 518 | 567 | #[test] | |
| 519 | 568 | fn slugify_emoji_input() { | |
| 520 | - | // Emoji are non-ASCII → become hyphens → collapsed | |
| 569 | + | // Emoji are non-ASCII → become hyphens → collapsed to nothing, so the | |
| 570 | + | // hash-fallback kicks in (was a constant "post"). | |
| 521 | 571 | let slug = slugify("\u{1f600}\u{1f600}\u{1f600}"); | |
| 522 | - | // All chars become hyphens, collapsed to nothing, falls back to "post" | |
| 523 | - | assert_eq!(slug.as_str(), "post"); | |
| 572 | + | assert!(slug.as_str().starts_with("post-")); | |
| 573 | + | assert!(crate::validation::validate_slug(slug.as_str()).is_ok()); | |
| 524 | 574 | } | |
| 525 | 575 | ||
| 526 | 576 | #[test] |
| @@ -93,24 +93,56 @@ pub(in crate::routes::api) async fn create_section( | |||
| 93 | 93 | ))); | |
| 94 | 94 | } | |
| 95 | 95 | ||
| 96 | - | let slug = slugify(&title).to_string(); | |
| 97 | 96 | let sort_order = count as i32; | |
| 98 | 97 | ||
| 99 | - | let section = db::item_sections::create( | |
| 100 | - | &state.db, | |
| 101 | - | item_id, | |
| 102 | - | &title, | |
| 103 | - | &slug, | |
| 104 | - | &req.body, | |
| 105 | - | sort_order, | |
| 106 | - | ) | |
| 107 | - | .await?; | |
| 98 | + | // Resolve a collision-free slug. Two sections whose titles slugify to the | |
| 99 | + | // same value (e.g. "Intro" twice, or any non-Latin titles sharing the | |
| 100 | + | // hash-fallback base) must get an auto-suffixed slug rather than a raw 500 | |
| 101 | + | // from the UNIQUE(item_id, slug) index (ultra-fuzz Run #1 UX). The index | |
| 102 | + | // stays the race-safe source of truth via the 23505 retry below. | |
| 103 | + | let base = slugify(&title).to_string(); | |
| 104 | + | let mut slug = base.clone(); | |
| 105 | + | let mut suffix = 2u32; | |
| 106 | + | while db::item_sections::slug_conflicts(&state.db, item_id, &slug, None).await? { | |
| 107 | + | slug = format!("{base}-{suffix}"); | |
| 108 | + | suffix += 1; | |
| 109 | + | } | |
| 110 | + | ||
| 111 | + | let section = loop { | |
| 112 | + | match db::item_sections::create( | |
| 113 | + | &state.db, | |
| 114 | + | item_id, | |
| 115 | + | &title, | |
| 116 | + | &slug, | |
| 117 | + | &req.body, | |
| 118 | + | sort_order, | |
| 119 | + | ) | |
| 120 | + | .await | |
| 121 | + | { | |
| 122 | + | Ok(section) => break section, | |
| 123 | + | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 124 | + | slug = format!("{base}-{suffix}"); | |
| 125 | + | suffix += 1; | |
| 126 | + | } | |
| 127 | + | Err(e) => return Err(e), | |
| 128 | + | } | |
| 129 | + | }; | |
| 108 | 130 | ||
| 109 | 131 | db::projects::bump_cache_generation(&state.db, item.project_id).await?; | |
| 110 | 132 | ||
| 111 | 133 | Ok(Json(SectionResponse::from(section))) | |
| 112 | 134 | } | |
| 113 | 135 | ||
| 136 | + | /// Whether an error is a Postgres unique-violation (slug collision on the | |
| 137 | + | /// `UNIQUE(item_id, slug)` index) — the TOCTOU backstop for the dedup loops. | |
| 138 | + | fn is_slug_conflict(e: &AppError) -> bool { | |
| 139 | + | matches!( | |
| 140 | + | e, | |
| 141 | + | AppError::Database(sqlx::Error::Database(db_err)) | |
| 142 | + | if db_err.code().as_deref() == Some("23505") | |
| 143 | + | ) | |
| 144 | + | } | |
| 145 | + | ||
| 114 | 146 | /// List all sections for a given item (public items only; drafts return 404). | |
| 115 | 147 | #[tracing::instrument(skip_all, name = "items::list_sections")] | |
| 116 | 148 | pub(in crate::routes::api) async fn list_sections( | |
| @@ -147,16 +179,34 @@ pub(in crate::routes::api) async fn update_section( | |||
| 147 | 179 | ||
| 148 | 180 | let (item, _) = verify_item_ownership(&state, section.item_id, user.id).await?; | |
| 149 | 181 | ||
| 150 | - | let slug = slugify(&title).to_string(); | |
| 182 | + | // Same dedup as create, but exclude this row so renaming a section to a | |
| 183 | + | // slug it already owns isn't a self-conflict. | |
| 184 | + | let base = slugify(&title).to_string(); | |
| 185 | + | let mut slug = base.clone(); | |
| 186 | + | let mut suffix = 2u32; | |
| 187 | + | while db::item_sections::slug_conflicts(&state.db, section.item_id, &slug, Some(section_id)).await? { | |
| 188 | + | slug = format!("{base}-{suffix}"); | |
| 189 | + | suffix += 1; | |
| 190 | + | } | |
| 151 | 191 | ||
| 152 | - | let updated = db::item_sections::update( | |
| 153 | - | &state.db, | |
| 154 | - | section_id, | |
| 155 | - | &title, | |
| 156 | - | &slug, | |
| 157 | - | &req.body, | |
| 158 | - | ) | |
| 159 | - | .await?; | |
| 192 | + | let updated = loop { | |
| 193 | + | match db::item_sections::update( | |
| 194 | + | &state.db, | |
| 195 | + | section_id, | |
| 196 | + | &title, | |
| 197 | + | &slug, | |
| 198 | + | &req.body, | |
| 199 | + | ) | |
| 200 | + | .await | |
| 201 | + | { | |
| 202 | + | Ok(updated) => break updated, | |
| 203 | + | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 204 | + | slug = format!("{base}-{suffix}"); | |
| 205 | + | suffix += 1; | |
| 206 | + | } | |
| 207 | + | Err(e) => return Err(e), | |
| 208 | + | } | |
| 209 | + | }; | |
| 160 | 210 | ||
| 161 | 211 | db::projects::bump_cache_generation(&state.db, item.project_id).await?; | |
| 162 | 212 |
| @@ -88,24 +88,54 @@ pub(super) async fn create_section( | |||
| 88 | 88 | ))); | |
| 89 | 89 | } | |
| 90 | 90 | ||
| 91 | - | let slug = slugify(&title).to_string(); | |
| 92 | 91 | let sort_order = count as i32; | |
| 93 | 92 | ||
| 94 | - | let section = db::project_sections::create( | |
| 95 | - | &state.db, | |
| 96 | - | project_id, | |
| 97 | - | &title, | |
| 98 | - | &slug, | |
| 99 | - | &req.body, | |
| 100 | - | sort_order, | |
| 101 | - | ) | |
| 102 | - | .await?; | |
| 93 | + | // Auto-suffix on slug collision instead of surfacing the UNIQUE(project_id, | |
| 94 | + | // slug) violation as a raw 500 (ultra-fuzz Run #1 UX). The index stays the | |
| 95 | + | // race-safe source of truth via the 23505 retry below. | |
| 96 | + | let base = slugify(&title).to_string(); | |
| 97 | + | let mut slug = base.clone(); | |
| 98 | + | let mut suffix = 2u32; | |
| 99 | + | while db::project_sections::slug_conflicts(&state.db, project_id, &slug, None).await? { | |
| 100 | + | slug = format!("{base}-{suffix}"); | |
| 101 | + | suffix += 1; | |
| 102 | + | } | |
| 103 | + | ||
| 104 | + | let section = loop { | |
| 105 | + | match db::project_sections::create( | |
| 106 | + | &state.db, | |
| 107 | + | project_id, | |
| 108 | + | &title, | |
| 109 | + | &slug, | |
| 110 | + | &req.body, | |
| 111 | + | sort_order, | |
| 112 | + | ) | |
| 113 | + | .await | |
| 114 | + | { | |
| 115 | + | Ok(section) => break section, | |
| 116 | + | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 117 | + | slug = format!("{base}-{suffix}"); | |
| 118 | + | suffix += 1; | |
| 119 | + | } | |
| 120 | + | Err(e) => return Err(e), | |
| 121 | + | } | |
| 122 | + | }; | |
| 103 | 123 | ||
| 104 | 124 | db::projects::bump_cache_generation(&state.db, project_id).await?; | |
| 105 | 125 | ||
| 106 | 126 | Ok(Json(SectionResponse::from(section))) | |
| 107 | 127 | } | |
| 108 | 128 | ||
| 129 | + | /// Whether an error is a Postgres unique-violation (slug collision on the | |
| 130 | + | /// `UNIQUE(project_id, slug)` index) — the TOCTOU backstop for the dedup loops. | |
| 131 | + | fn is_slug_conflict(e: &AppError) -> bool { | |
| 132 | + | matches!( | |
| 133 | + | e, | |
| 134 | + | AppError::Database(sqlx::Error::Database(db_err)) | |
| 135 | + | if db_err.code().as_deref() == Some("23505") | |
| 136 | + | ) | |
| 137 | + | } | |
| 138 | + | ||
| 109 | 139 | #[tracing::instrument(skip_all, name = "projects::list_sections")] | |
| 110 | 140 | pub(super) async fn list_sections( | |
| 111 | 141 | State(state): State<AppState>, | |
| @@ -140,16 +170,34 @@ pub(super) async fn update_section( | |||
| 140 | 170 | ||
| 141 | 171 | verify_project_ownership(&state, section.project_id, user.id).await?; | |
| 142 | 172 | ||
| 143 | - | let slug = slugify(&title).to_string(); | |
| 173 | + | // Same dedup as create, excluding this row so a no-op rename isn't a | |
| 174 | + | // self-conflict. | |
| 175 | + | let base = slugify(&title).to_string(); | |
| 176 | + | let mut slug = base.clone(); | |
| 177 | + | let mut suffix = 2u32; | |
| 178 | + | while db::project_sections::slug_conflicts(&state.db, section.project_id, &slug, Some(section_id)).await? { | |
| 179 | + | slug = format!("{base}-{suffix}"); | |
| 180 | + | suffix += 1; | |
| 181 | + | } | |
| 144 | 182 | ||
| 145 | - | let updated = db::project_sections::update( | |
| 146 | - | &state.db, | |
| 147 | - | section_id, | |
| 148 | - | &title, | |
| 149 | - | &slug, | |
| 150 | - | &req.body, | |
| 151 | - | ) | |
| 152 | - | .await?; | |
| 183 | + | let updated = loop { | |
| 184 | + | match db::project_sections::update( | |
| 185 | + | &state.db, | |
| 186 | + | section_id, | |
| 187 | + | &title, | |
| 188 | + | &slug, | |
| 189 | + | &req.body, | |
| 190 | + | ) | |
| 191 | + | .await | |
| 192 | + | { | |
| 193 | + | Ok(updated) => break updated, | |
| 194 | + | Err(e) if is_slug_conflict(&e) && suffix < 100 => { | |
| 195 | + | slug = format!("{base}-{suffix}"); | |
| 196 | + | suffix += 1; | |
| 197 | + | } | |
| 198 | + | Err(e) => return Err(e), | |
| 199 | + | } | |
| 200 | + | }; | |
| 153 | 201 | ||
| 154 | 202 | db::projects::bump_cache_generation(&state.db, section.project_id).await?; | |
| 155 | 203 |