Skip to content

fix: make idempotency reservation atomic and fail closed - #22

Merged
bscott merged 1 commit into
masterfrom
fix/idempotency-atomic-reservation
Aug 8, 2026
Merged

fix: make idempotency reservation atomic and fail closed#22
bscott merged 1 commit into
masterfrom
fix/idempotency-atomic-reservation

Conversation

@bscott

@bscott bscott commented Aug 8, 2026

Copy link
Copy Markdown
Owner

MEDIUM finding from the 0.2.6 security review. The duplicate-send guard could be defeated two ways.

It was racy

CheckIdempotencyKey ran early (mail.go:323), RecordIdempotencyKey ran only after the send returned (mail.go:422), and the send sat between them. Two concurrent invocations with the same key both observed it as unused and both sent — precisely what the key exists to prevent.

It also failed open

loadIdempotencyStore returned an empty store when json.Unmarshal failed:

if err := json.Unmarshal(data, store); err != nil {
    return store, nil // Return empty store on parse error
}

A corrupt or truncated file silently disabled the protection instead of reporting it.

The fix

One marker file per key, created with O_CREATE|O_EXCL. That makes the reservation atomic across processes with no lock file, and leaves no parsed content that can fail open. Expiry comes from the file mtime.

The key is hashed (SHA-256) to form the filename, so a user-supplied key containing path separators cannot steer the marker out of the store — pinned by TestKeyCannotEscapeIdempotencyDir.

Callers now reserve before sending and release on failure, so a failed send is still retryable with the same key. A key already held returns the existing "already sent" response unchanged, so the user-visible behavior for genuine duplicates is the same.

Verification

TestReserveIdempotencyKeyIsAtomic races 32 goroutines on one key and asserts exactly one winner. Clean under -race across repeated runs. Nine tests total, covering expiry reclamation, release-then-retry, empty-key passthrough, and 0600 marker permissions.

Compatibility

The store format changes from idempotency.json to an idempotency/ directory, with no migration. Keys expire after 24 hours, so the old file simply goes unused and can be deleted. Worth a line in the release notes, which I've added to the CHANGELOG.

gofmt, go vet, go test ./... clean.

🤖 Generated with Claude Code

The duplicate-send guard could be defeated two ways.

It was racy. CheckIdempotencyKey ran early, RecordIdempotencyKey ran
only after the send returned, and the send sat between them. Two
concurrent invocations with the same key both observed it as unused and
both sent, which is precisely what the key exists to prevent.

It also failed open. loadIdempotencyStore returned an empty store when
json.Unmarshal failed, so a corrupt or truncated file silently disabled
the protection rather than reporting it.

Replaces the shared JSON store with one marker file per key, created
with O_CREATE|O_EXCL. The reservation is therefore atomic across
processes without a lock file, and there is no parsed content that can
fail open. Expiry is read from the file mtime.

The key is hashed to form the filename, so a user-supplied key
containing path separators cannot steer the marker out of the store.

Callers now reserve before sending and release on failure, so a failed
send can be retried with the same key. A key that is already held
returns the existing "already sent" response unchanged.

The store format changes with no migration: keys expire after 24 hours,
so the old idempotency.json simply goes unused.

Verified with 32 goroutines racing on one key: exactly one wins, and the
suite is clean under -race.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bscott
bscott merged commit 08c5dd3 into master Aug 8, 2026
1 check passed
@bscott
bscott deleted the fix/idempotency-atomic-reservation branch August 8, 2026 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant