Fix redirects! And add tests! (#930)

* Fix redirects in new account flow?

* Fix redirects! And tests!

* Make tests better

* I will test this club

.github/workflows/ci.yml

@@ -115,7 +115,7 @@ jobs:
           PGPASSWORD: postgres
         run: |
           bin/rails db:create RAILS_ENV=test
-          bin/rails db:migrate RAILS_ENV=test
+          bin/rails db:schema:load RAILS_ENV=test
           # Create additional test databases
           psql -h localhost -U postgres -c "CREATE DATABASE test_wakatime;"
           psql -h localhost -U postgres -c "CREATE DATABASE test_sailors_log;"

app/controllers/application_controller.rb

@@ -54,9 +54,6 @@ class ApplicationController < ActionController::Base
     !!current_user
   end
 
-  # Validates a return URL is a safe relative path.
-  # Rejects absolute URLs, protocol-relative URLs, and javascript: URIs
-  # to prevent open redirect and XSS attacks.
   def safe_return_url(url)
     return nil if url.blank?
     return nil unless url.start_with?("/") && !url.start_with?("//")

app/controllers/sessions_controller.rb

@@ -37,6 +37,9 @@ class SessionsController < ApplicationController
         session[:return_data] = { "url" => safe_return_url(params[:continue].presence) }
         Rails.logger.info("Sessions return data: #{session[:return_data]}")
         redirect_to my_wakatime_setup_path, notice: "Successfully signed in with Hack Club Auth! Welcome!"
+      elsif session[:return_data]&.dig("url").present?
+        return_url = session[:return_data].delete("url")
+        redirect_to return_url, notice: "Successfully signed in with Hack Club Auth! Welcome!"
       else
         redirect_to root_path, notice: "Successfully signed in with Hack Club Auth! Welcome!"
       end
@@ -84,8 +87,8 @@ class SessionsController < ApplicationController
       elsif @user.created_at > 5.seconds.ago
         session[:return_data] = { "url" => safe_return_url(state["continue"].presence) }
         redirect_to my_wakatime_setup_path, notice: "Successfully signed in with Slack! Welcome!"
-      elsif state["continue"]
-        redirect_to state["continue"], notice: "Successfully signed in with Slack! Welcome!"
+      elsif state["continue"].present? && safe_return_url(state["continue"]).present?
+        redirect_to safe_return_url(state["continue"]), notice: "Successfully signed in with Slack! Welcome!"
       else
         redirect_to root_path, notice: "Successfully signed in with Slack! Welcome!"
       end
@@ -247,8 +250,8 @@ class SessionsController < ApplicationController
       session[:user_id] = valid_token.user_id
       session[:return_data] = valid_token.return_data || {}
 
-      if valid_token.continue_param.present?
-        redirect_to valid_token.continue_param, notice: "Successfully signed in!"
+      if valid_token.continue_param.present? && safe_return_url(valid_token.continue_param).present?
+        redirect_to safe_return_url(valid_token.continue_param), notice: "Successfully signed in!"
       else
         redirect_to root_path, notice: "Successfully signed in!"
       end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.1].define(version: 2026_02_04_113033) do
+ActiveRecord::Schema[8.1].define(version: 2026_02_10_094354) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
   enable_extension "pg_stat_statements"
@@ -329,8 +329,13 @@ ActiveRecord::Schema[8.1].define(version: 2026_02_04_113033) do
     t.index ["raw_heartbeat_upload_id"], name: "index_heartbeats_on_raw_heartbeat_upload_id"
     t.index ["source_type", "time", "user_id", "project"], name: "index_heartbeats_on_source_type_time_user_project"
     t.index ["user_agent_id"], name: "index_heartbeats_on_user_agent_id"
+    t.index ["user_id", "category", "time"], name: "idx_heartbeats_user_category_time", where: "(deleted_at IS NULL)"
+    t.index ["user_id", "editor", "time"], name: "idx_heartbeats_user_editor_time", where: "(deleted_at IS NULL)"
     t.index ["user_id", "id"], name: "index_heartbeats_on_user_id_with_ip", order: { id: :desc }, where: "((ip_address IS NOT NULL) AND (deleted_at IS NULL))"
+    t.index ["user_id", "language", "time"], name: "idx_heartbeats_user_language_time", where: "(deleted_at IS NULL)"
+    t.index ["user_id", "operating_system", "time"], name: "idx_heartbeats_user_operating_system_time", where: "(deleted_at IS NULL)"
     t.index ["user_id", "project", "time"], name: "idx_heartbeats_user_project_time_stats", where: "((deleted_at IS NULL) AND (project IS NOT NULL))"
+    t.index ["user_id", "project"], name: "index_heartbeats_on_user_id_and_project", where: "(deleted_at IS NULL)"
     t.index ["user_id", "time", "category"], name: "index_heartbeats_on_user_time_category"
     t.index ["user_id", "time", "language"], name: "idx_heartbeats_user_time_language_stats", where: "(deleted_at IS NULL)"
     t.index ["user_id", "time", "language_id"], name: "idx_heartbeats_user_time_language_id", where: "(deleted_at IS NULL)"

test/controllers/sessions_controller_test.rb

@@ -0,0 +1,173 @@
+require "test_helper"
+
+class SessionsControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    ActiveRecord::FixtureSet.reset_cache
+  end
+
+  # -- HCA: hca_new stores continue in session --
+
+  test "hca_new stores continue path for oauth authorize" do
+    continue_query = {
+      client_id: "Ck47_6hihaBqZO7z3CLmJlCB-0NzHtZHGeDBwG4CqRs",
+      redirect_uri: "https://game.hackclub.com/hackatime/callback",
+      response_type: "code",
+      scope: "profile read",
+      state: "a254695483383bd70ee41424b75d638a869e5d6769e11b50"
+    }
+    continue_path = "/oauth/authorize?#{Rack::Utils.build_query(continue_query)}"
+
+    get hca_auth_path(continue: continue_path)
+
+    assert_equal continue_path, session.dig(:return_data, "url")
+    assert_response :redirect
+    assert_redirected_to %r{/oauth/authorize}
+  end
+
+  test "hca_new rejects external continue URL" do
+    get hca_auth_path(continue: "https://evil.example.com/phish")
+
+    assert_nil session.dig(:return_data, "url")
+    assert_response :redirect
+    assert_redirected_to %r{/oauth/authorize}
+  end
+
+  test "hca_new rejects javascript continue URL" do
+    get hca_auth_path(continue: "javascript:alert(1)")
+
+    assert_nil session.dig(:return_data, "url")
+    assert_response :redirect
+    assert_redirected_to %r{/oauth/authorize}
+  end
+
+  test "hca_new rejects protocol-relative continue URL" do
+    get hca_auth_path(continue: "//evil.example.com/phish")
+
+    assert_nil session.dig(:return_data, "url")
+    assert_response :redirect
+    assert_redirected_to %r{/oauth/authorize}
+  end
+
+  # -- Minimal login: preserves continue param --
+
+  test "minimal_login renders with continue param available for auth links" do
+    oauth_path = "/oauth/authorize?client_id=test&response_type=code"
+
+    get minimal_login_path(continue: oauth_path)
+
+    assert_response :success
+    assert_select "a[href*='continue']", minimum: 1
+    assert_select "input[name='continue'][value=?]", oauth_path
+  end
+
+  test "minimal_login renders without continue param when not provided" do
+    get minimal_login_path
+
+    assert_response :success
+    assert_select "input[name='continue']", count: 0
+  end
+
+  # -- Email auth: persists continue into sign-in token --
+
+  test "email auth stores continue param in sign-in token" do
+    user = User.create!
+    user.email_addresses.create!(email: "test@example.com")
+
+    oauth_path = "/oauth/authorize?client_id=test&response_type=code"
+
+    # LoopsMailer forces SMTP delivery even in test; temporarily override
+    original_delivery_method = LoopsMailer.delivery_method
+    LoopsMailer.delivery_method = :test
+    post email_auth_path, params: { email: "test@example.com", continue: oauth_path }
+    LoopsMailer.delivery_method = original_delivery_method
+
+    assert_response :redirect
+
+    token = SignInToken.last
+    assert_not_nil token
+    assert_equal oauth_path, token.continue_param
+  end
+
+  test "email token redirects to continue param after sign in" do
+    user = User.create!
+    oauth_path = "/oauth/authorize?client_id=test&response_type=code"
+    sign_in_token = user.sign_in_tokens.create!(
+      auth_type: :email,
+      continue_param: oauth_path
+    )
+
+    get auth_token_path(token: sign_in_token.token)
+
+    assert_response :redirect
+    assert_redirected_to oauth_path
+    assert_equal user.id, session[:user_id]
+  end
+
+  test "email token falls back to root when no continue param" do
+    user = User.create!
+    sign_in_token = user.sign_in_tokens.create!(auth_type: :email)
+
+    get auth_token_path(token: sign_in_token.token)
+
+    assert_response :redirect
+    assert_redirected_to root_path
+    assert_equal user.id, session[:user_id]
+  end
+
+  test "email token rejects external continue URL" do
+    user = User.create!
+    sign_in_token = user.sign_in_tokens.create!(
+      auth_type: :email,
+      continue_param: "https://evil.example.com/phish"
+    )
+
+    get auth_token_path(token: sign_in_token.token)
+
+    assert_response :redirect
+    assert_redirected_to root_path
+    assert_equal user.id, session[:user_id]
+  end
+
+  test "email token rejects protocol-relative continue URL" do
+    user = User.create!
+    sign_in_token = user.sign_in_tokens.create!(
+      auth_type: :email,
+      continue_param: "//evil.example.com/phish"
+    )
+
+    get auth_token_path(token: sign_in_token.token)
+
+    assert_response :redirect
+    assert_redirected_to root_path
+  end
+
+  test "expired token redirects to root with alert" do
+    user = User.create!
+    sign_in_token = user.sign_in_tokens.create!(
+      auth_type: :email,
+      continue_param: "/oauth/authorize?client_id=test",
+      expires_at: 1.hour.ago
+    )
+
+    get auth_token_path(token: sign_in_token.token)
+
+    assert_response :redirect
+    assert_redirected_to root_path
+    assert_nil session[:user_id]
+  end
+
+  test "used token redirects to root with alert" do
+    user = User.create!
+    sign_in_token = user.sign_in_tokens.create!(
+      auth_type: :email,
+      continue_param: "/oauth/authorize?client_id=test",
+      used_at: 1.minute.ago
+    )
+
+    get auth_token_path(token: sign_in_token.token)
+
+    assert_response :redirect
+    assert_redirected_to root_path
+    assert_nil session[:user_id]
+  end
+end

test/fixtures/api_keys.yml

@@ -3,9 +3,9 @@
 one:
   user: one
   name: MyText
-  token: MyText
+  token: MyTextOne
 
 two:
   user: two
   name: MyText
-  token: MyText
+  token: MyTextTwo

test/test_helper.rb

@@ -10,6 +10,19 @@ module ActiveSupport
     # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order.
     fixtures :all
 
+    self.fixture_table_names -= [
+      "mailing_addresses",
+      "physical_mails",
+      "api_keys",
+      "heartbeats",
+      "project_repo_mappings",
+      "repositories",
+      "sailors_log_leaderboards",
+      "sailors_log_notification_preferences",
+      "sailors_log_slack_notifications",
+      "sailors_logs"
+    ]
+
     # Add more helper methods to be used by all tests here...
   end
 end