refactor(api-service): polish inbox channel endpoints#10781
Merged
djabarovgeorge merged 4 commits intonextfrom Apr 19, 2026
Merged
refactor(api-service): polish inbox channel endpoints#10781djabarovgeorge merged 4 commits intonextfrom
djabarovgeorge merged 4 commits intonextfrom
Conversation
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
Contributor
📝 WalkthroughWalkthroughThe PR simplifies inbox channel-connection and channel-endpoint APIs by introducing new DTO classes that expose only essential fields (identifier, type), removing creation endpoints from the controller, and updating response mapping logic while also removing most detailed response fields from client-side types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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. 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 |
@novu/js
@novu/nextjs
novu
@novu/react
@novu/react-native
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts (1)
221-229:⚠️ Potential issue | 🔴 CriticalCritical: Same hardcoded ngrok URL leaked into Slack OAuth redirect.
Mirrors the issue in
generate-msteams-oauth-url.usecase.ts:buildRedirectUri()returns a hardcoded personal ngrok origin while still requiring and computingbaseUrlfromAPI_ROOT_URLonly to throw it away. For Slack this is especially risky — the redirect URI must match the value registered on the Slack app, so every non-local environment (and every CI/e2e run) will fail OAuth, and once the ngrok tunnel is gone the subdomain could be claimed by someone else.Also note the stray
22in the commented-out fallback on line 228 — clearly a local editing artifact, which reinforces that this is debug code that shouldn't be merged.Please revert
buildRedirectUri()to the env-driven implementation and drop the commented-out fallback. As per coding guidelines forapps/api/**, this segment should be reviewed with focus on security and authentication.🔒 Proposed fix
static buildRedirectUri(): string { if (!process.env.API_ROOT_URL) { throw new Error('API_ROOT_URL environment variable is required'); } - const baseUrl = process.env.API_ROOT_URL.replace(/\/$/, ''); // Remove trailing slash - return `https://c9f4-79-177-157-205.ngrok-free.app${CHAT_OAUTH_CALLBACK_PATH}`; - // return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}`; 22 + const baseUrl = process.env.API_ROOT_URL.replace(/\/$/, ''); // Remove trailing slash + + return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts` around lines 221 - 229, The buildRedirectUri() function currently returns a hardcoded ngrok origin; change it to use the API_ROOT_URL env var instead: validate process.env.API_ROOT_URL, normalize it by removing a trailing slash into baseUrl, and return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}` (remove the hardcoded ngrok URL). Also delete the commented-out fallback line containing the stray "22" so no debug artifact remains. Ensure you edit the static method buildRedirectUri in the generate-slack-oauth-url.usecase class/function.apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-msteams-oath-url/generate-msteams-oauth-url.usecase.ts (1)
149-157:⚠️ Potential issue | 🔴 CriticalCritical: Remove hardcoded personal ngrok URL from OAuth redirect URI.
buildRedirectUri()now returns a hardcoded personal ngrok origin (https://c9f4-79-177-157-205.ngrok-free.app) for every environment, while still validatingAPI_ROOT_URLand computingbaseUrlonly to discard it. This looks like a local-development artifact that must not land onnext:
- In any non-local environment (staging/prod), the MS Teams OAuth redirect URI will no longer match the app's real callback, so admin consent will fail or, worse, redirect admins to a tunnel you don't control once the ngrok session expires and the subdomain is reused.
- It's a security-relevant change in an auth/OAuth flow and is outside the stated scope of this PR ("polish inbox channel endpoints"), so it almost certainly shouldn't be merged together with the inbox refactor.
baseUrlis now dead code, and the previous behavior is only preserved as a comment, which will silently rot.Please restore the env-driven behavior (and drop the dead local variable + commented fallback) before merging. As per coding guidelines for
apps/api/**, this segment should be reviewed with focus on security and authentication.🔒 Proposed fix
static buildRedirectUri(): string { if (!process.env.API_ROOT_URL) { throw new Error('API_ROOT_URL environment variable is required'); } - const baseUrl = process.env.API_ROOT_URL.replace(/\/$/, ''); // Remove trailing slash - return `https://c9f4-79-177-157-205.ngrok-free.app${CHAT_OAUTH_CALLBACK_PATH}`; - // return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}`; + const baseUrl = process.env.API_ROOT_URL.replace(/\/$/, ''); // Remove trailing slash + + return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-msteams-oath-url/generate-msteams-oauth-url.usecase.ts` around lines 149 - 157, The buildRedirectUri function currently returns a hardcoded ngrok URL; restore environment-driven behavior by removing the hardcoded origin and returning the computed baseUrl combined with CHAT_OAUTH_CALLBACK_PATH (i.e., compute baseUrl from process.env.API_ROOT_URL after trimming trailing slash and return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}`), remove the dead/commented fallback and ensure the existing API_ROOT_URL presence check/throw remains intact; reference: buildRedirectUri and CHAT_OAUTH_CALLBACK_PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/inbox/inbox.controller.ts`:
- Around line 692-696: The API no longer returns totalCount and totalCountCapped
but the internal SDK Zod schemas still require them; restore compatibility by
including those fields in the controller responses for
listChannelConnections/listChannelEndpoints. In the inbox controller (where
result is transformed with mapChannelConnectionToInboxDto) add totalCount:
result.totalCount ?? null and totalCountCapped: result.totalCountCapped ?? null
(or appropriate numeric defaults) to the returned object so the SDK validation
succeeds, or alternatively update the SDK Zod schemas in libs/internal-sdk to
make totalCount and totalCountCapped optional — choose one approach and apply it
consistently for both listChannelConnections and listChannelEndpoints.
---
Outside diff comments:
In
`@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-msteams-oath-url/generate-msteams-oauth-url.usecase.ts`:
- Around line 149-157: The buildRedirectUri function currently returns a
hardcoded ngrok URL; restore environment-driven behavior by removing the
hardcoded origin and returning the computed baseUrl combined with
CHAT_OAUTH_CALLBACK_PATH (i.e., compute baseUrl from process.env.API_ROOT_URL
after trimming trailing slash and return
`${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}`), remove the dead/commented fallback and
ensure the existing API_ROOT_URL presence check/throw remains intact; reference:
buildRedirectUri and CHAT_OAUTH_CALLBACK_PATH.
In
`@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts`:
- Around line 221-229: The buildRedirectUri() function currently returns a
hardcoded ngrok origin; change it to use the API_ROOT_URL env var instead:
validate process.env.API_ROOT_URL, normalize it by removing a trailing slash
into baseUrl, and return `${baseUrl}${CHAT_OAUTH_CALLBACK_PATH}` (remove the
hardcoded ngrok URL). Also delete the commented-out fallback line containing the
stray "22" so no debug artifact remains. Ensure you edit the static method
buildRedirectUri in the generate-slack-oauth-url.usecase class/function.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8028d06e-9fab-4018-8a7d-e099798fe944
📒 Files selected for processing (7)
apps/api/src/app/inbox/dtos/inbox-channel-connection-response.dto.tsapps/api/src/app/inbox/dtos/inbox-channel-endpoint-response.dto.tsapps/api/src/app/inbox/dtos/inbox-dto.mapper.tsapps/api/src/app/inbox/inbox.controller.tsapps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-msteams-oath-url/generate-msteams-oauth-url.usecase.tsapps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.tspackages/js/src/channel-connections/types.ts
💤 Files with no reviewable changes (1)
- packages/js/src/channel-connections/types.ts
… of hardcoded ngrok URL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts`:
- Around line 65-72: The state encoder/decoder currently splits on '.' which
breaks when subscriberId, context, or connectionIdentifier contain dots; change
the parsing to locate the last '.' (use lastIndexOf('.') on the state string)
and split into payload = state.slice(0, lastIndex) and signature =
state.slice(lastIndex + 1), then JSON.parse the payload and verify the signature
as before. Update the parsing logic wherever split('.') is used (e.g., in the
Slack and MSTEAMS callback handlers and any state parsing functions referenced
by createSecureState and generateSlackOauthUrl/generateMsteamsOauthUrl usecases)
so they use lastIndexOf('.') and handle missing/invalid separators robustly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59be5d00-8ce8-4365-b632-6d36fa9314ad
📒 Files selected for processing (1)
apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts
Comment on lines
65
to
72
| const secureState = await this.createSecureState( | ||
| command.integration, | ||
| subscriberId, | ||
| command.subscriberId, | ||
| command.context, | ||
| command.connectionIdentifier, | ||
| command.mode, | ||
| command.connectionMode | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect subscriberId constraints and OAuth state delimiter parsing.
rg -n -C4 --type=ts '\bsubscriberId\b|decoded\.split\('\''\.'\''\)'Repository: novuhq/novu
Length of output: 50369
🏁 Script executed:
#!/bin/bash
# Get the exact file and lines mentioned in the review
file_path="apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts"
# Check file exists and show relevant lines
if [ -f "$file_path" ]; then
echo "=== File context around lines 65-72 ==="
sed -n '60,75p' "$file_path"
echo ""
echo "=== File context around lines 197-202 ==="
sed -n '190,210p' "$file_path"
else
echo "File not found: $file_path"
# Try to find the file with a flexible search
find . -name "*generate-slack-oauth-url.usecase.ts" -type f
fiRepository: novuhq/novu
Length of output: 1492
🏁 Script executed:
#!/bin/bash
# Search for the split('.') pattern in the usecase file and related state handling
rg -n "split\('\.'\)|createSecureState|validateAndDecodeState" apps/api/src/app/integrations --type=ts -A 5 -B 2 | head -150Repository: novuhq/novu
Length of output: 14838
🏁 Script executed:
#!/bin/bash
# Check if there are any validations on subscriberId format in the codebase
rg -n "subscriberId.*pattern|subscriberId.*validate|subscriberId.*format" apps/api/src --type=ts | head -50Repository: novuhq/novu
Length of output: 172
🏁 Script executed:
#!/bin/bash
sed -n '159,195p' apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.tsRepository: novuhq/novu
Length of output: 1234
🏁 Script executed:
#!/bin/bash
# Check if there's any validation logic for subscriberId format
rg -n "subscriberId" apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts -B 2 -A 2 | head -80Repository: novuhq/novu
Length of output: 1727
Fix OAuth state parsing to handle subscriber IDs and context with dot delimiters.
Line 200 uses split('.') which fails if subscriberId, context, or connectionIdentifier contain dots (e.g., email-like subscriber IDs, domain names). The payload is JSON that may contain dots; splitting at the first dot instead of the last one breaks signature verification for otherwise valid states.
This issue also affects:
apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-msteams-oath-url/generate-msteams-oauth-url.usecase.ts:128- Callback handlers in slack-oauth-callback and msteams-oauth-callback
🔒 Proposed fix (use last dot as separator)
- const [payload, signature] = decoded.split('.');
+ const separatorIndex = decoded.lastIndexOf('.');
+ if (separatorIndex === -1) {
+ throw new Error('Invalid state format');
+ }
+
+ const payload = decoded.slice(0, separatorIndex);
+ const signature = decoded.slice(separatorIndex + 1);
+
+ if (!payload || !signature) {
+ throw new Error('Invalid state format');
+ }
const expectedSignature = createHash(environmentApiKey, payload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts`
around lines 65 - 72, The state encoder/decoder currently splits on '.' which
breaks when subscriberId, context, or connectionIdentifier contain dots; change
the parsing to locate the last '.' (use lastIndexOf('.') on the state string)
and split into payload = state.slice(0, lastIndex) and signature =
state.slice(lastIndex + 1), then JSON.parse the payload and verify the signature
as before. Update the parsing logic wherever split('.') is used (e.g., in the
Slack and MSTEAMS callback handlers and any state parsing functions referenced
by createSecureState and generateSlackOauthUrl/generateMsteamsOauthUrl usecases)
so they use lastIndexOf('.') and handle missing/invalid separators robustly.
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.
What changed? Why was the change needed?
What changed
This PR refactors the inbox channel endpoints to improve separation of concerns by removing channel connection and endpoint creation from the inbox controller and simplifying response DTOs. Creation of these resources is now handled exclusively through OAuth callback flows in the integrations and subscribers modules, while the inbox controller only handles read operations (list and get).
Affected areas
@novu/api-service: The inbox controller now only exposes list and get endpoints for channel connections and endpoints, removing the
POST /channel-connectionsandPOST /channel-endpointshandlers. New simplified DTOs (InboxChannelConnectionResponseDto,InboxChannelEndpointResponseDto) were introduced that reduce response payloads to essential fields:identifierfor connections andidentifier/typefor endpoints. Pagination shapes were normalized by droppingtotalCountandtotalCountCapped. A minor fix to the Slack OAuth URL generation usecase ensuressubscriberIdis always included in the secure state regardless of connection mode.@novu/js: Response types for channel connections and endpoints were simplified to match the new API shape, removing redundant fields like
integrationIdentifier,providerId,channel, timestamps, and other metadata.Key technical decisions
ChatOauthCallback,SlackOauthCallback) rather than direct inbox endpoints, providing a more appropriate flow for these operations.identifier(andtypefor endpoints), reducing API surface and improving contract stability.next/previous) are normalized to nullable strings with no cardinality counters.Testing
No tests are mentioned in the PR. Given that this is a refactoring that moves creation logic to existing callback flows (which already handle these operations), existing tests for those callback usecases should provide coverage. The inbox controller changes primarily involve removing endpoints and updating response mappers, which may require updates to existing inbox endpoint tests.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer