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) <noreply@anthropic.com>
This commit is contained in:
@@ -92,8 +92,8 @@ func (d *DB) PushManifest(p PushManifestParams) error {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Step b: insert or update manifest.
|
// Step b: insert or update manifest.
|
||||||
// Use INSERT OR REPLACE on the UNIQUE(repository_id, digest) constraint.
|
// Use INSERT ... ON CONFLICT DO UPDATE on the UNIQUE(repository_id, digest) constraint.
|
||||||
result, err := tx.Exec(
|
_, err = tx.Exec(
|
||||||
`INSERT INTO manifests (repository_id, digest, media_type, content, size)
|
`INSERT INTO manifests (repository_id, digest, media_type, content, size)
|
||||||
VALUES (?, ?, ?, ?, ?)
|
VALUES (?, ?, ?, ?, ?)
|
||||||
ON CONFLICT(repository_id, digest) DO UPDATE SET
|
ON CONFLICT(repository_id, digest) DO UPDATE SET
|
||||||
@@ -106,10 +106,22 @@ func (d *DB) PushManifest(p PushManifestParams) error {
|
|||||||
_ = tx.Rollback()
|
_ = tx.Rollback()
|
||||||
return fmt.Errorf("db: insert manifest: %w", err)
|
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 {
|
if err != nil {
|
||||||
_ = tx.Rollback()
|
_ = 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.
|
// Step c: populate manifest_blobs join table.
|
||||||
|
|||||||
@@ -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) {
|
func TestPushManifestIdempotent(t *testing.T) {
|
||||||
d := openTestDB(t)
|
d := openTestDB(t)
|
||||||
if err := d.Migrate(); err != nil {
|
if err := d.Migrate(); err != nil {
|
||||||
|
|||||||
Reference in New Issue
Block a user