nic-6443 commented on code in PR #14154: URL: https://github.com/apache/apisix/pull/14154#discussion_r4477181632
########## CHANGELOG.md: ########## @@ -93,6 +93,7 @@ title: Changelog ### Change +- :warning: fix(openapi-to-mcp): reject path parameter values that can change the operation's path. Encoded `/` and `\` separators are rejected by default; set `allow_encoded_path_separators: true` only for an upstream that intentionally treats them as data. Finished empty and dot path segments remain rejected, and the opt-in still rejects separator-delimited empty and dot components [#14154](https://github.com/apache/apisix/pull/14154) Review Comment: This puts the entry under `## 3.19.0`, but the 3.19.0 tag doesn't have this fix, so a reader would think 3.19.0 already rejects these values and has `allow_encoded_path_separators`. The last few updates to this file all came from `chore: release` PRs, so I'd drop the line here and leave it to the next one. The PR description needs a refresh too: it still says "Two checks", doesn't mention the new option, and has the backward-compatible box ticked while this line marks the change `:warning:`. ########## apisix/plugins/openapi-to-mcp/tools/handler.lua: ########## @@ -124,19 +125,114 @@ local function apply_query_defaults(tool, query) end -local function build_path(template, path_params) +-- A separator in a path parameter is percent-encoded, but servers disagree on +-- whether to preserve it as data or decode it into path syntax. The secure +-- default is to reject it; a Route may opt in for an API that deliberately +-- accepts encoded separators. +local function has_separator(value) + -- core.string.find is a plain search; this is a pattern, so it needs + -- string.find itself. A backslash is a separator on some servers. + return str_find(value, "[/\\]") ~= nil +end + + +-- Even with encoded separators enabled, refuse values whose components would +-- be resolved away after a server decodes them. Empty components cover leading, +-- trailing and repeated separators. Both slash spellings are considered: a +-- backslash is a separator on some servers. +local function resolved_away(segment) + return segment == "" or segment == "." or segment == ".." +end + + +local function separator_creates_unsafe_component(value) + if not has_separator(value) then + return false + end + + for component in str_gmatch(value .. "/", "([^/\\]*)[/\\]") do + if resolved_away(component) then + return true + end + end + return false +end + + +-- Return only the URI path. A template expression name may itself contain a +-- question mark or hash, so only a delimiter outside braces starts the query +-- or fragment. Parameter values cannot introduce one: escape_uri encodes it. +local function uri_path(value) + local in_expression = false + for index = 1, #value do + local char = str_sub(value, index, index) + if char == "{" then + in_expression = true + elseif char == "}" then + in_expression = false + elseif not in_expression and (char == "?" or char == "#") then + return str_sub(value, 1, index - 1) + end + end + return value +end + + +-- The segments of a path, with the empty one a leading or trailing slash +-- leaves: "/items/x" is { "", "items", "x" }. +local function path_segments(value) + local segments = {} + local path = uri_path(value) + for segment in str_gmatch(path .. "/", "([^/]*)/") do + segments[#segments + 1] = segment + end + return segments +end + + +-- A server receiving the request resolves "." and ".." away, but only where +-- one is a whole segment. A template expression may fill part of a segment, +-- and "{name}.json" filled with ".." comes out as "...json", which addresses +-- nothing else, so the value alone does not say whether it moves the request. +-- The finished path does. +-- +-- Only the segments an expression helped build are checked: one the template +-- contributes by itself is not the caller's to shift. Encoded separators do +-- not add a literal slash to the finished path, so the segment lists still +-- line up by index when the Route opts in to them. +local function build_path(template, path_params, allow_encoded_separators) local path = template for name, value in pairs(path_params) do + local text = tostring(value) + if has_separator(text) and not allow_encoded_separators then Review Comment: These two checks also run on expressions after the `?`, which is a regression now that query-suffixed templates are supported. `/search?q={term}` with `term=a/b` is rejected here, though `a%2Fb` is harmless in a query and went out fine before. The opt-in doesn't help either: `/search?url={u}` with `http://x/y` fails on the empty component from `//`. I'd run them only for expressions in the path part, e.g. guard both with `str_find(uri_path(template), "{" .. name .. "}", 1, true)`. ########## apisix/plugins/openapi-to-mcp/tools/handler.lua: ########## @@ -124,19 +125,114 @@ local function apply_query_defaults(tool, query) end -local function build_path(template, path_params) +-- A separator in a path parameter is percent-encoded, but servers disagree on +-- whether to preserve it as data or decode it into path syntax. The secure +-- default is to reject it; a Route may opt in for an API that deliberately +-- accepts encoded separators. +local function has_separator(value) + -- core.string.find is a plain search; this is a pattern, so it needs + -- string.find itself. A backslash is a separator on some servers. + return str_find(value, "[/\\]") ~= nil +end + + +-- Even with encoded separators enabled, refuse values whose components would +-- be resolved away after a server decodes them. Empty components cover leading, +-- trailing and repeated separators. Both slash spellings are considered: a +-- backslash is a separator on some servers. +local function resolved_away(segment) + return segment == "" or segment == "." or segment == ".." +end + + +local function separator_creates_unsafe_component(value) + if not has_separator(value) then + return false + end + + for component in str_gmatch(value .. "/", "([^/\\]*)[/\\]") do + if resolved_away(component) then + return true + end + end + return false +end + + +-- Return only the URI path. A template expression name may itself contain a +-- question mark or hash, so only a delimiter outside braces starts the query +-- or fragment. Parameter values cannot introduce one: escape_uri encodes it. +local function uri_path(value) + local in_expression = false + for index = 1, #value do + local char = str_sub(value, index, index) + if char == "{" then + in_expression = true + elseif char == "}" then + in_expression = false + elseif not in_expression and (char == "?" or char == "#") then Review Comment: My suggestion last round was incomplete here: cutting at `#` for the check assumes the fragment doesn't go out, but it does. `fetch.lua` goes through `httpc:parse_uri`, whose regex only splits on `?`, so a `#` stays in the path on the wire. `/x#/{a}` with `a=..` isn't rejected and sends `/x#/..`, and `/{Bucket}/{Key}#acl` sends a literal `#acl`. Whether the receiving server stops at the `#` is up to that server. A fragment isn't meant to go on the wire at all, so I'd drop it from the finished path before `build_path` returns. Values are encoded, so a literal `#` there can only come from the template. This check is right as it stands once that's done. ########## t/plugin/openapi-to-mcp.t: ########## @@ -1573,3 +1576,414 @@ streamable_http 5: true streamable_http 6: true streamable_http 7: true streamable_http 8: true + + + +=== TEST 86: a route over a document with a string path parameter +--- config + location /t { + content_by_lua_block { + local ok = require("lib.openapi_to_mcp_fixture").put_routes({ + { 1, "/mcp", { + transport = "streamable_http", + base_url = "http://127.0.0.1:11460";, + openapi_url = "http://127.0.0.1:11460/defaults.json";, + } }, + }) + if ok then ngx.say("passed") end + } + } +--- response_body +passed + + + +=== TEST 87: a path parameter fills the segment it was declared for +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getItem","arguments":{"pathParameters":{"id":"abc"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*items\/abc/ + + + +=== TEST 88: a dot segment in a path parameter fails the call +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getItem","arguments":{"pathParameters":{"id":".."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter in segment \{id\} cannot produce an empty or dot path segment)/ + + + +=== TEST 89: a single dot in a path parameter fails the call +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getItem","arguments":{"pathParameters":{"id":"."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter in segment \{id\} cannot produce an empty or dot path segment)/ + + + +=== TEST 90: a separator in a path parameter fails the call +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getItem","arguments":{"pathParameters":{"id":"../items"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter id cannot contain a path separator)/ + + + +=== TEST 91: a backslash in a path parameter fails the call +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getItem","arguments":{"pathParameters":{"id":"..\\\\items"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter id cannot contain a path separator)/ + + + +=== TEST 92: an empty path parameter fails the call +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getItem","arguments":{"pathParameters":{"id":""}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter in segment \{id\} cannot produce an empty or dot path segment)/ + + + +=== TEST 93: a route over a document whose parameter fills part of a segment +--- config + location /t { + content_by_lua_block { + local ok = require("lib.openapi_to_mcp_fixture").put_routes({ + { 1, "/mcp", { + transport = "streamable_http", + base_url = "http://127.0.0.1:11460";, + openapi_url = "http://127.0.0.1:11460/partialsegment.json";, + } }, + }) + if ok then ngx.say("passed") end + } + } +--- response_body +passed + + + +=== TEST 94: a parameter that fills part of a segment is sent as given +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getFile","arguments":{"pathParameters":{"name":"report"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*files\/report\.json/ + + + +=== TEST 95: a dot-dot value that is not a whole segment is allowed through +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getFile","arguments":{"pathParameters":{"name":".."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*files\/\.\.\.json/ + + + +=== TEST 96: a single dot that is not a whole segment is allowed through +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getFile","arguments":{"pathParameters":{"name":"."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*files\/\.\.json/ + + + +=== TEST 97: an empty value that leaves a non-empty segment is allowed through +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getFile","arguments":{"pathParameters":{"name":""}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*files\/\.json/ + + + +=== TEST 98: a separator is still rejected when the parameter fills part of a segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getFile","arguments":{"pathParameters":{"name":"../secret"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter name cannot contain a path separator)/ + + + +=== TEST 99: a route over path templates with several expressions and URI suffixes +--- config + location /t { + content_by_lua_block { + local ok = require("lib.openapi_to_mcp_fixture").put_routes({ + { 1, "/mcp", { + transport = "streamable_http", + base_url = "http://127.0.0.1:11460";, + openapi_url = "http://127.0.0.1:11460/path-safety.json";, + } }, + }) + if ok then ngx.say("passed") end + } + } +--- response_body +passed + + + +=== TEST 100: adjacent expressions are judged as one finished segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"adjacentPathParameters","arguments":{"pathParameters":{"a":".","b":"."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 101: literals between expressions take part in the finished segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"separatedPathParameters","arguments":{"pathParameters":{"a":"","b":""}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 102: a literal prefix can complete a dot segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"prefixedPathParameter","arguments":{"pathParameters":{"a":"."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 103: a query suffix does not hide a dot path segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getBucketAcl","arguments":{"pathParameters":{"Bucket":".."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 104: a query suffix does not hide an empty path segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getBucketAcl","arguments":{"pathParameters":{"Bucket":""}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 105: a fragment suffix does not hide a dot path segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getObjectAcl","arguments":{"pathParameters":{"Bucket":"b","Key":".."}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 106: a safe query-suffixed path is still sent unchanged +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getBucketAcl","arguments":{"pathParameters":{"Bucket":"reports"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*reports\?acl/ + + + +=== TEST 107: an ordinary separator is rejected by default and logged +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getEncodedPath","arguments":{"pathParameters":{"path":"group/project"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*path parameter path cannot contain a path separator)/ +--- error_log +rejected an openapi-to-mcp tool call: path parameter path cannot contain a path separator + + + +=== TEST 108: path validation also applies to flattened arguments +--- config + location /t { + content_by_lua_block { + local ok = require("lib.openapi_to_mcp_fixture").put_routes({ + { 1, "/mcp", { + transport = "streamable_http", + base_url = "http://127.0.0.1:11460";, + openapi_url = "http://127.0.0.1:11460/path-safety.json";, + flatten_parameters = true, + } }, + }) + if ok then ngx.say("passed") end + } + } +--- response_body +passed + + + +=== TEST 109: a flattened call is judged on its finished segment +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"adjacentPathParameters","arguments":{"a":".","b":"."}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/(?s)(?=.*"isError":true)(?=.*cannot produce an empty or dot path segment)/ + + + +=== TEST 110: a route can opt in to encoded path separators +--- config + location /t { + content_by_lua_block { + local ok = require("lib.openapi_to_mcp_fixture").put_routes({ + { 1, "/mcp", { + transport = "streamable_http", + base_url = "http://127.0.0.1:11460";, + openapi_url = "http://127.0.0.1:11460/path-safety.json";, + allow_encoded_path_separators = true, + } }, + }) + if ok then ngx.say("passed") end + } + } +--- response_body +passed + + + +=== TEST 111: the opt-in sends a slash as data +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getEncodedPath","arguments":{"pathParameters":{"path":"group/project"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*encoded\/group%2Fproject/ + + + +=== TEST 112: the opt-in sends a backslash as data +--- exec +timeout 5 curl -X POST -N -sS http://localhost:1984/mcp \ + -d '{"method":"tools/call","jsonrpc":"2.0","id":1,"params":{"name":"getEncodedPath","arguments":{"pathParameters":{"path":"group\\project"}}}}' \ + -H "Content-Type: application/json" \ + -H "Accept: application/json, text/event-stream" \ + 2>&1 | cat +--- response_body eval +qr/seen_path.*encoded\/group%5Cproject/ + + + +=== TEST 113: the opt-in still rejects a leading parent component Review Comment: The opt-in cases here all have a separator in the value (`../admin`, `a/../b`, `a//b`), so they go through `separator_creates_unsafe_component`. A plain `..` or `.` with the opt-in on relies only on the finished-path check, and nothing covers that. Worth one more case so a later refactor can't open it up quietly. -- 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]
