security: send lock_id via X-CacheKit-Lock-Id header, not query string (#24)#29
Conversation
#24) release_lock built the unlock URL as {key}/lock?lock_id={token}. Query strings leak into access/proxy logs and OpenTelemetry http.url spans (CWE-532) — even URL-encoded, the capability token is still in the URL and replayable. Extract release_request() (shared by release_lock + a server-free test), drop ?lock_id= from the URL, and send the token in the X-CacheKit-Lock-Id header. SaaS dual-reads header + legacy query (deployed in saas 0.1.7), so this is wire-compatible with prod. Ratified in protocol spec/saas-api.md. Audited crates/cachekit/src/backend/workers.rs — no lock_id query usage there. Test asserts the token is in the header and never in the DELETE URL.
WalkthroughThe CachekitIO ChangesLock token header transport
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 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 `@crates/cachekit/src/backend/cachekitio_lock.rs`:
- Around line 148-152: Replace the hardcoded header name in the test with the
LOCK_ID_HEADER constant: locate the assertion fetching the header in
cachekitio_lock.rs (the code block using req.headers().get(...).expect(...) and
assert_eq!(header, "lock-secret-123")) and change the header lookup to use
LOCK_ID_HEADER instead of the string literal so the test references the
canonical header name used by the implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cde2206-fc83-400f-8140-6397bc2e2586
📒 Files selected for processing (1)
crates/cachekit/src/backend/cachekitio_lock.rs
| let header = req | ||
| .headers() | ||
| .get("X-CacheKit-Lock-Id") | ||
| .expect("X-CacheKit-Lock-Id header must be set"); | ||
| assert_eq!(header, "lock-secret-123"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use the LOCK_ID_HEADER constant instead of the hardcoded string.
The test hardcodes "X-CacheKit-Lock-Id" rather than referencing the LOCK_ID_HEADER constant. If the header name ever changes in the implementation, this test would silently pass whilst verifying the wrong header.
♻️ Suggested fix
// ...it rides the X-CacheKit-Lock-Id header under the exact wire name.
let header = req
.headers()
- .get("X-CacheKit-Lock-Id")
+ .get(LOCK_ID_HEADER)
.expect("X-CacheKit-Lock-Id header must be set");
assert_eq!(header, "lock-secret-123");🤖 Prompt for 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.
In `@crates/cachekit/src/backend/cachekitio_lock.rs` around lines 148 - 152,
Replace the hardcoded header name in the test with the LOCK_ID_HEADER constant:
locate the assertion fetching the header in cachekitio_lock.rs (the code block
using req.headers().get(...).expect(...) and assert_eq!(header,
"lock-secret-123")) and change the header lookup to use LOCK_ID_HEADER instead
of the string literal so the test references the canonical header name used by
the implementation.
|
Merging despite the red |
1 participant
Closes #24.
release_lockbuilt the unlock URL as{key}/lock?lock_id={token}. Query strings leak into access/proxy logs and OpenTelemetryhttp.urlspans (CWE-532) — even URL-encoded, the capability token is still in the URL and replayable within the lock TTL.Change
CachekitIO::release_request()(shared byrelease_lockand a server-free test).?lock_id=from the URL; the token is sent in theX-CacheKit-Lock-Idheader (newLOCK_ID_HEADERconst).Wire compatibility
SaaS dual-reads the header + legacy
?lock_id=(prefers header), shipped in saas#140 / deployed in0.1.7. Wire-compatible with liveapi.cachekit.io. Ratified in protocolspec/saas-api.md.Tests / checks
RequestBuilder::build()(no mock server) and asserts the token is in the header and never in the DELETE URL (no query, nolock_id, no token substring).cargo test+cargo clippy --all-targets -- -D warnings+cargo fmt --checkall clean.crates/cachekit/src/backend/workers.rs(per track: lock_id query-string → header (pending protocol decision) #24) — nolock_idquery usage there.Summary by CodeRabbit
Release Notes