From 68c56c7b8714918e9ad0d8201c532c803f90016b Mon Sep 17 00:00:00 2001
From: Ryan Lue <rl@zammad.com>
Date: Tue, 30 Jul 2019 15:43:27 +0200
Subject: [PATCH] Maintenance: Added support for Cookie secure-Flag.

---
 config/initializers/session_store.rb          | 12 ++------
 ...20190718140450_forget_insecure_sessions.rb | 12 ++++++++
 .../application/initializer/session_store.rb  | 27 +++++++++++++++++
 .../migrate/forget_insecure_sessions_spec.rb  | 28 +++++++++++++++++
 .../initializer/session_store_spec.rb         | 30 +++++++++++++++++++
 5 files changed, 100 insertions(+), 9 deletions(-)
 create mode 100644 db/migrate/20190718140450_forget_insecure_sessions.rb
 create mode 100644 lib/zammad/application/initializer/session_store.rb
 create mode 100644 spec/db/migrate/forget_insecure_sessions_spec.rb
 create mode 100644 spec/lib/zammad/application/initializer/session_store_spec.rb

diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb
index 28ce12b26..a92c2bd77 100644
--- a/config/initializers/session_store.rb
+++ b/config/initializers/session_store.rb
@@ -1,10 +1,4 @@
-# Be sure to restart your server when you modify this file.
+# Rails' constant auto-loading resolves to 'rails/initializable' instead
+require_dependency 'zammad/application/initializer/session_store'
 
-#Rails.application.config.session_store :cookie_store, key: '_zammad_session'
-
-# Use the database for sessions instead of the cookie-based default,
-# which shouldn't be used to store highly confidential information
-# (create the session table with "rails generate session_migration")
-Rails.application.config.session_store :active_record_store, {
-  key: '_zammad_session_' + Digest::MD5.hexdigest(Rails.root.to_s).to_s[5..15]
-}
+Zammad::Application::Initializer::SessionStore.perform
diff --git a/db/migrate/20190718140450_forget_insecure_sessions.rb b/db/migrate/20190718140450_forget_insecure_sessions.rb
new file mode 100644
index 000000000..14540120f
--- /dev/null
+++ b/db/migrate/20190718140450_forget_insecure_sessions.rb
@@ -0,0 +1,12 @@
+# This migration removes all pre-existing user sessions
+# so that they can be replaced with sessions that use "secure cookies".
+# It is skipped on non-HTTPS deployments
+# because those are incompatible with secure cookies anyway.
+class ForgetInsecureSessions < ActiveRecord::Migration[5.2]
+  def up
+    return if !Setting.find_by(name: 'system_init_done')
+    return if Setting.get('http_type') != 'https'
+
+    ActiveRecord::SessionStore::Session.destroy_all
+  end
+end
diff --git a/lib/zammad/application/initializer/session_store.rb b/lib/zammad/application/initializer/session_store.rb
new file mode 100644
index 000000000..eb7b1f877
--- /dev/null
+++ b/lib/zammad/application/initializer/session_store.rb
@@ -0,0 +1,27 @@
+# Use the database for sessions instead of the cookie-based default,
+# which shouldn't be used to store highly confidential information
+# (create the session table with "rails generate session_migration")
+
+module Zammad
+  class Application
+    class Initializer
+      module SessionStore
+        STORE_TYPE  = :active_record_store  # default: :cookie_store
+        SESSION_KEY = ('_zammad_session_' + Digest::MD5.hexdigest(Rails.root.to_s)[5..15]).freeze  # default: '_zammad_session'
+
+        def self.perform
+          Rails.application.config.session_store STORE_TYPE,
+                                                 key:    SESSION_KEY,
+                                                 secure: secure?
+        end
+
+        def self.secure?
+          Setting.get('http_type') == 'https'
+        rescue ActiveRecord::StatementInvalid
+          false
+        end
+        private_class_method :secure?
+      end
+    end
+  end
+end
diff --git a/spec/db/migrate/forget_insecure_sessions_spec.rb b/spec/db/migrate/forget_insecure_sessions_spec.rb
new file mode 100644
index 000000000..4276ff30f
--- /dev/null
+++ b/spec/db/migrate/forget_insecure_sessions_spec.rb
@@ -0,0 +1,28 @@
+require 'rails_helper'
+
+RSpec.describe ForgetInsecureSessions, type: :db_migration do
+  before do
+    5.times do
+      ActiveRecord::SessionStore::Session.create(
+        session_id: SecureRandom.hex(16),
+        data:       SecureRandom.base64(10)
+      )
+    end
+  end
+
+  context 'for HTTP deployment' do
+    before { Setting.set('http_type', 'http') }
+
+    it 'does not delete existing sessions' do
+      expect { migrate }.not_to change(ActiveRecord::SessionStore::Session, :count)
+    end
+  end
+
+  context 'for HTTPS deployment' do
+    before { Setting.set('http_type', 'https') }
+
+    it 'deletes all existing sessions' do
+      expect { migrate }.to change(ActiveRecord::SessionStore::Session, :count).to(0)
+    end
+  end
+end
diff --git a/spec/lib/zammad/application/initializer/session_store_spec.rb b/spec/lib/zammad/application/initializer/session_store_spec.rb
new file mode 100644
index 000000000..64862f7bf
--- /dev/null
+++ b/spec/lib/zammad/application/initializer/session_store_spec.rb
@@ -0,0 +1,30 @@
+require 'rails_helper'
+require_dependency 'zammad/application/initializer/session_store'
+
+RSpec.describe Zammad::Application::Initializer::SessionStore do
+  describe '.perform' do
+    context 'for HTTP deployment' do
+      before { Setting.set('http_type', 'http') }
+
+      # Why not use the "change" matcher in this example?
+      #
+      # This initializer is already run when the application is loaded for testing.
+      # Since the test env always uses http, the :secure option is already set to false.
+      it 'adds { secure: false } to application session options' do
+        described_class.perform
+
+        expect(Rails.application.config.session_options).to include(secure: false)
+      end
+    end
+
+    context 'for HTTPS deployment' do
+      before { Setting.set('http_type', 'https') }
+
+      it 'adds { secure: true } to application session options' do
+        expect { described_class.perform }
+          .to change(Rails.application.config, :session_options)
+          .to include(secure: true)
+      end
+    end
+  end
+end