feat(oci): upstream URL, segment safety, name transform (S16 step 3)

What this ships

S16 Step 3 — the pure, S13-independent building blocks for container remote repositories. No route is wired to any of it; Steps 12–16 compose it.

  1. Upstream URL construction (remote_upstream.go) with per-segment path escaping, so a multi-segment image name stays multi-segment: library/nginx builds <base>/v2/library/nginx/manifests/…, never library%2Fnginx. Four endpoints — manifests, blobs, tags/list, referrers.
  2. The outbound reference gate, applying S12's tag grammar to a non-digest <reference> before it reaches an upstream URL or a cache lookup. It sits in the same file as the builder because it exists to guard the interpolation the builder performs. A sha256:-prefixed reference with a malformed hex portion routes to 400 DIGEST_INVALID through the same readReferenceDigestInvalid helper the hosted read path uses; everything else failing the grammar routes to the 404 MANIFEST_UNKNOWN sentinel.
  3. The tags/list name rewrite (remote_nametransform.go) as a constant-memory io.Reader decorator: the root object's name becomes the AR-facing path, every other byte is relayed verbatim. Pure bytes, no HTTP, which is why it lands here rather than in the handler step — the same place S15 puts its dist.tarball transform, for the same reason.

Two fuzz targets, both wired into fuzz:oci so they run coverage-guided rather than replaying a seed corpus: FuzzOutboundUpstreamURL and FuzzTagListNameRewrite. The job's changes: anchor gains internal/remote/**/*, matching the anchor fuzz:npm and fuzz:maven already carry, because the URL target reaches remote.ParseUpstreamBase and would fail at fuzz setup if the shared gate's refusal set changed.

Size

The plan estimates Step 3 at ~880 lines "roughly half of it table-driven tests". That estimate is wrong by about 3.5x, and the actual diff is 3,109 insertions across 7 files — 1,734 non-blank, non-comment Go lines (936 production, 2,126 test raw). This is above the 500-line ceiling in development-model.md, so per that document: here is the justification rather than a split.

Measured against the two sibling building-blocks MRs with the same filter (added Go lines, blanks and // comments stripped):

MR Reviewable Go Raw insertions Files
S14 maven Step 5 — remote building blocks 1,533 2,415 7
S15 npm Step 4a — remote building blocks 1,387 2,446 6
S15 npm Step 4b — remote rules and transform 2,254 3,775 41
S16 Step 3 (this MR) 1,734 3,109 7

This lands about 13% above S14's directly analogous MR on reviewable Go, at the same file count, and below S15's larger half. The weight is test, not production: 2,126 of the raw lines are tests, against 936 of production, which is the ratio the outbound security boundary warrants — every rejected byte class gets a row, and the refusal tables are asserted twice, once against the gate directly and once through the builder that calls it. The test share grew after review: four guards in the transform turned out to be deletable with the suite green, and the structural-mismatch, skipped-value, and truncation-tail tables that pin them are the largest single addition.

Why one MR rather than 3a/3b. Two seams exist and neither helps:

  • The file seam (URL builder + gate | name transform) is clean — the two halves share no symbol. But it splits into roughly 1,250 and 1,475 lines, so both halves stay 2–3x over the ceiling. It converts one warning into two.
  • The gate | builder seam inside remote_upstream.go is the only cut that yields a sub-500 MR (~380 for the gate and its tables). It is also precisely the split S14's own size justification rejected — "splitting would put the URL builder and its rejection tables in separate MRs, which is where the review value is lowest" — and what this plan argues against in Step 3's Files entry: the gate is in the builder's file because it guards the builder's interpolation. Separating them across two MRs means reviewing an interpolation whose guard is not in the diff.

The transform itself is one 584-line byte state machine; any cut inside it ships a half-working scanner. So the honest options are one MR at ~1,555 reviewable lines, matching what both siblings did for this exact class of work, or three MRs of which two are still over the ceiling. This is the first.

Conformance tests

Not applicable to this MR. These are pure functions with no route wired to them — no client can reach any of it until Steps 12–16 compose it into the dispatcher. Per the plan, Step 18's hermetic end-to-end proxy harness is this slice's conformance deliverable for a proxy path, as it is in both siblings.

E2E scenario catalog

No change, and deliberately so. docs/testing/e2e/oci.md lists "Virtual and remote (proxy/cache) repositories" under Out of scope until the capability ships, and it stays there for this MR: nothing here is reachable by a client, so no user journey changes. The plan assigns the catalog edit to Step 18, which moves remote out of that list and adds pull-through scenarios for a cache miss, a cache hit, a stale-tag revalidation, and a token-auth upstream, with docs/testing/e2e/docker.md getting the Docker-client-driven equivalents.

This is worth stating explicitly because it is not the answer S14 and S15 gave. Both said "no catalog exists for this format" — true for Maven and npm. An OCI catalog does exist, so the answer here is that the capability it gates has not finished shipping, and the step that finishes it owns the edit.

Notes for review

  • Copy, not import. The transform copies S15's TarballURLRewriter byte state machine including its key-candidate and nesting-depth caps; the URL builder copies S14's remote_upstream.go. Neither is imported: the linter allows it, ADR-023 does not. No internal/format/npm/... or internal/format/maven/... import appears in any new file.
  • Not npm's escaping. npm's Tarball escapes a scoped name as a single %2F segment, which is deliberately the opposite of what a multi-segment image name needs. The builder splits on / and escapes per component.
  • ErrUnsafeUpstreamSegment is coarse on purpose. It does not distinguish NAME_INVALID from DIGEST_INVALID, matching how S14 and S15 collapse error codes within one status class and split only across status classes. The plan's criterion table now assigns the remote arms' own digest check to Steps 11 and 14–16 rather than assuming S12 covers it — it does not, since S12's digest checks sit in the hosted per-action handlers, below where the kind branch goes.
  • Allocation bound is tested, not asserted by comment. The oversize-name test asserts only on emitted bytes, so a scanner that buffered the 4 MiB name and dropped it at the closing quote would pass it. TestTagListNameRewriter_BoundedAllocation measures gross allocation instead (5.4 KiB measured against a 64 KiB budget), following TestBlobGetRangeBoundedAllocation in this package. Verified by mutating the drop path to buffer and release: the new test fails at 21 MB while the byte-level one still passes.
  • The nesting cap fails closed. Past maxNameNestingDepth the scanner can no longer tell a root member from a nested one, so an open that would carry the document past the cap ends the stream with ErrTagListNestingTooDeep rather than relaying a tag list whose name it can no longer scope. Nesting to exactly the cap is still relayed, so the accepting side of the bound is unchanged. Reaching the cap takes 64 KiB of opening bytes, every one already relayed, so the refusal surfaces mid-stream: the client sees an interrupted 200, the line S16 and S14 both draw once streaming has started. The sentinel is exported for errors.Is triage — telling this refusal apart from a truncated body and a source read error — not for a status mapping.
  • npm's tarball rewriter still carries four shapes this file corrects. The decode placement, the nil-receiver guard on Transform, the one-shot output sizing, and the escape-encoded comment example were all corrected here and not in internal/format/npm/npmremote, per the copy-do-not-import rule. Fixing npm is tracked in #505 and is a separate fix(npm): MR. The receiver guard is the one worth watching: npm's Transform checks the zero value (rw.arTarballPrefix == ""), not a nil receiver, and reading that field off a nil pointer is itself the panic site, so it catches &TarballURLRewriter{} and not nil. It is latent rather than live — nothing outside that package's tests calls the constructor until npm remote is wired.

Related to #288

Edited by Radamanthus Batnag

Merge request reports

Loading
Loading