fix: critical and high issues from review.

Author: terylt

crates/apl-cpex/src/session_resolver.rs

@@ -3,55 +3,56 @@
 // SPDX-License-Identifier: Apache-2.0
 // Authors: Teryl Taylor
 //
-// 5-tier session-id resolver. Tiers 1–4 are ported from the Python
-// apl-plugins `SessionResolver` (cpex/framework/session.py). Tier 0 is
-// a cpex-Rust addition: `AgentExtension.session_id` is the slot any
-// upstream plugin/middleware uses to inject an already-resolved
-// session id (DB lookup, external service, custom routing). The
-// resolver walks these tiers in order, returning the first one that
-// hits:
+// 3-tier session-id resolver. The Python apl-plugins `SessionResolver`
+// (cpex/framework/session.py) shipped a 4-tier version including a
+// client-supplied `X-CPEX-Session-Id` header tier. **That tier is
+// excluded by design here**: an authenticated client can set the
+// header to another subject's known session id and inherit their
+// accumulated taint labels, or to a new value and escape their own
+// tainted session — defeating `session.labels`-based deny policies
+// entirely. The Python comment framed the header as a feature ("lets
+// a smart client maintain its own session boundary"); under threat
+// modeling it is a privilege-escalation channel with no surviving
+// use case the other tiers don't cover. If a future deployment needs
+// client-supplied session grouping, the right shape is a subject-
+// bound hash (`sha256(subject_id : client_value)`), not the raw
+// header value.
+//
+// The resolver walks these tiers in order, returning the first hit:
 //
 //   0. `agent`      — `AgentExtension.session_id`. A *pre-resolved*
-//      value: somebody upstream decided what the session is and wrote
-//      it here. Highest priority because it represents authority, not
-//      derivation — overriding this with a derived value would discard
-//      that upstream decision. Plugins that need bespoke session
-//      resolution (e.g., reading from a separate session-management
-//      service) write here and let the resolver pick it up.
+//      value: an upstream plugin or middleware decided what the
+//      session is and wrote it here. Highest priority because it
+//      represents authority, not derivation — overriding this with a
+//      derived value would discard that upstream decision. Plugins
+//      that need bespoke session resolution (e.g., reading from a
+//      separate session-management service) write here and let the
+//      resolver pick it up.
 //
 //   1. `token_claim` — explicit `session_id` claim in the inbound JWT.
 //      Strongest binding among the *derived* tiers: the auth issuer
 //      chose this session and signed it into the token. Read from
 //      `SecurityExtension.subject.claims["session_id"]`.
 //
-//   2. `header`     — `X-CPEX-Session-Id` request header. Client-
-//      controlled; lets a smart client maintain its own session boundary
-//      (e.g., grouping a multi-turn workflow that spans multiple JWTs).
-//      Read from `HttpExtension.request_headers` (case-insensitive).
-//
-//   3. `identity`   — derived: sha256(sub : caller_workload : this_workload)[:16].
+//   2. `identity`   — derived: sha256(sub : caller_workload : this_workload)[:16].
 //      No special infrastructure needed; the triple is already populated
 //      by `apl-identity-jwt`'s claim mapping. Same user + same agent +
 //      same gateway = same session, stable across token refresh (the
 //      claims are stable even when the token string isn't).
 //
-//   4. `none`       — no usable identifier; caller (CmfPluginInvoker)
+//   3. `none`       — no usable identifier; caller (CmfPluginInvoker)
 //      skips hydration / persistence. Returns `Ok(None)` so the caller
 //      can distinguish "no session" from "resolver error" if we ever
 //      add an error variant.
 //
 // Each tier reads from a typed `Extensions` field, not raw JWT/HTTP
 // payloads — those have already been mapped by upstream identity
-// plugins (apl-identity-jwt) and the praxis filter. The resolver
-// stays free of crypto/parsing logic.
+// plugins (apl-identity-jwt). The resolver stays free of crypto /
+// parsing logic.
 
 use cpex_core::extensions::Extensions;
 use sha2::{Digest, Sha256};
 
-/// Standard header name. Matches `cpex/framework/session.SESSION_HEADER`
-/// from the Python predecessor. Case-insensitive lookup at read time.
-const SESSION_HEADER: &str = "x-cpex-session-id";
-
 /// Which tier produced the session id. Useful for diagnostics / audit
 /// and to let downstream code branch on binding strength (e.g., only
 /// trust `token_claim`-derived sessions for the highest-stakes
@@ -63,8 +64,6 @@ pub enum SessionSource {
     Agent,
     /// JWT `session_id` claim — strongest binding among derived tiers.
     TokenClaim,
-    /// Client-provided `X-CPEX-Session-Id` header.
-    Header,
     /// Derived from the identity triple. Stable across token refresh.
     Identity,
 }
@@ -74,7 +73,6 @@ impl SessionSource {
         match self {
             SessionSource::Agent => "agent",
             SessionSource::TokenClaim => "token_claim",
-            SessionSource::Header => "header",
             SessionSource::Identity => "identity",
         }
     }
@@ -85,7 +83,7 @@ impl SessionSource {
 /// every tier comes up empty (anonymous request, no claims, no
 /// header, no identity).
 ///
-/// Identity-tier (3) requires at minimum `security.subject.id` to be
+/// Identity-tier (2) requires at minimum `security.subject.id` to be
 /// populated — without an end-user identifier there's no meaningful
 /// session boundary to hash against. The other two identity-triple
 /// components (caller_workload, this_workload) fall back to the
@@ -115,17 +113,7 @@ pub fn resolve_session(ext: &Extensions) -> Option<(String, SessionSource)> {
         }
     }
 
-    // Tier 2: client-supplied header. Case-insensitive lookup —
-    // praxis lowercases, but HTTP/1 spec allows any case.
-    if let Some(http) = ext.http.as_deref() {
-        for (k, v) in &http.request_headers {
-            if k.eq_ignore_ascii_case(SESSION_HEADER) && !v.is_empty() {
-                return Some((v.clone(), SessionSource::Header));
-            }
-        }
-    }
-
-    // Tier 3: identity-derived. Hash the triple
+    // Tier 2: identity-derived. Hash the triple
     // (end-user : calling agent : our gateway) — stable across token
     // refresh because all three components survive token rotation.
     if let Some(sec) = ext.security.as_deref() {
@@ -161,7 +149,7 @@ pub fn resolve_session(ext: &Extensions) -> Option<(String, SessionSource)> {
         }
     }
 
-    // Tier 4: no session.
+    // Tier 3: no session.
     None
 }
 
@@ -286,53 +274,17 @@ mod tests {
         assert_eq!(src, SessionSource::Identity);
     }
 
-    // --- Tier 2: header ---
+    // --- Tier 2 (`X-CPEX-Session-Id` header) is intentionally absent ---
+    //
+    // The Python `SessionResolver` included a header tier; cpex Rust
+    // does not. See the module-level doc comment for the threat model.
+    // A spoofing-regression guard lives below in
+    // `header_x_cpex_session_id_is_ignored`.
 
-    #[test]
-    fn tier2_header_hits_when_no_token_claim() {
-        let sec = SecurityExtension {
-            subject: Some(subject_with_claims(Some("alice"), &[])),
-            ..Default::default()
-        };
-        let mut http = HttpExtension::default();
-        http.request_headers
-            .insert("X-CPEX-Session-Id".into(), "sess-from-header".into());
-        let ext = Extensions {
-            security: Some(Arc::new(sec)),
-            http: Some(Arc::new(http)),
-            ..Default::default()
-        };
-
-        let (sid, src) = resolve_session(&ext).expect("should resolve");
-        assert_eq!(sid, "sess-from-header");
-        assert_eq!(src, SessionSource::Header);
-    }
+    // --- Tier 2: identity ---
 
     #[test]
-    fn tier2_header_lookup_is_case_insensitive() {
-        // Praxis lowercases all headers; cpex shouldn't care.
-        let sec = SecurityExtension {
-            subject: Some(subject_with_claims(Some("alice"), &[])),
-            ..Default::default()
-        };
-        let mut http = HttpExtension::default();
-        http.request_headers
-            .insert("x-cpex-session-id".into(), "lowercase-wins".into());
-        let ext = Extensions {
-            security: Some(Arc::new(sec)),
-            http: Some(Arc::new(http)),
-            ..Default::default()
-        };
-
-        let (sid, src) = resolve_session(&ext).expect("should resolve");
-        assert_eq!(sid, "lowercase-wins");
-        assert_eq!(src, SessionSource::Header);
-    }
-
-    // --- Tier 3: identity ---
-
-    #[test]
-    fn tier3_identity_derived_when_no_claim_no_header() {
+    fn tier2_identity_derived_when_no_claim() {
         let sec = SecurityExtension {
             subject: Some(subject_with_claims(Some("alice@corp.com"), &[])),
             caller_workload: Some(WorkloadIdentity {
@@ -355,7 +307,7 @@ mod tests {
     }
 
     #[test]
-    fn tier3_identity_is_stable_across_calls() {
+    fn tier2_identity_is_stable_across_calls() {
         // Same triple → same session id. Property guarantees that
         // a token refresh (which doesn't change sub/caller/this) keeps
         // the session intact.
@@ -381,7 +333,7 @@ mod tests {
     }
 
     #[test]
-    fn tier3_distinguishes_different_users() {
+    fn tier2_distinguishes_different_users() {
         let alice = SecurityExtension {
             subject: Some(subject_with_claims(Some("alice"), &[])),
             ..Default::default()
@@ -396,7 +348,7 @@ mod tests {
     }
 
     #[test]
-    fn tier3_distinguishes_different_agents() {
+    fn tier2_distinguishes_different_agents() {
         // Same user, two different agents → different sessions.
         // Important so a malicious agent's accumulated taints don't
         // affect a different agent that user runs.
@@ -415,18 +367,18 @@ mod tests {
         assert_ne!(sid1, sid2);
     }
 
-    // --- Tier 4: none ---
+    // --- Tier 3: none ---
 
     #[test]
-    fn tier4_no_session_when_no_data() {
+    fn tier3_no_session_when_no_data() {
         let ext = Extensions::default();
         assert!(resolve_session(&ext).is_none());
     }
 
     #[test]
-    fn tier4_no_session_when_no_subject_id() {
+    fn tier3_no_session_when_no_subject_id() {
         // Security exists but no subject id → identity can't hash.
-        // Header / claim are absent too. Should be None.
+        // Claim is absent too. Should be None.
         let sec = SecurityExtension {
             subject: Some(SubjectExtension::default()), // id = None
             ..Default::default()
@@ -435,52 +387,46 @@ mod tests {
         assert!(resolve_session(&ext).is_none());
     }
 
-    // --- Tier ordering ---
-
-    #[test]
-    fn token_claim_wins_over_header() {
-        let sec = SecurityExtension {
-            subject: Some(subject_with_claims(
-                Some("alice"),
-                &[("session_id", "from-token")],
-            )),
-            ..Default::default()
-        };
-        let mut http = HttpExtension::default();
-        http.request_headers
-            .insert("x-cpex-session-id".into(), "from-header".into());
-        let ext = Extensions {
-            security: Some(Arc::new(sec)),
-            http: Some(Arc::new(http)),
-            ..Default::default()
-        };
-
-        let (sid, src) = resolve_session(&ext).unwrap();
-        assert_eq!(sid, "from-token");
-        assert_eq!(src, SessionSource::TokenClaim);
-    }
+    // --- Spoofing guard (regression test for P0-2) ---
 
     #[test]
-    fn header_wins_over_identity() {
+    fn header_x_cpex_session_id_is_ignored() {
+        // The Python apl-plugins resolver honored an `X-CPEX-Session-Id`
+        // header tier between token_claim and identity. We deliberately
+        // dropped it: an authenticated client could set the header to
+        // another subject's session id and inherit their accumulated
+        // taints, or to a random unused value and escape their own
+        // tainted session. This test pins that behaviour: the header is
+        // present, no token claim exists, and the resolver still falls
+        // through to identity-derived (or none) rather than honoring
+        // the header. If a future PR adds a header tier without
+        // subject binding, this test fails.
         let sec = SecurityExtension {
             subject: Some(subject_with_claims(Some("alice"), &[])),
             caller_workload: Some(WorkloadIdentity {
-                client_id: Some("would-derive-this".into()),
+                client_id: Some("agent-007".into()),
                 ..Default::default()
             }),
             ..Default::default()
         };
         let mut http = HttpExtension::default();
         http.request_headers
-            .insert("x-cpex-session-id".into(), "from-header".into());
+            .insert("X-CPEX-Session-Id".into(), "sess-bob-stolen".into());
         let ext = Extensions {
             security: Some(Arc::new(sec)),
             http: Some(Arc::new(http)),
             ..Default::default()
         };
 
-        let (sid, src) = resolve_session(&ext).unwrap();
-        assert_eq!(sid, "from-header");
-        assert_eq!(src, SessionSource::Header);
+        let (sid, src) = resolve_session(&ext).expect("identity should still hit");
+        assert_eq!(
+            src,
+            SessionSource::Identity,
+            "header tier was removed; resolver must NOT honor X-CPEX-Session-Id",
+        );
+        assert_ne!(
+            sid, "sess-bob-stolen",
+            "header value must never become the session id",
+        );
     }
 }

crates/apl-delegator-oauth/src/config.rs

@@ -52,6 +52,16 @@ pub struct OAuthDelegatorConfig {
     /// want a different header (`X-Service-Token`, etc.).
     #[serde(default = "default_outbound_header")]
     pub default_outbound_header: String,
+
+    /// Explicitly allow `http://` for `token_endpoint`. By default,
+    /// the constructor rejects non-https URLs because the
+    /// token-exchange POST sends `client_id:client_secret` and the
+    /// inbound user JWT — leaking either over plaintext defeats the
+    /// whole exchange. Set to `true` ONLY for `http://localhost`
+    /// development against a docker-compose IdP. Production
+    /// deployments must leave this at the default (`false`).
+    #[serde(default)]
+    pub insecure_http: bool,
 }
 
 /// Where the gateway's OAuth client secret is loaded from. Three