phusion / passenger

A fast and robust web server and application server for Ruby, Python and Node.js
https://www.phusionpassenger.com/
MIT License
5.01k stars 547 forks source link

Issues with cookies when using latest Passenger and Rack 3 (w/ Ruby on Rails 7.1) #2503

Closed brunnopleffken closed 12 months ago

brunnopleffken commented 1 year ago

Issue report

Fill in as much as possible so that we can understand, find and fix the problem.

Are you sure this is a bug in Passenger? Me and the Ruby on Rails maintainers point that this could be a Rack 3 + Passenger compatibility issue, as forcing the application manifest to use Rack ~> 2.2 makes everything work well.

Please try with the newest version of Passenger to avoid issues that have already been fixed I'm already using Passenger 6.0.18.

Question 1: What is the problem? This issue is descibed in details in rails/rails#49422. Cookies names, like user_id are being prepended with [", turning it into ["user_id, so, important authentication scripts like

if cookies[:user_id].blank?

just doesn't work.

271427206-96f86b61-c7bf-4e1c-b616-bfba3ab664c2

This happens after upgrading a Ruby on Rails project from Rails 7.0.8 (using Rack 2.2) to Rails 7.1 (which uses Rack 3.0). This is not a Rails issue, as downgrading from Rack 3 to Rack 2 solves it.

There is no error when using Rack 3 without Passenger (for example, Nginx with Puma), so this is not a Rack issue either.

It's important to note that Rack 3 is the default for Ruby on Rails 7.1, so, new apps and upgraded apps could break!

Question 2: Passenger version and integration mode: open source 6.0.18/nginx 1.18.0

Question 3: OS or Linux distro, platform (including version): Ubuntu 22.04.3 LTS x86-64

Question 4: Passenger installation method:

Your answer: [ ] RubyGems + Gemfile [ ] RubyGems, no Gemfile [x] Phusion APT repo [ ] Phusion YUM repo [ ] OS X Homebrew [ ] source tarball [ ] Other, please specify:

Question 5: Your app's programming language (including any version managers) and framework (including versions): Ruby 3.2.2 with YJIT, RVM, Rails 7.1 RC2

Question 6: Are you using a PaaS and/or containerization? If so which one? No.

Question 7: Anything else about your setup that we should know? No.


We strive for quality and appreciate you taking the time to submit a report! Please note that if you want guaranteed response times and priority issue support we encourage you to join our enterprise customer base. They also provide us with the means to continue our high level of open source support!

ioquatix commented 1 year ago

For this specific issue, the correct solution is for Passenger to correctly handle response headers which can be an Array of values. In addition, using newline characters is no longer supported in Rack 3.

datanoise commented 1 year ago

I know that we can force to use gem install rack -v 2.2.4 to avoid this issue for now. But are there any plans to support Rack 3.x?

BTW, currently when you install passenger gem, it will automatically install Rack 3. Which I believe renders it impossible to run any Rails application without issues with cookies.

https://github.com/phusion/passenger/blob/467259e673ec66b121b10eff1dd84ac1188ea3d2/passenger.gemspec#L26C3-L26C26

CamJN commented 1 year ago

I will try to fix this, but am currently swamped trying to fix our CI, you may have noticed the 6.0.19 release is quite late at this point. A PR would be much faster than waiting for me to fix it, but I'll get to it eventually, it just likely won't be in the next release.

brunnopleffken commented 1 year ago

BTW, currently when you install passenger gem, it will automatically install Rack 3. Which I believe renders it impossible to run any Rails application without issues with cookies.

To add to the equation: even if you don't use Passenger gem, Ruby on Rails 7.1 now uses Rack 3 by default. So, upgrading an app to Rails 7.1 will break the app if it relies on cookies for authentication of their users. And it doesn't fire any log message, many think it's a Rails issue and it's not. So the devs are blind about this and the root of the problem.

Fjan commented 1 year ago

Just got bitten by this! In our test pipeline we use puma because installing a full apache there is a hassle, and trying out the site on a staging server appeared to work fine because the browsers we tried with apparently still had a cookie from Rails 7.0.8 / rack 2.

After 10 minutes in production our help desk started to light up with "can't log in" messages... ouch. More people are going to get bitten by this, you may want to send an email to your users to warn them. @FooBarWidget

CamJN commented 1 year ago

If someone feels like helping, I pushed https://github.com/phusion/passenger/commit/7353892025f245b1f29a35d4337cc0a152aa1bb8 to hopefully fix this, so you could test it and report back.

ioquatix commented 1 year ago

@CamJN this is but one of several changes required to support Rack 3.

I think a better short term option would be to detect Rack.release and fail if it's not a compatible (e.g. < 3 I suppose at the moment).

You should also consider adding a PR to test Rack 3 with Passenger to https://github.com/socketry/rack-conform - this will bring attention to Rack 3 specific issues. You should also consider running your own test suite with Rack::Lint from Rack 3.

CamJN commented 1 year ago

@ioquatix according to https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md

There is one changed feature in Rack 3 which is not backwards compatible Response header values can be an Array to handle multiple values (and no longer supports \n encoded headers).

ioquatix commented 1 year ago

That's "the one changed feature which is not backwards compatible" but there are other new features which you must support and handle correctly at the server level, most notably streaming.

CamJN commented 1 year ago

Well, that's good enough to fix this bug, if you want to request other features, perhaps open a separate feature request.

mitchellhenke commented 1 year ago

Thanks for passenger and appreciate the quick fix! Is there anything I can do to help get the change released?

CamJN commented 1 year ago

@mitchellhenke no. The release is done, just sadly being held up by some CI issues we're sorting out.

rmatovu987 commented 1 year ago

Hello @CamJN any update regarding the release?

CamJN commented 1 year ago

@rmatovu987 see the comment immediately before yours.

benkruger commented 1 year ago

This is blocking a critical bug fix with another gem for us :(

ansonhoyt commented 1 year ago

@benkruger why can't you just stick with Rack 2.2 for now? Even once @CamJN gets CI working and lands this fix 🙏, it's our choice to jump into Rack 3 or wait for things to smooth out. They'll get there in time.

With my apps, we're going to stay a bit cautious given https://github.com/phusion/passenger/issues/2503#issuecomment-1754003259 is authored by someone who has some authority on Rack conformance, and his nice suggestion would help Passenger find and fix any other Rack 3 compat issues that could be lurking, and demonstrate that Passenger is ready for production use with Rack 3.

akaspick commented 1 year ago

Playing around a bit with my Rails config, I can get Rails 7.1 working with Rack 3 if I change the ssl_options to be:

config.force_ssl = true # this is already the default for production apps
config.ssl_options = {secure_cookies: false} # added this config option otherwise cookies break if secure cookies is true

Not saying you should make this change to get your apps working in production, but it appears as though this is only a secure cookies issue.

Fjan commented 1 year ago

@akaspick SSL or not should not matter to the way cookies are set. What you may be seeing (what tricked me at some point) is that cookies set by rack 2 will work fine on rack 3. So I would be interested if you can reproduce this after clearing your cookies.

akaspick commented 1 year ago

@Fjan I tested this with all my cookies deleted first so it could create new ones. In my web console, I could see the secure cookie being set with my original config and things didn't work. Cleared cookies, changed to unsecure cookies, and the unsecure cookie was set and worked.

Anyway, this change in the secure setting seems to make a difference for some reason.

akaspick commented 1 year ago

So secure cookies do seem to work just fine after more testing. The issue appears when usingconfig.force_ssl = true which includes the SSL middleware (https://github.com/rails/rails/blob/main/actionpack/lib/action_dispatch/middleware/ssl.rb)

The commit to make the middleware compatible with Rack 3 https://github.com/rails/rails/commit/9d840a17197ffa5dec8cb2d4171450dfa12c156f changed to the array format at https://github.com/rails/rails/blob/9d840a17197ffa5dec8cb2d4171450dfa12c156f/actionpack/lib/action_dispatch/middleware/ssl.rb#L116 which breaks passenger.

That's why using config.ssl_options = {secure_cookies: false} makes things work because it skips the function that converts to an array.

pcantrell commented 1 month ago

If anyone like me is stuck on an old version of Linux that went outside the Passenger support window just before this patch, here’s a workaround you can add to your Rails app. Put this in config/initializers/passenger_cookie_workaround.rb:

# Workaround for https://github.com/phusion/passenger/issues/2503
# (which is fixed in the latest Passenger, but it no longer supports the server’s ancient Ubuntu)
class PassengerCookieWorkaround
  def initialize(app)
    @app = app
  end

  def call(env)
    if env["SERVER_SOFTWARE"] =~ %r{Phusion_Passenger/(\d+\.\d+\.(\d+)?)}
      if Gem::Version.new($1) < Gem::Version.new('6.0.19')
        workaround_enabled = true
        puts "Passenger SSL cookie workaround enabled; upgrade Passenger to 6.0.19 or later when possible"
      end
    end

    @app.call(env).tap do |status, headers, body|
      if workaround_enabled
        headers.each do |k,v|
          headers[k] = v.join("\n") if v.is_a?(Array)
        end
      end
    end
  end
end

Rails.application.config.middleware.unshift PassengerCookieWorkaround