Parse port numbers from MCP server URLs in CORS proxy#20208
Parse port numbers from MCP server URLs in CORS proxy#20208ngxson merged 4 commits intoggml-org:masterfrom
Conversation
ngxson
left a comment
There was a problem hiding this comment.
Technically this won't support something like https://myserver.com:8443
|
Why not? Because it doesn't recognize 8443 as a tls port? |
|
There is a check llama.cpp/tools/server/server-models.cpp Lines 1080 to 1101 in c5a7788 |
|
Ah ok I just need to pass the scheme in explicitly there. I will refresh the patch tomorrow morning when I'm back at my desk. |
a9f3981 to
a9daf8a
Compare
| } | ||
| auto proxy = std::make_unique<server_http_proxy>( | ||
| method, | ||
| "http", |
There was a problem hiding this comment.
While adding scheme to be passed around we also now need a scheme here (for the model router). I don't see an existing scheme here, but AFAICT the child processes always just use http so I can hard-code it (model routing still works in a trivial local test with it hard-coded like this). Wanted to call this out in case there's a flag I'm not familiar with for sending the communications to the child inference processes via https or something.
| } | ||
|
|
||
| SRV_INF("proxying %s request to %s://%s%s\n", method.c_str(), parsed_url.scheme.c_str(), parsed_url.host.c_str(), parsed_url.path.c_str()); | ||
| SRV_INF("proxying %s request to %s://%s:%i%s\n", method.c_str(), parsed_url.scheme.c_str(), parsed_url.host.c_str(), parsed_url.port, parsed_url.path.c_str()); |
There was a problem hiding this comment.
I don't love that we now always log the port explicitly, but parsed_url no longer indicates whether the port was explicit or implicit.
| #endif | ||
|
|
||
| httplib::Client cli(parts.scheme + "://" + parts.host); | ||
| httplib::Client cli(parts.scheme + "://" + parts.host + ":" + std::to_string(parts.port)); |
There was a problem hiding this comment.
I ran it through ChatGPT Codex and it pointed out that I'd broken http downloads on non-standard ports because that had been relying on the port being smushed into the host still. Not quite sure how to test this, but it seems right.
| parts.path = "/"; | ||
| } | ||
|
|
||
| auto colon_pos = parts.host.find(':'); |
There was a problem hiding this comment.
How do we feel about IPv6 addresses? They can have colons in their literals which will break here. But parsing those properly feels way outside the scope of a little internal helper method, and starts to feel like we should be pulling in a library which can do proper URL parsing...
|
@ngxson I went down a bit of a rabbit-hole trying to support IPv6 literals and it turned into a substantial refactor which I didn't manage to get to the bottom of before giving up. In particular, it adds more logic anywhere we convert from parts to full-URL or back (because a host of I haven't tried but I'm pretty sure those are already broken for MCP in master, because we're not stripping the Putting all that aside, I've addressed the tls-inferred-from-port issue you pointed out, and I think otherwise this PR is a reasonable improvement over the current state of master. |
ngxson
left a comment
There was a problem hiding this comment.
I think it's fine as a quick fix, we can refactor the parser to support ipv6 in a dedicated PR, but I think not many people will actually use it
…rg#20208) * Parse port numbers from MCP server URLs * Pass scheme to http proxy for determining whether to use SSL * Fix download on non-standard port and re-add port to logging * add test --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
…rg#20208) * Parse port numbers from MCP server URLs * Pass scheme to http proxy for determining whether to use SSL * Fix download on non-standard port and re-add port to logging * add test --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
|
There is one problem still left - server sets Host header with host only, but it should include port as well. There are some strict mcp servers that reject requests that have invalid Host header. Created MR: #20843 |
…rg#20208) * Parse port numbers from MCP server URLs * Pass scheme to http proxy for determining whether to use SSL * Fix download on non-standard port and re-add port to logging * add test --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
…rg#20208) * Parse port numbers from MCP server URLs * Pass scheme to http proxy for determining whether to use SSL * Fix download on non-standard port and re-add port to logging * add test --------- Co-authored-by: Xuan Son Nguyen <son@huggingface.co>
Fixes #20205.
When specifying an MCP server on a custom port (e.g.
http://127.0.0.1:8123/mcp), the CORS proxy was not parsing the port number, leading to a malformed host (with the trailing port still attached) and falling back to port 80/443.This is my first PR here and it's been a while since I touched C/C++ so likely I've done something wrong with the formatting or data types or something 😅
The only AI used in making this PR was Copilot's autocomplete, which I trusted to pick the right stdlib methods for
stoiand throwing a runtime error as my C++ is so rusty.