feat(slack): gate private channels/DMs behind allowPrivateChannels toggle#684
Open
fredwanteeed wants to merge 2 commits into
Open
feat(slack): gate private channels/DMs behind allowPrivateChannels toggle#684fredwanteeed wants to merge 2 commits into
fredwanteeed wants to merge 2 commits into
Conversation
…ggle Add a workspace-wide `allowPrivateChannels` Slack setting (default true, backward-compatible). When off, the bot only acts in public channels and declines private channels, group DMs, and 1:1 DMs with a polite reply. Enforcement is a single guard run first in all three entry points (handleAppMention, handleDirectMessage, handleRepoSelection), so no status or session is created before the gate. Privacy is read from conversations.info.is_private, fail-closed when the operator set deny; im/mpim are treated as private without an API call. The toggle is read (cached) from the control plane with a last-known-good KV fallback so a previously-observed deny survives a control-plane outage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a workspace-wide ChangesSlack Private Channels Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/slack-bot/src/integration-settings.ts (1)
101-117: ⚖️ Poor tradeoffCold-start outage path re-hits the control plane on every event.
When the fetch fails and no KV value was ever recorded, you intentionally return
DEFAULT_ALLOW_PRIVATE_CHANNELSwithout caching so a freshly-set deny is picked up on recovery. The tradeoff is that during a sustained outage on a fresh install, every Slack event issues an uncached control-plane request (no in-flight coalescing either), which can amplify load against an already-degraded control plane. The behavior is correct; consider a short negative-cache or single-flight to bound the request rate if event volume is non-trivial.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/slack-bot/src/integration-settings.ts` around lines 101 - 117, When kv.get(LAST_KNOWN_KV_KEY) fails and you return DEFAULT_ALLOW_PRIVATE_CHANNELS, add a short negative-cache or single-flight to avoid re-hitting the control plane on every event: either (A) set localCache = { allowPrivateChannels: DEFAULT_ALLOW_PRIVATE_CHANNELS, timestamp: Date.now() } and honor a small NEGATIVE_CACHE_TTL before trying kv.get again (do not persist this to KV), or (B) implement a single-flight/in-flight promise keyed for this fetch so concurrent events reuse the same kv.get promise (e.g., an inFlightFetch map used by the function that calls kv.get). Ensure you reference LAST_KNOWN_KV_KEY, localCache and DEFAULT_ALLOW_PRIVATE_CHANNELS when making the change and do not change the long-term KV write behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/integrations/SLACK.md`:
- Around line 211-215: The docs claim `im:read` and `mpim:read` are required for
the "decline private contexts" path, but refuseIfPrivateContext treats
channelType === "im" | "mpim" as private and returns without calling
conversations.info; only non-DM ambiguous channels use conversations.info and
thus require groups:read (and channels:read for public), so update
docs/integrations/SLACK.md to remove `im:read` and `mpim:read` from the
"Required Slack scopes when off" sentence and reword it to state that
conversations.info (and groups:read) are required only for non-DM channel
lookups (public vs private ambiguity), while DM/mpim denial does not call the
API.
In `@packages/slack-bot/src/index.test.ts`:
- Around line 386-389: The ordering assertions using
order.indexOf("channelInfo") can pass vacuously if "channelInfo" is missing, so
add an explicit presence guard for "channelInfo" before the two index
comparisons: assert that the test array (order) contains "channelInfo" (e.g.,
expect(order).toContain("channelInfo") or
expect(order.indexOf("channelInfo")).not.toBe(-1)) and only then perform the
existing expects that compare indexOf("channelInfo"), indexOf("status"), and
indexOf("session"); update the assertions around the existing order variable and
the lines referencing "channelInfo", "status", and "session" accordingly.
---
Nitpick comments:
In `@packages/slack-bot/src/integration-settings.ts`:
- Around line 101-117: When kv.get(LAST_KNOWN_KV_KEY) fails and you return
DEFAULT_ALLOW_PRIVATE_CHANNELS, add a short negative-cache or single-flight to
avoid re-hitting the control plane on every event: either (A) set localCache = {
allowPrivateChannels: DEFAULT_ALLOW_PRIVATE_CHANNELS, timestamp: Date.now() }
and honor a small NEGATIVE_CACHE_TTL before trying kv.get again (do not persist
this to KV), or (B) implement a single-flight/in-flight promise keyed for this
fetch so concurrent events reuse the same kv.get promise (e.g., an inFlightFetch
map used by the function that calls kv.get). Ensure you reference
LAST_KNOWN_KV_KEY, localCache and DEFAULT_ALLOW_PRIVATE_CHANNELS when making the
change and do not change the long-term KV write behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47031212-a533-478f-a746-def33565388b
📒 Files selected for processing (13)
docs/integrations/SLACK.mdpackages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/shared/src/slack/client.tspackages/shared/src/types/integrations.tspackages/slack-bot/src/dm-utils.test.tspackages/slack-bot/src/dm-utils.tspackages/slack-bot/src/index.test.tspackages/slack-bot/src/index.tspackages/slack-bot/src/integration-settings.test.tspackages/slack-bot/src/integration-settings.tspackages/web/src/components/settings/integrations/slack-integration-settings.test.tsxpackages/web/src/components/settings/integrations/slack-integration-settings.tsx
…ering - SLACK.md: im/mpim are declined from channel_type without a conversations.info call, so the gate needs only channels:read (public) + groups:read (private), not im:read/mpim:read. - index.test.ts: assert order contains "channelInfo" before the indexOf ordering checks so a regression can't pass vacuously via indexOf === -1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Thanks for the review. Addressed in 46a04a7:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a workspace-wide
allowPrivateChannelsSlack setting (Settings → Integrations → Slack), mirroring the existingmentionsPolicyglobal-only pattern.Motivation : https://shopify.engineering/under-the-river
true— backward-compatible; existing installs are unaffected.How it works
refuseIfPrivateContext) runs as the first action in all three session-creating ingresses —handleAppMention,handleDirectMessage, and the/interactionsrepo-select path (handleRepoSelection) — so nothing (status, "Working on…", session) is emitted before the gate. This also covers existing-thread follow-ups.conversations.info.is_private(fail-closed: only a confirmed public channel is allowed).im/mpimare private with no API call.GET /integration-settings/slack) via the service binding, cached ~60s in memory with a last-known-good KV fallback, so a previously-observed deny survives a control-plane outage and the bot doesn't storm a downed control plane.conversations.infocall and no guard work happen — behavior is unchanged with zero added cost.Why default-allow (note for maintainers)
Chosen for backward compatibility on upgrade. A coding agent reachable from private DMs is a data-exfiltration surface, so operators concerned about that should turn this off. The kill-switch is bounded, not absolute: a deny set during a control-plane outage, or on a cold isolate that never fetched, isn't enforced until a fetch succeeds (in-memory TTL + reconnect). If you'd prefer secure-by-default, it's a one-line default flip in
resolveAllowPrivateChannels.Changes
shared:SlackGlobalSettings.allowPrivateChannels,DEFAULT_ALLOW_PRIVATE_CHANNELS, sharedresolveAllowPrivateChannels(!== false);SlackChannelInfo.is_private.control-plane: validateallowPrivateChannels(global-only boolean); surface it inresolveSlackSettings.slack-bot: newintegration-settings.ts(cached reader + sticky last-known-good);isPrivateMessageDispatchable(im+mpim);refuseIfPrivateContextguard wired into the three entry points.web: toggle Switch + reset-dialog copy in the Slack settings panel.docs/integrations/SLACK.md: toggle behavior, required Slack scopes (groups:read/im:read/mpim:read), and the propagation SLA.Testing
npm run typecheck— clean across all packagesnpm test— shared 169, control-plane 1170, slack-bot 101, web 264 — all passingisPrivateMessageDispatchable(im/mpim/public), the gate end-to-end (private/public/im/mpim/fail-closed/toggle-on//interactionsback-door), andgetAllowPrivateChannelscache + fail-closed behavior.Summary by CodeRabbit
New Features
Behavior
Documentation
Tests