feat: added consent notice and intent mapping logic#1584
Closed
tipusinghaw wants to merge 5 commits intomainfrom
Closed
feat: added consent notice and intent mapping logic#1584tipusinghaw wants to merge 5 commits intomainfrom
tipusinghaw wants to merge 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds an Intent Notice feature: DTOs and interfaces, DB model + migrations, ecosystem repository/service/controller CRUD (with URL validation and auth checks), API Gateway controller/service endpoints, and propagation of ecosystemId through oid4vc verification flow to attach noticeUrl to created sessions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_GW as API Gateway
participant OID4VC as OID4VC Service
participant Ecosystem as Ecosystem Service
participant DB as Database
Client->>API_GW: POST /oid4vc/createIntentBasedVerificationPresentation (intent, ecosystemId)
API_GW->>OID4VC: createIntentBasedVerificationPresentation(payload, ecosystemId)
OID4VC->>DB: create verification session
DB-->>OID4VC: createdSession{id}
OID4VC->>Ecosystem: getIntentNoticeByIntentId(intentId, orgId, ecosystemId)
Ecosystem->>DB: query intent_notices (intentId, orgId/ecosystem scope)
DB-->>Ecosystem: {noticeUrl}
Ecosystem-->>OID4VC: {noticeUrl}
OID4VC->>OID4VC: append noticeUrl?transactionId=createdSession.id
OID4VC-->>API_GW: {createdSession with noticeUrl}
API_GW-->>Client: 200 {createdSession, noticeUrl}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
Contributor
Author
|
review @coderabbitai |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
322-353:⚠️ Potential issue | 🟠 MajorKeep
ecosystemIdoptional at runtime too.The new pipe is missing
optional: true, so callers that omitecosystemIdnow get a 400 even though@ApiQuerymarks it optional and the downstream service acceptsundefined. That is a breaking API change for existing clients.🛠️ Suggested fix
`@ApiQuery`({ name: 'ecosystemId', required: false }) @@ `@Query`( 'ecosystemId', new ParseUUIDPipe({ + optional: true, exceptionFactory: (): Error => { throw new BadRequestException('Invalid ecosystem ID'); } }) ) - ecosystemId: string, + ecosystemId?: string,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts` around lines 322 - 353, The ParseUUIDPipe used for the ecosystemId query in createIntentBasedVerificationPresentation currently forces validation and causes a 400 when omitted; update the ParseUUIDPipe instantiation for the ecosystemId parameter to include optional: true in its options (i.e., new ParseUUIDPipe({ optional: true, exceptionFactory: ... })) so the query can be omitted at runtime and still pass undefined through to downstream logic.
🧹 Nitpick comments (1)
apps/ecosystem/src/ecosystem.helper.ts (1)
1-4: Remove commented-out helper implementationLine 1-4 is dead commented code. Please remove it (or restore as active code in the same PR with usage/tests) to avoid stale logic in the codebase.
🧹 Proposed cleanup
-// export function isOrgExistForUser(userDetails: any, orgId: string): boolean { -// const userOrgRoles = userDetails.userOrgRoles as { orgId: string | null }[]; -// return userOrgRoles.some((role) => role.orgId === orgId); -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 1 - 4, Remove the dead commented helper implementation for isOrgExistForUser: either delete the commented block entirely or restore it as an active function (export function isOrgExistForUser(userDetails: any, orgId: string): boolean { ... }) and include/update any usages and unit tests that validate its behavior; ensure the restored function uses userDetails.userOrgRoles as { orgId: string | null }[] and returns userOrgRoles.some(role => role.orgId === orgId) if reinstating.
🤖 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-gateway/src/ecosystem/ecosystem.service.ts`:
- Around line 256-258: The createIntentNotice method is sending the full Prisma
user object over NATS; change the payload to only include the caller id (use {
id: userDetails.id }) so downstream services receive only the user id. Apply the
same change to the other notice/intent methods in this file that currently
forward the full user object (the block around the methods near
createIntentNotice and the methods around lines 285-291), replacing userDetails
with an object containing only the id while leaving the call to
this.natsClient.sendNatsMessage(this.serviceProxy, ...) intact.
In `@apps/api-gateway/src/ecosystem/intent/intent.controller.ts`:
- Around line 636-667: The GET /notice endpoint (getIntentNotices) currently
allows listing all notices when intentId is omitted; restrict it to the caller's
ecosystem or remove it. Either (A) require an ecosystemId path/query and
validate via EcosystemRolesGuard (or change the route to
/ecosystem/:ecosystemId/notice) and pass that ecosystemId into
ecosystemService.getIntentNotices, or (B) derive the caller's ecosystem from the
authenticated principal (e.g., request.user.ecosystemId) inside getIntentNotices
and pass that ecosystemId to ecosystemService.getIntentNotices so the service
only returns notices for that ecosystem. Ensure the Roles/EcosystemRolesGuard
checks still apply and update the ApiQuery/ApiOperation docs accordingly.
- Around line 782-808: The deleteIntentNotice route currently calls
ecosystemService.deleteIntentNotice(id) without the caller identity; update the
controller method deleteIntentNotice to extract the authenticated actor (e.g.,
via `@Req`() req or existing `@User`() decorator) and pass the user id as an
additional argument to ecosystemService.deleteIntentNotice(id, actorId); also
update the ecosystemService.deleteIntentNotice signature/implementation to
accept and use the actorId for ownership checks before deleting. Ensure the
parameter name (id) and service method name (deleteIntentNotice) are preserved
so callers are easy to locate.
In `@apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts`:
- Around line 10-13: The create DTO's noticeUrl field currently allows any
string (noticeUrl with `@IsDefined` and `@IsString`) while the update DTO enforces a
valid URL; add URL validation to the create DTO by applying the same `@IsUrl`
decorator (in
apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts for
the noticeUrl property) so both CreateIntentNoticeDto and UpdateIntentNoticeDto
consistently enforce a valid URL value.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1769-1775: getIntentNoticeByIntentId currently builds an exact
where { intentId, orgId } which prevents returning global notices (orgId = null)
when an orgId is supplied; modify the logic in getIntentNoticeByIntentId to,
when orgId is provided, either query with an OR condition so
prisma.intent_notices.findFirst searches for intentId AND (orgId = provided OR
orgId = null) or first attempt the org-specific lookup and if that returns null
call a second findFirst for intentId with orgId = null so global notices are
returned as a fallback.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 1133-1170: Both updateIntentNotice and deleteIntentNotice must
load the intent notice first and validate the caller is the ecosystem lead
before mutating: fetch the notice (e.g., via a repository method like
getIntentNotice/findIntentNotice using the provided id), extract its
ecosystemId, call validateEcosystemLead(ecosystemId, userId) (or similar
existing auth helper) and only proceed to call
ecosystemRepository.updateIntentNotice/deleteIntentNotice using the notice id
(or the ecosystem-scoped call) after validation; if the notice is not found
return NOT_FOUND as before, and if validateEcosystemLead fails return/throw a
forbidden/unauthorized RpcException.
- Around line 1021-1044: The createIntentNotice flow currently skips validating
that a provided orgId belongs to the same ecosystem and is active; update
createIntentNotice to, after loading intent (intent = await
this.ecosystemRepository.findIntentById(intentId)) and before creating the
notice (ecosystemRepository.createIntentNotice), fetch the organisation by orgId
(e.g., await this.ecosystemRepository.findOrgById or similar), verify
org.ecosystemId === intent['ecosystemId'] and that the org is active (check
org.status === 'ACTIVE' or the project’s active flag), and if the check fails
throw an RpcException (HttpStatus.FORBIDDEN or CONFLICT) with a clear message;
keep existing validateEcosystemLead call but do not rely on it to validate org
membership.
In `@apps/oid4vc-verification/interfaces/intent-notice.interfaces.ts`:
- Around line 1-4: The CreateIntentNotice interface is missing the optional
orgId field; update the interface (CreateIntentNotice) to include orgId?: string
so it matches CreateIntentNoticeDto and existing usage across
controllers/services (ensure any code referencing CreateIntentNotice continues
to accept an undefined orgId).
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts`:
- Line 424: Remove the raw stdout dump of sensitive session data by deleting the
console.log('createdSession...') call; instead, use the service's structured
logger (e.g., this.logger or processLogger) to either omit logging entirely or
log only non-sensitive, explicitly selected fields (for example session id or a
redacted URL) from createdSession in the oid4vc-verification.service.ts method
where createdSession is created so you avoid emitting full session objects to
stdout.
- Around line 316-317: The method now accepts ecosystemId but does not use it to
scope lookups; update the template resolution and notice fetch to include
ecosystemId so intents/notifications are looked up per-ecosystem. Specifically,
when resolving the template (where the code currently passes { intentName,
verifierOrgId }) add ecosystemId to that payload, and when fetching the notice
(where it uses { intentId, orgId }) include ecosystemId as well (or otherwise
pass ecosystemId into the query/filter). Ensure all lookups that rely on
intentName/intentId are scoped by the ecosystemId parameter to disambiguate
multi-ecosystem orgs.
- Around line 419-420: The current assignment to createdSession.noticeUrl
concatenates intentNotice.noticeUrl with "?transactionId=" and
createdSession.verificationSession.id, which will break URLs that already
contain query parameters or fragments; update the code that sets
createdSession.noticeUrl to call the project's URL/query helper (rather than
string concatenation) to merge { transactionId:
createdSession.verificationSession.id } into intentNotice.noticeUrl while
preserving existing query params and fragments (locate code around
intentNotice.noticeUrl and createdSession.verificationSession.id and replace the
concatenation with the query builder call).
In `@libs/prisma-service/prisma/schema.prisma`:
- Around line 743-757: Add database-level uniqueness to enforce one notice per
scope by creating two partial unique indexes for the intent_notices table: a
unique index on (intentId, orgId) where orgId IS NOT NULL (e.g.,
intent_notices_unique_org_scope) and a unique index on (intentId) where orgId IS
NULL (e.g., intent_notices_unique_global_scope). Implement these as a migration
that targets the intent_notices model (fields intentId and orgId) so concurrent
inserts cannot create duplicate notices for the same intent+org or the global
scope.
- Around line 743-753: The intent_notices relation lacks an onDelete behavior
while deleteIntent() performs a hard delete, causing FK violations; update the
Prisma schema model intent_notices to define explicit cascade behavior for the
intent relation (e.g., add onDelete: Cascade to the relation clause on intent in
intent_notices) or alternatively make intentId nullable and use onDelete:
SetNull, then run the Prisma migrate to apply the change so deleteIntent() can
remove intents without breaking FK constraints.
---
Outside diff comments:
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`:
- Around line 322-353: The ParseUUIDPipe used for the ecosystemId query in
createIntentBasedVerificationPresentation currently forces validation and causes
a 400 when omitted; update the ParseUUIDPipe instantiation for the ecosystemId
parameter to include optional: true in its options (i.e., new ParseUUIDPipe({
optional: true, exceptionFactory: ... })) so the query can be omitted at runtime
and still pass undefined through to downstream logic.
---
Nitpick comments:
In `@apps/ecosystem/src/ecosystem.helper.ts`:
- Around line 1-4: Remove the dead commented helper implementation for
isOrgExistForUser: either delete the commented block entirely or restore it as
an active function (export function isOrgExistForUser(userDetails: any, orgId:
string): boolean { ... }) and include/update any usages and unit tests that
validate its behavior; ensure the restored function uses
userDetails.userOrgRoles as { orgId: string | null }[] and returns
userOrgRoles.some(role => role.orgId === orgId) if reinstating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b66ccef9-eb60-4135-961f-8c3411d07860
📒 Files selected for processing (16)
apps/api-gateway/src/ecosystem/ecosystem.service.tsapps/api-gateway/src/ecosystem/intent/intent.controller.tsapps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.tsapps/ecosystem/repositories/ecosystem.repository.tsapps/ecosystem/src/ecosystem.controller.tsapps/ecosystem/src/ecosystem.helper.tsapps/ecosystem/src/ecosystem.service.tsapps/oid4vc-verification/interfaces/intent-notice.interfaces.tsapps/oid4vc-verification/src/oid4vc-verification.controller.tsapps/oid4vc-verification/src/oid4vc-verification.service.tslibs/common/src/response-messages/index.tslibs/prisma-service/prisma/migrations/20260312082536_added_intent_notice_table/migration.sqllibs/prisma-service/prisma/migrations/20260312084927_add_org_id_to_intent_notices/migration.sqllibs/prisma-service/prisma/schema.prisma
Comment on lines
+10
to
+13
| @ApiProperty({ description: 'URL of the notice', example: 'https://example.com/notice' }) | ||
| @IsDefined() | ||
| @IsString() | ||
| noticeUrl: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current validation decorators and where CreateIntentNoticeDto is used.
rg -n -C2 'class CreateIntentNoticeDto|class UpdateIntentNoticeDto|noticeUrl|IsString|IsUrl' apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts
rg -n -C2 'CreateIntentNoticeDto|noticeUrl' apps/api-gateway/src/ecosystem apps/api-gateway/src/oid4vc-verificationRepository: credebl/platform
Length of output: 4690
Enforce URL validation consistently for noticeUrl
Line 10-13 allows any string on create, while line 22-25 requires a valid URL on update. This inconsistency permits invalid URLs to be persisted at creation time, failing validation only during updates.
🔧 Proposed fix
-import { IsDefined, IsOptional, IsString, IsUrl, IsUUID } from 'class-validator';
+import { IsDefined, IsOptional, IsUrl, IsUUID } from 'class-validator';
@@
`@ApiProperty`({ description: 'URL of the notice', example: 'https://example.com/notice' })
`@IsDefined`()
- `@IsString`()
+ `@IsUrl`()
noticeUrl: string;Also applies to: 22-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts`
around lines 10 - 13, The create DTO's noticeUrl field currently allows any
string (noticeUrl with `@IsDefined` and `@IsString`) while the update DTO enforces a
valid URL; add URL validation to the create DTO by applying the same `@IsUrl`
decorator (in
apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts for
the noticeUrl property) so both CreateIntentNoticeDto and UpdateIntentNoticeDto
consistently enforce a valid URL value.
Contributor
Author
There was a problem hiding this comment.
URL validation is implemented in service file
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the validateNoticeUrl helper implementation
rg -n -A 30 'validateNoticeUrl' apps/ecosystem/src/ecosystem.helper.ts
# Also check how it's called in the service
rg -n -B2 -A5 'validateNoticeUrl' apps/ecosystem/src/ecosystem.service.ts
Length of output: 1865
@tipusinghaw, understood — the service-level validateNoticeUrl in apps/ecosystem/src/ecosystem.helper.ts does perform more thorough validation (live HTTP reachability check), which is stronger than a simple format check via @IsUrl().
However, there is a bug in that helper worth fixing: the AbortController signal is never passed to fetch, so the 5-second timeout is completely ineffective — the fetch will hang indefinitely regardless.
- const response = await fetch(noticeUrl);
+ const response = await fetch(noticeUrl, { signal: controller.signal });Regarding the original comment, while DTO-level @IsUrl() on CreateIntentNoticeDto would still be beneficial for early rejection (before hitting the service) and accurate Swagger documentation, I acknowledge that the service-layer validation does exist. The decision is yours.
✏️ Learnings added
Learnt from: tipusinghaw
Repo: credebl/platform PR: 1584
File: apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts:10-13
Timestamp: 2026-03-18T12:02:15.511Z
Learning: In the credebl/platform repository, `noticeUrl` URL validation for intent notices is intentionally handled at the service layer via the `validateNoticeUrl` helper in `apps/ecosystem/src/ecosystem.helper.ts`, which performs a live HTTP reachability check, rather than purely at the DTO layer with `IsUrl()`. Do not flag the absence of `IsUrl()` on `noticeUrl` in `CreateIntentNoticeDto` as a missing validation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+1769
to
+1775
| async getIntentNoticeByIntentId(intentId: string, orgId?: string | null): Promise<object | null> { | ||
| try { | ||
| const where: { intentId: string; orgId?: string | null } = { intentId }; | ||
| if (orgId !== undefined) { | ||
| where.orgId = orgId; | ||
| } | ||
| return await this.prisma.intent_notices.findFirst({ where }); |
There was a problem hiding this comment.
Fall back to the global notice when no org-specific record exists.
Global notices are supported (orgId is nullable), but this query does an exact { intentId, orgId } match whenever an org is supplied. The verifier flow always supplies orgId, so ecosystem-wide notices with orgId = null are never returned.
🛠️ Suggested fix
async getIntentNoticeByIntentId(intentId: string, orgId?: string | null): Promise<object | null> {
try {
- const where: { intentId: string; orgId?: string | null } = { intentId };
- if (orgId !== undefined) {
- where.orgId = orgId;
- }
- return await this.prisma.intent_notices.findFirst({ where });
+ if (orgId) {
+ const scopedNotice = await this.prisma.intent_notices.findFirst({
+ where: { intentId, orgId }
+ });
+ if (scopedNotice) {
+ return scopedNotice;
+ }
+ }
+ return await this.prisma.intent_notices.findFirst({
+ where: { intentId, orgId: null }
+ });
} catch (error) {
this.logger.error(`getIntentNoticeByIntentId error: ${error}`);
throw error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ecosystem/repositories/ecosystem.repository.ts` around lines 1769 -
1775, getIntentNoticeByIntentId currently builds an exact where { intentId,
orgId } which prevents returning global notices (orgId = null) when an orgId is
supplied; modify the logic in getIntentNoticeByIntentId to, when orgId is
provided, either query with an OR condition so prisma.intent_notices.findFirst
searches for intentId AND (orgId = provided OR orgId = null) or first attempt
the org-specific lookup and if that returns null call a second findFirst for
intentId with orgId = null so global notices are returned as a fallback.
Comment on lines
+316
to
+317
| expectedOrigins?: string[], | ||
| ecosystemId?: string |
There was a problem hiding this comment.
Use ecosystemId to scope the lookup.
Line 317 adds ecosystemId, but this method still resolves the template with { intentName, verifierOrgId } and later fetches the notice with { intentId, orgId } only. Since intent names are only unique within an ecosystem, the new parameter does not actually disambiguate multi-ecosystem orgs yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` around lines 316
- 317, The method now accepts ecosystemId but does not use it to scope lookups;
update the template resolution and notice fetch to include ecosystemId so
intents/notifications are looked up per-ecosystem. Specifically, when resolving
the template (where the code currently passes { intentName, verifierOrgId }) add
ecosystemId to that payload, and when fetching the notice (where it uses {
intentId, orgId }) include ecosystemId as well (or otherwise pass ecosystemId
into the query/filter). Ensure all lookups that rely on intentName/intentId are
scoped by the ecosystemId parameter to disambiguate multi-ecosystem orgs.
Comment on lines
+419
to
+420
| if (intentNotice?.noticeUrl) { | ||
| createdSession.noticeUrl = `${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}`; |
There was a problem hiding this comment.
Build the notice URL with a query helper.
Hardcoding ?transactionId= will corrupt any notice URL that already has query params or a fragment. Reuse the existing query builder here.
🛠️ Suggested fix
- createdSession.noticeUrl = `${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}`;
+ createdSession.noticeUrl = buildUrlWithQuery(intentNotice.noticeUrl, {
+ transactionId: createdSession.verificationSession.id
+ });📝 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 (intentNotice?.noticeUrl) { | |
| createdSession.noticeUrl = `${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}`; | |
| if (intentNotice?.noticeUrl) { | |
| createdSession.noticeUrl = buildUrlWithQuery(intentNotice.noticeUrl, { | |
| transactionId: createdSession.verificationSession.id | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` around lines 419
- 420, The current assignment to createdSession.noticeUrl concatenates
intentNotice.noticeUrl with "?transactionId=" and
createdSession.verificationSession.id, which will break URLs that already
contain query parameters or fragments; update the code that sets
createdSession.noticeUrl to call the project's URL/query helper (rather than
string concatenation) to merge { transactionId:
createdSession.verificationSession.id } into intentNotice.noticeUrl while
preserving existing query params and fragments (locate code around
intentNotice.noticeUrl and createdSession.verificationSession.id and replace the
concatenation with the query builder call).
| } | ||
| } | ||
| } | ||
| console.log('createdSession:::::::::::::::::;;', createdSession); |
There was a problem hiding this comment.
Remove the raw session dump.
createdSession can contain verification URLs and session identifiers. Writing the whole object to stdout makes those values much harder to redact/control than the structured logger.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` at line 424,
Remove the raw stdout dump of sensitive session data by deleting the
console.log('createdSession...') call; instead, use the service's structured
logger (e.g., this.logger or processLogger) to either omit logging entirely or
log only non-sensitive, explicitly selected fields (for example session id or a
redacted URL) from createdSession in the oid4vc-verification.service.ts method
where createdSession is created so you avoid emitting full session objects to
stdout.
Comment on lines
+743
to
+757
| model intent_notices { | ||
| id String @id @default(uuid()) @db.Uuid | ||
| intentId String @db.Uuid | ||
| orgId String? @db.Uuid | ||
| noticeUrl String | ||
| createDateTime DateTime @default(now()) @db.Timestamptz(6) | ||
| createdBy String @db.Uuid | ||
| lastChangedDateTime DateTime @default(now()) @db.Timestamptz(6) | ||
| lastChangedBy String | ||
| intent intents @relation(fields: [intentId], references: [id]) | ||
| organisation organisation? @relation(fields: [orgId], references: [id]) | ||
|
|
||
| @@index([intentId]) | ||
| @@index([orgId]) | ||
| } |
There was a problem hiding this comment.
Enforce the one-notice-per-scope rule in the database.
The service already treats notice scope as unique, but this schema does not enforce it. Concurrent creates can race past the existence check and leave duplicate notices for the same intent/scope, and because orgId is nullable the global row needs its own unique guard too.
🔐 Suggested migration indexes
CREATE UNIQUE INDEX intent_notices_unique_org_scope
ON intent_notices ("intentId", "orgId")
WHERE "orgId" IS NOT NULL;
CREATE UNIQUE INDEX intent_notices_unique_global_scope
ON intent_notices ("intentId")
WHERE "orgId" IS NULL;Based on learnings, PostgreSQL NULL uniqueness behavior needs explicit DB safeguards when the scope column is nullable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/prisma-service/prisma/schema.prisma` around lines 743 - 757, Add
database-level uniqueness to enforce one notice per scope by creating two
partial unique indexes for the intent_notices table: a unique index on
(intentId, orgId) where orgId IS NOT NULL (e.g.,
intent_notices_unique_org_scope) and a unique index on (intentId) where orgId IS
NULL (e.g., intent_notices_unique_global_scope). Implement these as a migration
that targets the intent_notices model (fields intentId and orgId) so concurrent
inserts cannot create duplicate notices for the same intent+org or the global
scope.
Comment on lines
+743
to
+753
| model intent_notices { | ||
| id String @id @default(uuid()) @db.Uuid | ||
| intentId String @db.Uuid | ||
| orgId String? @db.Uuid | ||
| noticeUrl String | ||
| createDateTime DateTime @default(now()) @db.Timestamptz(6) | ||
| createdBy String @db.Uuid | ||
| lastChangedDateTime DateTime @default(now()) @db.Timestamptz(6) | ||
| lastChangedBy String | ||
| intent intents @relation(fields: [intentId], references: [id]) | ||
| organisation organisation? @relation(fields: [orgId], references: [id]) |
There was a problem hiding this comment.
Define what happens to notices when an intent is deleted.
This new required intentId FK has no explicit onDelete, but deleteIntent() still hard-deletes from intents. Once a notice exists, that path will fail on the FK unless notices are deleted or cascaded first.
🛠️ Suggested fix
- intent intents `@relation`(fields: [intentId], references: [id])
+ intent intents `@relation`(fields: [intentId], references: [id], onDelete: Cascade)📝 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
| model intent_notices { | |
| id String @id @default(uuid()) @db.Uuid | |
| intentId String @db.Uuid | |
| orgId String? @db.Uuid | |
| noticeUrl String | |
| createDateTime DateTime @default(now()) @db.Timestamptz(6) | |
| createdBy String @db.Uuid | |
| lastChangedDateTime DateTime @default(now()) @db.Timestamptz(6) | |
| lastChangedBy String | |
| intent intents @relation(fields: [intentId], references: [id]) | |
| organisation organisation? @relation(fields: [orgId], references: [id]) | |
| model intent_notices { | |
| id String `@id` `@default`(uuid()) `@db.Uuid` | |
| intentId String `@db.Uuid` | |
| orgId String? `@db.Uuid` | |
| noticeUrl String | |
| createDateTime DateTime `@default`(now()) `@db.Timestamptz`(6) | |
| createdBy String `@db.Uuid` | |
| lastChangedDateTime DateTime `@default`(now()) `@db.Timestamptz`(6) | |
| lastChangedBy String | |
| intent intents `@relation`(fields: [intentId], references: [id], onDelete: Cascade) | |
| organisation organisation? `@relation`(fields: [orgId], references: [id]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/prisma-service/prisma/schema.prisma` around lines 743 - 753, The
intent_notices relation lacks an onDelete behavior while deleteIntent() performs
a hard delete, causing FK violations; update the Prisma schema model
intent_notices to define explicit cascade behavior for the intent relation
(e.g., add onDelete: Cascade to the relation clause on intent in intent_notices)
or alternatively make intentId nullable and use onDelete: SetNull, then run the
Prisma migrate to apply the change so deleteIntent() can remove intents without
breaking FK constraints.
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
tipusinghaw
force-pushed
the
feat/consent-notice
branch
from
2a03dd1 to
f146548
Compare
tipusinghaw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
322-344:⚠️ Potential issue | 🟡 MinorDocument
verifierIdon this route as well.The handler requires both
verifierIdandecosystemIdas query parameters, but onlyecosystemIdis declared in the Swagger contract. Generated clients and manual API consumers will not seeverifierIdin the OpenAPI schema, potentially leading to runtime validation failures.📝 Suggested Swagger update
- `@ApiQuery`({ name: 'ecosystemId', required: true }) + `@ApiQuery`({ + name: 'verifierId', + required: true, + type: String, + description: 'UUID of the verifier' + }) + `@ApiQuery`({ + name: 'ecosystemId', + required: true, + type: String, + description: 'UUID of the ecosystem' + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts` around lines 322 - 344, The route handler createIntentBasedVerificationPresentation documents only ecosystemId for Swagger; add an ApiQuery decorator for verifierId (matching the existing ecosystemId usage) so OpenAPI shows verifierId as a required query parameter; update the decorator to mark verifierId required and include a brief description to match the route parameter name verifierId used in the method signature and ParseUUIDPipe validation.
♻️ Duplicate comments (8)
apps/oid4vc-verification/src/oid4vc-verification.service.ts (3)
420-420:⚠️ Potential issue | 🟡 MinorBuild
noticeUrlwith the query helper, not string concat.Directly appending
?transactionId=breaks URLs that already contain query params or fragments.🛠️ Proposed fix
- createdSession.noticeUrl = `${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}`; + createdSession.noticeUrl = buildUrlWithQuery(intentNotice.noticeUrl, { + transactionId: createdSession.verificationSession.id + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` at line 420, The current assignment to createdSession.noticeUrl uses string concatenation which breaks when intentNotice.noticeUrl already has query params/fragments; instead parse intentNotice.noticeUrl with a URL/URLSearchParams helper, append or set the "transactionId" param using the URL.searchParams API (or equivalent helper) and then assign the resulting URL string to createdSession.noticeUrl; update the code around createdSession.noticeUrl = `${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}` to build the URL safely using the URL/URLSearchParams methods.
424-424:⚠️ Potential issue | 🟠 MajorRemove raw
createdSessionconsole dump.This can leak sensitive session identifiers/URLs to stdout. Use structured logger with explicitly selected non-sensitive fields (or remove the log).
🛠️ Proposed fix
- console.log('createdSession:::::::::::::::::;;', createdSession); + this.logger.debug( + `[createIntentBasedVerificationPresentation] session created with id=${createdSession?.verificationSession?.id}` + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` at line 424, Remove the raw console.log line that prints createdSession; instead either delete the log or replace it with a structured logger call (e.g., this.logger.debug or appLogger) that only emits explicitly safe fields from createdSession (for example status, type, and a truncated/hashed session id if needed) and never prints full session identifiers, URLs, or tokens; locate the usage of createdSession in oid4vc-verification.service.ts and update the logging there (or remove) to follow this policy.
413-416:⚠️ Potential issue | 🟠 MajorScope intent notice lookup by
ecosystemIdtoo.Notice retrieval is still queried without
ecosystemId, which can bypass the ecosystem boundary you added for template resolution.🛠️ Proposed fix
const intentNotice: any = await this.natsClient .sendNatsMessage(this.oid4vpVerificationServiceProxy, 'get-intent-notice-by-intent-id', { intentId, - orgId + orgId, + ecosystemId }) .catch(() => null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` around lines 413 - 416, The NATS request to fetch the intent notice is missing the ecosystem boundary; update the payload sent in the sendNatsMessage call (the invocation using this.oid4vpVerificationServiceProxy and subject 'get-intent-notice-by-intent-id') to include ecosystemId along with intentId and orgId so the lookup is scoped to the correct ecosystem (pass ecosystemId from the surrounding context/parameters).apps/api-gateway/src/ecosystem/ecosystem.service.ts (2)
256-259:⚠️ Potential issue | 🟠 MajorOnly send the caller id over NATS.
These notice commands forward the full Prisma
userobject even though the downstream service only readsid. This unnecessarily fans out email/client fields across the bus. This file already uses{ id: user.id }forcreateIntentTemplate(line 136).🛠️ Suggested fix
async createIntentNotice(createIntentNoticeDto: CreateIntentNoticeDto, userDetails: user): Promise<object> { - const payload = { createIntentNoticeDto, userDetails }; + const payload = { createIntentNoticeDto, userDetails: { id: userDetails.id } }; return this.natsClient.sendNatsMessage(this.serviceProxy, 'create-intent-notice', payload); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/ecosystem/ecosystem.service.ts` around lines 256 - 259, The createIntentNotice function is sending the full Prisma user object over NATS; change the payload to include only the caller id to avoid leaking user fields. In createIntentNotice, replace passing userDetails with a minimal object like { caller: { id: userDetails.id } } (or match the shape used by createIntentTemplate) before calling this.natsClient.sendNatsMessage(this.serviceProxy, 'create-intent-notice', payload) so only the user id is forwarded.
281-288:⚠️ Potential issue | 🟠 MajorApply the same fix to updateIntentNotice.
Consistent with the pattern used elsewhere in this file, only send the user id.
🛠️ Suggested fix
async updateIntentNotice( id: string, updateIntentNoticeDto: UpdateIntentNoticeDto, userDetails: user ): Promise<object> { - const payload = { id, updateIntentNoticeDto, userDetails }; + const payload = { id, updateIntentNoticeDto, userDetails: { id: userDetails.id } }; return this.natsClient.sendNatsMessage(this.serviceProxy, 'update-intent-notice', payload); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/ecosystem/ecosystem.service.ts` around lines 281 - 288, The updateIntentNotice function currently sends the entire userDetails in the payload to nats; change it to send only the user id to match other methods. In updateIntentNotice build the payload with id, updateIntentNoticeDto, and the user's id (e.g., userId: userDetails.id) and pass that into this.natsClient.sendNatsMessage(this.serviceProxy, 'update-intent-notice', payload) so the NATS message contains only the user id.apps/ecosystem/src/ecosystem.service.ts (2)
1131-1149:⚠️ Potential issue | 🔴 CriticalMissing ecosystem lead authorization before update.
updateIntentNoticemutates the notice by raw id without verifying the caller is the ecosystem lead. Once someone has a notice UUID, nothing prevents unauthorized updates. Load the notice first to retrieve its ecosystem context, then callvalidateEcosystemLead.🛠️ Suggested fix
async updateIntentNotice(id: string, noticeUrl: string, userId: string): Promise<object> { try { + // Load notice to get ecosystem context for authorization + const notices = await this.ecosystemRepository.getIntentNotices(id); + if (!notices || notices.length === 0) { + throw new RpcException({ + statusCode: HttpStatus.NOT_FOUND, + message: ResponseMessages.intentNotice.error.notFound + }); + } + const notice = notices[0] as { intent?: { ecosystemId?: string } }; + if (notice.intent?.ecosystemId) { + await this.validateEcosystemLead(userId, notice.intent.ecosystemId); + } + + await validateNoticeUrl(noticeUrl); + const updated = await this.ecosystemRepository.updateIntentNotice(id, noticeUrl, userId); - if (!updated) { - throw new RpcException({ - statusCode: HttpStatus.NOT_FOUND, - message: ResponseMessages.intentNotice.error.notFound - }); - } return updated; } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 1131 - 1149, updateIntentNotice currently updates by id without checking that the caller is the ecosystem lead; load the existing notice first (e.g., call a repository method to fetch the notice by id), extract its ecosystem identifier, call validateEcosystemLead(ecosystemId, userId) to authorize the caller, and only then call this.ecosystemRepository.updateIntentNotice(id, noticeUrl, userId); keep existing error handling and throw the same RpcException if validateEcosystemLead fails or the notice is not found.
1151-1169:⚠️ Potential issue | 🔴 CriticalMissing ecosystem lead authorization before delete.
deleteIntentNoticedeletes by raw id without verifying the caller is the ecosystem lead. Apply the same authorization pattern as suggested forupdateIntentNotice.🛠️ Suggested fix
async deleteIntentNotice(id: string): Promise<object> { try { + // Load notice to get ecosystem context for authorization + const notices = await this.ecosystemRepository.getIntentNotices(id); + if (!notices || notices.length === 0) { + throw new RpcException({ + statusCode: HttpStatus.NOT_FOUND, + message: ResponseMessages.intentNotice.error.notFound + }); + } + // Note: userId needs to be passed to this method for proper authorization + // const notice = notices[0] as { intent?: { ecosystemId?: string } }; + // if (notice.intent?.ecosystemId) { + // await this.validateEcosystemLead(userId, notice.intent.ecosystemId); + // } + const deleted = await this.ecosystemRepository.deleteIntentNotice(id); - if (!deleted) { - throw new RpcException({ - statusCode: HttpStatus.NOT_FOUND, - message: ResponseMessages.intentNotice.error.notFound - }); - } return deleted; } catch (error) {Note: The
deleteIntentNoticemethod signature needs to acceptuserIdto perform authorization. This requires updating the controller and gateway service as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 1151 - 1169, deleteIntentNotice currently deletes by id without verifying the caller is the ecosystem lead; update its signature to accept userId (matching the pattern used in updateIntentNotice), fetch the intent notice first via this.ecosystemRepository.getIntentNotice/getIntentNoticeById (or whichever read method exists), verify the caller is the ecosystem lead (e.g. compare intent.ecosystemLeadId or call a helper like this.ecosystemRepository.isEcosystemLead(userId, intent.ecosystemId)), and only then call this.ecosystemRepository.deleteIntentNotice(id); update the controller and gateway service to pass userId through to deleteIntentNotice and preserve existing error handling and RpcException behavior.apps/ecosystem/repositories/ecosystem.repository.ts (1)
1773-1784:⚠️ Potential issue | 🟠 MajorFall back to the global notice when no org-specific record exists.
The current implementation does an exact
{ intentId, orgId }match whenorgIdis supplied. If the verifier flow always suppliesorgId, ecosystem-wide notices withorgId = nullwill never be returned. Consider first querying for the org-specific notice, and if none exists, falling back to the global (orgId = null) notice.🛠️ Suggested fix
async getIntentNoticeByIntentId(intentId: string, orgId?: string | null): Promise<object | null> { try { - const where: { intentId: string; orgId?: string | null } = { intentId }; - if (orgId !== undefined) { - where.orgId = orgId; + if (orgId) { + const scopedNotice = await this.prisma.intent_notices.findFirst({ + where: { intentId, orgId } + }); + if (scopedNotice) { + return scopedNotice; + } } - return await this.prisma.intent_notices.findFirst({ where }); + // Fall back to global notice + return await this.prisma.intent_notices.findFirst({ + where: { intentId, orgId: null } + }); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/repositories/ecosystem.repository.ts` around lines 1773 - 1784, getIntentNoticeByIntentId currently does an exact match including orgId so when orgId is provided it never returns a global notice (orgId = null); change the logic to first query intent_notices for { intentId, orgId } when orgId is defined using the existing this.prisma.intent_notices.findFirst call (function: getIntentNoticeByIntentId), and if that returns null then perform a second query for the global notice { intentId, orgId: null } and return that result; if orgId is undefined keep the single query behavior. Ensure error handling/logging around both queries remains unchanged.
🧹 Nitpick comments (3)
apps/ecosystem/src/ecosystem.helper.ts (2)
4-10: Use optional chaining as suggested by static analysis.The condition can be simplified using optional chaining for better readability.
♻️ Suggested fix
export async function validateNoticeUrl(noticeUrl: string): Promise<void> { - if (!noticeUrl || !noticeUrl.trim()) { + if (!noticeUrl?.trim()) { throw new RpcException({ statusCode: HttpStatus.BAD_REQUEST, message: 'noticeUrl must not be empty.' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 4 - 10, The validation in validateNoticeUrl currently checks both falsy and trimmed-empty via separate conditions; simplify it using optional chaining by replacing the if-check with a single condition using noticeUrl?.trim() (e.g., if (!noticeUrl?.trim()) ) so empty, null or undefined values are handled concisely while keeping the existing RpcException (statusCode: HttpStatus.BAD_REQUEST, message: 'noticeUrl must not be empty.') unchanged.
11-21: Consider that some URLs may not support HEAD requests.Some servers and CDNs respond differently to
HEADvsGETrequests, or may not supportHEADat all. If false negatives become an issue, consider falling back to aGETrequest with a small byte range or simply accepting URLs without reachability validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 11 - 21, The current reachability check uses fetch(noticeUrl, { method: 'HEAD', signal: controller.signal }) which can produce false negatives for servers that don't support HEAD; update the logic around the AbortController/timeout and fetch call so that if the HEAD request fails with a network/error response or returns status 405/501 or throws, you perform a fallback GET (e.g., fetch(noticeUrl, { method: 'GET', signal: controller.signal }) or a ranged GET) before throwing the RpcException with HttpStatus.BAD_REQUEST; ensure you still clear the timeout and propagate the original error info into the RpcException message when ultimately rejecting.apps/ecosystem/repositories/ecosystem.repository.ts (1)
1827-1854: Prefer dot notation over bracket notation for known properties.
record['id']can be simplified torecord.idsinceidis a known property of the Prisma model. This improves readability and type safety.♻️ Suggested fix
if (!record) { return null; } return await this.prisma.intent_notices.update({ - where: { id: record['id'] }, + where: { id: record.id }, data: { noticeUrl, lastChangedBy: userId } }); @@ if (!record) { return null; } - return await this.prisma.intent_notices.delete({ where: { id: record['id'] } }); + return await this.prisma.intent_notices.delete({ where: { id: record.id } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/repositories/ecosystem.repository.ts` around lines 1827 - 1854, In updateIntentNotice and deleteIntentNotice replace bracket access record['id'] with dot notation record.id to improve readability and type safety; update the prisma calls that currently use where: { id: record['id'] } to use where: { id: record.id } (and similarly for any other known properties accessed via brackets) while keeping existing try/catch and logging unchanged.
🤖 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-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`:
- Around line 345-367: The controller currently passes an ecosystemId into
createIntentBasedVerificationPresentation without ensuring the caller's org is a
member of that ecosystem; before calling
oid4vcVerificationService.createIntentBasedVerificationPresentation (or inside
that service) add a membership validation that verifies the caller's
orgId/verifierOrgId exists in the ecosystem_orgs for the provided ecosystemId.
Either (a) perform an explicit check using the EcosystemRepository to query
ecosystem_orgs for a row matching ecosystemId + verifierOrgId and throw
BadRequest/Forbidden if missing, or (b) modify the template lookup in
EcosystemRepository (the template query that filters by intent.ecosystemId and
template.orgId) to also join/validate ecosystem_orgs contains the verifierOrgId
for that ecosystemId so templates from other ecosystems cannot be returned.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1786-1796: The intentNoticeExists + createIntentNotice flow is
vulnerable to a TOCTOU race leading to duplicate (intentId, orgId) rows in the
intent_notices table; either add a DB-level unique constraint on (intentId,
orgId) in the intent_notices model or change the create flow to use a
transactional-safe operation (e.g., a SERIALIZABLE transaction, row-level lock,
or a single upsert/insert ... on conflict) so createIntentNotice cannot insert a
duplicate when intentNoticeExists returns false. Update the Prisma schema/model
(intent_notices) to include the unique index or change the repository method
that performs insertion to use a transactional upsert or lock to enforce
uniqueness atomically.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Line 1131: The updateIntentNotice function is missing URL validation before
saving; mirror createIntentNotice by calling validateNoticeUrl(noticeUrl) at the
start of updateIntentNotice and handle failures (throw or return a descriptive
error) before persisting the new notice URL; ensure you only proceed to the
persistence/update logic in updateIntentNotice after validateNoticeUrl succeeds
so invalid or unreachable URLs cannot be saved.
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts`:
- Around line 316-317: The method signature in oid4vc-verification.service.ts
currently declares ecosystemId as optional (expectedOrigins?: string[],
ecosystemId?: string); change this to make ecosystemId required
(expectedOrigins?: string[], ecosystemId: string), add a defensive runtime check
at the start of the same function to throw an informative error if ecosystemId
is falsy for intent-based resolution, and update all callers to pass a valid
ecosystemId (or propagate one from context) so intent paths cannot proceed
without it; reference the parameter name ecosystemId when editing call sites and
adjust any related types/interfaces that previously allowed undefined.
---
Outside diff comments:
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`:
- Around line 322-344: The route handler
createIntentBasedVerificationPresentation documents only ecosystemId for
Swagger; add an ApiQuery decorator for verifierId (matching the existing
ecosystemId usage) so OpenAPI shows verifierId as a required query parameter;
update the decorator to mark verifierId required and include a brief description
to match the route parameter name verifierId used in the method signature and
ParseUUIDPipe validation.
---
Duplicate comments:
In `@apps/api-gateway/src/ecosystem/ecosystem.service.ts`:
- Around line 256-259: The createIntentNotice function is sending the full
Prisma user object over NATS; change the payload to include only the caller id
to avoid leaking user fields. In createIntentNotice, replace passing userDetails
with a minimal object like { caller: { id: userDetails.id } } (or match the
shape used by createIntentTemplate) before calling
this.natsClient.sendNatsMessage(this.serviceProxy, 'create-intent-notice',
payload) so only the user id is forwarded.
- Around line 281-288: The updateIntentNotice function currently sends the
entire userDetails in the payload to nats; change it to send only the user id to
match other methods. In updateIntentNotice build the payload with id,
updateIntentNoticeDto, and the user's id (e.g., userId: userDetails.id) and pass
that into this.natsClient.sendNatsMessage(this.serviceProxy,
'update-intent-notice', payload) so the NATS message contains only the user id.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1773-1784: getIntentNoticeByIntentId currently does an exact match
including orgId so when orgId is provided it never returns a global notice
(orgId = null); change the logic to first query intent_notices for { intentId,
orgId } when orgId is defined using the existing
this.prisma.intent_notices.findFirst call (function: getIntentNoticeByIntentId),
and if that returns null then perform a second query for the global notice {
intentId, orgId: null } and return that result; if orgId is undefined keep the
single query behavior. Ensure error handling/logging around both queries remains
unchanged.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 1131-1149: updateIntentNotice currently updates by id without
checking that the caller is the ecosystem lead; load the existing notice first
(e.g., call a repository method to fetch the notice by id), extract its
ecosystem identifier, call validateEcosystemLead(ecosystemId, userId) to
authorize the caller, and only then call
this.ecosystemRepository.updateIntentNotice(id, noticeUrl, userId); keep
existing error handling and throw the same RpcException if validateEcosystemLead
fails or the notice is not found.
- Around line 1151-1169: deleteIntentNotice currently deletes by id without
verifying the caller is the ecosystem lead; update its signature to accept
userId (matching the pattern used in updateIntentNotice), fetch the intent
notice first via this.ecosystemRepository.getIntentNotice/getIntentNoticeById
(or whichever read method exists), verify the caller is the ecosystem lead (e.g.
compare intent.ecosystemLeadId or call a helper like
this.ecosystemRepository.isEcosystemLead(userId, intent.ecosystemId)), and only
then call this.ecosystemRepository.deleteIntentNotice(id); update the controller
and gateway service to pass userId through to deleteIntentNotice and preserve
existing error handling and RpcException behavior.
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts`:
- Line 420: The current assignment to createdSession.noticeUrl uses string
concatenation which breaks when intentNotice.noticeUrl already has query
params/fragments; instead parse intentNotice.noticeUrl with a
URL/URLSearchParams helper, append or set the "transactionId" param using the
URL.searchParams API (or equivalent helper) and then assign the resulting URL
string to createdSession.noticeUrl; update the code around
createdSession.noticeUrl =
`${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}`
to build the URL safely using the URL/URLSearchParams methods.
- Line 424: Remove the raw console.log line that prints createdSession; instead
either delete the log or replace it with a structured logger call (e.g.,
this.logger.debug or appLogger) that only emits explicitly safe fields from
createdSession (for example status, type, and a truncated/hashed session id if
needed) and never prints full session identifiers, URLs, or tokens; locate the
usage of createdSession in oid4vc-verification.service.ts and update the logging
there (or remove) to follow this policy.
- Around line 413-416: The NATS request to fetch the intent notice is missing
the ecosystem boundary; update the payload sent in the sendNatsMessage call (the
invocation using this.oid4vpVerificationServiceProxy and subject
'get-intent-notice-by-intent-id') to include ecosystemId along with intentId and
orgId so the lookup is scoped to the correct ecosystem (pass ecosystemId from
the surrounding context/parameters).
---
Nitpick comments:
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1827-1854: In updateIntentNotice and deleteIntentNotice replace
bracket access record['id'] with dot notation record.id to improve readability
and type safety; update the prisma calls that currently use where: { id:
record['id'] } to use where: { id: record.id } (and similarly for any other
known properties accessed via brackets) while keeping existing try/catch and
logging unchanged.
In `@apps/ecosystem/src/ecosystem.helper.ts`:
- Around line 4-10: The validation in validateNoticeUrl currently checks both
falsy and trimmed-empty via separate conditions; simplify it using optional
chaining by replacing the if-check with a single condition using
noticeUrl?.trim() (e.g., if (!noticeUrl?.trim()) ) so empty, null or undefined
values are handled concisely while keeping the existing RpcException
(statusCode: HttpStatus.BAD_REQUEST, message: 'noticeUrl must not be empty.')
unchanged.
- Around line 11-21: The current reachability check uses fetch(noticeUrl, {
method: 'HEAD', signal: controller.signal }) which can produce false negatives
for servers that don't support HEAD; update the logic around the
AbortController/timeout and fetch call so that if the HEAD request fails with a
network/error response or returns status 405/501 or throws, you perform a
fallback GET (e.g., fetch(noticeUrl, { method: 'GET', signal: controller.signal
}) or a ranged GET) before throwing the RpcException with
HttpStatus.BAD_REQUEST; ensure you still clear the timeout and propagate the
original error info into the RpcException message when ultimately rejecting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b88c922-9423-4e95-b7ec-bf140764c67c
📒 Files selected for processing (10)
apps/api-gateway/src/ecosystem/ecosystem.service.tsapps/api-gateway/src/ecosystem/intent/intent.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.tsapps/ecosystem/repositories/ecosystem.repository.tsapps/ecosystem/src/ecosystem.controller.tsapps/ecosystem/src/ecosystem.helper.tsapps/ecosystem/src/ecosystem.service.tsapps/oid4vc-verification/src/oid4vc-verification.controller.tsapps/oid4vc-verification/src/oid4vc-verification.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api-gateway/src/ecosystem/intent/intent.controller.ts
- apps/oid4vc-verification/src/oid4vc-verification.controller.ts
- apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts
Comment on lines
+345
to
+367
| @Query( | ||
| 'ecosystemId', | ||
| new ParseUUIDPipe({ | ||
| exceptionFactory: (): Error => { | ||
| throw new BadRequestException('Invalid ecosystem ID'); | ||
| } | ||
| }) | ||
| ) | ||
| ecosystemId: string, | ||
| @User() user: user, | ||
| @Body() createIntentDto: CreateIntentBasedVerificationDto, | ||
| @Res() res: Response | ||
| ): Promise<Response> { | ||
| this.logger.debug( | ||
| `[createIntentBasedVerificationPresentation] Called with orgId=${orgId}, verifierId=${verifierId}, intent=${createIntentDto?.intent}, user=${user.id}` | ||
| `[createIntentBasedVerificationPresentation] Called with orgId=${orgId}, verifierId=${verifierId}, intent=${createIntentDto?.intent}, ecosystemId=${ecosystemId}, user=${user.id}` | ||
| ); | ||
|
|
||
| const presentation = await this.oid4vcVerificationService.createIntentBasedVerificationPresentation( | ||
| orgId, | ||
| verifierId, | ||
| createIntentDto, | ||
| user | ||
| user, | ||
| ecosystemId |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
echo "== createIntentBasedVerificationPresentation implementations =="
rg -n -C8 '\bcreateIntentBasedVerificationPresentation\s*\(' \
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts \
apps/oid4vc-verification/src/oid4vc-verification.service.ts || true
echo
echo "== org/ecosystem scoping around downstream lookups =="
rg -n -C6 '\becosystemId\b|\borgId\b|findUnique|findFirst|where:' \
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts \
apps/oid4vc-verification/src/oid4vc-verification.service.ts \
apps/ecosystem/src/ecosystem.service.ts \
apps/ecosystem/repositories/ecosystem.repository.ts || trueRepository: credebl/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -u
# Read the api-gateway service implementation
echo "=== API-Gateway Oid4vcVerificationService.createIntentBasedVerificationPresentation ==="
sed -n '24,80p' apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts
echo
echo "=== Check for org/ecosystem validation in the service before intent operations ==="
rg -n -B5 -A10 'oid4vpProxy\.|ecosystemId|orgId' \
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts \
| head -80Repository: credebl/platform
Length of output: 5978
🏁 Script executed:
#!/bin/bash
set -u
# Read the core oid4vc-verification service around the createIntentBasedVerificationPresentation method
echo "=== Core Service Implementation (lines 309-400) ==="
wc -l apps/oid4vc-verification/src/oid4vc-verification.service.ts
sed -n '309,400p' apps/oid4vc-verification/src/oid4vc-verification.service.tsRepository: credebl/platform
Length of output: 3898
🏁 Script executed:
#!/bin/bash
set -u
echo "=== Search for 'get-intent-template-by-intent-and-org' handler ==="
rg -n 'get-intent-template-by-intent-and-org' --type ts
echo
echo "=== Search for intent template fetching logic with orgId scoping ==="
rg -n -B5 -A15 'intentName|intent.*template.*orgId' \
apps/ecosystem/src/ecosystem.service.ts \
apps/oid4vc-verification/src/oid4vc-verification.service.ts \
2>/dev/null | head -100Repository: credebl/platform
Length of output: 5909
🏁 Script executed:
#!/bin/bash
set -u
echo "=== Repository implementation of getIntentTemplateByIntentAndOrg ==="
rg -n -A30 'async getIntentTemplateByIntentAndOrg' \
apps/ecosystem/repositories/ecosystem.repository.tsRepository: credebl/platform
Length of output: 1279
🏁 Script executed:
#!/bin/bash
set -u
echo "=== Ecosystem controller handler for get-intent-template-by-intent-and-org ==="
sed -n '237,255p' apps/ecosystem/src/ecosystem.controller.tsRepository: credebl/platform
Length of output: 698
Verify ecosystemId is scoped to the caller's org membership.
The downstream template lookup (apps/ecosystem/repositories/ecosystem.repository.ts, line 820-824) does not validate that the supplied ecosystemId corresponds to an ecosystem the caller's orgId belongs to. The repository query constrains by intent.ecosystemId and template orgId, but lacks an ecosystem_orgs membership check. An organization owner can supply an arbitrary ecosystemId from a different ecosystem and retrieve templates if their org ID matches the template's org ID or if the template is shared (orgId: null).
Add a check confirming verifierOrgId exists in ecosystem_orgs for the given ecosystemId before the template lookup, or constrain the repository query to validate the org-ecosystem relationship.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`
around lines 345 - 367, The controller currently passes an ecosystemId into
createIntentBasedVerificationPresentation without ensuring the caller's org is a
member of that ecosystem; before calling
oid4vcVerificationService.createIntentBasedVerificationPresentation (or inside
that service) add a membership validation that verifies the caller's
orgId/verifierOrgId exists in the ecosystem_orgs for the provided ecosystemId.
Either (a) perform an explicit check using the EcosystemRepository to query
ecosystem_orgs for a row matching ecosystemId + verifierOrgId and throw
BadRequest/Forbidden if missing, or (b) modify the template lookup in
EcosystemRepository (the template query that filters by intent.ecosystemId and
template.orgId) to also join/validate ecosystem_orgs contains the verifierOrgId
for that ecosystemId so templates from other ecosystems cannot be returned.
Comment on lines
+1786
to
+1796
| async intentNoticeExists(intentId: string, orgId: string | null): Promise<boolean> { | ||
| try { | ||
| const record = await this.prisma.intent_notices.findFirst({ | ||
| where: { intentId, orgId: orgId ?? null } | ||
| }); | ||
| return Boolean(record); | ||
| } catch (error) { | ||
| this.logger.error(`intentNoticeSlotExists error: ${error}`); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if a unique constraint exists on (intentId, orgId) in the schema
rg -n '@@unique.*intentId.*orgId|@@unique.*orgId.*intentId' libs/prisma-service/prisma/schema.prismaRepository: credebl/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Find the intent_notices model definition in the Prisma schema
rg -A 20 'model intent_notices' libs/prisma-service/prisma/schema.prismaRepository: credebl/platform
Length of output: 861
🏁 Script executed:
#!/bin/bash
# Locate and examine createIntentNotice method
rg -A 15 'createIntentNotice' apps/ecosystem/repositories/ecosystem.repository.tsRepository: credebl/platform
Length of output: 1011
🏁 Script executed:
#!/bin/bash
# Search for usage patterns: intentNoticeExists followed by createIntentNotice
rg -B 3 -A 3 'intentNoticeExists.*createIntentNotice|createIntentNotice.*intentNoticeExists' apps/Repository: credebl/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Find all calls to intentNoticeExists
rg -n 'intentNoticeExists' apps/ --type tsRepository: credebl/platform
Length of output: 354
🏁 Script executed:
#!/bin/bash
# Find all calls to createIntentNotice
rg -n 'createIntentNotice' apps/ --type tsRepository: credebl/platform
Length of output: 2127
🏁 Script executed:
#!/bin/bash
# Check if there's any transaction wrapper or error handling around these operations
rg -B 5 -A 5 'intentNoticeExists|createIntentNotice' apps/ecosystem/services/ --type tsRepository: credebl/platform
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Examine the service method that uses both intentNoticeExists and createIntentNotice
sed -n '1030,1070p' apps/ecosystem/src/ecosystem.service.tsRepository: credebl/platform
Length of output: 1775
Race condition: duplicate notices can be created under concurrency.
The intentNoticeExists check at line 1054 followed by createIntentNotice at line 1062 in ecosystem.service.ts is susceptible to a TOCTOU race condition. Two concurrent requests can both pass the existence check and insert duplicate (intentId, orgId) records since there's no unique constraint protecting the database. The application error message at line 1055 acknowledges that only one notice should exist per (intentId, orgId) pair, but this expectation is not enforced.
Add a unique constraint on (intentId, orgId) in the intent_notices model, or wrap both the check and insert in a serializable transaction with proper locking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ecosystem/repositories/ecosystem.repository.ts` around lines 1786 -
1796, The intentNoticeExists + createIntentNotice flow is vulnerable to a TOCTOU
race leading to duplicate (intentId, orgId) rows in the intent_notices table;
either add a DB-level unique constraint on (intentId, orgId) in the
intent_notices model or change the create flow to use a transactional-safe
operation (e.g., a SERIALIZABLE transaction, row-level lock, or a single
upsert/insert ... on conflict) so createIntentNotice cannot insert a duplicate
when intentNoticeExists returns false. Update the Prisma schema/model
(intent_notices) to include the unique index or change the repository method
that performs insertion to use a transactional upsert or lock to enforce
uniqueness atomically.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (12)
apps/oid4vc-verification/src/oid4vc-verification.service.ts (3)
424-424:⚠️ Potential issue | 🟠 MajorRemove
console.logthat dumps session data.
createdSessionmay contain verification URLs and session identifiers. Writing the full object to stdout exposes sensitive data and bypasses structured logging controls.🗑️ Proposed fix
- console.log('createdSession:::::::::::::::::;;', createdSession); return createdSession;If debugging output is needed, use the structured logger with selected fields:
this.logger.debug(`[createIntentBasedVerificationPresentation] session created, id=${createdSession?.verificationSession?.id}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` at line 424, Remove the raw console.log that prints the entire createdSession object (which may expose sensitive verification URLs/IDs) in the createIntentBasedVerificationPresentation flow; replace it with a structured logger call on the service (e.g., this.logger.debug) that logs only selected non-sensitive fields such as createdSession?.verificationSession?.id and a short descriptive message, ensuring you reference the createdSession variable and the createIntentBasedVerificationPresentation method when making the change.
409-421:⚠️ Potential issue | 🟡 MinorNotice lookup missing
ecosystemIdscope; URL building uses unsafe concatenation.Two issues remain in this block:
- The notice fetch (lines 412-416) uses
{ intentId, orgId }but omitsecosystemId, allowing potential cross-ecosystem data access.- Line 420 uses string concatenation which corrupts URLs that already contain query params or fragments.
🛠️ Proposed fix
if (intentId) { const intentNotice: any = await this.natsClient .sendNatsMessage(this.oid4vpVerificationServiceProxy, 'get-intent-notice-by-intent-id', { intentId, - orgId + orgId, + ecosystemId }) .catch(() => null); if (intentNotice?.noticeUrl) { - createdSession.noticeUrl = `${intentNotice.noticeUrl}?transactionId=${createdSession.verificationSession.id}`; + createdSession.noticeUrl = buildUrlWithQuery(intentNotice.noticeUrl, { + transactionId: createdSession.verificationSession.id + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` around lines 409 - 421, The notice fetch omits the ecosystem scope and the notice URL is built via unsafe string concatenation; update the this.natsClient.sendNatsMessage call for 'get-intent-notice-by-intent-id' to include the ecosystemId (e.g., pass { intentId, orgId, ecosystemId } using the same source as orgId/templateData), and replace the unsafe assignment to createdSession.noticeUrl with a safe URL builder: parse intentNotice.noticeUrl into a URL/URLSearchParams, set/append transactionId = createdSession.verificationSession.id, then serialize back to a string before assigning to createdSession.noticeUrl (use the URL/URLSearchParams APIs rather than manual string concatenation).
316-317:⚠️ Potential issue | 🟠 Major
ecosystemIdshould be required for intent-based verification.Per retrieved learnings, intent records are never created without an
ecosystemIdin the application workflow. Keeping this parameter optional allows unscoped intent resolution in multi-ecosystem organizations.🛡️ Proposed fix
async createIntentBasedVerificationPresentation( orgId: string, verifierId: string, intent: string, responseMode: string, requestSigner: IRequestSigner, userDetails: user, expectedOrigins?: string[], - ecosystemId?: string // kept optional here so existing callers don't break + ecosystemId: string ): Promise<object> {Based on learnings: "In the intents table, ecosystemId is NOT NULL because intent records are never created without an ecosystemId in the application workflow."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts` around lines 316 - 317, The parameter ecosystemId is mistakenly optional in the intent-based verification API; change the function/method signature that currently declares "expectedOrigins?: string[], ecosystemId?: string" so that ecosystemId is required (remove the ?), update any related type/interface declarations and callers to pass a non-null ecosystemId, and remove or adjust any fallback/unscoped intent-resolution logic (or runtime checks) relying on a missing ecosystemId; reference the parameter name ecosystemId and the function/method where this signature appears to locate and update all usages accordingly.apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
362-368:⚠️ Potential issue | 🟠 MajorVerify caller's org membership in the ecosystem before proceeding.
The controller passes
ecosystemIdto the service without validating thatorgIdis a member of that ecosystem. This could allow an authenticated user to probe or retrieve intent templates from ecosystems their organization does not belong to.Run the following script to check if ecosystem-org membership validation exists downstream:
#!/bin/bash # Description: Search for ecosystem_orgs membership checks in the verification flow echo "=== Checking ecosystem membership validation in service layer ===" rg -n -B3 -A10 'ecosystem_orgs|ecosystemOrg|checkOrgMembership|validateEcosystem' \ apps/oid4vc-verification/src/ \ apps/api-gateway/src/oid4vc-verification/ \ apps/ecosystem/src/ \ apps/ecosystem/repositories/ 2>/dev/null | head -100 echo echo "=== Check getIntentTemplateByIntentAndOrg for ecosystem scoping ===" rg -n -A20 'getIntentTemplateByIntentAndOrg' apps/ecosystem/repositories/ecosystem.repository.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts` around lines 362 - 368, The controller currently forwards ecosystemId to createIntentBasedVerificationPresentation without confirming that orgId belongs to that ecosystem; add an explicit membership check before calling this. Call a suitable ecosystem membership validator (e.g., ecosystemService.isOrgMemberOrThrow / verifyOrgInEcosystem or similar) with orgId and ecosystemId in oid4vc-verification.controller.ts, throw a 403 / BadRequest if the check fails, and only then invoke Oid4vcVerificationService.createIntentBasedVerificationPresentation with orgId and ecosystemId; ensure you reuse existing ecosystem repository/service methods (or add one) rather than embedding DB logic in the controller.apps/ecosystem/src/ecosystem.service.ts (3)
1044-1051:⚠️ Potential issue | 🟠 MajorReject non-ACTIVE org memberships for org-scoped notices.
This currently accepts any membership row. It should require active, non-deleted membership before creating an org-scoped notice.
🛠️ Suggested guard
if (orgId) { const orgEcosystemMembership = await this.ecosystemRepository.getEcosystemOrg(intent['ecosystemId'], orgId); - if (!orgEcosystemMembership) { + if ( + !orgEcosystemMembership || + orgEcosystemMembership.deletedAt || + orgEcosystemMembership.status !== EcosystemOrgStatus.ACTIVE + ) { throw new RpcException({ statusCode: HttpStatus.FORBIDDEN, message: 'The provided orgId is not a member or lead of this ecosystem.' }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 1044 - 1051, The current org-membership check in the code path that calls this.ecosystemRepository.getEcosystemOrg (variable orgEcosystemMembership) must reject non-active or deleted memberships before allowing creation of an org-scoped notice; update the guard after fetching orgEcosystemMembership to verify membership.status === 'ACTIVE' (or the equivalent active enum value used in the codebase) and that the membership is not soft-deleted (e.g., deletedAt is null/undefined or isDeleted flag is false), and if either check fails throw the same RpcException with HttpStatus.FORBIDDEN and the existing message; modify the block around orgEcosystemMembership to perform these checks so only active, non-deleted members are permitted.
1131-1168:⚠️ Potential issue | 🔴 CriticalAuthorize update/delete by notice ecosystem ownership before mutation.
Both paths mutate by raw notice id without
validateEcosystemLead.updateIntentNoticealso receivesuserIdbut doesn’t use it for authz checks;deleteIntentNoticehas no actor input at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 1131 - 1168, Before mutating notices, ensure the caller is authorized by validating ecosystem ownership: in updateIntentNotice, call the existing validateEcosystemLead (or equivalent authorization helper) using the provided userId and the notice's ecosystem (lookup via ecosystemRepository.get/getById or a method that returns the notice metadata) before calling ecosystemRepository.updateIntentNotice; likewise, add an actor parameter to deleteIntentNotice (e.g., userId) and validate ownership with validateEcosystemLead after fetching the notice (via ecosystemRepository.deleteIntentNotice’s pre-check or a separate fetch) and only proceed to call ecosystemRepository.deleteIntentNotice when validation passes; on failed validation, throw the same RpcException pattern used elsewhere.
1131-1134:⚠️ Potential issue | 🟡 MinorApply URL validation on notice update path.
Create validates
noticeUrl, but update persists directly. Please run the samevalidateNoticeUrlcheck before updating.🛠️ Suggested fix
async updateIntentNotice(id: string, noticeUrl: string, userId: string): Promise<object> { try { + await validateNoticeUrl(noticeUrl); const updated = await this.ecosystemRepository.updateIntentNotice(id, noticeUrl, userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 1131 - 1134, For updateIntentNotice, run the same notice URL validation used in create by calling validateNoticeUrl(noticeUrl) before calling this.ecosystemRepository.updateIntentNotice; if validation fails throw/return the same error shape as create (mirror the create path behavior) so invalid URLs are rejected consistently; reference the updateIntentNotice method and the validateNoticeUrl helper to implement this pre-check.apps/api-gateway/src/ecosystem/intent/intent.controller.ts (1)
636-679:⚠️ Potential issue | 🟠 MajorScope
GET /intent/noticeto ecosystem context.When both filters are omitted, this endpoint returns cross-ecosystem data and does not bind results to caller ecosystem scope. Please require ecosystem scoping (path/query/derived context) before listing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/ecosystem/intent/intent.controller.ts` around lines 636 - 679, The endpoint getIntentNotices currently returns cross-ecosystem data when no filters are supplied; require an ecosystem scope and pass it to the service. Update the controller method getIntentNotices to obtain an ecosystem identifier (either require a new required query/path param like ecosystemId or derive it from the authenticated principal/request using the existing EcosystemRolesGuard or JWT payload), validate its presence when id and intentId are both omitted, and call this.ecosystemService.getIntentNotices(id, intentId, ecosystemId) (or similar) so the service can filter by ecosystem; also update the ecosystemService.getIntentNotices signature/implementation to accept and enforce the ecosystemId filter.apps/ecosystem/repositories/ecosystem.repository.ts (1)
1773-1779:⚠️ Potential issue | 🟠 MajorAdd global-notice fallback when org-specific notice is absent.
With an
orgIdprovided, this does only an exact match and skips the global (orgId = null) notice. That breaks intended fallback behavior.🔁 Suggested query flow
async getIntentNoticeByIntentId(intentId: string, orgId?: string | null): Promise<object | null> { try { - const where: { intentId: string; orgId?: string | null } = { intentId }; - if (orgId !== undefined) { - where.orgId = orgId; + if (orgId) { + const scoped = await this.prisma.intent_notices.findFirst({ + where: { intentId, orgId } + }); + if (scoped) { + return scoped; + } } - return await this.prisma.intent_notices.findFirst({ where }); + return await this.prisma.intent_notices.findFirst({ + where: { intentId, orgId: null } + }); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/repositories/ecosystem.repository.ts` around lines 1773 - 1779, getIntentNoticeByIntentId currently only matches intent notices exactly by orgId when orgId is provided, skipping the global (orgId = null) fallback; change the lookup to return an org-specific notice if present and otherwise fall back to the global notice. In practice, modify the logic around prisma.intent_notices.findFirst in getIntentNoticeByIntentId to either (a) run two queries: first with where = { intentId, orgId } and if null then query { intentId, orgId: null }, or (b) use a single query with an OR clause matching { intentId, orgId } OR { intentId, orgId: null } and prefer the org-specific result — ensure the function still returns the found object or null.libs/prisma-service/prisma/schema.prisma (2)
743-757:⚠️ Potential issue | 🟠 MajorEnforce one-notice-per-scope at DB level.
The service treats scope as unique, but schema only has non-unique indexes. Concurrent creates can still produce duplicates.
🔐 Suggested migration indexes
+-- migration.sql +CREATE UNIQUE INDEX intent_notices_unique_org_scope + ON intent_notices ("intentId", "orgId") + WHERE "orgId" IS NOT NULL; + +CREATE UNIQUE INDEX intent_notices_unique_global_scope + ON intent_notices ("intentId") + WHERE "orgId" IS NULL;Based on learnings, PostgreSQL NULL uniqueness behavior needs explicit safeguards when the scope column is nullable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prisma-service/prisma/schema.prisma` around lines 743 - 757, The DB must enforce one-notice-per-scope for intent_notices: add unique constraints at the DB level because orgId is nullable and standard UNIQUE([intentId, orgId]) won’t prevent multiple NULL-scoped rows. Modify the migration (not just Prisma model) to create two PostgreSQL unique indexes for model intent_notices: one UNIQUE index on (intentId) WHERE orgId IS NULL to allow only a single global notice per intent, and another UNIQUE index on (intentId, orgId) WHERE orgId IS NOT NULL to enforce one notice per intent+organisation; reference the intent_notices model and fields intentId and orgId when implementing these SQL partial unique indexes in the migration.
752-753:⚠️ Potential issue | 🟠 MajorDefine delete behavior for
intent_notices.intentrelation.
deleteIntenthard-deletes intents, but this relation has no explicitonDelete. Once notices exist, intent deletion can fail with FK violations.🛠️ Suggested schema update
- intent intents `@relation`(fields: [intentId], references: [id]) + intent intents `@relation`(fields: [intentId], references: [id], onDelete: Cascade)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prisma-service/prisma/schema.prisma` around lines 752 - 753, The intent_notices relation to intents lacks an explicit onDelete behavior, causing FK errors when intents are hard-deleted; update the relation on the intent field (the line with intent and intentId in the intent_notices model) to include an onDelete rule—e.g. add `@relation`(fields: [intentId], references: [id], onDelete: Cascade) to cascade-delete notices when an intent is removed (or alternatively make intentId nullable and use onDelete: SetNull if you want to preserve notices).apps/api-gateway/src/ecosystem/ecosystem.service.ts (1)
256-258:⚠️ Potential issue | 🟠 MajorMinimize NATS payload to actor id only.
These methods forward the full Prisma
userobject, but downstream only needs identity. Send{ id }to reduce unnecessary PII propagation.✂️ Suggested change
async createIntentNotice(createIntentNoticeDto: CreateIntentNoticeDto, userDetails: user): Promise<object> { - const payload = { createIntentNoticeDto, userDetails }; + const payload = { createIntentNoticeDto, userDetails: { id: userDetails.id } }; return this.natsClient.sendNatsMessage(this.serviceProxy, 'create-intent-notice', payload); } @@ ): Promise<object> { - const payload = { id, updateIntentNoticeDto, userDetails }; + const payload = { id, updateIntentNoticeDto, userDetails: { id: userDetails.id } }; return this.natsClient.sendNatsMessage(this.serviceProxy, 'update-intent-notice', payload); }Also applies to: 281-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/ecosystem/ecosystem.service.ts` around lines 256 - 258, The createIntentNotice method currently sends the full Prisma user object in the NATS payload; change the payload so it only forwards the user's identity (e.g., { id: userDetails.id }) instead of the entire userDetails object—update payload construction in createIntentNotice (and the other similar methods that forward userDetails around the next block) to include only { id: userDetails.id } or a small actor object, then call this.natsClient.sendNatsMessage(this.serviceProxy, 'create-intent-notice', payload) with that minimized payload.
🤖 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/ecosystem/src/ecosystem.helper.ts`:
- Around line 13-16: Wrap the fetch call that uses controller and timeout in a
try/finally so clearTimeout(timeout) always runs even if fetch throws;
specifically around the code that sets const timeout = setTimeout(() =>
controller.abort(), 5000); and calls await fetch(noticeUrl, { method: 'HEAD',
signal: controller.signal }), move clearTimeout(timeout) into a finally block
(and keep any response.ok checks inside the try), and apply the same change to
the second occurrence covering lines 22-30 so the timeout is always cleared on
success or error.
- Around line 12-15: The HEAD fetch to noticeUrl using AbortController
(controller/timeout) performs an unvalidated server-side request and enables
SSRF; before calling fetch (in the code that creates controller/timeout and
calls fetch), parse and validate noticeUrl with the URL constructor, enforce
allowed protocols (only http/https), resolve and check the hostname/IP against a
denylist of private/reserved ranges and/or an allowlist of permitted hosts, and
reject any URL that resolves to loopback, link-local, RFC1918, or other
non-public addresses; only after these checks should you proceed to create the
AbortController and call fetch.
---
Duplicate comments:
In `@apps/api-gateway/src/ecosystem/ecosystem.service.ts`:
- Around line 256-258: The createIntentNotice method currently sends the full
Prisma user object in the NATS payload; change the payload so it only forwards
the user's identity (e.g., { id: userDetails.id }) instead of the entire
userDetails object—update payload construction in createIntentNotice (and the
other similar methods that forward userDetails around the next block) to include
only { id: userDetails.id } or a small actor object, then call
this.natsClient.sendNatsMessage(this.serviceProxy, 'create-intent-notice',
payload) with that minimized payload.
In `@apps/api-gateway/src/ecosystem/intent/intent.controller.ts`:
- Around line 636-679: The endpoint getIntentNotices currently returns
cross-ecosystem data when no filters are supplied; require an ecosystem scope
and pass it to the service. Update the controller method getIntentNotices to
obtain an ecosystem identifier (either require a new required query/path param
like ecosystemId or derive it from the authenticated principal/request using the
existing EcosystemRolesGuard or JWT payload), validate its presence when id and
intentId are both omitted, and call this.ecosystemService.getIntentNotices(id,
intentId, ecosystemId) (or similar) so the service can filter by ecosystem; also
update the ecosystemService.getIntentNotices signature/implementation to accept
and enforce the ecosystemId filter.
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`:
- Around line 362-368: The controller currently forwards ecosystemId to
createIntentBasedVerificationPresentation without confirming that orgId belongs
to that ecosystem; add an explicit membership check before calling this. Call a
suitable ecosystem membership validator (e.g.,
ecosystemService.isOrgMemberOrThrow / verifyOrgInEcosystem or similar) with
orgId and ecosystemId in oid4vc-verification.controller.ts, throw a 403 /
BadRequest if the check fails, and only then invoke
Oid4vcVerificationService.createIntentBasedVerificationPresentation with orgId
and ecosystemId; ensure you reuse existing ecosystem repository/service methods
(or add one) rather than embedding DB logic in the controller.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 1773-1779: getIntentNoticeByIntentId currently only matches intent
notices exactly by orgId when orgId is provided, skipping the global (orgId =
null) fallback; change the lookup to return an org-specific notice if present
and otherwise fall back to the global notice. In practice, modify the logic
around prisma.intent_notices.findFirst in getIntentNoticeByIntentId to either
(a) run two queries: first with where = { intentId, orgId } and if null then
query { intentId, orgId: null }, or (b) use a single query with an OR clause
matching { intentId, orgId } OR { intentId, orgId: null } and prefer the
org-specific result — ensure the function still returns the found object or
null.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 1044-1051: The current org-membership check in the code path that
calls this.ecosystemRepository.getEcosystemOrg (variable orgEcosystemMembership)
must reject non-active or deleted memberships before allowing creation of an
org-scoped notice; update the guard after fetching orgEcosystemMembership to
verify membership.status === 'ACTIVE' (or the equivalent active enum value used
in the codebase) and that the membership is not soft-deleted (e.g., deletedAt is
null/undefined or isDeleted flag is false), and if either check fails throw the
same RpcException with HttpStatus.FORBIDDEN and the existing message; modify the
block around orgEcosystemMembership to perform these checks so only active,
non-deleted members are permitted.
- Around line 1131-1168: Before mutating notices, ensure the caller is
authorized by validating ecosystem ownership: in updateIntentNotice, call the
existing validateEcosystemLead (or equivalent authorization helper) using the
provided userId and the notice's ecosystem (lookup via
ecosystemRepository.get/getById or a method that returns the notice metadata)
before calling ecosystemRepository.updateIntentNotice; likewise, add an actor
parameter to deleteIntentNotice (e.g., userId) and validate ownership with
validateEcosystemLead after fetching the notice (via
ecosystemRepository.deleteIntentNotice’s pre-check or a separate fetch) and only
proceed to call ecosystemRepository.deleteIntentNotice when validation passes;
on failed validation, throw the same RpcException pattern used elsewhere.
- Around line 1131-1134: For updateIntentNotice, run the same notice URL
validation used in create by calling validateNoticeUrl(noticeUrl) before calling
this.ecosystemRepository.updateIntentNotice; if validation fails throw/return
the same error shape as create (mirror the create path behavior) so invalid URLs
are rejected consistently; reference the updateIntentNotice method and the
validateNoticeUrl helper to implement this pre-check.
In `@apps/oid4vc-verification/src/oid4vc-verification.service.ts`:
- Line 424: Remove the raw console.log that prints the entire createdSession
object (which may expose sensitive verification URLs/IDs) in the
createIntentBasedVerificationPresentation flow; replace it with a structured
logger call on the service (e.g., this.logger.debug) that logs only selected
non-sensitive fields such as createdSession?.verificationSession?.id and a short
descriptive message, ensuring you reference the createdSession variable and the
createIntentBasedVerificationPresentation method when making the change.
- Around line 409-421: The notice fetch omits the ecosystem scope and the notice
URL is built via unsafe string concatenation; update the
this.natsClient.sendNatsMessage call for 'get-intent-notice-by-intent-id' to
include the ecosystemId (e.g., pass { intentId, orgId, ecosystemId } using the
same source as orgId/templateData), and replace the unsafe assignment to
createdSession.noticeUrl with a safe URL builder: parse intentNotice.noticeUrl
into a URL/URLSearchParams, set/append transactionId =
createdSession.verificationSession.id, then serialize back to a string before
assigning to createdSession.noticeUrl (use the URL/URLSearchParams APIs rather
than manual string concatenation).
- Around line 316-317: The parameter ecosystemId is mistakenly optional in the
intent-based verification API; change the function/method signature that
currently declares "expectedOrigins?: string[], ecosystemId?: string" so that
ecosystemId is required (remove the ?), update any related type/interface
declarations and callers to pass a non-null ecosystemId, and remove or adjust
any fallback/unscoped intent-resolution logic (or runtime checks) relying on a
missing ecosystemId; reference the parameter name ecosystemId and the
function/method where this signature appears to locate and update all usages
accordingly.
In `@libs/prisma-service/prisma/schema.prisma`:
- Around line 743-757: The DB must enforce one-notice-per-scope for
intent_notices: add unique constraints at the DB level because orgId is nullable
and standard UNIQUE([intentId, orgId]) won’t prevent multiple NULL-scoped rows.
Modify the migration (not just Prisma model) to create two PostgreSQL unique
indexes for model intent_notices: one UNIQUE index on (intentId) WHERE orgId IS
NULL to allow only a single global notice per intent, and another UNIQUE index
on (intentId, orgId) WHERE orgId IS NOT NULL to enforce one notice per
intent+organisation; reference the intent_notices model and fields intentId and
orgId when implementing these SQL partial unique indexes in the migration.
- Around line 752-753: The intent_notices relation to intents lacks an explicit
onDelete behavior, causing FK errors when intents are hard-deleted; update the
relation on the intent field (the line with intent and intentId in the
intent_notices model) to include an onDelete rule—e.g. add `@relation`(fields:
[intentId], references: [id], onDelete: Cascade) to cascade-delete notices when
an intent is removed (or alternatively make intentId nullable and use onDelete:
SetNull if you want to preserve notices).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2e9944a-7a78-4d10-9942-a7f5ee768959
📒 Files selected for processing (16)
apps/api-gateway/src/ecosystem/ecosystem.service.tsapps/api-gateway/src/ecosystem/intent/intent.controller.tsapps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.tsapps/ecosystem/repositories/ecosystem.repository.tsapps/ecosystem/src/ecosystem.controller.tsapps/ecosystem/src/ecosystem.helper.tsapps/ecosystem/src/ecosystem.service.tsapps/oid4vc-verification/interfaces/intent-notice.interfaces.tsapps/oid4vc-verification/src/oid4vc-verification.controller.tsapps/oid4vc-verification/src/oid4vc-verification.service.tslibs/common/src/response-messages/index.tslibs/prisma-service/prisma/migrations/20260312082536_added_intent_notice_table/migration.sqllibs/prisma-service/prisma/migrations/20260312084927_add_org_id_to_intent_notices/migration.sqllibs/prisma-service/prisma/schema.prisma
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/prisma-service/prisma/migrations/20260312084927_add_org_id_to_intent_notices/migration.sql
- apps/oid4vc-verification/interfaces/intent-notice.interfaces.ts
- apps/api-gateway/src/oid4vc-verification/dtos/create-intent-notice.dto.ts
- apps/oid4vc-verification/src/oid4vc-verification.controller.ts
- libs/prisma-service/prisma/migrations/20260312082536_added_intent_notice_table/migration.sql
- libs/common/src/response-messages/index.ts
Comment on lines
+12
to
+15
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 5000); | ||
| const response = await fetch(noticeUrl, { method: 'HEAD', signal: controller.signal }); | ||
| clearTimeout(timeout); |
There was a problem hiding this comment.
Harden URL validation against SSRF.
This performs a server-side request to a user-supplied URL without host/protocol restrictions. That enables probing internal addresses/services.
🛡️ Suggested hardening
export async function validateNoticeUrl(noticeUrl: string): Promise<void> {
if (!noticeUrl || !noticeUrl.trim()) {
@@
}
+ const parsed = new URL(noticeUrl);
+ if (parsed.protocol !== 'https:' || ['localhost', '127.0.0.1', '::1'].includes(parsed.hostname)) {
+ throw new RpcException({
+ statusCode: HttpStatus.BAD_REQUEST,
+ message: 'noticeUrl must be a public HTTPS URL.'
+ });
+ }
try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 12 - 15, The HEAD fetch
to noticeUrl using AbortController (controller/timeout) performs an unvalidated
server-side request and enables SSRF; before calling fetch (in the code that
creates controller/timeout and calls fetch), parse and validate noticeUrl with
the URL constructor, enforce allowed protocols (only http/https), resolve and
check the hostname/IP against a denylist of private/reserved ranges and/or an
allowlist of permitted hosts, and reject any URL that resolves to loopback,
link-local, RFC1918, or other non-public addresses; only after these checks
should you proceed to create the AbortController and call fetch.
Comment on lines
+13
to
+16
| const timeout = setTimeout(() => controller.abort(), 5000); | ||
| const response = await fetch(noticeUrl, { method: 'HEAD', signal: controller.signal }); | ||
| clearTimeout(timeout); | ||
| if (!response.ok) { |
There was a problem hiding this comment.
Always clear the timeout, even on fetch failure.
If fetch throws before the current clearTimeout, the timer remains scheduled unnecessarily.
🧹 Suggested fix
try {
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 5000);
- const response = await fetch(noticeUrl, { method: 'HEAD', signal: controller.signal });
- clearTimeout(timeout);
+ let response: Response;
+ try {
+ response = await fetch(noticeUrl, { method: 'HEAD', signal: controller.signal });
+ } finally {
+ clearTimeout(timeout);
+ }
if (!response.ok) {Also applies to: 22-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 13 - 16, Wrap the fetch
call that uses controller and timeout in a try/finally so clearTimeout(timeout)
always runs even if fetch throws; specifically around the code that sets const
timeout = setTimeout(() => controller.abort(), 5000); and calls await
fetch(noticeUrl, { method: 'HEAD', signal: controller.signal }), move
clearTimeout(timeout) into a finally block (and keep any response.ok checks
inside the try), and apply the same change to the second occurrence covering
lines 22-30 so the timeout is always cleared on success or error.
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/ecosystem/src/ecosystem.helper.ts (2)
11-15:⚠️ Potential issue | 🔴 CriticalHarden
noticeUrlbefore server-side fetch to prevent SSRF.Line 14 performs a server-side request to user-controlled input without protocol/host restrictions, which can probe internal services.
🛡️ Suggested hardening
export async function validateNoticeUrl(noticeUrl: string): Promise<void> { @@ + const parsed = new URL(noticeUrl); + if (!['http:', 'https:'].includes(parsed.protocol)) { + throw new RpcException({ + statusCode: HttpStatus.BAD_REQUEST, + message: 'noticeUrl must use http or https.' + }); + } + if (['localhost', '127.0.0.1', '::1'].includes(parsed.hostname)) { + throw new RpcException({ + statusCode: HttpStatus.BAD_REQUEST, + message: 'noticeUrl must be publicly reachable.' + }); + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 11 - 15, The server is fetching a user-controlled noticeUrl without validation (variables: noticeUrl, AbortController, controller, timeout, response, fetch), enabling SSRF; fix by parsing and validating noticeUrl before calling fetch: ensure the scheme is http or https, enforce a hostname whitelist or reject hostnames like localhost, 127.0.0.0/8, ::1 and private IP ranges (10/8, 172.16/12, 192.168/16) by resolving the hostname or checking against a trusted domain list, and reject non-conforming URLs; also pass controller.signal into fetch (fetch(noticeUrl, { signal: controller.signal })) to honor the timeout and keep the AbortController logic intact.
12-15:⚠️ Potential issue | 🟠 MajorConnect
AbortControllertofetchand clear timer infinally.At Line 14,
fetchis called withoutsignal, so timeout abort never applies. Also,clearTimeoutat Line 15 is skipped whenfetchthrows.⏱️ Suggested fix
try { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); - const response = await fetch(noticeUrl); - clearTimeout(timeout); + let response: Response; + try { + response = await fetch(noticeUrl, { + method: 'HEAD', + signal: controller.signal + }); + } finally { + clearTimeout(timeout); + } if (!response.ok) {#!/bin/bash # Verify timeout wiring and cleanup in the reviewed helper. rg -n -C3 'AbortController|setTimeout\(|fetch\(|clearTimeout\(' apps/ecosystem/src/ecosystem.helper.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.helper.ts` around lines 12 - 15, The AbortController is created and a timeout is set but its signal is not passed to fetch and clearTimeout is only called after a successful fetch; update the code where AbortController, timeout, fetch, and clearTimeout are used so that fetch receives the controller.signal (e.g., fetch(noticeUrl, { signal: controller.signal })), and move clearTimeout(timeout) into a finally block so the timer is always cleared whether fetch resolves or throws; keep the existing 5000ms timeout and ensure any thrown abort/error propagates as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/ecosystem/src/ecosystem.helper.ts`:
- Around line 11-15: The server is fetching a user-controlled noticeUrl without
validation (variables: noticeUrl, AbortController, controller, timeout,
response, fetch), enabling SSRF; fix by parsing and validating noticeUrl before
calling fetch: ensure the scheme is http or https, enforce a hostname whitelist
or reject hostnames like localhost, 127.0.0.0/8, ::1 and private IP ranges
(10/8, 172.16/12, 192.168/16) by resolving the hostname or checking against a
trusted domain list, and reject non-conforming URLs; also pass controller.signal
into fetch (fetch(noticeUrl, { signal: controller.signal })) to honor the
timeout and keep the AbortController logic intact.
- Around line 12-15: The AbortController is created and a timeout is set but its
signal is not passed to fetch and clearTimeout is only called after a successful
fetch; update the code where AbortController, timeout, fetch, and clearTimeout
are used so that fetch receives the controller.signal (e.g., fetch(noticeUrl, {
signal: controller.signal })), and move clearTimeout(timeout) into a finally
block so the timer is always cleared whether fetch resolves or throws; keep the
existing 5000ms timeout and ensure any thrown abort/error propagates as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81d72b2f-4638-416e-9d09-45fb40ed5d72
📒 Files selected for processing (1)
apps/ecosystem/src/ecosystem.helper.ts
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