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 |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes CWE-20 (Improper Input Validation) — the core fix. The prior silent clamp/ignore on CWE-703 (Improper Check or Handling of Exceptional Conditions) — verify Supply chain / interface contract: the Ordering injection — 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 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)
| | `page` | integer (int32)| No | `1` | Must be >= 1 | | ||
| | `pageSize` | integer (int32)| No | `20` | Must be between 1 and 100 | | ||
| | `orderBy` | string | No | `created_time` | Field name(s), optionally with direction | | ||
| | `order` | string | No | - | Must be `asc` or `desc` | |
There was a problem hiding this comment.
Align documented orderBy default with runtime value.
The table says default orderBy is created_time, but NewListArguments defaults to created_time desc. This mismatch can cause client-side expectation drift.
🤖 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 `@docs/api-resources.md` around lines 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.
| // Apply order direction to all orderBy fields that don't already have a direction | ||
| for i, field := range listArgs.OrderBy { | ||
| trimmedField := strings.TrimSpace(field) | ||
| parts := strings.Split(trimmedField, " ") | ||
| if len(parts) == 1 { | ||
| // Field has no direction specified, apply the order parameter | ||
| listArgs.OrderBy[i] = trimmedField + " " + v | ||
| } | ||
| // If field already has direction (e.g., "name asc"), leave it unchanged | ||
| } |
There was a problem hiding this comment.
Reject empty orderBy tokens before applying order (CWE-20).
orderBy=name,,created_time&order=asc currently produces an invalid entry (" asc"), because empty tokens are not filtered before direction injection. That can propagate malformed sort clauses downstream.
Proposed fix
// Validate and apply order parameter (asc/desc direction)
if v := strings.Trim(params.Get("order"), " "); v != "" {
if v != "asc" && v != "desc" {
return nil, errors.New(
errors.CodeValidationFormat,
"Invalid order parameter: must be 'asc' or 'desc', got '%s'",
v,
)
}
- // Apply order direction to all orderBy fields that don't already have a direction
- for i, field := range listArgs.OrderBy {
- trimmedField := strings.TrimSpace(field)
- parts := strings.Split(trimmedField, " ")
- if len(parts) == 1 {
- // Field has no direction specified, apply the order parameter
- listArgs.OrderBy[i] = trimmedField + " " + v
- }
- // If field already has direction (e.g., "name asc"), leave it unchanged
- }
+ // Apply order direction to fields without explicit direction and skip empty tokens.
+ normalized := make([]string, 0, len(listArgs.OrderBy))
+ for _, field := range listArgs.OrderBy {
+ trimmedField := strings.TrimSpace(field)
+ if trimmedField == "" {
+ continue
+ }
+ parts := strings.Fields(trimmedField)
+ if len(parts) == 1 {
+ normalized = append(normalized, parts[0]+" "+v)
+ continue
+ }
+ normalized = append(normalized, strings.Join(parts, " "))
+ }
+ listArgs.OrderBy = normalized
}As per coding guidelines, “Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers).”
🤖 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 `@pkg/services/types.go` around lines 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.
Source: Coding guidelines
|
@ldornele: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 378 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/handlers | +1 |
Computed by hyperfleet-risk-scorer
1 participant
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