Don't blow up when passing frozen string to send_file disposition. #1137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since `attachment` does an in-place mutation of `response['Content-Disposition']`, we need to guarantee that this object is not frozen. If a frozen string is passed in, `to_s` is a no-op and returned the same frozen string. So we need to make a new unfrozen string to allow this to work. Also this could have had interesting side effects if the user had passed in a string while holding a reference to it, they would have found their string to be changed by the call to send_file ``` x = 'inline' send_file(some_path, disposition: x) x == 'inline' #=> false ```
|
Also, if 1.4.x is still accepting backports, I'd love to see this back ported. I'm happy to do it if it would be accepted. |
| @@ -358,7 +358,7 @@ def content_type(type = nil, params = {}) | |||
| # Set the Content-Disposition to "attachment" with the specified filename, | |||
| # instructing the user agents to prompt to save. | |||
| def attachment(filename = nil, disposition = :attachment) | |||
| response['Content-Disposition'] = disposition.to_s | |||
| response['Content-Disposition'] = String.new(disposition.to_s) | |||
There was a problem hiding this comment.
Wouldn't dup be more natural here?
There was a problem hiding this comment.
Yeah, dup should work... let me update
|
Bump |
zzak
reviewed
May 6, 2017
| @@ -879,6 +879,12 @@ def send_file_app(opts={}) | |||
| assert_equal 'inline; filename="file.txt"', response['Content-Disposition'] | |||
| end | |||
|
|
|||
| it "does not raise an error when :disposition set to a frozen string" do | |||
| send_file_app :disposition => 'inline'.freeze | |||
There was a problem hiding this comment.
@zzak It happened to up when we added the magic frozen string literal comment at the top of our files.
Since we had inline as a literal in the file, it was frozen and then the in attachment method we get an error modifying a frozen string literal.
As rubocop, code climate, etc are now flagging files with string literals non-frozen I suspect this will be coming up more and more
There was a problem hiding this comment.
I have the same problem after passing the script through rubocop
|
+1 |
namusyaka
approved these changes
Aug 4, 2018
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Sep 23, 2018
## 2.0.4 / 2018-09-15 * Don't blow up when passing frozen string to `send_file` disposition [#1137](sinatra/sinatra#1137) by Andrew Selder * Fix ubygems LoadError [#1436](sinatra/sinatra#1436) by Pavel Rosick�«ò * Unescape regex captures [#1446](sinatra/sinatra#1446) by Jordan Owens * Slight performance improvements for IndifferentHash [#1427](sinatra/sinatra#1427) by Mike Pastore * Improve development support and documentation and source code by Will Yang, Jake Craige, Grey Baker and Guilherme Goettems Schneider ## 2.0.3 / 2018-06-09 * Fix the backports gem regression [#1442](sinatra/sinatra#1442) by Marc-Andr�«± Lafortune ## 2.0.2 / 2018-06-05 * Escape invalid query parameters [#1432](sinatra/sinatra#1432) by Kunpei Sakai * The patch fixes [CVE-2018-11627](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-11627). * Fix undefined method error for `Sinatra::RequiredParams` with hash key [#1431](sinatra/sinatra#1431) by Arpit Chauhan * Add xml content-types to valid html_types for Rack::Protection [#1413](sinatra/sinatra#1413) by Reenan Arbitrario * Encode route parameters using :default_encoding setting [#1412](sinatra/sinatra#1412) by Brian m. Carlson * Fix unpredictable behaviour from Sinatra::ConfigFile [#1244](sinatra/sinatra#1244) by John Hope * Add Sinatra::IndifferentHash#slice [#1405](sinatra/sinatra#1405) by Shota Iguchi * Remove status code 205 from drop body response [#1398](sinatra/sinatra#1398) by Shota Iguchi * Ignore empty captures from params [#1390](sinatra/sinatra#1390) by Shota Iguchi * Improve development support and documentation and source code by Zp Yuan, Andreas Finger, Olle Jonsson, Shota Iguchi, Nikita Bulai and Joshua O'Brien
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since
attachmentdoes an in-place mutation ofresponse['Content-Disposition'], we need to guarantee that this object is not frozen. If a string is passed in,to_sis a no-op and will retain the same frozen status. So we need to make a new unfrozen string to allow this to work.Also this could have had interesting side effects if the user had passed in a string while holding a reference to it, they would have found their string to be changed by the call to send_file.