fix: reject matchers and extractors with no values - #7605
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughMatchers and extractors now reject configurations missing their type-specific values during compilation. Tests cover missing and valid configurations, DSL matcher compilation, and a mock matcher updated with a required word value. ChangesOperator value validation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
a80d45c to
de47352
Compare
|
Thanks for the review @Mzack9999. Rebased all three of my open PRs onto current Your approval on #7605 carried over the force-push, so nothing is lost there. On the #7603 conflict, since it touched shared codeThe conflict was in ownsMetadataIndex := cfg.MetadataIndex == nil
store.metadataIndex = cfg.MetadataIndex
if store.metadataIndex == nil {
store.metadataIndex = store.loadTemplatesIndex()
}while my change added I re-verified the fix still does what it claims after the rebase, rather than assuming the tests passing was enough. Pointing validation back at the parsed cache (the pre-fix behaviour) flips exactly the test that should flip: The second one passing under both implementations is the point: it confirms the fix is not just refusing more templates.
Happy to squash or reword any of these if that helps them land. |
|
Thanks for pushing the fixture fix — I had just reproduced the same four failures locally when your commit landed, so let me add what I found, including one part that is not fixed by that commit. The three that your fix covers
So the failure was the change working as intended on a fixture that had been silently invalid. Verified on your commit: The two in
|
|
Windows came back red. I checked before saying anything, and it does not look related to this change. What failedThe
Why it is not this PRThis change only makes matcher/extractor compilation reject empty values. Everything it can affect is green on the same run:
The two Where that leaves itmacOS and ubuntu are green on this branch, and |
|
The red No individual test failed. Four packages crashed outright: with This PR touches none of those packages: And the same crash happens on a PR that isn't mine. #7598 shows the identical goleveldb stack on So it reproduces independently of this change. The I couldn't find an existing issue tracking it. Happy to open one with these two runs attached if that's useful, though it seemed better to ask than to file something that may already be known internally. The other checks are green, and |
|
@Mzack9999 thanks for the approval. Flagging that the one red check is not this PR, in case it is what is holding the merge.
What the Windows runner actually did: Three things say runner rather than code:
It is also not unique to this PR. On the same day, #7583 hit Nothing has changed on the branch since the approval, so a re-run of the Windows job is all that is needed. Happy to rebase onto the current |
2b1c91a to
579eb02
Compare
|
Rebased onto current The reason: the red
Verified locally on the rebased branch before pushing: Nothing to review again — flagging it only so the force-push does not look like a silent change under an approval. |
|
Windows is green after the rebase, so that half worked. The new red is The chain: // pkg/types/types.go:826 — DefaultOptions()
Logger: &gologger.Logger{}, // zero value: formatter is nil
// pkg/catalog/loader/loader.go:162
logger: cfg.Logger,
// pkg/catalog/loader/loader.go:196 — inside saveMetadataIndexOnce
store.logger.Warning().Msgf("Could not save metadata cache: %v", err)
This PR touches six files, all under Local state on the rebased branch: So both red jobs this PR has seen were runtime-level failures in Happy to send the |
|
The evidence that it is not either of my PRs: #7603 and #7605 fail the identical test with the identical stack, and they share no files — #7603 is two files under Both PRs are otherwise green: Lint, Spell Check, Tests (macOS), Tests (windows), CodeRabbit. |
|
The Short version: Nothing to change here. This PR and #7605 fail that test identically while sharing no files, and both are green on Lint, Spell Check, Tests (macOS) and Tests (windows). |
|
Confirmed by CI: #7614 is green on Same runner image, same test, and the only difference is the one-line change in For completeness, this PR is green on everything else: Lint, Spell Check, Tests (macOS), Tests (windows), CodeRabbit. No action needed from me — flagging it so the red check does not read as a reason to hold this one. |
dwisiswant0
left a comment
There was a problem hiding this comment.
- Error wording so clumsy.
- Bloated - helpers feel unnecessary in this situation.
- Still accepts empty-value entries - which become match-all conditions through empty-string matching.
Fixes #7604.
What was wrong
A matcher or extractor with no values compiled, validated and ran — then silently never matched. Proven with a controlled comparison against a local server, same target and template shape:
Every matcher type accepted it (
compile err=<nil>forword,regex,binary,status,size,dsl,xpath), and likewise for extractors.It is hiding real breakage in nuclei-templates today
This is the part worth reviewing. Running the fixed binary over the entire
nuclei-templatescorpus (13,342 templates) fails exactly 4 — and I inspected every one by hand. All four are genuinely broken:http/cves/2021/CVE-2021-44228.yaml(Log4Shell)kvalkval:present but emptyhttp/cves/2021/CVE-2021-45046.yamlkvalkval:present but emptyhttp/cves/2026/CVE-2026-42281.yamldsldsl:key is missing entirely — its list items are orphaned undername: dns, so the whole second matcher is inertdns/dns-saas-service-detection.yamlwordtype: wordwithpart/namebut nowords:Zero false positives — no other template in the corpus is affected.
Note the two Log4Shell ones would not be caught by a naive "is the key present" scan: the key exists with an empty value. That's why the check tests the parsed value rather than the YAML shape.
The fix
A
checkRequiredValues()on each side, called from the existing validation/compile path:Matcher.Validate(), right after the existingcheckFields()CompileExtractors(), next to the existingRegexGroup < 0guardBoth reuse the type→field mapping the code already expresses in its
switchon the operator type, and produce a message naming the missing field:No new concept — this sits alongside guards that already exist in both files.
Two existing tests updated, deliberately
TestMatcher_MatchDSLandTestMatcher_MatchDSL_ErrorHandlingconstructed a matcher by pre-seeding the unexporteddslCompiledfield while leavingDSLempty, to isolateMatchDSL.dslCompiledis only ever populated frommatcher.DSLduring compile, so that shape isn't reachable from a template. I changed them to declareDSL: [...]— the same way a template does — which keeps what they test while going through the real compile path.Tests
TestMatcherRejectsMissingValuesTestMatcherAcceptsProvidedValuesTestExtractorRejectsMissingValuesTestExtractorAcceptsProvidedValuesThe "accepts" pairs are the guard rails — a change that rejected too much would break them.
Bite-proofed: reverting the two source files makes every
Rejectssubtest fail.Verification
go test ./pkg/operators/... ./pkg/templates/... ./pkg/catalog/...— all ok.gofmtclean.One coordination note
This will fail those 4 templates in
nuclei-templatesuntil they're fixed. They're broken today and silently doing nothing, so I'd argue surfacing them is the point — but if you'd rather land this behind a flag, or fix the templates first, I'm happy to adjust. Flagged the same thing on #7604 before opening this.Summary by CodeRabbit