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

Set filename in Content-Disposition if filename=x is passed in URI query #4177

Merged
merged 1 commit into from Aug 21, 2018

Conversation

Projects
None yet
6 participants
@Voker57
Contributor

Voker57 commented Aug 28, 2017

Helps with referring to hashes by specific filename, eliminated unixfs node need for that

Example: http://ipfs.io/ipfs/Qmfoobar -- downloads Qmfoobar.zip (if mime was detected by first block)
http://ipfs.io/ipfs/Qmfoobar?filename=stuff.zip -- downloads stuff.zip

@crackcomm

This comment has been minimized.

Show comment
Hide comment
@crackcomm

crackcomm Nov 28, 2017

It works for .zip but .pdf or .png will need attachment to download (not inline).

crackcomm commented Nov 28, 2017

It works for .zip but .pdf or .png will need attachment to download (not inline).

@Voker57

This comment has been minimized.

Show comment
Hide comment
@Voker57

Voker57 Nov 28, 2017

Contributor

That's out of scope of this PR: I just want files that are saved from link to be named properly.

Contributor

Voker57 commented Nov 28, 2017

That's out of scope of this PR: I just want files that are saved from link to be named properly.

@crackcomm

This comment has been minimized.

Show comment
Hide comment
@crackcomm

crackcomm Nov 28, 2017

@Voker57 sounds good, I wanted to suggest a download query parameter but only after posting a comment :)

crackcomm commented Nov 28, 2017

@Voker57 sounds good, I wanted to suggest a download query parameter but only after posting a comment :)

Set filename in Content-Disposition if filename=x is passed in URI query
License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
@radfish

This comment has been minimized.

Show comment
Hide comment
@radfish

radfish Aug 18, 2018

@lgierth Is this waiting on any more changes? Is there a need for a branch based on MIME type? ('inline' for application/pdf, and 'attachment' for everything else) or is 'inline' ok to pass for all MIME types?

Without this feature linking to files is a big problem (I often specifically do not want wrapping directories, but a direct link). Luckily, for application/pdf, for whatever reason, Firefox seems to grab the filename if you append any query, even any parameter name at all, even verbatim .../hash?foo=XYZ opens in the PDF viewer and the title is set to XYZ and if you press the save button, the name is XYZ. Firefox does this, because the headers are identical between .../hash?foo=XYZ and .../hash. But, for other types, things are bad, no filename in the save dialog.

radfish commented Aug 18, 2018

@lgierth Is this waiting on any more changes? Is there a need for a branch based on MIME type? ('inline' for application/pdf, and 'attachment' for everything else) or is 'inline' ok to pass for all MIME types?

Without this feature linking to files is a big problem (I often specifically do not want wrapping directories, but a direct link). Luckily, for application/pdf, for whatever reason, Firefox seems to grab the filename if you append any query, even any parameter name at all, even verbatim .../hash?foo=XYZ opens in the PDF viewer and the title is set to XYZ and if you press the save button, the name is XYZ. Firefox does this, because the headers are identical between .../hash?foo=XYZ and .../hash. But, for other types, things are bad, no filename in the save dialog.

@Stebalien

This comment has been minimized.

Show comment
Hide comment
@Stebalien

Stebalien Aug 20, 2018

Contributor

Is this waiting on any more changes?

Looks like it just dropped through the cracks.

Is there a need for a branch based on MIME type? ('inline' for application/pdf, and 'attachment' for everything else) or is 'inline' ok to pass for all MIME types?

This PR doesn't appear to do any content sniffing, it just sets the appropriate file name.

Contributor

Stebalien commented Aug 20, 2018

Is this waiting on any more changes?

Looks like it just dropped through the cracks.

Is there a need for a branch based on MIME type? ('inline' for application/pdf, and 'attachment' for everything else) or is 'inline' ok to pass for all MIME types?

This PR doesn't appear to do any content sniffing, it just sets the appropriate file name.

@Stebalien

This comment has been minimized.

Show comment
Hide comment
@Stebalien

Stebalien Aug 20, 2018

Contributor

I've checked and this does not trigger #4025.

Contributor

Stebalien commented Aug 20, 2018

I've checked and this does not trigger #4025.

@Stebalien Stebalien requested a review from lgierth Aug 20, 2018

@Stebalien

This comment has been minimized.

Show comment
Hide comment
@Stebalien

Stebalien Aug 20, 2018

Contributor

@lgierth. I'd like to merge this. Any objections?

Contributor

Stebalien commented Aug 20, 2018

@lgierth. I'd like to merge this. Any objections?

@Stebalien

This comment has been minimized.

Show comment
Hide comment
@Stebalien

Stebalien Aug 20, 2018

Contributor

I've added some documentation here: #5393

Contributor

Stebalien commented Aug 20, 2018

I've added some documentation here: #5393

@Stebalien Stebalien merged commit fcc96a3 into ipfs:master Aug 21, 2018

7 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch 100% of diff hit (target 63.12%)
Details
codecov/project 63.13% (+0.01%) compared to 2140aa9
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Stebalien Stebalien removed the needs review label Aug 21, 2018

@radfish

This comment has been minimized.

Show comment
Hide comment
@radfish

radfish Aug 21, 2018

radfish commented Aug 21, 2018

@Stebalien

This comment has been minimized.

Show comment
Hide comment
@Stebalien

Stebalien Aug 22, 2018

Contributor

According to the docs, "inline" is the default value so this shouldn't cause a problem. We should probably also add a download=true flag but that's a separate issue.

(thanks for bringing this to our attention, by the way)

Contributor

Stebalien commented Aug 22, 2018

According to the docs, "inline" is the default value so this shouldn't cause a problem. We should probably also add a download=true flag but that's a separate issue.

(thanks for bringing this to our attention, by the way)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment