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

Various Fixes #1

Merged
merged 5 commits into from
May 2, 2022
Merged

Various Fixes #1

merged 5 commits into from
May 2, 2022

Conversation

jeremyevans
Copy link
Contributor

Fix autoloading.

Avoid defining constants that conflict with Rack.

Make Rack::Session::Cookie backwards compatible with :secret option.

These are the same autoloads previously used when the session
support was included in rack.

Remove the duplicate constant definitions to avoid constant
warnings when rack is also in use (which it must be for this
to work, as parts of rack-session depend on rack).
Previously, :secret was used to store the HMAC secret.  If it is
used, use it as a fallback to set both the encryption secret and
the legacy HMAC secret.

From a cryptographic perspective, it's best to avoid sharing
secrets like this, even though I'm guessing it is not vulnerable
(note: this is not an educated guess).  I think this is better
than completely breaking backwards compatibility.

The best way to handle conversion from legacy HMAC would be to
specify :secrets in addition to :secret (or :legacy_hmac_secret),
then remove :secret/:legacy_hmac_secret after all sessions have
been upgraded.
@jeremyevans
Copy link
Contributor Author

@ioquatix flagging you as reviewer (GitHub UI isn't offering you as an option).

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor discussion points.

lib/rack/session/constants.rb Show resolved Hide resolved
FNM_DOTMATCH is not needed (no additional files would match with
it).  base keyword is what breaks CI on Ruby 2.4, and is not needed
as gem is generally build already in the same directory as the
gemspec.
@ioquatix ioquatix merged commit 7071f65 into rack:main May 2, 2022
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.

2 participants