shreemaan-abhishek commented on code in PR #14154:
URL: https://github.com/apache/apisix/pull/14154#discussion_r4476614407
##########
apisix/plugins/openapi-to-mcp/tools/handler.lua:
##########
@@ -124,19 +124,72 @@ local function apply_query_defaults(tool, query)
end
+-- A path parameter fills part of one segment of the operation's path, so its
+-- value cannot carry a separator. A "/" goes out as "%2F", which a server that
+-- accepts an encoded separator decodes back into one, and the call would then
+-- address a path the operation does not describe. This reads the value as it
+-- arrived, because the encoding hides the separator from the finished path.
+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
+
+
+-- The segments of a path, with the empty one a leading or trailing slash
+-- leaves: "/items/x" is { "", "items", "x" }.
+local function path_segments(path)
+ local segments = {}
+ 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. Rejecting a separator
+-- above keeps both segment lists the same length, so they line up by index.
+local function resolved_away(segment)
+ return segment == "" or segment == "." or segment == ".."
+end
+
+
local function build_path(template, path_params)
local path = template
for name, value in pairs(path_params) do
+ local text = tostring(value)
+ if has_separator(text) then
Review Comment:
Addressed in 86c434ac2 with the compatibility/security compromise discussed
here: separators remain rejected by default, and a route can opt in with
`allow_encoded_path_separators: true`. In opt-in mode, clean values such as
`group/project` are percent-encoded and sent, while separator-delimited empty,
`.` and `..` components are still rejected. The flag and breaking-change
behavior are documented in both language pages and the changelog.
##########
apisix/plugins/openapi-to-mcp/tools/handler.lua:
##########
@@ -124,19 +124,72 @@ local function apply_query_defaults(tool, query)
end
+-- A path parameter fills part of one segment of the operation's path, so its
+-- value cannot carry a separator. A "/" goes out as "%2F", which a server that
+-- accepts an encoded separator decodes back into one, and the call would then
+-- address a path the operation does not describe. This reads the value as it
+-- arrived, because the encoding hides the separator from the finished path.
+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
+
+
+-- The segments of a path, with the empty one a leading or trailing slash
+-- leaves: "/items/x" is { "", "items", "x" }.
+local function path_segments(path)
+ local segments = {}
+ 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. Rejecting a separator
+-- above keeps both segment lists the same length, so they line up by index.
+local function resolved_away(segment)
+ return segment == "" or segment == "." or segment == ".."
+end
+
+
local function build_path(template, path_params)
local path = template
for name, value in pairs(path_params) do
+ local text = tostring(value)
+ if has_separator(text) then
+ return nil, "path parameter " .. name .. " cannot contain a path
separator"
Review Comment:
Addressed in 86c434ac2. `_M.call` now emits a warning containing the
`build_path` error for both separator and finished-segment rejections, with an
integration assertion for the log entry.
##########
apisix/plugins/openapi-to-mcp/tools/handler.lua:
##########
@@ -124,19 +124,72 @@ local function apply_query_defaults(tool, query)
end
+-- A path parameter fills part of one segment of the operation's path, so its
+-- value cannot carry a separator. A "/" goes out as "%2F", which a server that
+-- accepts an encoded separator decodes back into one, and the call would then
+-- address a path the operation does not describe. This reads the value as it
+-- arrived, because the encoding hides the separator from the finished path.
+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
+
+
+-- The segments of a path, with the empty one a leading or trailing slash
+-- leaves: "/items/x" is { "", "items", "x" }.
+local function path_segments(path)
+ local segments = {}
+ 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. Rejecting a separator
+-- above keeps both segment lists the same length, so they line up by index.
+local function resolved_away(segment)
+ return segment == "" or segment == "." or segment == ".."
+end
+
+
local function build_path(template, path_params)
local path = template
for name, value in pairs(path_params) do
+ local text = tostring(value)
+ if has_separator(text) then
+ return nil, "path parameter " .. name .. " cannot contain a path
separator"
+ end
-- the name is a literal, so escape any pattern magic in it
local pattern = "{" .. str_gsub(name, "([%^%$%(%)%%%.%[%]%*%+%-%?])",
"%%%1") .. "}"
- local escaped = escape_uri(tostring(value))
+ local escaped = escape_uri(text)
-- The replacement has to be a function: percent-encoding produces
"%2F"
-- and friends, and gsub would read those as capture references in a
-- string replacement and raise "invalid capture index".
path = str_gsub(path, pattern, function()
return escaped
end)
end
+
+ local declared = path_segments(template)
+ for index, segment in ipairs(path_segments(path)) do
Review Comment:
Addressed in 86c434ac2. Validation now isolates the URI path before
splitting segments, using a brace-aware scan so `?` or `#` inside an expression
name is not mistaken for a suffix. Tests cover dot/empty values in `?acl`
templates, a dot value in a `#acl` template, and a safe query-suffixed request.
##########
t/plugin/openapi-to-mcp.t:
##########
@@ -1573,3 +1573,173 @@ 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
Review Comment:
Addressed in 86c434ac2. Added coverage for adjacent expressions,
literal-separated expressions, literal prefixes, flattened parameters,
query/fragment suffixes, and both default and opt-in separator behavior.
##########
docs/en/latest/plugins/openapi-to-mcp.md:
##########
@@ -65,6 +65,7 @@ When a tool is called, the Plugin builds the request from the
operation:
* Query parameters are serialized according to their `style` and `explode`, as
defined by the [OpenAPI Parameter
Object](https://spec.openapis.org/oas/v3.0.3#style-values). With the defaults
(`form`, exploded), `tags: ["a", "b"]` is sent as `tags=a&tags=b` -- not as
`tags[]=a&tags[]=b`, and an array parameter declared `explode: false` is sent
as `tags=a,b`. `spaceDelimited`, `pipeDelimited` and `deepObject` are
supported. An API that expects the bracket form has to be reached through a
Plugin that rewrites the query string.
* A request body is sent with the media type the operation declares, unless
`headers` sets `Content-Type`.
* Header parameters are applied before the Plugin's own `headers`, so a tool
call cannot replace a credential the route adds, and a header whose value
contains a newline is dropped.
+* A path parameter cannot contain a slash or a backslash, and once every
parameter is filled in, a segment an expression helped build cannot be empty,
`.` or `..`. Either fails the call with `isError` set, because the server
receiving the request would resolve such a path to one the operation does not
describe. A parameter that fills only part of a segment is free to hold those
values: `/files/{name}.json` with `name` set to `..` addresses `/files/...json`.
Review Comment:
Addressed in 86c434ac2. The English and Chinese docs now state the
finished-segment rule explicitly with the `/files/{name}.json` example, and
document the new secure-default separator opt-in.
--
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]
