Reject an unusable data frame range where the request arrives - #858
Open
CleanCut wants to merge 7 commits into
Open
Reject an unusable data frame range where the request arrives#858CleanCut wants to merge 7 commits into
CleanCut wants to merge 7 commits into
Conversation
A bad slice, take, item, or add-col value panicked the worker thread rather than returning an error, so a caller could fault a request worker with nothing more than a query string. Two were reachable straight from the slice and take query parameters, and a third from an ordinary page parameter: page=0 and page_size=0 both produce the range 0..0. That panic reached production. Slice ranges are now parsed into a type that cannot hold an unusable range, at the two places untrusted input arrives — the HTTP query parser and the CLI argument parser — so the read path below them goes back to being infallible. Callers that build a range from page arithmetic construct that type directly instead of formatting integers into a string for something deeper to parse again. A page or page_size of zero reads as the first page, matching what the workspace data frame endpoint already did. A slice that was not shaped like a range, such as "5" or "1..2..3", was previously ignored and the whole frame returned. It is now rejected like any other malformed value. A negative start is rejected too: polars reads it as counting back from the end of the frame, which is not the range the caller named. The view layer turned these errors straight back into panics by unwrapping the data frame transform, so the diff endpoints kept crashing on a malformed take even once the parse returned cleanly. Those constructors now report the failure to their caller, which answers with a 400. The message on the range check said "Start must be greater than end" while firing on start >= end. It now states the rule it enforces. ENG-1329 Sentry: OXEN-SERVER-28, OXEN-SERVER-29
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces string-based dataframe slices with validated ChangesDataframe validation and error propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DataFrameController
participant QueryParser
participant JsonDataFrameView
participant TabularTransform
Client->>DataFrameController: request with slice or take parameters
DataFrameController->>QueryParser: parse_opts(query)
QueryParser-->>DataFrameController: DFOpts or OxenError
DataFrameController->>JsonDataFrameView: build view from DFOpts
JsonDataFrameView->>TabularTransform: transform and slice dataframe
TabularTransform-->>JsonDataFrameView: transformed dataframe or error
JsonDataFrameView-->>DataFrameController: view or error
DataFrameController-->>Client: response or 400 Bad Request
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/oxen-server/src/controllers/data_frames.rs (1)
61-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize pagination values before use.
At Line 77,
page=0orpage_size=0creates0..0.SliceRange::newrejects that range, so these requests return 400 instead of using the first page. Largeusizevalues can also overflow the multiplication.Normalize both values with
max(1)before pagination arithmetic. Use the bounded arithmetic already used by the workspace read path. Store the normalized values in response metadata and download options.
crates/oxen-server/src/controllers/data_frames.rs#L61-L77: normalizepageandpage_sizebefore range construction and updatepage_optswith normalized values.crates/oxen-server/src/controllers/diff.rs#L406-L411: normalize values before constructing the file-diff slice.crates/oxen-server/src/controllers/diff.rs#L835-L835: normalize values before constructing the derived-dataframe pagination slice.crates/oxen-server/src/controllers/workspaces/data_frames.rs#L470-L472: store normalized values inDFOptsfor both download handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/oxen-server/src/controllers/data_frames.rs` around lines 61 - 77, Normalize page and page_size with max(1) before pagination arithmetic, using the workspace read path’s bounded arithmetic to prevent overflow. In crates/oxen-server/src/controllers/data_frames.rs:61-77, update page_opts and the constructed slice; in crates/oxen-server/src/controllers/diff.rs:406-411 and :835-835, normalize values before each file-diff or derived-dataframe slice; and in crates/oxen-server/src/controllers/workspaces/data_frames.rs:470-472, store normalized values in DFOpts for both download handlers.crates/oxen-py/src/py_remote_data_frame.rs (1)
34-40: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winPass
optsto the API request.
opts.sliceis created at Line 34, butapi::client::data_frames::getreceivesDFOpts::empty()at Line 40. The validated0..1slice is discarded. The server therefore applies its default page instead of the requested one. Passopts, or remove the assignment ifsize()does not need a one-row view.Proposed fix
let response = api::client::data_frames::get( self.repo.repo()?, revision, &self.path, - DFOpts::empty(), + opts, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/oxen-py/src/py_remote_data_frame.rs` around lines 34 - 40, Pass the configured opts, including the 0..1 slice assigned in the surrounding method, to api::client::data_frames::get instead of DFOpts::empty().crates/liboxen/src/view/json_data_frame_view.rs (1)
219-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClamp pagination metadata here as well.
from_df_optsclampspage_sizeandpageto a minimum of one at Lines 143-147.from_df_opts_unpaginateddoes not. Withpage_size == 0the response reportspage_size: 0andtotal_pages: 0, becauseog_height as f64 / 0.0is infinite and the cast tousizesaturates. Use the same clamp so both constructors report the same pagination domain.♻️ Proposed clamp
- let page_size = opts.page_size.unwrap_or(constants::DEFAULT_PAGE_SIZE); - let page_number = opts.page.unwrap_or(constants::DEFAULT_PAGE_NUM); + // Match `from_df_opts`: pages are 1-based and a page of zero rows names no rows. + let page_size = opts + .page_size + .unwrap_or(constants::DEFAULT_PAGE_SIZE) + .max(1); + let page_number = opts.page.unwrap_or(constants::DEFAULT_PAGE_NUM).max(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/liboxen/src/view/json_data_frame_view.rs` around lines 219 - 222, Clamp the page_size and page values in from_df_opts_unpaginated to a minimum of one before computing total_pages, matching the existing behavior in from_df_opts. Ensure the returned pagination metadata never reports zero page size or page number, and calculate total_pages using the clamped page_size.crates/liboxen/src/view/tabular_diff_view.rs (1)
52-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFollow up on the remaining panics in this now-fallible function.
The signature change makes
from_data_framesable to report failures. The diff computations inside it still panic:compute_new_columns_from_dfs,compute_new_rows_proj,compute_new_rows, and thespawn_blockingclosure that collects the common-column frames all use.unwrap(). A panic there still faults the request worker, which is the failure mode this PR removes elsewhere. Convert those to?in a follow-up so the whole path reports instead of panicking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/liboxen/src/view/tabular_diff_view.rs` around lines 52 - 56, Update TabularDiffView::from_data_frames to remove all remaining unwrap-based panics: propagate errors with ? from compute_new_columns_from_dfs, compute_new_rows_proj, and compute_new_rows, and make the spawn_blocking closure collecting common-column frames return its collection error so the outer function can propagate it. Preserve the existing successful computation flow while ensuring each failure returns OxenError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/liboxen/src/core/df/tabular.rs`:
- Around line 641-651: Update paginate_df to clamp page_num and page_size to
valid minimums before calculating offsets, treating page_num == 0 as the first
page. Replace direct multiplications and subtraction with saturating arithmetic
so large caller-supplied values cannot overflow, while preserving the existing
slice and collection behavior.
In `@crates/liboxen/src/opts/df_opts.rs`:
- Around line 60-65: Update SliceRange::row_count to saturate the end-start
difference at u32::MAX instead of directly casting, so ranges wider than u32
remain bounded rather than wrapping to zero. Correct the accompanying comment to
describe only the non-negative difference guarantee, preserving existing
behavior for ranges within the u32 limit.
In `@crates/liboxen/src/view/json_data_frame_view.rs`:
- Around line 149-164: Extract the shared page-to-slice arithmetic into a helper
on DFOpts or beside SliceRange, then use it at
crates/liboxen/src/view/json_data_frame_view.rs:149-164 to clamp page to at
least 1, compute start with saturating_mul capped at usize::MAX - page_size, and
set end to start + page_size. Update
crates/liboxen/src/core/df/tabular.rs:641-651 to clamp page_opts.page_num and
page_opts.page_size before subtraction and use saturating multiplication. At
crates/liboxen/src/view/json_data_frame_view.rs:219-222, clamp page_size and
page to at least 1 so reported pagination matches from_df_opts.
- Around line 149-164: Update the pagination calculations in the JSON data-frame
view to cap the page offset using saturating multiplication and limit it to
usize::MAX minus page_size, matching the existing controller logic. Ensure start
remains below end for very large page values and prevent overflow in the
page_size * page computation while preserving normal pagination behavior.
In `@crates/oxen-py/src/py_remote_data_frame.rs`:
- Line 58: Update the row slicing logic around SliceRange::new to convert row
with i64::try_from and compute the exclusive end using checked_add(1),
propagating conversion or overflow failures as OxenError before constructing the
range.
---
Outside diff comments:
In `@crates/liboxen/src/view/json_data_frame_view.rs`:
- Around line 219-222: Clamp the page_size and page values in
from_df_opts_unpaginated to a minimum of one before computing total_pages,
matching the existing behavior in from_df_opts. Ensure the returned pagination
metadata never reports zero page size or page number, and calculate total_pages
using the clamped page_size.
In `@crates/liboxen/src/view/tabular_diff_view.rs`:
- Around line 52-56: Update TabularDiffView::from_data_frames to remove all
remaining unwrap-based panics: propagate errors with ? from
compute_new_columns_from_dfs, compute_new_rows_proj, and compute_new_rows, and
make the spawn_blocking closure collecting common-column frames return its
collection error so the outer function can propagate it. Preserve the existing
successful computation flow while ensuring each failure returns OxenError.
In `@crates/oxen-py/src/py_remote_data_frame.rs`:
- Around line 34-40: Pass the configured opts, including the 0..1 slice assigned
in the surrounding method, to api::client::data_frames::get instead of
DFOpts::empty().
In `@crates/oxen-server/src/controllers/data_frames.rs`:
- Around line 61-77: Normalize page and page_size with max(1) before pagination
arithmetic, using the workspace read path’s bounded arithmetic to prevent
overflow. In crates/oxen-server/src/controllers/data_frames.rs:61-77, update
page_opts and the constructed slice; in
crates/oxen-server/src/controllers/diff.rs:406-411 and :835-835, normalize
values before each file-diff or derived-dataframe slice; and in
crates/oxen-server/src/controllers/workspaces/data_frames.rs:470-472, store
normalized values in DFOpts for both download handlers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ae093cb-461c-4427-bd75-435b47815ce4
📒 Files selected for processing (19)
crates/liboxen/src/api/client/data_frames.rscrates/liboxen/src/core/df/tabular.rscrates/liboxen/src/error.rscrates/liboxen/src/model/diff/diff_entry.rscrates/liboxen/src/opts.rscrates/liboxen/src/opts/df_opts.rscrates/liboxen/src/view/json_data_frame.rscrates/liboxen/src/view/json_data_frame_view.rscrates/liboxen/src/view/tabular_diff_view.rscrates/oxen-cli/src/cmd/df.rscrates/oxen-py/src/py_remote_data_frame.rscrates/oxen-server/src/controllers/data_frames.rscrates/oxen-server/src/controllers/diff.rscrates/oxen-server/src/controllers/workspaces/data_frames.rscrates/oxen-server/src/controllers/workspaces/data_frames/columns.rscrates/oxen-server/src/controllers/workspaces/data_frames/embeddings.rscrates/oxen-server/src/controllers/workspaces/data_frames/rows.rscrates/oxen-server/src/errors.rscrates/oxen-server/src/params/df_opts_query.rs
paginate_df underflowed on a page number of zero and multiplied page size by page number unchecked. Neither function has a call site, so deleting them removes the defect and the surface it sits on rather than hardening arithmetic nothing can reach. ENG-1329
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/liboxen/src/core/df/tabular.rs (1)
631-639: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject slice ranges that exceed
u32::MAXrows.
SliceRange::newaccepts ranges such as0..4294967296.SliceRange::row_count()then casts that length tou32, which truncates the value before this slicing path uses it. The request can return an incorrect slice instead of HTTP 400.Add a maximum-length check in
SliceRange::new. Add a regression test for a range longer thanu32::MAX.Proposed fix
pub fn new(start: i64, end: i64) -> Result<Self, OxenError> { + if end - start > u32::MAX as i64 { + return invalid("range must not exceed 4294967295 rows"); + } // existing validation }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/liboxen/src/core/df/tabular.rs` around lines 631 - 639, Update SliceRange::new to reject ranges whose length exceeds u32::MAX, before row_count() can truncate the value; preserve existing validation for start/end ordering and valid ranges. Add a regression test constructing a range longer than u32::MAX and assert that construction is rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/liboxen/src/core/df/tabular.rs`:
- Around line 631-639: Update SliceRange::new to reject ranges whose length
exceeds u32::MAX, before row_count() can truncate the value; preserve existing
validation for start/end ordering and valid ranges. Add a regression test
constructing a range longer than u32::MAX and assert that construction is
rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4706d5e-5763-4da6-8483-1b5bd0bafcb4
📒 Files selected for processing (1)
crates/liboxen/src/core/df/tabular.rs
The page-to-slice arithmetic was written out at five call sites that disagreed about a degenerate request, and the workspace data frame endpoint capped its start against usize::MAX before casting to i64, so a large enough page number produced a negative start and answered a server-side arithmetic problem with a client error. Pages now convert to a row range through one helper that clamps page and page size to at least one and cannot overflow, so any page number names a range the reader can serve. The unpaginated view reported the page and size it was handed without clamping, so a page size of zero divided by zero and reported a total page count of usize::MAX. ENG-1329
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/liboxen/src/opts/df_opts.rs (1)
396-402: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject extra
itemcomponents.
column_atacceptscolumn:1:extraascolumn:1because it reads only the first two components. This does not meet the documentedcol:indexformat and prevents malformed requests from returning HTTP 400.Reject the value when
split.next().is_some()after readingindex.Proposed fix
let col = split.next().ok_or_else(malformed)?; let index = split.next().ok_or_else(malformed)?; +if split.next().is_some() { + return Err(malformed()); +} return Ok(Some(IndexedItem {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/liboxen/src/opts/df_opts.rs` around lines 396 - 402, Update the parsing logic that constructs IndexedItem to reject values containing more than two delimiter-separated components: after reading col and index, check split.next() and return malformed when it is present. Preserve the existing parsing and error behavior for valid col:index values and invalid indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/liboxen/src/opts/df_opts.rs`:
- Around line 296-298: The row-selection path in slice_indices must preserve
invalid row errors instead of converting them to None. Change slice_indices to
return Result<Option<SliceRange>, OxenError>, propagate SliceRange::for_row(row)
with ?, update callers through transform_slice_lazy and slice to handle the
result, and revise the test that currently expects None for an invalid row.
---
Outside diff comments:
In `@crates/liboxen/src/opts/df_opts.rs`:
- Around line 396-402: Update the parsing logic that constructs IndexedItem to
reject values containing more than two delimiter-separated components: after
reading col and index, check split.next() and return malformed when it is
present. Preserve the existing parsing and error behavior for valid col:index
values and invalid indices.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0684620b-e2b9-47d5-ae4e-adfec052ff09
📒 Files selected for processing (2)
crates/liboxen/src/opts/df_opts.rscrates/oxen-py/src/py_remote_data_frame.rs
An --item value with no colon was ignored and the whole data frame returned instead of the single value asked for, and one with extra colons silently dropped everything past the second segment, so "file:1:2" read as row 1 of "file". Both now report what is wrong with the value, as does an item with an empty column name, which could never match a column.
An empty data frame answered with a page number and page size of zero while a non-empty one echoed what the caller asked for, so a reader could not compute its position from an empty response.
A row index too large to convert into a row range read as no selection at all, so a request for a single row answered with the entire data frame. It now reports what is wrong with the index.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/oxen-server/src/controllers/data_frames.rs (1)
69-77: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winConstruct pagination with validated page bounds.
Lines 69-77 bypass
DFOpts::page_boundsandSliceRange::for_page. Whenpage=0,endbecomes zero, soSliceRange::new(0, 0)returns an error instead of reading the first page. Largepageorpage_sizevalues can also overflow during multiplication before range validation.Use
opts.page_bounds()to setpage_opts, then useSliceRange::for_page(page, page_size). Add regression coverage for zero values and maximum accepted query values.Proposed fix
- let page = query.page.unwrap_or(constants::DEFAULT_PAGE_NUM); - let page_size = query.page_size.unwrap_or(constants::DEFAULT_PAGE_SIZE); + let (page, page_size) = opts.page_bounds(); page_opts.page_num = page; page_opts.page_size = page_size; - let start = if page == 0 { 0 } else { page_size * (page - 1) }; - let end = page_size * page; - opts.slice = Some(SliceRange::new(start as i64, end as i64)?); + opts.slice = Some(SliceRange::for_page(page, page_size));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/oxen-server/src/controllers/data_frames.rs` around lines 69 - 77, Replace the manual page and slice calculations in the controller with validated pagination: use opts.page_bounds() to populate page_opts and construct opts.slice via SliceRange::for_page(page, page_size). Preserve the existing query defaults while ensuring zero values and maximum accepted page/page_size values are handled without overflow, and add regression coverage for those boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/oxen-server/src/controllers/data_frames.rs`:
- Around line 69-77: Replace the manual page and slice calculations in the
controller with validated pagination: use opts.page_bounds() to populate
page_opts and construct opts.slice via SliceRange::for_page(page, page_size).
Preserve the existing query defaults while ensuring zero values and maximum
accepted page/page_size values are handled without overflow, and add regression
coverage for those boundaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16108a32-d163-4a9d-a9c1-4b8c21b55428
📒 Files selected for processing (4)
crates/liboxen/src/core/df/tabular.rscrates/liboxen/src/opts/df_opts.rscrates/oxen-server/src/controllers/data_frames.rscrates/oxen-server/src/controllers/workspaces/data_frames.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix some panics we're seeing in production.
A bad slice, take, item, or add-col value panicked the worker thread rather than returning an error, so a caller could fault a request worker with nothing more than a query string. Two were reachable straight from the slice and take query parameters, and a third from an ordinary page parameter.
Slice ranges are now parsed into a type that cannot hold an unusable range, at the two places untrusted input arrives. Callers that build a range from page arithmetic construct that type directly instead of formatting integers into a string for something deeper to parse again. A
pageorpage_sizeof zero reads as the first page, matching what the workspace data frame endpoint already did.A slice that was not shaped like a range, such as "5" or "1..2..3", was previously ignored and the whole frame returned. It is now rejected like any other malformed value. A negative start is rejected too: polars reads it as counting back from the end of the frame, which is not the range the caller named.
The view layer turned these errors straight back into panics by unwrapping the data frame transform, so the diff endpoints kept crashing on a malformed take even once the parse returned cleanly. Those constructors now report the failure to their caller, which answers with a 400.
ENG-1329
Sentry: OXEN-SERVER-28, OXEN-SERVER-29