Add bearer-auth support via a local loopback proxy - #2
Conversation
The plugin previously registered the real upstream URL as its manifest endpoint address with no way to authenticate to it, since mesh-llm's shared HTTP-forwarding code does pure header pass-through with no credential-injection point anywhere in the plugin protocol. Spawn a loopback-only reverse proxy inside the plugin process that injects Authorization: Bearer <key> (read once at startup from a --api-key-file path, never an env var or bare CLI value, to avoid it appearing in ps/proc) into every forwarded request, and advertise the proxy's own 127.0.0.1 address to mesh-llm instead of the real upstream. This also sidesteps validate_external_endpoint_scheme's http-only rule when the real upstream is https. Any original Authorization header from the calling agent is dropped so only this proxy's own configured key ever reaches the upstream. Signed-off-by: MahdiHedhli <16087011+MahdiHedhli@users.noreply.github.com>
Found by a live test against a real LM Studio instance: the plugin always advertises its own base_url as http://<loopback>/v1, so every caller-constructed request path already starts with /v1 (e.g. /v1/models). The proxy handler was naively concatenating that whole path onto upstream_base_url, which is itself commonly configured with its own /v1 suffix (LM Studio, vLLM, etc.) — producing a broken .../v1/v1/models request that 404s, so no models were ever discovered. Strip the plugin's own advertised prefix before appending the remainder to upstream_base_url. The existing regression test never caught this because its mock upstream used a wildcard fallback route that matched any path regardless of correctness. Switched it to an exact /v1/models route and a "/v1"-suffixed upstream base, matching a real server's shape, so a reintroduced double-prefix bug 404s instead of silently passing. Signed-off-by: MahdiHedhli <16087011+MahdiHedhli@users.noreply.github.com>
…ngth content-length is stripped as hop-by-hop (it goes stale once headers are rewritten in general), and a streamed body with unknown length forces Transfer-Encoding: chunked. mesh-llm-host-runtime's non-streaming JSON relay path reads exactly `content_length` bytes and treats that as the whole body, so a chunked response gets its chunk-size/CRLF framing and terminating "0\r\n\r\n" parsed as part of the JSON payload, failing with a "trailing characters" error. Buffering the response and building it from known-size bytes lets content-length be set correctly again. Confirmed against a real chat completion round-trip through mesh-llm to an external LM Studio server. True SSE streaming pass-through (stream: true) is buffered in full before reaching the caller with this fix, rather than forwarded token-by-token — a known follow-up once the host runtime's streaming path is verified against a buffered-vs-chunked plugin response. Signed-off-by: MahdiHedhli <16087011+MahdiHedhli@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe plugin now runs a loopback Axum reverse proxy. It forwards OpenAI-compatible requests to the configured upstream, manages bearer credentials, advertises the loopback endpoint, and reports forwarding failures as ChangesLoopback proxy integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PluginClient
participant AxumProxy
participant Upstream
PluginClient->>AxumProxy: Send OpenAI-compatible request
AxumProxy->>Upstream: Rewrite path and inject bearer key
Upstream-->>AxumProxy: Return response
AxumProxy-->>PluginClient: Return buffered response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (5)
src/lib.rs (5)
402-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
api_key_from_args()reads the test harness arguments here.Inside a
cargo testbinary,std::env::args()yields the test runner's arguments, not the plugin's. The e2e test can only obtain a key if the operator appends-- --api-key-file <path>to thecargo testinvocation. That coupling is fragile and undocumented in the test.Read the key path from a dedicated environment variable in this test, in the same way
OPENAI_ENDPOINT_E2EandMESH_LLM_PLUGIN_URLare read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 402 - 411, Update e2e_llama_server_answers_openai_requests to read the API key file path from a dedicated environment variable instead of calling api_key_from_args(). Use the environment-based configuration alongside OPENAI_ENDPOINT_E2E and MESH_LLM_PLUGIN_URL, preserving the existing proxy setup and error propagation.
101-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the request body size and return
400for caller-body errors.
to_bytes(body, usize::MAX)buffers the whole request body with no limit. One large request can exhaust process memory. The listener is loopback-only, so the exposure is limited, but a fixed cap is cheap.The error also maps to
502 Bad Gateway. The failure happens while reading the caller's body, before any upstream call.400 Bad Requestdescribes it correctly.♻️ Proposed change
+const MAX_REQUEST_BODY_BYTES: usize = 64 * 1024 * 1024; + - let body_bytes = match axum::body::to_bytes(body, usize::MAX).await { + let body_bytes = match axum::body::to_bytes(body, MAX_REQUEST_BODY_BYTES).await { Ok(bytes) => bytes, Err(error) => { return ( - StatusCode::BAD_GATEWAY, + StatusCode::BAD_REQUEST, format!("reading request body: {error}"), ) .into_response(); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 101 - 110, Update the request-body handling around axum::body::to_bytes to use a fixed maximum byte limit instead of usize::MAX, and change the corresponding error response from StatusCode::BAD_GATEWAY to StatusCode::BAD_REQUEST while preserving the existing error message.
305-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot detect the regression it claims to prevent.
The loop varies
real_upstreambut passes a constantadvertised_base_urlof"http://127.0.0.1:59123/v1". The assertion therefore only proves thatbuild_pluginreturns its second argument in the manifest. The wiring that chooses the advertised value lives inrun_pluginat Lines 244-249, and that function is never called here. Ifrun_pluginwere changed to passupstream_base_urlas the advertised URL, this test would still pass.Extract the advertised-URL derivation so a test can cover it. For example, add a helper that maps a proxy
SocketAddrto the advertised base URL, call it fromrun_plugin, and assert on the helper's output. A test that callsspawn_auth_proxyand then asserts the derived advertised URL is loopback would close the gap directly.Also consider adding a case to
proxy_injects_bearer_key_and_strips_caller_authorizationwhereapi_keyisNone. That case must still return an emptyAuthorizationheader, which proves caller credentials are dropped independently of injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 305 - 329, The regression test bypasses run_plugin’s advertised-URL wiring and does not verify the derived value. Extract the advertised URL derivation used by run_plugin into a helper accepting the proxy SocketAddr, test that helper (ideally with spawn_auth_proxy) to assert a loopback URL, and add a None api_key case to proxy_injects_bearer_key_and_strips_caller_authorization that verifies the Authorization header remains empty.
185-201: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffLoopback binding limits network exposure, not local access.
Any local process or local user on the same host can send requests to
127.0.0.1:<port>and receive the injected bearer credential's privileges. The proxy performs no authentication of its own. On a single-tenant host this is acceptable. On a shared or multi-user host, this converts a file-protected key into an unauthenticated local service.Consider one of these, if the deployment target can be multi-tenant:
- Require a shared secret between the host runtime and the proxy.
- Bind a Unix domain socket with restrictive permissions instead of a TCP port, if the host runtime supports it.
Also confirm the key file's permissions are checked or documented, because the key's confidentiality now depends only on that file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 185 - 201, Update the local auth-proxy setup around ProxyState and the listener creation to prevent unauthenticated access by other local users: implement a shared-secret check between the host runtime and proxy, or use a restrictive-permission Unix domain socket where supported. Also verify or document secure permissions for the key file, and preserve the existing bearer-injection behavior for authenticated requests.
64-73: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDrop the remaining hop-by-hop and proxy-credential headers.
The filter omits
proxy-authorization,proxy-authenticate,keep-alive,te,trailer, andupgrade.proxy-authorizationis a caller-supplied credential. The same reason that justifies droppingauthorizationapplies to it. The other names are hop-by-hop per RFC 9110 and must not be forwarded end-to-end.♻️ Proposed filter extension
fn is_hop_by_hop_request_header(name: &str) -> bool { matches!( name, - "host" | "authorization" | "content-length" | "transfer-encoding" | "connection" + "host" + | "authorization" + | "proxy-authorization" + | "content-length" + | "transfer-encoding" + | "connection" + | "keep-alive" + | "te" + | "trailer" + | "upgrade" ) } fn is_hop_by_hop_response_header(name: &str) -> bool { - matches!(name, "content-length" | "transfer-encoding" | "connection") + matches!( + name, + "content-length" + | "transfer-encoding" + | "connection" + | "keep-alive" + | "proxy-authenticate" + | "trailer" + | "upgrade" + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 64 - 73, Update is_hop_by_hop_request_header to also filter proxy-authorization, proxy-authenticate, keep-alive, te, trailer, and upgrade, while preserving all existing request-header exclusions. Add the corresponding hop-by-hop names to is_hop_by_hop_response_header as appropriate, ensuring every listed RFC-defined hop-by-hop header is removed from forwarding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 136-141: In the response-header copy loop, replace the
`response_headers.insert` call with `HeaderMap::append` so repeated
non-hop-by-hop headers such as `set-cookie` and `www-authenticate` retain every
value while preserving the existing filtering logic.
- Around line 125-131: Update the upstream request error branch in the handler
to log the detailed reqwest error locally while returning a generic 502 response
message that does not expose the upstream URL or other error details. Apply the
same sanitization to the response-body error branch near the response handling
logic, preserving the existing BAD_GATEWAY status and response flow.
- Around line 112-124: Update the reqwest client construction in
spawn_auth_proxy to use ClientBuilder with an explicit connect_timeout and
read_timeout, ensuring the read timeout covers buffered upstream streaming
completion. Preserve the existing client behavior and reqwest 0.12.28-compatible
configuration.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 402-411: Update e2e_llama_server_answers_openai_requests to read
the API key file path from a dedicated environment variable instead of calling
api_key_from_args(). Use the environment-based configuration alongside
OPENAI_ENDPOINT_E2E and MESH_LLM_PLUGIN_URL, preserving the existing proxy setup
and error propagation.
- Around line 101-110: Update the request-body handling around
axum::body::to_bytes to use a fixed maximum byte limit instead of usize::MAX,
and change the corresponding error response from StatusCode::BAD_GATEWAY to
StatusCode::BAD_REQUEST while preserving the existing error message.
- Around line 305-329: The regression test bypasses run_plugin’s advertised-URL
wiring and does not verify the derived value. Extract the advertised URL
derivation used by run_plugin into a helper accepting the proxy SocketAddr, test
that helper (ideally with spawn_auth_proxy) to assert a loopback URL, and add a
None api_key case to proxy_injects_bearer_key_and_strips_caller_authorization
that verifies the Authorization header remains empty.
- Around line 185-201: Update the local auth-proxy setup around ProxyState and
the listener creation to prevent unauthenticated access by other local users:
implement a shared-secret check between the host runtime and proxy, or use a
restrictive-permission Unix domain socket where supported. Also verify or
document secure permissions for the key file, and preserve the existing
bearer-injection behavior for authenticated requests.
- Around line 64-73: Update is_hop_by_hop_request_header to also filter
proxy-authorization, proxy-authenticate, keep-alive, te, trailer, and upgrade,
while preserving all existing request-header exclusions. Add the corresponding
hop-by-hop names to is_hop_by_hop_response_header as appropriate, ensuring every
listed RFC-defined hop-by-hop header is removed from forwarding.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 550b78e3-3c25-48d9-9dfd-c5a0309d3b11
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlsrc/lib.rs
- Set connect/read timeouts on the upstream reqwest client (spawn_auth_proxy used the bare default client, so a stalled upstream could hold the proxying task open indefinitely). read_timeout is a sliding per-read window, not a total-duration cap, so it won't cut off a slow-but-progressing streaming completion. - Stop returning raw reqwest errors to the caller: their Display output commonly includes the real upstream host/scheme, which is exactly what this proxy exists to keep out of anything the mesh can see. Log locally, return a generic message, for both the upstream-request-send and response-body-read error paths. - HeaderMap::insert collapses repeated response headers (e.g. set-cookie) to their last value; use append to preserve every value. - Bound the caller's request body to 64MB instead of usize::MAX (the listener is loopback-only, so exposure is limited, but an unbounded buffer is still a needless memory-exhaustion footgun), and map that specific failure to 400 rather than 502 — it happens while reading the caller's own body, before any upstream call. - Extend the RFC 7230 hop-by-hop header filter with the entries it was missing (proxy-authorization, proxy-authenticate, keep-alive, te, trailer, upgrade). - Extract advertised_base_url_for_proxy() out of run_plugin() and add a test that exercises it against a real spawn_auth_proxy() address — the existing loopback regression test only proved build_plugin() echoes back whatever advertised_base_url string it's given, never touching run_plugin()'s actual derivation. - Add a no-api-key coverage case alongside the existing bearer-key-is-injected test, asserting no Authorization header reaches upstream when none is configured. Not addressed, with reasons: - Reading the API key file path from test-harness args in the e2e test (CodeRabbit nitpick, "Trivial | Low value") — real but minor, opt-in test-only fragility; skipping to keep this PR scoped to the actionable findings. - Adding a shared-secret or Unix-domain-socket authentication layer between the host runtime and this proxy (CodeRabbit nitpick) — the loopback-only local-privilege boundary this raises is a real, already-documented trade-off in the downstream consumer's own runbook (any other local user on the same machine can reach the proxy's ephemeral port), not something this bug-fix PR should expand scope to redesign. Worth a follow-up if that threat model changes. Signed-off-by: MahdiHedhli <16087011+MahdiHedhli@users.noreply.github.com>
|
Addressed all 3 actionable findings and 3 of 5 nitpicks from the CodeRabbit review in 8d41b94:
Not addressed, with reasons (skipping per the review's own "fix only still-valid issues, skip the rest with a brief reason" guidance):
All 7 unit tests pass (5 existing + 2 new), |
1 participant
Summary
This plugin currently has no auth support — it registers a bare
base_urland mesh-llm's shared forwarding code does pure header pass-through, so it can't be pointed at a server that requires a bearer key (e.g. a locked-down LM Studio, vLLM, or any real deployment that isn't wide open).Adds a local, loopback-only reverse proxy the plugin process hosts for its entire lifetime: instead of registering the real (possibly credentialed) upstream URL, it binds
127.0.0.1:0, injectsAuthorization: Bearer <key>into every proxied request, and advertises the loopback address to mesh-llm instead — so the key never appears in this process's manifest, in mesh gossip, or in any other pool member's view of "where this model is served."Changes
--api-key-file <path>(a file, not an env var or bare CLI arg — mesh-llm's plugin config has no generic env-passthrough channel to the child process, and a bare arg would show up inps//proc/<pid>/cmdline)./v1path-prefix fix: the proxy's own advertised base and a real upstream's configured base are both commonly/v1-suffixed (e.g. LM Studio) — naively concatenating the two produced broken URLs like.../v1/v1/models. Fixed with a strip-prefix step and a strengthened regression test (exact-route mock upstream instead of a wildcard fallback, so a reintroduced double-prefix bug 404s instead of silently passing).content-lengthas hop-by-hop, which forcesTransfer-Encoding: chunkedfor every response including small non-streaming JSON completions. mesh-llm-host-runtime's non-streaming JSON relay path reads exactlycontent_lengthbytes assuming that's the whole body, so chunked framing got parsed as part of the JSON payload ("trailing characters" error). Fixed by buffering the response and building it from known-size bytes. Note: this means a truestream: trueSSE completion is currently buffered in full before reaching the caller rather than forwarded token-by-token — flagged as a follow-up, not solved by this PR.advertised_endpoint_is_always_loopback_never_real_upstream— the actual security property this PR exists to buy. Whatever the real (possibly credentialed) upstream URL is, the manifest this plugin advertises to the mesh pool must always be a loopback address.Verification
All fixes were found and confirmed via live end-to-end testing against a real external LM Studio server through mesh-llm's Serve mode: model discovery and a real chat completion both round-trip correctly through this proxy.
Test plan
cargo test— all unit tests pass, including the loopback-only manifest regression test and the exact-route double-prefix regression testSummary by CodeRabbit
502 Bad Gatewayresponse.