-
Notifications
You must be signed in to change notification settings - Fork 23
Bug: fix VP JWT marshalling flattening single-entry arrays incorrectly #3958
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
Open
reinkrul
wants to merge
22
commits into
master
Choose a base branch
from
iss3957-vp-jwt-type-marshalling
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
c3ab3dc
Fix VP JWT type marshalling being a []string
reinkrul 7a27d5b
Claude AI tried to fix the e2e test
reinkrul a05b175
Claude AI tried to fix the e2e test
reinkrul 6cdd44d
Claude AI tried to fix the e2e test (revert 1)
reinkrul 7aa3568
fix test?
reinkrul 3647757
debug level for e2e test
reinkrul 7ec04b0
info level logging
reinkrul 021ad6c
Attempt
reinkrul 630345d
fixzz
reinkrul f37f7c8
fixzz
reinkrul ba4dbb3
Merge branch 'master' into iss3957-vp-jwt-type-marshalling
reinkrul ba228ea
reverts
reinkrul 7dd4275
Merge branch 'master' into iss3957-vp-jwt-type-marshalling
reinkrul 22a27a5
Merge remote-tracking branch 'origin/master' into iss3957-vp-jwt-type…
reinkrul 25af536
Switch to go-did for creating JWT VPs
reinkrul f8e2092
update dependency
reinkrul 912f62a
Merge branch 'master' into iss3957-vp-jwt-type-marshalling
reinkrul 56fdbe3
Merge branch 'master' into iss3957-vp-jwt-type-marshalling
reinkrul 780c91e
merge
reinkrul 41ad809
fix conflict
reinkrul 8978863
Fix backwards compatibility: update go-did to v0.18.1 and restore dow…
reinkrul dc51558
Update vcr/holder/presenter_test.go
reinkrul File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,9 @@ package holder | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ssi "github.com/nuts-foundation/go-did" | ||||||||||||||||||||||||||
| "github.com/nuts-foundation/go-did/did" | ||||||||||||||||||||||||||
| "github.com/nuts-foundation/go-did/vc" | ||||||||||||||||||||||||||
|
|
@@ -39,8 +42,6 @@ import ( | |||||||||||||||||||||||||
| "github.com/stretchr/testify/assert" | ||||||||||||||||||||||||||
| "github.com/stretchr/testify/require" | ||||||||||||||||||||||||||
| "go.uber.org/mock/gomock" | ||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func TestPresenter_buildPresentation(t *testing.T) { | ||||||||||||||||||||||||||
|
|
@@ -142,7 +143,7 @@ func TestPresenter_buildPresentation(t *testing.T) { | |||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| t.Run("JWT", func(t *testing.T) { | ||||||||||||||||||||||||||
| options := PresentationOptions{Format: JWTPresentationFormat} | ||||||||||||||||||||||||||
| options := PresentationOptions{Format: JWTPresentationFormat, ProofOptions: proof.ProofOptions{Created: time.Now()}} | ||||||||||||||||||||||||||
| t.Run("ok - one VC", func(t *testing.T) { | ||||||||||||||||||||||||||
| ctrl := gomock.NewController(t) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -162,7 +163,33 @@ func TestPresenter_buildPresentation(t *testing.T) { | |||||||||||||||||||||||||
| assert.NotNil(t, result.JWT()) | ||||||||||||||||||||||||||
| nonce, _ := result.JWT().Get("nonce") | ||||||||||||||||||||||||||
| assert.Empty(t, nonce) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| t.Run("#3957: Verifiable Presentation type is marshalled incorrectly in JWT format", func(t *testing.T) { | ||||||||||||||||||||||||||
| t.Run("make sure the fix is backwards compatible", func(t *testing.T) { | ||||||||||||||||||||||||||
| vp := vc.VerifiablePresentation{ | ||||||||||||||||||||||||||
| Type: []ssi.URI{ssi.MustParseURI("VerifiablePresentation")}, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| t.Run("sanity check: regular JSON marshalling yields type: string", func(t *testing.T) { | ||||||||||||||||||||||||||
| data, err := vp.MarshalJSON() | ||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||
| assert.Contains(t, string(data), `"type":"VerifiablePresentation"`) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| vpAsMap := result.JWT().PrivateClaims()["vp"].(map[string]any) | ||||||||||||||||||||||||||
| t.Run("make sure type now marshals as array", func(t *testing.T) { | ||||||||||||||||||||||||||
| typeProp := vpAsMap["type"].([]any) | ||||||||||||||||||||||||||
|
Comment on lines
+178
to
+180
|
||||||||||||||||||||||||||
| vpAsMap := result.JWT().PrivateClaims()["vp"].(map[string]any) | |
| t.Run("make sure type now marshalls as array", func(t *testing.T) { | |
| typeProp := vpAsMap["type"].([]any) | |
| vpClaim, ok := result.JWT().PrivateClaims()["vp"] | |
| require.True(t, ok, "vp claim must be present in JWT private claims") | |
| vpAsMap, ok := vpClaim.(map[string]any) | |
| require.True(t, ok, "vp claim must be a map[string]any, got %T", vpClaim) | |
| t.Run("make sure type now marshalls as array", func(t *testing.T) { | |
| typeVal, ok := vpAsMap["type"] | |
| require.True(t, ok, "vp.type must be present") | |
| typeProp, ok := typeVal.([]any) | |
| require.True(t, ok, "vp.type must be a []any, got %T", typeVal) |
Copilot
AI
Mar 26, 2026
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 PR description mentions the flattening bug also affected @context and verifiableCredential in JWT VPs, but this new regression test only asserts the type field. Consider extending this test to also assert that vp.@context and vp.verifiableCredential are encoded as arrays (even for a single entry), so the full intended fix is covered by automated tests.
Suggested change
| }) | |
| }) | |
| t.Run("make sure @context now marshalls as array", func(t *testing.T) { | |
| contextProp, ok := vpAsMap["@context"].([]any) | |
| require.True(t, ok, "@context must be encoded as an array") | |
| assert.GreaterOrEqual(t, len(contextProp), 1, "@context array must have at least one entry") | |
| }) | |
| t.Run("make sure verifiableCredential now marshalls as array", func(t *testing.T) { | |
| vcProp, ok := vpAsMap["verifiableCredential"].([]any) | |
| require.True(t, ok, "verifiableCredential must be encoded as an array") | |
| assert.Len(t, vcProp, 1, "single VC presentation must still use an array") | |
| }) |
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
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
Oops, something went wrong.
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.
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.
In JWT mode this always passes a non-nil IssuedAt pointer, even when options.ProofOptions.Created is the zero time. Previously, when Created was zero, the code defaulted the JWT time claim(s) to time.Now(); now a zero Created could produce an invalid/negative Unix timestamp (or an unexpectedly old value) in the JWT. Consider only setting IssuedAt when Created is non-zero (otherwise pass nil or set it to time.Now() first) to preserve the prior behavior and avoid generating invalid tokens.