-
Notifications
You must be signed in to change notification settings - Fork 11
fix(dashboard): prevent JSON.parse crashes in credentials #1275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(dashboard): prevent JSON.parse crashes in credentials #1275
Conversation
Add safeParse utility to safely parse credential.public_notes. Prevents component crashes from malformed JSON data.
|
@dmrdvn is attempting to deploy a commit to the idOS Engineering Team on Vercel. A member of the Team first needs to authorize it. |
@idos-network/client
@idos-network/consumer
@idos-network/credentials
@idos-network/issuer
@idos-network/utils
commit: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ditoglez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dmrdvn for your contribution!
📝 WalkthroughWalkthroughA utility function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/delete-credential.tsx (1)
162-203: Provide display fallbacks whenmetais empty.When parsing fails,
meta.type/meta.issuerare undefined, leading to blank dialog text. Add defaults to keep the message readable.✅ Suggested tweak
- const meta = safeParse<{ type?: string; issuer?: string }>(credential.public_notes); + const meta = safeParse<{ type?: string; issuer?: string }>(credential.public_notes); + const typeLabel = meta.type ?? "credential"; + const issuerLabel = meta.issuer ?? "unknown"; @@ - Deleting credential of type{" "} + Deleting credential of type{" "} <Text as="span" color="green.200" fontWeight="semibold"> - {meta.type} + {typeLabel} </Text>{" "} from issuer{" "} <Text as="span" color="green.200" fontWeight="semibold"> - {meta.issuer} + {issuerLabel} </Text>
🤖 Fix all issues with AI agents
In `@apps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts`:
- Around line 7-18: safeParse currently returns results like null or arrays when
JSON.parse succeeds but the value isn't a plain object (e.g., "null"), which
breaks callers expecting an object; after calling JSON.parse in safeParse, check
the parsed value (in function safeParse) and if it's not a non-null plain object
(typeof parsed === "object" && parsed !== null && !Array.isArray(parsed)),
return {} as T instead, preserving the existing try/catch fallback for parse
errors.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/credential-card.tsxapps/idos-data-dashboard/src/routes/dashboard/credentials/components/credential-details.tsxapps/idos-data-dashboard/src/routes/dashboard/credentials/components/delete-credential.tsxapps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/credential-card.tsx (1)
apps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts (1)
safeParse(13-19)
apps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts (2)
apps/dashboard-for-dapps/src/routes/credentials.tsx (1)
safeParse(56-62)packages/issuer/src/services/credential.service.ts (1)
T(115-120)
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/delete-credential.tsx (1)
apps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts (1)
safeParse(13-19)
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/credential-details.tsx (1)
apps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts (1)
safeParse(13-19)
🔇 Additional comments (2)
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/credential-details.tsx (1)
21-85: Good switch tosafeParseforpublic_notes.Prevents runtime crashes on malformed JSON while keeping typed access.
apps/idos-data-dashboard/src/routes/dashboard/credentials/components/credential-card.tsx (1)
5-21: Nice safety improvement by usingsafeParse.Avoids crashes from malformed
public_noteswhile keeping existing behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /** | ||
| * Safely parses JSON string with fallback to empty object | ||
| * @param json - JSON string to parse (can be null or undefined) | ||
| * @returns Parsed object or empty object if parsing fails | ||
| * @template T - Expected return type (defaults to Record<string, unknown>) | ||
| */ | ||
| export const safeParse = <T = Record<string, unknown>>(json?: string | null): T => { | ||
| try { | ||
| return JSON.parse(json ?? "{}") as T; | ||
| } catch (_e) { | ||
| return {} as T; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle valid JSON that isn’t an object (e.g., "null").
Right now JSON.parse("null") yields null and won’t hit the catch, so safeParse can still return null and crash Object.entries(...) / property access at call sites. Add a post-parse shape check and fall back to {} when the parsed value isn’t a plain object.
🛠️ Suggested fix
export const safeParse = <T = Record<string, unknown>>(json?: string | null): T => {
try {
- return JSON.parse(json ?? "{}") as T;
+ const parsed = JSON.parse(json ?? "{}");
+ if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) {
+ return parsed as T;
+ }
} catch (_e) {
- return {} as T;
}
+ return {} as T;
};🤖 Prompt for AI Agents
In `@apps/idos-data-dashboard/src/routes/dashboard/credentials/shared/index.ts`
around lines 7 - 18, safeParse currently returns results like null or arrays
when JSON.parse succeeds but the value isn't a plain object (e.g., "null"),
which breaks callers expecting an object; after calling JSON.parse in safeParse,
check the parsed value (in function safeParse) and if it's not a non-null plain
object (typeof parsed === "object" && parsed !== null &&
!Array.isArray(parsed)), return {} as T instead, preserving the existing
try/catch fallback for parse errors.
🐛 Bug Fix
Problem
Credential components crashed when
public_notesfield contained:nullorundefinedvaluesAffected components:
Solution
safeParseutility function inshared/index.tsJSON.parse()callsChanges
shared/index.ts- New safeParse utilitycredential-card.tsx- Import safeParsecredential-details.tsx- Import safeParse with typesdelete-credential.tsx- Import safeParse with typesTesting
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.