-
Notifications
You must be signed in to change notification settings - Fork 164
feat: add PLANE_SSL_VERIFY passthrough to PlaneClient #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,44 @@ class PlaneClientContext(NamedTuple): | |
| workspace_slug: str | ||
|
|
||
|
|
||
| def _resolve_ssl_verify(base_url: str) -> bool | str: | ||
| """ | ||
| Resolve TLS verification mode from PLANE_SSL_VERIFY. | ||
|
|
||
| - Unset (default): verify=True, normal certificate verification. | ||
| - "false"/"0"/"no" (case-insensitive): verify=False, disable verification | ||
| entirely. Logs a warning on every call since this drops protection | ||
| against a man-in-the-middle. | ||
| - Any other value that names an existing file path: treated as a CA | ||
| bundle and passed through verbatim. | ||
| - Anything else: verify=True (fail safe rather than silently misparse). | ||
| """ | ||
| raw = os.environ.get("PLANE_SSL_VERIFY") | ||
| if not raw: | ||
| return True | ||
|
|
||
| if raw.strip().lower() in ("false", "0", "no"): | ||
| logger.warning( | ||
| "TLS verification is DISABLED for %s (PLANE_SSL_VERIFY=%s). " | ||
| "The connection has no protection against a man-in-the-middle; " | ||
| "requests, responses, and the API key are exposed to anyone " | ||
| "positioned on the network path.", | ||
| base_url, | ||
| raw, | ||
| ) | ||
| return False | ||
|
|
||
| if os.path.exists(raw): | ||
| return raw | ||
|
|
||
| logger.warning( | ||
| "PLANE_SSL_VERIFY=%s is neither false/0/no nor an existing file path; " | ||
| "falling back to verify=True.", | ||
| raw, | ||
| ) | ||
| return True | ||
|
|
||
|
|
||
| def get_plane_client_context() -> PlaneClientContext: | ||
| """ | ||
| Initialize and return a PlaneClient instance with workspace context. | ||
|
|
@@ -30,6 +68,9 @@ def get_plane_client_context() -> PlaneClientContext: | |
| Environment variables: | ||
| - PLANE_INTERNAL_BASE_URL: Internal URL for Plane API (preferred for server-to-server calls) | ||
| - PLANE_BASE_URL: Base URL for Plane API (fallback, default: https://api.plane.so) | ||
| - PLANE_SSL_VERIFY: TLS verification mode. Unset/default = verify normally. | ||
| "false"/"0"/"no" = disable verification entirely (logs a warning on every | ||
| use). A path to an existing file = treated as a CA bundle. | ||
|
|
||
| Returns: | ||
| PlaneClientContext containing configured PlaneClient instance and workspace slug | ||
|
|
@@ -57,15 +98,19 @@ def get_plane_client_context() -> PlaneClientContext: | |
| else: | ||
| access_token = token | ||
|
|
||
| verify = _resolve_ssl_verify(base_url) | ||
|
|
||
| if access_token: | ||
| client = PlaneClient( | ||
| base_url=base_url, | ||
| access_token=access_token, | ||
| verify=verify, | ||
| ) | ||
| else: | ||
| client = PlaneClient( | ||
| base_url=base_url, | ||
| api_key=api_key, | ||
| verify=verify, | ||
|
Comment on lines
104
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -t f -i 'client.py|pyproject.toml|poetry.lock|uv.lock|requirements.*|pdm.lock|setup.cfg|setup.py' . | head -80
printf '%s\n' '--- changed file context ---'
cat -n plane_mcp/client.py | sed -n '1,145p'
printf '%s\n' '--- dependency declarations ---'
for f in pyproject.toml setup.cfg setup.py requirements.txt requirements-dev.txt poetry.lock uv.lock pdm.lock; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
rg -n -C 3 'plane-sdk|plane_sdk|plane' "$f" || true
fi
done
printf '%s\n' '--- PlaneClient bindings and calls ---'
rg -n -C 4 'PlaneClient|verify' --glob '*.py' .Repository: makeplane/plane-mcp-server Length of output: 23147 🌐 Web query:
💡 Result: In the plane-sdk (Python) version 0.2.23, the PlaneClient constructor initializes the client and handles authentication configuration [1][2]. The constructor method signature is as follows [2]: def init( self, *, base_url: str, api_key: str | None = None, access_token: str | None = None,) -> None: Key behavioral requirements for these parameters include: 1. Authentication Mutual Exclusivity: You must provide exactly one of either api_key or access_token [1][3]. Providing both, or failing to provide either, will raise a ConfigurationError [1][4]. 2. base_url: This is a required parameter [1]. You should provide the base URL of your Plane instance; do not include the /api/v1 suffix in this string, as the SDK appends it automatically [1][3]. 3. Additional Configuration: While not part of the primary positional arguments, the constructor also supports optional keyword arguments for timeout (default 30.0 seconds) and retry (RetryConfig) to further customize client behavior [1][3]. When initializing the client, these parameters are passed to an internal Configuration object [2]. The SDK ensures that these settings are validated during initialization [4]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- relevant uv.lock package record ---'
cat -n uv.lock | sed -n '880,955p'
printf '%s\n' '--- repository references to local SDK source and dependency lock metadata ---'
rg -n -C 3 'plane-sdk|plane-python-sdk|source = \{ directory' . --glob '!uv.lock' --glob '!pyproject.toml' --glob '!*.pyc'Repository: makeplane/plane-mcp-server Length of output: 9449 Update the The published 🤖 Prompt for AI Agents |
||
| ) | ||
|
|
||
| return PlaneClientContext( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| """Unit tests for PLANE_SSL_VERIFY parsing in plane_mcp.client._resolve_ssl_verify. | ||
|
|
||
| Pure function, no network and no live Plane instance required. | ||
| """ | ||
|
|
||
| import pytest | ||
|
|
||
| from plane_mcp.client import _resolve_ssl_verify | ||
|
|
||
|
|
||
| def test_default_is_true_when_unset(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.delenv("PLANE_SSL_VERIFY", raising=False) | ||
| assert _resolve_ssl_verify("https://plane.example.com") is True | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("raw", ["false", "False", "FALSE", "0", "no", "No"]) | ||
| def test_falsy_values_disable_verification(monkeypatch: pytest.MonkeyPatch, raw: str) -> None: | ||
| monkeypatch.setenv("PLANE_SSL_VERIFY", raw) | ||
| assert _resolve_ssl_verify("https://plane.example.com") is False | ||
|
|
||
|
|
||
| def test_existing_file_path_is_passed_through_verbatim( | ||
| monkeypatch: pytest.MonkeyPatch, tmp_path | ||
| ) -> None: | ||
| ca_bundle = tmp_path / "internal-ca.pem" | ||
| ca_bundle.write_text("fake cert content") | ||
| monkeypatch.setenv("PLANE_SSL_VERIFY", str(ca_bundle)) | ||
| assert _resolve_ssl_verify("https://plane.example.com") == str(ca_bundle) | ||
|
|
||
|
|
||
| def test_nonexistent_path_falls_back_to_true(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setenv("PLANE_SSL_VERIFY", "/no/such/path/ca.pem") | ||
| assert _resolve_ssl_verify("https://plane.example.com") is True | ||
|
|
||
|
|
||
| def test_empty_string_is_treated_as_unset(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setenv("PLANE_SSL_VERIFY", "") | ||
| assert _resolve_ssl_verify("https://plane.example.com") is True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accept only regular files as CA bundles.
os.path.exists(raw)also returnsTruefor directories. A directory is forwarded asverifyalthough the documented contract accepts only an existing file. Useos.path.isfile(raw)so a directory uses the safeTruefallback.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents