feat(security): implement end-to-end security pillar#34
Open
Arzu-N wants to merge 22 commits into
Open
Conversation
|
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 code scan alert and vulnerability contracts, persistence mappings and schema, scanner/service flows, REST endpoints, worker scheduling, and tests across the api, worker, scanning, persistence, domain, and common modules. ChangesSecurity Pillar: Code Scan Alerts, Vulnerability Priority, Mapper Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 19
🧹 Nitpick comments (4)
backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java (1)
33-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the response contract, not only the status.
This test currently passes even if
CodeScanAlertController.create(...)returns the wrong body, because the mock is unstubbed and only201is asserted. Given the PR objective is to return the expected contract shapes, stubcodeScanAlertService.create(...), verify delegation, and assert the JSON fields you expect back.🤖 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 `@backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java` around lines 33 - 43, The test for CodeScanAlertController.create is only checking the HTTP status, so it can pass even if the response body is wrong. In CodeScanAlertControllerTest, stub codeScanAlertService.create(...) with the expected DTO, verify the controller delegates to that service method, and add JSON assertions on the returned payload to validate the response contract instead of only asserting 201.backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java (1)
33-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the returned DTO shape as well.
SecretFindingController.createSecret(...)returns a created body, but this test only checks the status code. StubsecretFindingService.create(...), verify it was called, and assert the response JSON so this actually guards the contract introduced in this PR.🤖 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 `@backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java` around lines 33 - 43, The SecretFindingControllerTest currently only verifies the 201 status and does not validate the returned DTO contract. Update whenCreateSecret_thenShouldReturn201() to stub secretFindingService.create(...), verify that create(...) is invoked, and assert the response JSON/body shape from SecretFindingController.createSecret(...) so the test covers the created response payload as well as the status.backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java (1)
33-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the vulnerability payload, not just
201 Created.This endpoint’s response now includes security-specific fields such as priority, but the test never checks them. Stub
vulnerabilityService.create(...), verify the call, and assert the returned JSON so the contract change is actually protected.🤖 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 `@backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java` around lines 33 - 43, The create-vulnerability test currently only checks for 201 Created and does not verify the response contract. Update whenCreateVulnerability_thenShouldReturn201 in VulnerabilityControllerTest to stub vulnerabilityService.create(...), verify that the controller invokes it, and assert the returned JSON payload includes the security-specific fields such as priority. Use the existing MockMvc setup and ObjectMapper to validate the response body instead of only the status.backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead severity branch or implement the missing mapping.
Lines 22-27 set
Status.OPENin both paths, so severity currently has no effect. If all new alerts should start open, collapse this to one assignment; otherwise the low/medium rule is missing here.🤖 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 `@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java` around lines 22 - 27, The severity check in CodeScanAlertScanner is dead code because both branches set the same Status.OPEN value. Update the scan alert mapping logic in the method that processes CodeScanAlertEntity so either all severities are assigned OPEN with a single assignment, or add the missing low/medium severity-specific status mapping if that was intended. Use the Severity, Status, and CodeScanAlertEntity symbols to locate and simplify the conditional.Source: Linters/SAST tools
🤖 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
`@backend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.java`:
- Around line 18-21: The RuntimeException handler in GlobalExceptionHandler is
exposing raw exception text in the 500 response; change handleRuntimeException
to return a generic client-facing message instead of ex.getMessage(), and add
server-side logging of the exception details for debugging. Keep the response
body consistent and update the ResponseEntity/Map.of usage so only a safe
generic error string is returned.
In
`@backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java`:
- Line 34: Rename the test method in CodeScanAlertControllerTest to satisfy the
MethodName Checkstyle rule by removing underscores from
whenCreateCodeScanAlert_thenShouldBeReturn201; keep the same test behavior, but
use a camelCase method name so the class continues to compile and CI passes.
In
`@backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java`:
- Line 34: The test method name in SecretFindingControllerTest does not match
the configured MethodName pattern, causing Checkstyle failure. Rename
whenCreateSecret_thenShouldReturn201 to a valid lowerCamelCase test name that
fits the project’s naming rules, and keep the behavior and assertions unchanged.
In
`@backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java`:
- Line 34: Rename the test method whenCreateVulnerability_thenShouldReturn201 to
match the configured camelCase test naming rule used by Checkstyle, keeping it
in VulnerabilityControllerTest and preserving the same test behavior. Update
only the method identifier so the name uses standard lowerCamelCase while still
clearly describing the create vulnerability success case.
In
`@backend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.java`:
- Line 7: `CleatWorkerApplication` is not scanning the worker package, so
`VulnerabilityWorker` is never discovered and its scheduled job won’t run.
Update the `@SpringBootApplication` component scan on `CleatWorkerApplication`
to include `dev.cleat.worker` (or broaden it to `dev.cleat`) alongside the
existing `dev.cleat.persistence` and `dev.cleat.scanning` packages so the worker
bean is registered.
In `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`:
- Around line 10-15: The VulnerabilityWorker constructor is incorrectly
injecting VulnerabilityEntity, which is a JPA entity and not a Spring-managed
bean. Update VulnerabilityWorker so it only injects the service/dependency that
provides scan input, and create or fetch the VulnerabilityEntity inside
runSecurityScan() or via a proper repository/service instead of constructor
injection. Keep the constructor signature aligned with the actual Spring bean
dependencies and ensure the worker no longer depends on a directly injected
entity instance.
In
`@backend/libs/common/src/main/java/dev/cleat/common/dto/request/VulnerabilityRequestDto.java`:
- Line 3: Remove client-write access to priority from the vulnerability
create/update flow: VulnerabilityRequestDto should no longer expose or carry a
writable priority field, since it must be derived server-side. Update
VulnerabilityMapper.toVulnerabilityEntity(...) so it stops copying getPriority()
from the request DTO into VulnerabilityEntity, and compute/set priority in the
server-side mapping/service path based on CVSS/KEV/severity. Also remove any
unused Priority import and related DTO accessors/constructors tied to
request-supplied priority.
In `@backend/libs/domain/src/main/java/dev/cleat/domain/model/Vulnerability.java`:
- Line 5: The Vulnerability record currently allows nullable cvss and severity
values, but PriorityCalculator and VulnerabilityScanner assume both are present
for comparisons and switching. Update Vulnerability to enforce its invariants in
the canonical constructor, or adjust PriorityCalculator to handle missing
cvss/severity explicitly before doing numeric checks or the severity switch,
using the Vulnerability record, VulnerabilityScanner, and PriorityCalculator
symbols to locate the flow.
In `@backend/libs/domain/src/main/java/dev/cleat/domain/PriorityCalculator.java`:
- Around line 9-21: PriorityCalculator.calculate currently dereferences nullable
Vulnerability fields before the priority decision, causing failures instead of a
safe priority result. Update calculate(Vulnerability) to guard cvss() and
severity() up front: avoid auto-unboxing cvss() in the range check and handle
null severity before the switch, returning a sensible default priority when
either field is missing. Keep the existing URGENT/HIGH/MEDIUM/LOW mapping in
PriorityCalculator, but make the logic null-safe before evaluating the CVSS
threshold or switching on severity.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/entity/CodeScanAlertEntity.java`:
- Around line 56-57: The `detectedAt` field on `CodeScanAlertEntity` is
currently left unset before persistence, so `CodeScanAlertScanner` can save a
null value instead of relying on the timestamp default. Populate this field
either in `CodeScanAlertMapper` when building the entity or in a `@PrePersist`
hook on `CodeScanAlertEntity`, and ensure the `detectedAt` column is marked
non-null if the timestamp is required. Keep the fix centered on the entity
mapping/save flow so every alert has a timestamp before insert.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/ActivityEventMapper.java`:
- Around line 15-24: ActivityEventMapper is not null-safe when mapping the repo
field, so activityEventEntity.getRepo().getName() can throw if repo is absent.
Update the mapping logic in the mapper’s ActivityEventResponseDto conversion to
handle a null repo gracefully, either by guarding the getRepo() access before
calling getName() or by applying a required-field change consistently across the
entity/schema and mapper.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/SecretFindingMapper.java`:
- Around line 28-42: The SecretFindingMapper.toSecretFindingEntity create-flow
mapping is missing the account/repo data that SecretFindingService later needs
for the POST response. Update toSecretFindingEntity to also copy the account
identifier and repo association from SecretFindingRequestDto into
SecretFindingEntity so toSecretFindingDto can safely read getAccountId() and
getRepo().getName() without relying on scanner-side hydration.
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/UsageMapper.java`:
- Around line 20-30: The UsageMapper.toUsageResponseDto mapping dereferences
usageEntity.getSeries() directly, which can fail when
DashboardService.getDataset(...) supplies a fallback new UsageEntity() with a
null series. Update the mapper to safely handle a null series by defaulting to
an empty list before streaming, or initialize the series field in the
UsageEntity fallback path, so the UsageResponseDto mapping remains valid for
empty-usage accounts.
In
`@backend/libs/persistence/src/main/resources/db/migration/V3_alter_vulnerability_table.sql`:
- Line 1: The V3_alter_vulnerability_table migration only adds the priority
column and leaves existing vulnerability rows with NULL values. Update the
rollout so the vulnerability table change is paired with a backfill or explicit
post-migration rescan, using the migration script or the related vulnerability
persistence/rescan flow, so existing records get populated with priority instead
of remaining incomplete.
- Line 1: Rename the Flyway migration file to use the default versioned format
with a double underscore so it is picked up automatically. Update the migration
named V3_alter_vulnerability_table.sql to V3__alter_vulnerability_table.sql,
keeping the ALTER TABLE vulnerability statement unchanged.
In
`@backend/libs/persistence/src/main/resources/db/migration/V4__create_code_scan_alerts_table.sql`:
- Line 7: The schema for code scan alerts is hardcoding branch to "main" in the
migration, which can store the wrong value when ingest data omits branch. Remove
the default from the branch column in V4__create_code_scan_alerts_table.sql so
it stays nullable, and ensure the real branch is set earlier in the ingest flow
instead of being synthesized in the database schema.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java`:
- Around line 23-37: Move the null/input validation in
VulnerabilityScanner.processAndSave out of the try block so invalid inputs are
rejected before any persistence or priority calculation work starts. Keep the
null guard for VulnerabilityEntity at the top of the method, then run the
Vulnerability/priorityCalculator flow inside the try, and ensure the catch only
handles downstream failures. Also avoid calling vulnerabilityEntity.getId() in
the error path unless the entity is known non-null, so a null payload doesn’t
trigger a second exception and hide the original failure.
In
`@backend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java`:
- Around line 39-53: The test in VulnerabilityScannerTest is asserting against a
stubbed repository lookup instead of the actual save behavior. Update the
verification around VulnerabilityScanner.processAndSave(...) to assert directly
on vulnerabilityEntity after the call and verify that
vulnerabilityRepository.save(vulnerabilityEntity) was invoked. Keep the existing
priority expectations tied to the saved entity so the test validates persistence
rather than a mocked findById(...) path.
- Around line 60-67: The current test in VulnerabilityScannerTest only verifies
exception wrapping because priorityCalculator.calculate(...) is mocked to throw,
so it does not actually cover invalid CVSS handling. Update the test to either
use the real PriorityCalculator path with an out-of-range CVSS value in
VulnerabilityScanner.processAndSave, or rename the test/method to reflect that
it is testing downstream exception propagation rather than CVSS validation.
---
Nitpick comments:
In
`@backend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.java`:
- Around line 33-43: The test for CodeScanAlertController.create is only
checking the HTTP status, so it can pass even if the response body is wrong. In
CodeScanAlertControllerTest, stub codeScanAlertService.create(...) with the
expected DTO, verify the controller delegates to that service method, and add
JSON assertions on the returned payload to validate the response contract
instead of only asserting 201.
In
`@backend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.java`:
- Around line 33-43: The SecretFindingControllerTest currently only verifies the
201 status and does not validate the returned DTO contract. Update
whenCreateSecret_thenShouldReturn201() to stub secretFindingService.create(...),
verify that create(...) is invoked, and assert the response JSON/body shape from
SecretFindingController.createSecret(...) so the test covers the created
response payload as well as the status.
In
`@backend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.java`:
- Around line 33-43: The create-vulnerability test currently only checks for 201
Created and does not verify the response contract. Update
whenCreateVulnerability_thenShouldReturn201 in VulnerabilityControllerTest to
stub vulnerabilityService.create(...), verify that the controller invokes it,
and assert the returned JSON payload includes the security-specific fields such
as priority. Use the existing MockMvc setup and ObjectMapper to validate the
response body instead of only the status.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java`:
- Around line 22-27: The severity check in CodeScanAlertScanner is dead code
because both branches set the same Status.OPEN value. Update the scan alert
mapping logic in the method that processes CodeScanAlertEntity so either all
severities are assigned OPEN with a single assignment, or add the missing
low/medium severity-specific status mapping if that was intended. Use the
Severity, Status, and CodeScanAlertEntity symbols to locate and simplify the
conditional.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8972a510-5ec3-459d-a880-7f6bdf28e730
📒 Files selected for processing (50)
backend/apps/api/src/main/java/dev/cleat/api/CleatApiApplication.javabackend/apps/api/src/main/java/dev/cleat/api/controller/CodeScanAlertController.javabackend/apps/api/src/main/java/dev/cleat/api/controller/SecretFindingController.javabackend/apps/api/src/main/java/dev/cleat/api/controller/VulnerabilityController.javabackend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.javabackend/apps/api/src/test/java/dev/cleat/api/CodeScanAlertControllerTest.javabackend/apps/api/src/test/java/dev/cleat/api/SecretFindingControllerTest.javabackend/apps/api/src/test/java/dev/cleat/api/VulnerabilityControllerTest.javabackend/apps/worker/build.gradle.ktsbackend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.javabackend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.javabackend/libs/common/src/main/java/dev/cleat/common/dto/request/CodeScanAlertRequestDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/request/VulnerabilityRequestDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/response/CodeScanAlertResponseDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/response/DatasetDto.javabackend/libs/common/src/main/java/dev/cleat/common/dto/response/VulnerabilityResponseDto.javabackend/libs/common/src/main/java/dev/cleat/common/enums/Priority.javabackend/libs/common/src/main/java/dev/cleat/common/enums/Status.javabackend/libs/domain/build.gradle.ktsbackend/libs/domain/src/main/java/dev/cleat/domain/PriorityCalculator.javabackend/libs/domain/src/main/java/dev/cleat/domain/model/Vulnerability.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/DashboardService.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/CodeScanAlertEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/VulnerabilityEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/AccountMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/ActivityEventMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/CleatMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/CodeScanAlertMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/MemberMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/RepoMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/ScorecardCheckMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/SecretFindingMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/UsageMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/UsagePointMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/VulnerabilityMapper.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/repository/CodeScanAlertRepository.javabackend/libs/persistence/src/main/resources/db/migration/V2__update_repo_and_add_tables.sqlbackend/libs/persistence/src/main/resources/db/migration/V3_alter_vulnerability_table.sqlbackend/libs/persistence/src/main/resources/db/migration/V4__create_code_scan_alerts_table.sqlbackend/libs/persistence/src/main/resources/db/migration/V5__alter_secret_finding_table.sqlbackend/libs/scanning/build.gradle.ktsbackend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/SecretFindingScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/service/CodeScanAlertService.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/service/SecretFindingService.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/service/VulnerabilityService.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/CodeScanAlertScannerTest.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/SecretFindingScannerTest.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java
💤 Files with no reviewable changes (1)
- backend/libs/persistence/src/main/java/dev/cleat/persistence/mapper/CleatMapper.java
Comment on lines
+18
to
+21
| @ExceptionHandler(RuntimeException.class) | ||
| public ResponseEntity<Map<String, String>> handleRuntimeException(RuntimeException ex) { | ||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
| .body(Map.of("error", "An unexpected error occured: " + ex.getMessage())); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not echo raw exception messages in the 500 response.
This leaks internal details such as bean names, package structure, and persistence errors to clients. The current pipeline failure already shows framework exception text that would be serialized verbatim here. Return a generic message and log the exception server-side instead.
Proposed fix
`@ExceptionHandler`(RuntimeException.class)
public ResponseEntity<Map<String, String>> handleRuntimeException(RuntimeException ex) {
+ // log.error("Unhandled exception", ex);
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
- .body(Map.of("error", "An unexpected error occured: " + ex.getMessage()));
+ .body(Map.of("error", "An unexpected error occurred"));
}📝 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
| @ExceptionHandler(RuntimeException.class) | |
| public ResponseEntity<Map<String, String>> handleRuntimeException(RuntimeException ex) { | |
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) | |
| .body(Map.of("error", "An unexpected error occured: " + ex.getMessage())); | |
| `@ExceptionHandler`(RuntimeException.class) | |
| public ResponseEntity<Map<String, String>> handleRuntimeException(RuntimeException ex) { | |
| // log.error("Unhandled exception", ex); | |
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) | |
| .body(Map.of("error", "An unexpected error occurred")); | |
| } |
🤖 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
`@backend/apps/api/src/main/java/dev/cleat/api/exception/GlobalExceptionHandler.java`
around lines 18 - 21, The RuntimeException handler in GlobalExceptionHandler is
exposing raw exception text in the 500 response; change handleRuntimeException
to return a generic client-facing message instead of ex.getMessage(), and add
server-side logging of the exception details for debugging. Keep the response
body consistent and update the ResponseEntity/Map.of usage so only a safe
generic error string is returned.
Source: Pipeline failures
| @@ -0,0 +1 @@ | |||
| ALTER TABLE vulnerability ADD COLUMN priority VARCHAR(50 ); | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfill priority for existing vulnerabilities.
This only changes the schema. Every existing row keeps NULL priority, so older vulnerability records will return incomplete data until a full rescan rewrites them. Add a data backfill or an explicit post-migration rescan in the same rollout.
🤖 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
`@backend/libs/persistence/src/main/resources/db/migration/V3_alter_vulnerability_table.sql`
at line 1, The V3_alter_vulnerability_table migration only adds the priority
column and leaves existing vulnerability rows with NULL values. Update the
rollout so the vulnerability table change is paired with a backfill or explicit
post-migration rescan, using the migration script or the related vulnerability
persistence/rescan flow, so existing records get populated with priority instead
of remaining incomplete.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Flyway's migration separator was customized anywhere.
fd -HI 'application.*\.(yml|yaml|properties)$' backend
rg -n -C2 'flyway|sqlMigrationSeparator|sql-migration-separator' backendRepository: Devlaner/cleat
Length of output: 1277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migration filenames =="
fd -HI 'V*_*.sql' backend/libs/persistence/src/main/resources/db/migration -t f | sort
echo
echo "== application.yml flyway settings =="
for f in backend/apps/api/src/main/resources/application.yml \
backend/apps/worker/src/main/resources/application.yml \
backend/libs/github-client/src/main/resources/application.yml \
backend/libs/persistence/src/test/resources/application.yml; do
if [ -f "$f" ]; then
echo "-- $f"
rg -n -C3 'flyway|sqlMigrationSeparator|sql-migration-separator|spring:' "$f" || true
fi
done
echo
echo "== nearby migration contents =="
for f in backend/libs/persistence/src/main/resources/db/migration/V*_*.sql; do
echo "-- $f"
sed -n '1,3p' "$f"
doneRepository: Devlaner/cleat
Length of output: 2231
Rename this migration to V3__alter_vulnerability_table.sql.
Flyway’s default versioned migration format uses a double underscore separator. This filename will be skipped unless the separator is customized elsewhere, so the priority column won’t be applied.
🤖 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
`@backend/libs/persistence/src/main/resources/db/migration/V3_alter_vulnerability_table.sql`
at line 1, Rename the Flyway migration file to use the default versioned format
with a double underscore so it is picked up automatically. Update the migration
named V3_alter_vulnerability_table.sql to V3__alter_vulnerability_table.sql,
keeping the ALTER TABLE vulnerability statement unchanged.
| severity VARCHAR(50) NOT NULL, | ||
| file VARCHAR(255) NOT NULL, | ||
| line INTEGER CHECK (line>=0), | ||
| branch VARCHAR(250) DEFAULT 'main', |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't hardcode 'main' as the database default branch.
If the ingest path omits branch, this silently stores the wrong branch for repos using master, trunk, or any non-default branch alert. Leave it nullable, or populate the real branch upstream instead of synthesizing "main" in the schema.
Suggested change
- branch VARCHAR(250) DEFAULT 'main',
+ branch VARCHAR(250),📝 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
| branch VARCHAR(250) DEFAULT 'main', | |
| branch VARCHAR(250), |
🤖 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
`@backend/libs/persistence/src/main/resources/db/migration/V4__create_code_scan_alerts_table.sql`
at line 7, The schema for code scan alerts is hardcoding branch to "main" in the
migration, which can store the wrong value when ingest data omits branch. Remove
the default from the branch column in V4__create_code_scan_alerts_table.sql so
it stays nullable, and ensure the real branch is set earlier in the ingest flow
instead of being synthesized in the database schema.
Comment on lines
+23
to
+37
| public void processAndSave(VulnerabilityEntity vulnerabilityEntity) { | ||
|
|
||
| try { | ||
| if (vulnerabilityEntity == null) { | ||
| throw new IllegalArgumentException("VulnerabilityEntity cannot be null"); | ||
| } | ||
|
|
||
| Vulnerability vulnerability = new Vulnerability( | ||
| vulnerabilityEntity.getKev(), vulnerabilityEntity.getCvss(), vulnerabilityEntity.getSeverity()); | ||
| vulnerabilityEntity.setPriority(priorityCalculator.calculate(vulnerability)); | ||
| vulnerabilityRepository.save(vulnerabilityEntity); | ||
| log.info("Vulnerability processed successfully for ID: {}", vulnerabilityEntity.getId()); | ||
| } catch (Exception e) { | ||
| log.error("Failed to process vulnerability with ID: {}", vulnerabilityEntity.getId(), e); | ||
| throw new RuntimeException("Scanner processing error", e); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move input validation out of the try block.
PriorityCalculator assumes valid cvss/severity, so malformed input from VulnerabilityService.create(...) can fail before persistence. Because the null guard also lives inside this try, a null entity then hits the catch and vulnerabilityEntity.getId() throws a second NullPointerException, masking the real error.
Proposed fix
public void processAndSave(VulnerabilityEntity vulnerabilityEntity) {
-
- try {
- if (vulnerabilityEntity == null) {
- throw new IllegalArgumentException("VulnerabilityEntity cannot be null");
- }
+ if (vulnerabilityEntity == null) {
+ throw new IllegalArgumentException("VulnerabilityEntity cannot be null");
+ }
+ if (vulnerabilityEntity.getCvss() == null || vulnerabilityEntity.getSeverity() == null) {
+ throw new IllegalArgumentException("cvss and severity are required");
+ }
+ try {
Vulnerability vulnerability = new Vulnerability(
vulnerabilityEntity.getKev(), vulnerabilityEntity.getCvss(), vulnerabilityEntity.getSeverity());
vulnerabilityEntity.setPriority(priorityCalculator.calculate(vulnerability));
vulnerabilityRepository.save(vulnerabilityEntity);
log.info("Vulnerability processed successfully for ID: {}", vulnerabilityEntity.getId());
} catch (Exception e) {
log.error("Failed to process vulnerability with ID: {}", vulnerabilityEntity.getId(), e);
throw new RuntimeException("Scanner processing error", e);
}
}📝 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
| public void processAndSave(VulnerabilityEntity vulnerabilityEntity) { | |
| try { | |
| if (vulnerabilityEntity == null) { | |
| throw new IllegalArgumentException("VulnerabilityEntity cannot be null"); | |
| } | |
| Vulnerability vulnerability = new Vulnerability( | |
| vulnerabilityEntity.getKev(), vulnerabilityEntity.getCvss(), vulnerabilityEntity.getSeverity()); | |
| vulnerabilityEntity.setPriority(priorityCalculator.calculate(vulnerability)); | |
| vulnerabilityRepository.save(vulnerabilityEntity); | |
| log.info("Vulnerability processed successfully for ID: {}", vulnerabilityEntity.getId()); | |
| } catch (Exception e) { | |
| log.error("Failed to process vulnerability with ID: {}", vulnerabilityEntity.getId(), e); | |
| throw new RuntimeException("Scanner processing error", e); | |
| public void processAndSave(VulnerabilityEntity vulnerabilityEntity) { | |
| if (vulnerabilityEntity == null) { | |
| throw new IllegalArgumentException("VulnerabilityEntity cannot be null"); | |
| } | |
| if (vulnerabilityEntity.getCvss() == null || vulnerabilityEntity.getSeverity() == null) { | |
| throw new IllegalArgumentException("cvss and severity are required"); | |
| } | |
| try { | |
| Vulnerability vulnerability = new Vulnerability( | |
| vulnerabilityEntity.getKev(), vulnerabilityEntity.getCvss(), vulnerabilityEntity.getSeverity()); | |
| vulnerabilityEntity.setPriority(priorityCalculator.calculate(vulnerability)); | |
| vulnerabilityRepository.save(vulnerabilityEntity); | |
| log.info("Vulnerability processed successfully for ID: {}", vulnerabilityEntity.getId()); | |
| } catch (Exception e) { | |
| log.error("Failed to process vulnerability with ID: {}", vulnerabilityEntity.getId(), e); | |
| throw new RuntimeException("Scanner processing error", e); |
🤖 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
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java`
around lines 23 - 37, Move the null/input validation in
VulnerabilityScanner.processAndSave out of the try block so invalid inputs are
rejected before any persistence or priority calculation work starts. Keep the
null guard for VulnerabilityEntity at the top of the method, then run the
Vulnerability/priorityCalculator flow inside the try, and ensure the catch only
handles downstream failures. Also avoid calling vulnerabilityEntity.getId() in
the error path unless the entity is known non-null, so a null payload doesn’t
trigger a second exception and hide the original failure.
Comment on lines
+39
to
+53
| when(vulnerabilityRepository.findById(uuid)).thenReturn(Optional.of(vulnerabilityEntity)); | ||
|
|
||
| when(priorityCalculator.calculate(any())).thenReturn(Priority.URGENT); | ||
|
|
||
| // when | ||
| vulnerabilityScanner.processAndSave(vulnerabilityEntity); | ||
|
|
||
| // then | ||
| VulnerabilityEntity savedVulnerabilityEntity = | ||
| vulnerabilityRepository.findById(uuid).get(); | ||
| assertNotNull(savedVulnerabilityEntity.getPriority(), "Priority should not be null"); | ||
| assertEquals( | ||
| Priority.URGENT, | ||
| savedVulnerabilityEntity.getPriority(), | ||
| "Priority should be URGENT for high CVSS and KEV"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert against the saved call, not a stubbed findById(...).
VulnerabilityScanner.processAndSave(...) never reads from vulnerabilityRepository.findById(...), so this test can still pass if persistence is broken. Assert on vulnerabilityEntity directly and verify vulnerabilityRepository.save(vulnerabilityEntity) instead.
🤖 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
`@backend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java`
around lines 39 - 53, The test in VulnerabilityScannerTest is asserting against
a stubbed repository lookup instead of the actual save behavior. Update the
verification around VulnerabilityScanner.processAndSave(...) to assert directly
on vulnerabilityEntity after the call and verify that
vulnerabilityRepository.save(vulnerabilityEntity) was invoked. Keep the existing
priority expectations tied to the saved entity so the test validates persistence
rather than a mocked findById(...) path.
Comment on lines
+60
to
+67
| VulnerabilityEntity vulnerabilityEntity = new VulnerabilityEntity().setCvss(15.0); | ||
|
|
||
| when(priorityCalculator.calculate(any())).thenThrow(new IllegalArgumentException("Invalid CVSS")); | ||
|
|
||
| // then | ||
| assertThrows(RuntimeException.class, () -> { | ||
| vulnerabilityScanner.processAndSave(vulnerabilityEntity); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This does not actually test invalid CVSS handling.
The failure here is injected by mocking priorityCalculator.calculate(...) to throw, so the test only proves exception wrapping. If the intent is to validate CVSS bounds, use the real calculator for this case or rename the test to match what it really covers.
🤖 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
`@backend/libs/scanning/src/test/java/dev/cleat/scanning/VulnerabilityScannerTest.java`
around lines 60 - 67, The current test in VulnerabilityScannerTest only verifies
exception wrapping because priorityCalculator.calculate(...) is mocked to throw,
so it does not actually cover invalid CVSS handling. Update the test to either
use the real PriorityCalculator path with an out-of-range CVSS value in
VulnerabilityScanner.processAndSave, or rename the test/method to reflect that
it is testing downstream exception propagation rather than CVSS validation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java (1)
19-21: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid rescanning the entire table on every tick.
findAll()loads every vulnerability, andprocessAndSave(...)writes each entity back after recalculating priority. Once priorities are populated, this job turns into a full-table read/write every 10 minutes. Prefer paging plus a narrower query (for example, only rows with missing/stale priority) so the worker cost does not grow with table size.🤖 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 `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java` around lines 19 - 21, The scheduled scan in VulnerabilityWorker.runSecurityScan is doing a full-table read with vulnerabilityRepository.findAll() and then rewriting every row via vulnerabilityScanner.processAndSave, so it will keep rescanning everything on each tick. Update this job to use a narrower repository query and paging/batching, targeting only vulnerabilities with missing or stale priority instead of all rows, and keep the work bounded as the table grows.backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse or complete the severity branch.
Lines 22-27 ignore
severityand always setStatus.OPEN, so the conditional currently adds no behavior and can hide a missed mapping. Either remove the branch or implement the intended non-OPENpath.🤖 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 `@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java` around lines 22 - 27, The severity check in CodeScanAlertScanner is redundant because both branches in the status assignment set Status.OPEN. Update the logic in the scanner method that handles CodeScanAlertEntity so the Severity.CRITICAL/Severity.HIGH branch has distinct behavior, or collapse the conditional entirely if all severities should map to OPEN. Use the existing Severity and Status handling in CodeScanAlertScanner to keep the mapping explicit and avoid a no-op branch.Source: Linters/SAST tools
🤖 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 `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`:
- Around line 19-21: The batch scan in VulnerabilityWorker.runSecurityScan
currently stops on the first exception because
vulnerabilityScanner.processAndSave is called directly inside the forEach. Wrap
each per-row call in its own try/catch inside runSecurityScan so a failure for
one Vulnerability does not abort the scheduled run, and continue processing the
remaining entities while preserving the existing scheduler behavior.
---
Nitpick comments:
In `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`:
- Around line 19-21: The scheduled scan in VulnerabilityWorker.runSecurityScan
is doing a full-table read with vulnerabilityRepository.findAll() and then
rewriting every row via vulnerabilityScanner.processAndSave, so it will keep
rescanning everything on each tick. Update this job to use a narrower repository
query and paging/batching, targeting only vulnerabilities with missing or stale
priority instead of all rows, and keep the work bounded as the table grows.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.java`:
- Around line 22-27: The severity check in CodeScanAlertScanner is redundant
because both branches in the status assignment set Status.OPEN. Update the logic
in the scanner method that handles CodeScanAlertEntity so the
Severity.CRITICAL/Severity.HIGH branch has distinct behavior, or collapse the
conditional entirely if all severities should map to OPEN. Use the existing
Severity and Status handling in CodeScanAlertScanner to keep the mapping
explicit and avoid a no-op branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a27bb90a-2681-473c-9e0c-18c0cdc9fe76
📒 Files selected for processing (5)
backend/apps/api/src/test/java/dev/cleat/api/CleatApiTests.javabackend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/CodeScanAlertScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/SecretFindingScanner.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/libs/scanning/src/main/java/dev/cleat/scanning/VulnerabilityScanner.java
- backend/libs/scanning/src/main/java/dev/cleat/scanning/SecretFindingScanner.java
Comment on lines
+19
to
+21
| @Scheduled(fixedDelay = 600000) | ||
| public void runSecurityScan() { | ||
| vulnerabilityRepository.findAll().forEach(vulnerabilityScanner::processAndSave); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate per-row failures so one bad record doesn't stop the batch.
VulnerabilityScanner.processAndSave(...) rethrows on failure, so this forEach aborts on the first bad entity and skips the rest of the scheduled run. Catch around each item here and continue processing the remaining rows.
Suggested change
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
`@Component`
public class VulnerabilityWorker {
+ private static final Logger LOG = LoggerFactory.getLogger(VulnerabilityWorker.class);
private final VulnerabilityScanner vulnerabilityScanner;
private final VulnerabilityRepository vulnerabilityRepository;
@@
`@Scheduled`(fixedDelay = 600000)
public void runSecurityScan() {
- vulnerabilityRepository.findAll().forEach(vulnerabilityScanner::processAndSave);
+ vulnerabilityRepository.findAll().forEach(vulnerability -> {
+ try {
+ vulnerabilityScanner.processAndSave(vulnerability);
+ } catch (RuntimeException ex) {
+ LOG.error("Failed to process vulnerability {}", vulnerability.getId(), ex);
+ }
+ });
}
}📝 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
| @Scheduled(fixedDelay = 600000) | |
| public void runSecurityScan() { | |
| vulnerabilityRepository.findAll().forEach(vulnerabilityScanner::processAndSave); | |
| import org.slf4j.Logger; | |
| import org.slf4j.LoggerFactory; | |
| `@Component` | |
| public class VulnerabilityWorker { | |
| private static final Logger LOG = LoggerFactory.getLogger(VulnerabilityWorker.class); | |
| private final VulnerabilityScanner vulnerabilityScanner; | |
| private final VulnerabilityRepository vulnerabilityRepository; | |
| `@Scheduled`(fixedDelay = 600000) | |
| public void runSecurityScan() { | |
| vulnerabilityRepository.findAll().forEach(vulnerability -> { | |
| try { | |
| vulnerabilityScanner.processAndSave(vulnerability); | |
| } catch (RuntimeException ex) { | |
| LOG.error("Failed to process vulnerability {}", vulnerability.getId(), ex); | |
| } | |
| }); | |
| } | |
| } |
🤖 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 `@backend/apps/worker/src/main/java/dev/cleat/worker/VulnerabilityWorker.java`
around lines 19 - 21, The batch scan in VulnerabilityWorker.runSecurityScan
currently stops on the first exception because
vulnerabilityScanner.processAndSave is called directly inside the forEach. Wrap
each per-row call in its own try/catch inside runSecurityScan so a failure for
one Vulnerability does not abort the scheduled run, and continue processing the
remaining entities while preserving the existing scheduler behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@backend/apps/worker/src/test/java/dev/cleat/worker/CleatWorkerApplicationTests.java`:
- Around line 8-12: Disable background scheduling in CleatWorkerApplicationTests
so the smoke test does not trigger VulnerabilityWorker.runSecurityScan() or
other scheduled work; `@MockitoBean` on VulnerabilityScanner is not enough because
the scheduled path can still reach VulnerabilityRepository.findAll(). Update the
test setup around `@SpringBootTest/CleatWorkerApplicationTests` to either exclude
scheduling or load a narrower configuration so only the intended smoke-test
context starts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6088706b-686e-431c-8819-2282234b23ed
📒 Files selected for processing (1)
backend/apps/worker/src/test/java/dev/cleat/worker/CleatWorkerApplicationTests.java
…to fix context loading
…dundant exclude configurations
…-configuration for CI
…manage datasource
…tasource in test configuration
…at) to CleatWorkerApllicationTests(basePackages=dev.cleat)
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 does this change?
Related issue
Closes #8
Area
Screenshots
If this touches the UI, drop before and after screenshots here.
Checklist
bun run typecheck && bun run buildinapps/web(for frontend changes)Summary by CodeRabbit