feat(vc): JSON-LD credential issuance signing compatibility helper - #592
Open
rmlearney-digicatapult wants to merge 7 commits into
Open
rmlearney-digicatapult wants to merge 7 commits into
rmlearney-digicatapult wants to merge 7 commits into
Conversation
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
There was a problem hiding this comment.
🟡 Changes recommended
Proposal-derived offers and issuance-time verification-method overrides still bypass the new validation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds fail-fast JSON-LD credential signing compatibility checks by comparing the requested proof type with issuer DID verification methods.
Changes:
- Adds reusable proof-type/DID validation.
- Applies validation to direct credential-offer endpoints.
- Adds mismatch coverage and bumps the patch version.
Blockers (Must-Fix)
CredentialController.ts:296: JSON-LD offers created throughacceptProposalbypass validation.CredentialController.ts:103-108:acceptRequestverification-method overrides are not validated.
Targeted Suggestions
- Validate proposal-derived offers before accepting proposals.
- Validate the actual verification method selected during issuance.
Test Gap Analysis
- Add coverage for
/create-offer, proposal acceptance, and issuance-time verification-method overrides.
Integration Risks
Invalid JSON-LD exchanges can progress until issuance and then fail instead of returning the documented 400 response.
Scores
| Category | Score |
|---|---|
| Correctness | 6/10 |
| Security | 8/10 |
| Testing | 6/10 |
| Maintainability | 7/10 |
| Performance | 9/10 |
| Integration/Infra | 6/10 |
| Overall | 7/10 |
Release Notes Draft
- Validate JSON-LD proof types against issuer DID verification methods.
- Return HTTP 400 for incompatible direct credential offers.
- Add JSON-LD signing compatibility tests.
File summaries
| File | Description |
|---|---|
src/utils/credentials.ts |
Adds proof-type compatibility validation. |
src/controllers/v1/credentials/CredentialController.ts |
Applies validation to direct offer endpoints. |
tests/unit/credential.test.ts |
Tests compatible and incompatible proof types. |
package.json |
Bumps package version. |
package-lock.json |
Synchronizes package version. |
Review details
Suppressed comments (1)
src/controllers/v1/credentials/CredentialController.ts:296
- This validation is bypassed when an issuer accepts a JSON-LD proposal:
acceptProposalcreates the offer by copying the proposal attachment, but that controller path never calls this helper. A proposer can therefore supply an incompatibleproofType, receive an offer, and defer the same signing failure until issuance instead of getting the intended fail-fast 400. Validate the effective JSON-LD offer inacceptProposalas well.
await this.assertProofTypeMatchesIssuerDid(options.credentialFormats.jsonld)
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Checklist
PR Type
Please delete options that are irrelevant.
Linked tickets
High level description
Ensure the key type selected to sign a JSON-LD credential is one that we advertise in our DID document
Detailed description
Interesting error on testing. Key type selected to sign JSON-LD credential must match the key type we're advertising in our DID Document.
Created new helper to read our did document and available signature suites to confirm compatibility before proceeding to offer or issue the credential, with fail-fast
400error.Also removed
credentialFormatsfrom theAcceptCredentialRequestOptionsinterface because this is a security risk, there is no reason to permit override at this late stage of the credential exchange process to contradict the agreed state.Describe alternatives you've considered
Operational impact
Additional context