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.

Sign up for GitHub

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

Jump to bottom

Fix possible frozen hash mutation #172

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+2 −2
Open

Fix possible frozen hash mutation #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d09e455
Freeze original response headers and body in test
uasi Aug 6, 2018
ea03c9c
Fix possible frozen hash mutation
uasi Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rack/cors.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(', '))
Copy link

@swiknaba swiknaba Jan 31, 2024 •
edited

Choose a reason for hiding this comment

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

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".

Copy link

Choose a reason for hiding this comment

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

If you must, how about:

headers = headers.dup if headers.frozen?
headers['foo'] = 'bar'

To avoid memory pollution if headers aren't frozen.

end

if debug? && result = env[RACK_CORS]
Expand Down
2 changes: 1 addition & 1 deletion test/unit/cors_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Author

Choose a reason for hiding this comment

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

['success'].freeze is not related to the fix in this PR, but just in case...

Copy link
Author

Choose a reason for hiding this comment

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

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]

Copy link
Owner

Choose a reason for hiding this comment

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

@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

}
end
end
Expand Down