fix(vcr/verifier): handle unparseable issuer DID instead of nil deref#4247
fix(vcr/verifier): handle unparseable issuer DID instead of nil deref#4247SAY-5 wants to merge 2 commits into
Conversation
|
Coverage Impact ⬇️ Merging this pull request will decrease total coverage on Modified Files with Diff Coverage (1)
🛟 Help
|
| issuerDID, _ := did.ParseDID(credentialToVerify.Issuer.String()) | ||
| issuerDID, err := did.ParseDID(credentialToVerify.Issuer.String()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not validate issuer: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("could not validate issuer: %w", err) | |
| return fmt.Errorf("could not parse issuer: %w", err) |
| require.NotPanics(t, func() { | ||
| validationErr := ctx.verifier.Verify(malformed, true, true, nil) | ||
| assert.Error(t, validationErr) | ||
| assert.Contains(t, validationErr.Error(), "could not validate issuer") |
There was a problem hiding this comment.
| assert.Contains(t, validationErr.Error(), "could not validate issuer") | |
| assert.Contains(t, validationErr.Error(), "could not parse issuer") |
|
Thanks for the heads up. I'll re-sign the commits with a verified GPG key and force-push the updated branch shortly. |
Closes nuts-foundation#4235 Signed-off-by: SAY-5 <say.apm35@gmail.com>
8b8565f to
c22d536
Compare
|
Applied both suggestions in c22d536: the parse-error path now reads |
|
Applied both suggestions, the parse-path error now says 'could not parse issuer' and the assertion was updated to match. Pushed 2ce91f9. |
|
@SAY-5 your commits still appear unsigned/unverified, can't merge before that's fixed. We also need to rework our GH actions workflow to allow |

Closes #4235
Verifydiscarded the error fromdid.ParseDID(credentialToVerify.Issuer.String())and then dereferenced the resulting (possibly nil)*did.DIDatv.didResolver.Resolve(*issuerDID, &metadata), panicking the node when an external issuer emitted a non-RFC3986did:x509.Surface the parse error as a regular validation failure (
"could not validate issuer: ...") so the request fails cleanly with a 4xx instead of crashing the verifier goroutine.Added a regression test under
TestVerifier_Verify/with_signature_checkthat wraps the call inrequire.NotPanicsand asserts the error is surfaced. The test panics onmasterand passes with this change.