feat(core-api,console): add new vulnerability notifications - #541
Merged
Conversation
…point Add PATCH /integrations/:id endpoint for updating name, description, or config with JSON Schema validation and field encryption. Implement inline edit mode in integration detail sheet with dynamic form fields (text, password, boolean, array, textarea) and save/cancel flow.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntegration configurations can now be edited through schema-driven controls and persisted through a workspace-scoped PATCH API. Vulnerability processing now sends localized group notifications for newly discovered vulnerabilities. ChangesIntegration update workflow
Vulnerability notifications
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant IntegrationDetailSheet
participant IntegrationsController
participant IntegrationsService
User->>IntegrationDetailSheet: Edit integration fields
IntegrationDetailSheet->>IntegrationsController: PATCH integration
IntegrationsController->>IntegrationsService: Validate and save update
IntegrationsService-->>IntegrationDetailSheet: Updated integration
IntegrationDetailSheet-->>User: Show success and view mode
sequenceDiagram
participant DataAdapterService
participant WorkspacesService
participant NotificationsService
participant NotificationRouter
DataAdapterService->>WorkspacesService: Resolve job workspace members
WorkspacesService-->>DataAdapterService: Return recipients
DataAdapterService->>NotificationsService: Create vulnerability notification
NotificationsService->>NotificationRouter: Resolve target route
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🧹 Nitpick comments (3)
core-api/src/modules/data-adapter/data-adapter.service.spec.ts (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new notification branch.
The empty-members default bypasses all new behavior. Add a test with workspace members asserting recipients, scope, type, metadata, and workspace ID, plus a repeat-scan test ensuring existing vulnerabilities are not announced again.
Also applies to: 96-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-api/src/modules/data-adapter/data-adapter.service.spec.ts` at line 63, Extend the data-adapter service tests around getMemberOfWorkspaceByJobId to return workspace members and exercise the notification path, asserting recipients, scope, type, metadata, and workspace ID; add a repeat-scan case confirming existing vulnerabilities are not notified again. Retain the empty-members coverage separately.core-api/src/modules/data-adapter/data-adapter.service.ts (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse core-api alias imports consistently.
core-api/src/modules/data-adapter/data-adapter.service.ts#L8-L13: import enums from@/common/enums/enum.core-api/src/modules/data-adapter/data-adapter.service.ts#L20-L20: importNotificationsServicethrough@/modules/....core-api/src/modules/data-adapter/data-adapter.service.spec.ts#L11-L11: use the same alias for the test import.As per coding guidelines, “Use
@/alias for imports in core-api.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-api/src/modules/data-adapter/data-adapter.service.ts` around lines 8 - 13, Replace the relative enum import in data-adapter.service.ts (lines 8-13) with the `@/common/enums/enum` alias, and update the NotificationsService import in data-adapter.service.ts (line 20) to use the `@/modules/`... alias. Apply the same alias convention to the test import in data-adapter.service.spec.ts (line 11).Source: Coding guidelines
core-api/src/modules/integrations/integrations.controller.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required core-api import alias.
core-api/src/modules/integrations/integrations.controller.ts#L27-L27: import the DTO from@/modules/integrations/dto/update-integration.dto.core-api/src/modules/integrations/integrations.service.ts#L20-L20: use the same aliased path for the type import.As per coding guidelines, “Use
@/alias for imports in core-api (maps tosrc/directory).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-api/src/modules/integrations/integrations.controller.ts` at line 27, Replace the relative UpdateIntegrationDto import in integrations.controller.ts with the `@/modules/integrations/dto/update-integration.dto` alias, and apply the same aliased type import in integrations.service.ts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console/src/pages/integrations/components/integration-detail-sheet.tsx`:
- Around line 253-258: Update the default-value initialization around
SchemaProperty so missing array fields are not assigned ['']; only materialize
explicitly defined defaults, and let ArrayField render an empty row without
mutating values. Preserve initialization behavior for non-array properties and
arrays with a declared default.
- Around line 189-199: Update the numeric-field branch in the integration detail
component so existing number and integer values are rendered instead of blank,
while converting edited input values back to numbers before invoking onChange.
Preserve empty input handling appropriately and ensure the resulting value
satisfies number/integer schema validation.
In `@core-api/src/modules/data-adapter/data-adapter.service.ts`:
- Around line 292-302: Move the createNotification call out of the database
transaction in the surrounding data-adapter flow, ensuring the vulnerability
transaction commits before enqueueing the notification. Preserve the existing
notification payload and workspace scope, and make the post-commit enqueue
idempotent or route it through the established transactional-outbox mechanism.
- Around line 283-295: Update the vulnerability persistence flow that builds
vulsForAlert to distinguish rows inserted by the scan from rows returned after
an ON CONFLICT update, excluding conflict-updated vulnerabilities before
notification recipients are constructed in the workspace notification block.
Preserve alerts for genuinely new fingerprints, and add a regression test
covering a repeat scan that verifies NEW_VULNERABILITY_FOUND is not sent again.
In `@core-api/src/modules/integrations/integrations.service.ts`:
- Around line 236-244: Preserve existing sensitive credentials when updating
integration configuration: in integrations.service.ts (236-244), merge masked or
omitted secret fields from the decrypted stored config before
validateConfigOrThrow and encryptSensitiveConfigFields. In
integration-detail-sheet.tsx (249-251), prevent masked placeholders from being
treated as editable secret values; at (351-356), omit unchanged secret fields or
use the explicit unchanged-secret contract when submitting.
---
Nitpick comments:
In `@core-api/src/modules/data-adapter/data-adapter.service.spec.ts`:
- Line 63: Extend the data-adapter service tests around
getMemberOfWorkspaceByJobId to return workspace members and exercise the
notification path, asserting recipients, scope, type, metadata, and workspace
ID; add a repeat-scan case confirming existing vulnerabilities are not notified
again. Retain the empty-members coverage separately.
In `@core-api/src/modules/data-adapter/data-adapter.service.ts`:
- Around line 8-13: Replace the relative enum import in data-adapter.service.ts
(lines 8-13) with the `@/common/enums/enum` alias, and update the
NotificationsService import in data-adapter.service.ts (line 20) to use the
`@/modules/`... alias. Apply the same alias convention to the test import in
data-adapter.service.spec.ts (line 11).
In `@core-api/src/modules/integrations/integrations.controller.ts`:
- Line 27: Replace the relative UpdateIntegrationDto import in
integrations.controller.ts with the
`@/modules/integrations/dto/update-integration.dto` alias, and apply the same
aliased type import in integrations.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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 35f5ea5b-b16a-44f5-be0e-11119538ddda
⛔ Files ignored due to path filters (1)
console/src/services/apis/gen/queries.tsis excluded by!**/gen/**
📒 Files selected for processing (9)
console/src/pages/integrations/components/integration-detail-sheet.tsxcore-api/src/common/enums/enum.tscore-api/src/i18n/en/notification.jsoncore-api/src/i18n/routers/notification.jsoncore-api/src/modules/data-adapter/data-adapter.service.spec.tscore-api/src/modules/data-adapter/data-adapter.service.tscore-api/src/modules/integrations/dto/update-integration.dto.tscore-api/src/modules/integrations/integrations.controller.tscore-api/src/modules/integrations/integrations.service.ts
Comment on lines
+189
to
+199
| if (prop.type === 'number' || prop.type === 'integer') { | ||
| return ( | ||
| <Input | ||
| type="number" | ||
| name={key} | ||
| id={key} | ||
| autoComplete={autoComplete} | ||
| placeholder={placeholder} | ||
| value={typeof value === 'string' ? value : ''} | ||
| onChange={(e) => onChange(e.target.value)} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep numeric fields numeric.
Existing numbers render blank, while edited values become strings and fail number/integer schema validation.
Proposed fix
- value={typeof value === 'string' ? value : ''}
- onChange={(e) => onChange(e.target.value)}
+ value={
+ typeof value === 'number' || typeof value === 'string' ? value : ''
+ }
+ onChange={(e) => {
+ const rawValue = e.target.value;
+ onChange(rawValue === '' ? '' : Number(rawValue));
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (prop.type === 'number' || prop.type === 'integer') { | |
| return ( | |
| <Input | |
| type="number" | |
| name={key} | |
| id={key} | |
| autoComplete={autoComplete} | |
| placeholder={placeholder} | |
| value={typeof value === 'string' ? value : ''} | |
| onChange={(e) => onChange(e.target.value)} | |
| /> | |
| if (prop.type === 'number' || prop.type === 'integer') { | |
| return ( | |
| <Input | |
| type="number" | |
| name={key} | |
| id={key} | |
| autoComplete={autoComplete} | |
| placeholder={placeholder} | |
| value={ | |
| typeof value === 'number' || typeof value === 'string' ? value : '' | |
| } | |
| onChange={(e) => { | |
| const rawValue = e.target.value; | |
| onChange(rawValue === '' ? '' : Number(rawValue)); | |
| }} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/src/pages/integrations/components/integration-detail-sheet.tsx`
around lines 189 - 199, Update the numeric-field branch in the integration
detail component so existing number and integer values are rendered instead of
blank, while converting edited input values back to numbers before invoking
onChange. Preserve empty input handling appropriately and ensure the resulting
value satisfies number/integer schema validation.
Comment on lines
+253
to
+258
| const typedProp = prop as SchemaProperty; | ||
| if (typedProp.default !== undefined) { | ||
| values[key] = typedProp.default; | ||
| } else if (typedProp.type === 'array') { | ||
| values[key] = ['']; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not materialize absent arrays as [''].
Entering edit mode mutates every missing array field, so an unrelated save can persist a blank entry or fail schema validation. ArrayField already renders an empty row without storing it.
Proposed fix
if (typedProp.default !== undefined) {
values[key] = typedProp.default;
- } else if (typedProp.type === 'array') {
- values[key] = [''];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const typedProp = prop as SchemaProperty; | |
| if (typedProp.default !== undefined) { | |
| values[key] = typedProp.default; | |
| } else if (typedProp.type === 'array') { | |
| values[key] = ['']; | |
| } | |
| const typedProp = prop as SchemaProperty; | |
| if (typedProp.default !== undefined) { | |
| values[key] = typedProp.default; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/src/pages/integrations/components/integration-detail-sheet.tsx`
around lines 253 - 258, Update the default-value initialization around
SchemaProperty so missing array fields are not assigned ['']; only materialize
explicitly defined defaults, and let ArrayField render an empty row without
mutating values. Preserve initialization behavior for non-array properties and
arrays with a declared default.
Comment on lines
+292
to
+302
| await this.notificationsService.createNotification({ | ||
| recipients: recipientIds, | ||
| scope: NotificationScope.GROUP, | ||
| type: NotificationType.NEW_VULNERABILITY_FOUND, | ||
| metadata: { | ||
| count: String(vulsForAlert.length), | ||
| assetValue: job.asset.value, | ||
| targetId: job.asset.target.id, | ||
| }, | ||
| workspaceId, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant section with line numbers.
sed -n '240,340p' core-api/src/modules/data-adapter/data-adapter.service.ts
# Find the notification service implementation and its persistence/queue behavior.
rg -n "class .*NotificationsService|createNotification\(" core-api/src -g '*.ts'Repository: oasm-platform/open-asm
Length of output: 3580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== data-adapter.service.ts around transaction wrapper =="
sed -n '220,320p' core-api/src/modules/data-adapter/data-adapter.service.ts
echo
echo "== notifications.service.ts =="
sed -n '1,220p' core-api/src/modules/notifications/notifications.service.ts
echo
echo "== vulnerability-analysis.processor.ts =="
sed -n '1,120p' core-api/src/modules/vulnerabilities/processors/vulnerability-analysis.processor.tsRepository: oasm-platform/open-asm
Length of output: 8254
Move notification enqueueing out of the transaction
createNotification() enqueues a separate BullMQ job before the DB transaction commits. If the commit fails, users can get a notification for rolled-back vulnerabilities; if the queue add fails, the whole transaction is aborted. Commit first, then enqueue idempotently, or use a transactional outbox.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core-api/src/modules/data-adapter/data-adapter.service.ts` around lines 292 -
302, Move the createNotification call out of the database transaction in the
surrounding data-adapter flow, ensuring the vulnerability transaction commits
before enqueueing the notification. Preserve the existing notification payload
and workspace scope, and make the post-commit enqueue idempotent or route it
through the established transactional-outbox mechanism.
Comment on lines
+236
to
+244
| if (dto.config !== undefined) { | ||
| validateConfigOrThrow({ | ||
| appType: integration.appType, | ||
| category: integration.category, | ||
| config: dto.config, | ||
| }); | ||
|
|
||
| integration.config = encryptSensitiveConfigFields(dto.config); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve existing secrets when saving masked configuration.
Responses contain masked sensitive values, but the console submits those placeholders as the complete config and the service encrypts them as replacements. Any ordinary edit therefore destroys unchanged credentials.
core-api/src/modules/integrations/integrations.service.ts#L236-L244: merge masked or omitted secret fields from the decrypted stored config before validation and encryption.console/src/pages/integrations/components/integration-detail-sheet.tsx#L249-L251: do not treat masked placeholders as editable secret values.console/src/pages/integrations/components/integration-detail-sheet.tsx#L351-L356: omit unchanged secret fields or use an explicit unchanged-secret contract.
📍 Affects 2 files
core-api/src/modules/integrations/integrations.service.ts#L236-L244(this comment)console/src/pages/integrations/components/integration-detail-sheet.tsx#L249-L251console/src/pages/integrations/components/integration-detail-sheet.tsx#L351-L356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core-api/src/modules/integrations/integrations.service.ts` around lines 236 -
244, Preserve existing sensitive credentials when updating integration
configuration: in integrations.service.ts (236-244), merge masked or omitted
secret fields from the decrypted stored config before validateConfigOrThrow and
encryptSensitiveConfigFields. In integration-detail-sheet.tsx (249-251), prevent
masked placeholders from being treated as editable secret values; at (351-356),
omit unchanged secret fields or use the explicit unchanged-secret contract when
submitting.
- Add fingerprint pre-check in vulnerabilities() to skip notifications for existing vulns (no duplicate notifications on re-scan) - Remove severity filter: all new vulns (critical→low) now trigger notification - Add debug logging to trace notification pipeline failures - Add tests: new vuln notification, existing vuln skip, low severity notify
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core-api/src/modules/data-adapter/data-adapter.service.spec.ts`:
- Around line 5-9: Update the import of NotificationType, Severity, and
ToolCategory in the data-adapter service spec to use the configured `@/` alias
mapped to core-api/src instead of the relative ../../common path.
In `@core-api/src/modules/data-adapter/data-adapter.service.ts`:
- Around line 256-270: Replace the pre-check using existingRows and
existingFingerprints with atomic classification from the vulnerability
write/upsert operation, deriving only rows actually inserted by that operation
for notification decisions. Preserve fingerprint uniqueness enforcement and
ensure concurrent scans cannot both classify the same fingerprint as new. Add a
regression test that runs concurrent scans with the same fingerprint and
verifies only one notification is produced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3be88ee8-fbd7-4a10-a03a-6d84a12bf901
📒 Files selected for processing (3)
core-api/src/i18n/routers/notification.jsoncore-api/src/modules/data-adapter/data-adapter.service.spec.tscore-api/src/modules/data-adapter/data-adapter.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core-api/src/i18n/routers/notification.json
Comment on lines
+5
to
+9
| import { | ||
| NotificationType, | ||
| Severity, | ||
| ToolCategory, | ||
| } from '../../common/enums/enum'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the configured @/ import alias.
Proposed fix
import {
NotificationType,
Severity,
ToolCategory,
-} from '../../common/enums/enum';
+} from '`@/common/enums/enum`';As per coding guidelines, "core-api/src/**/*.{ts,tsx}: Use @/ alias for imports in core-api (maps to src/ directory)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { | |
| NotificationType, | |
| Severity, | |
| ToolCategory, | |
| } from '../../common/enums/enum'; | |
| import { | |
| NotificationType, | |
| Severity, | |
| ToolCategory, | |
| } from '`@/common/enums/enum`'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core-api/src/modules/data-adapter/data-adapter.service.spec.ts` around lines
5 - 9, Update the import of NotificationType, Severity, and ToolCategory in the
data-adapter service spec to use the configured `@/` alias mapped to core-api/src
instead of the relative ../../common path.
Source: Coding guidelines
Comment on lines
+256
to
+270
| // Pre-check: find which fingerprints already exist | ||
| // to avoid sending notifications for updated vulns | ||
| const existingRows = await manager | ||
| .createQueryBuilder() | ||
| .select('v.fingerprint', 'fingerprint') | ||
| .from(Vulnerability, 'v') | ||
| .where('v.fingerprint IN (:...fingerprints)', { | ||
| fingerprints: uniqueValues.map((v) => v.fingerprint), | ||
| }) | ||
| .getRawMany(); | ||
|
|
||
| const existingFingerprints = new Set<string>( | ||
| (existingRows as { fingerprint: string }[]).map((r) => r.fingerprint), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make new-vulnerability classification atomic.
Concurrent scans can both read a fingerprint as absent before either upsert completes, causing both transactions to send the same notification. Derive inserted rows atomically from the write operation and add a concurrent-scan regression test.
#!/bin/bash
# Verify database configuration and fingerprint uniqueness constraints.
rg -n -C3 \
"type:\s*'postgres'|fingerprint|conflict_target.*fingerprint|`@Index`" \
core-api -g '*.ts'Also applies to: 285-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core-api/src/modules/data-adapter/data-adapter.service.ts` around lines 256 -
270, Replace the pre-check using existingRows and existingFingerprints with
atomic classification from the vulnerability write/upsert operation, deriving
only rows actually inserted by that operation for notification decisions.
Preserve fingerprint uniqueness enforcement and ensure concurrent scans cannot
both classify the same fingerprint as new. Add a regression test that runs
concurrent scans with the same fingerprint and verifies only one notification is
produced.
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.
Summary by CodeRabbit
PATCH /integrations/:idto update integration name, description, and configuration.