feat(vc): JSON-LD credential issuance signing compatibility helper - #592
rmlearney-digicatapult wants to merge 11 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>
| ).to.equal(true) | ||
| }) | ||
|
|
||
| test('should return 400 when create-offer proofType does not match a verification method advertised by the issuer DID', async () => { |
There was a problem hiding this comment.
These tests only exercise the "proofType doesn't match the verification method type" branch. The other new 400 paths have no tests:
getCreatedDidsreturns[](issuer DID not created by this agent)- the record has no
didDocumentandresolveDidDocumentrejects - a
proofTypetheSignatureSuiteRegistrydoesn't know (thecatchbranch invalidateProofTypeAgainstIssuerDid) issuergiven as an object ({ id: 'did:key:123' }) instead of a string
Each of these returns an error message to API callers. Tests would lock in those messages and the error field paths.
| * @throws BadRequest if the issuer DID is not owned by this agent, cannot be resolved, or | ||
| * does not advertise a verification method compatible with the requested proofType | ||
| */ | ||
| private async assertProofTypeMatchesIssuerDid(jsonld: DidCommJsonLdCredentialDetailFormat) { |
There was a problem hiding this comment.
accept-proposal doesn't run this check yet. When the holder proposes the JSON-LD credential, the issuer and proofType come from the holder. acceptProposal then sends that proposal back as the offer without checking whether this agent can sign it. If the issuer DID isn't ours, or the proofType doesn't match a key in its DID document, the error only appears later at accept-request as a Credo Missing verification method for key type ... error, returned as a 500. That's the failure this PR is meant to catch early.
Credo's JSON-LD acceptProposal payload is EmptyObject, so the check can't use the request body. It needs to use the stored proposal, something like:
const { proposal } = await this.agent.didcomm.credentials.getFormatData(credentialRecordId)
if (proposal?.jsonld) await this.assertProofTypeMatchesIssuerDid(proposal.jsonld)
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