feat: Implement webhook signing functionality and update endpoints#1570
Merged
sagarkhole4 merged 13 commits intomainfrom Feb 27, 2026
Merged
feat: Implement webhook signing functionality and update endpoints#1570sagarkhole4 merged 13 commits intomainfrom
sagarkhole4 merged 13 commits intomainfrom
Conversation
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIGateway as API Gateway
participant NATS as NATS Broker
participant WebhookMS as Webhook Microservice
participant DB as Database
participant External as External Webhook
Note over Client,APIGateway: Register or update webhook (webhookUrl, webhookSecret, orgId)
Client->>APIGateway: POST/PATCH /webhook (webhookUrl, webhookSecret, orgId)
APIGateway->>NATS: publish register/update-webhook
NATS->>WebhookMS: deliver message
WebhookMS->>DB: store webhookUrl + webhookSecret (encrypted)
DB-->>WebhookMS: ack
WebhookMS-->>APIGateway: return { webhookUrl, webhookSecret }
Note over APIGateway,External: Webhook delivery with optional signing
APIGateway->>NATS: publish webhook-response (webhookUrl, data, webhookSecret)
NATS->>WebhookMS: deliver webhook-response
WebhookMS->>WebhookMS: if webhookSecret present compute HMAC-SHA256(timestamp+payload)
WebhookMS->>External: POST payload with X-Signature & X-Timestamp (if signed)
External-->>WebhookMS: HTTP response
WebhookMS-->>APIGateway: forward success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
503-520:⚠️ Potential issue | 🟠 Major
'webhookUrl' in webhookUrlInfodoes not guard against anullor emptywebhookUrlvalue.The JavaScript
inoperator checks only for property existence, not value truthiness. If the NATS response returns{ webhookUrl: null }or{ webhookUrl: '' }, the condition evaluates totrueand_postWebhookResponseis invoked with a null/empty URL, causing a runtime error in the downstream webhook service.🐛 Proposed fix
- if (webhookUrlInfo && 'webhookUrl' in webhookUrlInfo) { + if (webhookUrlInfo?.webhookUrl) {This same pattern is repeated in
connection.controller.ts(lines 391, 433) andoid4vc-issuance.controller.ts(line 656) and should be fixed in all three.🤖 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 503 - 520, The current check "'webhookUrl' in webhookUrlInfo" only verifies the property exists but not that it contains a usable value; change the guard to validate webhookUrl is a non-empty string (and optionally a valid URL) before calling _postWebhookResponse. Specifically, update the branch that reads webhookUrlInfo and uses _postWebhookResponse to use something like: ensure webhookUrlInfo is truthy and webhookUrlInfo.webhookUrl is a non-empty trimmed string (or validated URL) before invoking _postWebhookResponse; otherwise log/skip the webhook call. Apply the same fix pattern to the analogous checks in connection.controller (the branches calling _getWebhookUrl/_postWebhookResponse) and oid4vc-issuance.controller where webhookUrl is tested.apps/webhook/src/webhook.repository.ts (1)
14-31:⚠️ Potential issue | 🟠 MajorMissing
awaiton Prisma update —try/catchwon't catch database errors.
this.prisma.org_agents.update(...)returns aPrismaPromise. Withoutawait, the promise rejection bypasses thetry/catchblock, so the error logging on Line 28 will never execute for DB-level failures. The same issue exists in the pre-existinggetOrganizationDetails(Line 85), but since this line is modified, it should be fixed here.🐛 Proposed fix
- const agentInfo = this.prisma.org_agents.update({ + const agentInfo = await this.prisma.org_agents.update({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.repository.ts` around lines 14 - 31, The Prisma update call in registerWebhook is missing an await so database errors escape the try/catch; update the implementation of registerWebhook to await the PrismaPromise returned by this.prisma.org_agents.update(...) (and similarly fix getOrganizationDetails) so that rejections are caught, logged via this.logger.error(`[registerWebhookUrl] ...`) and then rethrown; ensure you still return the resolved value from registerWebhook after awaiting the update.
🧹 Nitpick comments (5)
apps/api-gateway/src/webhook/dtos/update-webhook-dto.ts (1)
6-21: Empty body produces a silent no-op update.Both
webhookUrlandwebhookSecretare@IsOptional(). A PATCH request with an empty body{}will pass validation and propagate through the NATS message to the repository layer, resulting in a no-op that still returns a 200 success response. Consider adding a class-level custom validator or a guard to ensure at least one field is provided.💡 Example approach using `class-validator`
import { ValidateIf } from 'class-validator'; // At class level or in the controller, add a check: // Option 1: Controller-level guard if (!updateWebhookDto.webhookUrl && !updateWebhookDto.webhookSecret) { throw new BadRequestException('At least one of webhookUrl or webhookSecret must be provided.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/webhook/dtos/update-webhook-dto.ts` around lines 6 - 21, UpdateWebhookDto currently allows an empty PATCH body to pass validation because both webhookUrl and webhookSecret are `@IsOptional`(); add a check to require at least one of them: either (A) add a class-level custom validator applied to UpdateWebhookDto (use a ValidatorConstraint and `@Validate` on the class) that fails if both webhookUrl and webhookSecret are null/undefined/empty, or (B) enforce the rule in the controller/handler after transformation (in the method handling UpdateWebhookDto) and throw a BadRequestException if both UpdateWebhookDto.webhookUrl and UpdateWebhookDto.webhookSecret are missing; update tests and error message to say "At least one of webhookUrl or webhookSecret must be provided."apps/api-gateway/src/connection/connection.service.ts (1)
115-143:_getWebhookUrl/_postWebhookResponseare copy-pasted identically across five services — extract to a shared helper.These two methods appear verbatim in
connection.service.ts,issuance.service.ts,verification.service.ts,oid4vc-issuance.service.ts, andoid4vc-verification.service.ts, differing only in the injectedClientProxyinstance. This is a DRY violation that multiplies thewebhookSecretsecurity concern and any future bugs.Extract a standalone utility function (or a
BaseServicemixin method) that accepts the proxy as a parameter:♻️ Proposed refactor (utility function approach)
// libs/common/src/webhook.helper.ts import { firstValueFrom } from 'rxjs'; import { ClientProxy } from '@nestjs/microservices'; import { Logger } from '@nestjs/common'; export async function getWebhookUrl( proxy: ClientProxy, logger: Logger, tenantId?: string, orgId?: string ): Promise<{ webhookUrl: string; webhookSecret?: string }> { const pattern = { cmd: 'get-webhookurl' }; try { return await firstValueFrom(proxy.send<{ webhookUrl: string; webhookSecret?: string }>(pattern, { tenantId, orgId })); } catch (error) { logger.error(`get-webhookurl failed: ${error?.message ?? JSON.stringify(error)}`); throw error; } } export async function postWebhookResponse( proxy: ClientProxy, logger: Logger, webhookUrl: string, data: object, webhookSecret?: string ): Promise<string> { const pattern = { cmd: 'post-webhook-response-to-webhook-url' }; try { return await firstValueFrom(proxy.send<string>(pattern, { webhookUrl, data, webhookSecret })); } catch (error) { logger.error(`post-webhook-response failed: ${error?.message ?? JSON.stringify(error)}`); throw error; } }Note: this file's
firstValueFromusage is the correct approach. The other four services still use the deprecated.toPromise()and should be aligned to this pattern as part of the same cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/connection/connection.service.ts` around lines 115 - 143, Extract the duplicated logic in _getWebhookUrl and _postWebhookResponse into shared helper functions (e.g., getWebhookUrl and postWebhookResponse) that accept a ClientProxy and a Logger as parameters; move them into a new common module (e.g., libs/common/src/webhook.helper.ts), implement the same patterns and payloads (pattern { cmd: 'get-webhookurl' } and { cmd: 'post-webhook-response-to-webhook-url' }) using firstValueFrom(proxy.send(...)), and log errors with a descriptive message plus error?.message or JSON.stringify(error) before rethrowing; then replace the duplicate methods in connection.service.ts, issuance.service.ts, verification.service.ts, oid4vc-issuance.service.ts, and oid4vc-verification.service.ts to call the shared helpers with their injected ClientProxy and Logger, and update any remaining toPromise() usages to firstValueFrom.apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts (1)
112-139: Replace deprecated.toPromise()withfirstValueFrom()for consistency withconnection.service.ts.
toPromise()is deprecated in RxJS 7 and removed in RxJS 8. UsefirstValueFrom()/lastValueFrom()instead.connection.service.tsin this PR already usesfirstValueFrom()correctly, while four services in the codebase (verification.service.ts, oid4vc-verification.service.ts, issuance.service.ts, oid4vc-issuance.service.ts) still use the deprecated method.♻️ Proposed fix
Import
firstValueFromat the top of the file:+import { firstValueFrom } from 'rxjs';Then update both methods (lines 118 and 132):
- const message = await this.oid4vpProxy.send<any>(pattern, payload).toPromise(); + const message = await firstValueFrom(this.oid4vpProxy.send<any>(pattern, payload));🤖 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.service.ts` around lines 112 - 139, Replace deprecated toPromise() calls in _getWebhookUrl and _postWebhookResponse by importing and using firstValueFrom from 'rxjs'. Locate usages of this.oid4vpProxy.send<any>(pattern, payload).toPromise() in methods _getWebhookUrl and _postWebhookResponse and change them to await firstValueFrom(this.oid4vpProxy.send<any>(pattern, payload)); keep existing try/catch and logging behavior and ensure firstValueFrom is added to the file imports.apps/webhook/src/webhook.service.ts (2)
39-52:webhookSecretis stored and returned in plaintext.For HMAC signing keys, plaintext storage is the norm (unlike passwords, these need to be used as-is, not hashed). However, ensure the secret is not included in application logs. The
JSON.stringify(error)calls in the repository (Line 28 ofwebhook.repository.ts) and service (Line 56) could potentially serialize the secret if it appears in an error context. Consider redacting sensitive fields in log output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 39 - 52, The code currently stores and returns webhookSecret in plaintext and may serialize it in logs; keep storing the secret plaintext (needed for HMAC) but stop returning or logging it: update the registration flow that uses registerWebhookDto and returns webhookUrl.webhookSecret to instead omit the secret from the API response or return a masked version (e.g., show only last 4 chars), and remove any direct serialization of errors that could include the secret by replacing JSON.stringify(error) usages (e.g., in webhook.repository.ts and the service error handling) with a sanitizer that redacts webhookSecret (and similar fields) before logging or use a safeLog/error wrapper that strips sensitive keys.
61-91: Extract shared webhook registration/update logic and alignIWebhookDtowith partial update intent.Both
registerWebhookandupdateWebhookfollow identical control flow: verify org existence → invoke repository method → validate result → return{ webhookUrl, webhookSecret }. The only differences are the repository method invoked and the corresponding error message. Consolidate this into a private helper to reduce duplication.Additionally, there is a type mismatch.
IWebhookDtodeclareswebhookUrlas required, but the repository'supdateWebhookmethod accepts it as optional (webhookUrl?: string), and the API gateway'sUpdateWebhookDtomarks it with@IsOptional(). If the intent is to support partial updates (updating only the secret without changing the URL),updateWebhookshould accept a separate interface with both fields optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 61 - 91, Consolidate the duplicated org-check → repo call → result validation → return logic into a private helper (e.g., a new method called upsertWebhookLogic or handleWebhookUpdate) and have both registerWebhook and updateWebhook call it, passing the repository method reference and appropriate error messages; also introduce a new DTO/interface for partial updates (e.g., IUpdateWebhookDto or IPartialWebhookDto) where webhookUrl and webhookSecret are optional, update the updateWebhook method signature to accept that new type instead of IWebhookDto, and ensure the repository.updateWebhook call and validation align with optional fields so callers can update just the secret or just the URL.
🤖 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 506-507: Replace the quiet debug logging inside webhook promise
.catch() handlers with an error-level (or at least warn-level) log and include
the actual error message/stack instead of JSON.stringify(error); specifically
update the .catch() in oid4vc-verification.controller (and the analogous
.catch() handlers in connection.controller and oid4vc-issuance.controller) to
call this.logger.error(...) and interpolate error?.message ?? error?.stack ??
JSON.stringify(error) (or similar) so operational webhook failures are visible
and Error instances are logged correctly.
- Around line 511-519: The webhookUrl from webhookUrlInfo.webhookUrl is
user-controlled and must be validated before calling
oid4vcVerificationService._postWebhookResponse; update the downstream handler
(the service method that actually posts to
"post-webhook-response-to-webhook-url" /
oid4vcVerificationService._postWebhookResponse) to enforce strict URL
validation: require https scheme, reject IP addresses in RFC1918/private ranges,
loopback addresses, link-local and cloud metadata endpoints (e.g.
169.254.169.254), and/or check against a configured allowlist of hostnames; if
validation fails, do not perform the outbound POST and return/log a clear error
including webhookUrlInfo.webhookSecret only when safe to do so.
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts`:
- Around line 126-129: The _postWebhookResponse function currently includes the
raw webhookSecret in the NATS payload (pattern
'post-webhook-response-to-webhook-url'), exposing secrets on the message bus;
remove webhookSecret from the payload and instead send a non-secret identifier
(e.g., orgId/tenantId/secretId) that allows the downstream webhook service to
fetch the secret itself, or if the secret must travel, encrypt it with KMS and
send only ciphertext and keyId plus enforce NATS TLS and disable payload debug
logging; update the payload construction in _postWebhookResponse to include the
identifier or ciphertext/keyId and ensure any caller of _postWebhookResponse
supplies the identifier rather than the raw secret.
In `@apps/api-gateway/src/webhook/dtos/register-webhook-dto.ts`:
- Around line 6-21: Remove the unused duplicate RegisterWebhookDto declaration
(the class containing webhookUrl and webhookSecret) from the agent-service DTO
module: ensure there are no remaining imports referencing RegisterWebhookDto,
delete that duplicate file, and update any index/barrel exports if they exported
the class so the codebase only uses the single canonical RegisterWebhookDto
(with webhookUrl and webhookSecret) in the api-gateway module; run the
build/tests to confirm no references remain.
In `@apps/webhook/interfaces/webhook.interfaces.ts`:
- Around line 1-4: ICreateWebhookUrl currently includes webhookSecret and the
service methods registerWebhook, updateWebhook, and getWebhookUrl are returning
secrets to clients; remove webhookSecret from the public interface
(ICreateWebhookUrl) and stop echoing it in registerWebhook, updateWebhook, and
getWebhookUrl by either creating a separate response interface (e.g.,
IWebhookResponse) that excludes webhookSecret or explicitly delete/redact
webhookSecret from the object before returning to callers so secrets are never
included in API responses.
- Around line 6-9: The IGetWebhookUrl type currently includes webhookSecret
which causes controllers returning DB records to leak secrets to roles allowed
by the Roles(...) decorator (OrgRoles.OWNER, OrgRoles.ISSUER, OrgRoles.VERIFIER,
OrgRoles.ADMIN); fix by either removing webhookSecret from the IGetWebhookUrl
interface and ensuring the controller/repository response conforms to the
sanitized shape, or by restricting access in the controller by changing the
Roles decorator to only OrgRoles.OWNER and OrgRoles.ADMIN; specifically locate
the IGetWebhookUrl interface and the controller method using Roles(...) and
either remove webhookSecret from the returned DTO or explicitly delete/sanitize
webhookSecret from the object returned by the repository (e.g., map the DB
record to a safe object) so non-owner roles cannot see secrets.
In `@apps/webhook/src/webhook.repository.ts`:
- Around line 33-55: In updateWebhook, await the prisma call (await
this.prisma.org_agents.update(...)) so DB errors are caught by the try/catch,
and change the truthy guards for webhookUrl/webhookSecret to explicit undefined
checks (e.g., if (webhookUrl !== undefined) / if (webhookSecret !== undefined))
so callers can clear the secret by passing null or empty string when intended;
update the local data object construction and keep the same return of agentInfo
from the updateWebhook method.
In `@libs/prisma-service/prisma/schema.prisma`:
- Around line 239-240: The webhookSecret field in the Prisma model is being
persisted in plaintext; update the code so webhookSecret is encrypted at the
application layer before saving and only decrypted when needed for HMAC-SHA256
signing (do the same treatment as clientSecret/apiKey). Modify persistence logic
that writes/reads the webhookSecret to call a centralized crypto helper that
uses a secure AEAD (e.g., AES-256-GCM) with a key fetched from your secrets
manager, store the ciphertext in the webhookSecret column, and ensure any API
responses never return the decrypted secret; update create/update and read paths
that reference webhookSecret and the signing code to use the new encrypt/decrypt
helper.
---
Outside diff comments:
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`:
- Around line 503-520: The current check "'webhookUrl' in webhookUrlInfo" only
verifies the property exists but not that it contains a usable value; change the
guard to validate webhookUrl is a non-empty string (and optionally a valid URL)
before calling _postWebhookResponse. Specifically, update the branch that reads
webhookUrlInfo and uses _postWebhookResponse to use something like: ensure
webhookUrlInfo is truthy and webhookUrlInfo.webhookUrl is a non-empty trimmed
string (or validated URL) before invoking _postWebhookResponse; otherwise
log/skip the webhook call. Apply the same fix pattern to the analogous checks in
connection.controller (the branches calling _getWebhookUrl/_postWebhookResponse)
and oid4vc-issuance.controller where webhookUrl is tested.
In `@apps/webhook/src/webhook.repository.ts`:
- Around line 14-31: The Prisma update call in registerWebhook is missing an
await so database errors escape the try/catch; update the implementation of
registerWebhook to await the PrismaPromise returned by
this.prisma.org_agents.update(...) (and similarly fix getOrganizationDetails) so
that rejections are caught, logged via this.logger.error(`[registerWebhookUrl]
...`) and then rethrown; ensure you still return the resolved value from
registerWebhook after awaiting the update.
---
Duplicate comments:
In `@apps/api-gateway/src/connection/connection.controller.ts`:
- Around line 386-443: The truthiness check "'webhookUrl' in webhookUrlInfo" is
insufficient and errors are only logged at debug level; update the logic in
getQuestionAnswerWebhook (and the earlier block using connectionDto) to verify
webhookUrlInfo?.webhookUrl (and optionally webhookUrlInfo?.webhookSecret) is
non-empty before calling _postWebhookResponse, and change the catch handlers on
both _getWebhookUrl and _postWebhookResponse calls to log the full error at
error level (use this.logger.error and include the error object/details rather
than just JSON.stringify) so failures are reliably detected and debuggable.
In `@apps/api-gateway/src/issuance/issuance.service.ts`:
- Around line 232-259: Replace the deprecated .toPromise() usage with
firstValueFrom from rxjs for both _getWebhookUrl and _postWebhookResponse (i.e.,
await firstValueFrom(this.issuanceProxy.send(...))). Also stop always embedding
the sensitive webhookSecret into the RPC payload: in _postWebhookResponse build
the payload so webhookSecret is omitted when undefined and, if a secret must be
transmitted, move it out of the main payload (e.g., send via headers/options or
a dedicated secure parameter) rather than including webhookSecret directly in
the payload sent by issuanceProxy.send. Ensure necessary imports
(firstValueFrom) are added and update both function bodies (_getWebhookUrl and
_postWebhookResponse) accordingly.
In `@apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts`:
- Around line 651-665: The current check 'webhookUrl' in webhookUrlInfo is too
weak and errors are logged at debug level; update the conditional to assert a
real URL (e.g., if (webhookUrlInfo && webhookUrlInfo.webhookUrl) or use optional
chaining webhookUrlInfo?.webhookUrl) before posting, and change the .catch
handlers around this. _getWebhookUrl and _postWebhookResponse should have their
promise rejections logged with logger.error and include the actual error
object/details (not just JSON.stringify) so failures aren't silently hidden;
update the two .catch blocks accordingly to log full error context and
return/handle failure consistently.
In `@apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts`:
- Around line 143-170: The code uses the deprecated Observable.toPromise and
sends sensitive webhookSecret inside the RPC payload; update both _getWebhookUrl
and _postWebhookResponse to await the RPC using firstValueFrom (imported from
'rxjs') instead of .toPromise() — e.g., await
firstValueFrom(this.issuanceProxy.send<any>(pattern, payload)) — and stop
including webhookSecret directly in the payload for _postWebhookResponse: send
only { webhookUrl, data } as the RPC payload and pass webhookSecret via a secure
channel/metadata or omit it entirely so secrets are not serialized into the
message; apply these changes to the issuanceProxy.send calls in both methods
(_getWebhookUrl and _postWebhookResponse).
In `@apps/api-gateway/src/verification/verification.service.ts`:
- Around line 115-141: Both methods use the deprecated Observable.toPromise and
send sensitive webhookSecret inside the RPC payload; update _getWebhookUrl and
_postWebhookResponse to use rxjs firstValueFrom (or lastValueFrom) with await
firstValueFrom(this.verificationServiceProxy.send(pattern, payload)) instead of
.toPromise(), and for _postWebhookResponse remove webhookSecret from the main
payload body—pass it separately (e.g., in a headers or meta field like {
payload: { webhookUrl, data }, meta: { webhookSecret } } or omit entirely if not
needed) when calling verificationServiceProxy.send(pattern, ...), and add the
necessary import for firstValueFrom from 'rxjs' and adjust types accordingly.
---
Nitpick comments:
In `@apps/api-gateway/src/connection/connection.service.ts`:
- Around line 115-143: Extract the duplicated logic in _getWebhookUrl and
_postWebhookResponse into shared helper functions (e.g., getWebhookUrl and
postWebhookResponse) that accept a ClientProxy and a Logger as parameters; move
them into a new common module (e.g., libs/common/src/webhook.helper.ts),
implement the same patterns and payloads (pattern { cmd: 'get-webhookurl' } and
{ cmd: 'post-webhook-response-to-webhook-url' }) using
firstValueFrom(proxy.send(...)), and log errors with a descriptive message plus
error?.message or JSON.stringify(error) before rethrowing; then replace the
duplicate methods in connection.service.ts, issuance.service.ts,
verification.service.ts, oid4vc-issuance.service.ts, and
oid4vc-verification.service.ts to call the shared helpers with their injected
ClientProxy and Logger, and update any remaining toPromise() usages to
firstValueFrom.
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts`:
- Around line 112-139: Replace deprecated toPromise() calls in _getWebhookUrl
and _postWebhookResponse by importing and using firstValueFrom from 'rxjs'.
Locate usages of this.oid4vpProxy.send<any>(pattern, payload).toPromise() in
methods _getWebhookUrl and _postWebhookResponse and change them to await
firstValueFrom(this.oid4vpProxy.send<any>(pattern, payload)); keep existing
try/catch and logging behavior and ensure firstValueFrom is added to the file
imports.
In `@apps/api-gateway/src/webhook/dtos/update-webhook-dto.ts`:
- Around line 6-21: UpdateWebhookDto currently allows an empty PATCH body to
pass validation because both webhookUrl and webhookSecret are `@IsOptional`(); add
a check to require at least one of them: either (A) add a class-level custom
validator applied to UpdateWebhookDto (use a ValidatorConstraint and `@Validate`
on the class) that fails if both webhookUrl and webhookSecret are
null/undefined/empty, or (B) enforce the rule in the controller/handler after
transformation (in the method handling UpdateWebhookDto) and throw a
BadRequestException if both UpdateWebhookDto.webhookUrl and
UpdateWebhookDto.webhookSecret are missing; update tests and error message to
say "At least one of webhookUrl or webhookSecret must be provided."
In `@apps/webhook/src/webhook.service.ts`:
- Around line 39-52: The code currently stores and returns webhookSecret in
plaintext and may serialize it in logs; keep storing the secret plaintext
(needed for HMAC) but stop returning or logging it: update the registration flow
that uses registerWebhookDto and returns webhookUrl.webhookSecret to instead
omit the secret from the API response or return a masked version (e.g., show
only last 4 chars), and remove any direct serialization of errors that could
include the secret by replacing JSON.stringify(error) usages (e.g., in
webhook.repository.ts and the service error handling) with a sanitizer that
redacts webhookSecret (and similar fields) before logging or use a safeLog/error
wrapper that strips sensitive keys.
- Around line 61-91: Consolidate the duplicated org-check → repo call → result
validation → return logic into a private helper (e.g., a new method called
upsertWebhookLogic or handleWebhookUpdate) and have both registerWebhook and
updateWebhook call it, passing the repository method reference and appropriate
error messages; also introduce a new DTO/interface for partial updates (e.g.,
IUpdateWebhookDto or IPartialWebhookDto) where webhookUrl and webhookSecret are
optional, update the updateWebhook method signature to accept that new type
instead of IWebhookDto, and ensure the repository.updateWebhook call and
validation align with optional fields so callers can update just the secret or
just the URL.
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/webhook/src/webhook.repository.ts (1)
15-32:⚠️ Potential issue | 🟠 MajorMissing
awaiton the Prisma call breaks thetry/catchfor DB errors inregisterWebhook.Line 17 lacks
await:const agentInfo = this.prisma.org_agents.update({...}); // ← PrismaPromise not awaitedBecause the function is
async, it returns a Promise that adopts thePrismaPromise's state, so the happy path is unaffected — callers still receive the DB result. However:
- Any Prisma rejection propagates past the
try/catchentirely. Thethis.logger.erroron line 29 never runs for DB failures.- Contrast with line 23 (
await encryptClientCredential(...)) which IS awaited and therefore properly logged if it throws.- This creates a silent inconsistency: crypto errors are caught and logged; DB errors escape unlogged to the calling service.
updateWebhook(line 44) correctly usesawait— apply the same fix here.🐛 Proposed fix
- const agentInfo = this.prisma.org_agents.update({ + const agentInfo = await this.prisma.org_agents.update({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.repository.ts` around lines 15 - 32, The Prisma update call in registerWebhook is missing an await, so DB errors escape the try/catch and aren't logged; modify registerWebhook to await the Prisma Promise returned by this.prisma.org_agents.update (just like updateWebhook does) so any rejection is caught by the catch block and logged, keeping the existing await on encryptClientCredential unchanged.apps/webhook/src/webhook.service.ts (1)
113-162:⚠️ Potential issue | 🟡 MinorURL-validation error message is swallowed, and invalid URLs are unnecessarily retried.
Two related problems:
Swallowed message (Lines 116–150):
throw new InternalServerErrorException('Invalid or blocked webhook URL')at line 117 is caught by the outercatch (err)at line 147, which rethrows a genericResponseMessages.webhook.error.webhookResponse. The specific reason is logged but lost from the RPC response.Unnecessary retries (Lines 153–162):
AsyncRetryretries every error fromwebhookFunc— including permanent failures like invalid URL. Withretries: 3andminTimeout: 2000, factor: 2this wastes up to ~14 seconds on errors that will never succeed.Consider exiting the outer
tryearly before thecatchwhen the URL is invalid (so theInternalServerErrorExceptionpropagates unchanged), and usingasync-retry'sbailto mark non-retryable errors:♻️ Proposed fix
+import AsyncRetry = require('async-retry'); async webhookFunc(webhookUrl: string, data: object, webhookSecret?: string): Promise<Response> { const isSafeUrl = await isValidWebhookUrl(webhookUrl); if (!isSafeUrl) { throw new InternalServerErrorException('Invalid or blocked webhook URL'); } + try { - const isSafeUrl = await isValidWebhookUrl(webhookUrl); - if (!isSafeUrl) { - throw new InternalServerErrorException('Invalid or blocked webhook URL'); - } // ... rest of signing and fetch logic } catch (err) { this.logger.error(`Error in sending webhook response to org webhook url: ${err}`); throw new InternalServerErrorException(ResponseMessages.webhook.error.webhookResponse); } }And in
webhookResponse, usebailfor non-retryable errors if you prefer a single try/catch inwebhookFunc:// Inside AsyncRetry callback, to mark permanent errors: if (/* permanent error condition */) bail(err);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 113 - 162, The webhookFunc currently swallows the specific "Invalid or blocked webhook URL" error in its catch and causes webhookResponse to retry permanent failures; change webhookFunc to let the InternalServerErrorException for invalid URLs propagate (move the URL check/throw out of the try or rethrow immediately before the outer catch) so the specific error is not replaced by ResponseMessages.webhook.error.webhookResponse, and in webhookResponse use async-retry's bail inside the retry callback to stop retrying for permanent errors (e.g., detect instances of the Invalid/blocked URL exception or other non-retryable exceptions and call bail(err)); also preserve useful error details in logger/error propagation where you rethrow.
🧹 Nitpick comments (1)
libs/common/src/utils/url-validator.util.ts (1)
74-77: Dead code —dns.promises.lookupnever resolves to a falsy address.
dns.promises.lookupeither resolves with{ address: string, family: number }(address is always a non-empty string) or rejects with an error caught by the surroundingcatch. Theif (!address)branch can never be true.♻️ Proposed cleanup
const { address } = await lookup(parsedUrl.hostname); - if (!address) { - logger.warn(`Could not resolve hostname for URL: ${url}`); - return false; - } - if (isPrivateIp(address)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/common/src/utils/url-validator.util.ts` around lines 74 - 77, The conditional checking for a falsy address after calling dns.promises.lookup is dead code because lookup always resolves with a non-empty address or rejects; remove the `if (!address) { logger.warn(...); return false; }` block from the function in url-validator.util.ts (leave the existing try/catch that handles lookup rejections intact) so only the resolved lookup path and the error catch remain; reference the dns.promises.lookup call and the `address` variable when locating the code to delete.
🤖 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/connection/connection.controller.ts`:
- Line 391: The guard using "'webhookUrl' in webhookUrlInfo" allows falsy values
through; change the checks after calling _getWebhookUrl to ensure the webhook
URL is truthy (e.g., webhookUrlInfo?.webhookUrl) before calling
_postWebhookResponse. Specifically, update both occurrences where webhookUrlInfo
is inspected (the call sites that invoke _getWebhookUrl and then call
_postWebhookResponse) to verify webhookUrlInfo?.webhookUrl is a non-empty string
(or otherwise validate with a utility) and skip/return early when it's null,
undefined, or an empty string.
- Line 373: The code calls connectionDto.contextCorrelationId.replace(...) with
no null/undefined guard and uses an unanchored plain-string replace; update both
handlers to first check that connectionDto.contextCorrelationId is defined
(e.g., if (connectionDto.contextCorrelationId) ...) and only then remove the
tenant- prefix using an anchored pattern so only a leading "tenant-" is removed
(i.e., replace the current replace call on connectionDto.contextCorrelationId
with a guarded replace using a ^tenant- anchor).
- Around line 386-397: The current code awaits _getWebhookUrl and
_postWebhookResponse before sending the 201 response, blocking the request
thread; change to fire-and-forget: start the _getWebhookUrl promise without
awaiting it, chain a .then that checks for 'webhookUrl' and invokes
_postWebhookResponse (also without awaiting) and attach .catch handlers to both
to log errors; ensure you do not await these promises before calling res.json()
so the webhook acknowledgment returns immediately (reference symbols:
_getWebhookUrl, webhookUrlInfo variable, _postWebhookResponse, and the
res.json() 201 response).
In `@apps/api-gateway/src/issuance/issuance.controller.ts`:
- Line 970: Change the null/empty check on webhookUrlInfo to use optional
chaining (check webhookUrlInfo?.webhookUrl) instead of the `'webhookUrl' in
webhookUrlInfo` guard so falsy/null/undefined/empty values are handled
correctly; and make the outbound webhook POST fire-and-forget by removing the
await when calling _postWebhookResponse (call it without awaiting before calling
res.json()) so the agent's response is not blocked.
- Line 949: Guard against a missing contextCorrelationId and only strip a
leading "tenant-" prefix: before calling replace on
issueCredentialDto.contextCorrelationId check that the property is defined (and
is a string) and then remove the prefix using a match anchored to the start
(e.g., check .startsWith('tenant-') or use a regex anchored with ^) so you won't
call .replace on undefined and you won't remove "tenant-" from the middle of the
value; update the usage of issueCredentialDto.contextCorrelationId in
issuance.controller.ts accordingly.
In `@apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts`:
- Line 657: The current guard uses "'webhookUrl' in webhookUrlInfo" which can
pass for null/empty values—change the check to use webhookUrlInfo?.webhookUrl to
ensure a real value exists; additionally, do not await the outbound notifier:
call _postWebhookResponse without awaiting so the POST is fire-and-forget and
the handler can immediately call res.json() to respond to the agent (leave
res.json() synchronous and ensure any errors from _postWebhookResponse are
handled internally or logged).
- Line 636: The code uses
oidcIssueCredentialDto.contextCorrelationId.replace('tenant-', '') which can
throw if contextCorrelationId is null/undefined and will remove 'tenant-'
anywhere in the string; update the logic in the oid4vc-issuance.controller
(around oidcIssueCredentialDto handling) to first guard that
oidcIssueCredentialDto.contextCorrelationId is defined, then strip the prefix
only if it actually starts with 'tenant-' (e.g., check startsWith('tenant-') and
remove that leading segment) so you avoid NPEs and only remove the prefix from
the beginning of the value.
In `@apps/webhook/src/webhook.service.ts`:
- Around line 51-54: The responses from registerWebhook and updateWebhook
currently return the AES encrypted ciphertext in webhookSecret; change these
response objects to not expose the ciphertext by removing the webhookSecret
field and instead return a masked indicator such as webhookSecretConfigured:
true (or omit the field entirely). Update the return statements inside the
registerWebhook and updateWebhook functions to replace webhookSecret:
webhookUrl.webhookSecret with webhookSecretConfigured: true so callers get a
clear boolean that a secret is stored without receiving the encrypted value.
- Around line 136-140: The fetch call in webhook.service.ts currently follows
redirects and can be abused for SSRF; update the fetch options in the send
request (the code that calls fetch(webhookUrl, {...})) to disable automatic
redirect following (e.g., set redirect: 'manual' or 'error') and, if you need to
support redirects, explicitly handle 3xx responses by extracting the Location
header and re-running isValidWebhookUrl on the redirected URL before issuing a
new fetch to that validated location; reference the fetch invocation and the
isValidWebhookUrl function when making these changes.
In `@libs/common/src/cast.helper.ts`:
- Around line 461-470: decryptClientCredential (and its counterpart
encryptClientCredential) do not check that process.env.CRYPTO_PRIVATE_KEY is
set, causing CryptoJS.AES to silently use "undefined" as a key; update
decryptClientCredential to guard early: if process.env.CRYPTO_PRIVATE_KEY is
falsy, log a clear error via logger.error and throw a specific Error indicating
the missing CRYPTO_PRIVATE_KEY (or surface this as a startup-level check
elsewhere), and apply the same guard to encryptClientCredential so both
functions fail fast with a descriptive message instead of producing garbage
plaintext or JSON.parse errors; reference the decryptClientCredential and
encryptClientCredential function names when making the change.
In `@libs/common/src/response-messages/index.ts`:
- Around line 571-572: The webhook message strings in the response-messages map
use inconsistent casing; change the values for the keys notFound and
updateWebhook to use title-case "Url" (e.g., "Webhook Url not found" and "Unable
to update Webhook Url") so they match other entries like "Webhook Url updated
successfully" and "Webhook Url registered successfully"; update the string
literals for notFound and updateWebhook accordingly.
- Line 373: The new success message key webhookUrlUpdate was added only under
agent.success, breaking the duplication and pairing pattern; add the same
key/value to ResponseMessages.webhook.success (duplicate the string 'Webhook Url
updated successfully') so consumers reading webhook.success find it, and
consider moving or duplicating the agent.success webhookUrlUpdate entry to keep
the success message co-located with its corresponding error
ResponseMessages.webhook.error.updateWebhook; update both
ResponseMessages.webhook.success and/or remove the mismatch in
ResponseMessages.agent.success to restore symmetry.
In `@libs/common/src/utils/url-validator.util.ts`:
- Around line 54-89: The current isValidWebhookUrl performs DNS resolution and
IP checks but webhookFunc later calls fetch which performs a second DNS lookup,
opening a TOCTOU DNS rebinding window; update the implementation so callers use
the validated IP by (a) resolving the hostname once inside isValidWebhookUrl (or
a new helper) and returning the resolved IP alongside validation result, and (b)
in webhook service (webhookFunc) use an HTTP client/Agent that pins the resolved
IP or connect via a socket to that IP while setting the original hostname in the
Host header (or fetch's headers) to preserve virtual-hosting, or alternatively
add a clear documented warning in isValidWebhookUrl that validation is
best-effort and does not protect against DNS rebinding if the fetch performs a
separate DNS resolution. Ensure references: isValidWebhookUrl and webhookFunc so
reviewers can find and modify the DNS resolution + fetch call to use the pinned
IP/Agent or to add the documented limitation.
- Around line 7-52: The isPrivateIp function misses IPv4-mapped IPv6, the IANA
shared/CGNAT block, and 0.0.0.0/8: update isPrivateIp to (1) detect and
normalize IPv4-mapped IPv6 addresses (strings starting with "::ffff:" or
"::FFFF:") by extracting the mapped IPv4 and running the existing IPv4 checks
against it; (2) add a check for the 100.64.0.0/10 range
(100.64.0.0–100.127.255.255) alongside the other IPv4 private checks; and (3)
treat the 0.0.0.0/8 range as private by matching addresses that start with "0.";
keep IPv6 regexes case-insensitive and ensure these new checks occur before
returning false in isPrivateIp.
---
Outside diff comments:
In `@apps/webhook/src/webhook.repository.ts`:
- Around line 15-32: The Prisma update call in registerWebhook is missing an
await, so DB errors escape the try/catch and aren't logged; modify
registerWebhook to await the Prisma Promise returned by
this.prisma.org_agents.update (just like updateWebhook does) so any rejection is
caught by the catch block and logged, keeping the existing await on
encryptClientCredential unchanged.
In `@apps/webhook/src/webhook.service.ts`:
- Around line 113-162: The webhookFunc currently swallows the specific "Invalid
or blocked webhook URL" error in its catch and causes webhookResponse to retry
permanent failures; change webhookFunc to let the InternalServerErrorException
for invalid URLs propagate (move the URL check/throw out of the try or rethrow
immediately before the outer catch) so the specific error is not replaced by
ResponseMessages.webhook.error.webhookResponse, and in webhookResponse use
async-retry's bail inside the retry callback to stop retrying for permanent
errors (e.g., detect instances of the Invalid/blocked URL exception or other
non-retryable exceptions and call bail(err)); also preserve useful error details
in logger/error propagation where you rethrow.
---
Duplicate comments:
In `@libs/prisma-service/prisma/schema.prisma`:
- Line 240: The schema currently stores webhookSecret as plaintext; change the
design to never persist raw signing secrets: in schema.prisma replace
webhookSecret with a hashed field (e.g., webhookSecretHash String `@db.VarChar`)
or remove it and store only a reference to a secrets manager, then update all
code paths that set or verify the secret to (a) hash the raw secret using a
secure KDF (e.g., HMAC-SHA256 or bcrypt/argon2) before storing into
webhookSecretHash and (b) perform constant-time hash comparisons when verifying
signatures (update any verifyWebhookSignature/createWebhookSecret functions
accordingly); alternatively, if you need retrieval for signing, switch to
storing the secret in a centralized KMS/Secrets Manager and persist only the KMS
reference in the schema.
---
Nitpick comments:
In `@libs/common/src/utils/url-validator.util.ts`:
- Around line 74-77: The conditional checking for a falsy address after calling
dns.promises.lookup is dead code because lookup always resolves with a non-empty
address or rejects; remove the `if (!address) { logger.warn(...); return false;
}` block from the function in url-validator.util.ts (leave the existing
try/catch that handles lookup rejections intact) so only the resolved lookup
path and the error catch remain; reference the dns.promises.lookup call and the
`address` variable when locating the code to delete.
…e-and-forgetk Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
apps/api-gateway/src/connection/connection.controller.ts (1)
434-451:⚠️ Potential issue | 🟠 MajorInconsistent guard and error handling compared to the connection webhook handler.
Two discrepancies with the pattern established in
getConnectionWebhook(lines 386–400):
Line 441: The guard still uses
webhookUrlInfo && 'webhookUrl' in webhookUrlInfo, which passes for{ webhookUrl: null }or{ webhookUrl: '' }. The connection handler was already fixed towebhookUrlInfo?.webhookUrl. Apply the same here.Line 438: The
.catch()doesn't explicitlyreturn null, unlike the connection handler (line 390). While functionally safe (result isundefined, and the&&short-circuits), it's inconsistent. Explicitreturn null+ a type assertion keeps both handlers aligned.Proposed fix
const webhookUrlInfoPromise = this.connectionService ._getWebhookUrl(questionAnswerWebhookDto?.contextCorrelationId, orgId) .catch((error) => { this.logger.debug(`error in getting webhook url ::: ${JSON.stringify(error)}`); + return null; }); - const webhookUrlInfo = await webhookUrlInfoPromise; + const webhookUrlInfo = (await webhookUrlInfoPromise) as { webhookUrl: string; webhookSecret: string } | null; - if (webhookUrlInfo && 'webhookUrl' in webhookUrlInfo) { + if (webhookUrlInfo?.webhookUrl) { this.connectionService ._postWebhookResponse( webhookUrlInfo.webhookUrl,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/connection/connection.controller.ts` around lines 434 - 451, The code calling this.connectionService._getWebhookUrl and then inspecting webhookUrlInfo should mirror getConnectionWebhook: modify the .catch on the _getWebhookUrl call to explicitly return null (so webhookUrlInfo becomes null on error) and adjust the guard before calling this.connectionService._postWebhookResponse to use webhookUrlInfo?.webhookUrl (instead of checking 'webhookUrl' in webhookUrlInfo) so falsy/empty/null webhookUrl is rejected; keep the same logger.debug usage for errors and reference the questionAnswerWebhookDto, _getWebhookUrl, webhookUrlInfo, and _postWebhookResponse symbols when making these changes.apps/webhook/src/webhook.service.ts (3)
83-86: Same encrypted-ciphertext leak asregisterWebhook.
updateWebhookhas the same issue: it returns the AES-encrypted ciphertext stored inwebhookSecret. Apply the same fix (omit the field or return a boolean indicator).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 83 - 86, The updateWebhook response currently returns the stored AES-encrypted ciphertext in updateWebhook.webhookSecret (same leak as registerWebhook); modify the updateWebhook handler to omit webhookSecret from the returned object (or replace it with a boolean like secretStored: true) so no encrypted ciphertext is sent to clients—locate the return that constructs { webhookUrl, webhookSecret } in the updateWebhook function and remove or replace the webhookSecret field to match the fix used for registerWebhook.
142-146:fetchstill follows redirects — SSRF bypass already flagged in a previous review.Setting
redirect: 'error'(orredirect: 'manual') is required to prevent a 3xx redirect from bypassing theisValidWebhookUrlcheck. This was raised in a previous review; it remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 142 - 146, The POST fetch call that uses fetchUrl (inside webhook.service.ts) still allows redirects which can bypass the prior isValidWebhookUrl check; modify the fetch invocation that sets { method: 'POST', headers, body: requestBody } to include redirect: 'error' (or 'manual') in the options so 3xx responses don’t follow redirects, and add handling for the resulting thrown/returned redirect response to surface an error; keep references to fetchUrl, headers, requestBody, and isValidWebhookUrl when making the change so you update the exact fetch call and its error handling in the same function.
51-54: Returning encrypted ciphertext aswebhookSecretin the response.Already flagged in a previous review: the caller submitted a plaintext secret; echoing back the AES ciphertext stored in the DB is confusing and provides no value. Either omit
webhookSecretfrom the response or return a masked indicator such aswebhookSecretConfigured: true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 51 - 54, The response currently returns the stored AES ciphertext via webhookUrl.webhookSecret; change the return shape so it no longer echoes the ciphertext — either remove webhookSecret entirely or replace it with a boolean/masked field such as webhookSecretConfigured: true. Locate the return in webhook.service.ts where webhookUrl is returned and replace the webhookSecret property with webhookSecretConfigured: !!webhookUrl.webhookSecret (or omit the key), keeping webhookUrl.webhookUrl unchanged and preserving the existing return object shape.
🤖 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/verification/verification.controller.ts`:
- Line 472: The variable is being cast incorrectly: change the cast of
webhookUrlInfo (currently "as { webhookUrl: string; webhookSecret: string } |
null") to reflect that webhookSecret can be null (use "webhookSecret: string |
null"), i.e. update the type for the result of webhookUrlInfoPromise so it
matches getWebhookUrl's return shape; locate the assignment using
webhookUrlInfoPromise and adjust the cast/type there (and update any related
references such as webhookUrlInfo and webhookFunc guard expectations if needed).
In `@apps/webhook/src/webhook.service.ts`:
- Around line 113-118: When isValidWebhookUrl returns isSafe: false webhookFunc
should raise a non-retryable error instead of an InternalServerErrorException so
AsyncRetry (used in webhookResponse) does not perform 3 wasted retries; create
or reuse a sentinel error type (e.g., NonRetryableError extends Error) and throw
that from webhookFunc for permanent validation failures (invalid/blocked URL,
missing org), then update the AsyncRetry invocation in webhookResponse to only
retry when the caught error is not an instance of NonRetryableError (i.e., treat
NonRetryableError as terminal and rethrow immediately).
- Around line 120-123: The current IP-pinning logic that builds fetchUrl by
replacing parsedWebhookUrl.hostname with resolvedIp breaks IPv6 (needs
brackets), attempts to override Host (blocked by undici), and wrongly
destructures the return of isValidWebhookUrl (which returns Promise<boolean>);
fix by keeping the original URL (do not rewrite parsedWebhookUrl.host into
fetchUrl), remove any attempt to set the Host header (drop originalHost usage
for headers), and enforce redirect: 'error' on the fetch options to prevent
redirect SSRF; also update isValidWebhookUrl to return an object { isSafe,
resolvedIp } or stop destructuring and await a boolean, and use resolvedIp only
for validation/logging — not for URL rewriting (adjust code around
isValidWebhookUrl, parsedWebhookUrl, fetchUrl, originalHost, and resolvedIp
accordingly).
---
Duplicate comments:
In `@apps/api-gateway/src/connection/connection.controller.ts`:
- Around line 434-451: The code calling this.connectionService._getWebhookUrl
and then inspecting webhookUrlInfo should mirror getConnectionWebhook: modify
the .catch on the _getWebhookUrl call to explicitly return null (so
webhookUrlInfo becomes null on error) and adjust the guard before calling
this.connectionService._postWebhookResponse to use webhookUrlInfo?.webhookUrl
(instead of checking 'webhookUrl' in webhookUrlInfo) so falsy/empty/null
webhookUrl is rejected; keep the same logger.debug usage for errors and
reference the questionAnswerWebhookDto, _getWebhookUrl, webhookUrlInfo, and
_postWebhookResponse symbols when making these changes.
In `@apps/webhook/src/webhook.service.ts`:
- Around line 83-86: The updateWebhook response currently returns the stored
AES-encrypted ciphertext in updateWebhook.webhookSecret (same leak as
registerWebhook); modify the updateWebhook handler to omit webhookSecret from
the returned object (or replace it with a boolean like secretStored: true) so no
encrypted ciphertext is sent to clients—locate the return that constructs {
webhookUrl, webhookSecret } in the updateWebhook function and remove or replace
the webhookSecret field to match the fix used for registerWebhook.
- Around line 142-146: The POST fetch call that uses fetchUrl (inside
webhook.service.ts) still allows redirects which can bypass the prior
isValidWebhookUrl check; modify the fetch invocation that sets { method: 'POST',
headers, body: requestBody } to include redirect: 'error' (or 'manual') in the
options so 3xx responses don’t follow redirects, and add handling for the
resulting thrown/returned redirect response to surface an error; keep references
to fetchUrl, headers, requestBody, and isValidWebhookUrl when making the change
so you update the exact fetch call and its error handling in the same function.
- Around line 51-54: The response currently returns the stored AES ciphertext
via webhookUrl.webhookSecret; change the return shape so it no longer echoes the
ciphertext — either remove webhookSecret entirely or replace it with a
boolean/masked field such as webhookSecretConfigured: true. Locate the return in
webhook.service.ts where webhookUrl is returned and replace the webhookSecret
property with webhookSecretConfigured: !!webhookUrl.webhookSecret (or omit the
key), keeping webhookUrl.webhookUrl unchanged and preserving the existing return
object shape.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api-gateway/src/connection/connection.controller.tsapps/api-gateway/src/issuance/issuance.controller.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.tsapps/api-gateway/src/verification/verification.controller.tsapps/webhook/src/webhook.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api-gateway/src/issuance/issuance.controller.ts
- apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts
- apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
libs/common/src/utils/url-validator.util.ts (1)
70-100:⚠️ Potential issue | 🟠 MajorDNS rebinding window remains unless outbound call pins
resolvedIp.This was previously flagged:
isValidWebhookUrlvalidates one DNS answer, but safety depends on callers actually using Line 100’sresolvedIpduring the HTTP request.Run this read-only check to confirm whether webhook sender logic pins connections to
resolvedIp:#!/bin/bash set -euo pipefail fd "webhook.service.ts" | while read -r file; do echo "== $file ==" rg -n -C4 "isValidWebhookUrl|resolvedIp|fetch\\(|http\\.request|https\\.request|Agent|lookup" "$file" doneExpected result: outbound webhook requests should use a pinned IP/agent path (or clearly document best-effort validation if not).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/common/src/utils/url-validator.util.ts` around lines 70 - 100, isValidWebhookUrl currently resolves a single DNS answer but the DNS-rebinding window remains unless callers pin that resolved IP when making the outbound HTTPS call; update the webhook sender(s) that call isValidWebhookUrl (e.g., webhook sending code in webhook.service.ts) to use the returned resolvedIp for the TCP connection and preserve the original hostname for TLS/HTTP Host header/SNI: set the request to connect to resolvedIp (or use a custom Agent with a lookup that returns resolvedIp) and include the original parsedUrl.hostname as the Host header and TLS servername, ensuring the outbound request is bound to the validated IP rather than relying on DNS at request time.apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts (1)
126-129:⚠️ Potential issue | 🟠 MajorRaw
webhookSecretis still sent in payload here.Line 128 still places plaintext
webhookSecreton the inter-service bus; this remains the same secret-exposure risk noted earlier.🤖 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.service.ts` around lines 126 - 129, The _postWebhookResponse function is sending plaintext webhookSecret in the inter-service payload (payload variable) which exposes the secret; instead, stop including the raw webhookSecret in payload—either replace it with a secure reference/id stored in a secrets store, or send only a derived HMAC/signature of the data (computed locally using webhookSecret) so recipients can verify without receiving the secret, and update the consumer code that expects webhookSecret to use the reference or verify signature accordingly.apps/api-gateway/src/connection/connection.controller.ts (2)
442-448:⚠️ Potential issue | 🟠 MajorUse a value guard (
?.webhookUrl) in question-answer path.Line 442 still uses key-existence (
'webhookUrl' in webhookUrlInfo) instead of checking a usable URL value, unlike the connection handler.✅ Suggested fix
- if (webhookUrlInfo && 'webhookUrl' in webhookUrlInfo) { + if (webhookUrlInfo?.webhookUrl) {Verify no key-existence guard remains in this file:
#!/bin/bash set -euo pipefail rg -n -C2 "webhookUrl' in webhookUrlInfo|webhookUrlInfo\\?\\.webhookUrl" apps/api-gateway/src/connection/connection.controller.tsExpected result: only
webhookUrlInfo?.webhookUrlchecks are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/connection/connection.controller.ts` around lines 442 - 448, The code is using a key-existence check ('webhookUrl' in webhookUrlInfo) instead of a value guard; change the conditional around the call to this.connectionService._postWebhookResponse so it checks webhookUrlInfo?.webhookUrl (truthy URL) before invoking _postWebhookResponse with webhookUrlInfo.webhookUrl and webhookUrlInfo.webhookSecret; also scan the file for any other occurrences of "'webhookUrl' in webhookUrlInfo" and replace them with the value guard to ensure only usable webhook URLs (e.g., references around questionAnswerWebhookDto and the _postWebhookResponse invocation).
387-394:⚠️ Potential issue | 🟠 MajorBoth webhook handlers still block on
_getWebhookUrlbefore returning 201.Line 393 and Line 440 are awaited on the request thread, so acknowledgment can still be delayed under NATS/DB latency.
⚡ Suggested non-blocking flow (apply in both handlers)
- const webhookUrlInfoPromise = this.connectionService - ._getWebhookUrl(connectionDto?.contextCorrelationId, orgId) - .catch((error) => { - this.logger.debug(`error in getting webhook url ::: ${JSON.stringify(error)}`); - return null; - }); - const webhookUrlInfo = (await webhookUrlInfoPromise) as IWebhookUrlInfo | null; - - if (webhookUrlInfo?.webhookUrl) { - this.connectionService - ._postWebhookResponse(webhookUrlInfo.webhookUrl, { data: connectionDto }, webhookUrlInfo.webhookSecret) - .catch((error) => { - this.logger.debug(`error in posting webhook response to webhook url ::: ${JSON.stringify(error)}`); - }); - } + void this.connectionService + ._getWebhookUrl(connectionDto?.contextCorrelationId, orgId) + .then((webhookUrlInfo: IWebhookUrlInfo | null) => { + if (!webhookUrlInfo?.webhookUrl) { + return; + } + return this.connectionService._postWebhookResponse( + webhookUrlInfo.webhookUrl, + { data: connectionDto }, + webhookUrlInfo.webhookSecret + ); + }) + .catch((error) => { + this.logger.debug(`error in webhook dispatch flow ::: ${JSON.stringify(error)}`); + });Also applies to: 435-441, 453-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/connection/connection.controller.ts` around lines 387 - 394, The code awaits _getWebhookUrl (webhookUrlInfoPromise) on the request thread which can delay sending the 201; change both handlers to kick off _getWebhookUrl asynchronously without awaiting before returning the 201—create webhookUrlInfoPromise = this.connectionService._getWebhookUrl(...).catch(...), immediately send the 201 response, then handle the promise in the background (e.g. webhookUrlInfoPromise.then(info => { /* process */ }).catch(err => { this.logger.debug(...) })) so the request acknowledgment is non-blocking; apply this change for the places using webhookUrlInfoPromise / webhookUrlInfo in these handlers.apps/webhook/src/webhook.service.ts (2)
90-93:updateWebhookalso returns the encryptedwebhookSecretciphertext — already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 90 - 93, The current return from updateWebhook includes the encrypted webhookSecret ciphertext (updateWebhook.webhookSecret); remove the ciphertext from the response or replace it with a decrypted/derived secret if you intend to expose the plain secret. Locate the return in webhook.service.ts where updateWebhook is used and either omit webhookSecret from the returned object or call the existing decrypt function (e.g., decryptWebhookSecret or similar) to supply the plaintext only when explicitly required and authorized.
58-61: Returning encryptedwebhookSecretciphertext in response — already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 58 - 61, The current return is exposing the encrypted webhookSecret ciphertext (webhookUrl.webhookSecret); change this to either omit the secret from the response or return the decrypted secret by calling the project’s decryption utility before returning (e.g., const secret = decryptWebhookSecret(webhookUrl.webhookSecret)) and then return webhookUrl: webhookUrl.webhookUrl and webhookSecret: secret; update any callers/tests accordingly to expect the decrypted or omitted value and ensure you reference webhookUrl.webhookUrl and webhookUrl.webhookSecret when locating the change.
🧹 Nitpick comments (2)
apps/webhook/src/webhook.service.ts (2)
132-140: Consider prefixingX-Signaturewith the algorithm name for interoperability.The current value is a raw hex HMAC digest. Most webhook signing conventions (GitHub, Stripe, Shopify) prefix the value with the algorithm, e.g.,
sha256=<hex>. Without it, receivers can't distinguish the algorithm used and future migration to a different digest algorithm would be a silent breaking change.♻️ Proposed change
- headers['X-Signature'] = signature; + headers['X-Signature'] = `sha256=${signature}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 132 - 140, The X-Signature header currently contains a raw hex HMAC; update the code that computes and sets the signature (in webhook.service.ts around the block that calls decryptClientCredential and crypto.createHmac) to prefix the hex digest with the algorithm identifier (e.g., "sha256=" + signature) before assigning to headers['X-Signature'], so receivers can detect the algorithm and future changes are not silently breaking; keep headers['X-Timestamp'] unchanged.
70-100:updateWebhookis a near-identical copy ofregisterWebhook— extract shared logic.Both methods share the same structure: org existence check → repo call → null guard → return
{ webhookUrl, webhookSecret }. The only differences are the repository method name and error message key. This duplication will cause drift over time.♻️ Suggested refactor — private helper
+ private async persistWebhook( + orgId: string, + webhookUrl: string, + webhookSecret: string | undefined, + repoFn: (orgId: string, url: string, secret: string | undefined) => Promise<{ webhookUrl: string; webhookSecret: string }>, + errorMsgKey: string + ): Promise<ICreateWebhookUrl> { + const orgData = await this.webhookRepository.getOrganizationDetails(orgId); + if (!orgData) { + throw new NotFoundException(ResponseMessages.organisation.error.notFound); + } + let result; + try { + result = await repoFn(orgId, webhookUrl, webhookSecret); + } catch { + throw new InternalServerErrorException(errorMsgKey); + } + if (!result) { + throw new InternalServerErrorException(errorMsgKey); + } + return { webhookUrl: result.webhookUrl, webhookSecret: result.webhookSecret }; + } async registerWebhook(dto: IWebhookDto): Promise<ICreateWebhookUrl> { try { - // ... 30 lines ... + return await this.persistWebhook( + dto.orgId, dto.webhookUrl, dto.webhookSecret, + this.webhookRepository.registerWebhook.bind(this.webhookRepository), + ResponseMessages.webhook.error.registerWebhook + ); } catch (error) { this.logger.error(`[registerWebhookUrl] - register webhook url details : ${JSON.stringify(error)}`); throw new RpcException(error.response ? error.response : error); } } async updateWebhook(dto: IWebhookDto): Promise<ICreateWebhookUrl> { try { + return await this.persistWebhook( + dto.orgId, dto.webhookUrl, dto.webhookSecret, + this.webhookRepository.updateWebhook.bind(this.webhookRepository), + ResponseMessages.webhook.error.updateWebhook + ); } catch (error) { this.logger.error(`[updateWebhook] - update webhook details : ${JSON.stringify(error)}`); throw new RpcException(error.response ? error.response : error); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webhook/src/webhook.service.ts` around lines 70 - 100, The updateWebhook method duplicates registerWebhook; extract the shared flow into a private helper (e.g., private async upsertWebhook(dto: IWebhookDto, repoFn: (orgId, url, secret) => Promise<any>, errorMsgKey: string): Promise<ICreateWebhookUrl>) that performs the org existence check using webhookRepository.getOrganizationDetails, calls the provided repoFn (map to webhookRepository.updateWebhook or webhookRepository.registerWebhook), handles repository errors by throwing InternalServerErrorException(ResponseMessages.webhook.error[errorMsgKey]) and null results, and returns { webhookUrl, webhookSecret }; then simplify updateWebhook and registerWebhook to call this helper and keep logging/exception wrapping behavior consistent.
🤖 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/connection/connection.service.ts`:
- Around line 131-134: In _postWebhookResponse, stop including plaintext
webhookSecret in the NATS payload; instead send a non-secret identifier (e.g.,
secretId or tenantId) and any necessary lookup metadata, update the payload
construction in _postWebhookResponse (and the similar payloads in oid4vcIssuance
flow and oid4vcVerification flow) to replace webhookSecret with secretId, and
update the webhook consumer/service to resolve the actual secret from secure
storage using that identifier before sending the external webhook; ensure no
code paths log or trace the secret value and add tests/mocks to verify secret
resolution happens only on the webhook service side.
In `@apps/api-gateway/src/issuance/issuance.controller.ts`:
- Around line 966-973: The current code awaits
issueCredentialService._getWebhookUrl (webhookUrlInfoPromise -> webhookUrlInfo)
before returning the 201 response, blocking the request; instead kick off the
URL lookup as a background task and do not await it before sending the response:
keep creating webhookUrlInfoPromise but remove the await and instead attach a
.then/.catch (or use void async IIFE) to handle the result and log errors
asynchronously so the controller returns the 201 immediately; refer to
issueCredentialService._getWebhookUrl, webhookUrlInfoPromise, webhookUrlInfo and
the controller return point to implement the non-blocking pattern.
In `@apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts`:
- Around line 653-660: The handler is awaiting
this.oid4vcIssuanceService._getWebhookUrl (webhookUrlInfoPromise ->
webhookUrlInfo) which blocks returning 201; change to fire-and-forget: start the
_getWebhookUrl call and then immediately return the 201 response without
awaiting its result, and likewise invoke
this.oid4vcIssuanceService._postWebhookResponse as non-blocking (do not await)
so both calls run asynchronously; ensure any errors from _getWebhookUrl or
_postWebhookResponse are caught and logged inside their own promise chains (or
by async IIFEs) so unhandled rejections are avoided, referencing the existing
webhookUrlInfoPromise, webhookUrlInfo, _getWebhookUrl and _postWebhookResponse
symbols to locate the code to change.
In `@libs/common/src/utils/url-validator.util.ts`:
- Around line 73-97: The logs currently emit the raw webhook URL (variable url)
which may contain sensitive tokens; change each logger.warn call that uses url
(the warnings around DEV skip, non-HTTPS protocol, unresolved hostname, and
private/internal IP) to log a redacted version instead: create a local sanitized
string (e.g., redactedUrl) by parsing url with new URL(url) and returning only
origin and pathname or by stripping/replacing query and fragment (and optionally
masking path segments), then use redactedUrl in logger.warn messages (also
include the resolved address when needed by logging e.g. `${redactedUrl} ->
${address}`); update all occurrences where logger.warn references url (and any
error logs) to use the new redactedUrl helper so no full URLs with tokens are
emitted.
- Around line 52-55: The current private-IP check only rejects the literal
'::1'; update the check around the ip variable in url-validator.util.ts to also
treat the expanded IPv6 loopback and unspecified forms as private by matching
'0:0:0:0:0:0:0:1', '::', and '0:0:0:0:0:0:0:0' (or normalize the input first and
then check) so all loopback/unspecified IPv6 variants are blocked; update the
conditional that now reads if ('::1' === ip) to include these additional
representations or perform a normalization step before comparison.
---
Duplicate comments:
In `@apps/api-gateway/src/connection/connection.controller.ts`:
- Around line 442-448: The code is using a key-existence check ('webhookUrl' in
webhookUrlInfo) instead of a value guard; change the conditional around the call
to this.connectionService._postWebhookResponse so it checks
webhookUrlInfo?.webhookUrl (truthy URL) before invoking _postWebhookResponse
with webhookUrlInfo.webhookUrl and webhookUrlInfo.webhookSecret; also scan the
file for any other occurrences of "'webhookUrl' in webhookUrlInfo" and replace
them with the value guard to ensure only usable webhook URLs (e.g., references
around questionAnswerWebhookDto and the _postWebhookResponse invocation).
- Around line 387-394: The code awaits _getWebhookUrl (webhookUrlInfoPromise) on
the request thread which can delay sending the 201; change both handlers to kick
off _getWebhookUrl asynchronously without awaiting before returning the
201—create webhookUrlInfoPromise =
this.connectionService._getWebhookUrl(...).catch(...), immediately send the 201
response, then handle the promise in the background (e.g.
webhookUrlInfoPromise.then(info => { /* process */ }).catch(err => {
this.logger.debug(...) })) so the request acknowledgment is non-blocking; apply
this change for the places using webhookUrlInfoPromise / webhookUrlInfo in these
handlers.
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts`:
- Around line 126-129: The _postWebhookResponse function is sending plaintext
webhookSecret in the inter-service payload (payload variable) which exposes the
secret; instead, stop including the raw webhookSecret in payload—either replace
it with a secure reference/id stored in a secrets store, or send only a derived
HMAC/signature of the data (computed locally using webhookSecret) so recipients
can verify without receiving the secret, and update the consumer code that
expects webhookSecret to use the reference or verify signature accordingly.
In `@apps/webhook/src/webhook.service.ts`:
- Around line 90-93: The current return from updateWebhook includes the
encrypted webhookSecret ciphertext (updateWebhook.webhookSecret); remove the
ciphertext from the response or replace it with a decrypted/derived secret if
you intend to expose the plain secret. Locate the return in webhook.service.ts
where updateWebhook is used and either omit webhookSecret from the returned
object or call the existing decrypt function (e.g., decryptWebhookSecret or
similar) to supply the plaintext only when explicitly required and authorized.
- Around line 58-61: The current return is exposing the encrypted webhookSecret
ciphertext (webhookUrl.webhookSecret); change this to either omit the secret
from the response or return the decrypted secret by calling the project’s
decryption utility before returning (e.g., const secret =
decryptWebhookSecret(webhookUrl.webhookSecret)) and then return webhookUrl:
webhookUrl.webhookUrl and webhookSecret: secret; update any callers/tests
accordingly to expect the decrypted or omitted value and ensure you reference
webhookUrl.webhookUrl and webhookUrl.webhookSecret when locating the change.
In `@libs/common/src/utils/url-validator.util.ts`:
- Around line 70-100: isValidWebhookUrl currently resolves a single DNS answer
but the DNS-rebinding window remains unless callers pin that resolved IP when
making the outbound HTTPS call; update the webhook sender(s) that call
isValidWebhookUrl (e.g., webhook sending code in webhook.service.ts) to use the
returned resolvedIp for the TCP connection and preserve the original hostname
for TLS/HTTP Host header/SNI: set the request to connect to resolvedIp (or use a
custom Agent with a lookup that returns resolvedIp) and include the original
parsedUrl.hostname as the Host header and TLS servername, ensuring the outbound
request is bound to the validated IP rather than relying on DNS at request time.
---
Nitpick comments:
In `@apps/webhook/src/webhook.service.ts`:
- Around line 132-140: The X-Signature header currently contains a raw hex HMAC;
update the code that computes and sets the signature (in webhook.service.ts
around the block that calls decryptClientCredential and crypto.createHmac) to
prefix the hex digest with the algorithm identifier (e.g., "sha256=" +
signature) before assigning to headers['X-Signature'], so receivers can detect
the algorithm and future changes are not silently breaking; keep
headers['X-Timestamp'] unchanged.
- Around line 70-100: The updateWebhook method duplicates registerWebhook;
extract the shared flow into a private helper (e.g., private async
upsertWebhook(dto: IWebhookDto, repoFn: (orgId, url, secret) => Promise<any>,
errorMsgKey: string): Promise<ICreateWebhookUrl>) that performs the org
existence check using webhookRepository.getOrganizationDetails, calls the
provided repoFn (map to webhookRepository.updateWebhook or
webhookRepository.registerWebhook), handles repository errors by throwing
InternalServerErrorException(ResponseMessages.webhook.error[errorMsgKey]) and
null results, and returns { webhookUrl, webhookSecret }; then simplify
updateWebhook and registerWebhook to call this helper and keep logging/exception
wrapping behavior consistent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/api-gateway/src/connection/connection.controller.tsapps/api-gateway/src/connection/connection.service.tsapps/api-gateway/src/issuance/issuance.controller.tsapps/api-gateway/src/issuance/issuance.service.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.tsapps/api-gateway/src/verification/verification.controller.tsapps/api-gateway/src/verification/verification.service.tsapps/webhook/src/webhook.service.tslibs/common/src/interfaces/webhook.interface.tslibs/common/src/response-messages/index.tslibs/common/src/utils/url-validator.util.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api-gateway/src/verification/verification.controller.ts
- apps/api-gateway/src/issuance/issuance.service.ts
- apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts
- apps/api-gateway/src/verification/verification.service.ts
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
522-522:⚠️ Potential issue | 🟡 MinorImprove error serialization in webhook dispatch log.
At Line 522,
JSON.stringify(error)can hide realErrordetails ({}). Logmessage/stackfirst for actionable ops visibility.🛠️ Suggested patch
- this.logger.error(`error in webhook dispatch flow ::: ${JSON.stringify(error)}`); + this.logger.error( + `error in webhook dispatch flow ::: ${error?.message ?? error?.stack ?? JSON.stringify(error)}` + );🤖 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` at line 522, The current webhook dispatch error log uses JSON.stringify(error) which can produce "{}" for Error objects; update the logger call in the webhook dispatch flow to log error.message and error.stack (e.g., include both in the log string) and fall back to a safe serializer like util.inspect(error) or JSON.stringify(error, Object.getOwnPropertyNames(error)) if message/stack are missing; adjust the call site using this.logger.error in the webhook dispatch function so logs include both a descriptive prefix and the error.message and error.stack for actionable debugging.
🤖 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/verification/verification.controller.ts`:
- Line 480: Replace the debug-level JSON.stringify logging with an error-level
log that preserves the real Error object and stack trace; specifically change
the this.logger.debug call in the webhook dispatch flow inside
VerificationController to use this.logger.error and pass the Error (or
error.stack) and contextual message so the logger emits the full message/stack
instead of a stringified object.
---
Duplicate comments:
In `@apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts`:
- Line 522: The current webhook dispatch error log uses JSON.stringify(error)
which can produce "{}" for Error objects; update the logger call in the webhook
dispatch flow to log error.message and error.stack (e.g., include both in the
log string) and fall back to a safe serializer like util.inspect(error) or
JSON.stringify(error, Object.getOwnPropertyNames(error)) if message/stack are
missing; adjust the call site using this.logger.error in the webhook dispatch
function so logs include both a descriptive prefix and the error.message and
error.stack for actionable debugging.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api-gateway/src/connection/connection.controller.tsapps/api-gateway/src/issuance/issuance.controller.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.tsapps/api-gateway/src/verification/verification.controller.tslibs/common/src/utils/url-validator.util.ts
✅ Files skipped from review due to trivial changes (1)
- libs/common/src/utils/url-validator.util.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts
- apps/api-gateway/src/issuance/issuance.controller.ts
- apps/api-gateway/src/connection/connection.controller.ts
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
Signed-off-by: Sagar Khole <sagar.khole@ayanworks.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement webhook signing functionality and update endpoints
Summary by CodeRabbit
New Features
Chores
link