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

When split cookie is a valid, json encoded String, a NoMethodError is raised #695

Closed
knarewski opened this issue Sep 7, 2022 · 2 comments · Fixed by #697
Closed

When split cookie is a valid, json encoded String, a NoMethodError is raised #695

knarewski opened this issue Sep 7, 2022 · 2 comments · Fixed by #697
Assignees

Comments

@knarewski
Copy link
Contributor

Describe the bug
When split cookie is a valid, json encoded String, a NoMethodError is raised.

To Reproduce
Steps to reproduce the behavior:

  1. Save the following script as test.rb
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'split'
  gem 'sinatra'
  gem 'sinatra-contrib'
  gem 'redis', '~> 4.8'
end

Split.configure do |config|
  config.persistence = :cookie
  config.experiments = { foo: { alternatives: ['a', 'b'] } }
end

class MySinatraApp < Sinatra::Base
  helpers Sinatra::Cookies
  helpers Split::Helper

  get '/' do
    ab_test(:foo)
  end
end

MySinatraApp.run!
  1. Run: ruby test.rb
  2. On a separate terminal window, invoke curl -H "User-Agent: not-a-bot" -H 'Cookie: split="valid-json-string"' localhost:4567
  3. See the app crashing with NoMethodError - undefined method `keys' for "valid-json-string":String

Expected behavior
The invalid content is ignored, similarly to how unparseable JSON is ignored

Additional context
The trivial check in cookie_adapter.rb works for me:

              parsed = JSON.parse(cookies)
              parsed.is_a?(String) ? {} : parsed

I'm happy to submit a PR if desired

@andrehjr
Copy link
Member

andrehjr commented Sep 8, 2022

Hi @knarewski! Thanks for the issue and the repro!

Please send a PR 🎉 We should also protect it against other types. if JSON.parse returns anything other than a Hash, split wouldn't know how to handle it.

An integer would also break:

curl -H "User-Agent: not-a-bot" -H 'Cookie: split=1' localhost:4567

If I remember correctly, this issue should happens with json >= 2.0. Because the behavior of JSON.parse changed.

@knarewski
Copy link
Contributor Author

Hi @andrehjr ! Thank you for such a swift response 🚀 ! I prepared a fix, but it's blocked by an unrelated CI failure. I'm happy to submit a PR once the CI fix is merged: #696

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 a pull request may close this issue.

2 participants