AlinsRan commented on code in PR #14154:
URL: https://github.com/apache/apisix/pull/14154#discussion_r4478569704
##########
apisix/plugins/openapi-to-mcp/tools/handler.lua:
##########
@@ -124,20 +125,133 @@ 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 everything before one of the given URI delimiters. A template
+-- expression name may itself contain a delimiter, so only one outside braces
+-- counts. Parameter values cannot introduce one: escape_uri encodes them.
+local function before_uri_delimiter(value, delimiters)
+ 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 str_find(delimiters, char, 1, true) then
+ return str_sub(value, 1, index - 1)
+ end
+ end
+ return value
+end
+
+
+local function uri_path(value)
+ return before_uri_delimiter(value, "?#")
+end
+
+
+local function without_fragment(value)
+ return before_uri_delimiter(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
Review Comment:
This also splits `/` inside a parameter name. [OpenAPI permits `/` in a
template parameter
name](https://spec.openapis.org/oas/v3.2.1.html#path-templating), and `{a/b}`
then takes two entries in the template's segment list but one after
substitution, so every later index is off by one. That cuts both ways on
`e309d12`:
| Template | Arguments | Result |
| --- | --- | --- |
| `/x/{scope/key}/{id}/..` | `scope/key="safe"`, `id="ok"` | `isError:
true`, `path parameter in segment {id} cannot produce an empty or dot path
segment` (succeeded on `6205e4a`) |
| `/x/{a/b}/{c}` | `a/b="v"`, `c=".."` | sends `/x/v/..` |
| `/x/{a/b}/{c}` | `a/b="v"`, `c=""` | sends `/x/v/` |
| `/x/{a/b}/{c}/y` | `a/b="v"`, `c=".."` | sends `/x/v/../y` |
In the first row the literal trailing `..` is checked as `{id}`. In the
others the `..` or empty value from `{c}` lines up with `b}`, which has no `{`,
so it is treated as a template-only segment and skipped.
Could we split the template only on slashes outside `{...}`, as
`before_uri_delimiter` already does for `?` and `#`, and add both the rejected
and the bypass cases as regressions? The substituted path can stay as is, since
values are encoded and cannot add a slash.
--
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]
