Conversation
Summary by CodeRabbit
WalkthroughAdds prompt data source and MCP server configuration models, a prompt-context loading service with SSRF protections, wires these into the /bots join flow with LLM provider/model and speech-speed overrides, persists persona data to file for subprocess handoff, implements MCP client transports and runner tool wiring, and adds OpenAPI snapshot export, docs, and tests. ChangesPrompt Data and MCP Integration
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5f105cf to
6ebdae1
Compare
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 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 `@app/models.py`:
- Line 4: Update the new public models in app/models.py to use modern built-in
collection annotations instead of typing.Dict/typing.List, and replace Optional
with the PEP 604 form using | None. Apply this consistently across the affected
model definitions and any related type hints in the referenced model blocks,
while keeping Literal and Any where appropriate. Focus on the imports and the
model classes/functions that currently use Dict, List, or Optional so the
annotations match the project’s style and Ruff expectations.
- Around line 92-93: The URL validation in the prompt/MCP model helpers
currently allows both HTTP and HTTPS, which should be tightened to reject
cleartext URLs. Update the validation logic in the prompt data source and MCP
URL checks (the helper that normalizes and validates URLs in app/models.py) so
user-configured endpoints require https:// by default, and only allow http://
behind an explicit local-development exception flag or equivalent gate. Ensure
the existing ValueError message and any related validation paths consistently
reflect the new HTTPS-only requirement.
- Around line 67-70: The inline prompt field in the model schema is currently
unbounded, so very large requests can be accepted before
`prompt_data_token_limit` is checked. Add a `max_length` constraint to the
`text` field in `app.models` (the `Field` definition for `text`) or enforce an
upstream request-body size limit so oversized inline payloads are rejected
earlier.
In `@app/services/prompt_context.py`:
- Around line 53-57: The prompt context is leaking full source URLs into
LLM-visible text and persona metadata. Update _section_header() to display only
a redacted/omitted URL, and ensure source_record uses the same sanitized value
before handing data to subprocesses. Apply the same redaction flow anywhere else
the URL is propagated in the prompt context build path so presigned or tokenized
URLs are never exposed downstream.
- Around line 81-89: The SSRF guard in prompt_context.py is only validating the
URL before aiohttp opens the connection, but the actual outbound request in the
fetch flow still re-resolves the hostname and can bypass
`_validate_fetch_url()`. Update the request path around `_validate_fetch_url`,
`aiohttp.ClientSession`, and `session.get` so the validated address is bound to
the real connection, either by using a connector/resolver pinned to the resolved
IP or by connecting to the resolved IP while preserving the original Host
header. Ensure the fix is applied in the same fetch routine that reads `url`,
`headers`, and `timeout`.
In `@core/process.py`:
- Around line 48-57: The persona payload file written in the parent needs
cleanup if the subprocess never successfully takes ownership of it. Update the
spawn flow around the payload creation in process.py so the parent tracks the
`persona_data_path` and unlinks it when `Popen` raises or startup fails before
the child parses `--persona-data-file`, and ensure the early-startup path also
removes it. Use the existing payload-writing block and the subprocess
launch/hand-off logic as the anchor points so the cleanup is tied to the same
`persona_data_path`/`client_id` lifecycle.
- Around line 160-161: The final process cleanup in the kill path is swallowing
exceptions, so the last process.kill() failure is lost. Update the exception
handling in the cleanup logic around the process.kill() attempts in process.py
to catch the final failure explicitly and log it with the relevant
process/context instead of using a bare except that passes. Keep the existing
retry flow, but ensure the last failure is surfaced through the same process
handling method used in this cleanup block.
In `@env.example`:
- Around line 89-91: The MCP connection comment is inaccurate because it points
operators to a nonexistent mcp.servers[].env field and suggests stdio support
that the API rejects. Update the note in env.example to reflect the actual
MCPServerConfig options and supported transports, using the relevant MCP server
config terminology and transport names from the README/tests (for example,
mcp.servers[] and the public HTTP-based transports) so it no longer implies
stdio or per-server env support.
In `@README.md`:
- Around line 402-405: The snapshot field list in the README is missing the
`llm_provider` and `llm_model` `BotRequest` fields, so update the documentation
where the current `BotRequest` fields are described to include both of those
schema fields alongside `prompt_data_sources`, `prompt_data_token_limit`, `mcp`,
and `speech_speed`. Keep the existing service snapshot list unchanged and ensure
the wording around `BotRequest` matches the generated schema used by the
snapshot docs.
In `@scripts/meetingbaas.py`:
- Around line 458-467: setup_mcp_tools currently leaves partially opened MCP
clients dangling if manager.connect() throws before main() reaches its shutdown
path. Update setup_mcp_tools to catch failures around LiveMCPManager.connect and
ensure the manager is closed/disconnected on error before re-raising, so any
subprocess/HTTP resources are cleaned up. Use the existing setup_mcp_tools and
LiveMCPManager symbols to locate the setup and teardown logic, and make the
cleanup happen for both the connect failure path and the no-discovery path if
needed.
- Around line 4-6: The CLI parser is still using json.load/json.loads while the
module only imports json as jsonlib, so persona-data parsing will fail with
NameError. Update the relevant parsing logic in meetingbaas.py so the import and
all JSON call sites are consistent, either by restoring the json name or by
switching every existing json.* usage (including the persona handoff path) to
jsonlib.*.
- Around line 1157-1170: The persona data file cleanup in the
args.persona_data_file block should happen regardless of whether json.load
succeeds. Move the os.remove(args.persona_data_file) attempt into a
finally-style path around the open/json.load logic in meetingbaas.py so the file
is deleted after every read attempt, and keep the persona_name extraction from
persona_data["path"] in the existing success path.
- Around line 403-411: `build_mcp_tool_name()` can generate duplicate function
names, and `self._tools[function_name] = tool_ref` currently overwrites earlier
entries in `meetingbaas.py`. Update the tool-registration path around
`build_mcp_tool_name`, `LiveMCPTool`, and `self._tools` to detect collisions
before inserting; if a name already exists, either reject the tool with a clear
error or generate a unique disambiguated name and keep the schema/dispatch
mapping consistent. Make sure the registration logic and any exposed MCP schema
use the same final function_name for each tool.
In `@speaking-bot-openapi.json`:
- Around line 396-409: The speech_speed schema in speaking-bot-openapi.json is
wider than the runtime clamp in the speech speed resolution path, so requests
can validate and then be silently adjusted. Align the OpenAPI bounds with the
Cartesia clamp used in scripts/meetingbaas.py (the speech speed resolution
logic) by narrowing the schema, or alternatively update the clamp and docs
together so the API contract and runtime behavior match.
In `@tests/test_mcp_client.py`:
- Around line 108-123: The URL validation tests are relying on shared process
environment state, so they can behave incorrectly if MCP_ALLOW_PRIVATE_URLS is
already set. Update test_validate_mcp_http_url_blocks_localhost and
test_validate_mcp_http_url_allows_exact_private_url to explicitly isolate the
environment by clearing or patching MCP_ALLOW_PRIVATE_URLS around the
validate_mcp_http_url calls, while preserving the existing
MCP_ALLOWED_PRIVATE_URLS setup/restore logic.
In `@tests/test_models.py`:
- Around line 11-18: Duplicate the dynamic module loading setup used around
MCPServerConfig into a shared test helper so it is not repeated across test
files. Move the importlib.util.spec_from_file_location / module_from_spec /
exec_module logic into a common helper such as tests/_helpers.py or a conftest
fixture, and have tests/test_models.py and the matching code in
tests/test_prompt_context.py call that helper to load models.py once.
In `@utils/mcp_client.py`:
- Around line 126-147: Add defensive limits and timeouts to read_stdio_message
so a stalled or malicious MCP subprocess cannot block forever or allocate
unbounded memory. Wrap the readline()/readexactly() path in asyncio timeout
handling, validate the parsed Content-Length against a reasonable maximum before
calling readexactly(), and surface McpClientError with clear context from
read_stdio_message so callers of the stdio MCP client fail fast instead of
hanging.
- Around line 378-382: The subprocess shutdown in the process.wait() timeout
path can still hang if the MCP server ignores SIGTERM. Update the termination
flow in the wait_for block around process.terminate() so it waits for a short
grace period and then force-kills the process if it does not exit, using the
existing process handle in mcp_client.py. Make sure the cleanup logic in this
timeout branch cannot block indefinitely and that any forced kill still waits
for the process to fully exit.
- Around line 76-85: The SSRF guard in _is_private_ip currently only blocks
private/loopback/link-local/etc. but still allows other non-global ranges like
100.64.0.0/10. Update the predicate to reject any address that is not globally
routable, and keep the allowlist override path separate so outbound MCP URLs are
permitted only when the target IP is explicitly allowlisted. Ensure the fix is
applied in _is_private_ip and any caller that relies on it for URL validation.
- Around line 39-44: The MCP tool name generator can collapse different
server/tool pairs into the same truncated value, which then causes
LiveMCPManager to overwrite entries in _tools. Update build_mcp_tool_name to
preserve uniqueness when truncating, such as by adding a deterministic hash or
suffix derived from the original raw name, and ensure the returned name still
fits the 64-character limit. Keep the existing sanitization logic, but make the
final name collision-resistant so distinct MCP tools cannot resolve to the same
function_name.
- Around line 108-117: validate_mcp_http_url() currently checks the resolved
host once, but session.post(self.url) still lets aiohttp resolve the hostname
again, which can allow DNS rebinding after validation. Update the MCP HTTP
client path in McpClient to reuse the validated address at connect time by
adding a custom resolver/connector that pins the resolved IP, or tighten the URL
handling so only IP literals and an explicit allowlist are accepted. Keep the
fix centered around validate_mcp_http_url() and the code that builds the aiohttp
session/request.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 2efd214c-447e-43eb-950e-264ceb5e089a
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
README.mdapp/models.pyapp/routes.pyapp/services/prompt_context.pycore/process.pyenv.examplemeeting-baas-openapi-v1.jsonopenapi.jsonpyproject.tomlscripts/export_openapi.pyscripts/meetingbaas.pyspeaking-bot-openapi.jsontests/test_mcp_client.pytests/test_models.pytests/test_openapi_snapshot.pytests/test_prompt_context.pyutils/mcp_client.py
|
|
||
| from datetime import datetime | ||
| from typing import Any, Dict, List, Optional | ||
| from typing import Any, Dict, List, Literal, Optional |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use built-in collection types and | None annotations.
Ruff flags typing.Dict/typing.List, and the project guideline requires modern built-in annotations. Apply this consistently across the new public models.
Proposed cleanup
-from typing import Any, Dict, List, Literal, Optional
+from typing import Any, Literal
- text: Optional[str] = Field(
+ text: str | None = Field(
...
- headers: Optional[Dict[str, str]] = Field(
+ headers: dict[str, str] | None = Field(
...
- tools: Optional[List[str]] = Field(
+ tools: list[str] | None = Field(
...
- prompt_data_sources: Optional[List[PromptDataSource]] = Field(
+ prompt_data_sources: list[PromptDataSource] | None = Field(Also applies to: 67-79, 123-151, 187-192, 248-290
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 4-4: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 4-4: typing.List is deprecated, use list instead
(UP035)
🤖 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 `@app/models.py` at line 4, Update the new public models in app/models.py to
use modern built-in collection annotations instead of typing.Dict/typing.List,
and replace Optional with the PEP 604 form using | None. Apply this consistently
across the affected model definitions and any related type hints in the
referenced model blocks, while keeping Literal and Any where appropriate. Focus
on the imports and the model classes/functions that currently use Dict, List, or
Optional so the annotations match the project’s style and Ruff expectations.
Sources: Coding guidelines, Linters/SAST tools
| text: Optional[str] = Field( | ||
| None, | ||
| description="Inline context. Required when type is text.", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files app/models.py && printf '\n---\n' && sed -n '1,140p' app/models.py && printf '\n---\n' && rg -n "prompt_data_token_limit|max_length|request-size|body size|content-length|token_limit|inline context|type is text" -S .Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 9434
🏁 Script executed:
sed -n '240,320p' app/models.py && printf '\n---\n' && sed -n '1,260p' app/services/prompt_context.py && printf '\n---\n' && sed -n '200,260p' app/routes.py && printf '\n---\n' && rg -n "max_request|body_size|max_body|limit.*bytes|Content-Length|request size|request_size|client_max_body_size|limit_bytes" -S app README.md .Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 14926
🏁 Script executed:
rg -n "FastAPI\\(|uvicorn|nginx|client_max_body_size|max_body|request size|body size|Content-Length|limit.*bytes|prompt_data_token_limit|PROMPT_DATA_SOURCE_MAX_BYTES" -S . && printf '\n---\n' && git ls-files | rg '(^|/)(main|app|server|run|docker|nginx|compose|proxy|deploy|README|docs).*'Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 7548
Cap inline prompt text app/models.py:67-70 — text is unbounded here, and there’s no request-body limit elsewhere in this path, so a client can send a very large inline payload and pay the parse/validation cost before prompt_data_token_limit applies. Add a max_length here or enforce an upstream body-size cap.
🤖 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 `@app/models.py` around lines 67 - 70, The inline prompt field in the model
schema is currently unbounded, so very large requests can be accepted before
`prompt_data_token_limit` is checked. Add a `max_length` constraint to the
`text` field in `app.models` (the `Field` definition for `text`) or enforce an
upstream request-body size limit so oversized inline payloads are rejected
earlier.
| if not normalized.startswith(("http://", "https://")): | ||
| raise ValueError("prompt data source url must start with http:// or https://") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not allow cleartext HTTP for user-configured prompt and MCP URLs.
These URLs can be fetched with caller-supplied headers or MCP credentials, so accepting http:// risks transmitting secrets and prompt data in cleartext. Prefer requiring https://, with any local-dev exception gated explicitly.
Proposed hardening
- if not normalized.startswith(("http://", "https://")):
- raise ValueError("prompt data source url must start with http:// or https://")
+ if not normalized.startswith("https://"):
+ raise ValueError("prompt data source url must start with https://")
...
- if not normalized.startswith(("http://", "https://")):
- raise ValueError("mcp server url must start with http:// or https://")
+ if not normalized.startswith("https://"):
+ raise ValueError("mcp server url must start with https://")Also applies to: 163-164
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 93-93: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 `@app/models.py` around lines 92 - 93, The URL validation in the prompt/MCP
model helpers currently allows both HTTP and HTTPS, which should be tightened to
reject cleartext URLs. Update the validation logic in the prompt data source and
MCP URL checks (the helper that normalizes and validates URLs in app/models.py)
so user-configured endpoints require https:// by default, and only allow http://
behind an explicit local-development exception flag or equivalent gate. Ensure
the existing ValueError message and any related validation paths consistently
reflect the new HTTPS-only requirement.
Source: Linters/SAST tools
| def _section_header(index: int, name: str, source_type: str, url: str | None) -> str: | ||
| heading = f"Source {index}: {name} ({source_type})" | ||
| if url: | ||
| heading += f"\nURL: {url}" | ||
| return heading |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact source URLs before adding them to prompt context or persona metadata.
_section_header() puts the full URL into LLM-visible context, and source_record keeps it for subprocess handoff. Presigned URLs or query tokens would be exposed downstream. Store/display a redacted URL, or omit it from the prompt entirely.
Proposed redaction helper
+def _redact_url_for_prompt(url: str | None) -> str | None:
+ if not url:
+ return None
+ parsed = urlparse(url)
+ return parsed._replace(query="", fragment="").geturl()
+
+
def _section_header(index: int, name: str, source_type: str, url: str | None) -> str:
heading = f"Source {index}: {name} ({source_type})"
- if url:
- heading += f"\nURL: {url}"
+ display_url = _redact_url_for_prompt(url)
+ if display_url:
+ heading += f"\nURL: {display_url}"
return heading
...
source_record = _dump_source(source)
source_record.pop("headers", None)
source_record.pop("text", None)
+ if source_record.get("url"):
+ source_record["url"] = _redact_url_for_prompt(source_record["url"])Also applies to: 212-215
🤖 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 `@app/services/prompt_context.py` around lines 53 - 57, The prompt context is
leaking full source URLs into LLM-visible text and persona metadata. Update
_section_header() to display only a redacted/omitted URL, and ensure
source_record uses the same sanitized value before handing data to subprocesses.
Apply the same redaction flow anywhere else the URL is propagated in the prompt
context build path so presigned or tokenized URLs are never exposed downstream.
| url = _get(source, "url") | ||
| _validate_fetch_url(url) | ||
| headers = _get(source, "headers") or {} | ||
| max_bytes = int(os.getenv("PROMPT_DATA_SOURCE_MAX_BYTES", DEFAULT_SOURCE_MAX_BYTES)) | ||
|
|
||
| timeout = aiohttp.ClientTimeout(total=12) | ||
| try: | ||
| async with aiohttp.ClientSession(timeout=timeout) as session: | ||
| async with session.get(url, headers=headers, allow_redirects=False) as resp: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether prompt URL fetching uses a custom aiohttp resolver/connector
# that binds DNS validation to the actual connection.
rg -n -C3 'ClientSession|TCPConnector|resolver|getaddrinfo|_validate_fetch_url' app/services/prompt_context.pyRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 1215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== prompt_context outline =="
ast-grep outline app/services/prompt_context.py --view expanded || true
echo
echo "== relevant file slice =="
sed -n '1,240p' app/services/prompt_context.py | cat -nRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 10567
Bind SSRF checks to the actual outbound connection. _validate_fetch_url() only checks DNS once, but aiohttp resolves the hostname again in session.get(), so a rebinding host can pass validation and still connect to a private address. Pin the validated address in the connector/resolver, or connect to the resolved IP while keeping the original Host header.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 88-89: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🤖 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 `@app/services/prompt_context.py` around lines 81 - 89, The SSRF guard in
prompt_context.py is only validating the URL before aiohttp opens the
connection, but the actual outbound request in the fetch flow still re-resolves
the hostname and can bypass `_validate_fetch_url()`. Update the request path
around `_validate_fetch_url`, `aiohttp.ClientSession`, and `session.get` so the
validated address is bound to the real connection, either by using a
connector/resolver pinned to the resolved IP or by connecting to the resolved IP
while preserving the original Host header. Ensure the fix is applied in the same
fetch routine that reads `url`, `headers`, and `timeout`.
| def build_mcp_tool_name(server_name: str, tool_name: str) -> str: | ||
| """Return an OpenAI/Pipecat-safe function name for an MCP tool.""" | ||
| raw = f"mcp_{server_name}_{tool_name}".lower() | ||
| safe = re.sub(r"[^a-z0-9_]", "_", raw) | ||
| safe = re.sub(r"_+", "_", safe).strip("_") | ||
| return safe[:64] or "mcp_tool" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent MCP function-name collisions.
Sanitizing plus truncating can map distinct server/tool pairs to the same 64-character name; LiveMCPManager then overwrites _tools[function_name], so calls can route to the wrong MCP tool.
Proposed fix
+import hashlib
+
def build_mcp_tool_name(server_name: str, tool_name: str) -> str:
"""Return an OpenAI/Pipecat-safe function name for an MCP tool."""
raw = f"mcp_{server_name}_{tool_name}".lower()
safe = re.sub(r"[^a-z0-9_]", "_", raw)
safe = re.sub(r"_+", "_", safe).strip("_")
- return safe[:64] or "mcp_tool"
+ if not safe:
+ return "mcp_tool"
+ digest = hashlib.sha1(raw.encode("utf-8")).hexdigest()[:8]
+ prefix = safe[:55].rstrip("_")
+ return f"{prefix}_{digest}"[:64]📝 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 build_mcp_tool_name(server_name: str, tool_name: str) -> str: | |
| """Return an OpenAI/Pipecat-safe function name for an MCP tool.""" | |
| raw = f"mcp_{server_name}_{tool_name}".lower() | |
| safe = re.sub(r"[^a-z0-9_]", "_", raw) | |
| safe = re.sub(r"_+", "_", safe).strip("_") | |
| return safe[:64] or "mcp_tool" | |
| import hashlib | |
| def build_mcp_tool_name(server_name: str, tool_name: str) -> str: | |
| """Return an OpenAI/Pipecat-safe function name for an MCP tool.""" | |
| raw = f"mcp_{server_name}_{tool_name}".lower() | |
| safe = re.sub(r"[^a-z0-9_]", "_", raw) | |
| safe = re.sub(r"_+", "_", safe).strip("_") | |
| if not safe: | |
| return "mcp_tool" | |
| digest = hashlib.sha1(raw.encode("utf-8")).hexdigest()[:8] | |
| prefix = safe[:55].rstrip("_") | |
| return f"{prefix}_{digest}"[:64] |
🤖 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 `@utils/mcp_client.py` around lines 39 - 44, The MCP tool name generator can
collapse different server/tool pairs into the same truncated value, which then
causes LiveMCPManager to overwrite entries in _tools. Update build_mcp_tool_name
to preserve uniqueness when truncating, such as by adding a deterministic hash
or suffix derived from the original raw name, and ensure the returned name still
fits the 64-character limit. Keep the existing sanitization logic, but make the
final name collision-resistant so distinct MCP tools cannot resolve to the same
function_name.
| def _is_private_ip(value: str) -> bool: | ||
| parsed = ip_address(value) | ||
| return ( | ||
| parsed.is_private | ||
| or parsed.is_loopback | ||
| or parsed.is_link_local | ||
| or parsed.is_multicast | ||
| or parsed.is_reserved | ||
| or parsed.is_unspecified | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject all non-global addresses in the SSRF guard.
100.64.0.0/10 and other non-global ranges are not caught by the current predicate. For outbound MCP URLs, allow only globally routable addresses unless explicitly allowlisted.
Proposed fix
def _is_private_ip(value: str) -> bool:
parsed = ip_address(value)
- return (
- parsed.is_private
- or parsed.is_loopback
- or parsed.is_link_local
- or parsed.is_multicast
- or parsed.is_reserved
- or parsed.is_unspecified
- )
+ return not parsed.is_global📝 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 _is_private_ip(value: str) -> bool: | |
| parsed = ip_address(value) | |
| return ( | |
| parsed.is_private | |
| or parsed.is_loopback | |
| or parsed.is_link_local | |
| or parsed.is_multicast | |
| or parsed.is_reserved | |
| or parsed.is_unspecified | |
| ) | |
| def _is_private_ip(value: str) -> bool: | |
| parsed = ip_address(value) | |
| return not parsed.is_global |
🤖 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 `@utils/mcp_client.py` around lines 76 - 85, The SSRF guard in _is_private_ip
currently only blocks private/loopback/link-local/etc. but still allows other
non-global ranges like 100.64.0.0/10. Update the predicate to reject any address
that is not globally routable, and keep the allowlist override path separate so
outbound MCP URLs are permitted only when the target IP is explicitly
allowlisted. Ensure the fix is applied in _is_private_ip and any caller that
relies on it for URL validation.
| try: | ||
| addresses = socket.getaddrinfo(host, None) | ||
| except socket.gaierror as e: | ||
| raise McpClientError(f"Could not resolve MCP HTTP host '{host}': {e}") from e | ||
|
|
||
| for address in addresses: | ||
| if _is_private_ip(address[4][0]): | ||
| raise McpClientError( | ||
| f"MCP HTTP URL resolves to private or local address: {host}" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path("utils/mcp_client.py")
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 1 <= i <= 260:
if "validate_mcp_http_url" in line or "session.post" in line or "aiohttp" in line or "resolver" in line or "connector" in line:
print(f"{i}: {line}")
PYRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- utils/mcp_client.py (lines 80-170) ---'
sed -n '80,170p' utils/mcp_client.py | cat -n
echo
echo '--- utils/mcp_client.py (lines 170-260) ---'
sed -n '170,260p' utils/mcp_client.py | cat -n
echo
echo '--- search for aiohttp session/connector/resolver usage ---'
rg -n "ClientSession|TCPConnector|resolver=|session\.post|validate_mcp_http_url|MCP HTTP URL" utils . -g '!**/__pycache__/**'Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 9917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- utils/mcp_client.py (lines 450-500) ---'
sed -n '450,500p' utils/mcp_client.py | cat -n
echo
echo '--- tests/test_mcp_client.py (lines 90-190) ---'
sed -n '90,190p' tests/test_mcp_client.py | cat -nRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 7533
🌐 Web query:
aiohttp TCPConnector default DNS resolution request hostname re-resolved DNS rebinding documentation
💡 Result:
In aiohttp, the TCPConnector manages DNS resolution and uses an internal cache by default to optimize performance [1][2]. Default DNS Behavior: - DNS Caching: The TCPConnector features an internal DNS cache enabled by default (use_dns_cache=True) [1][2]. - TTL: By default, cached DNS entries expire after 10 seconds (ttl_dns_cache=10) [1][2]. If this value is set to None, entries are cached indefinitely [1][2]. - Resolution Bypass: When the host is identified as a valid IP address literal (e.g., standard dotted-quad IPv4 or IPv6 literals), aiohttp bypasses the resolver and connects directly to the IP, avoiding DNS lookups [3]. Recent updates have tightened this logic to reject non-canonical numeric IPv4 forms (such as octal or hex-style representations) to prevent security bypasses [3]. DNS Rebinding Considerations: - aiohttp does not provide built-in, automated protection specifically designed to detect or prevent DNS rebinding attacks at the connector level. Because the connector relies on a local cache by default, if a domain's IP changes rapidly (faster than the 10-second TTL), the application may continue to connect to the stale, cached IP [4]. - To mitigate concerns related to DNS rebinding or to ensure real-time resolution for highly dynamic environments, you can disable the DNS cache by setting use_dns_cache=False when initializing the TCPConnector [4][1]. - Alternatively, for advanced scenarios, you can implement a custom resolver by subclassing aiohttp.abc.AbstractResolver to enforce stricter resolution policies or to bypass caching entirely [5][2].
Citations:
- 1: https://docs.aiohttp.org/en/latest/client_reference.html?highlight=TCPConnector
- 2: https://docs.aiohttp.org/en/v3.10.4/client_reference.html
- 3: Numeric ipv4 resolver bypass aio-libs/aiohttp#12827
- 4: ClientSession with default params caches DNS entries indefinitely aio-libs/aiohttp#1914
- 5: http://docs.aiohttp.org/en/stable/client_reference.html
Pin MCP HTTP DNS resolution. validate_mcp_http_url() resolves the hostname, but session.post(self.url) still hands the hostname to aiohttp’s connector, which performs its own DNS lookup with only a short-lived cache. An attacker-controlled host can pass validation on a public IP and then rebind to a private IP at connect time. Pin the validated address through a custom resolver/connector, or restrict MCP HTTP URLs to IP literals plus an explicit allowlist.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 111-111: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 115-117: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 `@utils/mcp_client.py` around lines 108 - 117, validate_mcp_http_url()
currently checks the resolved host once, but session.post(self.url) still lets
aiohttp resolve the hostname again, which can allow DNS rebinding after
validation. Update the MCP HTTP client path in McpClient to reuse the validated
address at connect time by adding a custom resolver/connector that pins the
resolved IP, or tighten the URL handling so only IP literals and an explicit
allowlist are accepted. Keep the fix centered around validate_mcp_http_url() and
the code that builds the aiohttp session/request.
| async def read_stdio_message(reader: asyncio.StreamReader) -> dict[str, Any]: | ||
| """Read one Content-Length framed JSON message from a subprocess stream.""" | ||
| headers: dict[str, str] = {} | ||
|
|
||
| while True: | ||
| line = await reader.readline() | ||
| if line == b"": | ||
| raise McpClientError("MCP stdio server closed stdout") | ||
| if line in {b"\r\n", b"\n"}: | ||
| break | ||
| try: | ||
| name, value = line.decode("ascii").split(":", 1) | ||
| except ValueError as e: | ||
| raise McpClientError("Invalid MCP stdio header") from e | ||
| headers[name.strip().lower()] = value.strip() | ||
|
|
||
| try: | ||
| content_length = int(headers["content-length"]) | ||
| except (KeyError, ValueError) as e: | ||
| raise McpClientError("Missing or invalid MCP stdio Content-Length") from e | ||
|
|
||
| body = await reader.readexactly(content_length) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add stdio request timeouts and frame-size limits.
A misbehaving MCP subprocess can hang readline()/readexactly() indefinitely or advertise an unbounded Content-Length, blocking bot startup/tool calls and risking memory exhaustion.
Proposed direction
+MAX_STDIO_FRAME_BYTES = 4 * 1024 * 1024
+STDIO_REQUEST_TIMEOUT_SECONDS = 12
+
async def read_stdio_message(reader: asyncio.StreamReader) -> dict[str, Any]:
@@
try:
content_length = int(headers["content-length"])
except (KeyError, ValueError) as e:
raise McpClientError("Missing or invalid MCP stdio Content-Length") from e
+ if content_length < 0 or content_length > MAX_STDIO_FRAME_BYTES:
+ raise McpClientError("MCP stdio Content-Length is outside allowed bounds")
- body = await reader.readexactly(content_length)
+ body = await asyncio.wait_for(
+ reader.readexactly(content_length),
+ timeout=STDIO_REQUEST_TIMEOUT_SECONDS,
+ )Also applies to: 384-395
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 133-133: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 139-139: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 145-145: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 `@utils/mcp_client.py` around lines 126 - 147, Add defensive limits and
timeouts to read_stdio_message so a stalled or malicious MCP subprocess cannot
block forever or allocate unbounded memory. Wrap the readline()/readexactly()
path in asyncio timeout handling, validate the parsed Content-Length against a
reasonable maximum before calling readexactly(), and surface McpClientError with
clear context from read_stdio_message so callers of the stdio MCP client fail
fast instead of hanging.
| try: | ||
| await asyncio.wait_for(process.wait(), timeout=2) | ||
| except asyncio.TimeoutError: | ||
| process.terminate() | ||
| await process.wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Kill subprocesses that ignore termination.
After process.terminate(), await process.wait() can still hang forever if the MCP server ignores SIGTERM.
Proposed fix
except asyncio.TimeoutError:
process.terminate()
- await process.wait()
+ try:
+ await asyncio.wait_for(process.wait(), timeout=2)
+ except asyncio.TimeoutError:
+ process.kill()
+ await process.wait()📝 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.
| try: | |
| await asyncio.wait_for(process.wait(), timeout=2) | |
| except asyncio.TimeoutError: | |
| process.terminate() | |
| await process.wait() | |
| try: | |
| await asyncio.wait_for(process.wait(), timeout=2) | |
| except asyncio.TimeoutError: | |
| process.terminate() | |
| try: | |
| await asyncio.wait_for(process.wait(), timeout=2) | |
| except asyncio.TimeoutError: | |
| process.kill() | |
| await process.wait() |
🤖 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 `@utils/mcp_client.py` around lines 378 - 382, The subprocess shutdown in the
process.wait() timeout path can still hang if the MCP server ignores SIGTERM.
Update the termination flow in the wait_for block around process.terminate() so
it waits for a short grace period and then force-kills the process if it does
not exit, using the existing process handle in mcp_client.py. Make sure the
cleanup logic in this timeout branch cannot block indefinitely and that any
forced kill still waits for the process to fully exit.
1 participant
Summary
llm_providerandllm_modelfields to the speaking bot APIDefaults
gpt-5.5claude-opus-4-8glm-5.2viaZAI_BASE_URL=https://api.z.ai/api/paas/v4/Validation
nix develop -c poetry check --locknix develop -c poetry run python -m py_compile scripts/meetingbaas.py app/models.py app/routes.pynix develop -c poetry run python -m unittest tests.test_models tests.test_prompt_context tests.test_runtime tests.test_mcp_clientnix develop -c ruff check scripts/meetingbaas.py app/models.py app/routes.py tests/test_models.py