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

Don't blow up when passing frozen string to send_file disposition. #1137

Merged
merged 2 commits into from Aug 4, 2018
Merged

Don't blow up when passing frozen string to send_file disposition. #1137

merged 2 commits into from Aug 4, 2018

Conversation

aselder
Copy link

@aselder aselder commented Jul 19, 2016

Since attachment does an in-place mutation of response['Content-Disposition'], we need to guarantee that this object is not frozen. If a string is passed in, to_s is 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.

x = 'inline'
send_file(some_path, disposition: x)
x == 'inline' #=> false

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
```
@aselder
Copy link
Author

aselder commented Jul 20, 2016

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't dup be more natural here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, dup should work... let me update

@aselder
Copy link
Author

aselder commented Feb 11, 2017

Bump

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this actually occur?

Copy link
Author

@aselder aselder May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same problem after passing the script through rubocop

@namusyaka namusyaka added this to the v2.0.2 milestone Feb 19, 2018
@flyerhzm
Copy link

flyerhzm commented May 2, 2018

+1

@namusyaka namusyaka modified the milestones: v2.0.2, v2.0.3, v2.0.4 Jun 5, 2018
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. But unfortunately, 1.4.x has not been maintained except security vulnerability.

@namusyaka namusyaka merged commit 89d0ff1 into sinatra:master 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants