Skip to content

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jul 23, 2022

The previous logic was only setting content-type for SVG. All other types did not consider the user's configured mime type mapping. Fix that by allowing all mappings to take effect. Also, added a default PDF mimetype as there is no harm in doing that.

The previous logic was only setting content-type for SVG and PDF files,
all other types did not consider the user's mime type map. Fix that by
allowing all mappings to take effect. Also, added a default support for
PDF mimetype as there is no harm in doing that.
@silverwind silverwind added the type/enhancement An improvement of existing functionality label Jul 23, 2022
@silverwind silverwind added this to the 1.18.0 milestone Jul 23, 2022
@silverwind silverwind changed the title Fix content-type header for non-text files Fix content-type header for non-text-file attachments Jul 23, 2022
@zeripath
Copy link
Contributor

Test failure is related

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 23, 2022
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

LGTM Apart from test failure.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 23, 2022
@silverwind
Copy link
Member Author

silverwind commented Jul 23, 2022

Hmm, that test failure is odd:

    wiki_test.go:216:
        	Error Trace:	wiki_test.go:216
        	Error:      	Not equal:
        	            	expected: "image/jpeg"
        	            	actual  : "application/octet-stream"

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-image/jpeg
        	            	+application/octet-stream
        	Test:       	TestWikiRaw

The only case the old code could have set such a value is if setting.MimeTypeMap.Map contained a .jpg entry, but I see none being added anywhere.

@silverwind
Copy link
Member Author

In any case, I think we can allow all image/* types by default as well as detected by file extension, which in turn should also fix this test.

@silverwind
Copy link
Member Author

I'll give this a more thorough rewrite, there's a lot of issues in the attachment serving code.

@silverwind silverwind marked this pull request as draft July 23, 2022 16:03
@silverwind
Copy link
Member Author

#20464

@silverwind silverwind closed this Jul 23, 2022
6543 pushed a commit that referenced this pull request Jul 29, 2022
- Always respect the user's configured mime type map
- Allow more types like image/pdf/video/audio to serve with correct content-type
- Shorten cache duration of raw files to 5 minutes, matching GitHub
- Don't set `content-disposition: attachment`, let the browser decide whether it wants to download or display a file directly
- Implement rfc5987 for filenames, remove previous hack. Confirmed it working in Safari.
- Make PDF attachment work in Safari by removing `sandbox` attribute.

This change will make a lot more file types open directly in browser now. Logic should generally be more readable than before with less `if` nesting and such.

Replaces: #20460
Replaces: #20455
Fixes: #20404
silverwind added a commit to silverwind/gitea that referenced this pull request Jul 29, 2022
- Always respect the user's configured mime type map
- Allow more types like image/pdf/video/audio to serve with correct content-type
- Shorten cache duration of raw files to 5 minutes, matching GitHub
- Don't set `content-disposition: attachment`, let the browser decide whether it wants to download or display a file directly
- Implement rfc5987 for filenames, remove previous hack. Confirmed it working in Safari.
- Make PDF attachment work in Safari by removing `sandbox` attribute.

This change will make a lot more file types open directly in browser now. Logic should generally be more readable than before with less `if` nesting and such.

Replaces: go-gitea#20460
Replaces: go-gitea#20455
Fixes: go-gitea#20404
6543 pushed a commit that referenced this pull request Jul 30, 2022
- Always respect the user's configured mime type map
- Allow more types like image/pdf/video/audio to serve with correct content-type
- Shorten cache duration of raw files to 5 minutes, matching GitHub
- Don't set `content-disposition: attachment`, let the browser decide whether it wants to download or display a file directly
- Implement rfc5987 for filenames, remove previous hack. Confirmed it working in Safari.
- Make PDF attachment work in Safari by removing `sandbox` attribute.

This change will make a lot more file types open directly in browser now. Logic should generally be more readable than before with less `if` nesting and such.

Replaces: #20460
Replaces: #20455
Fixes: #20404
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
- Always respect the user's configured mime type map
- Allow more types like image/pdf/video/audio to serve with correct content-type
- Shorten cache duration of raw files to 5 minutes, matching GitHub
- Don't set `content-disposition: attachment`, let the browser decide whether it wants to download or display a file directly
- Implement rfc5987 for filenames, remove previous hack. Confirmed it working in Safari.
- Make PDF attachment work in Safari by removing `sandbox` attribute.

This change will make a lot more file types open directly in browser now. Logic should generally be more readable than before with less `if` nesting and such.

Replaces: go-gitea#20460
Replaces: go-gitea#20455
Fixes: go-gitea#20404
@silverwind silverwind deleted the content-type branch September 23, 2022 04:54
@lunny lunny removed this from the 1.18.0 milestone Dec 20, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants