[Mirror] Parse port numbers from MCP server URLs in CORS proxy#88
[Mirror] Parse port numbers from MCP server URLs in CORS proxy#88ngxson wants to merge 4 commits intongxson:masterfrom
Conversation
📝 WalkthroughWalkthroughPort extraction logic added to HTTP URL parsing with default values (80 for HTTP, 443 for HTTPS). Proxy constructor updated to accept scheme and timeout parameters. HTTPS detection switched from port-based (443) to scheme-based comparison. Unit tests introduced for proxy functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/server/server-models.cpp (1)
1083-1091:⚠️ Potential issue | 🟠 MajorPreserve
:portin the forwardedHostheader.Now that explicit ports are parsed and passed through, the
Hostrewrite still strips the non-default port. A proxied request likehttp://127.0.0.1:8123/...will be forwarded asHost: 127.0.0.1, which can break upstream routing for the very custom-port case this patch is enabling. Either lethttplibsynthesizeHostor include:portwhen the port is non-default.🔧 Suggested fix
for (const auto & [key, value] : headers) { if (key == "Accept-Encoding") { // disable Accept-Encoding to avoid compressed responses continue; } if (key == "Host" || key == "host") { - req.set_header(key, host); + const bool default_port = + (scheme == "http" && port == 80) || + (scheme == "https" && port == 443); + req.set_header(key, default_port ? host : host + ":" + std::to_string(port)); } else { req.set_header(key, value); } }Also applies to: 1155-1162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/server/server-models.cpp` around lines 1083 - 1091, The Host header rewrite is stripping explicit ports; when forwarding, preserve the original :port for non-default ports or omit Host so httplib can synthesize it. Update the code that builds/overrides headers (uses the parameters scheme, host, port and the headers map) to either remove any explicit Host entry from headers so httplib will set it, or construct Host as host + ":" + std::to_string(port) when port is not the default for the scheme (default 80 for "http", 443 for "https"); apply the same change in the other occurrence noted (the block around the symbols mentioned at lines ~1155-1162).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/server/tests/unit/test_proxy.py`:
- Around line 27-30: The test currently proxies to http://example.com which
creates an external dependency and can hang; instead spin up a tiny local HTTP
server inside the test (e.g., using Python's http.server.HTTPServer in a
background thread) that returns a small known body like "Example Domain", then
build the proxy URL by embedding that local server's host:port into the existing
url variable (replace the hardcoded example.com with the local server address)
so the cors-proxy routes to the local server, call requests.get(url, timeout=2)
to avoid hanging, and ensure you cleanly shutdown the local HTTPServer/thread
after the assertion so server.server_host/server.server_port and the local
server lifecycle are handled within the test.
- Around line 1-2: Replace the wildcard import "from utils import *" with
explicit imports of the symbols used in this test (e.g., ServerPreset and
requests) so Ruff F403 is avoided and the test is self-contained; update the
import line to "from utils import ServerPreset, requests" (and add any other
specific names referenced in this file) and remove the star import.
---
Outside diff comments:
In `@tools/server/server-models.cpp`:
- Around line 1083-1091: The Host header rewrite is stripping explicit ports;
when forwarding, preserve the original :port for non-default ports or omit Host
so httplib can synthesize it. Update the code that builds/overrides headers
(uses the parameters scheme, host, port and the headers map) to either remove
any explicit Host entry from headers so httplib will set it, or construct Host
as host + ":" + std::to_string(port) when port is not the default for the scheme
(default 80 for "http", 443 for "https"); apply the same change in the other
occurrence noted (the block around the symbols mentioned at lines ~1155-1162).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79737ddf-b7bc-4eb9-b211-2538ea11f4db
📒 Files selected for processing (6)
common/http.htools/server/server-cors-proxy.htools/server/server-models.cpptools/server/server-models.htools/server/tests/unit/test_proxy.pytools/server/tests/utils.py
| import pytest | ||
| from utils import * |
There was a problem hiding this comment.
Replace the star import; it already trips Ruff.
from utils import * introduces the F403 error here and also obscures where ServerPreset and requests come from. Import those names explicitly so this test stays lint-clean and self-contained.
🧹 Suggested fix
import pytest
-from utils import *
+import requests
+
+from utils import ServerPreset📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import pytest | |
| from utils import * | |
| import pytest | |
| import requests | |
| from utils import ServerPreset |
🧰 Tools
🪛 Ruff (0.15.4)
[error] 2-2: from utils import * used; unable to detect undefined names
(F403)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/server/tests/unit/test_proxy.py` around lines 1 - 2, Replace the
wildcard import "from utils import *" with explicit imports of the symbols used
in this test (e.g., ServerPreset and requests) so Ruff F403 is avoided and the
test is self-contained; update the import line to "from utils import
ServerPreset, requests" (and add any other specific names referenced in this
file) and remove the star import.
| url = f"http://{server.server_host}:{server.server_port}/cors-proxy?url=http://example.com" | ||
| res = requests.get(url) | ||
| assert res.status_code == 200 | ||
| assert "Example Domain" in res.text |
There was a problem hiding this comment.
Avoid a live internet dependency in this test.
Proxying http://example.com makes this unit test flaky in offline or firewalled CI, and the bare requests.get can hang indefinitely if the network stalls. Please proxy to a tiny local HTTP server instead and keep a short client timeout.
🧰 Tools
🪛 Ruff (0.15.4)
[error] 28-28: requests may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/server/tests/unit/test_proxy.py` around lines 27 - 30, The test
currently proxies to http://example.com which creates an external dependency and
can hang; instead spin up a tiny local HTTP server inside the test (e.g., using
Python's http.server.HTTPServer in a background thread) that returns a small
known body like "Example Domain", then build the proxy URL by embedding that
local server's host:port into the existing url variable (replace the hardcoded
example.com with the local server address) so the cors-proxy routes to the local
server, call requests.get(url, timeout=2) to avoid hanging, and ensure you
cleanly shutdown the local HTTPServer/thread after the assertion so
server.server_host/server.server_port and the local server lifecycle are handled
within the test.
2 participants
Mirror from upstream PR: ggml-org#20208
Note: @coderabbitai use my 'Mirror PR' preset for reviewing this.
Summary by CodeRabbit
New Features
--webui-mcp-proxyserver startup option to enable/disable proxy functionality.Tests