From 1f136a84a60ea11218cd034e50269a1d35d5a2e9 Mon Sep 17 00:00:00 2001
From: Tom Hughes <tom@compton.nu>
Date: Wed, 10 Feb 2021 19:37:51 +0000
Subject: [PATCH] Prevent CSRF bypass with login form

---
 app/controllers/users_controller.rb       |  2 +-
 test/controllers/users_controller_test.rb | 19 +++++++++++++++++++
 test/integration/oauth_test.rb            | 12 ++++++++----
 test/integration/page_locale_test.rb      |  4 ++++
 test/test_helper.rb                       |  1 +
 5 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index c2cbca4ae..aa504e313 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -276,7 +276,7 @@ class UsersController < ApplicationController
 
     session[:referer] = safe_referer(params[:referer]) if params[:referer]
 
-    if params[:username].present? && params[:password].present?
+    if request.post?
       session[:remember_me] ||= params[:remember_me]
       password_authentication(params[:username], params[:password])
     end
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index ff75df548..54c737b97 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -406,6 +406,25 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     ActionMailer::Base.deliveries.clear
   end
 
+  def test_login
+    user = create(:user)
+
+    get login_path
+    assert_response :redirect
+    assert_redirected_to login_path(:cookie_test => true)
+    follow_redirect!
+    assert_response :success
+    assert_template "login"
+
+    get login_path, :params => { :username => user.display_name, :password => "test" }
+    assert_response :success
+    assert_template "login"
+
+    post login_path, :params => { :username => user.display_name, :password => "test" }
+    assert_response :redirect
+    assert_redirected_to root_path
+  end
+
   def test_logout_without_referer
     post logout_path
     assert_response :redirect
diff --git a/test/integration/oauth_test.rb b/test/integration/oauth_test.rb
index 3242df9e2..1505cb34a 100644
--- a/test/integration/oauth_test.rb
+++ b/test/integration/oauth_test.rb
@@ -6,8 +6,9 @@ class OAuthTest < ActionDispatch::IntegrationTest
   def test_oauth10_web_app
     client = create(:client_application, :callback_url => "http://some.web.app.example.org/callback", :allow_read_prefs => true, :allow_write_api => true, :allow_read_gpx => true)
 
-    post "/login", :params => { :username => client.user.email, :password => "test" }
+    get "/login"
     follow_redirect!
+    post "/login", :params => { :username => client.user.email, :password => "test" }
     follow_redirect!
     assert_response :success
 
@@ -19,8 +20,9 @@ class OAuthTest < ActionDispatch::IntegrationTest
   def test_oauth10_desktop_app
     client = create(:client_application, :allow_read_prefs => true, :allow_write_api => true, :allow_read_gpx => true)
 
-    post "/login", :params => { :username => client.user.email, :password => "test" }
+    get "/login"
     follow_redirect!
+    post "/login", :params => { :username => client.user.email, :password => "test" }
     follow_redirect!
     assert_response :success
 
@@ -31,8 +33,9 @@ class OAuthTest < ActionDispatch::IntegrationTest
   def test_oauth10a_web_app
     client = create(:client_application, :callback_url => "http://some.web.app.example.org/callback", :allow_read_prefs => true, :allow_write_api => true, :allow_read_gpx => true)
 
-    post "/login", :params => { :username => client.user.email, :password => "test" }
+    get "/login"
     follow_redirect!
+    post "/login", :params => { :username => client.user.email, :password => "test" }
     follow_redirect!
     assert_response :success
 
@@ -44,8 +47,9 @@ class OAuthTest < ActionDispatch::IntegrationTest
   def test_oauth10a_desktop_app
     client = create(:client_application, :allow_read_prefs => true, :allow_write_api => true, :allow_read_gpx => true)
 
-    post "/login", :params => { :username => client.user.email, :password => "test" }
+    get "/login"
     follow_redirect!
+    post "/login", :params => { :username => client.user.email, :password => "test" }
     follow_redirect!
     assert_response :success
 
diff --git a/test/integration/page_locale_test.rb b/test/integration/page_locale_test.rb
index 2c4939b05..239c1d847 100644
--- a/test/integration/page_locale_test.rb
+++ b/test/integration/page_locale_test.rb
@@ -12,6 +12,8 @@ class PageLocaleTest < ActionDispatch::IntegrationTest
   def test_defaulting
     user = create(:user, :languages => [])
 
+    get "/login"
+    follow_redirect!
     post "/login", :params => { :username => user.email, :password => "test" }
     follow_redirect!
 
@@ -33,6 +35,8 @@ class PageLocaleTest < ActionDispatch::IntegrationTest
     get "/diary", :params => { :locale => "es" }
     assert_select "html[lang=?]", "es"
 
+    get "/login"
+    follow_redirect!
     post "/login", :params => { :username => user.email, :password => "test" }
     follow_redirect!
 
diff --git a/test/test_helper.rb b/test/test_helper.rb
index a023dd610..da36be28b 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -243,6 +243,7 @@ module ActiveSupport
     end
 
     def session_for(user)
+      get login_path
       post login_path, :params => { :username => user.display_name, :password => "test" }
       follow_redirect!
     end
-- 
2.39.5