Uh oh!
There was an error while loading. Please reload this page.
fix(tempo): claim transaction hashes to prevent charge replay - #23
Open
ygd58 wants to merge 1 commit into
Open
Conversation
TempoChargeIntent verified credentials statelessly: neither the "transaction" (pull) nor "hash" (push) flow ever recorded that a given transaction hash had already produced a successful receipt. A client could keep a previously-accepted hash (or valid Authorization header) and resubmit it against the same or a different challenge with matching payment terms, and each resubmission was accepted as a fresh successful payment — the protected resource could be delivered repeatedly for a single on-chain transfer. For push-mode specifically this was the more direct vector: an attacker only needs a hash whose logs satisfy a request's currency/recipient/amount, which requires no signature or private key at all once such a hash is known. Ports the Store abstraction from tempoxyz/mpp-go's pkg/tempo (Store interface + MemoryStore, using ConcurrentHashMap.putIfAbsent for atomic claim semantics — no external locking needed) so a hash can be claimed exactly once. TempoChargeIntent gains a Store field (defaults to an in-process MemoryStore; a new public constructor accepts a caller-supplied Store, e.g. Redis-backed, for multi-instance deployments where an in-memory store alone wouldn't be visible across servers) and claims the hash in awaitReceipt, right after receipt logs are confirmed to satisfy the request and before returning success. Claiming after the match (not before) means a hash that fails to match never burns a replay slot. The claim lives in the shared awaitReceipt path used by both flows, so the replay namespace is shared too: a hash that already produced a receipt via "transaction" can't later satisfy a "hash" credential, and vice versa. Scope: this closes the concrete "same accepted hash reused" gap described in the issue. The pure pull-mode double-broadcast case (the exact same signed rawTx bytes resubmitted) already fails naturally at the RPC layer (nonce reuse / already-known-transaction), so it doesn't need the store. The issue's broader ask — claiming challenge.id generically in the shared Verify.java path for every payment method, not just Tempo — is a bigger, cross-cutting change I'd want maintainer input on before touching; Stripe's charge flow already gets replay protection through its own idempotency-key binding (see stripe#21), so Tempo's hash flow was the concrete gap with no protection at all. Adds: - MemoryStoreTest: get/put/putIfAbsent/delete semantics, plus a concurrency test asserting exactly one of 64 concurrent putIfAbsent calls against the same key wins. - TempoChargeIntentTest: hash replay is rejected, hash claiming is case-insensitive, a non-matching hash doesn't consume a slot, independent hashes claim independently, and a pull-mode hash can't be replayed via a hash-mode credential. As with the other PRs today, I could not compile/run this against the real dependencies in my sandbox (no Maven Central access) — please double-check compilation before merge. Fixesstripe#15
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
TempoChargeIntentverified credentials statelessly: neither the"transaction"(pull) nor"hash"(push) flow ever recorded that a given transaction hash had already produced a successful receipt. A client could keep a previously-accepted hash (or valid Authorization header) and resubmit it against the same or a different challenge with matching payment terms, and each resubmission was accepted as a fresh successful payment — the protected resource could be delivered repeatedly for a single on-chain transfer.For push-mode specifically this is the more direct vector: an attacker only needs a hash whose logs satisfy a request's currency/recipient/amount — no signature or private key required once such a hash is known.
Fix
Ports the
Storeabstraction fromtempoxyz/mpp-go'spkg/tempo(Storeinterface +MemoryStore, usingConcurrentHashMap.putIfAbsentfor atomic claim semantics — no external locking needed) so a hash can be claimed exactly once.TempoChargeIntentgains aStorefield, defaulting to an in-processMemoryStore. A new public constructorTempoChargeIntent(String rpcUrl, Store store)accepts a caller-suppliedStore(e.g. Redis-backed) for multi-instance deployments, where an in-memory store alone wouldn't be visible across servers.awaitReceiptpath (used by both flows), right after receipt logs are confirmed to satisfy the request and before returning success — claiming after the match, not before, means a non-matching hash never burns a replay slot."transaction"can't later satisfy a"hash"credential, and vice versa.Scope
This closes the concrete "same accepted hash reused" gap described in the issue. The pure pull-mode double-broadcast case (resubmitting the exact same signed
rawTxbytes) already fails naturally at the RPC layer (nonce reuse / already-known-transaction), so it doesn't need the store.The issue's broader ask — claiming
challenge.idgenerically in the sharedVerify.javapath for every payment method, not just Tempo — is a bigger, cross-cutting change I'd want maintainer input on before touching. Stripe's charge flow already gets replay protection through its own idempotency-key binding (#21), so Tempo's hash flow was the concrete gap with zero protection.Testing
MemoryStoreTest:get/put/putIfAbsent/deletesemantics, plus a concurrency test asserting exactly one of 64 concurrentputIfAbsentcalls against the same key wins.TempoChargeIntentTest: hash replay is rejected, hash claiming is case-insensitive, a non-matching hash doesn't consume a slot, independent hashes claim independently, and a pull-mode hash can't be replayed via a hash-mode credential.As with #21/#22 today, I couldn't compile/run this against the real dependencies in my sandbox (no Maven Central access) — please double-check compilation before merge, happy to fix anything that doesn't match.
Fixes#15