[simonchrz]

Two cache keys that more than one URL could share:

• Debian: a package was cached under its repository and file name but fetched from its full path. Docker's apt repository has a pool under each suite with the same file names (dists/bullseye/pool/.../containerd.io_1.7.25-1_amd64.deb, dists/bookworm/pool/...), so a bookworm client got the bullseye build that was fetched first and failed apt's hash check until the entry was evicted. Packages are now cached under their path. deb.debian.org wasn't affected, since its pool file names are unique.
• Conan: revision, package reference, package revision and file name were joined with _, which revisions, user and channel can contain, so .../revisions/R_P_Q/files/conan_package.tgz and .../revisions/R/packages/P/revisions/Q/files/conan_package.tgz shared an entry. They're now joined with /. Exploiting this needs someone who can make the upstream serve the first URL, e.g. upload rights on a shared private remote.

Entries cached under the old names are still served, but only for the URL recorded with them (cacheFilenameKeepingLegacy), so warm caches keep working and a colliding URL fetches its own file. The tests cover both collisions and the old entries.

Summary by CodeRabbit

• Bug Fixes
  • Fixed artifact cache mix-ups when different Conan recipe or package references use similar names.
  • Fixed Debian downloads with identical package filenames in different repository paths returning the wrong artifact.
  • Preserved access to previously cached artifacts for the URL they were originally downloaded from.
  • Conan requests containing encoded slashes in recipe, package, or filename paths now return HTTP 400 instead of being processed.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Conan and Debian downloads now use cache filenames that preserve path components. A shared helper checks whether a legacy cache entry belongs to the requested upstream URL before reusing it. Conan routes reject encoded slashes with HTTP 400. Tests cover path collisions, legacy cache lookup, and updated retention fixtures.

Changes: Artifact cache keys

Layer / File(s)	Summary
Conan cache key selection internal/handler/handler.go, internal/handler/conan.go, internal/handler/conan_test.go, internal/server/retention_conan_test.go	Conan recipe and package archives use slash-separated cache filenames. Routes reject slash-containing path fields with HTTP 400. Legacy filenames are reused only when the cached artifact record matches the requested URL. Tests cover lookalike paths and legacy cache behavior; retention fixtures use the revised paths.
Debian cache key selection internal/handler/debian.go, internal/handler/debian_test.go, internal/server/retention_deb_test.go	Debian archive cache filenames include package paths. Legacy filenames are reused only for the URL associated with the cached record. Tests cover same-name packages in different paths and legacy cache behavior; retention fixtures include package paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

• git-pkgs/proxy#678: Changes RPM cache keys to use full repository paths, similar to the Debian and Conan cache identity changes here.

Suggested reviewers: pinguinfuss, andrew

Merge Risk: Minimal · up to 3e50f

No merge-blocking issue was identified in the cache-key changes; normal checks can proceed.

Security architecture risk: Low · up to 3e50f

Path-aware keys separate downloads that previously shared cache entries, while matching legacy entries remain usable. No introduced security concern was established in the inspected paths. Confidence is limited by incomplete coverage and unverified mixed-version deployment behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant security outcome is substitution of upstream artifact bytes for clients sharing an artifact cache identity. Exposure is bounded by the version PURL and filename lookup, including Conan reference components or Debian repository and path. The inspected change does not establish any new credential, host or infrastructure privilege.

Security Findings and Attack Paths

• inferred — No introduced attack path was established for the inspected collision scenarios. A proposed cross-generation Conan collision was rejected: historical route-created artifact keys always place an underscore before a fixed eligible archive filename, whereas valid new keys place a slash there. Historical encoded slashes in other fields do not remove that distinction.

Trust Boundaries and Controls

• observed — Legacy reuse is bound to exact recorded upstream URL equality, not merely a matching legacy filename. This comparison occurs before the ordinary cache lookup. New primary-key hits continue to trust database identity without a separate URL comparison, so correct key construction remains the isolation control for those hits.

Resilience and Maintainability Implications

• inferred — Within the inspected head-side writers, a mismatched legacy record selects the new key rather than overwriting or reusing the colliding legacy identity. Repeated requests therefore remain separated across normal fetches and recovery from ordinary fetch failures. This conclusion does not cover concurrent writers running older code.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the main change: Debian and Conan archive cache keys now include the full path.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 7 files.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: a3a3b69a-11e9-4fe6-81bf-55aa3908e479

Commits

Reviewing files that changed from the base of the PR and between 4c3c925 and 3e50f45.

Files selected for processing: 3

• internal/handler/conan.go
• internal/handler/conan_test.go
• internal/handler/handler.go

Files skipped from review as they are similar to previous changes: 1

• internal/handler/handler.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

[andrew]

Both collisions are real and keying by path is the right fix. The legacy handling looks good too: old entries are only reused when the recorded upstream URL matches, and the tests cover that through the routes.

One thing to fix before merging. The new Conan keys still collide when a path segment contains an encoded slash. r.PathValue decodes %2F, so

/v2/conans/zlib/1.3/_/_/revisions/R%2Fpackages%2FP%2FQ/files/conan_package.tgz

matches the recipe route with revision set to R/packages/P/Q, and conanStorageFilename returns _/_/R/packages/P/Q/conan_package.tgz. That is the same key as the package file at revisions/R/packages/P/revisions/Q/files/conan_package.tgz. Before this change those two requests had different keys. The upstream would have to serve /revisions/R/packages/P/Q/files/conan_package.tgz for it to matter, which is not a Conan route, so it is narrow, but the point of the change is that the key can't be shared.

In handleRecipeFile and handlePackageFile (internal/handler/conan.go), reject the request with a 400 when any of user, channel, revision, pkgref, pkgrev or filename contains a /, and add a test that requests the %2F URL above. The doc comment on conanStorageFilename says the parts can't contain a slash, which is only true once that check is there.

The Debian side is fine: the key and the download URL are both built from the same decoded path.

Smaller: the doc comment on cacheFilenameKeepingLegacy in internal/handler/handler.go is five lines and the first sentence is hard to follow. Two lines would do: it returns legacy when an artifact recorded under it was fetched from downloadURL, and filename otherwise.

[simonchrz]

Fixed in 3e50f45.

• handleRecipeFile and handlePackageFile answer 400 when user, channel, revision, pkgref, pkgrev or filename contains a /. The check runs before the archive check, so it covers the small files too.
• TestConanFileRoutesRefuseEncodedSlash requests the %2F URL from your review after caching the package file, plus one URL per field. Without the check the first one returned the package file.
• The comment on conanStorageFilename now says the handlers refuse a slash, and the one on cacheFilenameKeepingLegacy is two lines.