Conversation
…ery fixes NV-7457
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds attachment preparation and delivery: enforces per-file and aggregate size/count limits, decodes/normalizes inline binary data, securely fetches URL attachments with SSRF/DNS/redirect/timeouts and size checks, drops attachments on unsupported platforms, and introduces Slack credentials paste UX and related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatSdkService
participant ValidatorFetcher as Validator/Fetcher
participant SSRF as SSRF/URL Validator
participant ChatAdapter as Chat SDK Adapter
Client->>ChatSdkService: send ReplyContentDto (may include files)
ChatSdkService->>ChatSdkService: prepareContentForDelivery(content, platform, agentId)
alt Inline file (data)
ChatSdkService->>ValidatorFetcher: validate & decode inline data (string/Uint8Array/ArrayBuffer/Blob)
ValidatorFetcher-->>ChatSdkService: Buffer (≤5 MB) or error
else Remote file (url)
ChatSdkService->>SSRF: validateUrlSsrf(url) + DNS/private-IP check
SSRF-->>ChatSdkService: safe/blocked
alt safe
ChatSdkService->>ValidatorFetcher: fetch URL (timeout, redirect limit)
ValidatorFetcher->>ValidatorFetcher: check content-length / stream limit (per-file cap)
ValidatorFetcher-->>ChatSdkService: Buffer + mimeType or error
end
end
ChatSdkService->>ChatSdkService: enforce max files & aggregate size limits
ChatSdkService->>ChatSdkService: drop attachments for Email/WhatsApp (log warning)
ChatSdkService->>ChatAdapter: post message with prepared files[]
ChatAdapter-->>ChatSdkService: response
ChatSdkService-->>Client: return result
sequenceDiagram
participant User
participant SlackPaste as SlackCredentialsPaste
participant Parser
participant Form as react-hook-form
User->>SlackPaste: paste credentials block
SlackPaste->>Parser: parseSlackCredentialsBlock(text)
Parser-->>SlackPaste: {values, matched, invalid, unknownLines}
alt matched >= 1
SlackPaste->>Form: setValue for matched fields (mark dirty/touched, trigger validation)
Form-->>SlackPaste: fields updated
SlackPaste->>User: show summary of filled/replaced/invalid fields
else
SlackPaste->>User: show parse results (no useful fields)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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 |
Co-authored-by: Dima Grossman <dima@grossman.io>
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/framework/src/resources/agent/agent.context.ts (1)
186-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject attachments on card replies instead of discarding them.
validateFiles()runs here, but the JSX/Card paths never usevalidFiles, soctx.reply(Card(...), { files })silently loses the attachments. This should fail fast with the same “files only allowed with markdown” contract the API already enforces.Suggested change
async function serializeContent(content: MessageContent, files?: FileRef[]): Promise<ReplyContent> { const validFiles = await validateFiles(files); + + if (validFiles && typeof content !== 'string') { + throw new Error('Files can only be sent with markdown content.'); + } if (typeof content === 'string') { return validFiles ? { markdown: content, files: validFiles } : { markdown: content }; }As per coding guidelines
packages/framework/**/*.{ts,tsx,js,jsx}:packages/frameworkdefines the code-first workflow SDK and serves as the interface between user-defined workflows and Novu's engine; write clear, minimal abstractions as changes affect the developer-facing API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.context.ts` around lines 186 - 205, serializeContent currently validates files but discards them for JSX/Card replies; update serializeContent to reject attachments when content is not a string by checking the result of validateFiles(files) and throwing an error (same contract/message used elsewhere, e.g., "files are only allowed with markdown replies") if validFiles exist for JSX or CardElement branches; modify the JSX path (isJSX -> toCardElement) and the isCardElement branch in serializeContent to throw when files were provided so attachments fail fast instead of being silently dropped.
🧹 Nitpick comments (2)
apps/dashboard/src/components/integrations/components/slack-credentials-paste.tsx (1)
200-202: ⚡ Quick winRemove nested ternary in summary copy
This expression uses a nested ternary and is hard to scan. Extract the suffix/segment to a variable before JSX.
💡 Suggested refactor
+ const overwrittenSuffix = outcome.overwritten.length === 1 ? '' : 's'; + const overwrittenText = + outcome.overwritten.length > 0 + ? ` · replaced ${outcome.overwritten.length} existing value${overwrittenSuffix}` + : ''; @@ - {outcome.overwritten.length > 0 - ? ` · replaced ${outcome.overwritten.length} existing value${outcome.overwritten.length === 1 ? '' : 's'}` - : ''} + {overwrittenText}As per coding guidelines: "Do not use nested ternaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/integrations/components/slack-credentials-paste.tsx` around lines 200 - 202, Extract the nested ternary into a precomputed variable in the component body: compute a pluralSuffix variable (e.g., const pluralSuffix = outcome.overwritten.length === 1 ? '' : 's') and then compute a single replacedText string (e.g., const replacedText = outcome.overwritten.length > 0 ? ` · replaced ${outcome.overwritten.length} existing value${pluralSuffix}` : '') and use {replacedText} in the JSX instead of the nested ternary; place these consts in the Slack credentials paste component scope (near where outcome is available) so the JSX line becomes simply {replacedText}.apps/api/src/app/agents/services/chat-sdk.service.ts (1)
52-60: 🏗️ Heavy liftConsolidate the attachment limits in shared constants.
These caps now live in framework serialization, DTO validation, API delivery, and bootstrap. The new
/v1/agentsparser limit has already drifted close enough to create a boundary bug, so keeping one source of truth would make the SDK and API fail consistently.As per coding guidelines
packages/@(shared|framework|js|react)/**: Use shared packages:packages/shared,packages/framework,packages/js,packages/reactfor shared code and utilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 52 - 60, The attachment limit constants (BASE64_REGEX, MAX_INLINE_FILE_BYTES, MAX_INLINE_AGGREGATE_FILE_BYTES, MAX_FILE_BYTES, MAX_FILES_PER_MESSAGE, MAX_AGGREGATE_FILE_BYTES, MAX_INLINE_FILE_BASE64_CHARS, FILE_FETCH_TIMEOUT_MS, MAX_FILE_FETCH_REDIRECTS) are duplicated across layers; extract these constants into a single shared module (e.g., packages/shared or packages/framework) and replace the local definitions in chat-sdk.service.ts with imports from that shared constants module so all validators/parsers/SDKs use the same source of truth; update any references to the renamed import identifiers if needed and run tests to ensure parity.
🤖 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/agents/services/chat-sdk.service.ts`:
- Around line 223-231: Remove logging of raw attachment filenames in the
UNSUPPORTED_FILE_PLATFORMS branch: update the logger.warn call in
chat-sdk.service.ts (the block checking
UNSUPPORTED_FILE_PLATFORMS.has(platform)) to only include agentId, platform, and
droppedCount (e.g., content.files.length), and remove any reference to
content.files.map(...) or filenames so user-controlled filenames are not
emitted; keep the existing message string unchanged.
In `@apps/api/src/bootstrap.ts`:
- Line 117: The body-parser limit for the /v1/agents route is too small
(currently '7mb') and causes premature 413 errors before the attachment
validator (agentRawBodyBuffer) runs; update the bodyParser.json call for the
'/v1/agents' route to raise the limit to a safer value (e.g., '8mb' or '10mb')
or replace the magic string with a named constant, ensuring the new limit
accounts for base64 expansion and JSON envelope so the attachment validator can
return attachment_failed as intended.
In
`@apps/dashboard/src/components/integrations/components/parse-slack-credentials-block.ts`:
- Around line 98-103: The loop advances the index by a fixed +1 after calling
consumeNextValue, but consumeNextValue can read several lines ahead, so the
consumed lines get reprocessed as unknownLines; update consumeNextValue (or add
a helper) to return both the parsed value and the index it consumed (e.g., {
value, nextIndex } or similar) and then set index = nextIndex (instead of index
+= 1) after assigning matchedField/value in the block that uses matchesLabel,
and apply the same change to the other identical block that handles matchedField
(the second occurrence referenced in the review).
---
Outside diff comments:
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Around line 186-205: serializeContent currently validates files but discards
them for JSX/Card replies; update serializeContent to reject attachments when
content is not a string by checking the result of validateFiles(files) and
throwing an error (same contract/message used elsewhere, e.g., "files are only
allowed with markdown replies") if validFiles exist for JSX or CardElement
branches; modify the JSX path (isJSX -> toCardElement) and the isCardElement
branch in serializeContent to throw when files were provided so attachments fail
fast instead of being silently dropped.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 52-60: The attachment limit constants (BASE64_REGEX,
MAX_INLINE_FILE_BYTES, MAX_INLINE_AGGREGATE_FILE_BYTES, MAX_FILE_BYTES,
MAX_FILES_PER_MESSAGE, MAX_AGGREGATE_FILE_BYTES, MAX_INLINE_FILE_BASE64_CHARS,
FILE_FETCH_TIMEOUT_MS, MAX_FILE_FETCH_REDIRECTS) are duplicated across layers;
extract these constants into a single shared module (e.g., packages/shared or
packages/framework) and replace the local definitions in chat-sdk.service.ts
with imports from that shared constants module so all validators/parsers/SDKs
use the same source of truth; update any references to the renamed import
identifiers if needed and run tests to ensure parity.
In
`@apps/dashboard/src/components/integrations/components/slack-credentials-paste.tsx`:
- Around line 200-202: Extract the nested ternary into a precomputed variable in
the component body: compute a pluralSuffix variable (e.g., const pluralSuffix =
outcome.overwritten.length === 1 ? '' : 's') and then compute a single
replacedText string (e.g., const replacedText = outcome.overwritten.length > 0 ?
` · replaced ${outcome.overwritten.length} existing value${pluralSuffix}` : '')
and use {replacedText} in the JSX instead of the nested ternary; place these
consts in the Slack credentials paste component scope (near where outcome is
available) so the JSX line becomes simply {replacedText}.
🪄 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: d903a308-c017-467e-9175-d3bac347cee0
📒 Files selected for processing (14)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/services/chat-sdk.service.spec.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/bootstrap.tsapps/dashboard/src/components/agents/slack-setup-guide.tsxapps/dashboard/src/components/integrations/components/integration-settings.tsxapps/dashboard/src/components/integrations/components/parse-slack-credentials-block.tsapps/dashboard/src/components/integrations/components/slack-credentials-paste.tsxapps/dashboard/src/components/integrations/components/use-slack-credentials-paste-fallback.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.errors.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/shared/src/consts/slack-agent-oauth-scopes.ts
Co-authored-by: Dima Grossman <dima@grossman.io>
…es NV-7457 Co-authored-by: Dima Grossman <dima@grossman.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/src/app/agents/services/chat-sdk.service.ts (1)
255-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log raw attachment filenames in this warning.
filenamesis user-controlled input and can easily contain names, ticket numbers, or other PII.agentId,platform, anddroppedCountshould be enough for this branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 255 - 264, Remove logging of raw attachment filenames to avoid recording user-controlled/PII data: in the branch that checks UNSUPPORTED_FILE_PLATFORMS (the block calling this.logger.warn) stop passing filenames: content.files.map(...) and only include agentId, platform and droppedCount (or a sanitized fileCount) in the logger metadata; keep the existing warning message but ensure any mention of file details is limited to the numeric count (use droppedCount = content.files.length) and do not log content.files or filenames anywhere in chat-sdk.service.ts.
🤖 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/agents/services/chat-sdk.service.ts`:
- Around line 104-125: The isPrivateIp function's IPv6 link-local regexes
(currently /^fe[89ab][0-9a-f]{2}:/i and /^::ffff:fe[89ab][0-9a-f]{2}:/i) don't
match valid link-local addresses like fe80::1, allowing SSRF bypass; update
those two patterns to accept 1–3 hex digits after the initial nibble (for
example use /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i and
/^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i) so IPv6 link-local addresses are
correctly detected in isPrivateIp.
---
Duplicate comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 255-264: Remove logging of raw attachment filenames to avoid
recording user-controlled/PII data: in the branch that checks
UNSUPPORTED_FILE_PLATFORMS (the block calling this.logger.warn) stop passing
filenames: content.files.map(...) and only include agentId, platform and
droppedCount (or a sanitized fileCount) in the logger metadata; keep the
existing warning message but ensure any mention of file details is limited to
the numeric count (use droppedCount = content.files.length) and do not log
content.files or filenames anywhere in chat-sdk.service.ts.
🪄 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: 53277179-a3ea-4ebf-b540-5d44d26d7ab5
📒 Files selected for processing (2)
apps/api/src/app/agents/services/chat-sdk.service.spec.tsapps/api/src/app/agents/services/chat-sdk.service.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/src/app/agents/services/chat-sdk.service.ts (1)
120-121:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix IPv6 link-local regexes; current patterns miss valid addresses.
Line 120 and Line 121 currently require an overlong first hextet, so valid link-local addresses (for example
fe80::1) won’t match and can bypass the private-IP SSRF block.Suggested patch
- /^fe[89ab][0-9a-f]{2}:/i, - /^::ffff:fe[89ab][0-9a-f]{2}:/i, + /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i, + /^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i,#!/bin/bash # Verify current regex behavior and confirm Line 120-121 patterns miss valid link-local samples. cat -n apps/api/src/app/agents/services/chat-sdk.service.ts | sed -n '116,123p' node - <<'NODE' const buggy = [/^fe[89ab][0-9a-f]{2}:/i, /^::ffff:fe[89ab][0-9a-f]{2}:/i]; const fixed = [/^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i, /^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i]; const samples = ['fe80::1', 'fe8a::1', 'fe9f::1', 'febf::1']; console.log('Buggy patterns:'); for (const re of buggy) { console.log(re.toString(), samples.map((s) => `${s}:${re.test(s)}`).join(' | ')); } console.log('\nFixed patterns:'); for (const re of fixed) { console.log(re.toString(), samples.map((s) => `${s}:${re.test(s)}`).join(' | ')); } NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 120 - 121, Update the IPv6 link-local regexes used in the SSRF/private-IP check in chat-sdk.service (the patterns currently /^fe[89ab][0-9a-f]{2}:/i and /^::ffff:fe[89ab][0-9a-f]{2}:/i) because they require an overly long first hextet and miss valid addresses like "fe80::1"; replace them with /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i and /^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i respectively so link-local addresses with 0–2 trailing hex digits in the first hextet are correctly matched.
🧹 Nitpick comments (1)
apps/api/src/app/agents/services/chat-sdk.service.ts (1)
94-102: Useinterfacefor backend object type definitions in this file.Lines 94–97 introduce backend object-shape definitions as
typealiases; backend files must useinterfacefor type definitions per repo guidelines.Suggested patch
-type ChatSdkFile = Omit<FileRef, 'data'> & { data?: Buffer }; -type ChatSdkReplyContent = Omit<ReplyContentDto, 'files'> & { files?: ChatSdkFile[] }; -type MaterializedFile = ChatSdkFile & { size: number; source: 'data' | 'url' }; -type PinnedFileResponse = { +interface ChatSdkFile extends Omit<FileRef, 'data'> { + data?: Buffer; +} +interface ChatSdkReplyContent extends Omit<ReplyContentDto, 'files'> { + files?: ChatSdkFile[]; +} +interface MaterializedFile extends ChatSdkFile { + size: number; + source: 'data' | 'url'; +} +interface PinnedFileResponse { status: number; statusText: string; headers: http.IncomingHttpHeaders; data: Buffer; -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 94 - 102, The file defines backend object shapes using type aliases; replace the type aliases ChatSdkFile, ChatSdkReplyContent, MaterializedFile, and PinnedFileResponse with equivalent interface declarations (keeping the same property names, optional markers, and unions like 'data?: Buffer' and 'source: "data" | "url"') so the backend follows the repo guideline to use interface for object type definitions; update any imports/uses if necessary to reference the new interfaces.
🤖 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/dashboard/src/components/integrations/components/parse-slack-credentials-block.ts`:
- Around line 166-169: matchesLabel currently only compares exact text so
headings with a trailing colon (e.g., "App ID:") don't match; update
matchesLabel to normalize trailing colons by stripping any trailing colon and
surrounding whitespace from the input line (and/or from each label/alias) before
doing case-insensitive comparison. Specifically, inside matchesLabel (function
name) normalize line with something like line.replace(/:\s*$/, '') and compare
against field.label and field.aliases similarly normalized so labels with or
without a trailing colon both match.
---
Duplicate comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 120-121: Update the IPv6 link-local regexes used in the
SSRF/private-IP check in chat-sdk.service (the patterns currently
/^fe[89ab][0-9a-f]{2}:/i and /^::ffff:fe[89ab][0-9a-f]{2}:/i) because they
require an overly long first hextet and miss valid addresses like "fe80::1";
replace them with /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i and
/^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i respectively so link-local addresses with
0–2 trailing hex digits in the first hextet are correctly matched.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 94-102: The file defines backend object shapes using type aliases;
replace the type aliases ChatSdkFile, ChatSdkReplyContent, MaterializedFile, and
PinnedFileResponse with equivalent interface declarations (keeping the same
property names, optional markers, and unions like 'data?: Buffer' and 'source:
"data" | "url"') so the backend follows the repo guideline to use interface for
object type definitions; update any imports/uses if necessary to reference the
new interfaces.
🪄 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: adee62d3-0409-46cb-a54f-e4ee7b5b50fc
📒 Files selected for processing (3)
apps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/bootstrap.tsapps/dashboard/src/components/integrations/components/parse-slack-credentials-block.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/bootstrap.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/app/agents/services/chat-sdk.service.ts (1)
101-109: ⚡ Quick winPrefer
interfacefor these backend transport shapes.These are structural contracts in
apps/api, so keeping them asinterfaces matches the repo’s backend TypeScript convention and makes later extension/implementation cleaner.As per coding guidelines,
**/*.{ts,tsx}: On the backend: useinterfacefor type definitions; on the frontend: usetypefor type definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 101 - 109, Change the four backend transport shapes from type aliases to interfaces: replace the type declarations for ChatSdkFile, ChatSdkReplyContent, MaterializedFile, and PinnedFileResponse with interface declarations that preserve the same properties (including optional fields and unions like { data?: Buffer } and source: 'data' | 'url'), so they follow the backend convention of using interface for structural contracts and remain extendable/implementable by other modules.
🤖 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/agents/services/chat-sdk.service.ts`:
- Around line 178-185: prepareContentForDelivery() returns deliveryContent with
.card and materialized .files, but the thread.post call in this branch calls the
card-only overload and drops attachments; update the logic around
deliveryContent handling so that when deliveryContent.card and
deliveryContent.files are both present you either (A) reject/throw a clear error
indicating "card+files not supported" from the caller (or in
prepareContentForDelivery), or (B) call the appropriate thread.post overload (or
construct a payload) that preserves both card and files and forwards
deliveryContent.files along with deliveryContent.card; change the call sites
using thread.post (the branches that currently do
thread.post(deliveryContent.card) and the similar branch later) to handle the
card+files combination consistently.
In `@libs/application-generic/src/utils/ssrf-url-validation.ts`:
- Around line 65-66: The two IPv6 regex literals /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i
and /^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i are too permissive because the tail
is optional; make the last hex nibble mandatory by changing the quantifier to
require 1–2 hex digits (e.g. {1,2}) so only fe80–febf matches; apply the same
change to the duplicate regex in the shared package's ssrf-url-validation
implementation and update the corresponding spec/test expectations to reflect
the corrected matches.
In `@packages/shared/src/utils/ssrf-url-validation.spec.ts`:
- Around line 47-52: The test for validateUrlSsrf should use a unique throwaway
hostname to avoid DNS-cache coupling with the module-scoped LRU memoization;
replace 'https://example.com/file.txt' with a unique '.invalid' host (e.g.,
'https://unique-host.invalid/file.txt') so the mocked dns.promises.lookup call
is always exercised and the assertion against the resolved address (fe80::1)
remains deterministic.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 101-109: Change the four backend transport shapes from type
aliases to interfaces: replace the type declarations for ChatSdkFile,
ChatSdkReplyContent, MaterializedFile, and PinnedFileResponse with interface
declarations that preserve the same properties (including optional fields and
unions like { data?: Buffer } and source: 'data' | 'url'), so they follow the
backend convention of using interface for structural contracts and remain
extendable/implementable by other modules.
🪄 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: a755673b-1cf0-4fd5-bc99-56add518999a
📒 Files selected for processing (5)
apps/api/src/app/agents/services/chat-sdk.service.spec.tsapps/api/src/app/agents/services/chat-sdk.service.tslibs/application-generic/src/utils/ssrf-url-validation.tspackages/shared/src/utils/ssrf-url-validation.spec.tspackages/shared/src/utils/ssrf-url-validation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/agents/services/chat-sdk.service.spec.ts
Comment on lines
+65
to
+66
| /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i, | ||
| /^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i, |
Contributor
There was a problem hiding this comment.
The widened IPv6 pattern now matches addresses outside fe80::/10.
fe8::1 expands to 0fe8::1, and feb::1 expands to 0feb::1; neither is in the fe80–febf first-hextet range. Making the tail optional with {0,2} turns those forms into false positives, so the last hex nibble should stay mandatory here. Please mirror the same correction in packages/shared/src/utils/ssrf-url-validation.ts and its new spec expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/application-generic/src/utils/ssrf-url-validation.ts` around lines 65 -
66, The two IPv6 regex literals /^fe(?:8|9|a|b)[0-9a-f]{0,2}:/i and
/^::ffff:fe(?:8|9|a|b)[0-9a-f]{0,2}:/i are too permissive because the tail is
optional; make the last hex nibble mandatory by changing the quantifier to
require 1–2 hex digits (e.g. {1,2}) so only fe80–febf matches; apply the same
change to the duplicate regex in the shared package's ssrf-url-validation
implementation and update the corresponding spec/test expectations to reflect
the corrected matches.
Detect and ignore Slack's masked secret glyphs when parsing pasted "App Credentials" blocks so users don't accidentally paste bullet characters into fields. parse-slack-credentials-block: add MASK_CHAR_REGEX/isMaskedValue, track masked fields, and count masked fields toward a valid Slack block. slack-credentials-paste.tsx: redesign paste affordance into a collapsible card with preview GIF + skeleton, improve outcome messaging/icons, keep paste box open when masked values are present, and surface masked-field guidance. use-slack-credentials-paste-fallback.ts: show toasts for filled and masked fields and prevent filling when secrets are masked. Also add the preview GIF asset.
scopsy
deleted the
nv-7457-sending-files-doesnt-work-502-error-on-all-platforms
branch
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
Test plan
What changed
Agents can now deliver files reliably: the framework accepts inline binary (Buffer/Uint8Array/ArrayBuffer/Blob), base64, or URL references and encodes/validates them; the API normalizes file refs, fetches URL files with SSRF-safe DNS/IP checks, bounded redirects, timeouts, and size limits, and materializes Buffer payloads for chat adapters so Slack and Teams receive real file uploads. The dashboard adds a smart-paste flow to auto-fill Slack app credentials. This prevents silent text-only fallbacks and surfaces clear attachment delivery errors.
Affected areas
Key technical decisions
Testing
New and extended unit tests in api (chat-sdk.service.spec.ts) and framework cover inline/binary encoding, base64 validation, URL fetch success/failure modes, SSRF rejection, redirect and size enforcement, per-message limits, platform-specific behavior, and AgentDeliveryError messaging; CI typechecks and builds included per package.