← Back to PRs

#21967: Harden Slack allow-from resolution against undefined catch crash

by graysurf open 2026-02-20 15:46 View on GitHub →
channel: slack size: XS
# Harden Slack allow-from resolution against undefined catch crash Closes #19799 ## Summary This patch hardens Slack allowlist resolution so malformed or non-promise pairing-store responses cannot crash `prepareSlackMessage`. It replaces chained `.catch()` usage with defensive `try/catch` + array validation and adds regression tests that cover throw and malformed return paths. ## Problem - Expected: `resolveSlackEffectiveAllowFrom` should gracefully fall back to empty store entries when pairing-store reads fail or return invalid data. - Actual: The implementation called `.catch()` directly on the read result; if the mocked/runtime value is `undefined`, it throws `TypeError: Cannot read properties of undefined (reading 'catch')`. - Impact: macOS CI Slack prepare tests fail, producing unrelated red CI failures across PRs. ## Reproduction 1. Call `prepareSlackMessage` in a test/runtime path where `readChannelAllowFromStore("slack")` returns `undefined` or a non-promise value. 2. Reach `resolveSlackEffectiveAllowFrom` during message preparation. - Expected result: No crash; fall back to channel-config allowlist. - Actual result: Throws on `.catch` against `undefined`. ## Issues Found Severity: high Confidence: high Status: fixed | ID | Severity | Confidence | Area | Summary | Evidence | Status | | --- | --- | --- | --- | --- | --- | --- | | PR-21967-BUG-01 | high | high | `src/slack/monitor/auth.ts` | Slack allow-from resolution assumed promise-like return and crashed on malformed value | `src/slack/monitor/auth.ts:6` | fixed | ## Fix Approach - Replace chained `.catch()` with `try/catch` around awaited store read. - Validate store result with `Array.isArray`; treat invalid values as empty. - Add focused regression tests for throw + malformed return scenarios. ## Testing - `pnpm test src/slack/monitor/auth.test.ts src/slack/monitor/message-handler/prepare.test.ts` (pass) - `pnpm lint` (pass) - `pnpm tsgo` (pass) ## Risk / Notes - Low risk: confined to Slack allowlist resolution and covered by targeted tests. <!-- greptile_comment --> <h3>Greptile Summary</h3> Replaces fragile promise chaining with defensive `try/catch` block in Slack allowlist resolution. Previously, when `readChannelAllowFromStore("slack")` returned `undefined` or other non-promise values, calling `.catch()` directly would throw `TypeError: Cannot read properties of undefined (reading 'catch')`. The fix wraps the store read in `try/catch`, validates the result with `Array.isArray`, and safely defaults to empty array on error or malformed data. Changes: - Replaced `await readChannelAllowFromStore("slack").catch(() => [])` with explicit `try/catch` + array validation in `src/slack/monitor/auth.ts:5-12` - Added regression test for promise rejection scenario in `src/slack/monitor/auth.test.ts:23-30` - Added regression test for `undefined` return value scenario in `src/slack/monitor/auth.test.ts:32-39` The fix properly addresses the root cause: the code assumed `readChannelAllowFromStore` always returns a promise, but mocks or edge cases can return non-promise values, causing the `.catch()` call to fail. <h3>Confidence Score: 5/5</h3> - This PR is safe to merge with minimal risk - The fix is narrowly scoped to error handling in a single function, adds proper defensive validation, and includes comprehensive regression tests. The change improves robustness without altering behavior in success cases. - No files require special attention <sub>Last reviewed commit: f79fc82</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs