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

Ensure some cookies don't cause failure #1645

Merged
merged 4 commits into from May 22, 2020
Merged

Conversation

@lukaso
Copy link
Contributor

@lukaso lukaso commented May 5, 2020

Cookies can on occasion have base 64 encoded strings as their value. Base 64 encoded strings tend to end in '=' as this is a filler character. Cookies of this form cause rack to fail.

An example is __cf_bm=_somebase64encodedstringwithequalsatthened=; array=awesome.

This fixes the bug.

Alternative approach would be that in the function I've patched, to only parse the headers required in the enclosing function.

lukaso added 2 commits May 5, 2020
@@ -257,7 +257,7 @@ def identify_cookie_attributes(cookie_filling)
cookie_bits.each do |bit|
if bit.include? '='
cookie_attribute, attribute_value = bit.split('=')

This comment has been minimized.

@ioquatix

ioquatix May 6, 2020
Member

Would it make mores sense to use split('=', 2)?

This comment has been minimized.

@lukaso

lukaso May 6, 2020
Author Contributor

Yes, also works. Either way, the results of this function are discarded other than for these areas:
https://github.com/rack/rack/pull/1645/files#diff-4fc9bc1f7d91630f4f9f47fc6663f3f7L240-L245

It is probably incorrect to assume '=' is a divider.

A third option is not to process the first element of the cookie_bits array at all, since this is the value. So cookie_bits[1..].each do |bit|.

Fourth option, do both since the value could possibly also contain '='.

This comment has been minimized.

@lukaso

lukaso May 6, 2020
Author Contributor

Done both in f48f0b8

@lukaso
Copy link
Contributor Author

@lukaso lukaso commented May 6, 2020

BTW: the build failures appear to be due to this: rubygems/rubygems#3570

Copy link
Member

@tenderlove tenderlove left a comment

This seems good, but we need to make sure the build passes. It looks like there is a legit failure in 2.5

lib/rack/mock.rb Outdated Show resolved Hide resolved
Co-authored-by: Aaron Patterson <aaron.patterson@gmail.com>
@lukaso
Copy link
Contributor Author

@lukaso lukaso commented May 8, 2020

This seems good, but we need to make sure the build passes. It looks like there is a legit failure in 2.5

Done in c17569c. Thanks!

@lukaso lukaso requested a review from tenderlove May 12, 2020
@tenderlove tenderlove merged commit 6a50e46 into rack:master May 22, 2020
12 of 15 checks passed
12 of 15 checks passed
test (ubuntu-latest, 2.3) test (ubuntu-latest, 2.3)
Details
test (ubuntu-latest, 2.4)
Details
test (ubuntu-latest, 2.5)
Details
test (ubuntu-latest, 2.6)
Details
test (ubuntu-latest, 2.7)
Details
test (ubuntu-latest, jruby)
Details
test (ubuntu-latest, truffleruby-head)
Details
test (macos-latest, 2.3) test (macos-latest, 2.3)
Details
test (macos-latest, 2.4)
Details
test (macos-latest, 2.5)
Details
test (macos-latest, 2.6)
Details
test (macos-latest, 2.7)
Details
test (macos-latest, jruby)
Details
test (macos-latest, truffleruby-head)
Details
external
Details
@lukaso lukaso deleted the simplybusiness:base64_cookie_values branch Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.