From 6e5f35b3069bec786e15b9fefe846ced02cbb6de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orange=F0=9F=8D=8A?= Date: Fri, 19 Jun 2026 23:14:43 +0800 Subject: [PATCH] fix(api): prevent API auth from inheriting impersonation (#2405) Build fresh API session contexts instead of reusing persisted web sessions that may carry impersonation state. Reject deactivated report export API key owners and strengthen regression coverage for API key, OAuth, and report export authentication paths. --- app/controllers/api/v1/base_controller.rb | 26 +++---- app/controllers/reports_controller.rb | 14 ++-- .../api/v1/chats_controller_test.rb | 62 ++++++++++++++++ test/controllers/reports_controller_test.rb | 73 +++++++++++++++++++ 4 files changed, 153 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index f1631ec1c..f69e6f4be 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -310,23 +310,15 @@ class Api::V1::BaseController < ApplicationController # Set up Current context for API requests since we don't use session-based auth def setup_current_context_for_api - # For API requests, we need to create a minimal session-like object - # or find/create an actual session for this user to make Current.user work - if @current_user - # Try to find an existing session for this user, or create a temporary one - session = @current_user.sessions.first - if session - Current.session = session - else - # Create a temporary session for this API request - # This won't be persisted but will allow Current.user to work - session = @current_user.sessions.build( - user_agent: request.user_agent, - ip_address: request.ip - ) - Current.session = session - end - end + return unless @current_user + + # Build a fresh unsaved session so API requests never reuse an existing + # web session that may already carry impersonation state. + Current.session = @current_user.sessions.build( + user_agent: request.user_agent, + ip_address: request.ip + ) + Current.session.active_impersonator_session = nil end # Check if AI features are enabled for the current user diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index d1b76204e..f1159b28c 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -1060,14 +1060,18 @@ class ReportsController < ApplicationController return false end - # Find or create a session for this API request - # We need to find or create a persisted session so that Current.user delegation works properly - session = @current_user.sessions.first_or_create!( + unless @current_user.active? + render plain: "Invalid or expired API key", status: :unauthorized + return false + end + + # Build a fresh unsaved session so API key exports never reuse an existing + # web session that may already carry impersonation state. + Current.session = @current_user.sessions.build( user_agent: request.user_agent, ip_address: request.ip ) - - Current.session = session + Current.session.active_impersonator_session = nil # Verify the delegation chain works unless Current.user diff --git a/test/controllers/api/v1/chats_controller_test.rb b/test/controllers/api/v1/chats_controller_test.rb index f56ed3536..c7dbcdf76 100644 --- a/test/controllers/api/v1/chats_controller_test.rb +++ b/test/controllers/api/v1/chats_controller_test.rb @@ -125,6 +125,68 @@ class Api::V1::ChatsControllerTest < ActionDispatch::IntegrationTest assert_response :success end + test "API key authentication should not inherit web session impersonation" do + support_user = users(:sure_support_staff) + support_user.update!(ai_enabled: true) + support_user.api_keys.active.destroy_all + support_chat = support_user.chats.create!(title: "Support Chat") + + token_value = ApiKey.generate_secure_key + credential = support_user.api_keys.build( + name: "Impersonation Guard Credential", + scopes: [ "read" ] + ) + credential.key = token_value + credential.save! + + impersonation_session = impersonation_sessions(:in_progress) + impersonated_chat = impersonation_session.impersonated.chats.create!(title: "Impersonated Chat") + support_user.sessions.destroy_all + support_user.sessions.create!( + user_agent: "Browser session", + ip_address: "127.0.0.1", + active_impersonator_session: impersonation_session + ) + + get "/api/v1/chats", headers: { "X-Api-Key" => token_value } + + assert_response :success + response_body = JSON.parse(response.body) + chat_ids = response_body["chats"].pluck("id") + + assert_includes chat_ids, support_chat.id + assert_not_includes chat_ids, impersonated_chat.id + end + + test "OAuth authentication should not inherit web session impersonation" do + support_user = users(:sure_support_staff) + support_user.update!(ai_enabled: true) + support_chat = support_user.chats.create!(title: "Support Chat") + support_token = Doorkeeper::AccessToken.create!( + application: @oauth_app, + resource_owner_id: support_user.id, + scopes: "read" + ) + + impersonation_session = impersonation_sessions(:in_progress) + impersonated_chat = impersonation_session.impersonated.chats.create!(title: "Impersonated Chat") + support_user.sessions.destroy_all + support_user.sessions.create!( + user_agent: "Browser session", + ip_address: "127.0.0.1", + active_impersonator_session: impersonation_session + ) + + get "/api/v1/chats", headers: bearer_auth_header(support_token) + + assert_response :success + response_body = JSON.parse(response.body) + chat_ids = response_body["chats"].pluck("id") + + assert_includes chat_ids, support_chat.id + assert_not_includes chat_ids, impersonated_chat.id + end + private def bearer_auth_header(token) diff --git a/test/controllers/reports_controller_test.rb b/test/controllers/reports_controller_test.rb index 4c75de2c3..01baf8b4f 100644 --- a/test/controllers/reports_controller_test.rb +++ b/test/controllers/reports_controller_test.rb @@ -247,6 +247,79 @@ class ReportsControllerTest < ActionDispatch::IntegrationTest assert_match /Category/, @response.body end + test "export transactions with API key does not inherit web session impersonation" do + support_user = users(:sure_support_staff) + support_user.api_keys.active.destroy_all + token_value = ApiKey.generate_secure_key + export_credential = support_user.api_keys.build( + name: "Support Export Credential", + scopes: [ "read" ], + source: "web" + ) + export_credential.key = token_value + export_credential.save! + + impersonation_session = impersonation_sessions(:in_progress) + impersonated_user = impersonation_session.impersonated + leaked_category = impersonated_user.family.categories.create!( + name: "Impersonated Export Leak Probe" + ) + impersonated_account = impersonated_user.finance_accounts.first + assert_not_nil impersonated_account, "Test setup failed: impersonated user has no finance account" + + create_transaction( + account: impersonated_account, + name: "Impersonated export leak probe", + date: Date.current, + amount: 123.45, + category: leaked_category + ) + + support_user.sessions.destroy_all + support_user.sessions.create!( + user_agent: "Browser session", + ip_address: "127.0.0.1", + active_impersonator_session: impersonation_session + ) + + get export_transactions_reports_path( + format: :csv, + period_type: :ytd, + start_date: Date.current.beginning_of_year, + end_date: Date.current, + api_key: token_value + ) + + assert_response :ok + assert_equal "text/csv", @response.media_type + assert_no_match /Impersonated Export Leak Probe/, @response.body + end + + test "export transactions with API key rejects deactivated user" do + user = users(:family_admin) + user.api_keys.active.destroy_all + token_value = ApiKey.generate_secure_key + export_access = user.api_keys.build( + name: "Inactive User Export Credential", + scopes: [ "read" ], + source: "web" + ) + export_access.key = token_value + export_access.save! + user.update_column(:active, false) + + get export_transactions_reports_path( + format: :csv, + period_type: :ytd, + api_key: token_value + ) + + assert_response :unauthorized + assert_match /Invalid or expired API key/, @response.body + ensure + user&.update_column(:active, true) + end + test "export transactions with invalid API key" do get export_transactions_reports_path( format: :csv,