diff --git a/CLAUDE.md b/CLAUDE.md index 6f698ee2..9e9c4038 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -136,6 +136,7 @@ The five kinds are the cross-plane taxonomy (cp-admin.yaml `kind`); this repo's - 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.) +- **Never change a projected field's shape or value domain in place — the previous release must keep loading the row.** The supported upgrade order is control plane first, then data planes, with an arbitrarily long window; a released DP is immutable, so whatever the new CP projects must still parse one release back. Unknown *fields* are tolerated by design (serde_ignored → partial-compat report), but a malformed *known* field — a reshape, a lost default, a new enum value — fails the row and the loader skips it whole (the blast radius of the rule above). A reshape therefore ships under a NEW field name (the old one is never reused), or as a **same-name dual-generation document** when the old and new keys don't collide: the CP emits one document valid for both generations, and this side carries a consumed-and-ignored tombstone for the old selector — `McpAccess::legacy_mode` is the template: `#[serde(default, rename = "...", skip_serializing)]` + `#[schemars(skip)]` so the strict write path still rejects it, with a comment naming the retirement condition. A new enum value in an existing field cannot be made safe DP-side at all (lenient parsing keeps enums closed — it row-kills every older DP), so the paired CP PR must gate it behind `dpCompatGate` until the fleet minimum reads it. Whenever new semantics are invisible to the old release, verify the old default direction there: fail-closed or no-op is required; if it is fail-open, the CP must project an old-shape tombstone at the most restrictive value. - **`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). diff --git a/crates/aisix-core/src/models/mcp_policy.rs b/crates/aisix-core/src/models/mcp_policy.rs index 7dc3f0d0..c623052a 100644 --- a/crates/aisix-core/src/models/mcp_policy.rs +++ b/crates/aisix-core/src/models/mcp_policy.rs @@ -101,6 +101,23 @@ pub struct McpAccess { /// effective grant, using the same single-`*` glob matching as `allow`. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub deny: Vec, + + /// Compatibility tombstone for the pre-0.10.0 `mode` selector. The + /// control plane projects `"mode": "deny"` alongside the layered + /// shape so a 0.9.x data plane — where `mode` is required and `deny` + /// means "no MCP tool access" — still loads the whole api_key row, + /// fail-closed, instead of skipping it (a skipped row stops the key + /// authenticating for EVERY kind of traffic). This generation + /// consumes and ignores the value; the field exists only so the + /// loader does not report the tombstone as partial compat on every + /// row. Any JSON shape is accepted so a malformed tombstone can + /// never kill the row. Hidden from the schemas — the strict write + /// path closes unknown fields, so resource authors cannot set it — + /// and never re-serialized. Retire together with the CP emission + /// once 0.9.x is out of the supported upgrade window. + #[serde(default, rename = "mode", skip_serializing)] + #[schemars(skip)] + pub legacy_mode: Option, } impl Resource for McpPolicy { @@ -225,6 +242,25 @@ mod tests { assert!(blocked.deny.is_empty()); } + #[test] + fn mcp_access_consumes_the_legacy_mode_tombstone() { + // The CP projects `"mode": "deny"` next to the layered shape so a + // 0.9.x DP loads the row fail-closed. This generation reads its + // own half, tolerates any tombstone shape, and never re-emits it. + let a: McpAccess = + serde_json::from_str(r#"{"mode":"deny","allow":["github__*"],"deny":["x__y"]}"#) + .unwrap(); + assert_eq!(a.allow, vec!["github__*"]); + assert_eq!(a.deny, vec!["x__y"]); + assert_eq!(a.legacy_mode, Some(serde_json::json!("deny"))); + + let malformed: McpAccess = serde_json::from_str(r#"{"allow":[],"mode":5}"#).unwrap(); + assert_eq!(malformed.legacy_mode, Some(serde_json::json!(5))); + + let v = serde_json::to_value(&a).unwrap(); + assert!(v.get("mode").is_none()); + } + #[test] fn empty_deny_stays_off_the_wire() { let p: McpPolicy = serde_json::from_str(r#"{"scope":"env","allow":["*"]}"#).unwrap(); diff --git a/crates/aisix-core/src/models/schema.rs b/crates/aisix-core/src/models/schema.rs index 66c1efdf..90ac2104 100644 --- a/crates/aisix-core/src/models/schema.rs +++ b/crates/aisix-core/src/models/schema.rs @@ -1964,12 +1964,23 @@ mod tests { #[test] fn apikey_mcp_access_rejects_the_removed_mode_field() { + // Write path: `mode` is gone from the authored shape. Read path: + // the CP projects `"mode": "deny"` as a tombstone so 0.9.x DPs + // load the row fail-closed, and this generation's loader must + // keep accepting the hybrid document. let v = json!({ "key_hash":"9df37f5e7cbc3c391d872742b5f286c242e733a09add9eeaa4d26a599bd90b20", "allowed_models":[], "mcp_access": {"mode": "inherit", "allow": ["*"]} }); assert!(validate_apikey(&v).is_err()); + validate_apikey_lenient(&v).unwrap(); + validate_apikey_lenient(&json!({ + "key_hash":"9df37f5e7cbc3c391d872742b5f286c242e733a09add9eeaa4d26a599bd90b20", + "allowed_models":[], + "mcp_access": {"mode": "deny", "allow": ["github__*"], "deny": ["x__y"]} + })) + .unwrap(); } #[test] diff --git a/crates/aisix-etcd/src/loader.rs b/crates/aisix-etcd/src/loader.rs index 10b871db..0ddc0997 100644 --- a/crates/aisix-etcd/src/loader.rs +++ b/crates/aisix-etcd/src/loader.rs @@ -957,6 +957,31 @@ mod tests { ); } + #[test] + fn api_key_legacy_mcp_access_mode_tombstone_is_not_partial_compat() { + // The CP projects `"mode": "deny"` inside `mcp_access` so a 0.9.x + // DP — where `mode` is required — loads the row fail-closed + // instead of skipping it. This generation consumes the tombstone + // (`McpAccess::legacy_mode`); it must not surface as a + // partial-compat field on every key that carries a block. + let entries = vec![raw( + "/aisix/api_keys/k-tombstone", + br#"{ + "key_hash": "1460db1b6902f8b1fc2a40d9381a24d0fd22c3bc1b2c6f999c521da73776fbe0", + "allowed_models": ["m"], + "mcp_access": {"mode": "deny", "allow": ["github__*"], "deny": ["x__y"]} + }"#, + 1, + )]; + let (snap, stats) = build_snapshot("/aisix", &entries); + assert_eq!(stats.accepted, 1, "rejections: {:?}", stats.rejections); + assert!(stats.partially_compatible.is_empty()); + let entry = snap.apikeys.get_by_id("k-tombstone").unwrap(); + let access = entry.value.mcp_access.as_ref().unwrap(); + assert_eq!(access.allow, vec!["github__*"]); + assert_eq!(access.deny, vec!["x__y"]); + } + #[test] fn nested_unknown_field_reports_dotted_path() { let entries = vec![raw( diff --git a/tests/e2e/src/cases/mcp-access-policy-e2e.test.ts b/tests/e2e/src/cases/mcp-access-policy-e2e.test.ts index f76c274f..cf2b746a 100644 --- a/tests/e2e/src/cases/mcp-access-policy-e2e.test.ts +++ b/tests/e2e/src/cases/mcp-access-policy-e2e.test.ts @@ -43,6 +43,7 @@ const KEY_T1_WIDE = "sk-mcp-t1-wide"; const KEY_T2 = "sk-mcp-t2"; const KEY_NARROW = "sk-mcp-t2-narrow"; const KEY_BLOCKED = "sk-mcp-blocked"; +const KEY_TOMBSTONE = "sk-mcp-tombstone"; const sha256 = (s: string) => createHash("sha256").update(s).digest("hex"); @@ -166,6 +167,7 @@ describe("mcp access policy e2e: env, team and key layers intersect", () => { [KEY_T2, ENV_GRANT], [KEY_NARROW, ["alpha__echo"]], [KEY_BLOCKED, []], + [KEY_TOMBSTONE, ["alpha__echo", "alpha__reverse"]], ]; beforeAll(async () => { @@ -231,6 +233,15 @@ describe("mcp access policy e2e: env, team and key layers intersect", () => { }), ); await seed.createApiKey(keyDoc(KEY_BLOCKED, { mcp_access: { allow: [] } })); + // The exact hybrid document the control plane projects: the layered + // shape plus the `"mode": "deny"` tombstone that keeps a 0.9.x DP + // loading the row fail-closed. This generation must consume the + // tombstone — authenticate the key and enforce the allow/deny half. + await seed.createApiKey( + keyDoc(KEY_TOMBSTONE, { + mcp_access: { mode: "deny", allow: ["alpha__*"] }, + }), + ); // Probe EVERY key to its expected steady state: keys are written at // higher revisions than servers/policies, but each key's list also @@ -250,6 +261,19 @@ describe("mcp access policy e2e: env, team and key layers intersect", () => { await beta?.close(); }); + test("a CP-projected legacy-mode tombstone is consumed, not enforced", async (ctx) => { + if (!etcdReachable || !app) return ctx.skip(); + + // `mode` would mean "no MCP access" one release back; here it must + // be inert — the key's own allow intersects the env layer as usual. + await expectList(KEY_TOMBSTONE, ["alpha__echo", "alpha__reverse"]); + const ok = await callTool(KEY_TOMBSTONE, "alpha__echo", "hi"); + expect(ok).toEqual({ ok: true, text: "alpha:hi" }); + const denied = await callTool(KEY_TOMBSTONE, "beta__echo", "hi"); + expect(denied.ok).toBe(false); + expect(denied.error).toContain("not available"); + }); + test("a key with no block of its own takes the env layer unchanged", async (ctx) => { if (!etcdReachable || !app) return ctx.skip();