shreemaan-abhishek commented on code in PR #14154:
URL: https://github.com/apache/apisix/pull/14154#discussion_r4476511727
##########
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:
Good catch. This is a pre-existing correctness/defense-in-depth issue rather
than something introduced by this change. I have tracked it in
https://github.com/apache/apisix/issues/14158 and will handle it in a focused
follow-up PR so #14154 stays scoped to path-segment safety.
--
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]
