fix(slack-work-app): Listen to deletion events#3050
Conversation
🦋 Changeset detectedLatest commit: 4acdbc3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
TL;DR — Handles Slack Key changes
Summary ｜ 6 files ｜ 3 commits ｜ base: Automatic workspace cleanup on Slack app removal
Both events call
Shared
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good refactor — extracting cleanupWorkspaceInstallation into a shared service keeps the uninstall logic DRY across the API route and the new event handlers. The event subscriptions and scope changes look correct. Three issues to address below, one of which is a bug.
Claude Opus ｜ 𝕏
| deleteWorkspaceInstallation, | ||
| findWorkspaceConnectionByTeamId, | ||
| revokeSlackToken, | ||
| } from '../services'; |
There was a problem hiding this comment.
Circular import. This file imports from ../services (the barrel index.ts), but that barrel re-exports ./workspace-cleanup. Node can resolve this in many cases but it's fragile and can cause undefined at import time depending on evaluation order.
Import directly from the source modules instead:
import { clearWorkspaceConnectionCache, deleteWorkspaceInstallation, findWorkspaceConnectionByTeamId } from './nango';
import { revokeSlackToken } from './client';| clearWorkspaceConnectionCache(teamId); | ||
| logger.info({ teamId, connectionId, result }, 'Workspace cleanup completed'); | ||
|
|
||
| result.success = result.dbCleaned && result.nangoCleaned; |
There was a problem hiding this comment.
Bug: result.success is set on line 102, but the log on line 100 captures result before that assignment — so success will always be logged as false, even on a fully successful cleanup. Move the assignment before the log.
| clearWorkspaceConnectionCache(teamId); | |
| logger.info({ teamId, connectionId, result }, 'Workspace cleanup completed'); | |
| result.success = result.dbCleaned && result.nangoCleaned; | |
| clearWorkspaceConnectionCache(teamId); | |
| result.success = result.dbCleaned && result.nangoCleaned; | |
| logger.info({ teamId, connectionId, result }, 'Workspace cleanup completed'); | |
| return result; |
| const workspace = await findWorkspaceConnectionByTeamId(teamId); | ||
| if (!workspace) { | ||
| return c.json({ error: 'Workspace not found' }, 404); | ||
| } | ||
|
|
||
| if (workspace.botToken) { | ||
| const tokenRevoked = await revokeSlackToken(workspace.botToken); | ||
| if (tokenRevoked) { | ||
| logger.info({ teamId }, 'Revoked Slack bot token'); | ||
| } else { | ||
| logger.warn({ teamId }, 'Failed to revoke Slack bot token, continuing with uninstall'); | ||
| } | ||
| } | ||
|
|
||
| // Delete from PostgreSQL first (recoverable), then Nango (point of no return) | ||
| const tenantId = workspace.tenantId; | ||
| const result = await cleanupWorkspaceInstallation({ teamId }); |
There was a problem hiding this comment.
Minor: findWorkspaceConnectionByTeamId is now called twice for the API uninstall path — once here for the 404 check, and again inside cleanupWorkspaceInstallation. cleanupWorkspaceInstallation already handles the "no workspace found" case gracefully (returns success: true), so this outer lookup + 404 could be removed. That said, preserving the 404 semantics for the API is reasonable — just calling it out as a potential simplification.
| const tokensEvent = payload.event as | ||
| | { tokens?: { oauth?: string[]; bot?: string[] } } | ||
| | undefined; | ||
| const revokedBotTokens = tokensEvent?.tokens?.bot ?? []; | ||
| const revokedOauthTokens = tokensEvent?.tokens?.oauth ?? []; |
There was a problem hiding this comment.
revokedOauthTokens is extracted and logged but never acted on. If only OAuth (user) tokens are revoked and no bot tokens are revoked, the event is silently handled with no cleanup. This is fine if the system doesn't depend on user tokens — but worth a brief inline note or log line to make this intentional behavior explicit.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) workspace-cleanup.ts:60-97 Partial failure cleanup proceeds to Nango deletion
Issue: When database cleanup fails in the try/catch block (lines 60-92), dbCleaned is set to false but the function continues to call deleteWorkspaceInstallation(connectionId) at line 94. This deletes OAuth tokens from Nango while database records (channel configs, user mappings, MCP configs, workspace) may still exist.
Why: This creates an inconsistent state that is difficult to recover from. Future cleanup retries will fail because findWorkspaceConnectionByTeamId may still return the workspace record, but the Nango token is already gone. Slack may redeliver app_uninstalled events if the response is slow, and retries will encounter this partial state.
Fix: Consider one of:
- Skip Nango deletion when DB fails: Return early after catching the error, before proceeding to Nango
- Reverse the order: Delete from Nango first (the "point of no return"), then clean up DB records
- Transaction: Wrap DB operations in a transaction for atomicity
- Idempotent cleanup: Make each step idempotent so retries can pick up where they left off
Refs:
🟠 2) slack-app-manifest.json Missing changeset for scope removal
Issue: This PR removes bot scopes (channels:join, search:read.public, search:read.files, search:read.users) without a corresponding changeset.
Why: Removing OAuth scopes is a potentially breaking change for existing Slack app installations. Production Slack apps must be manually updated at https://api.slack.com/apps. Per AGENTS.md, a changeset is required for user-facing changes to published packages, and manifest changes affect agents-work-apps.
Fix: Run:
pnpm bump patch --pkg agents-work-apps "Remove unused bot scopes (channels:join, search:read.*) and add app_uninstalled/tokens_revoked event listeners"Refs:
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
dispatcher.ts:205-208Missing error stack trace in app_uninstalled handler - 🟡 Minor:
dispatcher.ts:242-245Missing error stack trace in tokens_revoked handler - 🟡 Minor:
workspaces.ts:564-569API returns success:true even on partial failure
💭 Consider (1) 💭
💭 1) workspace-cleanup.ts:9-14 Circular import through barrel file
Issue: workspace-cleanup.ts imports from ../services (the barrel file) which re-exports workspace-cleanup.ts, creating a circular dependency.
Why: While this usually works in practice because the imports (clearWorkspaceConnectionCache, deleteWorkspaceInstallation, etc.) come from other files (nango.ts, client.ts), circular imports through barrels can cause initialization order issues in edge cases.
Fix: Consider importing directly from source files:
import { clearWorkspaceConnectionCache, deleteWorkspaceInstallation, findWorkspaceConnectionByTeamId } from './nango';
import { revokeSlackToken } from './client';💡 APPROVE WITH SUGGESTIONS
Summary: The refactoring to centralize cleanup logic is a good improvement, but the partial failure handling should be addressed. The main concern is that database cleanup failures don't prevent Nango deletion, which can leave the system in an inconsistent state. Additionally, a changeset is needed for the scope removals in the Slack manifest.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
slack-app-manifest.json:74-81 |
Event subscriptions need verification | Valid — handlers exist in dispatcher.ts, informational only |
slack-manifest/SKILL.md:56-61 |
Skill documentation drift | Out of PR scope — documentation update is separate concern |
nango.ts |
External Nango call lacks timeout | Pre-existing issue, not introduced by this PR |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-sre |
6 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-errors |
5 | 0 | 0 | 0 | 3 | 0 | 0 |
pr-review-devops |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-standards |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
| Total | 15 | 2 | 1 | 0 | 3 | 0 | 4 |
Note: SRE finding about partial failure merged with errors finding about same issue (both identified the Nango-proceeds-despite-DB-failure problem).
| result.dbCleaned = true; | ||
| } catch (error) { | ||
| logger.error({ error, teamId, connectionId }, 'Failed to clean up database records'); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Partial failure cleanup proceeds to Nango deletion
Issue: When database cleanup fails (caught at lines 60-92), the function still proceeds to delete from Nango (line 94). This creates an inconsistent state: OAuth tokens are deleted but database records remain orphaned.
Why: Future cleanup retries will fail because findWorkspaceConnectionByTeamId may still return the workspace record, but the Nango token is already gone. This makes recovery difficult and can leave stale data in the database.
Fix: Consider one of:
- Skip Nango deletion when
dbCleanedis false and return early - Reverse the order (delete Nango first since it's the "point of no return")
- Wrap DB operations in a transaction for atomicity
// Option 1: Early return on DB failure
} catch (error) {
logger.error({ error, teamId, connectionId }, 'Failed to clean up database records');
// Don't proceed to Nango deletion if DB cleanup failed
return result;
}Refs:
- Slack Events API retry behavior — events can be redelivered
| .catch((err: unknown) => { | ||
| const errorMessage = err instanceof Error ? err.message : String(err); | ||
| logger.error({ errorMessage, teamId }, 'Failed to handle app_uninstalled'); | ||
| }) |
There was a problem hiding this comment.
🟡 Minor: Missing error stack trace in catch handler
Issue: The error handler extracts err.message but discards the stack trace.
Why: For background work running asynchronously, stack traces are critical for diagnosing error origins. The existing handlers for app_mention (line 148) and direct_message (line 181) capture errorStack — this should follow the same pattern.
Fix:
| .catch((err: unknown) => { | |
| const errorMessage = err instanceof Error ? err.message : String(err); | |
| logger.error({ errorMessage, teamId }, 'Failed to handle app_uninstalled'); | |
| }) | |
| .catch((err: unknown) => { | |
| const errorMessage = err instanceof Error ? err.message : String(err); | |
| const errorStack = err instanceof Error ? err.stack : undefined; | |
| logger.error({ errorMessage, errorStack, teamId }, 'Failed to handle app_uninstalled'); | |
| }) |
Refs:
- dispatcher.ts:148 — existing pattern for app_mention
| .catch((err: unknown) => { | ||
| const errorMessage = err instanceof Error ? err.message : String(err); | ||
| logger.error({ errorMessage, teamId }, 'Failed to handle tokens_revoked (bot)'); | ||
| }) |
There was a problem hiding this comment.
🟡 Minor: Missing error stack trace in catch handler
Issue: Same as above — error handler discards the stack trace.
Fix:
| .catch((err: unknown) => { | |
| const errorMessage = err instanceof Error ? err.message : String(err); | |
| logger.error({ errorMessage, teamId }, 'Failed to handle tokens_revoked (bot)'); | |
| }) | |
| .catch((err: unknown) => { | |
| const errorMessage = err instanceof Error ? err.message : String(err); | |
| const errorStack = err instanceof Error ? err.stack : undefined; | |
| logger.error({ errorMessage, errorStack, teamId }, 'Failed to handle tokens_revoked (bot)'); | |
| }) |
| if (!result.success) { | ||
| logger.error({ teamId, result }, 'Workspace uninstall partially failed'); | ||
| } | ||
|
|
||
| clearWorkspaceConnectionCache(teamId); | ||
| logger.info({ connectionId, teamId }, 'Deleted workspace installation and cleared cache'); | ||
| logger.info({ teamId }, 'Workspace uninstalled via API'); | ||
| return c.json({ success: true }); |
There was a problem hiding this comment.
🟡 Minor: API returns success: true even on partial failure
Issue: When cleanupWorkspaceInstallation returns success: false (partial failure), the error is logged but the API still returns { success: true }.
Why: This misleads API consumers into thinking the uninstall was fully successful when it may have left orphaned data.
Fix: Return the actual result status:
| if (!result.success) { | |
| logger.error({ teamId, result }, 'Workspace uninstall partially failed'); | |
| } | |
| clearWorkspaceConnectionCache(teamId); | |
| logger.info({ connectionId, teamId }, 'Deleted workspace installation and cleared cache'); | |
| logger.info({ teamId }, 'Workspace uninstalled via API'); | |
| return c.json({ success: true }); | |
| if (!result.success) { | |
| logger.error({ teamId, result }, 'Workspace uninstall partially failed'); | |
| } | |
| logger.info({ teamId }, 'Workspace uninstalled via API'); | |
| return c.json({ success: result.success }); |
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
Ito Test Report ❌14 test cases ran. 7 failed, 1 additional finding, 6 passed. Overall, the unified run failed with 8 of 14 test cases failing (6 passed), driven mainly by regressions in Slack uninstall behavior and webhook cleanup despite several security and input-hardening checks passing. The most important issues were a PR-introduced contract mismatch where UI/dev mode sends connection IDs like dev: that DELETE parsing rejects with 400 (blocking desktop/mobile/resilience/idempotency uninstall flows), PR-introduced webhook cleanup logic that can return 200/success without actually removing installed workspaces for app_uninstalled and bot-token tokens_revoked (including duplicate-event scenarios), and an existing localhost-development auth flaw where non-admins can be treated as owner and perform admin-only uninstalls, while enterprise-format uninstall, malformed/hostile input handling, and forged or stale webhook rejection behaved as expected. ❌ Failed (7)
🟡 Rage-click uninstall cannot reach success state due identifier mismatch
Relevant code:
const handleUninstall = async () => {
if (!workspace?.connectionId) return;
setUninstalling(true);
await uninstallWorkspace(workspace.connectionId);
setShowUninstallDialog(false);
setUninstalling(false);
};
function buildDevWorkspaceConnection(
devConfig: SlackDevConfig,
teamId: string
): SlackWorkspaceConnection {
const connection: SlackWorkspaceConnection = {
connectionId: `dev:${teamId}`,
teamId,
if (workspaceIdentifier.includes(':')) {
const teamMatch = workspaceIdentifier.match(/T:([A-Z0-9]+)/);
if (!teamMatch) {
return c.json({ error: 'Invalid connectionId format' }, 400);
}
teamId = teamMatch[1];
}🟡 Mobile uninstall blocked by backend identifier parser
Relevant code:
const handleUninstall = async () => {
if (!workspace?.connectionId) return;
setUninstalling(true);
await uninstallWorkspace(workspace.connectionId);
setShowUninstallDialog(false);
setUninstalling(false);
};
function buildDevWorkspaceConnection(
devConfig: SlackDevConfig,
teamId: string
): SlackWorkspaceConnection {
const connection: SlackWorkspaceConnection = {
connectionId: `dev:${teamId}`,
teamId,
if (workspaceIdentifier.includes(':')) {
const teamMatch = workspaceIdentifier.match(/T:([A-Z0-9]+)/);
if (!teamMatch) {
return c.json({ error: 'Invalid connectionId format' }, 400);
}
teamId = teamMatch[1];
}🟡 Refresh/back-forward uninstall scenario blocked by parser mismatch
Relevant code:
const handleUninstall = async () => {
if (!workspace?.connectionId) return;
setUninstalling(true);
await uninstallWorkspace(workspace.connectionId);
setShowUninstallDialog(false);
setUninstalling(false);
};
function buildDevWorkspaceConnection(
devConfig: SlackDevConfig,
teamId: string
): SlackWorkspaceConnection {
const connection: SlackWorkspaceConnection = {
connectionId: `dev:${teamId}`,
teamId,
if (workspaceIdentifier.includes(':')) {
const teamMatch = workspaceIdentifier.match(/T:([A-Z0-9]+)/);
if (!teamMatch) {
return c.json({ error: 'Invalid connectionId format' }, 400);
}
teamId = teamMatch[1];
}🟠 Duplicate event_id deliveries do not double-execute cleanup
Relevant code:
const eventId = payload.event_id as string | undefined;
if (isDuplicateEvent(eventId)) {
outcome = 'ignored_duplicate_event';
span.setAttribute(SLACK_SPAN_KEYS.OUTCOME, outcome);
logger.info({ eventId }, 'Ignoring duplicate event');
return { outcome };
}
const workspace = await findWorkspaceConnectionByTeamId(teamId);
if (!workspace) {
logger.warn({ teamId }, 'No workspace found for cleanup');
clearWorkspaceConnectionCache(teamId);
result.success = true;
return result;
}
export async function getConnectionAccessToken(connectionId: string): Promise<string | null> {
if (isSlackDevMode()) {
const devConfig = loadSlackDevConfig();
return devConfig?.botToken ?? null;
}
try {
const nango = getSlackNango();
const integrationId = getSlackIntegrationId();
const connection = await nango.getConnection(integrationId, connectionId);
return (🟡 Slack dashboard uninstall rejects dev connection identifier
Relevant code:
const handleUninstall = async () => {
if (!workspace?.connectionId) return;
setUninstalling(true);
await uninstallWorkspace(workspace.connectionId);
setShowUninstallDialog(false);
setUninstalling(false);
};
function buildDevWorkspaceConnection(
devConfig: SlackDevConfig,
teamId: string
): SlackWorkspaceConnection {
const connection: SlackWorkspaceConnection = {
connectionId: `dev:${teamId}`,
teamId,
if (workspaceIdentifier.includes(':')) {
const teamMatch = workspaceIdentifier.match(/T:([A-Z0-9]+)/);
if (!teamMatch) {
return c.json({ error: 'Invalid connectionId format' }, 400);
}
teamId = teamMatch[1];
}
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Forged-signature Slack webhook was rejected with 401, and no workspace cleanup side effect was observed. |  |
| Adversarial | Correctly signed but stale-timestamp tokens_revoked webhook was rejected with 401, with workspace state unchanged. |  |
| Adversarial | Hostile identifiers returned non-500 responses and hostile workspace query content rendered safely as escaped text. |  |
| Edge | Malformed connectionId containing a colon correctly returned HTTP 400, and workspace state stayed unchanged. |
 |
| Edge | OAuth-only tokens_revoked left the workspace installed after a 200 acknowledgment, as expected when tokens.bot is empty. |
 |
| Happy-path | Not a real app bug. Original timeout/block was environmental; enterprise-format DELETE was re-run successfully with HTTP 200 and success=true. |
 |
ℹ️ Additional Findings (1)
These findings are unrelated to the current changes but were observed during testing.
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | 🟠 Non-admin authorization boundary is broken for localhost Slack workspace uninstall flow. |  |
🟠 Non-admin authorization boundary is broken for localhost Slack workspace uninstall flow.
- What failed: The uninstall path is expected to reject non-admin users with forbidden behavior, but localhost development requests can be assigned tenantRole = owner, allowing uninstall.
- Impact: Non-admin tenant members can execute admin-only uninstall operations in local development, breaking authorization boundaries for destructive actions. This can cause unintended workspace cleanup and makes role-based QA validation unreliable.
- Steps to reproduce:
- Sign in as a non-admin member in the local development environment.
- Open the tenant Slack workspace management page.
- Attempt uninstall from the UI or send DELETE /work-apps/slack/workspaces/<TEAM_ID> from localhost origin.
- Observe that the request can pass admin authorization instead of returning forbidden behavior.
- Stub / mock context: The test ran with local development auth shortcuts: localhost requests in
agents-api/src/middleware/workAppsAuth.tsassign an owner role before downstream authorization, and manage-session login was bypassed for development to keep local execution deterministic. No API response mocks were used for Slack uninstall behavior itself. - Code analysis: I verified the protected uninstall route uses requireWorkspaceAdmin, but the development branch in workAppsAuth sets tenantRole to owner for localhost origins before permission checks. Because requireWorkspaceAdmin accepts owner/admin, the authorization gate is bypassed for non-admin members in this flow.
- Why this is likely a bug: The production code path for localhost development explicitly assigns an admin-equivalent role before route authorization, so non-admin users can satisfy an admin-only permission check.
Relevant code:
agents-api/src/middleware/workAppsAuth.ts (lines 34-52)
if (env.ENVIRONMENT === 'development') {
const origin = c.req.header('Origin');
if (origin) {
const originUrl = new URL(origin);
if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1') {
if (sessionUser?.id) {
c.set('userId', sessionUser.id);
c.set('tenantId', session?.activeOrganizationId || 'default');
c.set('tenantRole', 'owner');
} else {
c.set('userId', 'dev-user');
c.set('tenantId', 'default');
c.set('tenantRole', 'owner');
}packages/agents-work-apps/src/slack/middleware/permissions.ts (lines 21-23)
export function isOrgAdmin(tenantRole: string | undefined): boolean {
return tenantRole === OrgRoles.OWNER || tenantRole === OrgRoles.ADMIN;
}packages/agents-work-apps/src/slack/routes/workspaces.ts (lines 509-516)
createProtectedRoute({
method: 'delete',
path: '/{teamId}',
summary: 'Uninstall Workspace',
tags: ['Work Apps', 'Slack', 'Workspaces'],
permission: requireWorkspaceAdmin(),Commit: fecd644
Tell us how we did: Give Ito Feedback
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: Focused on 1 commit since last review (
ff527cda9 cleanup)
Delta Assessment
The new commit addresses the key concerns from the prior review:
✅ Changeset added — .changeset/combative-beige-rhinoceros.md now documents the scope removal and event listener additions, satisfying the prior Major issue about missing changelog entry.
✅ Cleanup refactored to step-based approach — The new implementation at workspace-cleanup.ts:60-91 uses a well-structured sequential steps array with individual error handling. This addresses the prior concern about partial failures because:
- Each step is tried independently and failures are tracked in the
failuresarray - The
resultobject accurately reflects which parts succeeded (dbCleaned,nangoCleaned) - Failures are explicitly logged with step name, enabling debugging
- The API route now returns
result.successinstead of always returningtrue
🕐 Pending Recommendations (2)
These items from the prior review remain unaddressed:
- 🟡
dispatcher.ts:205-208Missing error stack trace inapp_uninstalledhandler (compare toapp_mentionhandler at line 148) - 🟡
dispatcher.ts:242-245Missing error stack trace intokens_revokedhandler (compare todirect_messagehandler at line 181)
💭 Consider (1)
💭 1) workspace-cleanup.ts:9-14 Barrel file import pattern
Issue: The file imports from ../services (barrel) which re-exports this file, creating a circular reference.
Why: While Node.js typically handles this, it's fragile and can cause initialization issues. This was noted in the prior pullfrog review.
Fix: Import directly from source modules:
import { clearWorkspaceConnectionCache, deleteWorkspaceInstallation, findWorkspaceConnectionByTeamId } from './nango';
import { revokeSlackToken } from './client';✅ APPROVE
Summary: The delta commit successfully addresses the major concerns from the previous review. The changeset is now in place, and the cleanup logic has been refactored to a cleaner step-based approach with proper failure tracking. The remaining items (error stack traces in dispatcher catch blocks) are minor consistency improvements that don't block approval.
Discarded (0)
No items discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 1 | 0 | 0 | 2 | 0 |
| Total | 3 | 0 | 1 | 0 | 0 | 2 | 0 |
Note: Delta-scoped review — sub-reviewers not dispatched for this small delta (changeset + cleanup refactor only). Issues from prior review carried forward as Pending Recommendations.
Ito Test Report ❌14 test cases ran. 1 failed, 13 passed. Across 14 executed test cases, 13 passed and 1 failed, so the unified run is not fully clean due to a confirmed high-severity authorization defect where a tenant-a owner could successfully delete a tenant-b Slack workspace via DELETE /work-apps/slack/workspaces/{teamId} (introduced by this PR). Otherwise, uninstall behavior was broadly stable and defensive: valid teamId and encoded connectionId paths succeeded, repeated/rage-click/interrupted/mobile/stale-tab flows remained controlled, malformed/unknown/fuzzed identifiers returned expected 400/404 with healthy list checks, post-uninstall dependent endpoints failed safely without 500s, non-admin uninstall was correctly blocked with 403, and forged unsigned Slack uninstall-related webhooks were rejected with 401. ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Non-admin DELETE returned 403 Forbidden and the tenant-a workspace remained present. |  |
| Adversarial | Injection-style workspace identifiers were rejected with controlled 4xx responses. |  |
| Adversarial | Unsigned forged app_uninstalled callback was rejected with 401 and workspace data remained unchanged. |  |
| Adversarial | Unsigned forged tokens_revoked callback was rejected with 401 and workspace data remained unchanged. |  |
| Edge | Malformed connection IDs were rejected with 400 and service health remained stable. |  |
| Edge | Unknown workspace uninstall returned controlled 404 and list endpoint stayed healthy. |  |
| Edge | Repeating uninstall on the same identifier returned deterministic controlled responses. |  |
| Edge | Mobile uninstall dialog controls remained present and actionable, with no code-level indication of a mobile-only uninstall regression. |  |
| Logic | Post-uninstall channels/users/settings calls returned safe 404/200/200 responses with no 500s. |  |
| Logic | Stale-tab uninstall behavior failed safely with no phantom success and converged to empty state after reload. |  |
| Happy-path | Plain teamId uninstall path succeeded after valid fixture setup. |  |
| Happy-path | Encoded connectionId uninstall path succeeded for a valid identifier. |  |
| Happy-path | Uninstall flow stayed consistent after rage-click and interruption, and workspace list returned a stable HTTP 200 empty state. |  |
Commit: ff527cd
Tell us how we did: Give Ito Feedback
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: Focused on 1 commit since last review (
4acdbc37a verify tenant id)
Delta Assessment
The new commit addresses the critical cross-tenant security vulnerability flagged by Ito testing:
✅ Tenant ownership verification added — Lines 562-564 in workspaces.ts now call verifyTenantOwnership(c, workspace.tenantId) before executing cleanupWorkspaceInstallation(). This ensures the authenticated user's session tenant matches the workspace's tenant before allowing deletion.
if (!verifyTenantOwnership(c, workspace.tenantId)) {
return c.json({ error: 'Access denied' }, 403);
}Why this fix is correct:
- The
verifyTenantOwnershiphelper (lines 55-62) comparessessionTenantIdfrom the authenticated context against the workspace'stenantId - This same pattern is already used consistently throughout the file for other workspace operations (GET, PUT, etc.)
- The fix returns HTTP 403 (Access denied) rather than 404, which is semantically correct — the resource exists but the user lacks permission
🕐 Pending Recommendations (2)
These items from the prior review remain unaddressed:
- 🟡
dispatcher.ts:205-208Missing error stack trace inapp_uninstalledhandler (compare toapp_mentionhandler at line 148 which logserrorStack) - 🟡
dispatcher.ts:242-245Missing error stack trace intokens_revokedhandler (compare todirect_messagehandler at line 181 which logserrorStack)
💭 Consider (1)
💭 1) workspace-cleanup.ts:9-14 Barrel file import pattern
Issue: The file imports from ../services (barrel) which re-exports this file, creating a circular reference.
Why: While Node.js typically handles this, it's fragile and can cause initialization issues. This was noted in the prior pullfrog review.
Fix: Import directly from source modules:
import { clearWorkspaceConnectionCache, deleteWorkspaceInstallation, findWorkspaceConnectionByTeamId } from './nango';
import { revokeSlackToken } from './client';✅ APPROVE
Summary: The delta commit successfully addresses the critical cross-tenant deletion vulnerability identified by Ito testing. The fix follows the existing tenant verification pattern used throughout the codebase. The remaining items (missing error stack traces in dispatcher catch blocks) are minor consistency improvements that don't block approval. 🎉
Discarded (0)
No items discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 1 | 0 | 0 | 2 | 0 |
| Total | 3 | 0 | 1 | 0 | 0 | 2 | 0 |
Note: Delta-scoped review — sub-reviewers not dispatched for this small delta (4-line security fix). Issues from prior review carried forward as Pending Recommendations.
…oin permission, document automatic cleanup on uninstall from Slack
Ito Test Report ❌19 test cases ran. 6 failed, 13 passed. Across 19 Slack workspace lifecycle tests, 13 passed and 6 failed, showing strong coverage success for baseline install and uninstall happy paths, plain teamId and enterprise-style connectionId DELETE handling, signed lifecycle handling (including app_uninstalled and bot-token tokens_revoked cleanup, oauth-only tokens_revoked no uninstall), forged/unauthenticated rejection, malformed payload robustness, and safe post-cleanup follow-up API behavior. The most important issues were a high-severity false-success uninstall defect where the API returns success even when cleanup fails and the workspace persists (seen in rapid confirm, refresh/navigation, and mobile flows), a high-severity lifecycle replay/dedup cleanup gap in non-Vercel runtimes because background work is only registered when waitUntil exists, and a medium cross-tenant authorization flaw where an existing foreign-tenant workspace can return 404 before the 403 ownership guard runs. ❌ Failed (6)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Forged lifecycle webhook signatures were rejected with 401 and did not alter workspace state. |  |
| Adversarial | Unauthenticated DELETE to the Slack workspace endpoint returned 401 and left workspace data intact. |  |
| Adversarial | Malformed signed tokens_revoked payloads stayed non-5xx and did not delete control workspace data. |  |
| Edge | DELETE with colon-format connectionId returned 400 and invalid format error as expected. |  |
| Edge | DELETE for unknown team returned 404 and existing seeded workspace rows remained unaffected. |  |
| Edge | Signed oauth-only tokens_revoked event acknowledged with 200 and did not remove TEDGE4 workspace. |  |
| Logic | Install baseline behavior was valid after local Slack config and Work Apps enablement were corrected. |  |
| Logic | After cleanup, dependent Slack workspace endpoints failed safely and the dashboard showed no ghost workspace state. |  |
| Happy-path | Executed dashboard uninstall flow end-to-end on localhost after local non-production bypass setup; confirm action removed workspace and refresh showed no Slack connections. |  |
| Happy-path | Seeded a plain-team workspace fixture, deleted via teamId endpoint, received 200 success, and verified workspace absence via API list and dashboard refresh. |  |
| Happy-path | Seeded enterprise-style connectionId fixture, deleted with encoded enterprise ID, received 200 success, and confirmed workspace removal from list/dashboard. |  |
| Happy-path | Signed app_uninstalled event returned 200 and asynchronous cleanup removed the TROUTE4 workspace record. |  |
| Happy-path | Signed tokens_revoked event with bot token payload returned 200 and cleanup removed TROUTE5 as expected. |  |
Commit: 4acdbc3
Tell us how we did: Give Ito Feedback
1 participant
No description provided.