From 923b2b223033f3ddbbc2ca5d1d9f60946d15015f Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Fri, 4 Sep 2026 13:24:27 +1000 Subject: [PATCH 1/3] security(cachekitio): reject all-dot cache-key segment (LAB-2878) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cache key of exactly `.` or `..` escapes the `/v1/cache/` prefix in cachekit-rs: reqwest parses the URL with rust-url (WHATWG URL Standard), which strips an all-dot path segment before the request leaves the process — `/v1/cache/..` -> `/v1/`, `.../../lock` -> `/v1/lock` — carrying the app bearer token to a route the SaaS cache-key-validator never sees (CWE-22). The Python-parity fix this ticket prescribed (rewrite to `%2E`/`%2E%2E`) does NOT work here: rust-url treats `%2e`/`%2e%2e` (case-insensitive) as dot-segments too, so the encoded form collapses identically (verified at the reqwest layer). Since every representation that decodes once back to `.`/`..` is a WHATWG dot-segment, no encoding survives — the only safe action is to refuse to build the request. Add a shared fallible `encode_key` in backend/mod.rs that rejects a key encoding to exactly `.`/`..` with a permanent BackendError, and thread Result through the `url`/`ttl_url`/`lock_url` builders (native cachekitio + wasm workers) and their callers, so every CachekitIO request path is type-forced through the one guard. Every other key encodes byte-identically. Aligns with the cachekit-ts twin (LAB-2877), which also rejects, and diverges deliberately from cachekit-py (whose RFC-3986 client keeps `%2E%2E` on the wire). `.`/`..` is never a canonical CacheKit key. Docs: README Security Properties note + encode_key/url doc comments. Expert-panel reviewed at high stakes (SHIP). --- README.md | 3 + crates/cachekit/src/backend/cachekitio.rs | 148 ++++++++++++++++-- .../cachekit/src/backend/cachekitio_lock.rs | 25 ++- crates/cachekit/src/backend/cachekitio_ttl.rs | 12 +- crates/cachekit/src/backend/mod.rs | 102 ++++++++++++ crates/cachekit/src/backend/workers.rs | 38 +++-- 6 files changed, 278 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index a771728..69980da 100644 --- a/README.md +++ b/README.md @@ -154,6 +154,9 @@ let ssn: String = secure.get("user:42:ssn").await?.unwrap(); | **AAD Binding** | Cache key bound to ciphertext (prevents substitution attacks) | | **Memory Safety** | [zeroize](https://crates.io/crates/zeroize) on drop for all key material | | **L1 Guarantee** | L1 stores ciphertext, never plaintext | +| **Cache-key path encoding (CWE-22)** | Keys are percent-encoded into the CachekitIO request path; a key of exactly `.` or `..` is **rejected** rather than sent | + +**Cache-key path encoding (CWE-22):** CachekitIO keys are percent-encoded (`urlencoding::encode`) so a key can only ever address `/v1/cache/{key}`. A key of exactly `.` or `..` is the one case encoding can't neutralise — `reqwest`'s WHATWG URL parser (rust-url) strips an all-dot path segment *before the request leaves the process* (`/v1/cache/..` → `/v1/`, `…/../lock` → `…/lock`), so the SDK **refuses** such a key with a permanent error instead of emitting a request whose path was rewritten. This is stricter than cachekit-py, whose RFC-3986 HTTP client keeps a re-encoded `%2E%2E` on the wire; `.`/`..` is never a canonical CacheKit key, so nothing legitimate is affected, and every other key encodes byte-identically across the SDKs. **AAD v0x03 wire format:** diff --git a/crates/cachekit/src/backend/cachekitio.rs b/crates/cachekit/src/backend/cachekitio.rs index cdc5f8e..cc53877 100644 --- a/crates/cachekit/src/backend/cachekitio.rs +++ b/crates/cachekit/src/backend/cachekitio.rs @@ -4,7 +4,7 @@ use std::time::Duration; use async_trait::async_trait; use zeroize::Zeroizing; -use crate::backend::{Backend, HealthStatus, LockableBackend}; +use crate::backend::{encode_key, Backend, HealthStatus, LockableBackend}; use crate::error::{BackendError, BackendErrorKind}; use crate::metrics::{metrics_headers, MetricsProvider}; use crate::session::session_headers; @@ -59,11 +59,31 @@ impl CachekitIO { /// Build the full URL for a cache key path segment. /// - /// Keys are percent-encoded so that slashes or special characters in the - /// cache key do not break the URL structure. - fn url(&self, key: &str) -> String { - let encoded = urlencoding::encode(key); - format!("{}/v1/cache/{}", self.api_url, encoded) + /// Keys are percent-encoded via [`encode_key`](crate::backend::encode_key) so + /// slashes or special characters do not break the URL structure. A key that + /// is exactly `.` or `..` is **rejected** (fallible return) rather than + /// encoded: `reqwest`'s WHATWG URL parser would strip an all-dot segment out + /// of the `/v1/cache/` prefix before the request is sent (CWE-22), and no + /// encoding survives that — see [`encode_key`](crate::backend::encode_key). + fn url(&self, key: &str) -> Result { + Ok(format!("{}/v1/cache/{}", self.api_url, encode_key(key)?)) + } + + /// Build the TTL URL for a cache key (`…/ttl`). Composes on [`Self::url`], so + /// it inherits the same [`encode_key`](crate::backend::encode_key) guard and + /// the `/v1/cache/` prefix lives in one place (matching the wasm `workers` + /// backend). `pub(crate)` so the [`TtlInspectable`](super::TtlInspectable) + /// impl in the sibling `cachekitio_ttl` module builds its path through it. + pub(crate) fn ttl_url(&self, key: &str) -> Result { + Ok(format!("{}/ttl", self.url(key)?)) + } + + /// Build the lock URL for a cache key (`…/lock`). Composes on [`Self::url`] + /// (same guard, same single prefix). `pub(crate)` so the + /// [`LockableBackend`](super::LockableBackend) impl in the sibling + /// `cachekitio_lock` module builds its path through it. + pub(crate) fn lock_url(&self, key: &str) -> Result { + Ok(format!("{}/lock", self.url(key)?)) } /// Build the health-check URL. @@ -125,7 +145,7 @@ impl Backend for CachekitIO { async fn get(&self, key: &str) -> Result>, BackendError> { let req = self.with_standard_headers( self.client - .get(self.url(key)) + .get(self.url(key)?) .bearer_auth(self.api_key.as_str()), ); @@ -155,7 +175,7 @@ impl Backend for CachekitIO { ) -> Result<(), BackendError> { let mut req = self .client - .put(self.url(key)) + .put(self.url(key)?) .bearer_auth(self.api_key.as_str()) .header(reqwest::header::CONTENT_TYPE, "application/octet-stream") .body(value); @@ -182,7 +202,7 @@ impl Backend for CachekitIO { async fn delete(&self, key: &str) -> Result { let req = self.with_standard_headers( self.client - .delete(self.url(key)) + .delete(self.url(key)?) .bearer_auth(self.api_key.as_str()), ); @@ -201,7 +221,7 @@ impl Backend for CachekitIO { async fn exists(&self, key: &str) -> Result { let req = self.with_standard_headers( self.client - .head(self.url(key)) + .head(self.url(key)?) .bearer_auth(self.api_key.as_str()), ); @@ -337,3 +357,111 @@ impl CachekitIOBuilder { }) } } + +// ── Cache-key path encoding tests (CWE-22) ──────────────────────────────────── + +#[cfg(test)] +#[allow(clippy::expect_used)] // test-only: a builder/parse failure on a fixture should panic loudly +mod path_encoding_tests { + use super::CachekitIO; + use url::Url; + + const API: &str = "https://api.cachekit.io"; + + fn backend() -> CachekitIO { + CachekitIO::builder() + .api_url(API) + .api_key("ck_test_key") + .build() + .expect("builder should succeed for the canonical host") + } + + /// AC-0 — Repro. Before any guard, a raw-encoded `.`/`..` key collapses in + /// rust-url (the parser `reqwest` uses) *before* the request leaves the + /// process: the segment is stripped and the path escapes `/v1/cache/`. + /// `%2E%2E` collapses identically, which is why the fix rejects rather than + /// re-encodes (WHATWG treats `%2e%2e` as a dot-segment too). + #[test] + fn repro_raw_dot_key_escapes_the_cache_prefix() { + let cases = [ + ("..", "", "/v1/"), + ("..", "/ttl", "/v1/ttl"), + ("..", "/lock", "/v1/lock"), + (".", "", "/v1/cache/"), + ]; + for (key, suffix, escaped) in cases { + let raw = format!("{API}/v1/cache/{}{suffix}", urlencoding::encode(key)); + let parsed = Url::parse(&raw).expect("parses"); + assert_eq!( + parsed.path(), + escaped, + "raw {key:?}{suffix} should collapse to {escaped}" + ); + // The %2E form the Python SDK emits collapses just the same in rust-url. + let pct = key.replace('.', "%2E"); + let enc = format!("{API}/v1/cache/{pct}{suffix}"); + assert_eq!( + Url::parse(&enc).expect("parses").path(), + escaped, + "%2E-encoded {key:?}{suffix} also collapses — encoding cannot fix this in rust-url" + ); + } + } + + /// AC-2 — the two all-dot keys are rejected by every builder (base, ttl, + /// lock): no URL is produced, so no rewritten request can ever be sent. + #[test] + fn dot_keys_are_rejected_by_every_builder() { + let b = backend(); + for key in [".", ".."] { + assert!(b.url(key).is_err(), "url({key:?}) must be rejected"); + assert!(b.ttl_url(key).is_err(), "ttl_url({key:?}) must be rejected"); + assert!( + b.lock_url(key).is_err(), + "lock_url({key:?}) must be rejected" + ); + } + } + + /// AC-2 — every non-dot vector builds a URL whose *parsed* path (the real + /// wire path, post-normalisation) stays inside `/v1/cache/`. Asserting on the + /// unparsed `format!` output would pass while still shipping a traversal, so + /// we parse with the same `url` crate `reqwest` uses. + #[test] + fn safe_keys_never_escape_the_cache_prefix() { + let b = backend(); + let vectors = [ + "a:..", + "default:../../admin", + "k?x=1#f", + "a b", + "ns:default:func:m.f:args:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef:", + ]; + for key in vectors { + let base = Url::parse(&b.url(key).expect("url")).expect("parse base"); + let ttl = Url::parse(&b.ttl_url(key).expect("ttl_url")).expect("parse ttl"); + let lock = Url::parse(&b.lock_url(key).expect("lock_url")).expect("parse lock"); + + assert!( + base.path().starts_with("/v1/cache/") && base.path().len() > "/v1/cache/".len(), + "base path {} escaped prefix for {key:?}", + base.path() + ); + assert_eq!( + base.path(), + format!("/v1/cache/{}", urlencoding::encode(key)), + "base wire path mismatch for {key:?}" + ); + assert!( + ttl.path().starts_with("/v1/cache/") && ttl.path().ends_with("/ttl"), + "ttl path {} escaped prefix for {key:?}", + ttl.path() + ); + assert!( + lock.path().starts_with("/v1/cache/") && lock.path().ends_with("/lock"), + "lock path {} escaped prefix for {key:?}", + lock.path() + ); + } + } +} diff --git a/crates/cachekit/src/backend/cachekitio_lock.rs b/crates/cachekit/src/backend/cachekitio_lock.rs index 7614ad1..4be6322 100644 --- a/crates/cachekit/src/backend/cachekitio_lock.rs +++ b/crates/cachekit/src/backend/cachekitio_lock.rs @@ -16,18 +16,18 @@ 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( + fn release_request( + &self, + key: &str, + lock_id: &str, + ) -> Result { + let url = self.lock_url(key)?; + Ok(self.with_standard_headers( self.client() .delete(&url) .bearer_auth(self.api_key_str()) .header(LOCK_ID_HEADER, lock_id), - ) + )) } } @@ -53,11 +53,7 @@ impl LockableBackend for CachekitIO { key: &str, timeout_ms: u64, ) -> Result, BackendError> { - let url = format!( - "{}/v1/cache/{}/lock", - self.api_url(), - urlencoding::encode(key) - ); + let url = self.lock_url(key)?; let body = serde_json::to_vec(&LockAcquireRequest { timeout_ms }).map_err(|e| { BackendError::permanent(format!("failed to serialize lock request: {e}")) @@ -92,7 +88,7 @@ impl LockableBackend for CachekitIO { // 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) + .release_request(key, lock_id)? .send() .await .map_err(|e| reqwest_err_sanitized(e, self.api_key_str()))?; @@ -130,6 +126,7 @@ mod tests { let req = backend .release_request("my-key", "lock-secret-123") + .expect("release_request should build for a normal key") .build() .expect("request should build"); diff --git a/crates/cachekit/src/backend/cachekitio_ttl.rs b/crates/cachekit/src/backend/cachekitio_ttl.rs index b1ed3ef..57dac77 100644 --- a/crates/cachekit/src/backend/cachekitio_ttl.rs +++ b/crates/cachekit/src/backend/cachekitio_ttl.rs @@ -25,11 +25,7 @@ struct RefreshTtlRequest { #[cfg_attr(feature = "unsync", async_trait(?Send))] impl TtlInspectable for CachekitIO { async fn ttl(&self, key: &str) -> Result, BackendError> { - let url = format!( - "{}/v1/cache/{}/ttl", - self.api_url(), - urlencoding::encode(key) - ); + let url = self.ttl_url(key)?; let req = self.with_standard_headers(self.client().get(&url).bearer_auth(self.api_key_str())); @@ -59,11 +55,7 @@ impl TtlInspectable for CachekitIO { )); } - let url = format!( - "{}/v1/cache/{}/ttl", - self.api_url(), - urlencoding::encode(key) - ); + let url = self.ttl_url(key)?; let body = serde_json::to_vec(&RefreshTtlRequest { ttl: secs }).map_err(|e| { BackendError::permanent(format!("failed to serialize refresh_ttl request: {e}")) diff --git a/crates/cachekit/src/backend/mod.rs b/crates/cachekit/src/backend/mod.rs index cab4362..1dc5476 100644 --- a/crates/cachekit/src/backend/mod.rs +++ b/crates/cachekit/src/backend/mod.rs @@ -185,6 +185,51 @@ pub(crate) async fn run_blocking( f() } +// ── Cache-key path encoding (CWE-22) ───────────────────────────────────────── + +/// Percent-encode a cache key for the `/v1/cache/{key}` path segment, shared by +/// the native `cachekitio` and wasm `workers` backends (DRY: every CachekitIO +/// path is built through this one fallible chokepoint). +/// +/// Almost every key is just [`urlencoding::encode`]. The exception is a key +/// that is exactly `.` or `..`, which is **rejected** with a permanent +/// [`BackendError`] rather than encoded — because there is no encoding of it +/// that survives the client's URL parser. +/// +/// A dot is RFC-3986 *unreserved*, so `urlencoding::encode("..") == ".."` +/// unchanged, and `reqwest`'s WHATWG URL parser (rust-url) then removes that +/// dot-segment **before the request leaves the process**: `/v1/cache/..` → +/// `/v1/`, `…/../ttl` → `…/ttl`, escaping the `/v1/cache/` prefix the SaaS +/// `cache-key-validator` guards and carrying the app's bearer token to a route +/// it never vetted (CWE-22). Percent-encoding does **not** help here: WHATWG +/// treats `%2e` / `%2e%2e` (case-insensitive) as dot-segments too, and rust-url +/// `%2E%2E` → `/v1/` and `%2E` → `/v1/cache/` are both verified to collapse +/// (see the `repro_raw_dot_key_escapes_the_cache_prefix` test). Since every representation that +/// `decodeURIComponent`s once back to `.`/`..` is a WHATWG dot-segment, no +/// encoding can both reach the wire intact **and** round-trip — so the only safe +/// action is to refuse to build the request at all. +/// +/// This diverges deliberately from cachekit-py's `_encode_key` +/// (`src/cachekit/backends/cachekitio/backend.py:247-250` @ `f000ba3`), which +/// rewrites to `%2E`: Python's HTTP client applies RFC-3986 `remove_dot_segments` +/// (which does **not** decode `%2e`), so `%2E%2E` survives there and the SaaS +/// rejects the decoded `..`. rust-url is stricter. `.`/`..` is never a canonical +/// CacheKit key (those always contain `:`), so refusing it breaks nothing +/// legitimate. For every **other** key the output is byte-identical to +/// `urlencoding::encode`, preserving cross-SDK wire parity. +#[cfg(any(feature = "cachekitio", feature = "workers", test))] +pub(crate) fn encode_key(key: &str) -> Result, BackendError> { + let encoded = urlencoding::encode(key); + if matches!(encoded.as_ref(), "." | "..") { + return Err(BackendError::permanent( + "cache key must not be `.` or `..`: the client URL parser strips an \ + all-dot path segment before the request is sent (CWE-22), so it \ + cannot be addressed on the wire", + )); + } + Ok(encoded) +} + // ── Feature-gated backend modules ───────────────────────────────────────────── /// JSON wire bodies for the SaaS lock/TTL endpoints. Compiled under `test` @@ -216,3 +261,60 @@ pub mod file; /// Cloudflare Workers backend using `worker::Fetch`. #[cfg(feature = "workers")] pub mod workers; + +// ── encode_key unit tests (CWE-22) ─────────────────────────────────────────── + +#[cfg(test)] +#[allow(clippy::expect_used)] // test-only: an encoding failure on a safe key should panic loudly +mod encode_key_tests { + use super::encode_key; + + /// Non-`.`/`..` vectors: canonical key, near-misses (`..` embedded, not a + /// whole segment), reserved chars, spaces, empty. + const SAFE_VECTORS: &[&str] = &[ + "a:..", + "..a", + "a..", + ".hidden", + "default:../../admin", + "ns:default:func:m.f:args:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef:", + "a b", + "k?x=1#f", + "", + ]; + + #[test] + fn all_dot_keys_are_rejected() { + // No encoding of `.`/`..` survives rust-url/WHATWG dot-segment removal + // (see `cachekitio::path_encoding_tests::dot_keys_are_rejected_by_every_builder`), + // so the guard refuses to build a request rather than emit one whose path + // was rewritten (CWE-22). + assert!(encode_key(".").is_err()); + assert!(encode_key("..").is_err()); + } + + #[test] + fn safe_keys_are_byte_identical_to_urlencoding() { + // AC-1: nothing but an exact `.`/`..` segment may change encoding, or the + // SDKs diverge on the wire. + for k in SAFE_VECTORS { + let enc = encode_key(k).expect("safe key must encode"); + assert_eq!( + enc, + urlencoding::encode(k), + "encode_key diverged from urlencoding for {k:?}" + ); + } + } + + #[test] + fn safe_keys_decode_once_back_to_the_original() { + // AC-3: the SaaS validator does a single `decodeURIComponent`; every safe + // vector must survive that exact round-trip untouched. + for k in SAFE_VECTORS { + let enc = encode_key(k).expect("safe key must encode"); + let decoded = urlencoding::decode(&enc).expect("single decode succeeds"); + assert_eq!(decoded, *k, "decode-once round-trip changed {k:?}"); + } + } +} diff --git a/crates/cachekit/src/backend/workers.rs b/crates/cachekit/src/backend/workers.rs index 49e73e0..6d19bf1 100644 --- a/crates/cachekit/src/backend/workers.rs +++ b/crates/cachekit/src/backend/workers.rs @@ -13,7 +13,7 @@ use zeroize::Zeroizing; use crate::backend::saas_wire::{ LockAcquireRequest, LockAcquireResponse, RefreshTtlRequest, TtlResponse, }; -use crate::backend::{Backend, HealthStatus, LockableBackend, TtlInspectable}; +use crate::backend::{encode_key, Backend, HealthStatus, LockableBackend, TtlInspectable}; use crate::error::BackendError; use crate::metrics::{metrics_headers, MetricsProvider}; use crate::session::session_headers; @@ -53,9 +53,15 @@ impl WorkersCachekitIO { } /// Build the full URL for a cache key path segment. - fn url(&self, key: &str) -> String { - let encoded = urlencoding::encode(key); - format!("{}/v1/cache/{}", self.api_url, encoded) + /// + /// Keys are percent-encoded via [`encode_key`](crate::backend::encode_key); a + /// key that is exactly `.` or `..` is **rejected** (fallible return) rather + /// than encoded, because the Workers runtime `fetch` (WHATWG URL) would strip + /// an all-dot segment out of the `/v1/cache/` prefix before the request is + /// sent (CWE-22) — see [`encode_key`](crate::backend::encode_key). + /// `ttl_url`/`lock_url` build on this, so all three wasm paths inherit the guard. + fn url(&self, key: &str) -> Result { + Ok(format!("{}/v1/cache/{}", self.api_url, encode_key(key)?)) } /// Build the health-check URL. @@ -65,13 +71,13 @@ impl WorkersCachekitIO { /// Build the lock URL for a cache key. Callers pass the bare cache key; /// the SaaS lock endpoint owns the lock namespace server-side. - fn lock_url(&self, key: &str) -> String { - format!("{}/lock", self.url(key)) + fn lock_url(&self, key: &str) -> Result { + Ok(format!("{}/lock", self.url(key)?)) } /// Build the TTL URL for a cache key. - fn ttl_url(&self, key: &str) -> String { - format!("{}/ttl", self.url(key)) + fn ttl_url(&self, key: &str) -> Result { + Ok(format!("{}/ttl", self.url(key)?)) } /// Convert a non-success response into a classified, sanitized error. @@ -171,7 +177,7 @@ impl WorkersCachekitIO { #[async_trait(?Send)] impl Backend for WorkersCachekitIO { async fn get(&self, key: &str) -> Result>, BackendError> { - let mut resp = self.fetch("GET", &self.url(key), None, vec![]).await?; + let mut resp = self.fetch("GET", &self.url(key)?, None, vec![]).await?; match resp.status_code() { 200 => { @@ -200,7 +206,7 @@ impl Backend for WorkersCachekitIO { } let mut resp = self - .fetch("PUT", &self.url(key), Some(value), headers) + .fetch("PUT", &self.url(key)?, Some(value), headers) .await?; let status = resp.status_code(); @@ -212,7 +218,7 @@ impl Backend for WorkersCachekitIO { } async fn delete(&self, key: &str) -> Result { - let mut resp = self.fetch("DELETE", &self.url(key), None, vec![]).await?; + let mut resp = self.fetch("DELETE", &self.url(key)?, None, vec![]).await?; match resp.status_code() { 200 | 204 => Ok(true), @@ -222,7 +228,7 @@ impl Backend for WorkersCachekitIO { } async fn exists(&self, key: &str) -> Result { - let resp = self.fetch("HEAD", &self.url(key), None, vec![]).await?; + let resp = self.fetch("HEAD", &self.url(key)?, None, vec![]).await?; match resp.status_code() { 200 => Ok(true), @@ -272,7 +278,7 @@ impl LockableBackend for WorkersCachekitIO { let mut resp = self .fetch( "POST", - &self.lock_url(key), + &self.lock_url(key)?, Some(body), vec![("Content-Type", "application/json".to_owned())], ) @@ -302,7 +308,7 @@ impl LockableBackend for WorkersCachekitIO { let resp = self .fetch( "DELETE", - &self.lock_url(key), + &self.lock_url(key)?, None, vec![(LOCK_ID_HEADER, lock_id.to_owned())], ) @@ -321,7 +327,7 @@ impl LockableBackend for WorkersCachekitIO { #[async_trait(?Send)] impl TtlInspectable for WorkersCachekitIO { async fn ttl(&self, key: &str) -> Result, BackendError> { - let mut resp = self.fetch("GET", &self.ttl_url(key), None, vec![]).await?; + let mut resp = self.fetch("GET", &self.ttl_url(key)?, None, vec![]).await?; match resp.status_code() { 200 => { @@ -356,7 +362,7 @@ impl TtlInspectable for WorkersCachekitIO { let resp = self .fetch( "PATCH", - &self.ttl_url(key), + &self.ttl_url(key)?, Some(body), vec![("Content-Type", "application/json".to_owned())], ) From 0a4a247670b2dd149426c1ab5dfaab36d8cdcb1b Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Fri, 4 Sep 2026 14:25:15 +1000 Subject: [PATCH 2/3] fix(cachekitio): reject all five reserved cache-key segments (LAB-2879 conformance) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widen the client-side reject set from `.`/`..` to the full five reserved segments the finalized protocol spec mandates — `.`, `..`, `health`, `ttl`, `lock` (spec/saas-api.md § Cache-Key Path Encoding rule 2, protocol#61). The dot segments collapse in rust-url before send; the route tokens collide with real routes: `/v1/cache/health` IS the health endpoint, and a trailing `ttl`/`lock` segment selects a sub-resource — so a key of exactly `health`, `ttl` or `lock` is routed off the `/v1/cache/{key}` path carrying the bearer token (CWE-22), the same class of escape as the dot segments. `encode_key` now rejects a key whose encoded form is any of the five; tests and vectors mirror protocol/test-vectors/path-encoding.json (five reject rows, the transmittable rows byte-identical to urlencoding). Route-token near-misses (`healthy`, `HEALTH`, `ttls`, `unlock`, embedded `x/../../health`) transmit unchanged. Docs (README + doc comments) updated to the five-segment rule. --- README.md | 4 +- crates/cachekit/src/backend/cachekitio.rs | 45 +++++++-- crates/cachekit/src/backend/mod.rs | 115 +++++++++++++++------- crates/cachekit/src/backend/workers.rs | 12 ++- 4 files changed, 124 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 69980da..6fb7086 100644 --- a/README.md +++ b/README.md @@ -154,9 +154,9 @@ let ssn: String = secure.get("user:42:ssn").await?.unwrap(); | **AAD Binding** | Cache key bound to ciphertext (prevents substitution attacks) | | **Memory Safety** | [zeroize](https://crates.io/crates/zeroize) on drop for all key material | | **L1 Guarantee** | L1 stores ciphertext, never plaintext | -| **Cache-key path encoding (CWE-22)** | Keys are percent-encoded into the CachekitIO request path; a key of exactly `.` or `..` is **rejected** rather than sent | +| **Cache-key path encoding (CWE-22)** | Keys are percent-encoded into the CachekitIO request path; a key encoding to a reserved segment (`.`, `..`, `health`, `ttl`, `lock`) is **rejected** rather than sent | -**Cache-key path encoding (CWE-22):** CachekitIO keys are percent-encoded (`urlencoding::encode`) so a key can only ever address `/v1/cache/{key}`. A key of exactly `.` or `..` is the one case encoding can't neutralise — `reqwest`'s WHATWG URL parser (rust-url) strips an all-dot path segment *before the request leaves the process* (`/v1/cache/..` → `/v1/`, `…/../lock` → `…/lock`), so the SDK **refuses** such a key with a permanent error instead of emitting a request whose path was rewritten. This is stricter than cachekit-py, whose RFC-3986 HTTP client keeps a re-encoded `%2E%2E` on the wire; `.`/`..` is never a canonical CacheKit key, so nothing legitimate is affected, and every other key encodes byte-identically across the SDKs. +**Cache-key path encoding (CWE-22):** CachekitIO keys are percent-encoded (`urlencoding::encode`) so a key can only ever address `/v1/cache/{key}`. A key whose encoded form is one of the five reserved path segments — `.`, `..`, `health`, `ttl`, `lock` — is **rejected** with a permanent error rather than sent (protocol `spec/saas-api.md` § Cache-Key Path Encoding, rule 2). `.`/`..` are dot segments that `reqwest`'s WHATWG URL parser (rust-url) strips *before the request leaves the process* (`/v1/cache/..` → `/v1/`); `health`/`ttl`/`lock` are live route tokens (`/v1/cache/health` is the health endpoint, a trailing `ttl`/`lock` selects a sub-resource). Encoding can't neutralise either — WHATWG collapses `%2E%2E` too — so the SDK refuses rather than emit a request whose path was rewritten. This matches the cachekit-ts twin and is stricter than cachekit-py's older `%2E` rewrite; none of the five is ever a canonical CacheKit key (those contain `:`), so nothing legitimate is affected and every other key encodes byte-identically across the SDKs. **AAD v0x03 wire format:** diff --git a/crates/cachekit/src/backend/cachekitio.rs b/crates/cachekit/src/backend/cachekitio.rs index cc53877..15e8ac8 100644 --- a/crates/cachekit/src/backend/cachekitio.rs +++ b/crates/cachekit/src/backend/cachekitio.rs @@ -60,11 +60,12 @@ impl CachekitIO { /// Build the full URL for a cache key path segment. /// /// Keys are percent-encoded via [`encode_key`](crate::backend::encode_key) so - /// slashes or special characters do not break the URL structure. A key that - /// is exactly `.` or `..` is **rejected** (fallible return) rather than - /// encoded: `reqwest`'s WHATWG URL parser would strip an all-dot segment out - /// of the `/v1/cache/` prefix before the request is sent (CWE-22), and no - /// encoding survives that — see [`encode_key`](crate::backend::encode_key). + /// slashes or special characters do not break the URL structure. A key whose + /// encoded form is a reserved segment (`.`, `..`, `health`, `ttl`, `lock`) is + /// **rejected** (fallible return) rather than sent: the dot segments are + /// stripped by `reqwest`'s WHATWG URL parser and the route tokens collide + /// with the health/sub-resource routes, both escaping `/v1/cache/{key}` + /// (CWE-22, spec rule 2) — see [`encode_key`](crate::backend::encode_key). fn url(&self, key: &str) -> Result { Ok(format!("{}/v1/cache/{}", self.api_url, encode_key(key)?)) } @@ -408,12 +409,13 @@ mod path_encoding_tests { } } - /// AC-2 — the two all-dot keys are rejected by every builder (base, ttl, - /// lock): no URL is produced, so no rewritten request can ever be sent. + /// AC-2 / spec rule 2 — all five reserved segments (`.`, `..`, `health`, + /// `ttl`, `lock`) are rejected by every builder (base, ttl, lock): no URL is + /// produced, so no rewritten or mis-routed request can ever be sent. #[test] - fn dot_keys_are_rejected_by_every_builder() { + fn reserved_segments_rejected_by_every_builder() { let b = backend(); - for key in [".", ".."] { + for key in [".", "..", "health", "ttl", "lock"] { assert!(b.url(key).is_err(), "url({key:?}) must be rejected"); assert!(b.ttl_url(key).is_err(), "ttl_url({key:?}) must be rejected"); assert!( @@ -423,6 +425,29 @@ mod path_encoding_tests { } } + /// spec rule 2 — the route tokens collide with real routes: a key of exactly + /// `health` builds the health endpoint's own path, and `ttl`/`lock` build the + /// bare sub-resource paths. This is why they are reserved (rejected above). + #[test] + fn route_token_keys_would_collide_with_reserved_routes() { + // What url("health")/url("ttl")/url("lock") *would* produce if unguarded, + // parsed with the same url crate reqwest uses. + assert_eq!( + Url::parse(&format!("{API}/v1/cache/health")) + .expect("parse") + .path(), + "/v1/cache/health", // identical to the health endpoint — a "health" key = the health route + ); + for token in ["ttl", "lock"] { + assert_eq!( + Url::parse(&format!("{API}/v1/cache/{token}")) + .expect("parse") + .path(), + format!("/v1/cache/{token}"), // reads as an empty key + sub-resource selector + ); + } + } + /// AC-2 — every non-dot vector builds a URL whose *parsed* path (the real /// wire path, post-normalisation) stays inside `/v1/cache/`. Asserting on the /// unparsed `format!` output would pass while still shipping a traversal, so @@ -435,6 +460,8 @@ mod path_encoding_tests { "default:../../admin", "k?x=1#f", "a b", + "healthy", // route-token near-miss: not reserved, must build fine + "x/../../health", // embedded route token, `/`→`%2F` keeps it one segment "ns:default:func:m.f:args:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef:", ]; for key in vectors { diff --git a/crates/cachekit/src/backend/mod.rs b/crates/cachekit/src/backend/mod.rs index 1dc5476..ccf7723 100644 --- a/crates/cachekit/src/backend/mod.rs +++ b/crates/cachekit/src/backend/mod.rs @@ -192,39 +192,50 @@ pub(crate) async fn run_blocking( /// path is built through this one fallible chokepoint). /// /// Almost every key is just [`urlencoding::encode`]. The exception is a key -/// that is exactly `.` or `..`, which is **rejected** with a permanent -/// [`BackendError`] rather than encoded — because there is no encoding of it -/// that survives the client's URL parser. +/// whose encoded form is one of the **five reserved path segments** — `.`, +/// `..`, `health`, `ttl`, `lock` — which is **rejected** with a permanent +/// [`BackendError`] rather than sent. This is the client's half of the protocol +/// `spec/saas-api.md` § Cache-Key Path Encoding, rule 2 (LAB-2879). /// -/// A dot is RFC-3986 *unreserved*, so `urlencoding::encode("..") == ".."` -/// unchanged, and `reqwest`'s WHATWG URL parser (rust-url) then removes that -/// dot-segment **before the request leaves the process**: `/v1/cache/..` → -/// `/v1/`, `…/../ttl` → `…/ttl`, escaping the `/v1/cache/` prefix the SaaS -/// `cache-key-validator` guards and carrying the app's bearer token to a route -/// it never vetted (CWE-22). Percent-encoding does **not** help here: WHATWG -/// treats `%2e` / `%2e%2e` (case-insensitive) as dot-segments too, and rust-url -/// `%2E%2E` → `/v1/` and `%2E` → `/v1/cache/` are both verified to collapse -/// (see the `repro_raw_dot_key_escapes_the_cache_prefix` test). Since every representation that -/// `decodeURIComponent`s once back to `.`/`..` is a WHATWG dot-segment, no -/// encoding can both reach the wire intact **and** round-trip — so the only safe -/// action is to refuse to build the request at all. +/// Two distinct hazards, both landing the app's bearer token on a route the SaaS +/// `cache-key-validator` never vets (CWE-22): /// -/// This diverges deliberately from cachekit-py's `_encode_key` -/// (`src/cachekit/backends/cachekitio/backend.py:247-250` @ `f000ba3`), which -/// rewrites to `%2E`: Python's HTTP client applies RFC-3986 `remove_dot_segments` -/// (which does **not** decode `%2e`), so `%2E%2E` survives there and the SaaS -/// rejects the decoded `..`. rust-url is stricter. `.`/`..` is never a canonical -/// CacheKit key (those always contain `:`), so refusing it breaks nothing -/// legitimate. For every **other** key the output is byte-identical to -/// `urlencoding::encode`, preserving cross-SDK wire parity. +/// - **Dot segments (`.`, `..`).** A dot is RFC-3986 *unreserved*, so +/// `urlencoding::encode("..") == ".."` unchanged, and `reqwest`'s WHATWG URL +/// parser (rust-url) removes that dot-segment **before the request leaves the +/// process**: `/v1/cache/..` → `/v1/`, `…/../ttl` → `…/ttl`. Percent-encoding +/// does not help: WHATWG treats `%2e` / `%2e%2e` (case-insensitive) as +/// dot-segments too, so `%2E%2E` → `/v1/` and `%2E` → `/v1/cache/` collapse +/// just the same (verified in `repro_raw_dot_key_escapes_the_cache_prefix`). +/// Since every representation that `decodeURIComponent`s once back to `.`/`..` +/// is a WHATWG dot-segment, no encoding both reaches the wire intact and +/// round-trips — the only safe action is to refuse to build the request. +/// - **Route tokens (`health`, `ttl`, `lock`).** These are live path tokens at +/// this level: `/v1/cache/health` IS the health endpoint (see `health_url`), +/// and a trailing `ttl` / `lock` segment selects a sub-resource. A key of +/// exactly one of those words routes elsewhere or reads as an empty key, so +/// the spec reserves them client-side too. +/// +/// Only an *entirely*-reserved segment is caught: `a:..`, `..a`, `x..y` are +/// inert and sent per rule 1 with their dots raw. Canonical and interop keys +/// always contain `:` and never meet this rule, so for every non-reserved key +/// the output is byte-identical to `urlencoding::encode` — preserving cross-SDK +/// wire parity. rust-url's uniform rejection matches the cachekit-ts twin +/// (LAB-2877); it diverges from cachekit-py's older `%2E` rewrite +/// (`src/cachekit/backends/cachekitio/backend.py:247-250` @ `f000ba3`), whose +/// RFC-3986 client kept `%2E%2E` on the wire — the spec now mandates uniform +/// client-side rejection on every stack. #[cfg(any(feature = "cachekitio", feature = "workers", test))] pub(crate) fn encode_key(key: &str) -> Result, BackendError> { let encoded = urlencoding::encode(key); - if matches!(encoded.as_ref(), "." | "..") { + // spec/saas-api.md § Cache-Key Path Encoding rule 2: reject a key whose + // encoded form is exactly one of the five reserved segments. + if matches!(encoded.as_ref(), "." | ".." | "health" | "ttl" | "lock") { return Err(BackendError::permanent( - "cache key must not be `.` or `..`: the client URL parser strips an \ - all-dot path segment before the request is sent (CWE-22), so it \ - cannot be addressed on the wire", + "cache key must not be a reserved path segment (`.`, `..`, `health`, \ + `ttl`, `lock`): the client URL parser or the SaaS router would route \ + it off the `/v1/cache/{key}` path (CWE-22), so it cannot be \ + addressed on the wire", )); } Ok(encoded) @@ -269,28 +280,60 @@ pub mod workers; mod encode_key_tests { use super::encode_key; - /// Non-`.`/`..` vectors: canonical key, near-misses (`..` embedded, not a - /// whole segment), reserved chars, spaces, empty. + /// The five reserved path segments of spec rule 2 — no wire form is + /// transmittable, so a conformant client rejects before building the URL. + const RESERVED_SEGMENTS: &[&str] = &[".", "..", "health", "ttl", "lock"]; + + /// Non-reserved vectors: canonical key, dot near-misses (`..` embedded, not a + /// whole segment), route-token near-misses (`healthy`, `HEALTH`, embedded + /// `x/../../health`), reserved chars, `%`, sub-delims, spaces, empty. Mirrors + /// the transmittable rows of `protocol/test-vectors/path-encoding.json`. const SAFE_VECTORS: &[&str] = &[ "a:..", "..a", "a..", ".hidden", "default:../../admin", + "x/../../health", + "healthy", + "HEALTH", + "ttls", + "unlock", "ns:default:func:m.f:args:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef:", "a b", "k?x=1#f", + "100%", + "f(x)!*'", "", ]; #[test] - fn all_dot_keys_are_rejected() { - // No encoding of `.`/`..` survives rust-url/WHATWG dot-segment removal - // (see `cachekitio::path_encoding_tests::dot_keys_are_rejected_by_every_builder`), - // so the guard refuses to build a request rather than emit one whose path - // was rewritten (CWE-22). - assert!(encode_key(".").is_err()); - assert!(encode_key("..").is_err()); + fn reserved_segments_are_rejected() { + // spec/saas-api.md rule 2: `.`/`..` collapse in the URL parser and + // `health`/`ttl`/`lock` are route tokens; none is addressable on the + // `/v1/cache/{key}` path (see + // `cachekitio::path_encoding_tests::reserved_segments_rejected_by_every_builder`). + for k in RESERVED_SEGMENTS { + assert!( + encode_key(k).is_err(), + "reserved segment {k:?} must be rejected" + ); + } + // Near-misses are NOT reserved — only an exact, whole-segment match is. + for k in [ + "healthy", + "HEALTH", + "ttls", + "unlock", + ".hidden", + "a..", + "x/../../health", + ] { + assert!( + encode_key(k).is_ok(), + "near-miss {k:?} must not be rejected" + ); + } } #[test] diff --git a/crates/cachekit/src/backend/workers.rs b/crates/cachekit/src/backend/workers.rs index 6d19bf1..5e70fda 100644 --- a/crates/cachekit/src/backend/workers.rs +++ b/crates/cachekit/src/backend/workers.rs @@ -55,11 +55,13 @@ impl WorkersCachekitIO { /// Build the full URL for a cache key path segment. /// /// Keys are percent-encoded via [`encode_key`](crate::backend::encode_key); a - /// key that is exactly `.` or `..` is **rejected** (fallible return) rather - /// than encoded, because the Workers runtime `fetch` (WHATWG URL) would strip - /// an all-dot segment out of the `/v1/cache/` prefix before the request is - /// sent (CWE-22) — see [`encode_key`](crate::backend::encode_key). - /// `ttl_url`/`lock_url` build on this, so all three wasm paths inherit the guard. + /// key whose encoded form is a reserved segment (`.`, `..`, `health`, `ttl`, + /// `lock`) is **rejected** (fallible return) rather than sent — the Workers + /// runtime `fetch` (WHATWG URL) strips the dot segments and the route tokens + /// collide with the health/sub-resource routes, both escaping + /// `/v1/cache/{key}` (CWE-22, spec rule 2) — see + /// [`encode_key`](crate::backend::encode_key). `ttl_url`/`lock_url` build on + /// this, so all three wasm paths inherit the guard. fn url(&self, key: &str) -> Result { Ok(format!("{}/v1/cache/{}", self.api_url, encode_key(key)?)) } From 0e277fbcec8dd3e23edc8b750bdcd4c9dfab976b Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Fri, 4 Sep 2026 14:29:23 +1000 Subject: [PATCH 3/3] test(cachekitio): drop two redundant path-encoding tests (panel cut-list) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pragmatism review of the five-segment widening flagged two ceremony tests: - `route_token_keys_would_collide_with_reserved_routes` was a tautology — it asserted `Url::parse(".../v1/cache/health").path() == "/v1/cache/health"`, i.e. that the url crate leaves a non-dot path unchanged. It passes even if the guard is deleted, so it caught nothing. The route-token rationale lives in the `encode_key` doc comment; rejection is asserted by `reserved_segments_rejected_by_every_builder`. - the inline near-miss `is_ok()` loop in `reserved_segments_are_rejected` was triple coverage — those keys are in `SAFE_VECTORS` and already asserted `is_ok()` by `safe_keys_are_byte_identical_to_urlencoding` and `safe_keys_decode_once_back_to_the_original` (a contains/case-insensitive regression panics on their `.expect`). No coverage lost. --- crates/cachekit/src/backend/cachekitio.rs | 23 ----------------------- crates/cachekit/src/backend/mod.rs | 18 +++--------------- 2 files changed, 3 insertions(+), 38 deletions(-) diff --git a/crates/cachekit/src/backend/cachekitio.rs b/crates/cachekit/src/backend/cachekitio.rs index 15e8ac8..09a34ba 100644 --- a/crates/cachekit/src/backend/cachekitio.rs +++ b/crates/cachekit/src/backend/cachekitio.rs @@ -425,29 +425,6 @@ mod path_encoding_tests { } } - /// spec rule 2 — the route tokens collide with real routes: a key of exactly - /// `health` builds the health endpoint's own path, and `ttl`/`lock` build the - /// bare sub-resource paths. This is why they are reserved (rejected above). - #[test] - fn route_token_keys_would_collide_with_reserved_routes() { - // What url("health")/url("ttl")/url("lock") *would* produce if unguarded, - // parsed with the same url crate reqwest uses. - assert_eq!( - Url::parse(&format!("{API}/v1/cache/health")) - .expect("parse") - .path(), - "/v1/cache/health", // identical to the health endpoint — a "health" key = the health route - ); - for token in ["ttl", "lock"] { - assert_eq!( - Url::parse(&format!("{API}/v1/cache/{token}")) - .expect("parse") - .path(), - format!("/v1/cache/{token}"), // reads as an empty key + sub-resource selector - ); - } - } - /// AC-2 — every non-dot vector builds a URL whose *parsed* path (the real /// wire path, post-normalisation) stays inside `/v1/cache/`. Asserting on the /// unparsed `format!` output would pass while still shipping a traversal, so diff --git a/crates/cachekit/src/backend/mod.rs b/crates/cachekit/src/backend/mod.rs index ccf7723..95b15cd 100644 --- a/crates/cachekit/src/backend/mod.rs +++ b/crates/cachekit/src/backend/mod.rs @@ -319,21 +319,9 @@ mod encode_key_tests { "reserved segment {k:?} must be rejected" ); } - // Near-misses are NOT reserved — only an exact, whole-segment match is. - for k in [ - "healthy", - "HEALTH", - "ttls", - "unlock", - ".hidden", - "a..", - "x/../../health", - ] { - assert!( - encode_key(k).is_ok(), - "near-miss {k:?} must not be rejected" - ); - } + // Near-miss acceptance (`healthy`, `HEALTH`, `x/../../health`, …) is + // covered by `SAFE_VECTORS` in the two tests below — a `contains`/ + // case-insensitive regression there panics on `.expect`. } #[test]