Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix content-type header for non-text-file attachments #20460

Closed
wants to merge 2 commits into from

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