Skip to content

Refactor storage content-type handling of SignedURL#36804

Open
ChristopherHX wants to merge 7 commits intogo-gitea:mainfrom
ChristopherHX:refac-blob-storage-mime-types
Open

Refactor storage content-type handling of SignedURL#36804
ChristopherHX wants to merge 7 commits intogo-gitea:mainfrom
ChristopherHX:refac-blob-storage-mime-types

Conversation

@ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Mar 2, 2026

  • replace raw url.Values by *storage.SignedURLParam
  • implement content-type in azblob
  • implement content-disposition in azblob
  • add tests for content types in response

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2026
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Mar 2, 2026
Add checks to avoid setting empty response parameters for Minio.
"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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not the blob storage already has some sort of domain sandbox???

If served directly from giteas domain, I would agree. Given you can not make assumptions about the blob signed url you cannot really reference anything more than ca. 5min that is part of the same domain.

e.g. this is how GitHub permits raw html artifacts to be served without sandbox

return convertAzureBlobErr(err)
}

func (a *AzureBlobStorage) GetSasURL(b *blob.Client, template sas.BlobSignatureValues) (string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the function from the azure package does not allow content-type + disposition to be provided. Since it is part of the signature we cannot just append this to the url.

@ChristopherHX ChristopherHX added type/refactoring Existing code has been cleaned up. There should be no new functionality. type/enhancement An improvement of existing functionality labels Mar 2, 2026
@silverwind silverwind requested a review from Copilot March 2, 2026 14:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the storage content-type handling for signed/presigned object storage URLs. Instead of passing raw url.Values for request parameters to ObjectStorage.URL(), it introduces a strongly-typed SignedURLParam struct with ContentType and ContentDisposition fields. It also implements proper content-type and content-disposition header support in Azure Blob Storage (which was previously missing, marked as a TODO), and adds integration tests for both Minio and Azure Blob Storage backends.

Changes:

  • Introduces SignedURLParam struct with a WithDefaults(name) method that auto-detects safe MIME types from file extensions (replacing duplicated inline MIME type maps).
  • Updates all ObjectStorage.URL() implementations (MinioStorage, AzureBlobStorage, LocalStorage, discardStorage) and all callers to use *SignedURLParam instead of url.Values.
  • Adds Azure Blob Storage SAS URL signing with ContentType and ContentDisposition support via a new GetSasURL helper, and adds CI integration tests for both Minio and Azure backends.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
modules/storage/storage.go Adds SignedURLParam struct and WithDefaults method; updates ObjectStorage interface signature
modules/storage/minio.go Refactors URL to use *SignedURLParam and WithDefaults instead of inline MIME type map
modules/storage/azureblob.go Adds GetSasURL helper and updates URL to include ContentType/ContentDisposition in SAS signing
modules/storage/local.go Updates URL signature to match new interface
modules/storage/helper.go Updates discardStorage.URL signature to match new interface
modules/public/mime_types.go Adds wellKnownSafeMimeTypes list and two new exported functions
modules/packages/content_store.go Updates GetServeDirectURL signature to use *storage.SignedURLParam
services/packages/packages.go Updates OpenBlobForDownload signature to use *storage.SignedURLParam
routers/api/packages/container/container.go Uses &storage.SignedURLParam{ContentType: ...} instead of url.Values for container blob serving
modules/storage/storage_test.go Adds shared test helper for content-type/disposition URL response validation
modules/storage/minio_test.go Adds CI integration test for Minio content-type/disposition URL behavior
modules/storage/azureblob_test.go Adds CI integration test for Azure Blob content-type/disposition URL behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ChristopherHX and others added 2 commits March 2, 2026 16:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: ChristopherHX <christopher.homberger@web.de>
@ChristopherHX ChristopherHX requested a review from Copilot March 2, 2026 19:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +255 to +257
t, err := time.Parse(blob.SnapshotTimeFormat, urlParts.Snapshot)
if err != nil {
t = time.Time{}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time.Parse will return a non-nil error when urlParts.Snapshot is an empty string (i.e., when the blob has no snapshot), and the error is silently swallowed. While defaulting to time.Time{} (zero time) is likely correct for a non-snapshot blob, the unconditional error suppression hides genuine parse failures. Consider only setting t = time.Time{} when urlParts.Snapshot is empty, and propagating the error otherwise.

Suggested change
t, err := time.Parse(blob.SnapshotTimeFormat, urlParts.Snapshot)
if err != nil {
t = time.Time{}
var t time.Time
if urlParts.Snapshot == "" {
t = time.Time{}
} else {
t, err = time.Parse(blob.SnapshotTimeFormat, urlParts.Snapshot)
if err != nil {
return "", err
}

Copilot uses AI. Check for mistakes.
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==",
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the well-known Azurite development account key, which is safe for local testing. However, hardcoding it directly in source may trigger secret scanning tools. Consider referencing it from an environment variable or a named constant with a comment clarifying it is the public Azurite development key, consistent with how similar credentials are typically handled.

Copilot uses AI. Check for mistakes.
Comment on lines +81 to +84
testSingleBlobStorageURLContentTypeAndDisposition(t, s, "test.txt", "test.pdf", SignedURLParam{
ContentType: "application/pdf",
ContentDisposition: "inline",
}, nil)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file saved is test.txt (plain text content), but the URL is requested with the name "test.pdf". The withDefaults logic uses the name parameter to detect MIME type and disposition, so the test is asserting that an object with a .pdf name gets ContentType: "application/pdf" and ContentDisposition: "inline" — but the expected value here reflects the request parameters inferred from the name, not the actual server response content-type of the stored object. If the storage backend serves the stored object's own content-type (e.g., Minio may sniff it), the assertion may fail or be misleading. Ensure the test accurately reflects what the server will actually return in the Content-Type header.

Copilot uses AI. Check for mistakes.
s, err := NewStorage(typStr, cfg)
assert.NoError(t, err)

data := "Q2xTckt6Y1hDOWh0"
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test data string "Q2xTckt6Y1hDOWh0" is an unexplained magic value. Adding a comment clarifying what this string represents (e.g., arbitrary test data, a base64-encoded value) would improve readability.

Suggested change
data := "Q2xTckt6Y1hDOWh0"
data := "Q2xTckt6Y1hDOWh0" // arbitrary test content; specific value is irrelevant to this test

Copilot uses AI. Check for mistakes.
Signed-off-by: ChristopherHX <christopher.homberger@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code type/enhancement An improvement of existing functionality type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants