Reject npm publish over an existing version

e2e-docker/npm_local_test.go

@@ -36,7 +36,7 @@ mk() { d="/w/src/$1-$2"; mkdir -p "$d"; cd "$d"
   echo "module.exports = '$1@$2';" > index.js; }
 mk e2e-npm-plain 1.0.0 && npm publish
 mk @e2e/npm-scoped 1.0.0 && npm publish
-mk e2e-npm-plain 1.0.0 && echo "// changed" >> index.js
+mk e2e-npm-plain 1.0.0
 if npm publish 2>/dev/null; then echo "republish accepted"; exit 1; fi
 mkdir -p /w/app && cd /w/app && npm init -y >/dev/null
 npm install --no-audit --cache /tmp/fresh-cache e2e-npm-plain @e2e/npm-scoped

internal/api/v1/npm.go

@@ -176,23 +176,27 @@ func (h *ProxyHandler) handleLocalPut(w http.ResponseWriter, r *http.Request) {
 	base := pkg[strings.LastIndex(pkg, "/")+1:]
 	stem := pkg + "/-/" + base + "-" + rel.version
 	// The manifest gates visibility and tags may only name visible versions, so
-	// the order is tarball, manifest, tags. Identical bytes make retries idempotent.
-	for _, f := range []struct {
-		path, ct string
-		data     []byte
-	}{
-		{stem + ".tgz", "application/octet-stream", rel.tarball},
-		{stem + ".json", "application/json", rel.manifest},
-	} {
-		ok, err := h.npmStoreImmutable(r.Context(), remote.Name, f.path, f.data, f.ct)
-		if err != nil {
-			npmError(w, http.StatusInternalServerError, err.Error())
-			return
-		}
-		if !ok {
-			npmError(w, http.StatusConflict, fmt.Sprintf("%s@%s already exists; overwrites are not allowed", pkg, rel.version))
+	// the order is tarball, manifest, tags. A tarball left by a failed publish is
+	// accepted again only with identical bytes; an existing manifest always conflicts.
+	conflict := func() {
+		npmError(w, http.StatusConflict, fmt.Sprintf("%s@%s already exists; overwrites are not allowed", pkg, rel.version))
+	}
+	ok, err = h.npmStoreImmutable(r.Context(), remote.Name, stem+".tgz", rel.tarball, "application/octet-stream")
+	if err != nil {
+		npmError(w, http.StatusInternalServerError, err.Error())
+		return
+	}
+	if !ok {
+		conflict()
+		return
+	}
+	if _, err := h.npmStore(r.Context(), remote.Name, stem+".json", rel.manifest, "application/json", false); err != nil {
+		if errors.Is(err, database.ErrAlreadyExists) {
+			conflict()
 			return
 		}
+		npmError(w, http.StatusInternalServerError, err.Error())
+		return
 	}
 	for _, tag := range rel.tags {
 		if _, err := h.npmStore(r.Context(), remote.Name, pkg+"/-/dist-tags/"+tag, []byte(rel.version), "text/plain", true); err != nil {

internal/api/v1/npm_handler_test.go

@@ -163,8 +163,14 @@ func TestNPMPublishHandler(t *testing.T) {
 		if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("different"), publishOpts{})); code != http.StatusConflict {
 			t.Errorf("different bytes = %d, want 409", code)
 		}
-		if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v1"), publishOpts{})); code != http.StatusCreated {
-			t.Errorf("identical republish = %d, want 201", code)
+		if code, b := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v2"), publishOpts{version: "1.1.0"})); code != http.StatusCreated {
+			t.Fatalf("publish 1.1.0 = %d: %s", code, b)
+		}
+		if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v1"), publishOpts{})); code != http.StatusConflict {
+			t.Errorf("identical republish = %d, want 409", code)
+		}
+		if tags := packumentTags(t, base+"repub"); tags["latest"] != "1.1.0" {
+			t.Errorf("latest = %q, want 1.1.0", tags["latest"])
 		}
 	})
 
@@ -204,8 +210,11 @@ func TestNPMPublishHandler(t *testing.T) {
 	t.Run("concurrent identical", func(t *testing.T) {
 		body := publishBody(t, "same", []byte("same tgz"), publishOpts{})
 		codes := concurrentPuts(t, base+"same", body, body)
-		if codes[0] != http.StatusCreated || codes[1] != http.StatusCreated {
-			t.Fatalf("codes = %v", codes)
+		if codes[0]+codes[1] != http.StatusCreated+http.StatusConflict || codes[0] == codes[1] {
+			t.Fatalf("codes = %v, want one 201 and one 409", codes)
+		}
+		if tags := packumentTags(t, base+"same"); len(tags) != 1 || tags["latest"] != "1.0.0" {
+			t.Errorf("tags = %v", tags)
 		}
 	})