nic-6443 commented on code in PR #14154:
URL: https://github.com/apache/apisix/pull/14154#discussion_r4475591373
##########
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:
I'd relax this check. Rejecting every `/` and `\` also turns away calls that
have nothing to do with traversal: GitLab's `/projects/{id}` documents
`group%2Fproject` as the way to call it, and GitHub's
`/repos/{owner}/{repo}/contents/{path}` with `src/index.js` or a FastAPI
`{file_path:path}` route have the same shape. The caller can't work around it
either, since sending `group%2Fproject` gets encoded again into
`group%252Fproject`. The OpenAPI spec only rules out an unescaped `/`, and the
value goes out escaped.
What we want to stop is a value that climbs out of its segment, not any
value with a slash in it. So I'd only reject a value with a separator when one
of its pieces is empty, `.` or `..`:
```lua
local function climbs_out(value)
if not str_find(value, "[/\\]") then
return false
end
for piece in str_gmatch(value .. "/", "([^/\\]*)[/\\]") do
if resolved_away(piece) then
return true
end
end
return false
end
```
(`resolved_away` has to move above it.) `../admin`, `a/../b` and `a//b`
still fail, `group/project` goes out as `group%2Fproject`, and a value with no
separator stays with the finished-path check below. `escape_uri` already
encodes the separator, so the two segment lists still line up by index. The
comment above `resolved_away`, the docs and the separator tests (TEST 90, 91,
98) would need to follow, and the backward-compatibility caveat in the
description goes away.
How a server treats `%2F` beyond that is the server's call. We can't keep up
with every decoder out there, but we can make sure the URL we build doesn't
climb.
##########
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:
Small one: `_M.call` warns when it drops a header parameter, but this
rejection and the one below leave nothing in the gateway log, so an operator
can't see someone probing for traversal or work out why a legit call is being
turned away. A `core.log.warn` with the error where `build_path` fails at line
418 would cover both.
##########
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:
This misses templates that carry a query or fragment, because the last
segment then holds more than the expression. `/{Bucket}?acl` with `Bucket=..`
goes out as `/..?acl`: the segment is `..?acl`, `resolved_away` doesn't match,
and the server resolves the path part to `/`. An empty `Bucket` gives `/?acl`,
and `/{Bucket}/{Key}#acl` with `Key=..` gives `/b/..#acl`. `endpoints.extract`
passes these templates through as they are, and S3-style documents use them.
A `?` or `#` in the finished path can only come from the template, since
values are encoded, so cutting both strings at the first one (`^[^?#]*`) before
splitting is enough.
##########
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
+ local from_template = declared[index]
+ if from_template and str_find(from_template, "{", 1, true)
+ and resolved_away(segment)
+ then
+ return nil, "path parameter in segment " .. from_template ..
+ " cannot produce an empty or dot path segment"
+ end
+ end
+
return path
Review Comment:
Not new in this PR, but this loop walks right past it: a `{expr}` that never
got a value goes out literally. When the document doesn't mark the parameter as
required, schema validation passes and `{}` sends `/items/{id}/detail`.
`/x/{a}{b}` with only `a=..` sends `/x/..{b}`, which also gets past the dot
check.
Values are encoded, so a `{` left in the finished path can only be an
unfilled expression, and failing the call on that would close both.
##########
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:
`those values` reads as if it covers the slash and backslash from earlier in
the sentence, but TEST 98 rejects a separator in a partly filled segment too.
It isn't unconditional for dots either: `/x/{a}{b}` with `.` and `.` is
rejected, and so is `/x/.{a}` with `.`.
How about stating the rule instead: "What counts is the finished segment, so
`/files/{name}.json` with `name` set to `..` is accepted and addresses
`/files/...json`." Line 68 of the zh page needs the same change.
##########
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:
Coverage gap: all of these use one expression with nested `pathParameters`,
so a check that only looks at segments that are exactly one expression would
still pass every one of them. Worth adding the cases the finished-path check
exists for: `/x/{a}{b}` with `.` and `.`, `/x/{a}.{b}` with two empty values,
`/x/.{a}` with `.`, a run with `flatten_parameters`, and `?` / `#` templates
for the case above.
--
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]
