feat: Add comprehensive test suite and enhance core logging/client#6
feat: Add comprehensive test suite and enhance core logging/client#6nkissick-del wants to merge 1 commit intojmagar:mainfrom
Conversation
📝 WalkthroughWalkthroughThe changes add comprehensive test infrastructure including pytest configuration setup, integration tests for GraphQL schema compliance and live queries, unit tests for utilities, and enhance the client logging with query sanitization and variable masking alongside new logging helper functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="unraid_mcp/core/client.py">
<violation number="1" location="unraid_mcp/core/client.py:31">
P2: The query sanitization regex stops at commas (`[^,)]+`), so variable defaults containing commas are only partially masked. This can leave sensitive default values in logs despite the intent to sanitize.</violation>
</file>
<file name="tests/integration/test_schema_compliance.py">
<violation number="1" location="tests/integration/test_schema_compliance.py:202">
P2: extract_root_field returns after the first root field token, so queries with multiple root fields only validate the first field and ignore the rest. This can let invalid root fields pass the compliance check.</violation>
<violation number="2" location="tests/integration/test_schema_compliance.py:262">
P2: Dynamically composed query strings are dropped because any f-string with variables returns `None`, so those queries are never validated by the compliance checker.</violation>
</file>
<file name="unraid_mcp/config/logging.py">
<violation number="1" location="unraid_mcp/config/logging.py:116">
P1: `tracebacks_show_locals=True` prints all local variables in tracebacks, which can leak sensitive secrets into logs. Consider disabling this (or making it opt-in) for production logging.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| show_path=False, | ||
| rich_tracebacks=True, | ||
| tracebacks_show_locals=True | ||
| tracebacks_show_locals=True, |
There was a problem hiding this comment.
P1: tracebacks_show_locals=True prints all local variables in tracebacks, which can leak sensitive secrets into logs. Consider disabling this (or making it opt-in) for production logging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unraid_mcp/config/logging.py, line 116:
<comment>`tracebacks_show_locals=True` prints all local variables in tracebacks, which can leak sensitive secrets into logs. Consider disabling this (or making it opt-in) for production logging.</comment>
<file context>
@@ -111,20 +113,16 @@ def setup_logger(name: str = "UnraidMCPServer") -> logging.Logger:
show_path=False,
rich_tracebacks=True,
- tracebacks_show_locals=True
+ tracebacks_show_locals=True,
)
console_handler.setLevel(numeric_log_level)
</file context>
| tracebacks_show_locals=True, | |
| tracebacks_show_locals=False, |
| # Remove variable definitions and replace with placeholders | ||
| # This is a basic sanitization; for production, consider a proper GraphQL parser | ||
| # Remove variable definitions like $var: Type | ||
| query = re.sub(r"\$[a-zA-Z_][a-zA-Z0-9_]*\s*:\s*[^,)]+", "$VARIABLE", query) |
There was a problem hiding this comment.
P2: The query sanitization regex stops at commas ([^,)]+), so variable defaults containing commas are only partially masked. This can leave sensitive default values in logs despite the intent to sanitize.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unraid_mcp/core/client.py, line 31:
<comment>The query sanitization regex stops at commas (`[^,)]+`), so variable defaults containing commas are only partially masked. This can leave sensitive default values in logs despite the intent to sanitize.</comment>
<file context>
@@ -15,7 +16,21 @@
+ # Remove variable definitions and replace with placeholders
+ # This is a basic sanitization; for production, consider a proper GraphQL parser
+ # Remove variable definitions like $var: Type
+ query = re.sub(r"\$[a-zA-Z_][a-zA-Z0-9_]*\s*:\s*[^,)]+", "$VARIABLE", query)
+ # Truncate to safe length
+ return query[:500]
</file context>
| continue | ||
|
|
||
| # Token found (not an alias) | ||
| return op_type, token |
There was a problem hiding this comment.
P2: extract_root_field returns after the first root field token, so queries with multiple root fields only validate the first field and ignore the rest. This can let invalid root fields pass the compliance check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/integration/test_schema_compliance.py, line 202:
<comment>extract_root_field returns after the first root field token, so queries with multiple root fields only validate the first field and ignore the rest. This can let invalid root fields pass the compliance check.</comment>
<file context>
@@ -0,0 +1,483 @@
+ continue
+
+ # Token found (not an alias)
+ return op_type, token
+
+ return op_type, None
</file context>
| for value in node.values: | ||
| part = self._extract_string(value) | ||
| if part is None: | ||
| return None # Non-string part found (e.g. variable), abort |
There was a problem hiding this comment.
P2: Dynamically composed query strings are dropped because any f-string with variables returns None, so those queries are never validated by the compliance checker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/integration/test_schema_compliance.py, line 262:
<comment>Dynamically composed query strings are dropped because any f-string with variables returns `None`, so those queries are never validated by the compliance checker.</comment>
<file context>
@@ -0,0 +1,483 @@
+ for value in node.values:
+ part = self._extract_string(value)
+ if part is None:
+ return None # Non-string part found (e.g. variable), abort
+ parts.append(part)
+ return "".join(parts)
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
unraid_mcp/config/logging.py (2)
49-87:⚠️ Potential issue | 🟡 MinorFix size check to work with lazy stream initialization.
When
delay=True,self.streamisNoneon firstemit()call, causing the size check to be skipped. The first write occurs without validation, allowing the log file to exceedmax_bytes.Change the condition to check
self.baseFilenameinstead, which is always set by the parent class regardless of stream state:Suggested fix
- if self.stream and hasattr(self.stream, "name"): + if hasattr(self, "baseFilename"):
161-196:⚠️ Potential issue | 🟠 MajorMultiple
OverwriteFileHandlerinstances targeting the same log file creates a race condition.When
fastmcp_loggerandroot_loggerwrite through separate handler instances, each maintains its own file stream. When one handler detects the file has reached the 10MB cap, it closes its stream and deletes the file viaos.remove(). The other handler's stream reference becomes stale, risking log loss or corruption on subsequent writes. Share a single handler instance between both loggers instead.🛠️ Suggested fix (share one handler)
- file_handler = OverwriteFileHandler(LOG_FILE_PATH, max_bytes=10 * 1024 * 1024, encoding="utf-8") - file_handler.setLevel(numeric_log_level) - file_formatter = logging.Formatter( - "%(asctime)s - %(name)s - %(levelname)s - %(module)s - %(funcName)s - %(lineno)d - %(message)s" - ) - file_handler.setFormatter(file_formatter) - fastmcp_logger.addHandler(file_handler) + shared_file_handler = OverwriteFileHandler( + LOG_FILE_PATH, max_bytes=10 * 1024 * 1024, encoding="utf-8" + ) + shared_file_handler.setLevel(numeric_log_level) + file_formatter = logging.Formatter( + "%(asctime)s - %(name)s - %(levelname)s - %(module)s - %(funcName)s - %(lineno)d - %(message)s" + ) + shared_file_handler.setFormatter(file_formatter) + fastmcp_logger.addHandler(shared_file_handler) @@ - root_file_handler = OverwriteFileHandler( - LOG_FILE_PATH, max_bytes=10 * 1024 * 1024, encoding="utf-8" - ) - root_file_handler.setLevel(numeric_log_level) - root_file_handler.setFormatter(file_formatter) - root_logger.addHandler(root_file_handler) + root_logger.addHandler(shared_file_handler)
🤖 Fix all issues with AI agents
In `@tests/conftest.py`:
- Around line 5-9: Rename the unused parameter in the pytest_configure hook from
config to _config to satisfy Ruff ARG001; update the function signature def
pytest_configure(config): to def pytest_configure(_config): and ensure any
references (there are none) would be updated accordingly so the hook still
matches pytest's expected signature while signaling the parameter is
intentionally unused.
In `@tests/integration/test_live_queries.py`:
- Around line 11-13: Replace the sys.path.append usage that adds PROJECT_ROOT
with sys.path.insert(0, str(PROJECT_ROOT)) so the local working-tree package
(PROJECT_ROOT) takes precedence over any installed packages; update the code
around the PROJECT_ROOT variable and the import modification (where
sys.path.append(str(PROJECT_ROOT)) is used) to use sys.path.insert(0,
str(PROJECT_ROOT)).
In `@tests/integration/test_schema_compliance.py`:
- Around line 238-248: The current heuristic in _check_value (which calls
_extract_string and works with val/stripped) uses startswith("query "), etc.,
and misses forms like "query($var...)" or "query\nName"; change the check to use
a word-boundary regex so it matches operations regardless of trailing space
(e.g., use re.match(r'^(query|mutation|subscription)\b', stripped)) and ensure
you import re at the top of the test file; update the conditional that currently
checks stripped.startswith(...) and "{" in stripped to use the new regex match
plus the existing "{" check.
- Around line 30-32: The test currently uses sys.path.append(PROJECT_ROOT) which
puts the repo at the end of sys.path and can cause an installed unraid_mcp
package to shadow the working tree; update the import prep to insert the project
root at the front of sys.path (use sys.path.insert(0, str(PROJECT_ROOT))) so the
local PROJECT_ROOT takes precedence—follow the same pattern used in
tests/conftest.py and replace the sys.path.append call accordingly (identify the
code around PROJECT_ROOT and sys.path.append to change).
In `@tests/unit/test_utils.py`:
- Around line 31-43: The format_kb function currently calls int(k) without
guarding against non-numeric or infinite values; update the code around the
int(k) conversion in format_kb to wrap it in a try-except that catches
TypeError, ValueError, and OverflowError and on those exceptions returns str(k)
(preserving "inf" and arbitrary non-numeric strings), while keeping the existing
special-case handling for None to return "N/A"; ensure the conversion fallback
prevents the tests for float("inf") and "not_a_number" from raising.
In `@unraid_mcp/core/client.py`:
- Around line 22-33: The sanitize_query function currently only masks GraphQL
variable definitions; update sanitize_query to also mask string literals to
prevent inline secrets by first replacing block strings (triple-quoted GraphQL
block strings) and then normal quoted strings (handling escaped quotes) with a
placeholder like "$STRING", keeping the existing variable-definition masking and
truncation behavior; reference the sanitize_query function and ensure the
replacement order handles block strings before normal strings to avoid partial
matches.
| def pytest_configure(config): | ||
| """Add project root to sys.path so tests can import the package.""" | ||
| project_root = Path(__file__).parent.parent | ||
| if str(project_root) not in sys.path: | ||
| sys.path.insert(0, str(project_root)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "conftest.py" -type f 2>/dev/null | head -20Repository: jmagar/unraid-mcp
Length of output: 43
🏁 Script executed:
cat -n tests/conftest.py 2>/dev/null || echo "File not found in root"Repository: jmagar/unraid-mcp
Length of output: 399
🏁 Script executed:
# Check if this is a pytest project and get more context
ls -la tests/ 2>/dev/null | head -20Repository: jmagar/unraid-mcp
Length of output: 342
🏁 Script executed:
# Search for pytest_configure usage in the codebase
rg "pytest_configure" --type pyRepository: jmagar/unraid-mcp
Length of output: 108
🏁 Script executed:
# Look for ruff configuration files
find . -type f \( -name "pyproject.toml" -o -name "ruff.toml" -o -name ".ruff.toml" \) | head -10Repository: jmagar/unraid-mcp
Length of output: 89
🏁 Script executed:
# Check for any existing noqa comments in the file
cat -n tests/conftest.pyRepository: jmagar/unraid-mcp
Length of output: 399
🏁 Script executed:
# Check if there are any ruff rules configured in pyproject.toml
fd "pyproject.toml" -x cat {} \; 2>/dev/null | head -100Repository: jmagar/unraid-mcp
Length of output: 2296
🏁 Script executed:
cat -n ruff.tomlRepository: jmagar/unraid-mcp
Length of output: 2286
🏁 Script executed:
# Check if ARG rule is enabled in ruff config
cat ruff.toml && echo "---" && cat pyproject.toml | grep -A 20 "\[tool.ruff"Repository: jmagar/unraid-mcp
Length of output: 2806
Rename unused config parameter to silence Ruff ARG001.
The pytest_configure hook requires the parameter by signature, but config is unused in the function body. Rename it to _config to follow Python convention for intentionally unused parameters.
Suggested fix
-def pytest_configure(config):
+def pytest_configure(_config):🧰 Tools
🪛 Ruff (0.14.14)
[warning] 5-5: Unused function argument: config
(ARG001)
🤖 Prompt for AI Agents
In `@tests/conftest.py` around lines 5 - 9, Rename the unused parameter in the
pytest_configure hook from config to _config to satisfy Ruff ARG001; update the
function signature def pytest_configure(config): to def
pytest_configure(_config): and ensure any references (there are none) would be
updated accordingly so the hook still matches pytest's expected signature while
signaling the parameter is intentionally unused.
| # Add project root to path | ||
| PROJECT_ROOT = Path(__file__).parent.parent.parent | ||
| sys.path.append(str(PROJECT_ROOT)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the target file
find . -name "test_live_queries.py" -type fRepository: jmagar/unraid-mcp
Length of output: 101
🏁 Script executed:
# Also search for sys.path usage patterns in tests directory
rg "sys\.path" tests/ -A 2 -B 2Repository: jmagar/unraid-mcp
Length of output: 1197
🏁 Script executed:
# Check if there's an installed unraid_mcp package referenced
rg "unraid_mcp" . --include="*.py" | head -20Repository: jmagar/unraid-mcp
Length of output: 502
🏁 Script executed:
# Read the actual test_live_queries.py file to see line numbers and full context
head -20 tests/integration/test_live_queries.pyRepository: jmagar/unraid-mcp
Length of output: 534
🏁 Script executed:
# Check the conftest.py to understand the import setup
cat tests/conftest.pyRepository: jmagar/unraid-mcp
Length of output: 336
🏁 Script executed:
# Check if there's a setup.py or pyproject.toml to understand package structure
ls -la | grep -E "setup|pyproject"Repository: jmagar/unraid-mcp
Length of output: 126
🏁 Script executed:
# Verify pyproject.toml to confirm unraid_mcp is a real package that could be installed
cat pyproject.toml | head -30Repository: jmagar/unraid-mcp
Length of output: 957
🏁 Script executed:
# Check if there are any other sys.path manipulations in integration tests
rg "sys\.path" tests/integration/ -A 1 -B 1Repository: jmagar/unraid-mcp
Length of output: 486
Use sys.path.insert(0, ...) to ensure the local package takes precedence on import.
sys.path.append() places the project root after site-packages, allowing an installed unraid_mcp package to be imported instead of the working tree version. This matters if the test is run directly as a script (bypassing pytest's conftest). Use insert(0, ...) instead.
🛠️ Suggested fix
-sys.path.append(str(PROJECT_ROOT))
+sys.path.insert(0, str(PROJECT_ROOT))📝 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.
| # Add project root to path | |
| PROJECT_ROOT = Path(__file__).parent.parent.parent | |
| sys.path.append(str(PROJECT_ROOT)) | |
| # Add project root to path | |
| PROJECT_ROOT = Path(__file__).parent.parent.parent | |
| sys.path.insert(0, str(PROJECT_ROOT)) |
🤖 Prompt for AI Agents
In `@tests/integration/test_live_queries.py` around lines 11 - 13, Replace the
sys.path.append usage that adds PROJECT_ROOT with sys.path.insert(0,
str(PROJECT_ROOT)) so the local working-tree package (PROJECT_ROOT) takes
precedence over any installed packages; update the code around the PROJECT_ROOT
variable and the import modification (where sys.path.append(str(PROJECT_ROOT))
is used) to use sys.path.insert(0, str(PROJECT_ROOT)).
| # Add project root to path to allow imports if needed, though we primarily parse text | ||
| PROJECT_ROOT = Path(__file__).parent.parent.parent | ||
| sys.path.append(str(PROJECT_ROOT)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/integration/test_schema_compliance.py | head -40Repository: jmagar/unraid-mcp
Length of output: 1507
🏁 Script executed:
rg "sys.path\.(append|insert)" --type pyRepository: jmagar/unraid-mcp
Length of output: 277
🏁 Script executed:
rg "sys.path.insert" -B 3 -A 3 tests/conftest.pyRepository: jmagar/unraid-mcp
Length of output: 268
Ensure the local package takes precedence on import.
sys.path.append places the repo after site-packages, so an installed unraid_mcp could be imported instead of the working tree. Use sys.path.insert(0, ...) to match the pattern already established in tests/conftest.py.
🛠️ Suggested fix
-sys.path.append(str(PROJECT_ROOT))
+sys.path.insert(0, str(PROJECT_ROOT))🤖 Prompt for AI Agents
In `@tests/integration/test_schema_compliance.py` around lines 30 - 32, The test
currently uses sys.path.append(PROJECT_ROOT) which puts the repo at the end of
sys.path and can cause an installed unraid_mcp package to shadow the working
tree; update the import prep to insert the project root at the front of sys.path
(use sys.path.insert(0, str(PROJECT_ROOT))) so the local PROJECT_ROOT takes
precedence—follow the same pattern used in tests/conftest.py and replace the
sys.path.append call accordingly (identify the code around PROJECT_ROOT and
sys.path.append to change).
| def _check_value(self, node, lineno): | ||
| val = self._extract_string(node) | ||
| if val: | ||
| stripped = val.strip() | ||
| # Simple heuristic: must look like a GraphQL operation or shorthand | ||
| if ( | ||
| stripped.startswith("query ") | ||
| or stripped.startswith("mutation ") | ||
| or stripped.startswith("subscription ") | ||
| or stripped.startswith("{") | ||
| ) and "{" in stripped: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Query discovery misses operations without a trailing space.
startswith("query ") won’t match query($var: ...) or query\nName, so some queries may be skipped. Use a keyword match to capture those formats.
🛠️ Suggested fix
- if (
- stripped.startswith("query ")
- or stripped.startswith("mutation ")
- or stripped.startswith("subscription ")
- or stripped.startswith("{")
- ) and "{" in stripped:
+ if (
+ re.match(r"^(query|mutation|subscription)\b", stripped)
+ or stripped.startswith("{")
+ ) and "{" in stripped:🛠️ Add import
-import ast
+import ast
+import re🧰 Tools
🪛 Ruff (0.14.14)
[warning] 238-238: Missing return type annotation for private function _check_value
Add return type annotation: None
(ANN202)
[warning] 244-247: Call startswith once with a tuple
Merge into a single startswith call
(PIE810)
🤖 Prompt for AI Agents
In `@tests/integration/test_schema_compliance.py` around lines 238 - 248, The
current heuristic in _check_value (which calls _extract_string and works with
val/stripped) uses startswith("query "), etc., and misses forms like
"query($var...)" or "query\nName"; change the check to use a word-boundary regex
so it matches operations regardless of trailing space (e.g., use
re.match(r'^(query|mutation|subscription)\b', stripped)) and ensure you import
re at the top of the test file; update the conditional that currently checks
stripped.startswith(...) and "{" in stripped to use the new regex match plus the
existing "{" check.
| def test_format_kb(self): | ||
| """ | ||
| Verify the logic of the real format_kb function directly. | ||
| """ | ||
| # Test overflow/special cases | ||
| self.assertEqual(format_kb(float("inf")), "inf") | ||
| self.assertEqual(format_kb("not_a_number"), "not_a_number") | ||
| self.assertEqual(format_kb(None), "N/A") | ||
|
|
||
| # Test formatting | ||
| self.assertEqual(format_kb(1024), "1.00 MB") | ||
| self.assertEqual(format_kb(1024 * 1024), "1.00 GB") | ||
| self.assertEqual(format_kb(512), "512 KB") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and inspect the format_kb function
rg -n "def format_kb" -A 15Repository: jmagar/unraid-mcp
Length of output: 1274
🏁 Script executed:
# Check the test file to confirm the assertions
cat -n tests/unit/test_utils.py | sed -n '31,43p'Repository: jmagar/unraid-mcp
Length of output: 674
🏁 Script executed:
# Look for the format_kb import in the test file
rg -n "from.*format_kb|import.*format_kb" tests/unit/test_utils.py -B 2 -A 2Repository: jmagar/unraid-mcp
Length of output: 250
format_kb needs error handling to make the edge-case tests pass.
The function uses int() without error handling on line 114 of unraid_mcp/tools/system.py. This will raise OverflowError for float("inf") and ValueError for non-numeric strings like "not_a_number", causing the test assertions on lines 36–37 to fail.
Wrap the int(k) call in a try-except block to catch TypeError, ValueError, and OverflowError, returning str(k) as a fallback:
Suggested fix
def format_kb(k: Any) -> str:
if k is None:
return "N/A"
- k = int(k) # Values are strings in SDL for PrefixedID containing types like capacity
+ try:
+ k = int(k) # Values are strings in SDL for PrefixedID containing types like capacity
+ except (TypeError, ValueError, OverflowError):
+ return str(k)🧰 Tools
🪛 Ruff (0.14.14)
[warning] 36-36: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 37-37: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 38-38: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 41-41: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 42-42: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
[warning] 43-43: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
In `@tests/unit/test_utils.py` around lines 31 - 43, The format_kb function
currently calls int(k) without guarding against non-numeric or infinite values;
update the code around the int(k) conversion in format_kb to wrap it in a
try-except that catches TypeError, ValueError, and OverflowError and on those
exceptions returns str(k) (preserving "inf" and arbitrary non-numeric strings),
while keeping the existing special-case handling for None to return "N/A";
ensure the conversion fallback prevents the tests for float("inf") and
"not_a_number" from raising.
| def sanitize_query(query: str) -> str: | ||
| """Remove potential secrets from query string before logging. | ||
|
|
||
| Primary secret protection is performed by variables masking. This function | ||
| serves as defense-in-depth for type naming and definitions. | ||
| """ | ||
| # Remove variable definitions and replace with placeholders | ||
| # This is a basic sanitization; for production, consider a proper GraphQL parser | ||
| # Remove variable definitions like $var: Type | ||
| query = re.sub(r"\$[a-zA-Z_][a-zA-Z0-9_]*\s*:\s*[^,)]+", "$VARIABLE", query) | ||
| # Truncate to safe length | ||
| return query[:500] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "client.py" -path "*/unraid_mcp/core/*"Repository: jmagar/unraid-mcp
Length of output: 88
🏁 Script executed:
# Read the specific file to see the current implementation
cat -n unraid_mcp/core/client.py | head -50Repository: jmagar/unraid-mcp
Length of output: 2156
🏁 Script executed:
# Search for usage of sanitize_query to understand context
rg "sanitize_query" -B 2 -A 2Repository: jmagar/unraid-mcp
Length of output: 593
🏁 Script executed:
# Look for tests of sanitize_query
find . -name "*test*.py" -type fRepository: jmagar/unraid-mcp
Length of output: 194
🏁 Script executed:
# Search for any tests related to sanitize_query
rg "sanitize_query" --include="*test*.py"Repository: jmagar/unraid-mcp
Length of output: 502
🏁 Script executed:
# Check if there are any other security measures in the codebase for logging
rg "logger.debug.*query|logger.info.*query" -B 2 -A 2Repository: jmagar/unraid-mcp
Length of output: 2157
🏁 Script executed:
# Look at the full context of how sanitized queries are logged
rg "sanitized = sanitize_query" -B 5 -A 5Repository: jmagar/unraid-mcp
Length of output: 790
🏁 Script executed:
# Test the suggested regex pattern to understand what it captures
python3 <<'EOF'
import re
# Test cases
test_queries = [
'mutation { login(password: "secret123") }',
'query { user(id: "123", token: "abc") }',
'mutation { create(data: """multiline\nstring""") }',
'query { search(term: "hello\\"world") }', # escaped quote
]
pattern = r'""".*?"""|"(?:\\.|[^"\\])*"'
for query in test_queries:
print(f"Original: {query}")
masked = re.sub(pattern, '""', query, flags=re.DOTALL)
print(f"Masked: {masked}")
print()
EOFRepository: jmagar/unraid-mcp
Length of output: 439
🏁 Script executed:
# Check if there are existing tests for the sanitize_query function
cat -n ./tests/unit/test_utils.py | head -100Repository: jmagar/unraid-mcp
Length of output: 1942
Add string literal masking to prevent inline secrets from reaching logs.
The current regex only removes GraphQL variable definitions ($var: Type). Inline argument values like password: "secret" remain exposed in logs. Add masking for string literals (including escaped quotes and block strings):
Suggested fix
# Remove variable definitions like $var: Type
query = re.sub(r"\$[a-zA-Z_][a-zA-Z0-9_]*\s*:\s*[^,)]+", "$VARIABLE", query)
+ # Replace string literals (including block strings) to avoid inline secret leakage
+ query = re.sub(r'""".*?"""|"(?:\\.|[^"\\])*"', '""', query, flags=re.DOTALL)
# Truncate to safe length
return query[:500]🤖 Prompt for AI Agents
In `@unraid_mcp/core/client.py` around lines 22 - 33, The sanitize_query function
currently only masks GraphQL variable definitions; update sanitize_query to also
mask string literals to prevent inline secrets by first replacing block strings
(triple-quoted GraphQL block strings) and then normal quoted strings (handling
escaped quotes) with a placeholder like "$STRING", keeping the existing
variable-definition masking and truncation behavior; reference the
sanitize_query function and ensure the replacement order handles block strings
before normal strings to avoid partial matches.
1 participant
Overview
This PR introduces a comprehensive testing framework to ensure tool stability and improves security in the core client.
Changes
Testing Framework
Core Client Security
Verification
Summary by cubic
Adds a comprehensive test suite and hardens core client logging to catch API mismatches early and prevent secrets from appearing in logs.
Written for commit 5f48d25. Summary will update on new commits.
Summary by CodeRabbit
Tests
New Features
Bug Fixes