From c5b406c2e2b306f2faf6ba9f23cdc118e00c354b Mon Sep 17 00:00:00 2001 From: Jarvis Date: Wed, 19 Aug 2026 07:09:07 +0000 Subject: [PATCH 1/2] fix(mcp): keep loading api_key rows projected before the layered ACL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A document written under the previous shape carries mcp_access.mode and no allow list. With allow required at the type level that row failed to deserialize, and the loader skips a row it cannot represent — so the key stopped authenticating for every kind of traffic, not just MCP. The runtime loader now defaults a missing allow to empty, which resolves to a layer allowing nothing: fail-closed on MCP while the key keeps working elsewhere. The write path is unchanged — the strict schema adds allow to required on both layers, so neither a resources file nor the admin API can leave it out. --- crates/aisix-core/src/models/mcp_policy.rs | 27 ++++++++-- crates/aisix-core/src/models/schema.rs | 63 ++++++++++++++++++++-- schemas/resources/api_key.schema.json | 3 +- schemas/resources/mcp_policy.schema.json | 7 +-- 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/crates/aisix-core/src/models/mcp_policy.rs b/crates/aisix-core/src/models/mcp_policy.rs index 7da93af3..7dc3f0d0 100644 --- a/crates/aisix-core/src/models/mcp_policy.rs +++ b/crates/aisix-core/src/models/mcp_policy.rs @@ -48,6 +48,14 @@ pub struct McpPolicy { /// exactly. An empty list allows nothing, which is how a policy blocks /// all MCP access; a policy that only means to subtract tools writes /// `["*"]` here and lists them under `deny`. + /// + /// The write path requires the field (the strict schema adds it to + /// `required`), so a layer never allows something by omission. The + /// runtime loader defaults it to empty instead of rejecting the row: + /// a document written before the layered shape would otherwise fail + /// to deserialize, and a skipped `api_key` row stops authenticating + /// altogether rather than merely losing MCP access. + #[serde(default)] pub allow: Vec, /// Namespaced `__` patterns subtracted from the effective @@ -83,6 +91,10 @@ pub struct McpAccess { /// with the environment and team layers. Same single-`*` glob matching a /// policy's `allow` uses; an empty list leaves the key no MCP access, /// and `["*"]` narrows nothing (useful with `deny` alone). + /// + /// Required on the write path and defaulted by the runtime loader, + /// for the reason given on [`McpPolicy::allow`]. + #[serde(default)] pub allow: Vec, /// Namespaced `__` patterns subtracted from this key's @@ -144,9 +156,18 @@ mod tests { } #[test] - fn allow_is_required_on_both_layers() { - assert!(serde_json::from_str::(r#"{"scope":"env"}"#).is_err()); - assert!(serde_json::from_str::(r#"{"deny":["github__*"]}"#).is_err()); + fn the_loader_defaults_a_missing_allow_to_empty() { + // A row written before the layered shape — e.g. the old + // `{"mode":"inherit"}` key block — must still deserialize. It + // resolves to a layer allowing nothing (fail-closed) rather than + // being skipped, which for an api_key would drop the whole key. + // The write path still rejects it; see + // `mcp_policy_requires_an_explicit_allow_side` in schema.rs. + let p: McpPolicy = serde_json::from_str(r#"{"scope":"env"}"#).unwrap(); + assert!(p.allow.is_empty()); + + let a: McpAccess = serde_json::from_str(r#"{"mode":"inherit"}"#).unwrap(); + assert!(a.allow.is_empty()); } #[test] diff --git a/crates/aisix-core/src/models/schema.rs b/crates/aisix-core/src/models/schema.rs index 59139abf..66c1efdf 100644 --- a/crates/aisix-core/src/models/schema.rs +++ b/crates/aisix-core/src/models/schema.rs @@ -92,7 +92,7 @@ fn closes_on_write(resource: &str) -> bool { pub fn resource_root_schema(resource: &str, strict: bool) -> Value { let mut schema = match resource { "model" => model_root_schema(strict), - "api_key" => apikey_root_schema(), + "api_key" => apikey_root_schema(strict), "provider_key" => provider_key_root_schema(), "guardrail" => guardrail_root_schema(), "guardrail_attachment" => guardrail_attachment_root_schema(), @@ -100,7 +100,7 @@ pub fn resource_root_schema(resource: &str, strict: bool) -> Value { "observability_exporter" => observability_exporter_root_schema(), "rate_limit_policy" => rate_limit_policy_root_schema(), "mcp_server" => mcp_server_root_schema(), - "mcp_policy" => mcp_policy_root_schema(), + "mcp_policy" => mcp_policy_root_schema(strict), "a2a_agent" => a2a_agent_root_schema(), "oidc_provider" => oidc_provider_root_schema(), "claim_mapping" => claim_mapping_root_schema(), @@ -468,8 +468,37 @@ pub fn model_root_schema(strict: bool) -> Value { /// `Option` representation so `team_id`/`user_id` keep accepting an explicit /// `null` (cp-api sends `null` to clear team/owner), matching the resource's /// wire contract. -pub fn apikey_root_schema() -> Value { - struct_root_schema::(true) +pub fn apikey_root_schema(strict: bool) -> Value { + let mut schema = struct_root_schema::(true); + if strict { + require_property( + schema + .pointer_mut("/definitions/McpAccess") + .expect("api_key schema defines McpAccess"), + "allow", + ); + } + schema +} + +/// Add `name` to a schema object's `required` list, creating the list +/// when absent. Used for fields the WRITE path must see spelled out +/// while the runtime loader defaults them — a stale row has to keep +/// loading, but a new one must not acquire its meaning by omission. +fn require_property(schema: &mut Value, name: &str) { + let obj = schema + .as_object_mut() + .expect("schema fragment is a JSON object"); + match obj.get_mut("required").and_then(Value::as_array_mut) { + Some(list) => { + if !list.iter().any(|v| v.as_str() == Some(name)) { + list.push(json!(name)); + } + } + None => { + obj.insert("required".to_string(), json!([name])); + } + } } /// Canonical JSON Schema for the `provider_key` resource, derived from the @@ -791,8 +820,11 @@ pub fn mcp_auth_settings_root_schema() -> Value { /// cross-field invariant `schemars` cannot express: a `team`-scoped policy /// must name its team in `scope_ref` (otherwise the row could shadow the /// environment layer). -pub fn mcp_policy_root_schema() -> Value { +pub fn mcp_policy_root_schema(strict: bool) -> Value { let mut schema = struct_root_schema::(true); + if strict { + require_property(&mut schema, "allow"); + } schema .as_object_mut() .expect("mcp_policy root schema is a JSON object") @@ -1980,6 +2012,27 @@ mod tests { validate_mcp_policy(&json!({"scope": "env", "allow": []})).unwrap(); } + #[test] + fn the_lenient_schema_still_loads_a_pre_layer_row() { + // Documents projected before the layered shape carry a `mode` + // and no `allow`. The strict write path rejects them, but the + // runtime loader must still accept them: a rejected `api_key` + // row is skipped entirely, so the key would stop authenticating + // for ALL traffic rather than merely losing MCP access. + let key = json!({ + "key_hash":"9df37f5e7cbc3c391d872742b5f286c242e733a09add9eeaa4d26a599bd90b20", + "allowed_models":[], + "allowed_tools":["github__*"], + "mcp_access": {"mode": "inherit"} + }); + validate_apikey_lenient(&key).unwrap(); + assert!(validate_apikey(&key).is_err()); + + let policy = json!({"scope": "env", "mode": "all"}); + validate_mcp_policy_lenient(&policy).unwrap(); + assert!(validate_mcp_policy(&policy).is_err()); + } + #[test] fn mcp_policy_requires_an_explicit_allow_side() { assert!(validate_mcp_policy(&json!({"scope": "env"})).is_err()); diff --git a/schemas/resources/api_key.schema.json b/schemas/resources/api_key.schema.json index d2580586..7dead82c 100644 --- a/schemas/resources/api_key.schema.json +++ b/schemas/resources/api_key.schema.json @@ -7,7 +7,8 @@ "description": "The API key's own layer of the MCP tool ACL, the same `allow`/`deny` shape an MCP access policy carries. Present means the key constrains its grant; omitted means the key adds no constraint of its own and takes whatever the environment and team layers leave.", "properties": { "allow": { - "description": "Namespaced `__` patterns this key allows, intersected with the environment and team layers. Same single-`*` glob matching a policy's `allow` uses; an empty list leaves the key no MCP access, and `[\"*\"]` narrows nothing (useful with `deny` alone).", + "default": [], + "description": "Namespaced `__` patterns this key allows, intersected with the environment and team layers. Same single-`*` glob matching a policy's `allow` uses; an empty list leaves the key no MCP access, and `[\"*\"]` narrows nothing (useful with `deny` alone).\n\nRequired on the write path and defaulted by the runtime loader, for the reason given on [`McpPolicy::allow`].", "items": { "type": "string" }, diff --git a/schemas/resources/mcp_policy.schema.json b/schemas/resources/mcp_policy.schema.json index 0f35bb6e..9edc5481 100644 --- a/schemas/resources/mcp_policy.schema.json +++ b/schemas/resources/mcp_policy.schema.json @@ -46,7 +46,8 @@ }, "properties": { "allow": { - "description": "Namespaced `__` patterns this layer allows. Entries are matched as single-`*` globs: `\"*\"` allows every tool, `\"__*\"` every tool on one server, and an entry without a `*` matches one tool exactly. An empty list allows nothing, which is how a policy blocks all MCP access; a policy that only means to subtract tools writes `[\"*\"]` here and lists them under `deny`.", + "default": [], + "description": "Namespaced `__` patterns this layer allows. Entries are matched as single-`*` globs: `\"*\"` allows every tool, `\"__*\"` every tool on one server, and an entry without a `*` matches one tool exactly. An empty list allows nothing, which is how a policy blocks all MCP access; a policy that only means to subtract tools writes `[\"*\"]` here and lists them under `deny`.\n\nThe write path requires the field (the strict schema adds it to `required`), so a layer never allows something by omission. The runtime loader defaults it to empty instead of rejecting the row: a document written before the layered shape would otherwise fail to deserialize, and a skipped `api_key` row stops authenticating altogether rather than merely losing MCP access.", "items": { "type": "string" }, @@ -82,8 +83,8 @@ } }, "required": [ - "allow", - "scope" + "scope", + "allow" ], "title": "McpPolicy", "type": "object" From aa9e383cf2ccf1eccbb5d258e64cb7b44cae8248 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Wed, 19 Aug 2026 07:11:40 +0000 Subject: [PATCH 2/2] docs(agents): a projected resource's fields are never type-level required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loader skips a row it cannot deserialize, and a skipped api_key row stops authenticating every kind of traffic — so requiredness belongs in the strict schema, with a fail-closed serde default on the struct. --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index 6e9041b3..6f698ee2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,6 +135,7 @@ The five kinds are the cross-plane taxonomy (cp-admin.yaml `kind`); this repo's - When you touch a model-keyed mechanism (a limit, a guard, an ACL, a config knob, usage/metric attribution, cache keying), answer in the doc comment: does it key on the **requested** entry, the **dispatched** target, or **both**, and what is the behavior for each of the six shapes. - The per-target invariant (`crates/aisix-proxy/AGENTS.md`: "a per-model gate binds each target") is written around `resolve_attempt_models` — the routing-group trunk. **Ensemble panel/judge (`ProxyModelCaller::call`, the streaming judge) and semantic targets (`semantic::resolve`) bypass that trunk**, so a gate wired only into the trunk is silently absent there (the 2026-08 audit found member IP allowlist, health consumption, and retries all missing on the semantic path for exactly this reason — #958). A new per-target gate must be wired into the sub-dispatch paths too, or explicitly deferred with a filed issue. Prefer routing every dispatch through one shared chokepoint so the family can't drift. - **Strict writes, lenient loads.** `model_one_of` has two variants: the **strict** schema (declarative resources file, the published `schemas/resources/model.schema.json`, every strict validator consumer) forbids a knob a kind never resolves — accepted-but-unread config is the #962 class; the **lenient** loader keeps the base XOR so stored rows written by an older build still load, with `Model::strip_kind_inapplicable` dropping the dead knob and reporting it as `inapplicable:` through the partial-compat channel. The two lists MUST mirror each other exactly (strict-forbidden ⇔ lenient-stripped) — a field forbidden-but-not-stripped half-honors; stripped-but-not-forbidden vanishes on load while the write path accepts it. A knob is enforced exactly as written or rejected, never half-honored (#963). +- **Never make a field of a projected resource required at the TYPE level.** Requiredness belongs in the strict schema (`require_property` in `models/schema.rs`), never in the struct: the loader validates leniently and then deserializes, and a row it cannot deserialize is **skipped entirely** (`aisix-etcd/src/loader.rs`). Skipping is survivable for a resource the request path treats as optional, but an `api_key` row that fails to load stops authenticating **every** kind of traffic, not just the feature whose field changed — a far worse outcome than the field defaulting. So a new non-`Option` field, or one that loses `#[serde(default)]`, silently turns every already-projected row into a dead one. Give it a serde default whose meaning is fail-closed, and add it to `required` in the strict schema so the write path still refuses to guess. The control plane must also re-emit the affected collection once (`ReprojectMcpAclOnce` is the pattern) — the stored shape changed, but nothing else re-projects a row whose *content* did not. (Lesson from #993: `allow` was required at the type level in #992, which made every key still projected as `mcp_access: {"mode": "inherit"}` unloadable.) - **`ensemble` is an experimental surface.** Its known parity gaps — member `allowed_cidrs`/guardrail/cooldown/health consumption, Prometheus token+spend attribution, response caching, parent-level generic knobs — are deliberate TODOs under a single future design pass. Do NOT piecemeal-fix one gap ahead of that pass, and do NOT re-audit them as fresh findings. (The one exception is a marshal-family or shared-chokepoint change where covering ensemble is a one-line parallel edit, e.g. projecting an entry-level field the DP already enforces.) - Adding a NEW kind = sweeping every existing model-keyed mechanism against it (grep the kind predicates in `models/model.rs`; every hit re-answers the questions above).