feat: OID4VC, OID4VP and X509 capabilities - experimental feature #1517
Conversation
RinkalBhojani
requested review from
GHkrishna,
ankita-p17,
shitrerohit and
tipusinghaw
WalkthroughAdds OIDC4VC issuance and verification plus X.509 services across the monorepo: new API Gateway modules/controllers/services/DTOs, new microservices (issuance, verification, x509) with NATS message patterns and repositories, Dockerfiles, CI matrix entries, env seeds and feature flag, Prisma migrations/seed data, many DTOs/interfaces, and a broad NATS-call refactor to direct natsClient usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant API as API Gateway
participant IssSvc as Oid4vcIssuanceService
participant NATS as NATS
participant Agent as Agent
participant DB as Database
Client->>API: POST /orgs/:orgId/openid4vc/issuer
API->>IssSvc: validate & call oidcIssuerCreate(orgId, dto, user)
IssSvc->>NATS: send(create-oid4vc-issuer, payload)
NATS->>Agent: forward to agent microservice
Agent-->>NATS: reply (issuer created)
NATS-->>IssSvc: response
IssSvc->>DB: persist issuer metadata
IssSvc-->>API: return created issuer
API-->>Client: 201 Created + issuer
sequenceDiagram
participant Client as API Client
participant API as API Gateway
participant X509Svc as X509Service
participant Repo as X509Repository
participant NATS as NATS
participant Agent as Agent
Client->>API: POST /orgs/:orgId/x509
API->>X509Svc: createX509(orgId, dto, user)
X509Svc->>Repo: hasDateCollision(options)
alt no collision
X509Svc->>NATS: send('create-x509-certificate', payload)
NATS->>Agent: request certificate generation
Agent-->>NATS: certificate data
NATS-->>X509Svc: cert data
X509Svc->>Repo: create(record)
X509Svc-->>API: 201 + certificate
API-->>Client: 201 + certificate
else collision
X509Svc-->>API: 409 Conflict
API-->>Client: 409 Conflict
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 57
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.service.ts (2)
28-54: Fix return type inconsistency inregisterWebhook.The method signature declares
Promise<ICreateWebhookUrl>but line 47 returnswebhookUrl.webhookUrl(a string property). This creates a type mismatch between the declared return type and the actual return value.Apply this diff to return the complete object:
- return webhookUrl.webhookUrl; + return webhookUrl;Or update the method signature if returning just the URL string is intentional:
-async registerWebhook(registerWebhookDto: IWebhookDto): Promise<ICreateWebhookUrl> { +async registerWebhook(registerWebhookDto: IWebhookDto): Promise<string> {
56-70: Fix return type inconsistency ingetWebhookUrl.Similar to
registerWebhook, the method signature declaresPromise<IGetWebhookUrl>but line 65 returnswebhookUrlInfo.webhookUrl(a string property), creating a type mismatch.Apply this diff to return the complete object:
- return webhookUrlInfo.webhookUrl; + return webhookUrlInfo;Or update the method signature if returning just the URL string is intentional:
-async getWebhookUrl(getWebhook: IWebhookUrl): Promise<IGetWebhookUrl> { +async getWebhookUrl(getWebhook: IWebhookUrl): Promise<string> {
🧹 Nitpick comments (40)
apps/issuance/src/issuance.service.ts (1)
1519-1521: LGTM! Type annotation improves type safety.The explicit type annotation for the
msparameter is a good improvement for type safety. The method implementation is correct.As an optional refinement, consider changing the return type from
Promise<unknown>toPromise<void>for consistency with the localdelayfunction defined at line 1533, since this method doesn't resolve with any meaningful value:-async delay(ms: number): Promise<unknown> { +async delay(ms: number): Promise<void> { return new Promise((resolve) => setTimeout(resolve, ms)); }apps/api-gateway/src/connection/connection.service.ts (2)
39-49: Apply consistent error handling pattern.The
sendBasicMessagemethod still uses the old error handling pattern without the safe fallback. For consistency with the updatedsendQuestionmethod and to prevent potential loss of error context, apply the same pattern here.Apply this diff:
} catch (error) { - throw new RpcException(error.response); + throw new RpcException(error?.response ?? error); }
56-65: Apply consistent error handling pattern.The
getUrlmethod also uses the old error handling pattern. Apply the same safe fallback pattern for consistency across the file.Apply this diff:
} catch (error) { - throw new RpcException(error.response); + throw new RpcException(error?.response ?? error); }apps/api-gateway/src/connection/enums/connections.enum.ts (1)
1-4: Consider consolidating duplicate enum definitions.The
HandshakeProtocolenum is duplicated identically in bothapps/api-gateway/src/connection/enums/connections.enum.tsandapps/api-gateway/src/cloud-wallet/enums/connections.enum.ts. Consolidating into a shared location (e.g., a common enums directory) would reduce maintenance overhead and prevent potential divergence.apps/ledger/src/schema/schema.service.ts (1)
405-405: Consider addressing these TODOs or tracking them in an issue.The TODO comments correctly identify that the
map((response) => ({ response }))pattern adds unnecessary nesting. The string response from NATS is wrapped in an object, only to be unwrapped by callers accessing.response. This could be simplified by either:
- Returning the response directly and updating return types
- Removing the map operator entirely if no transformation is needed
However, these TODOs appear unrelated to the PR's main objectives (OID4VC/X509 features).
Would you like me to:
- Generate a refactored implementation that removes this nested mapping across all three methods?
- Open a separate issue to track this technical debt for future cleanup?
Also applies to: 433-433, 558-558
apps/utility/src/utilities.repository.ts (1)
13-28: Add type annotation for thepayloadparameter.The
payloadparameter lacks a type annotation, reducing type safety and IDE support. Consider defining an interface or type for the expected shape.Apply this diff to add a type annotation:
+interface ShorteningUrlPayload { + referenceId: string; + invitationPayload: unknown; +} + - async saveShorteningUrl(payload): Promise<object> { + async saveShorteningUrl(payload: ShorteningUrlPayload): Promise<object> {apps/webhook/src/webhook.service.ts (1)
72-90: Consider logging error details in the catch block.The catch block at line 87-89 doesn't log the actual error details before throwing the exception, which could make debugging webhook failures more difficult.
Apply this diff to improve error visibility:
} catch (err) { + this.logger.error(`Error in webhookFunc: ${JSON.stringify(err)}`); throw new InternalServerErrorException(ResponseMessages.webhook.error.webhookResponse); }libs/context/src/contextModule.ts (2)
18-19: Remove redundant camelCase header check.HTTP header names are case-insensitive, but Express (used by NestJS) normalizes all header names to lowercase. The check for
req.headers['contextId'](camelCase) will never match because Express converts it to'contextid'.Simplify to use only lowercase header keys:
- const contextIdHeader = req.headers['contextid'] ?? req.headers['context-id'] ?? req.headers['contextId']; + const contextIdHeader = req.headers['contextid'] ?? req.headers['context-id']; const correlationIdHeader = req.headers['x-correlation-id'];
24-31: Use structured logging instead of console.log.The direct
console.logstatements bypass the application's logging infrastructure. Using a proper logger would provide structured logging, appropriate log levels, and better integration with observability tools—especially important for this experimental feature.Consider using the application's logger service:
import { Logger } from '@nestjs/common'; // Inside the module or as a constant const logger = new Logger('ContextModule'); // In the idGenerator function if (resolvedContextId) { logger.log(`ContextId received in request headers: ${resolvedContextId}`); } else { resolvedContextId = v4(); logger.log(`ContextId not received in request headers, generated: ${resolvedContextId}`); }Alternatively, if this is intentional for debugging the experimental feature, consider using
logger.debug()so it can be toggled via log level configuration.libs/context/src/contextInterceptorModule.ts (2)
19-19: Move Logger instantiation to module scope.Creating a new
Loggerinstance on every request adds unnecessary overhead. SinceidGeneratoris invoked for each incoming request, this could degrade performance under load.Apply this diff to move the Logger to module scope:
+const logger = new Logger('ContextInterceptorModule'); + @Global() @Module({ imports: [ ClsModule.forRoot({ global: true, interceptor: { mount: true, generateId: true, idGenerator: (context: ExecutionContext) => { try { - const logger = new Logger('ContextInterceptorModule'); const rpcContext = context.switchToRpc().getContext();
20-22: Inconsistent defensive patterns for header handling - consider normalizing approach.The review comment's observation about type inconsistency is confirmed. The codebase uses two different defensive patterns:
- logging.interceptor.ts (line 27-30): Guard-based approach using
isNullUndefinedOrEmpty()check before calling.get()- contextInterceptorModule.ts (line 21-22): Defensive fallback + optional chaining approach with
?? {}andheaders.get?.()Both patterns handle the case where
getHeaders()can returnnull/undefined, but the inconsistency suggests either:
getHeaders()may returnnull/undefinedin RPC contexts- Or there's uncertainty about the return type across the codebase
Both implementations work correctly, but for improved consistency and type safety, consider normalizing to one pattern across both files. The logging.interceptor.ts pattern is slightly more explicit about the guard condition.
apps/cloud-wallet/src/cloud-wallet.service.ts (1)
43-47: Remove unused dependency registrations from CloudWalletModule.The constructor cleanup is correct—
cloudWalletServiceProxyandcacheServiceare not referenced in any service methods. However, the module configuration still registers unused dependencies:NATS_CLIENTandCacheModule.register()are imported and registered but no longer injected intoCloudWalletService. Remove these unused registrations fromapps/cloud-wallet/src/cloud-wallet.module.tsto avoid instantiating unused providers on app boot.apps/api-gateway/src/agent-service/dto/create-schema.dto.ts (1)
29-31: Consider validating orgId as a string, not just non-empty
orgIdcurrently only has@IsNotEmpty, unlike the other string fields which also use@IsString. For consistency and stricter runtime validation, it’s better to enforce the type as well.Suggested change:
@ApiProperty() - @IsNotEmpty({ message: 'please provide orgId' }) - orgId: string; + @IsString({ message: 'orgId must be a string' }) + @IsNotEmpty({ message: 'please provide orgId' }) + orgId: string;If you want to confirm that this pattern matches current best practices for
class-validator, you can reuse the same documentation lookup as in the previous comment.libs/prisma-service/prisma/migrations/20250904181430_added_fields_in_oidc_issuer/migration.sql (1)
4-6: Consider if array type is necessary foraccessTokenSignerKeyType.The
AccessTokenSignerKeyTypeENUM currently has only one value ('ed25519'), and the column is defined as an array. If multiple signer key types per issuer are not immediately required, a scalar type might be simpler.libs/prisma-service/prisma/migrations/20250822104325_oidc_issuer_templates/migration.sql (1)
4-17: Consider using JSONB instead of JSON for better performance.The
attributesandappearancecolumns are defined asJSONtype. PostgreSQL'sJSONBtype offers better performance for querying and indexing. If later migrations update these toJSONB, this comment can be ignored.apps/issuance/constant/issuance.ts (1)
4-4: Verify if emptycredentialConfigurationsSupportedis intentional.The
credentialConfigurationsSupportedconstant is an empty object. This may be a placeholder for future implementation or work in progress.Do you want me to track this as a TODO or is this intentionally empty for the experimental feature?
apps/api-gateway/src/helper-files/file-operation.helper.ts (1)
16-26: Async wrappers forgetFile/deleteFilelook fine
getFileanddeleteFilecorrectly wrapfs.readFile/fs.unlinkwithpromisifyand expose promise-based APIs. No functional concerns here. You could optionally hoist the promisified functions to module scope to avoid re-creating them on each call, but that’s not critical.apps/connection/src/connection.controller.ts (1)
49-52: Confirm whetheruser/userIdshould be forwarded to the serviceIn both handlers:
- Line 49:
getConnectionListFromAgent(payload: GetAllConnections)– payload includesuser, but onlyorgIdandconnectionSearchCriteriaare forwarded.- Line 67:
getConnectionRecordsByOrgId(payload: { orgId: string; userId: string })–userIdis accepted but never used.If authorization/auditing is expected at the service layer, these fields might need to be forwarded; otherwise they can be dropped from the payload types to avoid confusion.
Also applies to: 67-70
apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts (1)
1-47: Template interfaces are coherent; consider type-only Prisma importThe SdJwt/Mdoc template and credential attribute structures look consistent and expressive for OID4VC template handling, including recursive
childrenandappearanceasPrisma.JsonValue.Since
PrismaandSignerOptionare used purely as types here, you could switch to a type-only import to avoid unnecessary runtime dependency wiring:-import { Prisma, SignerOption } from '@prisma/client'; +import type { Prisma, SignerOption } from '@prisma/client';This is a minor build/runtime cleanliness improvement.
apps/oid4vc-issuance/interfaces/oid4vc-wh-interfaces.ts (1)
1-48: AlignCredentialPayloadnaming and verifycredDefidspellingThere are a couple of potential confusion points here:
This file exports
CredentialPayloadwith specific fields (orgId,schemaId?,credDefid?, etc.), butapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tsalready defines another exportedCredentialPayloadwith a different shape. Sharing the same name for different concepts in the same bounded context makes mis-imports very likely.The property
credDefid(line 24) looks like it may be intended ascredDefId. If other parts of the codebase usecredDefId, this mismatch will silently produceundefinedat runtime.Consider either:
- Renaming one of the interfaces (e.g.,
IssuerSessionCredentialPayloadvsWebhookCredentialPayload), or- Unifying them if they are meant to represent the same structure.
And please verify the
credDefidkey name across usages.libs/common/src/response-messages/index.ts (1)
506-622: New OIDC/OID4VP/x509 message blocks look good; consider minor cleanupThe added
oidcIssuer,oidcTemplate,oidcIssuerSession,oid4vp,oid4vpSession, andx509message groups provide good coverage of the new flows’ success and error cases and align with the rest of the catalog.A couple of small nits you may want to address:
oidcTemplate.success.deleteTemplateis a literal string containing${templateId}. Unless you explicitly.replaceor interpolate this at call sites, logs/clients will see the placeholder verbatim. You might prefer a static message (e.g.,'[createTemplate] compensating delete succeeded') and log the templateId separately, or move to a helper that formats the message with a parameter.
x509.error.errorActivationcurrently reads'Failed to activate x509 certificate..'(double period). Dropping one dot would improve polish.These are cosmetic improvements and don’t block functionality.
.env.sample (1)
70-71: Address formatting issues for consistency.The static analysis tool flags ordering and formatting inconsistencies. While the comment is helpful, consider moving this variable to maintain alphabetical grouping with other configuration flags.
-POSTGRES_DATABASE= // Please provide your DB name +POSTGRES_DATABASE= // Please provide your DB name + # Default is true too, set to 'false', to view OIDC controllers in openAPI docs HIDE_EXPERIMENTAL_OIDC_CONTROLLERS=true -SENDGRID_API_KEY=xxxxxxxxxxxxxx // Please provide your sendgrid API key + +SENDGRID_API_KEY=xxxxxxxxxxxxxx // Please provide your sendgrid API key.env.demo (1)
147-148: Consider ordering for consistency.Static analysis suggests moving
HIDE_EXPERIMENTAL_OIDC_CONTROLLERSearlier in the file for better organization. While this is a demo file and the flag is functional, maintaining consistent ordering with.env.samplewould improve maintainability.apps/api-gateway/src/oid4vc-verification/oid4vc-verification.module.ts (1)
26-26: Clarify module naming convention.The module is named
Oid4vpModulebut the file isoid4vc-verification.module.tsand the controller/service useOid4vcVerificationprefix. Consider renaming toOid4vcVerificationModulefor consistency across the codebase.Apply this diff to align the naming:
-export class Oid4vpModule {} +export class Oid4vcVerificationModule {}libs/prisma-service/prisma/data/credebl-master-table.json (1)
3-20: Treat this file strictly as template data; avoid real secrets and env‑specific IDs.Values like
sgApiKey,password,externalIp, andapiEndpointare clearly placeholders, but the presence of a concretesupabaseUserIdsuggests this file could accidentally be edited with real tenant‑specific data. Please ensure:
- No real API keys, passwords, or tenant identifiers are ever committed here.
- If this is used as a seed template, document that deployers must override these values via environment‑specific seed files or env variables rather than editing this one in‑repo.
Also applies to: 75-138, 170-177
apps/oid4vc-issuance/src/oid4vc-issuance.module.ts (1)
1-36: Module wiring looks consistent; consider adding MICRO_SERVICE_NAME for observability consistency.The NATS setup and imports align with existing patterns (e.g., IssuanceModule). To keep logging/metrics consistent across services, you may want to also provide
MICRO_SERVICE_NAMEhere (similar to Issuance/Agent modules) so downstream code can tag logs with a stable service identifier.Dockerfiles/Dockerfile.oid4vc-verification (1)
1-45: Image is functional; consider separating migrations from app startup.The multi‑stage build looks fine and reuses the pattern used elsewhere. Two things to consider:
CMDrunsprisma migrate deployandprisma generateon every container start. In multi‑replica setups this can cause redundant or conflicting migration runs. It’s usually safer to run migrations in a dedicated job/init container and keep the app container focused onnode dist/.../main.js.- You install dependencies with
pnpmbut build vianpm run build. That’s valid (sincenpm runjust invokes the script and uses the existingnode_modules), but it’s worth double‑checking that your root scripts don’t assume a lockfile or a different package manager at runtime.apps/oid4vc-verification/src/oid4vc-verification.module.ts (1)
1-52: Avoid duplicating PrismaService in both module imports and providers.You import
PrismaServiceModuleand also listPrismaServicedirectly inproviders. Nest will tolerate this, but it’s redundant and can be confusing.I’d suggest either:
- Keep
PrismaServiceModuleinimportsand dropPrismaServicefromproviders, or- Remove
PrismaServiceModuleand only providePrismaServicedirectly (matching your pattern in other services).The rest of the module wiring (NATS client, MICRO_SERVICE_NAME, common infra modules) looks consistent.
apps/oid4vc-issuance/constant/issuance.ts (1)
4-4: Clarify the purpose of the empty object.
credentialConfigurationsSupportedis exported as an empty object. If this is a placeholder for future configuration, consider adding a TODO comment or type annotation to document its intended structure.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vp-presentation-wh.dto.ts (1)
32-38: Add @IsString decorator to optional string fields.The optional fields
typeandorgIdare missing the@IsString()decorator. While@IsOptional()allows these fields to be omitted, when they are provided, they should still be validated as strings for consistency with the required fields.Apply this diff:
@ApiPropertyOptional() @IsOptional() + @IsString() type: string; @ApiPropertyOptional() @IsOptional() + @IsString() orgId: string;apps/api-gateway/src/verification/dto/webhook-proof.dto.ts (1)
11-13: Consider a more specific type for metadata.The
metadataproperty is typed asobject, which provides minimal type safety. If the structure of metadata is known, consider defining an interface or using a more specific type likeRecord<string, unknown>.apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier.dto.ts (1)
6-20: DTOs look consistent; consider stricter validation forlogo_uri.The DTOs line up cleanly with the shared
ClientMetadata/CreateVerifiercontracts, and nested validation is wired correctly. If you want to tighten validation a bit, you could add@IsUrl()onlogo_uri(and keep@IsString()if desired) to fail fast on invalid logo URLs, but that’s optional.Also applies to: 22-42, 44-44
apps/x509/src/interfaces/x509.interface.ts (1)
4-13: Alignstatustype withx5cRecordStatusfor consistency.
CreateX509CertificateEntity.statusis typed asstringwhile you already usex5cRecordStatuselsewhere and import it here. Using the enum here as well would improve type-safety and keep the X.509 model consistent:export interface CreateX509CertificateEntity { orgId: string; // We'll accept orgId and find orgAgent internally keyType: x5cKeyType; - status: string; + status: x5cRecordStatus; validFrom: Date; expiry: Date; certificateBase64: string; createdBy: string; lastChangedBy: string; }apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (3)
69-104: Align documented vs actual HTTP status codes for create/update verifier.Both verifier create/update endpoints declare
@ApiResponse({ status: HttpStatus.OK, ... })but actually returnHttpStatus.CREATEDand setstatusCode: HttpStatus.CREATEDin the payload. This mismatch will confuse API consumers and tooling that rely on the OpenAPI spec.Consider either:
- changing the decorators to
HttpStatus.CREATED, or- returning
HttpStatus.OK(200) andstatusCode: HttpStatus.OK, especially for the update endpoint where 200 is more conventional.Also applies to: 114-155
208-249: Use an appropriate success message for delete verifier.The delete endpoint returns
ResponseMessages.oid4vp.success.fetcheven though it performs a delete. That’s likely a copy‑paste from the GET handler and could be misleading for clients or logs.If you have a dedicated delete message key (e.g.
success.delete), it’d be clearer to use that here; otherwise, at least consider a more delete‑specific message string.
421-459: Webhook handler is consistent with issuance flow; consider optional sanitization.The webhook endpoint mirrors the OID4VC issuance controller pattern (type tagging,
orgIdfallback oncontextCorrelationId === 'default', service delegation, and optional org webhook repost). From a behavior standpoint that’s consistent.If you expect high-sensitivity data in
oid4vpPresentationWhDto, you may later want a sanitization step before logging or reposting to external webhooks, but that’s more of a hardening pass than a blocker right now.libs/common/src/interfaces/oid4vp-verification.ts (1)
1-26: Interfaces are coherent; you can reuseClientMetadatainVerifierRecord.The interfaces nicely mirror the DTOs and service expectations. To avoid duplicating the shape of client metadata, you could slightly tighten
VerifierRecord:export interface VerifierRecord { _tags: Record<string, unknown>; metadata: Record<string, unknown>; id: string; createdAt: string; // ISO timestamp verifierId: string; - clientMetadata: { - client_name: string; - logo_uri: string; - }; + clientMetadata: ClientMetadata; updatedAt: string; // ISO timestamp }Not required, but it keeps types DRY and easier to evolve.
apps/oid4vc-verification/src/oid4vc-verification.controller.ts (1)
1-105: Microservice handlers look clean and aligned with the gateway/service contracts.The message handlers are thin, log useful context, and delegate cleanly to
Oid4vpVerificationServicewith correctly typed payloads. The overall structure matches the API-gateway controller semantics, which should keep the OID4VP flow easy to trace end‑to‑end. You can revisit the TODO about renaming the'oid4vp-verification-session-create'command later, but nothing blocking here.apps/agent-service/src/agent-service.service.ts (1)
1426-1556: Consider extracting a common HTTP call helper.All OIDC methods (oidcIssuerCreate, deleteOidcIssuer, oidcGetIssuerById, etc.) follow an identical pattern:
- Get organization API key
- Make HTTP call (POST/GET/PUT/DELETE)
- Return response
- Log and rethrow errors
This could be refactored into a generic helper method to reduce the ~130 lines of duplication.
Example helper:
private async agentHttpCall<T>( method: 'POST' | 'GET' | 'PUT' | 'DELETE', url: string, orgId: string, operationName: string, payload?: unknown ): Promise<T> { try { const getApiKey = await this.getOrgAgentApiKey(orgId); const httpMethod = { POST: this.commonService.httpPost.bind(this.commonService), GET: this.commonService.httpGet.bind(this.commonService), PUT: this.commonService.httpPut.bind(this.commonService), DELETE: this.commonService.httpDelete.bind(this.commonService) }[method]; const response = await httpMethod(url, payload, { headers: { authorization: getApiKey } }); return response; } catch (error) { this.logger.error(`Error in ${operationName} in agent service : ${JSON.stringify(error)}`); throw error; } }apps/api-gateway/src/x509/dtos/x509.dto.ts (1)
207-207: Remove commented code.Lines 207 and 295-296 contain commented-out code that should either be removed (if not needed) or uncommented (if needed). Leaving commented code in production reduces maintainability.
If not needed, remove:
- Line 207:
@ApiExtraModels(X509CertificateIssuerAndSubjectOptionsDto)- Lines 295-296:
stateOrProvinceNameandorganizationalUnitpropertiesIf needed, uncomment and ensure proper validation decorators are applied.
Also applies to: 295-296
| /* eslint-disable @typescript-eslint/explicit-function-return-type */ | ||
| // TODO: Remove this |
There was a problem hiding this comment.
🧩 Analysis chain
Remove ESLint disable and TODO before merge; technical debt.
The ESLint disable rule for explicit-function-return-type and the associated TODO comment should not be committed. All methods in this service already have explicit return type annotations (e.g., lines 42–45, 61–63, 123–127, etc.). This disable rule is unnecessary and masks code quality checks.
Run the following script to verify all methods have explicit return types and confirm the disable can be safely removed:
Expected result: All method definitions should show explicit return type annotations. If any methods lack return types, they must be added before removing the ESLint disable.
🏁 Script executed:
#!/bin/bash
# Verify that all public/exported methods have explicit return types
ast-grep --pattern $'export class IssuanceService extends BaseService {
$$$
$_($$$): $return_type {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for method definitions in IssuanceService that may lack explicit return types
# Look for lines with method signatures to analyze pattern
rg -nP -A1 '^\s*(async\s+)?\w+\s*\(' apps/api-gateway/src/issuance/issuance.service.ts | head -50Length of output: 1849
🏁 Script executed:
#!/bin/bash
# More targeted: search for method signatures and check return type presence
# Pattern: find lines with parentheses followed by { without : (colon for return type)
rg -nP '^\s+(async\s+)?\w+\s*\([^)]*\)\s*\{' apps/api-gateway/src/issuance/issuance.service.tsLength of output: 156
🏁 Script executed:
#!/bin/bash
# Show the actual method definitions in the file with context
cat apps/api-gateway/src/issuance/issuance.service.ts | head -100Length of output: 2873
🏁 Script executed:
#!/bin/bash
# Find method definitions (public methods in class) without explicit return types
# Look for pattern: ) { without preceding :
rg -nP '^\s+(async\s+)?[a-zA-Z_][a-zA-Z0-9_]*\s*\([^)]*\)\s*:\s*' apps/api-gateway/src/issuance/issuance.service.ts | wc -lLength of output: 2
🏁 Script executed:
#!/bin/bash
# Find the entire file to see all methods
wc -l apps/api-gateway/src/issuance/issuance.service.tsLength of output: 53
🏁 Script executed:
#!/bin/bash
# Extract all method signatures (lines with method definitions) to review comprehensively
# Search for methods in the class - look for ) { or ): ReturnType {
rg -nP '^\s+(async\s+)?[a-zA-Z_][a-zA-Z0-9_]*\s*\([^)]*\)\s*(?::\s*\S+\s*)?\{' apps/api-gateway/src/issuance/issuance.service.tsLength of output: 1174
🏁 Script executed:
#!/bin/bash
# Find all method definitions without explicit return types
# Methods without return types would have ) { without preceding :
# Look for method patterns: ) { where there's no : before the {
rg -nP '^\s+(async\s+)?[a-zA-Z_][a-zA-Z0-9_]*\s*\([^)]*\)\s*\{' apps/api-gateway/src/issuance/issuance.service.tsLength of output: 156
🏁 Script executed:
#!/bin/bash
# Show methods around lines 42, 123, 132, 143, 153, 200 to verify their signatures
sed -n '42,48p; 123,128p; 132,140p; 143,151p; 153,162p; 200,210p' apps/api-gateway/src/issuance/issuance.service.tsLength of output: 1732
Remove ESLint disable and TODO before merge.
All methods in this service have explicit return type annotations. The @typescript-eslint/explicit-function-return-type disable rule and TODO comment should not be committed—they represent unnecessary technical debt. Lines 2–3 must be removed.
🤖 Prompt for AI Agents
In apps/api-gateway/src/issuance/issuance.service.ts around lines 2 to 3, remove
the file-level ESLint disable comment "// eslint-disable
@typescript-eslint/explicit-function-return-type" and the "// TODO: Remove this"
comment; delete those two lines and ensure no other file-level disables remain,
then run the linter to confirm no rule violations and commit the change.
| -- Drop the table if it exists (safe for dev) | ||
| DROP TABLE IF EXISTS "public"."credential_templates"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that DROP TABLE is safe for all environments.
The migration includes a DROP TABLE IF EXISTS statement, which will delete all data in the credential_templates table if it exists. While the comment states "safe for dev", migrations typically run in all environments unless explicitly controlled.
Run the following script to check if this migration has already been applied in production:
🏁 Script executed:
#!/bin/bash
# Description: Check migration history and verify if safe to drop table
# Check if migration is already applied
echo "Checking migration timestamp: 20250822104325"
# Look for any references to credential_templates table being used
rg -n "credential_templates" --type sql -C 3Length of output: 7287
To complete verification, I need to confirm whether this migration file is new to the repository or already exists:
Remove the misleading comment about development-only safety.
The comment "safe for dev" incorrectly implies this migration applies only in development. Prisma migrations apply in production and testing environments the same way they do in development. However, since the migration is already applied (as evidenced by later migrations successfully referencing the credential_templates table), it won't re-run due to Prisma's migration tracking.
The DROP TABLE IF EXISTS pattern is appropriate here for initial schema creation. However, clarify the comment to avoid confusion:
-- Initial table creation (dropping any stale table if it exists)
DROP TABLE IF EXISTS "public"."credential_templates";🤖 Prompt for AI Agents
In
libs/prisma-service/prisma/migrations/20250822104325_oidc_issuer_templates/migration.sql
around lines 1 to 2, the comment "safe for dev" is misleading because Prisma
migrations run in all environments; replace that comment with a neutral,
accurate one such as indicating this is the initial table creation and that the
DROP TABLE IF EXISTS is used to remove any stale table before creation (e.g.,
"-- Initial table creation (dropping any stale table if it exists)"), leaving
the DROP statement unchanged.
...risma-service/prisma/migrations/20250902082321_add_org_agent_id_to_oidc_issuer/migration.sql
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 44
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.service.ts (2)
28-54: Return type mismatch: returning string instead of interface object.The method signature declares the return type as
ICreateWebhookUrl, but line 47 returnswebhookUrl.webhookUrl, which is just a string. This creates a type mismatch that could cause issues with NATS message serialization or consuming code that expects an object.Either:
- Return the full object:
return webhookUrl;to match the interface type, or- Update the return type to
Promise<string>if only the URL string is neededThe same issue exists in the API gateway service snippet, so verify consistency across both services.
Apply this diff if the full object should be returned:
- return webhookUrl.webhookUrl; + return webhookUrl;Or update the return type if only the string is needed:
- async registerWebhook(registerWebhookDto: IWebhookDto): Promise<ICreateWebhookUrl> { + async registerWebhook(registerWebhookDto: IWebhookDto): Promise<string> {
56-70: Return type mismatch: returning string instead of interface object.Similar to
registerWebhook, this method declares the return type asIGetWebhookUrlbut returns only the stringwebhookUrlInfo.webhookUrlon line 65. This creates the same type mismatch issue.Apply this diff if the full object should be returned:
- return webhookUrlInfo.webhookUrl; + return webhookUrlInfo;Or update the return type if only the string is needed:
- async getWebhookUrl(getWebhook: IWebhookUrl): Promise<IGetWebhookUrl> { + async getWebhookUrl(getWebhook: IWebhookUrl): Promise<string> {
♻️ Duplicate comments (31)
apps/issuance/src/issuance.controller.ts (1)
1-2: The blanket ESLint disable remains unresolved.This issue was already flagged in a previous review. The blanket ESLint disable continues to mask potential code quality issues across the entire controller, and the TODO comment provides no actionable timeline or specifics for resolution.
apps/connection/src/connection.service.ts (2)
64-64: ReplacePromise<unknown>with a specific return type.As previously noted, the return type
Promise<unknown>provides no type safety. Properties accessed at lines 534, 540, and 557 confirm this should return a typed interface.
576-577: ReplacePromise<any>with a specific interface.As previously noted, the
anyreturn type weakens type safety. Define a specific interface based on the properties accessed at lines 534, 540, and 557.apps/utility/src/utilities.repository.ts (1)
30-43: HandlefindUniquenull case and addreferenceIdtype.This method still has the two issues previously flagged in earlier review comments:
findUniquecan returnnull, but:
- The return type is
Promise<shortening_url>, suggesting a record is always present.UtilitiesService.getShorteningUrlimmediately dereferences properties on the result, which will throw at runtime when no record exists.
referenceIdis untyped, which weakens type-safety for a key lookup.A safer contract is to keep the non-null return type and convert the “not found” case into a clear error, plus add the parameter type:
- // eslint-disable-next-line camelcase - async getShorteningUrl(referenceId): Promise<shortening_url> { + // eslint-disable-next-line camelcase + async getShorteningUrl(referenceId: string): Promise<shortening_url> { try { const storeShorteningUrl = await this.prisma.shortening_url.findUnique({ where: { referenceId } }); - this.logger.log(`[getShorteningUrl] - shortening url details ${referenceId}`); - return storeShorteningUrl; + if (!storeShorteningUrl) { + this.logger.warn(`[getShorteningUrl] - no shortening url found for referenceId ${referenceId}`); + throw new Error(`Shortening URL not found for referenceId: ${referenceId}`); + } + + this.logger.log(`[getShorteningUrl] - shortening url details ${referenceId}`); + return storeShorteningUrl;This keeps the repository API aligned with how the service currently uses it and avoids null-dereference crashes.
libs/common/src/cast.helper.ts (1)
527-542: Critical logic error requires fix before merge.This issue was previously identified but not yet resolved. The condition on Line 530 has a critical flaw:
if (!queryParams || (queryParams?.length >= 0))Problem: For arrays,
length >= 0is always true, causing the function to return early and never process array inputs. Only objects without alengthproperty will be processed.Impact: The function will silently fail for any array-based query parameters.
Apply the previously suggested fix:
- if (!queryParams || (queryParams?.length >= 0)) { - return baseUrl + if (!queryParams || Object.keys(queryParams).length === 0) { + return baseUrl; }Additionally, address the ESLint violations:
- Add missing semicolon on line 531
- Fix yoda conditions (lines 536, 541)
- Consider using
Record<string, unknown>instead ofanyfor better type safetyBased on past review comment.
apps/oid4vc-verification/src/oid4vc-verification.repository.ts (4)
42-57: Handle possiblenullfromorg_agents_type.findFirstbefore destructuring
findFirstcan returnnull, but the code destructures immediately:const { agent } = await this.prisma.org_agents_type.findFirst({ where: { id: orgAgentId } });If no record exists, this will throw when trying to destructure
null, and the error path won’t be a clearNotFoundException.Consider the pattern suggested in the earlier review:
- const { agent } = await this.prisma.org_agents_type.findFirst({ - where: { - id: orgAgentId - } - }); - - this.logger.debug(`[getOrgAgentType] Found type=${agent}`); - return agent; + const result = await this.prisma.org_agents_type.findFirst({ + where: { + id: orgAgentId + } + }); + + if (!result) { + this.logger.warn(`[getOrgAgentType] No agent type found for orgAgentId=${orgAgentId}`); + // Use a domain-appropriate error here if you have one + throw new NotFoundException(ResponseMessages.agent.error.orgAgentNotFound); + } + + this.logger.debug(`[getOrgAgentType] Found type=${result.agent}`); + return result.agent;
101-120: FixupdateOid4vpVerifiercatch log to reference the correct methodThe catch block currently logs:
this.logger.error(`[updateOid4vpVerifier] Error in createOid4vpVerifier: ${error.message}`);This is misleading when debugging. It should reference
updateOid4vpVerifier:- this.logger.error(`[updateOid4vpVerifier] Error in createOid4vpVerifier: ${error.message}`); + this.logger.error(`[updateOid4vpVerifier] Error in updateOid4vpVerifier: ${error.message}`);
138-155: Consider handling optionalverifierIdand fix log message ingetVerifiersByVerifierIdTwo points here:
- The signature allows
verifierId?: stringbut thewhereclause always includesid: verifierId. Passingundefinedwill either be rejected by Prisma or yield surprising behavior. IfverifierIdis truly optional, build the filter conditionally:const result = await this.prisma.oid4vp_verifier.findMany({ - where: { - id: verifierId, - orgAgent: { - orgId - } - } + where: { + orgAgent: { orgId }, + ...(verifierId && { id: verifierId }) + } });
- The error log refers to the wrong method:
this.logger.error(`[getVerifiersByVerifierId] Error in getVerifiersByPublicVerifierId: ${error.message}`);This should be
getVerifiersByVerifierIdto keep logs consistent.- this.logger.error(`[getVerifiersByVerifierId] Error in getVerifiersByPublicVerifierId: ${error.message}`); + this.logger.error(`[getVerifiersByVerifierId] Error in getVerifiersByVerifierId: ${error.message}`);
157-193: Fix PrismafindUnique/deleteusage and optionalverifierIdin verifier lookupsIn both
getVerifierByIdanddeleteVerifierByVerifierId:const result = await this.prisma.oid4vp_verifier.findUnique({ where: { id: verifierId, orgAgent: { orgId } } }); await this.prisma.oid4vp_verifier.delete({ where: { id: verifierId, orgAgent: { orgId } } });Issues:
findUnique/deletewherefilters must match a unique constraint shape; combiningid(already unique) with a relational filter is not supported by Prisma and will either fail type‑checking or at runtime. If you want both conditions, usefindFirst+deletebyidonce you’ve verified org ownership.verifierIdis optional in the signature but always passed intoid: verifierId. If it’s everundefined, that’s invalid for a unique ID.A safer pattern:
- async getVerifierById(orgId: string, verifierId?: string): Promise<oid4vp_verifier | null> { + async getVerifierById(orgId: string, verifierId: string): Promise<oid4vp_verifier | null> { // ... - const result = await this.prisma.oid4vp_verifier.findUnique({ - where: { - id: verifierId, - orgAgent: { - orgId - } - } - }); + const result = await this.prisma.oid4vp_verifier.findFirst({ + where: { + id: verifierId, + orgAgent: { orgId } + } + });- async deleteVerifierByVerifierId(orgId: string, verifierId?: string): Promise<oid4vp_verifier | null> { + async deleteVerifierByVerifierId(orgId: string, verifierId: string): Promise<oid4vp_verifier | null> { // ... - const deleted = await this.prisma.oid4vp_verifier.delete({ - where: { - id: verifierId, - orgAgent: { - orgId - } - } - }); + const record = await this.prisma.oid4vp_verifier.findFirst({ + where: { + id: verifierId, + orgAgent: { orgId } + } + }); + + if (!record) { + this.logger.warn( + `[deleteVerifierByVerifierId] No verifier found for orgId=${orgId}, verifierId=${verifierId}` + ); + return null; + } + + const deleted = await this.prisma.oid4vp_verifier.delete({ + where: { id: record.id } + });Also, the
getVerifierByIdcatch block still logs the wrong method name:- this.logger.error(`[getVerifierById] Error in getVerifiersByPublicVerifierId: ${error.message}`); + this.logger.error(`[getVerifierById] Error in getVerifierById: ${error.message}`);apps/api-gateway/src/verification/interfaces/verification.interface.ts (1)
3-9: Align proof request interfaces with other modules (optional vs required, andnamestype)This file’s proof request types still differ from the versions in
apps/verificationandlibs/common:
IProofRequestAttributehere makesattributeName,credDefId, andcredentialNamerequired, and omitsattributeNamesandschemaId.IRequestedAttributesName.namesisstring, but in other places it’sstring[].These inconsistencies will make it hard to reuse shared code and can introduce subtle bugs at the API boundary.
To keep everything compatible, consider aligning with the other definitions:
-export interface IProofRequestAttribute { - attributeName: string; - condition?: string; - value?: string; - credDefId: string; - credentialName: string; -} +export interface IProofRequestAttribute { + attributeName?: string; + attributeNames?: string[]; + condition?: string; + value?: string; + credDefId?: string; + schemaId?: string; + credentialName?: string; +}interface IRequestedAttributesName { name?: string; - names?: string; + names?: string[]; restrictions: IRequestedRestriction[]; }Also applies to: 62-66
apps/api-gateway/src/dtos/credential-offer.dto.ts (1)
10-38: Flatten@ApiPropertyexamples to match actual field shapesThe
IssueCredentialOfferexamples are still wrapped in extra objects, e.g.:@ApiProperty({ example: { protocolVersion: 'v1' } }) protocolVersion: string;But the field itself is just a string, so Swagger will show a slightly confusing example. Same pattern applies to
attributes,credentialDefinitionId,autoAcceptCredential,comment, andconnectionId.Suggested adjustments:
- @ApiProperty({ example: { protocolVersion: 'v1' } }) + @ApiProperty({ example: 'v1' }) protocolVersion: string; - @ApiProperty({ example: { attributes: [{ value: 'string', name: 'string' }] } }) + @ApiProperty({ example: [{ value: 'string', name: 'string' }] }) attributes: attributeValue[]; - @ApiProperty({ example: { credentialDefinitionId: 'string' } }) + @ApiProperty({ example: 'string' }) credentialDefinitionId: string; - @ApiProperty({ example: { autoAcceptCredential: 'always' } }) + @ApiProperty({ example: 'always' }) autoAcceptCredential: string; - @ApiProperty({ example: { comment: 'string' } }) + @ApiProperty({ example: 'string' }) comment: string; - @ApiProperty({ example: { connectionId: '3fa85f64-5717-4562-b3fc-2c963f66afa6' } }) + @ApiProperty({ example: '3fa85f64-5717-4562-b3fc-2c963f66afa6' }) connectionId: string;apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (1)
156-179: Add validation and API metadata forIssuerUpdationDto.issuerId
IssuerUpdationDto.issuerIdis currently undecorated, so non‑string or malformed values can bypass validation and reach the service layer. This matches the previously raised concern and is still unresolved.You can align it with the creation DTO and keep it optional:
@ApiPropertyOptional({ description: 'Optional override for the issuer identifier', example: 'credebl-university' }) @IsOptional() @IsString({ message: 'issuerId must be a string' }) issuerId?: string;Also, you can drop the extra
@ApiPropertyonbatchCredentialIssuanceSizeand keep only@ApiPropertyOptionalto avoid duplicate/contradictory Swagger metadata.Please confirm this change still matches your update semantics and runs cleanly with your existing validation pipeline.
apps/oid4vc-verification/test/app.e2e-spec.ts (1)
18-18: Remove the HTTP test—this is a microservices-only module without HTTP routes.Based on the pattern across the PR, the
Oid4vcVerificationModuleis configured exclusively for NATS message-driven communication. The controller likely contains only@MessagePatternhandlers, not HTTP route decorators. The test attempts a GET request to "/", which does not exist. The test will fail.Either remove this test file if HTTP testing isn't needed for this module, or refactor the controller to include actual HTTP routes and update the test accordingly.
apps/oid4vc-issuance/test/app.e2e-spec.ts (1)
18-18: Remove the HTTP test—this is a microservices-only module without HTTP routes.The
Oid4vcIssuanceModuleis configured exclusively for NATS message-driven communication (viaClientsModule.register()withTransport.NATS). TheOid4vcIssuanceControllercontains only@MessagePatternhandlers, not HTTP route decorators. The test attempts a GET request to "/", which does not exist. The test will fail.Either remove this test file if HTTP testing isn't needed for this module, or refactor the controller to include actual HTTP routes and update the test accordingly.
.env.sample (1)
126-127: Correct the service names in comments.Lines 126 and 127 incorrectly reference "x509 service" in their comments. These should reference the OIDC4VC issuance and verification services respectively.
Apply this diff:
-OIDC4VC_ISSUANCE_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for x509 service -OIDC4VC_VERIFICATION_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for x509 service +OIDC4VC_ISSUANCE_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for OIDC4VC issuance service +OIDC4VC_VERIFICATION_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for OIDC4VC verification serviceapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.module.ts (1)
17-17: Critical: NATS service name mismatch prevents gateway-microservice communication.This gateway module uses
CommonConstants.ISSUANCE_SERVICE, but the OID4VC issuance microservice at apps/oid4vc-issuance/src/main.ts:15 usesCommonConstants.OIDC4VC_ISSUANCE_SERVICE. The mismatch will prevent proper NATS message routing between the gateway and microservice.Apply this diff:
- options: getNatsOptions(CommonConstants.ISSUANCE_SERVICE, process.env.API_GATEWAY_NKEY_SEED) + options: getNatsOptions(CommonConstants.OIDC4VC_ISSUANCE_SERVICE, process.env.API_GATEWAY_NKEY_SEED)libs/common/src/nats.config.ts (2)
17-18: Critical: Removing NATS queue group breaks load balancing across all microservices.Commenting out the queue assignment fundamentally changes NATS message distribution. Without queue groups, every instance of each microservice receives every message (broadcast) instead of load-balanced distribution. This affects 30+ services and will cause duplicate message processing when multiple instances run.
Apply this diff to restore queue-based load balancing:
servers: `${process.env.NATS_URL}`.split(','), maxReconnectAttempts: NATSReconnects.maxReconnectAttempts, - reconnectTimeWait: NATSReconnects.reconnectTimeWait - // queue: serviceName + reconnectTimeWait: NATSReconnects.reconnectTimeWait, + queue: serviceNameIf broadcast behavior is intentional, document this architectural decision in code comments and repository documentation.
17-17: Fix syntax error: Missing trailing comma.Line 17 is missing a trailing comma after
reconnectTimeWait, which will cause a syntax error in JavaScript/TypeScript.Apply this diff:
- reconnectTimeWait: NATSReconnects.reconnectTimeWait + reconnectTimeWait: NATSReconnects.reconnectTimeWait,libs/prisma-service/prisma/migrations/20250822104325_oidc_issuer_templates/migration.sql (1)
1-2: Misleading comment: Migrations apply in all environments, not just dev.The comment "safe for dev" incorrectly suggests this migration only affects development environments. Prisma migrations apply to all environments (development, staging, production) based on your migration tracking. The
DROP TABLE IF EXISTSis appropriate for initial table creation, but the comment should be clarified.Replace the comment with:
-- Initial table creation (dropping any stale table if it exists)apps/api-gateway/src/x509/x509.module.ts (1)
17-24: Fix incorrect module placement in ClientsModule.register().
CommonModuleis incorrectly placed inside theClientsModule.register()array. Theregister()method expects client configuration objects (withname,transport,optionsproperties), not module imports. This will cause a runtime error.Apply this diff to fix the module placement:
imports: [ HttpModule, ConfigModule.forRoot(), + CommonModule, ClientsModule.register([ { name: 'NATS_CLIENT', transport: Transport.NATS, options: getNatsOptions(CommonConstants.X509_SERVICE, process.env.API_GATEWAY_NKEY_SEED) - }, - CommonModule + } ]) ],libs/prisma-service/prisma/migrations/20250902082321_add_org_agent_id_to_oidc_issuer/migration.sql (1)
1-21: Migration requires backfill strategy and FK restoration.This migration has the same critical issues previously identified:
- NOT NULL columns without defaults: Adding
orgAgentId UUID NOT NULLandpublicIssuerId TEXT NOT NULLwithout defaults or backfill will fail on non-empty tables- Dropped referential integrity: Removing
oidc_issuer_createdBy_fkeyleavescreatedByunconstrainedThe migration needs:
- A backfill strategy: add columns as nullable, populate from
agentIssuerIdor other sources, then tighten to NOT NULL- FK restoration or clear documentation if
createdByconstraint removal is intentionalapps/issuance/constant/issuance.ts (1)
1-6: Code duplication detected with apps/oid4vc-issuance/constant/issuance.ts.These constants (lines 3-6) duplicate those in
apps/oid4vc-issuance/constant/issuance.ts. This duplication was already flagged in the review of that file. Consider consolidating into a shared module.apps/verification/src/verification.controller.ts (1)
48-48: Replace overly broadobjectreturn type with specific interface.The return type
Promise<object>was already flagged in a previous review. UsePromise<IProofPresentationDetails>or a similar specific interface for type safety.Also applies to lines 71, 82.
libs/common/src/date-only.ts (1)
5-24: Constructor validation and normalization issues remain.The issues flagged in the previous review are still present:
- Line 18: Regex
/^\d{4}-\d{2}-\d{2}(T.*Z)?$/rejects valid ISO timestamps with timezone offsets (e.g.,2025-11-17T10:00:00+01:00)- Lines 18-19: Convoluted
iso || datefallback logic- Lines 13-15:
new Date()captures time-of-day, inconsistent with "date-only" semantics- No normalization to UTC midnight
Please address these issues as outlined in the previous review comment.
apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (1)
240-253: Missing required field validation for template.The
templatefield lacks@IsDefined()validation, allowing payloads that omit it to pass validation. This causes runtime failures when downstream code attempts to dereference the template structure.Please add the
@IsDefined()decorator as outlined in the previous review comment.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts (1)
34-37: Enable nested validation for credentialOfferPayload.The
credentialOfferPayloadfield is missing@ValidateNested()and@Type()decorators, preventing validation of the nestedcredential_configuration_idsarray constraints. Malformed payloads will pass validation, potentially causing downstream errors.Please add the decorators as outlined in the previous review comment.
apps/api-gateway/src/x509/x509.service.ts (1)
55-64: Correct return type for paginated list method.The
getX509CertificatesByOrgIdmethod is typed to returnPromise<X509CertificateRecord>(a single record), but the X509 microservice returns a paginated envelope{ data: X509CertificateRecord[], total: number }. This type mismatch erodes type safety and may lead to runtime errors in consuming code.Please update the return type as outlined in the previous review comment.
apps/oid4vc-verification/src/oid4vc-verification.service.ts (3)
43-98: Avoid dereferencingcreatedVerifierDetailsin 409 conflict pathIn
oid4vpCreateVerifier, the inner catch does:let createdVerifierDetails; ... try { createdVerifierDetails = await this._createOid4vpVerifier(...); ... } catch (error) { const status409 = ...; if (status409) { throw new ConflictException(`Verifier with id '${createdVerifierDetails.verifierId}' already exists`); } throw error; }If the agent returns a 409 before we ever set
createdVerifierDetails, this dereference throws and masks the intended conflict.Use the known
verifierIdfrom the request instead:- let createdVerifierDetails; + let createdVerifierDetails: VerifierRecord | undefined; ... try { createdVerifierDetails = await this._createOid4vpVerifier(createVerifier, url, orgId); @@ } catch (error) { const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${createdVerifierDetails.verifierId}' already exists`); + throw new ConflictException(`Verifier with id '${verifierId}' already exists`); } throw error; }This preserves the correct 409 response without introducing a secondary error.
100-158: Fix empty-verifier guard and update conflict handling inoid4vpUpdateVerifierTwo problems in
oid4vpUpdateVerifier:
- The guard never triggers:
const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); if (0 > existingVerifiers.length) { throw new NotFoundException(...); }
existingVerifiers.lengthis0or more, so0 > lengthis alwaysfalse, and you can then dereferenceexistingVerifiers[0]even when the array is empty.
- In the inner catch, the 409 path dereferences a possibly-undefined
updatedVerifierDetails:if (status409) { throw new ConflictException(`Verifier with id '${updatedVerifierDetails.verifierId}' already exists`); }If the agent rejects the update before returning a body,
updatedVerifierDetailsisundefinedand this throws a new error.Suggested fix:
- let updatedVerifierDetails; - const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); - if (0 > existingVerifiers.length) { + let updatedVerifierDetails; + const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); + if (!existingVerifiers || existingVerifiers.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } @@ try { updatedVerifierDetails = await this._updateOid4vpVerifier(updateVerifier, url, orgId); @@ } catch (error) { const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${updatedVerifierDetails.verifierId}' already exists`); + const conflictId = existingVerifiers?.[0]?.publicVerifierId ?? verifierId; + throw new ConflictException(`Verifier with id '${conflictId}' already exists`); } throw error; }This correctly throws
NotFoundwhen no verifier exists and avoids dereferencingupdatedVerifierDetailsin the conflict path.
207-284: Fix X.509 signer lookup: use the realmethodinstead ofmethodvIn the X.509 branch of
oid4vpCreateVerificationSession:} else if ( sessionRequest.requestSigner.method === SignerOption.X509_P256 || sessionRequest.requestSigner.method === SignerOption.X509_ED25519 ) { ... const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( orgId, sessionRequest.requestSigner.methodv );
sessionRequest.requestSigner.methodvdoes not exist and will always beundefined, sogetCurrentActiveCertificatenever finds a matching record and you always hit the “No active certificate” path.Use the actual
methodvalue:- const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( - orgId, - sessionRequest.requestSigner.methodv - ); + const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( + orgId, + sessionRequest.requestSigner.method + ); @@ - throw new NotFoundException( - `No active certificate(${sessionRequest.requestSigner.method}}) found for issuer` - ); + throw new NotFoundException( + `No active certificate(${sessionRequest.requestSigner.method}) found for issuer` + );You may also want to explicitly map from
SignerOptionto thex5cKeyTypeused in your X.509 store if those enums differ.apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts (1)
93-98: TypeIssuerUpdation.displayinstead of leaving it asany
IssuerUpdation.displaycurrently has no type annotation:export interface IssuerUpdation { issuerId: string; accessTokenSignerKeyType: AccessTokenSignerKeyType; display; batchCredentialIssuanceSize?: number; }That makes it implicitly
anyand removes type-safety for one of the most important fields in issuer updates, increasing the risk of malformed data passing through.Given you already have a
Displayinterface above, consider:export interface IssuerUpdation { issuerId: string; accessTokenSignerKeyType: AccessTokenSignerKeyType; - display; + display?: Display[]; batchCredentialIssuanceSize?: number; }(or make it non-optional if updates must always carry a
displayarray). This was flagged previously and still looks worth fixing before depending on these interfaces widely.
| id?: string; | ||
| clientSocketId?: string; | ||
| agentEndPoint?: string; | ||
| agentToken?: string; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify usage of the new agentToken field.
The addition of the optional agentToken field to IStoreOrgAgent is backward compatible. Ensure that the new field is properly utilized in the agent storage and retrieval logic.
🏁 Script executed:
#!/bin/bash
# Verify where agentToken is being used in the codebase
rg -n "agentToken" --type=ts -C3Length of output: 5778
I need to verify the storage and retrieval logic to ensure the new field is properly handled in the database layer.
🏁 Script executed:
#!/bin/bash
# Check for database/storage operations related to IStoreOrgAgent or agent details storage
rg -n "IStoreOrgAgent|saveAgent|updateAgent|getAgent|storeOrgAgent" --type=ts -C2 | head -100Length of output: 7615
🏁 Script executed:
#!/bin/bash
# Check for database schema or entity definitions related to agents
fd -e ts -e js | xargs grep -l "agent.*schema\|agent.*entity" --include="*.ts" 2>/dev/null | head -10Length of output: 235
🏁 Script executed:
#!/bin/bash
# Search for where IStoreOrgAgent interface is being implemented or used
rg -n "IStoreOrgAgent" --type=ts -B2 -A5Length of output: 9243
I need to examine the complete interface definitions and the storage implementation more carefully.
🏁 Script executed:
#!/bin/bash
# Get the complete IStoreOrgAgentDetails interface definition
rg -n "export interface IStoreOrgAgentDetails" --type=ts -A30Length of output: 2632
🏁 Script executed:
#!/bin/bash
# Get the complete storeOrgAgentDetails method implementation
rg -n "async storeOrgAgentDetails" --type=ts -A20Length of output: 2135
🏁 Script executed:
#!/bin/bash
# Check if agentToken is used in the agent payload construction (line 552 area)
rg -n "agentPayload: IStoreOrgAgentDetails" --type=ts -B5 -A15Length of output: 1909
I found a critical issue. Let me verify how agent details are retrieved to confirm the storage gap.
🏁 Script executed:
#!/bin/bash
# Find where agentDetails is retrieved (lines 543, 576 context)
rg -n "agentDetails\s*=" --type=ts -B3 -C2 | head -60Length of output: 5341
🏁 Script executed:
#!/bin/bash
# Check for methods that retrieve agent data from database
rg -n "getOrgAgent|findAgent|getAgent\(" --type=ts -C2 | grep -A5 "repository\|prisma" | head -40Length of output: 2816
🏁 Script executed:
#!/bin/bash
# Check if org_agents Prisma schema includes agentToken column
fd -name "schema.prisma" -o -name "*prisma*" | xargs cat 2>/dev/null | grep -A30 "model org_agents"Length of output: 230
Let me search for the Prisma schema file more carefully:
🏁 Script executed:
#!/bin/bash
# Find Prisma schema file
find . -name "schema.prisma" -type f 2>/dev/nullLength of output: 42
🏁 Script executed:
#!/bin/bash
# If found, display org_agents model
find . -name "schema.prisma" -type f 2>/dev/null | head -1 | xargs cat | grep -A 40 "model org_agents"Length of output: 3636
🏁 Script executed:
#!/bin/bash
# Alternative: search for prisma directory structure
find . -path "*/prisma/schema.prisma" -o -path "*/libs/prisma/*" -type f 2>/dev/null | head -5Length of output: 42
Add agentToken field to database schema and storage interface to complete integration.
The new agentToken field in IStoreOrgAgent is not properly persisted to the database. The Prisma org_agents model lacks the agentToken column, and the storage interface IStoreOrgAgentDetails does not include this field. This breaks storage and retrieval:
storeOrgAgentDetails()inapps/agent-service/src/repositories/agent-service.repository.ts:158receivesIStoreOrgAgentDetails(which lacksagentToken), so the field is never saved- When agent details are retrieved from the database,
agentTokenwill be undefined - Currently
agentTokenis read from files (apps/agent-provisioning/src/agent-provisioning.service.ts:71), not the database
Required fixes:
- Add
agentTokencolumn toorg_agentsmodel inlibs/prisma-service/prisma/schema.prisma - Add
agentToken?: string;field toIStoreOrgAgentDetailsinterface inapps/agent-service/src/interface/agent-service.interface.ts - Include
agentTokenin the payload atapps/agent-service/src/agent-service.service.ts:552-567when storing agent details
🤖 Prompt for AI Agents
In apps/agent-service/src/interface/agent-service.interface.ts around line 216,
add the missing agentToken?: string field to IStoreOrgAgentDetails so the
interface includes the token; in libs/prisma-service/prisma/schema.prisma update
the org_agents model to add an agentToken column (string, optional) and run
prisma migrate to update the DB schema; and in
apps/agent-service/src/agent-service.service.ts around lines 552-567 include
agentToken from the incoming data into the payload passed to
storeOrgAgentDetails() so the token is persisted.
| export class SignerOptionsDto { | ||
| @IsString() | ||
| @IsIn(['did', 'x5c'], { message: 'method must be either "did" or "x5c"' }) | ||
| method: string; | ||
|
|
||
| @IsString() | ||
| @IsOptional() | ||
| did?: string; | ||
|
|
||
| @IsArray() | ||
| @IsOptional() | ||
| x5c?: string[]; | ||
| } | ||
|
|
||
| export class CredentialDto { | ||
| @ApiProperty({ | ||
| description: 'Unique ID of the supported credential', | ||
| example: 'DrivingLicenseCredential-mdoc' | ||
| }) | ||
| @IsString() | ||
| credentialSupportedId: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Signer options for credential issuance', | ||
| example: { | ||
| method: 'x5c', | ||
| x5c: [ | ||
| 'MIIB3jCCAZCgAwIBAgIQQfBdIK9v3TIzKR+1HjlixDAFBgMrZXAwJDEUMBIGA1UEAxMLRFkgdGVzdCBvcmcxDDAKBgNVBAYTA0lORDAeFw0yNTA5MjQwMDAwMDBaFw0yODA5MjQwMDAwMDBaMCQxFDASBgNVBAMTC0RZIHRlc3Qgb3JnMQwwCgYDVQQGEwNJTkQwKjAFBgMrZXADIQDIkLycOlkHP6+MG4rprj8fyxRfwqhH8Xx9v0XxCd175aOB1zCB1DAdBgNVHQ4EFgQUbqjjbQgbAx3lPjkPBVQwvvF14agwDgYDVR0PAQH/BAQDAgGGMBUGA1UdJQEB/wQLMAkGByiBjF0FAQIwOwYDVR0SBDQwMoIXaHR0cDovL3Rlc3QuZXhhbXBsZS5jb22GF2h0dHA6Ly90ZXN0LmV4YW1wbGUuY29tMDsGA1UdEQQ0MDKCF2h0dHA6Ly90ZXN0LmV4YW1wbGUuY29thhdodHRwOi8vdGVzdC5leGFtcGxlLmNvbTASBgNVHRMBAf8ECDAGAQH/AgEAMAUGAytlcANBALTqC64XSTRUoMmwYbCD/z46U/Je6IeQsh6qq4qXh+wfnMIfJMvLQnG+nMkfeAs3zYAwjK6sCZ/7lHkEJnYObQ4=' | ||
| ] | ||
| } | ||
| }) | ||
| @ValidateNested() | ||
| @Type(() => SignerOptionsDto) | ||
| signerOptions: SignerOptionsDto; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Credential format type', | ||
| enum: ['mso_mdoc', 'vc+sd-jwt'], | ||
| example: 'mso_mdoc' | ||
| }) | ||
| @IsString() | ||
| @IsIn(['mso_mdoc', 'vc+sd-jwt'], { message: 'format must be either "mso_mdoc" or "vc+sd-jwt"' }) | ||
| format: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Credential payload (namespace data, validity info, etc.)', | ||
| example: [ | ||
| { | ||
| namespaces: { | ||
| 'org.iso.23220.photoID.1': { | ||
| birth_date: '1970-02-14', | ||
| family_name: 'Müller-Lüdenscheid', | ||
| given_name: 'Ford Praxibetel', | ||
| document_number: 'LA001801M' | ||
| } | ||
| }, | ||
| validityInfo: { | ||
| validFrom: '2025-04-23T14:34:09.188Z', | ||
| validUntil: '2026-05-03T14:34:09.188Z' | ||
| } | ||
| }, | ||
| { | ||
| full_name: 'Garry', | ||
| address: { | ||
| street_address: 'M.G. Road', | ||
| locality: 'Pune', | ||
| country: 'India' | ||
| }, | ||
| iat: 1698151532, | ||
| nbf: dateToSeconds(new Date()), | ||
| exp: dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000)) | ||
| } | ||
| ] | ||
| }) | ||
| @ValidateNested() | ||
| payload: object; | ||
| } |
There was a problem hiding this comment.
SignerOptionsDto / CredentialDto: address ESLint hints and nested payload validation
Functionally these DTOs look fine, but:
- ESLint flags the
x5cexample array forarray-bracket-newline. You can satisfy it by putting the element on the same line:
x5c: ['MIIB3jCCAZC...']- ESLint also flags the
dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000))example for mixed+/*. Adding parentheses clarifies intent:
exp: dateToSeconds(new Date(Date.now() + (5 * 365 * 24 * 60 * 60 * 1000)))Also, payload is annotated with @ValidateNested() but not @Type or a concrete class type; if you don’t actually need nested validation here, you could drop @ValidateNested() to avoid confusion, or define a dedicated payload DTO if you do.
🧰 Tools
🪛 ESLint
[error] 269-269: There should be no linebreak after '['.
(array-bracket-newline)
[error] 271-271: There should be no linebreak before ']'.
(array-bracket-newline)
[error] 313-313: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
[error] 313-313: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
🤖 Prompt for AI Agents
In apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts around lines
243-319, address three issues: 1) fix the ESLint array-bracket-newline warning
by collapsing the long x5c example into a single-line array element; 2) fix the
mixed operator ESLint warning by wrapping the multiplier expression in
parentheses when computing the exp example (e.g. (5 * 365 * 24 * 60 * 60 *
1000)); 3) resolve the misleading @ValidateNested() on payload by either
removing @ValidateNested() if no nested validation is required, or define a
concrete payload DTO class and add @Type(() => PayloadDto) (and annotate payload
with that class) to enable proper nested validation.
libs/prisma-service/prisma/migrations/20250822145801_fix_issuer_relation/migration.sql
Show resolved
Hide resolved
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| async deleteTemplate(templateId: string, orgId: string, userDetails: user, issuerId: string): Promise<any> { | ||
| try { | ||
| const template = await this.oid4vcIssuanceRepository.getTemplateById(templateId); | ||
| if (!template) { | ||
| throw new NotFoundException(ResponseMessages.oidcTemplate.error.notFound); | ||
| } | ||
| const deleteTemplate = await this.oid4vcIssuanceRepository.deleteTemplate(templateId); | ||
| if (!deleteTemplate) { | ||
| throw new NotFoundException(ResponseMessages.oidcTemplate.error.deleteTemplate); | ||
| } | ||
|
|
||
| const issuerTemplateConfig = await this.buildOidcIssuerConfig(issuerId); | ||
| const agentDetails = await this.oid4vcIssuanceRepository.getAgentEndPoint(orgId); | ||
| if (!agentDetails) { | ||
| throw new NotFoundException(ResponseMessages.issuance.error.agentEndPointNotFound); | ||
| } | ||
| const { agentEndPoint } = agentDetails; | ||
| const issuerDetails = await this.oid4vcIssuanceRepository.getOidcIssuerDetailsById(issuerId); | ||
| if (!issuerDetails) { | ||
| throw new NotFoundException(ResponseMessages.oidcTemplate.error.issuerDetailsNotFound); | ||
| } | ||
| const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_ISSUER_TEMPLATE, issuerDetails.publicIssuerId); | ||
|
|
||
| const createTemplateOnAgent = await this._createOIDCTemplate(issuerTemplateConfig, url, orgId); | ||
| if (!createTemplateOnAgent) { | ||
| throw new NotFoundException(ResponseMessages.issuance.error.agentEndPointNotFound); | ||
| } | ||
| return deleteTemplate; | ||
| } catch (error) { | ||
| this.logger.error(`[deleteTemplate] - error: ${JSON.stringify(error)}`); | ||
| throw new RpcException(error.response ?? error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Handle agent failure when deleting templates to keep DB and agent in sync.
deleteTemplate deletes the DB row first and only then updates the agent. If the agent call fails, there is no rollback, leaving the agent and DB inconsistent (unlike createTemplate and updateTemplate, which compensate).
Consider either:
- Flipping the order (update agent first, then delete DB), or
- Adding a rollback path that recreates the template in the DB when
_createOIDCTemplatefails.
| async createOidcCredentialOfferD2A(oidcCredentialD2APayload, orgId: string): Promise<object | string> { | ||
| try { | ||
| for (const credential of oidcCredentialD2APayload.credentials) { | ||
| const { signerOptions } = credential; | ||
|
|
||
| if (!signerOptions?.method) { | ||
| throw new BadRequestException(`signerOptions.method is required`); | ||
| } | ||
| if (signerOptions.method === SignerMethodOption.X5C) { | ||
| if (!signerOptions.x5c || 0 === signerOptions.x5c.length) { | ||
| // const x5cFromDb = await this.oid4vcIssuanceRepository.getIssuerX5c( | ||
| const x5cFromDb = 'Test'; | ||
| // If you want to use the actual DB call, uncomment and use: | ||
| // const x5cFromDb = await this.oid4vcIssuanceRepository.getIssuerX5c( | ||
| // oidcCredentialD2APayload.publicIssuerId, | ||
| // orgId | ||
| // ); | ||
| if (!x5cFromDb || 0 === x5cFromDb.length) { | ||
| throw new BadRequestException(`No x5c found for issuer`); | ||
| } | ||
| signerOptions.x5c = x5cFromDb; | ||
| } |
There was a problem hiding this comment.
Remove hard-coded 'Test' x5c stub in createOidcCredentialOfferD2A.
For X5C signer options, you currently inject a static 'Test' value:
const x5cFromDb = 'Test';
...
signerOptions.x5c = x5cFromDb;This breaks real flows and hides the intended DB lookup. Either:
- Wire up the actual repository call (as per the commented-out code), or
- Fail fast when
signerOptions.x5cis missing and the DB lookup is not yet implemented.
Leaving this stub in will produce invalid credential offers in any path relying on X.509.
🤖 Prompt for AI Agents
In apps/oid4vc-issuance/src/oid4vc-issuance.service.ts around lines 624-645,
remove the hard-coded const x5cFromDb = 'Test' stub and either invoke the
commented repository call to fetch the issuer x5c (await
this.oid4vcIssuanceRepository.getIssuerX5c(oidcCredentialD2APayload.publicIssuerId,
orgId)) and assign that to signerOptions.x5c, or if the DB integration is not
ready, immediately throw a BadRequestException when signerOptions.x5c is missing
instead of inserting a fake value; ensure you preserve the existing check that
throws if the retrieved x5c is empty.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 43
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/api-gateway/src/connection/connection.service.ts (1)
31-49: Apply consistent error handling pattern across both methods.The
sendQuestionmethod now useserror?.response ?? error(line 35), butsendBasicMessagestill useserror.responsedirectly (line 47). This inconsistency meanssendBasicMessagecould throw an error withundefinediferror.responsedoesn't exist.Apply this diff to align the error handling:
sendBasicMessage(basicMessageDto: BasicMessageDto): Promise<object> { try { return this.natsClient.sendNatsMessage( this.connectionServiceProxy, 'send-basic-message-on-connection', basicMessageDto ); } catch (error) { - throw new RpcException(error.response); + throw new RpcException(error?.response ?? error); } }apps/cloud-wallet/src/cloud-wallet.service.ts (2)
151-151: Fix URL syntax error - extra closing brace.The URL construction contains an extra closing brace that will cause the API request to fail.
Apply this diff to fix the syntax error:
- const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${proofRecordId}}`; + const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${proofRecordId}`;
173-173: Fix URL syntax error - extra closing brace.The URL construction contains an extra closing brace that will cause the API request to fail.
Apply this diff to fix the syntax error:
- const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${threadParam}}`; + const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${threadParam}`;apps/verification/src/verification.service.ts (1)
65-69: Remove unused injected dependencies.Verification confirms that
userActivityService(line 66) andcacheService(line 69) are not used anywhere in this service. Remove both to eliminate unnecessary coupling:private readonly userActivityService: UserActivityService, @Inject(CACHE_MANAGER) private readonly cacheService: Cache,Delete these two lines from the constructor.
♻️ Duplicate comments (33)
apps/api-gateway/src/issuance/issuance.service.ts (1)
29-29: Import reordering looks good; confirm past ESLint disable is resolved.The Prisma
usertype import is correctly positioned and used consistently in method signatures (e.g., line 261). All methods in the file already have explicit return type annotations, so the import change does not introduce any ESLint violations.However, per the past review comment, verify that the file-level ESLint disable for
@typescript-eslint/explicit-function-return-type(if it existed at lines 2–3) has been removed. I don't see it in the current code, so this may be already addressed.apps/api-gateway/src/dtos/credential-offer.dto.ts (1)
10-38: Fix ApiProperty examples - remove unnecessary object wrapping.The
@ApiPropertyexamples remain incorrectly wrapped in objects. For instance,{ protocolVersion: 'v1' }should be'v1', and{ attributes: [...] }should be[...]. This affects the generated OpenAPI/Swagger documentation.Please apply the fix suggested in the previous review to unwrap each example value.
apps/utility/src/utilities.repository.ts (1)
31-45: Missing null handling and type annotation (previously flagged).The issues identified in the previous review remain unaddressed:
- Critical: Missing null handling -
findUniquereturnsnullwhen no record is found, but the service layer accesses properties on the result without checking, causing runtime errors.- Missing type annotation - The
referenceIdparameter lacks an explicit type.Fix 1: Add null handling
async getShorteningUrl(referenceId): Promise<shortening_url> { try { const storeShorteningUrl = await this.prisma.shortening_url.findUnique({ where: { referenceId } }); + if (!storeShorteningUrl) { + throw new Error(`Shortening URL not found for referenceId: ${referenceId}`); + } + this.logger.log(`[getShorteningUrl] - shortening url details ${referenceId}`); return storeShorteningUrl;Fix 2: Add type annotation
- async getShorteningUrl(referenceId): Promise<shortening_url> { + async getShorteningUrl(referenceId: string): Promise<shortening_url> {apps/connection/src/connection.service.ts (2)
64-82: Persistent issue: ReplacePromise<unknown>with a specific interface.This is a repeat of a previous review comment. The return type
Promise<unknown>at line 64 provides no type safety. Based on usage at lines 534, 540, and 557 where properties likeinvitationUrl,invitationDid, andoutOfBandRecord.idare accessed, define a specific interface.Consider creating an interface:
interface IConnectionInvitationResult { invitationUrl: string; invitationDid?: string; outOfBandRecord: { id: string }; }Then update the return type:
- async _createConnectionInvitation(connectionPayload: object, url: string, orgId: string): Promise<unknown> { + async _createConnectionInvitation(connectionPayload: object, url: string, orgId: string): Promise<IConnectionInvitationResult> {
572-595: Persistent issue: ReplacePromise<any>with a specific interface.This is a repeat of a previous review comment. The
eslint-disable-next-line @typescript-eslint/no-explicit-anycomment at line 576 indicates awareness of the type safety issue. Define a specific interface for the return type instead of usingany.Apply this pattern:
async _createOutOfBandConnectionInvitation( connectionPayload: ICreateConnectionInvitation, url: string, orgId: string - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ): Promise<any> { + ): Promise<IConnectionInvitationResult> {Where
IConnectionInvitationResultis the interface suggested in the previous comment.apps/api-gateway/src/verification/interfaces/verification.interface.ts (2)
3-9: Interface inconsistency:IProofRequestAttributehas breaking changes and missing fields.The issues previously identified still exist:
attributeName,credDefId, andcredentialNameare required but should be optional to match other definitions in the codebase- Missing fields:
attributeNames?: string[]andschemaId?: string
62-66: Type error:namesshould be an array.The issue previously identified still exists:
namesis typed asstringbut should bestring[]to represent an array of attribute names (consistent with proof request patterns in the codebase).Apply this diff:
interface IRequestedAttributesName { name?: string; - names?: string; + names?: string[]; restrictions: IRequestedRestriction[]; }apps/verification/src/verification.controller.ts (1)
48-48: Replace overly broadobjectreturn types with specific interfaces.These methods return
Promise<object>orPromise<object | object[]>, which removes type safety and IntelliSense support. This issue was previously flagged. Consider using specific interfaces from the codebase (e.g.,IProofPresentationDetails,IProofPresentationList) to provide proper type contracts for API consumers.Also applies to: 71-71, 82-82
Also applies to: 71-71, 82-82
apps/issuance/src/issuance.controller.ts (1)
1-2: Address ESLint disable directives as technical debt.The blanket ESLint disables mask potential code quality issues across the entire controller. While the TODO comment acknowledges this, consider incrementally enabling rules and fixing violations method-by-method, or using granular disables only where justified.
libs/common/src/nats.config.ts (1)
17-18: Critical: NATS queue group removal affects message distribution.Commenting out the queue assignment changes NATS to broadcast messages to all service instances instead of load-balanced distribution. This affects 30+ microservices and could cause duplicate message processing. Additionally, line 17 is missing a trailing comma after
reconnectTimeWait.Apply this diff to restore the queue assignment and fix the syntax:
- reconnectTimeWait: NATSReconnects.reconnectTimeWait - // queue: serviceName + reconnectTimeWait: NATSReconnects.reconnectTimeWait, + queue: serviceNameapps/oid4vc-verification/src/oid4vc-verification.repository.ts (4)
42-57: Add null check fororg_agents_typeresult.
101-120: Fix error message in catch block.
138-155: Fix inconsistent error messages in catch blocks.
157-174: UsefindFirstinstead offindUniquefor multi-condition queries.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts (1)
34-37: Enable nested validation forcredentialOfferPayload.apps/oid4vc-issuance/test/app.e2e-spec.ts (1)
18-18: Remove the HTTP test—this is a microservices-only module without HTTP routes.apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.module.ts (1)
17-17: Fix NATS service name to match the OIDC microservice.apps/oid4vc-issuance/src/main.ts (1)
15-15: API gateway oid4vc-issuance module uses incorrect NATS service name.apps/api-gateway/src/x509/x509.module.ts (1)
17-24: Fix incorrect module placement in ClientsModule.register().
CommonModuleis incorrectly placed inside theClientsModule.register()array. Theregister()method expects client configuration objects (withname,transport,optionsproperties), not module imports. This will cause a runtime error.Apply this diff to fix the module placement:
imports: [ HttpModule, ConfigModule.forRoot(), + CommonModule, ClientsModule.register([ { name: 'NATS_CLIENT', transport: Transport.NATS, options: getNatsOptions(CommonConstants.X509_SERVICE, process.env.API_GATEWAY_NKEY_SEED) - }, - CommonModule + } ]) ],.env.sample (1)
126-127: Fix incorrect service references in NKEY seed comments.Lines 126 and 127 both reference "x509 service" in their comments, but line 126 should reference "OIDC4VC issuance service" and line 127 should reference "OIDC4VC verification service". This documentation error could confuse developers configuring these environment variables.
Apply this diff to correct the comments:
-OIDC4VC_ISSUANCE_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for x509 service -OIDC4VC_VERIFICATION_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for x509 service +OIDC4VC_ISSUANCE_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for OIDC4VC issuance service +OIDC4VC_VERIFICATION_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for OIDC4VC verification serviceapps/oid4vc-issuance/constant/issuance.ts (1)
1-6: Code duplication with apps/issuance/constant/issuance.ts.These constants duplicate definitions in
apps/issuance/constant/issuance.ts. Consider consolidating into a shared location (e.g.,libs/common/constants) to maintain a single source of truth and avoid drift between the two modules.libs/common/src/cast.helper.ts (1)
527-542: Critical logic error: condition prevents function from working with arrays.The condition on line 530
if (!queryParams || (queryParams?.length >= 0))is fundamentally flawed:
- For arrays:
queryParams?.length >= 0is alwaystrue(any array has length ≥ 0), causing the function to return early and never process array inputs.- For objects: While this evaluates to
falseand allows processing, the check doesn't validate whether the object is empty.The likely intent was to check for null/undefined or empty objects.
Apply this diff to fix the logic and address ESLint issues:
-export function buildUrlWithQuery<T extends Record<string, any>>(baseUrl: string, queryParams: T): string { +export function buildUrlWithQuery<T extends Record<string, unknown>>(baseUrl: string, queryParams: T): string { const criteriaParams: string[] = []; - if (!queryParams || (queryParams?.length >= 0)) { - return baseUrl + if (!queryParams || Object.keys(queryParams).length === 0) { + return baseUrl; } for (const [key, value] of Object.entries(queryParams)) { // Skip undefined or null values - if (value !== undefined && value !== null) { + if (undefined !== value && null !== value) { criteriaParams.push(`${encodeURIComponent(key)}=${encodeURIComponent(String(value))}`); } } - return criteriaParams.length > 0 ? `${baseUrl}?${criteriaParams.join('&')}` : baseUrl; + return 0 < criteriaParams.length ? `${baseUrl}?${criteriaParams.join('&')}` : baseUrl; }This fix:
- Properly checks for empty objects using
Object.keys(queryParams).length === 0- Adds missing semicolon
- Fixes Yoda conditions to satisfy ESLint
- Replaces
anywithunknownfor better type safetyapps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier.dto.ts (1)
22-42: AlignverifierIdsemantics between DTOs and controller.
CreateVerifierDto.verifierIdis illustrated as'verifier-12345'(non-UUID), but the controller’sGET /orgs/:orgId/oid4vp/verifierendpoint documents and validatesverifierIdas a UUID v4. Clients following this DTO example will hit validation errors on that GET/DELETE path.Decide whether
verifierIdis a public identifier or an internal UUID and then:
- either update this DTO’s description/example (and any other usages) to show a UUID, or
- treat
verifierIdconsistently as a public string in the controller (dropParseUUIDPipethere and query by the public ID field).apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (2)
157-237: ReconcileverifierIdUUID validation with how verifier IDs are modeled.
getVerifierDetailsvalidates the optionalverifierIdquery param withParseUUIDPipe({ version: '4', optional: true, ... })and documents it as a UUID, butCreateVerifierDtoshowsverifierIdas a non-UUID string (e.g.'verifier-12345'). This mismatch will 400 clients that follow the DTO sample.As previously suggested, decide whether this param is the internal UUID or a public identifier and then either:
- keep
ParseUUIDPipeand update DTOs/docs to use UUIDs, or- treat it as a string identifier (remove
ParseUUIDPipehere and align repository lookups accordingly).
308-413: Use the sharedIResponseenvelope for presentation-related GETs.
getVerificationPresentationandgetVerificationPresentationResponsecurrently return a custom envelope:res.status(HttpStatus.OK).json({ success: true, message: '...', data: result });while the rest of this controller uses the shared
IResponse/IResponseTypeshape withstatusCodeandResponseMessages.*. For consistency with existing consumers andApiResponseDto, it’s better to:
- build an
IResponseobject withstatusCode, a message fromResponseMessages.oid4vpSession.*, anddata: result, and- return it via
res.status(HttpStatus.OK).json(finalResponse)as in the other methods.apps/api-gateway/src/x509/x509.service.ts (1)
55-64: AligngetX509CertificatesByOrgIdreturn type with paginated response.
getX509CertificatesByOrgIdis declared to returnPromise<X509CertificateRecord>, but the X509 microservice controller responds with a paginated envelope{ data: X509CertificateRecord[]; total: number }, and the REST controller treatsrecordas that envelope. The current signature erodes type safety and can mislead consumers.Consider updating the signature as follows (or aliasing a dedicated type):
- async getX509CertificatesByOrgId( - orgId: string, - x509SearchCriteriaDto: X509SearchCriteriaDto, - reqUser: user - ): Promise<X509CertificateRecord> { + async getX509CertificatesByOrgId( + orgId: string, + x509SearchCriteriaDto: X509SearchCriteriaDto, + reqUser: user + ): Promise<{ data: X509CertificateRecord[]; total: number }> {…and ensure any callers and DTOs expect the envelope rather than a single record.
libs/common/src/date-only.ts (1)
5-23: DateOnly constructor still mixes “date-only” semantics with full Date/time and has edge‑case parsing issuesRight now:
- A
Datekeeps its time‑of‑day.- Missing/falsey strings like
''end up as “now”.DateOnlyinstances are not normalized to a consistent point in time, which can produce different ISO dates across timezones and makesdateToSecondsharder to reason about.To make this truly “date‑only” and align with prior feedback, normalize to UTC midnight and simplify parsing:
- public constructor(date?: string | Date) { - if (date instanceof Date) { - if (isNaN(date.getTime())) { - throw new TypeError('Invalid Date'); - } - this.date = date; - return; - } - if (!date) { - this.date = new Date(); - return; - } - // Accept only YYYY-MM-DD or full ISO strings - const iso = /^\d{4}-\d{2}-\d{2}(T.*Z)?$/.test(date) ? date : ''; - const d = new Date(iso || date); - if (isNaN(d.getTime())) { - throw new TypeError('Invalid date string'); - } - this.date = d; - } + public constructor(date?: string | Date) { + let d: Date; + + if (date instanceof Date) { + d = date; + } else if (!date) { + d = new Date(); + } else { + // Parse ISO date string (YYYY-MM-DD) or full ISO timestamp + d = new Date(date); + } + + if (isNaN(d.getTime())) { + throw new TypeError('Invalid date'); + } + + // Normalize to UTC midnight for date-only semantics + this.date = new Date(Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate())); + }This also makes
toISOString()anddateToSecondsmuch more predictable across environments.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (1)
240-253:templateis still not required; invalid payloads can slip through and break downstream
templateis mandatory for both SD‑JWT and mDL templates, but there’s no@IsDefined()here. The validator will happily accept bodies withouttemplate, and your service code will then dereferenceundefined.You can fix this by marking it as required before nested validation:
@ApiProperty({ type: Object, oneOf: [{ $ref: getSchemaPath(SdJwtTemplateDto) }, { $ref: getSchemaPath(MdocTemplateDto) }], description: 'Credential template definition (depends on credentialFormat)' }) + @IsDefined({ message: 'template is required' }) @ValidateNested() @Type(({ object }) => { if (object.format === CredentialFormat.Mdoc) { return MdocTemplateDto; } else if (object.format === CredentialFormat.SdJwtVc) { return SdJwtTemplateDto; } }) template: SdJwtTemplateDto | MdocTemplateDto;apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (1)
156-158: Add validation metadata forissuerIdon update DTO
issuerIdcurrently has no decorators, so any type (number/object) can flow through updates unchecked, and it also won’t be described in the OpenAPI schema. Mirroring the create DTO keeps update payloads well-formed.export class IssuerUpdationDto { - issuerId?: string; + @ApiPropertyOptional({ + description: 'Optional override for the issuer identifier', + example: 'credebl-university' + }) + @IsOptional() + @IsString({ message: 'issuerId must be a string' }) + issuerId?: string;apps/oid4vc-verification/src/oid4vc-verification.service.ts (3)
63-78: Fix conflict handling on verifier creation (avoid undefined dereference)If the agent returns a 409 before a body is set,
createdVerifierDetailsis stillundefinedand the conflict path throws when accessing.verifierId, masking the intended 409 with a 500. Use the knownverifierIdfrom the request instead.try { createdVerifierDetails = await this._createOid4vpVerifier(createVerifier, url, orgId); @@ } catch (error) { const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${createdVerifierDetails.verifierId}' already exists`); + throw new ConflictException(`Verifier with id '${verifierId}' already exists`); } throw error; }
100-139: Guard empty verifier list and avoid using possibly-undefinedupdatedVerifierDetailsTwo issues here:
if (0 > existingVerifiers.length)never triggers for an empty array, so you fall through and dereferenceexistingVerifiers[0]when nothing exists.- In the conflict path you reference
updatedVerifierDetails.verifierId, which isundefinedif the agent rejected the update before returning data, causing a new error.- const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); - if (0 > existingVerifiers.length) { + const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); + if (!existingVerifiers || existingVerifiers.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } @@ } catch (error) { @@ - if (status409) { - throw new ConflictException(`Verifier with id '${updatedVerifierDetails.verifierId}' already exists`); - } + if (status409) { + const conflictId = existingVerifiers?.[0]?.publicVerifierId ?? verifierId; + throw new ConflictException(`Verifier with id '${conflictId}' already exists`); + } throw error; }
207-259: Correct request signer key type usage for X.509 flowsIn the X.509 branch you call
getCurrentActiveCertificatewithsessionRequest.requestSigner.methodv(always undefined) and your error message has a stray}. This breaks x5c-based flows and yields confusing errors.- const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( - orgId, - sessionRequest.requestSigner.methodv - ); + const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( + orgId, + sessionRequest.requestSigner.method + ); @@ - if (!activeCertificate) { - throw new NotFoundException( - `No active certificate(${sessionRequest.requestSigner.method}}) found for issuer` - ); - } + if (!activeCertificate) { + throw new NotFoundException( + `No active certificate(${sessionRequest.requestSigner.method}) found for issuer` + ); + }apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts (1)
93-98: TypeIssuerUpdation.displayexplicitly and keep it optional
displayis currently untyped, so it becomesanyand defeats compile-time validation for a core field in issuer updates. Making it an optional, properly typed property keeps updates safe and allows partial patches.export interface IssuerUpdation { issuerId: string; accessTokenSignerKeyType: AccessTokenSignerKeyType; - display; + display?: Display[]; batchCredentialIssuanceSize?: number; }
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts
Outdated
Show resolved
Hide resolved
| export class CredentialDto { | ||
| @ApiProperty({ | ||
| description: 'Unique ID of the supported credential', | ||
| example: 'DrivingLicenseCredential-mdoc' | ||
| }) | ||
| @IsString() | ||
| credentialSupportedId: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Signer options for credential issuance', | ||
| example: { | ||
| method: 'x5c', | ||
| x5c: [ | ||
| 'MIIB3jCCAZCgAwIBAgIQQfBdIK9v3TIzKR+1HjlixDAFBgMrZXAwJDEUMBIGA1UEAxMLRFkgdGVzdCBvcmcxDDAKBgNVBAYTA0lORDAeFw0yNTA5MjQwMDAwMDBaFw0yODA5MjQwMDAwMDBaMCQxFDASBgNVBAMTC0RZIHRlc3Qgb3JnMQwwCgYDVQQGEwNJTkQwKjAFBgMrZXADIQDIkLycOlkHP6+MG4rprj8fyxRfwqhH8Xx9v0XxCd175aOB1zCB1DAdBgNVHQ4EFgQUbqjjbQgbAx3lPjkPBVQwvvF14agwDgYDVR0PAQH/BAQDAgGGMBUGA1UdJQEB/wQLMAkGByiBjF0FAQIwOwYDVR0SBDQwMoIXaHR0cDovL3Rlc3QuZXhhbXBsZS5jb22GF2h0dHA6Ly90ZXN0LmV4YW1wbGUuY29tMDsGA1UdEQQ0MDKCF2h0dHA6Ly90ZXN0LmV4YW1wbGUuY29thhdodHRwOi8vdGVzdC5leGFtcGxlLmNvbTASBgNVHRMBAf8ECDAGAQH/AgEAMAUGAytlcANBALTqC64XSTRUoMmwYbCD/z46U/Je6IeQsh6qq4qXh+wfnMIfJMvLQnG+nMkfeAs3zYAwjK6sCZ/7lHkEJnYObQ4=' | ||
| ] | ||
| } | ||
| }) | ||
| @ValidateNested() | ||
| @Type(() => SignerOptionsDto) | ||
| signerOptions: SignerOptionsDto; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Credential format type', | ||
| enum: ['mso_mdoc', 'vc+sd-jwt'], | ||
| example: 'mso_mdoc' | ||
| }) | ||
| @IsString() | ||
| @IsIn(['mso_mdoc', 'vc+sd-jwt'], { message: 'format must be either "mso_mdoc" or "vc+sd-jwt"' }) | ||
| format: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Credential payload (namespace data, validity info, etc.)', | ||
| example: [ | ||
| { | ||
| namespaces: { | ||
| 'org.iso.23220.photoID.1': { | ||
| birth_date: '1970-02-14', | ||
| family_name: 'Müller-Lüdenscheid', | ||
| given_name: 'Ford Praxibetel', | ||
| document_number: 'LA001801M' | ||
| } | ||
| }, | ||
| validityInfo: { | ||
| validFrom: '2025-04-23T14:34:09.188Z', | ||
| validUntil: '2026-05-03T14:34:09.188Z' | ||
| } | ||
| }, | ||
| { | ||
| full_name: 'Garry', | ||
| address: { | ||
| street_address: 'M.G. Road', | ||
| locality: 'Pune', | ||
| country: 'India' | ||
| }, | ||
| iat: 1698151532, | ||
| nbf: dateToSeconds(new Date()), | ||
| exp: dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000)) | ||
| } | ||
| ] | ||
| }) | ||
| @ValidateNested() | ||
| payload: object; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
CredentialDto.payload should be validated as a plain object instead of ValidateNested
payload is currently:
@ValidateNested()
payload: object;Since object is not a DTO class and there is no @Type, ValidateNested is effectively a no-op and may confuse readers. If this is intended to be an arbitrary JSON object:
@IsObject()
payload: Record<string, unknown>;If you need structured validation, consider defining a dedicated DTO and using @Type(() => ThatDto) instead.
🧰 Tools
🪛 ESLint
[error] 269-269: There should be no linebreak after '['.
(array-bracket-newline)
[error] 271-271: There should be no linebreak before ']'.
(array-bracket-newline)
[error] 313-313: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
[error] 313-313: Unexpected mix of '+' and '*'. Use parentheses to clarify the intended order of operations.
(no-mixed-operators)
🤖 Prompt for AI Agents
In apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts around lines
257-319, the payload property uses @ValidateNested() on a plain object which is
ineffective; replace the decorator and type to validate arbitrary JSON by using
@IsObject() and typing payload as Record<string, unknown>, or if the payload has
a specific structure create a dedicated DTO and use @ValidateNested() with
@Type(() => ThatDto) instead.
| /* | ||
| Warnings: | ||
| - You are about to drop the column `issuer` on the `credential_templates` table. All the data in the column will be lost. | ||
| - Added the required column `issuerId` to the `credential_templates` table without a default value. This is not possible if the table is not empty. | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
Address the data migration risk for non-empty tables.
The warning on lines 4–5 indicates that issuerId is added as NOT NULL without a default value. This migration will fail if credential_templates already contains rows (which is possible if the first migration ran in any environment before this one).
The migration lacks a backfill strategy or seed data to populate issuerId before the constraint is applied.
Verify:
- Whether
credential_templateshad any rows when this migration ran. - If a backfill/seed procedure exists to populate
issuerIdbefore applying the NOT NULL constraint. - Consider adding a data migration step (e.g., using Prisma's
sql()helper or a separate backfill script) to handle existing rows.
I'll verify the data migration risk by examining the full migration file and checking for backfill strategies.
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the full migration file
cat libs/prisma-service/prisma/migrations/20250822145801_fix_issuer_relation/migration.sql
Length of output: 1320
🏁 Script executed:
#!/bin/bash
# Search for seed files or backfill scripts
fd -t f -e seed -e backfill -e "migration" | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for any SQL backfill or UPDATE statements in migration files
rg -n "UPDATE.*credential_templates" libs/prisma-service/prisma/migrations/ || echo "No UPDATE found for credential_templates"Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there are any seed scripts in the prisma directory
find libs/prisma-service -name "seed*" -o -name "*backfill*" | head -20Length of output: 34
🏁 Script executed:
#!/bin/bash
# List all migrations to understand the sequence
ls -la libs/prisma-service/prisma/migrations/ 2>/dev/null | head -30Length of output: 2515
🏁 Script executed:
#!/bin/bash
# Search for Prisma schema definition to understand the structure
cat libs/prisma-service/prisma/schema.prisma 2>/dev/null | grep -A 20 "credential_templates" || echo "Schema file or model not found"Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Check for any issuerId references in migrations or backfill logic
rg -n "issuerId" libs/prisma-service/prisma/migrations/ -C 2Length of output: 2376
🏁 Script executed:
#!/bin/bash
# Look for sql() helpers or raw SQL in the migration that might do backfill
grep -r "sql\|UPDATE\|INSERT\|DEFAULT" libs/prisma-service/prisma/migrations/20250822145801_fix_issuer_relation/Length of output: 803
Migration will fail on non-empty credential_templates table.
The migration file lacks any backfill logic. Line 10 adds issuerId as UUID NOT NULL without a default value and without an UPDATE statement to populate existing rows. This violates the NOT NULL constraint and will cause the migration to fail with a constraint violation error if any rows exist in credential_templates.
Additionally, the migration drops the issuer column (line 9) without preserving its data or mapping it to the new oidc_issuer table, making data recovery impossible after the migration fails or succeeds.
Required fix: Add an UPDATE statement (before the ADD COLUMN) to populate issuerId with a valid UUID from the oidc_issuer table, or provide a default mechanism (e.g., a migration-time value or backfill script) to satisfy the foreign key constraint.
libs/prisma-service/prisma/migrations/20250822145801_fix_issuer_relation/migration.sql
Show resolved
Hide resolved
| attributes: attributeValue[]; | ||
| @ApiProperty({ example: { attributes: [{ value: 'string', name: 'string' }] } }) | ||
| @IsNotEmpty({ message: 'Please provide valid attributes' }) | ||
| @IsArray({ message: 'attributes should be array' }) |
There was a problem hiding this comment.
Ig here we can add the @ArrayNotEmpty instead
apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts
Outdated
Show resolved
Hide resolved
| ClientsModule.register([ | ||
| { | ||
| name: 'NATS_CLIENT', | ||
| transport: Transport.NATS, | ||
| options: getNatsOptions(CommonConstants.X509_SERVICE, process.env.API_GATEWAY_NKEY_SEED) | ||
| }, | ||
| CommonModule | ||
| ]) |
There was a problem hiding this comment.
Ig this is the reason why we might've needed to add CommonService as provider
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| this.logger.error('CRYPTO_PRIVATE_KEY is not configured'); | ||
| throw new InternalServerErrorException('Encryption key is not configured'); | ||
| } | ||
| const encryptedToken = CryptoJS.AES.encrypt(JSON.stringify(token), secret).toString(); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
To fix the problem, the secret should be protected using a proper key derivation function (e.g., bcrypt, scrypt, Argon2, or PBKDF2). If the intent is to securely hash API keys for storage/validation rather than to encrypt/decrypt them (e.g., to validate later, or store safely), use a slow/fixed-cost hash (e.g., bcrypt). If you need to be able to recover the original data (not just verify), then you must ensure your symmetric encryption is using a robust, unique key per secret (often derived with a KDF from a password), and a random initialization vector (IV); as is, a static key and lack of IV make it easy to brute force.
Assuming encryption is being used to store API keys safely without the requirement to retrieve (only to check validity), then using bcrypt to hash the API key and storing the hash is appropriate. The tokenEncryption function should use bcrypt to hash the API key and return the hash string. Since bcrypt does not have a decrypt function, only a compare, downstream code that expects to decrypt the API key (i.e., recover the original API key) would need review---but based on what's shown, this appears to only be about safe storage.
You should:
- Replace the use of
CryptoJS.AES.encrypt(JSON.stringify(token), secret).toString();with a bcrypt hash. - Add the required import statement for bcrypt.
- If you need interoperability (e.g., other parts of the code depend on decryption), further design changes may be needed outside what's visible.
- Example: change tokenEncryption to return
bcrypt.hash(async) result, using a salt (provided or generated).
| @@ -18,6 +18,7 @@ | ||
| import { ClientProxy, RpcException } from '@nestjs/microservices'; | ||
| import * as dotenv from 'dotenv'; | ||
| import * as fs from 'fs'; | ||
| import * as bcrypt from 'bcrypt'; | ||
| import { map } from 'rxjs/operators'; | ||
| dotenv.config(); | ||
| import { | ||
| @@ -2201,14 +2202,10 @@ | ||
|
|
||
| private async tokenEncryption(token: string): Promise<string> { | ||
| try { | ||
| const secret = process.env.CRYPTO_PRIVATE_KEY; | ||
| if (!secret) { | ||
| this.logger.error('CRYPTO_PRIVATE_KEY is not configured'); | ||
| throw new InternalServerErrorException('Encryption key is not configured'); | ||
| } | ||
| const encryptedToken = CryptoJS.AES.encrypt(JSON.stringify(token), secret).toString(); | ||
|
|
||
| return encryptedToken; | ||
| // Use bcrypt to hash the token with a generated salt (cost factor 12 is a good default) | ||
| const saltRounds = 12; | ||
| const hashedToken = await bcrypt.hash(token, saltRounds); | ||
| return hashedToken; | ||
| } catch (error) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/webhook/src/webhook.service.ts (2)
28-54: Type mismatch: returning string instead of interface object.The method signature declares
Promise<ICreateWebhookUrl>, but Line 47 returnswebhookUrl.webhookUrl, which is a string property. The repository'sregisterWebhookreturns the fullorg_agentsobject (result of Prisma'supdate), so this should return the entire object, not just the string field.Apply this diff:
- return webhookUrl.webhookUrl; + return webhookUrl;Alternatively, if
ICreateWebhookUrlis intended to be a string type alias, update the return statement to match that contract. Verify the interface definition to confirm the correct behavior.
56-70: Type mismatch: returning string instead of interface object.The method signature declares
Promise<IGetWebhookUrl>, but Line 65 returnswebhookUrlInfo.webhookUrl, which is a string property. The repository'sgetWebhookUrlreturns the fullorg_agentsobject from Prisma, so this should return the entire object, not just the string field.Apply this diff:
- return webhookUrlInfo.webhookUrl; + return webhookUrlInfo;Alternatively, if
IGetWebhookUrlis intended to be a string type alias, update the return statement to match that contract. Verify the interface definition to confirm the correct behavior.apps/cloud-wallet/src/cloud-wallet.service.ts (2)
146-159: Fix template literal syntax error.Line 151 contains a syntax error with an extra closing brace in the template literal, which will result in a malformed URL and cause API calls to fail.
Apply this diff:
- const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${proofRecordId}}`; + const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${proofRecordId}`;
166-180: Fix template literal syntax error.Line 173 contains a syntax error with an extra closing brace in the template literal, which will result in a malformed URL and cause API calls to fail.
Apply this diff:
- const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${threadParam}}`; + const url = `${agentEndpoint}${CommonConstants.CLOUD_WALLET_GET_PROOF_REQUEST}/${threadParam}`;apps/agent-service/src/agent-service.service.ts (1)
1930-1940: Bug:if (OrgAgentType.SHARED)is always true indeleteWalletIn
deleteWallet:let getApiKey; if (OrgAgentType.SHARED) { const platformAdminSpinnedUp = await this.agentServiceRepository.platformAdminAgent(...); getApiKey = await this.commonService.decryptPassword(platformAdminSpinnedUp?.org_agents[0].apiKey); } else { getApiKey = getApiKeyResult?.value; }
OrgAgentType.SHAREDis an enum value, so this condition is always truthy. You never use the org-specific API key path, even for dedicated agents.This can break deletion for dedicated wallets (wrong key) and hides config problems.
You likely meant to branch on
orgAgentTypeResult.agent(or equivalent):- let getApiKey; - if (OrgAgentType.SHARED) { + let getApiKey; + if (orgAgentTypeResult.agent === OrgAgentType.SHARED) { const platformAdminSpinnedUp = await this.agentServiceRepository.platformAdminAgent( CommonConstants.PLATFORM_ADMIN_ORG ); getApiKey = await this.commonService.decryptPassword(platformAdminSpinnedUp?.org_agents[0].apiKey); } else { getApiKey = getApiKeyResult?.value; }This is a functional bug and should be corrected before release.
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts
Outdated
Show resolved
Hide resolved
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts
Outdated
Show resolved
Hide resolved
| export function dateToSeconds(date: Date | DateOnly): number { | ||
| const realDate = date instanceof DateOnly ? new Date(date.toISOString()) : date; | ||
| if (isNaN(realDate.getTime())) { | ||
| throw new TypeError('dateToSeconds: invalid date'); | ||
| } | ||
| return Math.floor(realDate.getTime() / 1000); |
There was a problem hiding this comment.
dateToSeconds has environment‑dependent behavior for DateOnly inputs
For DateOnly values, this function does:
const realDate = date instanceof DateOnly ? new Date(date.toISOString()) : date;Given date.toISOString() returns YYYY-MM-DD, parsing this string is implementation‑ and environment‑dependent (some engines treat it as local time, some as UTC), so:
- The same
DateOnlycan produce different epoch seconds on different runtimes or host timezones. - Any original timezone intent that went into constructing
DateOnlyis fully lost in a non‑obvious way. - Callers may assume “midnight UTC” but actually get “midnight local” (or vice versa).
To make this deterministic:
- Decide whether
DateOnlyrepresents a UTC date or a local calendar date. - Then derive seconds accordingly, e.g. for UTC semantics:
export function dateToSeconds(date: Date | DateOnly): number {
const d =
date instanceof DateOnly
? new Date(Date.UTC(
// Get the date parts from the string or from internal Date
Number(date.toISOString().slice(0, 4)),
Number(date.toISOString().slice(5, 7)) - 1,
Number(date.toISOString().slice(8, 10)),
))
: date;
if (isNaN(d.getTime())) {
throw new TypeError('dateToSeconds: invalid date');
}
return Math.floor(d.getTime() / 1000);
}Or, better, expose a toDate()/toEpochSeconds() method on DateOnly and delegate to that so the normalization logic lives in one place.
🤖 Prompt for AI Agents
In libs/common/src/date-only.ts around lines 52-57, dateToSeconds currently
builds a Date from date.toISOString(), which parses "YYYY-MM-DD" in an
environment-dependent way; make it deterministic by treating DateOnly as a
calendar date in UTC and constructing the epoch seconds from the ISO date parts
(year, month-1, day) using UTC arithmetic, or preferably call a new/existing
DateOnly.toDate() or toEpochSeconds() method that encapsulates this logic;
ensure invalid dates still throw the same TypeError and return
Math.floor(epochMillis/1000).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (2)
298-308: DecouplerequestSignerfrom Prisma client types.The DTO imports
SignerOptionfrom@prisma/client(line 17) and uses it as the type forrequestSigner.method(line 307). This couples the API contract to the persistence layer, which was flagged in the previous review.There's a separate
SignerOptioninterface inapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tswith a different shape ({ method: SignerMethodOption; did?: string; x5c?: string[] }), creating naming ambiguity.Recommendation: Replace the Prisma import with a domain-level DTO or enum from a shared interface module:
-import { SignerOption } from '@prisma/client'; +import { SignerMethodOption } from '@credebl/common/interfaces/oid4vc'; // or appropriate pathThen update the type:
requestSigner?: { - method: SignerOption; + method: SignerMethodOption; };This keeps the API surface independent of database schema changes.
Based on learnings
341-347: Remove contradictory@ApiPropertyOptionaldecorator.Lines 341–344 apply both
@ApiHideProperty()and@ApiPropertyOptional()to_oneOfCheck. These decorators contradict each other:
@ApiHideProperty()excludes the field from the OpenAPI schema.@ApiPropertyOptional()includes it.Since the intent (per the comment) is to keep this field internal to validation, remove the
@ApiPropertyOptionaldecorator:@ApiHideProperty() -@ApiPropertyOptional({ - description: 'Internal: ensures exactly one of dcql or presentationExchange is present' -}) @Validate(OnlyOneOfConstraint, ['dcql', 'presentationExchange']) // eslint-disable-next-line @typescript-eslint/no-explicit-any _oneOfCheck?: any;Based on learnings
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (3)
141-198: Re-verify issuer service method names (oidcGetIssuerById/oidcGetIssuers)The controller calls:
const oidcIssuer = await this.oid4vcIssuanceService.oidcGetIssuerById(id, orgId); ... const oidcIssuer = await this.oid4vcIssuanceService.oidcGetIssuers(orgId);A previous review noted that the service defined
oidcIssuerGetByIdandoidcIssuersinstead. If the service names were not updated to match the controller, this will fail TypeScript compilation.Please confirm that
Oid4vcIssuanceServiceactually exposesoidcGetIssuerById(id, orgId)andoidcGetIssuers(orgId)(or adjust one side so they match consistently).You can verify alignment with a quick search:
#!/bin/bash # From repo root service_file=$(fd oid4vc-issuance.service.ts | head -n 1) echo "Service file: $service_file" rg -n "oidcIssuerGetById\\(|oidcGetIssuerById\\(" "$service_file" -n -C2 || echo "No issuer get-by-id methods found" rg -n "oidcIssuers\\(|oidcGetIssuers\\(" "$service_file" -n -C2 || echo "No issuer list methods found"
240-438: Template endpoints: double-check service signatures and issuer scopingThere are a couple of points worth tightening up in the template handlers:
Service call signatures vs service implementation
Create template:
const template = await this.oid4vcIssuanceService.createTemplate( CredentialTemplate, user, orgId, issuerId );Update template:
const updated = await this.oid4vcIssuanceService.updateTemplate( user, orgId, templateId, dto, issuerId );A previous review indicated the service originally exposed different parameter orders (e.g.,
(credentialTemplate, orgId, issuerId)for create,(templateId, dto, orgId, issuerId)for update). If the service signatures were not updated to match these controller calls, you’ll hit compile-time errors or, worse, subtle bugs if any of the args are structurally compatible.Please re-check
Oid4vcIssuanceService.createTemplate/.updateTemplatesignatures and align the argument order on one side only (controller or service), and keep that ordering consistent across all template flows.
issuerIdis parsed but not forwarded on delete
IndeleteTemplateyou validateissuerIdviaParseUUIDPipe, but the service is called without it:await this.oid4vcIssuanceService.deleteTemplate(user, orgId, templateId);That means issuer scoping for deletions cannot rely on the path parameter. If templates are meant to be issuer‑scoped (as suggested by the route and descriptions), it’s safer to forward
issuerIdinto the service and validate that the template belongs to that issuer.For example (controller side only):
- await this.oid4vcIssuanceService.deleteTemplate(user, orgId, templateId);
- await this.oid4vcIssuanceService.deleteTemplate(user, orgId, templateId, issuerId);
and then extend the service signature accordingly. --- `440-588`: **Fix credential-offer delete response status and re-check “get all offers” service name** 1. **204 with JSON body & Swagger mismatch (delete offer)** `deleteCredentialOffers` currently builds a body but returns 204: ```ts const finalResponse: IResponse = { statusCode: HttpStatus.NO_CONTENT, message: ResponseMessages.oidcIssuerSession.success.delete, data: deletedofferDetails }; return res.status(HttpStatus.NO_CONTENT).json(finalResponse);
- HTTP 204 semantics imply no response body.
@ApiResponsedocumentsHttpStatus.OK.This combination is inconsistent and can confuse clients and tooling. Easiest fix is to return 200 with the body, matching the Swagger annotation:
- const finalResponse: IResponse = { - statusCode: HttpStatus.NO_CONTENT, + const finalResponse: IResponse = { + statusCode: HttpStatus.OK, message: ResponseMessages.oidcIssuerSession.success.delete, data: deletedofferDetails }; - - return res.status(HttpStatus.NO_CONTENT).json(finalResponse); + return res.status(HttpStatus.OK).json(finalResponse);
getAllCredentialOffersvs service method name
The controller calls:const connectionDetails = await this.oid4vcIssuanceService.getAllCredentialOffers( orgId, getAllCredentialOffer );A previous review noted the service method was named
getCredentialOffers(orgId, dto). If that name remains, this will not compile.Please ensure the service now exposes
getAllCredentialOffersor, if not, rename the call (or method) so both sides agree.You can quickly confirm the service method name with:
#!/bin/bash # From repo root service_file=$(fd oid4vc-issuance.service.ts | head -n 1) echo "Service file: $service_file" rg -n "getAllCredentialOffers\\(" "$service_file" -n -C2 || echo "getAllCredentialOffers not found" rg -n "getCredentialOffers\\(" "$service_file" -n -C2 || echo "getCredentialOffers not found"
🧹 Nitpick comments (4)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (3)
137-160: Validate or document theformatfield structure.The
formatfield (line 153) uses an unvalidatedRecord<string, unknown>, which permits arbitrary key-value pairs. Per PEX spec, format identifiers typically follow a known set (e.g.,jwt_vc,jwt_vp,ldp_vc,vc+sd-jwt). Consider either:
- Creating a dedicated DTO class with specific format keys and
@ValidateNested(), or- Documenting the expected structure in the
@ApiPropertydescription if validation is intentionally deferred to downstream services.
214-243: Validate or document themetafield structure.The
metafield (line 227) uses an unvalidatedRecord<string, unknown>. Similar to the PEXformatfield, this allows arbitrary key-value pairs. For DCQL queries,metatypically contains credential-type metadata (e.g.,vctfor VC types). Consider creating a dedicated DTO or documenting the expected structure to prevent runtime surprises.
276-296: Consider conventional comparison order.Line 285 uses
null !== object[p]instead of the more conventionalobject[p] !== null. While functionally equivalent, the conventional order improves readability and consistency with typical JavaScript/TypeScript style.- if (object[p] !== undefined && null !== object[p]) { + if (object[p] !== undefined && object[p] !== null) {apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (1)
74-139: Align issuer create/update HTTP status codes and docs
oidcIssuerCreateandoidcIssuerUpdateboth respond withHttpStatus.CREATED(201) infinalResponse.statusCodeandres.status(...), but the@ApiResponsedecorators documentHttpStatus.OK(200). That drift will confuse clients relying on the OpenAPI.Consider:
- For create: keep 201 but change
@ApiResponse({ status: HttpStatus.CREATED, ... }).- For update: either switch to 200 in code and docs, or explicitly document 201 if you want “upsert”-like semantics.
Also, the JSDoc above
oidcIssuerCreatestill talks about “status of the deletion operation”; it should describe issuer creation for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfiles/Dockerfile.x509(1 hunks)apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts(1 hunks)apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfiles/Dockerfile.x509
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (2)
libs/common/src/cast.helper.ts (2)
ValidatorConstraint(129-165)ValidatorConstraint(382-397)apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts (1)
SignerOption(16-20)
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (6)
libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-639)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (2)
IssuerCreationDto(84-154)IssuerUpdationDto(156-180)libs/common/src/interfaces/response.interface.ts (2)
IResponse(8-13)IResponseType(1-7)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (1)
UpdateCredentialTemplateDto(288-288)apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (4)
CreateOidcCredentialOfferDto(169-190)UpdateCredentialRequestDto(210-223)GetAllCredentialOfferDto(192-208)CreateCredentialOfferD2ADto(303-389)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts (1)
OidcIssueCredentialDto(21-65)
🔇 Additional comments (1)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (1)
23-72: LGTM: Query parameter validation is appropriate.The
VerificationPresentationQueryDtoproperly validates optional query parameters with appropriate decorators. The use of@IsUrlforauthorizationRequestUriand@IsEnumfor the state field ensures type safety.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (22)
libs/common/src/response-messages/index.ts (2)
530-530: Literal${templateId}won't be interpolated.The string uses
${templateId}inside single quotes, so it will render literally instead of being interpolated. This is inconsistent withfailedDeleteTemplateon line 542, which expects the ID to be appended externally.As per the previous review, either remove the placeholder:
- deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=${templateId}' + deleteTemplate: '[createTemplate] compensating delete succeeded for templateId='Or convert to a function that accepts
templateIdat call time.
593-593: Incorrect terminology: "template" in session context.The
deleteTemplatekey and message reference "template" within theoid4vpSessionerror section, which handles session operations, not templates.Update to session-appropriate terminology:
- deleteTemplate: 'Error while deleting template.' + deleteSession: 'Error while deleting session.'Or if more specificity is needed:
- deleteTemplate: 'Error while deleting template.' + deleteVerifierSession: 'Error while deleting verifier session.'apps/oid4vc-verification/src/oid4vc-verification.service.ts (6)
64-78: Fix undefined dereference in conflict error message.When the agent returns a 409 status before
createdVerifierDetailsis assigned, line 75 will throw aTypeErrorattempting to read.verifierIdfrom undefined. Use theverifierIdfrom the request parameter instead.Apply this diff:
} catch (error) { const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${createdVerifierDetails.verifierId}' already exists`); + throw new ConflictException(`Verifier with id '${verifierId}' already exists`); } throw error; }
108-140: Fix impossible array guard and undefined dereference in conflict handler.Line 109's condition
0 > existingVerifiers.lengthchecks for negative length, which is impossible. Additionally,getVerifiersByVerifierIdreturnsPromise<oid4vp_verifier[] | null>, so line 109 will throw if null. Line 137 dereferencesupdatedVerifierDetails.verifierIdwhen it may be undefined if the agent rejects the update.Apply this diff:
let updatedVerifierDetails; const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); - if (0 > existingVerifiers.length) { + if (!existingVerifiers || existingVerifiers.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } // updateVerifier['verifierId'] = existingVerifiers[0].publicVerifierId @@ -133,7 +133,8 @@ const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${updatedVerifierDetails.verifierId}' already exists`); + const conflictId = existingVerifiers?.[0]?.publicVerifierId ?? verifierId; + throw new ConflictException(`Verifier with id '${conflictId}' already exists`); } throw error; }
175-205: Add null guard and use agent-facing ID.Line 179 will throw if
checkIdExistis null (repository returnsoid4vp_verifier[] | null). Line 188 passes the internalverifierIdto the agent, but other methods (e.g., line 121 inoid4vpUpdateVerifier) correctly usepublicVerifierIdfor agent communication.Apply this diff:
const checkIdExist = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); - if (0 == checkIdExist.length) { + if (!checkIdExist || checkIdExist.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } @@ -185,7 +185,7 @@ throw new NotFoundException(ResponseMessages.issuance.error.agentEndPointNotFound); } const { agentEndPoint, id } = agentDetails; - const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_VERIFIER_DELETE, checkIdExist[0].verifierId); + const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_VERIFIER_DELETE, checkIdExist[0].publicVerifierId); this.logger.debug(`[deleteVerifierById] calling agent URL=${url}`);
234-244: Fix typo:methodvshould bemethod.Line 241 passes
sessionRequest.requestSigner.methodv(undefined) instead ofsessionRequest.requestSigner.method. This breaks X.509 certificate lookups, causing every x5c-based flow to fail with "No active certificate found".Apply this diff:
const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( orgId, - sessionRequest.requestSigner.methodv + sessionRequest.requestSigner.method );
304-304: Remove redundantawait.Line 304 has
await await, which is redundant and indicates a copy-paste error.Apply this diff:
- const verifiers = await await this._getOid4vpVerifierSession(url, orgId); + const verifiers = await this._getOid4vpVerifierSession(url, orgId);
331-331: Remove redundantawait.Line 331 has the same double
awaitissue as line 304.Apply this diff:
- const verifiers = await await this._getOid4vpVerifierSession(url, orgId); + const verifiers = await this._getOid4vpVerifierSession(url, orgId);libs/logger/src/logging.interceptor.ts (1)
24-33: Add explicit string type validation before using contextId from headers.As flagged in previous reviews, the current truthiness check doesn't enforce that
contextIdis a string. Ifheaders.get('contextId')returns a non-string truthy value (e.g.,123,{ id: 'abc' },true), it will bypass validation and get passed tosetupContextId, potentially causing type violations downstream.While you expect
contextIdto be a string or empty, the code doesn't enforce this expectation, making it vulnerable to incorrect upstream data.Apply this diff to add explicit type validation:
- const contextIdFromHeader = headers && 'function' === typeof headers.get ? headers.get('contextId') : undefined; + const rawContextId = headers && typeof headers.get === 'function' ? headers.get('contextId') : undefined; + const contextIdFromHeader = typeof rawContextId === 'string' && rawContextId ? rawContextId : undefined;This ensures that:
- Only non-empty strings are accepted as valid context IDs
- Any other type (number, object, boolean) triggers UUID generation
- Empty strings are treated as missing and trigger UUID generation
- The type contract of
setupContextId(contextIdFromHeader: string)is never violatedBased on past review comments.
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (4)
61-67: JSDoc and method name still describe an issuer while implementing a verifier createThe block comment and method name
oidcIssuerCreateare misleading for an endpoint that clearly creates an OID4VP verifier (/orgs/:orgId/oid4vp/verifier,ResponseMessages.oid4vp.*). This makes the code harder to follow and can confuse future maintainers.Consider at least fixing the JSDoc, and ideally renaming the method to something verifier-specific (e.g.
createVerifier/oid4vpCreateVerifier) to align with the route and service:- /** - * Create issuer against a org(tenant) - * @param orgId The ID of the organization - * @param user The user making the request - * @param res The response object - * @returns The status of the deletion operation - */ + /** + * Create OID4VP verifier for an organization + * @param orgId The ID of the organization (tenant) + * @param user The user making the request + * @param createVerifier The verifier creation payload + * @param res The response object + * @returns The created verifier details + */Also applies to: 78-104
308-352: Response envelope for presentation GETs diverges fromIResponse/ApiResponseDtocontract
getVerificationPresentationandgetVerificationPresentationResponsecurrently return a custom object:return res.status(HttpStatus.OK).json({ success: true, message: '...', data: result });Every other route in this controller (and the OID4VC issuance controller) returns an
IResponse/IResponseTypewithstatusCode,messagefromResponseMessages.*, and optionaldata, matchingApiResponseDto.For consistency and to avoid surprising consumers:
- Construct an
IResponseobject withstatusCode: HttpStatus.OK, a suitableResponseMessages.oid4vpSession.success.*key, anddata: result.- Return it via
res.status(HttpStatus.OK).json(finalResponse)like the other methods.You could also rely on
CustomExceptionFilterinstead of the manualtry/catch + BadRequestExceptionif you don’t need to normalize all errors to 400.Also applies to: 354-413
106-113: Update verifier JSDoc is incorrect and response statusCode doesn’t match HTTP statusTwo issues here:
- The JSDoc still talks about “Create issuer” even though this is an OID4VP verifier update, and omits the
updateVerifierpayload parameter.@ApiResponseandres.statususeHttpStatus.OK, butfinalResponse.statusCodeisHttpStatus.CREATED. That gives clients conflicting information between the HTTP status line and the JSON body.Suggested fix:
- /** - * Create issuer against a org(tenant) - * @param orgId The ID of the organization - * @param verifierId The ID of the Verifier - * @param user The user making the request - * @param res The response object - * @returns The status of the verifier update operation - */ + /** + * Update OID4VP verifier for an organization + * @param orgId The ID of the organization (tenant) + * @param verifierId The ID of the verifier + * @param user The user making the request + * @param updateVerifier The verifier update payload + * @param res The response object + * @returns The updated verifier details + */ @@ - const finalResponse: IResponse = { - statusCode: HttpStatus.CREATED, + const finalResponse: IResponse = { + statusCode: HttpStatus.OK, message: ResponseMessages.oid4vp.success.update, data: updateVerifierRes };(As above, also consider renaming
oidcIssuerUpdateto something verifier-specific.)Also applies to: 114-155
157-194:verifierIdUUID validation conflicts with DTO example and public verifier identifier usageAcross multiple endpoints,
verifierIdis validated as a UUID v4:
GET /orgs/:orgId/oid4vp/verifier(@Query('verifierId', new ParseUUIDPipe({ version: '4', optional: true, ... })))DELETE /orgs/:orgId/oid4vp/verifier(@Query('verifierId', new ParseUUIDPipe(...)))POST /orgs/:orgId/oid4vp/presentation(@Query('verifierId', new ParseUUIDPipe(...)))However,
CreateVerifierDtodocumentsverifierIdas an arbitrary string with example'verifier-12345', andVerificationPresentationQueryDtointroduces a separatepublicVerifierIdfield, suggesting a public, non-UUID identifier.As written, any client following the DTO example (
'verifier-12345') will get a 400 due to UUID validation, and it’s unclear whether these routes expect the internal UUID (id) or the public identifier.You should reconcile this contract:
If API callers are meant to use the public verifier ID (which matches the DTO example):
- Drop
ParseUUIDPipeforverifierId(or rename the parameter topublicVerifierId) and treat it asstring.- Update
@ApiQuery/Swagger descriptions to say “Public identifier of the verifier (e.g.verifier-12345)”.- Ensure
oid4vcVerificationServicequeries by the public field rather than the internal UUID.If the endpoints are intended to use the internal UUID:
- Update
CreateVerifierDtoto document a UUID example and clarify thatverifierIdis a UUID.- Consider introducing a separate
publicVerifierIdin the DTOs if a human-friendly ID is still needed, and adjust service/database layers accordingly.#!/bin/bash # Quick sanity check: how verifier identifiers are modeled and queried rg -n "publicVerifierId" apps -C3 || true rg -n "getVerifierById" apps -C5 || true rg -n "verifierId" apps/oid4vc-verification apps/api-gateway/src/oid4vc-verification -C3Also applies to: 208-236, 275-282
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (2)
618-666: Webhook handler wiring looks correct; logging is scoped and avoids raw console usage
getIssueCredentialWebhook:
- Sets
orgIdonto the DTO whencontextCorrelationId === 'default'andidis present.- Delegates to the service for core processing.
- Resolves a webhook URL via
_getWebhookUrland, if present, posts back using_postWebhookResponse, with errors logged via the Nest logger rather thanconsole.log.Given your earlier note about not wanting to apply the sanitizer yet, this is a reasonable balance between functionality and observability.
566-588: Don’t return a JSON body with HTTP 204 indeleteCredentialOffersThe delete-offer endpoint currently does:
const deletedofferDetails = await this.oid4vcIssuanceService.deleteCredentialOffers(orgId, credentialId); const finalResponse: IResponse = { statusCode: HttpStatus.NO_CONTENT, message: ResponseMessages.oidcIssuerSession.success.delete, data: deletedofferDetails }; return res.status(HttpStatus.NO_CONTENT).json(finalResponse);HTTP 204 expressly means “no content”; clients typically won’t expect or process a body for that status. Either:
- Switch to
HttpStatus.OK(or 202) if you want to return details, or- Keep 204 and omit the body entirely.
I’d lean toward 200/OK here, given you already have a useful response payload.
apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (2)
42-51: AlignCredentialConfig.claimstype with the nested claims object actually produced
CredentialConfig.claimsis declared asClaim[], but:
buildNestedClaimsbuilds a nestedRecord<string, Claim>structure, and even merges child claim maps into eachClaimnode.buildClaimsFromTemplatereturnsRecord<string, any>.- Both
buildSdJwtCredentialConfigandbuildMdocCredentialConfigassign that object directly toclaims.So at runtime
claimsis a nested object keyed by attribute/namespace, not aClaim[]. This type/shape drift makes downstream use error‑prone and defeats TS guarantees.Consider either:
- Updating the type to match the produced shape, e.g.:
type CredentialConfig = { // ... - claims: Claim[]; + claims: Record<string, Claim | Record<string, Claim>>; };(and tightening
buildClaimsFromTemplate’s return type), or
- Refactoring the builders to emit a
Claim[](or a well-typed tree) if that’s the intended consumer contract.Right now types and values disagree in a core helper.
Also applies to: 243-301, 305-357
391-405: GuardappearanceJsonbefore accessing.displayto avoid null/primitive crashes
coerceJsonObjectcan returnnullor a non-object primitive, butdisplayConfigurationsdoes:const appearanceJson = coerceJsonObject<unknown>(templateRow.appearance); const displayConfigurations = (appearanceJson as Appearance).display?.map(/* ... */) ?? [];If
appearanceJsonisnull/non-object, the cast and.displayaccess will throw before?.mapruns.Wrap with a type guard:
const appearanceJson = coerceJsonObject<unknown>(templateRow.appearance); const appearance = appearanceJson && typeof appearanceJson === 'object' ? (appearanceJson as Appearance) : null; const displayConfigurations = appearance?.display?.map((displayEntry) => ({ // ... })) ?? [];This keeps issuer metadata building robust to bad/empty appearance JSON.
libs/common/src/common.constant.ts (1)
120-143: Resolve TODO onURL_OIDC_VERIFIER_CREATE(and verify OIDC/OID4VP URL constants)
URL_OIDC_VERIFIER_CREATEis still:URL_OIDC_VERIFIER_CREATE = '/openid4vc/verifier', //TODO: correct this URLand several other OIDC4VC/OID4VP URLs were newly added here. Because these constants are wired into
getAgentUrland agent-service calls, leaving an explicitly “TODO: correct this URL” in production code will surface as opaque runtime failures when those paths are exercised.Before enabling this feature, either:
- Update this and related verifier URLs to the final, correct paths and remove the TODO, or
- Gate their usage behind a feature flag and track the URL correction in an issue so it doesn’t get forgotten.
Right now the comment advertises that at least one URL is known wrong.
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (3)
200-212: Fix null‑check inoidcIssuerGetByIdto avoid crash when issuer is missingCurrently:
const getIssuerDetails = await this.oid4vcIssuanceRepository.getOidcIssuerDetailsById(id); if (!getIssuerDetails && !getIssuerDetails.publicIssuerId) { throw new NotFoundException(...); }If
getIssuerDetailsisnull/undefined,!getIssuerDetailsistrueand the right-hand side of&&still evaluates, causingCannot read properties of undefined (reading 'publicIssuerId').Change this to an OR with optional chaining:
if (!getIssuerDetails || !getIssuerDetails?.publicIssuerId) { throw new NotFoundException(ResponseMessages.oidcIssuer.error.notFound); }so you never dereference
publicIssuerIdon a missing issuer.
265-292: UseRpcExceptioninstead of throwing a rawErrorindeleteOidcIssuerIn the
deleteOidcIssuercatch block you special-case Prisma errors, but for everything else you:if (error.name === 'PrismaClientKnownRequestError') { ... } throw new Error(error.response ? error.response : error);Throwing a plain
Errorhere is inconsistent with the rest of the service (which usesRpcException) and will lose structured status/message information for consumers.Prefer:
} catch (error) { if (error?.name === 'PrismaClientKnownRequestError') { throw new BadRequestException(error.meta?.cause ?? ResponseMessages.oidcIssuer.error.deleteFailed); } this.logger.error(`[deleteOidcIssuer] - error: ${JSON.stringify(error)}`); throw new RpcException(error?.response ?? error); }to preserve RPC semantics and logging.
627-649: Remove hard‑coded'Test'x5c stub increateOidcCredentialOfferD2AThis path still uses:
if (signerOptions.method === SignerMethodOption.X5C) { if (!signerOptions.x5c || 0 === signerOptions.x5c.length) { const x5cFromDb = 'Test'; // ... signerOptions.x5c = x5cFromDb; } }This introduces two problems:
- A dummy
'Test'value will produce invalid credential offers if this path is hit.signerOptions.x5cis typed asstring[], but you assign a bare string.You should either:
- Implement the real DB lookup (e.g.,
await this.oid4vcIssuanceRepository.getIssuerX5c(oidcCredentialD2APayload.publicIssuerId, orgId)), assigning the returned array tosignerOptions.x5c, and error if it’s empty, or- Fail fast with a
BadRequestExceptionwhenx5cis missing until the DB integration is ready, instead of injecting a fake value.Leaving this stub in will break any real X.509 flows using the D2A endpoint.
apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts (1)
34-37: Enable nested validation forcredentialOfferPayloadso inner array constraints runRight now:
@ApiProperty({ type: CredentialOfferPayloadDto }) @IsObject() credentialOfferPayload!: CredentialOfferPayloadDto;Without
@ValidateNested()and a@Typehint, class-validator treatscredentialOfferPayloadas an opaque object, so the@IsArray()onCredentialOfferPayloadDto.credential_configuration_idsis never enforced. A malformed webhook payload (e.g. scalar instead of array) will slip through andextractCredentialConfigurationIdswill just return[], hiding the error.Wire up nested validation:
import { ValidateNested } from 'class-validator'; import { Type } from 'class-transformer'; @ApiProperty({ type: CredentialOfferPayloadDto }) @IsObject() @ValidateNested() @Type(() => CredentialOfferPayloadDto) credentialOfferPayload!: CredentialOfferPayloadDto;so invalid inner shapes are rejected instead of silently ignored.
Also applies to: 70-73
🧹 Nitpick comments (10)
apps/oid4vc-verification/test/app.e2e-spec.ts (1)
8-15: Add cleanup inafterEachwhen implementing tests.The
appis initialized inbeforeEachbut there's no corresponding cleanup. When you add actual test cases, ensure proper resource cleanup to prevent memory leaks and port conflicts.Add this after the
beforeEachblock:+ afterEach(async () => { + await app.close(); + });libs/common/src/response-messages/index.ts (1)
557-557: Add space after "Info" for better readability.Minor formatting improvement: add a space between "Info" and the opening parenthesis.
- missingValidityInfo: 'Validity Info(validFrom, validTo) is required for validity of credential' + missingValidityInfo: 'Validity Info (validFrom, validTo) is required for validity of credential'apps/oid4vc-verification/src/oid4vc-verification.service.ts (1)
1-8: Consider progressively re-enabling ESLint rules.Disabling many linting rules (camelcase, no-console, no-unused-vars, no-explicit-any, etc.) increases technical debt. Since this is marked experimental, consider creating follow-up issues to address violations and re-enable rules incrementally as the code matures.
apps/api-gateway/src/x509/dtos/x509.dto.ts (2)
25-25: Remove commented code.Multiple locations contain commented-out code that should be removed to improve maintainability:
- Line 25: Commented default value
- Line 276: Commented default value
- Lines 295-296: Commented optional fields
Apply this diff:
@ApiProperty({ enum: x5cKeyType, - //default: x5cKeyType.P256.toString(), description: 'Type of the key used for signing the X.509 Certificate (default is p256)' })@ApiProperty({ enum: x5cKeyType, - //default: x5cKeyType.P256.toString(), description: 'Type of the key used for signing the X.509 Certificate (default is p256)' })export class X509CertificateSubjectOptionsDto { @ApiProperty() @IsNotEmpty() @IsString() countryName: string; - // @ApiPropertyOptional() @IsOptional() @IsString() stateOrProvinceName?: string; - // @ApiPropertyOptional() @IsOptional() @IsString() organizationalUnit?: string; @ApiProperty() @IsNotEmpty() @IsString() commonName: string; }Also applies to: 276-276, 295-296
284-291: Use PascalCase for class names.The classes
x509Inputandx509OptionsDtouse camelCase, which is inconsistent with TypeScript conventions and other DTOs in this file (e.g.,NameDto,KeyUsageDto). Class names should use PascalCase.Apply this diff:
-export class x509Input { +export class X509Input { @ApiProperty({ description: 'certificate', required: true }) @IsString() certificate: string; }-export class x509OptionsDto { +export class X509OptionsDto { @ApiProperty({ example: 'exampleOrg' }) @IsNotEmpty() @IsString() commonName: string;Note: Update any imports/usages of these classes in other files.
Also applies to: 324-334
apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (1)
280-283: Tighten typings to reduceanyand improve safetyTwo quick wins:
buildClaimsFromTemplatecurrently returnsRecord<string, any>; you can make this more precise (e.g.Record<string, Claim | Record<string, Claim>>) based onbuildNestedClaimsrather than falling back toany.buildCredentialConfigurationsSupportedtakestemplateRows: anyeven though you already have aTemplateRowPrismashape; usingTemplateRowPrisma[](or the generated Prisma type) will catch mismatches between DB schema and builder logic.These changes would make changes to template/appearance structures safer and satisfy static-analysis concerns without altering behavior.
Also applies to: 378-417
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (1)
242-268: Typecertificateparameter for date‑window validation
validateCredentialDatesInCertificateWindowleavescertificateuntyped:function validateCredentialDatesInCertificateWindow(credentialValidityInfo: validityInfo, certificate) { ... }Since callers always pass an
X509CertificateRecord, giving this parameter that type (or at least{ validFrom: Date; expiry: Date; certificateBase64: string }) will improve readability and catch misuse at compile time with no runtime impact.apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (2)
740-761: Avoid returning HTTP 204 semantics with a non‑empty body from microservice helpers
deleteCredentialOffersbuilds a response where the controller ultimately sends:
statusCode: HttpStatus.NO_CONTENTand- a JSON body containing
data: deletedofferDetails.While the actual HTTP status is set by the controller, having service‑level semantics that imply “no content” but still include a body can cause confusion for upstream code and API docs. Prefer to treat a successful delete as 200/202 with a payload, and reserve 204 for “no response body expected”.
Consider standardizing this at the controller level (see comment in the controller file) and keeping service methods agnostic about specific HTTP codes.
963-1007: Clean up unused locals instoreOidcCredentialWebhookor use them explicitlyInside
storeOidcCredentialWebhookyou compute:
contextCorrelationId,credentialOfferPayload,issuedCredentials,issuanceSessionIdvia destructuring.cfgIds,issuedCredentialsArr.sanitized.But only
CredentialOfferWebhookPayloadandorganisationIdare passed tostoreOidcCredentialDetails. The intermediate derived structures aren’t used, and static analysis already flags some of these assignments as useless.Either:
- Use
cfgIds/issuedCredentialsArr/sanitizedin what you persist, or- Remove the dead code to keep the function focused and quiet static analysis.
Right now this logic is misleading and suggests partial sanitization that doesn’t actually happen.
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (1)
240-268: Template routes and service calls now align; consider adding orgId ParseUUIDPipe for symmetryThe create/update/delete template routes now correctly forward
(CredentialTemplate, user, orgId, issuerId)/(user, orgId, templateId, dto, issuerId)/(user, orgId, templateId, issuerId)to the service, which matches the service signatures and resolves prior mismatches.Optionally, for consistency with other endpoints, you could also apply
ParseUUIDPipetoorgIdandissuerIdon the create route, but that’s a nice-to-have rather than a blocker.Also applies to: 349-381, 392-438
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts(1 hunks)apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts(1 hunks)apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts(1 hunks)apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts(1 hunks)apps/api-gateway/src/x509/dtos/x509.dto.ts(1 hunks)apps/api-gateway/src/x509/x509.service.ts(1 hunks)apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts(1 hunks)apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts(1 hunks)apps/oid4vc-issuance/src/oid4vc-issuance.service.ts(1 hunks)apps/oid4vc-verification/src/oid4vc-verification.service.ts(1 hunks)apps/oid4vc-verification/test/app.e2e-spec.ts(1 hunks)libs/common/src/common.constant.ts(3 hunks)libs/common/src/common.utils.ts(3 hunks)libs/common/src/response-messages/index.ts(1 hunks)libs/logger/src/logging.interceptor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api-gateway/src/x509/x509.service.ts
🧰 Additional context used
🧬 Code graph analysis (9)
libs/logger/src/logging.interceptor.ts (1)
libs/logger/src/logger.interface.ts (2)
LoggerKey(4-4)Logger(6-20)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (11)
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (1)
Controller(58-667)apps/agent-service/src/agent-service.controller.ts (1)
Controller(37-469)apps/oid4vc-verification/src/oid4vc-verification.controller.ts (1)
Controller(9-105)libs/logger/src/logger.interface.ts (1)
Logger(6-20)apps/api-gateway/src/authz/decorators/roles.decorator.ts (1)
Roles(6-6)libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-634)libs/common/src/interfaces/organization.interface.ts (1)
User(26-33)apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier.dto.ts (2)
CreateVerifierDto(22-42)UpdateVerifierDto(44-44)libs/common/src/interfaces/response.interface.ts (2)
IResponse(8-13)IResponseType(1-7)apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (2)
PresentationRequestDto(298-348)VerificationPresentationQueryDto(23-72)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vp-presentation-wh.dto.ts (1)
Oid4vpPresentationWhDto(3-39)
apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (4)
apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts (4)
Claim(34-38)SdJwtTemplate(3-6)MdocTemplate(8-14)CredentialAttribute(40-47)apps/oid4vc-issuance/constant/issuance.ts (1)
batchCredentialIssuanceDefault(6-6)apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts (1)
CreateOidcCredentialOffer(45-49)apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts (1)
IssuerResponse(138-151)
apps/api-gateway/src/x509/dtos/x509.dto.ts (2)
libs/common/src/interfaces/x509.interface.ts (1)
IX509SearchCriteria(217-220)libs/common/src/cast.helper.ts (2)
toNumber(58-76)trim(39-43)
apps/oid4vc-verification/src/oid4vc-verification.service.ts (8)
apps/oid4vc-verification/src/oid4vc-verification.repository.ts (1)
Injectable(12-262)libs/service/base.service.ts (1)
BaseService(2-9)libs/common/src/interfaces/oid4vp-verification.ts (3)
CreateVerifier(6-9)VerifierRecord(15-26)UpdateVerifier(11-13)libs/common/src/common.utils.ts (1)
getAgentUrl(70-140)libs/common/src/interfaces/x509.interface.ts (1)
X509CertificateRecord(203-215)apps/oid4vc-verification/interfaces/oid4vp-verification-sessions.interfaces.ts (2)
RequestSigner(23-23)Oid4vpPresentationWh(3-11)apps/oid4vc-verification/interfaces/oid4vp-verifier.interfaces.ts (1)
VerificationSessionQuery(20-27)libs/common/src/cast.helper.ts (1)
buildUrlWithQuery(527-542)
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (4)
apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts (3)
DisclosureFrame(30-30)GetAllCredentialOffer(51-57)SignerOption(16-20)apps/oid4vc-issuance/interfaces/oid4vc-template.interfaces.ts (3)
CredentialAttribute(40-47)SdJwtTemplate(3-6)MdocTemplate(8-14)libs/common/src/interfaces/x509.interface.ts (1)
X509CertificateRecord(203-215)libs/common/src/date-only.ts (1)
dateToSeconds(52-58)
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (7)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
Controller(52-460)apps/issuance/src/issuance.controller.ts (1)
Controller(26-140)libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-634)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (2)
IssuerCreationDto(84-154)IssuerUpdationDto(156-180)libs/common/src/interfaces/response.interface.ts (2)
IResponse(8-13)IResponseType(1-7)apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (4)
CreateOidcCredentialOfferDto(169-190)UpdateCredentialRequestDto(210-223)GetAllCredentialOfferDto(192-208)CreateCredentialOfferD2ADto(303-389)apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts (1)
OidcIssueCredentialDto(21-65)
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts (3)
apps/api-gateway/src/issuance/issuance.service.ts (1)
Injectable(30-272)apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts (1)
Injectable(10-123)libs/service/base.service.ts (1)
BaseService(2-9)
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (8)
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts (1)
deleteTemplate(56-59)apps/agent-service/src/agent-service.service.ts (1)
deleteOidcIssuer(1439-1452)apps/oid4vc-issuance/src/oid4vc-issuance.repository.ts (3)
addOidcIssuerDetails(191-211)deleteOidcIssuer(232-241)deleteTemplate(346-355)apps/oid4vc-issuance/interfaces/oid4vc-issuance.interfaces.ts (6)
IssuerCreation(54-62)IssuerInitialConfig(64-74)IssuerMetadata(76-82)IssuerUpdation(93-98)IssuerResponse(138-151)IAgentOIDCIssuerCreate(106-106)libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-634)libs/common/src/common.utils.ts (1)
getAgentUrl(70-140)apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (5)
encodeIssuerPublicId(202-207)normalizeJson(192-200)extractTemplateIds(184-190)buildCredentialConfigurationsSupported(378-417)buildIssuerPayload(159-182)apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (3)
CredentialOfferPayload(84-99)buildCredentialOfferPayload(401-484)buildCredentialOfferUrl(140-165)
🪛 GitHub Check: SonarCloud Code Analysis
apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts
[warning] 21-21: "number" is overridden by string in this union type.
[warning] 21-21: "string" is overridden by string in this union type.
[warning] 21-21: "boolean" is overridden by string in this union type.
[warning] 214-215: Remove this commented out code.
[warning] 221-226: Remove this commented out code.
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts
[warning] 395-395: Unexpected negated condition.
[warning] 26-26: Remove this unused import of 'Prisma'.
[failure] 179-183: Unexpected await of a non-Promise (non-"Thenable") value.
[warning] 396-396: Unexpected negated condition.
[failure] 728-728: Unexpected await of a non-Promise (non-"Thenable") value.
[warning] 984-984: Remove this useless assignment to variable "issuanceSessionId".
[failure] 211-211: Unexpected await of a non-Promise (non-"Thenable") value.
[failure] 95-95: Unexpected await of a non-Promise (non-"Thenable") value.
[failure] 336-340: Unexpected await of a non-Promise (non-"Thenable") value.
[warning] 243-251: Remove this commented out code.
[warning] 981-981: Remove this useless assignment to variable "contextCorrelationId".
[failure] 490-490: Unexpected await of a non-Promise (non-"Thenable") value.
🔇 Additional comments (9)
apps/oid4vc-verification/src/oid4vc-verification.service.ts (1)
160-173: LGTM: Proper null safety.The guard at line 164 correctly handles both null and empty array cases from the repository.
libs/logger/src/logging.interceptor.ts (3)
12-17: LGTM: Constructor refactoring improves code clarity.The rename from
_loggertologgerfollows standard TypeScript conventions and improves readability throughout the file.
44-48: Good refactor: Centralized context ID setup improves maintainability.The
setupContextIdmethod consolidates the three storage operations into a single, reusable method. The type signature correctly requires astringparameter, which will be enforced once the string validation is added at line 24.
23-23: Fix the fallback reference to avoid potential ReferenceError.The fallback
getHeadersreferences an undefined identifier. IfrpcContext.getHeaders?.()is nullish, the code will try to access a standalonegetHeadersvariable that doesn't exist.Apply this diff to fix the fallback:
- const headers = rpcContext.getHeaders?.() ?? rpcContext.getHeaders; + const headers = rpcContext.getHeaders?.() ?? rpcContext.getHeaders ?? {};This ensures you always have a valid object to work with, even if neither the method call nor the property exists.
Likely an incorrect or invalid review comment.
apps/api-gateway/src/x509/dtos/x509.dto.ts (2)
336-382: Well-structured search criteria DTO.The
X509SearchCriteriaDtoproperly implements pagination, filtering, and sorting with appropriate transformations and validation:
- Query parameters are transformed using
toNumberandtrimhelpers- Page size is bounded between 1 and 100
- Enum validations ensure type safety
- Default values provide sensible fallbacks
234-244: Union type requires explicit string validation handling.The
issuerandsubjectproperties accept bothX509CertificateIssuerAndSubjectOptionsDtoandstringtypes (DN format). The@ValidateNested()decorator only validates object instances, leaving string values completely unvalidated. While the interface intentionally supports both formats (as shown in the documentation), the DTO should either:
- Add a custom validator to validate string DN format (e.g., with a regex or dedicated
@IsDN()decorator)- Or use a discriminated union approach with separate validation paths for each type
Currently, malformed DN strings could pass through validation uncaught.
libs/common/src/common.utils.ts (1)
142-146: LGTM! Safe default for experimental features.The function correctly gates experimental OIDC modules with a safe default (hidden when env var is unset or 'true'). The inverted logic (
!hide) clearly expresses "should load when NOT hidden."apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
421-459: Webhook handler aligns with existing issuance pattern and looks soundThe
storePresentationWebhookflow matches the issuance webhook pattern:
- Normalizes the payload (
type = 'Oid4vpPresentation', conditionally setsorgIdwhencontextCorrelationId === 'default').- Delegates persistence/processing to
oid4vcVerificationService.oid4vpPresentationWebhook.- Returns an
IResponseTypewithstatusCode: HttpStatus.CREATEDandResponseMessages.oid4vpSession.success.webhookReceived.- Optionally posts back to a configured webhook URL via
_getWebhookUrl/_postWebhookResponse, logging but not throwing on webhook errors.This is coherent and should behave well, assuming the service methods are implemented safely.
apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.service.ts (1)
17-170: Gateway service wrappers look consistent with microservice APIThe NATS payloads and subject names for issuers, templates, credential offers, and webhooks are assembled consistently and match the described microservice contracts. I don’t see correctness or safety issues here.
| export class NameListDto { | ||
| @ApiProperty({ type: [NameDto] }) | ||
| @ArrayNotEmpty() | ||
| @IsArray() | ||
| @ValidateNested() | ||
| @Type(() => NameDto) | ||
| name: NameDto[]; | ||
|
|
||
| @ApiPropertyOptional() | ||
| @IsOptional() | ||
| @IsBoolean() | ||
| markAsCritical?: boolean; | ||
| } |
There was a problem hiding this comment.
Add { each: true } to ValidateNested for array validation.
The name property is an array of NameDto objects, but @ValidateNested() is missing the { each: true } option. Without this, the validator won't validate each array element properly, potentially allowing invalid data.
Apply this diff:
export class NameListDto {
@ApiProperty({ type: [NameDto] })
@ArrayNotEmpty()
@IsArray()
- @ValidateNested()
+ @ValidateNested({ each: true })
@Type(() => NameDto)
name: NameDto[];📝 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.
| export class NameListDto { | |
| @ApiProperty({ type: [NameDto] }) | |
| @ArrayNotEmpty() | |
| @IsArray() | |
| @ValidateNested() | |
| @Type(() => NameDto) | |
| name: NameDto[]; | |
| @ApiPropertyOptional() | |
| @IsOptional() | |
| @IsBoolean() | |
| markAsCritical?: boolean; | |
| } | |
| export class NameListDto { | |
| @ApiProperty({ type: [NameDto] }) | |
| @ArrayNotEmpty() | |
| @IsArray() | |
| @ValidateNested({ each: true }) | |
| @Type(() => NameDto) | |
| name: NameDto[]; | |
| @ApiPropertyOptional() | |
| @IsOptional() | |
| @IsBoolean() | |
| markAsCritical?: boolean; | |
| } |
🤖 Prompt for AI Agents
In apps/api-gateway/src/x509/dtos/x509.dto.ts around lines 103 to 115, the
NameListDto's name property uses @ValidateNested() but lacks the { each: true }
option so nested validation won't run for each array element; update the
decorator to @ValidateNested({ each: true }) (keeping @Type(() => NameDto) and
other decorators) so each NameDto in the name array is validated.
| function buildSdJwtCredential( | ||
| credentialRequest: CredentialRequestDtoLike, | ||
| templateRecord: any, | ||
| signerOptions: SignerOption[], | ||
| activeCertificateDetails?: X509CertificateRecord[] | ||
| ): BuiltCredential { | ||
| // For SD-JWT format we expect payload to be a flat map of claims (no namespaces) | ||
| let payloadCopy = { ...(credentialRequest.payload as Record<string, unknown>) }; | ||
|
|
||
| // // strip vct if present per requirement | ||
| // delete payloadCopy.vct; | ||
|
|
||
| const templateSignerOption: SignerOption = signerOptions.find( | ||
| (x) => templateRecord.signerOption.toLowerCase() === x.method | ||
| ); | ||
| if (!templateSignerOption) { | ||
| throw new UnprocessableEntityException( | ||
| `Signer option "${templateRecord.signerOption}" is not configured for template ${templateRecord.id}` | ||
| ); | ||
| } | ||
|
|
||
| if (templateRecord.signerOption === SignerMethodOption.X5C && credentialRequest.validityInfo) { | ||
| if (!activeCertificateDetails?.length) { | ||
| throw new UnprocessableEntityException('Active x.509 certificate details are required for x5c signer templates.'); | ||
| } | ||
| const certificateDetail = activeCertificateDetails.find( | ||
| (x) => x.certificateBase64 === templateSignerOption.x5c?.[0] | ||
| ); | ||
| if (!certificateDetail) { | ||
| throw new UnprocessableEntityException('No active x.509 certificate matches the configured signer option.'); | ||
| } | ||
|
|
||
| const validationResult = validateCredentialDatesInCertificateWindow( | ||
| credentialRequest.validityInfo, | ||
| certificateDetail | ||
| ); |
There was a problem hiding this comment.
Fix signer mapping for X.509 SD‑JWT credentials (buildSdJwtCredential)
Here:
const templateSignerOption: SignerOption = signerOptions.find(
(x) => templateRecord.signerOption.toLowerCase() === x.method
);signerOptions[].method is 'did' | 'x5c' (from SignerMethodOption), but templateRecord.signerOption is the Prisma enum (DID, X509_P256, X509_ED25519, …). For X.509 templates, .toLowerCase() yields "x509_p256" / "x509_ed25519", which never matches "x5c", so you always hit the “Signer option not configured” path even when you’ve correctly populated signerOptions in the service.
Consider normalizing the DB value into a SignerMethodOption first, then matching on that, e.g.:
const rawSigner = (templateRecord.signerOption ?? '').toString().toLowerCase();
if (!rawSigner) {
throw new UnprocessableEntityException('Template signerOption is not configured');
}
const expectedMethod =
rawSigner === 'did' ? SignerMethodOption.DID : SignerMethodOption.X5C;
const templateSignerOption = signerOptions?.find((x) => x.method === expectedMethod);
if (!templateSignerOption) {
throw new UnprocessableEntityException(
`Signer option "${templateRecord.signerOption}" is not configured for template ${templateRecord.id}`
);
}This keeps DID templates working and lets X.509 templates correctly resolve their signer and certificate.
🤖 Prompt for AI Agents
In apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts around lines
270-305, the code compares templateRecord.signerOption.toLowerCase() directly to
signerOptions[].method causing X.509 enum values like "x509_p256" /
"x509_ed25519" to never match "x5c" and throw an incorrect
UnprocessableEntityException; normalize the DB enum to the expected
SignerMethodOption first (map any X.509 variants to SignerMethodOption.X5C and
DID to SignerMethodOption.DID), then find the templateSignerOption by matching
signerOptions[].method to that normalized value and keep the existing error
handling if no match is found.
| function buildMdocCredential( | ||
| credentialRequest: CredentialRequestDtoLike, | ||
| templateRecord: any, | ||
| signerOptions: SignerOption[], | ||
| activeCertificateDetails: X509CertificateRecord[] | ||
| ): BuiltCredential { | ||
| let incomingPayload = { ...(credentialRequest.payload as Record<string, unknown>) }; | ||
|
|
||
| if ( | ||
| !credentialRequest.validityInfo || | ||
| !credentialRequest.validityInfo.validFrom || | ||
| !credentialRequest.validityInfo.validUntil | ||
| ) { | ||
| throw new UnprocessableEntityException(`${ResponseMessages.oidcIssuerSession.error.missingValidityInfo}`); | ||
| } | ||
|
|
||
| if (!signerOptions?.length || !signerOptions[0].x5c?.length) { | ||
| throw new UnprocessableEntityException('An x5c signer configuration is required for mdoc credentials.'); | ||
| } | ||
| if (!activeCertificateDetails?.length) { | ||
| throw new UnprocessableEntityException('Active x.509 certificate details are required for mdoc credentials.'); | ||
| } | ||
| const certificateDetail = activeCertificateDetails.find((x) => x.certificateBase64 === signerOptions[0].x5c[0]); | ||
| if (!certificateDetail) { | ||
| throw new UnprocessableEntityException('No active x.509 certificate matches the configured signer option.'); | ||
| } | ||
| const validationResult = validateCredentialDatesInCertificateWindow( | ||
| credentialRequest.validityInfo, | ||
| certificateDetail | ||
| ); | ||
|
|
||
| if (!validationResult.isValid) { | ||
| throw new UnprocessableEntityException(`${JSON.stringify(validationResult.details)}`); | ||
| } | ||
| incomingPayload = { | ||
| ...incomingPayload, | ||
| validityInfo: credentialRequest.validityInfo | ||
| }; | ||
|
|
||
| const apiFormat = mapDbFormatToApiFormat(templateRecord.format); | ||
| const idSuffix = formatSuffix(apiFormat); | ||
| const credentialSupportedId = `${templateRecord.name}-${idSuffix}`; | ||
|
|
||
| return { | ||
| credentialSupportedId, | ||
| signerOptions: signerOptions ? signerOptions[0] : undefined, | ||
| format: apiFormat, | ||
| payload: incomingPayload | ||
| }; | ||
| } |
There was a problem hiding this comment.
Avoid hard‑wiring signerOptions[0] for mdoc credentials
buildMdocCredential currently assumes:
if (!signerOptions?.length || !signerOptions[0].x5c?.length) { ... }
...
const certificateDetail = activeCertificateDetails.find(
(x) => x.certificateBase64 === signerOptions[0].x5c[0]
);
...
return {
credentialSupportedId,
signerOptions: signerOptions ? signerOptions[0] : undefined,
// ...
};For multi‑template offers or mixed signer configurations, this couples every mdoc credential to the first signer option and certificate, regardless of which template it belongs to.
To keep behavior predictable:
- Either resolve a per‑template signer (similar to the SD‑JWT path) and pass a single
ResolvedSignerOptionintobuildMdocCredential, or - At minimum, derive the correct signer based on
templateRecord.signerOptioninstead of always index0.
Right now, issuing multiple mdoc credentials with different signers will produce incorrect bindings.
🤖 Prompt for AI Agents
In apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts around lines
350–399, the function hard-wires signerOptions[0] which binds every mdoc to the
first signer; instead, select the signer that belongs to the current template
(e.g. resolve signer by templateRecord.signerOption identifier or thumbprint),
validate that this resolvedSigner exists and has x5c, pick the matching
activeCertificateDetails entry using resolvedSigner.x5c[0], run the same
certificate/date validations against that resolved signer/certificate, and
return that resolvedSigner (not signerOptions[0]) in the BuiltCredential; if
templateRecord.signerOption is missing, fall back to a clear error or existing
behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
libs/common/src/response-messages/index.ts (2)
523-543: Avoid literal${templateId}placeholder in success message.
oidcTemplate.success.deleteTemplateuses a single-quoted string with${templateId}, so the placeholder is rendered literally and never interpolated. That’s inconsistent witherror.failedDeleteTemplate, which expects the caller to append the ID.Suggested change (keep callers appending the ID, like
failedDeleteTemplate):getById: 'OID4VC template details fetched successfully.', - deleteTemplate: '[createTemplate] compensating delete succeeded for templateId=${templateId}' + deleteTemplate: '[createTemplate] compensating delete succeeded for templateId='
587-595: Session error string still references "template" instead of session.In the OID4VP session block,
error.deleteTemplatementions "template", which is misleading in a verifier-session context.Minimal fix to correct the user-facing wording while keeping the key name stable:
deleteFailed: 'Failed to delete OID4VP session verifier.', - deleteTemplate: 'Error while deleting template.', + deleteTemplate: 'Error while deleting verification session.', responseNotFound: 'Verification session response not found.'Optionally, you may also rename the key to
deleteSessionin a follow-up refactor and update usages to better reflect the domain.apps/oid4vc-verification/src/oid4vc-verification.service.ts (6)
64-78: Fix undefined dereference in conflict handling.When the agent responds with 409 before
createdVerifierDetailsis set, line 75 attempts to access.verifierIdonundefined, throwing aTypeErrorand leaking a 500 error instead of the intendedConflictException. Use the already-knownverifierIdfrom line 47.} catch (error) { const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${createdVerifierDetails.verifierId}' already exists`); + throw new ConflictException(`Verifier with id '${verifierId}' already exists`); } throw error; }
108-139: Fix inverted guard and undefined dereference.Line 109's condition
0 > existingVerifiers.lengthis logically inverted—it checks whether 0 is greater than the length, which is always false. This allows the code to proceed whenexistingVerifiersis null or empty, causing a crash at line 121 when accessingexistingVerifiers[0].Additionally, line 137 dereferences
updatedVerifierDetails.verifierIdwhen it may beundefinedif the agent rejects with 409 before setting a response body.let updatedVerifierDetails; const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); - if (0 > existingVerifiers.length) { + if (!existingVerifiers || existingVerifiers.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } @@ } catch (error) { // We'll not need this const status409 = 409 === error?.status?.message?.statusCode || 409 === error?.response?.status || 409 === error?.statusCode; if (status409) { - throw new ConflictException(`Verifier with id '${updatedVerifierDetails.verifierId}' already exists`); + const conflictId = existingVerifiers?.[0]?.publicVerifierId ?? verifierId; + throw new ConflictException(`Verifier with id '${conflictId}' already exists`); } throw error; }
175-205: Add null guard and use agent-facing ID in delete.Line 179 checks only
checkIdExist.length === 0but doesn't guard againstnull, whichgetVerifiersByVerifierIdcan return. This causes a crash when the repository returnsnull.Line 188 passes the internal
verifierIdto the agent, but lines 60 (create) and 121 (update) correctly usepublicVerifierIdfor agent communication. The delete call should be consistent.const checkIdExist = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); - if (0 == checkIdExist.length) { + if (!checkIdExist || checkIdExist.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } const agentDetails = await this.oid4vpRepository.getAgentEndPoint(orgId); if (!agentDetails) { throw new NotFoundException(ResponseMessages.issuance.error.agentEndPointNotFound); } const { agentEndPoint, id } = agentDetails; - const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_VERIFIER_DELETE, checkIdExist[0].verifierId); + const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_VERIFIER_DELETE, checkIdExist[0].publicVerifierId); this.logger.debug(`[deleteVerifierById] calling agent URL=${url}`);
234-249: Fix typo preventing X.509 certificate resolution.Line 241 passes
sessionRequest.requestSigner.methodv(undefined) togetCurrentActiveCertificate. The correct property ismethod(without the trailingv). This typo breaks all X.509-based verification flows with the error message at line 247.this.logger.debug('X5C based request signer method selected'); const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( orgId, - sessionRequest.requestSigner.methodv + sessionRequest.requestSigner.method ); this.logger.debug(`activeCertificate=${JSON.stringify(activeCertificate)}`);
304-304: Remove duplicate await.Line 304 has
await await, which is redundant. The inner await already returns the resolved promise.this.logger.debug(`[getVerifierSession] calling agent URL=${url}`); - const verifiers = await await this._getOid4vpVerifierSession(url, orgId); + const verifiers = await this._getOid4vpVerifierSession(url, orgId); if (!verifiers || 0 === verifiers.length) {
331-331: Remove duplicate await.Line 331 has
await await, which is redundant.); - const verifiers = await await this._getOid4vpVerifierSession(url, orgId); + const verifiers = await this._getOid4vpVerifierSession(url, orgId); if (!verifiers || 0 === verifiers.length) {
🧹 Nitpick comments (1)
libs/common/src/response-messages/index.ts (1)
612-613: Polish x509 collision error message grammar.The
collisionForActivatingX5cmessage has several grammatical issues (“In order to active this you need to Inactivate…”). A clearer version will read better in UI/logs.Proposed rewording:
- collisionForActivatingX5c: - 'Certificate date range collides with existing certificates for this organization, In order to active this you need to Inactivate the previous one.', + collisionForActivatingX5c: + 'Certificate date range collides with existing certificates for this organization. To activate this certificate, you must first deactivate the previous one.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/oid4vc-verification/src/oid4vc-verification.service.ts(1 hunks)libs/common/src/response-messages/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/oid4vc-verification/src/oid4vc-verification.service.ts (5)
libs/service/base.service.ts (1)
BaseService(2-9)libs/common/src/common.utils.ts (1)
getAgentUrl(70-140)libs/common/src/interfaces/x509.interface.ts (1)
X509CertificateRecord(203-215)apps/oid4vc-verification/interfaces/oid4vp-verification-sessions.interfaces.ts (2)
RequestSigner(23-23)Oid4vpPresentationWh(3-11)apps/oid4vc-verification/interfaces/oid4vp-verifier.interfaces.ts (1)
VerificationSessionQuery(20-27)
| /* eslint-disable quotes */ | ||
| /* eslint-disable no-useless-catch */ | ||
| /* eslint-disable camelcase */ | ||
| /* eslint-disable no-console */ | ||
| /* eslint-disable @typescript-eslint/no-unused-vars */ | ||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| /* eslint-disable @typescript-eslint/naming-convention, @typescript-eslint/explicit-function-return-type, @typescript-eslint/explicit-module-boundary-types, camelcase */ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excessive ESLint disables mask code quality issues.
Seven disable directives suggest the code violates project standards. no-useless-catch indicates redundant try-catch blocks, no-console flags improper logging (confirmed at lines 264, 267), and no-explicit-any compromises type safety throughout.
Refactor to eliminate these violations rather than suppressing warnings.
🤖 Prompt for AI Agents
In apps/oid4vc-verification/src/oid4vc-verification.service.ts lines 1-8, the
file-wide ESLint disable comments mask real issues; remove these broad disables
and fix the underlying problems: eliminate redundant try/catch blocks flagged by
no-useless-catch (or rethrow with context), replace console.* uses at lines ~264
and ~267 with the project's logger, and remove any use of the implicit any type
by adding precise TypeScript types or generics; update naming to follow
conventions and add minimal targeted eslint overrides only where a justified
exception remains.
| console.log(`[oid4vpCreateVerificationSession] sessionRequest=${JSON.stringify(sessionRequest)}`); | ||
|
|
||
| const url = await getAgentUrl(agentEndPoint, CommonConstants.OID4VP_VERIFICATION_SESSION); | ||
| console.log(`[oid4vpCreateVerificationSession] calling agent URL=${url}`); |
There was a problem hiding this comment.
Replace console.log with logger.
Lines 264 and 267 use console.log instead of this.logger.debug, which is the pattern used consistently throughout the service (lines 44, 208, 237, etc.). This breaks observability and violates the no-console ESLint rule disabled at line 4.
- console.log(`[oid4vpCreateVerificationSession] sessionRequest=${JSON.stringify(sessionRequest)}`);
+ this.logger.debug(`[oid4vpCreateVerificationSession] sessionRequest=${JSON.stringify(sessionRequest)}`);
const url = await getAgentUrl(agentEndPoint, CommonConstants.OID4VP_VERIFICATION_SESSION);
- console.log(`[oid4vpCreateVerificationSession] calling agent URL=${url}`);
+ this.logger.debug(`[oid4vpCreateVerificationSession] calling agent URL=${url}`);🤖 Prompt for AI Agents
In apps/oid4vc-verification/src/oid4vc-verification.service.ts around lines 264
to 267, replace the two console.log calls with this.logger.debug calls to match
the service's logging pattern: log the sessionRequest JSON using
this.logger.debug(`[oid4vpCreateVerificationSession]
sessionRequest=${JSON.stringify(sessionRequest)}`) and the agent URL call using
this.logger.debug(`[oid4vpCreateVerificationSession] calling agent URL=${url}`);
remove the console.log statements and ensure imports/members already provide
this.logger (no other behavior changes).
Signed-off-by: Tipu_Singh <[email protected]>
* feat: oidc issunace Signed-off-by: Tipu_Singh <[email protected]> * feat:create seperate microservice for oid4vc Signed-off-by: Tipu_Singh <[email protected]> * feat:removed logs Signed-off-by: Tipu_Singh <[email protected]> * refactor: removed duplicate code Signed-off-by: Tipu_Singh <[email protected]> * feat: create credential offer API Signed-off-by: Tipu_Singh <[email protected]> * fix: missing credential details repository logic Signed-off-by: Tipu_Singh <[email protected]> * feat: added docker file for oid4vc-issuance Signed-off-by: Tipu_Singh <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
* refactor: update oid4vc flows Signed-off-by: Tipu_Singh <[email protected]> * refactor: removed commented code Signed-off-by: Tipu_Singh <[email protected]> * refactor: changed example Signed-off-by: Tipu_Singh <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
* added x509 cert for mdoc Signed-off-by: Rinkal Bhojani <[email protected]> * removed commented code Signed-off-by: Rinkal Bhojani <[email protected]> * removed commented code Signed-off-by: Rinkal Bhojani <[email protected]> --------- Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
…le, fixed other values Signed-off-by: Rinkal Bhojani <[email protected]>
* feat:oid4vc verifications Signed-off-by: Tipu_Singh <[email protected]> * feat: CRU for verifier Signed-off-by: Krishna Waske <[email protected]> * fix: get verifier api Signed-off-by: Krishna Waske <[email protected]> * feat: add delete verifier Signed-off-by: Krishna Waske <[email protected]> * fix: delete verifier reselient Signed-off-by: Krishna Waske <[email protected]> * feat: oid4vc verification verifier session (#1502) * fix: verification session Signed-off-by: Krishna Waske <[email protected]> * feat: add verifier session response by id Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Krishna Waske <[email protected]> * feat: verifier session post API (#1503) * feat: verifier session post API Signed-off-by: Tipu_Singh <[email protected]> * fix: delete oid4vc verifier Signed-off-by: Krishna Waske <[email protected]> * fix: change DTO example Signed-off-by: Krishna Waske <[email protected]> * fix: change CREDEBL to example Signed-off-by: Krishna Waske <[email protected]> * fix: session to presentation Signed-off-by: Krishna Waske <[email protected]> * feat: added docker file for verification Signed-off-by: Tipu_Singh <[email protected]> * fix: remove unnecessary column for oid4vp_verifier table Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]> Signed-off-by: Krishna Waske <[email protected]> Co-authored-by: Krishna Waske <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]> Signed-off-by: Krishna Waske <[email protected]> Co-authored-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
* feat: debug logs init Signed-off-by: Krishna Waske <[email protected]> * fix: add missing debug logs Signed-off-by: Krishna Waske <[email protected]> * fix: exend base service for oid4vp Signed-off-by: Krishna Waske <[email protected]> * fix: nested response Signed-off-by: Krishna Waske <[email protected]> * fix: unwanted description in logs Signed-off-by: Krishna Waske <[email protected]> * fix: remove commented code Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Krishna Waske <[email protected]>
* feat: verification webhook implementation Signed-off-by: Tipu_Singh <[email protected]> * fix: batch size and nats config Signed-off-by: Tipu_Singh <[email protected]> * fix: review comment Signed-off-by: Tipu_Singh <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
…ication request Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
* feat: trace contextId across services, including passed from headers of http request Signed-off-by: Krishna Waske <[email protected]> * fix: import issues caused by merge Signed-off-by: Krishna Waske <[email protected]> * fix: contextId refactoring and coderabbit resolve Signed-off-by: Krishna Waske <[email protected]> * fix: make contextInterceptorModule resilient Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Krishna Waske <[email protected]>
* feat: add optionally enable disable oidc module Signed-off-by: Krishna Waske <[email protected]> * chore: add some descriptive logs Signed-off-by: Krishna Waske <[email protected]> * fix: update the registration of experimental modules Signed-off-by: Krishna Waske <[email protected]> * fix: common function from utils Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Krishna Waske <[email protected]>
* fix: nats call from nats client for verification service Signed-off-by: Krishna Waske <[email protected]> * fix: ledger nats fix in agent-service Signed-off-by: Krishna Waske <[email protected]> * fix: remove unused variable Signed-off-by: Krishna Waske <[email protected]> * fix: nats calls in connection service Signed-off-by: Krishna Waske <[email protected]> * fix: webhook service unwanted imports Signed-off-by: Krishna Waske <[email protected]> * chore: add todos to remove nested mapping Signed-off-by: Krishna Waske <[email protected]> * fix: sonar cube issues Signed-off-by: Krishna Waske <[email protected]> * fix: coderabbit suggestions Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Rinkal Bhojani <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
tipusinghaw
force-pushed
the
feat/oidc-main-sync
branch
from
c8781a8 to
e9477f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agent-service/src/agent-service.service.ts (1)
501-585: Fix asyncfs.unlinkthrow and improve new ledger validation error causesTwo issues in this block:
- Throwing from async
fs.unlinkcallback can crash the processInside the
fs.unlinkcallback you do:fs.unlink(filePath, (err) => { if (err) { this.logger.error(`Error removing file: ${err.message}`); throw new InternalServerErrorException(err.message); } ... });Because this throw happens in an async callback, it will not be caught by the surrounding
try/catchand can terminate the Node process if file removal fails (e.g., file missing or permission error).Refactor to an awaited promise-based call so errors stay within the
try/catch:- if (agentDetails?.agentToken) { - fs.unlink(filePath, (err) => { - if (err) { - this.logger.error(`Error removing file: ${err.message}`); - throw new InternalServerErrorException(err.message); - } else { - this.logger.log(`File ${filePath} has been removed successfully`); - } - }); - } + if (agentDetails?.agentToken) { + try { + await fs.promises.unlink(filePath); + this.logger.log(`File ${filePath} has been removed successfully`); + } catch (err) { + this.logger.error(`Error removing file: ${(err as Error).message}`); + // Decide whether to fail the whole spin-up or just log the issue; + // if non-fatal, drop the throw here. + } + }
new Error()without message in the new ledger checksWhen
ledgerListis falsy you throw:throw new BadRequestException(ResponseMessages.agent.error.invalidLedger, { cause: new Error(), description: ResponseMessages.errorMessages.notFound });Using a bare
new Error()loses context and is what Sonar is flagging. Prefer passing the same message you surface or omittingcauseentirely:- if (!ledgerList) { - throw new BadRequestException(ResponseMessages.agent.error.invalidLedger, { - cause: new Error(), - description: ResponseMessages.errorMessages.notFound - }); - } + if (!ledgerList) { + throw new BadRequestException(ResponseMessages.agent.error.invalidLedger, { + cause: new Error(ResponseMessages.agent.error.invalidLedger), + description: ResponseMessages.errorMessages.notFound + }); + }The rest of the spin-up flow (ledger existence check against
_getALlLedgerDetails, base wallet token derivation, encrypted API key, and agent details persistence) looks coherent.
♻️ Duplicate comments (68)
.env.sample (1)
126-127: Fix incorrect comments and add quotes to NKEY seed values.The comments for lines 126-127 both incorrectly reference "x509 service" when they should specify the correct services. Additionally, the placeholder values lack quotes which is inconsistent with the previous review feedback and dotenv best practices.
Apply this diff to fix the comment accuracy and value quoting:
-OIDC4VC_ISSUANCE_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for x509 service -OIDC4VC_VERIFICATION_NKEY_SEED=xxxxxxxxxxx // Please provide Nkeys secret for x509 service +OIDC4VC_ISSUANCE_NKEY_SEED="xxxxxxxxxxx" // Please provide Nkeys secret for OIDC4VC issuance service +OIDC4VC_VERIFICATION_NKEY_SEED="xxxxxxxxxxx" // Please provide Nkeys secret for OIDC4VC verification servicelibs/prisma-service/prisma/data/credebl-master-table.json (1)
140-168: Misaligned indentation in ledgerConfig persists from prior review.The closing braces for the
detailsobject at lines 150, 159, and 166 remain at 4-space indentation instead of 6 spaces (matching the opening properties). This was flagged in the prior review; however, JSON remains syntactically valid since parsers ignore whitespace outside strings.Per your previous decision to defer this, no action is required. If you decide to address formatting consistency in a future cleanup, a JSON formatter can correct the indentation automatically.
apps/api-gateway/src/helper-files/file-operation.helper.ts (2)
16-20: Hoist promisifiedreadFileto module scope.Promisifying inside the function creates a new promisified version on every call, which is inefficient. Move it to module scope like
writeFileandmkdir.Apply this diff:
const writeFile = promisify(fs.writeFile); const mkdir = promisify(fs.mkdir); +const readFile = promisify(fs.readFile); export const getFile = async (path: string, encoding: BufferEncoding): Promise<string | Buffer> => { - const readFile = promisify(fs.readFile); - return encoding ? readFile(path, { encoding }) : readFile(path, {}); };
22-26: Hoist promisifiedunlinkto module scope.Promisifying inside the function creates overhead on every call. Move it to module scope for consistency with
writeFileandmkdir.Apply this diff:
const writeFile = promisify(fs.writeFile); const mkdir = promisify(fs.mkdir); +const unlink = promisify(fs.unlink); export const deleteFile = async (path: string): Promise<void> => { - const unlink = promisify(fs.unlink); - return unlink(path); };apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (1)
298-348: DecouplerequestSignerfrom PrismaSignerOptionand simplify_oneOfCheckSwagger metadataTwo related points here:
DTO coupling to Prisma and
SignerOptionname collisionimport { SignerOption } from '@prisma/client'; ... @IsOptional() requestSigner?: { method: SignerOption; };
- This DTO is importing
SignerOptionfrom@prisma/client, which couples your public API contract directly to the persistence layer.- There is also a separate
SignerOptioninterface inapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tswith a different shape ({ method: SignerMethodOption; did?: string; x5c?: string[]; }), so the same name now refers to different things in different layers, which is confusing and error‑prone.Recommended direction:
Introduce or reuse a domain‑level enum/interface (e.g.,
SignerMethodOption+ aRequestSignerDto) from a shared interfaces module, and stop importingSignerOptionfrom@prisma/clientin this DTO.Model
requestSigneras a proper DTO with nested validation, e.g.:// from a shared domain module, not @prisma/client export enum SignerMethodOption { DID = 'DID', X5C = 'X5C', // ... } export class RequestSignerDto { @ApiProperty({ enum: SignerMethodOption, description: 'Signer method' }) @IsEnum(SignerMethodOption) method: SignerMethodOption; @ApiPropertyOptional({ description: 'DID to use for signing' }) @IsOptional() @IsString() did?: string; @ApiPropertyOptional({ type: [String], description: 'X.509 certificate chain (PEM)' }) @IsOptional() @IsArray() @IsString({ each: true }) x5c?: string[]; } // In PresentationRequestDto: @ApiPropertyOptional({ type: RequestSignerDto, description: 'Signer option type' }) @IsOptional() @ValidateNested() @Type(() => RequestSignerDto) requestSigner?: RequestSignerDto;This avoids Prisma leakage, removes the naming collision, and gives you proper validation on the nested object.
Make
_oneOfCheckpurely internal to Swagger@ApiHideProperty() @ApiPropertyOptional({ description: 'Internal: ensures exactly one of dcql or presentationExchange is present' }) @Validate(OnlyOneOfConstraint, ['dcql', 'presentationExchange']) _oneOfCheck?: any;Once
@ApiHideProperty()is present, the extra@ApiPropertyOptionalis redundant and may still confuse tooling. You can safely drop@ApiPropertyOptionaland keep it as a hidden internal validation hook:@ApiHideProperty() @Validate(OnlyOneOfConstraint, ['dcql', 'presentationExchange']) _oneOfCheck?: any;apps/webhook/src/webhook.service.ts (2)
28-53: Fix mismatch between declared return type and actual value inregisterWebhook
registerWebhookis declared to returnPromise<ICreateWebhookUrl>but currently returnswebhookUrl.webhookUrl, which is just a string. This violates the declared interface and will either fail type-checking or surprise callers expecting an object.Either change the return type to
Promise<string>or, more in line with the interface, return an object that matchesICreateWebhookUrl. For example:- if (!webhookUrl) { - throw new InternalServerErrorException(ResponseMessages.webhook.error.registerWebhook); - } else { - return webhookUrl.webhookUrl; - } + if (!webhookUrl) { + throw new InternalServerErrorException(ResponseMessages.webhook.error.registerWebhook); + } + + return { webhookUrl: webhookUrl.webhookUrl };
56-69: AligngetWebhookUrlreturn type with implementation
getWebhookUrlis declared to returnPromise<IGetWebhookUrl>but actually returnswebhookUrlInfo.webhookUrl(a string). Same issue as above: the implementation does not satisfy the declared interface and breaks the contract for callers.Either change the return type to
Promise<string>or return an object compatible withIGetWebhookUrl. For example:- if (!webhookUrlInfo) { - throw new NotFoundException(ResponseMessages.webhook.error.notFound); - } - - return webhookUrlInfo.webhookUrl; + if (!webhookUrlInfo) { + throw new NotFoundException(ResponseMessages.webhook.error.notFound); + } + + return { webhookUrl: webhookUrlInfo.webhookUrl };apps/agent-service/src/agent-service.service.ts (1)
800-807: Keep ledger validation consistent and giveErrora message here as wellThe new shared-agent tenant path mirrors the ledger validation you added in
_agentSpinup, which is good for consistency. As there, you construct aBadRequestExceptionusingcause: new Error()whenledgerListis falsy.Apply the same improvement here (pass a descriptive message into
Erroror drop thecausefield) so Sonar stops flagging it and logs contain meaningful context.libs/common/src/nats.config.ts (1)
17-18: NATS queue group removal issue already flagged.The queue assignment remains commented out, which was previously identified as a critical behavior change affecting 30+ microservices. The trailing comma after
reconnectTimeWaitis also still missing.libs/common/src/common.utils.ts (2)
70-70: Breaking signature change from async to sync already flagged.The
getAgentUrlsignature change fromasyncto sync was previously identified with 30+ call sites usingawait. The author acknowledged this will be addressed later.
108-108: Incorrect URL mapping for OIDC_DELETE_CREDENTIAL_OFFER already flagged.This mapping issue was previously identified -
OIDC_DELETE_CREDENTIAL_OFFERshould map toURL_OIDC_ISSUER_SESSIONS_GET(with#placeholder) instead ofURL_OIDC_ISSUER_SESSIONS_GET_ALL. The author acknowledged this will be improved later.libs/common/src/cast.helper.ts (1)
527-542: Logic error and ESLint issues already flagged.The flawed early-return condition on line 530 (
queryParams?.length >= 0) and associated ESLint issues (missing semicolon, Yoda conditions,anytype) were previously identified with a suggested fix usingObject.keys(queryParams).length === 0.apps/oid4vc-verification/src/oid4vc-verification.repository.ts (7)
42-57: Missing null check forfindFirstresult already flagged.The destructuring of
{ agent }on line 45 without null-checking thefindFirstresult was previously identified as a major issue that will throw if no record is found.
59-75: Return type and misleading error message already flagged.Both issues were previously identified: the return type should be
Promise<org_agents | null>, and the error message incorrectly references "issuance repository" instead of "verification repository".
101-120: Incorrect error message already flagged; add type annotations.The error message referencing "createOid4vpVerifier" was previously identified. Additionally, add type annotations similar to the
createOid4vpVerifiermethod:- async updateOid4vpVerifier(verifierDetails, userId: string, verifierId: string): Promise<object> { + async updateOid4vpVerifier( + verifierDetails: { clientMetadata: object }, + userId: string, + verifierId: string + ): Promise<oid4vp_verifier> {
138-155: Incorrect error message already flagged; return type includes unnecessary| null.The error message referencing "getVerifiersByPublicVerifierId" was previously identified. Additionally,
findManynever returnsnull:- async getVerifiersByVerifierId(orgId: string, verifierId?: string): Promise<oid4vp_verifier[] | null> { + async getVerifiersByVerifierId(orgId: string, verifierId?: string): Promise<oid4vp_verifier[]> {
157-174: PrismafindUniqueusage and incorrect error message already flagged.Both issues were previously identified:
findUniquedoesn't support nested where conditions (usefindFirst), and the error message incorrectly references "getVerifiersByPublicVerifierId".
176-193: Prismadeletewith nested where condition already flagged.The issue with using nested conditions in
deletewas previously identified. The suggested alternatives (usedeleteMany, verify at service layer, or two-step approach) were discussed, and the author expressed preference to avoid extra DB calls.
234-261: Return type should allownull- already flagged.The return type issue was previously identified:
findFirstcan returnnull, so the signature should bePromise<X509CertificateRecord | null>.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-credential-wh.dto.ts (1)
34-36: Enable nested validation forcredentialOfferPayload.The
credentialOfferPayloadproperty lacks@ValidateNested()and@Type(() => CredentialOfferPayloadDto)decorators, so the validator won't enforce the@IsArray()constraint oncredential_configuration_idswithin the nested DTO. Malformed payloads will pass validation, andextractCredentialConfigurationIdswill silently return[].Apply this diff:
@ApiProperty({ type: CredentialOfferPayloadDto }) @IsObject() + @ValidateNested() + @Type(() => CredentialOfferPayloadDto) credentialOfferPayload!: CredentialOfferPayloadDto;apps/api-gateway/src/x509/x509.controller.ts (1)
26-26: Fix IResponse import—use named import instead of default.Line 26 imports
IResponseas a default export, but the interface is exported as a named export inlibs/common/src/interfaces/response.interface.ts. This will cause a TypeScript compilation error.Apply this diff:
-import IResponse from '@credebl/common/interfaces/response.interface'; +import { IResponse } from '@credebl/common/interfaces/response.interface';apps/api-gateway/src/x509/dtos/x509.dto.ts (1)
103-109: Add{ each: true }to ValidateNested for array validation.The
nameproperty is an array ofNameDtoobjects, but@ValidateNested()is missing the{ each: true }option. Without it, the validator won't properly validate each array element, potentially allowing invalidNameDtoobjects.Apply this diff:
@ApiProperty({ type: [NameDto] }) @ArrayNotEmpty() @IsArray() - @ValidateNested() + @ValidateNested({ each: true }) @Type(() => NameDto) name: NameDto[];apps/verification/src/verification.service.ts (1)
238-249: Inconsistent error extraction pattern with risky fallback.Line 242 uses
error?.error?.reason || error?.messageas a fallback, then calls.includes('not found')on it. Iferror?.error?.reasonis undefined anderror?.messageis not a string (or is undefined), the.includes()call could throw a runtime error.This pattern differs from line 412, which uses only
error?.error?.reasonwithout a fallback. Consider standardizing on a defensive pattern that checks string type before calling.includes().Apply this diff:
- const errorMessage = error?.error?.reason || error?.message; + const errorMessage = error?.error?.reason; - if (errorMessage?.includes('not found')) { + if (typeof errorMessage === 'string' && errorMessage.includes('not found')) { throw new NotFoundException(errorMessage); } + else if (!errorMessage && error?.message) { + throw new RpcException(error.message); + } throw new RpcException(error);apps/oid4vc-issuance/test/app.e2e-spec.ts (1)
18-18: Remove HTTP test for NATS microservices module.The
Oid4vcIssuanceModuleis configured exclusively for NATS message-driven communication with only@MessagePatternhandlers, not HTTP routes. This test attempts a GET request to "/", which doesn't exist, and will fail.Either remove this test file entirely if HTTP testing isn't needed, or remove just this test:
- it('/ (GET)', () => request(app.getHttpServer()).get('/').expect(200).expect('Hello World!'));apps/oid4vc-issuance/src/main.ts (1)
15-15: Verify NATS service name consistency with gateway module.This microservice uses
CommonConstants.OIDC4VC_ISSUANCE_SERVICE, but the corresponding gateway module at line 17 ofapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.module.tsusesCommonConstants.ISSUANCE_SERVICE. If these constants differ, NATS message routing will fail.apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.module.ts (1)
17-17: Critical: NATS service name mismatch will break message routing.The gateway module uses
CommonConstants.ISSUANCE_SERVICE, but the OID4VC issuance microservice (apps/oid4vc-issuance/src/main.ts:15) usesCommonConstants.OIDC4VC_ISSUANCE_SERVICE. This mismatch prevents the gateway from communicating with the microservice.Apply this diff:
- options: getNatsOptions(CommonConstants.ISSUANCE_SERVICE, process.env.API_GATEWAY_NKEY_SEED) + options: getNatsOptions(CommonConstants.OIDC4VC_ISSUANCE_SERVICE, process.env.API_GATEWAY_NKEY_SEED)apps/oid4vc-verification/src/main.ts (1)
17-18: Uncomment the global exception filter for consistency.All other NATS microservices in the codebase use
app.useGlobalFilters(new HttpExceptionFilter())for consistent exception handling. This service should follow the same pattern.Uncomment line 18:
app.useLogger(app.get(NestjsLoggerServiceAdapter)); - // TODO: Not sure if we want the below - // app.useGlobalFilters(new HttpExceptionFilter()); + app.useGlobalFilters(new HttpExceptionFilter());You'll also need to import
HttpExceptionFilter:import { HttpExceptionFilter } from 'libs/http-exception.filter';apps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.controller.ts (7)
161-161: Verify service method name matches controller call.The controller calls
oidcGetIssuerById, but the service may define a different method name (e.g.,oidcIssuerGetById). Verify the service API matches this call.
191-191: Verify service method name matches controller call.The controller calls
oidcGetIssuers, but the service may define a different method name (e.g.,oidcIssuers). Verify the service API matches this call.
259-259: Verify service signature matches controller call.The controller passes
(CredentialTemplate, user, orgId, issuerId), but the service signature may differ in argument order or count. Verify the service expects these parameters in this order.
381-381: Verify service signature matches controller call.The controller passes
(user, orgId, templateId, dto, issuerId), but the service signature may differ. Verify parameter order and types match the service definition.
431-431: Verify service signature matches controller call.The controller passes
(user, orgId, templateId, issuerId)todeleteTemplate. Verify the service signature matches, particularly thatissuerIdis included.
556-556: Verify service method name matches controller call.The controller calls
getAllCredentialOffers, but the service may definegetCredentialOffersinstead. Verify the method name matches.
583-587: HTTP 204 should not include a response body.The endpoint returns
HttpStatus.NO_CONTENT(204) but includes a JSON body. HTTP 204 means "no content" and should not have a response body. Either change toHttpStatus.OK(200) if a body is needed, or remove the body for 204.Apply one of these fixes:
Option 1: Return 200 with body:
const finalResponse: IResponse = { - statusCode: HttpStatus.NO_CONTENT, + statusCode: HttpStatus.OK, message: ResponseMessages.oidcIssuerSession.success.delete, data: deletedofferDetails }; - return res.status(HttpStatus.NO_CONTENT).json(finalResponse); + return res.status(HttpStatus.OK).json(finalResponse);Option 2: Return 204 without body:
- const finalResponse: IResponse = { - statusCode: HttpStatus.NO_CONTENT, - message: ResponseMessages.oidcIssuerSession.success.delete, - data: deletedofferDetails - }; - return res.status(HttpStatus.NO_CONTENT).json(finalResponse); + return res.status(HttpStatus.NO_CONTENT).send();apps/verification/src/verification.controller.ts (1)
48-50: TightenPromise<object>/Promise<object | object[]>return types to domain interfaces.These three endpoints still return very broad
objecttypes even though the module already defines richer interfaces (e.g.,IProofPresentationDetails,IProofPresentation, etc.). This weakens type safety and tooling support.Consider updating the signatures to use concrete response types aligned with what
VerificationServiceactually returns (for example,Promise<IProofPresentationDetails>forgetProofPresentationByIdand a suitable domain union forsendProofRequest/verifyPresentation), and adjust the service typings accordingly.Also applies to: 68-73, 82-84
libs/logger/src/logging.interceptor.ts (1)
23-33: Add explicit string guard when readingcontextIdfrom headers.
contextIdFromHeaderis currently any truthy value returned byheaders.get('contextId'). A non-string truthy value (e.g., number or object) will pass theif (contextIdFromHeader)check and be stored as the correlation ID, even thoughsetupContextIdis typed to take astring.Refine the extraction to enforce a non-empty string:
- const contextIdFromHeader = headers && 'function' === typeof headers.get ? headers.get('contextId') : undefined; + const rawContextId = headers && typeof headers.get === 'function' ? headers.get('contextId') : undefined; + const contextIdFromHeader = + typeof rawContextId === 'string' && rawContextId ? rawContextId : undefined;This ensures only valid string IDs are re-used; everything else falls back to generating a new UUID.
Also applies to: 44-47
apps/x509/src/x509.controller.ts (1)
31-33: Normalize method naming and user parameter usage in X.509 controller.Two issues remain here:
deActivateCertificate/'deActivate-x509-certificate'are inconsistently cased relative toactivateCertificate; preferdeactivateCertificateand'deactivate-x509-certificate'for consistency.getCertificateByOrgIdandgetCertificateacceptuser: IUserRequestbut never use it, while the mutating endpoints use Prismauser. Either:
- Remove
userfrom the read-only endpoints, or- Standardize on a single user type (e.g.,
IUserRequest) across all methods and propagate it through the service layer if needed.Also applies to: 36-48
apps/x509/src/interfaces/x509.interface.ts (1)
4-13: AlignstatusandOrgAgenttypings with existing enums and DB nullability.Two existing concerns still apply:
CreateX509CertificateEntity.statusis typed asstringwhile other DTOs usex5cRecordStatus. Using the enum here as well would constrain creation-time values and keep status handling consistent.OrgAgentcurrently marks all fields as required, but several are nullable in the Prisma schema (e.g.,orgDid,verkey,agentEndPoint,agentId,isDidPublic,ledgerId,orgAgentTypeId,tenantId). These should be optional (?) to match DB reality and avoid incorrect non-null assumptions downstream.Also applies to: 29-43
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (2)
61-67: Fix misleading JSDoc for verifier create/update methods.Both JSDoc blocks still describe “Create issuer against a org(tenant)” and mention deletion status, while the methods actually create/update verifiers:
- Line 61–67 for
oidcIssuerCreate- Line 106–113 for
oidcIssuerUpdateUpdate the comments to accurately reflect verifier operations and parameters (orgId, verifierId, create/update DTO, response). Renaming the methods to something like
createVerifier/updateVerifierwould also reduce confusion, but the minimum is to correct the JSDoc so it matches current behavior.Also applies to: 106-113
308-413: Normalize response envelope for presentation GET endpoints.
getVerificationPresentationandgetVerificationPresentationResponsereturn a custom shape:return res.status(HttpStatus.OK).json({ success: true, message: '...retrieved successfully.', data: result });while the rest of the controller (and
ApiResponseDto) use the sharedIResponse/IResponseTypecontract withstatusCodeandResponseMessages.*.For consistency and to avoid surprising clients, consider:
- return res.status(HttpStatus.OK).json({ - success: true, - message: 'Verifier details retrieved successfully.', - data: result - }); + const finalResponse: IResponse = { + statusCode: HttpStatus.OK, + message: ResponseMessages.oid4vpSession.success.fetch, + data: result + }; + return res.status(HttpStatus.OK).json(finalResponse);and a similar change in
getVerificationPresentationResponseusing the appropriateResponseMessages.oid4vpSession.*key.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer.dto.ts (1)
156-179: Add validation and API metadata forIssuerUpdationDto.issuerId.
issuerIdis currently undecorated, so it’s effectivelyanyat runtime, skipped by class-validator and Swagger. Given it’s used to identify which issuer to update, this should be constrained to a string and marked optional/explicit in the contract. This was already raised on a previous commit and still applies.export class IssuerUpdationDto { - issuerId?: string; + @ApiPropertyOptional({ + description: 'Optional override for the issuer identifier', + example: 'credebl-university' + }) + @IsOptional() + @IsString({ message: 'issuerId must be a string' }) + issuerId?: string;You may also want to drop the redundant
@ApiPropertyonbatchCredentialIssuanceSizeand keep only@ApiPropertyOptionalfor clarity.apps/api-gateway/src/oid4vc-issuance/dtos/oid4vc-issuer-template.dto.ts (1)
240-253: Enforcetemplateas required inCreateCredentialTemplateDto.
templateis mandatory for both SD-JWT and mDL, but there’s no@IsDefined()guard. Validators will accept payloads withtemplateomitted, and downstream code will receiveundefined, causing runtime errors when accessing its structure. This matches an earlier review comment and is still unresolved.@ApiProperty({ type: Object, oneOf: [{ $ref: getSchemaPath(SdJwtTemplateDto) }, { $ref: getSchemaPath(MdocTemplateDto) }], description: 'Credential template definition (depends on credentialFormat)' }) + @IsDefined({ message: 'template is required' }) @ValidateNested() @Type(({ object }) => {apps/api-gateway/src/connection/dtos/connection.dto.ts (1)
71-83: Type and validateorgIdinCreateOutOfBandConnectionInvitation.
orgIdis still declared without a type or decorators:invitationDid?: string; orgId;This leaves it as
anyand outside Swagger/class-validator, which is risky for a routing/authorization field.Align it with other string fields:
@ApiPropertyOptional() @IsString() @IsOptional() @IsNotEmpty({ message: 'Please provide invitation did' }) invitationDid?: string; - orgId; + @ApiPropertyOptional() + @IsString() + @IsOptional() + @IsNotEmpty({ message: 'Please provide orgId' }) + orgId?: string;If
orgIdis meant to be injected from context rather than client-supplied, consider removing it from this DTO entirely to avoid confusion.apps/x509/src/repositories/x509.repository.ts (1)
124-145: FixfindByIdto usefindFirst(relation filters are invalid onfindUnique).
findByIdstill callsfindUniquewith a relation filter:const certificate = await this.prisma.x509_certificates.findUnique({ where: { id, org_agents: { orgId } } });
findUniqueonly accepts aWhereUniqueinput; relation filters likeorg_agentsare not supported and will either fail or ignore the org constraint. UsefindFirst(or a composite unique) instead:- const certificate = await this.prisma.x509_certificates.findUnique({ + const certificate = await this.prisma.x509_certificates.findFirst({ where: { id, org_agents: { orgId } } });The NotFoundException logic below can remain unchanged.
apps/oid4vc-issuance/src/oid4vc-issuance.repository.ts (3)
40-53: Handle missing org agent type before destructuring.
getOrgAgentTypedestructuresagentdirectly fromfindFirst:const { agent } = await this.prisma.org_agents_type.findFirst({ where: { id: orgAgentId } }); return agent;If no record exists, this throws a TypeError before you can raise a meaningful 404. Use an intermediate variable and explicit not-found handling:
async getOrgAgentType(orgAgentId: string): Promise<string> { try { - const { agent } = await this.prisma.org_agents_type.findFirst({ - where: { - id: orgAgentId - } - }); - - return agent; + const record = await this.prisma.org_agents_type.findFirst({ + where: { id: orgAgentId } + }); + + if (!record) { + throw new NotFoundException(`Org agent type not found for id=${orgAgentId}`); + } + + return record.agent;
55-66: AligngetOrganizationByTenantIdreturn type withfindFirst(nullable).
getOrganizationByTenantIdwrapsfindFirstbut returnsPromise<org_agents>:async getOrganizationByTenantId(tenantId: string): Promise<org_agents> { return this.prisma.org_agents.findFirst({ where: { tenantId } }); }Since
findFirstcan returnnull, this type is misleading. Either:
- Change to
Promise<org_agents | null>and keep current behavior, or- Throw a
NotFoundExceptionwhen no org agent is found and keep the non-nullable type.Given current callers already null-check, updating the return type is likely the least disruptive.
161-188: Clarify nullability forgetCurrentActiveCertificate(issuance).Same pattern here:
findFirstis used, but the method promises a non-null certificate:async getCurrentActiveCertificate(orgId: string, keyType: x5cKeyType): Promise<X509CertificateRecord> { const certificate = await this.prisma.x509_certificates.findFirst({ ... }); return certificate; }If there is no active certificate, this returns
nulldespite the signature. Either:
- Update the signature to
Promise<X509CertificateRecord | null>, or- Throw a
NotFoundExceptionwhencertificateis missing to guarantee a non-null return.Make sure the choice aligns with how issuers and verifiers handle “no active certificate” states elsewhere.
apps/oid4vc-issuance/libs/helpers/issuer.metadata.ts (2)
42-51: AlignCredentialConfig.claimstype with actual claims object shape.
CredentialConfigdeclares:type CredentialConfig = { ... claims: Claim[]; ... };but both builders do:
const claims = buildClaimsFromTemplate(template); // Record<string, any> ... claimsSo at runtime
claimsis a nested object (Record<string, any>), notClaim[]. This mismatch can confuse callers and undermines type safety, and it was already raised in earlier reviews.Either:
- Change the type to match the object shape:
type CredentialConfig = { format: string; vct?: string; scope: string; doctype?: string; - claims: Claim[]; + claims: Record<string, any>; credential_signing_alg_values_supported: string[]; cryptographic_binding_methods_supported: string[]; display: { name: string; description?: string; locale?: string }[]; };or
- Refactor
buildClaimsFromTemplateand the builders to actually returnClaim[].Right now the implementation and contract diverge.
Also applies to: 312-327, 347-355
391-411: MakeappearanceJsonhandling null‑safe before accessing.display.
coerceJsonObjectcan returnnull, but the code immediately casts and dereferences:const appearanceJson = coerceJsonObject<unknown>(templateRow.appearance); const displayConfigurations = (appearanceJson as Appearance).display?.map((displayEntry) => ({ ... })) ?? [];If
appearanceJsonisnullor a primitive,(appearanceJson as Appearance).displaythrows before?.is applied.Guard the cast:
const appearanceJson = coerceJsonObject<unknown>(templateRow.appearance); - - // Prepare the display configuration - const displayConfigurations = - (appearanceJson as Appearance).display?.map((displayEntry) => ({ + const appearance = + appearanceJson && 'object' === typeof appearanceJson + ? (appearanceJson as Appearance) + : null; + + const displayConfigurations = + appearance?.display?.map((displayEntry) => ({ name: displayEntry.name, description: displayEntry.description, locale: displayEntry.locale, logo: displayEntry.logoThis keeps the builder robust when
appearanceis missing or malformed.apps/api-gateway/src/oid4vc-issuance/dtos/issuer-sessions.dto.ts (4)
269-301: Replace@ValidateNestedonpayloadwith object validation
CredentialDto.payloadis typed asobjectand decorated with@ValidateNested(), but there is no DTO class or@Typehint.ValidateNestedis effectively a no-op here and misleading.Prefer explicit object validation:
@ApiProperty({ /* existing example */ }) @IsObject() payload: Record<string, unknown>;or, if the payload has a fixed structure, introduce a dedicated DTO and use
@ValidateNested()with@Type(() => ThatDto).[Suggest updating the decorator and type accordingly.]
346-377: Align example configs withPreAuthorizedCodeFlowConfigDto/AuthorizationCodeFlowConfigDtoThe
CreateCredentialOfferD2ADtoexamples include fields that do not exist on the DTOs:
preAuthorizedCodeFlowConfigexample usespreAuthorizedCode,userPinRequired, andtxCode.inputMode, but DTO only hastxCodeandauthorizationServerUrl(andTxCodeDto.input_modeis spelled with underscore).authorizationCodeFlowConfigexample showsclientId,redirectUri,scope,state, butAuthorizationCodeFlowConfigDtoonly exposesauthorizationServerUrl.Swagger examples should reflect the actual DTO shape or the DTOs should be extended to match the intended API. As-is, generated docs are misleading and fields would be stripped by validation/whitelisting.
161-167: Add nested validation forvalidityInfo
CredentialRequestDto.validityInfois aValidityInfoobject but only has@IsOptional(). Without@ValidateNested()and@Type(() => ValidityInfo), nested date constraints (validFrom/validUntil) won’t run, and invalid date payloads will slip through DTO validation.Recommend:
@IsOptional() @ValidateNested() @Type(() => ValidityInfo) validityInfo?: ValidityInfo;
251-253: Fix ESLint errors forx5cexamples and mixed operators inexpESLint is flagging:
array-bracket-newlinefor multi-linex5carrays (lines 251–253, 319–321).no-mixed-operatorsforDate.now() + 5 * 365 * 24 * 60 * 60 * 1000(line 295).Minimal fixes:
- x5c: [ - 'MIIB3jCCAZCgAwIBAgIQQfBdIK9v3TIzKR+1HjlixD...' - ] + x5c: ['MIIB3jCCAZCgAwIBAgIQQfBdIK9v3TIzKR+1HjlixD...']- exp: dateToSeconds(new Date(Date.now() + 5 * 365 * 24 * 60 * 60 * 1000)) + exp: dateToSeconds(new Date(Date.now() + (5 * 365 * 24 * 60 * 60 * 1000)))Apply the same single-line
x5cpattern in all example blocks.Also applies to: 319-321, 293-296
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts (1)
44-69: Add explicitstringtypes toorgIdparametersMethods
oid4vpGetVerifier,oid4vpDeleteVerifier,oid4vpGetVerifierSession, andgetVerificationSessionResponseomit a type fororgId, so it defaults toanyunder non‑strict configs:async oid4vpGetVerifier(orgId, verifierId?: string): Promise<object> { ... }For consistency with the rest of the service and better type safety, annotate:
async oid4vpGetVerifier(orgId: string, verifierId?: string): Promise<object> { ... } async oid4vpDeleteVerifier(orgId: string, verifierId: string): Promise<object> { ... } async oid4vpGetVerifierSession(orgId: string, query?: VerificationPresentationQueryDto): Promise<object> { ... } async getVerificationSessionResponse(orgId: string, verificationSessionId: string): Promise<object> { ... }apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (2)
350-399: Avoid hard-wiringsignerOptions[0]inbuildMdocCredential
buildMdocCredentialassumes the first signer applies to every mdoc credential:if (!signerOptions?.length || !signerOptions[0].x5c?.length) { ... } ... const certificateDetail = activeCertificateDetails.find( (x) => x.certificateBase64 === signerOptions[0].x5c[0] ); ... return { credentialSupportedId, signerOptions: signerOptions ? signerOptions[0] : undefined, ... };For multi-template offers or mixed signer configurations, this binds all mdocs to the first signer/certificate, regardless of template.
Prefer resolving the signer per template, similar to the SD‑JWT path (e.g., via
templateRecord.signerOption), and use that resolved signer both for certificate matching and the returnedsignerOptionsfield.
270-345: Fix signer mapping and handle missingsignerOptionsinbuildSdJwtCredentialTwo issues here:
- Incorrect signer mapping for X.509 templates
const templateSignerOption: SignerOption = signerOptions.find( (x) => templateRecord.signerOption.toLowerCase() === x.method ); ... if (templateRecord.signerOption === SignerMethodOption.X5C && credentialRequest.validityInfo) { ... }
SignerOption.methodis'did' | 'x5c', while DBtemplateRecord.signerOptionuses enum values likeDID,X509_P256,X509_ED25519. For X.509 templates,.toLowerCase()yields"x509_p256"/"x509_ed25519", which never equals"x5c". Result: X.509 templates can’t resolve their signer option and never enter the X5C validation path.You need to normalize the DB enum into
SignerMethodOptionfirst, e.g.:const rawSigner = (templateRecord.signerOption ?? '').toString().toLowerCase(); if (!rawSigner) { throw new UnprocessableEntityException('Template signerOption is not configured'); } const expectedMethod = rawSigner === 'did' ? SignerMethodOption.DID : SignerMethodOption.X5C; if (!Array.isArray(signerOptions)) { throw new UnprocessableEntityException('signerOptions must be provided for credential building'); } const templateSignerOption = signerOptions.find((x) => x.method === expectedMethod); if (!templateSignerOption) { throw new UnprocessableEntityException( `Signer option "${templateRecord.signerOption}" is not configured for template ${templateRecord.id}` ); }and keep your existing X5C/date validation logic.
signerOptionsmay beundefined
buildCredentialOfferPayloaddeclaressignerOptions?: SignerOption[]but calls:return buildSdJwtCredential(credentialRequest, templateRecord, signerOptions, activeCertificateDetails);If callers omit
signerOptions,signerOptions.find(...)will throw. Either:
- Make
signerOptionsrequired in both signatures, or- Guard inside
buildSdJwtCredentialand throw a clearUnprocessableEntityExceptionwhen it’s missing.Together, these fixes avoid crashes and make X.509 SD‑JWT templates usable.
apps/connection/src/connection.service.ts (1)
64-72: Tighten return types for invitation helpers instead ofunknown/anyBoth helpers are central to connection-invitation flows but are weakly typed:
async _createConnectionInvitation(...): Promise<unknown> { ... } ... // eslint-disable-next-line @typescript-eslint/no-explicit-any async _createOutOfBandConnectionInvitation(...): Promise<any> { ... }Callers expect specific fields:
createConnectionInvitation?.invitationUrlcreateConnectionInvitation?.invitationDidcreateConnectionInvitation.outOfBandRecord.idDefine an interface, e.g.:
interface IConnectionInvitationResult { invitationUrl: string; invitationDid?: string; outOfBandRecord: { id: string }; }and update signatures:
async _createConnectionInvitation(...): Promise<IConnectionInvitationResult> { ... } async _createOutOfBandConnectionInvitation(...): Promise<IConnectionInvitationResult> { ... }Then remove the
anyeslint-disable and let TypeScript enforce the shape across usages.Also applies to: 572-585
apps/x509/src/x509.service.ts (2)
42-60: Guard required X.509 options before using nested fields
createCertificateassumesoptions.validity.notBefore/notAfterandoptions.authorityKey.keyTypeare always present. Ifoptions.validityoroptions.authorityKeyis missing, you’ll get a TypeError before you can return a clean 4xx:validFrom: options.validity.notBefore, expiry: options.validity.notAfter, keyType: options.authorityKey.keyType,Add upfront validation and fail with a clear domain error:
const { options, user, orgId } = payload; if (!options?.validity?.notBefore || !options?.validity?.notAfter || !options?.authorityKey?.keyType) { throw new UnprocessableEntityException(ResponseMessages.x509.error.errorCreate); }before building
certificateDateCheckDto.Also applies to: 84-93
145-183: Validate decoded certificate/publicKey shape before use inimportCertificateAfter
_decodeX509CertificateForOrg, the code trustsdecodedResult.responseto containpublicKey.keyTypeandx509Certificate.notBefore/notAfter:const { publicKey } = decodedResult.response; const decodedCert = decodedResult.response.x509Certificate; ... const isValidKeyType = Object.values(x5cKeyType).includes(publicKey.keyType as x5cKeyType); const validFrom = new Date(decodedCert.notBefore); const expiry = new Date(decodedCert.notAfter);If the agent ever returns a partial/malformed response, these dereferences will throw.
Mirror the defensive checks suggested for
createCertificate:const { publicKey, x509Certificate } = decodedResult.response ?? {}; if (!publicKey?.keyType || !x509Certificate?.notBefore || !x509Certificate?.notAfter) { throw new UnprocessableEntityException(ResponseMessages.x509.error.errorDecode); } const validFrom = new Date(x509Certificate.notBefore); const expiry = new Date(x509Certificate.notAfter);Also applies to: 203-218
libs/common/src/common.constant.ts (1)
129-137: Resolve TODO onURL_OIDC_VERIFIER_CREATEbefore shipping
URL_OIDC_VERIFIER_CREATEis marked:URL_OIDC_VERIFIER_CREATE = '/openid4vc/verifier', //TODO: correct this URLThis suggests the path is known to be wrong but is now a shared constant used by
getAgentUrland verifier flows. Once wired, debugging mismatched agent URLs will be painful.Either correct the URL now (preferred) or gate any usage behind a feature flag and track a follow‑up task, but avoid merging with a known-bad constant.
apps/oid4vc-verification/src/oid4vc-verification.service.ts (4)
43-98: Avoid dereferencing undefinedcreatedVerifierDetailson conflictsIn
oid4vpCreateVerifier, the 409 path usescreatedVerifierDetails.verifierId:let createdVerifierDetails; ... try { createdVerifierDetails = await this._createOid4vpVerifier(...); ... } catch (error) { const status409 = ...; if (status409) { throw new ConflictException(`Verifier with id '${createdVerifierDetails.verifierId}' already exists`); } throw error; }If the agent responds with 409 before returning a body,
createdVerifierDetailsis stillundefined, and this catch block will throw a newTypeError, masking the real conflict.Use the known
verifierIdfrom the request instead:if (status409) { throw new ConflictException(`Verifier with id '${verifierId}' already exists`); }and type
createdVerifierDetailsasVerifierRecord | undefinedto make this clearer.
100-158: Fix verifier existence guard and conflict handling inoid4vpUpdateVerifierIssues in the update flow:
- Existence check is wrong:
const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); if (0 > existingVerifiers.length) { ... } // never true, and will throw if array is null
- Conflict branch in the inner catch references
updatedVerifierDetails.verifierIdeven whenupdatedVerifierDetailsis stillundefined.Recommended adjustments:
const existingVerifiers = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); if (!existingVerifiers || existingVerifiers.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } ... let updatedVerifierDetails: VerifierRecord | undefined; try { const agentResult = await this._updateOid4vpVerifier(updateVerifier, url, orgId); if (!agentResult) { throw new InternalServerErrorException(ResponseMessages.oid4vp.error.updateFailed); } updatedVerifierDetails = agentResult.data as VerifierRecord; } catch (error) { const status409 = ...; if (status409) { const conflictId = existingVerifiers?.[0]?.publicVerifierId ?? verifierId; throw new ConflictException(`Verifier with id '${conflictId}' already exists`); } throw error; }
175-205: Guard against missing verifiers and use public ID for delete callsIn
deleteVerifierById:const checkIdExist = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); if (0 == checkIdExist.length) { ... } // will throw if null ... const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_VERIFIER_DELETE, checkIdExist[0].verifierId);
- If the repository returns
null,checkIdExist.lengthcrashes.- For create/update flows, the agent-facing ID is
publicVerifierId; deleting by internalverifierIdrisks a mismatch.Consider:
const checkIdExist = await this.oid4vpRepository.getVerifiersByVerifierId(orgId, verifierId); if (!checkIdExist || checkIdExist.length === 0) { throw new NotFoundException(ResponseMessages.oid4vp.error.notFound); } const { publicVerifierId, verifierId: internalId } = checkIdExist[0]; const url = await getAgentUrl( agentEndPoint, CommonConstants.OIDC_VERIFIER_DELETE, publicVerifierId ?? internalId );This avoids null crashes and keeps the agent call consistent with other flows.
207-283: Fix request signer keyType lookup and replaceconsole.logwith loggerInside
oid4vpCreateVerificationSession:if (sessionRequest.requestSigner.method === SignerOption.X509_P256 || ...) { const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( orgId, sessionRequest.requestSigner.methodv ); ... requestSigner = { method: SignerMethodOption.X5C, x5c: [activeCertificate.certificateBase64] }; activeCertificateDetails.push(activeCertificate); } ... console.log(`[oid4vpCreateVerificationSession] sessionRequest=${JSON.stringify(sessionRequest)}`); const url = await getAgentUrl(...); console.log(`[oid4vpCreateVerificationSession] calling agent URL=${url}`);
RequestSignerdoes not exposemethodv; this argument is alwaysundefined, sogetCurrentActiveCertificatecannot resolve a certificate and all x5c flows fail.console.logbypasses the service logger and exists only because of a file‑wideno-consoledisable.Fixes:
const activeCertificate = await this.oid4vpRepository.getCurrentActiveCertificate( orgId, sessionRequest.requestSigner.method ); ... this.logger.debug(`[oid4vpCreateVerificationSession] sessionRequest=${JSON.stringify(sessionRequest)}`); ... this.logger.debug(`[oid4vpCreateVerificationSession] calling agent URL=${url}`);This aligns with the enum type and your logging pattern.
apps/oid4vc-issuance/src/oid4vc-issuance.service.ts (4)
200-211: Fix null-check to avoid dereferencinggetIssuerDetailswhen missingThe condition:
if (!getIssuerDetails && !getIssuerDetails.publicIssuerId) {will still evaluate
getIssuerDetails.publicIssuerIdwhengetIssuerDetailsisnull/undefined, causing aTypeErrorinstead of a cleanNotFoundException.Use an OR and optional chaining so you short-circuit on a missing record:
- if (!getIssuerDetails && !getIssuerDetails.publicIssuerId) { + if (!getIssuerDetails || !getIssuerDetails?.publicIssuerId) { throw new NotFoundException(ResponseMessages.oidcIssuer.error.notFound); }This also aligns with the intended behavior previously discussed in older review comments.
265-292:deleteOidcIssueruses the wrong source of truth and rethrows rawErrorTwo issues here:
- Wrong type for
issuerRecordId
oidcIssuerGetByIdreturns an issuer response from the agent, not the DB row, so:const issuerRecordId = await this.oidcIssuerGetById(id, orgId); if (!issuerRecordId.id) {will almost certainly see
issuerRecordId.idasundefined, making valid issuers look missing and building the delete URL with the wrong identifier.You likely want the DB record instead:
- const issuerRecordId = await this.oidcIssuerGetById(id, orgId); - if (!issuerRecordId.id) { + const issuerRecord = await this.oid4vcIssuanceRepository.getOidcIssuerDetailsById(id); + if (!issuerRecord) { throw new NotFoundException(ResponseMessages.oidcIssuer.error.notFound); } ... - const url = await getAgentUrl(agentEndPoint, CommonConstants.OIDC_ISSUER_DELETE, issuerRecordId.id); + const url = getAgentUrl( + agentEndPoint, + CommonConstants.OIDC_ISSUER_DELETE, + issuerRecord.publicIssuerId + );Adjust the identifier passed to the agent (
publicIssuerIdvs PK) to match the API contract you use elsewhere (e.g.,oidcIssuerGetById).
- Inconsistent error propagation
The catch block currently rethrows a rawError, unlike the rest of the service which usesRpcException:} catch (error) { if ('PrismaClientKnownRequestError' === error.name) { throw new BadRequestException(...); } throw new Error(error.response ? error.response : error); }Better to log and rethrow as
RpcException:- } catch (error) { - if ('PrismaClientKnownRequestError' === error.name) { - throw new BadRequestException(error.meta?.cause ?? ResponseMessages.oidcIssuer.error.deleteFailed); - } - throw new Error(error.response ? error.response : error); - } + } catch (error) { + if ('PrismaClientKnownRequestError' === error?.name) { + throw new BadRequestException(error.meta?.cause ?? ResponseMessages.oidcIssuer.error.deleteFailed); + } + this.logger.error(`[deleteOidcIssuer] - error: ${JSON.stringify(error)}`); + throw new RpcException(error?.response ?? error); + }This keeps RPC semantics consistent and avoids surfacing untyped
Errors to callers.
468-497: DB/agent inconsistency risk indeleteTemplate(no rollback on agent failure)
deleteTemplatedeletes the DB record before updating the agent, and does not attempt any rollback if_createOIDCTemplatefails:const deleteTemplate = await this.oid4vcIssuanceRepository.deleteTemplate(templateId); // ... const createTemplateOnAgent = await this._createOIDCTemplate(issuerTemplateConfig, url, orgId); if (!createTemplateOnAgent) { throw new NotFoundException(...); }If the agent call fails, the template is gone from the DB but still present (or partially present) in the agent, leaving the system inconsistent. Contrast this with
createTemplateandupdateTemplate, which include compensation logic.Either:
- Flip the order (update agent first, then delete DB), or
- Add rollback that recreates the DB row if the agent update fails.
Example rollback pattern (mirroring
updateTemplate):- const deleteTemplate = await this.oid4vcIssuanceRepository.deleteTemplate(templateId); + const deleteTemplate = await this.oid4vcIssuanceRepository.deleteTemplate(templateId); if (!deleteTemplate) { throw new NotFoundException(ResponseMessages.oidcTemplate.error.deleteTemplate); } ... - const createTemplateOnAgent = await this._createOIDCTemplate(issuerTemplateConfig, url, orgId); + const createTemplateOnAgent = await this._createOIDCTemplate(issuerTemplateConfig, url, orgId); if (!createTemplateOnAgent) { - throw new NotFoundException(ResponseMessages.issuance.error.agentEndPointNotFound); + // Roll back DB delete or surface a structured failure + await this.oid4vcIssuanceRepository.createTemplate(issuerId, template); + throw new RpcException(ResponseMessages.oidcTemplate.error.failedDeleteTemplate); }(Exact rollback shape depends on what
createTemplateexpects, but the key point is to keep DB and agent in sync.)
627-675: Remove hard-coded'Test'x5c stub and fixsignerOptions.x5ctype in D2A flowIn
createOidcCredentialOfferD2A, for X.509 flows you still inject a hard-coded'Test'value:if (signerOptions.method === SignerMethodOption.X5C) { if (!signerOptions.x5c || 0 === signerOptions.x5c.length) { // const x5cFromDb = await this.oid4vcIssuanceRepository.getIssuerX5c( const x5cFromDb = 'Test'; ... if (!x5cFromDb || 0 === x5cFromDb.length) { throw new BadRequestException(`No x5c found for issuer`); } signerOptions.x5c = x5cFromDb; } }Problems:
'Test'produces invalid credential offers in any real environment.signerOptions.x5cis supposed to be astring[], but you assign astring.You should either wire the real DB lookup or fail fast when
x5cis missing; until the DB wiring is ready, failing fast is safer than injecting a dummy value.For a minimal safe fix:
- if (signerOptions.method === SignerMethodOption.X5C) { - if (!signerOptions.x5c || 0 === signerOptions.x5c.length) { - // const x5cFromDb = await this.oid4vcIssuanceRepository.getIssuerX5c( - const x5cFromDb = 'Test'; - ... - if (!x5cFromDb || 0 === x5cFromDb.length) { - throw new BadRequestException(`No x5c found for issuer`); - } - signerOptions.x5c = x5cFromDb; - } - } + if (signerOptions.method === SignerMethodOption.X5C) { + if (!Array.isArray(signerOptions.x5c) || signerOptions.x5c.length === 0) { + throw new BadRequestException(`No x5c found for issuer`); + } + }If you do want automatic lookup, replace the stub with
await this.oid4vcIssuanceRepository.getIssuerX5c(oidcCredentialD2APayload.publicIssuerId, orgId)and keep thestring[]typing.Also, consider updating the log/error message in this method (
[createOidcCredentialOffer]) to reflect the D2A path name for easier debugging.
| @ApiResponse({ status: HttpStatus.OK, description: 'Verifier updated successfully.', type: ApiResponseDto }) | ||
| @ApiBearerAuth() | ||
| @Roles(OrgRoles.OWNER) | ||
| @UseGuards(AuthGuard('jwt'), OrgRolesGuard) | ||
| async oidcIssuerUpdate( | ||
| @Param( | ||
| 'orgId', | ||
| new ParseUUIDPipe({ | ||
| exceptionFactory: (): Error => { | ||
| throw new BadRequestException(ResponseMessages.organisation.error.invalidOrgId); | ||
| } | ||
| }) | ||
| ) | ||
| orgId: string, | ||
| @Param('verifierId') | ||
| verifierId: string, | ||
| @User() user: user, | ||
| @Body() updateVerifier: UpdateVerifierDto, | ||
| @Res() res: Response | ||
| ): Promise<Response> { | ||
| this.logger.debug(`[oidcIssuerUpdate] Called with orgId=${orgId}, verifierId=${verifierId}, user=${user.id}`); | ||
| const updateVerifierRes = await this.oid4vcVerificationService.oid4vpUpdateVerifier( | ||
| updateVerifier, | ||
| orgId, | ||
| verifierId, | ||
| user | ||
| ); | ||
|
|
||
| this.logger.debug(`[oidcIssuerUpdate] Verifier updated: ${updateVerifierRes.id}`); | ||
|
|
||
| const finalResponse: IResponse = { | ||
| statusCode: HttpStatus.CREATED, | ||
| message: ResponseMessages.oid4vp.success.update, | ||
| data: updateVerifierRes | ||
| }; | ||
| return res.status(HttpStatus.OK).json(finalResponse); | ||
| } |
There was a problem hiding this comment.
Align oidcIssuerUpdate status code in payload with HTTP response.
In oidcIssuerUpdate, the decorator and HTTP response use HttpStatus.OK, but the payload’s statusCode is HttpStatus.CREATED:
const finalResponse: IResponse = {
statusCode: HttpStatus.CREATED, // ← mismatch
message: ResponseMessages.oid4vp.success.update,
data: updateVerifierRes
};
return res.status(HttpStatus.OK).json(finalResponse);Set both to HttpStatus.OK for consistency:
- const finalResponse: IResponse = {
- statusCode: HttpStatus.CREATED,
+ const finalResponse: IResponse = {
+ statusCode: HttpStatus.OK,
message: ResponseMessages.oid4vp.success.update,
data: updateVerifierRes
};🤖 Prompt for AI Agents
In apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts
around lines 119-155, the JSON payload's statusCode is HttpStatus.CREATED while
the actual HTTP response and decorator use HttpStatus.OK; change the
finalResponse.statusCode value from HttpStatus.CREATED to HttpStatus.OK so the
payload matches the HTTP response (leave the res.status(HttpStatus.OK) as-is).
| async storeOidcCredentialWebhook( | ||
| CredentialOfferWebhookPayload: Oid4vcCredentialOfferWebhookPayload | ||
| ): Promise<object> { | ||
| try { | ||
| // pick fields | ||
| let organisationId: string; | ||
| const { oidcIssueCredentialDto, id } = CredentialOfferWebhookPayload; | ||
|
|
||
| if ('default' !== oidcIssueCredentialDto?.contextCorrelationId) { | ||
| const getOrganizationId = await this.oid4vcIssuanceRepository.getOrganizationByTenantId( | ||
| oidcIssueCredentialDto?.contextCorrelationId | ||
| ); | ||
| organisationId = getOrganizationId?.orgId; | ||
| } else { | ||
| organisationId = id; | ||
| } | ||
|
|
||
| const { | ||
| contextCorrelationId, | ||
| credentialOfferPayload, | ||
| issuedCredentials, | ||
| id: issuanceSessionId | ||
| } = oidcIssueCredentialDto ?? {}; | ||
| const cfgIds: string[] = Array.isArray(credentialOfferPayload?.credential_configuration_ids) | ||
| ? credentialOfferPayload.credential_configuration_ids | ||
| : []; | ||
| const issuedCredentialsArr: string[] | undefined = | ||
| Array.isArray(issuedCredentials) && 0 < issuedCredentials.length | ||
| ? issuedCredentials.map((c: any) => ('string' === typeof c ? c : JSON.stringify(c))) | ||
| : issuedCredentials && Array.isArray(issuedCredentials) && 0 === issuedCredentials.length | ||
| ? [] | ||
| : undefined; | ||
|
|
||
| const sanitized = { | ||
| ...CredentialOfferWebhookPayload, | ||
| credentialOfferPayload: { | ||
| credential_configuration_ids: cfgIds | ||
| } | ||
| }; | ||
|
|
||
| const agentDetails = await this.oid4vcIssuanceRepository.storeOidcCredentialDetails( | ||
| CredentialOfferWebhookPayload, | ||
| organisationId | ||
| ); | ||
| return agentDetails; |
There was a problem hiding this comment.
Webhook storage: use the sanitized payload and guard organisationId
storeOidcCredentialWebhook computes a sanitized version of the payload and some derived fields, but then ignores them when persisting:
const sanitized = {
...CredentialOfferWebhookPayload,
credentialOfferPayload: {
credential_configuration_ids: cfgIds
}
};
const agentDetails = await this.oid4vcIssuanceRepository.storeOidcCredentialDetails(
CredentialOfferWebhookPayload,
organisationId
);Issues:
- If
getOrganizationByTenantIdreturnsnull,organisationIdstaysundefined, but you still callstoreOidcCredentialDetailswithout failing fast. - If the intent of
sanitizedis to avoid storing fullissuedCredentialsor other sensitive blobs, that intent isn’t honored: you persist the original payload instead.
Given this is a webhook from issuance, it can easily include PII or full credential data. You probably want to either:
- Persist the sanitized payload and enforce a valid
organisationId, or - Delete the dead code and explicitly document that you intend to store the raw payload.
Example aligning with (1):
- if ('default' !== oidcIssueCredentialDto?.contextCorrelationId) {
- const getOrganizationId = await this.oid4vcIssuanceRepository.getOrganizationByTenantId(
- oidcIssueCredentialDto?.contextCorrelationId
- );
- organisationId = getOrganizationId?.orgId;
- } else {
- organisationId = id;
- }
+ if ('default' !== oidcIssueCredentialDto?.contextCorrelationId) {
+ const getOrganizationId = await this.oid4vcIssuanceRepository.getOrganizationByTenantId(
+ oidcIssueCredentialDto?.contextCorrelationId
+ );
+ organisationId = getOrganizationId?.orgId;
+ if (!organisationId) {
+ throw new NotFoundException('Organization not found for given tenant id');
+ }
+ } else {
+ organisationId = id;
+ }
...
- const agentDetails = await this.oid4vcIssuanceRepository.storeOidcCredentialDetails(
- CredentialOfferWebhookPayload,
- organisationId
- );
- return agentDetails;
+ const record = await this.oid4vcIssuanceRepository.storeOidcCredentialDetails(sanitized, organisationId);
+ return record;This both enforces a valid org context and ensures you only persist the stripped-down data you actually need.
🤖 Prompt for AI Agents
In apps/oid4vc-issuance/src/oid4vc-issuance.service.ts around lines 963-1007,
the function builds a sanitized payload and computes organisationId but then
persists the original payload and can call storeOidcCredentialDetails with
organisationId undefined; update the code to (1) validate organisationId after
getOrganizationByTenantId and throw or return a controlled error if no orgId is
found (fail-fast), and (2) pass the sanitized object (which contains only
credential_configuration_ids and omits issuedCredentials/full blobs) into
this.oid4vcIssuanceRepository.storeOidcCredentialDetails instead of the original
CredentialOfferWebhookPayload so sensitive/large data aren’t stored.
| // UTILITY - Find expiring certificates for an org | ||
| async findExpiringCertificatesByOrg(orgId: string, days: number = 30): Promise<X509CertificateRecord[]> { | ||
| try { | ||
| const expiryDate = new Date(); | ||
| expiryDate.setDate(expiryDate.getDate() + days); | ||
|
|
||
| const certificates = await this.prisma.x509_certificates.findMany({ | ||
| where: { | ||
| org_agents: { | ||
| orgId | ||
| }, | ||
| expiry: { | ||
| lte: expiryDate | ||
| }, | ||
| status: 'Active' | ||
| }, | ||
| orderBy: { | ||
| expiry: 'asc' | ||
| } | ||
| }); | ||
|
|
||
| return certificates; | ||
| } catch (error) { | ||
| this.logger.error(`Error in findExpiringCertificatesByOrg: ${error.message}`); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| async getCurrentActiveCertificate(orgId: string): Promise<X509CertificateRecord> { | ||
| try { | ||
| const now = new Date(); | ||
|
|
||
| const certificate = await this.prisma.x509_certificates.findFirst({ | ||
| where: { | ||
| org_agents: { | ||
| orgId | ||
| }, | ||
| status: x5cRecordStatus.Active, | ||
| validFrom: { | ||
| lte: now | ||
| }, | ||
| expiry: { | ||
| gte: now | ||
| } | ||
| }, | ||
| orderBy: { | ||
| createdAt: 'desc' | ||
| } | ||
| }); | ||
|
|
||
| return certificate; | ||
| } catch (error) { | ||
| this.logger.error(`Error in getCurrentActiveCertificate: ${error.message}`); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the repository file and check its size
fd -e ts "x509.repository" -t fRepository: credebl/platform
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Check file size
wc -l apps/x509/src/repositories/x509.repository.tsRepository: credebl/platform
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Read the relevant section of the file
sed -n '240,310p' apps/x509/src/repositories/x509.repository.tsRepository: credebl/platform
Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Search for usages of getCurrentActiveCertificate
rg -n "getCurrentActiveCertificate" --type ts -A 3 -B 1Repository: credebl/platform
Length of output: 4169
🏁 Script executed:
#!/bin/bash
# Check usage context at oid4vc-verification.service.ts
sed -n '235,250p' apps/oid4vc-verification/src/oid4vc-verification.service.tsRepository: credebl/platform
Length of output: 658
🏁 Script executed:
#!/bin/bash
# Check usage at oid4vc-issuance.service.ts
sed -n '550,575p' apps/oid4vc-issuance/src/oid4vc-issuance.service.tsRepository: credebl/platform
Length of output: 1006
Based on my verification, the review comment is correct and valid. Here's what I found:
Verification Summary:
- ✅
getCurrentActiveCertificatedeclares return typePromise<X509CertificateRecord>(non-nullable) - ✅ The method uses
findFirst()which returnsT | null - ✅ No null-check or exception is thrown before returning
- ✅ All callers are explicitly null-checking despite the type saying non-nullable:
oid4vc-verification.service.tsline 244:if (!activeCertificate) { throw new NotFoundException(...) }oid4vc-issuance.service.tslines 559-561 and 575-577: same pattern
This pattern confirms the developers know the method can return null, but the TypeScript type signature doesn't reflect this reality. It's a genuine type safety violation.
Align getCurrentActiveCertificate return type with nullable findFirst result.
getCurrentActiveCertificate uses findFirst:
const certificate = await this.prisma.x509_certificates.findFirst({ ... });
return certificate;but declares:
async getCurrentActiveCertificate(orgId: string): Promise<X509CertificateRecord>Since findFirst can return null, the return type must be nullable. Evidence: all callers (oid4vc-verification.service.ts, oid4vc-issuance.service.ts) explicitly null-check the result, indicating the type signature is incorrect.
Either make the return type nullable:
- async getCurrentActiveCertificate(orgId: string): Promise<X509CertificateRecord> {
+ async getCurrentActiveCertificate(orgId: string): Promise<X509CertificateRecord | null> {or throw NotFoundException when certificate is falsy before returning.
🤖 Prompt for AI Agents
In apps/x509/src/repositories/x509.repository.ts around lines 248-303,
getCurrentActiveCertificate is declared to return Promise<X509CertificateRecord>
but uses prisma.findFirst which can return null; update the method to accurately
reflect nullability or enforce non-null before returning: either change the
signature to Promise<X509CertificateRecord | null> and keep returning the
findFirst result, or keep the current non-nullable signature and throw a
NotFoundException (or other appropriate error) when certificate is falsy before
returning; update any related typings/imports and adjust callers if you choose
the nullable signature.
| @@ -0,0 +1,43 @@ | |||
| # Stage 1: Build the application | |||
| FROM node:18-alpine as build | |||
There was a problem hiding this comment.
Use uppercase "AS" for Dockerfile multi-stage build syntax.
Docker best practices recommend using uppercase keywords for Dockerfile instructions, including the multi-stage build alias.
Apply this diff:
-FROM node:18-alpine as build
+FROM node:18-alpine AS build📝 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.
| FROM node:18-alpine as build | |
| FROM node:18-alpine AS build |
🤖 Prompt for AI Agents
In Dockerfiles/Dockerfile.oid4vc-issuance around line 2, the multi-stage build
alias uses lowercase "as"; change "as" to uppercase "AS" so the FROM instruction
reads with "AS" to follow Dockerfile keyword casing conventions and best
practices.
| # Copy package.json and package-lock.json | ||
| COPY package.json ./ | ||
| COPY pnpm-workspace.yaml ./ |
There was a problem hiding this comment.
Update comment to match the actual code.
The comment on line 9 references copying package-lock.json, but the code copies pnpm-workspace.yaml instead.
Apply this diff:
-# Copy package.json and package-lock.json
+# Copy package.json and pnpm-workspace.yaml📝 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.
| # Copy package.json and package-lock.json | |
| COPY package.json ./ | |
| COPY pnpm-workspace.yaml ./ | |
| # Copy package.json and pnpm-workspace.yaml | |
| COPY package.json ./ | |
| COPY pnpm-workspace.yaml ./ |
🤖 Prompt for AI Agents
In Dockerfiles/Dockerfile.oid4vc-issuance around lines 9 to 11, the comment says
"Copy package.json and package-lock.json" but the commands copy package.json and
pnpm-workspace.yaml; update the comment to accurately reflect the files being
copied (e.g., "Copy package.json and pnpm-workspace.yaml") so the comment
matches the actual COPY instructions.
| @@ -0,0 +1,43 @@ | |||
| # Stage 1: Build the application | |||
| FROM node:18-alpine as build | |||
There was a problem hiding this comment.
Use uppercase "AS" for Dockerfile multi-stage build syntax.
Docker best practices and linters recommend using uppercase keywords for Dockerfile instructions, including the multi-stage build alias.
Apply this diff:
-FROM node:18-alpine as build
+FROM node:18-alpine AS build📝 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.
| FROM node:18-alpine as build | |
| FROM node:18-alpine AS build |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 2-2: Replace "as" with upper case format "AS".
🤖 Prompt for AI Agents
In Dockerfiles/Dockerfile.x509 around line 2, the multi-stage build alias uses
lowercase "as" ("FROM node:18-alpine as build"); update it to use uppercase "AS"
to conform with Dockerfile syntax conventions and linters by changing the
instruction to "FROM node:18-alpine AS build".
| @@ -0,0 +1,58 @@ | |||
| const customInspectSymbol = Symbol.for('nodejs.util.inspect.custom'); | |||
| export class DateOnly { | |||
| private date: Date; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make the date field readonly.
The date field is never reassigned after construction, so marking it as readonly enforces immutability and clarifies the design intent.
Apply this diff:
- private date: Date;
+ private readonly date: Date;📝 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.
| private date: Date; | |
| private readonly date: Date; |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 3-3: Member 'date' is never reassigned; mark it as readonly.
🤖 Prompt for AI Agents
In libs/common/src/date-only.ts around line 3, the private field declaration
"private date: Date;" should be made readonly since it's only assigned in the
constructor and never reassigned; update the declaration to "private readonly
date: Date;" so the field is immutable and signals intent.
* feat: dcapi support Signed-off-by: Tipu_Singh <[email protected]> * feat: added id validation in create presentation Signed-off-by: Tipu_Singh <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (2)
303-313: DecouplerequestSignerfrom PrismaSignerOptionand align with domain type
requestSigner.methodis typed asSignerOptionfrom@prisma/client, while there is also a domain-levelSignerOptioninterface inapps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.tswith a different shape. This both couples the DTO to the persistence layer and reuses the same name for different concepts, which is confusing.Prefer a DTO/domain type here (e.g., a method enum or the domain “method” type) instead of importing from Prisma, so the API contract is independent of DB details and naming collisions are avoided.
345-356: Use only@ApiHidePropertyon_oneOfCheckto keep it internalYou’ve added
@ApiHideProperty()(good) but left@ApiPropertyOptionalon_oneOfCheck. Keeping the Swagger decorator is unnecessary and risks conflicting metadata for a field that should remain internal.You can safely drop
@ApiPropertyOptionaland rely solely on@ApiHidePropertyplus the comment above for documentation:- @ApiHideProperty() - @ApiPropertyOptional({ - description: 'Internal: ensures exactly one of dcql or presentationExchange is present' - }) + @ApiHideProperty() @Validate(OnlyOneOfConstraint, ['dcql', 'presentationExchange'])This preserves validation behavior while ensuring
_oneOfCheckdoes not appear in the OpenAPI schema.
🧹 Nitpick comments (3)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (2)
1-6: Consolidate@nestjs/swaggerimports to a single statement
@nestjs/swaggeris imported twice (lines 2 and 5). You can merge them into one import for clarity and to satisfy the Sonar warning.-import { ApiHideProperty, ApiPropertyOptional } from '@nestjs/swagger'; -/* eslint-disable camelcase */ -import { ApiProperty } from '@nestjs/swagger'; +import { ApiHideProperty, ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; +/* eslint-disable camelcase */
333-343: Tighten validation forexpectedOriginsas an array of strings
expectedOriginsis annotated only with@IsOptional(). If this is intended to bestring[], consider adding explicit array + element validation for safer inputs:- @IsOptional() - expectedOrigins: string[]; + @IsOptional() + @IsArray() + @IsString({ each: true }) + expectedOrigins: string[];This will ensure the field is actually an array of strings when provided.
libs/enum/src/enum.ts (1)
275-339: Unify x5c key/status enums with existing patterns and normalize status stringsThe new enums look conceptually sound, but a couple of details are worth tightening:
x5cKeyTypeduplicates values already present inKeyType(Ed25519,P256). Unless there is a strong reason to keep them separate, consider reusingKeyType(or a narrowed union of it) instead of introducing a parallel enum that can drift over time.x5cRecordStatusmixes capitalization and spaces in its string values ('Active','Pending activation','In Active'). This is inconsistent with most other enums here that use machine‑friendly tokens (e.g., kebab/snake case, no spaces). Consider normalizing the values (e.g.,'active' | 'pending_activation' | 'inactive') and deriving user‑facing labels at the UI layer.These are non‑blocking but will reduce surprises around comparisons, persistence, and future extensions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts(1 hunks)libs/enum/src/enum.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (1)
apps/oid4vc-issuance/interfaces/oid4vc-issuer-sessions.interfaces.ts (1)
SignerOption(16-20)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts
[warning] 2-2: '@nestjs/swagger' imported multiple times.
🔇 Additional comments (1)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts (1)
80-272: PEX/DCQL DTO modeling and validation look consistentThe PEX and DCQL DTOs are well-structured: required/optional fields align with examples, array and nested validations are wired correctly, and the shapes match the expected OID4VC/DCQL structures. No changes needed here from this file’s perspective.
| export declare enum HandshakeProtocol { | ||
| Connections = 'https://didcomm.org/connections/1.0', | ||
| DidExchange = 'https://didcomm.org/didexchange/1.0' | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all definitions and usages of HandshakeProtocol
rg -n "HandshakeProtocol" -C3Repository: credebl/platform
Length of output: 2065
🏁 Script executed:
#!/bin/bash
# Look for runtime access patterns of HandshakeProtocol enum members
rg -n "HandshakeProtocol\." -C2Repository: credebl/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for validation or factory patterns that might use HandshakeProtocol values
rg -n "Connections|DidExchange" -C2 --type tsRepository: credebl/platform
Length of output: 31901
export declare enum HandshakeProtocol will not emit a runtime enum (likely runtime bug)
Using declare here makes HandshakeProtocol ambient-only: TypeScript will not emit any JS for it, but code that imports and uses HandshakeProtocol.Connections / HandshakeProtocol.DidExchange will still reference the enum at runtime, yielding undefined and causing failures.
In this source file you almost certainly want a normal exported enum:
-export declare enum HandshakeProtocol {
+export enum HandshakeProtocol {
Connections = 'https://didcomm.org/connections/1.0',
DidExchange = 'https://didcomm.org/didexchange/1.0'
}Additionally, there is a duplicate definition of HandshakeProtocol in apps/api-gateway/src/cloud-wallet/enums/connections.enum.ts that also uses export declare enum. Remove this duplicate and use only the definition in libs/enum/src/enum.ts.
📝 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.
| export declare enum HandshakeProtocol { | |
| Connections = 'https://didcomm.org/connections/1.0', | |
| DidExchange = 'https://didcomm.org/didexchange/1.0' | |
| } | |
| export enum HandshakeProtocol { | |
| Connections = 'https://didcomm.org/connections/1.0', | |
| DidExchange = 'https://didcomm.org/didexchange/1.0' | |
| } |
🤖 Prompt for AI Agents
In libs/enum/src/enum.ts around lines 341 to 344, the enum is declared with
"export declare enum HandshakeProtocol" which prevents TypeScript from emitting
runtime code and will make HandshakeProtocol.Connections/DidExchange undefined
at runtime; change it to a normal exported enum (remove "declare") so the enum
is emitted into JS, and delete the duplicate enum definition in
apps/api-gateway/src/cloud-wallet/enums/connections.enum.ts (or replace its
imports to reference libs/enum) so there is a single canonical runtime-emitted
HandshakeProtocol definition.
What?
Below features are added for experimental purpose. Functionalities are working but may have some issues
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.