feat: enhance manifest retrieval error handling

[AKatruwar]

fix: return 404 instead of 500 for unrecognized manifest content

Addresses: https://do-internal.atlassian.net/browse/DOCR-2302

manifestStore.Get returned untyped errors when blob content was readable but was not a recognizable manifest. The GET/HEAD handler only converts distribution.ErrManifestUnknownRevision into 404 MANIFEST_UNKNOWN, so every other error fell through to errcode.ErrorCodeUnknown and returned HTTP 500. Clients that pass an image config digest to the manifests endpoint hit unrecognized manifest schema version 0 and burned the availability SLO on what is a client-side request error.

Return ErrManifestUnknownRevision, with a warning log, from the three Get paths that mean "this content is not a manifest":

• json.Unmarshal failure on the blob content
• unrecognized mediaType under schemaVersion 2, previously ErrManifestVerification, which only the PUT path inspects
• the schemaVersion fallthrough, which returned a bare fmt.Errorf and logged nothing

The blobStore.Get error path is unchanged, so genuine Spaces and storage failures still return 500.

Also fixes a reachable panic in the empty-mediaType branch. The image index result was type-asserted before its error was checked, and ocischemaIndexHandler.Unmarshal returns a nil manifest when unmarshalling fails, so malformed content panicked the request.

Behaviour is unchanged for valid manifests. This only changes which error is returned on an already-failing path, so there is no change to what is read from storage and no perf impact.

Testing: added TestManifestGetNonManifestContent covering an image config blob, an unregistered media type, and non-JSON content, each asserting ErrManifestUnknownRevision. Added TestManifestGetMalformedIndexWithoutMediaType, which panics against the pre-fix code.

Note for reviewers: TestManifestStorage already fails on master at the post-delete Get assertion, unrelated to this change. linkedBlobStore.Stat uses the global statter, so a deleted repo link does not hide a blob that still exists globally.

[Copilot]

Pull request overview

This PR adjusts manifest retrieval to return distribution.ErrManifestUnknownRevision (mapped to HTTP 404 MANIFEST_UNKNOWN) when stored blob content is readable but not a valid/recognized manifest, and fixes a panic in the schemaVersion=2 empty-mediaType branch when index unmarshalling fails.

Changes:

• Convert non-manifest content cases (invalid JSON, unrecognized schemaVersion, unrecognized schemaVersion=2 mediaType) from generic/untyped errors to ErrManifestUnknownRevision with warning logs.
• Prevent a panic by checking the OCI index unmarshal error before type-asserting the returned manifest.
• Add targeted unit tests to ensure these failure modes return ErrManifestUnknownRevision and that malformed index content does not panic.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
registry/storage/manifeststore.go	Normalizes “not a manifest” retrieval failures to ErrManifestUnknownRevision and hardens empty-mediaType index detection to avoid panics.
registry/storage/manifeststore_test.go	Adds regression tests covering non-manifest content and the previously reachable panic path.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rohinsuri]

Nice catch on the panic — type-asserting the index result before checking the error was a genuine crash, and the guard is right.

One thing I'd like fixed before this lands, in the empty-mediaType branch. Skipping the failed index unmarshal is correct, but the fallthrough to ocischemaHandler.Unmarshal succeeds on that same content: ocischema.Manifest has no manifests field, so encoding/json silently ignores it, and an empty mediaType passes the only validation there. So for {"schemaVersion":2,"manifests":"not-a-list"}, Get returns a nil error and a DeserializedManifest with a zero-value Config and nil Layers, and the API serves 200 OK with that body.

So the panic is gone, but it's replaced by a silent wrong answer: the client is handed a manifest that describes nothing, with no error and nothing in the logs to explain it. That's harder to debug than either the crash or a clean 404. Could we return the same ErrManifestUnknownRevision when the fallback doesn't produce a manifest with actual content?

Related: TestManifestGetMalformedIndexWithoutMediaType can't fail as written — it only logs if an error comes back, and per the above none does. Worth asserting the concrete expected outcome so it would catch a regression here.

[rohinsuri]

LGTM. The config-descriptor check is the right servability test, and the malformed-index test actually asserts something now. This closes the reported DOCR-2302 case — an image config blob reads as schemaVersion 0 and now returns a 404 instead of a 500.

Two non-blocking follow-ups:

1. The same two problems still exist in the four explicit media-type cases above case "":. Content like {"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json"}, or the schema2 equivalent, parses fine — both UnmarshalJSON implementations validate only the media type, not the config — so it's still served as 200 OK with an empty manifest, and a malformed body with a known media type still returns the raw parse error as a 500. Only reachable for content that was a real manifest at some point, i.e. corruption rather than the client-error case here, so happy for it to be a separate PR.

2. Before you merge, could you paste the output of go test ./registry/storage/...? There's no Go build or test job in this repo's CI, only the semgrep scan, so nothing has actually run these assertions.

[AKatruwar]

LGTM. The config-descriptor check is the right servability test, and the malformed-index test actually asserts something now. This closes the reported DOCR-2302 case — an image config blob reads as schemaVersion 0 and now returns a 404 instead of a 500.

Two non-blocking follow-ups:

1. The same two problems still exist in the four explicit media-type cases above case "":. Content like {"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json"}, or the schema2 equivalent, parses fine — both UnmarshalJSON implementations validate only the media type, not the config — so it's still served as 200 OK with an empty manifest, and a malformed body with a known media type still returns the raw parse error as a 500. Only reachable for content that was a real manifest at some point, i.e. corruption rather than the client-error case here, so happy for it to be a separate PR.
2. Before you merge, could you paste the output of go test ./registry/storage/...? There's no Go build or test job in this repo's CI, only the semgrep scan, so nothing has actually run these assertions.

Agreed on the explicit media-type branches — confirmed both deserializers validate only the media type, so {"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.v2+json"} still comes back as 200 with an empty manifest, and a malformed body with a valid media type still returns the raw parse error as a 500. The index deserializer is looser still, it does no media type check at all. Happy to take that as a separate PR.

One thing I'd adjust in the framing: it isn't only reachable through corruption. Because Stat resolves registry-wide rather than per-repository, anyone who can push a blob can upload that two-line JSON as a layer and then request its digest through the manifests endpoint — same reachability as the case this PR fixes. Impact is still low, since you get back a useless 200 for content in your own repository, so it doesn't change the priority. I'll create the follow-up.

On the test run — you were right that nothing had exercised these assertions, and it turned out to be worse than that. TestManifestStorage panics partway through the package, which kills the test binary, and both new tests are declared later in the same file, so they could never run. That panic is pre-existing on master: after Delete, the test expects Exists to be false, but with the registry-wide statter the blob still resolves, so Get succeeds and reflect.ValueOf(nil).Type() panics while reporting the failure.

I've included a one-line fix for that here, printing the error type with %T instead of via reflect. It doesn't make that test pass, it just stops it taking the whole run down with it. With that in place:

--- PASS: TestManifestGetNonManifestContent (0.00s)
    --- PASS: TestManifestGetNonManifestContent/image_config_blob_carries_no_schema_version (0.00s)
    --- PASS: TestManifestGetNonManifestContent/schema_version_2_with_an_unregistered_media_type (0.00s)
    --- PASS: TestManifestGetNonManifestContent/content_is_not_JSON_at_all (0.00s)
    --- PASS: TestManifestGetMalformedIndexWithoutMediaType (0.00s)

--- FAIL: TestSimpleBlobUpload (0.28s)
--- FAIL: TestBlobMount (0.06s)
--- FAIL: TestLinkedBlobStoreCreateWithMountFrom (0.07s)
--- FAIL: TestManifestStorage (0.07s)
FAIL	github.com/docker/distribution/registry/storage	4.911s

40 pass, 4 fail. All four fail identically on a clean master checkout — I diffed the full results before and after, and the only differences were timings and pointer addresses. They share one cause: each expects that a deleted or unmounted blob stops resolving, which the registry-wide statter no longer does. Worth a separate ticket to bring those expectations in line; I didn't want to mix that judgement call into this PR.

Note that go test ./registry/storage/... with the sub-packages can't complete today, for two unrelated pre-existing reasons. The in-memory driver suite hangs until Go's ten-minute timeout kills it, and the S3 driver's test file doesn't compile:

vet: registry/storage/driver/s3-aws/s3_test.go:971:30:
     res.Deleted undefined (type *driver.BulkDeleteOutput has no field or method Deleted)

Everything else under that tree passes. A test file that no longer compiles is probably the strongest argument for adding a Go build and test job alongside the semgrep scan — happy to open that separately if you think it's worth doing.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[rohinsuri]

Fair correction on the reachability — you're right, a pushed blob reaches it the same way as the case fixed here. One thing for the follow-up ticket: the statter resolves across the whole bucket, not just the repository, but in DOCR each registry gets its own Spaces bucket resolved per-request from the auth subject, so it doesn't cross tenants. Worth writing down so it isn't re-triaged later as a cross-tenant leak.

Good catch on the panic. The %T fix is right, and thanks for keeping the four pre-existing failures out of this PR.

Yes please on the Go build and test job. We can keep that separate.

LGTM.

[Bala-Nallamilli]

LGTM