HYPERFLEET-1366 - refactor: remove search and order field allowlists - #321
HYPERFLEET-1366 - refactor: remove search and order field allowlists#321Mischulee wants to merge 1 commit into
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
WalkthroughSearch and order parsing no longer uses field allowlists. Structurally valid search names and regex-valid order clauses proceed to SQL generation, while PostgreSQL Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant loadList
participant PostgreSQL
Client->>API: Search or order query
API->>loadList: Load list
loadList->>PostgreSQL: Execute generated SQL
PostgreSQL-->>loadList: undefined_column error
loadList-->>API: BadRequest
API-->>Client: 400 invalid field in search or order query
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 104 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/db/sql_helpers.go (1)
72-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject unknown ORDER BY fields before SQL generation.
ArgsToOrder()only checks syntax;nonexistent_field ascstill reachesd.OrderBy()and becomes a DB error instead of a 400. CWE-20. Validate against the resource’s orderable-field mapping inpkg/db/sql_helpers.go/pkg/services/generic.go, and keep an unknown-field rejection case intest/integration/order_field_mapping_test.go.🤖 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 72 - 73, Validate parsed ORDER BY fields from ArgsToOrder against the resource’s orderable-field mapping before d.OrderBy() generates SQL, rejecting unknown fields as a 400 while preserving valid mappings; update the related handling in pkg/db/sql_helpers.go and pkg/services/generic.go. In pkg/db/sql_helpers.go lines 72-73 and 747-750, apply the validation at both affected sites as appropriate, and add an unknown-field rejection case in test/integration/order_field_mapping_test.go lines 151-160.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.
Outside diff comments:
In `@pkg/db/sql_helpers.go`:
- Around line 72-73: Validate parsed ORDER BY fields from ArgsToOrder against
the resource’s orderable-field mapping before d.OrderBy() generates SQL,
rejecting unknown fields as a 400 while preserving valid mappings; update the
related handling in pkg/db/sql_helpers.go and pkg/services/generic.go. In
pkg/db/sql_helpers.go lines 72-73 and 747-750, apply the validation at both
affected sites as appropriate, and add an unknown-field rejection case in
test/integration/order_field_mapping_test.go lines 151-160.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 090b05a3-e57e-4e15-a581-563406d7702a
📒 Files selected for processing (4)
pkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/services/generic_test.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)
💤 Files with no reviewable changes (2)
- pkg/services/generic_test.go
- pkg/db/sql_helpers_test.go
a59d594 to
0838035
Compare
0838035 to
6a124b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/db/sql_helpers.go (1)
72-73: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winReject arbitrary identifiers here before they reach
sq.Expr.
getFieldreturnstrimmedNameunchanged for non-spec.inputs, and the TSL SQL walker emitsKindIdentifiervalues as raw SQL. That leaves a CWE-89 path unless every allowed field is validated against a strict grammar or mapped/quoted before returning it.🤖 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 72 - 73, Update getField so non-spec. inputs are not returned unchanged: validate them against a strict identifier grammar or resolve them through an allowlist and quote/map them before they reach sq.Expr. Preserve the existing handling for valid spec.-prefixed fields while rejecting any unsupported identifier input.Sources: Path instructions, MCP tools
🧹 Nitpick comments (1)
pkg/db/transaction_middleware.go (1)
79-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct unit coverage for
IsUndefinedColumnError.This exported classifier controls the 400/500 boundary. Add table-driven tests for nil, wrapped
*pq.Error{Code: "42703"}, another PostgreSQL code, and a non-pqerror; the integration test alone will not catch driver or wrapping regressions.As per path instructions, new exported functions and critical error paths SHOULD have tests.
🤖 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/transaction_middleware.go` around lines 79 - 84, Add direct table-driven unit tests for IsUndefinedColumnError covering nil, a wrapped *pq.Error with code 42703, a different PostgreSQL code, and a non-pq error. Assert only the expected boolean classification for each case, including wrapped errors, to protect the exported classifier’s behavior.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.go`:
- Around line 732-734: Update ArgsToOrder and its orderPattern validation to use
the same field grammar as getField, allowing valid digit-bearing names and
nested spec.* paths while retaining the existing direction handling and safety
checks; only restrict sorting to flat lowercase columns if that limitation is
explicitly documented.
In `@pkg/services/generic.go`:
- Around line 281-282: Update the error handling around d.Fetch and the
db.IsUndefinedColumnError(err) case so only errors explicitly tagged as
originating from user-supplied search or order fields return errors.BadRequest.
Propagate untagged undefined-column errors through the existing server-error
path, preserving the distinction between invalid query input and broken static
queries or schema drift.
---
Outside diff comments:
In `@pkg/db/sql_helpers.go`:
- Around line 72-73: Update getField so non-spec. inputs are not returned
unchanged: validate them against a strict identifier grammar or resolve them
through an allowlist and quote/map them before they reach sq.Expr. Preserve the
existing handling for valid spec.-prefixed fields while rejecting any
unsupported identifier input.
---
Nitpick comments:
In `@pkg/db/transaction_middleware.go`:
- Around line 79-84: Add direct table-driven unit tests for
IsUndefinedColumnError covering nil, a wrapped *pq.Error with code 42703, a
different PostgreSQL code, and a non-pq error. Assert only the expected boolean
classification for each case, including wrapped errors, to protect the exported
classifier’s behavior.
🪄 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: a5c85b26-8a02-4166-9740-65a60cf7c44f
📒 Files selected for processing (7)
pkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/db/transaction_middleware.gopkg/services/generic.gopkg/services/generic_test.gotest/integration/order_field_mapping_test.gotest/integration/search_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)
💤 Files with no reviewable changes (2)
- pkg/services/generic_test.go
- pkg/db/sql_helpers_test.go
| var orderPattern = regexp.MustCompile(`^[a-z_][a-z_]*(\s+(asc|desc))?$`) | ||
|
|
||
| // ArgsToOrder validates and cleans order arguments against the allowed fields whitelist. | ||
| // ArgsToOrder validates and cleans order arguments. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file map ==\n'
fd -a 'sql_helpers.go|.*\.go$' pkg/db || true
printf '\n== relevant ranges in pkg/db/sql_helpers.go ==\n'
nl -ba pkg/db/sql_helpers.go | sed -n '680,840p'
printf '\n== search for getField and order helpers ==\n'
rg -n "func .*getField|orderPattern|ArgsToOrder|ArgsToSearch|regexp.MustCompile|field contract|dotted" pkg/db/sql_helpers.go pkg/db -g '*.go'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pkg/db/sql_helpers.go around order helpers ==\n'
sed -n '720,820p' pkg/db/sql_helpers.go | cat -n
printf '\n== pkg/db/sql_helpers.go search for getField ==\n'
rg -n "func .*getField|getField\\(|orderPattern|ArgsToOrder|ArgsToSearch|fieldPattern|regexp.MustCompile" pkg/db/sql_helpers.go pkg/db/sql_helpers_test.go -g '*.go'
printf '\n== order/search tests ==\n'
sed -n '1,260p' pkg/db/sql_helpers_test.go | cat -nRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 14295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== getField implementation ==\n'
sed -n '1,120p' pkg/db/sql_helpers.go | cat -n
printf '\n== ArgsToOrder tests ==\n'
sed -n '940,1115p' pkg/db/sql_helpers_test.go | cat -n
printf '\n== ArgsToOrder call sites ==\n'
rg -n "ArgsToOrder\\(" -g '*.go' .Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 11023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pkg/services/generic.go around order handling ==\n'
sed -n '100,180p' pkg/services/generic.go | cat -n
printf '\n== any order-field allowlists / mappers ==\n'
rg -n "allowed.*order|order.*allowed|Order.*field|field.*Order|ArgsToOrder|Sort|orderBy|OrderBy" pkg/services pkg/db -g '*.go'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 4176
Align order-field validation with the existing field grammar (CWE-20). ArgsToOrder only accepts [a-z_][a-z_]*, so digit-bearing names and dotted paths are rejected before the DB layer, even though getField already accepts nested spec.* paths. Reuse the same field parser here, or document that sorting is limited to flat lowercase columns only.
🤖 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 732 - 734, Update ArgsToOrder and its
orderPattern validation to use the same field grammar as getField, allowing
valid digit-bearing names and nested spec.* paths while retaining the existing
direction handling and safety checks; only restrict sorting to flat lowercase
columns if that limitation is explicitly documented.
| case db.IsUndefinedColumnError(err): | ||
| return errors.BadRequest("invalid field in search or order query") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generic.go around the reported lines =="
sed -n '240,330p' pkg/services/generic.go
echo
echo "== where db.IsUndefinedColumnError is defined/used =="
rg -n "IsUndefinedColumnError|undefined_column|BadRequest\\(\"invalid field in search or order query\"\\)" pkg -SRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 3859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== db.IsUndefinedColumnError definition =="
sed -n '1,140p' pkg/db/transaction_middleware.go
echo
echo "== search/order query construction in generic service =="
rg -n "search|order|Sort|Order|FieldNameWalk|tslTree|walkFn|BadRequest\\(" pkg/services/generic.go -n -SRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 5714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GenericDao interface and Fetch implementations =="
rg -n "type GenericDao|func .*Fetch\\(" pkg -S
echo
echo "== Fetch definitions with surrounding context =="
for f in $(rg -l "func .*Fetch\\(" pkg -S); do
echo "--- $f ---"
sed -n '1,240p' "$f"
echo
doneRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 6342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== buildOrderBy and search pipeline =="
sed -n '120,240p' pkg/services/generic.go
echo
echo "== ArgsToOrder / FieldNameWalk definitions =="
rg -n "func ArgsToOrder|func FieldNameWalk|ExtractConditionQueries|ExtractLabelQueries|IdentWalk" pkg -SRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 5998
Do not map every undefined_column to 400. (CWE-703) d.Fetch can surface 42703 from a broken static query or schema drift, not just user-supplied search/order fields. Only convert tagged search/order errors to BadRequest; leave the rest as server errors.
🤖 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/generic.go` around lines 281 - 282, Update the error handling
around d.Fetch and the db.IsUndefinedColumnError(err) case so only errors
explicitly tagged as originating from user-supplied search or order fields
return errors.BadRequest. Propagate untagged undefined-column errors through the
existing server-error path, preserving the distinction between invalid query
input and broken static queries or schema drift.
1 participant
Summary
Removes the
searchAllowedFieldsandorderAllowedFieldsallowlists frompkg/db/sql_helpers.goso all fields are now valid search and order targetsAdds a handler in
loadListto return a 400 instead of a 500 when a field name passes syntax validation but doesn't exist in the database to preserve correct error classificationTicket: HYPERFLEET-1366
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)