fix(http): refuse HTTPS→HTTP redirect downgrades - #3
Conversation
When the HTTP server returns a response without a Content-Type header, the transport now defaults to treating the body as plain text instead of failing. This improves compatibility with servers that omit the header for simple text responses. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the HTTP transport receives a request without a Content-Type header, the server now defaults to treating the body as JSON instead of failing. This improves compatibility with clients that omit the header when sending JSON payloads. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the HTTP transport receives a response without a Content-Type header, it now defaults to treating the body as plain text instead of failing. This improves robustness when interacting with servers that omit the header. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the HTTP transport receives a request without a Content-Type header, the server now defaults to treating the body as JSON instead of failing. This improves compatibility with clients that omit the header when sending JSON payloads. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the HTTP transport receives a request without a Content-Type header, the server now defaults to treating the body as JSON instead of returning an error. This improves compatibility with clients that omit the header while sending JSON payloads. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe HTTP transport now uses a custom redirect policy. It follows up to five redirects, rejects HTTPS-to-HTTP downgrades, and tests secure, HTTP, downgrade, and excessive redirect scenarios. ChangesHTTP redirect security
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The client now rejects HTTPS-to-HTTP redirects while retaining the existing redirect limit, preventing credential exposure over plaintext without changing the public API. No actionable merge-blocking risk remains; the PR is merge-ready after normal checks. Sequence Diagram(s)sequenceDiagram
participant McpHttpClientBuilder
participant reqwest HTTP client
participant redirect policy
participant redirect target
McpHttpClientBuilder->>reqwest HTTP client: Configure custom redirect policy
reqwest HTTP client->>redirect target: Send request
redirect target-->>reqwest HTTP client: Return redirect
reqwest HTTP client->>redirect policy: Evaluate target and hop count
redirect policy-->>reqwest HTTP client: Follow redirect or return error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows1 changed behaviour across 2 relationships. 2 surrounding behaviours are shown (60 graph nodes walked). 50 further behaviours left out to keep the diagram readable. flowchart LR
n0["McpHttpClientBuilder<br/>changed"]:::changed
n1["build"]:::impacted
n2["builder"]:::impacted
n1 -->|calls| n2
n2 -->|uses| n0
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
The HTTP transport client followed every redirect up to
MAX_REDIRECTSviareqwest::redirect::Policy::limited. That policy follows an HTTPS→HTTPdowngrade without complaint, and
reqwestonly stripsAuthorization/Cookieon a cross-origin hop — so a same-host downgrade carries a bearerin cleartext, and any custom-header or query-parameter credential follows on
any hop. This is a CWE-319 (cleartext transmission of sensitive information)
gap: a passive network attacker who can inject a 3xx can move a credentialed
request to plaintext.
This replaces the limited policy with a custom one that:
MAX_REDIRECTS(unchanged behaviour), andthan a silent leak.
The decision is a pure function (
redirect_decision) of the origin scheme,target scheme, and hop count, so the rule is unit-testable without standing up
a redirect server.
Scope
This is the per-request client path (
McpHttpClient). The dial-timeresolution path (
registry::connections::dial::credential_safe_dial_url)already refuses HTTPS→HTTP downgrades by falling back to the original URL —
this closes the complementary gap on the authenticated request path. The two
are now consistent: a downgrade is refused at resolution and at request time.
Public API / behavior changes
None to the public surface. A request that would have followed an
HTTPS→HTTP downgrade now errors instead of silently downgrading — the
intended behaviour.
Validation
The four contract commands, run from the repo root:
cargo fmt --all -- --check— cleancargo clippy --all-targets --all-features -- -D warnings— clean on thechanged code. (One pre-existing
unknown-lintwarning forclippy::unused_async_trait_implintinybus_module/service.rssurfacesonly on a clippy newer than the
@stableCI pins; it is untouched by thisPR and present on
main.)cargo build --all-targets --all-features— greencargo test --all-features— 661 lib + 4 + 149 + 10 + 18, all pass; 0failed. New tests:
follows_an_https_to_https_redirectrefuses_an_https_to_http_downgradedoes_not_refuse_a_plain_http_redirect_that_started_on_httpcaps_the_redirect_chain_at_max_redirectsRelated
Surfaced as a security finding on openhuman PR tinyhumansai/openhuman#5671
(MCP client/registry extraction into this crate).
Summary by CodeRabbit