Skip to content

fix: reject matchers and extractors with no values - #7605

Open
DPS0340 wants to merge 2 commits into
projectdiscovery:devfrom
DPS0340:fix/reject-empty-operators
Open

fix: reject matchers and extractors with no values#7605
DPS0340 wants to merge 2 commits into
projectdiscovery:devfrom
DPS0340:fix/reject-empty-operators

Conversation

@DPS0340

@DPS0340 DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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:

control  (words: ["a"]) → [probe-ctl] [http] [info] http://127.0.0.1:18099
empty    (no words:)    → No results found

Every matcher type accepted it (compile err=<nil> for word, 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-templates corpus (13,342 templates) fails exactly 4 — and I inspected every one by hand. All four are genuinely broken:

template operator what's actually wrong
http/cves/2021/CVE-2021-44228.yaml (Log4Shell) kval kval: present but empty
http/cves/2021/CVE-2021-45046.yaml kval kval: present but empty
http/cves/2026/CVE-2026-42281.yaml dsl the dsl: key is missing entirely — its list items are orphaned under name: dns, so the whole second matcher is inert
dns/dns-saas-service-detection.yaml word type: word with part/name but no words:

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:

  • matchers: called from Matcher.Validate(), right after the existing checkFields()
  • extractors: called from CompileExtractors(), next to the existing RegexGroup < 0 guard

Both reuse the type→field mapping the code already expresses in its switch on the operator type, and produce a message naming the missing field:

matcher word has no words values specified
extractor kval has no kval values specified

No new concept — this sits alongside guards that already exist in both files.

Two existing tests updated, deliberately

TestMatcher_MatchDSL and TestMatcher_MatchDSL_ErrorHandling constructed a matcher by pre-seeding the unexported dslCompiled field while leaving DSL empty, to isolate MatchDSL. dslCompiled is only ever populated from matcher.DSL during compile, so that shape isn't reachable from a template. I changed them to declare DSL: [...] — the same way a template does — which keeps what they test while going through the real compile path.

Tests

test asserts
TestMatcherRejectsMissingValues all 7 matcher types error, message names the field
TestMatcherAcceptsProvidedValues all 7 still compile when values are present
TestExtractorRejectsMissingValues all 5 extractor types error
TestExtractorAcceptsProvidedValues all 5 still compile when values are present

The "accepts" pairs are the guard rails — a change that rejected too much would break them.

Bite-proofed: reverting the two source files makes every Rejects subtest fail.

Verification

go test ./pkg/operators/... ./pkg/templates/... ./pkg/catalog/... — all ok. gofmt clean.

One coordination note

This will fail those 4 templates in nuclei-templates until 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

  • Bug Fixes
    • Extractors now fail compilation when the selected extractor type has no required values configured, preventing silent no-op behavior.
    • Matchers now fail validation when the selected matcher type is missing required values.
  • Tests
    • Added table-driven tests ensuring extractors and matchers both reject missing required values and accept valid configurations.
    • Updated DSL matcher tests to build expressions via the DSL field and verify compilation and error-handling behavior.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0626fbe7-c319-428b-b3d2-8b18fa4ccc3f

📥 Commits

Reviewing files that changed from the base of the PR and between 2b1c91a and 579eb02.

📒 Files selected for processing (6)
  • pkg/operators/extractors/compile.go
  • pkg/operators/extractors/extract_test.go
  • pkg/operators/matchers/match_test.go
  • pkg/operators/matchers/validate.go
  • pkg/operators/matchers/validate_test.go
  • pkg/protocols/file/operators_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/protocols/file/operators_test.go
  • pkg/operators/matchers/validate.go
  • pkg/operators/extractors/compile.go

Walkthrough

Matchers 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.

Changes

Operator value validation

Layer / File(s) Summary
Extractor required-value validation
pkg/operators/extractors/compile.go, pkg/operators/extractors/extract_test.go
Extractor compilation checks the configured type’s required value field and tests missing-value errors alongside valid configurations.
Matcher required-value validation
pkg/operators/matchers/validate.go, pkg/operators/matchers/validate_test.go, pkg/operators/matchers/match_test.go, pkg/protocols/file/operators_test.go
Matcher validation rejects empty type-specific values, DSL tests compile expressions through Matcher.DSL, and the mock operator supplies a word value.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

Suggested reviewers: mzack9999

Poem

A rabbit found empty fields in the lane,
So matchers and extractors now complain.
DSLs compile where strings belong,
Missing values can’t sneak along.
Hop, hop—silent no-ops are gone!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: rejecting matchers and extractors with empty values.
Linked Issues check ✅ Passed The changes implement the issue's required validation for empty matcher/extractor values and add coverage.
Out of Scope Changes check ✅ Passed The DSL test updates and file-operator fixture change support the new validation and stay within scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @Mzack9999.

Rebased all three of my open PRs onto current dev — they were 6420 commits behind, and #7603 had gone dirty in the meantime. All three are unstable now (CI running) rather than conflicting:

#7601  fix: avoid panic in BuildRequest when request response has no request
#7603  fix: make -validate report operator compilation errors
#7605  fix: reject matchers and extractors with no values   <- approved

Your approval on #7605 carried over the force-push, so nothing is lost there.

On the #7603 conflict, since it touched shared code

The conflict was in pkg/catalog/loader/loader.go, in the block that initialises the store. Upstream added the MetadataIndex reuse:

ownsMetadataIndex := cfg.MetadataIndex == nil
store.metadataIndex = cfg.MetadataIndex
if store.metadataIndex == nil {
    store.metadataIndex = store.loadTemplatesIndex()
}

while my change added compiledParserCacheOnce just above it. They are independent, so I kept both — the new cache initialiser first, then upstream's metadata-index block unchanged. No upstream logic was dropped or reordered.

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:

TestValidateTemplatesReportsOperatorCompileErrors   FAIL
TestValidateTemplatesAcceptsValidOperators          PASS   <- guard rail, passes either way

The second one passing under both implementations is the point: it confirms the fix is not just refusing more templates.

go test ./pkg/catalog/loader/..., ./pkg/operators/... and ./pkg/input/... are all green on the rebased branches.

Happy to squash or reword any of these if that helps them land.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for pushing the fixture fix — Words: []string{"test"} is the right call, and macOS is green now.

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

TestFileOperatorMatch, TestFileOperatorExtract and TestFindInputPaths all trace back to the same newMockOperator() in pkg/protocols/file/operators_test.go. The fixture declared a WordsMatcher with no Words, which is exactly what this PR now rejects:

could not compile operators: could not compile matcher: matcher word has no words values specified

So the failure was the change working as intended on a fixture that had been silently invalid. Verified on your commit:

go test ./pkg/protocols/file/...        ok
go test ./pkg/protocols/offlinehttp/... ok

The two in pkg/protocols/network are pre-existing

These two still fail:

--- FAIL: TestNetworkGeneratorPayloadInteractshMarkerRendersBeforeInput
--- FAIL: TestNetworkCompileDefersInteractshMarkerHelpersToRuntime
        Messages: could not execute network request

I checked whether they are mine before saying anything, by running them on origin/dev with none of my changes:

origin/dev (clean)  -> both FAIL, same message
this PR             -> both FAIL, same message

Identical on both sides, and the failure is could not execute network request rather than an operator-compilation error, so it is unrelated to this PR. They look environment-dependent (both involve interactsh markers and an actual network request). Not something I should paper over here, but worth knowing they are red independently of this branch.

Where this leaves the PR

Lint            success
Spell Check     success
Tests (macOS)   success
Tests (ubuntu)  running
Tests (windows) running

If ubuntu or windows comes back red on those two network tests, that would be the pre-existing failure rather than this change. Happy to help track that down separately if it is useful.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Windows came back red. I checked before saying anything, and it does not look related to this change.

What failed

FAIL  lib/tests                    3960.096s   <- 66 minutes, then killed
FAIL  pkg/js/libs/grpc              0.710s
FAIL  pkg/protocols/http           16.702s
FAIL  pkg/protocols/network         0.848s

The lib/tests stack trace ends here:

lib.(*NucleiEngine).ExecuteCallbackWithCtx
    D:/a/nuclei/nuclei/lib/sdk.go:328
lib/tests_test.TestSimpleNuclei.func1
    D:/a/nuclei/nuclei/lib/tests/sdk_test.go:50

sdk_test.go:50 is ne.ExecuteWithCallback(nil) on a target of scanme.sh — a real DNS scan against an external host. It hung for 66 minutes and the run was killed, which is what took the job down.

Why it is not this PR

This change only makes matcher/extractor compilation reject empty values. Everything it can affect is green on the same run:

ok  pkg/operators                 0.154s
ok  pkg/operators/cache           0.032s
ok  pkg/operators/common/dsl      0.371s
ok  pkg/operators/extractors      0.155s
ok  pkg/operators/matchers        0.141s

TestHasOperatorsPanicRegression passes too. There is no could not compile / no values specified anywhere in the windows log — the fixture problem you fixed in 2b1c91a is gone, and nothing replaced it.

The two pkg/protocols/network failures are the pre-existing ones I mentioned earlier: they reproduce on clean origin/dev with none of my changes, and fail with could not execute network request.

Where that leaves it

macOS and ubuntu are green on this branch, and dev's own latest run is green on all three, so this looks like the network-dependent tests being flaky on the windows runner rather than a real regression. Happy to have it re-run if you want to confirm, but I would rather not push an empty commit at it while the cause is external.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

The red Tests (windows-latest) on this PR is not caused by it — evidence below, since a failing check is the obvious reason to skip a PR.

No individual test failed. Four packages crashed outright:

FAIL github.com/projectdiscovery/nuclei/v3/lib/tests                3960.096s
FAIL github.com/projectdiscovery/nuclei/v3/pkg/js/libs/grpc            0.710s
FAIL github.com/projectdiscovery/nuclei/v3/pkg/protocols/http         16.702s
FAIL github.com/projectdiscovery/nuclei/v3/pkg/protocols/network       0.848s

with fatal error: unexpected signal during runtime execution inside goleveldb:

github.com/syndtr/goleveldb/leveldb.(*DB).compactionError(...)
runtime.sigpanic()
panic during panic

This PR touches none of those packages:

pkg/operators/extractors/compile.go        pkg/operators/matchers/validate.go
pkg/operators/extractors/extract_test.go   pkg/operators/matchers/validate_test.go
pkg/operators/matchers/match_test.go       pkg/protocols/file/operators_test.go

And the same crash happens on a PR that isn't mine. #7598 shows the identical goleveldb stack on Tests (windows-latest):

github.com/syndtr/goleveldb/leveldb.(*DB).mpoolDrain(...)
goleveldb@v1.0.0/leveldb/db_state.go:101
created by ...leveldb.openDB in goroutine 16494

So it reproduces independently of this change. The lib/tests run also took 3960s (66 minutes) before dying, which looks like resource exhaustion on the Windows runner rather than a logic fault — goroutine 16494 in that trace points the same way.

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 #7601 from the same batch merged earlier today. Nothing needed from me here unless you'd like the PR rebased to pick up a fresh CI run.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@Mzack9999 thanks for the approval. Flagging that the one red check is not this PR, in case it is what is holding the merge.

Tests (windows-latest) failed, and the other three platforms plus Lint passed. The failure is in lib/tests, which this PR does not touch: the diff is confined to pkg/operators/matchers and pkg/operators/extractors.

What the Windows runner actually did:

FAIL github.com/projectdiscovery/nuclei/v3/lib/tests    3960.096s

fatal error: unexpected signal during runtime execution
[signal 0xc0000005 code=0x0 addr=0xe8 pc=0x7ff7e574a525]
runtime.(*unwinder).next(...)
  runtime/traceback.go:459
...
internal/poll.(*FD).execIO(0x3261376236327830, 0x7d20306134313964, 0x309c0315013eeead)
  internal/poll/fd_windows.go:311
panic during panic

Three things say runner rather than code:

  1. It is an access violation inside the Go runtime's own traceback, not a Go panic from nuclei code. panic during panic means the runtime crashed while printing the first crash.
  2. The execIO arguments are not pointers. 0x3261376236327830 and 0x7d20306134313964 decode as the ASCII text 0x26b7a2 and d914a0 } read little-endian. The stack that the runtime is walking has been corrupted with fragments of its own crash output, which is a runner/runtime-level failure mode.
  3. 3960 seconds for that one package (66 minutes) against TestSimpleNuclei, versus the whole macOS job finishing in 9 minutes.

It is also not unique to this PR. On the same day, #7583 hit fatal error: s.allocCount != s.nelems && freeIndex == s.nelems on Windows in the same lib/tests package, and also failed it on Ubuntu. Those are both Go runtime assertion failures, not test assertions.

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 dev if you would rather have a fresh run of the full matrix; just say which you prefer.

@DPS0340
DPS0340 force-pushed the fix/reject-empty-operators branch from 2b1c91a to 579eb02 Compare July 26, 2026 22:50
@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current dev (ad1447b) to get the Windows job a fresh run. Your approval is still on the PR and the diff is unchanged — git rebase replayed both commits with no conflicts, and your fix file tests commit is preserved with you as its author.

The reason: the red Tests (windows-latest) was a Go runtime crash, not a test failure:

FAIL github.com/projectdiscovery/nuclei/v3/lib/tests    3960.096s
fatal error: unexpected signal during runtime execution
[signal 0xc0000005 code=0x0 addr=0xe8 pc=0x7ff7e574a525]
runtime.(*unwinder).next(...)   runtime/traceback.go:459
panic during panic

panic during panic inside the runtime's own traceback, and the execIO arguments decode as ASCII fragments of the crash output rather than pointers — the stack being walked is corrupted. It also reproduced on #7598, which is not mine. I do not have rerun rights, so a rebase was the only way to trigger a new run.

Verified locally on the rebased branch before pushing:

go build ./...                    ok
go test ./pkg/operators/...       all 5 packages ok
go test ./pkg/protocols/file/...  ok
go vet ./pkg/operators/...        clean
gofmt                             clean

Nothing to review again — flagging it only so the force-push does not look like a silent change under an approval.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Windows is green after the rebase, so that half worked. The new red is Tests (ubuntu-latest), and it is also not this PR — different failure, same conclusion. Tracing it out because the root cause is a real bug in dev that will keep failing other people's PRs.

--- FAIL: TestWithVarsNuclei (3.89s)
panic: runtime error: invalid memory address or nil pointer dereference

  gologger@v1.1.71/gologger.go:56
  gologger@v1.1.71/gologger.go:158
  nuclei/v3/pkg/catalog/loader.New.func3()
      pkg/catalog/loader/loader.go:196
  sync/oncefunc.go:33
  lib/tests_test.TestWithVarsNuclei.func1()

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)

gologger.go:56 is l.formatter.Format(...). A zero-value &gologger.Logger{} has no formatter, so the first line that actually logs dereferences nil. It only fires when the Save() above it returns an error, which is why it is intermittent rather than constant — and saveMetadataIndexOnce is a sync.OnceFunc, so the panic escapes through once.Do and takes the test binary down rather than failing one test.

This PR touches six files, all under pkg/operators/ and one pkg/protocols/file test. It does not touch loader.go, types.go, or anything on that path.

Local state on the rebased branch:

go build ./...                    ok
go test ./pkg/operators/...       all 5 packages ok
go test ./pkg/protocols/file/...  ok
go vet / gofmt                    clean

So both red jobs this PR has seen were runtime-level failures in lib/tests unrelated to the diff: the Windows one was a corrupted-stack panic during panic that also hit #7598, and this one is the nil formatter above.

Happy to send the DefaultOptions() fix as a separate PR if it is useful — gologger.DefaultLogger or a New() in place of &gologger.Logger{} would do it — but I did not want to fold an unrelated change into an approved PR.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Tests (ubuntu-latest) here is the same dev bug, now filed as #7613 with the full trace: DefaultOptions() hands out &gologger.Logger{} (nil formatter), which panics at loader.go:196 the first time that line actually logs.

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 pkg/catalog/loader, #7605 is six under pkg/operators/*. Both pass macOS and Windows on the same commit.

Both PRs are otherwise green: Lint, Spell Check, Tests (macOS), Tests (windows), CodeRabbit.

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

The Tests (ubuntu-latest) failure on this PR is now fixed in #7614, with the root cause written up in #7613.

Short version: DefaultOptions() returns &gologger.Logger{} (nil formatter), the SDK raises the log level without setting one, and the first Warning that passes the level filter dereferences nil — inside a sync.OnceFunc, so it aborts the whole lib/tests package instead of failing one test.

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).

@DPS0340

DPS0340 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed by CI: #7614 is green on Tests (ubuntu-latest) — the exact job that is red here.

#7614   Tests (ubuntu-latest)  success
#7603   Tests (ubuntu-latest)  failure  (TestWithVarsNuclei, nil formatter)
#7605   Tests (ubuntu-latest)  failure  (same test, same stack)

Same runner image, same test, and the only difference is the one-line change in DefaultOptions(). That settles that the failure here is dev's, not this PR's.

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 dwisiswant0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just not it.

@dwisiswant0 dwisiswant0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Matchers/extractors with no values compile and run, silently never matching

3 participants