Refactor storage content-type handling of SignedURL

modules/packages/content_store.go

@@ -36,7 +36,7 @@ func (s *ContentStore) ShouldServeDirect() bool {
 	return setting.Packages.Storage.ServeDirect()
 }
 
-func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename, method string, reqParams url.Values) (*url.URL, error) {
+func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename, method string, reqParams *storage.SignedURLParam) (*url.URL, error) {
 	return s.store.URL(KeyToRelativePath(key), filename, method, reqParams)
 }
 

modules/public/mime_types.go

@@ -3,7 +3,10 @@
 
 package public
 
-import "strings"
+import (
+	"slices"
+	"strings"
+)
 
 // wellKnownMimeTypesLower comes from Golang's builtin mime package: `builtinTypesLower`, see the comment of detectWellKnownMimeType
 var wellKnownMimeTypesLower = map[string]string{
@@ -28,6 +31,19 @@ var wellKnownMimeTypesLower = map[string]string{
 	".txt": "text/plain; charset=utf-8",
 }
 
+var wellKnownSafeMimeTypes = []string{
+	"text/plain",
+	"text/plain; charset=utf-8",
+	"image/png",
+	"image/jpeg",
+	"image/gif",
+	"image/webp",
+	"image/avif",
+	// ATTENTION! Don't support unsafe types like HTML/SVG due to security concerns: they can contain JS code, and maybe they need proper Content-Security-Policy
+	// HINT: PDF-RENDER-SANDBOX: PDF won't render in sandboxed context, it seems fine to render it inline
+	"application/pdf",
+}
+
 // detectWellKnownMimeType will return the mime-type for a well-known file ext name
 // The purpose of this function is to bypass the unstable behavior of Golang's mime.TypeByExtension
 // mime.TypeByExtension would use OS's mime-type config to overwrite the well-known types (see its document).
@@ -38,3 +54,14 @@ func detectWellKnownMimeType(ext string) string {
 	ext = strings.ToLower(ext)
 	return wellKnownMimeTypesLower[ext]
 }
+
+func IsWellKnownSafeInlineMimeType(mimeType string) bool {
+	mimeType = strings.ToLower(mimeType)
+	return slices.Contains(wellKnownSafeMimeTypes, mimeType)
+}
+
+func DetectWellKnownSafeInlineMimeType(ext string) (mimeType string, safe bool) {
+	mimeType = detectWellKnownMimeType(ext)
+	safe = IsWellKnownSafeInlineMimeType(mimeType)
+	return mimeType, safe
+}

modules/storage/azureblob.go

@@ -246,16 +246,48 @@ func (a *AzureBlobStorage) Delete(path string) error {
 	return convertAzureBlobErr(err)
 }
 
+func (a *AzureBlobStorage) getSasURL(b *blob.Client, template sas.BlobSignatureValues) (string, error) {
+	urlParts, err := blob.ParseURL(b.URL())
+	if err != nil {
+		return "", err
+	}
+
+	t, err := time.Parse(blob.SnapshotTimeFormat, urlParts.Snapshot)
+	if err != nil {
+		t = time.Time{}
+	}
+
+	template.ContainerName = urlParts.ContainerName
+	template.BlobName = urlParts.BlobName
+	template.SnapshotTime = t
+	template.Version = sas.Version
+
+	qps, err := template.SignWithSharedKey(a.credential)
+	if err != nil {
+		return "", err
+	}
+
+	endpoint := b.URL() + "?" + qps.Encode()
+
+	return endpoint, nil
+}
+
 // URL gets the redirect URL to a file. The presigned link is valid for 5 minutes.
-func (a *AzureBlobStorage) URL(path, name, _ string, reqParams url.Values) (*url.URL, error) {
-	blobClient := a.getBlobClient(path)
+func (a *AzureBlobStorage) URL(storePath, name, _ string, reqParams *SignedURLParam) (*url.URL, error) {
+	blobClient := a.getBlobClient(storePath)
+
+	startTime := time.Now().UTC()
+
+	param := reqParams.withDefaults(name)
 
-	// TODO: OBJECT-STORAGE-CONTENT-TYPE: "browser inline rendering images/PDF" needs proper Content-Type header from storage
-	startTime := time.Now()
-	u, err := blobClient.GetSASURL(sas.BlobPermissions{
-		Read: true,
-	}, time.Now().Add(5*time.Minute), &blob.GetSASURLOptions{
-		StartTime: &startTime,
+	u, err := a.getSasURL(blobClient, sas.BlobSignatureValues{
+		Permissions: (&sas.BlobPermissions{
+			Read: true,
+		}).String(),
+		StartTime:          startTime,
+		ExpiryTime:         startTime.Add(5 * time.Minute),
+		ContentDisposition: param.ContentDisposition,
+		ContentType:        param.ContentType,
 	})
 	if err != nil {
 		return nil, convertAzureBlobErr(err)

modules/storage/azureblob_test.go

@@ -31,6 +31,23 @@ func TestAzureBlobStorageIterator(t *testing.T) {
 	})
 }
 
+func TestAzureBlobStorageURLContentTypeAndDisposition(t *testing.T) {
+	if os.Getenv("CI") == "" {
+		t.Skip("azureBlobStorage not present outside of CI")
+		return
+	}
+	testBlobStorageURLContentTypeAndDisposition(t, setting.AzureBlobStorageType, &setting.Storage{
+		AzureBlobConfig: setting.AzureBlobStorageConfig{
+			// https://learn.microsoft.com/azure/storage/common/storage-use-azurite?tabs=visual-studio-code#ip-style-url
+			Endpoint: "http://devstoreaccount1.azurite.local:10000",
+			// https://learn.microsoft.com/azure/storage/common/storage-use-azurite?tabs=visual-studio-code#well-known-storage-account-and-key
+			AccountName: "devstoreaccount1",
+			AccountKey:  "Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==",
+			Container:   "test",
+		},
+	})
+}
+
 func TestAzureBlobStoragePath(t *testing.T) {
 	m := &AzureBlobStorage{cfg: &setting.AzureBlobStorageConfig{BasePath: ""}}
 	assert.Empty(t, m.buildAzureBlobPath("/"))

modules/storage/helper.go

@@ -30,7 +30,7 @@ func (s discardStorage) Delete(_ string) error {
 	return fmt.Errorf("%s", s)
 }
 
-func (s discardStorage) URL(_, _, _ string, _ url.Values) (*url.URL, error) {
+func (s discardStorage) URL(_, _, _ string, _ *SignedURLParam) (*url.URL, error) {
 	return nil, fmt.Errorf("%s", s)
 }
 

modules/storage/local.go

@@ -134,7 +134,7 @@ func (l *LocalStorage) Delete(path string) error {
 }
 
 // URL gets the redirect URL to a file
-func (l *LocalStorage) URL(path, name, _ string, reqParams url.Values) (*url.URL, error) {
+func (l *LocalStorage) URL(path, name, _ string, reqParams *SignedURLParam) (*url.URL, error) {
 	return nil, ErrURLNotSupported
 }
 

modules/storage/minio.go

@@ -279,36 +279,16 @@ func (m *MinioStorage) Delete(path string) error {
 }
 
 // URL gets the redirect URL to a file. The presigned link is valid for 5 minutes.
-func (m *MinioStorage) URL(storePath, name, method string, serveDirectReqParams url.Values) (*url.URL, error) {
-	// copy serveDirectReqParams
-	reqParams, err := url.ParseQuery(serveDirectReqParams.Encode())
-	if err != nil {
-		return nil, err
-	}
+func (m *MinioStorage) URL(storePath, name, method string, serveDirectReqParams *SignedURLParam) (*url.URL, error) {
+	reqParams := url.Values{}
 
-	// Here we might not know the real filename, and it's quite inefficient to detect the mine type by pre-fetching the object head.
-	// So we just do a quick detection by extension name, at least if works for the "View Raw File" for an LFS file on the Web UI.
-	// Detect content type by extension name, only support the well-known safe types for inline rendering.
-	// TODO: OBJECT-STORAGE-CONTENT-TYPE: need a complete solution and refactor for Azure in the future
-	ext := path.Ext(name)
-	inlineExtMimeTypes := map[string]string{
-		".png":  "image/png",
-		".jpg":  "image/jpeg",
-		".jpeg": "image/jpeg",
-		".gif":  "image/gif",
-		".webp": "image/webp",
-		".avif": "image/avif",
-		// ATTENTION! Don't support unsafe types like HTML/SVG due to security concerns: they can contain JS code, and maybe they need proper Content-Security-Policy
-		// HINT: PDF-RENDER-SANDBOX: PDF won't render in sandboxed context, it seems fine to render it inline
-		".pdf": "application/pdf",
-
-		// TODO: refactor with "modules/public/mime_types.go", for example: "DetectWellKnownSafeInlineMimeType"
+	param := serveDirectReqParams.withDefaults(name)
+	// minio does not ignore empty params
+	if param.ContentType != "" {
+		reqParams.Set("response-content-type", param.ContentType)
 	}
-	if mimeType, ok := inlineExtMimeTypes[ext]; ok {
-		reqParams.Set("response-content-type", mimeType)
-		reqParams.Set("response-content-disposition", "inline")
-	} else {
-		reqParams.Set("response-content-disposition", fmt.Sprintf(`attachment; filename="%s"`, quoteEscaper.Replace(name)))
+	if param.ContentDisposition != "" {
+		reqParams.Set("response-content-disposition", param.ContentDisposition)
 	}
 
 	expires := 5 * time.Minute

modules/storage/minio_test.go

@@ -32,6 +32,22 @@ func TestMinioStorageIterator(t *testing.T) {
 	})
 }
 
+func TestMinioStorageURLContentTypeAndDisposition(t *testing.T) {
+	if os.Getenv("CI") == "" {
+		t.Skip("minioStorage not present outside of CI")
+		return
+	}
+	testBlobStorageURLContentTypeAndDisposition(t, setting.MinioStorageType, &setting.Storage{
+		MinioConfig: setting.MinioStorageConfig{
+			Endpoint:        "localhost:9000",
+			AccessKeyID:     "123456",
+			SecretAccessKey: "12345678",
+			Bucket:          "gitea",
+			Location:        "us-east-1",
+		},
+	})
+}
+
 func TestMinioStoragePath(t *testing.T) {
 	m := &MinioStorage{basePath: ""}
 	assert.Empty(t, m.buildMinioPath("/"))

modules/storage/storage.go

@@ -10,9 +10,12 @@
 	"io"
 	"net/url"
 	"os"
+	"path"
 
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/public"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // ErrURLNotSupported represents url is not supported
@@ -56,6 +59,37 @@
 	Stat() (os.FileInfo, error)
 }
 
+type SignedURLParam struct {
+	ContentType        string
+	ContentDisposition string
+}
+
+func (s *SignedURLParam) withDefaults(name string) *SignedURLParam {
+	// Here we might not know the real filename, and it's quite inefficient to detect the MIME type by pre-fetching the object head.
+	// So we just do a quick detection by extension name, at least it works for the "View Raw File" for an LFS file on the Web UI.
+	// Detect content type by extension name, only support the well-known safe types for inline rendering.
+	// TODO: OBJECT-STORAGE-CONTENT-TYPE: need a complete solution and refactor for Azure in the future
+
+	ext := path.Ext(name)
+	param := *util.Iif(s == nil, &SignedURLParam{}, s)
+
+	isSafe := false
+	if param.ContentType != "" {
+		isSafe = public.IsWellKnownSafeInlineMimeType(param.ContentType)
+	} else if mimeType, safe := public.DetectWellKnownSafeInlineMimeType(ext); safe {
+		param.ContentType = mimeType
+		isSafe = true
+	}
+	if param.ContentDisposition == "" {
+		if isSafe {
+			param.ContentDisposition = "inline"
+		} else {
+			param.ContentDisposition = fmt.Sprintf(`attachment; filename="%s"`, quoteEscaper.Replace(name))
+		}
+	}
+	return &param
+}
+
 // ObjectStorage represents an object storage to handle a bucket and files
 type ObjectStorage interface {
 	Open(path string) (Object, error)
@@ -67,9 +101,9 @@
 
 	Stat(path string) (os.FileInfo, error)
 	Delete(path string) error
-	URL(path, name, method string, reqParams url.Values) (*url.URL, error)
-
-	// IterateObjects calls the iterator function for each object in the storage with the given path as prefix
+	URL(path, name, method string, reqParams *SignedURLParam) (*url.URL, error)
+	
+	// URL(path, name, method string, reqParams *SignedURLParam) (*url.URL, error)IterateObjects calls the iterator function for each object in the storage with the given path as prefix
 	// The "fullPath" argument in callback is the full path in this storage.
 	// * IterateObjects("", ...): iterate all objects in this storage
 	// * IterateObjects("sub-path", ...): iterate all objects with "sub-path" as prefix in this storage, the "fullPath" will be like "sub-path/xxx"

modules/storage/storage_test.go

@@ -4,12 +4,14 @@
 package storage
 
 import (
+	"net/http"
 	"strings"
 	"testing"
 
 	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func testStorageIterator(t *testing.T, typStr Type, cfg *setting.Storage) {
@@ -50,3 +52,54 @@ func testStorageIterator(t *testing.T, typStr Type, cfg *setting.Storage) {
 		assert.Len(t, expected, count)
 	}
 }
+
+func testSingleBlobStorageURLContentTypeAndDisposition(t *testing.T, s ObjectStorage, path, name string, expected SignedURLParam, reqParams *SignedURLParam) {
+	u, err := s.URL(path, name, http.MethodGet, reqParams)
+	require.NoError(t, err)
+	resp, err := http.Get(u.String())
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	assert.Equal(t, expected.ContentType, resp.Header.Get("Content-Type"))
+	if expected.ContentDisposition != "" {
+		assert.Equal(t, expected.ContentDisposition, resp.Header.Get("Content-Disposition"))
+	}
+}
+
+func testBlobStorageURLContentTypeAndDisposition(t *testing.T, typStr Type, cfg *setting.Storage) {
+	s, err := NewStorage(typStr, cfg)
+	assert.NoError(t, err)
+
+	data := "Q2xTckt6Y1hDOWh0"
+	_, err = s.Save("test.txt", strings.NewReader(data), int64(len(data)))
+	assert.NoError(t, err)
+
+	testSingleBlobStorageURLContentTypeAndDisposition(t, s, "test.txt", "test.txt", SignedURLParam{
+		ContentType:        "text/plain; charset=utf-8",
+		ContentDisposition: "inline",
+	}, nil)
+
+	testSingleBlobStorageURLContentTypeAndDisposition(t, s, "test.txt", "test.pdf", SignedURLParam{
+		ContentType:        "application/pdf",
+		ContentDisposition: "inline",
+	}, nil)
+
+	testSingleBlobStorageURLContentTypeAndDisposition(t, s, "test.txt", "test.wasm", SignedURLParam{
+		ContentType:        "application/octet-stream",
+		ContentDisposition: `attachment; filename="test.wasm"`,
+	}, nil)
+
+	testSingleBlobStorageURLContentTypeAndDisposition(t, s, "test.txt", "test.wasm", SignedURLParam{
+		ContentType:        "application/octet-stream",
+		ContentDisposition: `attachment; filename="test.wasm"`,
+	}, &SignedURLParam{})
+
+	testSingleBlobStorageURLContentTypeAndDisposition(t, s, "test.txt", "test.txt", SignedURLParam{
+		ContentType:        "application/octet-stream",
+		ContentDisposition: `inline; filename="test.xml"`,
+	}, &SignedURLParam{
+		ContentType:        "application/octet-stream",
+		ContentDisposition: `inline; filename="test.xml"`,
+	})
+
+	assert.NoError(t, s.Delete("test.txt"))
+}

routers/api/packages/container/container.go

@@ -26,6 +26,7 @@ import (
 	packages_module "code.gitea.io/gitea/modules/packages"
 	container_module "code.gitea.io/gitea/modules/packages/container"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/storage"
 	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/routers/api/packages/helper"
 	auth_service "code.gitea.io/gitea/services/auth"
@@ -704,9 +705,9 @@ 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.OpenBlobForDownload(ctx, pfd.File, pfd.Blob, ctx.Req.Method, serveDirectReqParams)
+	s, u, _, err := packages_service.OpenBlobForDownload(ctx, pfd.File, pfd.Blob, ctx.Req.Method, &storage.SignedURLParam{
+		ContentType: pfd.Properties.GetByName(container_module.PropertyMediaType),
+	})
 	if err != nil {
 		apiError(ctx, http.StatusInternalServerError, err)
 		return

services/packages/packages.go

@@ -599,7 +599,7 @@ func OpenBlobStream(pb *packages_model.PackageBlob) (io.ReadSeekCloser, error) {
 
 // OpenBlobForDownload returns the content of the specific package blob and increases the download counter.
 // If the storage supports direct serving and it's enabled, only the direct serving url is returned.
-func OpenBlobForDownload(ctx context.Context, pf *packages_model.PackageFile, pb *packages_model.PackageBlob, method string, serveDirectReqParams url.Values) (io.ReadSeekCloser, *url.URL, *packages_model.PackageFile, error) {
+func OpenBlobForDownload(ctx context.Context, pf *packages_model.PackageFile, pb *packages_model.PackageBlob, method string, serveDirectReqParams *storage.SignedURLParam) (io.ReadSeekCloser, *url.URL, *packages_model.PackageFile, error) {
 	key := packages_module.BlobHash256Key(pb.HashSHA256)
 
 	cs := packages_module.NewContentStore()