You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Deploying librefang-docs with    Cloudflare Pages

View logs

Code Review: PR #3193

Overview

Closes #3050 by layering three granular controls onto the existing MCP taint scanner: a per-tool kill switch (default = "skip"), reusable named [[taint_rules]] sets with severity actions (block / warn / log), and a dashboard tree editor backed by API surfacing of taint_scanning + taint_policy. The taint scanner is refactored to thread a rule-set registry through walk_taint, and a new detect_outbound_text_violation_with_skip returns the specific TaintRuleId that fired so the caller can apply rule-set actions.

🔴 Correctness: multi-rule downgrade can silently mask unrelated violations

detect_outbound_text_violation_with_skip (taint.rs +1923) short-circuits on the first rule that matches:

if let Some(rule) = detect_secret_rule_with_skip(payload, skip_rules) {
    // build labels = { Secret }, check sink, return Some(rule)
}
if sink.blocked_labels.contains(&TaintLabel::Pii) {
    if let Some(rule) = detect_pii_rule_with_skip(...) { ... }
}

But the OLD check_outbound_text_violation_with_skip builds a combined label set and runs the sink check once. The two functions differ in observable behaviour for payloads that trip both Secret AND PII rules.

Concrete failure mode: a payload like "contact [email protected] with sk-realtoken1234567890abcdef" triggers WellKnownPrefix AND PiiEmail. With a rule set

[[taint_rules]]
name = "browser_handles"
action = "warn"
rules = ["well_known_prefix"]

referenced by the tool, walk_taint calls detect_outbound_text_violation_with_skip → gets Some(WellKnownPrefix) → apply_rule_set_action finds well_known_prefix in the set with action warn → returns false → the PII violation is silently allowed through, even though the operator never authorized PII downgrade.

Suggested fix: either return all matching rules (Vec<TaintRuleId>) and require every fired rule to be downgrade-authorized before allowing the call, or run secret + PII detection independently in walk_taint and && their downgrade decisions. The current design lets one allowlisted rule mask any number of unauthorized ones.

No test in the new suite covers this case — adding one would be a good guard.

🟡 Snapshot semantics for taint_rule_sets are operationally surprising

McpServerConfig.taint_rule_sets is set from self.config.load().taint_rules.clone() at five distinct kernel sites (mod.rs +843, +11653, +11803, +11928, +12075). The rustdoc says "snapshot … at boot" but doesn't call out the consequence: operators editing [[taint_rules]] and reloading config will not see the new rules apply to already-connected MCP servers until those servers themselves are reloaded.

Given that everywhere else in CLAUDE.md/docs the kernel claims hot-reload semantics, this divergence is worth either (a) fixing — pull taint_rules from self.config.load() at scan time instead of snapshotting — or (b) documenting prominently on McpServerConfig.taint_rule_sets and in the operator-facing docs.

If the snapshot is intentional (consistency-during-call argument), at least add a note.

🟡 Five-fold duplication of the snapshot expression

taint_rule_sets: self.config.load().taint_rules.clone(), appears verbatim five times in kernel/mod.rs. The five other surrounding fields (oauth_config, taint_scanning, taint_policy, roots) are also boilerplate-y, but the new line repeats the cross-field lookup. Suggest extracting a LibreFangKernel::build_mcp_runtime_config(&server_config) -> McpServerConfig helper. Not blocking, but five copies is the threshold where future drift gets expensive.

🟡 Dashboard mutation couples GET and PUT shapes

useUpdateMcpTaintPolicy (mutations/mcp.ts +85) sends { ...existing, taint_scanning, taint_policy } to PUT /api/mcp/servers/{name} because the PUT requires a full McpServerConfigEntry. The mutation's own comment flags this. Implication: any future required field added to McpServerConfigEntry that isn't surfaced in list_mcp_servers / get_mcp_server (or that gets renamed in the GET shape) will silently break the taint editor with a 400.

A safer alternative would be a dedicated PATCH /api/mcp/servers/{name}/taint endpoint that accepts just { taint_scanning?, taint_policy? }. Server already has the entry; client doesn't need to round-trip everything else. Worth considering as a follow-up.

🟢 Strengths

• Backward compat is well-protected — default = Scan and empty rule_sets match the old shape; legacy_taint_policy_without_new_fields_still_loads covers exactly this.
• Tests for the priority logic (test_rule_set_warn_takes_precedence_over_block) and key-name path (test_rule_set_warn_downgrades_sensitive_key_name) are good.
• Wildcard rustdoc + matcher tests (test_documented_wildcards_match_expected_paths) — the doc and the implementation are pinned together.
• Violation string still doesn't echo payload — the security-critical invariant from the old function is preserved in the new one (taint.rs +263–268).
• schemars::JsonSchema and #[serde(skip_serializing_if = "Vec::is_empty")] derives match project convention for new optional collections.

🟢 Minor nits

• lookup_rule_set_action does O(refs × registry) linear scans per rule fire. Fine for current scale (handful of sets) but if [[taint_rules]] ever grows large, a HashMap<&str, &NamedTaintRuleSet> cached on McpServerConfig would be cheap.
• action_priority returns u8 — readable, but a derive(PartialOrd) on a properly ordered enum would be even clearer (the current ordering Block=0, Warn=1, Log=2 works because cmp on u8 matches the intent).
• The PR description has the "Live integration smoke" item unchecked. CLAUDE.md mandates live tests for new endpoint behaviour — recommend running through PUT /api/mcp/servers/{name} with a default = "skip" payload and verifying the round-trip before merge.

Verdict

Architecturally sound and well-tested for the single-rule case. The multi-rule masking issue (🔴) is the one blocker — once that's resolved (or proven to be a non-issue with a test), this is good to merge. The snapshot-semantics concern (🟡) should at minimum get an explicit doc note.

Pushed ef54effb addressing the review:

🔴 Multi-rule masking fixed

• Added librefang_types::taint::detect_outbound_text_violation_rules_with_skip returning every fired rule (Vec<TaintRuleId>).
• walk_taint now iterates every fired rule and blocks on the first one not authorized by a rule_set, so a Secret-family downgrade can't mask an unrelated PII fire on the same payload.
• The original first-only detect_outbound_text_violation_with_skip is kept as a thin wrapper (with a docstring directing rule-set callers to the plural variant) so existing callers don't break.
• New regression test test_rule_set_downgrade_does_not_mask_unrelated_rule exercises [email protected] (KeyValueSecret + PiiEmail co-fire) under a rule_set that warns Secret rules only — must still block.

🟡 Kernel helper extracted

• New LibreFangKernel::snapshot_taint_rules() helper; replaced the five inline self.config.load().taint_rules.clone() call sites.

🟡 Snapshot semantics documented

• Added a "Hot-reload caveat" block on KernelConfig.taint_rules rustdoc: edits don't propagate to already-connected MCP servers until those servers themselves are reloaded.

Pre-existing test fix

• config::types::tests::tool_policy_rule_sets_reference_round_trips was using [mcp_servers.tools.*] toml shape, but McpTaintPolicy has no mcp_servers field. The test was being silently masked by the -- taint name filter (it doesn't contain "taint"). Fixed the toml prefix to [tools.*].

Verification

• cargo test -p librefang-runtime-mcp -p librefang-types -p librefang-kernel --test-threads=1 — all 559+ kernel tests + 87+ runtime-mcp + 30+ types pass.
• cargo clippy -p librefang-runtime-mcp -p librefang-types -p librefang-kernel --all-targets -- -D warnings — clean.
• Multi-threaded cargo test showed an unrelated SIGABRT in kernel tests (flaky; passes single-threaded) — not introduced by this commit.

The earlier dashboard-coupling concern about useUpdateMcpTaintPolicy round-tripping the full server config is left as a possible follow-up — could be addressed by a dedicated PATCH /api/mcp/servers/{name}/taint endpoint, but that's a separate API surface change.

Pushed e4b40588 finishing the remaining 🟡 items from the review:

Hot-reload [[taint_rules]] — was previously only documented as a snapshot, now actually live:

• McpServerConfig.taint_rule_sets changed from Vec<NamedTaintRuleSet> to Arc<ArcSwap<Vec<NamedTaintRuleSet>>> shared across every connected MCP server.
• LibreFangKernel.taint_rules_swap owns the master swap, initialised from config.taint_rules at boot and updated via .store(Arc::new(new_rules)) inside reload_config (executed before self.config.store(...) so no scanner observes a window where the two disagree).
• The scanner takes a single .load() per scan call, so a mid-walk reload can't mutate rules under an in-flight tool invocation.
• New helpers empty_taint_rule_sets_handle() / static_taint_rule_sets_handle() for tests + non-kernel callers.
• The hot-reload caveat in the rustdoc on KernelConfig.taint_rules is gone — operators no longer need to reload servers to pick up rule-set edits.

Dedicated PATCH /api/mcp/servers/{name}/taint — closes the GET/PUT coupling concern:

• New endpoint accepts { taint_scanning?, taint_policy? }, no other server fields required.
• useUpdateMcpTaintPolicy rewritten to call PATCH; TaintPolicyEditor passes only { id, taint_scanning, taint_policy } instead of spreading the entire server config.
• The "future required field added → dashboard silently drops it" risk is gone.

Pre-existing kernel-test SIGABRT verified pre-existing

• Reproduced multi-threaded cargo test -p librefang-kernel --lib on 02eb92af (original PR head before any of my fixes) — same SIGABRT under default thread parallelism, passes single-threaded. Not introduced by this change.

Going to run a live integration smoke (PATCH round-trip → config.toml persistence → reload visible) before declaring complete.

Live integration smoke done — pushed 0abf3848 after smoke surfaced one more gap.

Smoke setup

Isolated LIBREFANG_HOME=/tmp/librefang-smoke-3193, fresh release binary, single MCP server stub in config.

PATCH endpoint round-trip ✅

PATCH /api/mcp/servers/stub-test/taint  {"taint_scanning": false}
  → {"reload":"applied","status":"updated","name":"stub-test"}
GET /api/mcp/servers/stub-test
  → taint_scanning: false   ✓ live
config.toml on disk
  → taint_scanning = false  ✓ persisted

PATCH /api/mcp/servers/stub-test/taint  with full taint_policy
  → {"reload":"applied","status":"updated"}
GET /api/mcp/servers/stub-test
  → taint_policy.tools.navigate.default = "skip"
                       .rule_sets        = ["browser_handles"]   ✓ live
config.toml
  → [mcp_servers.taint_policy.tools.navigate]
    default = "skip"
    rule_sets = ["browser_handles"]                              ✓ persisted

Hot-reload of [[taint_rules]] ⚠️ → ✅ (caught a gap)

First smoke iteration showed status: no_changes when only [[taint_rules]] changed — build_reload_plan wasn't diffing that field, so should_apply_hot returned false and the swap-store never ran. Pushed 0abf3848:

• New HotAction::ReloadTaintRules variant.
• Diff in build_reload_plan checks old.taint_rules != new.taint_rules and pushes the variant.
• apply_hot_actions_inner logs the action.
• Derived PartialEq + Eq on NamedTaintRuleSet (other types in the chain already had them).
• Reverted the temporary else if fallback in reload_config — now the canonical should_apply_hot path correctly drives the swap.

After fix:

append [[taint_rules]] then POST /api/config/reload
  → hot_actions_applied: ["UpdateDefaultModel", "ReloadTaintRules"]
     status: applied                                              ✓
second reload (no edits)
  → hot_actions_applied: []
     status: no_changes                                           ✓ idempotent
edit action: "warn" → "log", reload
  → hot_actions_applied: ["ReloadTaintRules"]
     status: applied                                              ✓ targeted

Status

All four review concerns now fixed and live-verified:

• 🔴 multi-rule masking — fixed in ef54effb, regression test added.
• 🟡 snapshot semantics — replaced with live Arc<ArcSwap> shared registry in e4b40588; reload-plan integration in 0abf3848.
• 🟡 5x duplicated snapshot expr — snapshot_taint_rules() helper in ef54effb.
• 🟡 dashboard PUT coupling — dedicated PATCH /api/mcp/servers/{name}/taint endpoint in e4b40588.

Plus the pre-existing test toml bug (mcp_servers.tools.* shape) fixed in ef54effb. The multi-threaded kernel-test SIGABRT was confirmed pre-existing on 02eb92af (the original PR head before any fixes).