security: Fix all critical and high findings from security audit by patrick-chinchill · Pull Request #1 · Chinchill-AI/chat-sdk-python

patrick-chinchill · 2026-04-06T21:51:21Z

Security Fixes

From external security researcher review of the repo before PyPI publication.

Critical

Teams JWT verification — incoming webhooks were unauthenticated. Now validates Bot Framework JWT tokens (signature + audience).
Teams SSRF — service_url in thread IDs was user-controlled and forwarded with Bearer token. Now validated against Microsoft endpoint allowlist.
Discord timing attack — gateway token compared with !=. Now uses hmac.compare_digest.

High

Lock token predictability — Redis/Memory used random.choices (non-CSPRNG). Now uses secrets.token_hex.
WhatsApp SSRF — media download followed API-returned URL with auth header. Now validates domain + strips auth from CDN request.
CI supply chain — GitHub Actions pinned to commit SHAs (was using mutable tags).

Medium

GChat JWKS caching — PyJWKClient was instantiated per-request. Now cached on adapter instance.

Also

README quick start: added missing create_slack_adapter import
Error types exported from top-level __init__.py

Critical: - Teams: Add JWT verification for incoming webhooks (Bot Framework spec) - Teams: Validate service_url against Microsoft endpoint allowlist (SSRF fix) - Discord: Use hmac.compare_digest for gateway token (timing-safe) High: - Redis/Memory: Use secrets.token_hex for lock tokens (CSPRNG) - WhatsApp: Validate media download URLs + strip auth header from CDN - CI: Pin all GitHub Actions to commit SHAs Medium: - Google Chat: Cache JWKS client instance (was creating per-request) Also: - Fix README quick start (add missing imports) - Export error types from top-level __init__.py

gemini-code-assist

Code Review

This pull request introduces several security and performance enhancements across multiple chat adapters. Key improvements include the implementation of constant-time token comparisons for Discord, SSRF protection for Teams and WhatsApp by validating service and media download URLs, and the transition to cryptographically secure token generation using the secrets module for memory and Redis state adapters. Additionally, JWT verification was added for the Teams adapter, and JWKS client caching was implemented for both Google Chat and Teams to reduce per-request overhead. Feedback was provided regarding the Teams JWT verification process, specifically suggesting an explicit check of the HTTP response status when fetching the OpenID configuration to ensure more robust error handling.

gemini-code-assist · 2026-04-06T21:53:26Z

+                    async with session.get(BOT_FRAMEWORK_OPENID_CONFIG_URL) as resp:
+                        openid_config = await resp.json()


For improved robustness and clearer error logging, it's a good practice to explicitly check if the HTTP request to fetch the OpenID configuration was successful before attempting to parse the JSON response. This will provide more specific logs if the endpoint is unavailable or returns an error.

Suggested change

async with session.get(BOT_FRAMEWORK_OPENID_CONFIG_URL) as resp:

openid_config = await resp.json()

async with session.get(BOT_FRAMEWORK_OPENID_CONFIG_URL) as resp:

if not resp.ok:

self._logger.error(

"Failed to fetch Bot Framework OpenID config",

{"status": resp.status, "url": BOT_FRAMEWORK_OPENID_CONFIG_URL},

)

return self._make_response("Unauthorized", 401)

openid_config = await resp.json()

Already addressed in the existing code -- the resp.status != 200 check was present at line 1700 before this review comment was posted. No changes needed.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f37fa43fba

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- WhatsApp: expand media URL allowlist to include *.fbsbx.com, *.whatsapp.net, *.whatsapp.com (real Meta CDN domains used by Cloud API) - Teams: expand service URL allowlist to include sovereign cloud endpoints (GCC, GCCH, DoD: *.teams.microsoft.com, *.teams.microsoft.us, smba.infra.*) - Teams: check OpenID config HTTP response status before parsing JSON Note: The upstream TS SDK does not validate service_url or media URLs at all (Teams delegates auth to @microsoft/teams.apps SDK, WhatsApp follows any URL). Our validation is additive security hardening not present in the original.

- Fix Slack rate limit detection: _handle_slack_error now correctly handles SlackResponse objects (not just dicts) by checking resp.data, and extracts the Retry-After header for rate limit errors - Fix Teams JWT verification blocking: wrap synchronous PyJWKClient.get_signing_key_from_jwt in asyncio.to_thread to avoid blocking the event loop during JWKS fetch - Make Slack client cache max configurable: add client_cache_max field to SlackAdapterConfig (defaults to 100) - Fix unsafe session cleanup on LRU eviction: remove create_task session close that could break concurrent requests using evicted clients; rely on garbage collection instead - Add tests for SlackResponse handling, rate limiting, and configurable cache max Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: Address unresolved PR review comments from #1, #7, #8

- process_options_load: switch to `is not None` check so handlers returning [] short-circuit correctly (Port Rule #1 truthiness trap) - Slack adapter: replace `or ""` fallbacks in OptionsLoadEvent construction with explicit None checks - test helper: URL-encode payload JSON in signed-request body - Narrow OptionsLoadHandler return type and typed Protocol signature for process_options_load (replace Any with OptionsLoadEvent) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- chat.py:2107 — type the rehydrate-attachment callable so the list comprehension narrows to list[Attachment]. Unblocks CI. - _coerce_attachments: replace `or` fallbacks with `is not None` (Port Rule #1 truthiness trap) - google_chat rehydrate_attachment: preserve resolved URL when reconstructing, drop truthiness fallback on meta["url"] - Harden telegram and whatsapp rehydrate tests to execute the async callback and verify download-method wiring (AsyncMock). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ness fallbacks Second review pass on PR #67 (rehydrate_attachment). The previous fixup addressed pyrefly only — this commit resolves the remaining review feedback. SSRF guards (3 adapters) - Slack, Teams, Google Chat all rebuild fetch_data closures from serialized fetch_metadata["url"] in rehydrate_attachment. A tampered URL in persisted queue state could exfiltrate the workspace bot/OAuth token to an attacker-controlled host. Each adapter now validates the URL's scheme (https only) and host against a platform-specific allowlist before forwarding the auth header. Upstream TS does not validate; this is a Python-first divergence documented in docs/UPSTREAM_SYNC.md. - Slack: files.slack.com, slack.com, *.slack.com, *.slack-edge.com - Teams: Microsoft-owned hosts (graph.microsoft.com, smba.trafficmanager.net, *.sharepoint.com, *.botframework.com, *.office.com, attachments.office.net, …) - Google Chat: chat.googleapis.com, *.googleapis.com, *.googleusercontent.com, *.google.com Message-instance rehydration (P1) - Chat._rehydrate_message used to early-return on Message inputs, matching upstream TS's `raw instanceof Message` shortcut. That shortcut is safe in upstream because its state adapters return raw JSON dicts from dequeue. Our RedisStateAdapter / PostgresStateAdapter both upgrade the dequeued dict to `Message.from_json(...)` before returning, so the early return would skip rehydrate_attachment for every persistent-backend dequeue and leave fetch_data stripped. We now fall through and apply the rehydrate pass on Message inputs too (already-hydrated attachments with fetch_data are filtered out). Truthiness fallbacks (Port Rule #1) - telegram, whatsapp rehydrate_attachment and types.py dual-key fetch_metadata lookup now use explicit `is not None` instead of `or`, so an empty-dict fetch_metadata is preserved. Teams connection pooling - _build_teams_fetch_data used httpx.AsyncClient as a throwaway context manager per download. Refactored to use the shared aiohttp session (_get_http_session) that the rest of the adapter already goes through. Test hardening - test_slack_webhook.py and test_teams_adapter.py now stub the fetch path with AsyncMock, await rehydrated.fetch_data(), and assert the URL + token that were forwarded. Previously the tests only checked that `fetch_data is not None` — they would have passed even if rehydration returned a dummy closure. - New tests per adapter verify the SSRF guard rejects untrusted hosts and the allowlist accepts the intended Slack/Teams/GCP hosts. - New regression test in test_chat_faithful.py drives a Message- instance dequeue through the chat queue and asserts rehydrate_attachment still fires. Slack adapter connection pooling (deferred) - _fetch_slack_file still uses httpx.AsyncClient per call. The Slack adapter has no pooled aiohttp helper (only slack_sdk.AsyncWebClient for Slack API calls), so adding one is a larger refactor left for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- StreamingPlanOptions mapping: replace truthy checks with `is not None` so update_interval_ms=0 and end_with=[] propagate correctly (thread.py post dispatcher and _fallback_stream interval guard). Per CLAUDE.md Port Rule #1; diverges from upstream thread.ts, which has the same latent bug. - StreamingPlan: raise RuntimeError from is_supported() and get_fallback_text() so misroutes through post_postable_object / Channel.post fail loudly instead of posting "" or attempting a wrong-shape adapter.post_object("stream", ...). Also guard post_postable_object() with an early kind=="stream" check for a clearer error message. Diverges from upstream postable-object.ts, which silently posts the empty fallback string. - Fallback test: spy on _fallback_stream to capture the StreamOptions actually reaching the fallback path and assert task_display_mode, stop_blocks, and update_interval_ms all propagate -- previous test passed even when options were silently dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Round-2 fixup over-applied Port Rule #1. Upstream JS uses || chain intentionally — empty-string username falls through to name/user_id. The is-not-None chain preserved "" and diverged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- process_options_load: switch to `is not None` check so handlers returning [] short-circuit correctly (Port Rule #1 truthiness trap) - Slack adapter: replace `or ""` fallbacks in OptionsLoadEvent construction with explicit None checks - test helper: URL-encode payload JSON in signed-request body - Narrow OptionsLoadHandler return type and typed Protocol signature for process_options_load (replace Any with OptionsLoadEvent) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Round-2 fixup over-applied Port Rule #1. Upstream JS uses || chain intentionally — empty-string username falls through to name/user_id. The is-not-None chain preserved "" and diverged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- StreamingPlanOptions mapping: replace truthy checks with `is not None` so update_interval_ms=0 and end_with=[] propagate correctly (thread.py post dispatcher and _fallback_stream interval guard). Per CLAUDE.md Port Rule #1; diverges from upstream thread.ts, which has the same latent bug. - StreamingPlan: raise RuntimeError from is_supported() and get_fallback_text() so misroutes through post_postable_object / Channel.post fail loudly instead of posting "" or attempting a wrong-shape adapter.post_object("stream", ...). Also guard post_postable_object() with an early kind=="stream" check for a clearer error message. Diverges from upstream postable-object.ts, which silently posts the empty fallback string. - Fallback test: spy on _fallback_stream to capture the StreamOptions actually reaching the fallback path and assert task_display_mode, stop_blocks, and update_interval_ms all propagate -- previous test passed even when options were silently dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- chat.py:2107 — type the rehydrate-attachment callable so the list comprehension narrows to list[Attachment]. Unblocks CI. - _coerce_attachments: replace `or` fallbacks with `is not None` (Port Rule #1 truthiness trap) - google_chat rehydrate_attachment: preserve resolved URL when reconstructing, drop truthiness fallback on meta["url"] - Harden telegram and whatsapp rehydrate tests to execute the async callback and verify download-method wiring (AsyncMock). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ness fallbacks Second review pass on PR #67 (rehydrate_attachment). The previous fixup addressed pyrefly only — this commit resolves the remaining review feedback. SSRF guards (3 adapters) - Slack, Teams, Google Chat all rebuild fetch_data closures from serialized fetch_metadata["url"] in rehydrate_attachment. A tampered URL in persisted queue state could exfiltrate the workspace bot/OAuth token to an attacker-controlled host. Each adapter now validates the URL's scheme (https only) and host against a platform-specific allowlist before forwarding the auth header. Upstream TS does not validate; this is a Python-first divergence documented in docs/UPSTREAM_SYNC.md. - Slack: files.slack.com, slack.com, *.slack.com, *.slack-edge.com - Teams: Microsoft-owned hosts (graph.microsoft.com, smba.trafficmanager.net, *.sharepoint.com, *.botframework.com, *.office.com, attachments.office.net, …) - Google Chat: chat.googleapis.com, *.googleapis.com, *.googleusercontent.com, *.google.com Message-instance rehydration (P1) - Chat._rehydrate_message used to early-return on Message inputs, matching upstream TS's `raw instanceof Message` shortcut. That shortcut is safe in upstream because its state adapters return raw JSON dicts from dequeue. Our RedisStateAdapter / PostgresStateAdapter both upgrade the dequeued dict to `Message.from_json(...)` before returning, so the early return would skip rehydrate_attachment for every persistent-backend dequeue and leave fetch_data stripped. We now fall through and apply the rehydrate pass on Message inputs too (already-hydrated attachments with fetch_data are filtered out). Truthiness fallbacks (Port Rule #1) - telegram, whatsapp rehydrate_attachment and types.py dual-key fetch_metadata lookup now use explicit `is not None` instead of `or`, so an empty-dict fetch_metadata is preserved. Teams connection pooling - _build_teams_fetch_data used httpx.AsyncClient as a throwaway context manager per download. Refactored to use the shared aiohttp session (_get_http_session) that the rest of the adapter already goes through. Test hardening - test_slack_webhook.py and test_teams_adapter.py now stub the fetch path with AsyncMock, await rehydrated.fetch_data(), and assert the URL + token that were forwarded. Previously the tests only checked that `fetch_data is not None` — they would have passed even if rehydration returned a dummy closure. - New tests per adapter verify the SSRF guard rejects untrusted hosts and the allowlist accepts the intended Slack/Teams/GCP hosts. - New regression test in test_chat_faithful.py drives a Message- instance dequeue through the chat queue and asserts rehydrate_attachment still fires. Slack adapter connection pooling (deferred) - _fetch_slack_file still uses httpx.AsyncClient per call. The Slack adapter has no pooled aiohttp helper (only slack_sdk.AsyncWebClient for Slack API calls), so adding one is a larger refactor left for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Previously `verify_test_fidelity.py` printed "SKIPPED (file not found)" for any mapped TS test whose source didn't exist under `TS_ROOT`, then summed 0 matches + 0 missing and exited 0 with "All TS tests have Python equivalents." Combined with `continue-on-error: true` on the upstream-clone step, that made a silently-failing clone report "fidelity check passed" in CI. Now the script tracks missing-TS-file hits separately from real successes. If any mapped TS file is absent at end-of-run, the script prints a clear "upstream checkout missing — cannot verify fidelity" message naming every missing path, includes the clone command hint, and exits 1. This fires before strict/baseline/update-baseline success branches so no mode can accidentally mask it. Reproducer: TS_ROOT=/tmp/definitely-missing uv run python scripts/verify_test_fidelity.py Before: exit 0. After: exit 1 with infra-level error message. Closes self-review gap #1 on #72.

Three review-driven fixes: 1. Adversarial input sweep on _extract_slack_recipient_team_id (Medium): extend the parametrized test from 4 happy paths to 11 cases, covering the helper's fallthrough guards (team dict missing id, empty team_id/team strings, non-dict raw, non-string user.team_id, team.id non-string). Per docs/SELF_REVIEW.md principle #1. 2. Defense-in-depth GUID guard on aadObjectId before formatting it into the Graph chat ID (Nit). Bot Framework JWT verification authenticates the activity envelope but does not constrain from.aadObjectId; a malformed value containing /, ?, # could otherwise inject into the Graph URL path. New _AAD_OBJECT_ID_PATTERN reject malformed shapes; adversarial test covers 6 attack patterns. 3. End-to-end legacy-cache-shape test for #403 backwards-compat (Nit). Cached entries written before the discriminator landed lack a "type" key. The test asserts _get_graph_context loads them and _chat_id_from_context returns the raw conversation ID (channel semantics), not a misclassified Graph DM URL. 4. Wire-shape parity (Nit): the channel-context cache now omits the "type": "channel" key to match upstream TS, which only sets the discriminator on the DM branch. Functionally equivalent (absent type is treated as channel by the dispatcher), but the cross-language wire shape is now identical. All TestGraphDmConversationIdResolution tests pass (10 cases). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

Address re-review on PR #87 (Medium #1). Slack's url_verification ping arrives at app-install / event-subscription time and only expects the challenge echo — no bot token / API call required. Previously the single-workspace resolver was invoked at handle_webhook entry, BEFORE the url_verification short-circuit, so a flaky/down secret manager would block app installation with a 500. Move the JSON peek for url_verification ahead of _resolve_default_token() and short-circuit there. Mirrors upstream where getToken() is only called at per-API-call sites, never at webhook entry. Adds test_url_verification_bypasses_broken_resolver: configures a resolver that raises and asserts URL verification still returns 200 with the challenge echo. Also documents two related divergences in docs/UPSTREAM_SYNC.md non-parity table (Medium #2 + Nit from re-review): - Slack bot_token resolver invocation site: TS resolves on every API call site (cron-mode works); Python resolves once at handle_webhook entry into a ContextVar (cron callers must await current_token_async() first). - Within-request resolver caching scope: TS calls per API call; Python caches per request to keep _get_token sync. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

, #397) (#84) * feat(externalselect): add initialOption + option_groups (vercel/chat#410, #397) Ports two upstream PRs that together complete ExternalSelect support: - vercel/chat#397 introduced ExternalSelectElement and the block_suggestion / onOptionsLoad runtime; the runtime half landed here in #66 but the modal element type was deferred. This PR adds the missing ExternalSelectElement TypedDict + ExternalSelect builder and wires up _external_select_to_block in the Slack modal renderer. - vercel/chat#410 adds two new optional fields on top: initialOption (pre-selected option object) and option_groups (labeled sections, Slack max 100 groups x 100 options, label max 75 chars). The handler return type widens to OptionsLoadResult = list[options] | list[OptionsLoadGroup]; the Slack adapter detects grouped form by the presence of an "options" key on the first entry and emits Slack's option_groups response (mutually exclusive with options per Slack's spec). Hazard #1 (truthiness): min_query_length=0 is preserved (0 means "fire on every keystroke"); not silently dropped by an `or` fallback. Hazard #7 (omit vs None): unset initial_option / placeholder / min_query_length are omitted from the rendered Block Kit element, not serialized as null. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * fix(slack): use is-not-None guard for initial_option in external_select renderer Address review on PR #84 (modals.py:209). The TS expression ``if (select.initialOption)`` only filters null/undefined since ``{}`` is truthy in JS. Python ``if initial_option:`` falsely drops a hand-constructed ``initial_option={}`` because empty dicts are falsy. Switch to ``is not None`` for parity with TS and consistency with the ``min_query_length is not None`` check three lines above. Adds test_external_select_initial_option_empty_dict_renders regression test that fails before the fix. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --------- Co-authored-by: Claude <noreply@anthropic.com>

Codex caught a Codex P2 #1 variant introduced by the throttle fix: chunks coalesced inside a throttle window only ship in ``_close_stream_session``'s final ``message`` activity. If THAT send fails (429, network blip), ``Thread.stream`` has already built a ``SentMessage`` from ``_stream_via_emit``'s return value containing text Teams never accepted. The chat handler returns and the ``SentMessage`` / ``_message_history`` entry is created BEFORE the close runs from the handler's finally block, so a swallowed close failure produces history the user never saw. Fix: at end of ``_stream_via_emit``'s loop, force one final ``typing`` emit when the throttle window buffered text since the last successful send. After the flush, ``accumulated`` is confirmed-accepted by Teams before the method returns, so the close-path final ``message`` activity becomes a UI-clearing marker — its failure is a stale-streaming-UI cost rather than a recording inconsistency. Flush guards: skipped when the session is already canceled (mid-stream abort) and when there's nothing buffered (last chunk happened to land beyond the throttle window). Re-raises on send failure with the same shape as in-loop emits, so ``Thread.stream``'s outer accumulator can't record the failed flush either. Refactored the per-emit payload-build + send block into ``_emit_streaming_activity`` since both the in-loop and end-of-stream flush sites build the same shape. Tests added (4): - ``test_buffered_text_flushed_at_end_of_stream`` — pin the flush - ``test_flush_failure_propagates_and_cancels_session`` — pin re-raise - ``test_no_flush_when_iterator_ended_at_window_boundary`` — no redundant duplicate-text emit when each chunk already had its own - ``test_no_flush_after_session_canceled_mid_stream`` — cancellation guard Tests updated: - ``test_chunks_within_throttle_interval_are_coalesced`` renamed to ``test_intermediate_chunks_within_window_are_coalesced`` and adjusted to expect 2 sends (initial + flush) instead of 1. - ``test_coalesced_text_ships_in_final_close_activity`` renamed to ``test_close_path_final_message_carries_full_accumulated_text`` and adjusted: close index is now [2] because the flush already emitted at [1]. Total send count: 3 (initial + flush + close). Updated ``_close_stream_session``'s except-block comment to reflect that the user has already seen ``session.text`` via the flush, so close-failure is a UI-clearing miss rather than a content loss.

Two defects flagged by CodeRabbit in the dynamic-bot-token port: 1. Truthiness-based auth fallbacks (discussion_r3285672704). The constructor used `config.signing_secret or ...`, `not (config.X or ...)` patterns that let an explicit empty-string config value silently fall through to environment credentials. Per docs/UPSTREAM_SYNC.md hazard #1 the rule when porting from TS is `x if x is not None else default`. Rewrites the signing_secret cascade, the validation guard, the zero_config check, and the env bot-token fallback to use explicit `is not None` checks so empty strings fail validation instead of flipping the adapter into the wrong auth mode. 2. Sync token cache not refreshed after resolver (discussion_r3285672709). `_resolve_default_token` wrote only the per-request ContextVar (`_resolved_default_token`), so callers reading the documented sync `current_token` / `current_client` accessors *outside* that ContextVar scope still saw the pre-resolution state and raised `AuthenticationError`. Adds `self._default_bot_token_cache = token` right before the ContextVar set so the process-wide cache that the sync `_get_token` path reads is refreshed too. Regression test `test_resolver_refreshes_sync_token_cache` clears the ContextVar after the resolver runs and asserts sync `current_token` returns the freshly resolved value (verified to fail without the fix). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

…d/secret Self-review on PR #87 surfaced two follow-on hazards left after 2ecd451's empty-string normalization sweep: 1. ``SlackAdapterConfig(bot_token="")`` was still accepted at init: the constructor primed ``_default_bot_token_cache`` with ``""`` and the sync ``_get_token`` path happily returned the empty string, producing ``Authorization: Bearer `` API calls and opaque ``invalid_auth`` errors from Slack. The async resolver path (``_resolve_default_token``) already raises on empty results, but failing fast at construction is strictly better than failing on every Slack API call in production — same rationale as the ``signing_secret=""`` rejection in 2ecd451. Callable resolvers are unaffected (they're validated at resolve time). 2. ``self._client_id = config.client_id or env`` / ditto for ``client_secret`` (lines 312-315) still used truthiness fallback — hazard #1 in the same constructor block 5a648ec was supposed to have cleaned up. An explicit ``client_id=""`` silently flipped to either the env value (zero_config) or ``None`` (non-zero-config), neither of which matches the user's intent. Rewrites to per-field ``is not None`` gating with an empty-env-as-unset rule mirroring SLACK_BOT_TOKEN env (empty env is "nothing here", not a configured value — would otherwise surface as opaque OAuth ``invalid_client`` errors mid-flow). Three regression tests in ``test_slack_dynamic_token_and_verifier.py``: ``test_empty_string_bot_token_rejected_at_construction``, ``test_empty_client_id_does_not_fall_back_to_env``, and ``test_empty_env_client_id_treated_as_unset``. All three fail against 2ecd451 and pass after this fix. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

Second-round self-review on PR #87 found one remaining truthiness trap the first round missed: ``encryption_key = config.encryption_key or os.environ.get("SLACK_ENCRYPTION_KEY")`` at line 347 used the same fallback pattern 7c30c13 cleaned up for ``client_id`` / ``client_secret``. When the user explicitly passes ``encryption_key=""`` with ``SLACK_ENCRYPTION_KEY`` set in the env, env silently shadowed the user's explicit "opt out" intent and downstream installation tokens were encrypted with a key the user didn't ask for. Per-field ``is not None`` gating, mirroring the client_id / client_secret cascade. Functional blast radius is narrower than the bot_token case (the final ``if encryption_key_raw`` short-circuits if both user and env are empty), but the explicit-user-config-wins-over-env rule should be uniform across all four secret-bearing fields. Adding a regression test pins the behavior so a future re-introduction of the ``or`` shortcut would fail CI. Validated: - ``test_empty_encryption_key_does_not_fall_back_to_env`` fails against 7c30c13 and passes after this fix. - Full Slack test suite (70 tests) green. - Full repo test suite green except pre-existing GitHub failure (unrelated, mentioned in task instructions). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

Independent review surfaced three actionable items: 1. **Math/escape ordering** (review #1) -- `_strip_math_regions` ran BEFORE `_strip_escape_sequences`, so input like `\$opener *unclosed text closer\$` had its two escaped `$`s paired as math, eating the `*` opener inside. Reordered: escape strip first, then math strip. Verified the original bug repro now correctly produces a closing `*`. 2. **Empty task items** (review #5) -- `r"^\[([ xX])\]\s+(.*)"` required at least one whitespace AFTER `]`, so `- [ ]` with no trailing content silently fell through to plain text. Loosened to `r"^\[([ xX])\](?:\s+(.*))?$"` -- trailing whitespace+content is now optional. `- [ ]` and `- [x]` (no trailing) both produce `listItem(checked=False/True, children=[])`. 3. **Strikethrough test strengthened** (review #7) -- the existing `test_escaped_tilde_does_not_form_strikethrough` only asserted the absence of a `delete` node. A buggy impl that dropped the tildes entirely would have passed. Added a content-shape assertion that `~~not strike~~` appears in the text leaves. Reviewer's #3 (the `r"a *b\* * c"` case) was investigated and determined NOT to be a bug -- the trailing `*` between two spaces is not a valid CommonMark closer (whitespace-flanked), so italic correctly stays open. Pre-fix behavior happened to balance by ignoring flanking; post-fix is CommonMark-correct. Pre-existing edge cases #4 (`\\*text*`) and #6 (link-text unescape implicit) are documented in the existing code; not blockers. Tests added: - `test_empty_unchecked_task_item_no_content` and `_checked_` variant - `test_remend_escaped_dollar_does_not_pair_with_unescaped_dollar` (the exact #1 repro) - `test_remend_escaped_dollar_does_not_create_phantom_math_region` - Strengthened `test_escaped_tilde_does_not_form_strikethrough` 3,625 pass / 1 pre-existing failure unrelated.

… (issue #69) (#99) * fix(streaming-markdown): list-marker awareness + table chunk-boundary Three production Slack streaming bugs from issue #69 comment: 1. `_remend("* item one\n")` appended a stray `*`. `_close_emphasis` counted the line-leading bullet as an italic opener. Now skips single-`*` runs preceded by line-leading whitespace and followed by a space/tab (list-marker shape), matching upstream `remend`'s awareness of list markers. 2. `StreamingMarkdownRenderer.finish()` on an odd-count bullet list produced `...item three\n*` — same root cause as #1; the fix covers both. 3. `_get_committable_prefix` released a confirmed table the moment a separator arrived, even with zero body rows. Slack `chat.appendStream` saw header+separator alone as broken syntax and then bare body rows in subsequent appends, never reconnecting them as a single table. Now the header+separator block is held until at least one body row arrives, then released atomically. The original tests pinned the buggy "commit on separator" behavior; updated to require a body row before committing the table block, and added a TestIssue69Regressions class with the exact reproducers from the issue comment plus invariants for the new contract. Closes part of #69 (the streaming bugs). The broader hand-rolled parser question (Options A/B/C in the issue) remains open. * refactor(_remend): match remend's flanking check for excluded asterisks The previous `_is_list_marker` skipped only line-leading single `*` followed by horizontal whitespace. Tighten to match upstream remend's `shouldSkipAsterisk` (packages/remend/src/emphasis-handlers.ts): exclude any single `*` flanked by whitespace (or text boundary) on both sides, which is what CommonMark says isn't a valid emphasis delimiter anyway. The line-leading bullet case still falls out naturally. Picks up three additional cases the previous narrower check missed: - `text * more` -- whitespace-flanked mid-line - `trailing *\n` -- asterisk at end of line - `partial *` -- asterisk at end of buffer Same `_remend` over-counting failure mode as the original issue #69 bugs, just different surface forms. Renamed the helper to `_is_excluded_asterisk` to reflect the broader scope. Remaining remend divergences (word-internal asterisks, math-block contents, escaped sequences, multi-backtick spans, setext headings, indented code, raw HTML, footnotes) are tracked on issue #69 for the parser-strategy discussion -- out of scope for this PR. * fix(streaming-markdown): preserve monotonicity on back-to-back tables PR #99 review (chatgpt-codex P1): in `_get_committable_prefix`, the "hold pre-separator block" backward walk uses `TABLE_ROW_RE` to extend `table_start`, but separator lines also match that pattern. For a stream like `|A|B|\n|---|---|\n|1|2|\n|C|D|\n|---|---|\n` (two tables with no blank line between them), the walk crosses the first table's separator and collapses `_get_committable_prefix` back to "", violating the monotonic append-only contract of `get_committable_text()`. Adapters that compute deltas from prior committed length then emit incorrect/no deltas. Fix: the backward walk now stops at empty lines (existing), non-row content (existing), AND at prior separators (new). When it hits a prior separator -- meaning the candidate "new header" row above it was already committed as a body row of the prior table -- the function falls back to `return text` instead of holding. That emits one "stray separator" on the rollback delta, which is broken markup but the lesser evil compared to non-monotonic rollback. The fix preserves the well-formed multi-table case where the second table is separated from the first by a blank line; the empty-line break in the walk fires before reaching the prior separator. Tests added: - `test_back_to_back_tables_keep_committable_monotonic` (the exact reviewer repro) - `test_second_table_after_blank_line_still_holds_header` (verifies the fix doesn't regress the blank-line-separated case) The outdated comment from gemini-code-assist on `_is_excluded_asterisk` suggesting we also include `\n` and end-of-string in the list-marker exclusion is already addressed in the second commit on this branch, which broadened the helper to match remend's `shouldSkipAsterisk` exactly. 79 streaming tests pass / 3598 total / 1 pre-existing unrelated failure. --------- Co-authored-by: Claude <noreply@anthropic.com>

gemini-code-assist Bot reviewed Apr 6, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 6, 2026

View reviewed changes

Comment thread src/chat_sdk/adapters/whatsapp/adapter.py Outdated

Comment thread src/chat_sdk/adapters/teams/adapter.py Outdated

patrick-chinchill added 3 commits April 6, 2026 15:15

security: Validate Slack response_url against *.slack.com (SSRF fix)

4a70933

ci: Add TestPyPI publish workflow (manual trigger)

1edeff5

patrick-chinchill merged commit 5b99696 into main Apr 6, 2026
0 of 3 checks passed

patrick-chinchill added a commit that referenced this pull request Apr 7, 2026

Merge pull request #13 from Chinchill-AI/fix/pr-review-comments

6d0340a

fix: Address unresolved PR review comments from #1, #7, #8

patrick-chinchill mentioned this pull request Apr 7, 2026

Copy TS webhook JSON fixtures and write parity tests #18

Closed

patrick-chinchill mentioned this pull request Apr 24, 2026

Promote per-adapter card helpers into shared/adapter_utils.py (emoji converter, button styles, GFM table, fallback text) #70

Open

4 tasks

patrick-chinchill mentioned this pull request May 22, 2026

Revisit hand-rolled markdown parser: close setext / footnotes / HTML / indented-code gaps #69

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: Fix all critical and high findings from security audit#1

security: Fix all critical and high findings from security audit#1
patrick-chinchill merged 4 commits into
mainfrom
security-fixes

patrick-chinchill commented Apr 6, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Apr 6, 2026

Uh oh!

patrick-chinchill Apr 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		async with session.get(BOT_FRAMEWORK_OPENID_CONFIG_URL) as resp:
		openid_config = await resp.json()

-                    async with session.get(BOT_FRAMEWORK_OPENID_CONFIG_URL) as resp:
-                        openid_config = await resp.json()
+                    async with session.get(BOT_FRAMEWORK_OPENID_CONFIG_URL) as resp:
+                        if not resp.ok:
+                            self._logger.error(
+                                "Failed to fetch Bot Framework OpenID config",
+                                {"status": resp.status, "url": BOT_FRAMEWORK_OPENID_CONFIG_URL},
+                            )
+                            return self._make_response("Unauthorized", 401)
+                        openid_config = await resp.json()

Conversation

patrick-chinchill commented Apr 6, 2026

Security Fixes

Critical

High

Medium

Also

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

patrick-chinchill Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant