security: sanitize exception messages in v1 API responses (FIX-11) (#1521)

* fix(security): sanitize exception messages in API responses (FIX-11) Replace raw e.message/error.message interpolations in response bodies with generic error strings, and log class+message server-side. Prevents leaking internal exception details (stack traces, SQL fragments, record data) to API clients. Covers: - API v1 accounts, categories (index/show), holdings, sync, trades, transactions (index/show/create/update/destroy), valuations (show/create/update): replace "Error: #{e.message}" with "An unexpected error occurred". - API v1 auth: device-registration rescue paths now log "[Auth] Device registration failed: ..." and respond with "Failed to register device". - WebhooksController#plaid and #plaid_eu: log full error and respond with "Invalid webhook". - Settings::ProvidersController: generic user-facing flash alert, detailed log line with error class + message. Updates providers_controller_test assertion to match sanitized flash. * fix(security): address CodeRabbit review Major — partial-commit on device registration failure: - Strengthened valid_device_info? to also run MobileDevice's model validations up-front (device_type inclusion, attribute presence), not just a flat "are the keys present?" check. A client that sends a bad device_type ("windows", etc.) is now rejected at the API boundary BEFORE signup commits any user/family/invite state. - Wrapped the signup path (user.save + InviteCode.claim + MobileDevice upsert + token issuance) in ActiveRecord::Base.transaction. A post-save RecordInvalid from device registration (e.g., racing uniqueness on device_id) now rolls back the user/invite/family so clients don't see a partial-account state. - Rescue branch logs the exception class + message ("#{e.class} - #{e.message}") for better postmortem debugging, matching the providers controller pattern. Nit: - Tightened providers_controller_test log expectation regex to assert on both the exception class name AND the message ("StandardError - Database error"), so a regression that drops either still fails the test. Tests: - New: "should reject signup with invalid device_type before committing any state" — POST /api/v1/auth/signup with device_type="windows" returns 400 AND asserts no User, MobileDevice, or Doorkeeper::AccessToken row was created. Note on SSO path (sso_exchange → issue_mobile_tokens, lines 173/225): the device_info in those flows comes from Rails.cache (populated by an earlier request that already passed valid_device_info?), so the pre-validation covers it indirectly. Wrapping the full SSO account creation (user + invitation + OidcIdentity + issue_mobile_tokens) in one transaction would be a meaningful architectural cleanup but is out of scope for this error-hygiene PR — filed it as a mental note for a follow-up.
2026-04-25 17:15:07 +02:00 · 2026-04-19 18:38:23 +02:00
parent cb842d0d9b
commit 7f17fbf6da
12 changed files with 80 additions and 38 deletions
--- 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

--- 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
--- 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

--- 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
--- 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

--- 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

--- 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

--- 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

--- 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
--- 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
--- 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: {
--- 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