fix rule by filter bugs

[Elutrixx]

Fixes 2 of the reported bugs

1. behavior change in how we understand the infield field. Before: Only apply filter to objects of said field Now: MinPrefix filter should apply to both fields and disregard inField
2. Port now shows portEnd correctly

[Elutrixx]

F3 — no code change requested; please just confirm the acceptance below is what you intended, since I inferred it from the documentation you wrote rather than from a reply.

Yup its intentional

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[tpurschke]

AI Review — PR #5303 "fix rule by filter bugs" — round 3 (commits since round 2)

Reviewer: Claude Opus 5.5 (1M context), model id claude-opus-5-5[1m], reasoning effort low, review depth standard (6 files in the PR, +488/−60; no DDL, no auth surface, no installer surface).
Capability used: I ran both required passes (correctness/quality and security) myself against the fwo-review-pr checklists, on the primary model. I used no separate review command or review subagent.
Delegated to a reduced model tier: nothing. The C# delta this round is test code only, so every check ran on the primary model.
Usage budget: measured from the session transcript (method 2, session log on disk). I summed input_tokens + output_tokens + cache_creation_input_tokens over the session's requests: ≈130k review tokens, against the 400k standard ceiling, as of drafting this comment. Posting adds a little more. Cached input that was reused (≈2.1M cache_read_input_tokens) is excluded. No sub-agents were dispatched.

Scope of this round: two commits since round 2 (head 584eea7):

• 6efb4b6 fix remaining findings, which changes test code only.
• a1e3876 Merge branch 'develop' into rulebyfilter-fixes. Compared with 6efb4b6, this merge changes none of the PR's C# files (0-line diff on all five). It does change fworch-texts.sql, as described under F9.

Verified locally at head a1e3876 in a clean worktree, not the working tree:

• dotnet build --configuration Debug roles/FWO.sln: 0 warnings, 0 errors.
• dotnet test … --filter "FullyQualifiedName~RuleController|FullyQualifiedName~NetworkObjectRangeAnalyzer": 40/40 passed.

---

Findings

#	Criticality	Confidence	Status	Subject
F1	🟠 medium	🟢 high	✅ fixed	Minimum-prefix rule enforced in two places with different semantics
F2	🟠 medium	🟢 high	✅ fixed	Prefix violation evaluated for every rule even when the IP check already failed
F8	🟠 medium	🟢 high	✅ fixed	Both prefix-violation exits of the FilterFields.Both branch are untested
F3	🟠 medium	🟡 medium	🤝 accepted	InField-scoped prefix filtering no longer expressible (rules with "Any" on the other side are dropped)
F4	🟢 low	🟢 high	🔴 still open (regressed)	service[].portEnd not documented in H6935 or the help page. The round-2 fix was reverted by the develop merge
F5	🟢 low	🟢 high	🔴 still open (regressed)	port uses the -1 sentinel while portEnd is omitted. The documentation that resolved this was reverted by the develop merge
F9	🟢 low	🟢 high	🔴 new	Develop merge a1e3876 restored the old H6934/H6935 help texts, which now contradict the new prefix semantics
F6	🟢 low	🟢 high	✅ fixed	Breaking REST contract change missing from whats_new_facts
F7	🟢 low	🟢 high	✅ fixed	Inline array argument new[] { "Source", "AnySource" } in the new test

No critical or high findings, and no security findings. The code is in good shape. What remains open is the help text: F4, F5 and F9 all come from one merge-conflict resolution and share one fix.

---

Recommendations

Should fix before merge

• F9 (together with F4 and F5): restore the H6934 and H6935 rows, German and English, from 6efb4b6 (or 584eea7). For example: git show 6efb4b6:roles/database/files/sql/idempotent/fworch-texts.sql, then take those four INSERT lines. Develop has not touched these keys since, so keeping the PR's version is a straight replacement with nothing to merge. Otherwise the in-app API help describes the behavior before this PR, and F3's acceptance rests on that documentation.

Nice to have

• Nothing further.

Details for the open findings are in this comment rather than as inline comments. The repo's review authorization covers a single top-level comment only.

---

Details

F9 — 🟢 low · 🟢 high · 🆕 new — develop merge reverted the endpoint help text

In a1e3876, the conflict in fworch-texts.sql was resolved in favour of develop for the H6934/H6935 rows. Only the two whats_new_facts <li> lines from this PR remain. At head, H6935 (English) reads:

Filter.MinPrefixLength: Minimum prefix length of the matching network object … Filter.InField: Defines whether source, destination, or both are evaluated. A rule is returned as soon as at least one resolved object in the checked field matches; additional non-matching or unsupported objects do not block the match.

The code does the opposite. NetworkObjectRangeAnalyzer.EvaluateField returns PrefixViolation as soon as any supported object is broader than MinPrefixLength. MatchesRuleIpFilter also applies that check to the field the caller did not ask about. H6934 makes the same outdated statement, in both languages.

Failure scenario: an integrator reads Help → API → GetRulesByFilter and sends IpAddress 10.1.2.3, MinPrefixLength 24, InField source. They expect a rule with source {10.1.2.3/32, 10.0.0.0/8} or destination Any to be returned. It is silently omitted. The whats_new_facts entry states the new behavior, so the release notes and the reference page now contradict each other.

Rated low because it is documentation, which is on the closed list for low. It is still listed under should fix before merge because it is a regression from a merge, not a gap that was there from the start, and the fix is a four-line restore.

F4 — 🟢 low · 🟢 high · 🔴 still open (regressed)

The sentence added in 584eea7, "service[].portEnd contains the inclusive end only for a genuine port range; the field is omitted for single or missing ports", is missing from H6935 at head. That text was the only documentation of the new response field. The help page has no response example to fall back on. Rating unchanged from rounds 1 and 2.

F5 — 🟢 low · 🟢 high · 🔴 still open (regressed)

In round 2, F5 was resolved by documenting both conventions side by side: port is -1 when missing, and portEnd is omitted when missing. The code half of that fix is still in place: NormalizePortEnd and its [TestCase]s in RuleControllerWiringTest. The documentation half went with the same H6935 row, so the asymmetry is undocumented again. Rating unchanged.

F8 — 🟠 medium · 🟢 high · ✅ fixed — verified

6efb4b6 adds GetRulesByFilter_ShouldRejectSourcePrefixViolationWhenInFieldIsBoth and …ShouldRejectDestinationPrefixViolationWhenInFieldIsBoth, using the includeBroadSourceRule / includeBroadDestinationRule fixtures. I checked both mutations named in round 2:

• Deleting the sourceEvaluation == PrefixViolation early return lets DestinationWithBroadSource through, because the destination is Match. The count becomes 2 and the first test fails.
• Flipping destinationEvaluation != PrefixViolation admits SourceWithBroadDestination, and the second test fails.

Both exits are now covered.

F7 — 🟢 low · 🟢 high · ✅ fixed — verified

The inline new[] { "Source", "AnySource" } is now private static readonly string[] kExpectedZeroMinPrefixRuleNames at RuleControllerBranchingTest.cs:22. The same commit removes kExpectedSourceRuleNames, which no longer had any users. The SonarQube quality gate now passes.

F3 — 🟠 medium · 🟡 medium · 🤝 accepted

Confirmed by the author on 2026-09-22 ("Yup its intentional"). Please note that this acceptance depends on the new semantics being documented, which F9 currently undoes.

F1, F2, F6 — ✅ fixed — re-verified

The merge leaves NetworkObjectRangeAnalyzer.cs and RuleController.cs byte-identical to round 2, so the fixes verified there still hold. Both whats_new_facts <li> lines (DE and EN) are still present at head.

---

Security pass

I ran this separately from the correctness pass. Nothing found.

• Production C# is unchanged since round 2, so the round-2 analysis still applies:
  • The rewritten filter can only narrow a result set.
  • [Authorize(Roles = Admin, Auditor)] is unchanged.
  • InField is validated before the switch, and the default throw is caught and returned as a generic 500.
  • There are no new query variables, secrets, logging, deserialization, or fetch paths.
• The only new non-test change is static txt text. It contains no user input and no DDL, and lives under sql/idempotent/, so existing installations are not at risk on upgrade.

Checks run, with nothing to report

• CODING_GUIDELINES.md:
  • No new inline array arguments. The [CreateRule…()] collection literals passed to InvokeConvertRuleList fall under the object-graph exception granted in round 2.
  • RuleControllerBranchingTest.cs is still below 1000 lines.
  • No new method is over 100 lines, and none has more than 7 parameters.
  • No commented-out code.
• Localization / whats_new_facts: DE and EN are in sync for the lines this PR still carries. The keys that are out of sync with the code are covered by F9.
• Help page: HelpApiRulesByFilter.cshtml renders H6934–H6936 via Html.Raw. Restoring the text rows is enough; the page needs no change.

Residual risk

These are unchanged from round 2 and not numbered:

• CommonPrefixLength treats an object with a null IpEnd as /32, so such an object passes any MinPrefixLength.
• There is still no integration test against real imported data, so the practical impact of F3 on existing API consumers is not quantified.

_{Reviewed by Claude Opus 5.5 (1M context), reasoning effort low · depth standard · no reduced-tier delegation · ≈130k review tokens against a 400k ceiling, measured from the session transcript}