diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index e5e4cad3c..07b654112 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -28,7 +28,7 @@ class Api::V1::AccountsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end diff --git a/app/controllers/api/v1/auth_controller.rb b/app/controllers/api/v1/auth_controller.rb index e522fb03f..c92e80334 100644 --- a/app/controllers/api/v1/auth_controller.rb +++ b/app/controllers/api/v1/auth_controller.rb @@ -45,23 +45,28 @@ module Api user.family = family user.role = User.role_for_new_family_creator - if user.save - # Claim invite code if provided - InviteCode.claim!(params[:invite_code]) if params[:invite_code].present? - - # Create device and OAuth token - begin + # Atomic: user creation, invite-code claim, and device/token issuance + # either all commit or none do. Without this, a post-commit device + # failure (e.g., racing uniqueness) would leave the user/invite/family + # committed while the client got a 422 "Failed to register device". + token_response = nil + begin + ActiveRecord::Base.transaction do + unless user.save + render json: { errors: user.errors.full_messages }, status: :unprocessable_entity + raise ActiveRecord::Rollback + end + InviteCode.claim!(params[:invite_code]) if params[:invite_code].present? device = MobileDevice.upsert_device!(user, device_params) token_response = device.issue_token! - rescue ActiveRecord::RecordInvalid => e - render json: { error: "Failed to register device: #{e.message}" }, status: :unprocessable_entity - return end - - render json: token_response.merge(user: mobile_user_payload(user)), status: :created - else - render json: { errors: user.errors.full_messages }, status: :unprocessable_entity + rescue ActiveRecord::RecordInvalid => e + Rails.logger.error("[Auth] Device registration failed: #{e.class} - #{e.message}") + render json: { error: "Failed to register device" }, status: :unprocessable_entity + return end + + render json: token_response.merge(user: mobile_user_payload(user)), status: :created if token_response end def login @@ -90,7 +95,8 @@ module Api device = MobileDevice.upsert_device!(user, device_params) token_response = device.issue_token! rescue ActiveRecord::RecordInvalid => e - render json: { error: "Failed to register device: #{e.message}" }, status: :unprocessable_entity + Rails.logger.error("[Auth] Device registration failed: #{e.message}") + render json: { error: "Failed to register device" }, status: :unprocessable_entity return end @@ -312,7 +318,20 @@ module Api return false if device.nil? required_fields = %w[device_id device_name device_type os_version app_version] - required_fields.all? { |field| device[field].present? } + return false unless required_fields.all? { |field| device[field].present? } + + # Run MobileDevice's attribute-level validations up front (e.g., + # device_type must be ios/android/web) so a misconfigured client + # is rejected BEFORE signup commits user/family/invite. Skip + # errors we can't evaluate without a user: the :user belongs_to + # presence check, and device_id uniqueness scoped to user_id + # (upsert_device! treats collisions as updates anyway). + preview = MobileDevice.new(device_params) + preview.valid? + relevant_errors = preview.errors.errors.reject do |err| + err.type == :taken || err.attribute == :user + end + relevant_errors.empty? end def device_params @@ -373,7 +392,8 @@ module Api render json: token_response.merge(user: mobile_user_payload(user)) rescue ActiveRecord::RecordInvalid => e - render json: { error: "Failed to register device: #{e.message}" }, status: :unprocessable_entity + Rails.logger.error("[Auth] Device registration failed: #{e.message}") + render json: { error: "Failed to register device" }, status: :unprocessable_entity end def ensure_write_scope diff --git a/app/controllers/api/v1/categories_controller.rb b/app/controllers/api/v1/categories_controller.rb index c810ffa25..fa30d53d4 100644 --- a/app/controllers/api/v1/categories_controller.rb +++ b/app/controllers/api/v1/categories_controller.rb @@ -29,7 +29,7 @@ class Api::V1::CategoriesController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -41,7 +41,7 @@ class Api::V1::CategoriesController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end diff --git a/app/controllers/api/v1/holdings_controller.rb b/app/controllers/api/v1/holdings_controller.rb index 58d094abd..7f08dfe3b 100644 --- a/app/controllers/api/v1/holdings_controller.rb +++ b/app/controllers/api/v1/holdings_controller.rb @@ -102,7 +102,7 @@ class Api::V1::HoldingsController < Api::V1::BaseController Rails.logger.error exception.backtrace.join("\n") render json: { error: "internal_server_error", - message: "Error: #{exception.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end end diff --git a/app/controllers/api/v1/sync_controller.rb b/app/controllers/api/v1/sync_controller.rb index b5f89fb60..add56fd44 100644 --- a/app/controllers/api/v1/sync_controller.rb +++ b/app/controllers/api/v1/sync_controller.rb @@ -21,7 +21,7 @@ class Api::V1::SyncController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end diff --git a/app/controllers/api/v1/trades_controller.rb b/app/controllers/api/v1/trades_controller.rb index 8f442c81a..0538fe321 100644 --- a/app/controllers/api/v1/trades_controller.rb +++ b/app/controllers/api/v1/trades_controller.rb @@ -292,7 +292,7 @@ class Api::V1::TradesController < Api::V1::BaseController Rails.logger.error exception.backtrace.join("\n") render json: { error: "internal_server_error", - message: "Error: #{exception.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end diff --git a/app/controllers/api/v1/transactions_controller.rb b/app/controllers/api/v1/transactions_controller.rb index b5942b7fc..c3538c79e 100644 --- a/app/controllers/api/v1/transactions_controller.rb +++ b/app/controllers/api/v1/transactions_controller.rb @@ -47,7 +47,7 @@ class Api::V1::TransactionsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -61,7 +61,7 @@ class Api::V1::TransactionsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -102,7 +102,7 @@ class Api::V1::TransactionsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -148,7 +148,7 @@ end render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -171,7 +171,7 @@ end render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end diff --git a/app/controllers/api/v1/valuations_controller.rb b/app/controllers/api/v1/valuations_controller.rb index 633a90461..495259bf5 100644 --- a/app/controllers/api/v1/valuations_controller.rb +++ b/app/controllers/api/v1/valuations_controller.rb @@ -13,7 +13,7 @@ class Api::V1::ValuationsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -100,7 +100,7 @@ class Api::V1::ValuationsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end @@ -181,7 +181,7 @@ class Api::V1::ValuationsController < Api::V1::BaseController render json: { error: "internal_server_error", - message: "Error: #{e.message}" + message: "An unexpected error occurred" }, status: :internal_server_error end diff --git a/app/controllers/settings/providers_controller.rb b/app/controllers/settings/providers_controller.rb index ed32b8893..f95e94a2b 100644 --- a/app/controllers/settings/providers_controller.rb +++ b/app/controllers/settings/providers_controller.rb @@ -71,8 +71,8 @@ class Settings::ProvidersController < ApplicationController redirect_to settings_providers_path, notice: "No changes were made" end rescue => error - Rails.logger.error("Failed to update provider settings: #{error.message}") - flash.now[:alert] = "Failed to update provider settings: #{error.message}" + Rails.logger.error("Failed to update provider settings: #{error.class} - #{error.message}") + flash.now[:alert] = "Failed to update provider settings. Please try again." prepare_show_context render :show, status: :unprocessable_entity end diff --git a/app/controllers/webhooks_controller.rb b/app/controllers/webhooks_controller.rb index 54cf8331f..e50325dcd 100644 --- a/app/controllers/webhooks_controller.rb +++ b/app/controllers/webhooks_controller.rb @@ -15,7 +15,8 @@ class WebhooksController < ApplicationController render json: { received: true }, status: :ok rescue => error Sentry.capture_exception(error) - render json: { error: "Invalid webhook: #{error.message}" }, status: :bad_request + Rails.logger.error("Webhook error: #{error.class} - #{error.message}") + render json: { error: "Invalid webhook" }, status: :bad_request end def plaid_eu @@ -31,7 +32,8 @@ class WebhooksController < ApplicationController render json: { received: true }, status: :ok rescue => error Sentry.capture_exception(error) - render json: { error: "Invalid webhook: #{error.message}" }, status: :bad_request + Rails.logger.error("Webhook error: #{error.class} - #{error.message}") + render json: { error: "Invalid webhook" }, status: :bad_request end def stripe diff --git a/test/controllers/api/v1/auth_controller_test.rb b/test/controllers/api/v1/auth_controller_test.rb index 52207df3a..9760c22dc 100644 --- a/test/controllers/api/v1/auth_controller_test.rb +++ b/test/controllers/api/v1/auth_controller_test.rb @@ -94,6 +94,25 @@ class Api::V1::AuthControllerTest < ActionDispatch::IntegrationTest assert_equal "Device information is required", response_data["error"] end + test "should reject signup with invalid device_type before committing any state" do + # Pre-validation catches bad device_type and returns 400 without creating + # user/family/device/token. Guards against a partial-commit state where the + # account exists but the mobile session handoff fails. + assert_no_difference([ "User.count", "MobileDevice.count", "Doorkeeper::AccessToken.count" ]) do + post "/api/v1/auth/signup", params: { + user: { + email: "newuser@example.com", + password: "SecurePass123!", + first_name: "New", + last_name: "User" + }, + device: @device_info.merge(device_type: "windows") # not in allowlist + } + end + + assert_response :bad_request + end + test "should not signup with invalid password" do assert_no_difference("User.count") do post "/api/v1/auth/signup", params: { diff --git a/test/controllers/settings/providers_controller_test.rb b/test/controllers/settings/providers_controller_test.rb index 63ac52888..a7358e06e 100644 --- a/test/controllers/settings/providers_controller_test.rb +++ b/test/controllers/settings/providers_controller_test.rb @@ -270,16 +270,17 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest # We'll force an error by making the []= method raise Setting.expects(:[]=).with("plaid_client_id", "test").raises(StandardError.new("Database error")).once - # Mock logger to verify error is logged - Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings.*Database error/)).once + # Mock logger to verify error is logged (pin both the exception class + # name and the message so a regression that drops one still fails). + Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings: StandardError - Database error/)).once patch settings_providers_url, params: { setting: { plaid_client_id: "test" } } - # Controller should handle the error gracefully + # Controller should handle the error gracefully with generic message (no internal details) assert_response :unprocessable_entity - assert_equal "Failed to update provider settings: Database error", flash[:alert] + assert_equal "Failed to update provider settings. Please try again.", flash[:alert] end end