Close the Composer download gaps left after #430

internal/database/queries.go

@@ -359,6 +359,7 @@ func (db *DB) upsertArtifactFrom(a *Artifact, previous sql.NullString) (bool, er
 			                       created_at, updated_at)
 			VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12)
 			ON CONFLICT(version_purl, filename) DO UPDATE SET
+				upstream_url = EXCLUDED.upstream_url,
 				storage_path = EXCLUDED.storage_path,
 				content_hash = EXCLUDED.content_hash,
 				size = EXCLUDED.size,
@@ -374,6 +375,7 @@ func (db *DB) upsertArtifactFrom(a *Artifact, previous sql.NullString) (bool, er
 			                       created_at, updated_at)
 			VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
 			ON CONFLICT(version_purl, filename) DO UPDATE SET
+				upstream_url = excluded.upstream_url,
 				storage_path = excluded.storage_path,
 				content_hash = excluded.content_hash,
 				size = excluded.size,

internal/handler/composer.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"path"
 	"regexp"
 	"strings"
@@ -372,7 +373,7 @@ func (h *ComposerHandler) proxyDistURL(packageName, version, upstreamURL, distTy
 		return "", false
 	}
 	return fmt.Sprintf("%s/composer/files/%s/%s/%s/%s",
-		h.proxyURL, parts[0], parts[1], version, distFilename(upstreamURL, distType)), true
+		h.proxyURL, parts[0], parts[1], url.PathEscape(version), distFilename(upstreamURL, distType)), true
 }
 
 // distFilename returns the file name this proxy serves an archive under: the
@@ -410,6 +411,16 @@ func upstreamDistURL(listedURL, distType, filename string, branch bool) (string,
 	if filename == listed {
 		return listedURL, true
 	}
+	if commit, ok := shaQueryCommit(listed); ok {
+		requested, ok := shaQueryCommit(filename)
+		listedSha, requestedSha := "sha="+commit, "sha="+requested
+		idx := strings.LastIndex(listed, listedSha)
+		if !branch || !ok || idx < 0 || listed[:idx]+requestedSha+listed[idx+len(listedSha):] != filename {
+			return "", false
+		}
+		idx = strings.LastIndex(listedURL, listedSha)
+		return listedURL[:idx] + requestedSha + listedURL[idx+len(listedSha):], true
+	}
 	ext := path.Ext(listed)
 	commit := strings.TrimSuffix(listed, ext)
 	requested, ok := strings.CutSuffix(filename, ext)
@@ -420,6 +431,22 @@ func upstreamDistURL(listedURL, distType, filename string, branch bool) (string,
 	return listedURL[:idx] + requested + listedURL[idx+len(commit):], true
 }
 
+// shaQueryCommit returns the commit an archive name passes in a sha query
+// parameter. GitLab names the archive of every commit archive.zip and picks
+// the commit that way.
+func shaQueryCommit(filename string) (string, bool) {
+	_, query, ok := strings.Cut(filename, "?")
+	if !ok {
+		return "", false
+	}
+	values, err := url.ParseQuery(query)
+	if err != nil {
+		return "", false
+	}
+	commit := values.Get("sha")
+	return commit, composerCommitPattern.MatchString(commit)
+}
+
 // composerFields holds one version's fields as Composer sees them after
 // expanding the minified format: raw values under their decoded names, in the
 // order the names first appeared. A removed field keeps its slot with a nil
@@ -544,8 +571,7 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request)
 
 	// The archive is cached under the file name from the path, so fetch the
 	// one that name stands for. Compare it as sent, the way rewriteDist handed
-	// it out. A query such as GitLab's ?sha= is compared but is not part of
-	// the cache key.
+	// it out, query included.
 	requested := path.Base(r.URL.EscapedPath())
 	if r.URL.RawQuery != "" {
 		requested += "?" + r.URL.RawQuery
@@ -564,16 +590,60 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request)
 		"package", packageName, "version", version,
 		"download_url", downloadURL)
 
+	// Archives picked by ?sha= share one file name, so the commit goes into
+	// the name they are cached under. Proxy versions before that cached every
+	// commit under the shared name, which is dropped once a commit is fetched.
+	var sharedFilename string
+	if commit, ok := shaQueryCommit(requested); ok {
+		sharedFilename = h.routes.cacheFilename(packageName, filename)
+		ext := path.Ext(filename)
+		filename = strings.TrimSuffix(filename, ext) + "-" + commit + ext
+	}
 	cacheFilename := h.routes.cacheFilename(packageName, filename)
+	if commit := strings.TrimSuffix(filename, path.Ext(filename)); isDevVersion(version) && composerCommitPattern.MatchString(commit) {
+		h.discardOtherCommit(packageName, version, cacheFilename, commit)
+	}
 	result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "composer", packageName, version, cacheFilename, downloadURL)
 	if err != nil {
 		h.proxy.serveArtifactError(w, err, "failed to fetch package")
 		return
 	}
+	if sharedFilename != "" && !result.Cached {
+		if err := h.proxy.ClearCachedArtifact("composer", packageName, version, sharedFilename); err != nil {
+			h.proxy.Logger.Warn("failed to discard composer archive cached under a shared name",
+				"package", packageName, "version", version, "filename", sharedFilename, "error", err)
+		}
+	}
 
 	ServeArtifactRequest(w, r, result)
 }
 
+// discardOtherCommit clears a cached branch archive named for commit that was
+// fetched from another commit's URL. Proxy versions before the pinned-commit
+// fix cached the branch head under whatever commit a lock file asked for. The
+// record is only cleared while it still points at the object checked here.
+func (h *ComposerHandler) discardOtherCommit(packageName, version, cacheFilename, commit string) {
+	_, versionPURL, err := packagePURLStrings("composer", packageName, version)
+	if err != nil {
+		return
+	}
+	record, err := h.proxy.DB.GetArtifact(versionPURL, cacheFilename)
+	if err != nil {
+		h.proxy.Logger.Warn("failed to read composer cache record",
+			"purl", versionPURL, "filename", cacheFilename, "error", err)
+		return
+	}
+	if record == nil || !record.StoragePath.Valid || strings.Contains(record.UpstreamURL, commit) {
+		return
+	}
+	h.proxy.Logger.Warn("cached composer archive is from another commit, discarding",
+		"purl", versionPURL, "filename", cacheFilename, "upstream_url", record.UpstreamURL)
+	if err := h.proxy.DB.DiscardArtifact(versionPURL, cacheFilename, record.StoragePath.String); err != nil {
+		h.proxy.Logger.Warn("failed to discard composer cache record",
+			"purl", versionPURL, "filename", cacheFilename, "error", err)
+	}
+}
+
 // isDevVersion reports whether a Composer version string refers to a
 // development (unstable, branch) version rather than a tagged release.
 // Composer formats these as either "dev-<branch>" (e.g. "dev-master") or