Skip to content

[4.41/SL0] Slack: installation-scoped caches, drop unresolved installs, strict response_url, bounded regexes #205

Description

@patrick-chinchill

Summary

This ports the Slack parts of four upstream hardening commits. In multi-workspace mode, installation-owned state (user profiles, the display-name index, channel names, unfurl metadata) is keyed globally, so data resolved with one workspace's token can be served to another. Slash and interactive payloads whose installation cannot be resolved still reach handlers with no token context. response_url validation accepts any *.slack.com host, and ephemeral-id decoding falls back to the raw string. One link-extraction regex over untrusted text is unbounded.

Upstream changes

  • f485255b fix(adapters): harden webhook tenant isolation (#877) — chat@4.40.0 — replaces userCacheScope() with installationCacheScope(). It scopes the channel-name cache and keys unfurl metadata by installation, channel and ts, so enrichLinks gains a channelId. Unresolved slash, interactive and socket-interactive payloads get an empty 200 and are not dispatched. The logging change in this commit belongs to [4.41/LOG1] Stop logging raw webhook bodies / PII across adapters #187.
  • 907450d7 fix(slack): Enterprise Grid support (#724) — chat@4.35.0 — only the cache part belongs here: installationId in the request context, plus scoped slack:user: / slack:user-by-name: keys, including user_change invalidation. The rest of the commit belongs to [4.41/SL8] Slack: guarded downloads, Enterprise Grid org-wide installs, egress proxy #213.
  • 7609d8f6 fix(adapters): validate external request targets (#876) — chat@4.40.0 — isTrustedSlackResponseUrl requires https, no userinfo, no port, and a host in {hooks.slack.com, hooks.slack-gov.com}. It is checked at encode, at decode and before the send. There is no raw-string decode fallback. editMessage/deleteMessage reject undecodable ephemeral: ids.
  • 4cc3445c fix(teams,slack): follow-up hardening for html and url parsing (#779) — chat@4.37.0 — the Slack half bounds the link-unfurl fallback to /<(https?:\/\/[^>]{1,2048})>/g. The Teams half belongs to [4.41/T1] Teams routing & outbound text: stop <at> rewriting, conversationType in thread ids, per-service-URL clients #216.

Current Python behavior

  • src/chat_sdk/adapters/slack/types.py:506-516: RequestContext has no installation_id.
  • Unscoped keys in src/chat_sdk/adapters/slack/adapter.py:
    • :1284 slack:user:{user_id}
    • :1346 slack:user-by-name:{name}
    • :1373 slack:channel:{channel_id}
    • :2859 the user_change delete
    • :3031 the reverse-index read in _resolve_outgoing_mentions
    • :3208 / :3246 slack:unfurls:{ts}
  • adapter.py:3226 _enrich_links(links, message_ts) has no channel. Its caller is at :3386.
  • adapter.py:1591-1617: an HTTP slash command with a missing or unresolved installation falls through to _handle_slash_command (:1617). Interactive payloads do the same at :1620-1639. The event path (:1661-1683) already returns early.
  • adapter.py:2513-2526: socket slash in multi-workspace mode with no team_id dispatches without context. Socket interactive (:2531-2547) does the same when the team id is falsy. A None resolution already acks and returns.
  • adapter.py:4738-4741: encode does no validation. At :4761-4762, decode returns {"response_url": decoded, "user_id": ""} on JSON failure. At :4784-4786, _send_to_response_url accepts any https host ending in .slack.com, allows a port and userinfo, and rejects GovSlack.
  • adapter.py:3736 (edit_message) and :3788 (delete_message): a malformed ephemeral: id falls through to chat.update/chat.delete.
  • api/__init__.py:685-688 _assert_slack_response_url uses the same *.slack.com check (documented at docs/UPSTREAM_SYNC.md:681).
  • adapter.py:3100 re.finditer(r"<(https?://[^>]+)>", ...) is unbounded.

Scope

  • RequestContext.installation_id: str | None = None. Set it everywhere a context is built from a resolved installation: HTTP slash/interactive/events and socket events/slash/interactive.
  • _installation_cache_scope() returns f"{installation_id}:", or "" when unset. Add _unfurl_cache_key(channel_id, message_ts). Apply both to every key listed above.
  • _enrich_links(links, channel_id, message_ts) returns early unless all three are present. Pass event.get("channel") at the caller and at the _handle_message_changed writer.
  • Multi-workspace HTTP slash/interactive with a missing or unresolved installation: return {"body": "", "status": 200} with no dispatch. Socket slash/interactive: ack and return. Single-workspace mode is unchanged.
  • _SLACK_RESPONSE_URL_HOSTS = frozenset({"hooks.slack.com", "hooks.slack-gov.com"}) plus _is_trusted_slack_response_url(url) using urlsplit: scheme == "https", no username/password, port is None, lower-cased hostname in the set.
  • Encode raises ValidationError("slack", "Refusing to encode an untrusted Slack response_url"). Decode returns None unless it gets a dict with a trusted str responseUrl and a non-empty str userId, including on JSON failure. _send_to_response_url raises before httpx is imported.
  • edit_message/delete_message raise ValidationError("slack", "Invalid Slack ephemeral message ID") for an undecodable ephemeral: id.
  • Route api/__init__.py _assert_slack_response_url through the same helper.
  • Add a module-level _BRACKETED_URL_PATTERN = re.compile(r"<(https?://[^>]{1,2048})>") at :3100.

Out of scope

Porting notes

  • Read the scope from the ContextVar at call time. Event dispatch runs in contextvars.copy_context() (adapter.py:1675-1677). Tasks copy context at creation, so work spawned from handlers inherits the installation.
  • Only a truthy installation_id scopes the key. "" means unscoped, matching upstream's installationId ? … : "".
  • urlsplit(...).port raises ValueError on a non-numeric port. Treat that as untrusted. Compare hosts by exact set membership, never endswith, so lookalike suffix hosts are rejected.
  • Decision: in _handle_block_actions (adapter.py:1857-1858), let an untrusted response_url raise, as upstream does. Slack signs these payloads, so this only fires on forged or verifier-bypassed input.
  • is_enterprise_install is the string "true" in forms and a bool over socket. Keep today's parsing. Grid changes belong to [4.41/SL8] Slack: guarded downloads, Enterprise Grid org-wide installs, egress proxy #213.
  • Changed key shapes cause one-time cache misses after upgrade. That is harmless.

Tests

packages/adapter-slack/src/index.test.ts is not fidelity-mapped (MAPPING covers packages/chat/src only). Port to tests/test_slack_webhook.py / tests/test_slack_api.py:

  • drops slash commands when no installation is found, drops interactive payloads when no installation is found, drops socket interactive payloads with no installation
  • describe installation-scoped caches: scopes the user profile cache by installation, scopes the channel-name cache by installation, uses unscoped keys without a request context (single-workspace), scopes the display-name reverse index by installation, scopes unfurl metadata by installation and channel, resolves outgoing mentions from the installation-scoped index, invalidates the scoped cache entry on user_change
  • rejects the legacy non-JSON responseUrl format; it.each rejects untrusted response_url %s (http scheme, lookalike suffix host, userinfo, non-default port, foreign host; use pytest.mark.parametrize); rejects an untrusted encoded response_url before fetching; defends the response_url fetch sink against untrusted callers
  • parses bracketed links from text and bounds their length
  • Python-specific:
    • hooks.slack-gov.com round-trips through encode and decode.
    • A malformed ephemeral: id in delete_message raises, and the chat_delete AsyncMock is assert_not_awaited().
    • A socket slash with no team_id in multi-workspace mode does not dispatch.
    • A non-numeric port is rejected.
    • Rewrite the existing unfurl tests in tests/test_slack_webhook.py (:1211, :1255, :1281, :1320, :1387) to the new key rather than duplicating them.
    • Tighten tests/test_slack_api_primitives.py::test_rejects_non_slack_response_urls.

Acceptance criteria

  • The full validation command from CLAUDE.md passes.
  • docs/UPSTREAM_SYNC.md:681 is rewritten. Upstream's adapter now validates response_url, so the only remaining divergence is that the SDK-free primitive also validates.
  • CHANGELOG entry (security) under "Unreleased (4.41 wave)".
  • Consumer-visible changes are called out:
    • Multi-workspace apps no longer run handlers for unknown installations.
    • Cache key shapes change.
    • response_url is limited to hooks.slack.com / hooks.slack-gov.com.

Dependencies

None; this can start immediately. Blocks #213.

Metadata

  • Effort: M (~400 LOC incl. tests)
  • Consumer impact: low. Single-workspace deployments, such as downstream consumers (e.g. chinchill), see only the stricter response_url check and new key shapes. Streaming is unaffected.
  • Suggested branch: sync/4.41-sl0

Part of #184.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions