From d80477bef8a8b653123669c250915b5581546e18 Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Thu, 30 Jul 2026 15:46:43 +0800 Subject: [PATCH 1/5] fix(core): enforce the a2a/mcp name and credential rules on every config path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-`auth_type` credential coupling and the name-shape rules lived only in this gateway's Admin API write handlers. Declaratively configured gateways never saw them, so a `resources.yaml` like a2a_agents: - name: bad/name url: https://agents.example.com/a2a auth_type: bearer loaded clean on main (`OK: loaded 1 resource(s)`) even though the name is the `/a2a/` path segment and `bearer` has no secret to send. Lifts four rules into the canonical schemas, following the `model_one_of` pattern — defined once, injected into the generated schema, so the published JSON Schema and every runtime validator share one definition: - a2a agent `name` must not contain `/` (it is the URL path segment) - mcp server `name` must not contain `__` (tools are exposed as `__`, so the separator would make the split ambiguous) - a2a `bearer`/`api_key` require a non-empty `secret` - mcp `bearer`/`api_key` require `secret`; `oauth2` additionally requires `client_id` and `token_url` One assertion is deliberately inverted: `mcp_server_schema_stays_permissive_on_credential_coupling` asserted that an incomplete oauth2 row still validates, on the reasoning that the write path would catch it and the runtime would degrade that server gracefully. That reasoning depends on a write path existing. Rejecting at load is also the more diagnosable failure — a rejected row is named in `GET /status/config`'s `rejected` array, where a loaded-but-degraded server silently serves no tools. The rationale is recorded at the test. The write handlers' own checks now sit behind the schema gate, so their tests see `AdminError::Schema` instead of `BadRequest`. Both map to 400, so the wire contract is unchanged; those assertions now check the status. Verified with a locally built `aisix validate`: each bad document is rejected with a precise pointer, and valid ones — including a trailing single underscore in an mcp name — still load. --- crates/aisix-admin/src/a2a_agents_handlers.rs | 10 ++- .../aisix-admin/src/mcp_servers_handlers.rs | 14 +++- crates/aisix-core/src/models/a2a_agent.rs | 75 +++++++++++++++-- crates/aisix-core/src/models/mcp_server.rs | 80 +++++++++++++++++- crates/aisix-core/src/models/schema.rs | 42 ++++++++-- schemas/resources/a2a_agent.schema.json | 50 +++++++++++ schemas/resources/mcp_server.schema.json | 83 +++++++++++++++++++ 7 files changed, 335 insertions(+), 19 deletions(-) diff --git a/crates/aisix-admin/src/a2a_agents_handlers.rs b/crates/aisix-admin/src/a2a_agents_handlers.rs index 60cdd16b..a845ad1b 100644 --- a/crates/aisix-admin/src/a2a_agents_handlers.rs +++ b/crates/aisix-admin/src/a2a_agents_handlers.rs @@ -137,6 +137,12 @@ fn assert_unique_name( #[cfg(test)] mod tests { + // These rules moved into the canonical schema (see + // `a2a_agent_credential_coupling` / `mcp_server_credential_coupling` and the + // `name` patterns), so the schema gate now rejects these payloads before + // `decode`'s own checks run. The variant is `Schema` rather than + // `BadRequest`; both map to 400, so the wire contract is unchanged. The + // assertions below check the status-bearing outcome, not the variant. use super::*; use serde_json::json; @@ -144,7 +150,7 @@ mod tests { fn decode_rejects_slash_in_name() { let err = decode(&json!({"display_name": "a/b", "url": "https://x/a2a"})) .expect_err("`/` in the agent name must be rejected"); - assert!(matches!(err, AdminError::BadRequest(_))); + assert_eq!(err.status(), axum::http::StatusCode::BAD_REQUEST); } #[test] @@ -155,7 +161,7 @@ mod tests { "auth_type": "bearer" })) .expect_err("bearer auth without a secret must be rejected"); - assert!(matches!(err, AdminError::BadRequest(_))); + assert_eq!(err.status(), axum::http::StatusCode::BAD_REQUEST); } #[test] diff --git a/crates/aisix-admin/src/mcp_servers_handlers.rs b/crates/aisix-admin/src/mcp_servers_handlers.rs index ff20f647..f0a201ab 100644 --- a/crates/aisix-admin/src/mcp_servers_handlers.rs +++ b/crates/aisix-admin/src/mcp_servers_handlers.rs @@ -197,6 +197,12 @@ fn assert_unique_name( #[cfg(test)] mod tests { + // These rules moved into the canonical schema (see + // `a2a_agent_credential_coupling` / `mcp_server_credential_coupling` and the + // `name` patterns), so the schema gate now rejects these payloads before + // `decode`'s own checks run. The variant is `Schema` rather than + // `BadRequest`; both map to 400, so the wire contract is unchanged. The + // assertions below check the status-bearing outcome, not the variant. use super::*; use serde_json::json; @@ -204,7 +210,7 @@ mod tests { fn decode_rejects_separator_in_name() { let err = decode(&json!({"display_name": "a__b", "url": "https://x/mcp"})) .expect_err("`__` in the server name must be rejected"); - assert!(matches!(err, AdminError::BadRequest(_))); + assert_eq!(err.status(), axum::http::StatusCode::BAD_REQUEST); } #[test] @@ -215,7 +221,7 @@ mod tests { "auth_type": "bearer" })) .expect_err("bearer auth without a secret must be rejected"); - assert!(matches!(err, AdminError::BadRequest(_))); + assert_eq!(err.status(), axum::http::StatusCode::BAD_REQUEST); } #[test] @@ -226,7 +232,7 @@ mod tests { "auth_type": "api_key" })) .expect_err("api_key auth without a secret must be rejected"); - assert!(matches!(err, AdminError::BadRequest(_))); + assert_eq!(err.status(), axum::http::StatusCode::BAD_REQUEST); } #[test] @@ -244,7 +250,7 @@ mod tests { v.as_object_mut().unwrap().remove(missing); let err = decode(&v).unwrap_err(); assert!( - matches!(err, AdminError::BadRequest(_)), + err.status() == axum::http::StatusCode::BAD_REQUEST, "oauth2 without `{missing}` must be a BadRequest" ); } diff --git a/crates/aisix-core/src/models/a2a_agent.rs b/crates/aisix-core/src/models/a2a_agent.rs index f1b5113b..2fd92bb7 100644 --- a/crates/aisix-core/src/models/a2a_agent.rs +++ b/crates/aisix-core/src/models/a2a_agent.rs @@ -17,6 +17,8 @@ use serde::{Deserialize, Serialize}; +use serde_json::{json, Value}; + use crate::resource::Resource; #[derive(Debug, Clone, Serialize, Deserialize, schemars::JsonSchema, PartialEq, Eq)] @@ -30,7 +32,7 @@ pub struct A2aAgent { // lives in `schema::a2a_agent_root_schema`). Re-serialization always // emits `name`. #[serde(alias = "display_name")] - #[schemars(length(min = 1))] + #[schemars(regex(pattern = "^[^/]+$"), length(min = 1))] pub name: String, /// The upstream agent's base URL, such as `https://agents.example.com/a2a`. @@ -58,12 +60,12 @@ pub struct A2aAgent { #[serde(default, skip_serializing_if = "Option::is_none")] pub secret: Option, - // Cross-field coupling (`bearer`/`api_key` require `secret`) is - // deliberately NOT expressed in this flat schema — that would force - // restructuring the resource into a oneOf. The control plane enforces the - // coupling strictly at write time, this gateway's own Admin API re-checks - // it on write, and the runtime degrades gracefully when a snapshot-loaded - // agent is misconfigured. + // Cross-field coupling (`bearer`/`api_key` require a non-empty `secret`) is + // expressed as an injected `allOf` of `if`/`then` subschemas rather than in + // this flat struct — see `a2a_agent_credential_coupling`. That keeps the + // resource flat (no oneOf restructuring) while giving the published schema + // and every runtime validator one shared definition, so a declarative + // `resources.yaml` and the control plane reject the same documents. /// Maximum time, in milliseconds, to wait for a single upstream operation, /// including fetching the agent card or invoking the agent. When omitted, /// AISIX applies a built-in default. @@ -130,10 +132,69 @@ impl Resource for A2aAgent { } } +/// The `auth_type` → credential coupling, as a JSON Schema `allOf` that +/// [`crate::models::schema::a2a_agent_root_schema`] injects into the generated +/// schema. `schemars` cannot express a cross-field conditional, so this is the +/// single definition the published schema and every runtime validator share: +/// declaring `bearer` or `api_key` without a non-empty `secret` leaves the +/// gateway sending an empty credential upstream, so it is rejected at load. +pub fn a2a_agent_credential_coupling() -> Value { + json!([ + { + "if": { "properties": { "auth_type": { "const": "bearer" } }, "required": ["auth_type"] }, + "then": { + "required": ["secret"], + "properties": { "secret": { "type": "string", "minLength": 1 } } + } + }, + { + "if": { "properties": { "auth_type": { "const": "api_key" } }, "required": ["auth_type"] }, + "then": { + "required": ["secret"], + "properties": { "secret": { "type": "string", "minLength": 1 } } + } + } + ]) +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn schema_rejects_slash_in_name() { + // The name is the `/a2a/` path segment, so a slash would split + // into two segments and route somewhere else entirely. + let doc = json!({"name": "a/b", "url": "https://x/a2a"}); + let err = crate::models::schema::validate_a2a_agent(&doc) + .expect_err("a name containing `/` must be rejected"); + assert!(err.path.contains("name"), "unexpected path: {}", err.path); + } + + #[test] + fn schema_requires_secret_for_bearer_and_api_key() { + for auth in ["bearer", "api_key"] { + let doc = json!({"name": "agent", "url": "https://x/a2a", "auth_type": auth}); + crate::models::schema::validate_a2a_agent(&doc) + .expect_err(&format!("{auth} without a secret must be rejected")); + + let empty = + json!({"name": "agent", "url": "https://x/a2a", "auth_type": auth, "secret": ""}); + crate::models::schema::validate_a2a_agent(&empty) + .expect_err(&format!("{auth} with an empty secret must be rejected")); + + let ok = json!({"name": "agent", "url": "https://x/a2a", "auth_type": auth, "secret": "tok"}); + crate::models::schema::validate_a2a_agent(&ok) + .expect("a complete credential set must be accepted"); + } + } + + #[test] + fn schema_accepts_none_auth_without_secret() { + let doc = json!({"name": "agent", "url": "https://x/a2a"}); + crate::models::schema::validate_a2a_agent(&doc).expect("auth_type none needs no secret"); + } + #[test] fn deserialises_minimal_a2a_agent() { let a: A2aAgent = serde_json::from_str( diff --git a/crates/aisix-core/src/models/mcp_server.rs b/crates/aisix-core/src/models/mcp_server.rs index 423c9eb5..626d0c8a 100644 --- a/crates/aisix-core/src/models/mcp_server.rs +++ b/crates/aisix-core/src/models/mcp_server.rs @@ -11,6 +11,7 @@ //! etcd path: `{prefix}/mcp_servers/{uuid}`. Secondary index on `name`. use serde::{Deserialize, Serialize}; +use serde_json::{json, Value}; use crate::resource::Resource; @@ -27,7 +28,11 @@ pub struct McpServer { // lives in `schema::mcp_server_root_schema`). Re-serialization always // emits `name`. #[serde(alias = "display_name")] - #[schemars(length(min = 1))] + // The name is the tool-namespace prefix: this server's tools are exposed to + // MCP clients as `__`, so a name containing the `__` separator + // would make the split ambiguous. Rejected by the pattern below on every + // configuration path. + #[schemars(regex(pattern = "^(?:[^_]|_[^_])*_?$"), length(min = 1))] pub name: String, /// What backs this server: a real upstream MCP server (`mcp`, the @@ -186,10 +191,83 @@ impl Resource for McpServer { } } +/// The `auth_type` → credential coupling, as a JSON Schema `allOf` that +/// [`crate::models::schema::mcp_server_root_schema`] injects into the generated +/// schema. `schemars` cannot express a cross-field conditional, so this is the +/// single definition the published schema and every runtime validator share: +/// an incomplete credential set leaves the gateway authenticating upstream with +/// nothing, so it is rejected at load rather than at first tool call. +pub fn mcp_server_credential_coupling() -> Value { + let secret_required = json!({ + "required": ["secret"], + "properties": { "secret": { "type": "string", "minLength": 1 } } + }); + json!([ + { + "if": { "properties": { "auth_type": { "const": "bearer" } }, "required": ["auth_type"] }, + "then": secret_required + }, + { + "if": { "properties": { "auth_type": { "const": "api_key" } }, "required": ["auth_type"] }, + "then": secret_required + }, + { + "if": { "properties": { "auth_type": { "const": "oauth2" } }, "required": ["auth_type"] }, + "then": { + "required": ["secret", "client_id", "token_url"], + "properties": { + "secret": { "type": "string", "minLength": 1 }, + "client_id": { "type": "string", "minLength": 1 }, + "token_url": { "type": "string", "minLength": 1 } + } + } + } + ]) +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn schema_rejects_tool_namespace_separator_in_name() { + // Tools are exposed as `__`, so `__` inside the name makes + // the split ambiguous. + let doc = json!({"name": "git__hub", "url": "https://x/mcp"}); + let err = crate::models::schema::validate_mcp_server(&doc) + .expect_err("a name containing `__` must be rejected"); + assert!(err.path.contains("name"), "unexpected path: {}", err.path); + + // A single underscore is fine, including a trailing one. + for good in ["git_hub", "github_", "github"] { + let doc = json!({"name": good, "url": "https://x/mcp"}); + crate::models::schema::validate_mcp_server(&doc) + .unwrap_or_else(|e| panic!("{good} should be accepted: {e:?}")); + } + } + + #[test] + fn schema_requires_credentials_per_auth_type() { + for auth in ["bearer", "api_key"] { + let doc = json!({"name": "s", "url": "https://x/mcp", "auth_type": auth}); + crate::models::schema::validate_mcp_server(&doc) + .expect_err(&format!("{auth} without a secret must be rejected")); + } + + // oauth2 needs the client credentials and the token endpoint too. + let partial = + json!({"name": "s", "url": "https://x/mcp", "auth_type": "oauth2", "secret": "cs"}); + crate::models::schema::validate_mcp_server(&partial) + .expect_err("oauth2 without client_id/token_url must be rejected"); + + let complete = json!({ + "name": "s", "url": "https://x/mcp", "auth_type": "oauth2", + "secret": "cs", "client_id": "cid", "token_url": "https://auth/token" + }); + crate::models::schema::validate_mcp_server(&complete) + .expect("a complete oauth2 credential set must be accepted"); + } + #[test] fn deserialises_minimal_mcp_server() { let s: McpServer = serde_json::from_str( diff --git a/crates/aisix-core/src/models/schema.rs b/crates/aisix-core/src/models/schema.rs index bc3fa9b7..84287aa5 100644 --- a/crates/aisix-core/src/models/schema.rs +++ b/crates/aisix-core/src/models/schema.rs @@ -290,6 +290,13 @@ fn accept_renamed_field(schema: &mut Value, canonical: &str, former: &str, note: /// `display_name` (see [`accept_renamed_field`]). pub fn mcp_server_root_schema() -> Value { let mut schema = struct_root_schema::(true); + schema + .as_object_mut() + .expect("mcp server root schema is a JSON object") + .insert( + "allOf".to_string(), + super::mcp_server::mcp_server_credential_coupling(), + ); accept_renamed_field( &mut schema, "name", @@ -331,6 +338,13 @@ pub fn mcp_server_root_schema() -> Value { /// [`accept_renamed_field`]). pub fn a2a_agent_root_schema() -> Value { let mut schema = struct_root_schema::(true); + schema + .as_object_mut() + .expect("a2a agent root schema is a JSON object") + .insert( + "allOf".to_string(), + super::a2a_agent::a2a_agent_credential_coupling(), + ); accept_renamed_field( &mut schema, "name", @@ -2966,16 +2980,34 @@ mod tests { } #[test] - fn mcp_server_schema_stays_permissive_on_credential_coupling() { - // The per-`auth_type` credential coupling (oauth2 ⇒ client_id + - // secret + token_url) is enforced by write paths, not this schema — - // an incomplete oauth2 row must still validate so the snapshot loader - // keeps it (the runtime degrades that server gracefully instead). + fn mcp_server_schema_enforces_credential_coupling() { + // This assertion is the inverse of what it used to be, deliberately. + // The coupling (oauth2 ⇒ client_id + secret + token_url) used to be + // left to write paths, on the reasoning that an incomplete row should + // still load and degrade at runtime. That reasoning depended on a write + // path existing to catch it; with resource writes removed from this + // gateway, leaving the schema permissive means nothing checks the + // coupling at all on the declarative and etcd paths. + // + // Rejecting at load is also the more diagnosable of the two failures: a + // rejected row is named in `GET /status/config`'s `rejected` array, + // whereas a loaded-but-degraded server silently serves no tools. let v = json!({ "display_name": "x", "url": "https://x/mcp", "auth_type": "oauth2" }); + assert!(validate_mcp_server(&v).is_err()); + + // The complete set still validates. + let v = json!({ + "display_name": "x", + "url": "https://x/mcp", + "auth_type": "oauth2", + "secret": "cs", + "client_id": "cid", + "token_url": "https://auth/token" + }); validate_mcp_server(&v).unwrap(); } diff --git a/schemas/resources/a2a_agent.schema.json b/schemas/resources/a2a_agent.schema.json index dc178767..b6d72931 100644 --- a/schemas/resources/a2a_agent.schema.json +++ b/schemas/resources/a2a_agent.schema.json @@ -1,6 +1,54 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "additionalProperties": false, + "allOf": [ + { + "if": { + "properties": { + "auth_type": { + "const": "bearer" + } + }, + "required": [ + "auth_type" + ] + }, + "then": { + "properties": { + "secret": { + "minLength": 1, + "type": "string" + } + }, + "required": [ + "secret" + ] + } + }, + { + "if": { + "properties": { + "auth_type": { + "const": "api_key" + } + }, + "required": [ + "auth_type" + ] + }, + "then": { + "properties": { + "secret": { + "minLength": 1, + "type": "string" + } + }, + "required": [ + "secret" + ] + } + } + ], "anyOf": [ { "required": [ @@ -80,6 +128,7 @@ "display_name": { "description": "Accepted as an alternative spelling of `name`. Provide the label under exactly one of the two names.", "minLength": 1, + "pattern": "^[^/]+$", "type": "string" }, "enabled": { @@ -90,6 +139,7 @@ "name": { "description": "Operator-facing label, unique within the gateway. It is the path segment under which the agent is exposed to callers as `/a2a/`, so it must be a single non-empty URL path segment.", "minLength": 1, + "pattern": "^[^/]+$", "type": "string" }, "protocol_version": { diff --git a/schemas/resources/mcp_server.schema.json b/schemas/resources/mcp_server.schema.json index 836c46f4..382eaf4d 100644 --- a/schemas/resources/mcp_server.schema.json +++ b/schemas/resources/mcp_server.schema.json @@ -1,6 +1,87 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "additionalProperties": false, + "allOf": [ + { + "if": { + "properties": { + "auth_type": { + "const": "bearer" + } + }, + "required": [ + "auth_type" + ] + }, + "then": { + "properties": { + "secret": { + "minLength": 1, + "type": "string" + } + }, + "required": [ + "secret" + ] + } + }, + { + "if": { + "properties": { + "auth_type": { + "const": "api_key" + } + }, + "required": [ + "auth_type" + ] + }, + "then": { + "properties": { + "secret": { + "minLength": 1, + "type": "string" + } + }, + "required": [ + "secret" + ] + } + }, + { + "if": { + "properties": { + "auth_type": { + "const": "oauth2" + } + }, + "required": [ + "auth_type" + ] + }, + "then": { + "properties": { + "client_id": { + "minLength": 1, + "type": "string" + }, + "secret": { + "minLength": 1, + "type": "string" + }, + "token_url": { + "minLength": 1, + "type": "string" + } + }, + "required": [ + "secret", + "client_id", + "token_url" + ] + } + } + ], "anyOf": [ { "required": [ @@ -116,6 +197,7 @@ "display_name": { "description": "Accepted as an alternative spelling of `name`. Provide the label under exactly one of the two names.", "minLength": 1, + "pattern": "^(?:[^_]|_[^_])*_?$", "type": "string" }, "enabled": { @@ -126,6 +208,7 @@ "name": { "description": "Operator-facing label, unique within the gateway. It is used as the namespace prefix for this server's tools, which are exposed to clients as `__`, so it must not contain the reserved separator `__`.", "minLength": 1, + "pattern": "^(?:[^_]|_[^_])*_?$", "type": "string" }, "scopes": { From 12c36410e08d930b09976afeff28eaebbee843da Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Thu, 30 Jul 2026 16:39:24 +0800 Subject: [PATCH 2/5] fix(core): lift the remaining mcp field-coupling rules, and reject a trailing `_` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit findings on the previous commit. The mcp name pattern was wrong, and the previous commit message argued for the wrong side of it. A trailing `_` is not cosmetic: tools are advertised as `__` and parsed back with `split_once("__")`, so `github_` + `list` serializes to `github___list` and resolves to the non-existent server `github` — every tool call against that server fails. Worse, `gh_` + `x` and `gh` + `_x` produce the same wire name, which is the ambiguity the rule exists to prevent. Pattern reverted to reject both `__` and a trailing `_`, with the reasoning recorded. Eight more write-handler rules were left behind, not one. All eight are plain JSON Schema and are now lifted: `spec` is required for and limited to `type: openapi`, must be an object, and must not be a Swagger 2.0 document; `api_key_header` is limited to `type: openapi` with `auth_type: api_key` and must be a legal RFC 7230 header name. Only the three rules that need to parse the OpenAPI document itself (`aisix_mcp::validate_spec`, header-name typing, duplicate tool names) remain, blocked by the aisix-mcp → aisix-core dependency direction. The first draft of that lift silently did nothing: it used `dependentSchemas`, a draft 2019-09 keyword, in a schema that declares draft-07, so the validator ignored it as unknown. Unit tests passed. Caught by running each case through a built `aisix validate`; now spelled `dependencies` and covered by a regression test that says why. Also from the audit: - the a2a name pattern only excluded `/`, but the name is interpolated into the advertised agent-card URL unencoded, so `a?b` advertises a URL whose path is `/a2a/a`. Now excludes `/?#%`, whitespace and control characters - four comments asserted the schema stays permissive, directly above the code that now enforces it - restored the `client_id` doc comment an earlier comment rewrite swallowed, and titled the new `anyOf` branches for the ReDoc tabs - `decode_openapi_type_coupling` asserted on handler-authored messages that the schema gate now pre-empts; it asserts the 400 and the failing pointer instead One diagnosability regression is accepted and recorded at the constraint: rejecting Swagger 2.0 in the schema loses the write path's "convert the spec to OpenAPI 3.x" hint, because a validator reports a pointer and a constraint, not advice. A post-schema semantic hook in the loaders would let the rule and the advice live together. --- crates/aisix-admin/src/a2a_agents_handlers.rs | 5 +- .../aisix-admin/src/mcp_servers_handlers.rs | 23 ++- crates/aisix-core/src/models/a2a_agent.rs | 7 +- crates/aisix-core/src/models/mcp_server.rs | 144 ++++++++++++++++-- crates/aisix-core/src/models/schema.rs | 6 +- schemas/resources/a2a_agent.schema.json | 6 +- schemas/resources/mcp_server.schema.json | 81 +++++++++- 7 files changed, 241 insertions(+), 31 deletions(-) diff --git a/crates/aisix-admin/src/a2a_agents_handlers.rs b/crates/aisix-admin/src/a2a_agents_handlers.rs index a845ad1b..dfedd52e 100644 --- a/crates/aisix-admin/src/a2a_agents_handlers.rs +++ b/crates/aisix-admin/src/a2a_agents_handlers.rs @@ -4,8 +4,9 @@ //! reject duplicate names (409), generate a uuid v4 on POST, bump revision on //! PUT. The name is the path segment under which the agent is exposed //! (`/a2a/`), so it must be a single URL path segment (no `/`). The -//! per-auth_type credential coupling is enforced here too, since the flat -//! schema stays permissive on it. +//! per-auth_type credential coupling is enforced by the canonical schema, so +//! every configuration path rejects an incomplete credential set; the checks +//! below are defense in depth. use aisix_core::models::validate_a2a_agent; use aisix_core::resource::ResourceEntry; diff --git a/crates/aisix-admin/src/mcp_servers_handlers.rs b/crates/aisix-admin/src/mcp_servers_handlers.rs index f0a201ab..58c215b4 100644 --- a/crates/aisix-admin/src/mcp_servers_handlers.rs +++ b/crates/aisix-admin/src/mcp_servers_handlers.rs @@ -284,7 +284,7 @@ mod tests { })) .unwrap_err(); assert!( - matches!(&err, AdminError::BadRequest(m) if m.contains("spec")), + err.status() == axum::http::StatusCode::BAD_REQUEST && err.to_string().contains("spec"), "{err:?}" ); @@ -296,8 +296,13 @@ mod tests { "spec": { "swagger": "2.0", "paths": { "/a": { "get": {} } } } })) .unwrap_err(); + // The schema gate rejects the `swagger` key before the handler's own + // check runs, so the targeted "convert to OpenAPI 3.x" hint is no longer + // what the caller sees — the pointer names `/spec` instead. The rule is + // intact; only the message is less actionable. See the note on + // `mcp_server_credential_coupling`. assert!( - matches!(&err, AdminError::BadRequest(m) if m.contains("OpenAPI 3")), + err.status() == axum::http::StatusCode::BAD_REQUEST && err.to_string().contains("spec"), "{err:?}" ); @@ -313,7 +318,8 @@ mod tests { })) .unwrap_err(); assert!( - matches!(&err, AdminError::BadRequest(m) if m.contains("duplicate tool names")), + err.status() == axum::http::StatusCode::BAD_REQUEST + && err.to_string().contains("duplicate tool names"), "{err:?}" ); @@ -327,7 +333,8 @@ mod tests { })) .unwrap_err(); assert!( - matches!(&err, AdminError::BadRequest(m) if m.contains("auth_type")), + err.status() == axum::http::StatusCode::BAD_REQUEST + && err.to_string().contains("auth_type"), "{err:?}" ); let err = decode(&json!({ @@ -341,7 +348,8 @@ mod tests { })) .unwrap_err(); assert!( - matches!(&err, AdminError::BadRequest(m) if m.contains("header name")), + err.status() == axum::http::StatusCode::BAD_REQUEST + && err.to_string().contains("api_key_header"), "{err:?}" ); @@ -356,8 +364,11 @@ mod tests { field: value })) .unwrap_err(); + // The schema names the offending field via its pointer rather + // than explaining that it is openapi-only. assert!( - matches!(&err, AdminError::BadRequest(m) if m.contains("openapi")), + err.status() == axum::http::StatusCode::BAD_REQUEST + && err.to_string().contains(field), "{field}: {err:?}" ); } diff --git a/crates/aisix-core/src/models/a2a_agent.rs b/crates/aisix-core/src/models/a2a_agent.rs index 2fd92bb7..42c97e2e 100644 --- a/crates/aisix-core/src/models/a2a_agent.rs +++ b/crates/aisix-core/src/models/a2a_agent.rs @@ -26,13 +26,16 @@ use crate::resource::Resource; pub struct A2aAgent { /// Operator-facing label, unique within the gateway. It is the path segment /// under which the agent is exposed to callers as `/a2a/`, so it must - /// be a single non-empty URL path segment. + /// be a single non-empty URL path segment. The name is interpolated into the + /// advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` + /// and whitespace are rejected: `a?b` would advertise a URL whose path is + /// just `/a2a/a`, and the lookup is an exact match on the stored name. // `display_name` is the field's former name; stored documents and // callers that still use it keep deserializing (schema-side acceptance // lives in `schema::a2a_agent_root_schema`). Re-serialization always // emits `name`. #[serde(alias = "display_name")] - #[schemars(regex(pattern = "^[^/]+$"), length(min = 1))] + #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f]+$"), length(min = 1))] pub name: String, /// The upstream agent's base URL, such as `https://agents.example.com/a2a`. diff --git a/crates/aisix-core/src/models/mcp_server.rs b/crates/aisix-core/src/models/mcp_server.rs index 626d0c8a..2b9982ef 100644 --- a/crates/aisix-core/src/models/mcp_server.rs +++ b/crates/aisix-core/src/models/mcp_server.rs @@ -29,10 +29,12 @@ pub struct McpServer { // emits `name`. #[serde(alias = "display_name")] // The name is the tool-namespace prefix: this server's tools are exposed to - // MCP clients as `__`, so a name containing the `__` separator - // would make the split ambiguous. Rejected by the pattern below on every - // configuration path. - #[schemars(regex(pattern = "^(?:[^_]|_[^_])*_?$"), length(min = 1))] + // MCP clients as `__` and parsed back with `split_once("__")` + // (see `aisix_mcp::gateway`). So the name must contain no `__` AND must not + // end in `_`: `gh_` + `x` and `gh` + `_x` both serialize to `gh___x`, and the + // split resolves the former to the non-existent server `gh`. The pattern + // below rejects both shapes on every configuration path. + #[schemars(regex(pattern = "^(?:[^_]|_[^_])*$"), length(min = 1))] pub name: String, /// What backs this server: a real upstream MCP server (`mcp`, the @@ -81,14 +83,12 @@ pub struct McpServer { #[serde(default, skip_serializing_if = "Option::is_none")] pub secret: Option, - // Cross-field coupling (`oauth2` requires `client_id` + `secret` + - // `token_url`; `bearer`/`api_key` require `secret`) is deliberately NOT - // expressed in this flat schema — that would force restructuring the - // resource into a oneOf. The control plane enforces the coupling strictly - // at write time, this gateway's own Admin API re-checks it on write, and - // the runtime degrades gracefully when a snapshot-loaded server is - // mis-configured: its credential exchange fails, its tools become - // unavailable, and the failure is logged like any other upstream failure. + // Cross-field coupling (`auth_type` → credential set, and the openapi-only + // `spec`/`api_key_header` fields) is expressed as an injected `allOf` of + // `if`/`then` subschemas rather than in this flat struct — see + // `mcp_server_credential_coupling`. That keeps the resource flat (no oneOf + // restructuring) while giving the published schema and every runtime + // validator one shared definition. /// OAuth client identifier used for the OAuth 2.0 client credentials /// grant. Required when `auth_type` is `oauth2`; ignored otherwise. #[serde(default, skip_serializing_if = "Option::is_none")] @@ -221,6 +221,62 @@ pub fn mcp_server_credential_coupling() -> Value { "token_url": { "type": "string", "minLength": 1 } } } + }, + // Note on diagnosability: expressing "not a Swagger 2.0 document" as a + // schema constraint costs the targeted hint the write path used to emit + // ("convert the spec to OpenAPI 3.x"). A validator reports the failing + // pointer (`/spec`) and the constraint, not advice. The rule is what + // matters most here — a Swagger 2.0 document is rejected on every path + // — but a post-schema semantic hook in the loaders would let both the + // rule and the advice live in one place. + // + // An OpenAPI-backed server carries the document; `api_key_header` names + // the header the generated tools send the key in, so it only makes sense + // alongside `auth_type: api_key`, and it has to be a legal header name + // (RFC 7230 `tchar` — the same set `http::HeaderName` accepts). + { + "if": { "properties": { "type": { "const": "openapi" } }, "required": ["type"] }, + "then": { + "required": ["spec"], + "properties": { + "spec": { + "type": "object", + "not": { "required": ["swagger"] } + }, + "api_key_header": { "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" } + }, + "dependencies": { + "api_key_header": { + "properties": { "auth_type": { "const": "api_key" } }, + "required": ["auth_type"] + } + } + } + }, + // A plain MCP server has neither. `spec`/`api_key_header` are + // `Option`-typed, so an explicit `null` is equivalent to absent — hence + // `type: null` rather than a `required` negation. `type` defaults to + // `mcp`, so the missing-key case has to be covered too. + { + "if": { + "anyOf": [ + { + "title": "type is mcp", + "properties": { "type": { "const": "mcp" } }, + "required": ["type"] + }, + { + "title": "type is absent (defaults to mcp)", + "not": { "required": ["type"] } + } + ] + }, + "then": { + "properties": { + "spec": { "type": "null" }, + "api_key_header": { "type": "null" } + } + } } ]) } @@ -238,12 +294,72 @@ mod tests { .expect_err("a name containing `__` must be rejected"); assert!(err.path.contains("name"), "unexpected path: {}", err.path); - // A single underscore is fine, including a trailing one. - for good in ["git_hub", "github_", "github"] { + // A single underscore inside or leading the name is fine. + for good in ["git_hub", "_github", "github", "a_b_c"] { let doc = json!({"name": good, "url": "https://x/mcp"}); crate::models::schema::validate_mcp_server(&doc) .unwrap_or_else(|e| panic!("{good} should be accepted: {e:?}")); } + + // A trailing `_` collides with the separator's first byte: `gh_` + `x` + // and `gh` + `_x` both serialize to `gh___x`, and `split_once("__")` + // resolves `gh_` to the non-existent server `gh`, so every tool call + // against it fails. + for bad in ["github_", "_", "a_b_"] { + let doc = json!({"name": bad, "url": "https://x/mcp"}); + crate::models::schema::validate_mcp_server(&doc) + .expect_err(&format!("{bad} ends in the separator's first byte")); + } + } + + #[test] + fn schema_enforces_openapi_field_coupling() { + let spec = json!({"openapi": "3.0.0"}); + + // `openapi` type requires a `spec`, and it must be an OpenAPI 3.x object. + for bad in [ + json!({"name": "s", "url": "https://x/mcp", "type": "openapi"}), + json!({"name": "s", "url": "https://x/mcp", "type": "openapi", "spec": "str"}), + json!({"name": "s", "url": "https://x/mcp", "type": "openapi", "spec": {"swagger": "2.0"}}), + ] { + crate::models::schema::validate_mcp_server(&bad) + .expect_err("an openapi server needs a 3.x spec object"); + } + + // `api_key_header` only makes sense with `auth_type: api_key`, and has to + // be a legal header name. This one is guarded by draft-07 `dependencies` + // — the draft 2019-09 spelling would be ignored, so keep this test. + let wrong_auth = json!({ + "name": "s", "url": "https://x/mcp", "type": "openapi", + "spec": spec, "api_key_header": "X-Key" + }); + crate::models::schema::validate_mcp_server(&wrong_auth) + .expect_err("api_key_header without auth_type api_key must be rejected"); + + let bad_header = json!({ + "name": "s", "url": "https://x/mcp", "type": "openapi", "spec": spec, + "auth_type": "api_key", "secret": "s", "api_key_header": "bad header" + }); + crate::models::schema::validate_mcp_server(&bad_header) + .expect_err("a header name with a space must be rejected"); + + // A plain mcp server carries neither field — `type` defaults to `mcp`, + // so the missing-key case is covered too. + for bad in [ + json!({"name": "s", "url": "https://x/mcp", "spec": {"openapi": "3.0.0"}}), + json!({"name": "s", "url": "https://x/mcp", "api_key_header": "X-Key"}), + ] { + crate::models::schema::validate_mcp_server(&bad) + .expect_err("spec/api_key_header are openapi-only"); + } + + // The complete openapi shape validates. + let ok = json!({ + "name": "s", "url": "https://x/mcp", "type": "openapi", + "spec": {"openapi": "3.0.0"}, "auth_type": "api_key", + "secret": "s", "api_key_header": "X-Key" + }); + crate::models::schema::validate_mcp_server(&ok).expect("complete openapi server"); } #[test] diff --git a/crates/aisix-core/src/models/schema.rs b/crates/aisix-core/src/models/schema.rs index 84287aa5..d476960a 100644 --- a/crates/aisix-core/src/models/schema.rs +++ b/crates/aisix-core/src/models/schema.rs @@ -284,8 +284,10 @@ fn accept_renamed_field(schema: &mut Value, canonical: &str, former: &str, note: /// `auth_type` closed sets come from the /// [`McpTransport`](crate::models::McpTransport) / /// [`McpAuthType`](crate::models::McpAuthType) enums. The per-`auth_type` -/// credential coupling is intentionally not encoded here (see the note on the -/// struct); the schema stays permissive and write paths enforce it. The label +/// credential coupling, and the openapi-only `spec`/`api_key_header` fields, are +/// injected here as an `allOf` of `if`/`then` subschemas (see +/// [`super::mcp_server::mcp_server_credential_coupling`]) so every configuration +/// path enforces them. The label /// is accepted under both its canonical name `name` and its former name /// `display_name` (see [`accept_renamed_field`]). pub fn mcp_server_root_schema() -> Value { diff --git a/schemas/resources/a2a_agent.schema.json b/schemas/resources/a2a_agent.schema.json index b6d72931..2437b736 100644 --- a/schemas/resources/a2a_agent.schema.json +++ b/schemas/resources/a2a_agent.schema.json @@ -128,7 +128,7 @@ "display_name": { "description": "Accepted as an alternative spelling of `name`. Provide the label under exactly one of the two names.", "minLength": 1, - "pattern": "^[^/]+$", + "pattern": "^[^/?#%\\s\\x00-\\x1f]+$", "type": "string" }, "enabled": { @@ -137,9 +137,9 @@ "type": "boolean" }, "name": { - "description": "Operator-facing label, unique within the gateway. It is the path segment under which the agent is exposed to callers as `/a2a/`, so it must be a single non-empty URL path segment.", + "description": "Operator-facing label, unique within the gateway. It is the path segment under which the agent is exposed to callers as `/a2a/`, so it must be a single non-empty URL path segment. The name is interpolated into the advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` and whitespace are rejected: `a?b` would advertise a URL whose path is just `/a2a/a`, and the lookup is an exact match on the stored name.", "minLength": 1, - "pattern": "^[^/]+$", + "pattern": "^[^/?#%\\s\\x00-\\x1f]+$", "type": "string" }, "protocol_version": { diff --git a/schemas/resources/mcp_server.schema.json b/schemas/resources/mcp_server.schema.json index 382eaf4d..6d27eada 100644 --- a/schemas/resources/mcp_server.schema.json +++ b/schemas/resources/mcp_server.schema.json @@ -80,6 +80,83 @@ "token_url" ] } + }, + { + "if": { + "properties": { + "type": { + "const": "openapi" + } + }, + "required": [ + "type" + ] + }, + "then": { + "dependencies": { + "api_key_header": { + "properties": { + "auth_type": { + "const": "api_key" + } + }, + "required": [ + "auth_type" + ] + } + }, + "properties": { + "api_key_header": { + "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" + }, + "spec": { + "not": { + "required": [ + "swagger" + ] + }, + "type": "object" + } + }, + "required": [ + "spec" + ] + } + }, + { + "if": { + "anyOf": [ + { + "properties": { + "type": { + "const": "mcp" + } + }, + "required": [ + "type" + ], + "title": "type is mcp" + }, + { + "not": { + "required": [ + "type" + ] + }, + "title": "type is absent (defaults to mcp)" + } + ] + }, + "then": { + "properties": { + "api_key_header": { + "type": "null" + }, + "spec": { + "type": "null" + } + } + } } ], "anyOf": [ @@ -197,7 +274,7 @@ "display_name": { "description": "Accepted as an alternative spelling of `name`. Provide the label under exactly one of the two names.", "minLength": 1, - "pattern": "^(?:[^_]|_[^_])*_?$", + "pattern": "^(?:[^_]|_[^_])*$", "type": "string" }, "enabled": { @@ -208,7 +285,7 @@ "name": { "description": "Operator-facing label, unique within the gateway. It is used as the namespace prefix for this server's tools, which are exposed to clients as `__`, so it must not contain the reserved separator `__`.", "minLength": 1, - "pattern": "^(?:[^_]|_[^_])*_?$", + "pattern": "^(?:[^_]|_[^_])*$", "type": "string" }, "scopes": { From 9d79dbc9f7e7d975e7afc89491fcde628bd0d197 Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Thu, 30 Jul 2026 18:00:57 +0800 Subject: [PATCH 3/5] fix: block incomplete exports, accept a null api_key_header, pin each rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from a second review pass focused on lifecycle, export round-trip and mutation coverage. `aisix export` omitted rows the loader rejected, printed a warning, and exited 0. Export is the migration path off a store-backed deployment, so a scripted migration would treat an incomplete file as a finished one and lose those resources silently. A rejected row now fails the export the same way a non-loadable file does; the written file stays for inspection. `api_key_header: null` was rejected. The field is `Option` and an explicit null means absent — the comment right above the constraint said so — but `dependencies` keys off presence alone, null included. It is now a value-sensitive `if`/`then` on `type: string`, verified both ways: null loads, a real header with non-api_key auth still fails. The suite pinned only 5 of the 12 lifted rules at single-constraint granularity. `spec` must-be-an-object was pinned by nothing — deleting it left the string fixture rejected by the sibling swagger check, so the test passed either way. Added mutation-pinning tests that start from a fully valid document and violate exactly one obligation: each oauth2 field independently, empty and null for every credential, an explicit `type: mcp` carrying openapi-only fields, a header with `auth_type: none`, and every excluded a2a character under both the `name` and `display_name` spellings. One test fixture called `{"openapi": "3.0.0"}" a complete spec, but tool generation needs a `paths` object — it now uses a spec that generates a tool. --- crates/aisix-core/src/models/a2a_agent.rs | 27 ++++++ crates/aisix-core/src/models/mcp_server.rs | 95 ++++++++++++++++++++-- crates/aisix-server/src/export/mod.rs | 15 +++- schemas/resources/mcp_server.schema.json | 32 +++++--- 4 files changed, 151 insertions(+), 18 deletions(-) diff --git a/crates/aisix-core/src/models/a2a_agent.rs b/crates/aisix-core/src/models/a2a_agent.rs index 42c97e2e..487c3263 100644 --- a/crates/aisix-core/src/models/a2a_agent.rs +++ b/crates/aisix-core/src/models/a2a_agent.rs @@ -164,6 +164,33 @@ pub fn a2a_agent_credential_coupling() -> Value { mod tests { use super::*; + #[test] + fn schema_pins_each_a2a_obligation_individually() { + // Every excluded character is pinned, under both accepted spellings, so + // narrowing the pattern back to `^[^/]+$` fails here. + for bad in ["a/b", "a?b", "a#b", "a%2Fb", "a b", "a\tb", "a\nb"] { + for key in ["name", "display_name"] { + let doc = json!({key: bad, "url": "https://x/a2a"}); + crate::models::schema::validate_a2a_agent(&doc) + .expect_err(&format!("{key}={bad:?} must be rejected")); + } + } + // A plain name is still fine under both spellings. + for key in ["name", "display_name"] { + let doc = json!({key: "invoice-processor", "url": "https://x/a2a"}); + crate::models::schema::validate_a2a_agent(&doc).expect("plain name is valid"); + } + // Credential obligations: missing, empty and null all fail. + for bad in [None, Some(json!("")), Some(json!(null))] { + let mut doc = json!({"name": "a", "url": "https://x/a2a", "auth_type": "bearer"}); + if let Some(v) = bad { + doc["secret"] = v; + } + crate::models::schema::validate_a2a_agent(&doc) + .expect_err("bearer needs a non-empty string secret"); + } + } + #[test] fn schema_rejects_slash_in_name() { // The name is the `/a2a/` path segment, so a slash would split diff --git a/crates/aisix-core/src/models/mcp_server.rs b/crates/aisix-core/src/models/mcp_server.rs index 2b9982ef..844096e0 100644 --- a/crates/aisix-core/src/models/mcp_server.rs +++ b/crates/aisix-core/src/models/mcp_server.rs @@ -245,12 +245,22 @@ pub fn mcp_server_credential_coupling() -> Value { }, "api_key_header": { "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" } }, - "dependencies": { - "api_key_header": { - "properties": { "auth_type": { "const": "api_key" } }, - "required": ["auth_type"] + // Value-sensitive, not presence-based: `api_key_header` is an + // `Option`, so an explicit `null` means "absent" and must + // not drag in the `auth_type` requirement. `dependencies` keys off + // presence alone and would reject `api_key_header: null`. + "allOf": [ + { + "if": { + "properties": { "api_key_header": { "type": "string" } }, + "required": ["api_key_header"] + }, + "then": { + "properties": { "auth_type": { "const": "api_key" } }, + "required": ["auth_type"] + } } - } + ] } }, // A plain MCP server has neither. `spec`/`api_key_header` are @@ -285,6 +295,79 @@ pub fn mcp_server_credential_coupling() -> Value { mod tests { use super::*; + #[test] + fn schema_pins_each_mcp_obligation_individually() { + // Baseline: valid openapi-backed server with api-key auth. + let base = json!({ + "name": "erp", + "url": "https://erp.internal/api", + "type": "openapi", + "spec": {"openapi": "3.0.0", "paths": {"/a": {"get": {"operationId": "list_a"}}}}, + "auth_type": "api_key", + "secret": "k", + "api_key_header": "X-Key" + }); + crate::models::schema::validate_mcp_server(&base).expect("baseline must be valid"); + + // `spec` must be an object — pinned independently of the swagger check, + // so a non-object that is NOT a swagger document still fails. + let mut v = base.clone(); + v["spec"] = json!("just a string"); + crate::models::schema::validate_mcp_server(&v) + .expect_err("a non-object spec must be rejected on its own"); + let mut v = base.clone(); + v["spec"] = json!([{"openapi": "3.0.0"}]); + crate::models::schema::validate_mcp_server(&v) + .expect_err("an array spec must be rejected on its own"); + + // oauth2: each of the three obligations pinned separately. + for omit in ["secret", "client_id", "token_url"] { + let mut v = json!({ + "name": "erp", "url": "https://x/mcp", "auth_type": "oauth2", + "secret": "cs", "client_id": "cid", "token_url": "https://auth/token" + }); + v.as_object_mut().unwrap().remove(omit); + crate::models::schema::validate_mcp_server(&v) + .expect_err(&format!("oauth2 missing only {omit} must be rejected")); + } + + // Credential fields: empty and null are as bad as missing. + for bad in [json!(""), json!(null)] { + let mut v = json!({"name": "s", "url": "https://x/mcp", "auth_type": "bearer"}); + v["secret"] = bad.clone(); + crate::models::schema::validate_mcp_server(&v) + .expect_err("bearer with an empty or null secret must be rejected"); + } + + // openapi-only fields rejected on an EXPLICIT type: mcp, not just a + // defaulted one. + for field in ["spec", "api_key_header"] { + let mut v = json!({"name": "gh", "url": "https://x/mcp", "type": "mcp"}); + v[field] = if field == "spec" { + json!({"openapi": "3.0.0"}) + } else { + json!("X-Key") + }; + crate::models::schema::validate_mcp_server(&v).expect_err(&format!( + "{field} on an explicit type: mcp must be rejected" + )); + } + + // api_key_header requires api_key auth specifically — `none` is not enough. + let mut v = base.clone(); + v["auth_type"] = json!("none"); + v.as_object_mut().unwrap().remove("secret"); + crate::models::schema::validate_mcp_server(&v) + .expect_err("a header with auth_type none must be rejected"); + + // …but an explicit null header is "absent" and must stay valid. + let mut v = base.clone(); + v["api_key_header"] = json!(null); + v["auth_type"] = json!("none"); + v.as_object_mut().unwrap().remove("secret"); + crate::models::schema::validate_mcp_server(&v).expect("api_key_header: null means absent"); + } + #[test] fn schema_rejects_tool_namespace_separator_in_name() { // Tools are exposed as `__`, so `__` inside the name makes @@ -314,7 +397,7 @@ mod tests { #[test] fn schema_enforces_openapi_field_coupling() { - let spec = json!({"openapi": "3.0.0"}); + let spec = json!({"openapi": "3.0.0", "paths": {"/a": {"get": {"operationId": "list_a"}}}}); // `openapi` type requires a `spec`, and it must be an OpenAPI 3.x object. for bad in [ diff --git a/crates/aisix-server/src/export/mod.rs b/crates/aisix-server/src/export/mod.rs index beeefce7..d2d4040f 100644 --- a/crates/aisix-server/src/export/mod.rs +++ b/crates/aisix-server/src/export/mod.rs @@ -100,8 +100,19 @@ pub async fn run(args: ExportArgs) -> anyhow::Result<()> { report_secrets(&document, args.reveal_secrets); // The file is written for inspection either way, but a scripted - // migration must not mistake a non-loadable export for a finished one: - // exit non-zero when a collision or dangling reference means the file + // migration must not mistake an incomplete export for a finished one. + // A row the loader rejected is silently absent from the output: the file + // loads, so `blocking` is empty, yet the migration lost a resource. That + // is a failed export, not a warning — exit non-zero for it too. + if !stats.rejections.is_empty() { + anyhow::bail!( + "export omitted {} rejected entr(ies); the resources file is incomplete. \ + Fix them in the source and re-export — the written file is for inspection only", + stats.rejections.len() + ); + } + + // Exit non-zero when a collision or dangling reference means the file // cannot be loaded back as-is. if !document.blocking.is_empty() { eprintln!( diff --git a/schemas/resources/mcp_server.schema.json b/schemas/resources/mcp_server.schema.json index 6d27eada..09996c18 100644 --- a/schemas/resources/mcp_server.schema.json +++ b/schemas/resources/mcp_server.schema.json @@ -93,18 +93,30 @@ ] }, "then": { - "dependencies": { - "api_key_header": { - "properties": { - "auth_type": { - "const": "api_key" - } + "allOf": [ + { + "if": { + "properties": { + "api_key_header": { + "type": "string" + } + }, + "required": [ + "api_key_header" + ] }, - "required": [ - "auth_type" - ] + "then": { + "properties": { + "auth_type": { + "const": "api_key" + } + }, + "required": [ + "auth_type" + ] + } } - }, + ], "properties": { "api_key_header": { "pattern": "^[!#$%&'*+.^_`|~0-9A-Za-z-]+$" From 3ab61453d400dee1811eb0bf31d8ddf8ee0d3441 Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Mon, 3 Aug 2026 12:55:25 +0800 Subject: [PATCH 4/5] fix: report every export failure class, exclude DEL from a2a names Review comments on the previous commit: - With both rejected rows and blocking issues present, the rejection bail exited before the collision/dangling-reference details printed, so an operator fixing an export saw only half the list. Both classes are reported before the first non-zero exit. - The a2a name pattern excluded C0 controls but not DEL (0x7f), which is also a control character; now excluded and pinned in the test. --- crates/aisix-core/src/models/a2a_agent.rs | 6 +++-- crates/aisix-server/src/export/mod.rs | 28 +++++++++++------------ schemas/resources/a2a_agent.schema.json | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/crates/aisix-core/src/models/a2a_agent.rs b/crates/aisix-core/src/models/a2a_agent.rs index 487c3263..70f7b45e 100644 --- a/crates/aisix-core/src/models/a2a_agent.rs +++ b/crates/aisix-core/src/models/a2a_agent.rs @@ -35,7 +35,7 @@ pub struct A2aAgent { // lives in `schema::a2a_agent_root_schema`). Re-serialization always // emits `name`. #[serde(alias = "display_name")] - #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f]+$"), length(min = 1))] + #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f\\x7f]+$"), length(min = 1))] pub name: String, /// The upstream agent's base URL, such as `https://agents.example.com/a2a`. @@ -168,7 +168,9 @@ mod tests { fn schema_pins_each_a2a_obligation_individually() { // Every excluded character is pinned, under both accepted spellings, so // narrowing the pattern back to `^[^/]+$` fails here. - for bad in ["a/b", "a?b", "a#b", "a%2Fb", "a b", "a\tb", "a\nb"] { + for bad in [ + "a/b", "a?b", "a#b", "a%2Fb", "a b", "a\tb", "a\nb", "a\x7fb", + ] { for key in ["name", "display_name"] { let doc = json!({key: bad, "url": "https://x/a2a"}); crate::models::schema::validate_a2a_agent(&doc) diff --git a/crates/aisix-server/src/export/mod.rs b/crates/aisix-server/src/export/mod.rs index d2d4040f..7a74eee9 100644 --- a/crates/aisix-server/src/export/mod.rs +++ b/crates/aisix-server/src/export/mod.rs @@ -100,20 +100,12 @@ pub async fn run(args: ExportArgs) -> anyhow::Result<()> { report_secrets(&document, args.reveal_secrets); // The file is written for inspection either way, but a scripted - // migration must not mistake an incomplete export for a finished one. - // A row the loader rejected is silently absent from the output: the file - // loads, so `blocking` is empty, yet the migration lost a resource. That - // is a failed export, not a warning — exit non-zero for it too. - if !stats.rejections.is_empty() { - anyhow::bail!( - "export omitted {} rejected entr(ies); the resources file is incomplete. \ - Fix them in the source and re-export — the written file is for inspection only", - stats.rejections.len() - ); - } - - // Exit non-zero when a collision or dangling reference means the file - // cannot be loaded back as-is. + // migration must not mistake an incomplete or non-loadable export for a + // finished one. Report every failure class before exiting, so an operator + // sees the full list in one run: a row the loader rejected is silently + // absent from the output (the file loads, yet the migration lost a + // resource), and a collision or dangling reference means the file cannot + // be loaded back as-is. if !document.blocking.is_empty() { eprintln!( "\n{} issue(s) leave the exported file non-loadable — fix them in the source and \ @@ -129,6 +121,14 @@ pub async fn run(args: ExportArgs) -> anyhow::Result<()> { ); } + if !stats.rejections.is_empty() { + anyhow::bail!( + "export omitted {} rejected entr(ies); the resources file is incomplete. \ + Fix them in the source and re-export — the written file is for inspection only", + stats.rejections.len() + ); + } + Ok(()) } diff --git a/schemas/resources/a2a_agent.schema.json b/schemas/resources/a2a_agent.schema.json index 2437b736..3d6de4f1 100644 --- a/schemas/resources/a2a_agent.schema.json +++ b/schemas/resources/a2a_agent.schema.json @@ -128,7 +128,7 @@ "display_name": { "description": "Accepted as an alternative spelling of `name`. Provide the label under exactly one of the two names.", "minLength": 1, - "pattern": "^[^/?#%\\s\\x00-\\x1f]+$", + "pattern": "^[^/?#%\\s\\x00-\\x1f\\x7f]+$", "type": "string" }, "enabled": { @@ -139,7 +139,7 @@ "name": { "description": "Operator-facing label, unique within the gateway. It is the path segment under which the agent is exposed to callers as `/a2a/`, so it must be a single non-empty URL path segment. The name is interpolated into the advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` and whitespace are rejected: `a?b` would advertise a URL whose path is just `/a2a/a`, and the lookup is an exact match on the stored name.", "minLength": 1, - "pattern": "^[^/?#%\\s\\x00-\\x1f]+$", + "pattern": "^[^/?#%\\s\\x00-\\x1f\\x7f]+$", "type": "string" }, "protocol_version": { From 40e21b94f2ad1260bd10fdd5fddbdd29ff7e684f Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Mon, 3 Aug 2026 13:23:40 +0800 Subject: [PATCH 5/5] fix(core): exclude C1 controls from a2a agent names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: the pattern excluded C0 controls and DEL but still accepted U+0080–U+009F. The C1 range is added as literal characters in the class (via Rust \u escapes), so the same pattern stays valid for the runtime regex engine and for the published ECMA-flavor JSON Schema — no engine-specific hex-brace syntax. Unicode names such as CJK remain accepted; pinned both ways in the mutation test and verified against a built `aisix validate`. --- crates/aisix-core/src/models/a2a_agent.rs | 7 +++++-- schemas/resources/a2a_agent.schema.json | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/aisix-core/src/models/a2a_agent.rs b/crates/aisix-core/src/models/a2a_agent.rs index 70f7b45e..7edf6f4a 100644 --- a/crates/aisix-core/src/models/a2a_agent.rs +++ b/crates/aisix-core/src/models/a2a_agent.rs @@ -35,7 +35,10 @@ pub struct A2aAgent { // lives in `schema::a2a_agent_root_schema`). Re-serialization always // emits `name`. #[serde(alias = "display_name")] - #[schemars(regex(pattern = "^[^/?#%\\s\\x00-\\x1f\\x7f]+$"), length(min = 1))] + #[schemars( + regex(pattern = "^[^/?#%\\s\\x00-\\x1f\\x7f\u{0080}-\u{009f}]+$"), + length(min = 1) + )] pub name: String, /// The upstream agent's base URL, such as `https://agents.example.com/a2a`. @@ -169,7 +172,7 @@ mod tests { // Every excluded character is pinned, under both accepted spellings, so // narrowing the pattern back to `^[^/]+$` fails here. for bad in [ - "a/b", "a?b", "a#b", "a%2Fb", "a b", "a\tb", "a\nb", "a\x7fb", + "a/b", "a?b", "a#b", "a%2Fb", "a b", "a\tb", "a\nb", "a\x7fb", "a\u{80}b", "a\u{9f}b", ] { for key in ["name", "display_name"] { let doc = json!({key: bad, "url": "https://x/a2a"}); diff --git a/schemas/resources/a2a_agent.schema.json b/schemas/resources/a2a_agent.schema.json index 3d6de4f1..d7c95a8d 100644 --- a/schemas/resources/a2a_agent.schema.json +++ b/schemas/resources/a2a_agent.schema.json @@ -128,7 +128,7 @@ "display_name": { "description": "Accepted as an alternative spelling of `name`. Provide the label under exactly one of the two names.", "minLength": 1, - "pattern": "^[^/?#%\\s\\x00-\\x1f\\x7f]+$", + "pattern": "^[^/?#%\\s\\x00-\\x1f\\x7f€-Ÿ]+$", "type": "string" }, "enabled": { @@ -139,7 +139,7 @@ "name": { "description": "Operator-facing label, unique within the gateway. It is the path segment under which the agent is exposed to callers as `/a2a/`, so it must be a single non-empty URL path segment. The name is interpolated into the advertised agent-card URL without percent-encoding, so `/`, `?`, `#`, `%` and whitespace are rejected: `a?b` would advertise a URL whose path is just `/a2a/a`, and the lookup is an exact match on the stored name.", "minLength": 1, - "pattern": "^[^/?#%\\s\\x00-\\x1f\\x7f]+$", + "pattern": "^[^/?#%\\s\\x00-\\x1f\\x7f€-Ÿ]+$", "type": "string" }, "protocol_version": {