Fix conditions dropping all but the first pattern's findings

[Giulia Stocco (gfs)]

Problem

WithinOperation.WithinOperationDelegate had its return statement inside the foreach loop over its input captures:

foreach (var capture in captures ?? Array.Empty<ClauseCapture>())
{
    if (capture is TypedClauseCapture<List<(int, Boundary)>> tcc) { /* filter */ }

    var passedOrFailed = wc.Invert ? failed : passed;
    return new OperationResult(...);   // <-- returns after the first capture
}

Every pattern clause in a rule contributes its own ClauseCapture. Because the delegate returned after the first one, any rule with more than one pattern and at least one condition reported only the findings of its first pattern.

Measured on main (3b89116), a rule with patterns alpha/beta/gamma and a same-line condition on gate:

Content	Reported before	Expected
alpha gate\nbeta gate\ngamma gate	[alpha]	[alpha, beta, gamma]
beta gate\nalpha gate (order swapped)	[beta]	[alpha, beta]

How it got here

Worth spelling out, because it explains why the fix is two changes rather than one.

#423 (c055aa3) established the original design: the return was outside the capture loop, and the operation pruned each pattern capture in place via tcc.Result.RemoveAll(toRemove). Iterating every capture was essential — that was the mechanism by which each pattern clause's match list got cleaned up.

At that point within-captures were TypedClauseCapture<List<Boundary>> while pattern captures were TypedClauseCapture<List<(int, Boundary)>>, so the type test in the loop structurally excluded other conditions' captures.

#495 (dce5493, "Refactor Conditions") changed two things at once: it replaced in-place pruning with accumulating passed/failed and returning a new capture, and it changed the within-capture type to TypedClauseCapture<List<(int, Boundary)>> — making it indistinguishable from a pattern capture. In the process the return ended up inside the loop.

Those two defects concealed each other. Because the delegate bailed after the first capture, it never reached another condition's capture, so the lost type-based exclusion never manifested.

Fix

Hoist the return back out of the loop so all captures accumulate first, and skip captures produced by other WithinClauses — restoring the exclusion the type change silently removed.

That second half matters. Conditions each filter the raw pattern matches independently, and RuleProcessor.FilterCaptures intersects the survivors to AND the conditions together. Letting a condition consume another condition's already-filtered capture double counts matches and breaks that intersection. The existing WithinClauseWithMultipleConditions test catches exactly this, and did catch it while I was writing the fix.

Impact

Seven shipped default rules have multiple patterns plus a condition and are under-reporting today:

AI016300, AI036000, AI036622, AI038210, AI038500, AI080001, AI084000

Scans using them will now report additional findings. This is a behavior change worth a release note. All default rules still verify (verifyrules -r AppInspector/rules/default/).

Also: surface why a rule failed verification

RuleStatus carries Errors, OatIssues and SchemaValidationErrors, but VerifyRulesTextWriter printed only:

Ruleid: AI000000, Rulename: Some Rule, Status: False

OAT's own EnumerateRuleIssues already detects real problems — duplicate clause labels, malformed clause grammar — and RulesVerifier already collects them, but the only place they were ever surfaced was TestDefaultRules. A user running verifyrules on a broken custom rule got a bare False with no explanation. Failing rules now print their errors, OAT issues, and schema errors indented beneath the status line.

OatIssues is also now materialized — it was a generator that re-ran the entire check on each enumeration, and RuleStatus.Verified plus the writer read it more than once.

Tests

Three new tests, all of which fail on main and pass here:

• MultiplePatternsWithConditionReportAllMatchingPatterns — three patterns with a same-line condition, covering all-gated, partially-gated, and ungated content
• MultiplePatternsWithConditionAreOrderIndependent — swapping pattern order must not change the result
• TextWriterReportsWhyARuleFailedVerification — verify output contains the failure reason, not just Status: False

Full suite: 350 passing, 0 failing (347 before, plus these 3).

[Giulia Stocco (gfs)]

SDL Security Review

🤖 Disclosure: this is an automated COMPASS/SDL security review, produced by GitHub Copilot running the sdl-security-review skill against the Microsoft SDL rule set. It is machine-generated, was not authored by a human reviewer, and is not a substitute for maintainer review. Please validate the findings and the verdict before relying on them.

Scope: changed lines only, application source. .github/ configuration, instruction, and skill files are excluded per SDL review scope.

Reviewed: AbstractRuleSet.cs, OatExtensions/WithinOperation.cs, OatExtensions/WithinClause.cs, RuleProcessor.cs, RulesVerifier.cs, SearchCondition.cs, SearchPattern.cs, AppInspector.CLI/Writers/VerifyRulesTextWriter.cs, rule-schema-v1.json, and the three test files.

---

🔴 Blocking

None.

🟡 Warning

None.

---

🟢 Informational

1. Algorithmic work amplification on attacker-supplied source files — AppInspector.RulesEngine/OatExtensions/WithinOperation.cs

Hoisting the return out of the capture loop is the correct fix, but it does increase the worst-case work per condition:

foreach ((var clauseNum, var boundary) in governed)
{
    var boundaryToCheck = GetBoundaryToCheck();
    if (boundaryToCheck is not null)
    {
        var operationResult = ProcessLambda(boundaryToCheck);   // sub-clause regex over the region

ProcessLambda now runs once per governed match across all pattern captures, where previously it ran only over the first capture's matches. For same-file, only-before, and only-after the region is the whole file, so the cost is O(matches × file size) per condition.

This is not a new complexity class — a single-pattern rule with a same-file condition was already quadratic in file size before this change — and the fix multiplies it only by the pattern count (a small constant). Application Inspector's primary use case is scanning untrusted third-party source, and the rules engine has no wall-clock budget per file (EnableNonBacktrackingRegex is opt-in and only addresses catastrophic backtracking, not this). Flagging for awareness rather than as a defect in this PR; a per-file time budget would be the durable mitigation.

2. Rule verification diagnostics now written to CLI output — AppInspector.CLI/Writers/VerifyRulesTextWriter.cs

foreach (var error in ruleStatus.Errors)
{
    TextWriter.WriteLine("    Error: {0}", error);
}

RuleStatus.Errors includes e.Message from failed JsonPath/XPath/YamlPath compilation, and the output may include rule file paths.

Assessed as not a finding: this is a local developer CLI, the diagnostics describe rules the operator themselves supplied, the output goes to that operator's console or chosen output file, and every one of these strings was already emitted through ILogger.LogError before this change. There is no client/server trust boundary being crossed and no stack traces are surfaced — only Message. Making failures explainable is a net improvement.

3. search_in schema tightening — rule-schema-v1.json

-"pattern": "^(file|finding-region\\(-?\\d+,\\d+\\)|finding-only|same-line|same-file)$",
+"pattern": "^(finding-region\\(-?\\d+,\\d+\\)|finding-only|same-line|same-file|only-before|only-after)$",

Not a security issue — noted only because it is an availability/compatibility change. file was never handled by GenerateCondition, so this aligns the schema with the implementation, but custom rules that currently specify search_in: "file" move from "silently ignored with a warning" to "fails schema validation." Worth a release note alongside the seven under-reporting rules already called out in the description. The added schema regex itself has no nested quantifiers and is linear-time.

---

Verified clean

SDL area	Result
Expression injection	AbstractRuleSet builds an OAT boolean expression by string concatenation, which is the shape that usually earns a blocking finding. Every token is derived from integer counters (patternLabel, ConditionLabel → "c" + int) and never from rule-file text, so no rule content reaches the expression grammar. ✅
Injection (SQL / OS / XSS / format string / eval)	None introduced. ✅
Unsafe deserialization	New SearchCondition.AppliesTo/DoesNotApplyTo and SearchPattern.Conditions deserialize via System.Text.Json into concrete typed models. No polymorphic type resolution, no TypeNameHandling equivalent. ✅
Path traversal / SSRF	No file-path or URL handling added in product code. ✅
Secrets	No hardcoded credentials, keys, or tokens in the diff. The test_token / javascript_only strings in PatternConditionsTests.cs are pattern literals, not credentials. ✅
Cryptography	No cryptographic primitives, RNG, hashing-for-security, or TLS configuration touched. ✅
Culture-sensitive comparison in allow/deny logic	ConditionAppliesToLanguage uses StringComparer.OrdinalIgnoreCase for both the allow list and the deny list. This is the right call — culture-sensitive comparison in list matching is a known bypass vector (e.g. Turkish dotless-I). Enumerable.Contains with that comparer is also null-safe, so a null or absent Language cannot throw. ✅
Fail-safe direction of the new language filter	When a condition does not apply to the file's language, the new pass-through gate returns everything the condition governs and filters nothing. The failure direction is therefore more findings, never fewer — including for inverted (negate_finding) conditions used to suppress false positives. For a detection tool, failing toward over-reporting is the correct posture. ✅
Concurrency	The old ConcurrentDictionary in FilterCaptures is replaced with plain HashSet/Dictionary. Safe: all four collections (ruleGates, patternGates, allMatches, seen) are locals of a single FilterCaptures invocation, never shared. The one piece of shared state read, ruleCapture.Rule.Clauses, is enumerated read-only and rules are constructed before analysis begins. GovernedMatches is static and allocates its own state. ✅
Gate identity	Boundary is a reference type with no Equals/GetHashCode override, so the new HashSet<(int, Boundary)> gate lookups match on reference identity. The original Boundary instances are threaded through from the pattern captures into passed/failed and into the gates unmodified, so gate.Contains(match) resolves correctly — and this matches the pre-existing ConcurrentDictionary<(int, Boundary), int> semantics. Not a security issue; noted because the correctness of the new gate intersection depends on it. ✅
Error handling	No exceptions swallowed in security-critical paths. ✅
Resource handling	The new TestVerifyRulesCmd temp file uses a Guid.NewGuid()-randomized name (not predictable in a world-writable temp dir) and is deleted in a finally. ✅

CodeQL (csharp, javascript-typescript) and the SDL Agentless Tag check are green on this head.

---

APPROVE — no blocking or warning-severity SDL issues in the changed lines.