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

Rework raw file http header logic #20464

Closed
wants to merge 0 commits into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jul 23, 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

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 23, 2022
@silverwind
Copy link
Member Author

silverwind commented Jul 24, 2022

Added the sandbox attribute removal for PDF here as well. Problem is that PDF may render using browser plugins and plugins are generally disabled in sandboxed content.

I think it's reasonable safe to do this. The worst that could happen is that a PDF could CPU resources like described in https://bugs.chromium.org/p/chromium/issues/detail?id=413851#c24, but I don't think a full compromise is possible because PDF scripts can normally not escape the PDF document environment, and if they can, it's a plugin bug.

@lafriks
Copy link
Member

lafriks commented Jul 24, 2022

But GitHub does set sandbox for pdf files imho

@silverwind
Copy link
Member Author

silverwind commented Jul 24, 2022

No, they don't, at least not any longer. If they would, pdf attachments would not work in Safari and possibly Chrome as well. The do a 302 redirect to the separate domain. the redirect response does have CSP without sandbox but I think it's not in effect, the destination is served without any CSP at all. See for yourself:

curl -sLi 'https://github.com/silverwind/symlink-test/files/9175640/test.pdf' | grep -i content-security

@silverwind
Copy link
Member Author

silverwind commented Jul 25, 2022

Oops, I destroyed the PR by pushing main onto it, trying to restore. New PR: #20484.

@6543 6543 removed this from the 1.18.0 milestone Jul 29, 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.

5 participants