mirror of
https://github.com/we-promise/sure.git
synced 2026-09-08 16:14:23 +00:00
fix(transactions): prevent duplicate creation on double-submit (#3338)
* fix(transactions): prevent duplicate creation on double-submit TransactionsController#create had no protection against a repeated form submission - a double-click, a browser retry, or two near-simultaneous requests could all create a separate identical transaction. Adds a per-form idempotency key (a UUID hidden field, generated fresh on page load) that reuses the existing entries(account_id, source, external_id) partial unique index, with a pre-check for the sequential case and a RecordNotUnique rescue as the authoritative backstop for genuine concurrent requests - the same pattern already used by mark_as_recurring and the public API's idempotency-key support. Fixes #3334. Co-Authored-By: Claude <noreply@anthropic.com> * fix(transactions): store the idempotency key in its own column, not external_id Codex review finding: reusing external_id/source for the web-form idempotency token made every manually-created transaction satisfy Entry#linked? (external_id.present?), since the form always supplies a key. That incorrectly made manual entries look provider-synced - disabling their date/nature/amount/currency fields in the editor (app/views/transactions/show.html.erb), and hiding them from future provider dedup matching (which filters to external_id: nil). Adds a dedicated entries.idempotency_key column with its own partial unique index scoped by account_id, used only for this de-duplication and with no meaning anywhere else in the app, so it can't collide with provider-linkage semantics. TransactionsController now tags/looks up entries by this column instead of source/external_id. Added a regression test asserting a transaction created via this path is not linked? and has no external_id/source set. Co-Authored-By: Claude <noreply@anthropic.com> * fix(migration): rebuild an invalid index left by an interrupted CONCURRENTLY build Codex review finding: index_exists? alone doesn't distinguish a valid index from an INVALID one left behind by an interrupted CREATE INDEX CONCURRENTLY (e.g. a deploy killed mid-build). A retry after such a failure would short-circuit on the early-return and record this migration as applied, while the actual uniqueness constraint stays missing/broken. Checks pg_index.indisvalid directly before deciding whether to skip the rebuild. Co-Authored-By: Claude <noreply@anthropic.com> * fix(transactions): rotate idempotency token on bfcache/Turbo restore, keep index removal concurrent Codex flagged that a page restored from the browser bfcache or Turbo's snapshot cache (back button, duplicated tab) keeps the already-consumed idempotency token in the hidden field. Submitting a different, edited transaction from that restored page would then match the old committed entry and silently redirect onto it instead of creating the new one. transaction_form_controller now rotates the token on turbo:before-cache so any later restore starts from a fresh, unconsumed value. Also address CodeRabbit's note that the migration's down block did a blocking DROP INDEX instead of DROP INDEX CONCURRENTLY. * fix(transactions): also rotate idempotency token on native bfcache restore CodeRabbit noted turbo:before-cache only covers Turbo's own snapshot cache, not the browser's native bfcache (e.g. a full navigation away and back, not through Turbo drive). Add a persisted-pageshow handler alongside it, and wire both through declarative data-action bindings on the form per this repo's Stimulus convention instead of manual addEventListener/connect/disconnect. * fix(transactions): fall back to manual UUID when crypto.randomUUID is unavailable crypto.randomUUID() requires a secure context, but this app's self-hosted mode is commonly reached over plain HTTP (LAN, reverse proxy without TLS). On such a deployment, calling it inside the cache-restore rotation handlers throws, leaving the stale, already-consumed idempotency token in the hidden field — a later edited resubmission would then silently match the old entry via find_duplicate_manual_entry and drop the user's edits. Build a v4 UUID manually from crypto.getRandomValues (which has no secure-context restriction) when randomUUID is missing. Also drops a stale comment reference to a MANUAL_FORM_SOURCE constant that doesn't exist anywhere in the codebase, and corrects a rescue comment that still described the old (account_id, source, external_id) index instead of the (account_id, idempotency_key) index actually backing this constraint. --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user