From 61b8c2fcefda39e886ee6e020491f45bc1b78d4e Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Thu, 26 Mar 2026 12:35:39 -0700 Subject: [PATCH] Fix manifest push 500: use explicit SELECT instead of LastInsertId SQLite's last_insert_rowid() only updates on actual INSERTs, not ON CONFLICT DO UPDATE. When pushing a second tag for an existing manifest digest, the upsert fires the conflict branch (no new row), so LastInsertId() returns a stale ID from a previous insert. This caused manifest_blobs and tags to reference the wrong manifest, producing a 500 on the PUT manifest response. Replace LastInsertId() with a SELECT id WHERE repository_id AND digest query within the same transaction. Security: manifest_blobs and tag foreign keys now always reference the correct manifest. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/db/push.go | 20 ++++++++-- internal/db/push_test.go | 85 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 4 deletions(-) diff --git a/internal/db/push.go b/internal/db/push.go index 7129c48..06d2aee 100644 --- a/internal/db/push.go +++ b/internal/db/push.go @@ -92,8 +92,8 @@ func (d *DB) PushManifest(p PushManifestParams) error { } // Step b: insert or update manifest. - // Use INSERT OR REPLACE on the UNIQUE(repository_id, digest) constraint. - result, err := tx.Exec( + // Use INSERT ... ON CONFLICT DO UPDATE on the UNIQUE(repository_id, digest) constraint. + _, err = tx.Exec( `INSERT INTO manifests (repository_id, digest, media_type, content, size) VALUES (?, ?, ?, ?, ?) ON CONFLICT(repository_id, digest) DO UPDATE SET @@ -106,10 +106,22 @@ func (d *DB) PushManifest(p PushManifestParams) error { _ = tx.Rollback() return fmt.Errorf("db: insert manifest: %w", err) } - manifestID, err := result.LastInsertId() + + // Retrieve the manifest ID by querying the row directly. We cannot use + // result.LastInsertId() here because SQLite's last_insert_rowid() is + // unreliable after an ON CONFLICT DO UPDATE — it returns the rowid of + // the most recent *insert* in the connection, not the upserted row. + // When the conflict branch fires (no new row inserted), the stale + // last_insert_rowid from a previous insert is returned, causing + // manifest_blobs and tags to reference the wrong manifest. + var manifestID int64 + err = tx.QueryRow( + `SELECT id FROM manifests WHERE repository_id = ? AND digest = ?`, + repoID, p.Digest, + ).Scan(&manifestID) if err != nil { _ = tx.Rollback() - return fmt.Errorf("db: manifest last insert id: %w", err) + return fmt.Errorf("db: get manifest id after upsert: %w", err) } // Step c: populate manifest_blobs join table. diff --git a/internal/db/push_test.go b/internal/db/push_test.go index d06086b..2f39422 100644 --- a/internal/db/push_test.go +++ b/internal/db/push_test.go @@ -243,6 +243,91 @@ func TestPushManifestTagMove(t *testing.T) { } } +func TestPushManifestSecondTagSameDigest(t *testing.T) { + d := openTestDB(t) + if err := d.Migrate(); err != nil { + t.Fatalf("Migrate: %v", err) + } + + // Insert blobs. + if err := d.InsertBlob("sha256:cfgA", 50); err != nil { + t.Fatalf("InsertBlob cfgA: %v", err) + } + if err := d.InsertBlob("sha256:lyrA", 1000); err != nil { + t.Fatalf("InsertBlob lyrA: %v", err) + } + + contentA := []byte(`{"schemaVersion":2,"image":"A"}`) + digestA := "sha256:digestA" + + // Push app:latest — creates manifest id=1. + if err := d.PushManifest(PushManifestParams{ + RepoName: "app", + Digest: digestA, + MediaType: "application/vnd.oci.image.manifest.v1+json", + Content: contentA, + Size: int64(len(contentA)), + Tag: "latest", + BlobDigests: []string{"sha256:cfgA", "sha256:lyrA"}, + }); err != nil { + t.Fatalf("Push app:latest: %v", err) + } + + // Push a different manifest to a different repo to advance the + // autoincrement counter, simulating normal production traffic. + if err := d.PushManifest(PushManifestParams{ + RepoName: "lib", + Digest: "sha256:digestB", + MediaType: "application/vnd.oci.image.manifest.v1+json", + Content: []byte(`{"schemaVersion":2,"image":"B"}`), + Size: 30, + Tag: "latest", + }); err != nil { + t.Fatalf("Push lib:latest: %v", err) + } + + // Push app:v1.0.0 with the SAME digest as app:latest. This triggers + // the ON CONFLICT DO UPDATE branch. Before the fix, LastInsertId() + // returned the wrong manifest ID (lib's manifest), causing the tag + // and manifest_blobs to reference the wrong manifest. + if err := d.PushManifest(PushManifestParams{ + RepoName: "app", + Digest: digestA, + MediaType: "application/vnd.oci.image.manifest.v1+json", + Content: contentA, + Size: int64(len(contentA)), + Tag: "v1.0.0", + BlobDigests: []string{"sha256:cfgA", "sha256:lyrA"}, + }); err != nil { + t.Fatalf("Push app:v1.0.0: %v", err) + } + + // Verify both tags in "app" point to the same manifest with the correct digest. + repoID, err := d.GetRepositoryByName("app") + if err != nil { + t.Fatalf("GetRepositoryByName: %v", err) + } + + mLatest, err := d.GetManifestByTag(repoID, "latest") + if err != nil { + t.Fatalf("GetManifestByTag latest: %v", err) + } + mVersion, err := d.GetManifestByTag(repoID, "v1.0.0") + if err != nil { + t.Fatalf("GetManifestByTag v1.0.0: %v", err) + } + + if mLatest.ID != mVersion.ID { + t.Fatalf("tags point to different manifests: latest=%d, v1.0.0=%d", mLatest.ID, mVersion.ID) + } + if mLatest.Digest != digestA { + t.Fatalf("latest digest: got %q, want %q", mLatest.Digest, digestA) + } + if mVersion.Digest != digestA { + t.Fatalf("v1.0.0 digest: got %q, want %q", mVersion.Digest, digestA) + } +} + func TestPushManifestIdempotent(t *testing.T) { d := openTestDB(t) if err := d.Migrate(); err != nil {