HYPERFLEET-1366 - refactor: remove search and order field allowlists

[Mischulee]

Summary

Removes the searchAllowedFields and orderAllowedFields allowlists from pkg/db/sql_helpers.go so all fields are now valid search and order targets.

• Ticket: HYPERFLEET-1366

Test Plan

• Unit tests added/updated
• make test-all passes
• make lint passes
• Helm chart changes validated with make test-helm (if applicable)
• Deployed to a development cluster and verified (if Helm/config changes)
• E2E tests passed (if cross-component or major changes)

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ldornele for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Expanded support for querying and sorting by field names beyond the previously restricted allowlists.
  • Retained validation for field format, nesting depth, and sort direction.

• Tests

  • Updated unit, service, and integration coverage to reflect the broader field support.

Walkthrough

getField no longer checks non-JSONB fields against searchAllowedFields. It still rejects names with more than two dotted components. ArgsToOrder no longer checks fields against orderAllowedFields; syntax and direction validation remain. The obsolete ordering whitelist and tests for rejected fields were removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
No Injection Vectors	⚠️ Warning	CWE-89: removing the search allowlist lets request identifiers such as id/1 reach sq.Expr as raw SQL (resources.id/1 = ?).	Validate regular field names with a strict identifier grammar or quote identifiers before passing them to SQL; retain parameter binding for values.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Sec-02: Secrets In Log Output	✅ Passed	The only changed non-test source is pkg/db/sql_helpers.go; its diff adds no logging call or secret-related field, and no slog, log, logr, zap, or fmt.Print* call exists.
No Hardcoded Secrets	✅ Passed	The diff adds only a comment. It contains no API keys, credentials, private keys, credential URLs, or long base64 literals; CWE-798 is not triggered.
No Weak Cryptography	✅ Passed	PR modifies database field allowlists only. No cryptographic imports, banned primitives (crypto/md5, crypto/des, crypto/rc4, SHA1 for security), ECB mode, custom crypto implementations, or non-cons...
No Privileged Containers	✅ Passed	No privileged Kubernetes settings were found; Dockerfile USER root is justified for dnf, then switches to USER 1001, while runtime uses USER 65532 and Helm defaults enforce non-root execution.
No Pii Or Sensitive Data In Logs	✅ Passed	The four-file PR patch adds no slog, logr, zap, log, or fmt.Print* statements and exposes no log data; no CWE-532 finding applies.
Title check	✅ Passed	The title clearly identifies the removal of search and order field allowlists, which is the main change.
Description check	✅ Passed	The description directly explains the allowlist removal and documents the related test and validation results.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

✨ Simplify code

• Create PR with simplified code

---

_{Comment @coderabbitai help to get the list of available commands.}

[hyperfleet-ci-bot]

Risk Score: 0 — risk/low

Signal	Detail	Points
PR size	72 lines	+0
Sensitive paths	none	+0
Test coverage	Tests cover changed packages	+0

_{Computed by hyperfleet-risk-scorer}

[coderabbitai]

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)

772-772: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Allow digits in order field names.

After Line 772 removes the whitelist check, orderPattern is the only field validator. Its actual pattern excludes digits, so field2 desc still returns invalid order format. The pattern comment claims that digits are accepted, but the regex does not. (github.com)

Update the pattern and add a regression test:

-var orderPattern = regexp.MustCompile(`^[a-z_][a-z_]*(\s+(asc|desc))?$`)
+var orderPattern = regexp.MustCompile(`^[a-z_][a-z0-9_]*(\s+(asc|desc))?$`)

As per PR objectives, “all fields are now valid search and order targets.”

🤖 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` at line 772, The order-field validation pattern used
by orderPattern must accept digits in field names, matching its documented
behavior and allowing inputs such as field2 desc. Update the pattern while
preserving existing direction validation, and add a regression test covering a
digit-containing order field; all fields should remain valid search and order
targets.

🤖 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`:
- Line 772: The order-field validation pattern used by orderPattern must accept
digits in field names, matching its documented behavior and allowing inputs such
as field2 desc. Update the pattern while preserving existing direction
validation, and add a regression test covering a digit-containing order field;
all fields should remain valid search and order targets.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 582f02a5-eae8-47f0-8f87-3da82c9b2d8a

📥 Commits

Reviewing files that changed from the base of the PR and between 2227af2 and e50d067.

📒 Files selected for processing (4)

• pkg/db/sql_helpers.go
• pkg/db/sql_helpers_test.go
• pkg/services/generic_test.go
• test/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 (3)

• test/integration/order_field_mapping_test.go
• pkg/db/sql_helpers_test.go
• pkg/services/generic_test.go

[pnguyen44]

The spec.* branch validates identifiers via validateJSONBKey (^[a-z0-9_]+$), but removing the search allowlist leaves the plain-field branch of getField with no character-level validation. Identifiers like id/1 or version() pass through to raw SQL text. Consider adding a similar regex for non-spec field segments.

[pnguyen44]

The PR removes four tests that guarded against invalid field names (including the properties.owner test whose own comment noted it prevented raw pq: error leakage). With the allowlists gone, replacement coverage would help:

1. getField plain-field branch — no unit tests exist for non-spec.* inputs. Positive cases (id, created_at, status.phase) would document the intended behavior; negative cases would be needed once character validation is added.
2. orderPattern with digits — the regex ^[a-z_][a-z_]* rejects digit-containing fields, so release_v2 asc fails despite being valid. A test case would surface this.
3. Search with nonexistent column — now reachable without the allowlist. An integration test asserting a clean 400 (not a raw pq: error) would replace the removed properties.owner guard.