Conversation
The "Connect to Slack" button was shown even when Slack env vars (SLACK_CLIENT_ID) were not configured, leading to a raw JSON error page. - Add `available` field to /api/integrations/slack/status response that checks both table existence and SLACK_CLIENT_ID presence - Hide the connect button in the UI when Slack is unavailable, showing an informational message instead - Fix OAuth redirect URI to use the request Host header instead of REPLIT_DOMAINS, ensuring correct behavior on custom domains https://claude.ai/code/session_01MUfXseCaHHx1oj1WWBpKme
- Verify redirect_uri uses request host instead of REPLIT_DOMAINS - Verify power tier users can initiate Slack OAuth flow https://claude.ai/code/session_01MUfXseCaHHx1oj1WWBpKme
- Add getValidatedAppUrl() that checks the Host header against the REPLIT_DOMAINS allowlist, preventing host header injection from redirecting OAuth callbacks to attacker-controlled domains - Replace specific "not configured on this server" messages with generic "not available" to avoid leaking server configuration details - Add test for rejected host headers
…ards 1. Extract inline DDL from registerRoutes() into server/services/ensureTables.ts so route registration doesn't own migration logic (single source of truth). 2. Extract getValidatedAppUrl into server/utils/hostValidation.ts with unit tests, making it independently testable and reusable for future OAuth flows. 3. Merge PowerProtectedRoute into ProtectedRoute with an optional requiredTier prop, eliminating duplicated auth/loading logic. https://claude.ai/code/session_01MUfXseCaHHx1oj1WWBpKme
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Slack availability signaling, host validation for Slack OAuth to prevent SSRF, and refactors DB bootstrap into three ensure* helpers. Client-side route protection gains a LoadingSpinner and optional tier-based Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant DB
participant Slack
Browser->>Server: GET /slack/install (Host header)
Server->>Server: validateHost(Host)
alt host valid
Server->>DB: ensureChannelTables(), ensureApiKeysTable()
Server->>Slack: Redirect to Slack OAuth (built with validated host redirect_uri)
Slack-->>Browser: Redirect to Server callback with code
Browser->>Server: GET /slack/callback?code=...
Server->>Server: validateHost(Host)
Server->>Slack: Exchange code for token
Server->>DB: store slack_connections
Server-->>Browser: Redirect success
else host invalid
Server-->>Browser: 400 / redirect invalid_host
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/components/NotificationChannelsPanel.tsx (1)
43-45:⚠️ Potential issue | 🟠 MajorDon't collapse an unresolved Slack status into “not available.”
slackStatusisundefinedon the initial render, so this branch shows “Slack integration is not available” while the status request is still pending. Gate on the query loading state first, or only enter this branch whenavailable === false.Suggested fix
- const { data: slackStatus } = useSlackStatus(); + const { data: slackStatus, isLoading: isSlackStatusLoading } = useSlackStatus(); ... - ) : !slackStatus?.available ? ( + ) : isSlackStatusLoading ? null : slackStatus?.available === false ? ( <p className="text-sm text-muted-foreground"> Slack integration is not available. </p>Also applies to: 213-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/NotificationChannelsPanel.tsx` around lines 43 - 45, The component is treating an undefined slackStatus as "not available" on initial render; update NotificationChannelsPanel to respect the useSlackStatus loading state (from useSlackStatus) or explicitly check slackStatus.available === false before rendering "Slack integration is not available." Locate the slackStatus usage in NotificationChannelsPanel (where useSlackStatus is called) and change the conditional to first gate on the query's loading/idle state (or existence of slackStatus) and only display the unavailable message when slackStatus.available === false, applying the same change to the later branch around lines 213-217 that also reads slackStatus.shared/routes.ts (1)
248-275:⚠️ Potential issue | 🟠 Major
slack.installis still missing the new 400/503 response contracts.The route tests now cover
400 BAD_REQUESTfor invalid hosts and503 NOT_CONFIGUREDwhen channel tables are unavailable, but this shared definition still only advertises302/401/403/501. That leaves client-side typing and any contract validation out of sync with the server.Suggested contract update
install: { method: 'GET' as const, path: '/api/integrations/slack/install', responses: { + 400: z.object({ message: z.string(), code: z.string() }), + 503: z.object({ message: z.string(), code: z.string().optional() }), }, },As per coding guidelines,
shared/**: “Changes here affect both server and client. Review for: Type safety.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/routes.ts` around lines 248 - 275, Update the integrations.slack.install route's responses in shared/routes.ts to include the two new contracts the server tests expect: add a 400 response (use errorSchemas.badRequest) for invalid-host requests and add a 503 response (e.g., z.object({ message: z.string() }) or the existing not-configured error schema if present) for NOT_CONFIGURED/channel-table-unavailable errors so the client/server contract matches the route tests; modify the responses map under integrations.slack.install accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/App.tsx`:
- Around line 49-65: ProtectedRoute currently renders <LandingPage /> which does
not change the browser location; instead perform a client-side redirect so the
URL updates. In the ProtectedRoute function, replace the render of <LandingPage
/> for unauthenticated or unauthorized cases with a react-router redirect (e.g.,
return <Navigate to="/" replace />) or use useNavigate() to programmatically
navigate; update both the unauthenticated branch and the requiredTier mismatch
branch (also apply the same change where the same pattern appears around line
with reference 82) so users are actually sent off /developer rather than merely
shown the landing UI.
In `@server/utils/hostValidation.ts`:
- Around line 5-11: The validateHost function currently accepts any host when
process.env.REPLIT_DOMAINS is unset or empty; change it to "fail closed" by
returning null if REPLIT_DOMAINS is missing or yields an empty allowed array. In
validateHost, read process.env.REPLIT_DOMAINS into allowed as you already do,
but if allowed.length === 0 return null immediately; otherwise continue to
trim/normalize the incoming host and only return host when it exactly matches an
entry in allowed (use the existing allowed.includes(host) check after
normalization). This ensures validateHost (and code that builds redirect_uri)
never trusts an unset REPLIT_DOMAINS.
---
Outside diff comments:
In `@client/src/components/NotificationChannelsPanel.tsx`:
- Around line 43-45: The component is treating an undefined slackStatus as "not
available" on initial render; update NotificationChannelsPanel to respect the
useSlackStatus loading state (from useSlackStatus) or explicitly check
slackStatus.available === false before rendering "Slack integration is not
available." Locate the slackStatus usage in NotificationChannelsPanel (where
useSlackStatus is called) and change the conditional to first gate on the
query's loading/idle state (or existence of slackStatus) and only display the
unavailable message when slackStatus.available === false, applying the same
change to the later branch around lines 213-217 that also reads slackStatus.
In `@shared/routes.ts`:
- Around line 248-275: Update the integrations.slack.install route's responses
in shared/routes.ts to include the two new contracts the server tests expect:
add a 400 response (use errorSchemas.badRequest) for invalid-host requests and
add a 503 response (e.g., z.object({ message: z.string() }) or the existing
not-configured error schema if present) for
NOT_CONFIGURED/channel-table-unavailable errors so the client/server contract
matches the route tests; modify the responses map under
integrations.slack.install accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b286703e-b782-4868-901e-1c55ae650b28
📒 Files selected for processing (9)
client/src/App.tsxclient/src/components/NotificationChannelsPanel.tsxclient/src/hooks/use-slack.tsserver/routes.notificationChannels.test.tsserver/routes.tsserver/services/ensureTables.tsserver/utils/hostValidation.test.tsserver/utils/hostValidation.tsshared/routes.ts
Comment on lines
+49
to
+65
| function ProtectedRoute({ component: Component, requiredTier, ...rest }: any) { | ||
| const { user, isLoading } = useAuth(); | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <div className="min-h-screen flex items-center justify-center bg-background" role="status"> | ||
| <Loader2 className="h-8 w-8 animate-spin text-primary" aria-hidden="true" /> | ||
| <span className="sr-only">Loading…</span> | ||
| </div> | ||
| ); | ||
| return <LoadingSpinner />; | ||
| } | ||
|
|
||
| if (!user) { | ||
| return <LandingPage />; | ||
| } | ||
|
|
||
| if (user.tier !== "power") { | ||
| if (requiredTier && user.tier !== requiredTier) { | ||
| return <LandingPage />; | ||
| } | ||
|
|
||
| return ( | ||
| <Suspense fallback={ | ||
| <div className="min-h-screen flex items-center justify-center bg-background" role="status"> | ||
| <Loader2 className="h-8 w-8 animate-spin text-primary" aria-hidden="true" /> | ||
| <span className="sr-only">Loading…</span> | ||
| </div> | ||
| }> | ||
| <Component {...rest} /> | ||
| </Suspense> | ||
| ); | ||
| const content = <Component {...rest} />; | ||
| return requiredTier ? <Suspense fallback={<LoadingSpinner />}>{content}</Suspense> : content; |
There was a problem hiding this comment.
This guard does not actually redirect away from /developer.
Rendering <LandingPage /> keeps the browser on the protected URL. Non-Power users are denied the page, but they still stay on /developer, which breaks the “redirect away” behavior in the PR goal and leaves refresh/back behavior inconsistent.
Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/App.tsx` around lines 49 - 65, ProtectedRoute currently renders
<LandingPage /> which does not change the browser location; instead perform a
client-side redirect so the URL updates. In the ProtectedRoute function, replace
the render of <LandingPage /> for unauthenticated or unauthorized cases with a
react-router redirect (e.g., return <Navigate to="/" replace />) or use
useNavigate() to programmatically navigate; update both the unauthenticated
branch and the requiredTier mismatch branch (also apply the same change where
the same pattern appears around line with reference 82) so users are actually
sent off /developer rather than merely shown the landing UI.
… flash, route contracts - validateHost now rejects non-localhost hosts when REPLIT_DOMAINS is unset, preventing host-header injection if the env var is accidentally missing - NotificationChannelsPanel gates on query loading state before showing "Slack integration is not available" to avoid flash on initial render - Add 400 and 503 response schemas to slack.install route contract in shared/routes.ts for type-safe client/server alignment https://claude.ai/code/session_01MUfXseCaHHx1oj1WWBpKme
bd73-com
pushed a commit
that referenced
this pull request
Mar 6, 2026
Instead of showing a scary "Slack integration is not available" error to end users (who cannot fix server configuration), hide the entire Slack section when the backend reports available=false. This stops the recurring cycle of "improving" the error message (#82, #83, #85). https://claude.ai/code/session_017WdTqmNUwoVYvsgEDoLrHK
bd73-com
added a commit
that referenced
this pull request
Mar 7, 2026
…ements (#88) * fix: hide Slack section when server credentials are not configured Instead of showing a scary "Slack integration is not available" error to end users (who cannot fix server configuration), hide the entire Slack section when the backend reports available=false. This stops the recurring cycle of "improving" the error message (#82, #83, #85). https://claude.ai/code/session_017WdTqmNUwoVYvsgEDoLrHK * test: add tests for ensureTables, date-format, and Slack error path - ensureTables.test.ts: 8 tests covering all 3 startup migration functions (happy path, error handling, console logging) - date-format.test.ts: 7 tests covering formatDate, formatTime, formatDateTime with edge cases (midnight, single-digit padding) - notificationChannels: add test verifying unhandled getSlackConnection error propagates (no try-catch in status endpoint) https://claude.ai/code/session_017WdTqmNUwoVYvsgEDoLrHK * fix: harden Slack status endpoint (info leak + unhandled error) - Collapse unavailableReason from "setup_incomplete"/"not_configured" to a single generic "unavailable" to avoid leaking infrastructure state to authenticated users - Wrap getSlackConnection() in try-catch so a DB failure returns 500 instead of hanging the request as an unhandled async rejection https://claude.ai/code/session_017WdTqmNUwoVYvsgEDoLrHK * fix: redirect to dashboard on tier mismatch + add schema sync test - ProtectedRoute now redirects to /dashboard instead of showing the landing page when an authenticated user doesn't have the required tier. Previously indistinguishable from being logged out. - Add schema sync assertions in ensureTables.test.ts that compare the DDL column names in ensureChannelTables() against the Drizzle schema definitions. If either side drifts, the test fails — preventing the root cause of the recurring Slack integration bug. https://claude.ai/code/session_017WdTqmNUwoVYvsgEDoLrHK --------- Co-authored-by: Claude <noreply@anthropic.com>
7 tasks
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
2 participants
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
This PR fixes several issues with the Slack integration: the "Connect Slack" button appeared even when Slack wasn't configured (causing 501 errors), the OAuth flow was vulnerable to host header injection, and inline DDL migrations cluttered route registration. It also adds stealth evasion for the scraper, restricts the Developer page to Power-tier users, and removes stale nav links.
Changes
Slack integration fixes
/api/integrations/slack/statusendpoint returning{ connected, available }so the UI can hide the connect button when Slack isn't configured (SLACK_CLIENT_IDunset or channel tables missing)Hostheader againstREPLIT_DOMAINSallowlist before using it in OAuth redirect URIs, preventing host header injection attacksArchitecture refactors
CREATE TABLE/ALTER TABLEDDL fromregisterRoutes()intoserver/services/ensureTables.tswith three focused functions (ensureErrorLogColumns,ensureApiKeysTable,ensureChannelTables)server/utils/hostValidation.tsas a purevalidateHost()function with dedicated unit testsPowerProtectedRouteintoProtectedRoutewith an optionalrequiredTierprop, sharing a commonLoadingSpinnercomponentScraper improvements
User-Agent,Sec-CH-UA, etc.) to static fetches to reduce bot detectionextraHTTPHeaders) and an init script that patchesnavigator.webdriver,navigator.plugins, andnavigator.languagesAccess control & nav
/developerroute to Power-tier users viarequiredTier="power"guardDashboardNavPublicNavnotification_channels,delivery_log,slack_connections) on startup soschema:pushisn't requiredTests
validateHost(undefined, empty, allowed, rejected, whitespace trimming)/api/integrations/slack/installavailablefieldHow to test
SLACK_CLIENT_IDset. Navigate to a monitor's notification channels panel — the Slack section should show "Slack integration is not available." instead of a connect button.REPLIT_DOMAINS=your-domain.com, send a request to/api/integrations/slack/installwith a spoofedHost: evil.comheader — should return 400./developer— should redirect to landing page.npm run test— all 1036 tests should pass.https://claude.ai/code/session_01MUfXseCaHHx1oj1WWBpKme
Summary by CodeRabbit
New Features
Improvements
Tests