Fix possible frozen hash mutation #172
Conversation
| @@ -55,7 +55,7 @@ def load_app(name) | |||
| eval File.read(File.dirname(__FILE__) + "/#{name}.ru") | |||
| map('/') do | |||
| run proc { |env| | |||
| [200, {'Content-Type' => 'text/html'}, ['success']] | |||
| [200, {'Content-Type' => 'text/html'}.freeze, ['success'].freeze] | |||
There was a problem hiding this comment.
['success'].freeze is not related to the fix in this PR, but just in case...
There was a problem hiding this comment.
This change makes some test cases raise error as expected.
$ bundle exec rake test
Run options: --seed 54117
# Running:
..........................EE...EEE..E.E............
Finished in 0.065923s, 773.6298 runs/s, 1365.2291 assertions/s.
1) Error:
Rack::Cors#test_0009_should not mix up path rules across origins:
RuntimeError: can't modify frozen Hash
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/lib/rack/cors.rb:120:in `call'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-2.0.5/lib/rack/lint.rb:49:in `_call'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-2.0.5/lib/rack/lint.rb:37:in `call'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/test/unit/cors_test.rb:37:in `call'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-2.0.5/lib/rack/builder.rb:153:in `call'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-test-1.1.0/lib/rack/mock_session.rb:29:in `request'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-test-1.1.0/lib/rack/test.rb:266:in `process_request'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-test-1.1.0/lib/rack/test.rb:129:in `custom_request'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/vendor/bundle/gems/rack-test-1.1.0/lib/rack/test.rb:58:in `get'
/Users/uasi/Dev/Git/github.com/cyu/rack-cors/test/unit/cors_test.rb:104:in `block (2 levels) in <top (required)>'
2) Error:
Rack::Cors#test_0002_should not return CORS headers if Origin header isn't present:
RuntimeError: can't modify frozen Hash
[snip]
There was a problem hiding this comment.
@uasi Thank you for this! Can you do me a favor and create a specific test for this? I'd want to make sure we document (in a test) that we support this explicitly.
Thanks!
Calvin
| @@ -117,7 +117,7 @@ def call(env) | |||
| else | |||
| DEFAULT_VARY_HEADERS | |||
| end | |||
| headers[VARY] = ((vary ? vary.split(/,\s*/) : []) + cors_vary_headers).uniq.join(', ') | |||
| headers = headers.merge(VARY => ((vary ? vary.split(/,\s*/) : []) + cors_vary_headers).uniq.join(', ')) | |||
There was a problem hiding this comment.
Using .merge creates a new hash, which will bloat memory. As of version 3, the rack specification states that headers must be an unfrozen hash, see https://github.com/rack/rack/blob/main/CHANGELOG.md#spec-changes-2 & rack/rack#1597
Thus, once you're on Rack 3, this issue should resolve itself, and we don't need to bloat memory "just in case".
There was a problem hiding this comment.
If you must, how about:
headers = headers.dup if headers.frozen?
headers['foo'] = 'bar'To avoid memory pollution if headers aren't frozen.
rack-cors can raise runtime error when the headers hash returned by app is frozen.