FLPATH-4634 | fix(catalog): parse array indices in nested field paths - #34
Open
LinskId wants to merge 1 commit into
Open
Conversation
Assisted by: Cursor Signed-off-by: igavra <igavra@redhat.com>
LinskId
requested review from
chadcrum,
gabriel-farache,
gciavarrini,
jenniferubah and
machacekondra
July 26, 2026 15:40
PR Summary by QodoFix catalog nested field paths to support array indices
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Unbounded index allocation
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+183
to
+197
| raw, exists := parent[name] | ||
| var slice []any | ||
| if !exists { | ||
| slice = make([]any, index+1) | ||
| } else { | ||
| var ok bool | ||
| slice, ok = raw.([]any) | ||
| if !ok { | ||
| return nil, fmt.Errorf("path segment %q is not an array", pathSoFar) | ||
| } | ||
| if index >= len(slice) { | ||
| grown := make([]any, index+1) | ||
| copy(grown, slice) | ||
| slice = grown | ||
| } |
There was a problem hiding this comment.
1. Unbounded index allocation 🐞 Bug ☼ Reliability
setNestedValue/ensureSliceElementMap allocate/grow slices to length index+1 from the parsed path without any maximum bound or overflow guard, so a large (but valid) index can cause OOM or a runtime panic (e.g., index==MaxInt makes index+1 overflow) during spec building. These functions are called with catalog field paths in spec_builder, so a misconfigured/malicious catalog field path can crash the service while merging defaults/user values.
Agent Prompt
### Issue description
Indexed path segments (e.g., `disks[123]`) directly drive slice allocation via `make([]any, index+1)` and growth to `index+1` in multiple places. With no cap and no overflow guard, large indices can exhaust memory or panic due to `index+1` overflow.
### Issue Context
These helpers are used when building resource specs from catalog field defaults and user values.
### Fix Focus Areas
- Add an explicit maximum allowed array index (and/or maximum slice length) and return a descriptive error if exceeded.
- Add an overflow guard before computing `index+1` (e.g., reject indices where `index+1` would overflow or exceed the max).
- Apply the same validation consistently for both intermediate segments (`ensureSliceElementMap`) and last-segment indexed sets (`setNestedValue`).
- Add unit tests that assert large indices are rejected without attempting allocation.
- internal/catalog/service/nested_map.go[21-44]
- internal/catalog/service/nested_map.go[93-110]
- internal/catalog/service/nested_map.go[181-199]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+206
to
+209
| elem, ok := slice[index].(map[string]any) | ||
| if !ok { | ||
| return nil, fmt.Errorf("path segment %q[%d] is not a map", pathSoFar, index) | ||
| } |
There was a problem hiding this comment.
2. Duplicated index in error 🐞 Bug ⚙ Maintainability
ensureSliceElementMap can produce misleading error text by appending "[index]" to pathSoFar even though pathSoFar already includes the indexed segment, resulting in messages like "storage.disks[0][0]". This makes troubleshooting malformed shapes harder and can confuse error matching.
Agent Prompt
### Issue description
An error path is formatted as `pathSoFar[index]`, but `pathSoFar` already includes the bracketed index from the original segment (e.g., `storage.disks[0]`). This duplicates the index in error messages.
### Issue Context
`pathSoFar` is built from the original `parts` strings (which still contain `[n]`).
### Fix Focus Areas
- Change the error format to avoid re-appending `[index]` (e.g., `fmt.Errorf("path segment %q is not a map", pathSoFar)`), or pass an unindexed path string into `ensureSliceElementMap`.
- internal/catalog/service/nested_map.go[55-61]
- internal/catalog/service/nested_map.go[206-210]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
chadcrum
approved these changes
Jul 28, 2026
| } | ||
|
|
||
| // parsePathSegment parses "key" or "key[n]"; malformed brackets return an error. | ||
| func parsePathSegment(seg string) (pathSegment, error) { |
Contributor
There was a problem hiding this comment.
can't we use a regexp for that?
var segRe = regexp.MustCompile(`^([^\[\]]+?)(?:\[(\d+)\])?$`)
func parsePathSegment(seg string) (pathSegment, error) {
if seg == "" {
return pathSegment{}, fmt.Errorf("path segment cannot be empty")
}
m := segRe.FindStringSubmatch(seg)
if m == nil {
return pathSegment{}, fmt.Errorf("invalid path segment %q: expected name or name[index]", seg)
}
if m[2] == "" {
return pathSegment{name: m[1]}, nil
}
idx, _ := strconv.Atoi(m[2])
return pathSegment{name: m[1], index: idx, hasIndex: true}, nil
}
machacekondra
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test plan:
Code generation succeeds
go build ./... passes
go test ./internal/catalog/service/ -ginkgo.focus='array index paths|FLPATH-4634' -v passed (10 Passed 0 failed)
make fmt vet lint clean (0 issues)