HYPERFLEET-1259 - fix: SQL injection protection for orderBy/search

[ma-hill]

Summary

Implements allowlist-based SQL injection protection for orderBy and search query parameters. Previously, the API validated field names against a disallowedFields blocklist, but attackers could potentially inject SQL functions (like pg_sleep(), version()) or arbitrary column names that weren't explicitly blocked. This adds a comprehensive allowlist (OrderByAllowedFields) per resource type that explicitly enumerates valid sortable/searchable columns, rejecting anything not in the allowlist.

HYPERFLEET-1259

Changes

• Added OrderByAllowedFields map in pkg/db/sql_helpers.go defining valid columns for Cluster, NodePool, and Resource types (includes standard fields like id, name, created_time, status_conditions, plus resource-specific fields like owner_id for NodePools)
• Updated getField() to validate field names against the allowlist after existing checks, returning HYPERFLEET-VAL-005 error for disallowed fields
• Threaded allowedFields parameter through FieldNameWalk(), cleanOrderBy(), and ArgsToOrderBy() functions to enforce validation at every field name resolution point
• Added validation in newListContext() to ensure resource types have allowlist configured, returning clear error message if missing
• Refactored cleanOrderBy() to use explicit error returns instead of naked returns, improving readability
• Fixed typo: trimedName → trimmedName
• Updated existing unit tests to pass nil for allowedFields where allowlist validation is not under test

Test Plan

• Unit tests added/updated
• make test-all passes
• make lint passes
• Integration test validates SQL injection protection through full HTTP request flow

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[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]

Caution

Review failed

An error occurred during the review process. Please try again later.

📝 Walkthrough

Summary by CodeRabbit

Release Notes

• New Features

  • Implemented field allowlist validation for sorting and search operations on resources.

• Improvements

  • Sort and search requests with invalid or disallowed fields now return HTTP 400 errors with validation error codes.

• Tests

  • Added integration tests validating field behavior for sorting and search operations across resource types.

Walkthrough

Adds a per-resource OrderByAllowedFields allowlist to pkg/db/sql_helpers.go and enforces it in getField, returning BadRequest for non-allowlisted field names. The FieldNameWalk and ArgsToOrderBy exported signatures are updated to accept allowedFields map[string]bool, propagated through all recursive paths. cleanOrderBy gains stricter direction and token-count parsing. In pkg/services/generic.go, listContext carries allowedFields loaded from db.OrderByAllowedFields at construction time, wired into both buildOrderBy and buildSearchValues. Integration tests assert HTTP 400 (HYPERFLEET-VAL-005) for SQL injection attempts in orderBy and search parameters.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 9 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
No Injection Vectors	⚠️ Warning	SQL injection vector (CWE-89) exists in propertiesNodeConverter (line 223): propertyName extracted from user TSL without validation, directly interpolated via fmt.Sprintf("properties ->> '%s'", pro...	Apply validateLabelKey to propertyName in propertiesNodeConverter before fmt.Sprintf interpolation, or use parameterized SQL instead of string concatenation.

✅ Passed checks (9 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title directly matches the changeset: implements allowlist-based SQL injection protection for orderBy and search parameters, which is the core objective across all modified files.
Description check	✅ Passed	Description comprehensively covers the changeset: documents the allowlist mechanism, parameter threading, validation logic, test coverage, and references HYPERFLEET-1259.
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	No logging statements in modified non-test files (pkg/db/sql_helpers.go, pkg/services/generic.go) include tokens, passwords, credentials, or secrets. All logger calls contain only hardcoded, generi...
No Hardcoded Secrets	✅ Passed	No hardcoded secrets detected. PR contains legitimate field allowlists and test utilities using proper authentication patterns.
No Weak Cryptography	✅ Passed	No weak cryptographic primitives found. PR contains no crypto/md5, crypto/des, crypto/rc4, SHA1-for-security, ECB mode, or custom crypto implementations. No unsafe token/secret/HMAC comparisons det...
No Privileged Containers	✅ Passed	No Kubernetes/OpenShift manifests, Helm templates, or Dockerfiles modified. PR contains only Go application code changes for SQL injection protection.
No Pii Or Sensitive Data In Logs	✅ Passed	No logging statements (slog, logr, zap, log, fmt.Print*) were added. New error messages contain no PII/sensitive data exposure.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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.}

[ma-hill]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.