-
Notifications
You must be signed in to change notification settings - Fork 1
fix: launch must-fix items — security, perf, docs #7
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
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 |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # Changelog | ||
|
|
||
| ## 0.0.1a3 (2026-04-06) | ||
|
|
||
| Initial alpha release. | ||
|
|
||
| - Core SDK: Chat orchestrator, Thread, Channel, Message, Cards, Modals, Emoji | ||
| - 8 adapters: Slack, Discord, Teams, Telegram, WhatsApp, Google Chat, GitHub, Linear | ||
| - 3 state backends: Memory, Redis, PostgreSQL | ||
| - 2,467 tests passing | ||
| - Security hardened: JWT verification, SSRF protection, timing-safe comparisons |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| # Contributing to chat-sdk-python | ||
|
|
||
| Thanks for your interest in contributing! This guide covers the essentials. | ||
|
|
||
| ## Dev Environment Setup | ||
|
|
||
| ```bash | ||
| # Clone and install (requires Python 3.11+ and uv) | ||
| git clone https://github.com/Chinchill-AI/chat-sdk-python.git | ||
| cd chat-sdk-python | ||
| uv sync --group dev | ||
| ``` | ||
|
|
||
| ## Running Tests | ||
|
|
||
| ```bash | ||
| uv run pytest tests/ # all tests | ||
| uv run pytest tests/ -x # stop on first failure | ||
| uv run pytest tests/unit/ # unit tests only | ||
| ``` | ||
|
|
||
| ## Code Quality | ||
|
|
||
| ```bash | ||
| uv run ruff check src/ tests/ # lint | ||
| uv run ruff format src/ tests/ # auto-format | ||
| ``` | ||
|
|
||
| All PRs must pass `ruff check` with zero errors. | ||
|
|
||
| ## Adding a New Adapter | ||
|
|
||
| 1. Create `src/chat_sdk/adapters/<platform>/` with at minimum: | ||
| - `adapter.py` -- the adapter class implementing the Adapter protocol | ||
| - `__init__.py` -- public exports and a `create_<platform>_adapter()` factory | ||
| 2. Follow the patterns in existing adapters (Slack and Teams are good references). | ||
| 3. Add an optional-dependency group in `pyproject.toml`. | ||
| 4. Add tests under `tests/unit/adapters/<platform>/`. | ||
|
|
||
| ## Pull Request Expectations | ||
|
|
||
| - **Tests required.** Every bugfix or feature needs at least one test. | ||
| - **Ruff clean.** `uv run ruff check src/ tests/` must pass with no errors. | ||
| - **Small, focused PRs** are easier to review than large ones. | ||
| - **Descriptive commit messages.** Explain *why*, not just *what*. | ||
|
|
||
| ## Issues and PRs | ||
|
|
||
| - Check existing issues before opening a new one. | ||
| - Reference the issue number in your PR description (e.g., `Fixes #42`). | ||
| - For large changes, open an issue first to discuss the approach. | ||
|
|
||
| ## License | ||
|
|
||
| By contributing you agree that your contributions will be licensed under the MIT License. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,6 +164,12 @@ def __init__(self, config: TeamsAdapterConfig | None = None) -> None: | |
| "Use app_password (client secret) or federated (workload identity) authentication instead.", | ||
| ) | ||
|
|
||
| if not self._app_id: | ||
| self._logger.warn( | ||
| "Teams app_id is empty — webhook verification will reject all incoming requests. " | ||
| "Set TEAMS_APP_ID or pass app_id in config." | ||
| ) | ||
|
|
||
| self._bot_user_id: str | None = self._app_id or None | ||
| self._access_token: str | None = None | ||
| self._token_expiry: float = 0 | ||
|
|
@@ -207,10 +213,13 @@ async def handle_webhook( | |
| self._logger.debug("Teams webhook raw body", {"body": body[:500] if body else ""}) | ||
|
|
||
| # ---- JWT verification (Bot Framework tokens) ---- | ||
| if self._app_id: | ||
| auth_result = await self._verify_bot_framework_token(request) | ||
| if auth_result is not None: | ||
| return auth_result | ||
| if not self._app_id: | ||
| self._logger.warn("Rejecting Teams webhook: app_id is not configured, cannot verify JWT") | ||
| return self._make_response("Unauthorized – Teams app_id not configured", 401) | ||
|
|
||
| auth_result = await self._verify_bot_framework_token(request) | ||
| if auth_result is not None: | ||
| return auth_result | ||
|
|
||
| try: | ||
| activity: dict[str, Any] = json.loads(body) | ||
|
|
@@ -259,10 +268,19 @@ async def _cache_user_context(self, activity: dict[str, Any]) -> None: | |
| ttl = CACHE_TTL_MS | ||
| state = self._chat.get_state() | ||
|
|
||
| # Cache serviceUrl | ||
| # Cache serviceUrl (validate against SSRF allow-list first) | ||
| service_url = activity.get("serviceUrl") | ||
| if service_url and state: | ||
| await state.set(f"teams:serviceUrl:{user_id}", service_url, ttl) | ||
| try: | ||
| _validate_service_url(service_url) | ||
| except ValidationError: | ||
| self._logger.warn( | ||
| "Refusing to cache disallowed serviceUrl", | ||
| {"serviceUrl": service_url}, | ||
| ) | ||
| service_url = None | ||
| if service_url: | ||
| await state.set(f"teams:serviceUrl:{user_id}", service_url, ttl) | ||
|
|
||
| # Cache tenantId | ||
| channel_data = activity.get("channelData", {}) | ||
|
|
@@ -1112,6 +1130,8 @@ async def open_dm(self, user_id: str) -> str: | |
| if not service_url: | ||
| service_url = "https://smba.trafficmanager.net/teams/" | ||
|
|
||
| _validate_service_url(service_url) | ||
|
|
||
| import aiohttp | ||
|
|
||
| token = await self._get_access_token() | ||
|
|
@@ -1693,6 +1713,7 @@ async def _verify_bot_framework_token(self, request: Any) -> Any | None: | |
| signing_key.key, | ||
| algorithms=["RS256"], | ||
| audience=self._app_id, | ||
| issuer="https://api.botframework.com", | ||
|
Contributor
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. The JWT verification process relies on
Collaborator
Author
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. Fixed in PR #13 -- |
||
| ) | ||
| self._logger.debug( | ||
| "Teams JWT verified", | ||
|
|
||
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.
The
_client_cachedictionary storesAsyncWebClientinstances indefinitely. In multi-tenant applications with a large number of workspaces or frequent token rotations, this could lead to significant memory consumption over time. Consider using a cache with an eviction policy (such as an LRU cache) or limiting the maximum number of cached clients to prevent unbounded growth.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.
Addressed in PR #8 (LRU-bounded OrderedDict cache with max=100) and further improved in PR #13 by making
client_cache_maxconfigurable viaSlackAdapterConfig.