From e8c0d2010a97e26b53670169d5324cacb347476c Mon Sep 17 00:00:00 2001 From: Tyagiquamar Date: Tue, 25 Aug 2026 21:37:19 +0530 Subject: [PATCH 1/2] fix(lfs): reject invalid lock list limit instead of panicking The `limit` query parameter of GET /info/lfs/locks flows as a raw string into `limit.parse::().unwrap()`: - `?limit=abc` panics on the unwrap. - A negative value such as `?limit=-1` passes the parse, then `size as usize` wraps to usize::MAX and `split_off` / indexing panic out of bounds. Both are unauthenticated request-triggered handler panics. Parse the limit as usize (rejecting negatives naturally), map malformed values to a GeneralError that the router maps to a 400, and extract the pagination into a pure helper so it is directly unit-testable. Signed-off-by: Tyagiquamar --- ceres/src/lfs/handler.rs | 69 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/ceres/src/lfs/handler.rs b/ceres/src/lfs/handler.rs index 4c08a4ffd..660cc3710 100644 --- a/ceres/src/lfs/handler.rs +++ b/ceres/src/lfs/handler.rs @@ -1,4 +1,4 @@ -use std::{cmp::min, time::Duration}; +use std::time::Duration; use anyhow::Result; use bytes::Bytes; @@ -391,15 +391,27 @@ async fn lfs_get_filtered_locks( locks = filterd; } + apply_lock_limit(locks, limit) +} + +/// Applies the `limit` parameter to an ordered lock list, returning the page and +/// the id of the first lock past the page (empty when no further page exists). +/// +/// The limit comes straight from the query string, so anything non-numeric +/// (including negative values) is rejected instead of being parsed leniently. +fn apply_lock_limit(locks: Vec, limit: &str) -> Result<(Vec, String), GitLFSError> { let mut next = "".to_string(); + let mut locks = locks; if !limit.is_empty() { - let mut size = limit.parse::().unwrap(); - size = min(size, locks.len() as i64); + let size = limit + .parse::() + .map_err(|_| GitLFSError::GeneralError(format!("Invalid limit parameter: {limit}")))?; + let size = size.min(locks.len()); - if size + 1 < locks.len() as i64 { - locks[size as usize].id.clone_into(&mut next); + if size + 1 < locks.len() { + locks[size].id.clone_into(&mut next); } - let _ = locks.split_off(size as usize); + let _ = locks.split_off(size); } Ok((locks, next)) @@ -625,6 +637,51 @@ mod tests { use super::*; use crate::lfs::lfs_structs::{Action, Ref, ResCondition, ResponseObject}; + fn lock(id: &str) -> Lock { + Lock { + id: id.to_string(), + path: format!("/dir/{id}.bin"), + owner: None, + locked_at: "2026-01-01T00:00:00Z".to_string(), + } + } + + #[test] + fn lock_limit_slices_page_and_reports_next_cursor() { + let locks = vec![lock("1"), lock("2"), lock("3"), lock("4")]; + + let (page, next) = apply_lock_limit(locks, "2").unwrap(); + assert_eq!( + page.iter().map(|l| l.id.as_str()).collect::>(), + ["1", "2"] + ); + assert_eq!(next, "3"); + } + + #[test] + fn lock_limit_beyond_list_returns_everything_without_cursor() { + let locks = vec![lock("1"), lock("2")]; + + let (page, next) = apply_lock_limit(locks, "10").unwrap(); + assert_eq!(page.len(), 2); + assert_eq!(next, ""); + } + + #[test] + fn empty_limit_returns_unpaged_locks() { + let locks = vec![lock("1")]; + + let (page, next) = apply_lock_limit(locks, "").unwrap(); + assert_eq!(page.len(), 1); + assert_eq!(next, ""); + } + + #[test] + fn non_numeric_and_negative_limits_are_rejected() { + assert!(apply_lock_limit(vec![lock("1")], "abc").is_err()); + assert!(apply_lock_limit(vec![lock("1")], "-1").is_err()); + } + #[test] fn response_object_download_existing() { let meta = MetaObject { From 9b27fc8f46346d6ce30650b96345d1ec0b5b7b6d Mon Sep 17 00:00:00 2001 From: Tyagiquamar Date: Tue, 25 Aug 2026 22:09:54 +0530 Subject: [PATCH 2/2] fix(lfs): keep invalid lock list limit classified as 400 lfs_retrieve_lock replaced every error from lfs_get_filtered_locks with the generic 'Lookup operation failed!', so the 'Invalid limit parameter' error produced by apply_lock_limit lost its classification and the router's map_lfs_error turned it into a 500 instead of the intended 400. Pass input-validation errors through unmasked and keep masking only genuine lookup failures; strengthen the limit-rejection test to pin the 'Invalid' message prefix the router contract depends on. Signed-off-by: Tyagiquamar Signed-off-by: Tyagiquamar --- ceres/src/lfs/handler.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/ceres/src/lfs/handler.rs b/ceres/src/lfs/handler.rs index 660cc3710..2a79781ab 100644 --- a/ceres/src/lfs/handler.rs +++ b/ceres/src/lfs/handler.rs @@ -45,6 +45,12 @@ pub async fn lfs_retrieve_lock( lock_list.next_cursor = next; Ok(lock_list) } + // Client-input errors (e.g. a malformed `limit`) must reach the router + // unmasked so `map_lfs_error` can classify them as 400; only genuine + // lookup failures are hidden behind the generic message. + Err(GitLFSError::GeneralError(msg)) if msg.starts_with("Invalid") => { + Err(GitLFSError::GeneralError(msg)) + } Err(_) => Err(GitLFSError::GeneralError( "Lookup operation failed!".to_string(), )), @@ -678,8 +684,17 @@ mod tests { #[test] fn non_numeric_and_negative_limits_are_rejected() { - assert!(apply_lock_limit(vec![lock("1")], "abc").is_err()); - assert!(apply_lock_limit(vec![lock("1")], "-1").is_err()); + // The router classifies by message content ("Invalid..." -> 400), so the + // rejection must carry that prefix to survive the lookup-error masking + // in `lfs_retrieve_lock`. + for limit in ["abc", "-1"] { + match apply_lock_limit(vec![lock("1")], limit) { + Err(GitLFSError::GeneralError(msg)) => { + assert!(msg.starts_with("Invalid"), "unexpected message: {msg}"); + } + other => panic!("expected GeneralError, got {other:?}"), + } + } } #[test]