Skip to content

feat(mcp): allow dev Slack MCP canvas reads - #111067

Open
dmarticus wants to merge 3 commits into
masterfrom
posthog/slack-mcp-read-canvases
Open

dmarticus wants to merge 3 commits into
masterfrom
posthog/slack-mcp-read-canvases

Conversation

@dmarticus

@dmarticus dmarticus commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Internal Slack MCP testers cannot read canvases through the restricted development app.

Why: Canvas reads let internal testers validate Slack MCP without widening the production connector.

Changes

  • The restricted dev connection requests canvases:read; the production catalog keeps its existing scope list.
  • The dev catalog description discloses access to canvases that the authorizing user can access.
  • The setup guide includes the user scope, canvas verification, and reconnection requirement.

How did you test this code?

The catalog sync suite covers the dev-only scope transformation, template disclosure, and production exclusion.

Test rationale: The existing dev Slack sync test now fails if canvas access leaks into production or disappears from the dev template.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

Updated the internal Slack setup guide with the user scope, canvas verification, and reconnection requirement.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: PostHog Desktop Codex, GPT-5

Skills: /writing-user-facing-copy, /writing-tests, and /writing-pr-descriptions. Greptile feedback moved canvas access to the restricted dev override. The diff contains no customer data or private operational details.


Created with PostHog Desktop

Generated-By: PostHog Desktop
Task-Id: 2005db76-9172-4edb-83b9-1f99828992f0
@dmarticus dmarticus self-assigned this Oct 2, 2026
@trunk-io

trunk-io Bot commented Oct 2, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Generated-By: PostHog Desktop
Task-Id: 2005db76-9172-4edb-83b9-1f99828992f0
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 8e596bf2-1b23-47d5-8e44-bff48257f491

📥 Commits

Reviewing files that changed from the base of the PR and between 14a4e55 and c340583.

📒 Files selected for processing (3)
  • docs/internal/slack-local-setup-guide.md
  • products/mcp_store/backend/catalog_sync.py
  • products/mcp_store/backend/test/test_catalog_sync.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The Slack development catalog now describes canvas access and appends the canvases:read scope to the template. Its test checks the scope passed to the probe. The setup guide adds the scope to the Slack manifest, documents its access, and adds canvas verification and reconnect instructions.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to c3405

The development authorization retains canvas access through Slack’s advertised-scope filter, and the setup guide explains when to reconnect. No actionable merge-blocking risk remains beyond normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c3405

Canvas reading increases the data available to authorized development connections. Production permissions remain unchanged, and project restrictions are enforced during installation and connection use. No introduced authorization bypass was established, but deployed restrictions and provider-side token behavior were not verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The intended new authority covers canvases accessible to the authorizing Slack user, not merely canvases in public channels. Application-mediated access is restricted to configured development projects. Provider-side enforcement of the user-accessible canvas boundary was not independently verified.

Trust Boundaries and Controls

  • observed — An attacker-supplied template ID does not bypass project eligibility: installation performs a team-scoped template lookup. OAuth context resolution, proxy authentication, token-header construction, and tool transport also check the current development credential-source allowlist, including for existing installations.
  • observed — The development feature flag adds a user-facing gate and fails closed on evaluation errors. The security restriction is also enforced by server-side project eligibility rather than relying solely on feature-flag visibility.

Resilience and Maintainability Implications

  • observed — The credential-source conversion guard checks for installations separately from the subsequent template update, while installation creation proceeds independently. This concurrency window predates the PR. Runtime project checks remain intact, and no new authorization bypass was established; terminal refresh behavior for a production token linked to a development-converted template remains unresolved.

Hardening Proposals

  • proposed — Consider serializing credential-source conversion with installation creation so the existing disconnect-before-conversion invariant holds under concurrency. This addresses pre-existing transition hardening, not an established vulnerability introduced by the canvas permission.
🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the problem, user-visible changes, testing, test rationale, release status, documentation, and agent context. However, it marks the change as behind a feature flag, while the pr… Verify the changed code for feature-flag checks and select the correct release-status option. Add the agent-session link, and record the local CodeRabbit CLI findings and dispositions or the reason the local pass was skipped. Include any re…
Full details: Description check

Explanation

The description covers the problem, user-visible changes, testing, test rationale, release status, documentation, and agent context. However, it marks the change as behind a feature flag, while the provided change summary shows a restricted development-app scope and no feature-flag change. The agent context also omits the required session link and explicit local CodeRabbit CLI result or skip reason.

Resolution

Verify the changed code for feature-flag checks and select the correct release-status option. Add the agent-session link, and record the local CodeRabbit CLI findings and dispositions or the reason the local pass was skipped. Include any required duplicate, patch-coverage, new-events-schema, and public-artifact gate results if applicable.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

@dmarticus
dmarticus marked this pull request as ready for review October 2, 2026 18:32
@parameterai

parameterai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Risk: Medium · 1 medium

This increment is the fix for the earlier Slack canvas-scope finding: canvases:read is removed from the production catalog entry and instead granted only by the internal dev entry, which stays disabled and invisible except for teams an operator lists in MCP_STORE_SLACK_DEV_ALLOWED_TEAM_IDS. The dev gate is enforced at listing, authorization, token refresh, and upstream proxying, and the entry's description now matches what the scope grants. No new security issue found in this increment.

Sentinel reviewed c340583 · Review settings

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Adds a new permission scope to the Slack integration.

The development-app setup needs a canvas-scope configuration step before the new authorization flow can be relied on.

Reviews (2) · Last reviewed commit: "docs(mcp): clarify Slack scope reauthori..."

Comment thread products/mcp_store/backend/catalog.py Outdated
Comment thread docs/internal/slack-local-setup-guide.md Outdated
Comment thread products/mcp_store/backend/catalog.py Outdated
disabled=True,
# Private-channel, DM, email, and write scopes require separate security approval.
oauth_scope_allowlist=(
"canvases:read",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Slack MCP scope add exposes private-channel and DM canvas content, crossing the entry's own security boundary

Adding canvases:read grants the Slack MCP connection read access to canvases attached to private channels and DMs — the exact content class the allowlist's own comment (catalog.py:325: "Private-channel, DM, email, and write scopes require separate security approval") reserves for separate approval. Every other scope in the list is public-only or profile-only, including the deliberate search:read.public; Slack has no public-only canvas scope, so this one is broad by necessity. The list is the only public/private control: it flows via catalog_sync.py:105 into template.oauth_scope_allowlist and then into the authorize URL (views.py:1559, requested_oauth_scopes at oauth.py:115), and nothing downstream filters by publicness.

How:

  1. A member in an allowed project connects Slack via PostHog; the OAuth request now includes canvases:read.
  2. Slack's MCP server enables canvas read tools for that user token.
  3. Canvas content attached to private channels or DMs — content the allowlist otherwise excludes (no groups:history/dms:history) — becomes readable through PostHog MCP tools and the AI canvas path (call_member_server_tool, facade/api.py).
  4. For a team-scope shared installation, member_installation_for_host (facade/api.py:590) lets any team member ride that token, exposing the connecting user's private/DM canvases to others.

Fix: Hold canvases:read until the separate private-channel/DM security approval the comment requires is documented in the PR, or drop it if Slack later offers a public-only canvas scope — and update the comment either way.


React with 👍 if useful or 👎 if not

Comment thread docs/internal/slack-local-setup-guide.md
Comment thread products/mcp_store/backend/catalog.py Outdated
@dmarticus dmarticus added the stamphog Request AI approval (no full review) label Oct 2, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not approved — escalated to a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

This adds an OAuth scope that reaches private-channel and DM canvases, which the entry's own comment reserves for separate security approval. Two reviewers raised this as unresolved security concerns, and the diff doesn't address them.

  • Author wrote 100% of the modified lines and has 2 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot].
  • Unresolved security concern from @greptile-apps and @parameterai on products/mcp_store/backend/catalog.py: canvases:read has no public-only variant, so it exposes private-channel and DM canvas content. The catalog comment says that kind of access needs separate security approval.
  • The Slack catalog description still says only public channels, messages, and user profiles. Users authorizing the connection are not told that canvas access is requested.
  • Unresolved @greptile-apps comment on docs/internal/slack-local-setup-guide.md: the dev Slack app must be granted canvases:read before the new canvas check can succeed, and the guide doesn't say so.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 1L, 1F substantive, 5L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (5L, 2F, two-areas, feat)
stamphog 2.3.1 .stamphog/policy.yml @ fc9382a · reviewed head fc9382a

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Oct 2, 2026
@greptile-apps

greptile-apps Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

The escalation is valid. canvases:read is not public-only, so adding it expands access to private-channel and DM canvases while the catalog comment explicitly reserves those scopes for separate security approval. The catalog description also still promises only public-channel, message, and profile access, so it under-discloses the requested scope.

This should remain unapproved until security explicitly approves the broader access, or Slack provides a public-only canvas scope. Once approved, update the Slack catalog description and dev-app setup guide/manifest to document and grant canvases:read before the canvas smoke test. Retain the reconnect instruction for installations authorized before the scope sync. Re-add stamphog only after those changes and the required security approval are documented.

Generated-By: PostHog Desktop
Task-Id: 2005db76-9172-4edb-83b9-1f99828992f0
@dmarticus dmarticus changed the title feat(mcp): allow Slack MCP canvas reads feat(mcp): allow dev Slack MCP canvas reads Oct 2, 2026
@trunk-io

trunk-io Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant