Cookie domain calculation broken with domain: all after 6.1.7.1 upgrade

[ilya-konanykhin]

The recent 6.1.7.1 update changed how domain: :all for cookies works and is now handling two letter domains incorrectly: v6.1.7...v6.1.7.1#diff-ec27bb8290189e09e65a52e49641cd7952660dcf237a327311f79ddd65d01eecR457-R462

The REGEXP in the code above grabs ANY xx(x).xx host suffix (example.nl -> le.nl) and falls into the else branch when it shouldn't. I assume there is a leading dot missing .xx(x).xx, so the REGEXP should be smth like /\.([^.]{2,3}\.[^.]{2})$/ but not sure, please check.

Why tests didn't catch it? Because they test with www.nextangle.com host which is not affected due to 3 letter com ending.

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "rails", "~> 6.1.7"
end

require "rack/test"
require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts = []
  config.session_store :cookie_store, key: "cookie_store_key"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    get "/" => "test#index"
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    cookies["test"] = {value: "1", domain: :all}
    render plain: "Home"
  end
end

require "minitest/autorun"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_example_org
    get "http://example.org/"
    assert last_response.ok?
    assert_equal ".example.org", last_response.cookies["test"].domain
  end

  def test_www_example_org
    get "http://www.example.org/"
    assert last_response.ok?
    assert_equal ".example.org", last_response.cookies["test"].domain
  end

  def test_example_nl
    get "http://example.nl/"
    assert last_response.ok?
    assert_equal ".example.nl", last_response.cookies["test"].domain
  end

  def test_www_example_nl
    get "http://www.example.nl/"
    assert last_response.ok?
    assert_equal ".example.nl", last_response.cookies["test"].domain
  end

  private
    def app
      Rails.application
    end
end

Expected behavior

www.example.nl should yield .example.nl cookie domain if used with domain: :all.

Actual behavior

.www.example.nl is produced, the last test fails test_www_example_nl.

System configuration

Rails version: 6.1.7.1

Ruby version: 2.5.1

[ilya-konanykhin]

Also in that REGEXP brackets are redundant I think, they aren't used neither for grouping nor for matching, but may be wrong.

[sgnh]

I ran into this problem in production too on 7.0.4.1, and the result is that a session cookie is now set for both: .domain.co and .www.domain.co

This means that rolling back to 7.0.4 switches the cookie back to .domain.co, but the existing .www.domain.co gets picked first as it's more specific. So our users are now stuck with the old invalid session, unable to sign in.

So even when this gets fixed in Rails, it doesn't fix it for the users as far as I can tell. Does anyone have any idea on how we can wipe the invalid cookie, without asking the users to do it locally?

[sgnh]

Quick update for anyone who gets their users locked out by this. I ended up rolling back to 7.0.4 and changing the "key" on Rails.application.config.session_store. This will effectively reset everybody's session and stop any looping caused by the wrong session being picked.

[zzak]

@jhawthorn Could you take a look? 🙇

[ilya-konanykhin]

Quick update for anyone who gets their users locked out by this. I ended up rolling back to 7.0.4

FYI if you don't want/can rollback and your app responds to a single (set of) known up front domain(s), you can specify the session cookie domain explicitly:

Rails.application.config.session_store :cookie_store, {...otheroptions..., domain: ".example.nl"}
# or domain: [".foo.nl", ".bar.nl"]

This will disable the :all logic at least for session cookies, which is the main problem for the majority of users I think.

[KevinSchiffmann]

We have the same cookie issue with Rails 7.0.4.1 (Ruby 3.1.2), with domain like "https://customer.project.ch".

Thank you!

[langalex]

Here's a temporary fix/monkey patch. It basically loads the code before the change.

Use #47087 instead.

unless ActionPack::VERSION::STRING == '7.0.4.1'
  raise 'check if patch is still needed. version changed'
end

module ActionDispatch
  module RequestCookieMethods
    def cookies_same_site_protection
      get_header(Cookies::COOKIES_SAME_SITE_PROTECTION)&.call(self)
    end
  end

  ActiveSupport.on_load(:action_dispatch_request) do
    include RequestCookieMethods
  end

  class Cookies
    class CookieJar # :nodoc:
      include ChainedCookieJars
      include Enumerable

      DOMAIN_REGEXP = /[^.]*\.([^.]*|..\...|...\...)$/

      def self.build(req, cookies)
        jar = new(req)
        jar.update(cookies)
        jar
      end

      attr_reader :request

      def initialize(request)
        @set_cookies = {}
        @delete_cookies = {}
        @request = request
        @cookies = {}
        @committed = false
      end

      def committed?
        @committed
      end

      def commit!
        @committed = true
        @set_cookies.freeze
        @delete_cookies.freeze
      end

      def each(&block)
        @cookies.each(&block)
      end

      # Returns the value of the cookie by +name+, or +nil+ if no such cookie exists.
      def [](name)
        @cookies[name.to_s]
      end

      def fetch(name, *args, &block)
        @cookies.fetch(name.to_s, *args, &block)
      end

      def key?(name)
        @cookies.key?(name.to_s)
      end
      alias has_key? key?

      # Returns the cookies as Hash.
      alias to_hash to_h

      def update(other_hash)
        @cookies.update(other_hash.stringify_keys)
        self
      end

      def update_cookies_from_jar
        request_jar = @request.cookie_jar.instance_variable_get(:@cookies)
        set_cookies =
          request_jar.reject do |k, _|
            @delete_cookies.key?(k) || @set_cookies.key?(k)
          end

        @cookies.update(set_cookies) if set_cookies
      end

      def to_header
        @cookies.map { |k, v| "#{escape(k)}=#{escape(v)}" }.join('; ')
      end

      # Sets the cookie named +name+. The second argument may be the cookie's
      # value or a hash of options as documented above.
      def []=(name, options)
        if options.is_a?(Hash)
          options.symbolize_keys!
          value = options[:value]
        else
          value = options
          options = { value: }
        end

        handle_options(options)

        if @cookies[name.to_s] != value || options[:expires]
          @cookies[name.to_s] = value
          @set_cookies[name.to_s] = options
          @delete_cookies.delete(name.to_s)
        end

        value
      end

      # Removes the cookie on the client machine by setting the value to an empty string
      # and the expiration date in the past. Like <tt>[]=</tt>, you can pass in
      # an options hash to delete cookies with extra data such as a <tt>:path</tt>.
      def delete(name, options = {})
        return unless @cookies.has_key?(name.to_s)

        options.symbolize_keys!
        handle_options(options)

        value = @cookies.delete(name.to_s)
        @delete_cookies[name.to_s] = options
        value
      end

      # Whether the given cookie is to be deleted by this CookieJar.
      # Like <tt>[]=</tt>, you can pass in an options hash to test if a
      # deletion applies to a specific <tt>:path</tt>, <tt>:domain</tt> etc.
      def deleted?(name, options = {})
        options.symbolize_keys!
        handle_options(options)
        @delete_cookies[name.to_s] == options
      end

      # Removes all cookies on the client machine by calling <tt>delete</tt> for each cookie.
      def clear(options = {})
        @cookies.each_key { |k| delete(k, options) }
      end

      def write(headers)
        if (header = make_set_cookie_header(headers[HTTP_HEADER]))
          headers[HTTP_HEADER] = header
        end
      end

      mattr_accessor :always_write_cookie, default: false

      private

      def escape(string)
        ::Rack::Utils.escape(string)
      end

      def make_set_cookie_header(header)
        header =
          @set_cookies.inject(header) do |m, (k, v)|
            write_cookie?(v) ? ::Rack::Utils.add_cookie_to_header(m, k, v) : m
          end
        @delete_cookies.inject(header) do |m, (k, v)|
          ::Rack::Utils.add_remove_cookie_to_header(m, k, v)
        end
      end

      def write_cookie?(cookie)
        request.ssl? || !cookie[:secure] || always_write_cookie ||
          request.host.end_with?('.onion')
      end

      def handle_options(options)
        options[:expires] = options[:expires].from_now if options[
          :expires
        ].respond_to?(:from_now)

        options[:path] ||= '/'

        options[
          :same_site
        ] = request.cookies_same_site_protection unless options.key?(:same_site)

        if options[:domain] == :all || options[:domain] == 'all'
          # If there is a provided tld length then we use it otherwise default domain regexp.
          domain_regexp =
            (
              if options[:tld_length]
                /([^.]+\.?){#{options[:tld_length]}}$/
              else
                DOMAIN_REGEXP
              end
            )

          # If host is not ip and matches domain regexp.
          # (ip confirms to domain regexp so we explicitly check for ip)
          options[:domain] = if !request.host.match?(/^[\d.]+$/) &&
               (request.host =~ domain_regexp)
            ".#{::Regexp.last_match(0)}"
          end
        elsif options[:domain].is_a?(Array)
          # If host matches one of the supplied domains.
          options[:domain] = options[:domain].find do |domain|
            domain = domain.delete_prefix('.')
            request.host == domain || request.host.end_with?(".#{domain}")
          end
        elsif options[:domain].respond_to?(:call)
          options[:domain] = options[:domain].call(request)
        end
      end
    end
  end
end

[rafaelfranca]

It basically loads the code before the change.

@langalex Isn't this reintroducing the security issue? At that point, it is better to not upgrade (that I don't recommend as well).

[jhawthorn]

I've opened #47087. Would love help confirming that fixes the issue 🙇‍♂️

[langalex]

It basically loads the code before the change.

@langalex Isn't this reintroducing the security issue? At that point, it is better to not upgrade (that I don't recommend as well).

yes. updated it.

[ilya-konanykhin]

I've opened #47087. Would love help confirming that fixes the issue

Fixes for me, thanks! Please don't forget to port to 6.1 branch as well.

[jhawthorn]

Rails 7.0.4.2 and 6.1.7.2 have been released with a fix for this issue