Skip to content

fix(http): centralize outbound hop-by-hop sanitization - #887

Open
mkoushni wants to merge 13 commits into
praxis-proxy:mainfrom
mkoushni:fix/669-mcp-hop-by-hop-headers
Open

fix(http): centralize outbound hop-by-hop sanitization#887
mkoushni wants to merge 13 commits into
praxis-proxy:mainfrom
mkoushni:fix/669-mcp-hop-by-hop-headers

Conversation

@mkoushni

@mkoushni mkoushni commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • is_blocked_mcp_header already dropped Connection but left Keep-Alive and Proxy-Connection, so client-supplied MCP tool headers survived into the outbound HTTP/1 transport.
  • Hop-by-hop names now live in one shared predicate (praxis_ai_apis::http_hop::is_hop_by_hop). MCP, OpenAI forward_headers, file-search replay, experimental http_callout, and fixture replay all delegate to it so those lists cannot drift again. MCP also blocks proxy-authenticate for parity.
  • Outbound copies strip every field named by a Connection token list before dropping Connection itself, so {"connection":"x-smuggle","x-smuggle":"secret"} cannot smuggle a header onto MCP, API callouts, or http_callout.

Fixes #669

Test plan

  • cargo test -p praxis-ai-apis --lib hop_by_hop
  • cargo test -p praxis-ai-apis --lib connection_nominat
  • cargo test -p praxis-ai-apis --lib keep_alive_and_proxy
  • cargo test -p praxis-ai-filters --features http-callout-filter --lib hop_by_hop_forward
  • cargo test -p praxis-ai-filters --features http-callout-filter --lib connection_nominated_forward
  • make lint

…rs (praxis-proxy#669)

is_blocked_mcp_header already dropped Connection but left the Keep-Alive
field and de-facto Proxy-Connection hop-by-hop header, so client-supplied
MCP tool headers could reach the outbound HTTP/1 transport.

Signed-off-by: mkoushni <mkoushni@redhat.com>
@mkoushni
mkoushni requested review from a team and nerdalert September 2, 2026 12:01

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Centralize outbound hop-by-hop sanitization" from 669 is not done nor is "Connection token-list expansion"

@mkoushni

mkoushni commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@leseb i choosing option 2 - to complete full implementation

)

Share one hop-by-hop predicate across MCP, API callouts, http_callout,
and fixture replay, and strip every field named by Connection before
dropping Connection itself so header lists cannot drift.

Signed-off-by: mkoushni <mkoushni@redhat.com>
@mkoushni mkoushni changed the title fix(mcp_client): strip Keep-Alive and Proxy-Connection from MCP headers fix(http): centralize outbound hop-by-hop sanitization Sep 3, 2026
Signed-off-by: mkoushni <mkoushni@redhat.com>
Praxis core main gated admin endpoints behind praxis-protocol's
admin-api feature. test-praxis-main patches that tree in, so the
praxis-main feature must turn the admin API on or the server crate
does not compile.

Signed-off-by: mkoushni <mkoushni@redhat.com>
Cargo rejects praxis-protocol/admin-api while the lockfile is still
0.5.3, so lint and unit tests fail before patch-praxis runs.
test-praxis-main stays a known core-compat break until 0.5.4.

Signed-off-by: mkoushni <mkoushni@redhat.com>

@praxis-bot praxis-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praxis-bot review: fix(http): centralize outbound hop-by-hop sanitization

Purpose

Consolidates five independent hop-by-hop header lists into a single http_hop module and adds Connection-nominated header stripping across all outbound header-copy paths (MCP, OpenAI forward headers, file-search replay, http_callout). Closes the {"connection":"x-smuggle","x-smuggle":"secret"} smuggling vector.

Assessment

No actionable findings. This is a clean, well-tested security hardening PR.

Correctness:

  • is_hop_by_hop covers the full RFC 9110 Section 7.6.1 set plus obsolete Proxy-Connection -- correct.
  • connection_nominates_header fails closed on non-text values (filter_map with to_str().ok()) -- correct security posture.
  • MCP path uses connection_nominated_from_json (pre-computed HashSet before the filter loop) rather than connection_nominates_header because headers are JSON, not HeaderMap -- correct adaptation.
  • is_blocked_mcp_header removes CONNECTION, PROXY_AUTHORIZATION, TE, TRAILER, TRANSFER_ENCODING, UPGRADE, keep-alive, proxy-connection from its explicit checks since all are now covered by the is_hop_by_hop delegation. The remaining matches (AUTHORIZATION, CONTENT_LENGTH, COOKIE, FORWARDED, HOST, SET_COOKIE) are end-to-end headers that belong in the MCP-specific denylist -- correct split.
  • HTTP callout is_disallowed_forward_header is a strict superset of the old DISALLOWED_FORWARD_HEADERS array (adds keep-alive, proxy-authenticate, proxy-connection, te which were previously missing) -- correct hardening.
  • should_replay_original_header in file-search callout correctly delegates hop-by-hop checking to is_hop_by_hop and keeps the body-rewrite-stale headers (accept-encoding, content-encoding, content-length, content-md5, signature, signature-input) as the residual check.

Test coverage:

  • New http_hop module has unit tests for all four public functions.
  • MCP: three new tests covering keep-alive/proxy-connection stripping, Connection-nominated smuggling, and proxy-authenticate parity.
  • OpenAI API client: forward_headers_skips_connection_nominated_fields test.
  • HTTP callout: two integration tests (hop_by_hop_forward_headers_not_sent, connection_nominated_forward_header_not_sent) that exercise the full filter pipeline via MockServer.
  • File-search: proxy-connection added to the existing exclusion test.
  • Inference fixture header policy updated to delegate, maintaining fixture validation consistency.

Conventions: License header (Apache-2.0) matches peer files in the ai repo. Module ordering in lib.rs is alphabetical. Doc comments on all public items with doctests on is_hop_by_hop.

Severity Count
Critical 0
Large 0
Medium 0

Clean PR. No changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP custom-header filtering misses additional hop-by-hop headers

3 participants