Conversation
… caller A security review of the merged plugin turned up six issues, all of them reachable by someone who can call a tool or serve the OpenAPI document. Tool call arguments were validated but not filtered. 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 `headerParameters` reached the API as it arrived: a caller could add request headers, replace the credential the route configures, or put a newline in a value and, since resty.http writes the header line as given, append a request of its own. Arguments are now filtered to the parameters the operation declares, in both the flat and the nested shape; the route's own headers are written last so a tool call cannot replace them; and a header whose name or value cannot appear in a request header is dropped. The same filtering closes the query-parameter side, where an undeclared key could be appended to the upstream query string. The upstream response was read whole and then pretty-printed by building one table entry per character, so a large response cost several times its size in memory with no bound. The body is now read in chunks up to `max_response_body_size` (1 MiB by default) and a larger one fails the call; pretty-printing falls back to the compact form past 256 KiB. $ref expansion inlines, so a document whose every level fans out several ways grows as the product of those widths -- MAX_DEPTH alone left room for billions of nodes. Expansion now stops at 50k nodes. An external $ref is also a URL the document chooses and the gateway dials, so it is followed only to the host the document itself came from, plus whatever `allowed_ref_hosts` names. An SSE session was a bare id in a shared dict: any route could accept it, so a session id issued on an authenticated route could be used from another one to push a tool result into that stream. Sessions now carry the route and consumer they were issued for, and their dict keys carry a prefix of their own rather than sharing a namespace with mcp-bridge. `allowed_origins` adds the Origin check the MCP spec asks of an HTTP transport; it is off until the route names its origins. Configured headers can no longer carry a newline or a name that is not a header name. The docs get a security section: the document is an input rather than trusted configuration, `base_url` should not be built from client-controlled variables, and an SSE route reachable by untrusted clients wants limit-conn.
lua-resty-http rejects a '?' inside the request path, so the query string has to travel in its own field.
Review of the previous commit found two behaviours that the switch from request_uri() to connect()+request() had changed by accident. A body with neither Content-Length nor chunked encoding ends when the connection does, and the streaming reader reports that as "closed" alongside the last piece of it. Treating every read error as a failure threw a complete body away and answered with a transport error instead. The read now keeps the chunk it was handed and only treats "closed" as an error when the response declared a length it did not deliver. Setting Host from the parsed URL dropped the port: the client appends it itself when it is not the default for the scheme, and only when the caller left the header unset, so a backend on any other port was addressed as if it were on 80. The header is left to the client again. Both live in a new fetch module, which openapi/loader.lua now uses too: the document fetch was still unbounded, and up to eight more documents are pulled in by $ref from hosts the document names, so the same cap applies there (4 MiB). Also from the review: - The route's own headers are checked for a newline at request time, not only in the schema. They are templates resolved per request, so the schema cannot vouch for what a variable puts in them. - The schema patterns anchor with \z. PCRE's $ also matches before a trailing newline, so "X-Trace: v\n" passed the check it was written to fail. - The Origin rejection is emitted after the JSON content type is set. - allowed_origins no longer treats "*" as an entry: leaving the field out is what turns the check off. - openapi/loader.lua says "declares no paths" rather than calling a document with webhooks and no paths "not an openapi document". New tests: the Host the API receives, a connection-close-delimited body, a body larger than one read, the compact fallback past 256 KiB, a trailing newline in a configured header, a newline arriving through a variable, and Origin on the SSE transport.
The new case was appended with the number it had in another file; reindex requires them to run in sequence.
apache#13956 landed on master and touched the same files. Three places needed a real merge rather than one side or the other: - session.lua: master stores the values a stream resolved in the :alive entry, this branch stores the owner the session belongs to. The entry now carries both, as one JSON object, and `exists()` still refuses a session id presented by anyone else. - cache.lua: master's `loader.validate()` check runs before this branch's host-restricted `$ref` resolution. - loader.lua: master's `validate()` alongside this branch's bounded fetch. The docs and the two test files had both sides appending to the same place; every case from both is kept and renumbered.
Six problems, all in code this branch added.
- The $ref node budget charged the document's own nodes to it. A large but
perfectly ordinary spec would exhaust it, and since Lua does not define
the order the traversal takes, the subtree that degraded to a generic
object could be paths -- leaving tools/list empty with only a warning to
say why. Only what an expansion produces is counted now.
- The same-origin rule for an external $ref compared the host alone. A
document served from 127.0.0.1 could name any other port on that address,
the Admin API and etcd among them, and publish the shape of what came
back through tools/list. Scheme, host and port are compared, and an
allowed_ref_hosts entry may pin a port.
- A body was treated as framed whenever any Transfer-Encoding was present,
but the client reads chunked framing only for "Transfer-Encoding:
chunked" on HTTP/1.1. An "identity" body, which ends at the close like an
unframed one, was reported as a network error. Framing is now decided by
the client's own predicate, and a Content-Length that is not reached is
the one case where a close is a truncated body.
- The cache key joined allowed_ref_hosts with a comma, so { "a.com,b.com" }
-- one entry, allowing nothing -- collided with { "a.com", "b.com" }, and
the same set in another order took a second slot. It is sorted and joined
with a NUL.
- The header schema anchored with \z, which only PCRE has. The schema is
served over the Admin API and validated by clients whose regex flavour
reads \z as a literal "z", where every valid configuration looks invalid.
It anchors with $, and the gap that leaves -- $ also matches before a
trailing newline -- is closed at request time, where the header is
dropped.
- The document size ceiling was a constant while the response ceiling was
an option. It is max_document_size, and it covers the documents an
external $ref pulls in as well.
Two ways a call could reach past its arguments, both through headers the document declares as parameters. A declared "Transfer-Encoding" or "Content-Length" let the caller frame the request the gateway sends. resty.http drops Content-Length as soon as a Transfer-Encoding says chunked, then writes the body exactly as given and unchunked, so the upstream reads the argument body as chunk headers and the caller decides where the request ends -- and the connection goes back to the pool for the next tool call. Those, Host, and the hop-by-hop headers are refused from a call whatever the document says; a Route's own headers are left alone, since an operator naming one means it. Header names are case-insensitive and the client normalises them at the end, but a plain Lua table holds "Authorization" and "authorization" as two keys. Both reached the client, which resolved the collision in pairs() order, so a caller who picked a spelling the hash put last replaced the Route's credential -- the opposite of what this code says it guarantees. Headers are now written one entry per lowercased name, which makes the Route's, written last, the one that counts.
25345a0 to
7d631ea
Compare
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P1] Origin validation is disabled by default
origin_rejected() returns false when allowed_origins is unset, so every existing openapi-to-mcp route still accepts arbitrary browser origins. This leaves the DNS-rebinding issue described by the PR unfixed by default and conflicts with the MCP Streamable HTTP requirement that servers validate the Origin header on all incoming connections.
Please make Origin validation secure by default: reject a present Origin unless it matches an explicitly trusted origin (or a safely derived route origin), and require an explicit escape hatch for intentionally accepting any origin. Add SSE and Streamable HTTP regressions showing that a malicious Origin is rejected when allowed_origins is omitted, while requests without Origin remain supported for non-browser clients.
MCP asks an HTTP transport to validate Origin, and checking it only where allowed_origins was configured left every existing Route accepting a call from any page in any browser. A request that carries an Origin is now accepted only from the origin it was addressed to. allowed_origins names the other origins a Route accepts, and ["*"] accepts any, which is the escape hatch for a deployment that means it. A request with no Origin is untouched: no non-browser MCP client sends one, and those are most of them. Two things this does not claim. An opaque origin -- "null", which a sandboxed frame and a file:// page send -- matches nothing and is refused. And it is not by itself a defence against DNS rebinding, where the attacker owns the name and both Origin and Host are theirs; a Route that declares `hosts` is not reachable that way at all, since a request carrying another Host does not match it. The docs say so rather than implying more.
…s own A Route that lists the origins it accepts does not additionally accept its own: where an attacker controls the name a request was sent to, Origin and Host are both theirs and match each other, so treating that as trusted would go around the list the operator wrote. The docs said what happens with no list; they now say what happens with one.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P1] The default same-origin check still permits DNS rebinding
When allowed_origins is unset, origin_rejected() accepts a request whenever its Origin equals the scheme and Host taken from that same request. In a DNS rebinding attack, the browser sends both Origin: http://attacker.example and Host: attacker.example after DNS changes attacker.example to the APISIX or private address. The two values still compare equal, so a URI-only Route with no hosts constraint accepts the request. The test fixture creates exactly that kind of Route.
Comparing two attacker-controlled request headers blocks ordinary cross-origin calls, but it does not provide the independent trust anchor needed to close the DNS-rebinding issue. The new tests use an evil Origin with the default Host: localhost, so they do not exercise this bypass.
Please validate both Host and Origin against an independent allowlist (for example, secure localhost defaults plus explicitly configured hosts/origins), or reject requests carrying Origin when there is no trusted configuration. Add a regression with matching Host: attacker.example and Origin: http://attacker.example on a Route without a host predicate and expect 403.
…Host Comparing the Origin of a request with the Host of that same request is comparing two headers the same client wrote. Under DNS rebinding the attacker owns the name, so both carry it and agree, and a Route with no host predicate accepted the call -- the case the check was added for. The Origin is now checked against something the operator configured: allowed_origins, or the hosts the Route declares in host/hosts, which the attacker's name does not satisfy. Where the Route declares neither, the one remaining case is loopback at both ends: a page can only have http://localhost as its origin if it is served from the machine itself, and no rebinding produces that. Anything else carrying an Origin is refused. A request with no Origin is still accepted, unchanged: no non-browser MCP client sends one.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P1] Wildcard Route hosts still permit DNS rebinding
When allowed_origins is unset, host_matches() treats a Route entry such as *.example.com as trusted and origin_rejected() accepts every matching subdomain. An attacker who controls attacker.example.com can serve a page from it and then rebind its DNS to APISIX. The browser continues to send Host: attacker.example.com and Origin: http://attacker.example.com; APISIX matches the wildcard Route and this check returns false, so the same DNS-rebinding path remains open for supported wildcard-host configurations.
A Route wildcard is a routing predicate, not a safe Origin allowlist. Please derive default trust only from literal Route hosts and require an explicit, exact allowed_origins configuration when the matched host predicate contains a wildcard. Add Streamable HTTP and SSE regressions using hosts: ["*.example.com"] with matching Host and Origin for an attacker-controlled subdomain, expecting 403.
…igin A "*.example.com" entry says which requests reach the Route. It does not say which origins are trusted: whoever controls one name under it can serve a page there and rebind it to the gateway, and the request then arrives with Host and Origin both on that name and matches the Route. Only a literal host the Route declares counts towards the default trust now. A Route matched on a wildcard accepts an origin through allowed_origins and nowhere else.
Agreed — a wildcard says which requests reach the Route, not which origins are trusted. Fixed in 76c1fba. Default trust is now derived only from literal hosts in New regressions,
A Route mixing both — Docs in both languages now state the wildcard case explicitly rather than saying "the hosts the Route declares". |
…e budget on components core.request.header() hands back the first of a repeated header, so the check for more than one Origin could never fire and the second one was quietly ignored. It reads the header table now. $ref expansion covered the whole document although tools are generated from paths alone, so a document of any size spent the node budget on components -- exactly where a large one keeps its schemas -- and whatever was left, possibly paths itself, degraded to a generic object. Only paths is expanded now, and max_expanded_nodes makes the budget a Route option, since nothing distinguishes a legitimately large document from one built to exhaust memory except how large the operator says their specs get. Alongside, three smaller ones: - The Route-host anchor compared only the hostname, so a page on another port of a declared host passed. The port now has to agree, where agreeing means both spelled it out the same way or neither did -- which is also what a TLS-terminating proxy in front produces. - An allowed_origins entry without a scheme matched nothing, leaving the Route refusing every browser with no way to see why. The schema rejects it, and the 403 body names allowed_origins. - fetch.lua says why an HTTP/1.0 response framed as chunked is refused.
4 participants
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 theheaderParametersobject through as it arrived. The generated input schema does not forbid extra properties, and an operation that declares no header parameter gets noheaderParameterscontainer to constrain either, so for most operations the object was free-form.tools/handlerthen wrote every entry into the outgoing request, after the Route's ownheaders.A tool call could therefore:
Authorization, an API key), because the caller's headers were written last;X-Forwarded-FororHost;resty.httpwrites"<name>: <value>\r\n"as given, so that appends headers — or a whole second request — to the one being sent.{"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
headersare 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 secondapi_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 withRESPONSE_TOO_LARGErather than being buffered. Pretty-printing falls back to the compact form past 256 KiB.3.
$refexpansion had no total budgetExpansion inlines, so a document whose every level fans out b ways produces b^depth nodes; with
MAX_DEPTH = 16and 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
$refwas an SSRF primitiveAn
http(s)$refis a URL the document chooses and the gateway dials. There was no host restriction, so a document could point the gateway at169.254.169.254or an internal admin port, with the fetched structure visible intools/list. A$refis now followed only to the host the document itself came from, plus whatever the newallowed_ref_hostsnames.5. SSE sessions were not bound to the Route that issued them
session.create()stored a bare id andhandle_postonly checked that the id existed. A session id issued on an authenticated Route could be replayed against anotheropenapi-to-mcpRoute 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 withmcp-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 newallowed_originsdoes that; it stays off until a Route names its origins, so nothing changes for existing configurations.Also in this PR: configured
headerscan 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_urlbuilt from client-controlled variables, andlimit-connfor SSE Routes.Tests
t/plugin/openapi-to-mcp*.t, 769 assertions, all passing locally:$refto another host is not followed,allowed_ref_hosts(including a wildcard) lets one through;openapi_to_mcp_cross_route.py), a session keeps its owner across a refresh, and the dict keys are prefixed;Which issue(s) this PR fixes:
N/A
Checklist