test: add unit tests for plugin payload attribute validation#581
Open
arpitjain099 wants to merge 1 commit into
Open
test: add unit tests for plugin payload attribute validation#581arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Adds table-driven coverage for the payload descriptor checks introduced in notaryproject#178, which previously had no unit tests (tracked in notaryproject#194). isPayloadDescriptorValid is exercised for a matching descriptor, an appended annotation, and rejection on a tampered digest, size, or mediaType, plus rejection when a plugin overrides an existing annotation. areUnknownAttributesAdded is exercised for the all-known case and for unknown attributes added inside the descriptor and at the payload top level. Coverage of both functions goes from 0 percent to full statement coverage. No production code changes. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
arpitjain099
requested review from
NiazFK,
gokarnm,
priteshbandi,
rgnote,
shizhMSFT,
toddysm,
vaninrao10 and
yizha1
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds unit tests for payload validation logic in the signer package to detect descriptor tampering and unexpected JSON fields during envelope generation.
Changes:
- Introduces
TestIsPayloadDescriptorValidcovering allowed vs. rejected descriptor mutations. - Introduces
TestAreUnknownAttributesAddedcovering detection of unknown JSON attributes at multiple levels. - Adds a shared
baseDescriptor()helper to reduce duplication across cases.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+93
to
+99
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| if got := isPayloadDescriptorValid(original, tc.newDesc); got != tc.want { | ||
| t.Fatalf("isPayloadDescriptorValid() = %v, want %v", got, tc.want) | ||
| } | ||
| }) | ||
| } |
Comment on lines
+129
to
+139
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| got := areUnknownAttributesAdded([]byte(tc.content)) | ||
| sort.Strings(got) | ||
| want := append([]string{}, tc.want...) | ||
| sort.Strings(want) | ||
| if !reflect.DeepEqual(got, want) { | ||
| t.Fatalf("areUnknownAttributesAdded() = %+q, want %+q", got, want) | ||
| } | ||
| }) | ||
| } |
| } | ||
| } | ||
|
|
||
| func TestAreUnknownAttributesAdded(t *testing.T) { |
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.
What this does
Adds unit tests for the plugin payload attribute validation that #178 introduced. As noted in #194, those checks shipped without unit coverage because they were hard to exercise through the full signing flow without refactoring business logic. This PR tests the two helper functions directly instead, so no production code changes are needed.
Closes #194
Tests added
In a new
signer/payload_test.go:TestIsPayloadDescriptorValidis table-driven and covers a plugin returning the same subject descriptor it was given (valid), appending an annotation (valid), and the tamper cases that the function exists to catch: a changed digest, a changed size, a changed mediaType, and an existing annotation being overridden. These map to the spec requirement that a plugin must not alter the payload subject during signing.TestAreUnknownAttributesAddedcovers a payload where every key is an allowed descriptor field (nothing reported), an unknown attribute injected inside thetargetArtifactdescriptor, an unknown attribute added at the payload top level, and both at once. This matches the rule from the plugin signing workflow that no extra top-level attributes may be added to the subject.Verification
go test ./signer/...passes; statement coverage ofisPayloadDescriptorValidandareUnknownAttributesAddedgoes from 0 percent to full.gofmt -landgo vet ./signer/...are clean.