feat: bbs2023 verification fragment#115
Conversation
WalkthroughAdds a factory for W3C DataIntegrityProof verifiers, introduces BBS-2023 fixtures and a BBS-2023 verifier via that factory, refactors the ECDSA verifier to use the factory, integrates the new verifier into the verifier list, and converts tests to data-driven scenarios including BBS-2023 expectations and snapshots. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Verify as verify()
participant Verifier as createW3CSignatureIntegrityVerifier
participant Sig as verifyW3CSignature
participant Derive as deriveCredential
Caller->>Verify: verify(document, options)
Verify->>Verifier: select applicable verifier (test)
alt proof.type=DataIntegrityProof & cryptosuite matches
Verifier->>Sig: verifyW3CSignature(document)
alt Signature valid
Sig-->>Verifier: OK
Verifier-->>Verify: VALID (isDerived: false)
else Fails with DERIVE_CREDENTIAL_ERROR
Sig-->>Verifier: Error (derive hint)
Verifier->>Derive: deriveCredential(document, derivationPaths)
Derive-->>Verifier: derivedDocument
Verifier->>Sig: verifyW3CSignature(derivedDocument)
alt Derived signature valid
Sig-->>Verifier: OK
Verifier-->>Verify: VALID (isDerived: true)
else Still invalid
Sig-->>Verifier: Error
Verifier-->>Verify: INVALID (error)
end
else Other failure
Sig-->>Verifier: Error
Verifier-->>Verify: INVALID (error)
end
else Not applicable
Verifier-->>Verify: SKIPPED
end
Verify-->>Caller: Aggregated result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/__tests__/core/verify.test.ts(9 hunks)src/__tests__/fixtures/fixtures.ts(4 hunks)src/verify/fragments/document-integrity/bbs2023W3CSignatureIntegrity.ts(1 hunks)src/verify/verify.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/__tests__/core/verify.test.ts (2)
src/__tests__/fixtures/fixtures.ts (2)
BBS2023_W3C_VERIFIABLE_DOCUMENT_V2_0(1615-1701)BBS2023_W3C_DERIVED_DOCUMENT_V2_0(1703-1735)src/core/verify.ts (1)
verifyDocument(51-81)
src/verify/fragments/document-integrity/bbs2023W3CSignatureIntegrity.ts (1)
src/w3c/verify.ts (1)
verifyW3CSignature(11-17)
src/verify/verify.ts (1)
src/verify/fragments/document-integrity/bbs2023W3CSignatureIntegrity.ts (1)
bbs2023W3CSignatureIntegrity(13-96)
🪛 GitHub Actions: CI
src/verify/verify.ts
[error] 1-1: Failed to load url ./fragments/document-integrity/bbs2023w3cSignatureIntegrity (resolved id: ./fragments/document-integrity/bbs2023w3cSignatureIntegrity) in verify.ts. Does the file exist?
🪛 GitHub Check: Tests / Test Build (18.x)
src/verify/verify.ts
[failure] 39-39:
Cannot find module './fragments/document-integrity/bbs2023w3cSignatureIntegrity' or its corresponding type declarations.
🪛 GitHub Check: Tests / Test Build (20.x)
src/verify/verify.ts
[failure] 39-39:
Cannot find module './fragments/document-integrity/bbs2023w3cSignatureIntegrity' or its corresponding type declarations.
🪛 Gitleaks (8.28.0)
src/__tests__/fixtures/fixtures.ts
[high] 1635-1635: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1636-1636: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1723-1723: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1724-1724: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/verify/fragments/document-integrity/ecdsaW3CSignatureIntegrity.ts (1)
1-9: LGTM! Clean refactoring.The migration to the factory pattern successfully eliminates code duplication while maintaining the same functionality. The ECDSA configuration is correct with an empty
derivationPathsarray.Consider adding a brief comment explaining why
derivationPathsis empty:export const ecdsaW3CSignatureIntegrity: Verifier<VerificationFragment> = createW3CSignatureIntegrityVerifier({ cryptosuite: 'ecdsa-sd-2023', name: 'EcdsaW3CSignatureIntegrity', + // ECDSA verification doesn't require derivation paths derivationPaths: [], });src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts (4)
17-19: Strengthen the type guard or document the minimal check.The type guard only verifies that a
proofproperty exists, without validating its structure or other required properties ofSignedVerifiableCredential. While downstreamverifyW3CSignaturewill catch structural issues, a malformed document could produce less clear error messages.Consider either:
- Strengthening the guard to validate key properties:
function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential { - return typeof document === 'object' && document !== null && 'proof' in document; + return ( + typeof document === 'object' && + document !== null && + 'proof' in document && + typeof (document as any).proof === 'object' && + (document as any).proof !== null + ); }
- Or documenting the minimal nature of the check:
+// Minimal type guard - full validation happens in verifyW3CSignature function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential { return typeof document === 'object' && document !== null && 'proof' in document; }
21-43: Factory pattern looks good, minor type safety note.The factory cleanly creates verifiers with proper skip and test methods. The
testmethod's optional chaining (doc.proof?.type) safely handles missing proof objects.Note: Line 41 casts to
SignedVerifiableCredentialwithout validation, but the optional chaining prevents runtime errors. Consider adding a comment explaining this pattern.
17-19: Consider strengthening the type guard validation.The current type guard only checks for the presence of a
proofproperty. Consider validating additional required properties ofSignedVerifiableCredential(e.g.,@context,type, proof structure) to catch malformed documents earlier with clearer error messages.Example refinement:
function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential { - return typeof document === 'object' && document !== null && 'proof' in document; + if (typeof document !== 'object' || document === null || !('proof' in document)) { + return false; + } + const doc = document as { proof?: unknown }; + return ( + typeof doc.proof === 'object' && + doc.proof !== null && + 'type' in doc.proof && + 'cryptosuite' in doc.proof + ); }
40-43: Avoid unsafe type assertion in test method.The cast
document as SignedVerifiableCredentialbypasses type safety. While optional chaining protects against runtime errors, consider using a type guard or safer checks.Apply this diff:
test: (document: unknown) => { - const doc = document as SignedVerifiableCredential; - return doc.proof?.type === PROOF_TYPE && doc.proof?.cryptosuite === cryptosuite; + if (!isSignedVerifiableCredential(document)) { + return false; + } + return document.proof?.type === PROOF_TYPE && document.proof?.cryptosuite === cryptosuite; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/verify/fragments/document-integrity/bbs2023W3CSignatureIntegrity.ts(1 hunks)src/verify/fragments/document-integrity/ecdsaW3CSignatureIntegrity.ts(1 hunks)src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/verify/fragments/document-integrity/bbs2023W3CSignatureIntegrity.ts
👮 Files not reviewed due to content moderation or server errors (2)
- src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts
- src/verify/fragments/document-integrity/ecdsaW3CSignatureIntegrity.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/verify/fragments/document-integrity/ecdsaW3CSignatureIntegrity.ts (1)
src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts (1)
createW3CSignatureIntegrityVerifier(21-108)
src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts (1)
src/w3c/verify.ts (1)
verifyW3CSignature(11-17)
🔇 Additional comments (12)
src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts (10)
1-6: LGTM!The imports and constant definitions are clean and appropriate for the factory's functionality.
8-15: LGTM!The
CryptosuiteConfiginterface is well-designed with clear documentation, providing good flexibility for different cryptosuite implementations.
26-43: LGTM with note on type safety.The
skipandtestmethods are correctly implemented. The unsafe cast intestis acceptable here since:
- The method only checks proof properties using optional chaining
- Full validation occurs in the
verifymethod- This is a common pattern for test predicates
72-106: LGTM!The success and error handling logic is comprehensive and follows good practices:
- Provides clear success messages
- Safely accesses error properties with fallbacks
- Properly handles exceptions with type checking
1-6: LGTM!The imports and constant declarations are appropriate for the factory's functionality. Using
as constfor the constants ensures type literal guarantees.
8-15: LGTM!The interface is well-defined with clear documentation. The optional
derivationPathsfield provides flexibility for different cryptosuites.
84-105: LGTM!The error handling is comprehensive with appropriate fallback messages. The catch block properly checks for
Errorinstances and provides a generic fallback for unexpected error types.
58-83: Fix invertedisDerivedflag logic.The
isDerivedflag logic is inverted:
- Line 60 sets
isDerived = trueinitially (before derivation)- Line 69 sets
isDerived = falseafter derivation occurs- Lines 78-80 use
isDerived ? 'verified successfully' : 'verified after derivation'This produces incorrect messages: documents that go through derivation will report "verified successfully" instead of "verified after derivation".
Apply this diff to fix the logic:
try { let verificationResult = await verifyW3CSignature(document, verifierOptions); - let isDerived = true; + let isDerived = false; // Handle derivation if needed if ( !verificationResult.verified && verificationResult.error?.includes(DERIVE_CREDENTIAL_ERROR) ) { const derivedCredential = await deriveCredential(document, derivationPaths); verificationResult = await verifyW3CSignature(derivedCredential.derived, verifierOptions); - isDerived = false; + isDerived = true; } if (verificationResult.verified) { return { type: 'DOCUMENT_INTEGRITY', name, data: true, reason: { message: isDerived - ? 'Document verified successfully' - : 'Document verified after derivation', + ? 'Document verified after derivation' + : 'Document verified successfully', }, status: 'VALID', };Likely an incorrect or invalid review comment.
95-105: LGTM!The error handling properly catches exceptions, uses
instanceof Errorto safely access the error message, and provides a sensible fallback for unknown errors.
58-84: Fix invertedisDerivedflag logic.The
isDerivedvariable logic is backwards:
- Line 60: Initialized to
true(meaning "not derived")- Line 69: Set to
falseafter derivation occurs- Lines 78-80: Message incorrectly maps the inverted values
This causes the success message to be incorrect.
Apply this diff to fix the logic:
try { let verificationResult = await verifyW3CSignature(document, verifierOptions); - let isDerived = true; + let isDerived = false; // Handle derivation if needed if ( !verificationResult.verified && verificationResult.error?.includes(DERIVE_CREDENTIAL_ERROR) ) { const derivedCredential = await deriveCredential(document, derivationPaths); verificationResult = await verifyW3CSignature(derivedCredential.derived, verifierOptions); - isDerived = false; + isDerived = true; } if (verificationResult.verified) { return { type: 'DOCUMENT_INTEGRITY', name, data: true, reason: { - message: isDerived - ? 'Document verified successfully' - : 'Document verified after derivation', + message: isDerived + ? 'Document verified after derivation' + : 'Document verified successfully', }, status: 'VALID', };Likely an incorrect or invalid review comment.
src/verify/fragments/document-integrity/ecdsaW3CSignatureIntegrity.ts (2)
1-9: LGTM! Clean refactor to factory pattern.The refactor successfully delegates to the new factory while maintaining the same public API. The configuration correctly specifies the ECDSA cryptosuite with an empty derivation path array.
This pattern enables easy addition of new cryptosuites (like BBS2023) without code duplication.
1-9: Approve factory pattern usage and configuration
Refactoring correctly delegates tocreateW3CSignatureIntegrityVerifierwith valid cryptosuiteecdsa-sd-2023, matching name, and emptyderivationPaths; no changes needed.
| function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential { | ||
| return typeof document === 'object' && document !== null && 'proof' in document; | ||
| } |
There was a problem hiding this comment.
Consider strengthening the type guard.
The type guard only checks for the presence of a proof property, which is minimal validation for a SignedVerifiableCredential. While the downstream verifyW3CSignature may perform thorough validation, a stronger guard would catch structural issues earlier and provide clearer error messages.
Consider enhancing the guard to validate essential credential structure:
function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential {
- return typeof document === 'object' && document !== null && 'proof' in document;
+ return (
+ typeof document === 'object' &&
+ document !== null &&
+ 'proof' in document &&
+ '@context' in document &&
+ 'type' in document
+ );
}📝 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.
| function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential { | |
| return typeof document === 'object' && document !== null && 'proof' in document; | |
| } | |
| function isSignedVerifiableCredential(document: unknown): document is SignedVerifiableCredential { | |
| return ( | |
| typeof document === 'object' && | |
| document !== null && | |
| 'proof' in document && | |
| '@context' in document && | |
| 'type' in document | |
| ); | |
| } |
| try { | ||
| let verificationResult = await verifyW3CSignature(document, verifierOptions); | ||
| let isDerived = true; | ||
|
|
||
| // Handle derivation if needed | ||
| if ( | ||
| !verificationResult.verified && | ||
| verificationResult.error?.includes(DERIVE_CREDENTIAL_ERROR) | ||
| ) { | ||
| const derivedCredential = await deriveCredential(document, derivationPaths); | ||
| verificationResult = await verifyW3CSignature(derivedCredential.derived, verifierOptions); | ||
| isDerived = false; | ||
| } |
There was a problem hiding this comment.
Refactor confusing variable name.
The variable isDerived has inverted semantics that make the code confusing:
- Line 60:
isDerived = trueactually means "verification succeeded without derivation" - Line 69:
isDerived = falseactually means "derivation was performed"
This is backwards from what the name suggests and creates a maintenance hazard.
Apply this diff to clarify the logic:
try {
let verificationResult = await verifyW3CSignature(document, verifierOptions);
- let isDerived = true;
+ let requiresDerivation = false;
// Handle derivation if needed
if (
!verificationResult.verified &&
verificationResult.error?.includes(DERIVE_CREDENTIAL_ERROR)
) {
const derivedCredential = await deriveCredential(document, derivationPaths);
verificationResult = await verifyW3CSignature(derivedCredential.derived, verifierOptions);
- isDerived = false;
+ requiresDerivation = true;
}
if (verificationResult.verified) {
return {
type: 'DOCUMENT_INTEGRITY',
name,
data: true,
reason: {
- message: isDerived
+ message: requiresDerivation
- ? 'Document verified successfully'
- : 'Document verified after derivation',
+ ? 'Document verified after derivation'
+ : 'Document verified successfully',
},
status: 'VALID',
};📝 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.
| try { | |
| let verificationResult = await verifyW3CSignature(document, verifierOptions); | |
| let isDerived = true; | |
| // Handle derivation if needed | |
| if ( | |
| !verificationResult.verified && | |
| verificationResult.error?.includes(DERIVE_CREDENTIAL_ERROR) | |
| ) { | |
| const derivedCredential = await deriveCredential(document, derivationPaths); | |
| verificationResult = await verifyW3CSignature(derivedCredential.derived, verifierOptions); | |
| isDerived = false; | |
| } | |
| try { | |
| let verificationResult = await verifyW3CSignature(document, verifierOptions); | |
| let requiresDerivation = false; | |
| // Handle derivation if needed | |
| if ( | |
| !verificationResult.verified && | |
| verificationResult.error?.includes(DERIVE_CREDENTIAL_ERROR) | |
| ) { | |
| const derivedCredential = await deriveCredential(document, derivationPaths); | |
| verificationResult = await verifyW3CSignature(derivedCredential.derived, verifierOptions); | |
| requiresDerivation = true; | |
| } | |
| if (verificationResult.verified) { | |
| return { | |
| type: 'DOCUMENT_INTEGRITY', | |
| name, | |
| data: true, | |
| reason: { | |
| message: requiresDerivation | |
| ? 'Document verified after derivation' | |
| : 'Document verified successfully', | |
| }, | |
| status: 'VALID', | |
| }; |
🤖 Prompt for AI Agents
src/verify/fragments/document-integrity/w3cModernSignatureIntegrityFactory.ts
around lines 58 to 70: the boolean isDerived has inverted semantics (true means
no derivation, false means derivation occurred); rename it to usedDerivation (or
performedDerivation) and flip its initial value and assignment so it accurately
reflects whether derivation was performed — initialize usedDerivation = false
before attempting verification, and set usedDerivation = true inside the block
where deriveCredential is called; ensure any subsequent logic that referenced
isDerived now uses the new name and correct truth value.
## [2.2.0](v2.1.0...v2.2.0) (2025-10-09) ### Features * bbs2023 verification fragment ([#115](#115)) ([9bf7c7f](9bf7c7f))
|
🎉 This PR is included in version 2.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Summary
Summary by CodeRabbit
New Features
Enhancements
Tests