test: tests and general improvements

[shaneutt]

This provides several new tests for coverage, and makes some general improvements.

[praxis-bot]

PR Review

Tests and general improvements across the policy engine: CEL float-narrowing bugfix, OPA subtree collision check, JWT Bearer case-insensitive stripping, parallel branch panic fail-closed, field-op array fan-out, route cache tag keying, executor deny synthesis, label merge refactor, delegation hop field checks, delegated_tokens serialization fix, duplicate key rejection, authentication step validation, and multi-elicit rejection.

Overall this is a high-quality set of security-hardening and correctness improvements with thorough test coverage for each change. The fail-closed posture is consistent throughout.

Severity	Count
Critical	0
Large	0
Medium	1

No non-inlineable findings.

[szedan-rh]

Thanks for putting this together — I went through the whole branch and there's a lot of genuinely good work in here. Before the critical notes, the things I think are clearly right and should land as-is:

• The CEL float change (builtins/pdps/cel/src/activation.rs) is the standout. The old narrowing produced an actual outcome inversion — maximum confidence denied while a lower one was allowed — and the new tests pin both operand orders and the in operator. The deliberate divergence from OPA (Rego's unified number type) is well reasoned and documented on both sides.
• extensions_changed missing http/custom (route_handler.rs) was a real dropped-header bug, and the test covers both the None → Some and the replaced-Arc arms.
• The executor synth-deny for a blocking plugin with no violation closes a genuine fail-open.
• attenuation being dropped by the delegation invoker was minting broader tokens than the author asked for. deny_unknown_fields on AttenuationConfig is exactly right for a narrowing structure.
• The delegated_tokens serde adapter fixes a field that literally could not serialize once a token was minted, and the legacy-map read path is a nice touch.

I also verified the branch locally: cargo check --workspace --all-targets is clean with no warnings, and cargo test across ppe-apl-core / ppe-core / ppe-apl-runtime is fully green. So none of what follows is a build or test failure — it's design and behaviour.

I do have some concerns, mostly around two of the fixes trading a fail-open for a fail-under-load, and around how the change set is packaged. Happy to be wrong on any of these.

---

Two things I'd like to resolve before merge

1. The array fan-out can become ~100k sequential plugin round-trips per request

crates/ppe-apl-core/src/route.rs:118,228 and crates/ppe-apl-core/src/evaluator.rs:1214-1265

The fix is right in intent — result.rows.ssn | redact silently no-op'ing on an array was a real PII leak, and I'm glad it's closed. My concern is the implementation shape: it's a serial for path in paths loop calling evaluate_pipeline(...).await per leaf, and Stage::Plugin issues a host-bridged (often network-bound) plugins.invoke(...).await.

So something as ordinary as:

result:
  rows.ssn: "plugin(dlp-scanner)"

against a tool returning 100k rows now performs 100,000 sequential plugin invocations inside a single request, bounded only by MAX_EXPANDED_PATHS. Before this change it performed zero.

The underlying tension is that fan-out cost is leaves × stages, and a plugin/scan stage is orders of magnitude more expensive than a redact stage — so a single bound on leaves can't serve both cases.

Suggestion: make the cap pipeline-aware. A pure-transform pipeline (redact/mask/hash/omit) can fan out wide and cheaply; a pipeline containing Plugin or Scan wants a much smaller cap plus a concurrency-limited fan-out rather than a serial loop (and ideally a batched invoker shape so one call carries N values). Deriving the cap from pipeline.stages at compile time and stashing it on FieldRule would keep the decision where the information is.

2. The packaging makes this hard to review and hard to release

25 files, +2068/−288, with four omnibus commits each bundling several independent security-semantic changes. The title and body describe roughly the test additions; the rest is ~10 independent behaviour changes, at least six of which are breaking for existing config — and CHANGELOG.md is untouched.

By my count the config-breaking set is:

1. Duplicate keys in routes: / args: / result: / route plugins: now hard-error
2. Two elicitations reachable in one phase now hard-error
3. taint: / when: / do: as a step map key now hard-errors
4. authentication: steps naming an unregistered plugin now hard-error
5. attenuation: with an unknown key now hard-errors
6. An OPA inline module in a parent-or-child package of a global one now denies
7. CEL no longer narrows whole-valued floats to int — this changes evaluation outcomes for existing, unmodified policy files
8. Field pipelines now traverse arrays, and can now deny a request that previously passed

(7) is the one I'd most want called out. An operator upgrading gets different authorization decisions from a policy they never touched, with nothing telling them to re-test. This repo already has the **Breaking for existing config** convention in the changelog (the perform_http entry) — I think these all warrant it.

Suggestion: split along the existing commit boundaries (apl-core parser/evaluator; core executor/cache/merges; builtins; tests) and add the Changed / Security entries. The builtins group looks closest to ready.

---

Higher-priority notes

The expand_field_paths cap bounds output, not work — route.rs:415-475

The doc says the caps exist because fanning out over attacker-shaped payload.result is "otherwise unbounded in stack depth and allocation." But out.len() > MAX_EXPANDED_PATHS counts emitted leaves, while the cost is in the traversal:

for (i, item) in items.iter().enumerate() {
    if !walk(item, segs, &join(prefix, &i.to_string()), depth + 1, out) { return false; }
}

Every visit allocates two Strings before recursing. When elements don't reach the leaf — an array of scalars, or objects lacking the target key — out never grows, so the width guard never fires and the walk runs the whole subtree. expand_field_paths(result, "rows.ssn") over a large scalar array does O(nodes) allocations and returns Some(vec![]).

Two aggravating factors: walk is a synchronous recursive CPU loop inside an async fn with no yield point, so it holds a worker thread for its full duration; and MAX_FANOUT_DEPTH = 128 is well above where multiplicative breadth cost starts to bite.

Total work is linear in payload size rather than truly unbounded, so this isn't a hang from a small input — but the allocation amplification is precisely what the constant was introduced to bound, and I don't think it does.

Suggestion: count visits rather than emissions (thread a node budget through walk), and build paths as segment slices rather than re-format!ing at every hop, materialising the dotted string only for emitted leaves.

The label-laundering defence now lives only in the executor — container.rs:389-410 / executor.rs:562-568

Removing the superset gate from merge_security is correct for the stated bug — an append_labels-only plugin sees a filtered empty set, so its legitimate additions were being dropped whenever canonical was non-empty. Good catch, good test, and the layering argument (capability context belongs in the executor) is sound.

My concern is that Extensions::merge_owned stayed pub while its safety property moved out of it. The crate explicitly contemplates callers outside the executor — chain_extends is documented as "Public so out-of-process hosts can apply the same validation to a chain arriving over the wire before it reaches the merge." A host that assembles an OwnedExtensions from the wire and calls merge_owned (exactly the pattern that sibling doc invites) now silently loses the read_labels laundering check, because it never had the capability set to run labels_ok with.

Suggestion: move the context to the check rather than the check away from the data — either merge_owned(&mut self, owned, capabilities: &CapabilitySet), or make merge_owned pub(crate) and expose merge_plugin_result(owned, capabilities) as the only public merge entry point.

Fan-out breaks the documented plugin field-name contract — route.rs:135,245

Inside the fan-out loop, evaluate_pipeline is still passed &rule.field — the unexpanded path — so every element's plugin invocation gets field_name = "rows.ssn".

get_dotted's own doc says it's public "because host bridges read fields back out of their own payload projections — a plugin dispatched from a pipeline stage reports a new value for a field it was handed by name." That lookup now returns None, since segment_get on an Array with a non-numeric segment yields nothing. So a plugin stage under a fan-out receives a name it can't resolve and can't distinguish row 0 from row 99,999.

The tests only exercise redact and omit, both of which ignore field_name, so this stays invisible in CI.

Suggestion: pass the concrete expanded path (rows.0.ssn) — it resolves correctly through the now-array-aware get_dotted. If rule.field is wanted for reporting, carry both on PluginInvocation::Field.

The one-elicitation-per-phase ban is broader than the constraint requires — rules.rs:337-375, parser.rs:2997, visitor.rs:807

I traced the underlying constraint and it's real — retry state does ride one echoed id. But count_elicits sums mutually exclusive branches: When arms are summed ("the compiler cannot prove two conditions disjoint"), and PDP on_allow/on_deny are summed on a verdict-flip argument.

That makes this a hard load error:

pre_invocation:
  - when: "args.amount > 10000"
    do: ["require_approval(cfo, from: user.manager)"]
  - when: "args.amount <= 10000"
    do: ["confirm(user, from: user.sub)"]

Tiered approval by amount feels close to the canonical policy shape rather than an exotic one, and there's no annotation, opt-out, or documented workaround short of splitting into two routes with duplicated conditions.

The thing I keep coming back to: the single shared retry id is the actual defect, and this fixes it in the config validator instead. ElicitStep already carries a source string — would hash(request_id, step.source) give a stable per-step id without a protocol change on the echo side?

If that's out of scope for this PR, a smaller step would be counting When arms as max rather than sum (a single evaluation walk enters at most one), leaving the PDP-arm conservatism as a separately justified case.

Route cache key now includes the raw tag set, against a 10k cap with no eviction — engine.rs:1696-1745,1798-1805

The correctness argument is right — omitting tags served a tagged request the untagged lineup, a real fail-open.

The cost side is what worries me. route_cache is capped at DEFAULT_ROUTE_CACHE_MAX_ENTRIES = 10_000 with reject-on-full and no eviction, so once full it stops caching everything until the next config reload. Key cardinality was entity × hook × scope, bounded by the policy; it's now multiplied by the number of distinct tag sets the host injects. A host injecting anything request-correlated (tenant, priority, experiment bucket) fills 10k entries quickly and then permanently degrades every request to a full resolution, with one warn! to show for it.

Two smaller things in the same area: the comment at engine.rs:1704 still reads "Fast path: zero-allocation cache lookup with raw_entry", but the lines just above it now do a Vec<&str> allocation plus a sort on every request. And the doc says tags are held "sorted and deduplicated" while the code only sorts (harmless today since meta.tags is a HashSet, but the two disagree).

Suggestion: key on the resolution-relevant projection — compute at load time which tags any policy group actually selects on, intersect at request time, and key on that. Cardinality then tracks the policy rather than the caller. LRU eviction would also make cache-full degrade gracefully instead of cliff-edging.

---

Medium — click to expand (8 items)

• Panic payloads reach the client-visible deny reason. evaluator.rs:1138 → route_handler.rs:572. format!("parallel branch {idx} panicked: {msg}") becomes PluginViolation::message. Rust panic payloads routinely carry internal state (expect("no signing key for {kid}"), assert_eq! dumps). Fail-closed is right; surfacing the payload isn't. Consider logging msg under an alarm and returning a generic reason plus a correlation id.

• BranchOutcome::TimedOut still fails open right beside the new fail-closed Panicked arm (evaluator.rs:1125). The commit's own reasoning — an effect that cannot complete denies rather than permits — applies verbatim. It's unreachable today, but it's armed for whenever someone wires up timeout_per_branch.

• Tightened chain_extends fails silently. container.rs:457-490; refusal at 431-435 is a bare return with no warn, no alarm, no audit event — a newly minted delegated token vanishes and the request proceeds. Also, timestamp equality is brittle across a lossy round-trip (this tree already uses SecondsFormat::Secs for delegation output at delegation_invoker.rs:281). Adding authorization_details and ttl_seconds is clearly right; timestamp might be better as a hop content hash.

• The OPA package check is narrower than the threat. packages_share_subtree stops "inline module in a sub-package of a global package", but a global rule can read any data path — a global package authz with allow if data.overrides[input.subject.id] is still fully overridable by an inline package overrides, which passes the check. Worth stating the residual gap in the module doc. Separately, the new symmetric arm now rejects a global package authz.helpers library plus an inline package authz route policy, which previously worked.

• Reserved-keyword handling is a two-place denylist that already disagrees. parser.rs:1475-1484 skips delegate/sequential/parallel/restrict/taint/when/do; parser.rs:1493-1502 rejects only taint/when/do. And the bug class isn't fully closed — - allow:\n - deny still compiles to IsTrue("allow") guarding a deny. An allowlist of predicate-shaped keys would close it structurally.

• Duplicate-key rejection covers four maps. ConfigYaml.other (#[serde(flatten)]) and everything under it — global:, defaults:, tags:, plugin_settings: — still last-wins, and the stated rationale applies equally to a duplicated global: block.

• Valkey EXPIRE returning false is still swallowed. store.rs:128-150. Folding timeout into the alarm path is right, but Ok(Ok(_)) => None discards false, which means the key did not exist — arguably the more likely production shape, and precisely the "taint expires mid-session with no signal" case the commit message targets.

• The elicit-count validator is duplicated verbatim in parser.rs:2997 and visitor.rs:807, with near-identical message strings. The visitor's check subsumes the parser's; two copies of a security rule will drift.

• deserialize_any in the raw-credentials adapter forecloses non-self-describing formats (bincode/postcard/MessagePack-compact), which matters given the doc frames this as serving the wire channel and snapshots.

Low / nits — click to expand

• remove_dotted on a numeric leaf now does items.remove(i), which shifts subsequent indices. Safe today only because expand_field_paths emits at most one explicit index per array — an invariant that isn't stated anywhere.
• HashMap::with_capacity(access.size_hint().unwrap_or(0)) in both new visitors takes an attacker-influenceable hint; worth clamping.
• expand_field_paths allocates a Vec<String> per field rule per request even when the path contains no arrays (the common case).
• Numeric-segment ambiguity: an object key "0" and array index 0 are now indistinguishable in a dotted path. Probably fine, worth a doc line.
• validate_identity reports route {i}; the route index is the least useful identifier available when route.tool is right there.
• route_result_over_large_fanout_denies asserts !r.result_modified under "nothing may be redacted on a fail-closed deny" — that holds only because the failing rule is first. A preceding rule that already redacted would leave the payload half-mutated on a deny.
• get_dotted is pub and its semantics changed (numeric segments now index arrays) — semver-visible for host bridges.
• The manual Debug for JwtIdentityResolver emits both pending_jwks_count and a redacted pending_jwks; the latter adds nothing once the count is there.
• strip_bearer_prefix splits on ' ' only — RFC 9110 OWS permits a tab. Not a regression and the failure mode is a clean deny, but the doc claims conformance a little more strongly than the code delivers.
• visitor_config_errors.rs restyles every section banner and relocates the two helpers to the bottom of the file — ~40 lines of unrelated diff in a security-relevant PR.

---

Questions I couldn't answer from the code

• What's the largest tool result a deployment is expected to redact, and what pipelines are attached to it? MAX_EXPANDED_PATHS = 100_000 reads as chosen without a plugin-bearing pipeline in mind — is there a workload behind it?
• Is passing the unexpanded rule.field to per-element plugin invocations intentional? If so, how is a plugin meant to identify which element it was handed?
• Is a per-step elicitation retry id (derived from ElicitStep.source) not viable? And was rejecting the tiered-approval-by-amount shape a considered trade, or a side effect of the conservative sum?
• What do hosts actually put in meta.tags? If anything request-correlated goes in there, the reject-on-full cache becomes a permanent cliff after a short warm-up.
• Is there a real out-of-process host calling merge_owned directly? If not, narrowing the API makes the whole question go away.
• Does any supported host round-trip a delegation chain through a lossy time representation?
• Is the residual "global rule reads an arbitrary data.* path" gap in the OPA check accepted, or was it believed closed?
• Would you consider the CEL float change breaking for existing policy files? I'd lean yes, and fairly prominently, since it changes decisions on configs nobody edited.

---

Sorry for the length — the density of real fixes in here is what made it worth going through carefully. The builtins group in particular looks close to ready. Happy to pair on any of the above, or to open separate issues for the ones you'd rather not carry in this PR.

[araujof]

Thanks for the PR, @shaneutt ! I did a quick scan last week, and overall it looks good to me.

There is one issue that we need to address before merging, just to avoid some bigger reconciliation later. I'd like to merge #55 first, then merge/rebase it here, and fix the APL/Config examples. For example, apl: and other "legacy" keys were removed from the grammar, along with many other cleanups and fixes. So, please hold on on merging.

I assigned @terylt to review #55 but if you like to give it a second pair of eyes, that would be great. :)

[shaneutt]

Due to constraints on my time I'm going to close this for now. Please feel free to take anything from it you like later.

[araujof]

Due to constraints on my time I'm going to close this for now. Please feel free to take anything from it you like later.

I will create another branch cherry picking from this one. It's probably going to be easier than rebasing it. Thanks!

[araujof]

@shaneutt see #63