fix(idempotency): release MemoryLock keys on unlock (GHSA-c5qf-v26h-rp5r) - #4569
Conversation
…p5r) MemoryLock.Unlock released the per-key mutex but never removed the map entry, so every unique X-Idempotency-Key permanently retained ~286 bytes. An unauthenticated client could exhaust process memory by sending unique 36-character keys to any route behind idempotency.New(). Backport of #3263: reference-count each key and delete the entry once the last holder released it. A plain delete would drop entries still awaited by blocked goroutines.
Walkthrough
ChangesMemoryLock key lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/idempotency/locker_internal_test.go`:
- Around line 33-53: Update the contention test around the goroutine loop to
deterministically exercise a blocked Lock: acquire and hold a designated key,
start a second goroutine that attempts Lock on that key and signal when it
begins waiting, release the holder, then wait for the blocked goroutine to
complete and verify the locker's cleanup state. Preserve error reporting and
ensure all goroutines and locks are properly released.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a712369-7093-46f5-96c8-ba8a14486880
📒 Files selected for processing (2)
middleware/idempotency/locker.gomiddleware/idempotency/locker_internal_test.go
Description
Backport of #3263 to v2.
MemoryLock.Unlockreleased the per-key mutex but never removed the map entry, soevery unique
X-Idempotency-Keypermanently retained one*sync.Mutexplus its keystring. An unauthenticated client can exhaust process memory by sending unique
36-character keys to any route behind
idempotency.New(), since the defaultKeyHeaderValidatechecks length only.Reported as GHSA-c5qf-v26h-rp5r (medium severity), affecting all v2 releases up to
v2.52.13. v3 has been fixed since #3263 (Dec 2024); v2 never received the backport.
Fix
Reference-count each key:
Lockincrements underl.mu,Unlockdecrements anddeletes the entry only once the count reaches zero. A plain
deleteinUnlockwould be incorrect, because it drops an entry that other goroutines are still
blocked on. The logic is identical to the version shipped in v3.
Verification
The advisory PoC was run verbatim against both builds, 50,000 POSTs with unique keys:
MemoryLock.keysretainedThe remaining 187 B/key is the response cache (
cfg.Storage), which is TTL swept(
Lifetime30 min,GCInterval15 min), so it is bounded rather than leaked.New regression test
Test_MemoryLock_NoKeyLeakasserts the map drains to zero after1000 sequential and 16x1000 contended lock/unlock cycles. The package passes under
-race.Changes introduced