HYPERFLEET-1259 - fix: SQL injection protection for order queries#244
HYPERFLEET-1259 - fix: SQL injection protection for order queries#244ma-hill wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOrder-by handling now uses a fixed field allowlist and a format regex, defaults missing direction to Sequence Diagram(s)sequenceDiagram
participant Client
participant sqlGenericService.buildOrderBy
participant db.ArgsToOrder
participant ListQuery
Client->>sqlGenericService.buildOrderBy: submit OrderBy args
sqlGenericService.buildOrderBy->>db.ArgsToOrder: validate and normalize args
db.ArgsToOrder-->>sqlGenericService.buildOrderBy: cleaned list or BadRequest
sqlGenericService.buildOrderBy->>ListQuery: apply cleaned order clauses
ListQuery-->>Client: sorted response or 400
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 review |
✅ Action performedReview finished.
|
ma-hill
force-pushed
the
HYPERFLEET-1259
branch
3 times, most recently
from
a88f369 to
52c393a
Compare
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 553 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/services | +1 |
Computed by hyperfleet-risk-scorer
![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
🧹 Nitpick comments (1)
pkg/db/sql_helpers.go (1)
817-862: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegex contract mismatch + unreachable error branches.
orderByPatternis^[a-z_][a-z_]*(\s+(asc|desc))?$— it rejects digits and uppercase, yet the comment on Line 817 claims "letters, digits, underscore." That's a latent foot-gun: adding any digit-bearing field (e.g.image_v2) toorderByAllowedFieldswould silently 400 despite being whitelisted.Separately, because the regex already gates format, two branches below are dead:
- the
direction != "asc" && direction != "desc"check (Line 849-851) — regex only permitsasc|desc.- the
defaultcase (Line 855-857) —strings.Fieldscan only yield 1 or 2 tokens after the regex passes.The granular
invalid sort directionmessage is therefore never emitted;name ascendingandname asc extraboth fall through to the format error (consistent with the tests). Fix the comment to match the pattern, and either drop the unreachable branches or include0-9in the field class to honor the documented contract.Proposed alignment
-// orderByPattern matches valid orderBy syntax: field name (letters, digits, underscore) followed by optional asc/desc. -// This regex rejects SQL injection attempts (semicolons, parentheses, dashes, comments, etc). -var orderByPattern = regexp.MustCompile(`^[a-z_][a-z_]*(\s+(asc|desc))?$`) +// orderByPattern matches valid orderBy syntax: a lowercase field name +// (letters, digits, underscore) followed by an optional asc/desc direction. +// It rejects SQL injection attempts (semicolons, parentheses, dashes, comments, etc). +var orderByPattern = regexp.MustCompile(`^[a-z_][a-z0-9_]*(\s+(asc|desc))?$`)🤖 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/db/sql_helpers.go` around lines 817 - 862, The ArgsToOrderBy validation in orderByPattern and the surrounding parsing logic are inconsistent: the regex currently only allows lowercase letters/underscores, while the comment claims digits are supported, and the later direction/default checks in ArgsToOrderBy are unreachable after the regex gate. Update the comment and either keep the stricter regex and remove the dead validation branches, or расширить orderByPattern to match the intended field contract; make sure orderByAllowedFields remains the final whitelist check.Source: Path instructions
🤖 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 `@pkg/db/sql_helpers_test.go`:
- Around line 957-958: The parallel subtests in TestArgsToOrderBy are sharing
Gomega’s global registration via RegisterTestingT, which can race when
t.Parallel is used. Update each subtest to create its own local Gomega instance
with NewWithT(t) and replace any shared Expect calls with g.Expect in that test
scope so each subtest uses its own fail handler.
In `@test/integration/order_field_mapping_test.go`:
- Around line 102-136: The MultipleOrderByFields test is asserting a composite
sort, but the current data makes the kind comparison trivial and the
comments/assertions don’t match the actual kind asc,name desc input. Update the
test in order_field_mapping_test.go around MultipleOrderByFields to verify a
field that actually varies or assert the full (kind, name) ordering sequence
using the GetClustersWithResponse result, and make the inline comments and name
comparison consistent with descending name order.
---
Nitpick comments:
In `@pkg/db/sql_helpers.go`:
- Around line 817-862: The ArgsToOrderBy validation in orderByPattern and the
surrounding parsing logic are inconsistent: the regex currently only allows
lowercase letters/underscores, while the comment claims digits are supported,
and the later direction/default checks in ArgsToOrderBy are unreachable after
the regex gate. Update the comment and either keep the stricter regex and remove
the dead validation branches, or расширить orderByPattern to match the intended
field contract; make sure orderByAllowedFields remains the final whitelist
check.
🪄 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: 80dd1400-1a49-4cb3-bcee-ad077adc7a1a
📒 Files selected for processing (4)
pkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/services/generic.gotest/integration/order_field_mapping_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)
ma-hill
force-pushed
the
HYPERFLEET-1259
branch
3 times, most recently
from
86a1652 to
648cccb
Compare
| // orderAllowedFields defines the whitelist of fields that are allowed to be ordered. | ||
| // This prevents SQL injection and restricts invalid order queries. | ||
| var orderAllowedFields = map[string]bool{ | ||
| "id": true, | ||
| "name": true, | ||
| "created_time": true, | ||
| "updated_time": true, | ||
| "deleted_time": true, | ||
| "kind": true, | ||
| "created_by": true, | ||
| "updated_by": true, | ||
| "deleted_by": true, | ||
| "generation": true, | ||
| "href": true, | ||
| } |
There was a problem hiding this comment.
This list is missing deleted_time and deleted_by from the orderByAllowedFields map, so 2 of 11 allowed fields don't have integration coverage. Consider adding them so the test lives up to its "all whitelisted fields" intent.
There was a problem hiding this comment.
deleted_time ( line 563 ) and deleted-by ( line 567 )
There was a problem hiding this comment.
The allowlist in sql_helpers.go is correct. My comment was about the integration test TestOrderByAllowedFields in order_field_mapping_test.go, which only tests 9 of 11 fields. deleted_time and deleted_by aren't in the test's allowedFields slice, so they don't have integration coverage.
| Expect(currKind == prevKind).To(BeTrue(), fmt.Sprintf("Kinds should be equal: %s == %s", currKind, prevKind)) | ||
|
|
||
| // Secondary key: within same kind, names should be ascending | ||
| Expect(currName <= prevName).To(BeTrue(), | ||
| fmt.Sprintf("Within same kind, names should be descending: %s <= %s", | ||
| currName, prevName)) | ||
| } | ||
| } |
There was a problem hiding this comment.
All test clusters share the same kind value, so the kind sort assertion always passes trivially. Consider ordering by name asc, created_time desc instead, since both fields vary across the test data.
There was a problem hiding this comment.
@pnguyen44 Yes, this is true. But this test is verifying that if the primary key fields are the same the order by functionality will order based on the secondary key. For this test, it would be the cluster names since kind is always Cluster, using name asc, created_time desc won't really show us anything because you cannot a cluster with the same name. So will just order by name. I guess I could extend this tests to include other resources besides clusters.
| t.Run("InvalidFieldName", func(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| orderBy := openapi.QueryParamsOrderBy("nonexistent_field asc") | ||
| params := &openapi.GetClustersParams{ | ||
| OrderBy: &orderBy, | ||
| } | ||
| resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) | ||
| Expect(string(resp.Body)).To(ContainSubstring("not allowed for ordering")) | ||
| }) |
There was a problem hiding this comment.
These 7 subtests (InvalidFieldName through TooManyParts) all share the same structure with only input and expected error varying. Consider collapsing them into a table-driven test, matching the pattern already used by TestArgsToOrderBy in the unit tests.
|
Missing CHANGELOG entry. This restricts which fields are orderable, which is a user-visible behavioral change. Consider adding an entry. |
| injectionAttempts := []struct { | ||
| name string | ||
| input string | ||
| }{ | ||
| {"union injection", "name UNION SELECT password FROM users"}, | ||
| {"comment injection", "name--"}, | ||
| {"semicolon terminator", "name; DROP TABLE resources;"}, | ||
| {"quote escape", "name' OR '1'='1"}, | ||
| {"parentheses", "name) OR (1=1"}, | ||
| {"wildcard", "name*"}, | ||
| {"backtick", "name`"}, | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for uppercase rejection to verify the lowercase-only enforcement.
ma-hill
force-pushed
the
HYPERFLEET-1259
branch
2 times, most recently
from
cc225eb to
f6f114e
Compare
ma-hill
force-pushed
the
HYPERFLEET-1259
branch
2 times, most recently
from
32ceb82 to
95e8e84
Compare
ma-hill
changed the title
|
Looks like the rebase reverted the ArgsToOrderBy → ArgsToOrder rename in a few places. The function and one test still use OrderBy while everything else uses order. Can you do a quick search for any remaining OrderBy references and align them? |
2 participants
Summary
Adds SQL injection protection and field whitelisting to the
orderByquery parameter handling. Previously, theArgsToOrderByfunction used a per-resource blacklist approach that was vulnerable to SQL injection attacks through malicious field names or sort directions. This refactors the validation to use a hardcoded allowlist of permitted fields and a regex pattern that rejects all SQL injection attempts (semicolons, comments, parentheses, etc.), providing defense-in-depth protection for all list endpoints.HYPERFLEET-1259
Changes
disallowedFieldsblacklist parameter with a hardcodedorderByAllowedFieldswhitelist map containing only safe, permitted field names (id,name,created_time,updated_time,deleted_time,kind,created_by,updated_by,deleted_by,generation,href)orderByPattern) that enforces strict syntax: lowercase field names (letters, underscore only) followed by optionalasc/descdirectionArgsToOrderBy->ArgsToOrderand signature to remove thedisallowedFieldsparameter, simplifying the API to just accept the raw orderBy arguments arrayTestArgsToOrderBy,TestArgsToOrderBy_SecurityValidation) covering valid inputs, edge cases (empty strings, whitespace), and SQL injection attemptsTestOrderByFieldMapping,TestOrderByFieldValidation,TestOrderByAllowedFields,TestOrderByEmptyStrings) that verify ordering behavior end-to-end via HTTP and validate that injection attempts return 400 Bad Requestpkg/services/generic.goto use the new single-parameter signatureTest Plan
TestArgsToOrderBy,TestArgsToOrderBy_SecurityValidation)TestOrderByFieldMapping,TestOrderByFieldValidation)make test-allpassesmake lintpasses