diff --git a/app/controllers/transactions_controller.rb b/app/controllers/transactions_controller.rb index 23fdf90bf..6b2255e5b 100644 --- a/app/controllers/transactions_controller.rb +++ b/app/controllers/transactions_controller.rb @@ -5,6 +5,8 @@ class TransactionsController < ApplicationController before_action :set_entry_for_tags, only: :update_tags before_action :store_params!, only: :index + helper_method :new_transaction_idempotency_key + def show super assign_mark_recurring_state @@ -134,7 +136,19 @@ class TransactionsController < ApplicationController return unless require_account_permission!(account) - @entry = account.entries.new(entry_params) + idempotency_key = submitted_idempotency_key + + # Sequential double-submit guard: the form was already submitted + # successfully once (double-click, browser retry, user reopening the + # dialog after a slow response) and the first request already committed + # by the time this one runs. Treat it as a success instead of creating a + # second, identical entry. + if idempotency_key && (existing_entry = find_duplicate_manual_entry(account, idempotency_key)) + respond_with_created_entry(existing_entry) + return + end + + @entry = account.entries.new(entry_params_with_idempotency_key(idempotency_key)) if @entry.save @entry.sync_account_later @@ -142,16 +156,22 @@ class TransactionsController < ApplicationController @entry.mark_user_modified! @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any? - flash[:notice] = t(".created") - - respond_to do |format| - format.html { redirect_back_or_to account_path(@entry.account) } - format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account)) } - end + respond_with_created_entry(@entry) else set_new_transaction_form_options render :new, status: :unprocessable_entity end + rescue ActiveRecord::RecordNotUnique + # Concurrent-request backstop: two near-simultaneous submissions both + # passed the pre-check above (neither saw the other's row yet) and both + # reached #save. The partial unique index on + # entries(account_id, idempotency_key) lets exactly one INSERT win; + # this rescues the loser and redirects it to the winning entry instead of + # creating a duplicate or surfacing a 500 to the user. + existing_entry = idempotency_key && find_duplicate_manual_entry(account, idempotency_key) + raise unless existing_entry + + respond_with_created_entry(existing_entry) end def update @@ -565,6 +585,50 @@ class TransactionsController < ApplicationController entry_params end + def entry_params_with_idempotency_key(idempotency_key) + return entry_params unless idempotency_key + + # A dedicated column, deliberately not external_id/source: those are + # provider-linkage fields (Entry#linked? = external_id.present?), and + # reusing them here would make a manual entry look provider-synced - + # disabling its date/nature/amount/currency fields in the editor, and + # hiding it from future provider dedup matching. + entry_params.merge(idempotency_key: idempotency_key) + end + + def find_duplicate_manual_entry(account, idempotency_key) + account.entries.find_by(idempotency_key: idempotency_key) + end + + # The hidden "entry[idempotency_key]" field is rendered fresh (a random + # UUID) every time the new-transaction form loads, and echoed back + # unchanged by the browser on submit. It's never trusted for anything but + # de-duplication scoped to the current user's own account (see #create), + # so we only require that it looks like a UUID we could have generated - + # anything else (missing field, tampered value, non-string type from a + # malformed request) just disables the idempotency check for that + # request rather than being treated as an error. + UUID_FORMAT = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i + private_constant :UUID_FORMAT + + def submitted_idempotency_key + key = params.dig(:entry, :idempotency_key) + key if key.is_a?(String) && key.match?(UUID_FORMAT) + end + + def new_transaction_idempotency_key + @new_transaction_idempotency_key ||= submitted_idempotency_key || SecureRandom.uuid + end + + def respond_with_created_entry(entry) + flash[:notice] = t(".created") + + respond_to do |format| + format.html { redirect_back_or_to account_path(entry.account) } + format.turbo_stream { stream_redirect_back_or_to(account_path(entry.account)) } + end + end + def tag_ids_param Array(params[:tag_ids]).reject(&:blank?) end diff --git a/app/javascript/controllers/transaction_form_controller.js b/app/javascript/controllers/transaction_form_controller.js index 739b923da..48de7dd71 100644 --- a/app/javascript/controllers/transaction_form_controller.js +++ b/app/javascript/controllers/transaction_form_controller.js @@ -5,9 +5,59 @@ export default class extends ExchangeRateFormController { static targets = [ ...ExchangeRateFormController.targets, "account", - "currency" + "currency", + "idempotencyKey" ]; + // Two independent restoration paths can hand a user this exact page - and + // its hidden idempotency field - back without a server round trip: Turbo's + // own snapshot cache (back button within the app, wired via + // turbo:before-cache) and the browser's native bfcache (back/forward + // across a full navigation, or a duplicated tab, wired via a persisted + // pageshow). Either one skips the "new" action's SecureRandom.uuid, so if + // the original submission already committed, replaying that token on a + // *different*, edited submission would silently redirect onto the stale + // entry instead of creating the new one. Rotating on both events - rather + // than only one - ensures any later restore starts from a fresh, + // unconsumed token regardless of which cache served the page. + refreshIdempotencyKey() { + if (this.hasIdempotencyKeyTarget) { + this.idempotencyKeyTarget.value = this.#generateUUID(); + } + } + + refreshIdempotencyKeyIfPersisted(event) { + if (event.persisted) { + this.refreshIdempotencyKey(); + } + } + + // crypto.randomUUID() only exists in secure contexts (HTTPS/localhost), + // but self-hosted deployments of this app are commonly reverse-proxied or + // reached over plain HTTP on a LAN, where it's undefined and would throw + // from inside the cache-restore handlers above - leaving the stale, + // already-consumed token in place. crypto.getRandomValues has no such + // restriction, so build a v4 UUID manually when randomUUID is missing; + // the server's UUID_FORMAT check requires this exact shape. + #generateUUID() { + if (crypto.randomUUID) { + return crypto.randomUUID(); + } + + const bytes = crypto.getRandomValues(new Uint8Array(16)); + bytes[6] = (bytes[6] & 0x0f) | 0x40; + bytes[8] = (bytes[8] & 0x3f) | 0x80; + const hex = [ ...bytes ].map((byte) => byte.toString(16).padStart(2, "0")); + + return [ + hex.slice(0, 4).join(""), + hex.slice(4, 6).join(""), + hex.slice(6, 8).join(""), + hex.slice(8, 10).join(""), + hex.slice(10, 16).join("") + ].join("-"); + } + hasRequiredExchangeRateTargets() { if (!this.hasAccountTarget || !this.hasCurrencyTarget || !this.hasDateTarget) { return false; diff --git a/app/views/transactions/_form.html.erb b/app/views/transactions/_form.html.erb index ec068b645..6f58e19f2 100644 --- a/app/views/transactions/_form.html.erb +++ b/app/views/transactions/_form.html.erb @@ -1,6 +1,6 @@ <%# locals: (entry:, account_currencies:, manual_accounts:, categories:, merchants:, tags:) %> -<%= styled_form_with model: entry, url: transactions_path, class: "space-y-4", data: { controller: "transaction-form", transaction_form_exchange_rate_url_value: exchange_rate_path, transaction_form_account_currencies_value: account_currencies.to_json } do |f| %> +<%= styled_form_with model: entry, url: transactions_path, class: "space-y-4", data: { controller: "transaction-form", action: "turbo:before-cache@document->transaction-form#refreshIdempotencyKey pageshow@window->transaction-form#refreshIdempotencyKeyIfPersisted", transaction_form_exchange_rate_url_value: exchange_rate_path, transaction_form_account_currencies_value: account_currencies.to_json } do |f| %> <% if entry.errors.any? %> <%= render "shared/form_errors", model: entry %> <% end %> @@ -10,6 +10,17 @@ <%= f.hidden_field :nature, value: params[:nature] || "outflow", data: { "transaction-type-tabs-target": "natureField" } %> <%= f.hidden_field :entryable_type, value: "Transaction" %> + <%# Anti-double-submit token: a fresh UUID per page load, echoed back on + submit. TransactionsController#create uses it to recognize a repeat + submission (double-click, retry, or a genuine concurrent race) and + avoid creating a duplicate transaction. + transaction_form_controller#refreshIdempotencyKey replaces this value + on turbo:before-cache (Turbo's own snapshot cache) and on a + persisted pageshow (the browser's native bfcache), so a page + restored via either path - browser back, duplicated tab - can't + replay a token that already committed an entry and silently + redirect a distinct submission onto that old one. %> + <%= hidden_field_tag "entry[idempotency_key]", new_transaction_idempotency_key, data: { "transaction-form-target": "idempotencyKey" } %>
diff --git a/db/migrate/20260902180400_add_idempotency_key_to_entries.rb b/db/migrate/20260902180400_add_idempotency_key_to_entries.rb new file mode 100644 index 000000000..dd25a8fe3 --- /dev/null +++ b/db/migrate/20260902180400_add_idempotency_key_to_entries.rb @@ -0,0 +1,46 @@ +class AddIdempotencyKeyToEntries < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + INDEX_NAME = "index_entries_on_account_and_idempotency_key" + + def up + add_column :entries, :idempotency_key, :string unless column_exists?(:entries, :idempotency_key) + + # index_exists? alone isn't enough: CREATE INDEX CONCURRENTLY leaves an + # INVALID index behind if it's interrupted (e.g. a deploy killed + # mid-build), and index_exists? still reports that catalog entry as + # present - short-circuiting here would record this migration as + # applied while the constraint is actually missing/broken. + return if valid_index_exists? + + # Deliberately a separate column from external_id/source: those two are + # provider-linkage fields (Entry#linked? = external_id.present?), and + # reusing them for a web-form anti-double-submit token would make a + # perfectly ordinary manual entry look provider-synced - disabling its + # date/nature/amount/currency fields in the editor, and making it + # invisible to future provider dedup matching (which filters to + # external_id: nil). idempotency_key has no such meaning anywhere else, + # so it's safe to set on manual entries without side effects. + execute "DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME}" + + add_index :entries, [ :account_id, :idempotency_key ], + unique: true, + where: "(idempotency_key IS NOT NULL)", + name: INDEX_NAME, + algorithm: :concurrently + end + + def down + remove_index :entries, name: INDEX_NAME, if_exists: true, algorithm: :concurrently + remove_column :entries, :idempotency_key, if_exists: true + end + + private + def valid_index_exists? + select_value(<<~SQL.squish) == true + SELECT indisvalid FROM pg_index + JOIN pg_class ON pg_class.oid = pg_index.indexrelid + WHERE pg_class.relname = '#{INDEX_NAME}' + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index 20971ebc7..a7283fa73 100644 --- a/db/schema.rb +++ b/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_08_26_090000) do +ActiveRecord::Schema[8.1].define(version: 2026_09_02_180400) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -649,6 +649,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_08_26_090000) do t.string "entryable_type" t.boolean "excluded", default: false t.string "external_id" + t.string "idempotency_key" t.uuid "import_id" t.boolean "import_locked", default: false, null: false t.jsonb "locked_attributes", default: {} @@ -664,6 +665,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_08_26_090000) do t.index "lower((name)::text)", name: "index_entries_on_lower_name" t.index ["account_id", "date", "entryable_id"], name: "index_entries_on_investment_totals_lookup", where: "(((entryable_type)::text = 'Trade'::text) AND (excluded = false))" t.index ["account_id", "date"], name: "index_entries_on_account_id_and_date" + t.index ["account_id", "idempotency_key"], name: "index_entries_on_account_and_idempotency_key", unique: true, where: "(idempotency_key IS NOT NULL)" t.index ["account_id", "reconciled_at"], name: "index_entries_on_account_and_reconciled_at", where: "(reconciled_at IS NOT NULL)" t.index ["account_id", "source", "external_id"], name: "index_entries_on_account_source_and_external_id", unique: true, where: "((external_id IS NOT NULL) AND (source IS NOT NULL))" t.index ["account_id"], name: "index_entries_on_account_id" diff --git a/test/controllers/transactions_controller_test.rb b/test/controllers/transactions_controller_test.rb index 23f7dbddb..570ea1c65 100644 --- a/test/controllers/transactions_controller_test.rb +++ b/test/controllers/transactions_controller_test.rb @@ -98,6 +98,151 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest assert_enqueued_with(job: SyncJob) end + test "resubmitting the same idempotency key does not create a duplicate transaction" do + idempotency_key = SecureRandom.uuid + params = { + entry: { + account_id: @entry.account_id, + name: "New transaction", + date: Date.current, + currency: "USD", + amount: 100, + nature: "inflow", + entryable_type: @entry.entryable_type, + entryable_attributes: { category_id: Category.first.id }, + idempotency_key: idempotency_key + } + } + + assert_difference [ "Entry.count", "Transaction.count" ], 1 do + post transactions_url, params: params + end + assert_response :redirect + first_entry = Entry.order(:created_at).last + + # Simulates a double-click or a browser retry: same form, same + # idempotency key, submitted again after the first request already + # completed and committed. + assert_no_difference [ "Entry.count", "Transaction.count" ] do + post transactions_url, params: params + end + assert_response :redirect + assert_equal "Transaction created", flash[:notice] + assert_redirected_to account_url(first_entry.account) + end + + test "the idempotency key does not mark the created transaction as provider-linked" do + # Regression test: the idempotency key must not be stored in + # external_id/source (Entry#linked? = external_id.present?), or a plain + # manual entry would incorrectly look provider-synced - disabling its + # editable fields in the UI and hiding it from future provider dedup. + post transactions_url, params: { + entry: { + account_id: @entry.account_id, + name: "New transaction", + date: Date.current, + currency: "USD", + amount: 100, + nature: "inflow", + entryable_type: @entry.entryable_type, + entryable_attributes: { category_id: Category.first.id }, + idempotency_key: SecureRandom.uuid + } + } + + created_entry = Entry.order(:created_at).last + assert_not created_entry.linked? + assert_nil created_entry.external_id + assert_nil created_entry.source + end + + test "handles a genuine concurrent double-submit without raising or duplicating" do + idempotency_key = SecureRandom.uuid + + # Simulates the race: another request with the same idempotency key wins + # and commits its INSERT in the window between our pre-check (which + # therefore still sees nothing, hence the first `nil`) and our own + # #save (which then hits the real partial unique index on + # entries(account_id, idempotency_key) and raises RecordNotUnique, + # exactly like the DB would under real concurrent requests). The rescue + # then re-runs the same lookup, this time finding the winner. + winning_entry = @entry.account.entries.create!( + name: "New transaction", date: Date.current, currency: "USD", amount: 100, + idempotency_key: idempotency_key, + entryable: Transaction.new + ) + TransactionsController.any_instance.stubs(:find_duplicate_manual_entry).returns(nil, winning_entry) + Entry.any_instance.stubs(:save).raises(ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint")) + + assert_no_difference [ "Entry.count", "Transaction.count" ] do + post transactions_url, params: { + entry: { + account_id: @entry.account_id, + name: "New transaction", + date: Date.current, + currency: "USD", + amount: 100, + nature: "inflow", + entryable_type: "Transaction", + idempotency_key: idempotency_key + } + } + end + + assert_response :redirect + assert_redirected_to account_url(winning_entry.account) + assert_equal "Transaction created", flash[:notice] + end + + test "a RecordNotUnique with no matching entry is not silently swallowed" do + idempotency_key = SecureRandom.uuid + + # Defensive-branch coverage: if the unique index ever rejects an insert + # for a reason other than "another request with this exact idempotency + # key already won" (e.g. a different constraint), we must not pretend it + # succeeded - the error should propagate instead of being hidden behind + # a fake success redirect. + TransactionsController.any_instance.stubs(:find_duplicate_manual_entry).returns(nil) + Entry.any_instance.stubs(:save).raises(ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint")) + + assert_raises(ActiveRecord::RecordNotUnique) do + post transactions_url, params: { + entry: { + account_id: @entry.account_id, + name: "New transaction", + date: Date.current, + currency: "USD", + amount: 100, + nature: "inflow", + entryable_type: "Transaction", + idempotency_key: idempotency_key + } + } + end + end + + test "create without an idempotency key still creates a transaction as before" do + # A raw POST that doesn't go through the rendered form (e.g. a script) + # simply skips the idempotency check rather than being rejected - the + # form always supplies a key in normal browser usage. + assert_difference [ "Entry.count", "Transaction.count" ], 2 do + 2.times do + post transactions_url, params: { + entry: { + account_id: @entry.account_id, + name: "New transaction", + date: Date.current, + currency: "USD", + amount: 100, + nature: "inflow", + entryable_type: "Transaction", + entryable_attributes: { category_id: Category.first.id } + } + } + end + end + end + test "create without an account re-renders the form instead of raising" do assert_no_difference [ "Entry.count", "Transaction.count" ] do post transactions_url, params: {