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

Discussion FormatCoverter "octet-stream"! #57

Closed
LordRekishi opened this issue Feb 2, 2022 · 7 comments
Closed

Discussion FormatCoverter "octet-stream"! #57

LordRekishi opened this issue Feb 2, 2022 · 7 comments
Labels
question Further information is requested wontfix This will not be worked on

Comments

@LordRekishi
Copy link
Contributor

In Issue #12 by @helenahalldiniths we added the FormatConverter for MIME types
MIME types: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types

After reading about the different types I came across "application/octet-stream" and it being labeled as a bad way to solve default header behaviour since it will open a save dialog and might execute whatever you saved in order to try and find the file type. This could perhaps be a security risk?

It is found in our FormatConverter class on line 26-29:

if (type.isEmpty()) {
            type = Optional.of("application");
            subtype = "octet-stream";
        }

According to this quite old stackoverflow question a better solution might be to use to use a more specific type.
https://stackoverflow.com/questions/20508788/do-i-need-content-type-application-octet-stream-for-file-download

There is also an interesting discussion on this GitHub issue here:
broofa/mime#139

I believe they first used text/plain as a default and later changed it to just skip default headers all together.

What are your thoughts?

@LordRekishi LordRekishi added the question Further information is requested label Feb 2, 2022
@kappsegla
Copy link
Contributor

kappsegla commented Feb 2, 2022

According to this link:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types

application/octet-stream should be the default value for non text files.

If the server encounters a file with a file ending not present in our list of known MIME types the best option would be to add this new type with the right type. To avoid needing to recompile the server for doing this a good spot would be a configuration file. Issue #5

@LordRekishi
Copy link
Contributor Author

What about the security aspect of downloading and opening these files with a file dialog?

@kappsegla
Copy link
Contributor

kappsegla commented Feb 2, 2022

What about the security aspect of downloading and opening these files with a file dialog?

If there is a security problem with the file we shouldn't really have it available for download from the server but downloading it and saving it to disk shouldn't be a problem. Most modern browsers would also probably try to scan the file in some way and if it contains problematic code a virus scanner should pick that up.

@kappsegla
Copy link
Contributor

Another way to handle this would be that file types that are unknown for our server aren't allowed to send to the client. Instead of returning 200 OK and send the file we can send an error code like 404 content not found. Makes sense on the client side but can be a bit confusing for someone who sees the file in the web root folder but the server refuses to handle a request for it.

@kappsegla
Copy link
Contributor

Tried downloading some unknown files and files missing file ending from some common web servers and all used application/octet-stream but also added header content-disposition: attachment;filename="CalcTest";filename*=UTF-8''CalcTest

@LordRekishi
Copy link
Contributor Author

How did you go about downloading unknown files and such from random web servers? And, good then we have this solved.

@kappsegla
Copy link
Contributor

How did you go about downloading unknown files and such from random web servers? And, good then we have this solved.

Started the servers myself as docker containers :)

@kappsegla kappsegla added the wontfix This will not be worked on label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants