Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:<field>` 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).

Expand Down
36 changes: 36 additions & 0 deletions crates/aisix-core/src/models/mcp_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,

/// 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<serde_json::Value>,
}

impl Resource for McpPolicy {
Expand Down Expand Up @@ -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();
Expand Down
11 changes: 11 additions & 0 deletions crates/aisix-core/src/models/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
25 changes: 25 additions & 0 deletions crates/aisix-etcd/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
24 changes: 24 additions & 0 deletions tests/e2e/src/cases/mcp-access-policy-e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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
Expand All @@ -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");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test("a key with no block of its own takes the env layer unchanged", async (ctx) => {
if (!etcdReachable || !app) return ctx.skip();

Expand Down