Skip to content

fix(lfs): reject invalid lock list limit instead of panicking - #2175

Open
Tyagiquamar wants to merge 2 commits into
gitmono-dev:mainfrom
Tyagiquamar:fix/lfs-lock-limit-validation
Open

fix(lfs): reject invalid lock list limit instead of panicking#2175
Tyagiquamar wants to merge 2 commits into
gitmono-dev:mainfrom
Tyagiquamar:fix/lfs-lock-limit-validation

Conversation

@Tyagiquamar

Copy link
Copy Markdown

Problem

The limit query parameter of GET /info/lfs/locks (and the LFS lock list API generally) is a raw string from the query string that reaches limit.parse::<i64>().unwrap() in lfs_get_filtered_locks (ceres/src/lfs/handler.rs):

  • ?limit=abc panics on the unwrap.
  • A negative value such as ?limit=-1 parses fine, then size as usize wraps to usize::MAX, and locks[size as usize] / split_off(...) panic out of bounds.

Both are handler panics triggered by an unauthenticated request.

Fix

Parse the limit as usize, so negative values are rejected naturally, and map malformed values to a GitLFSError::GeneralError("Invalid limit parameter: …"), which the router's error mapping already turns into a 400 response. The pagination logic is extracted into a pure apply_lock_limit helper so it can be tested directly; behavior for valid numeric limits is unchanged.

Testing

New unit tests in ceres/src/lfs/handler.rs:

  • valid limit slices the page and reports the next cursor
  • limit beyond list size returns everything without a cursor
  • empty limit returns unpaged locks
  • non-numeric and negative limits are rejected with an error

Verification:

  • cargo test -p ceres --lib lfs — 8 passed (4 new)
  • cargo clippy -p ceres --all-targets --all-features -- -D warnings — clean
  • cargo +nightly fmt --check — clean on the touched file

Signed-off-by: Tyagiquamar Tyagiquamar@users.noreply.github.com

The `limit` query parameter of GET /info/lfs/locks flows as a raw
string into `limit.parse::<i64>().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 <Tyagiquamar@users.noreply.github.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8c0d2010a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ceres/src/lfs/handler.rs
Comment on lines +406 to +408
let size = limit
.parse::<usize>()
.map_err(|_| GitLFSError::GeneralError(format!("Invalid limit parameter: {limit}")))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the invalid-limit error through the list handler

When GET /info/lfs/locks?limit=abc (or a negative limit) reaches this branch, apply_lock_limit creates an error containing Invalid, but lfs_retrieve_lock catches every error and replaces it with Lookup operation failed!. Consequently, list_locks passes that replacement to map_lfs_error, which classifies it as HTTP 500 rather than the intended HTTP 400. Propagate the original error or otherwise preserve its invalid-input classification.

Useful? React with 👍 / 👎.

@Tyagiquamar Tyagiquamar Aug 25, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — valid finding, and exactly the gap between the commit message's promise ("maps to a 400") and reality: lfs_retrieve_lock masked the Invalid limit parameter error into Lookup operation failed!, which map_lfs_error classified as a 500.

Fixed in 9b27fc8: lfs_retrieve_lock now passes GitLFSError::GeneralError values whose message carries the router's classification prefix (Invalid...) through unmasked, and keeps masking only genuine lookup failures behind the generic message. GET /info/lfs/locks?limit=abc (or a negative limit) now returns HTTP 400 with the descriptive message; cursor-not-found and storage failures keep their existing Lookup operation failed! behavior.

The handler-layer unit test was strengthened to assert the Invalid message prefix, since that string is the contract map_lfs_error depends on (the router already documents the string-matching as temporary until typed error variants exist).

Verified in Docker (cargo test -p ceres --lib lfs::handler: 8 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean).

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 <Tyagiquamar@users.noreply.github.com>
Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant