test: add unit tests for plugin payload attribute validation - #581
test: add unit tests for plugin payload attribute validation#581arpitjain099 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>
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.
| 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) | ||
| } | ||
| }) | ||
| } |
| 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 PR is stale because it has been opened for 45 days with no activity. Remove stale label or comment. Otherwise, it will be closed in 30 days. |
|
Still relevant, and the stale label caught me out rather than the change being abandoned. Going through Copilot's three points: On rebinding On the The tests themselves cover |
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.