AlinsRan opened a new pull request, #13959:
URL: https://github.com/apache/apisix/pull/13959
### Description
A security review of `openapi-to-mcp` (added in #13942, merged into master
and not in any release) found six issues, all reachable by someone who can
either call a tool or serve the OpenAPI document the Route points at. This PR
fixes them, with a test per issue.
#### 1. Arguments were validated but not filtered
`split_arguments()` passed the `headerParameters` object through as it
arrived. The generated input schema does not forbid extra properties, and an
operation that declares no header parameter gets no `headerParameters`
container to constrain either, so for most operations the object was free-form.
`tools/handler` then wrote every entry into the outgoing request, after the
Route's own `headers`.
A tool call could therefore:
- replace a credential the Route adds (`Authorization`, an API key), because
the caller's headers were written last;
- add headers the API trusts, such as `X-Forwarded-For` or `Host`;
- put a newline in a value. `resty.http` writes `"<name>: <value>\r\n"` as
given, so that appends headers — or a whole second request — to the one being
sent.
```json
{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"getPetById",
"arguments":{"pathParameters":{"petId":1},
"headerParameters":{"Authorization":"attacker","X-A":"v\r\nX-Injected:
1"}}}}
```
Arguments are now filtered to the parameters the operation declares, in both
the flat and the nested shape; the Route's `headers` are applied last so a call
cannot replace them; and a header whose name or value cannot appear in a
request header is dropped with a warning. The same filtering closes the query
side, where an undeclared key could be appended to the upstream query string
(`admin=true`, a second `api_key=`).
#### 2. No bound on the upstream response
The response was buffered whole and then pretty-printed by building one
table entry per character, which costs several times the body's size in memory.
Nothing limited either step, so one tool call against an endpoint that returns
a large body could exhaust a worker.
The body is now read in chunks up to `max_response_body_size` (1 MiB by
default, configurable); a larger response fails the call with
`RESPONSE_TOO_LARGE` rather than being buffered. Pretty-printing falls back to
the compact form past 256 KiB.
#### 3. `$ref` expansion had no total budget
Expansion inlines, so a document whose every level fans out *b* ways
produces b^depth nodes; with `MAX_DEPTH = 16` and b = 4 that is about 4×10⁹.
Ordinary nesting does not increase depth either. Expansion now stops after 50k
nodes and degrades what is left to `{"type": "object"}`.
#### 4. An external `$ref` was an SSRF primitive
An `http(s)` `$ref` is a URL the *document* chooses and the gateway dials.
There was no host restriction, so a document could point the gateway at
`169.254.169.254` or an internal admin port, with the fetched structure visible
in `tools/list`. A `$ref` is now followed only to the host the document itself
came from, plus whatever the new `allowed_ref_hosts` names.
#### 5. SSE sessions were not bound to the Route that issued them
`session.create()` stored a bare id and `handle_post` only checked that the
id existed. A session id issued on an authenticated Route could be replayed
against another `openapi-to-mcp` Route on the same instance, and the answer —
computed with *that* Route's configuration — landed in the first stream.
Sessions now carry the Route and the consumer they were issued for, and the
message endpoint refuses anything else. The dict keys also carry a prefix of
their own instead of sharing a namespace with `mcp-bridge`.
#### 6. No Origin check
MCP asks an HTTP transport to validate `Origin`, because a browser page can
otherwise reach a server bound to localhost (DNS rebinding). The new
`allowed_origins` does that; it stays off until a Route names its origins, so
nothing changes for existing configurations.
Also in this PR: configured `headers` can no longer carry a newline, or a
name that is not a header name (the schema constrained values of matching names
only); and the docs get a security section covering the document as an
untrusted input, `base_url` built from client-controlled variables, and
`limit-conn` for SSE Routes.
### Tests
`t/plugin/openapi-to-mcp*.t`, 769 assertions, all passing locally:
- an undeclared header or query parameter never reaches the API; a declared
one does; a newline in a declared value drops the header; the Route's
credential survives a call that tries to replace it;
- a response over the limit fails the call, one under it comes back whole;
- expansion stops at the node budget; a `$ref` to another host is not
followed, `allowed_ref_hosts` (including a wildcard) lets one through;
- a session id from one Route is refused by another
(`openapi_to_mcp_cross_route.py`), a session keeps its owner across a refresh,
and the dict keys are prefixed;
- an allowed Origin is served, another is refused with 403, and no Origin
header still works;
- configured headers with a newline or a bad name are rejected by the schema.
#### Which issue(s) this PR fixes:
N/A
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not,
please discuss on the [APISIX mailing
list](https://github.com/apache/apisix/tree/master#community) first)
--
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]
