Fixes #242 - feat(client): optional ZAMMAD_INSECURE for TLS verification bypass#243
Open
Leafgard wants to merge 2 commits intobasher83:mainfrom
Open
Fixes #242 - feat(client): optional ZAMMAD_INSECURE for TLS verification bypass#243Leafgard wants to merge 2 commits intobasher83:mainfrom
Leafgard wants to merge 2 commits intobasher83:mainfrom
Conversation
- Set requests session verify=False using zammad-py 3.x ZammadAPI.session - Fallback for older _connection.session layout; clear error if unsupported - Document ZAMMAD_INSECURE in README and .env.example; add tests
Contributor
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
TIP This summary will be updated as you push new changes. Give us feedback
Author
|
Fixes #242 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcp_zammad/client.py (1)
18-26: 🧹 Nitpick | 🔵 TrivialMake
insecurekeyword-only to avoid boolean positional trap (FBT001).A boolean positional parameter invites silent misuse at call sites (e.g.,
ZammadClient(url, user, pw, tok, oauth, True)is unreadable and order-sensitive). Since this flag meaningfully weakens security guarantees, make it keyword-only so callers must spell it out.♻️ Proposed refactor
def __init__( self, url: str | None = None, username: str | None = None, password: str | None = None, http_token: str | None = None, oauth2_token: str | None = None, + *, insecure: bool | None = None, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/client.py` around lines 18 - 26, The __init__ signature for ZammadClient (the __init__ method) currently accepts insecure as a positional boolean which can be accidentally passed positionally; make insecure a keyword-only argument to avoid the boolean positional trap by moving it after a bare * (e.g., change the parameter list so insecure is declared after * or add * before insecure) so callers must pass insecure=..., and update any internal references or tests that call ZammadClient(...) positionally to use the keyword form.README.md (1)
82-88:⚠️ Potential issue | 🟠 MajorDon't put
ZAMMAD_INSECURE=truein the "Basic usage" Docker example.This section is labeled "Basic usage" and is the first command users copy-paste. Including
-e ZAMMAD_INSECURE=trueby default trains readers to disable TLS verification as a normal step, which directly contradicts the "NOT recommended for production" warning elsewhere in the README and the PR's security intent (self-signed/internal certs only).Keep the basic example strictly secure-by-default, and document the insecure flag in a separate, clearly caveated subsection (the Troubleshooting entry at line 537 already does this well).
🛡️ Proposed fix
# Basic usage with environment variables docker run --rm -i \ -e ZAMMAD_URL=https://your-instance.zammad.com/api/v1 \ -e ZAMMAD_HTTP_TOKEN=your-api-token \ - -e ZAMMAD_INSECURE=true \ ghcr.io/basher83/zammad-mcp:latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 82 - 88, The "Basic usage" Docker example currently includes the environment variable ZAMMAD_INSECURE=true which encourages disabling TLS; remove the -e ZAMMAD_INSECURE=true entry from the basic docker run snippet so the example is secure-by-default, and instead add or update a separate clearly labeled "Insecure / Troubleshooting" subsection that documents ZAMMAD_INSECURE (its purpose, caveats, and NOT recommended for production) — search for the Docker run example and the ZAMMAD_INSECURE env var in README.md to make the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 18-20: The example in .env.example is misleading because
_parse_bool_env treats only {"1","true","yes","on"} as truthy; change the
commented example from "# ZAMMAD_INSECURE=false" to an opt-in truthy example
(e.g., "# ZAMMAD_INSECURE=true") and update the explanatory comment to state
that only values "1/true/yes/on" enable insecure TLS, referencing the
ZAMMAD_INSECURE variable and the _parse_bool_env behavior so users know to set a
truthy value to opt in.
In `@mcp_zammad/client.py`:
- Around line 142-146: The method _parse_bool_env currently defined as an
instance method but never uses self; convert it into a `@staticmethod` by adding
the `@staticmethod` decorator and removing the implicit self parameter from its
signature so it accepts only env_var: str, and update any internal calls (e.g.,
self._parse_bool_env(...) -> ClassName._parse_bool_env(...) or call via self
still allowed but not required) to match the new static signature; ensure the
docstring remains and tests import/instantiate accordingly.
- Around line 72-85: The call to
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) creates a
process-wide side effect — remove that global suppression and instead either (A)
simply set session.verify = False and keep the logger.warning("TLS certificate
verification is disabled (ZAMMAD_INSECURE=true).") so we document the insecure
mode without muting urllib3 globally, or (B) if you must suppress warnings only
for this client, replace the global call with a scoped suppression (e.g., a
warnings.filterwarnings context scoped to
urllib3.exceptions.InsecureRequestWarning around calls that use
self.api.session) so that urllib3.disable_warnings is not invoked; update
handling around self.insecure, session.verify, and the ConfigException logic
accordingly and document the chosen approach for ZAMMAD_INSECURE.
In `@README.md`:
- Around line 166-167: Update the README's example for disabling TLS
verification to match the parser behavior and the corrected .env.example by
showing the opt-in form ZAMMAD_INSECURE=true (instead of the misleading
ZAMMAD_INSECURE=false), and add a short parenthetical note clarifying that only
1/true/yes/on are treated as truthy by the parser so users must set
ZAMMAD_INSECURE=true to enable insecure TLS.
In `@tests/test_client.py`:
- Around line 62-90: Tests only cover the happy path for TLS/insecure handling;
add parametrized tests that exercise the fallback and error branches in
mcp_zammad.client: create a test where ZammadAPI (mock_api.return_value) has
session set to None and has _connection.session (ensure fallback_session.verify
gets set to False when ZammadClient(..., insecure=True) is instantiated), add a
test where both session and _connection are None and assert ZammadClient(...,
insecure=True) raises ConfigException with a message about not exposing a
requests session, and add an assertion that urllib3.disable_warnings is
called/logged when insecure=True; target symbols: ZammadClient, ZammadAPI,
ConfigException, session, _connection.session, and urllib3.disable_warnings.
---
Outside diff comments:
In `@mcp_zammad/client.py`:
- Around line 18-26: The __init__ signature for ZammadClient (the __init__
method) currently accepts insecure as a positional boolean which can be
accidentally passed positionally; make insecure a keyword-only argument to avoid
the boolean positional trap by moving it after a bare * (e.g., change the
parameter list so insecure is declared after * or add * before insecure) so
callers must pass insecure=..., and update any internal references or tests that
call ZammadClient(...) positionally to use the keyword form.
In `@README.md`:
- Around line 82-88: The "Basic usage" Docker example currently includes the
environment variable ZAMMAD_INSECURE=true which encourages disabling TLS; remove
the -e ZAMMAD_INSECURE=true entry from the basic docker run snippet so the
example is secure-by-default, and instead add or update a separate clearly
labeled "Insecure / Troubleshooting" subsection that documents ZAMMAD_INSECURE
(its purpose, caveats, and NOT recommended for production) — search for the
Docker run example and the ZAMMAD_INSECURE env var in README.md to make the
edits.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f012dd2-ad72-49ac-93be-435e099ef89b
📒 Files selected for processing (4)
.env.exampleREADME.mdmcp_zammad/client.pytests/test_client.py
- Clarify truthy env values and opt-in examples in docs - Remove process-wide urllib3.disable_warnings; expand TLS bypass log note - Make _parse_bool_env a staticmethod; use msg var for ConfigException (TRY003) - Add tests for _connection session fallback and missing-session error - Docker README: omit insecure flag from default run example
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_zammad/client.py (1)
17-32:⚠️ Potential issue | 🟠 MajorMake the
insecureparameter keyword-only and complete the public API signature.
insecureis a boolean argument at line 24 flagged by Ruff FBT001 for being positional, which invites accidental misuse when multiple positional arguments are passed. Add*beforeinsecure, annotate__init__with-> Nonereturn type (required by guidelines for all functions), and document the parameter in the docstring.♻️ Proposed changes
def __init__( self, url: str | None = None, username: str | None = None, password: str | None = None, http_token: str | None = None, oauth2_token: str | None = None, + *, insecure: bool | None = None, - ): + ) -> None: """Initialize Zammad client with environment variables or provided credentials. Supports reading secrets from files using Docker secrets pattern: - ZAMMAD_HTTP_TOKEN_FILE: Path to file containing HTTP token - ZAMMAD_OAUTH2_TOKEN_FILE: Path to file containing OAuth2 token - ZAMMAD_PASSWORD_FILE: Path to file containing password + + Set insecure=True, or set ZAMMAD_INSECURE to 1/true/yes/on, only for trusted + self-signed/internal certificate chains. """This addresses FBT001 linting error and aligns with guidelines: "Use type hints for all functions and methods" and "Add type hints and docstrings to all public APIs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/client.py` around lines 17 - 32, Update the public API of the constructor: make the `insecure` parameter keyword-only by inserting a `*` before it in the `__init__` signature, add an explicit return type annotation `-> None` to `__init__`, and update the constructor docstring to document the `insecure` parameter (its purpose and default behavior). Target the `__init__` method where `insecure` is declared so the signature and docstring match the project linting guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_client.py`:
- Around line 66-83: Parametrize the test_client_insecure_mode_from_env to
iterate the documented truthy values ("1", "true", "yes", "on"): use
pytest.mark.parametrize to supply a variable (e.g., truthy) and replace the
hardcoded "ZAMMAD_INSECURE" value in the patch.dict with that variable; keep the
current mocks (patch("mcp_zammad.client.ZammadAPI") / mock_api and
mock_instance) and the assertions (client.insecure is True and
mock_instance.session.verify is False) so the test verifies all documented
truthy env inputs for ZAMMAD_INSECURE against the ZammadClient behavior.
- Around line 11-12: Rename the test placeholder variable `_TEST_HTTP_TOKEN` to
`_TEST_AUTH_VALUE` and update all its usages (currently referenced in tests at
the locations corresponding to the original lines 75, 92, 112, and 130) to the
new name; ensure imports/locals in tests/test_client.py and any assertions or
kwargs that pass `_TEST_HTTP_TOKEN` (e.g., http_token=...) are updated to use
`_TEST_AUTH_VALUE` so Ruff S105 no longer flags the identifier.
---
Outside diff comments:
In `@mcp_zammad/client.py`:
- Around line 17-32: Update the public API of the constructor: make the
`insecure` parameter keyword-only by inserting a `*` before it in the `__init__`
signature, add an explicit return type annotation `-> None` to `__init__`, and
update the constructor docstring to document the `insecure` parameter (its
purpose and default behavior). Target the `__init__` method where `insecure` is
declared so the signature and docstring match the project linting guidelines.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c9213a4-c287-4769-9a8e-bf6e57e860f4
📒 Files selected for processing (4)
.env.exampleREADME.mdmcp_zammad/client.pytests/test_client.py
Comment on lines
+11
to
+12
| # Non-secret placeholder for tests (avoids bandit S106 on http_token kwargs) | ||
| _TEST_HTTP_TOKEN = "test-http-token" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect: no S105/S106 findings in the updated test file.
ruff check tests/test_client.py --select S105,S106Repository: basher83/Zammad-MCP
Length of output: 961
🏁 Script executed:
# Check current state of tests/test_client.py - focus on variable usage
rg "_TEST_HTTP_TOKEN|http_token.*test" tests/test_client.py -nRepository: basher83/Zammad-MCP
Length of output: 336
🏁 Script executed:
# Read the relevant sections to verify all usages
cat -n tests/test_client.py | sed -n '10,20p'Repository: basher83/Zammad-MCP
Length of output: 495
🏁 Script executed:
# Also check the test function context around lines mentioned in "Also applies to"
cat -n tests/test_client.py | sed -n '65,75p'Repository: basher83/Zammad-MCP
Length of output: 509
🏁 Script executed:
# Check lines around 90-95
cat -n tests/test_client.py | sed -n '85,95p'Repository: basher83/Zammad-MCP
Length of output: 611
🏁 Script executed:
# Check lines around 110-115
cat -n tests/test_client.py | sed -n '105,115p'Repository: basher83/Zammad-MCP
Length of output: 444
🏁 Script executed:
# Check lines around 125-135
cat -n tests/test_client.py | sed -n '125,135p'Repository: basher83/Zammad-MCP
Length of output: 444
🏁 Script executed:
# View the env dict section that contains the literal "test-token"
cat -n tests/test_client.py | sed -n '70,82p'Repository: basher83/Zammad-MCP
Length of output: 457
🏁 Script executed:
# Create a test file with proposed changes and verify Ruff S105/S106 are resolved
cat > /tmp/test_changes.txt << 'EOF'
# Proposed changes to validate fix
- Line 12: _TEST_HTTP_TOKEN → _TEST_AUTH_VALUE
- Line 75: "ZAMMAD_HTTP_TOKEN": "test-token" → "ZAMMAD_HTTP_TOKEN": _TEST_AUTH_VALUE
- Line 92: _TEST_HTTP_TOKEN → _TEST_AUTH_VALUE
- Line 112: _TEST_HTTP_TOKEN → _TEST_AUTH_VALUE
- Line 130: _TEST_HTTP_TOKEN → _TEST_AUTH_VALUE
EOF
cat /tmp/test_changes.txtRepository: basher83/Zammad-MCP
Length of output: 374
🏁 Script executed:
# Verify the variable name itself won't trigger S105 by checking Ruff's detection logic
# A variable like _TEST_AUTH_VALUE should not trigger S105 as it doesn't contain "password", "token", "secret" etc.
# Let's create a minimal test to confirm
cd /tmp && python3 << 'PYEOF'
import re
# Ruff S105 typically detects variable names containing patterns like:
# password, token, secret, api_key, etc. (case-insensitive)
suspicious_patterns = ['password', 'token', 'secret', 'api_key', 'apikey', 'auth']
test_names = [
"_TEST_HTTP_TOKEN", # Original - contains "TOKEN"
"_TEST_AUTH_VALUE", # Proposed - contains "AUTH" but less specific
"TEST_AUTH_PLACEHOLDER", # Alternative
]
for name in test_names:
matches = [p for p in suspicious_patterns if p.lower() in name.lower()]
print(f"{name:25} -> Suspicious patterns: {matches if matches else 'None'}")
PYEOFRepository: basher83/Zammad-MCP
Length of output: 240
🏁 Script executed:
# Test if renaming to _TEST_AUTH_VALUE would actually suppress S105
# by creating a mock test file and running ruff on it
cd /tmp && cat > mock_test.py << 'EOF'
# Test variables
_TEST_HTTP_TOKEN = "test-http-token"
_TEST_AUTH_VALUE = "not-a-secret"
_TEST_PLACEHOLDER = "test-value"
def test_example():
token = _TEST_HTTP_TOKEN
auth = _TEST_AUTH_VALUE
placeholder = _TEST_PLACEHOLDER
return token, auth, placeholder
EOF
# Run Ruff to see which variable names trigger S105
python3 -m pip install -q ruff 2>/dev/null || true
ruff check /tmp/mock_test.py --select S105,S106 || trueRepository: basher83/Zammad-MCP
Length of output: 490
🏁 Script executed:
# Also test if the full proposed fix (with actual variable usage) resolves the errors
cd /tmp && cat > proposed_fix.py << 'EOF'
# Non-secret placeholder for tests (avoids bandit S105/S106 on auth kwargs)
_TEST_AUTH_VALUE = "not-a-secret"
def test_client_from_env():
env_dict = {
"ZAMMAD_URL": "https://test.zammad.com/api/v1",
"ZAMMAD_HTTP_TOKEN": _TEST_AUTH_VALUE,
"ZAMMAD_INSECURE": "true",
}
return env_dict
def test_client_from_param():
client = {
"url": "https://test.zammad.com/api/v1",
"http_token": _TEST_AUTH_VALUE,
"insecure": True
}
return client
def test_fallback():
client = {
"url": "https://test.zammad.com/api/v1",
"http_token": _TEST_AUTH_VALUE,
"insecure": True,
}
return client
def test_missing_session():
client = {
"url": "https://test.zammad.com/api/v1",
"http_token": _TEST_AUTH_VALUE,
"insecure": True,
}
return client
EOF
ruff check /tmp/proposed_fix.py --select S105,S106 || trueRepository: basher83/Zammad-MCP
Length of output: 81
Rename the placeholder to suppress Ruff S105 secret detection.
The variable _TEST_HTTP_TOKEN triggers S105 because the name contains "token". Rename it to _TEST_AUTH_VALUE and update all usages (lines 75, 92, 112, 130) to be consistent. This removes the suspicious keyword pattern and resolves both S105 findings from the current linting.
🧹 Proposed cleanup
-# Non-secret placeholder for tests (avoids bandit S106 on http_token kwargs)
-_TEST_HTTP_TOKEN = "test-http-token"
+# Non-secret placeholder for tests (avoids bandit S105/S106 on auth kwargs)
+_TEST_AUTH_VALUE = "not-a-secret" "ZAMMAD_URL": "https://test.zammad.com/api/v1",
- "ZAMMAD_HTTP_TOKEN": "test-token",
+ "ZAMMAD_HTTP_TOKEN": _TEST_AUTH_VALUE,
"ZAMMAD_INSECURE": "true",- client = ZammadClient(
- url="https://test.zammad.com/api/v1", http_token=_TEST_HTTP_TOKEN, insecure=True
- )
+ client = ZammadClient(
+ url="https://test.zammad.com/api/v1", http_token=_TEST_AUTH_VALUE, insecure=True
+ ) url="https://test.zammad.com/api/v1",
- http_token=_TEST_HTTP_TOKEN,
+ http_token=_TEST_AUTH_VALUE,
insecure=True, url="https://test.zammad.com/api/v1",
- http_token=_TEST_HTTP_TOKEN,
+ http_token=_TEST_AUTH_VALUE,
insecure=True,🧰 Tools
🪛 Ruff (0.15.10)
[error] 12-12: Possible hardcoded password assigned to: "_TEST_HTTP_TOKEN"
(S105)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_client.py` around lines 11 - 12, Rename the test placeholder
variable `_TEST_HTTP_TOKEN` to `_TEST_AUTH_VALUE` and update all its usages
(currently referenced in tests at the locations corresponding to the original
lines 75, 92, 112, and 130) to the new name; ensure imports/locals in
tests/test_client.py and any assertions or kwargs that pass `_TEST_HTTP_TOKEN`
(e.g., http_token=...) are updated to use `_TEST_AUTH_VALUE` so Ruff S105 no
longer flags the identifier.
Comment on lines
+66
to
+83
| @patch("mcp_zammad.client.ZammadAPI") | ||
| def test_client_insecure_mode_from_env(mock_api: MagicMock) -> None: | ||
| """Test that insecure mode disables TLS verification.""" | ||
| mock_instance = mock_api.return_value | ||
|
|
||
| with patch.dict( | ||
| os.environ, | ||
| { | ||
| "ZAMMAD_URL": "https://test.zammad.com/api/v1", | ||
| "ZAMMAD_HTTP_TOKEN": "test-token", | ||
| "ZAMMAD_INSECURE": "true", | ||
| }, | ||
| clear=True, | ||
| ): | ||
| client = ZammadClient() | ||
|
|
||
| assert client.insecure is True | ||
| assert mock_instance.session.verify is False |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parametrize the env parser cases documented by the feature.
The docs promise 1, true, yes, and on, but this test only covers "true". Parametrizing the truthy values keeps the implementation, README, and .env.example locked together.
🧪 Proposed test coverage
+@pytest.mark.parametrize("env_value", ["1", "true", "yes", "on"])
`@patch`("mcp_zammad.client.ZammadAPI")
-def test_client_insecure_mode_from_env(mock_api: MagicMock) -> None:
+def test_client_insecure_mode_from_env(mock_api: MagicMock, env_value: str) -> None:
"""Test that insecure mode disables TLS verification."""
mock_instance = mock_api.return_value
+ mock_instance.session.verify = True
with patch.dict(
os.environ,
{
"ZAMMAD_URL": "https://test.zammad.com/api/v1",
- "ZAMMAD_HTTP_TOKEN": "test-token",
- "ZAMMAD_INSECURE": "true",
+ "ZAMMAD_HTTP_TOKEN": _TEST_AUTH_VALUE,
+ "ZAMMAD_INSECURE": env_value,
},
clear=True,
):
client = ZammadClient()As per coding guidelines, “Use parametrized tests for multiple scenarios.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_client.py` around lines 66 - 83, Parametrize the
test_client_insecure_mode_from_env to iterate the documented truthy values ("1",
"true", "yes", "on"): use pytest.mark.parametrize to supply a variable (e.g.,
truthy) and replace the hardcoded "ZAMMAD_INSECURE" value in the patch.dict with
that variable; keep the current mocks (patch("mcp_zammad.client.ZammadAPI") /
mock_api and mock_instance) and the assertions (client.insecure is True and
mock_instance.session.verify is False) so the test verifies all documented
truthy env inputs for ZAMMAD_INSECURE against the ZammadClient behavior.
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Tests