fix: anchor capafmt's features offset to the features block - #3135
fix: anchor capafmt's features offset to the features block#3135Makeph wants to merge 2 commits into
Conversation
The indentation fix-up added for mandiant#263 needs to know where the features section starts, so that only descriptions inside features get the extra two spaces and the meta description is left alone. It located that point with `doc.find("features")`, which matches the first occurrence of the substring anywhere in the document. A rule whose namespace contains the word `features` (for example `namespace: impact/features`) therefore matched inside the meta block. The meta description was then re-indented from 4 spaces to 6, producing YAML that no parser accepts: capafmt corrupted the rule it was asked to format. Anchor the search to the top-level `features:` key instead. This survived six years because only a handful of nursery rules use a `/features` namespace and none of them carried a description field until mandiant/capa-rules#1173. Closes mandiant#3134
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed
mike-hunhoff
left a comment
There was a problem hiding this comment.
Great, thank you @Makeph for providing a fix, with tests to verify, and describing your AI usage 🚀
@Makeph please address this to get CI passing. |
@Makeph please address this before we can merge. |
CHANGELOG updated or no update needed, thanks! 😄
|
@Makeph please sign the CLA to get this merged, otherwise we'll need to close without merge. |
Closes #3134.
The bug
Rule.to_yaml()applies an indentation fix-up (added for #263) so thatdescriptionfields inside the features section get two extra spaces, while the meta description is left alone. To know where "inside features" begins, it did:str.findreturns the first occurrence of the substring anywhere in the document. For a rule whose namespace contains the wordfeatures— saynamespace: impact/features— that lands in the meta block. Everything after it, including the metadescription, then gets re-indented from 4 spaces to 6, and the result is YAML that no parser will load. capafmt corrupts the rule it was asked to format.The fix
Anchor the search to the top-level
features:key rather than a bare substring:One deliberate change in behaviour worth your call: I used
.index()rather than.find(). The existing comment right above states "assumes features section always exists", and.find()returning-1would silently apply the fix-up to the whole document — the same class of failure as this bug..index()makes that assumption fail loudly instead. Happy to switch back to.find()if you'd rather keep the change strictly minimal.Why it took six years to surface
Only a handful of nursery rules use a
/featuresnamespace, and none carried adescriptionfield until mandiant/capa-rules#1173. It needed both to appear in the same rule.Testing
Added
test_rule_reformat_meta_description_with_features_namespace, which round-trips a rule withnamespace: impact/featuresand a meta description.Verified locally against this branch:
6 passedintests/test_fmt.pycapa/rules/__init__.pyand keeping the test:1 failed, 5 passed, failing withyaml.parser.ParserError: while parsing a block collection ... did not find expected '-' indicator— i.e. the test reproduces the exact corruption from the issuetests/test_rules.py tests/test_fmt.py tests/test_rule_cache.py:45 passedPrepared with AI assistance; I reviewed every line and ran the verification above locally.