From eb126e61b8b18b2d7cb6f38f99d813f60dee5287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adolfo=20Garc=C3=ADa=20Veytia=20=28Puerco=29?= Date: Tue, 1 Sep 2026 22:00:00 -0600 Subject: [PATCH] Treat a missing notes reference as a commit without attestations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading attestations from a repository that has no refs/notes/commits yet failed instead of returning none, so the first attestation of a repository could never be written: producing it required reading the attestations that would have created the reference. vcslocator v0.5.0 reports a missing reference with ErrRefNotFound and its error lists can be unwrapped, so replace the string matching in extractCommitBundle with typed checks: no attestations when every failed locator is either the missing reference or a missing note file, an error otherwise. This also covers a nil error with empty note files, which used to panic. Use go-git's typed error for the same check in the storer's own fetch of the notes reference. Signed-off-by: Adolfo García Veytia (Puerco) --- go.mod | 2 +- go.sum | 6 +-- repository/note/collector.go | 34 +++++++++--- repository/note/collector_test.go | 88 +++++++++++++++++++++++++++++++ repository/note/storer.go | 2 +- 5 files changed, 119 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index 54072f1..e54076f 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( github.com/carabiner-dev/signer v0.6.2-0.20260901235141-7abffe0fc666 github.com/carabiner-dev/spdx3 v0.1.0 github.com/carabiner-dev/stash v0.0.0-20260716192412-e2fe293d76a5 - github.com/carabiner-dev/vcslocator v0.4.7 + github.com/carabiner-dev/vcslocator v0.5.0 github.com/cenkalti/backoff/v5 v5.0.3 github.com/github/smimesign v0.2.0 github.com/go-git/go-billy/v5 v5.9.1 diff --git a/go.sum b/go.sum index 5d5b8fd..5509a48 100644 --- a/go.sum +++ b/go.sum @@ -110,16 +110,14 @@ github.com/carabiner-dev/predicates v0.5.0 h1:CG2xO5xTXWXakjJkAFuS2xSA2olP9Ew25k github.com/carabiner-dev/predicates v0.5.0/go.mod h1:EUm2p0CwKoUuc+OLbGkoxLdRqBrg/r957b8iN/ACWSA= github.com/carabiner-dev/sbomfs v0.2.0 h1:7OG0Su6iz9sPWcAFzSsaauVZQpKayzrJvGWe0NoxbPw= github.com/carabiner-dev/sbomfs v0.2.0/go.mod h1:KaTNcHjm5qbu5G0RCc/7uo3U1KZj1IqRZLFiG+nwC8g= -github.com/carabiner-dev/signer v0.6.1 h1:3GnpNt16Qzw476lgCGMZRThtofTy7Ke1A60HqNboWnc= -github.com/carabiner-dev/signer v0.6.1/go.mod h1:dg1OvK3lTePsPrbvoBZO5SBt0DHd2/NLdM2wfB2/4/8= github.com/carabiner-dev/signer v0.6.2-0.20260901235141-7abffe0fc666 h1:YMToHatLXJK1205jRGyfVZcBFrgUN+u/3ytCb+uRnL0= github.com/carabiner-dev/signer v0.6.2-0.20260901235141-7abffe0fc666/go.mod h1:Vhx/hRcLL4pxnDTofmDZAkLuUmMlTvo3dYqvgfB2FA0= github.com/carabiner-dev/spdx3 v0.1.0 h1:Q6nMLXV0BhtqDQuwRDOaJDZR670TK4jdOvZqvTW/ctY= github.com/carabiner-dev/spdx3 v0.1.0/go.mod h1:d/t010TrZvYZBeYGpvHeId6QpRvGLlqK7cy0E81npPA= github.com/carabiner-dev/stash v0.0.0-20260716192412-e2fe293d76a5 h1:ZPlLrqZAZLf8iuJAReJjOTHM8gL3BeVpXtto5+KRDm0= github.com/carabiner-dev/stash v0.0.0-20260716192412-e2fe293d76a5/go.mod h1:2RhCP3fDQ9TEuGBhxkd+gWDOlzQYkNSXvmjr8fxfAR4= -github.com/carabiner-dev/vcslocator v0.4.7 h1:w4548miXp0WaiuBhLyiNLwbX5/j6hqr7Uy3ofgjaZj8= -github.com/carabiner-dev/vcslocator v0.4.7/go.mod h1:emrQwITbcy5tK/wxUIOGVeMSW8HDcfhSZ6a0kpfJFH8= +github.com/carabiner-dev/vcslocator v0.5.0 h1:UpqilJXRJBgwzQHk0MsD03w+8/0GcyC1HbXxX0wZKcU= +github.com/carabiner-dev/vcslocator v0.5.0/go.mod h1:s4zrF5l7b2iZFsxVDKz6Y+6LcaqP4GTshSCGuCbu+Ls= github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/cenkalti/backoff/v5 v5.0.3 h1:ZN+IMa753KfX5hd8vVaMixjnqRZ3y8CuJKRKj1xcsSM= diff --git a/repository/note/collector.go b/repository/note/collector.go index e990722..de8332b 100644 --- a/repository/note/collector.go +++ b/repository/note/collector.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "io" + "io/fs" "strings" "github.com/carabiner-dev/attestation" @@ -202,18 +203,37 @@ func (c *Collector) extractCommitBundle() (io.Reader, error) { return &bufferShard, nil case bufferFile.Len() > 0: return &bufferFile, nil + case err == nil || noteMissing(err): + // No note data for the commit is not an error + return &bytes.Buffer{}, nil default: - // Now, here we need to check. If the error is not found, then it means - // there is no attestation data, not that there is an error - err1, err2, _ := strings.Cut(err.Error(), "\n") - if strings.Contains(err1, "file does not exist") && - strings.Contains(err2, "file does not exist") { - return &bytes.Buffer{}, nil - } return nil, err } } +// noteMissing returns true when the error returned reading the note locators +// means the commit has no note rather than that reading failed: either the +// repository has no notes reference yet or none of the locators point to an +// existing file. +func noteMissing(err error) bool { + var list *vcslocator.ErrorList + if !errors.As(err, &list) { + return errors.Is(err, vcslocator.ErrRefNotFound) + } + + found := false + for _, e := range list.Errors { + if e == nil { + continue + } + if !errors.Is(e, vcslocator.ErrRefNotFound) && !errors.Is(e, fs.ErrNotExist) { + return false + } + found = true + } + return found +} + // FetchBySubject calls the attestation reader with a filter preconfigured // with subject hashes. func (c *Collector) FetchBySubject(ctx context.Context, opts attestation.FetchOptions, subj []attestation.Subject) ([]attestation.Envelope, error) { diff --git a/repository/note/collector_test.go b/repository/note/collector_test.go index a7788d5..393e400 100644 --- a/repository/note/collector_test.go +++ b/repository/note/collector_test.go @@ -4,9 +4,19 @@ package note import ( + "errors" + "fmt" "io" + "io/fs" + "os" + "path/filepath" "testing" + "github.com/carabiner-dev/attestation" + "github.com/carabiner-dev/vcslocator" + git "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/require" ) @@ -64,3 +74,81 @@ func TestExtractCommitBundle(t *testing.T) { }) } } + +// TestFetchWithoutNotes reads attestations from a local repository before +// it has a notes reference, then for a commit without a note once the +// reference exists. Neither is an error, both are just empty. +func TestFetchWithoutNotes(t *testing.T) { + t.Parallel() + repoPath := filepath.Join(t.TempDir(), "repo") + repo, err := git.PlainInit(repoPath, false) + require.NoError(t, err) + wt, err := repo.Worktree() + require.NoError(t, err) + + commit := func(name string) string { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(repoPath, name), []byte(name), 0o600)) + _, err := wt.Add(name) + require.NoError(t, err) + hash, err := wt.Commit(name, &git.CommitOptions{ + Author: &object.Signature{Name: "Carabiner Test Robot", Email: "bot@carabiner.dev"}, + }) + require.NoError(t, err) + return hash.String() + } + first := commit("first.txt") + second := commit("second.txt") + + collectorFor := func(sha string) *Collector { + t.Helper() + c, err := New(WithLocator(string(vcslocator.NewFromPath(repoPath))+"@"+sha), WithPush(false)) + require.NoError(t, err) + return c + } + + // No notes reference in the repository yet + _, err = repo.Reference(plumbing.ReferenceName("refs/notes/commits"), true) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound) + fetched, err := collectorFor(first).Fetch(t.Context(), attestation.FetchOptions{}) + require.NoError(t, err) + require.Empty(t, fetched) + + // Store the first note, which creates the reference + require.NoError(t, collectorFor(first).Store( + t.Context(), attestation.StoreOptions{}, []attestation.Envelope{createTestAttestation(t)}, + )) + fetched, err = collectorFor(first).Fetch(t.Context(), attestation.FetchOptions{}) + require.NoError(t, err) + require.Len(t, fetched, 1) + + // The reference exists but the second commit has no note + fetched, err = collectorFor(second).Fetch(t.Context(), attestation.FetchOptions{}) + require.NoError(t, err) + require.Empty(t, fetched) +} + +func TestNoteMissing(t *testing.T) { + t.Parallel() + notExist := fmt.Errorf("opening path 0: %w", fs.ErrNotExist) + refMissing := fmt.Errorf("reading %q: %w", "git+https://example.com/repo@refs/notes/commits", vcslocator.ErrRefNotFound) + other := errors.New("connection refused") + for name, tc := range map[string]struct { + err error + want bool + }{ + "nil": {nil, false}, + "unrelated": {fmt.Errorf("cloning: %w", other), false}, + "ref missing": {fmt.Errorf("error cloning repositories: %w", &vcslocator.ErrorList{Errors: []error{refMissing, refMissing}}), true}, + "ref missing unwrapped": {refMissing, true}, + "both files missing": {&vcslocator.ErrorList{Errors: []error{notExist, notExist}}, true}, + "one file missing": {&vcslocator.ErrorList{Errors: []error{notExist, nil}}, true}, + "one missing one failing": {&vcslocator.ErrorList{Errors: []error{notExist, other}}, false}, + "empty list": {&vcslocator.ErrorList{Errors: []error{nil, nil}}, false}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.want, noteMissing(tc.err)) + }) + } +} diff --git a/repository/note/storer.go b/repository/note/storer.go index cac2515..fff75a4 100644 --- a/repository/note/storer.go +++ b/repository/note/storer.go @@ -151,7 +151,7 @@ func (c *Collector) openOrCloneRepoForNotes(components *vcslocator.Components) ( }) if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { // Notes ref may not exist yet, which is fine - if !strings.Contains(err.Error(), "couldn't find remote ref") { + if !errors.Is(err, git.NoMatchingRefSpecError{}) { return nil, fmt.Errorf("fetching notes: %w", err) } }