Fix mdoc fallback field assembly in openId4VCI Utils - #2397
Conversation
Signed-off-by: Chandra Keshav Mishra <chandrakeshavmishra@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughBug fix in credential issuer utilities where field accumulation logic was corrected from a no-op Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
654c8c2 to
f391f10
Compare
abhip2565
left a comment
abhip2565
left a comment
There was a problem hiding this comment.
Can we remove unrelated formatiing changes?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/openId4VCI/Utils.ts (1)
162-170:⚠️ Potential issue | 🟠 MajorBuild mdoc fallback fields locally before replacing defaults.
Line 163 clears
fieldsbefore claim parsing succeeds, so a missing/malformedclaimsobject makes the catch return[]instead of the promised default fields. This branch also leaveswellknownFieldsFlagfalse even when fields are derived from well-known metadata.🐛 Proposed fix
if (format === VCFormat.mso_mdoc) { - fields = []; - Object.keys(matchingWellknownDetails.claims).forEach(namespace => { - Object.keys(matchingWellknownDetails.claims[namespace]).forEach( - claim => { - fields = fields.concat(`${namespace}~${claim}`); - }, - ); - }); + const claims = matchingWellknownDetails.claims ?? {}; + const mdocFields = Object.keys(claims).flatMap(namespace => + Object.keys(claims[namespace] ?? {}).map( + claim => `${namespace}~${claim}`, + ), + ); + + if (mdocFields.length > 0) { + fields = mdocFields; + wellknownFieldsFlag = true; + } } else if (format === VCFormat.ldp_vc) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/openId4VCI/Utils.ts` around lines 162 - 170, The code clears the shared variable fields at the start of the VCFormat.mso_mdoc branch which causes an empty array to be returned if matchingWellknownDetails.claims is missing/malformed and also never sets wellknownFieldsFlag when fields are successfully derived; fix by building a local fallback array (e.g., localMdocFields) from matchingWellknownDetails.claims inside a try block (iterating namespace and claim as currently done), only assign it to the outer fields when parsing succeeds, and set wellknownFieldsFlag = true when you assign those derived values; ensure you do not mutate fields until parsing completes and handle missing/malformed matchingWellknownDetails.claims by leaving the default fields intact.
🧹 Nitpick comments (1)
shared/openId4VCI/Utils.test.ts (1)
265-272: Assert the well-known flag in the mdoc fallback regression.This test now protects the field list, but it should also lock down that callers can recognize these fields as well-known-derived.
🧪 Proposed test assertion
expect(result.fields).toEqual( expect.arrayContaining([ 'org.iso.18013.5.1~family_name', 'org.iso.18013.5.1~given_name', ]), ); expect(result.fields).toHaveLength(2); + expect(result.wellknownFieldsFlag).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/openId4VCI/Utils.test.ts` around lines 265 - 272, The test currently locks the field list via result.fields but doesn't assert that those fields are marked as well-known; add an assertion that the same field identifiers are present in the well-known flag collection (e.g. expect(result.wellKnown).toEqual(expect.arrayContaining(['org.iso.18013.5.1~family_name','org.iso.18013.5.1~given_name'])) and also assert its length (e.g. expect(result.wellKnown).toHaveLength(2)) so callers can detect these as well-known-derived fields alongside the existing result.fields checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shared/openId4VCI/Utils.ts`:
- Around line 162-170: The code clears the shared variable fields at the start
of the VCFormat.mso_mdoc branch which causes an empty array to be returned if
matchingWellknownDetails.claims is missing/malformed and also never sets
wellknownFieldsFlag when fields are successfully derived; fix by building a
local fallback array (e.g., localMdocFields) from
matchingWellknownDetails.claims inside a try block (iterating namespace and
claim as currently done), only assign it to the outer fields when parsing
succeeds, and set wellknownFieldsFlag = true when you assign those derived
values; ensure you do not mutate fields until parsing completes and handle
missing/malformed matchingWellknownDetails.claims by leaving the default fields
intact.
---
Nitpick comments:
In `@shared/openId4VCI/Utils.test.ts`:
- Around line 265-272: The test currently locks the field list via result.fields
but doesn't assert that those fields are marked as well-known; add an assertion
that the same field identifiers are present in the well-known flag collection
(e.g.
expect(result.wellKnown).toEqual(expect.arrayContaining(['org.iso.18013.5.1~family_name','org.iso.18013.5.1~given_name']))
and also assert its length (e.g. expect(result.wellKnown).toHaveLength(2)) so
callers can detect these as well-known-derived fields alongside the existing
result.fields checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8aaf672-3064-4440-8574-ce195fb5d5c1
📒 Files selected for processing (2)
shared/openId4VCI/Utils.test.tsshared/openId4VCI/Utils.ts
Signed-off-by: dhruv1955 <dhruvyadav042905@gmail.com>
Signed-off-by: dhruv1955 <dhruvyadav042905@gmail.com>
Signed-off-by: dhruv1955 <dhruvyadav042905@gmail.com>
91efcca to
65b33fd
Compare
|
Thanks for the review @abhip2565! Removed all unrelated formatting changes - the PR now contains only:
|
Thanks. Did we do any dev testing to verify the scenario? If yes can we attach relevant video(s)? |
|
@abhip2565 Here's a screenshot demonstrating the bug and fix. Before the fix - fields.concat(...) result was not stored, so fields stays empty []. After the fix - fields = fields.concat(...) correctly populates the derived mdoc field paths. 
|
abhip2565
left a comment
abhip2565
left a comment
There was a problem hiding this comment.
LGTM. @sanchi-singh24 @swatigoel Thinking should we verify once, the scenario (wellknown w/o order), in vc detail view... to catch any other errors upstream which can be fixed together for this edge case scenario,
|
@dhruv1955 can you raise this PR for develop branch? |
4 participants
What and Why
Array.concat()returns a new array - the result wasn't being stored, so derived mdoc fallback fields were silently dropped before reaching the UI. This broke the detail view for mdoc credentials missing issuer-defined ordering.Changes
Utils.ts- store the result offields.concat(...)back intofieldsUtils.test.ts- regression test for the mdoc fallback pathValidation
npx tsc --noEmitnpx eslinton both files - warnings only, no errorsSummary by CodeRabbit
Bug Fixes
Tests