HYPERFLEET-1376 - fix: avoid leaking internal type names in validation errors#306
HYPERFLEET-1376 - fix: avoid leaking internal type names in validation errors#306mliptak0 wants to merge 4 commits 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 |
📝 WalkthroughWalkthroughJSON unmarshalling type mismatches now produce structured validation errors naming the affected field and expected JSON kind across shared, force-delete, and create-with-no-content handlers. Other decoding failures retain existing fallback behavior. Patch validation now accepts Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@pkg/handlers/framework.go`:
- Around line 159-172: Update describeJSONKind to explicitly map all numeric
reflect.Kind values to "a number", and handle unsupported kinds such as
reflect.Ptr, reflect.Interface, and reflect.Invalid with a safe non-numeric
fallback. Remove the unjustified catch-all behavior so unhandled enum values
cannot produce factually incorrect API errors.
In `@pkg/handlers/validation.go`:
- Around line 253-261: Guard the reflection setup before dereferencing in the
validation flow: validate that reflect.ValueOf(i) is valid, is a pointer or
interface, and is non-nil before calling Elem, returning the existing validation
error path for invalid inputs. Before each IsNil call for Spec, Labels, and
References, only call it for nil-capable kinds (pointer, slice, map, channel,
func, or interface); treat other valid concrete fields as present without
panicking.
🪄 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: e5824cff-bef1-4940-87e6-d39a49bdbe4d
📒 Files selected for processing (2)
pkg/handlers/framework.gopkg/handlers/validation.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)
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 200 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
…n errors Co-authored-by: Cursor <cursoragent@cursor.com>
3a2f3c5 to
21a802a
Compare
mliptak0
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
…requests
handleForceDelete (POST /{plural}/{id}/force-delete) was still leaking
the Go struct name in its error detail (e.g. "cannot unmarshal number
into Go struct field ForceDeleteRequest.reason of type string") because
it didn't route decode errors through cleanTypeMismatchError like the
other three decode paths. Applying the same check there closes the last
unsanitized endpoint.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…d references-only patch fixes Re-adds the regression tests for findings 1-4 (labels/conditions type mismatch clean messages, references-only PATCH) that were dropped when the framework.go/validation.go fixes were rewritten in 21a802a and 0576411. Assertions updated to match the current cleanTypeMismatchError message format ("field '<name>' must be <kind>"). Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@mliptak0: 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/handlers/resource_handler_test.go`:
- Around line 279-297: Strengthen the “Success - references only” test’s Patch
expectation by capturing or structurally matching the *api.ResourcePatch
argument instead of accepting any value. Assert that References preserves the
parents entry with kind “Cluster” and ID “cluster-1”, while retaining the
existing successful response and status assertions.
🪄 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: 003292cb-bbd6-4542-8473-1e2155e8f961
📒 Files selected for processing (4)
pkg/handlers/framework.gopkg/handlers/resource_handler_test.gopkg/handlers/resource_status_handler_test.gopkg/handlers/validation_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)
| { | ||
| // References-only patch must be accepted, not rejected as "no field provided" — HYPERFLEET-1376 finding 8. | ||
| name: "Success - references only", | ||
| id: "ch-123", | ||
| body: `{"references":{"parents":[{"kind":"Cluster","id":"cluster-1"}]}}`, | ||
| setupMock: func(mock *services.MockResourceService) { | ||
| mock.EXPECT().Patch(gomock.Any(), "Channel", "ch-123", gomock.AssignableToTypeOf(&api.ResourcePatch{})). | ||
| Return(&api.Resource{ | ||
| Meta: api.Meta{ID: "ch-123", CreatedTime: now, UpdatedTime: now}, | ||
| Kind: "Channel", | ||
| Name: "stable", | ||
| Spec: datatypes.JSON(`{"is_default":false}`), | ||
| Generation: 2, | ||
| CreatedBy: "user@test.com", | ||
| UpdatedBy: "user@test.com", | ||
| }, nil) | ||
| }, | ||
| expectedStatusCode: http.StatusOK, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the references payload passed to Patch.
Line 285 accepts any *api.ResourcePatch, so this passes if decoding or handler wiring drops References before the service call. Capture or match the argument and verify the parents reference, kind, and ID; Sentinel relies on this map/array contract being preserved.
As per path instructions, “New exported functions and critical logic 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/handlers/resource_handler_test.go` around lines 279 - 297, Strengthen the
“Success - references only” test’s Patch expectation by capturing or
structurally matching the *api.ResourcePatch argument instead of accepting any
value. Assert that References preserves the parents entry with kind “Cluster”
and ID “cluster-1”, while retaining the existing successful response and status
assertions.
Sources: Path instructions, Linked repositories
1 participant
Summary
What
ResourcePatchRequest,openapi.ConditionRequest) in 400 error messages when a request sends the wrong JSON type forlabelsorconditions.references(nolabels/spec) — these were wrongly rejected with "at least one field must be provided for update".Why
API callers shouldn't see our internal Go type names in error messages, and a references-only PATCH is a perfectly valid update that shouldn't be blocked.
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)