fix: allowed any dcql query support, fixed nested attribute's disclos…#1539
fix: allowed any dcql query support, fixed nested attribute's disclos…#1539RinkalBhojani merged 2 commits intomainfrom
Conversation
…ure frame Signed-off-by: Rinkal Bhojani <rinkal.bhojani@ayanworks.com>
📝 WalkthroughWalkthroughA DCQL verification DTO relaxes its Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 2
🧹 Nitpick comments (1)
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (1)
497-497: Clarify: Comment contradicts the code.The comment states "Pass undefined to enable no auth implementation," but the code passes
DEFAULT_TXCODE(a defined object with description, length, and input_mode).If this is a TODO for future functionality, please rephrase to clarify: "TODO: Support passing undefined to enable no-auth implementation. Currently uses DEFAULT_TXCODE."
If undefined should be passed now, update the code to match the comment.
🔎 Proposed clarification
- txCode: DEFAULT_TXCODE, // Pass undefined to enable no auth implementation, TODO: Need to make it configuarble. + txCode: DEFAULT_TXCODE // TODO: Make txCode configurable; support undefined to enable no-auth implementation
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.tsapps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts
🔇 Additional comments (1)
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (1)
228-259: Approve: Disclosure frame refactoring correctly handles nested attributes.The refactored logic properly distinguishes between:
- Simple disclosed claims (no children): Added to the
_sdarray- Objects with disclosed children: Nested
childFrameattached to the parent keyThis addresses the PR objective of fixing "nested attribute's disclosure frame" and aligns with SD-JWT selective disclosure semantics, where parent structure is implicitly revealed when children are selectively disclosed.
Note: When an attribute has both
disclose: trueand children (line 242-245), only the child disclosures are processed. This is correct behavior for SD-JWT, as you cannot selectively disclose a parent object independently of its children.
| //@ApiProperty({ type: DcqlQueryDto }) | ||
| @IsDefined() | ||
| @ValidateNested() | ||
| @Type(() => DcqlQueryDto) | ||
| query: DcqlQueryDto; | ||
| //@ValidateNested() | ||
| //@Type(() => DcqlQueryDto) | ||
| query: unknown; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure and locate the file
echo "=== Repository structure ==="
ls -la apps/ 2>/dev/null | head -20
echo -e "\n=== File location check ==="
find . -name "*oid4vc-verifier-presentation.dto.ts" -type f 2>/dev/null
echo -e "\n=== Directory structure for api-gateway ==="
ls -la apps/api-gateway/src/oid4vc-verification/dtos/ 2>/dev/null || echo "Directory not found"Repository: credebl/platform
Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Let's read the specific file to understand the context
echo "=== Reading oid4vc-verifier-presentation.dto.ts ==="
cat -n apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts | sed -n '260,280p'
echo -e "\n=== Full file for context ==="
wc -l apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.tsRepository: credebl/platform
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Search for DcqlQueryDto and related types
echo "=== Search for DcqlQueryDto definition ==="
rg -n --type=ts 'export.*DcqlQueryDto|interface DcqlQueryDto|class DcqlQueryDto'
echo -e "\n=== Search for DcqlDto usage ==="
rg -n --type=ts 'DcqlQueryDto|dcql-query.dto'Repository: credebl/platform
Length of output: 607
🏁 Script executed:
#!/bin/bash
# Search for where the query property is accessed
echo "=== Direct query property access patterns ==="
rg -n --type=ts '\bquery\b' apps/api-gateway/src/oid4vc-verification/ | head -40
echo -e "\n=== Search for imports from the presentation DTO ==="
rg -n --type=ts 'import.*oid4vc-verifier-presentation'
echo -e "\n=== Search for usages of the DTO ==="
rg -n --type=ts 'OidD4VcVerifierPresentationDto|OidD4VcVerifierPresentation'Repository: credebl/platform
Length of output: 1826
🏁 Script executed:
#!/bin/bash
# Read the full file to understand context
cat -n apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.tsRepository: credebl/platform
Length of output: 11588
🏁 Script executed:
#!/bin/bash
# Search for DcqlDto usage (not VerificationPresentationQueryDto)
echo "=== Direct DcqlDto usages ==="
rg -n --type=ts 'DcqlDto'
echo -e "\n=== VerificationPresentationQueryDto context ==="
rg -n --type=ts -B3 -A5 'VerificationPresentationQueryDto' apps/api-gateway/src/oid4vc-verification/Repository: credebl/platform
Length of output: 6613
🏁 Script executed:
#!/bin/bash
# Look at the service to see how queries are processed
echo "=== oid4vc-verification.service.ts - check for query handling ==="
cat -n apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts | head -150Repository: credebl/platform
Length of output: 7643
🏁 Script executed:
#!/bin/bash
# Search for dcql processing logic
echo "=== Search for dcql handling in service ==="
rg -n --type=ts -i 'dcql' apps/api-gateway/src/oid4vc-verification/ -A3 -B1Repository: credebl/platform
Length of output: 7550
🏁 Script executed:
#!/bin/bash
# Search for where dcql is processed after it's sent
echo "=== Search for DCQL processing in microservices or handlers ==="
find apps -type f -name "*.ts" | xargs grep -l "dcql" | grep -E "(microservice|handler|processor)" | head -10
echo -e "\n=== Look for DCQL validation in oid4vc-verification microservice ==="
ls -la apps/oid4vc-verification/src/ 2>/dev/null || echo "Not found in oid4vc-verification"
echo -e "\n=== Search across all DCQL handlers ==="
rg -n --type=ts -i 'dcql.*query|query.*dcql' apps/ --max-count=20
echo -e "\n=== Check where dcql is used in PresentationRequestDto ==="
rg -n --type=ts -B5 -A10 'dcql\?: DcqlDto' apps/Repository: credebl/platform
Length of output: 3343
🏁 Script executed:
#!/bin/bash
# Look at the interface definition to understand how dcql is treated
echo "=== Interface definition for dcql ==="
cat -n libs/common/src/interfaces/oid4vp-verification.ts | head -60
echo -e "\n=== Search for where IPresentationRequest is used ==="
rg -n --type=ts 'IPresentationRequest' apps/api-gateway/ -B2 -A2Repository: credebl/platform
Length of output: 3311
🏁 Script executed:
#!/bin/bash
# Check if there's any runtime validation logic in the service
echo "=== Search for custom validation in oid4vc-verification service ==="
rg -n --type=ts 'ValidateNested|Validate\(' apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts
echo -e "\n=== Check for any schema validation libraries ==="
rg -n --type=ts -i 'zod|joi|schema|validate' apps/api-gateway/src/oid4vc-verification/ | grep -v node_modules | head -20Repository: credebl/platform
Length of output: 2387
🏁 Script executed:
#!/bin/bash
# Check the controller endpoint that uses PresentationRequestDto
echo "=== Controller endpoint using PresentationRequestDto ==="
rg -n --type=ts -B10 -A15 'PresentationRequestDto' apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts | head -80Repository: credebl/platform
Length of output: 2598
Remove unknown type from DcqlDto.query and restore validation decorators.
The query property lacks runtime type validation and transformation. While the parent PresentationRequestDto validates that dcql is a DcqlDto instance, the inner query field accepts any structure without schema validation. This allows malformed or unexpected query structures to propagate to downstream microservices that may not safely handle untyped inputs.
Restore the commented decorators (@ValidateNested(), @Type(() => DcqlQueryDto), @ApiProperty()) to enforce the expected query schema, or if the DCQL specification genuinely requires flexible query structures, replace unknown with a union type defining valid alternatives and add explicit schema validation at the service layer.
🤖 Prompt for AI Agents
In
apps/api-gateway/src/oid4vc-verification/dtos/oid4vc-verifier-presentation.dto.ts
around lines 268-272, the query property is typed as unknown and the
validation/transformation decorators were commented out; restore strong typing
and validation by changing the property type to DcqlQueryDto (or an explicit
union of allowed query shapes if DCQL requires flexibility) and re-enable
@ApiProperty({ type: DcqlQueryDto }), @ValidateNested(), and @Type(() =>
DcqlQueryDto) so class-transformer/class-validator will validate and transform
the nested query object at runtime; if you intentionally need flexible
structures instead, replace unknown with a documented union and add explicit
validation logic in the service layer.
| preAuthorizedCodeFlowConfig: | ||
| | { | ||
| txCode: { description?: string; length: number; input_mode: 'numeric' | 'text' | 'alphanumeric' }; | ||
| authorizationServerUrl: string; | ||
| } | ||
| | undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the context around the specified lines
find . -name "credential-sessions.builder.ts" -type fRepository: credebl/platform
Length of output: 126
🏁 Script executed:
# Read the file to see the context around lines 98-103 and understand the type definition
cat -n ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts | head -150Repository: credebl/platform
Length of output: 6273
🏁 Script executed:
# Check line 512 and surrounding context
sed -n '505,520p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsRepository: credebl/platform
Length of output: 552
🏁 Script executed:
# Check more context around line 512, especially where hasPreAuthFromDto is set
sed -n '495,525p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsRepository: credebl/platform
Length of output: 948
🏁 Script executed:
# Look at the full context of this function to understand what's being returned
sed -n '470,525p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsRepository: credebl/platform
Length of output: 2258
🏁 Script executed:
# Search for all usages of CredentialOfferPayload to see how consumers handle it
rg -n 'CredentialOfferPayload' --type=ts -A 3 -B 3Repository: credebl/platform
Length of output: 9186
🏁 Script executed:
# Search for usages of preAuthorizedCodeFlowConfig to see how it's accessed
rg -n 'preAuthorizedCodeFlowConfig' --type=ts -B 2 -A 2Repository: credebl/platform
Length of output: 5899
🏁 Script executed:
# Check the entire CredentialOfferPayload usage pattern and understand the intent
sed -n '480,525p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsRepository: credebl/platform
Length of output: 1733
🏁 Script executed:
# Look at the beginning of the buildCredentialOfferPayload function to understand full context
sed -n '436,460p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsRepository: credebl/platform
Length of output: 1148
🏁 Script executed:
# Check if there are any other code paths that could return preAuthorizedCodeFlowConfig as undefined
sed -n '436,525p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts | grep -n "return"Repository: credebl/platform
Length of output: 506
🏁 Script executed:
# Check if there's any comment or documentation about why undefined is allowed
sed -n '89,115p' ./apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.tsRepository: credebl/platform
Length of output: 834
Remove | undefined from preAuthorizedCodeFlowConfig type.
The discriminated union structure enforces XOR semantics: exactly one of preAuthorizedCodeFlowConfig or authorizationCodeFlowConfig must be present. However, the type allows preAuthorizedCodeFlowConfig: undefined | { ... }, which contradicts this contract—if it's undefined, neither config is actually present, violating the XOR guarantee.
The function implementation never returns undefined for this field when selected; all code paths return a defined object. The | undefined should be removed from line 103 to align the type with actual semantics and implementation.
🤖 Prompt for AI Agents
In apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts around lines
98 to 103, the type for preAuthorizedCodeFlowConfig currently includes "|
undefined" which breaks the intended XOR/discriminated-union guarantee; remove
the "| undefined" from that field's type so it is strictly the object type
(txCode/authorizationServerUrl) when present, and ensure any callers rely on the
union to express absence via the alternate discriminant rather than an explicit
undefined.
Signed-off-by: Rinkal Bhojani <rinkal.bhojani@ayanworks.com>
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts (1)
226-257: Critical logic bug: disclosed parent attributes with non-disclosed children are lost.The refactored logic has a flaw when handling attributes that are marked for disclosure but have children that are NOT disclosed:
Scenario:
{ key: 'address', disclose: true, children: [ { key: 'street', disclose: false }, { key: 'city', disclose: false } ] }Execution trace:
- Line 231-232:
childFrameis built recursively → returns{}(empty object, since children havedisclose: false)- Line 234-235:
hasChildDisclosureevaluates tofalse(no_sdarray, no keys other than_sd)- Line 238-243:
attr.discloseistrue, butattr.childrenexists, so the key is NOT added tosdarray- Line 247-249:
hasChildDisclosureisfalse, so child frame is NOT attached- Result: The
addressattribute vanishes entirely from the disclosure frameThis violates the disclosure contract—a field marked
disclose: trueshould appear in the frame.🔎 Proposed fix
// Case 1: this attribute itself is disclosed if (attr.disclose) { // If it has children, children are handled separately if (!attr.children || 0 === attr.children.length) { sd.push(attr.key); continue; } + // Parent is disclosed and has children - add parent to sd if children are not disclosed + if (!hasChildDisclosure) { + sd.push(attr.key); + continue; + } } // Case 2: attribute has disclosed children if (hasChildDisclosure) { frame[attr.key] = childFrame!; }This ensures that:
- Disclosed attributes without children → added to
sd- Disclosed attributes with non-disclosed children → added to
sd(parent as a whole is disclosed)- Non-disclosed attributes with disclosed children → child frame attached (selective disclosure within)
- Disclosed attributes with disclosed children → child frame attached (nested selective disclosure)
| preAuthorizedCodeFlowConfig: { | ||
| txCode: { description?: string; length: number; input_mode: 'numeric' | 'text' | 'alphanumeric' }; | ||
| authorizationServerUrl: string; | ||
| txCode: { description?: string; length: number; input_mode: 'numeric' | 'text' | 'alphanumeric' } | undefined; | ||
| authorizationServerUrl?: string; | ||
| }; | ||
| authorizationCodeFlowConfig?: never; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to see the actual type definitions
cat -n apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts | head -120 | tail -30Repository: credebl/platform
Length of output: 1186
🏁 Script executed:
# Search for preAuthorizedCodeFlowConfig usage
rg -n 'preAuthorizedCodeFlowConfig' --type=ts -C 2Repository: credebl/platform
Length of output: 5938
🏁 Script executed:
# Search for authorizationServerUrl usage
rg -n 'authorizationServerUrl' --type=ts -C 2Repository: credebl/platform
Length of output: 9362
Type definition loosens constraints from input interface, creating inconsistency between contract and implementation.
The input interface CreateOidcCredentialOfferDtoLike (lines 51-60) requires both fields:
txCode: { description?: string; length: number; input_mode: 'numeric' | 'text' | 'alphanumeric' };
authorizationServerUrl: string;
But the output type CredentialOfferPayload (lines 98-103) makes both optional:
txCode: ... | undefined;
authorizationServerUrl?: string;
This type mismatch is problematic because:
-
Actual usage contradicts the permissive type: Line 582 uses
authorizationServerUrldirectly in a template string (${authorizationServerUrl}/oid4vci/${publicIssuerId}) without null checks. If undefined, this silently produces invalid URLs. -
Misleading comment on line 495: The comment says "Pass undefined to enable no auth implementation" but the code passes
DEFAULT_TXCODE(a concrete object), not undefined. This inconsistency between comment and code suggests confusion about the intended behavior. -
Implementation compensates with guards: While the actual implementation uses non-null assertions (line 510) and validation checks (lines 488-491) to prevent undefined values at runtime, the type definition should accurately reflect these constraints rather than allowing a state the code doesn't actually handle.
Tighten the type definition to match actual requirements, and correct the misleading comment.
🤖 Prompt for AI Agents
In apps/oid4vc-issuance/libs/helpers/credential-sessions.builder.ts around lines
98-103, tighten the CredentialOfferPayload type to match the input DTO and
runtime checks: make preAuthorizedCodeFlowConfig.txCode required (remove the |
undefined) and make authorizationServerUrl a mandatory string (remove the ?), so
the type reflects that these values must be present; also update the misleading
comment near line 495 to state that DEFAULT_TXCODE is passed when using the
no-auth path (not undefined), and adjust any nearby type annotations or callers
that relied on optionality to accept the now-required properties.
2 participants
What?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.