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 array of cookies in response containing a blank cookie #343

Merged
merged 1 commit into from Feb 1, 2024

Conversation

martinemde
Copy link
Contributor

@martinemde martinemde commented Feb 1, 2024

Without the fix, the blank cookie is compared and since it has no name the comparison fails on name.downcase. Rather than correct it there, this expands the processing of merged cookies to remove blank cookies not just in strings, but also in arrays.

I altered the way empty cookies are removed from the raw_cookies because a passed array should not be mutable, so we just skip them during iteration now in both cases. This should also save a cycle or two updating the array result of split.

Also, my editor auto-cleaned up some whitespace. Let me know if that's undesirable and I will revert that line.

Without the fix, the blank cookie is compared and since it has no name
the comparison fails on name.downcase. Rather than correct it there, this
expands the processing of merged cookies to remove blank cookies not just
in strings, but also in arrays.
@jeremyevans
Copy link
Contributor

Thanks for the report and patch. The fix looks good from a brief review. I'll do some testing with this tomorrow.

@jeremyevans jeremyevans merged commit 35b7310 into rack:main Feb 1, 2024
24 checks passed
@martinemde martinemde deleted the fix-empty-cookie-merge branch February 1, 2024 17:23
@martinemde
Copy link
Contributor Author

martinemde commented Feb 1, 2024

If you need to test, the way we were triggering this was:

  set_header("set-cookie", "") # this was actually `cookiejar.to_header` but it returns "" when there's no cookies

# then in content_security_policy middleware nonce generator, we forced the session to initialize so that a session id would be available as the nonce.

Rails.application.config.content_security_policy_nonce_generator = lambda { |request|
  # Suggested nonce generator doesn't work on first page load https://github.com/rails/rails/issues/48463
  # Related PR attempting to fix: https://github.com/rails/rails/pull/48510
  request.session.update({}) # force session to be created
  request.session.id.to_s.presence || SecureRandom.base64(16)
}

It's a bit convoluted and took me hours to track down. Ultimately the fix for us was to not write the set-cookie header if the cookiejar was empty, but the downstream problem was that rack-test just raised no method downcase on nil when it tried to figure out if it should replace? the blank cookie with the newly initialized session cookie.

This looked like cookiejar in rack-test receiving: cookiejar.merge(["", "sessioncookiestring"])

it '#merge ignores empty cookies in cookie strings' do
jar = Rack::Test::CookieJar.new
jar.merge('', URI.parse('/'))
jar.merge("\nc=d")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh hey, I missed a URI second arg here. Maybe that should be added if it's needed.

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.

None yet

2 participants