HYPERFLEET-1241 - fix: pagination parameter validation#232
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Security-Critical ChecklistCWE-20 (Improper Input Validation)
CWE-703 (Improper Check or Handling of Exceptional Conditions)
CWE-89 (SQL Injection) / Ordering Direction Injection
Contract Break (Compile-time enforcement)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/api-resources.md`:
- Around line 688-691: The documented default value for the `orderBy` parameter
in the API resources table shows `created_time`, but the actual runtime default
from `NewListArguments` is `created_time desc`. Update the `orderBy` row in the
documentation table to change the default value from `created_time` to
`created_time desc` to align the documentation with the actual implementation
behavior.
In `@pkg/services/types.go`:
- Around line 111-120: The orderBy field processing loop in the section where
listArgs.OrderBy is modified does not filter out empty tokens, allowing
malformed entries like " asc" to be created when empty values are passed (e.g.,
"name,,created_time"). Add a validation check after trimming each field to skip
empty tokens and prevent them from being processed with the order direction
applied. Specifically, after creating trimmedField via strings.TrimSpace, check
if the length is zero and continue to the next iteration if so, ensuring only
non-empty fields receive the direction suffix.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 11340d16-364f-4c4c-ae7e-44e94e0e0cfd
📒 Files selected for processing (10)
docs/api-resources.mdpkg/handlers/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_status.gopkg/handlers/framework.gopkg/handlers/node_pool.gopkg/handlers/nodepool_status.gopkg/handlers/resource_handler.gopkg/services/types.gopkg/services/types_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 418 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/handlers | +1 |
Computed by hyperfleet-risk-scorer
|
Bug (nit) —
This means Consider applying the // Set default sorting — respect order param if provided
if len(listArgs.OrderBy) == 0 {
defaultDir := "desc"
if v := strings.Trim(params.Get("order"), " "); v == "asc" || v == "desc" {
defaultDir = v
}
listArgs.OrderBy = []string{"created_time " + defaultDir}
} |
| if trimmed := strings.TrimSpace(field); trimmed != "" { | ||
| listArgs.OrderBy = append(listArgs.OrderBy, trimmed) | ||
| } |
There was a problem hiding this comment.
Inconsistent whitespace normalization: without order, TrimSpace preserves internal multi-spaces ("name asc" stays as-is). With order, strings.Fields + strings.Join normalizes them ("name asc" → "name asc").
Consider normalizing in the initial parsing:
| if trimmed := strings.TrimSpace(field); trimmed != "" { | |
| listArgs.OrderBy = append(listArgs.OrderBy, trimmed) | |
| } | |
| if trimmed := strings.TrimSpace(field); trimmed != "" { | |
| listArgs.OrderBy = append(listArgs.OrderBy, strings.Join(strings.Fields(trimmed), " ")) | |
| } |
| // If fields are specified, it filters the resource and returns only the requested fields. | ||
| func applyFieldFilter(r *http.Request, presented interface{}) (interface{}, *errors.ServiceError) { | ||
| listArgs := services.NewListArguments(r.URL.Query()) | ||
| listArgs, err := services.NewListArguments(r.URL.Query()) |
There was a problem hiding this comment.
applyFieldFilter only uses the Fields from ListArguments, but now validates pagination params too. On single-resource GETs (e.g., GET /clusters/{id}?fields=name&page=abc), this returns 400 for an irrelevant parameter.
This is defensively correct ("validate at boundaries"), but could surprise clients. Consider extracting field parsing separately, or documenting this as intentional.
|
Improvement (nit) —
|
2 participants
Summary
Fixes pagination and ordering query parameter validation issues identified in code review.
Changes
orderparameter validation (asc/desconly)pageSize(was platform-dependent)Testing
✅ All unit tests passing
✅ Added test coverage for order parameter edge cases
Related