🧹 chore: Fix cache cleanup and redact token values #3757
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughDocumentation for Ctx.Err was shortened. Cache middleware refactored heap index handling and centralized cleanup on store errors. CSRF storageManager added a redacted key constant, a storage field, adjusted error messages, copies in-memory keys/values, and removed msgp methods/tests. Session data disabled msgp generation and removed its msgp methods/tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Cache as Cache Middleware
participant Heap as EntryHeap
participant Store as External Storage
participant Mem as In-Memory Map
Client->>Cache: New(request)
rect rgba(230,245,255,0.6)
note right of Cache: prepare entry, compute local heapIdx
Cache->>Heap: Push(entry with heapIdx)
note right of Cache: define cleanupOnStoreError(...)
end
alt External storage path
Cache->>Store: setRaw(key, value)
alt setRaw fails
Cache->>Cache: cleanupOnStoreError(removeFromHeap, adjustBytes, release, delRaw?)
Cache-->>Client: error
else setRaw ok
Cache-->>Client: success
end
else In-memory path
Cache->>Mem: set(key, value)
alt set fails
Cache->>Cache: cleanupOnStoreError(removeFromHeap, adjustBytes, release)
Cache-->>Client: error
else set ok
Cache-->>Client: success
end
end
par Expiration
Cache->>Heap: Pop(using local idx copy)
Cache->>Cache: release(entry)
end
sequenceDiagram
autonumber
actor App
participant CSRF as CSRF storageManager
participant Ext as fiber.Storage
participant Mem as In-Memory
rect rgba(245,235,255,0.5)
note right of CSRF: getRaw(key)
end
alt using external storage
CSRF->>Ext: Get(key)
alt error
CSRF-->>App: error "failed to get value from storage" (key redacted)
else ok
CSRF-->>App: bytes
end
else in-memory
CSRF->>Mem: Lookup(key)
alt type mismatch
CSRF-->>App: error "type assertion failed in storage"
else ok
CSRF-->>App: bytes
end
end
rect rgba(235,255,235,0.5)
note right of CSRF: setRaw(key, bytes)
end
alt external storage
CSRF->>Ext: Set(key, bytes)
alt error
CSRF-->>App: error with key "[redacted]"
else ok
CSRF-->>App: nil
end
else in-memory
CSRF->>CSRF: Copy key and bytes (CopyString/CopyBytes)
CSRF->>Mem: Store(copy)
CSRF-->>App: nil
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (7)
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 |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable fixes and improvements. The changes in the cache middleware correctly handle resource cleanup on storage errors, preventing potential leaks. In the CSRF middleware, redacting keys in errors and copying byte slices are great security and correctness enhancements. Skipping msgp generation for specific types is also a good move for maintainability. My review includes a couple of suggestions for the cache middleware to address a potential dead code path and to reduce some code duplication.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3757 +/- ##
==========================================
- Coverage 91.49% 91.41% -0.09%
==========================================
Files 113 113
Lines 11806 11828 +22
==========================================
+ Hits 10802 10812 +10
- Misses 738 748 +10
- Partials 266 268 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gaby
changed the title
gaby
changed the title
gaby
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes and improvements. The redaction of token values in CSRF error messages is a good security enhancement. The fixes for cache cleanup in the cache middleware correctly address potential resource leaks and stale data issues by ensuring heap and byte count are properly managed on storage errors. The addition of utils.CopyBytes when storing CSRF tokens in memory is a crucial fix to prevent data corruption. Finally, skipping msgp generation for structs with sync fields is the right approach to resolve the go vet warnings. The changes are well-implemented. I have one minor suggestion to improve a comment for better maintainability.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses security concerns and fixes resource management issues in cache and CSRF middleware. The changes prevent token value leaks, fix memory management, and resolve code generation issues.
Key changes:
- Redacts sensitive token values in error messages to prevent information leakage
- Fixes cache cleanup logic to prevent memory leaks and properly track heap accounting
- Excludes specific data types from msgp generation to resolve go vet copylock warnings
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/csrf/storage_manager.go | Redacts token values in error messages and copies byte slices before storage |
| middleware/cache/cache.go | Fixes cache cleanup logic and removes redundant pool releases |
| middleware/session/data.go | Adds msgp:ignore directive to prevent code generation |
| middleware/csrf/storage_manager_msgp.go | Removes generated storageManager serialization code |
| middleware/session/data_msgp.go | Removes generated data serialization code |
| middleware/csrf/storage_manager_msgp_test.go | Removes generated storageManager test code |
| middleware/session/data_msgp_test.go | Removes generated data test code |
| ctx_interface_gen.go | Simplifies comment documentation for Err method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ctx_interface_gen.go(1 hunks)middleware/cache/cache.go(2 hunks)middleware/csrf/storage_manager.go(3 hunks)middleware/csrf/storage_manager_msgp.go(0 hunks)middleware/csrf/storage_manager_msgp_test.go(0 hunks)middleware/session/data.go(1 hunks)middleware/session/data_msgp.go(0 hunks)middleware/session/data_msgp_test.go(0 hunks)
💤 Files with no reviewable changes (4)
- middleware/session/data_msgp_test.go
- middleware/csrf/storage_manager_msgp_test.go
- middleware/csrf/storage_manager_msgp.go
- middleware/session/data_msgp.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/session/data.gomiddleware/cache/cache.goctx_interface_gen.gomiddleware/csrf/storage_manager.go
🧬 Code graph analysis (1)
middleware/cache/cache.go (1)
storage_interface.go (1)
Storage(10-46)
🔇 Additional comments (13)
ctx_interface_gen.go (1)
47-51: Doc tweak for Ctx.Err: LGTM.Accurately reflects parity with context.Err while noting fasthttp no-op.
middleware/cache/cache.go (4)
163-168: Fix: capture heap index before release — correct.Capturing
idx := e.heapidxbeforemanager.release(e)prevents using a potentially recycled entry; heap accounting now stays consistent.
306-312: Heap index handling on insert: LGTM.Storing
heapIdxlocally and mirroring intoe.heapidxkeeps both paths coherent and simplifies later removals.
314-323: Centralized cleanup helper reduces duplication.The
cleanupOnStoreErrorclosure consolidates heap unwind and optional release, addressing earlier repetition.
337-341: In-memory set error path likely unreachable — verify.If
cfg.Storage == nil,manager.settypically can’t fail; the cleanup call may be dead code. Either document thatmanager.setcan error in-memory or simplify.#!/bin/bash # Inspect manager.set implementation and its error behavior. rg -nP -C3 --type=go 'func\s+\(\s*\*?manager\s*\)\s*set\s*\(' rg -nP -C2 --type=go '\bmanager\.set\('middleware/csrf/storage_manager.go (7)
19-19: Key redaction constant: good addition.Prevents leaking sensitive tokens in errors.
21-27: Skip msgp on storageManager: LGTM.Avoids copylock and keeps generated code minimal.
52-54: Storage get error message: safer phrasing.Generic message plus wrapped error avoids exposing keys.
61-62: Type guard message: fine.Covers unexpected in-memory value type without leaking internals.
73-74: Redacted key on store errors: correct.Keeps secrets out of logs.
78-80: Comment clarity and typo.Fix “crsf” and note both key and raw are copied to avoid buffer reuse issues.
- // the key is crucial in crsf and sometimes a reference to another value which can be reused later(pool/unsafe values concept), so a copy is made here + // To prevent corruption from reused buffers, copy both the key and raw bytes + // before storing them in memory (pool/unsafe value reuse).
85-88: Redacted delete errors: LGTM.Consistent with set/get redaction.
middleware/session/data.go (1)
10-10: Remove or disable msgp generator — directive still present.
middleware/session/data.go:9 contains//go:generate msgp -o=data_msgp.go -tests=true -unexported; middleware/session/data_msgp.go exists — runninggo generatewill recreate the file. Remove or scope the//go:generatedirective (or stop committing the generated file).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
middleware/cache/cache.go (1)
314-322: Nice centralization of rollback; consider clearer param semantics.
entryReleased boolis inverted (true = skip release). Rename toreleaseEntry booland flip the condition to reduce cognitive load. Non-functional polish.Apply:
- cleanupOnStoreError := func(entryReleased bool) { + cleanupOnStoreError := func(releaseEntry bool) { if cfg.MaxBytes > 0 { _, size := heap.remove(heapIdx) storedBytes -= size } - if !entryReleased { + if releaseEntry { manager.release(e) } }Then invert the booleans at call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/cache/cache.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/cache/cache.go
🧬 Code graph analysis (1)
middleware/cache/cache.go (1)
storage_interface.go (1)
Storage(10-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (4)
middleware/cache/cache.go (4)
163-168: Correct order: capture heap handle before release; then evict.Good fix. Grabbing
idx := e.heapidxbeforemanager.release(e)avoids losing the handle when the entry is zeroed, and then adjustingstoredBytesviaheap.remove(idx)keeps accounting consistent.
337-341: Unreachable error branch for in‑memory set (duplicate of prior feedback).Per previous review, in‑memory
manager.setdoes not return an error; this cleanup block is dead code. Safe to keep, but consider removing to reduce noise.
307-312: Confirmed — indexedHeap.remove expects the stable index returned by put.
indexedHeap.remove(idx) looks up h.indices[idx] → current slice position; pushInternal and Swap update that mapping, so storing e.heapidx from heap.put and later calling remove on that handle is correct.
324-336: Entry ownership confirmed — no extra release requiredmanager.set (middleware/cache/manager.go) calls m.release(it) on marshal/store errors and also on success, so
eis released; keep the code as-is and do not add another manager.release(e).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
middleware/cache/cache.go (2)
163-168: Good fix: remove by saved heap index; add underflow guard.Capturing
e.heapidxbeforemanager.release(e)avoids using a possibly reset index. Also, consider guardingstoredBytes -= sizeto prevent uint underflow if accounting ever drifts.- _, size := heap.remove(idx) - storedBytes -= size + _, size := heap.remove(idx) + if storedBytes >= size { + storedBytes -= size + } else { + storedBytes = 0 + }
314-318: Mirror the underflow guard here as well.Same accounting protection as above inside the cleanup helper.
- if cfg.MaxBytes > 0 { - _, size := heap.remove(heapIdx) - storedBytes -= size - } + if cfg.MaxBytes > 0 { + _, size := heap.remove(heapIdx) + if storedBytes >= size { + storedBytes -= size + } else { + storedBytes = 0 + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/cache/cache.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/cache/cache.go
🧬 Code graph analysis (1)
middleware/cache/cache.go (1)
storage_interface.go (1)
Storage(10-46)
🪛 GitHub Check: lint
middleware/cache/cache.go
[failure] 323-323:
Error return value of manager.del is not checked (errcheck)
[failure] 314-314:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: golangci-lint
middleware/cache/cache.go
[error] 314-314: File is not properly formatted (gofumpt). Run 'gofmt -w' and re-run golangci-lint.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (4)
middleware/cache/cache.go (4)
306-312: Heap index assignment is consistent and clear.Using a local
heapIdxand copying it intoe.heapidxkeeps later cleanup deterministic.
329-333: Error path releases and accounting look correct for setRaw failure.On
setRawerror you unwind heap and release the entry. Good.
341-343: Uniform in-memory unwind is fine.Even if in-memory
setcurrently can’t fail, keeping the error path consistent is OK and low-cost.
335-337: Leak: release pooled entry on metadata store failure (external storage).If
manager.set(c, key, e, expiration)fails aftersetRawsucceeded,eis leaked. PassreleaseEntry=trueto return it to the pool.- if err := manager.set(c, key, e, expiration); err != nil { - cleanupOnStoreError(false, true) + if err := manager.set(c, key, e, expiration); err != nil { + cleanupOnStoreError(true, true) return err }
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 8750440 | Previous: 8136af2 | Ratio |
|---|---|---|---|
Benchmark_SetValWithStruct/error_type_should_ignore |
14.99 ns/op 0 B/op 0 allocs/op |
7.778 ns/op 0 B/op 0 allocs/op |
1.93 |
Benchmark_SetValWithStruct/error_type_should_ignore - ns/op |
14.99 ns/op |
7.778 ns/op |
1.93 |
This comment was automatically generated by workflow using github-action-benchmark.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes and improvements. It enhances security by redacting sensitive token values from error messages in the CSRF middleware. It also fixes potential memory leaks and incorrect heap accounting in the cache middleware by ensuring proper cleanup of expired and failed-to-store entries. Additionally, it resolves go vet copylock warnings by excluding certain structs containing mutexes from msgp generation. The changes are well-implemented. I have one minor suggestion regarding a comment in the CSRF storage manager to improve clarity and correctness.
Summary