Allow to display embed images/pdfs when SERVE_DIRECT was enabled on MinIO storage

[lifegpc]

Releated issue: #30487

[dangjinghao]

As far as I know, this issue is specific to PDF LFS files. The application/octet-stream type works correctly in other cases. Therefore, I recommend a minimal change to address this edge case.

Additionally, have we considered using mime.TypeByExtension? This would eliminate the need to maintain a custom MIME type map.

[dangjinghao]

What is the expected behavior if we rely on file extension name for LFS type detection and the extension name is wrong? Specifically, will the file content still be displayed correctly?

[lifegpc]

What is the expected behavior if we rely on file extension name for LFS type detection and the extension name is wrong? Specifically, will the file content still be displayed correctly?

For images, wrong mimetype still works.

[silverwind]

For images, wrong mimetype still works.

It depends on the client. Some like browsers can handle wrong mime type and do another content type detection based on the data.

[wxiaoguang]

This is a quick fix for the "View Raw File" on the Web UI, which have the filename in the URL. So it should work.

[silverwind]

Why not just exclude image/svg? Having to maintain a list of "safe" image types seems less maintainable because new image formats are added all the time. If we keep it, define it as a constant in a shared place.

[wxiaoguang]

Why not just exclude image/svg? Having to maintain a list of "safe" image types seems less maintainable because new image formats are added all the time. If we keep it, define it as a constant in a shared place.

Will you also remember to exclude html?

[silverwind]

Why not just exclude image/svg? Having to maintain a list of "safe" image types seems less maintainable because new image formats are added all the time. If we keep it, define it as a constant in a shared place.

Will you also remember to exclude html?

Just use

if mimetype == "application/pdf" || (strings.HasPrefix(mimetype, "image/") && mimetype != "image/svg+xml")

We have similar code in other places already so it could be moved to a isSafeImageMimeType.

[silverwind]

Also, what about audio/video files? See

gitea/modules/typesniffer/typesniffer.go

Lines 80 to 83 in 0ce7d66

	// IsBrowsableBinaryType returns whether a non-text type can be displayed in a browser
	func (ct SniffedType) IsBrowsableBinaryType() bool {
	return ct.IsImage() || ct.IsSvgImage() || ct.IsPDF() || ct.IsVideo() || ct.IsAudio()
	}

[wxiaoguang]

Why not just exclude image/svg? Having to maintain a list of "safe" image types seems less maintainable because new image formats are added all the time. If we keep it, define it as a constant in a shared place.

Will you also remember to exclude html?

Just use

if mimetype == "application/pdf" || (strings.HasPrefix(mimetype, "image/") && mimetype != "image/svg+xml")

We have similar code in other places already so it could be moved to a isSafeImageMimeType.

I don't like it, it doesn't feel safe, I don't want to spend more time on fixing security bugs in the future.

If you can fix the security problems when they appear in the future, feel free to make your changes.

[wxiaoguang]

Also, what about audio/video files? See

gitea/modules/typesniffer/typesniffer.go

Lines 80 to 83 in 0ce7d66

	// IsBrowsableBinaryType returns whether a non-text type can be displayed in a browser
	func (ct SniffedType) IsBrowsableBinaryType() bool {
	return ct.IsImage() || ct.IsSvgImage() || ct.IsPDF() || ct.IsVideo() || ct.IsAudio()
	}

.

No, I don't want to use it at the moment. There are still more logic after IsBrowsableBinaryType call, and the function name itself is not accurate.

---

This PR aims to fix the clear problems as it stated. To fix more, the comment keyword HINT: PDF-RENDER-SANDBOX will lead developers to more related places.

[wxiaoguang]

To fully refactor the related code and add more supports, it needs to use the approach from "Fix mime-type detection for HTTP server (#18370)" (and also refactor the existing code, a lot of changes)

Then it will introduce much more changes in this PR. I don't think these works need to be in this PR's scope (since this PR's aim is clear)

[silverwind]

I think this will work as a quick fix, but it would be better to define a safeFileTypes that holds image, video, audio, pdf etc metadata somewhere.

[lunny]

I think this can be merged so that it will be easier to backport to v1.25.

[wxiaoguang]

I think this will work as a quick fix, but it would be better to define a safeFileTypes that holds image, video, audio, pdf etc metadata somewhere.

I think this PR's code is clear, safe, and well-commented, its logic is complete for its goal and doesn't make any blocker for future refactoring. I don't see a must to do more changes. If you have ideas about making more changes, feel free to edit it directly.