Conversation
TaylorBeeston
requested review from
Custard7,
gerardopar,
goblincore,
rhen92 and
smurflo2
🦋 Changeset detectedLatest commit: 7b62ad8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for staging-learncardapp canceled.
|
✅ Deploy Preview for learncarddocs canceled.
|
![gitstream-cm[bot]](https://avatars.githubusercontent.com/in/230441?s=60&v=4){kind=small}
There was a problem hiding this comment.
✨ PR Review
The PR adds comprehensive app DID support for app store listings. The implementation spans multiple layers (API, service, UI) and appears well-structured. However, there are a few areas where error handling could be improved and where more robust DID validation should be used.
3 issues detected:
🐞 Bug - Silent error handling in ensureAppSigningAuthority can leave listings in a broken state where they cannot issue credentials
Details: The ensureAppSigningAuthority function catches all errors and only logs warnings, allowing listings to be created/updated without signing authorities. This will cause credential issuance to fail later when the app attempts to issue credentials, resulting in a broken state that's difficult to debug.
File:apps/learn-card-app/src/pages/appStoreDeveloper/useDeveloperPortal.ts🐞 Bug - Simple string matching for DID validation could produce false positives
Details: The check for app DIDs uses a simple string includes check (issuerDid?.includes(':app:')) rather than the proper isAppDidWeb helper function. This could produce false positives if a regular DID happens to contain ':app:' in its method-specific identifier. The same issue exists in VC2FrontFaceInfo.tsx, MeritBadgeFrontFace.tsx, and VCIDDisplayFrontFace.tsx.
File:packages/react-learn-card/src/components/CertificateFrontFace/CertificateFrontFace.tsx🧾 Readability - Debug console.log statements left in production code 🛠️
Details: Debug console.log statements appear to have been left in the production code. These should be removed or converted to proper logging using the request logger.
File:services/learn-card-network/brain-service/src/routes/app-store.ts (1298-1299)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| console.log('integration', integration, isOwner); | ||
| console.log('listingId', listingId); |
There was a problem hiding this comment.
🧾 Readability - Debug Code: Remove the console.log statements at lines 1298-1299, or replace them with proper logging using ctx.log or request.log if they're needed for debugging purposes.
| console.log('integration', integration, isOwner); | |
| console.log('listingId', listingId); |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
There was a problem hiding this comment.
✨ PR Review
The PR introduces comprehensive app DID support with proper slug generation, credential issuance flow, and DID document resolution. However, several issues from previous reviews remain unresolved, and the app DID check is inconsistently implemented across react-learn-card components.
1 issues detected:
🐞 Bug - Using string includes for DID pattern matching is fragile and could produce false positives, while a proper helper function exists and is already imported in some files. 🛠️
Details: The code uses a simple string includes check (issuerDid?.includes(':app:')) to detect app DIDs, but the proper isAppDidWeb helper function exists and is used elsewhere (e.g., CredentialVerificationDisplay.tsx). The includes check could produce false positives if a regular DID happens to contain ':app:' in its method-specific identifier. The same issue exists in MeritBadgeFrontFace.tsx (line 108), VC2FrontFaceInfo.tsx (line 90), and VCIDDisplayFrontFace.tsx (line 57).
File:packages/react-learn-card/src/components/CertificateDisplayCard/CertificateFrontFace.tsx (107-107)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
|
||
| const issuerDid = | ||
| typeof credential.issuer === 'string' ? credential.issuer : credential.issuer.id; | ||
| const isAppIssuerDid = issuerDid?.includes(':app:'); |
There was a problem hiding this comment.
🐞 Bug - Improper DID Check: Import and use the isAppDidWeb helper function from '@learncard/helpers' or 'learn-card-base/helpers/credentialHelpers' instead of the string includes check. Replace 'issuerDid?.includes(':app:')' with 'isAppDidWeb(issuerDid)' at lines 107, 108, 90, and 57 in the respective files.
| const isAppIssuerDid = issuerDid?.includes(':app:'); | |
| const isAppIssuerDid = isAppDidWeb(issuerDid); |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
TaylorBeeston
had a problem deploying
to
ci-tests
GitHub Actions
Failure
|
🥷 Code experts: Custard7 Custard7, TaylorBeeston have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
🦞 @claude please review this PR and provide a recommendation on whether or not it should be merged. Looking for your assessment of the App DIDs implementation, code quality, test coverage, and any potential issues or improvements. |
|
Claude finished @TaylorBeeston's task —— View job PR Review: App DIDs ImplementationI've conducted a comprehensive review of the App DIDs implementation across the entire stack. Here's my assessment: 🔍 Security & Code Quality IssuesCritical Issues Found:
✅ Implementation StrengthsWell-Designed Architecture:
Good Security Practices:
🏗️ Architecture AssessmentThe implementation follows LearnCard's plugin architecture well:
📊 Test Coverage ConcernsMajor Gap: No automated tests found for the App DIDs functionality despite the significant scope (2238 additions across 52 files). The PR relies entirely on manual validation. 🎯 RecommendationsMust Fix Before Merge:
Should Consider:
📋 Final Recommendation❌ DO NOT MERGE YET While the implementation demonstrates solid architectural thinking and comprehensive functionality, the security vulnerability and error handling issues create significant risks. The lack of automated test coverage for such a substantial feature (spanning authentication, cryptography, and credential issuance) is concerning. Merge Criteria:
The core concept and implementation approach are sound, but these issues need resolution before production deployment. |
|
@claude please fix the above errors |
|
Claude finished @TaylorBeeston's task —— View job Fixing App DIDs Implementation Issues - ✅ CompleteI have successfully fixed all the issues identified in the previous review: Todo List:
🛡️ Security Issues Fixed
🔧 Code Quality Improvements
📝 Changes MadeCore Security Fix:
UI Component Fixes:
Error Handling Enhancement:
Production Cleanup:
✅ Commit StatusChanges have been committed and pushed to the The App DIDs implementation is now secure and ready for production deployment. All critical security vulnerabilities have been addressed, fragile code patterns have been replaced with robust helper functions, and error handling has been improved to surface failures that would prevent credential issuance. |
- Add slug validation to prevent XSS attacks in DID endpoint - Replace fragile string matching with proper isAppDidWeb() helper across UI components - Improve error handling in signing authority creation to surface critical failures - Remove debug console.log statements from production code 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Taylor Beeston <TaylorBeeston@users.noreply.github.com>
|
Claude finished @TaylorBeeston's task —— View job Final Comprehensive Security ReviewI've completed a thorough analysis of all DID routes and the broader App DIDs implementation. Here are my findings: Todo List:
🛡️ Security Issues Found & FixedCritical XSS Vulnerability (Fixed) - Manager DID Route
Minor Code Quality Issue (Fixed)
🔍 DID Route Security AssessmentBrain Service (
LCA API Service (
✅ Implementation Strengths Confirmed
📋 Final AssessmentStatus: ✅ SECURE FOR DEPLOYMENT All critical security vulnerabilities have been resolved:
🎯 Key Security Features
✨ Recommendation✅ APPROVED FOR MERGE The App DIDs implementation is now production-ready with robust security measures:
The implementation provides secure app-scoped DID functionality that enables embedded apps to issue credentials with their own cryptographic identity while maintaining strong security boundaries. Security fixes committed: |
- Add UUID validation for manager ID parameter in /manager/:id/did.json route - Prevent injection attacks by validating manager IDs match UUID format - Remove debug console.log statement from workflows route - Consistent with existing app slug validation security pattern 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Taylor Beeston <TaylorBeeston@users.noreply.github.com>
|
@claude please add unit tests in the brain service and tests/e2e tests for full coverage of the new functionality, capturing core functionality, edge cases, and security |
|
Claude finished @Custard7's task —— View job Adding Comprehensive Test Coverage for App DIDs ✅I've successfully added comprehensive unit tests for the brain service and e2e tests to cover the new App DIDs functionality, including core features, edge cases, and security aspects. Todo List:
✅ Test Suite Complete📋 Files Added:
🧪 Brain Service Unit Tests:
🌐 E2E Integration Tests:
🛡️ Security Coverage:
📈 Coverage Highlights:
The test suite provides comprehensive coverage ensuring the App DIDs implementation is robust, secure, and ready for production use. Committed: |
- Brain service unit tests covering slug generation, DID resolution, and security validation - E2E tests for complete App DIDs workflow including credential issuance - Security tests for directory traversal and XSS prevention - Edge case handling for missing signing authorities and collision detection - Tests support both development (draft) and production (listed) app statuses 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Jacksón Smith <Custard7@users.noreply.github.com>
TaylorBeeston
had a problem deploying
to
ci-tests
GitHub Actions
Failure
TaylorBeeston
had a problem deploying
to
ci-tests
GitHub Actions
Failure
| tasks.push( | ||
| createListingSentCredentialRelationship( | ||
| listingId, | ||
| to, | ||
| credentialInstance, | ||
| metadata, | ||
| activityId, | ||
| integrationId | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
We should test this on staging - I promise.all()'d some neo operations on a previous PR and it worked all the way until deploy, and then there were some odd locking operations errors - so we should double check
TaylorBeeston
had a problem deploying
to
ci-tests
GitHub Actions
Failure
TaylorBeeston
had a problem deploying
to
ci-tests
GitHub Actions
Failure
TaylorBeeston
had a problem deploying
to
ci-tests
GitHub Actions
Failure
Custard7
left a comment
Custard7
left a comment
There was a problem hiding this comment.
Excellent work! 🔥 🚀
✅ All tests are passing, e2e and brain service in particular
✅ Verified backwards compat with apps created on main pre-changes
✅ Verified the upgrade modal seamlessly transitions apps.
✅ Verified credentials issue from the app DID which appears correctly on credentials
Two main questions:
-
⚠️ Just want to confirm logic and reasoning for removing integration <> signing authority -
⚠️ To ensure we test the neo4j locking behavior doesn't cause any errors on staging before deploying to prod
|
❌ Although @TaylorBeeston the CI test runner is failing because of rust cargo NAPI issue. So we should fix that before merging 🙏 
|
3 participants
Overview
🎟 Relevant Jira Issues
LC-1529 App DIDs: allow App Store apps to have their own issuer identities when issuing credentials
LC-1530 LCA API: allow creating signing authority for DID other than yourself
LC-1531 Brain service: serve app DID docs for did:web:${url}:app:${slug}
LC-1532 Deterministic app slug generation + collision handling
LC-1533 UI: support app DIDs for issuing
📚 What is the context and goal of this PR?
Add app DID support so app store listings can issue credentials with their own did:web identity.
This includes signing authority creation for non-self DIDs, app did:web document resolution,
deterministic slugging, listing-based credential relationships, and UI issuer resolution for app DIDs.
🥴 TL; RL:
💡 Feature Breakdown (screenshots & videos encouraged!)
🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
Issue a credential from an app (send-credential event) and confirm issuer is app DID.
node serve.jsintests/test-embed-apppnpm devinapps/learn-card-appResolve
https://<network>/app/<slug>/did.jsonand verify it maps to the app signing authority.did:web:localhost%3A4000:app:testhttp://localhost:4000/app/test/did.json#lca-saat the end of its ID!View the credential in LearnCard App and verify issuer shows app name/icon and links to the app.
Confirm verification badge shows “App Issuer” state.
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Documentation
📝 Documentation Checklist
User-Facing Docs (
docs/→ docs.learncard.com)docs/tutorials/)docs/how-to-guides/)docs/sdks/)docs/core-concepts/)docs/apps/)Internal/AI Docs
Visual Documentation
💭 Documentation Notes
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Add app-scoped DID Web support to enable embedded apps to issue credentials with their own cryptographic identity and verifiable provenance.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how