Feat: withNamedMethodParameter() for python detection rules and tests

[san-zrl]

Add withNamedMethodParameter to the detection rule DSL

Engine · Python · keyword-argument-aware parameter matching

Motivation

Python callers routinely pass arguments out of order using keyword syntax, and routinely omit optional parameters entirely:

SHA512.new(truncate="256")                                       # data omitted
SHA512.new(truncate="256", data=b"msg")                          # reordered
PBKDF2HMAC(iterations=480000, algorithm=SHA256(), salt=s, length=32)  # fully reordered

The existing withMethodParameter(type) method is purely positional: it reads argument n for parameter n and requires an exact argument count enforced by MethodMatcher. It cannot handle reordered arguments, omitted optional parameters, or the keyword-argument call style that is idiomatic in Python cryptography APIs. withNamedMethodParameter solves all three.

New DSL method

ParametersFactoryBuilder<T> withNamedMethodParameter(
    @Nonnull String  name,      // Python keyword-argument name
    @Nonnull String  type,      // accepted type; use ANY for no constraint
             boolean optional   // true → absent or type-mismatched argument is silently skipped
);

Ordering constraint

withMethodParameter declarations must precede all withNamedMethodParameter declarations in a rule. Violation throws IllegalStateException at rule construction time.

MethodMatcher mode selection

When all parameters are declared with withMethodParameter the existing exact-count type-list MethodMatcher is used — no change in behaviour. As soon as at least one withNamedMethodParameter is declared the no-type-list constructor is used instead, which accepts any call to the matched method and delegates all structural checking to the extraction loop described below.

Extraction algorithm

For each withNamedMethodParameter declaration, in declaration order:

1. Keyword-name lookup — scan the call-site argument list for a RegularArgument whose keywordArgument().name() equals name. If found, use that argument regardless of its position.
2. Positional fallback — if step 1 finds nothing, check the argument at the parameter's declaration index. Accept it only if that argument is itself positional (no keyword marker). If a preceding optional=true parameter was skipped, emit a WARN log before accepting (positional attribution may be ambiguous).
3. Type check — once an argument is resolved, verify its static type against the declared type. Skipped when type is ANY. When the type cannot be statically determined, resolveTreeType returns an accept-all predicate — the same conservative fallback used by MethodMatcher for positional parameters. On mismatch: optional=true → treat as absent and skip; optional=false → rule does not fire.
4. Not found — optional=false: rule does not fire. optional=true: skip silently.

Pre-extraction guards (all run before any finding is emitted)

The following checks apply only when at least one named parameter is declared. All run before MethodDetection is emitted, so a rejected call produces no finding at all.

• Unpacking guard — if any call-site argument is an UnpackingExpression (**d or *args), the call is rejected. The contents of unpacked iterables and dicts cannot be statically inspected, so no structural guarantee can be made.
• Unknown-keyword guard — if any call-site keyword argument has a name that is not declared in any withNamedMethodParameter in the rule, the call is rejected. The call does not match the function signature the rule was written for.

Example — SHA512.new(data=None, truncate=None)

new DetectionRuleBuilder<Tree>()
    .createDetectionRule()
    .forObjectTypes("Crypto.Hash.SHA512", "Cryptodome.Hash.SHA512")
    .forMethods("new")
    .shouldBeDetectedAs(new ValueActionFactory<>("SHA512"))
    .withNamedMethodParameter("data",     ANY,   /*optional*/ true)
    .withNamedMethodParameter("truncate", "str", /*optional*/ true)
        .shouldBeDetectedAs(new AlgorithmParameterFactory<>(AlgorithmParameter.Kind.ANY))
        .asChildOfParameterWithId(0)
    .buildForContext(new DigestContext())
    .inBundle(() -> "PyCrypto")
    .withoutDependingDetectionRules();

Call site	Matches?	truncate captured?	Reason
SHA512.new()	✓	—	Both absent, both optional
SHA512.new(data=b"msg")	✓	—	data by keyword; truncate absent → skipped
SHA512.new(truncate="256")	✓	✓	truncate by keyword; data absent → skipped
SHA512.new(data=b"msg", truncate="256")	✓	✓	Both by keyword, canonical order
SHA512.new(truncate="256", data=b"msg")	✓	✓	Both by keyword, reordered
SHA512.new(b"msg", "256")	✓	✓	Both by positional fallback
SHA512.new(b"msg", truncate="256")	✓	✓	data positional fallback; truncate by keyword
SHA512.new(truncate=256)	✓	—	int literal fails "str" type check; optional → skipped
SHA512.new(unknown_kwarg="x")	✗	—	Unknown keyword guard
SHA512.new(**d)	✗	—	Unpacking guard
SHA512.new(*args)	✗	—	Unpacking guard

Changed files

File	Change
engine/…/rule/Parameter.java	Added keywordName and keywordOptional fields, two new constructors, and getKeywordName() / isKeywordOptional() accessors.
engine/…/rule/DetectableParameter.java	New constructor threading both new fields through super().
engine/…/rule/IDetectionRule.java	Added withNamedMethodParameter(name, type, optional) to all six builder step interfaces.
engine/…/rule/builder/DetectionRuleBuilderImpl.java	Implemented the new method; enforces ordering constraint; updates build() to select the no-type-list MethodMatcher when named params are present. Introduced a copy() helper to eliminate repeated constructor calls.
engine/…/language/python/PythonDetectionEngine.java	Updated analyseExpression(): unpacking guard; unknown-keyword guard; MethodDetection emission moved after both guards; keyword-name lookup with positional fallback and Hole 1 WARN log; per-argument type check using PythonSemantic.resolveTreeType. Added findArgumentByKeyword() helper and extracted shared loop body into processParameterExpression(). Updated getTraceSymbol() to apply the same keyword-name lookup.
python/…/detection/pycrypto/hash/PycryptoCryptoHash.java	New example rule for Crypto.Hash.SHA512.new / Cryptodome.Hash.SHA512.new demonstrating the feature.
python/…/detection/pycrypto/hash/PycryptoCryptoHashSHA512Test.java + 15 Python fixture files	15 test cases covering all positive call-site variants, type-mismatch skipping, unknown-keyword rejection, unpacking rejection, wrong-algorithm and wrong-method negative cases.

Known limitation

There is no upper-bound arity check: extra positional arguments beyond the declared parameter count are silently ignored. This does not occur in valid Python (the interpreter would raise TypeError), so it is not a source of false positives in practice. The unpacking guard already handles the one realistic case where extra arguments could arrive opaquely (**d).

[n1ckl0sk0rtge]

Inline comments below. Summary and verdict in the follow-up review.

[n1ckl0sk0rtge]

Blocker — the main example of this PR does not work.

checkCurrentIndexState still starts with if (arguments.size() <= index) return false;, and here we pass the declared index. For a named parameter the argument was already resolved, possibly at a lower position, so this check throws the result away.

Walk through SHA512.new(truncate="256") — one argument in the call:

1. data (index 0): no data= keyword; the argument at position 0 is truncate="256", which has a keyword, so it is not taken as positional. data is optional, so it is skipped and positionalIndex becomes 1.
2. truncate (index 1): found by name at position 0. Good.
3. This line: checkCurrentIndexState(1, ...) -> arguments.size() (1) <= 1 -> returns false -> the parameter is dropped.

I verified it by printing the child stores and running each positive test alone:

call site	truncate captured?
SHA512.new(truncate="256")	no (0 children)
SHA512.new(data=b"msg", truncate="256")	yes
SHA512.new(truncate="256", data=b"msg")	yes
SHA512.new(b"msg", "256")	yes
SHA512.new(b"msg", truncate="256")	yes
Cryptodome variant	yes

This is exactly the case the PR is about: an optional parameter left out, a later one passed by name.

Suggested fix: for named parameters the argument is already resolved, so the arity part of the check is pointless. Either pass the real position of the resolved argument, or split checkCurrentIndexState so the arguments.size() <= index part is skipped for named parameters.

[san-zrl]

Fixed the main example.

PythonDetectionEngine.java:

• Line 579 (named-parameter path): replaced checkCurrentIndexState(positionalIndex, …) with checkSymbolTraceState(isInvocation, traceSymbol, expressionTree). The arity guard is no longer invoked because findArgumentByKeyword already guarantees the argument exists before this point is reached.
• Lines 677–693 (checkCurrentIndexState): the symbol-trace body was extracted into the new private method checkSymbolTraceState; checkCurrentIndexState now delegates to it after its arity guard, so the positional path is completely unchanged.
• The new checkSymbolTraceState() carries a Javadoc that makes the separation explicit and points back to findArgumentByKeyword as the reason the arity guard is not needed there.

[n1ckl0sk0rtge]

Blocker — optional=false does not stop the rule from firing.

The finding is created here, before the extraction loop. The required-parameter paths below only return afterwards, so the MethodDetection is already in the store.

I tested it: I changed the rule to .withNamedMethodParameter("truncate", "str", /* optional */ false) and ran testNoArgs (SHA512.new(), no truncate at all). The test still passes, which means the # Noncompliant finding is still reported.

So optional=false only removes the captured child value. It does not prevent detection, which is what the PR description promises.

Either fix the description, or move the required-parameter checks up into the guard block above, next to the unpacking and unknown-keyword guards.

[san-zrl]

PythonDetectionEngine.java

Inside the existing if (hasNamedParams) guard block — after the unknown-keyword check but before MethodDetection is emitted — a pre-flight loop now scans all declared parameters (line 487):

// Reject if any required named parameter is absent from the call site.
// This must run before MethodDetection is emitted so that a call that is missing a
// required argument produces no finding at all (not just a missing child detection).
int preCheckIndex = 0;
for (Parameter<Tree> p : detectionRule.parameters()) {
    if (p.getKeywordName().isPresent() && !p.isKeywordOptional()) {
        Optional<Argument> present =
                findArgumentByKeyword(p.getKeywordName().get(), preCheckIndex, arguments);
        if (present.isEmpty()) {
            return;   // required arg absent → no finding emitted
        }
    }
    preCheckIndex++;
}

The loop reuses findArgumentByKeyword (the same resolver used later in the extraction loop) so keyword lookup and positional-fallback semantics are identical. Because preCheckIndex advances for every parameter — not just required ones — the positional fallback slot stays correct relative to the rule's declared parameter order.

SHA512NewNegativeRequiredNamedParamAbsentTest.py

A plain SHA512.new() call with no # Noncompliant marker — verifyNoIssue asserts zero findings.

PycryptoCryptoHashSHA512RequiredParamTest.java

A self-contained test class that wires a rule variant where truncate is optional=false. testNegativeRequiredNamedParamAbsent calls verifyNoIssue and its asserts() override throws if it is ever reached, making the intent explicit.

[n1ckl0sk0rtge]

See the comment on the MethodDetection emission above. This return runs after the finding was already created, so a missing required parameter does not prevent the rule from firing — it only skips the child value.

[san-zrl]

Resolved. Execution order is

Step	Line	What happens
1	490–504	Pre-flight loop: every required named parameter is checked for presence. If any is absent → return before anything is written to the store.
2	507–510	MethodDetection emitted — only reached if step 1 passed.
3	524–533	Extraction loop: the else { return; } at line 532 is now a defensive dead branch — the pre-flight already ensures every required parameter exists.

The stale comment "rule does not fire" was the misleading part, because by that point the finding had already been emitted (step 2 runs before step 3). The updated comment now accurately reflects that this path is unreachable in normal operation.

[n1ckl0sk0rtge]

declaredKeywordNames only collects names from withNamedMethodParameter. Parameters declared with withMethodParameter have no name, so they are never in this set.

That means a mixed rule rejects legal Python. Example:

.withMethodParameter("str")                       // no name
.withNamedMethodParameter("length", "int", true)

f(algorithm="sha256", length=32)   # rejected: "algorithm" is an "unknown keyword"

Passing a positional parameter by keyword is completely normal Python, so every mixed rule has this hole. Worth deciding whether mixed rules are supported at all — if yes, positional declarations need a name too.

[san-zrl]

PythonDetectionEngine.java:

            // Reject if any keyword argument does not match any rule parameter — neither by
            // keyword name nor by positional index. The index check handles mixed rules where a
            // leading withMethodParameter slot is passed by name at the call site, which Python
            // allows.
            List<Parameter<Tree>> params = detectionRule.parameters();
            boolean hasUnknownKeyword =
                    IntStream.range(0, arguments.size())
                            .anyMatch(
                                    i -> {
                                        Argument a = arguments.get(i);
                                        if (!(a instanceof RegularArgument ra)
                                                || ra.keywordArgument() == null) {
                                            return false;
                                        }
                                        String name = ra.keywordArgument().name();
                                        return params.stream()
                                                .noneMatch(
                                                        p ->
                                                                p.getKeywordName()
                                                                                .map(name::equals)
                                                                                .orElse(false)
                                                                        || (p.getKeywordName()
                                                                                        .isEmpty()
                                                                                && p.getIndex()
                                                                                        == i));
                                    });
            if (hasUnknownKeyword) {
                return;
            }

A keyword argument at index i with name n is "unknown" if no rule parameter satisfies:
(a) p.getKeywordName() == n —> named param whose declared name matches
(b) p.getKeywordName().isEmpty() && p.getIndex() == i —> unnamed param at the same slot

Also added PycryptoCryptoHashSHA512MixedRuleTest.java with a hypothetical rule

                    .withMethodParameter(ANY)
                    .withNamedMethodParameter("truncate", "str", /* optional */ true)

and corresponding test files SHA512MixedRuleWithTruncateTest.py (detecting truncate correctly)

from Crypto.Hash import SHA512

# Rule: withMethodParameter(ANY) + withNamedMethodParameter("truncate", "str", optional=true)
# data slot is unnamed/required (index 0), truncate is optional named (index 1).

# --- truncate present → must fire and capture truncate ---

# data positional, truncate positional fallback
h = SHA512.new(b"msg", "256") # Noncompliant {{(MessageDigest) SHA-512}}

# data positional, truncate by keyword name
h = SHA512.new(b"msg", truncate="256") # Noncompliant {{(MessageDigest) SHA-512}}

# data by keyword (unnamed slot — Python allows naming positional params), truncate by keyword
h = SHA512.new(data=b"msg", truncate="256") # Noncompliant {{(MessageDigest) SHA-512}}

# truncate by keyword, data absent — must NOT fire (data is required); no Noncompliant marker

# both by keyword reordered: NOT included — the unnamed slot has no declared keyword name so the
# engine cannot match 'data=' at a non-natural position; that call site is unsupported for mixed rules.

and SHA512MixedRuleNoTruncateTest.java

from Crypto.Hash import SHA512

# Rule: withMethodParameter(ANY) + withNamedMethodParameter("truncate", "str", optional=true)
# data slot is unnamed/required (index 0), truncate is optional named (index 1).

# --- truncate absent → must fire, no truncate child ---

# data positional only
h = SHA512.new(b"msg") # Noncompliant {{(MessageDigest) SHA-512}}

# data by keyword (unnamed slot), truncate absent
h = SHA512.new(data=b"msg") # Noncompliant {{(MessageDigest) SHA-512}}

[n1ckl0sk0rtge]

I would not put two loose fields on the base class. Here is what we could do instead.

	plain	named
not detectable	Parameter	NamedParameter
detectable	DetectableParameter	NamedDetectableParameter

And this PR already creates both named variants: a plain named Parameter for the depending-rules path, and a named DetectableParameter for the value path.

There is a second problem: Parameter.is() compares the type tag with equals, not instanceof. A subclass of DetectableParameter would either fail is(DetectableParameter.class), or have to pass the parent tag and lie about its real class. The engine would then need instanceof in some places and is() in others.

**Nonnull concern: ** This state is representable and means nothing:

new Parameter<>(type, 0, false, rules, null, true);   // no name, but "optional" = true

Also the new public constructor below copies all five field assignments instead of delegating to the protected one.

Suggestion — one nullable field holding a small record:

public record NamedArgument(@Nonnull String name, boolean optional) {}

@Nullable private final NamedArgument namedArgument;

@Nonnull
public Optional<NamedArgument> getNamedArgument() {
    return Optional.ofNullable(namedArgument);
}

• the name is non-null by construction, enforced in one place
• null means "not a named parameter" — the meaningless combination disappears
• one new constructor instead of two, and the existing ones can delegate
• the engine reads one field instead of two

I would also rename keyword* to argument*. Named arguments exist in several languages, and Parameter is the shared language-neutral class — keyword is Python wording.

[n1ckl0sk0rtge]

This method sits on IDetectionRule<T>, so a Java or Go rule can call it too. But the feature is Python-only:

• JavaDetectionEngine and the Go engine never read getKeywordName() — I grepped, there are no callers there.
• DetectionRuleBuilderImpl.build() still drops the MethodMatcher type list for any rule with a named parameter, whatever the language.

Combined, a Java rule using this would match any call with that method name and any argument count, and then read arguments by position. That is a false-positive generator.

Suggestion: reject named parameters for non-Python rules, or at minimum add a clear warning in the javadoc here.

[n1ckl0sk0rtge]

This if is why the suite is green while the feature is broken.

If nothing was captured, truncateStore is null, the block is skipped, and the test passes. See my comment on PythonDetectionEngine — SHA512.new(truncate="256") captures nothing today, and this assert does not notice.

The # Noncompliant comments do not help either, because every fixture uses the same expected message:

h = SHA512.new(truncate="256")   # Noncompliant {{(MessageDigest) SHA-512}}
h = SHA512.new(data=b"msg")      # Noncompliant {{(MessageDigest) SHA-512}}

The message says nothing about truncate.

As it stands, 6 of the 10 positive tests only prove "SHA-512 was found", which the old positional code could already do.

Suggestion: let each test declare whether it expects the child, then assert it:

assertThat(truncateStore).isNotNull();                     // capture expected
assertThat(truncateValue.asString()).isEqualTo("256");

assertThat(truncateStore).isNull();                        // no capture expected

[san-zrl]

PycryptoCryptoHashSHA512Test.java

Added a per-instance flag, defaulting to false, set explicitly by each test method before calling verify(). Because JUnit creates a fresh instance per test, there is no shared-state risk between tests.

Each test method sets the flag

• the 6 fixtures that pass truncate="256", set expectTruncateChild = true
• the 4 that don't set it false.

asserts() now has two hard branches instead of a soft if (store != null)

case	assertion
expectTruncateChild = true	assertThat(truncateStore).isNotNull() + full value check
expectTruncateChild = false	assertThat(truncateStore).isNull()

[n1ckl0sk0rtge]

This is the test for the PR's headline example, and today it passes without capturing anything — see the comment on the conditional assert below.

Once that assert is made unconditional, this test will fail until the arity bug in PythonDetectionEngine is fixed. That is the right order: make the test fail first, then fix the engine.

[san-zrl]

Engine is fixed. Test is green. It also asserts the detection of truncate.

[n1ckl0sk0rtge]

Blockers

1. SHA512.new(truncate="256") — the headline example — captures nothing. The arity check in checkCurrentIndexState still uses the declared index, so a keyword argument at a lower position is dropped.
2. The tests cannot catch that. asserts() wraps the check in if (truncateStore != null), and all fixtures expect the same # Noncompliant message, so 6 of the 10 positive tests assert nothing about the new feature.
3. optional=false still reports a finding — MethodDetection is emitted before the required-parameter checks. Verified by making truncate required and running SHA512.new().
4. Mixed positional + named rules lose the positional type and arity checks, and the unknown-keyword guard then rejects legal Python.
5. withNamedMethodParameter followed by addDependingDetectionRules silently loses the name.

Worth deciding

The DSL sits on IDetectionRule<T> but only Python implements it. PycryptoCryptoHash is not registered, and the captured value never becomes SHA-512/256. docs/DETECTION_RULE_STRUCTURE.md is not updated.

The same keyword handling is also missing in extractArgumentFromMethodCaller and PythonSemantic.resolveValues — that one probably matters more in real code, and needs no DSL change.

[san-zrl]

This PR is obsolete. A new implementation of the feature is in PR #527.