Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 62 additions & 11 deletions crates/cachekit/src/backend/cachekitio_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,30 @@ use super::cachekitio::{reqwest_err_sanitized, CachekitIO};
use super::LockableBackend;
use crate::error::BackendError;

/// Lock capability token travels in this request header, never the query string
/// (CWE-532): a `?lock_id=` query leaks the token into access/proxy logs and
/// OpenTelemetry `http.url` spans. SaaS dual-reads header + legacy query during
/// rollout, preferring the header. See protocol `spec/saas-api.md`.
const LOCK_ID_HEADER: &str = "X-CacheKit-Lock-Id";

impl CachekitIO {
/// Build the unlock request. Extracted so tests can assert the lock_id rides the
/// `X-CacheKit-Lock-Id` header and never appears in the URL (CWE-532).
fn release_request(&self, key: &str, lock_id: &str) -> reqwest::RequestBuilder {
let url = format!(
"{}/v1/cache/{}/lock",
self.api_url(),
urlencoding::encode(key)
);
self.with_standard_headers(
self.client()
.delete(&url)
.bearer_auth(self.api_key_str())
.header(LOCK_ID_HEADER, lock_id),
)
}
}

#[derive(Serialize)]
#[serde(rename_all = "camelCase")]
struct LockAcquireRequest {
Expand Down Expand Up @@ -64,17 +88,10 @@ impl LockableBackend for CachekitIO {
}

async fn release_lock(&self, key: &str, lock_id: &str) -> Result<bool, BackendError> {
let url = format!(
"{}/v1/cache/{}/lock?lock_id={}",
self.api_url(),
urlencoding::encode(key),
urlencoding::encode(lock_id),
);

let req =
self.with_standard_headers(self.client().delete(&url).bearer_auth(self.api_key_str()));

let resp = req
// lock_id is a capability token → X-CacheKit-Lock-Id header, not the query string
// (CWE-532). See `release_request`.
let resp = self
.release_request(key, lock_id)
.send()
.await
.map_err(|e| reqwest_err_sanitized(e, self.api_key_str()))?;
Expand All @@ -100,4 +117,38 @@ mod tests {
_assert_lockable(backend);
}
}

#[test]
#[allow(clippy::expect_used)] // test-only: a failed build/missing header should panic loudly
fn release_lock_sends_token_in_header_not_url() {
let backend = CachekitIO::builder()
.api_url("https://api.cachekit.io")
.api_key("ck_test_key")
.build()
.expect("builder should succeed for the canonical host");

let req = backend
.release_request("my-key", "lock-secret-123")
.build()
.expect("request should build");

// CWE-532: the capability token must never appear in the URL.
let url = req.url();
assert!(url.query().is_none(), "unexpected query string: {url}");
assert!(
!url.as_str().contains("lock_id"),
"lock_id leaked into URL: {url}"
);
assert!(
!url.as_str().contains("lock-secret-123"),
"token leaked into URL: {url}"
);

// ...it rides the X-CacheKit-Lock-Id header under the exact wire name.
let header = req
.headers()
.get("X-CacheKit-Lock-Id")
.expect("X-CacheKit-Lock-Id header must be set");
assert_eq!(header, "lock-secret-123");
Comment on lines +148 to +152

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

}
}
Loading