Use browser's default application open handler for non-dangerous MIME types

[Eriner]

This changes PDF downloads to not be displayed inline in the browser (or preferred application) rather than being saved as a download. The current implementation requires multiple clicks to open PDFs included in generic packages.

There is no existing issue for this, and I have not created one.

Notably, this is not the way GitHub behaves, but is the way I wish GitHub behaved.

Additionally, this adds quotes to the filename parameter per MSDN:

Warning: The string following filename should always be put into quotes; but, for compatibility reasons, many browsers try to parse unquoted names that contain spaces.

I have not tested this code or checked if this change has any (security?) consequences for usages of this function outside of serving generic-type package files. A MIME-type check (as opposed to this implementation's file extension check) may or may not be necessary.

[codecov-commenter]

Codecov Report

Merging #20720 (b767431) into main (0066bc5) will decrease coverage by 0.01%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##             main   #20720      +/-   ##
==========================================
- Coverage   46.93%   46.91%   -0.02%     
==========================================
  Files         981      981              
  Lines      136275   136280       +5     
==========================================
- Hits        63961    63942      -19     
- Misses      64463    64484      +21     
- Partials     7851     7854       +3     

Impacted Files	Coverage Δ
modules/context/context.go	64.51% <42.85%> (-0.46%)	⬇️
modules/queue/queue_bytefifo.go	47.29% <0.00%> (-2.89%)	⬇️
modules/queue/queue_channel.go	80.55% <0.00%> (-2.78%)	⬇️
modules/notification/mail/mail.go	39.04% <0.00%> (-2.74%)	⬇️
modules/log/event.go	58.18% <0.00%> (-2.10%)	⬇️
modules/notification/ui/ui.go	58.92% <0.00%> (-1.79%)	⬇️
models/unit/unit.go	47.00% <0.00%> (-1.71%)	⬇️
models/notification.go	62.22% <0.00%> (-1.10%)	⬇️
routers/web/repo/view.go	41.72% <0.00%> (-0.49%)	⬇️
models/issues/comment.go	50.76% <0.00%> (+0.43%)	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[KN4CK3R]

This may need more logic like in func ServeData.

ping @silverwind, you worked on this some days ago.

[silverwind]

What UI actions does this concern? View raw pdf?

I'm against this as we should not enforce what a browser does with a certain file type. Users should be able to set their preference in their browser instead, for example in Firefox options, one can set PDF to always download:

[pchampio]

Users should be able to set their preference in their browser instead, for example in Firefox options, one can set PDF to always download:

Firefox preference uses the "Content-Type" header to find whether the resource will be downloaded or directly viewed.
Currently the "Content-Type" header is always application/octet-stream for all files, meaning it will ignore Firefox's user preference.

I'll love to have this feature, and not only for PDF, for JPG, mp3, txt....

Using http.DetectContentType() is a super clean way to implement this (fallback to application/octet-stream when the ContentType cannot be determine).

[KN4CK3R]

But then the question is if the package registry is the correct place for these files. I may be better to put them into issues in a repo?

[silverwind]

Currently the "Content-Type" header is always"application/octet-stream" for all files, meaning it will ignore Firefox's user preference.

Yes, we should set a proper content-type, you can use the existing typesniffer module. The header logic from ServeData should be re-used as setting correct content-type does have security implications and we do use CSP there to limit attack surface.

[Eriner]

Yes, we should set a proper content-type, you can use the existing typesniffer module. The header logic from ServeData should be re-used as setting correct content-type does have security implications and we do use CSP there to limit attack surface.

If you'd like me to give implementing this a go, I'll need a few days to get around to this and better familiarize myself with the existing code (you referenced above).

Re: security implications only a whitelisted subset of MIME types should be served with Content-Disposition: inline, as opposed to creating a blacklist of dangerous types (text/html, application/javascript, text/css, etc.).

Thanks for all your input!

[Eriner]

But then the question is if the package registry is the correct place for these files. I may be better to put them into issues in a repo?

For my use-case, I'm using CI to generate a PDF document that is built from the contents of the repo at a fixed point in time (each commit). The use-case is the same as building a binary blob from the source code in the repo, I just happen to be building a PDF instead of an executable.

[silverwind]

Re: security implications only a whitelisted subset of MIME types should be served with Content-Disposition: inline, as opposed to creating a blacklist of dangerous types (text/html, application/javascript, text/css, etc.).

Yes, that's how the logic in ServeData already does it, whitelist a few known types, serve the rest with application/octet-stream. I think we can extract most of this function to a common function and then use it in both places.

What UI actions does this concern? View raw pdf?

Can you answer? Is this change about view raw?

[Eriner]

What UI actions does this concern? View raw pdf?

I don't know what "view raw" means in this context.

Currently, all generic package downloads are sent with Content-Disposition: attachment and with Content-Type: application/octet-stream. Regardless of your browser's "open with X by default" setting, the file will only be downloaded. Opening it requires clicking on the downloaded file in all circumstances.

By making this change, clicking a file in a generic package would respect the browser's open handler, which may be the browser itself (PDF, mp3, webm, etc.).

Using PDF as an example, if the browser is the default handler, clicking a PDF in a generic package should open the PDF in a new tab (as opposed to downloading the file and requiring an additional click to view).

[Eriner]

Oh, I understand what you're asking. The UI interaction is clicking this hyperlink on the sidebar:

[KN4CK3R]

To be honest, it was never intended to access the files with a web browser but use the specific package type cli.

Currently, all generic package downloads are sent with Content-Disposition: attachment and with Content-Type: application/octet-stream. Regardless of your browser's "open with X by default" setting, the file will only be downloaded. Opening it requires clicking on the downloaded file in all circumstances.

I can't confirm that. That's a new (anti) feature Firefox introduced some versions ago. You have to manually add the file types to make Firefox use the old logic:

[Eriner]

This behavior is not exclusive to FireFox: Chrome behavior is also to download the file, not open, if the Content-Disposition: attachment header is present.

[denyskon]

Is this PR still relevant?