F-05 (High): Detect idempotency-key reuse with a different payload#5
Merged
Conversation
The dedup path returned the previously-stored entry on any key match, without comparing payloads — and the pre-check ran before validation. Two different business transactions accidentally sharing a key (a recycled request id, bad key derivation) meant the second was silently never recorded: the caller received a well-formed Entry for a $999 sale that does not exist in the ledger. A lost posting, discovered only at reconciliation, if ever. - pluts_entry_keys gains payload_hash: a SHA-256 fingerprint of the posted payload's business content (description, date, ordered lines of account id + minor units), computed via computeEntryFingerprint - Ledger.postEntry now validates first, then compares fingerprints on a key hit: identical => return the original entry (genuine retry); different => throw the new IdempotencyConflictError (carrying the key and existing entry id); the same comparison guards the unique-constraint race-recovery path in SqlStorageRepository - migrate() adds the column to legacy databases via a table_info probe (ALTER TABLE is not idempotent); legacy rows keep the '' default, treated as "no recorded fingerprint" so pre-upgrade retries still work - Repository gains getEntryKeyRecord(key); in-memory repo mirrors the unique-constraint semantics Audit finding F-05 (High). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387bd8c36b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Jul 17, 2026
A date-less request has its date defaulted to "today" in buildEntry before the idempotency fingerprint was computed, so retrying the identical request after a UTC day rollover produced a different fingerprint and threw IdempotencyConflictError instead of returning the original entry. EntryPayload now records dateWasDefaulted, and computeEntryFingerprint hashes null for defaulted dates — a date-less retry fingerprints identically regardless of when it lands. Explicit dates hash as before. Addresses PR #5 review (P1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both sides touched the repository insertEntry seam: main added assertBalanced (F-03) and this branch added fingerprint-based idempotency. Kept both — balance is asserted first (invalid input is never a dedup hit), then the key check runs, mirroring Ledger.postEntry's ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f5c38cfdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
postEntry spread input.debits/input.credits before any validation ran, so a structurally malformed body from untyped JS (e.g. missing credits) threw a raw TypeError instead of the promised path-tagged ValidationError — the validate-first ordering introduced for the idempotency fix made this reachable on the reused-key path too. The scan now skips non-array collections and non-string names; buildEntry's schema parse remains the validation authority. Addresses PR #5 review (P2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c074357d60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Numbers like 1e21 or 1e-7 pass z.number().finite().nonnegative() but stringify in exponential notation, which Amount.fromMajor's digit parser rejects with RangeError — escaping the schema transform as an uncaught runtime error instead of the promised path-tagged ValidationError (newly reachable on the keyed-retry path since validation now runs before the dedup lookup). The transform now catches the parse failure and reports a Zod issue instead. Addresses PR #5 review (P2, transform throws). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kept both sides at the insertEntry seam: this branch's payload fingerprint and main's storage-int bridging (F-06). Reconciled the frozen-arrays/aliasing test combination from main (push now throws), and re-pinned the exponential-notation amount case against main's fromMajor, which now parses that notation — the schema transform's issue-reporting wrapper stays as defense-in-depth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding
F-05 (High) from the ledger audit:
Ledger.postEntryreturned the previously-stored entry on any idempotency-key match, without comparing payloads — and the dedup pre-check ran before validation. The audit demonstrated (probe P5): post sale A ($100, keyk1), then sale B ($999, same key) — the second call returned sale A's entry with no error, and the ledger contains one entry. $999 of activity silently vanished.Idempotency keys exist to make retries of the same transaction safe. When two different transactions collide on a key (recycled request ID, bad key derivation — client bugs), the correct behavior is a loud conflict (cf. Stripe's 409
idempotency_error). Failing open converts a client bug into a lost posting discovered only at reconciliation, if ever.Fix
pluts_entry_keysgainspayload_hash: SHA-256 fingerprint of the posted payload's business content (description, date, ordered lines of[accountId, minorUnits]), via the new exportedcomputeEntryFingerprint(payload).Ledger.postEntrynow validates first (an invalid payload is invalid input, never a dedup hit — previously an invalid retry with a used key "succeeded"), then on a key hit compares fingerprints:IdempotencyConflictError(carrieskey+existingEntryId); nothing is writtenSqlStorageRepository.insertEntry, so two concurrent posts racing past the pre-check get the same semantics.migrate()adds the column to already-provisioned databases via aPRAGMA table_infoprobe (ALTER TABLEisn't idempotent;table_infois an allowed pragma in DO SQL storage). Legacy rows keep the''default, treated as "no recorded fingerprint" — retries of pre-upgrade postings keep working.RepositorygainsgetEntryKeyRecord(key); the in-memory test repository mirrors the unique-constraint semantics exactly.Testing
Written first and watched fail: key reuse with different payload →
IdempotencyConflictErrorwith nothing written; invalid payload with a used key →ValidationError. Plus newmigrate.spec.tsrunning realmigrate()against SQLite vianode:sqlite: fresh provision includes the column, legacy database gains it with''for existing rows, idempotent double-run. Existing identical-retry and no-key tests still pass.Full suite: 83 passed.
tsc --noEmitandbiome checkclean.Note: shares the
test/types/node-sqlite.d.tsdeclaration with #4 (identical content — merges cleanly in either order).🤖 Generated with Claude Code