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

fix getting null id for some versions of rack #294

Closed
wants to merge 2 commits into from

Conversation

rzjfr
Copy link
Contributor

@rzjfr rzjfr commented Dec 20, 2020

Users with rake version that do not have rack/rack#1462 will have problem getting session.id.to_s after GHSA-hrqr-hxpp-chr3

I'd say having public_id is much safer, it should not break any version.

I also bumped into this issue #292 which might be fixed with this change.

@@ -149,13 +149,13 @@ def session_hash
# # request.session_options[:id].encode("ISO-8859-1")
if Rails::VERSION::MAJOR >= 4
session["init"] = true
id = session.id.to_s
id = session.id.public_id.to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

@rzjfr
If session.id is null, It will raise exception here. You should check null value for session.id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, title is misleading

Copy link
Member

@invalidusrname invalidusrname May 3, 2021

Choose a reason for hiding this comment

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

Thanks digging into this @rzjfr. I think something like this might work for any rack version >= 2.1.0

def session_hash
  id = session.id || request.session_options[:id]

  if id.respond_to?(:cookie_value)
    id.cookie_value
  elsif id.is_a?(Rack::Session::SessionId)
    id.public_id
  end
end

@invalidusrname invalidusrname changed the title fix getting null id for some rake versions fix getting null id for some versions of rack May 3, 2021
@invalidusrname
Copy link
Member

invalidusrname commented Dec 21, 2021

I noticed we have a couple of PRs related to method. I'm going to close this one out in favor of #298

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.

3 participants