fix(mesh): strip credentials before mesh peer forwarding - #1189
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPeer forwarding now validates and reconstructs complete raw requests, removes caller credentials, filters hop-by-hop headers, preserves supported bodies, and rejects malformed headers. Request parsing uses the shared finalizer. Remote routing separates tunnel setup, sanitized forwarding, and response handling. ChangesPeer forwarding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteRouting
participant forward_peer_request
participant prepare_peer_forwarded_request
participant PeerTunnel
participant ResponseHandling
RemoteRouting->>forward_peer_request: Forward prefetched request
forward_peer_request->>prepare_peer_forwarded_request: Reparse and remove caller credentials
prepare_peer_forwarded_request-->>forward_peer_request: Return sanitized request
forward_peer_request->>PeerTunnel: Write sanitized request
PeerTunnel-->>RemoteRouting: Return forwarding result
RemoteRouting->>ResponseHandling: Handle response after successful forwarding
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
c9aa7c9 to
0f61a0b
Compare
ndizazzo
changed the title
ndizazzo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs (1)
378-398: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove headers nominated by
Connection.Line 380 removes
Connectionbut does not remove its nominated fields. If a client sendsConnection: X-Private-ModeandX-Private-Mode: enabled, line 397 forwardsX-Private-Modeto the remote peer.Parse the comma-separated
Connectionvalue before rebuilding headers. Skip every nominated header name. Add a regression test for this case.🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs` around lines 378 - 398, Update the header-rebuilding logic around the request parsing function to parse each comma-separated token from the incoming Connection header and add those names to the omitted-header set. Ensure the existing filtering skips both Connection and every nominated header, including case-insensitive matches such as X-Private-Mode, and add a regression test covering a nominated header being excluded from the rebuilt request.
🤖 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.
Outside diff comments:
In `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs`:
- Around line 378-398: Update the header-rebuilding logic around the request
parsing function to parse each comma-separated token from the incoming
Connection header and add those names to the omitted-header set. Ensure the
existing filtering skips both Connection and every nominated header, including
case-insensitive matches such as X-Private-Mode, and add a regression test
covering a nominated header being excluded from the rebuilt request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a96dcaa0-401c-4736-b76c-8f20f73d77f8
📒 Files selected for processing (2)
crates/mesh-llm-host-runtime/src/network/openai/request_parse.rscrates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
185d546 to
7ab25f6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs`:
- Around line 397-405: Update the request parsing logic around the
connection-nominated header handling to reject requests where Connection
nominates Content-Length or Transfer-Encoding, before removing headers or
forwarding the original wire body. Preserve existing handling for other
nominated headers, and add regression coverage for both nominated framing-header
cases.
- Line 383: Update the header parsing logic in request parsing to preserve
ordinary field values as raw bytes instead of converting invalid UTF-8 to an
empty string. Validate the Connection header as an ASCII token list and reject
the entire value when malformed, including invalid bytes, rather than partially
filtering it. Add tests covering opaque non-UTF-8 field-value preservation and
malformed Connection values.
🪄 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: ab5dc7dc-7d34-4efe-92c9-9a16f0b1abff
📒 Files selected for processing (2)
crates/mesh-llm-host-runtime/src/network/openai/request_parse.rscrates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs (3)
389-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the token validation loop.
valueis already a validated&strafter line 389, so thestd::str::from_utf8(h.value).unwrap_or("<binary>")calls at lines 397 and 404 are redundant. The ASCII check and the control-character check also iterate the same bytes twice. One combined predicate reads more directly.Note that the current predicate accepts non-token ASCII characters such as
",{, and,-adjacent whitespace beyondtrim. That is acceptable here because the tokens are only compared against header names, but atcharcheck would match RFC 7230 exactly.♻️ Proposed refactor
for tok in value.split(',') { let t = tok.trim(); - if !t.is_empty() && !t.as_bytes().iter().all(|&b| b.is_ascii()) { - bail!( - "Connection header contains non-ASCII token '{}'; reject as malformed", - std::str::from_utf8(h.value).unwrap_or("<binary>") - ); - } - - if !t.is_empty() && t.as_bytes().iter().any(|&b| b <= 0x1F || b == 0x7F) { - bail!( - "Connection header contains control character in '{}'; reject as malformed", - std::str::from_utf8(h.value).unwrap_or("<binary>") - ); - } - - if !t.is_empty() { - connection_nominated.push(t.to_lowercase()); + if t.is_empty() { + continue; } + if !t.bytes().all(|b| b.is_ascii_graphic()) { + bail!( + "Connection header value '{value}' contains an invalid token; reject as malformed" + ); + } + connection_nominated.push(t.to_lowercase()); }🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs` around lines 389 - 411, In the token-validation loop around value, replace the redundant from_utf8(...).unwrap_or(...) calls with the already validated value, and combine the ASCII and control-character checks into one byte predicate. Preserve the current accepted-character behavior and error semantics; do not add stricter tchar validation.
1596-1605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the non-ASCII token branch.
\xFFis not valid UTF-8, so this input fails at line 389, not at the ASCII token check on line 394. No test currently reaches line 394. Add a Connection value that is valid UTF-8 but contains a non-ASCII character.💚 Proposed additional test
/// Regression: a Connection token that is valid UTF-8 but not ASCII is /// invalid HTTP/1.x token syntax and must be rejected. #[test] fn finalize_rejects_connection_non_ascii_token() { let raw = "GET / HTTP/1.1\r\nHost: x\r\nConnection: keep-alivé\r\nContent-Length: 0\r\n\r\n"; let result = finalize_forwarded_request(raw.as_bytes(), false, None, None, &[]); assert!( result.is_err(), "non-ASCII Connection token must be rejected, got: {result:?}" ); }🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs` around lines 1596 - 1605, Add a separate regression test near finalize_rejects_malformed_connection_value named finalize_rejects_connection_non_ascii_token, using valid UTF-8 in the Connection header with a non-ASCII character in the token. Pass the string bytes to finalize_forwarded_request and assert the result is an error, ensuring the non-ASCII token validation branch is exercised rather than the invalid-UTF-8 path.
432-434: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the per-header
to_lowercaseallocation.
name.to_lowercase()allocates aStringfor every header on each iteration of the inner loop.connection_nominatedalready holds lowercase tokens, soeq_ignore_ascii_casegives the same result without allocation.♻️ Proposed refactor
let is_connection_nominated = connection_nominated .iter() - .any(|n| n == &name.to_lowercase()); + .any(|n| n.eq_ignore_ascii_case(name));🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs` around lines 432 - 434, Update the `is_connection_nominated` check to compare each `connection_nominated` token with `name` using `eq_ignore_ascii_case`, removing the per-header `to_lowercase()` allocation while preserving case-insensitive matching against the existing lowercase tokens.
🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs`:
- Around line 376-428: Move finalize_forwarded_request,
prepare_peer_forwarded_request, the inline tests module, and related forwarding
logic from request_parse.rs into a semantically named
network/openai/forwarded_request.rs module. Extract the Connection-header
parsing and rejected framing-nomination checks into clearly named helpers there,
then update module wiring and call sites so behavior remains unchanged. The
anchor site request_parse.rs:376-428 and sibling site request_parse.rs:452-470
both require removal or relocation into the new module.
- Around line 379-427: Replace every corrupted “ยง” character in the RFC 7230
section references within the Connection-header parsing comments and bail!
messages with the correct “§” character, including the references near
connection_nominated validation and both protocol-violation errors. Do not
change the parsing or rejection behavior.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs`:
- Around line 389-411: In the token-validation loop around value, replace the
redundant from_utf8(...).unwrap_or(...) calls with the already validated value,
and combine the ASCII and control-character checks into one byte predicate.
Preserve the current accepted-character behavior and error semantics; do not add
stricter tchar validation.
- Around line 1596-1605: Add a separate regression test near
finalize_rejects_malformed_connection_value named
finalize_rejects_connection_non_ascii_token, using valid UTF-8 in the Connection
header with a non-ASCII character in the token. Pass the string bytes to
finalize_forwarded_request and assert the result is an error, ensuring the
non-ASCII token validation branch is exercised rather than the invalid-UTF-8
path.
- Around line 432-434: Update the `is_connection_nominated` check to compare
each `connection_nominated` token with `name` using `eq_ignore_ascii_case`,
removing the per-header `to_lowercase()` allocation while preserving
case-insensitive matching against the existing lowercase tokens.
🪄 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: c5ace403-d1f1-46bd-9c72-767e42a099cd
📒 Files selected for processing (1)
crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/forwarded_request.rs`:
- Around line 45-53: Update the Connection-option validation in the forwarded
request parser to trim only SP and HTAB, then require every character to belong
to the complete HTTP tchar set, rejecting separators such as semicolons and
non-ASCII whitespace. Add regression tests covering invalid separators and
non-ASCII whitespace, using the existing parser/test symbols in
forwarded_request.rs.
🪄 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: d953c4a8-82fe-4679-ae80-5724fd044f4b
📒 Files selected for processing (4)
crates/mesh-llm-host-runtime/src/network/openai/forwarded_request.rscrates/mesh-llm-host-runtime/src/network/openai/mod.rscrates/mesh-llm-host-runtime/src/network/openai/request_parse.rscrates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
- crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs
f9d6868 to
267d9d5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mesh-llm-host-runtime/src/network/openai/forwarded_request.rs (1)
167-172: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider stripping
cookieat the peer boundary.
CALLER_CREDENTIAL_HEADERScovers the four API-key style headers. It does not covercookie. ACookieheader is also a caller credential and is forwarded to the remote peer unchanged. If your ingress can receive cookie-authenticated requests, addcookieto the list.🔒 Proposed change
const CALLER_CREDENTIAL_HEADERS: &[&str] = &[ "authorization", "proxy-authorization", "x-api-key", "api-key", + "cookie", ];🤖 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 `@crates/mesh-llm-host-runtime/src/network/openai/forwarded_request.rs` around lines 167 - 172, Update CALLER_CREDENTIAL_HEADERS to include the cookie header so cookie-based caller credentials are stripped before forwarding requests to the remote peer.
🤖 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.
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/network/openai/forwarded_request.rs`:
- Around line 167-172: Update CALLER_CREDENTIAL_HEADERS to include the cookie
header so cookie-based caller credentials are stripped before forwarding
requests to the remote peer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c4ce4e3-cffa-4d10-9ed3-d084d6b3e72e
📒 Files selected for processing (5)
crates/mesh-llm-host-runtime/src/network/openai/forwarded_request.rscrates/mesh-llm-host-runtime/src/network/openai/mod.rscrates/mesh-llm-host-runtime/src/network/openai/request_parse.rscrates/mesh-llm-host-runtime/src/network/openai/response.rscrates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/mesh-llm-host-runtime/src/network/openai/mod.rs
- crates/mesh-llm-host-runtime/src/network/openai/request_parse.rs
- crates/mesh-llm-host-runtime/src/network/openai/response/routing.rs
|
@coderabbitai why tag as blocker? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Mesh-LLM/mesh-llm"
pr_number="1189"
echo "== Top-level comments =="
gh api "repos/${repo}/issues/${pr_number}/comments?per_page=100" \
--paginate \
--jq '.[] | {user: .user.login, created_at, body}'
echo "== Review comments =="
gh api "repos/${repo}/pulls/${pr_number}/comments?per_page=100" \
--paginate \
--jq '.[] | {user: .user.login, path, line, side, created_at, body}'Length of output: 28172
The classifier likely selected it because this PR changes the authorization-header security boundary. The actual review findings were marked Major or Minor, and the major findings were addressed. The label is misleading in this context. You are interacting with an AI system. |
2 participants
Strip caller authorization headers at the remote peer boundary.
Closes project-loupe/audit-mesh-llm#322.
Tests: focused header tests; host-runtime and mesh checks.
Summary by CodeRabbit
Security
Reliability