fix(cachekitio): reject reserved cache-key segments in request path (LAB-2878) - #76
Conversation
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).
This comment has been minimized.
This comment has been minimized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
WalkthroughCache-key URL construction now percent-encodes keys and rejects exact ChangesCache-key path protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to Cache keys are now encoded and five reserved path segments are rejected, preventing route normalization and endpoint collisions while preserving safe-key behavior. No current merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant CacheOperation
participant Backend
participant encode_key
participant Endpoint
CacheOperation->>Backend: Build cache, lock, or TTL URL
Backend->>encode_key: Encode cache key
encode_key-->>Backend: Encoded key or BackendError
Backend->>Endpoint: Compose guarded endpoint URL
Endpoint-->>CacheOperation: Request URL or BackendError
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 5 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
…9 conformance)
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.
This comment has been minimized.
This comment has been minimized.
…ist)
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.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
…CWE-22, LAB-2877) (#118) ## Summary Reject the five reserved cache-key path segments — `.`, `..`, `health`, `ttl`, `lock` — client-side in the CachekitIO backend, before any URL is built, per protocol `spec/saas-api.md` § Cache-Key Path Encoding rule 2 ([cachekit-io/protocol#61](cachekit-io/protocol#61)). CWE-22 defence-in-depth, and cross-SDK parity with cachekit-rs ([cachekit-io/cachekit-rs#76](cachekit-io/cachekit-rs#76)). - **Shared `encodeKey()`** replaces the five raw `encodeURIComponent(key)` sites (core GET/PUT/DELETE/HEAD, TTL GET/PATCH, lock POST/DELETE) and throws `ConfigurationError` for a reserved segment or malformed UTF-16 (lone surrogate). Every other key is exactly `encodeURIComponent(key)`. - **URL construction hoisted above each network `try`**, so the `ConfigurationError` reaches the caller unwrapped instead of being re-thrown as `BackendError` (CodeRabbit finding). Same pattern `refreshTTL` already used for `validateTtl`. - **`Backend.validateKey?` capability**, mirroring `validateTtl`: `CachekitIOCore` implements it, the TTL / Lockable / combined wrappers forward it, and `CacheImpl` calls it synchronously in `get` / `set` / `delete` / `exists` before the reliability executor. Without it the public `createCache(...).get('health')` path retried the deterministic error, counted it against the circuit breaker (five reserved keys in 60 s opened the breaker and blackholed legitimate keys), then degraded it into a silent miss / no-store (expert-panel finding). - **Tests** move to the protocol lane (`test/protocol/path-encoding.protocol.test.ts`, 148 tests) and drive the real `CachekitIOCore` / `TTLCachekitIO` / `LockableCachekitIO` through a fetch spy, asserting on the WHATWG-parsed `new URL(url).pathname` that `fetch` received. Vectors are the vendored `protocol/test-vectors/path-encoding.json` v1.0.0 (15 rows, 5 reject). A `cache.test.ts` regression pins the `validateKey` pre-flight beside the existing `validateTtl` one. - **SECURITY.md** gains a "Cache-Key Path Encoding (CWE-22)" section. ## Why reject rather than encode (AC-0 repro) `.` is RFC-3986 unreserved, so `encodeURIComponent('..') === '..'`, and the WHATWG parser behind `fetch` removes the dot segment before the request leaves the process: ```js new URL('https://api.cachekit.io/v1/cache/..').pathname // '/v1/' new URL('https://api.cachekit.io/v1/cache/../ttl').pathname // '/v1/ttl' new URL('https://api.cachekit.io/v1/cache/%2E%2E/lock').pathname // '/v1/lock' (%2E does not help) ``` The SaaS worker parses the request URL with WHATWG `new URL()` too, so `%2E%2E` collapses server-side even from an RFC-3986 client (spec evidence: `GET /v1/cache/%2E%2E/health` returns the health payload). No wire form of `.` / `..` reaches the key validator from any client, so the spec mandates client-side rejection on every stack. `health`, `ttl`, `lock` are route tokens at the same level: `/v1/cache/health` is the health endpoint, and a trailing `ttl` / `lock` selects a sub-resource with an empty key. The SaaS router matches them exactly and case-sensitively (`apps/cache/src/index.ts:509,655`), so only the lowercase words are reserved; `HEALTH`, `ttls`, `a:..`, `..a` transmit unchanged. ## Cross-SDK position (AC-4) - **cachekit-rs** ([cachekit-io/cachekit-rs#76](cachekit-io/cachekit-rs#76), merged) rejects the same five tokens in `encode_key`. Same behaviour; ts and rs are **decode-equivalent, not byte-identical**: `urlencoding::encode` escapes `! * ' ( )` where `encodeURIComponent` leaves them raw (spec rule 4, fixture `encoded_alternates`). The SaaS decodes both to the same key, and every key the server accepts is drawn from `[A-Za-z0-9_.:-]`, on which all encoders agree, so canonical and interop keys are byte-identical on the wire across SDKs. - **cachekit-py** (`_encode_key` @ `f000ba3`) still rewrites `.` / `..` to `%2E`, which only moves the collapse to the server; a py follow-up ticket tracks the switch to rejection. For every non-reserved key, py and ts are decode-equivalent as above. ## Test plan - [x] AC-0 repro: raw dots and `%2E` collapse under `new URL()`, pinned as the design premise - [x] AC-1 (as amended by the spec): `encodeKey` rejects the five reserved segments; identity with `encodeURIComponent` for every transmittable vector and for near-misses - [x] AC-2: 8 operations × 10 transmittable vectors assert the exact WHATWG-parsed pathname inside `/v1/cache/`; 8 operations × 5 reserved keys reject with `ConfigurationError` and never call `fetch` - [x] AC-3: decode-once round-trip asserted on the real wire path for every vector - [x] AC-4: this section - [x] AC-5: SECURITY.md - [x] AC-6: expert panel at high stakes, post-spec — FIX-FIRST, every finding applied (detail on the ticket) - [x] `validateKey` pre-flight regression through `createCache` (`cache.test.ts`) Local: eslint, prettier, tsc clean; 148/148 protocol tests; full suite 899 pass with 16 failures confined to key-rotation / bin-envelope tests that need the not-yet-published core-ts 0.1.3 native binding (no cargo in this workdir, so the 0.1.2 npm binary stood in). Closes LAB-2877 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added safer cache-key handling for path-based operations. * Invalid, reserved, or malformed keys are rejected before network requests. * Added validation across standard cache, TTL, and locking operations. * Documented cache-key encoding and validation rules. * **Bug Fixes** * Prevented path traversal and URL normalisation issues during cache operations. * Ensured invalid keys fail immediately rather than being retried as backend errors. * **Tests** * Added coverage for key encoding, reserved keys, traversal attempts, and related cache operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Winston <ray@insighttimer.com>
LAB-2878 — security(cachekitio): reject all-dot cache-key segment (
./..) in the request path (CWE-22)Closes LAB-2878.
The finding: the prescribed Python-parity fix does not work in Rust
LAB-2878 asked to mirror cachekit-py's
f000ba3fix — rewrite an all-dot key./..to%2E/%2E%2Eso it survives to the wire and the SaaS rejects the decoded... AC-0 ("repro first") required verifying this againstreqwest. It does not hold.reqwestparses the URL string with theurlcrate (rust-url 2.5.8), which implements the WHATWG URL Standard. WHATWG treats%2e/%2e%2e(case-insensitive) as dot-segments and removes them — so%2E%2Ecollapses just like a raw... Verified at the reqwest layer:Python's fix works only because httpx/requests apply RFC-3986
remove_dot_segments, which does not decode%2e. rust-url is stricter. This is exactly the "verify reqwest" question the Python commit flagged, and it matches the protocol resolution in LAB-2879 (WHATWG stacks — fetch/undici/Workers/rust-url — cannot carry an encoded all-dot key intact).Since every representation that
decodeURIComponents once back to./..is a WHATWG dot-segment, no encoding can both reach the wire intact and round-trip. The only safe behaviour is to refuse to build the request.The fix: reject, don't encode (aligns with cachekit-ts twin)
One shared fallible chokepoint in
crates/cachekit/src/backend/mod.rs:Resultis threaded through the URL buildersurl/ttl_url/lock_url(nativecachekitio.rsand wasmworkers.rs) and their ~8 request callers, so every CachekitIO path — base,/ttl,/lock, native and wasm — is type-forced through the one guard; a./..key yields a permanentBackendErrorand no authenticated request is ever emitted. Every other key encodes byte-identically to before.This matches the sibling cachekit-ts decision (LAB-2877, cachekit-io/cachekit-ts#118), which rejects
./..with aConfigurationError.Cross-SDK wire position (AC-4)
./..behaviour%2E/%2E%2E; SaaS rejects decoded..For every key except
./.., cachekit-rs is byte-identical to cachekit-py on the wire (urlencoding::encode≡quote(safe="")on the reserved set)../..is never a canonical CacheKit key (those always contain:), so rejection breaks nothing legitimate. Reference: cachekit-py_encode_key(src/cachekit/backends/cachekitio/backend.py:247-250@f000ba3); SaaS validatorsaas/apps/cache/src/cache-key-validator.ts(single decode, charset[a-zA-Z0-9_.:-], rejects any key containing..).Tests (AC-0/1/2/3)
repro_raw_dot_key_escapes_the_cache_prefix— proves raw..(and the%2Eform) collapse in rust-url to/v1/,/v1/ttl,/v1/lock,/v1/cache/.dot_keys_are_rejected_by_every_builder+safe_keys_never_escape_the_cache_prefix— assert on the parsedUrl::path()(the real post-normalisation wire path) for base/ttl/lock builders across.,..,a:..,default:../../admin,k?x=1#f,a b, canonicalns:….safe_keys_are_byte_identical_to_urlencoding— parity withurlencoding::encodefor every non-dot vector.safe_keys_decode_once_back_to_the_original— SaaS single-decodeURIComponentround-trip.Docs (AC-5)
README.mdSecurity Properties: new "Cache-key path encoding (CWE-22)" row + paragraph stating the reject behaviour and the py divergence.fn url/encode_keydoc comments carry the WHATWG/CWE-22 rationale so a later dev doesn't "harmonize" back to%2Eand silently reintroduce the gap.Quality gates
cargo clippy --all-targets --features "cachekitio,redis,encryption,l1,macros,memcached,file" -- -D warnings— cleancargo test --features "cachekitio,redis,encryption,l1,macros,memcached,file"— all passcargo check --target wasm32-unknown-unknown --features workers,encryption --no-default-features— compiles (no new warnings)cargo fmt --check— cleanExpert-panel review (AC-6)
Ran at high stakes (crypto/protocol wire gate). bug-hunter and security-specialist returned NO FINDINGS (security independently re-derived the guard-completeness proof from the
urlencodingsource). code-craftsman flagged two doc issues (phantom test-name references; native/wasm path-construction asymmetry) — both applied. catchphrase-agent proposed cutting the decode-round-trip test — rebutted, it's mandated by AC-3 and documents the SaaS single-decode contract. Verdict: SHIP.Summary by CodeRabbit
Security
.,..,health,ttl, andlockkeys are rejected to prevent path normalisation and route conflicts.Bug Fixes
Documentation