-
Notifications
You must be signed in to change notification settings - Fork 43
Feat: w3c field request & full cred subject disclosure #329
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?
Conversation
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.
Pull Request Overview
This PR adds support for W3C Verifiable Credential v1 fields (credentialStatus, expirationDate, issuanceDate) in zero-knowledge proof queries and verifiable presentations. The changes enable selective disclosure of these fields alongside credential subject data.
Key Changes:
- Extended ProofQuery interface to support credentialStatus, expirationDate, and issuanceDate fields
- Modified presentation generation to include credentialStatus information in verifiable presentations
- Updated proof generation and verification logic to handle W3C v1 credential fields
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/proofs/sig.test.ts | Updated test expectations to verify verifiable presentations are now always generated (changing from undefined to not.to.be.undefined), added credentialStatus to test credentials |
| tests/proofs/sig-onchain.test.ts | Updated test expectation for verifiable presentation generation |
| tests/proofs/mtp.test.ts | Updated test expectations for verifiable presentation generation |
| tests/proofs/mtp-onchain.test.ts | Updated test expectation for verifiable presentation generation |
| tests/proofs/common.test.ts | Fixed field names to include credentialSubject. prefix for consistency |
| tests/mocks/schema.ts | Added BasicPerson schema definitions for new test cases |
| tests/handlers/auth.test.ts | Added comprehensive test cases for W3C v1 field queries including credential status and selective disclosure |
| src/verifiable/proof.ts | Extended ProofQuery interface with optional W3C v1 fields |
| src/verifiable/presentation.ts | Refactored presentation creation to support credential status and dynamic field resolution |
| src/verifiable/constants.ts | Added CREDENTIAL_STATUS_PATH constant |
| src/storage/filters/jsonQuery.ts | Added filter support for credentialStatus, expirationDate, and issuanceDate queries |
| src/proof/verifiers/query.ts | Updated field value extraction path for verifiable presentations |
| src/proof/verifiers/pub-signals-verifier.ts | Modified to use new parseProofQueryMetadata function with VP support |
| src/proof/proof-service.ts | Added preprocessing for full object SD and handling of credential status in queries |
| src/proof/common.ts | Added extensive utility functions for parsing W3C v1 fields and property queries |
| src/iden3comm/types/protocol/auth.ts | Extended type definitions for W3C v1 proof query fields and verifiable presentation structure |
| package.json | Version bump from 1.37.1 to 1.38.0 |
| package-lock.json | Version bump in lockfile to match package.json |
Comments suppressed due to low confidence (1)
src/verifiable/presentation.ts:56
- Potential runtime error: The function doesn't handle cases where intermediate properties in the path might be undefined or null. When traversing nested properties (line 53), accessing properties on
undefinedornullvalues will throw a runtime error. Consider adding null/undefined checks or using optional chaining with proper error handling.
export const findValue = (fieldName: string, credential: W3CCredential): JsonDocumentObject => {
const [first, ...rest] = fieldName.split('.');
let v: unknown = credential[first as keyof W3CCredential];
for (const part of rest) {
v = (v as JsonDocumentObject)[part];
}
return v as JsonDocumentObject;
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let credentialType = proofReq.query['type'] as string; | ||
| // todo: check if we can move this to the parseQueryMetadata function | ||
| if (propertyMetadata.fieldName.startsWith('credentialStatus.')) { | ||
| credentialType = preparedCredential.credential.credentialStatus.type; |
Copilot
AI
Nov 7, 2025
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.
Potential null pointer exception: preparedCredential.credential.credentialStatus may be undefined. Add a null check before accessing .type property to prevent runtime errors. Consider: credentialType = preparedCredential.credential.credentialStatus?.type ?? credentialType; or throw a descriptive error if credentialStatus is required.
| credentialType = preparedCredential.credential.credentialStatus.type; | |
| credentialType = preparedCredential.credential.credentialStatus?.type ?? credentialType; |
src/proof/proof-service.ts
Outdated
| if (credentialSubject && Object.keys(credentialSubject).length === 0) { | ||
| request.query.credentialSubject = flattenToQueryShape(cred.credentialSubject); | ||
| } | ||
| if (credentialStatus && Object.keys(credentialStatus).length === 0) { |
Copilot
AI
Nov 7, 2025
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.
Potential null pointer exception: cred.credentialStatus may be undefined when calling flattenToQueryShape. Add a null check before accessing credentialStatus to prevent runtime errors when credentials don't have a credentialStatus field. Consider: if (credentialStatus && Object.keys(credentialStatus).length === 0 && cred.credentialStatus) { ... }
| if (credentialStatus && Object.keys(credentialStatus).length === 0) { | |
| if (credentialStatus && Object.keys(credentialStatus).length === 0 && cred.credentialStatus) { |
| const [fieldParentObj] = propertyQuery.fieldName.split('.'); | ||
| switch (fieldParentObj) { | ||
| case 'credentialStatus': | ||
| propertyQuery.fieldName = propertyQuery.fieldName.replace('credentialStatus.', ''); |
Copilot
AI
Nov 7, 2025
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.
Potential bug with .replace(): Using replace('credentialStatus.', '') only removes the first occurrence. If the field name is nested like 'credentialStatus.credentialStatus.field', this will incorrectly produce 'credentialStatus.field'. Consider using a more robust approach like removing only the prefix: propertyQuery.fieldName.substring('credentialStatus.'.length) or ensuring the replacement only happens at the start of the string.
| propertyQuery.fieldName = propertyQuery.fieldName.replace('credentialStatus.', ''); | |
| if (propertyQuery.fieldName.startsWith('credentialStatus.')) { | |
| propertyQuery.fieldName = propertyQuery.fieldName.substring('credentialStatus.'.length); | |
| } |
src/verifiable/presentation.ts
Outdated
| '@context': credential['@context'], | ||
| type: vcTypes, | ||
| credentialSubject: { | ||
| // id: credential.credentialSubject.id, |
Copilot
AI
Nov 7, 2025
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.
The commented-out line should either be removed or uncommented with proper justification. Leaving commented code in production reduces code readability and maintainability. If this ID field is intentionally excluded from the presentation, consider adding a comment explaining why.
| // id: credential.credentialSubject.id, | |
| // The 'id' field is intentionally excluded from credentialSubject to comply with privacy requirements. |
tests/handlers/auth.test.ts
Outdated
| const expectedCredSubject = basicPersonCred.credentialSubject; | ||
| delete expectedCredSubject.id; |
Copilot
AI
Nov 7, 2025
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.
Mutating test data (delete expectedCredSubject.id) is an anti-pattern that can cause issues if the same object is reused elsewhere in tests. Consider creating a new object without the id field instead: const expectedCredSubject = { ...basicPersonCred.credentialSubject }; delete expectedCredSubject.id; or use object destructuring: const { id, ...expectedCredSubject } = basicPersonCred.credentialSubject;
| const expectedCredSubject = basicPersonCred.credentialSubject; | |
| delete expectedCredSubject.id; | |
| const { id, ...expectedCredSubject } = basicPersonCred.credentialSubject; |
| const flattened = flattenToQueryShape( | ||
| (vp.verifiableCredential as Record<string, any>)[documentName], |
Copilot
AI
Nov 7, 2025
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.
Potential runtime error: When accessing (vp.verifiableCredential as Record<string, any>)[documentName], there's no check that the property exists. If verifiableCredential doesn't have a credentialStatus field when documentName is 'credentialStatus', this will pass undefined to flattenToQueryShape, which may cause unexpected behavior. Add a check: const docValue = (vp.verifiableCredential as Record<string, any>)[documentName]; if (!docValue) { throw new Error(...) }
| const flattened = flattenToQueryShape( | |
| (vp.verifiableCredential as Record<string, any>)[documentName], | |
| const docValue = (vp.verifiableCredential as Record<string, any>)[documentName]; | |
| if (!docValue) { | |
| throw new Error(`VerifiableCredential is missing the required property '${documentName}' for full disclosure`); | |
| } | |
| const flattened = flattenToQueryShape( | |
| docValue, |
| // todo: check if we can move this to the parseQueryMetadata function | ||
| if (propertyMetadata.fieldName.startsWith('credentialStatus.')) { |
Copilot
AI
Nov 7, 2025
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.
TODO comment left in production code: "todo: check if we can move this to the parseQueryMetadata function". Consider either implementing this refactoring now or creating a tracked issue for it. Leaving TODO comments without follow-up reduces code maintainability.
| break; | ||
| case 'credentialSubject': | ||
| case '': | ||
| propertyQuery.fieldName = propertyQuery.fieldName.replace('credentialSubject.', ''); |
Copilot
AI
Nov 7, 2025
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.
Potential bug with .replace(): Using replace('credentialSubject.', '') only removes the first occurrence. If the field name is nested like 'credentialSubject.credentialSubject.field', this will incorrectly produce 'credentialSubject.field'. Consider using a more robust approach like removing only the prefix: propertyQuery.fieldName.substring('credentialSubject.'.length) or ensuring the replacement only happens at the start of the string.
| propertyQuery.fieldName = propertyQuery.fieldName.replace('credentialSubject.', ''); | |
| if (propertyQuery.fieldName.startsWith('credentialSubject.')) { | |
| propertyQuery.fieldName = propertyQuery.fieldName.substring('credentialSubject.'.length); | |
| } |
| import { calculateCoreSchemaHash, ProofQuery, ProofType } from '../../verifiable'; | ||
| import { | ||
| calculateCoreSchemaHash, | ||
| CredentialStatusType, |
Copilot
AI
Nov 7, 2025
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.
Unused import CredentialStatusType.
| CredentialStatusType, |
No description provided.