Copilot commented on code in PR #13957: URL: https://github.com/apache/apisix/pull/13957#discussion_r4509239843
########## apisix/plugins/openapi-to-mcp.lua: ########## @@ -178,6 +181,30 @@ local function resolve_conf(conf, ctx) end +-- base_url may be built from request variables, so the host it resolves to is +-- only known per request. +local function base_url_allowed(conf, base_url) + if not conf.allowed_base_url_hosts then + return true + end + + -- neither the URL nor the host is logged: after resolution they may carry + -- values taken from the request + local origin, host, port = ref.origin_of(base_url) Review Comment: The allow-list checks only `base_url`, but the handler directly appends document path keys that `endpoints.extract()` accepts without a leading slash. An untrusted document path `.attacker.example/` turns an allowed base `http://api.example.com` into `http://api.example.com.attacker.example/`, sending the route's configured headers to a different host. Reject non-path keys and validate the completed outbound URL against the allow-list before fetching it. ########## apisix/plugins/openapi-to-mcp/tools/generator.lua: ########## @@ -179,6 +179,87 @@ local function extract_custom_annotations(operation, op_id) end +-- Which response the output schema is taken from: the success answer, most +-- specific first. `default` is not a success response and is not considered. +local function success_response(responses) + -- appended one by one: a nil in the middle of a table constructor makes + -- ipairs stop at it, and an operation that declares only a 201 would then + -- be read as declaring nothing + local ordered = {} + if responses["200"] ~= nil then + ordered[#ordered + 1] = responses["200"] + end + if responses["201"] ~= nil then + ordered[#ordered + 1] = responses["201"] + end + + local others = {} + for code in pairs(responses) do + if type(code) == "string" and #code == 3 and str_sub(code, 1, 1) == "2" + and code ~= "200" and code ~= "201" and code ~= "2XX" then + others[#others + 1] = code + end + end + table_sort(others) + for _, code in ipairs(others) do + ordered[#ordered + 1] = responses[code] + end + + if responses["2XX"] ~= nil then + ordered[#ordered + 1] = responses["2XX"] + end + return ordered +end + + +-- Validate the body the way the client will: the MCP SDKs check +-- `structuredContent` against the advertised schema with a validator that does +-- not assert `format`, so asserting it here would reject bodies the client +-- would have accepted. The keyword is a string; a table under that key is a +-- property named "format" and is kept. +local function without_formats(schema) + if type(schema) ~= "table" then + return schema + end + local out = {} + for key, value in pairs(schema) do + if key ~= "format" or type(value) ~= "string" then + out[key] = without_formats(value) + end + end + return out +end Review Comment: This traversal also removes `format` members from literal values, not just schema keywords. For example, a property's `enum = { { format = "json" } }` becomes `enum = { {} }`, so a response containing the documented `{ "format": "json" }` value passes the advertised schema but fails `output_check_schema` and is returned as an error. Traverse schema positions only, keeping literal values under `enum`, `const`, `default`, and examples unchanged. ########## apisix/plugins/openapi-to-mcp/tools/generator.lua: ########## @@ -179,6 +179,87 @@ local function extract_custom_annotations(operation, op_id) end +-- Which response the output schema is taken from: the success answer, most +-- specific first. `default` is not a success response and is not considered. +local function success_response(responses) + -- appended one by one: a nil in the middle of a table constructor makes + -- ipairs stop at it, and an operation that declares only a 201 would then + -- be read as declaring nothing + local ordered = {} + if responses["200"] ~= nil then + ordered[#ordered + 1] = responses["200"] + end + if responses["201"] ~= nil then + ordered[#ordered + 1] = responses["201"] + end + + local others = {} + for code in pairs(responses) do + if type(code) == "string" and #code == 3 and str_sub(code, 1, 1) == "2" + and code ~= "200" and code ~= "201" and code ~= "2XX" then + others[#others + 1] = code + end + end + table_sort(others) + for _, code in ipairs(others) do + ordered[#ordered + 1] = responses[code] + end + + if responses["2XX"] ~= nil then + ordered[#ordered + 1] = responses["2XX"] + end + return ordered +end + + +-- Validate the body the way the client will: the MCP SDKs check +-- `structuredContent` against the advertised schema with a validator that does +-- not assert `format`, so asserting it here would reject bodies the client +-- would have accepted. The keyword is a string; a table under that key is a +-- property named "format" and is kept. +local function without_formats(schema) + if type(schema) ~= "table" then + return schema + end + local out = {} + for key, value in pairs(schema) do + if key ~= "format" or type(value) ~= "string" then + out[key] = without_formats(value) + end + end + return out +end + + +-- The JSON object a successful call returns, when the document describes one. +-- Only a plain object with properties qualifies: an array, a composition the +-- document leaves unmerged (`allOf`, `oneOf`) and a bare `type: object` carry +-- nothing a client could bind to, and advertising them would only promise +-- structure that is not there. +local function build_output_schema(operation) + local responses = operation.responses + if type(responses) ~= "table" then + return nil + end + + for _, response in ipairs(success_response(responses)) do + if type(response) == "table" and type(response.content) == "table" then + local json_content = response.content["application/json"] + if type(json_content) == "table" and type(json_content.schema) == "table" then + local schema = oas_schema.to_json_schema(json_content.schema) Review Comment: The shared converter strips `writeOnly` but leaves the property's name in `required`, although OpenAPI requires write-only properties only in requests. A shared model with a required write-only password therefore advertises a password requirement in `outputSchema` and turns a valid response that omits it into `isError`. Add response-aware conversion that recursively removes write-only properties from response requirements before advertising and validating the output, without changing input-schema behavior. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
