fix(handlers): return 416 for a malformed chunk Content-Range header (!2962) · Merge requests · GitLab.org / container-registry

What does this MR do?

A blob chunk PATCH whose Content-Range header cannot be parsed now gets 416 Requested Range Not Satisfiable with CONTENT_RANGE_INVALID, instead of 500 Internal Server Error with UNKNOWN.

PatchBlobData hands the header to parseContentRange, and when that fails (the value does not split on - into exactly two parts, or a part is not a decimal number) the handler wrapped the error in errcode.ErrorCodeUnknown (blobupload.go#L190-195), as it has since the range check was added in fae36b77 (fix(v2): handle content range header during layer chunk upload, 2023-08-17). So Content-Range: bogus, 0-x or bytes=0-9 got a 500, and a client that retries on 5xx kept sending a request that can never succeed. Each of those requests also counted as a server error and, where error reporting is configured, was sent to Sentry, since logError reports only UNKNOWN errors (app.go#L1615-1626). The same handler already returns CONTENT_RANGE_INVALID a few lines below for a range that does not start at the upload offset, and the API specification documents 416 for PATCH Blob Upload when the Content-Range "cannot be accepted, either because it does not overlap with the current progress or it is invalid" (api.md#L4496). The OCI distribution spec requires the range to match ^[0-9]+-[0-9]+$ and answers an out-of-order chunk with 416.

The change is one line: the parse error now goes into v2.ErrorCodeInvalidContentRange.WithDetail(...), so the detail still says what was wrong with the header. Requests without the header and requests with a well-formed range behave as before.

TestBlobAPI_Patch_InvalidContentRange in api_integration_test.go starts an upload and sends a 10-byte chunk with five malformed ranges (bogus, x-9, 0-x, bytes=0-9, 0-4-9), expecting 416 with CONTENT_RANGE_INVALID in the body. A sixth case sends 0-9 and must still get 202 with Range: 0-9.

Behavior change: a client that sends a malformed Content-Range gets 416 where it got 500. Nothing that succeeded before fails now.

Three things I left as they are. The end of the range is still not compared with the body length (0-100 with a 10-byte body is accepted and answered with Range: 0-9), and a range whose end is below its start is still accepted when its start matches the offset. I kept the second one on purpose: this repository's own client sends <offset>-<offset - 1> for an empty write (blob_writer.go#L75), and the server accepts that today (10-9 after a 10-byte chunk gets 202), so rejecting it could break that client. At offset 0 the same header is 0--1, which does not parse, so it is the one value from that client that moves from 500 to 416 with this change. The third is the Content-Type check just above the range check, which also answers a client mistake (a type other than application/octet-stream) with UNKNOWN and a 500 (blobupload.go#L182-185); it needs its own choice of status code, so I kept it out of this change. I can follow up on any of the three if you want.

Closes #2986 (closed)

Related to #2125, which plans to split the legacy filesystem and database code paths of this handler: this MR changes one line in the part they share, so whichever merges second needs a small rebase.

Related to #2160, which tracks client-side failures the registry answers with a 5xx: this is one more of them, in the chunk upload handler, and it does not touch the cases listed there.

The test, and how it fails on master

On master at 2f9d3333, Go 1.26.8, go test -tags integration,handlers_test ./registry/handlers -run TestBlobAPI_Patch_InvalidContentRange -count=1 -v, without a database:

--- FAIL: TestBlobAPI_Patch_InvalidContentRange (0.01s)
    --- FAIL: TestBlobAPI_Patch_InvalidContentRange/no_separator (0.00s)
    --- FAIL: TestBlobAPI_Patch_InvalidContentRange/non_numeric_start (0.00s)
    --- FAIL: TestBlobAPI_Patch_InvalidContentRange/non_numeric_end (0.00s)
    --- FAIL: TestBlobAPI_Patch_InvalidContentRange/unit_prefix (0.00s)
    --- FAIL: TestBlobAPI_Patch_InvalidContentRange/too_many_separators (0.00s)
    --- PASS: TestBlobAPI_Patch_InvalidContentRange/valid_range_is_accepted (0.00s)

Each failing case reports expected: 416 and actual : 500, and the registry logs level=error msg="unknown error" code=UNKNOWN with details such as invalid content range format, bogus and invalid content range values, 0-x: strconv.ParseInt: parsing "x": invalid syntax. The same five cases fail the same way with the metadata database enabled.

With the change, all six cases pass, with and without the database. Mapping the parse error to BLOB_UPLOAD_INVALID instead (404), or dropping the return after it (202), makes the five malformed cases fail again.

Checks I ran locally

go test -tags integration,handlers_test ./registry/handlers -count=1 (the api job), Go 1.26.8, no database: pass.
The same with REGISTRY_DATABASE_ENABLED=true against PostgreSQL 17 (the database:api job): pass.
go test -tags integration,api_conformance_test ./registry/handlers -count=1 without a database and against PostgreSQL 17 (api:conformance, database:api-conformance): pass.
The new test with TestBlobAPI on Go 1.25.8, 1.26.8 and 1.27.2 (the CI matrix): pass. Under -race with -count=3: pass.
go test -race ./registry/handlers/... ./registry/api/... -count=1: pass.
golangci-lint run over the whole repository with golangci/golangci-lint:v2.13.1 (the version CI pins) and the project's .golangci.yml: 0 issues. go vet on the package and gofmt -l on both files: clean.
commitlint with @commitlint/config-conventional in node:lts-alpine: 0 problems.
A registry built from master and from this branch, with inmemory storage and the database disabled, answered with curl as in the issue: 500 UNKNOWN on master and 416 CONTENT_RANGE_INVALID on this branch for bogus, 0-x and bytes=0-9, and 202 on both for 0-9.

Author checklist

Documentation/resources

Code review guidelines

Go Style guidelines

Feature flags

When documentation is required

Documentation workflow

Reviewer checklist

Ensure the commit and MR tittle are still accurate.
If the change contains a breaking change, verify the breaking change label.
If the change is considered high risk, verify the label high-risk-change
Identify if the change can be rolled back safely. (note: all other reasons for not being able to rollback will be sufficiently captured by major version changes).

fix(handlers): return 416 for a malformed chunk Content-Range header

What does this MR do?

Author checklist

Reviewer checklist

Merge request reports