fix: close fail-open paths found by the security analysis

[mkoushni]

Summary

Closes the first dedicated security review of the engine (#16). /code-review, Bugbot, and a compound-engineering pass over the priority surfaces (APL evaluator, JWT, OAuth, executor, PDPs, bundled HTTP) produced seven fail-open or leak findings that are fixed here, plus one accepted stub. Dispositions are in docs/security-analysis.md.

Fixed

• F1 — A panicking parallel: branch was dropped, so a sibling Allow stood in for a gate that never finished.
• F2 — The bundled hyper transport now enforces http_addr (loopback, RFC 1918, link-local/metadata, CGNAT, IPv6 literals including bracketed mapped forms). with_allow_private_destinations is the hatch for a local IdP.
• F3 — Leg-2 token-exchange denials no longer forward error_description or the raw body (the IdP can echo subject_token).
• F4 — An omitted or empty JWT audiences list no longer disables aud checking. Hatch: skip_audience_validation: true.
• F5 — != on a missing attribute is true, matching !(x == y), so role != "admin": deny fires when the role is absent.
• F6 — A present amount that is not a finite number ("NaN", "Infinity", "lots", …) Denies an order comparison. Treating the comparison as false skipped args.amount > 10000: deny; ! cannot invert the new Deny into Allow. Missing amounts stay false (F5).
• F7 — A handler result the executor cannot downcast is an execution error (on_error: fail halts), not Allow.

Accepted with reason

• F8 — injection.scan / pii.detect are taint markers; they do not inspect the field. Detection lives in plugin(...).
• Deliberate knobs (CEL/OPA on_error: allow, plugin on_error: ignore, cache TTL ceiling, delegation_without_identity_resolution as an alarm, etc.) are tabulated in the write-up with the operator-visible consequence.

Breaking: a JWT issuer config that listed no audiences is refused at load unless skip_audience_validation: true is set. An order comparison on a non-numeric present value now Denies the phase (previously the comparison was false and a deny rule did not fire).

Test plan

• Fail-closed regressions for F1–F7 (happy-path-only tests were not treated as closing a finding)
• F6 rule-level test: "NaN" / "Infinity" / "lots" against args.amount > 10000: deny Denies, including under !(...); "5000" still Allows
• make audit (cargo deny check) green
• make ci on this PR
• Confirm a JWT config that omits audiences fails at load, and skip_audience_validation: true still accepts a token minted for another app
• Confirm HyperTransport without the hatch rejects 169.254.169.254 and [::ffff:169.254.169.254] without dialling

[praxis-bot]

PR Review

Summary: Comprehensive security pass closing 7 fail-open paths (F1-F7). The fixes correctly implement fail-closed semantics and are well-tested. Found 2 Critical issues and 3 Large concerns.

Severity	Count
Critical	2
Large	3

---

Critical Issues

[Critical] crates/ppe-apl-core/src/evaluator.rs - coerce_f64_attr precision loss

The coerce_f64_attr function can silently truncate large integers (>2^53) during coercion in mixed int/float comparisons. While integer-to-integer comparisons are handled exactly (lines 262-269), a comparison like args.amount > 9007199254740993.0 (float literal) would coerce a 2^53+1 integer through f64, collapsing it to 2^53 and making the comparison incorrectly return false. This could create a fail-open for large amounts.

Fix: Document this limitation, or reject mixed int/float comparisons on large integers with an Unorderable error to fail closed.

---

[Critical] builtins/plugins/identity-jwt/src/config.rs - Validation order inconsistency

The validation checks skip_audience_validation && !audiences.is_empty() for conflicting config (line 770), then checks for empty audiences without skip (line 777). An explicitly empty audiences: [] with skip_audience_validation: false will hit the second check, but an omitted audiences field hits the same check. The error messages will differ based on whether the list was omitted vs. explicitly empty, which could confuse operators.

Fix: Reorder checks so "must list at least one audience" comes before the conflict check.

---

Large Issues

[Large] crates/ppe-apl-core/src/evaluator.rs - Parallel branch panic messages

The panic handling for parallel branches (F1 fix, line ~1220) correctly fails closed but only includes branch index in the error message. Since each branch knows its effect, including the effect type (plugin name, PDP dialect, etc.) would significantly aid debugging.

Fix: Enhance panic message to include identifying information about which effect panicked.

---

[Large] crates/ppe/src/http_hyper.rs - TOCTOU in SSRF protection

The SSRF protection (F2) correctly checks the resolved IP address, but there's a time-of-check-time-of-use gap if DNS changes between resolution and connection. An attacker controlling DNS could return a safe IP during check, then update to 169.254.169.254 before connection. DNS TTL caching mitigates this but doesn't eliminate it.

Fix: Document this limitation, or re-verify IP immediately before connect if the transport allows.

---

[Large] builtins/plugins/identity-jwt/src/resolver.rs - Confusing error code for config fault

The new NoAudiences error (F4 fix, line ~1047) uses code auth.no_audiences, but this is a configuration error discovered at request time, not an authentication failure. The token itself may be valid. Operators might be confused why this appears as an auth error rather than a config error.

Fix: Use auth.config_error or ensure this is always caught at config load time (it is in config.rs:777), treating the runtime path as a defensive fallback only.

[mkoushni]

Addressed the five review findings in 68b5114:

• Mixed int/float order past 2^53 is Unorderable (fail-closed) instead of rounding through f64.
• Audience validation now checks the empty-list fault before the skip-vs-list conflict, so omitted and audiences: [] share one message.
• Parallel panic/timeout reasons include the branch effect (plugin(boom), dialect, …).
• Documented that EgressResolver returns the filtered SocketAddrs and the connector dials those, so a later DNS rebind does not change the peer. Residual: a public host we accepted could still forward privately.
• Empty audiences after load denies as auth.config_error (config fault fallback), not auth.no_audiences.

[araujof]

@mkoushni please ping @terylt when this PR is ready to review. I know you mentioned were going to do some additional revision based on #55. Conflict wise, I only see a simple changelog conflict. Thanks!

[terylt]

Hi @mkoushni, Really nice work! I have a few findings that might be worth a look at:

Recommendation: three findings to address before merge. Findings 1 and 2 each leave a finding the write-up records as closed partly open. Finding 3 is an incompleteness in the 2^53 guard rather than a fail-open in the original shape.

Findings

#	Severity	Location	Claim
1	Medium	crates/ppe/src/http_hyper.rs:282	The egress refusal never becomes HttpTransportError::Rejected, so a filtered hostname reports idp_unreachable and is retried
2	Medium	builtins/plugins/identity-jwt/src/resolver.rs:1050	A token carrying no aud claim passes a configured audience list
3	Low	crates/ppe-apl-core/src/evaluator.rs:316	The 2^53 exactness guard covers Int and Float but not numeric strings

Findings 1 and 2 were confirmed by running probes against the branch, not by reading alone. Both probes are reproduced below and were reverted afterwards.

1. The egress refusal never reaches HttpTransportError::Rejected

classify looks for EGRESS_DENIED_PREFIX inside err.to_string():

let msg = err.to_string();
if let Some(reason) = msg.split(EGRESS_DENIED_PREFIX).nth(1) {
    return HttpTransportError::Rejected(reason.trim().to_owned());
}

hyper_util::client::legacy::Error formats as write!(f, "client error ({:?})", self.kind) (hyper-util 0.1.20, src/client/legacy/client.rs:1628). The source chain is reachable through source() but is not in the Display string, so the marker written at http_hyper.rs:338 never arrives and the branch is dead.

Probe, http://localhost:1/jwks with no hatch, so the name resolves to loopback and EgressResolver drops it:

PROBE err = Connect("client error (Connect)")

The block itself holds. Nothing dials the private address, so this is not an SSRF bypass. What it costs is the three mechanisms built around the distinction:

• delegator.rs:305 maps Rejected to delegation.egress_denied, and approver.rs:195 does the same for elicitation.egress_denied. A token_endpoint hostname resolving to 10.x now reports delegation.idp_unreachable, which is the phantom network problem the comment three lines above says costs an afternoon in DNS.
• http_retry.rs:157 returns false for Rejected. Connect falls through to line 164 and is retried across the whole backoff budget, on every request, against a destination that can never be reached.
• The private_address_reason string is discarded. http_addr.rs states the reason exists so an operator reads "private address" and reaches for the escape hatch instead of a generic denial. They get client error (Connect).

The CHANGELOG line "The bundled hyper transport produces the refusal" is true for IP literals only.

Suggested fix: walk std::error::Error::source() in classify, or carry a typed error through the resolver and downcast, rather than matching on a formatted string.

The gap is also a test gap. All five new F2 tests use IP literals, which take the pre-connect check at http_hyper.rs:365 and never enter EgressResolver. One hostname case would have caught this.

2. A token with no aud claim passes a configured audience list

validate_token sets the audience but leaves required_spec_claims at its default:

} else {
    let aud_refs: Vec<&str> = issuer.audiences.iter().map(String::as_str).collect();
    validation.set_audience(&aud_refs);
}

jsonwebtoken 10.4 documents on Validation::validate_aud: "Validation only happens if aud claim is present in the token. Adding aud to required_spec_claims will make it required." set_required_spec_claims is never called, so the default stays ["exp"] and a token with the claim absent takes the _ => {} arm of the audience match.

Probe against resolver_on_header, which configures audiences: ["test-aud"]. Token signed by the trusted issuer with iss, sub and exp, no aud:

PROBE no-aud continue_processing = true violation = None

This is a residual of F4's own surface rather than a separate issue. The hatch's doc comment says skip_audience_validation: true accepts a token minted "for any app (or none)", and the knob table at security-analysis.md:178 repeats "Any aud (or none)". Both read as though the default path refuses "none". It does not.

Suggested fix, one line in the else branch:

validation.set_required_spec_claims(&["exp", "iss", "aud"]);

Worth a regression test in the same shape as empty_audience_list_rejects_the_token.

3. The 2^53 exactness guard skips numeric strings

coerce_f64_attr and coerce_f64_lit route Int through i64_as_exact_f64 and Float through finite_f64, but the String arm is a bare parse::<f64>() that rounds silently:

AttributeValue::Int(a) => i64_as_exact_f64(*a),
AttributeValue::Float(a) => finite_f64(*a),
AttributeValue::String(s) => s.trim().parse::<f64>().ok().and_then(finite_f64),

Probe on args.amount > 9007199254740992: deny:

PROBE string decision = Allow      // args.amount = "9007199254740993"
PROBE int decision    = Deny       // args.amount = Int(9007199254740993)

The comment directly above the coercion gives string arguments as the reason the coercion exists at all ("LLM tool arguments routinely arrive as strings"), so the guard misses the operand shape it was written for. Whether it is worth closing depends on whether amounts above 9e15 are in scope; in minor units they are not unreachable.

Suggested fix: in the String arm, try parse::<i64>() first and send that through i64_as_exact_f64, falling back to f64 only for genuinely fractional strings.

Non-blocking

• Or propagates Unorderable. evaluator.rs:107 uses ?, so an unorderable left operand denies even when a right operand would be true. Separately, evaluate_rules returns Deny before it looks at effects, so an unorderable comparison in a rule whose effect is taint or allow denies the whole phase. Both are defensible fail-closed choices. The blast radius is wider than "the deny rule fires", and neither is in the operator-visible table.
• No CHANGELOG entry for 68b5114 (mixed int/float order past 2^53). That is a policy-visible change and belongs beside the F6 bullet.
• install_default_http_transport (crates/ppe/src/lib.rs:247) does not mention the new refusal or with_allow_private_destinations. A standalone embedder pointed at a local Keycloak gets an unexplained failure, worse in combination with finding 1.
• security-analysis.md:86-87 says all five F2 tests assert may_have_reached_peer() == false. Three assert only the variant.
• Optional hardening on F3, not a regression. err.error is still IdP-controlled text forwarded verbatim, so an IdP that echoes subject_token there still lands it on the violation. Leg 1 has the same shape, so this is consistent rather than new. An allow-list of the RFC 6749 / 8693 codes, or a length cap, would close the stated threat model rather than narrow it.

What was verified and held

• F1, parallel panic. BranchOutcome::Panicked and TimedOut become Decision::Deny with a labelled reason, Aborted stays a no-op. Reading dispatch_parallel, the ordering is by branch index, and a panic does not short-circuit siblings because the predicate only matches EffectOutcome::Halt, so an explicit sibling Deny is not displaced by index. TimedOut has no injection test, which the write-up states.
• F5, != on a missing attribute. CompareOp::NotEq is produced only from an explicit != in policy text (parser.rs:690), so no desugaring inherits the new truth value. not in was already true on a missing key, so the two are now consistent.
• F6 and the Unorderable plumbing. All seven eval_expression call sites are converted: evaluate_rules, both And and Or walks, Not, Effect::When, the elicitation scope predicate, and the Stage::Redact condition. No caller outside evaluator.rs uses the changed helpers.
• F7, unreadable handler result. The serial else at executor.rs:597 fires exactly when the if let Some(erased) at :488 fails, and its on_error match is identical to the existing Ok(Err(e)) arm. The concurrent path routes None to BranchData::Error, which the post-loop already handles per plugin.
• F3, leg-2 sanitization. Leg 1 and leg 2 now produce the same shape, and TokenErrorResponse no longer models error_description at all, so serde drops it rather than relying on a call site to omit it.
• Tests and lint. cargo test green for praxis-policy-apl-core and praxis-policy-core (697 lib tests), praxis-policy-plugin-identity-jwt and praxis-policy-plugin-delegator-oauth (375 across all targets). cargo clippy --all-targets -- -D warnings clean for those four crates and for praxis-policy with http-hyper.
• docs/security-analysis.md is the strongest part of the PR. It triages rather than lists, names the operator-visible consequence for each accepted fail-open knob, and records what was rejected as a false positive. The "surfaces reviewed without a new accepted finding" section is the part that makes the pass auditable later.

What was not checked

• No live IdP, no real DNS beyond localhost, and no network egress. Finding 1's probe resolves through the OS resolver to loopback, which is enough to reach EgressResolver but does not exercise a public name that rebinds.
• make ci and make audit were not run end to end. The PR asserts both are green and the per-crate runs above are consistent with that, but the full two-pass workspace test and cargo deny were not repeated here.
• Integration test bodies were read for the changed assertions only, not in full.
• Cargo.lock was not reviewed beyond noting the single tower-service addition.

Reproducing the probes

Worktree at scratchpad/pr45, branch pr-45-review, base 355a586.

Finding 1, appended to crates/ppe/src/http_hyper.rs and run with cargo test -p praxis-policy --features http-hyper --lib:

#[tokio::test]
async fn hostname_resolving_to_loopback_is_rejected_not_connect() {
    let err = HyperTransport::new()
        .execute(HttpRequest::get("http://localhost:1/jwks"))
        .await
        .expect_err("localhost resolves to loopback");
    assert!(matches!(err, HttpTransportError::Rejected(_)), "got {err:?}");
}

Finding 2, appended to the tests module in builtins/plugins/identity-jwt/src/resolver.rs:

#[tokio::test]
async fn a_token_with_no_aud_claim_is_refused() {
    let resolver = resolver_on_header("authorization");
    let claims = json!({
        "iss": "https://idp.example",
        "sub": "alice",
        "exp": seconds_from_now(3_600),
    });
    let token = sign_with(b"test-secret", &claims);
    let payload = IdentityPayload::new(token, TokenSource::Bearer);
    let r = resolver
        .handle(&payload, &Extensions::default(), &mut PluginContext::new())
        .await;
    assert!(!r.continue_processing, "a token with no aud must not pass a configured list");
}

Finding 3, appended to the tests module in crates/ppe-apl-core/src/evaluator.rs:

#[test]
fn a_string_encoded_large_int_does_not_round_through_f64() {
    let rule = crate::parser::parse_rule("args.amount > 9007199254740992: deny", "test")
        .expect("parses");
    let mut bag = AttributeBag::new();
    bag.set("args.amount", "9007199254740993");
    assert!(
        matches!(evaluate_rules(std::slice::from_ref(&rule), &bag), Decision::Deny { .. }),
        "2^53 + 1 as a string is over the cap"
    );
}

All three fail on 1cf055b.

[terylt]

One thing that came up while verifying this branch, flagging it here so it does not look like a finding against the PR: running make ci locally hit a failure in a_bundle_dropping_inherited_authentication_is_reported_at_load (crates/ppe-core/tests/identity_route_e2e.rs:1092, "one report per affected route: []"). It is not from this PR. It reproduces on the base commit 355a586, this branch does not touch that file or the alarm code, and CI is green here (all seven checks) as it is on main. It is parallel-only: the whole identity_route_e2e binary failed roughly one run in ten on both the branch and the base, the test passes every time on its own, and eight runs with --test-threads=1 never failed. The likely mechanism, inferred from the shape rather than instrumented, is that two tests call alarms_raised_by_loading, which installs a scoped subscriber with tracing::subscriber::with_default on its own test thread, while tracing's callsite interest cache is process-global, so two thread-local dispatchers registering the same warn! callsite concurrently can leave the event filtered out for one of them. The companion assertion at :1112 expects the alarm to be absent, which is consistent with only the positive one flaking. Probably worth its own issue against main: either route both tests through one process-wide subscriber that fans out to a per-thread sink, or serialize the pair, rather than relying on two scoped subscribers racing.

[mkoushni]

Addressed the three review findings in 8855038:

#	Finding	Fix
1	Hostname egress was Connect (idp_unreachable, retried)	classify walks Error::source() for ppe-egress-denied:. localhost now returns Rejected.
2	Token with no aud claim passed a configured list	set_required_spec_claims(&["exp", "iss", "aud"]). Missing aud is auth.audience_mismatch. Hatch still accepts none.
3	String amounts past 2^53 rounded through f64	Integer spellings go through i64_as_exact_f64 before parse::<f64>(). "9007199254740993" > 9007199254740992: deny Denies.

Also: F2 tests now assert may_have_reached_peer() == false; install_default_http_transport documents the refusal and the local-IdP hatch; CHANGELOG + docs/security-analysis.md updated.