Validate checkpoint ID in RefName (reject malformed refs)

bc296fd·

Soph·2w ago·8 files·+80 added/-26 removed

RefName previously returned refs/entire/checkpoints// for an empty/invalid checkpoint ID, and callers built on the convention that the ID was already validated. Make RefName return (plumbing.ReferenceName, error), erroring when the ID is empty or an unrecognized format, so a bad ID can't silently become a malformed ref that gets pushed/fetched/looked up. Store call sites (refBase/setRef/resolveRefMaybeFetch/GetCheckpointAuthor) and the explain-on-clone fetch propagate the error; tests use a mustRefName helper for known-valid IDs and add a RefName_RejectsInvalidID case.

Sessions

Changes

8

• cmd/entire/cli

• checkpoint

• Mpushqueue_test.go+6/-6

• Mrefs_naming.go+11/-2

• Mrefs_naming_test.go+21/-2

• Mrefs_store.go+18/-4

• Mrefs_store_seam_test.go+1/-1

• Mrefs_store_test.go+7/-7

• Mexplain_export.go+5/-1

• strategy

• Mrefs_push_test.go+11/-3

13 unmodified lines

14
15
16
17
18
17
18
19
20
21
29 unmodified lines

51
52
53
54
54
55
56
57
7 unmodified lines

65
66
67
68
69
68
69
70
71
72
14 unmodified lines

87
88
89
90
90
91
92
93

13 unmodified lines

t.Parallel()
q := NewPushQueue(t.TempDir())

a := RefName("a1b2c3d4e5f6")
b := RefName("b2c3d4e5f6a1")
a := mustRefName(t, "a1b2c3d4e5f6")
b := mustRefName(t, "b2c3d4e5f6a1")

// Empty queue drains to nothing.
refs, err := q.Drain()
29 unmodified lines

func TestPushQueue_DrainDedupes(t *testing.T) {
t.Parallel()
q := NewPushQueue(t.TempDir())
a := RefName("a1b2c3d4e5f6")
a := mustRefName(t, "a1b2c3d4e5f6")

require.NoError(t, q.Enqueue(a))
require.NoError(t, q.Enqueue(a))
7 unmodified lines

func TestPushQueue_RemovePreservesLaterEntries(t *testing.T) {
t.Parallel()
q := NewPushQueue(t.TempDir())
a := RefName("a1b2c3d4e5f6")
b := RefName("b2c3d4e5f6a1")
a := mustRefName(t, "a1b2c3d4e5f6")
b := mustRefName(t, "b2c3d4e5f6a1")

// Simulate: drain sees [a], then b is enqueued during the push, then we
// Remove(a). b must survive for the next pre-push.
14 unmodified lines

t.Parallel()
dir := t.TempDir()
q := NewPushQueue(dir)
a := RefName("a1b2c3d4e5f6")
a := mustRefName(t, "a1b2c3d4e5f6")
require.NoError(t, q.Enqueue(a))

// Append a garbage line + a blank line directly.

Mcmd/entire/cli/checkpoint/pushqueue_test.go+6/-6

1
2
3
4
5
6
7
11 unmodified lines

19
20
21
21
22
22
23
24
25
26
27
28
29
30
31
32
33
34

package checkpoint

import (
    "fmt"
    "strings"

"github.com/go-git/go-git/v6/plumbing"

11 unmodified lines

// refs/entire/checkpoints/<shard>/<id>, where <shard> is id.ShardFor() (the
// first two chars for legacy hex IDs, the last two for ULIDs). The full ID is
// always the leaf, so the ref round-trips through ParseRef.
func RefName(cid id.CheckpointID) plumbing.ReferenceName {
    return plumbing.ReferenceName(CheckpointRefPrefix + cid.ShardFor() + "/" + cid.String())
// It errors on an empty or unrecognized checkpoint ID rather than returning a
// malformed ref (e.g. "refs/entire/checkpoints//"), so callers at trust
// boundaries — and future ones — can't silently push, fetch, or look up a bad
// ref.
func RefName(cid id.CheckpointID) (plumbing.ReferenceName, error) {
    if cid.Kind() == id.KindUnknown {
        return "", fmt.Errorf("cannot build checkpoint ref: invalid checkpoint ID %q", cid)
    }
    return plumbing.ReferenceName(CheckpointRefPrefix + cid.ShardFor() + "/" + cid.String()), nil
}

// ParseRef extracts the checkpoint ID from a per-checkpoint ref name,

Mcmd/entire/cli/checkpoint/refs_naming.go+11/-2

4 unmodified lines

5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
17 unmodified lines

41
42
43
35
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
55 unmodified lines

117
118
119
101
120
121
122
123

4 unmodified lines

"github.com/go-git/go-git/v6/plumbing"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"

"github.com/entireio/cli/cmd/entire/cli/checkpoint/id"

// mustRefName is a test helper for the common case of a known-valid checkpoint ID.
func mustRefName(t *testing.T, cid id.CheckpointID) plumbing.ReferenceName {
    t.Helper()
    ref, err := RefName(cid)
    require.NoError(t, err)
    return ref
}

func TestRefName(t *testing.T) {
t.Parallel()

17 unmodified lines

for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            t.Parallel()
            assert.Equal(t, tt.want, RefName(tt.cid))
            got, err := RefName(tt.cid)
            require.NoError(t, err)
            assert.Equal(t, tt.want, got)
        })
    }
}

func TestRefName_RejectsInvalidID(t *testing.T) {
t.Parallel()
    for _, cid := range []id.CheckpointID{"", "not-an-id", "A1B2C3D4E5F6"} {
        _, err := RefName(cid)
        assert.Error(t, err, "RefName(%q) should error rather than build a malformed ref", cid)
    }
}

func TestParseRef(t *testing.T) {
t.Parallel()

55 unmodified lines

if tt.wantOK {
            assert.Equal(t, tt.wantID, gotID)
            // Round-trip: building the ref from the parsed ID reproduces it.
            assert.Equal(t, tt.ref, RefName(gotID))
            assert.Equal(t, tt.ref, mustRefName(t, gotID))
        } else {
            assert.Equal(t, id.EmptyCheckpointID, gotID)
        }
    }
}

Mcmd/entire/cli/checkpoint/refs_naming_test.go+21/-2

73 unmodified lines

74
75
76
77
77
78
79
80
81
82
83
84
13 unmodified lines

98
99
100
97
101
102
103
104
105
106
107
150 unmodified lines

258
259
260
254
261
262
263
264
265
266
267
111 unmodified lines

379
380
381
372
382
383
384
385
386
387
388
389

73 unmodified lines

// next write) and subtree object (the checkpoint's current contents). A missing
// ref yields (ZeroHash, nil) so the next write becomes an orphan commit.
func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree, error) {
    ref, err := s.repo.Reference(RefName(cid), true)
    refName, err := RefName(cid)
    if err != nil {
        return plumbing.ZeroHash, nil, err
    }
    ref, err := s.repo.Reference(refName, true)
    if err != nil {
        return plumbing.ZeroHash, nil, nil //nolint:nilerr // no ref yet → new checkpoint
    }
}

// not fail condensation. The ref is still local; only its remote sync is missed
// until a later write to the same checkpoint re-enqueues it.
func (s *gitRefsStore) setRef(ctx context.Context, cid id.CheckpointID, hash plumbing.Hash) error {
    refName := RefName(cid)
    refName, err := RefName(cid)
    if err != nil {
        return err
    }
    if err := s.repo.Storer.SetReference(plumbing.NewHashReference(refName, hash)); err != nil {
        return fmt.Errorf("set checkpoint ref %s to %s: %w", refName, hash, err)
    }
}

150 unmodified lines

// checkpoint may have been written on another machine). A failed fetch returns
// the original not-found error so the read resolves to ErrCheckpointNotFound.
func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.CheckpointID) (*plumbing.Reference, error) {
    refName := RefName(cid)
    refName, err := RefName(cid)
    if err != nil {
        return nil, err
    }
    ref, err := s.repo.Reference(refName, true)
    if err == nil {
        return ref, nil
    }
}

111 unmodified lines

if err := ctx.Err(); err != nil {
    return Author{}, err //nolint:wrapcheck // Propagating context cancellation
}
ref, err := s.repo.Reference(RefName(checkpointID), true)
refName, err := RefName(checkpointID)
if err != nil {
    return Author{}, nil //nolint:nilerr // invalid ID → unknown author
}
ref, err := s.repo.Reference(refName, true)
if err != nil {
    return Author{}, nil //nolint:nilerr // no ref → unknown author
}

Mcmd/entire/cli/checkpoint/refs_store.go+18/-4

64 unmodified lines

65
66
67
68
68
69
70
71

64 unmodified lines

t.Run("git-refs primary", func(t *testing.T) {
        assertSeamVariants(t, stores.Persistent, cid, CheckpointVersionRefsV1)
        // The primary is the per-checkpoint-ref store, not a fan-out of nothing.
        _, err := repo.Reference(RefName(cid), true)
        _, err := repo.Reference(mustRefName(t, cid), true)
        assert.NoError(t, err, "primary should have written the per-checkpoint ref")
    })

Mcmd/entire/cli/checkpoint/refs_store_seam_test.go+1/-1

50 unmodified lines

51
52
53
54
54
55
56
57
3 unmodified lines

61
62
63
64
64
65
66
67
68
69
70
70
71
72
73
46 unmodified lines

120
121
122
123
123
124
125
126
34 unmodified lines

161
162
163
164
164
165
166
167
44 unmodified lines

212
213
214
215
215
216
217
218
3 unmodified lines

222
223
224
225
225
226
227
228

50 unmodified lines

require.NoError(t, err)
    refs, err := q.Drain()
    require.NoError(t, err)
    assert.Contains(t, refs, RefName(cid), "a session write should enqueue its checkpoint ref for push")
    assert.Contains(t, refs, mustRefName(t, cid), "a session write should enqueue its checkpoint ref for push")
}

func TestGitRefsStore_OnDemandRefFetch(t *testing.T) {
3 unmodified lines

cid := id.MustCheckpointID("a1b2c3d4e5f6")

refsWrite(t, store, cid, "sess-1", "transcript")
    ref, err := store.repo.Reference(RefName(cid), true)
    ref, err := store.repo.Reference(mustRefName(t, cid), true)
    require.NoError(t, err)
    commitHash := ref.Hash()

// Simulate "not present locally" by dropping the ref (the commit object
    // survives, so a fetch can restore the ref).
    require.NoError(t, store.repo.Storer.RemoveReference(RefName(cid)))
    require.NoError(t, store.repo.Storer.RemoveReference(mustRefName(t, cid)))

// No fetcher configured: read resolves to not-found (nil summary).
    summary, err := store.Read(ctx, cid)
46 unmodified lines

}))

// The per-checkpoint ref exists at the sharded name.
    _, err := store.repo.Reference(RefName(cid), true)
    _, err := store.repo.Reference(mustRefName(t, cid), true)
    require.NoError(t, err, "checkpoint ref should exist")

summary, err := store.Read(ctx, cid)
34 unmodified lines

// id.CheckpointID JSON (un)marshaling to accept ULIDs, which lands with the
    // deferred ULID-generation switch.
    ulid := id.CheckpointID("01KVBJCWYA4YW6J5M9GP655HZN")
    assert.Equal(t, "refs/entire/checkpoints/ZN/01KVBJCWYA4YW6J5M9GP655HZN", RefName(ulid).String())
    assert.Equal(t, "refs/entire/checkpoints/ZN/01KVBJCWYA4YW6J5M9GP655HZN", mustRefName(t, ulid).String())
}

func TestGitRefsStore_MultipleSessions(t *testing.T) {
44 unmodified lines

refsWrite(t, store, cid, "sess-1", "t")

// First write is an orphan (no parent).
    ref, err := store.repo.Reference(RefName(cid), true)
    ref, err := store.repo.Reference(mustRefName(t, cid), true)
    require.NoError(t, err)
    first, err := store.repo.CommitObject(ref.Hash())
    require.NoError(t, err)
3 unmodified lines

require.NoError(t, store.Write(ctx, SessionSummary{
        CheckpointID: cid, Summary: &Summary{Intent: "later"},
    }))
    ref, err = store.repo.Reference(RefName(cid), true)
    ref, err = store.repo.Reference(mustRefName(t, cid), true)
    require.NoError(t, err)
    second, err := store.repo.CommitObject(ref.Hash())
    require.NoError(t, err)

Mcmd/entire/cli/checkpoint/refs_store_test.go+7/-7

205 unmodified lines

206
207
208
209
210
211
212
213
210
214
215
216
217

205 unmodified lines

// then re-list. Falls through to the v1-branch fetch below otherwise.
    if cpCfg, _ := settings.LoadCheckpointsConfig(ctx); checkpoint.PrimaryIsRefs(cpCfg) { //nolint:errcheck // fail-soft: bad config surfaces via Open elsewhere
        if cid, err := id.NewCheckpointID(prefix); err == nil {
            refName, refErr := checkpoint.RefName(cid)
            if refErr != nil {
                return nil, lookup
            }
            stop := startSpinner(errW, "Fetching checkpoint from remote")
            fetchErr := FetchCheckpointRef(ctx, checkpoint.RefName(cid))
            fetchErr := FetchCheckpointRef(ctx, refName)
            stop(false)
            if fetchErr == nil {
                if fresh, freshErr := newExplainCheckpointLookup(ctx); freshErr == nil {

Mcmd/entire/cli/explain_export.go+5/-1

15 unmodified lines

16
17
18
19
20
21
22
23
24
25
26
27
28
29
12 unmodified lines

42
43
44
37
38
45
46
47
48
49
15 unmodified lines

65
66
67
60
68
69
70
71

15 unmodified lines

"github.com/entireio/cli/cmd/entire/cli/testutil"

// mustRefName builds a checkpoint ref for a known-valid ID in tests.
func mustRefName(t *testing.T, cid id.CheckpointID) plumbing.ReferenceName {
    t.Helper()
    ref, err := checkpoint.RefName(cid)
    require.NoError(t, err)
    return ref
}

// setupRepoWithCheckpointRefs creates a work repo with two per-checkpoint refs
// pointing at HEAD, plus a fresh bare remote. Returns (workDir, bareDir, refs).
func setupRepoWithCheckpointRefs(t *testing.T) (string, string, []plumbing.ReferenceName) {
12 unmodified lines

require.NoError(t, err)

refs := []plumbing.ReferenceName{
        checkpoint.RefName(id.MustCheckpointID("a1b2c3d4e5f6")),
        checkpoint.RefName(id.MustCheckpointID("b2c3d4e5f6a1")),
        mustRefName(t, id.MustCheckpointID("a1b2c3d4e5f6")),
        mustRefName(t, id.MustCheckpointID("b2c3d4e5f6a1")),
    }
    for _, ref := range refs {
        require.NoError(t, repo.Storer.SetReference(plumbing.NewHashReference(ref, head.Hash())))
15 unmodified lines

repo, err := git.PlainOpen(workDir)
    require.NoError(t, err)

stale := checkpoint.RefName(id.MustCheckpointID("ffffffffffff"))
    stale := mustRefName(t, id.MustCheckpointID("ffffffffffff"))
    existing, missing := partitionLocalRefs(repo, append([]plumbing.ReferenceName{stale}, refs...))

assert.ElementsMatch(t, refs, existing, "local refs are pushable")