Fix searchKey line detection for array-indexed paths#199
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: e921624 | Docs | Datadog PR Page | Give us feedback! |
whitemerch
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
522a641 to
7c0ccd9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
d391263 to
c2c254a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
06ac7d9 to
e921624
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e921624102
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| filtered := splitSanitized[:0] | ||
| for _, s := range splitSanitized { | ||
| if _, err := strconv.Atoi(s); err != nil { |
There was a problem hiding this comment.
When a searchKey crosses nested arrays, GetLineBySearchLine only builds the line-info array path for the last numeric segment, so a path such as spec.template.spec.containers.1.env.0.name falls through to this fallback. Dropping every numeric segment then searches ...containers.env.name, causing the text matcher to report the first env.name under the first container rather than the indexed container/env entry. Please keep the index context or extend the structured lookup instead of stripping all indices on failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The doubly-nested fallback limitation existed before in a more broken form.
2 participants
Motivation
searchKeypaths that cross an array boundary (e.g.rules.0.apiGroups) were resolving to the wrong line. The Levenshtein text-matcher indefaultDetectLinehas no concept of YAML list indices, when it hit a pure numeric segment like"0", it could not find matching text in the source file and broke early, reporting a line on the parent key instead of the target key inside the array element.This is the scanner-side fix that unblocks the default-rules PR, which simplifies rule authorship by consolidating line detection to
searchKeyonly.Changes
pkg/detector/default_detect.go: filter out pure numeric segments from the dot-splitsearchKeypath before passing it to the text-matching loop. The numeric index carries no literal text in the source file; removing it lets the detector advance to the next meaningful key (apiGroups,CidrIp, etc.) and report the correct line.Author Checklist
QA Instruction
Run the default-rules test suite against a build with this fix. The 43 rules that currently report a line on the parent key (e.g. the
rules:line instead of theapiGroups:line) should now report the precise target-field line.Blast Radius
Only
defaultDetectLine, which is the fallback detector used for all non-Terraform / non-Helm / non-Docker file kinds (YAML, JSON, BICEP, …). Terraform and Helm have dedicated detectors and are unaffected. The change only adds a pre-filter step; paths without numeric segments behave identically to before.Additional Notes
The
searchLinecode path invulnerability_builder.go(which usesGetLineBySearchLinefor structured tree navigation) is not removed in this PR — that cleanup will follow once the default-rules repo stops emittingsearchLinefields.I submit this contribution under the Apache-2.0 license.