[Test Improver] test: add unit tests for install/mcp_warnings.py (0% -> ~100%)#828
Open
danielmeppiel wants to merge 2 commits intomainfrom
Open
Conversation
Cover all three public-facing functions in the F5/F7 MCP safety warning module: _is_internal_or_metadata_host, warn_ssrf_url, and warn_shell_metachars. 50 new tests across four test classes. Key scenarios covered: - Loopback, link-local, RFC1918 private, and cloud metadata IPs - IPv6 literals (raw and bracket-quoted) - Hostname resolution to internal/public addresses - Hostname resolution failures (OSError, UnicodeError) - SSRF warnings for all internal URL patterns - No warning for public URLs - Shell metachar warnings for all nine metachar tokens - Multiple env keys each warned independently - None/integer env values do not crash - Non-string command skipped gracefully Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…2-c4c01f3f5ab2590c
Contributor
There was a problem hiding this comment.
Pull request overview
Adds unit test coverage for apm_cli.install.mcp_warnings (install-time, non-blocking safety warnings for SSRF-ish URLs and shell metacharacters) to move the module from untested to effectively fully covered.
Changes:
- Adds branch-complete tests for
_is_internal_or_metadata_host(IP literals, bracketed IPv6, hostname resolution, and resolution failures). - Adds tests for
warn_ssrf_urlcovering internal/metadata/private/public URLs plus malformed/no-host cases. - Adds tests for
warn_shell_metacharsacross all metacharacter tokens, env typing edge-cases, and command handling.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/install/test_mcp_warnings.py | New unit test module covering F5/F7 warning helpers and host classification logic. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
Comment on lines
+128
to
+134
| def test_internal_url_warns(self): | ||
| logger = self._make_logger() | ||
| warn_ssrf_url("http://127.0.0.1:8080/api", logger) | ||
| logger.warning.assert_called_once() | ||
| msg = logger.warning.call_args[0][0] | ||
| assert "127.0.0.1" in msg | ||
|
|
There was a problem hiding this comment.
Avoid substring-matching a URL/host inside the warning text (assert "127.0.0.1" in msg). Our test convention (and CodeQL py/incomplete-url-substring-sanitization) requires parsing the URL from the message (e.g., extract the quoted URL and compare urlparse(...).hostname) or asserting the full expected message shape in a way that doesn't use in against URL-like data.
Comment on lines
+9
to
+13
| import socket | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
There was a problem hiding this comment.
socket and pytest are imported but unused in this test module. Please remove unused imports to keep the test file minimal and avoid future lint/static-analysis noise.
Suggested change
| import socket | |
| from unittest.mock import MagicMock, patch | |
| import pytest | |
| from unittest.mock import MagicMock, patch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Test Improver — automated AI assistant
Goal and rationale
src/apm_cli/install/mcp_warnings.pyhad zero tests despite being a security-critical module. It implements two non-blocking safety checks that fire duringapm install --mcp:warn_ssrf_url— warns when a self-defined remote MCP URL points at loopback, link-local, RFC1918, or cloud metadata addresses (AWS/Azure/GCP IMDS, Alibaba Cloud IMDS).warn_shell_metachars— warns when env values or thecommandfield contain shell metacharacters that would be passed literally throughexecve-style MCP stdio spawning rather than being shell-evaluated.These are the front-line guards against misconfigured MCP server installs surfacing confused/dangerous config to users.
Approach
50 new tests across 4 classes covering every branch:
TestIsInternalOrMetadataHost_is_internal_or_metadata_hostTestWarnSsrfUrlwarn_ssrf_urlTestWarnShellMetacharswarn_shell_metacharsKey scenarios:
[::1],[fc00::1])OSError,UnicodeError) — no crash_SHELL_METACHAR_TOKENS:$(,`,;,&&,||,|,>>,>,<None/integer env values coerced safelycommand(e.g. list) skipped without crashCoverage impact
install/mcp_warnings.pyTest status
Reproducibility