fix(security): address remaining review findings

Follow-up to #1. These were raised in the final CodeRabbit review, which I merged
without reading — the automation I used watched inline-comment counts and missed
that the findings were in the review body.

- `validate_channel_against_youtube` took `&mut Channel` but dropped the
  validated result, so the client-supplied channel name was persisted instead of
  the canonical one from the feed. Upstream behaved the same way, but the
  refactor in #1 left the two paths inconsistent, since the video path does
  assign it back. Names are only fuzzily matched before being accepted, and the
  channel table is shared between accounts.
- `validate_videos_against_youtube` zipped two slices whose equal length was
  documented but unenforced. `zip` truncates, so a short plan would silently skip
  RSS validation for the tail. No caller does that today, but it gates metadata
  validation and is the same failure mode as the dedup bypass fixed in #1.
- `verify_image_url` checked the host but not the scheme, so `http://ytimg.com/…`
  was accepted and handed to clients to load. Now requires https.
- Single `subscribe` only pre-checked the quota, outside any transaction, so
  concurrent requests could each see an under-quota count and together exceed it.
  Now matches the bulk path.
- `add_to_playlist` checked ownership before the RSS round-trips and never again,
  so a playlist deleted in that window surfaced as an opaque database error rather
  than PlaylistNotExists. Re-checked inside the transaction.
- `add_to_watch_history` answered `200 null` if the stored batch came back empty.
- `exceeds_row_quota` no longer takes an `incoming` count: every caller passes 0
  now that the quota is enforced on rows that actually exist.

Author: D3SOX

src/database/quota.rs

@@ -62,14 +62,12 @@ pub async fn count_playback_speeds(
         .await
 }
 
-/// Whether storing `incoming` further rows would exceed the per-table quota.
+/// Whether an account holds more rows than the per-table quota allows.
 ///
-/// `incoming` is an upper bound: some of those rows may be updates to existing
-/// ones, so this can reject slightly early at the very top of the range.
-pub fn exceeds_row_quota(stored_rows: i64, incoming: usize) -> bool {
-    let incoming = i64::try_from(incoming).unwrap_or(i64::MAX);
-
-    stored_rows.saturating_add(incoming) > MAX_ROWS_PER_ACCOUNT
+/// Callers check the rows that actually exist rather than predicting how many a
+/// batch will add, because every bulk write path is an upsert.
+pub fn exceeds_row_quota(stored_rows: i64) -> bool {
+    stored_rows > MAX_ROWS_PER_ACCOUNT
 }
 
 #[cfg(test)]
@@ -78,14 +76,10 @@ mod tests {
 
     #[test]
     fn quota_allows_up_to_the_limit() {
-        assert!(!exceeds_row_quota(0, 1));
-        assert!(!exceeds_row_quota(MAX_ROWS_PER_ACCOUNT - 1, 1));
-        assert!(exceeds_row_quota(MAX_ROWS_PER_ACCOUNT, 1));
-    }
-
-    #[test]
-    fn quota_does_not_overflow_on_absurd_batches() {
-        assert!(exceeds_row_quota(i64::MAX, usize::MAX));
-        assert!(exceeds_row_quota(0, usize::MAX));
+        assert!(!exceeds_row_quota(0));
+        assert!(!exceeds_row_quota(MAX_ROWS_PER_ACCOUNT - 1));
+        assert!(!exceeds_row_quota(MAX_ROWS_PER_ACCOUNT));
+        assert!(exceeds_row_quota(MAX_ROWS_PER_ACCOUNT + 1));
+        assert!(exceeds_row_quota(i64::MAX));
     }
 }

src/handlers.rs

@@ -142,7 +142,7 @@ pub fn check_already_over_quota(stored_rows: i64) -> HandlerResult<()> {
 /// incoming entries were updates rather than inserts. Returning an error from
 /// inside the transaction rolls the writes back.
 pub fn check_stored_rows(stored_rows: i64) -> HandlerResult<()> {
-    if crate::database::quota::exceeds_row_quota(stored_rows, 0) {
+    if crate::database::quota::exceeds_row_quota(stored_rows) {
         return Err(HandlerError::StorageQuotaExceeded);
     }
 

src/handlers/playlists.rs

@@ -244,6 +244,12 @@ async fn add_to_playlist(
     conn.transaction::<_, HandlerError, _>(|conn| {
         let (groups, playlist_id, account_id) = (&groups, &*playlist_id, &account.id);
         async move {
+            // Re-check ownership: the earlier check happened before the RSS
+            // round-trips, which can take seconds, so the playlist may have been
+            // deleted since. Without this the inserts fail as an opaque database
+            // error instead of PlaylistNotExists.
+            get_owned_playlist_or_error(conn, playlist_id, account_id).await?;
+
             for (channel, videos) in groups {
                 // store channel information first before storing video to ensure data integrity
                 create_or_update_channel(conn, channel)

src/handlers/subscriptions.rs

@@ -167,12 +167,25 @@ async fn subscribe(
     }
 
     let mut conn = get_db_conn!(pool);
-    match add_subscription_by_account_id(&mut conn, &channel, &account.id).await {
-        Ok(_) => Ok(HttpResponse::Ok()),
-        Err(err) => Err(HandlerError::InternalDatabaseErrorWithContext(
-            err.to_string(),
-        )),
-    }
+    // Mirrors the bulk path: without an authoritative post-write check inside a
+    // transaction, concurrent single subscribes each see an under-quota count and
+    // can together push the account past the limit.
+    conn.transaction::<_, HandlerError, _>(|conn| {
+        let (channel, account_id) = (&channel, &account.id);
+        async move {
+            add_subscription_by_account_id(conn, channel, account_id).await?;
+            check_stored_rows(
+                count_subscriptions(conn, account_id)
+                    .await
+                    .map_err(|_| HandlerError::InternalDatabaseError)?,
+            )?;
+            Ok(())
+        }
+        .scope_boxed()
+    })
+    .await?;
+
+    Ok(HttpResponse::Ok())
 }
 
 #[utoipa::path(responses((status = OK)), security(("api_jwt_token" = [])))]

src/handlers/watch_history.rs

@@ -273,7 +273,11 @@ async fn add_to_watch_history(
         store_watch_history_items(&pool, &account.id, vec![watch_history_item.into_inner()])
             .await?;
 
-    Ok(HttpResponse::Ok().json(stored.pop()))
+    // One item in, one item out. Answering `200 null` if that ever stops holding
+    // would be a silently wrong response, so make it an error instead.
+    let stored = stored.pop().ok_or(HandlerError::InternalDatabaseError)?;
+
+    Ok(HttpResponse::Ok().json(stored))
 }
 
 #[utoipa::path(responses((status = OK)), security(("api_jwt_token" = [])))]

src/validation.rs

@@ -31,6 +31,11 @@ fn verify_image_url(image_url: &str) -> bool {
         return false;
     };
 
+    // Clients load these URLs, so do not store plaintext or exotic schemes.
+    if url.scheme() != "https" {
+        return false;
+    }
+
     let Some(host) = url.host_str() else {
         return false;
     };
@@ -143,7 +148,11 @@ pub async fn validate_channel_against_youtube(channel: &mut Channel) -> HandlerR
         .await
         .map_err(|_| HandlerError::YouTubeConnectError)?;
 
-    validate_channel_information(channel.clone(), &rss_channel)
+    // Assign the result back: `validate_channel_information` replaces the name
+    // with the canonical one from the feed, and dropping it here would persist
+    // the client-supplied name instead. `validate_videos_against_youtube` does
+    // the same, so both paths normalize identically.
+    (*channel) = validate_channel_information(channel.clone(), &rss_channel)
         .map_err(|_| HandlerError::ValidationError)?;
 
     Ok(())
@@ -209,6 +218,15 @@ pub async fn validate_videos_against_youtube(
         return Ok(());
     }
 
+    // `zip` below truncates to the shorter slice, which would silently skip
+    // validation for the tail. This gates metadata validation, so enforce the
+    // contract rather than documenting it.
+    if needs_validation.len() != video_datas.len() {
+        return Err(HandlerError::ValidationErrorWithContext(
+            "validation plan does not match the batch".to_owned(),
+        ));
+    }
+
     for video in video_datas.iter() {
         if video.uploader != *channel {
             return Err(HandlerError::ValidationErrorWithContext(
@@ -390,6 +408,9 @@ mod test {
         assert!(!verify_image_url("https://ytimg.com.evil.net/a.jpg"));
         // real subdomains are still accepted
         assert!(verify_image_url("https://yt3.googleusercontent.com/a.jpg"));
+        // only https, since clients load whatever is stored here
+        assert!(!verify_image_url("http://i1.ytimg.com/vi/x/hqdefault.jpg"));
+        assert!(!verify_image_url("ftp://i1.ytimg.com/vi/x/hqdefault.jpg"));
     }
 
     #[actix_rt::test]