fix(auth): Fix unnecesary header sanitization in tansport #137
Conversation
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Runner
participant Client
participant Transport as HTTP/SSE/StreamableHTTP
CLI->>Client: Create with client_args (safety_enabled)
Client->>Client: Extract safety_enabled from config
Client->>Transport: __init__(safety_enabled=True)
Transport->>Transport: Store self.safety_enabled
Note over Transport: When sending request:
Transport->>Transport: _prepare_headers_with_auth(headers)
alt safety_enabled is True
Transport->>Transport: Sanitize headers via _prepare_safe_headers
end
Transport->>Transport: Merge sanitized headers with auth_headers
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_fuzzer/transport/http.py (1)
65-83: Critical: Method defined inside__init__breaks code structure.The
_prepare_headers_with_authmethod is incorrectly placed inside the__init__method (lines 65-73), causing the subsequent__init__code (lines 75-83) to reference undefined variables liketimeout. This structural error explains the pipeline failures (F821: Undefined nametimeout).Apply this diff to move the method definition outside
__init__:self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None} # Track last activity for process management self._last_activity = time.time() - -def _prepare_headers_with_auth(self, headers: dict[str, str]) -> dict[str, str]: - """Prepare headers with optional safety sanitization and auth headers.""" - if self.safety_enabled: - safe_headers = self._prepare_safe_headers(headers) - else: - safe_headers = headers.copy() - # Add auth headers after sanitization (they are user-configured and safe) - safe_headers.update(self.auth_headers) - return safe_headers - + # Initialize process manager for any subprocesses (like proxy servers) watchdog_config = WatchdogConfig( check_interval=1.0, process_timeout=timeout, extra_buffer=5.0, max_hang_time=timeout + 10.0, auto_kill=True, ) self.process_manager = ProcessManager(watchdog_config) + +def _prepare_headers_with_auth(self, headers: dict[str, str]) -> dict[str, str]: + """Prepare headers with optional safety sanitization and auth headers.""" + if self.safety_enabled: + safe_headers = self._prepare_safe_headers(headers) + else: + safe_headers = headers.copy() + # Add auth headers after sanitization (they are user-configured and safe) + safe_headers.update(self.auth_headers) + return safe_headers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
mcp_fuzzer/cli/runner.py(1 hunks)mcp_fuzzer/client/__init__.py(1 hunks)mcp_fuzzer/transport/http.py(5 hunks)mcp_fuzzer/transport/sse.py(4 hunks)mcp_fuzzer/transport/streamable_http.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: examples/auth_config.json:3-6
Timestamp: 2025-08-08T11:47:21.915Z
Learning: In repo Agent-Hellboy/mcp-server-fuzzer, the file examples/auth_config.json uses "api_key": "secret123" intentionally for the dummy example server. Do not flag this as a leaked secret in future reviews; maintainers prefer keeping it unchanged. If needed, suggest adding a README note rather than changing the example value.
Learnt from: Agent-Hellboy
Repo: Agent-Hellboy/mcp-server-fuzzer PR: 25
File: mcp_fuzzer/strategy/tool_strategies.py:105-139
Timestamp: 2025-08-08T11:50:01.877Z
Learning: Repo Agent-Hellboy/mcp-server-fuzzer: In mcp_fuzzer/strategy/tool_strategies.py, ToolStrategies._generate_aggressive_integer can return None in the "special" branch despite an int annotation. Maintainership decision: ignore for now and handle in issue #18; do not re-flag this as blocking in interim reviews.
🧬 Code graph analysis (5)
mcp_fuzzer/client/__init__.py (1)
mcp_fuzzer/config/manager.py (1)
get(39-41)
mcp_fuzzer/cli/runner.py (1)
mcp_fuzzer/config/manager.py (1)
get(39-41)
mcp_fuzzer/transport/http.py (4)
mcp_fuzzer/transport/sse.py (1)
_prepare_headers_with_auth(28-36)mcp_fuzzer/transport/streamable_http.py (1)
_prepare_headers_with_auth(67-80)mcp_fuzzer/transport/mixins.py (1)
_prepare_safe_headers(280-289)mcp_fuzzer/config/manager.py (1)
update(47-49)
mcp_fuzzer/transport/sse.py (3)
mcp_fuzzer/transport/http.py (1)
_prepare_headers_with_auth(65-83)mcp_fuzzer/transport/streamable_http.py (1)
_prepare_headers_with_auth(67-80)mcp_fuzzer/safety_system/policy.py (1)
sanitize_headers(158-166)
mcp_fuzzer/transport/streamable_http.py (2)
mcp_fuzzer/transport/http.py (1)
_prepare_headers_with_auth(65-83)mcp_fuzzer/transport/sse.py (1)
_prepare_headers_with_auth(28-36)
🪛 GitHub Actions: Lint
mcp_fuzzer/transport/http.py
[error] 60-60: E501 Line too long (92 > 88)
[error] 78-78: F821 Undefined name timeout
[error] 80-80: F821 Undefined name timeout
mcp_fuzzer/transport/sse.py
[error] 26-26: E501 Line too long (92 > 88)
mcp_fuzzer/transport/streamable_http.py
[error] 30-30: F401: 'safety_system.policy.sanitize_headers' imported but unused.
[error] 63-63: E501 Line too long (92 > 88)
🔇 Additional comments (2)
mcp_fuzzer/client/__init__.py (1)
59-59: LGTM!The safety_enabled parameter is correctly extracted from config with a sensible default of True and properly propagated to transport creation.
mcp_fuzzer/transport/sse.py (1)
28-36: LGTM!The
_prepare_headers_with_authimplementation correctly callssanitize_headersfor safety sanitization and properly merges authentication headers afterward.
|
|
||
| # Get safety settings | ||
| safety_enabled = client_args.get("safety_enabled", True) | ||
| if safety_enabled: | ||
| factory_kwargs["safety_enabled"] = safety_enabled |
There was a problem hiding this comment.
Fix the conditional logic for safety_enabled.
The current implementation only adds safety_enabled to factory_kwargs when it's True. If a user explicitly sets safety_enabled=False, it won't be passed to the transport, and the transport will default to True, ignoring the user's preference.
Apply this diff to always pass the safety_enabled value:
factory_kwargs = {"timeout": args.timeout}
-
- # Get safety settings
- safety_enabled = client_args.get("safety_enabled", True)
- if safety_enabled:
- factory_kwargs["safety_enabled"] = safety_enabled
-
+
+ # Pass safety_enabled to transport (defaults to True if not provided)
+ factory_kwargs["safety_enabled"] = client_args.get("safety_enabled", True)
+ 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get safety settings | |
| safety_enabled = client_args.get("safety_enabled", True) | |
| if safety_enabled: | |
| factory_kwargs["safety_enabled"] = safety_enabled | |
| factory_kwargs = {"timeout": args.timeout} | |
| # Pass safety_enabled to transport (defaults to True if not provided) | |
| factory_kwargs["safety_enabled"] = client_args.get("safety_enabled", True) | |
🤖 Prompt for AI Agents
In mcp_fuzzer/cli/runner.py around lines 50 to 54, the code only sets
factory_kwargs["safety_enabled"] when safety_enabled is True, so an explicit
False is ignored; always assign the retrieved safety_enabled value into
factory_kwargs (i.e., remove the conditional and unconditionally set
factory_kwargs["safety_enabled"] = safety_enabled) so the transport receives the
user's preference.
| } | ||
| if auth_headers: | ||
| self.headers.update(auth_headers) | ||
| self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None} |
There was a problem hiding this comment.
Line exceeds maximum length.
Line 60 exceeds the project's 88-character limit (92 characters). Consider breaking it into multiple lines for better readability.
Apply this diff:
- self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None}
+ self.auth_headers = {
+ k: v for k, v in (auth_headers or {}).items() if v is not None
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None} | |
| self.auth_headers = { | |
| k: v for k, v in (auth_headers or {}).items() if v is not None | |
| } |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 60-60: E501 Line too long (92 > 88)
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/http.py around line 60, the dict comprehension creating
self.auth_headers exceeds the 88-character line limit; break the expression
across multiple lines to stay within the limit while preserving behavior — e.g.,
compute the source mapping on one line (src = auth_headers or {}), iterate or
build the dict comprehension on the next line(s), or split the comprehension
into a multiline dict(...) or a simple for-loop that assigns self.auth_headers =
{ ... } across lines; ensure the filter "if v is not None" remains and no
semantic changes are made.
| } | ||
| if auth_headers: | ||
| self.headers.update(auth_headers) | ||
| self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None} |
There was a problem hiding this comment.
Line exceeds maximum length.
Line 26 exceeds the project's 88-character limit (92 characters).
Apply this diff:
- self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None}
+ self.auth_headers = {
+ k: v for k, v in (auth_headers or {}).items() if v is not None
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 26-26: E501 Line too long (92 > 88)
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/sse.py around line 26, the dict comprehension assigning
self.auth_headers is longer than the 88-character limit; break the expression
across multiple lines so no source line exceeds 88 chars. Split the
comprehension into a multi-line expression (for example, put the opening brace
on the first line, place the k: v for k, v ... clause on the next line, and the
closing brace on its own line) or build the dict using a short call (e.g.,
dict(...) or a small helper) spread across lines to satisfy the line-length rule
while preserving the same filtering logic that excludes None values.
| } | ||
| if auth_headers: | ||
| self.headers.update(auth_headers) | ||
| self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None} |
There was a problem hiding this comment.
Line exceeds maximum length.
Line 63 exceeds the project's 88-character limit (92 characters).
Apply this diff:
- self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None}
+ self.auth_headers = {
+ k: v for k, v in (auth_headers or {}).items() if v is not None
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.auth_headers = {k: v for k, v in (auth_headers or {}).items() if v is not None} | |
| self.auth_headers = { | |
| k: v for k, v in (auth_headers or {}).items() if v is not None | |
| } |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 63-63: E501 Line too long (92 > 88)
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/streamable_http.py around line 63, the dict
comprehension assignment for self.auth_headers exceeds the 88-character line
length limit; split the expression across multiple lines (for example assign the
source dict to a short local variable or break the dict comprehension into a
multi-line comprehension with each k, v pair on its own line) so the resulting
lines are under 88 characters while preserving the same behavior filtering out
None values.
| def _prepare_headers_with_auth(self, headers: dict[str, str]) -> dict[str, str]: | ||
| """Prepare headers with optional safety sanitization and auth headers.""" | ||
| if self.safety_enabled: | ||
| safe_headers = self._prepare_headers_with_auth(headers) | ||
| else: | ||
| safe_headers = headers.copy() | ||
| # Add auth headers after sanitization (they are user-configured and safe) | ||
| safe_headers.update(self.auth_headers) | ||
| return safe_headers |
There was a problem hiding this comment.
Critical: Infinite recursion in _prepare_headers_with_auth.
Line 70 calls self._prepare_headers_with_auth(headers) recursively within the same method when safety_enabled is True, causing infinite recursion and a stack overflow.
Apply this diff to call sanitize_headers instead (matching the pattern in sse.py and http.py):
def _prepare_headers_with_auth(self, headers: dict[str, str]) -> dict[str, str]:
"""Prepare headers with optional safety sanitization and auth headers."""
if self.safety_enabled:
- safe_headers = self._prepare_headers_with_auth(headers)
+ safe_headers = sanitize_headers(headers)
else:
safe_headers = headers.copy()
# Add auth headers after sanitization (they are user-configured and safe)
safe_headers.update(self.auth_headers)
return safe_headers
- self.session_id: str | None = None
- self.protocol_version: str | None = None
- self._initialized: bool = False
- self._init_lock: asyncio.Lock = asyncio.Lock()
- self._initializing: bool = FalseNote: The code snippet also shows lines 76-80 that appear to be misplaced (they should be in __init__). Please verify the file structure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _prepare_headers_with_auth(self, headers: dict[str, str]) -> dict[str, str]: | |
| """Prepare headers with optional safety sanitization and auth headers.""" | |
| if self.safety_enabled: | |
| safe_headers = self._prepare_headers_with_auth(headers) | |
| else: | |
| safe_headers = headers.copy() | |
| # Add auth headers after sanitization (they are user-configured and safe) | |
| safe_headers.update(self.auth_headers) | |
| return safe_headers | |
| def _prepare_headers_with_auth(self, headers: dict[str, str]) -> dict[str, str]: | |
| """Prepare headers with optional safety sanitization and auth headers.""" | |
| if self.safety_enabled: | |
| safe_headers = sanitize_headers(headers) | |
| else: | |
| safe_headers = headers.copy() | |
| # Add auth headers after sanitization (they are user-configured and safe) | |
| safe_headers.update(self.auth_headers) | |
| return safe_headers |
🤖 Prompt for AI Agents
In mcp_fuzzer/transport/streamable_http.py around lines 67 to 75, the method
_prepare_headers_with_auth currently calls itself when self.safety_enabled is
True which causes infinite recursion; replace that recursive call with a call to
the header sanitizer (e.g., self.sanitize_headers(headers)) so headers are
sanitized instead of recursing, then update safe_headers accordingly and merge
auth headers as before; additionally inspect lines ~76-80 which look like
constructor-only initialization and move any misplaced initialization into
__init__ so file structure and class initialization are correct.
No description provided.