Skip to content

mcp: add tools/call request transcocding.#43742

Merged
botengyao merged 13 commits intoenvoyproxy:mainfrom
guoyilin42:mcp
Mar 12, 2026
Merged

mcp: add tools/call request transcocding.#43742
botengyao merged 13 commits intoenvoyproxy:mainfrom
guoyilin42:mcp

Conversation

@guoyilin42
Copy link
Copy Markdown
Contributor

Commit Message: mcp: add tools/call request transcocding.
Additional Description: N/A
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yilin Guo <guoyilin@google.com>
@guoyilin42
Copy link
Copy Markdown
Contributor Author

@paulhong01

@yanavlasov
Copy link
Copy Markdown
Contributor

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for tools/call request transcoding in the MCP JSON REST bridge. However, it introduces a high-severity path traversal vulnerability in the URL construction logic due to permissive percent encoding in path templates, potentially allowing attackers to inject path traversal sequences. Additionally, there is a medium-severity risk of sensitive data exposure because the full request body is logged at the info level, which may leak sensitive tool arguments. The core logic is in http_request_builder.cc and has unit tests. Please address these security concerns to ensure the safety and integrity of the system.

Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
@tyxia
Copy link
Copy Markdown
Member

tyxia commented Mar 5, 2026

/gemini review

@agrawroh
Copy link
Copy Markdown
Member

agrawroh commented Mar 5, 2026

@tyxia I think the Gemini Review stopped working for some reason :(

@botengyao
Copy link
Copy Markdown
Member

It was disabled intentionally to figure out the trigger per @yanavlasov

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Mar 5, 2026

@agrawroh @botengyao , yeah it is probably disabled in #43772

I hope it still can be invoked manually by command like /gemini review / or auto-enabled on the PR that is ready for review.

Copy link
Copy Markdown
Contributor

@paulhong01 paulhong01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Signed-off-by: Yilin Guo <guoyilin@google.com>
@agrawroh
Copy link
Copy Markdown
Member

agrawroh commented Mar 6, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for tools/call request transcoding in the MCP JSON REST bridge filter, introducing a new HttpRequestBuilder for constructing HTTP requests from JSON-RPC tool calls. However, a high-severity Path Traversal / URL Manipulation vulnerability has been identified in http_request_builder.cc. The constructBaseUrl function fails to URL-encode values from the untrusted arguments JSON object before inserting them into the URL path template, which could allow an attacker to manipulate the resulting URL and access unintended backend endpoints. While the overall implementation is well-organized and thoroughly tested, there is also a suggestion to enhance the readability and performance of JSON parsing within the filter.

Signed-off-by: Yilin Guo <guoyilin@google.com>
botengyao
botengyao previously approved these changes Mar 9, 2026
Copy link
Copy Markdown
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for moving this forward! lgtm module some questions.

/wait

Copy link
Copy Markdown
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

As buffering issue i pointed out below, please add the integration test to verify that this PR is working end to end.

@botengyao botengyao dismissed their stale review March 9, 2026 17:25

click wrong approval here.

Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
Copy link
Copy Markdown
Member

@agrawroh agrawroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll wait for @botengyao to do a final pass.

Copy link
Copy Markdown
Member

@agrawroh agrawroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an offline discussion with @tyxia and re-reviewed some of the parts which I didn't pay close attention to. This feedback from @tyxia is a MUST-fix before we should ship it. Please address the gaps.

Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
tyxia
tyxia previously approved these changes Mar 12, 2026
Copy link
Copy Markdown
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution! LGTM as a baseline implementation.

Please add buffer protection in the next PR to ensure this filter is production ready,

botengyao
botengyao previously approved these changes Mar 12, 2026
Copy link
Copy Markdown
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guoyilin42 could also merge main?

Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
@guoyilin42
Copy link
Copy Markdown
Contributor Author

@guoyilin42 could also merge main?

Done.

@guoyilin42 guoyilin42 dismissed stale reviews from botengyao and tyxia via 4afd7fb March 12, 2026 18:03
@guoyilin42 guoyilin42 requested a review from agrawroh March 12, 2026 18:39
@botengyao
Copy link
Copy Markdown
Member

/retest

Copy link
Copy Markdown
Member

@agrawroh agrawroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@botengyao botengyao merged commit 43e718e into envoyproxy:main Mar 12, 2026
29 checks passed
bmjask pushed a commit to bmjask/envoy that referenced this pull request Mar 14, 2026
Commit Message: mcp: add tools/call request transcocding.
Additional Description: N/A
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: bjmask <11672696+bjmask@users.noreply.github.com>
@guoyilin42 guoyilin42 deleted the mcp branch March 16, 2026 18:38
fishcakez pushed a commit to fishcakez/envoy that referenced this pull request Mar 25, 2026
Commit Message: mcp: add tools/call request transcocding.
Additional Description: N/A
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Yilin Guo <guoyilin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants