Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds external prompt-context loading and MCP server integration to the speaking bot API. It also updates persona handoff, Cartesia TTS speed configuration, OpenAPI snapshots, environment docs, and tests. ChangesPrompt context and MCP API surface
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Routes as app/routes.py
participant PromptContext as app/services/prompt_context.py
participant Process as core/process.py
participant Meetingbaas as scripts/meetingbaas.py
participant MCPServer as MCP server
Client->>Routes: POST /bots (prompt_data_sources, mcp, speech_speed)
Routes->>PromptContext: load_prompt_context(sources, token_limit)
PromptContext-->>Routes: PromptContextResult(block, sources, estimated_tokens)
Routes->>Process: persona_data with context, mcp, speech_speed
Process->>Meetingbaas: spawn subprocess --persona-data-file path
Meetingbaas->>MCPServer: connect and discover tools
MCPServer-->>Meetingbaas: tools and metadata
Meetingbaas-->>Client: meeting bot starts with configured prompt/tools/TTS
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
🤖 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`:
- Around line 4-6: Update the new Pydantic models in models.py to use modern
built-in generics and union syntax instead of typing.Dict, typing.List, and
Optional. Replace these annotations throughout the affected model definitions
and related validators with dict, list, and | None so the new fields and models
match the project guidelines and avoid Ruff UP035 warnings. Focus on the model
classes and any helper methods that currently reference the old typing aliases.
In `@app/routes.py`:
- Around line 250-251: The MCP persona persistence path is currently serializing
sensitive headers into resolved_persona_data via
request.mcp.model_dump(exclude_none=True), which later gets written to disk.
Update the logic in the request.mcp handling block to exclude servers[].headers
from the persisted MCP payload, keeping only non-sensitive MCP fields in
resolved_persona_data before it is saved.
In `@app/services/prompt_context.py`:
- Around line 135-172: The SSRF guard in _validate_fetch_url only checks the
hostname before the request is opened, so DNS rebinding can still reach a
private IP later. Update the fetch path that uses _validate_fetch_url to either
pin the resolved address in the aiohttp connector or re-validate the actual peer
IP after connect, and keep the existing private-host/address checks in place.
In `@core/process.py`:
- Around line 48-59: The temp payload written in core/process.py by the persona
payload setup before launching the Pipecat subprocess can be left behind if
Popen fails. Update the parent error handling around the subprocess startup path
so that persona_data_path is removed on failure, using the existing payload
creation flow near payload_dir, persona_data_path, and the Popen launch logic to
ensure the 0600 file does not persist with persona/MCP data.
In `@env.example`:
- Around line 69-70: The env example is missing the allowlist variable name used
elsewhere, and the current entry only documents MCP_ALLOW_PRIVATE_URLS. Update
the env.example entry to include MCP_ALLOWED_PRIVATE_URLS alongside the existing
private-URL setting, matching the names referenced by README.md and
utils/mcp_client.py so both supported env vars are documented consistently.
In `@scripts/export_openapi.py`:
- Around line 17-20: Add a Google-style docstring to main() in
export_openapi.py, since it is a public function and currently lacks one. Keep
the docstring concise but complete, describing that main() generates the OpenAPI
schema from create_app().openapi(), writes it to OUTPUT, and prints the written
path. Use the function name main() as the anchor when updating the script.
In `@scripts/meetingbaas.py`:
- Around line 4-6: The module is importing the same JSON library under two
names, which creates unnecessary aliasing and confusion. Update the imports in
meetingbaas.py to use the existing json module name everywhere, and adjust
_tool_result_to_text to reference json instead of jsonlib while keeping cli()
and other JSON usages consistent with the single import.
- Around line 276-320: The connect() flow in MeetingBaaS leaks already-open MCP
clients when a later required server fails, because the bare re-raise exits
before cleanup. Update connect() to close every client that was successfully
added to self._clients before propagating the error, or ensure
setup_mcp_tools/main performs cleanup on connect() failure; use the connect,
self._clients, self._build_client, and log_and_flush symbols to locate the
affected path.
In `@utils/mcp_client.py`:
- Around line 384-395: The _request method in mcp_client.py currently assumes
the next framed stdout message is always the matching JSON-RPC response, which
can misattribute interleaved notifications or other replies. Update _request to
use the request id returned by self._state.request(method, params), then keep
reading from self._process.stdout with read_stdio_message until a frame with a
matching id is found, ignoring unrelated notifications or responses. Make sure
the final _extract_result call only runs on the response whose id matches the
original request.
- Around line 88-117: DNS rebinding can still slip past validate_mcp_http_url()
because it validates the hostname once, but _post() later hands the original URL
to aiohttp for a second resolution at connect time. Update the connection path
in _post() (and any related aiohttp session/connector setup) so the validated
address is pinned or enforced during connection, using the existing helpers
validate_mcp_http_url(), _is_private_ip(), and _private_mcp_urls_allowed() as
the reference points.
🪄 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: 9c152c2f-9468-47ff-ad7f-84db40d7eaf5
📒 Files selected for processing (14)
README.mdapp/models.pyapp/routes.pyapp/services/prompt_context.pycore/process.pyenv.examplescripts/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 typing import Any, Dict, List, Literal, Optional | ||
|
|
||
| from pydantic import BaseModel, ConfigDict, Field, field_validator | ||
| from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use modern type-hint syntax for the new models.
The new fields and models use typing.Dict, typing.List, and Optional[...] (e.g., Lines 67-84, 122-154, 262-276). Ruff (UP035) also flags Dict/List as deprecated. Prefer built-in generics (dict, list) and | None unions to match guidelines.
As per coding guidelines: "Use type hints with modern Python syntax (|) for unions instead of Union" and "Prefer built-in types for annotations where possible".
🧰 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` around lines 4 - 6, Update the new Pydantic models in
models.py to use modern built-in generics and union syntax instead of
typing.Dict, typing.List, and Optional. Replace these annotations throughout the
affected model definitions and related validators with dict, list, and | None so
the new fields and models match the project guidelines and avoid Ruff UP035
warnings. Focus on the model classes and any helper methods that currently
reference the old typing aliases.
Sources: Coding guidelines, Linters/SAST tools
| if request.mcp: | ||
| resolved_persona_data["mcp"] = request.mcp.model_dump(exclude_none=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how persona_data is written to disk and its permissions/cleanup
fd 'process.py' core | xargs -I{} sed -n '1,200p' {}
rg -nP 'persona[-_]data[-_]file|persona_data|chmod|0o600|NamedTemporaryFile|unlink|os\.remove' -C3Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 5237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/routes.py around the referenced lines =="
sed -n '220,270p' app/routes.py
echo
echo "== Locate MCP-related schema/model definitions =="
rg -n "class .*MCP|mcp:|headers|Authorization|model_dump|servers" app -g '*.py' -C 3
echo
echo "== Find the subprocess handoff script and cleanup behavior =="
rg -n --hidden --glob '*.py' "persona_data_path|--persona-data-file|persona_payloads|unlink|os\.remove|rmtree|NamedTemporaryFile|chmod|0o600" .
echo
echo "== Inspect the process writer file if present =="
fd -a 'process.py' . | while read -r f; do
echo "--- $f ---"
sed -n '1,220p' "$f"
doneRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 20618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/services/prompt_context.py MCP formatter =="
sed -n '242,310p' app/services/prompt_context.py
echo
echo "== scripts/meetingbaas.py persona-data handling and cleanup =="
sed -n '1030,1085p' scripts/meetingbaas.py
echo
echo "== any direct logging of MCP headers or request.mcp dumps =="
rg -n "mcp.*header|headers.*mcp|model_dump\\(exclude_none=True\\).*mcp|persona_data_file|prompt_context.*mcp|format_mcp_context" app scripts core -C 2Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 5539
Strip MCP headers before persisting the persona payload (app/routes.py:250-251)
request.mcp.model_dump(exclude_none=True) copies servers[].headers into resolved_persona_data. That object is later written to persona_payloads/<client>.json, so bearer tokens can sit on disk briefly even with 0600 permissions and cleanup after startup. Keep headers out of the persisted persona object.
🤖 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/routes.py` around lines 250 - 251, The MCP persona persistence path is
currently serializing sensitive headers into resolved_persona_data via
request.mcp.model_dump(exclude_none=True), which later gets written to disk.
Update the logic in the request.mcp handling block to exclude servers[].headers
from the persisted MCP payload, keeping only non-sensitive MCP fields in
resolved_persona_data before it is saved.
| def _validate_fetch_url(url: str) -> None: | ||
| """Block obvious SSRF targets unless explicitly allowed.""" | ||
| parsed = urlparse(url) | ||
| if parsed.scheme not in {"http", "https"} or not parsed.hostname: | ||
| raise PromptContextError( | ||
| f"Invalid prompt data source URL: {url}", | ||
| status_code=400, | ||
| ) | ||
|
|
||
| if _private_urls_allowed(): | ||
| return | ||
|
|
||
| host = parsed.hostname | ||
| try: | ||
| if _is_private_ip(host): | ||
| raise PromptContextError( | ||
| f"Prompt data source URL host is private or local: {host}", | ||
| status_code=400, | ||
| ) | ||
| return | ||
| except ValueError: | ||
| pass | ||
|
|
||
| try: | ||
| addresses = socket.getaddrinfo(host, None) | ||
| except socket.gaierror as e: | ||
| raise PromptContextError( | ||
| f"Could not resolve prompt data source host '{host}': {e}", | ||
| status_code=400, | ||
| ) from e | ||
|
|
||
| for address in addresses: | ||
| resolved_ip = address[4][0] | ||
| if _is_private_ip(resolved_ip): | ||
| raise PromptContextError( | ||
| f"Prompt data source URL resolves to private or local address: {host}", | ||
| status_code=400, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
aiohttp pin resolved IP prevent DNS rebinding SSRF custom resolver
💡 Result:
To prevent SSRF and DNS rebinding attacks in aiohttp, relying on standard hostname-based validation is insufficient due to the TOCTOU (time-of-check to time-of-use) gap inherent in DNS resolution [1][2]. Because standard DNS lookups and subsequent TCP connections occur as separate operations, an attacker can modify DNS records between the check and the connection to redirect traffic to unauthorized IP addresses (e.g., private ranges, metadata services) [3][2]. To effectively secure aiohttp, implement the following architectural patterns: 1. Use a Custom Resolver Implement a custom resolver that intercepts the resolution process [4][2]. This resolver should: - Resolve the hostname to an IP address [3][2]. - Perform a security check on the returned IP address before it is returned to the connector [1][2]. - Validate that the IP does not fall within restricted ranges (e.g., RFC 1918, link-local 169.254/16, loopback, or cloud metadata endpoints) [1][2]. - By integrating this into the aiohttp TCPConnector lifecycle, you ensure that only validated IP addresses are ever used for the connection [1][3]. 2. Pin the Resolved IP To close the TOCTOU window, ensure that your logic pins the connection to the specific, validated IP address [1][3]. - After the custom resolver validates an IP, the transport layer should be instructed to connect directly to that IP [1][3]. - Update the request headers (specifically the Host header) and TLS SNI parameters to match the original hostname while the connection is established to the validated IP [3]. This preserves application-level requirements (like virtual hosting) without re-initiating a potentially malicious DNS resolution [3]. 3. Validate Redirects DNS rebinding often occurs or is weaponized during HTTP redirects [3]. Ensure your implementation is not just validating the initial URL, but also hooking into the request lifecycle to re-validate every subsequent redirect target against the same blocklist/policy before allowing the connection [1][3]. 4. Mitigate Resolver Bypasses Ensure your code correctly handles and restricts IP literals. Recent security updates emphasize that non-canonical IP formats (e.g., certain legacy numeric IPv4 forms) should be strictly validated or rejected, as they can sometimes bypass standard resolution checks [5]. For a robust, ready-to-use approach, consider using security-focused wrapper libraries that implement these "resolve-validate-pin" patterns as a single atomic operation, as manually implementing secure resolvers requires careful handling of edge cases like IPv4-mapped IPv6 addresses and non-standard host strings [1][3][2].
Citations:
- 1: Fix/aiohttp ssrf cwe918 BerriAI/litellm#28252
- 2: fix(aiohttp): block private/metadata IPs in api_base to close SSRF gap from PR #26264 (CWE-918) BerriAI/litellm#28246
- 3: https://github.com/tachyon-oss/drawbridge
- 4: Recipe for preventing SSRF? aio-libs/aiohttp#10224
- 5: Numeric ipv4 resolver bypass aio-libs/aiohttp#12827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby helpers.
git ls-files app/services/prompt_context.py
wc -l app/services/prompt_context.py
sed -n '1,260p' app/services/prompt_context.pyRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 8513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find fetch and validation helpers referenced in the review comment.
rg -n "_validate_fetch_url|_fetch_url_source|private_urls_allowed|getaddrinfo|aiohttp" app/services/prompt_context.pyRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract only the relevant section with line numbers for precise inspection.
nl -ba app/services/prompt_context.py | sed -n '120,240p'Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the fetch implementation and surrounding code paths.
nl -ba app/services/prompt_context.py | sed -n '1,260p'Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 211
🌐 Web query:
aiohttp ClientSession TCPConnector DNS resolution connect time custom resolver documentation
💡 Result:
To configure a custom DNS resolver and manage connection/DNS resolution times in aiohttp, you primarily use the TCPConnector class [1][2]. Custom DNS Resolver To use custom nameservers for DNS resolution, you must have the aiodns library installed [1][3]. You can then pass an AsyncResolver instance to the TCPConnector [1][4]: from aiohttp.resolver import AsyncResolver import aiohttp # Create a resolver with custom nameservers resolver = AsyncResolver(nameservers=["8.8.8.8", "8.8.4.4"]) # Pass the resolver to the TCPConnector connector = aiohttp.TCPConnector(resolver=resolver) session = aiohttp.ClientSession(connector=connector) Note: For best results in an asynchronous context, initialize your resolver within an async function or ensure it is created within the running event loop to avoid potential warnings or lifecycle issues [5]. Managing Resolution and Connection Time aiohttp does not have a single flag for "DNS resolution time" independent of the connection process; instead, DNS resolution is handled as the first step of the connection phase [6]. You can manage timeouts and resolution behavior using the following: 1. DNS Cache Tuning: By default, TCPConnector uses an internal DNS cache (TTL of 10 seconds) [1][2]. You can disable this or change the TTL to ensure more frequent resolutions: - Disable cache: TCPConnector(use_dns_cache=False) [1][7]. - Adjust TTL: TCPConnector(ttl_dns_cache=300) [1][8]. 2. Connection Timeouts: Use aiohttp.ClientTimeout to control the total time spent connecting (which includes DNS resolution) [6]. - timeout = aiohttp.ClientTimeout(connect=5.0) - session.get(url, timeout=timeout) If the DNS lookup hangs or takes too long, it will be subject to the connect timeout defined in your ClientTimeout configuration [6]. If you encounter issues with DNS errors being swallowed or timeouts, ensure you are on a recent version of aiohttp, as fixes for DNS resolution error handling have been actively addressed in recent releases [6].
Citations:
- 1: https://docs.aiohttp.org/en/stable/client%5Fadvanced.html?highlight=proxy
- 2: https://docs.aiohttp.org/en/latest/client_reference.html?highlight=TCPConnector
- 3: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/resolver.py
- 4: https://www.peterspython.com/en/blog/aiohttp-with-custom-dns-servers-unbound-and-docker
- 5: https://blog.gainskills.top/2019/01/23/try-aiohttp/
- 6: DNS resolution errors are sometimes swallowed aio-libs/aiohttp#9447
- 7: http://docs.aiohttp.org/en/stable/client_advanced.html
- 8: https://docs.aiohttp.org/en/latest/client%5Fadvanced.html
Close the DNS-rebinding gap in this SSRF check _validate_fetch_url only validates the hostname before aiohttp opens the connection, so a rebinding attacker can still swap in a private IP at connect time. Pin the resolved address in the connector or re-check the peer IP after connect.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 139-142: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 150-153: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 154-154: Consider moving this statement to an else block
(TRY300)
[warning] 161-164: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 169-172: 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/services/prompt_context.py` around lines 135 - 172, The SSRF guard in
_validate_fetch_url only checks the hostname before the request is opened, so
DNS rebinding can still reach a private IP later. Update the fetch path that
uses _validate_fetch_url to either pin the resolved address in the aiohttp
connector or re-validate the actual peer IP after connect, and keep the existing
private-host/address checks in place.
| payload_dir = os.path.join(get_state_dir(), "persona_payloads") | ||
| os.makedirs(payload_dir, exist_ok=True) | ||
| persona_data_path = os.path.join(payload_dir, f"{client_id}.json") | ||
| payload_fd = os.open( | ||
| persona_data_path, | ||
| os.O_WRONLY | os.O_CREAT | os.O_TRUNC, | ||
| 0o600, | ||
| ) | ||
| with os.fdopen(payload_fd, "w") as f: | ||
| json.dump(persona_data, f) | ||
| if (persona_data or {}).get("mcp"): | ||
| logger.info(f"Passing MCP metadata to Pipecat process for client {client_id}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'start_pipecat_process\s*\(' --type=py
rg -nP -C3 '\bclient_id\b\s*=' --type=py -g 'app/**'Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map repository files around the relevant areas.
git ls-files 'core/*' '*/process.py' '*/process/*.py' | sed -n '1,200p'
# Inspect the relevant file structure.
if [ -f core/process.py ]; then
wc -l core/process.py
ast-grep outline core/process.py --view expanded || true
sed -n '1,220p' core/process.py | cat -n
fi
# Find all references to client_id and persona_data_file.
rg -n --hidden --glob '!**/.git/**' '\bclient_id\b|persona_data_file|persona_payloads|start_pipecat_process' .Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 24932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,210p' app/websockets.py | cat -n
sed -n '370,420p' app/routes.py | cat -n
sed -n '1045,1070p' scripts/meetingbaas.py | cat -n
# Check whether any cleanup happens on start failure.
rg -n --hidden --glob '!**/.git/**' 'persona_data_path|os\.remove\(args\.persona_data_file\)|persona_payloads|start_pipecat_process\(' core app scripts utilsRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 8612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for validation/sanitization of client_id before it reaches core.process.
rg -n --hidden --glob '!**/.git/**' 'client_id.*(uuid|validate|sanitize|safe|basename|replace)|basename\(.*client_id|client_id.*basename|re\.match\(.*client_id|re\.fullmatch\(.*client_id' app core utils scriptsRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the websocket flow that reaches start_pipecat_process.
sed -n '1,140p' app/websockets.py | cat -n
sed -n '140,220p' app/websockets.py | cat -n
# Show the route that creates bot_client_id.
sed -n '90,140p' app/routes.py | cat -n
# Check for any other start_pipecat_process call sites.
rg -n --hidden --glob '!**/.git/**' 'start_pipecat_process\(' app core scripts utilsRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 14510
Delete the temp payload file if Popen fails. core/process.py:48-59 writes persona_data_path before the subprocess starts, so a startup exception leaves the 0600 payload on disk with persona/MCP data. Remove it in the parent error path.
🤖 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 `@core/process.py` around lines 48 - 59, The temp payload written in
core/process.py by the persona payload setup before launching the Pipecat
subprocess can be left behind if Popen fails. Update the parent error handling
around the subprocess startup path so that persona_data_path is removed on
failure, using the existing payload creation flow near payload_dir,
persona_data_path, and the Popen launch logic to ensure the 0600 file does not
persist with persona/MCP data.
Source: Linters/SAST tools
| # Set true only in trusted networks if remote MCP URLs must reach localhost/private IPs. | ||
| MCP_ALLOW_PRIVATE_URLS=false |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'MCP_ALLOWED_PRIVATE_URLS|MCP_ALLOW_PRIVATE_URLS' --type=pyRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- README references --\n'
rg -n 'MCP_ALLOWED_PRIVATE_URLS|MCP_ALLOW_PRIVATE_URLS' README.md env.example || true
printf '\n-- All repository references --\n'
rg -n 'MCP_ALLOWED_PRIVATE_URLS|MCP_ALLOW_PRIVATE_URLS' . || trueRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 1716
Document MCP_ALLOWED_PRIVATE_URLS in env.example. README.md and utils/mcp_client.py both support the allowlist env var, but only MCP_ALLOW_PRIVATE_URLS is listed here.
🤖 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 `@env.example` around lines 69 - 70, The env example is missing the allowlist
variable name used elsewhere, and the current entry only documents
MCP_ALLOW_PRIVATE_URLS. Update the env.example entry to include
MCP_ALLOWED_PRIVATE_URLS alongside the existing private-URL setting, matching
the names referenced by README.md and utils/mcp_client.py so both supported env
vars are documented consistently.
| def main() -> None: | ||
| schema = create_app().openapi() | ||
| OUTPUT.write_text(json.dumps(schema, indent=2) + "\n", encoding="utf-8") | ||
| print(f"Wrote {OUTPUT.relative_to(ROOT)}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider adding a docstring to main().
Per coding guidelines, public functions should have Google-style docstrings. main() currently has none.
As per coding guidelines, "Write comprehensive docstrings for public APIs following Google-style docstring format."
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 18-18: use jsonify instead of json.dumps for JSON output
Context: json.dumps(schema, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@scripts/export_openapi.py` around lines 17 - 20, Add a Google-style docstring
to main() in export_openapi.py, since it is a public function and currently
lacks one. Keep the docstring concise but complete, describing that main()
generates the OpenAPI schema from create_app().openapi(), writes it to OUTPUT,
and prints the written path. Use the function name main() as the anchor when
updating the script.
Source: Coding guidelines
| import inspect | ||
| import json as jsonlib | ||
| from dataclasses import dataclass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Avoid importing json twice under two names.
The module already imports json (used in cli() at Lines 1058/1071); adding import json as jsonlib for _tool_result_to_text means two aliases for the same module. Consolidate on json to reduce confusion.
🤖 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 `@scripts/meetingbaas.py` around lines 4 - 6, The module is importing the same
JSON library under two names, which creates unnecessary aliasing and confusion.
Update the imports in meetingbaas.py to use the existing json module name
everywhere, and adjust _tool_result_to_text to reference json instead of jsonlib
while keeping cli() and other JSON usages consistent with the single import.
| async def connect(self) -> list[dict]: | ||
| discovered = [] | ||
| for server in self._mcp_config.get("servers") or []: | ||
| if not isinstance(server, dict) or server.get("enabled") is False: | ||
| continue | ||
| if not server.get("transport"): | ||
| continue | ||
|
|
||
| client = self._build_client(server) | ||
| server_name = str(server.get("name") or "mcp") | ||
| try: | ||
| await client.initialize() | ||
| self._clients.append(client) | ||
| tool_allowlist = set(server.get("tool_allowlist") or []) | ||
| for tool in await client.list_tools(): | ||
| tool_name = str(tool.get("name") or "") | ||
| if not tool_name: | ||
| continue | ||
| if tool_allowlist and tool_name not in tool_allowlist: | ||
| continue | ||
| function_name = build_mcp_tool_name(server_name, tool_name) | ||
| tool_ref = LiveMCPTool( | ||
| function_name=function_name, | ||
| server_name=server_name, | ||
| tool_name=tool_name, | ||
| client=client, | ||
| schema=tool, | ||
| ) | ||
| self._tools[function_name] = tool_ref | ||
| discovered.append( | ||
| { | ||
| **tool, | ||
| "server_name": server_name, | ||
| "function_name": function_name, | ||
| } | ||
| ) | ||
| except Exception as exc: | ||
| await client.close() | ||
| if server.get("required"): | ||
| raise | ||
| log_and_flush( | ||
| logging.WARNING, | ||
| f"[MCP] Could not connect server {server_name}: {exc}", | ||
| ) | ||
| return discovered |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Required-server failure leaks already-connected clients and aborts startup.
When an earlier server connects successfully (appended to self._clients) and a later required server raises, the raise on Line 315 propagates without closing the earlier clients. Because setup_mcp_tools(...) is called at Line 767 — before the try/finally at Line 982 — the exception escapes main entirely: mcp_manager is never assigned, the shutdown teardown at Lines 1001-1006 never runs, and the connected MCP sessions leak while the bot fails to start.
Close all opened clients before re-raising (or wrap connect() in setup_mcp_tools and close the manager on failure).
🔒️ Proposed fix in connect()
except Exception as exc:
await client.close()
if server.get("required"):
+ await self.close()
raise
log_and_flush(
logging.WARNING,
f"[MCP] Could not connect server {server_name}: {exc}",
)📝 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.
| async def connect(self) -> list[dict]: | |
| discovered = [] | |
| for server in self._mcp_config.get("servers") or []: | |
| if not isinstance(server, dict) or server.get("enabled") is False: | |
| continue | |
| if not server.get("transport"): | |
| continue | |
| client = self._build_client(server) | |
| server_name = str(server.get("name") or "mcp") | |
| try: | |
| await client.initialize() | |
| self._clients.append(client) | |
| tool_allowlist = set(server.get("tool_allowlist") or []) | |
| for tool in await client.list_tools(): | |
| tool_name = str(tool.get("name") or "") | |
| if not tool_name: | |
| continue | |
| if tool_allowlist and tool_name not in tool_allowlist: | |
| continue | |
| function_name = build_mcp_tool_name(server_name, tool_name) | |
| tool_ref = LiveMCPTool( | |
| function_name=function_name, | |
| server_name=server_name, | |
| tool_name=tool_name, | |
| client=client, | |
| schema=tool, | |
| ) | |
| self._tools[function_name] = tool_ref | |
| discovered.append( | |
| { | |
| **tool, | |
| "server_name": server_name, | |
| "function_name": function_name, | |
| } | |
| ) | |
| except Exception as exc: | |
| await client.close() | |
| if server.get("required"): | |
| raise | |
| log_and_flush( | |
| logging.WARNING, | |
| f"[MCP] Could not connect server {server_name}: {exc}", | |
| ) | |
| return discovered | |
| except Exception as exc: | |
| await client.close() | |
| if server.get("required"): | |
| await self.close() | |
| raise | |
| log_and_flush( | |
| logging.WARNING, | |
| f"[MCP] Could not connect server {server_name}: {exc}", | |
| ) |
🤖 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 `@scripts/meetingbaas.py` around lines 276 - 320, The connect() flow in
MeetingBaaS leaks already-open MCP clients when a later required server fails,
because the bare re-raise exits before cleanup. Update connect() to close every
client that was successfully added to self._clients before propagating the
error, or ensure setup_mcp_tools/main performs cleanup on connect() failure; use
the connect, self._clients, self._build_client, and log_and_flush symbols to
locate the affected path.
| def validate_mcp_http_url(url: str) -> None: | ||
| """Block obvious SSRF targets unless explicitly allowed.""" | ||
| parsed = urlparse(url) | ||
| if parsed.scheme not in {"http", "https"} or not parsed.hostname: | ||
| raise McpClientError(f"Invalid MCP HTTP URL: {url}") | ||
|
|
||
| if _private_mcp_urls_allowed(): | ||
| return | ||
|
|
||
| if _is_allowed_private_mcp_url(url): | ||
| return | ||
|
|
||
| host = parsed.hostname | ||
| try: | ||
| if _is_private_ip(host): | ||
| raise McpClientError(f"MCP HTTP URL host is private or local: {host}") | ||
| return | ||
| except ValueError: | ||
| pass | ||
|
|
||
| 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:
#!/bin/bash
set -euo pipefail
# Locate the file and relevant symbols.
git ls-files | rg '^utils/mcp_client\.py$'
echo '--- outline ---'
ast-grep outline utils/mcp_client.py --view expanded || true
echo '--- relevant lines ---'
nl -ba utils/mcp_client.py | sed -n '1,260p'Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 3097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the prompt_context implementation and any URL validation / private IP handling.
rg -n "prompt_context|validate_mcp_http_url|_is_private_ip|_private_mcp_urls_allowed|_is_allowed_private_mcp_url|allow_redirects|aiohttp" -S utils/mcp_client.py .Repository: Meeting-BaaS/speaking-meeting-bot
Length of output: 30163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any helper that might share the same validation/pinning pattern.
rg -n "private.*mcp|DNS rebinding|rebinding|pin.*IP|getaddrinfo|resolve.*private" -S utils . || trueRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 1562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
("utils/mcp_client.py", 80, 130),
("utils/mcp_client.py", 460, 500),
("app/services/prompt_context.py", 140, 180),
("tests/test_mcp_client.py", 100, 190),
]:
print(f"--- {path}:{start}-{end} ---")
lines = Path(path).read_text().splitlines()
for i in range(start - 1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
print()
PYRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 9940
🌐 Web query:
aiohttp DNS resolution connection time re-resolve raw URL connect time SSRF rebinding
💡 Result:
AIOHTTP is susceptible to DNS rebinding attacks if not explicitly configured to prevent them, primarily due to its internal DNS caching mechanism and the Time-of-Check to Time-of-Use (TOCTOU) window between validation and connection [1][2][3]. DNS Rebinding and TOCTOU DNS rebinding exploits the gap between when a URL is validated (e.g., checking if it resolves to a public IP) and when the actual TCP connection is established [3]. If the DNS TTL is short or the cache is bypassed/expired, the hostname may be re-resolved to an attacker-controlled internal or metadata IP (e.g., 127.0.0.1, 169.254.169.254) for the second request, effectively bypassing initial security checks [1][2][3]. Mitigation Strategies 1. Custom Resolver: To prevent TOCTOU, implement a custom resolver that validates IP addresses at the exact moment of connection, rather than relying on pre-request checks [1][2]. By hooking into the TCPConnector, you can ensure that every connection attempt—including those for redirect targets—is validated against an allowlist or blocklist [1][2]. 2. Disable DNS Cache: You can disable the internal DNS cache in AIOHTTP to force resolution on every request, though this alone does not eliminate the TOCTOU window for a single request if it involves redirects or multiple resolution steps [4][5][6]. This is done via TCPConnector(use_dns_cache=False) [7][6]. 3. Redirect Protection: Because AIOHTTP follows redirects by default, SSRF guards must be applied recursively [1][2]. Use custom middleware or hooks to inspect the Location header of redirect responses and validate the destination host before the library follows it [8][9]. 4. IP Literal Validation: Recent updates to AIOHTTP have tightened the handling of IP literals to prevent bypasses where non-canonical numeric forms (e.g., octal or decimal representations) were treated as IP addresses rather than hostnames [10]. Ensure you are using updated versions that correctly canonicalize and validate these inputs [10]. Official Documentation Reference The AIOHTTP TCPConnector provides parameters such as use_dns_cache (boolean) and ttl_dns_cache (integer, in seconds) to manage resolution behavior [11][6]. However, for robust SSRF protection, community best practices emphasize implementing a CustomResolver that performs active validation at connect-time [12][1][2].
Citations:
- 1: Fix/aiohttp ssrf cwe918 BerriAI/litellm#28252
- 2: fix(aiohttp): block private/metadata IPs in api_base to close SSRF gap from PR #26264 (CWE-918) BerriAI/litellm#28246
- 3: https://github.com/agent-fox-dev/agent-fox/issues/580
- 4: http://docs.aiohttp.org/en/stable/client_advanced.html
- 5: Disable DNS cache aio-libs/aiohttp#2262
- 6: Questions about _DNSCacheTable and its memory leak aio-libs/aiohttp#3684
- 7: https://docs.aiohttp.org/en/v3.8.1/client_advanced.html
- 8: https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/aiohttp_client.py
- 9: indico/indico@70d3418
- 10: Numeric ipv4 resolver bypass aio-libs/aiohttp#12827
- 11: http://docs.aiohttp.org/en/stable/client_reference.html
- 12: Recipe for preventing SSRF? aio-libs/aiohttp#10224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
import sys
try:
import aiohttp
except Exception as e:
print(f"IMPORT_ERROR: {type(e).__name__}: {e}")
sys.exit(0)
print(f"aiohttp_version={getattr(aiohttp, '__version__', 'unknown')}")
mods = []
for qualname in [
"client.ClientSession._request",
"connector.TCPConnector._resolve_host",
"connector.TCPConnector.connect",
]:
mod_name, attr_path = qualname.split(".", 1)
obj = getattr(aiohttp, mod_name)
for part in attr_path.split("."):
obj = getattr(obj, part)
print(f"\n--- {qualname} ---")
try:
src = inspect.getsource(obj)
print(src[:6000])
except Exception as e:
print(f"SOURCE_ERROR: {type(e).__name__}: {e}")
PYRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 12525
DNS rebinding still bypasses this SSRF check. validate_mcp_http_url() resolves the host once, but _post() passes the original URL to aiohttp, which resolves again when connecting. A rebinding hostname can still reach a private IP; pin the validated address in the connection path or use a resolver/connector that enforces the check at connect time.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 92-92: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 103-103: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 104-104: Consider moving this statement to an else block
(TRY300)
[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 88 - 117, DNS rebinding can still slip past
validate_mcp_http_url() because it validates the hostname once, but _post()
later hands the original URL to aiohttp for a second resolution at connect time.
Update the connection path in _post() (and any related aiohttp session/connector
setup) so the validated address is pinned or enforced during connection, using
the existing helpers validate_mcp_http_url(), _is_private_ip(), and
_private_mcp_urls_allowed() as the reference points.
| async def _request( | ||
| self, | ||
| method: str, | ||
| params: Mapping[str, Any] | None = None, | ||
| ) -> dict[str, Any]: | ||
| await self.start() | ||
| assert self._process and self._process.stdin and self._process.stdout | ||
| message = self._state.request(method, params) | ||
| self._process.stdin.write(encode_stdio_message(message)) | ||
| await self._process.stdin.drain() | ||
| response = await read_stdio_message(self._process.stdout) | ||
| return _extract_result(response) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for request/response handling.
git ls-files 'utils/mcp_client.py' 'utils/*' | sed -n '1,120p'
echo '--- outline ---'
ast-grep outline utils/mcp_client.py --view expanded || true
echo '--- request-id related search ---'
rg -n "request id|request_id|request_id|id matches|read_stdio_message|encode_stdio_message|_state.request|notification|progress|out of order|stdin|stdout" utils -S || true
echo '--- relevant file excerpt ---'
sed -n '1,260p' utils/mcp_client.py
echo '--- later excerpt ---'
sed -n '260,460p' utils/mcp_client.pyRepository: Meeting-BaaS/speaking-meeting-bot
Length of output: 19745
🌐 Web query:
Model Context Protocol stdio transport notifications progress logs response ordering request id correlation
💡 Result:
The Model Context Protocol (MCP) relies on JSON-RPC 2.0 to manage request-response correlation, message ordering, and transport mechanics [1]. Stdio Transport In the stdio transport, the client launches the MCP server as a subprocess, with communication occurring over standard input (stdin) and standard output (stdout) [2][3]. - Framing: Messages are delimited by newlines and must not contain embedded newlines [2][4]. Each line represents a single valid JSON-RPC request, notification, or response [2]. - Logging: Servers may write UTF-8 strings to stderr for logging, which clients may capture or ignore [2][3]. This is distinct from structured protocol logging [5]. - Exclusivity: The client and server must not write anything to the other's stream that is not a valid MCP message [2][3]. Request ID Correlation MCP uses the JSON-RPC id field to correlate requests and responses [1]. When a client sends a request, it includes an id that the server must mirror in its response to allow the client to match the result to the specific request [1]. Since MCP is stateless, all necessary information is contained within the request itself; servers treat each request independently regardless of connection or process lifetime [6]. Notifications, Progress, and Logs - Request-Scoped Notifications: Certain notifications, specifically notifications/progress and notifications/message (logs), are request-scoped [5][7]. They are delivered on the same response stream as the request they relate to [5][7]. - Progress Tracking: If a client requires progress updates, it includes a progressToken in the request metadata (_meta) [8][9]. The server then sends notifications/progress containing the same token, allowing the client to associate the notification with the active operation [8][9]. - Structured Logging: Clients control logging verbosity per request by including io.modelcontextprotocol/logLevel in the request's _meta [5]. If enabled, the server may send notifications/message on that request's response stream [5]. The server must not emit these for requests lacking the log level field [5]. Response Ordering While MCP messages are delivered sequentially over the stdio stream, the protocol is inherently asynchronous [1]. Because JSON-RPC uses explicit request IDs, clients do not rely on implicit ordering to correlate responses; they can handle interleaved or out-of-order responses by matching them against the corresponding request id [1]. Clients should not assume a specific session or conversation boundary for the stdio process, as unrelated requests may be interleaved [6].
Citations:
- 1: https://callsphere.ai/blog/mcp-protocol-deep-dive-json-rpc-foundation
- 2: https://modelcontextprotocol.io/specification/draft/basic/transports/stdio
- 3: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/2025-11-25/docs/specification/2025-11-25/basic/transports.mdx
- 4: https://modelcontextprotocol.io/specification/2024-11-05/basic/transports
- 5: https://mcp.mintlify.app/specification/draft/server/utilities/logging
- 6: https://modelcontextprotocol.io/specification/draft/basic
- 7: https://modelcontextprotocol.io/specification/draft/changelog
- 8: https://modelcontextprotocol.io/specification/2024-11-05/basic/utilities/progress
- 9: https://modelcontextprotocol.io/specification/2025-11-25/schema.md
Match stdio responses by JSON-RPC id utils/mcp_client.py:384-395 _request consumes the next framed stdout message as the result, so a progress/log notification or another interleaved response can be misattributed. Read until the message id matches the request id, and skip unrelated frames.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 390-390: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 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 384 - 395, The _request method in
mcp_client.py currently assumes the next framed stdout message is always the
matching JSON-RPC response, which can misattribute interleaved notifications or
other replies. Update _request to use the request id returned by
self._state.request(method, params), then keep reading from self._process.stdout
with read_stdio_message until a frame with a matching id is found, ignoring
unrelated notifications or responses. Make sure the final _extract_result call
only runs on the response whose id matches the original request.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Superseded by the master feature PR #34 — this branch is 100% contained in it (0 commits outside). Consolidating review there. |
1 participant
Adds external prompt data sources, live remote MCP tool execution, speech speed controls, committed Speaking Bot OpenAPI snapshot, and security hardening for public MCP inputs/URL fetching.\n\nValidation:\n- nix develop -c poetry run python -m unittest tests.test_mcp_client tests.test_models tests.test_prompt_context tests.test_runtime tests.test_openapi_snapshot\n- nix develop -c ruff check app/models.py app/services/prompt_context.py utils/mcp_client.py scripts/meetingbaas.py tests/test_mcp_client.py tests/test_models.py tests/test_prompt_context.py tests/test_openapi_snapshot.py scripts/export_openapi.py