Merge pull request #91 from barbacane-dev/security/plugin-bypass-fixes

Security: fix plugin auth/access bypasses + fail-open controls

Author: ndreno

crates/barbacane-plugin-sdk/src/types.rs

@@ -143,6 +143,44 @@ pub fn streamed_response() -> Response {
     }
 }
 
+/// Resolve the effective client IP, honoring trusted proxies.
+///
+/// `req.client_ip` is the peer the gateway actually observed. The forwarded
+/// headers (`X-Forwarded-For`, `X-Real-IP`) are attacker-controlled, so they are
+/// consulted **only** when the immediate peer is one of `trusted_proxies`; then
+/// the right-most forwarded address that is not itself a trusted proxy is
+/// returned. With no trusted proxies configured (the default), forwarded headers
+/// are ignored entirely and the observed peer is used.
+///
+/// This is the single source of truth for client-IP resolution so security
+/// middlewares (ip-restriction, rate-limit, ai-token-limit) cannot drift into
+/// independently spoofable implementations.
+pub fn resolve_client_ip(req: &Request, trusted_proxies: &[String]) -> String {
+    let peer = req.client_ip.trim();
+    let is_trusted = |ip: &str| trusted_proxies.iter().any(|p| p.trim() == ip);
+
+    if !is_trusted(peer) {
+        return peer.to_string();
+    }
+
+    if let Some(xff) = req.headers.get("x-forwarded-for") {
+        for hop in xff.split(',').map(str::trim).rev() {
+            if !hop.is_empty() && !is_trusted(hop) {
+                return hop.to_string();
+            }
+        }
+    }
+
+    if let Some(real) = req.headers.get("x-real-ip") {
+        let real = real.trim();
+        if !real.is_empty() {
+            return real.to_string();
+        }
+    }
+
+    peer.to_string()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -471,4 +509,38 @@ mod tests {
             .unwrap();
         assert_eq!(bytes, original);
     }
+
+    fn req_with(peer: &str, xff: Option<&str>) -> Request {
+        let mut headers = BTreeMap::new();
+        if let Some(v) = xff {
+            headers.insert("x-forwarded-for".to_string(), v.to_string());
+        }
+        Request {
+            method: "GET".into(),
+            path: "/".into(),
+            query: None,
+            headers,
+            body: None,
+            client_ip: peer.to_string(),
+            path_params: BTreeMap::new(),
+        }
+    }
+
+    #[test]
+    fn resolve_client_ip_ignores_xff_from_untrusted_peer() {
+        // No trusted proxies: a spoofed XFF must be ignored.
+        let req = req_with("203.0.113.9", Some("10.0.0.5"));
+        assert_eq!(resolve_client_ip(&req, &[]), "203.0.113.9");
+    }
+
+    #[test]
+    fn resolve_client_ip_uses_xff_only_behind_trusted_proxy() {
+        let trusted = vec!["203.0.113.9".to_string()];
+        // Peer is the trusted proxy: take the right-most non-trusted hop.
+        let req = req_with("203.0.113.9", Some("198.51.100.7, 203.0.113.9"));
+        assert_eq!(resolve_client_ip(&req, &trusted), "198.51.100.7");
+        // Peer is NOT trusted: XFF ignored even though one is configured.
+        let req2 = req_with("198.51.100.200", Some("10.0.0.5"));
+        assert_eq!(resolve_client_ip(&req2, &trusted), "198.51.100.200");
+    }
 }

plugins/ai-prompt-guard/config-schema.json

@@ -61,6 +61,11 @@
       "description": "Named policy profiles.",
       "additionalProperties": { "$ref": "#/$defs/PromptProfile" },
       "minProperties": 1
+    },
+    "fail_open": {
+      "type": "boolean",
+      "default": false,
+      "description": "When the request body can't be inspected (unparseable JSON or a non-array 'messages'), allow it through instead of rejecting. Defaults to false (fail closed) so the guard can't be bypassed with a malformed body."
     }
   }
 }

plugins/ai-prompt-guard/src/lib.rs

@@ -91,6 +91,13 @@ pub struct AiPromptGuard {
     /// Named profiles the operator can select between.
     profiles: BTreeMap<String, PromptProfile>,
 
+    /// When the request body can't be inspected (unparseable JSON, or a
+    /// `messages` field that isn't an array), allow it through (fail open)
+    /// instead of rejecting. Defaults to false (fail closed), so a client can't
+    /// bypass the guard by sending a body the parser rejects.
+    #[serde(default)]
+    fail_open: bool,
+
     /// Compiled regex cache, keyed by profile name. Populated lazily.
     #[serde(skip)]
     compiled: BTreeMap<String, Vec<Regex>>,
@@ -129,17 +136,37 @@ impl AiPromptGuard {
             return Action::ShortCircuit(regex_compile_error_response(&profile_name, &err));
         }
 
+        // No body: nothing to inspect (and nothing to inject into).
         let Some(body_bytes) = req.body.as_deref() else {
             return Action::Continue(req);
         };
 
+        // Unparseable body is the classic guard-bypass vector. Fail closed by
+        // default so a client can't skip filtering by sending malformed JSON.
         let mut root: serde_json::Value = match serde_json::from_slice(body_bytes) {
             Ok(v) => v,
-            Err(_) => return Action::Continue(req),
+            Err(_) => {
+                if self.fail_open {
+                    return Action::Continue(req);
+                }
+                return Action::ShortCircuit(reject(&profile, "request body is not valid JSON"));
+            }
         };
 
-        let Some(messages) = root.get("messages").and_then(|v| v.as_array()).cloned() else {
-            return Action::Continue(req);
+        let messages = match root.get("messages") {
+            // No `messages` field at all: a different request shape with no chat
+            // messages to guard — pass through.
+            None => return Action::Continue(req),
+            // Present but not an array: malformed; fail closed unless opted out.
+            Some(v) => match v.as_array() {
+                Some(arr) => arr.clone(),
+                None => {
+                    if self.fail_open {
+                        return Action::Continue(req);
+                    }
+                    return Action::ShortCircuit(reject(&profile, "'messages' is not an array"));
+                }
+            },
         };
 
         // --- Message count limit ---
@@ -493,6 +520,7 @@ mod tests {
                 .into_iter()
                 .map(|(k, v)| (k.to_string(), v))
                 .collect(),
+            fail_open: false,
             compiled: BTreeMap::new(),
             compile_errors: BTreeMap::new(),
         }
@@ -855,13 +883,36 @@ mod tests {
         assert!(matches!(p.on_request(r), Action::Continue(_)));
     }
 
+    // PL-6: an unparseable body must NOT bypass the guard by default.
     #[test]
-    fn non_json_body_continues() {
+    fn non_json_body_fails_closed() {
         mock_host::reset();
         let mut p = single_profile_plugin(profile_with(Some(5), None, vec![]));
+        match p.on_request(req("not json")) {
+            Action::ShortCircuit(resp) => assert_eq!(resp.status, 400),
+            Action::Continue(_) => panic!("unparseable body must fail closed"),
+        }
+    }
+
+    #[test]
+    fn non_json_body_continues_when_fail_open() {
+        mock_host::reset();
+        let mut p = single_profile_plugin(profile_with(Some(5), None, vec![]));
+        p.fail_open = true;
         assert!(matches!(p.on_request(req("not json")), Action::Continue(_)));
     }
 
+    // PL-6: a `messages` field that isn't an array fails closed by default.
+    #[test]
+    fn non_array_messages_fails_closed() {
+        mock_host::reset();
+        let mut p = single_profile_plugin(profile_with(Some(5), None, vec![]));
+        match p.on_request(req(r#"{"messages":"oops"}"#)) {
+            Action::ShortCircuit(resp) => assert_eq!(resp.status, 400),
+            Action::Continue(_) => panic!("non-array messages must fail closed"),
+        }
+    }
+
     #[test]
     fn body_without_messages_continues() {
         mock_host::reset();

plugins/ai-proxy/config-schema.json

@@ -23,7 +23,7 @@
         },
         "api_key": {
           "type": "string",
-          "description": "Provider API key. Supports `env://VAR` substitution. Omit for Ollama (unauthenticated local endpoint)."
+          "description": "Provider API key. Use a secret reference (`env://VAR` or `file://...`) rather than a literal value, so the key is resolved at runtime and never baked into the compiled artifact. Omit for Ollama (unauthenticated local endpoint)."
         },
         "base_url": {
           "type": "string",
@@ -58,7 +58,7 @@
         },
         "api_key": {
           "type": "string",
-          "description": "Provider API key. Supports `env://VAR` substitution."
+          "description": "Provider API key. Use a secret reference (`env://VAR` or `file://...`) rather than a literal value, so the key is resolved at runtime and never baked into the compiled artifact."
         },
         "base_url": {
           "type": "string",
@@ -84,7 +84,7 @@
     },
     "api_key": {
       "type": "string",
-      "description": "API key for the flat config. Supports `env://VAR` substitution."
+      "description": "API key for the flat config. Use a secret reference (`env://VAR` or `file://...`) rather than a literal value, so the key is resolved at runtime and never baked into the compiled artifact."
     },
     "base_url": {
       "type": "string",

plugins/ai-token-limit/config-schema.json

@@ -51,6 +51,17 @@
       "description": "Source of the per-consumer partition key. Accepted forms: `client_ip`, `header:<name>`, `context:<key>`, or a literal string (shared budget across all requests). Matches the `rate-limit` plugin's semantics.",
       "default": "client_ip"
     },
+    "trusted_proxies": {
+      "type": "array",
+      "items": { "type": "string" },
+      "default": [],
+      "description": "Exact IPs of trusted reverse proxies. With partition_key 'client_ip', X-Forwarded-For / X-Real-IP are honored only behind these; otherwise the observed peer IP is used so clients cannot rotate XFF to evade their budget."
+    },
+    "fail_open": {
+      "type": "boolean",
+      "default": false,
+      "description": "When the host rate limiter is unavailable, allow the request (fail open) instead of rejecting with 503. Defaults to false (fail closed)."
+    },
     "count": {
       "type": "string",
       "description": "Which token counts charge against the budget. `prompt` counts input tokens only, `completion` counts output tokens only, `total` counts both.",