From a8e375eb282d447db7739707a585b8b2011385af Mon Sep 17 00:00:00 2001 From: Julian Schlarb Date: Mon, 9 Jun 2025 08:43:41 +0200 Subject: [PATCH] fix: omit Content-Length on 307 redirects when serving direct manifest for containers (#8037) Containers have been refactored to use the same serve method as other packages, ensuring consistent response handling. fixes #7888 ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. ## Release notes - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/8037): omit Content-Length on 307 redirects when serving direct manifest for containers Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8037 Reviewed-by: Earl Warren Co-authored-by: Julian Schlarb Co-committed-by: Julian Schlarb --- modules/httplib/serve.go | 8 +++++ routers/api/packages/container/container.go | 35 +++++++++---------- routers/api/packages/helper/helper.go | 16 ++++----- services/context/base.go | 2 +- services/context/base_test.go | 22 ++++++++++++ .../api_packages_container_test.go | 28 +++++++++++++-- 6 files changed, 81 insertions(+), 30 deletions(-) diff --git a/modules/httplib/serve.go b/modules/httplib/serve.go index cd35367bc9..c5f0658d4e 100644 --- a/modules/httplib/serve.go +++ b/modules/httplib/serve.go @@ -35,6 +35,8 @@ type ServeHeaderOptions struct { Filename string CacheDuration time.Duration // defaults to 5 minutes LastModified time.Time + AdditionalHeaders http.Header + RedirectStatusCode int } // ServeSetHeaders sets necessary content serve headers @@ -82,6 +84,12 @@ func ServeSetHeaders(w http.ResponseWriter, opts *ServeHeaderOptions) { // http.TimeFormat required a UTC time, refer to https://pkg.go.dev/net/http#TimeFormat header.Set("Last-Modified", opts.LastModified.UTC().Format(http.TimeFormat)) } + + if opts.AdditionalHeaders != nil { + for k, v := range opts.AdditionalHeaders { + header[k] = v + } + } } // ServeData download file from io.Reader diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 5276dd5706..4d59e391a5 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -691,33 +691,30 @@ func DeleteManifest(ctx *context.Context) { func serveBlob(ctx *context.Context, pfd *packages_model.PackageFileDescriptor) { serveDirectReqParams := make(url.Values) serveDirectReqParams.Set("response-content-type", pfd.Properties.GetByName(container_module.PropertyMediaType)) - s, u, _, err := packages_service.GetPackageBlobStream(ctx, pfd.File, pfd.Blob, serveDirectReqParams) + s, u, pf, err := packages_service.GetPackageBlobStream(ctx, pfd.File, pfd.Blob, serveDirectReqParams) if err != nil { + if errors.Is(err, packages_model.ErrPackageFileNotExist) { + apiError(ctx, http.StatusNotFound, err) + return + } apiError(ctx, http.StatusInternalServerError, err) return } - headers := &containerHeaders{ - ContentDigest: pfd.Properties.GetByName(container_module.PropertyDigest), - ContentType: pfd.Properties.GetByName(container_module.PropertyMediaType), - ContentLength: pfd.Blob.Size, - Status: http.StatusOK, + opts := &context.ServeHeaderOptions{ + ContentType: pfd.Properties.GetByName(container_module.PropertyMediaType), + RedirectStatusCode: http.StatusTemporaryRedirect, + AdditionalHeaders: map[string][]string{ + "Docker-Distribution-Api-Version": {"registry/2.0"}, + }, } - if u != nil { - headers.Status = http.StatusTemporaryRedirect - headers.Location = u.String() - - setResponseHeaders(ctx.Resp, headers) - return + if d := pfd.Properties.GetByName(container_module.PropertyDigest); d != "" { + opts.AdditionalHeaders["Docker-Content-Digest"] = []string{d} + opts.AdditionalHeaders["ETag"] = []string{fmt.Sprintf(`"%s"`, d)} } - defer s.Close() - - setResponseHeaders(ctx.Resp, headers) - if _, err := io.Copy(ctx.Resp, s); err != nil { - log.Error("Error whilst copying content to response: %v", err) - } + helper.ServePackageFile(ctx, s, u, pf, opts) } // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#content-discovery @@ -725,7 +722,7 @@ func GetTagList(ctx *context.Context) { image := ctx.Params("image") if _, err := packages_model.GetPackageByName(ctx, ctx.Package.Owner.ID, packages_model.TypeContainer, image); err != nil { - if err == packages_model.ErrPackageNotExist { + if errors.Is(err, packages_model.ErrPackageNotExist) { apiErrorDefined(ctx, errNameUnknown) } else { apiError(ctx, http.StatusInternalServerError, err) diff --git a/routers/api/packages/helper/helper.go b/routers/api/packages/helper/helper.go index 99c0867bbb..f9b91d9a09 100644 --- a/routers/api/packages/helper/helper.go +++ b/routers/api/packages/helper/helper.go @@ -39,16 +39,9 @@ func LogAndProcessError(ctx *context.Context, status int, obj any, cb func(strin } } -// Serves the content of the package file +// ServePackageFile Serves the content of the package file // If the url is set it will redirect the request, otherwise the content is copied to the response. func ServePackageFile(ctx *context.Context, s io.ReadSeekCloser, u *url.URL, pf *packages_model.PackageFile, forceOpts ...*context.ServeHeaderOptions) { - if u != nil { - ctx.Redirect(u.String()) - return - } - - defer s.Close() - var opts *context.ServeHeaderOptions if len(forceOpts) > 0 { opts = forceOpts[0] @@ -59,5 +52,12 @@ func ServePackageFile(ctx *context.Context, s io.ReadSeekCloser, u *url.URL, pf } } + if u != nil { + ctx.Redirect(u.String(), opts.RedirectStatusCode) + return + } + + defer s.Close() + ctx.ServeContent(s, opts) } diff --git a/services/context/base.go b/services/context/base.go index 0275ea8a99..dc3d226bb0 100644 --- a/services/context/base.go +++ b/services/context/base.go @@ -250,7 +250,7 @@ func (b *Base) PlainText(status int, text string) { // Redirect redirects the request func (b *Base) Redirect(location string, status ...int) { code := http.StatusSeeOther - if len(status) == 1 { + if len(status) == 1 && status[0] > 0 { code = status[0] } diff --git a/services/context/base_test.go b/services/context/base_test.go index bf746766d9..9e058d8f24 100644 --- a/services/context/base_test.go +++ b/services/context/base_test.go @@ -36,6 +36,7 @@ func TestRedirect(t *testing.T) { cleanup() has := resp.Header().Get("Set-Cookie") == "i_like_gitea=dummy" assert.Equal(t, c.keep, has, "url = %q", c.url) + assert.Equal(t, http.StatusSeeOther, resp.Code) } req, _ = http.NewRequest("GET", "/", nil) @@ -47,3 +48,24 @@ func TestRedirect(t *testing.T) { assert.Equal(t, "/other", resp.Header().Get("HX-Redirect")) assert.Equal(t, http.StatusNoContent, resp.Code) } + +func TestRedirectOptionalStatus(t *testing.T) { + defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/")() + req, _ := http.NewRequest("GET", "/", nil) + + cases := []struct { + expected int + actual int + }{ + {expected: 303}, + {http.StatusTemporaryRedirect, 307}, + {http.StatusPermanentRedirect, 308}, + } + for _, c := range cases { + resp := httptest.NewRecorder() + b, cleanup := NewBaseContext(resp, req) + b.Redirect("/", c.actual) + cleanup() + assert.Equal(t, c.expected, resp.Code) + } +} diff --git a/tests/integration/api_packages_container_test.go b/tests/integration/api_packages_container_test.go index 0e977e9ae7..e3f7d010b3 100644 --- a/tests/integration/api_packages_container_test.go +++ b/tests/integration/api_packages_container_test.go @@ -430,14 +430,19 @@ func TestPackageContainer(t *testing.T) { assert.Equal(t, manifestDigest, resp.Header().Get("Docker-Content-Digest")) }) - t.Run("GetManifest", func(t *testing.T) { + t.Run("GetManifest unknown-tag", func(t *testing.T) { defer tests.PrintCurrentTest(t)() req := NewRequest(t, "GET", fmt.Sprintf("%s/manifests/unknown-tag", url)). AddTokenAuth(userToken) MakeRequest(t, req, http.StatusNotFound) + }) - req = NewRequest(t, "GET", fmt.Sprintf("%s/manifests/%s", url, tag)). + t.Run("GetManifest serv indirect", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + defer test.MockVariableValue(&setting.Packages.Storage.MinioConfig.ServeDirect, false)() + + req := NewRequest(t, "GET", fmt.Sprintf("%s/manifests/%s", url, tag)). AddTokenAuth(userToken) resp := MakeRequest(t, req, http.StatusOK) @@ -446,6 +451,25 @@ func TestPackageContainer(t *testing.T) { assert.Equal(t, manifestDigest, resp.Header().Get("Docker-Content-Digest")) assert.Equal(t, manifestContent, resp.Body.String()) }) + + t.Run("GetManifest serv direct", func(t *testing.T) { + if setting.Packages.Storage.Type != setting.MinioStorageType { + t.Skip("Test skipped for non-Minio-storage.") + return + } + + defer tests.PrintCurrentTest(t)() + defer test.MockVariableValue(&setting.Packages.Storage.MinioConfig.ServeDirect, true)() + + req := NewRequest(t, "GET", fmt.Sprintf("%s/manifests/%s", url, tag)). + AddTokenAuth(userToken) + resp := MakeRequest(t, req, http.StatusTemporaryRedirect) + + assert.Empty(t, resp.Header().Get("Content-Length")) + assert.NotEmpty(t, resp.Header().Get("Location")) + assert.Equal(t, "text/html; charset=utf-8", resp.Header().Get("Content-Type")) + assert.Empty(t, resp.Header().Get("Docker-Content-Digest")) + }) }) }