From 04dde7889b77643581a9ee7ca287ce06f7f30718 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Fri, 4 Sep 2026 13:00:30 +1000 Subject: [PATCH 1/6] fix(cachekitio): reject dot-segment cache keys to prevent path traversal (CWE-22, LAB-2877) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cache key of exactly '.' or '..' triggers WHATWG URL Standard dot-segment removal in fetch/undici, causing the authenticated request to escape the /v1/cache/ prefix. Python's fix (encode to %2E) does not work in JS because WHATWG treats %2E identically to '.' for path normalization. Reject these keys with ConfigurationError instead — fail-fast over silent misdirection. - Add shared encodeKey() replacing 5 raw encodeURIComponent() call sites - 43 tests: repro, WHATWG %2E proof, rejection, pathname assertions, round-trip - SECURITY.md: document the CWE-22 encoding note --- SECURITY.md | 4 + .../src/backends/cachekitio-lockable.ts | 6 +- .../backends/cachekitio-path-encoding.test.ts | 157 ++++++++++++++++++ .../cachekit/src/backends/cachekitio-ttl.ts | 6 +- packages/cachekit/src/backends/cachekitio.ts | 30 +++- 5 files changed, 196 insertions(+), 7 deletions(-) create mode 100644 packages/cachekit/src/backends/cachekitio-path-encoding.test.ts diff --git a/SECURITY.md b/SECURITY.md index 241a74e..3afd4db 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -14,6 +14,10 @@ Instead, use [GitHub's private vulnerability reporting](https://github.com/cache | ------- | --------- | | 0.x | Yes | +## Cache-Key Path Encoding (CWE-22) + +The CachekitIO backend transmits cache keys as a single URL path segment (`/v1/cache/{key}`). Keys are percent-encoded with `encodeURIComponent`; keys that are exactly `.` or `..` are rejected with a `ConfigurationError` because the WHATWG URL Standard (used by `fetch` / undici / Cloudflare Workers) treats both literal and percent-encoded dots (`%2E`) as dot-segments and removes them before the request reaches the wire. + ## Scope This policy covers the `@cachekit-io/cachekit` and `@cachekit-io/cachekit-core-ts` packages. For issues with the CacheKit SaaS platform (api.cachekit.io), contact security@cachekit.io. diff --git a/packages/cachekit/src/backends/cachekitio-lockable.ts b/packages/cachekit/src/backends/cachekitio-lockable.ts index 4c02adc..dba3e8f 100644 --- a/packages/cachekit/src/backends/cachekitio-lockable.ts +++ b/packages/cachekit/src/backends/cachekitio-lockable.ts @@ -1,5 +1,5 @@ import type { LockableBackend } from './types.js'; -import { CachekitIOCore } from './cachekitio.js'; +import { CachekitIOCore, encodeKey } from './cachekitio.js'; import { BackendError, TimeoutError } from '../errors.js'; import { classifyHttpError, classifyNetworkError } from './error-classifier.js'; @@ -45,7 +45,7 @@ export class LockableCachekitIO implements LockableBackend { async acquireLock(key: string, timeoutMs = 5000): Promise { try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeURIComponent(key)}/lock`; + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/lock`; const response = await this.inner.requestJson('POST', url, { timeout_ms: timeoutMs }); // Contested lock: the protocol spec (saas-api.md) answers 409 Conflict; // the deployed SaaS currently answers 200 {lock_id: null}. Both mean @@ -76,7 +76,7 @@ export class LockableCachekitIO implements LockableBackend { async releaseLock(key: string, lockId: string): Promise { try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeURIComponent(key)}/lock`; + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/lock`; const response = await this.inner.requestJson('DELETE', url, undefined, { [LOCK_ID_HEADER]: lockId, }); diff --git a/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts b/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts new file mode 100644 index 0000000..968107f --- /dev/null +++ b/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts @@ -0,0 +1,157 @@ +import { describe, it, expect } from 'vitest'; +import { encodeKey } from './cachekitio.js'; +import { ConfigurationError } from '../errors.js'; + +const BASE = 'https://api.cachekit.io'; + +const cacheUrl = (key: string) => `${BASE}/v1/cache/${encodeKey(key)}`; +const ttlUrl = (key: string) => `${BASE}/v1/cache/${encodeKey(key)}/ttl`; +const lockUrl = (key: string) => `${BASE}/v1/cache/${encodeKey(key)}/lock`; + +// AC-0 — Repro: raw encodeURIComponent lets dot-segments escape /v1/cache/ +describe('AC-0: dot-segment traversal repro (pre-fix behavior)', () => { + it('"." collapses to /v1/cache/ with raw encodeURIComponent', () => { + const raw = `${BASE}/v1/cache/${encodeURIComponent('.')}`; + expect(new URL(raw).pathname).toBe('/v1/cache/'); + }); + + it('".." escapes /v1/cache/ with raw encodeURIComponent', () => { + const raw = `${BASE}/v1/cache/${encodeURIComponent('..')}`; + expect(new URL(raw).pathname).toBe('/v1/'); + }); + + it('"../ttl" path collapses to /v1/ttl', () => { + const raw = `${BASE}/v1/cache/${encodeURIComponent('..')}/ttl`; + expect(new URL(raw).pathname).toBe('/v1/ttl'); + }); + + it('"../lock" path collapses to /v1/lock', () => { + const raw = `${BASE}/v1/cache/${encodeURIComponent('..')}/lock`; + expect(new URL(raw).pathname).toBe('/v1/lock'); + }); + + // Proves that %2E encoding (Python's approach) does NOT survive WHATWG + // URL normalization — justifies the rejection approach for TS. + it('%2E is also collapsed by WHATWG URL parser (unlike RFC-3986)', () => { + expect(new URL(`${BASE}/v1/cache/%2E`).pathname).toBe('/v1/cache/'); + expect(new URL(`${BASE}/v1/cache/%2E%2E`).pathname).toBe('/v1/'); + expect(new URL(`${BASE}/v1/cache/%2E%2E/ttl`).pathname).toBe('/v1/ttl'); + expect(new URL(`${BASE}/v1/cache/%2E%2E/lock`).pathname).toBe('/v1/lock'); + }); +}); + +// AC-1 — encodeKey rejects bare dot-segments, passes everything else through +describe('AC-1: encodeKey helper', () => { + it('rejects "." with ConfigurationError', () => { + expect(() => encodeKey('.')).toThrow(ConfigurationError); + expect(() => encodeKey('.')).toThrow(/CWE-22/); + }); + + it('rejects ".." with ConfigurationError', () => { + expect(() => encodeKey('..')).toThrow(ConfigurationError); + expect(() => encodeKey('..')).toThrow(/CWE-22/); + }); + + const passthrough: [string, string][] = [ + ['a:..', 'key containing dots is not all-dot'], + ['..a', 'prefix dots with trailing alpha'], + [`ns:default:func:m.f:args:${'a'.repeat(64)}:`, 'canonical 7-segment key'], + ['a b', 'space-containing key'], + ['k?x=1#f', 'query/fragment characters'], + ['...', 'triple dot is not a traversal segment'], + ]; + + for (const [key, label] of passthrough) { + it(`${label} ("${key}") matches encodeURIComponent`, () => { + expect(encodeKey(key)).toBe(encodeURIComponent(key)); + }); + } +}); + +// AC-2 — Post-normalisation pathname stays inside /v1/cache/ for all safe vectors +describe('AC-2: URL pathname assertions', () => { + const safeVectors: [string, string][] = [ + ['a:..', `/v1/cache/${encodeURIComponent('a:..')}`], + ['default:../../admin', `/v1/cache/${encodeURIComponent('default:../../admin')}`], + ['k?x=1#f', `/v1/cache/${encodeURIComponent('k?x=1#f')}`], + ['a b', `/v1/cache/${encodeURIComponent('a b')}`], + [ + `ns:default:func:m.f:args:${'a'.repeat(64)}:`, + `/v1/cache/${encodeURIComponent(`ns:default:func:m.f:args:${'a'.repeat(64)}:`)}`, + ], + ['...', `/v1/cache/${encodeURIComponent('...')}`], + ]; + + describe('base backend (/v1/cache/{key})', () => { + for (const [key, expectedPath] of safeVectors) { + it(`key "${key}" → ${expectedPath}`, () => { + const parsed = new URL(cacheUrl(key)); + expect(parsed.pathname).toBe(expectedPath); + expect(parsed.pathname.startsWith('/v1/cache/')).toBe(true); + }); + } + }); + + describe('TTL wrapper (/v1/cache/{key}/ttl)', () => { + for (const [key, expectedPath] of safeVectors) { + it(`key "${key}" → ${expectedPath}/ttl`, () => { + const parsed = new URL(ttlUrl(key)); + expect(parsed.pathname).toBe(`${expectedPath}/ttl`); + expect(parsed.pathname.startsWith('/v1/cache/')).toBe(true); + }); + } + }); + + describe('lockable wrapper (/v1/cache/{key}/lock)', () => { + for (const [key, expectedPath] of safeVectors) { + it(`key "${key}" → ${expectedPath}/lock`, () => { + const parsed = new URL(lockUrl(key)); + expect(parsed.pathname).toBe(`${expectedPath}/lock`); + expect(parsed.pathname.startsWith('/v1/cache/')).toBe(true); + }); + } + }); + + it('dot-segment keys are rejected before URL construction', () => { + for (const key of ['.', '..']) { + for (const builder of [cacheUrl, ttlUrl, lockUrl]) { + expect(() => builder(key)).toThrow(ConfigurationError); + } + } + }); + + it('no safe vector escapes /v1/cache/ prefix', () => { + for (const [key] of safeVectors) { + for (const builder of [cacheUrl, ttlUrl, lockUrl]) { + const pathname = new URL(builder(key)).pathname; + expect(pathname.startsWith('/v1/cache/')).toBe(true); + } + } + }); +}); + +// AC-3 — Decode-once round-trip for all safe keys +describe('AC-3: decode-once round-trip', () => { + const keys = [ + 'a:..', + '..a', + 'default:../../admin', + '...', + `ns:default:func:m.f:args:${'a'.repeat(64)}:`, + 'a b', + 'k?x=1#f', + 'hello', + '', + ]; + + for (const key of keys) { + it(`round-trips "${key}"`, () => { + expect(decodeURIComponent(encodeKey(key))).toBe(key); + }); + } + + it('dot-segment keys cannot round-trip (rejected)', () => { + expect(() => encodeKey('.')).toThrow(); + expect(() => encodeKey('..')).toThrow(); + }); +}); diff --git a/packages/cachekit/src/backends/cachekitio-ttl.ts b/packages/cachekit/src/backends/cachekitio-ttl.ts index 494b76f..ec895db 100644 --- a/packages/cachekit/src/backends/cachekitio-ttl.ts +++ b/packages/cachekit/src/backends/cachekitio-ttl.ts @@ -1,5 +1,5 @@ import type { TTLBackend } from './types.js'; -import { CachekitIOCore, validateTtl } from './cachekitio.js'; +import { CachekitIOCore, validateTtl, encodeKey } from './cachekitio.js'; import { BackendError, TimeoutError } from '../errors.js'; import { classifyHttpError, classifyNetworkError } from './error-classifier.js'; @@ -37,7 +37,7 @@ export class TTLCachekitIO implements TTLBackend { async getTTL(key: string): Promise { try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeURIComponent(key)}/ttl`; + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/ttl`; const response = await this.inner.requestJson('GET', url); if (response.status === 404) return null; if (!response.ok) @@ -67,7 +67,7 @@ export class TTLCachekitIO implements TTLBackend { // Before the try — the catch below would wrap it as a BackendError. const validTtl = validateTtl(ttl); try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeURIComponent(key)}/ttl`; + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/ttl`; const response = await this.inner.requestJson('PATCH', url, { ttl: validTtl }); if (response.status === 404) return false; if (!response.ok) diff --git a/packages/cachekit/src/backends/cachekitio.ts b/packages/cachekit/src/backends/cachekitio.ts index 2d97553..c5690c5 100644 --- a/packages/cachekit/src/backends/cachekitio.ts +++ b/packages/cachekit/src/backends/cachekitio.ts @@ -6,6 +6,34 @@ import { buildMetricsHeaders } from './metrics-headers.js'; import { classifyHttpError, classifyNetworkError } from './error-classifier.js'; import { validateCachekitUrl } from './url-validator.js'; +/** + * Percent-encode a cache key for use as a single URL path segment. + * + * `encodeURIComponent` leaves `.` untouched (RFC-3986 unreserved), so a key + * of exactly `.` or `..` triggers dot-segment removal in the HTTP client's + * URL parser — the request escapes /v1/cache/ before it hits the wire + * (CWE-22). + * + * Python's fix encodes dots to `%2E`, which works because Python HTTP + * clients (httpx/requests) use RFC-3986 where `%2E` is opaque. In JS, + * `fetch`/undici parse URLs per the WHATWG URL Standard, which treats `%2E` + * identically to `.` for dot-segment removal — there is no percent-encoding + * of `.` that survives WHATWG normalisation in a path segment. We reject + * these keys instead: fail-fast with a clear error rather than silently + * sending an authenticated request to the wrong path. + */ +export function encodeKey(key: string): string { + const encoded = encodeURIComponent(key); + if (encoded === '.' || encoded === '..') { + throw new ConfigurationError( + `Cache key "${key}" is a bare dot-segment and cannot be used as a URL path segment — ` + + `the WHATWG URL parser (used by fetch) would collapse it, sending the request outside ` + + `/v1/cache/ (CWE-22). Use a namespaced key instead.` + ); + } + return encoded; +} + const DEFAULT_API_URL = 'https://api.cachekit.io'; const DEFAULT_TIMEOUT_MS = 30_000; @@ -199,7 +227,7 @@ export class CachekitIOCore implements Backend { // ── Internal ────────────────────────────────────────────── private cacheUrl(key: string): string { - return `${this.apiUrl}/v1/cache/${encodeURIComponent(key)}`; + return `${this.apiUrl}/v1/cache/${encodeKey(key)}`; } private async request( From 367be29c72f1025fc5b7519226e602400bfefb1d Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Fri, 4 Sep 2026 13:05:51 +1000 Subject: [PATCH 2/6] test(cachekitio): remove redundant summary assertion (expert-panel cut) The "no safe vector escapes /v1/cache/ prefix" test re-asserts what every individual AC-2 parameterized test already checks. Removed per catchphrase-agent finding in expert-panel review. --- .../src/backends/cachekitio-path-encoding.test.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts b/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts index 968107f..d5fffed 100644 --- a/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts +++ b/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts @@ -119,15 +119,6 @@ describe('AC-2: URL pathname assertions', () => { } } }); - - it('no safe vector escapes /v1/cache/ prefix', () => { - for (const [key] of safeVectors) { - for (const builder of [cacheUrl, ttlUrl, lockUrl]) { - const pathname = new URL(builder(key)).pathname; - expect(pathname.startsWith('/v1/cache/')).toBe(true); - } - } - }); }); // AC-3 — Decode-once round-trip for all safe keys From 0984c33d126b9b8f6c369a7583af6d3946b040a7 Mon Sep 17 00:00:00 2001 From: Winston Date: Mon, 7 Sep 2026 09:28:28 +1000 Subject: [PATCH 3/6] fix(cachekitio): reject all five reserved key segments, surface ConfigurationError unwrapped (LAB-2877) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Protocol spec/saas-api.md § Cache-Key Path Encoding rule 2 (protocol#61) reserves `.`, `..`, `health`, `ttl`, `lock`: the dot segments collapse under WHATWG parsing (client-side in fetch, server-side in the worker, `%2E` included) and the three route tokens collide with live routes at the /v1/cache/ level. encodeKey now rejects all five, case-sensitive and exact, matching the SaaS router and the cachekit-rs twin (cachekit-rs#76). URL construction moves above each network `try` (core get/set/delete/exists, TTL getTTL/refreshTTL, lock acquire/release) so the ConfigurationError reaches the caller instead of being wrapped as a BackendError (CodeRabbit finding on cachekitio.ts:230). The ttl wrapper already hoisted validateTtl for the same reason; this follows that pattern. Tests move to the protocol lane and drive the real backend classes through a fetch spy, asserting on the WHATWG-parsed pathname that fetch received rather than on the template string. Vectors are the vendored protocol/test-vectors/path-encoding.json v1.0.0 (15 rows, 5 reject). --- SECURITY.md | 2 +- .../src/backends/cachekitio-lockable.ts | 6 +- .../backends/cachekitio-path-encoding.test.ts | 148 ---------------- .../cachekit/src/backends/cachekitio-ttl.ts | 7 +- packages/cachekit/src/backends/cachekitio.ts | 56 +++--- .../test/protocol/fixtures/path-encoding.json | 105 ++++++++++++ .../protocol/path-encoding.protocol.test.ts | 161 ++++++++++++++++++ 7 files changed, 311 insertions(+), 174 deletions(-) delete mode 100644 packages/cachekit/src/backends/cachekitio-path-encoding.test.ts create mode 100644 packages/cachekit/test/protocol/fixtures/path-encoding.json create mode 100644 packages/cachekit/test/protocol/path-encoding.protocol.test.ts diff --git a/SECURITY.md b/SECURITY.md index 3afd4db..8ff7577 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -16,7 +16,7 @@ Instead, use [GitHub's private vulnerability reporting](https://github.com/cache ## Cache-Key Path Encoding (CWE-22) -The CachekitIO backend transmits cache keys as a single URL path segment (`/v1/cache/{key}`). Keys are percent-encoded with `encodeURIComponent`; keys that are exactly `.` or `..` are rejected with a `ConfigurationError` because the WHATWG URL Standard (used by `fetch` / undici / Cloudflare Workers) treats both literal and percent-encoded dots (`%2E`) as dot-segments and removes them before the request reaches the wire. +The CachekitIO backend transmits cache keys as a single URL path segment (`/v1/cache/{key}`, `…/{key}/ttl`, `…/{key}/lock`). Keys are percent-encoded with `encodeURIComponent`; a key that is exactly `.`, `..`, `health`, `ttl` or `lock` is rejected with a `ConfigurationError` before any request is built ([protocol `spec/saas-api.md` § Cache-Key Path Encoding](https://github.com/cachekit-io/protocol/blob/main/spec/saas-api.md#cache-key-path-encoding), rule 2): the WHATWG URL parser behind `fetch` removes literal and percent-encoded (`%2E`) dot segments before the request reaches the wire, and the other three words are live route tokens at that path level. Every other key — including `a:..` and every canonical `ns:…` key — is sent unchanged and decodes once server-side to the original key. ## Scope diff --git a/packages/cachekit/src/backends/cachekitio-lockable.ts b/packages/cachekit/src/backends/cachekitio-lockable.ts index dba3e8f..f87a5ab 100644 --- a/packages/cachekit/src/backends/cachekitio-lockable.ts +++ b/packages/cachekit/src/backends/cachekitio-lockable.ts @@ -44,8 +44,9 @@ export class LockableCachekitIO implements LockableBackend { } async acquireLock(key: string, timeoutMs = 5000): Promise { + // Before the try — the catch below would wrap encodeKey's ConfigurationError as a BackendError. + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/lock`; try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/lock`; const response = await this.inner.requestJson('POST', url, { timeout_ms: timeoutMs }); // Contested lock: the protocol spec (saas-api.md) answers 409 Conflict; // the deployed SaaS currently answers 200 {lock_id: null}. Both mean @@ -75,8 +76,9 @@ export class LockableCachekitIO implements LockableBackend { } async releaseLock(key: string, lockId: string): Promise { + // Before the try — the catch below would wrap encodeKey's ConfigurationError as a BackendError. + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/lock`; try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/lock`; const response = await this.inner.requestJson('DELETE', url, undefined, { [LOCK_ID_HEADER]: lockId, }); diff --git a/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts b/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts deleted file mode 100644 index d5fffed..0000000 --- a/packages/cachekit/src/backends/cachekitio-path-encoding.test.ts +++ /dev/null @@ -1,148 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { encodeKey } from './cachekitio.js'; -import { ConfigurationError } from '../errors.js'; - -const BASE = 'https://api.cachekit.io'; - -const cacheUrl = (key: string) => `${BASE}/v1/cache/${encodeKey(key)}`; -const ttlUrl = (key: string) => `${BASE}/v1/cache/${encodeKey(key)}/ttl`; -const lockUrl = (key: string) => `${BASE}/v1/cache/${encodeKey(key)}/lock`; - -// AC-0 — Repro: raw encodeURIComponent lets dot-segments escape /v1/cache/ -describe('AC-0: dot-segment traversal repro (pre-fix behavior)', () => { - it('"." collapses to /v1/cache/ with raw encodeURIComponent', () => { - const raw = `${BASE}/v1/cache/${encodeURIComponent('.')}`; - expect(new URL(raw).pathname).toBe('/v1/cache/'); - }); - - it('".." escapes /v1/cache/ with raw encodeURIComponent', () => { - const raw = `${BASE}/v1/cache/${encodeURIComponent('..')}`; - expect(new URL(raw).pathname).toBe('/v1/'); - }); - - it('"../ttl" path collapses to /v1/ttl', () => { - const raw = `${BASE}/v1/cache/${encodeURIComponent('..')}/ttl`; - expect(new URL(raw).pathname).toBe('/v1/ttl'); - }); - - it('"../lock" path collapses to /v1/lock', () => { - const raw = `${BASE}/v1/cache/${encodeURIComponent('..')}/lock`; - expect(new URL(raw).pathname).toBe('/v1/lock'); - }); - - // Proves that %2E encoding (Python's approach) does NOT survive WHATWG - // URL normalization — justifies the rejection approach for TS. - it('%2E is also collapsed by WHATWG URL parser (unlike RFC-3986)', () => { - expect(new URL(`${BASE}/v1/cache/%2E`).pathname).toBe('/v1/cache/'); - expect(new URL(`${BASE}/v1/cache/%2E%2E`).pathname).toBe('/v1/'); - expect(new URL(`${BASE}/v1/cache/%2E%2E/ttl`).pathname).toBe('/v1/ttl'); - expect(new URL(`${BASE}/v1/cache/%2E%2E/lock`).pathname).toBe('/v1/lock'); - }); -}); - -// AC-1 — encodeKey rejects bare dot-segments, passes everything else through -describe('AC-1: encodeKey helper', () => { - it('rejects "." with ConfigurationError', () => { - expect(() => encodeKey('.')).toThrow(ConfigurationError); - expect(() => encodeKey('.')).toThrow(/CWE-22/); - }); - - it('rejects ".." with ConfigurationError', () => { - expect(() => encodeKey('..')).toThrow(ConfigurationError); - expect(() => encodeKey('..')).toThrow(/CWE-22/); - }); - - const passthrough: [string, string][] = [ - ['a:..', 'key containing dots is not all-dot'], - ['..a', 'prefix dots with trailing alpha'], - [`ns:default:func:m.f:args:${'a'.repeat(64)}:`, 'canonical 7-segment key'], - ['a b', 'space-containing key'], - ['k?x=1#f', 'query/fragment characters'], - ['...', 'triple dot is not a traversal segment'], - ]; - - for (const [key, label] of passthrough) { - it(`${label} ("${key}") matches encodeURIComponent`, () => { - expect(encodeKey(key)).toBe(encodeURIComponent(key)); - }); - } -}); - -// AC-2 — Post-normalisation pathname stays inside /v1/cache/ for all safe vectors -describe('AC-2: URL pathname assertions', () => { - const safeVectors: [string, string][] = [ - ['a:..', `/v1/cache/${encodeURIComponent('a:..')}`], - ['default:../../admin', `/v1/cache/${encodeURIComponent('default:../../admin')}`], - ['k?x=1#f', `/v1/cache/${encodeURIComponent('k?x=1#f')}`], - ['a b', `/v1/cache/${encodeURIComponent('a b')}`], - [ - `ns:default:func:m.f:args:${'a'.repeat(64)}:`, - `/v1/cache/${encodeURIComponent(`ns:default:func:m.f:args:${'a'.repeat(64)}:`)}`, - ], - ['...', `/v1/cache/${encodeURIComponent('...')}`], - ]; - - describe('base backend (/v1/cache/{key})', () => { - for (const [key, expectedPath] of safeVectors) { - it(`key "${key}" → ${expectedPath}`, () => { - const parsed = new URL(cacheUrl(key)); - expect(parsed.pathname).toBe(expectedPath); - expect(parsed.pathname.startsWith('/v1/cache/')).toBe(true); - }); - } - }); - - describe('TTL wrapper (/v1/cache/{key}/ttl)', () => { - for (const [key, expectedPath] of safeVectors) { - it(`key "${key}" → ${expectedPath}/ttl`, () => { - const parsed = new URL(ttlUrl(key)); - expect(parsed.pathname).toBe(`${expectedPath}/ttl`); - expect(parsed.pathname.startsWith('/v1/cache/')).toBe(true); - }); - } - }); - - describe('lockable wrapper (/v1/cache/{key}/lock)', () => { - for (const [key, expectedPath] of safeVectors) { - it(`key "${key}" → ${expectedPath}/lock`, () => { - const parsed = new URL(lockUrl(key)); - expect(parsed.pathname).toBe(`${expectedPath}/lock`); - expect(parsed.pathname.startsWith('/v1/cache/')).toBe(true); - }); - } - }); - - it('dot-segment keys are rejected before URL construction', () => { - for (const key of ['.', '..']) { - for (const builder of [cacheUrl, ttlUrl, lockUrl]) { - expect(() => builder(key)).toThrow(ConfigurationError); - } - } - }); -}); - -// AC-3 — Decode-once round-trip for all safe keys -describe('AC-3: decode-once round-trip', () => { - const keys = [ - 'a:..', - '..a', - 'default:../../admin', - '...', - `ns:default:func:m.f:args:${'a'.repeat(64)}:`, - 'a b', - 'k?x=1#f', - 'hello', - '', - ]; - - for (const key of keys) { - it(`round-trips "${key}"`, () => { - expect(decodeURIComponent(encodeKey(key))).toBe(key); - }); - } - - it('dot-segment keys cannot round-trip (rejected)', () => { - expect(() => encodeKey('.')).toThrow(); - expect(() => encodeKey('..')).toThrow(); - }); -}); diff --git a/packages/cachekit/src/backends/cachekitio-ttl.ts b/packages/cachekit/src/backends/cachekitio-ttl.ts index ec895db..38084c3 100644 --- a/packages/cachekit/src/backends/cachekitio-ttl.ts +++ b/packages/cachekit/src/backends/cachekitio-ttl.ts @@ -36,8 +36,9 @@ export class TTLCachekitIO implements TTLBackend { } async getTTL(key: string): Promise { + // Before the try — the catch below would wrap encodeKey's ConfigurationError as a BackendError. + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/ttl`; try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/ttl`; const response = await this.inner.requestJson('GET', url); if (response.status === 404) return null; if (!response.ok) @@ -64,10 +65,10 @@ export class TTLCachekitIO implements TTLBackend { async refreshTTL(key: string, ttl: number): Promise { // Same normative rules as X-CacheKit-TTL (spec: PATCH body follows them). - // Before the try — the catch below would wrap it as a BackendError. + // Both before the try — the catch below would wrap their ConfigurationError as a BackendError. const validTtl = validateTtl(ttl); + const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/ttl`; try { - const url = `${this.inner['apiUrl']}/v1/cache/${encodeKey(key)}/ttl`; const response = await this.inner.requestJson('PATCH', url, { ttl: validTtl }); if (response.status === 404) return false; if (!response.ok) diff --git a/packages/cachekit/src/backends/cachekitio.ts b/packages/cachekit/src/backends/cachekitio.ts index c5690c5..2c4ecc7 100644 --- a/packages/cachekit/src/backends/cachekitio.ts +++ b/packages/cachekit/src/backends/cachekitio.ts @@ -7,28 +7,38 @@ import { classifyHttpError, classifyNetworkError } from './error-classifier.js'; import { validateCachekitUrl } from './url-validator.js'; /** - * Percent-encode a cache key for use as a single URL path segment. + * Reserved `{key}` path segments (protocol spec/saas-api.md § Cache-Key Path + * Encoding, rule 2). Two hazards, one CWE-22 class — both land the bearer + * token on a route the SaaS key validator never sees: * - * `encodeURIComponent` leaves `.` untouched (RFC-3986 unreserved), so a key - * of exactly `.` or `..` triggers dot-segment removal in the HTTP client's - * URL parser — the request escapes /v1/cache/ before it hits the wire - * (CWE-22). + * - Dot segments `.` / `..`: `encodeURIComponent` leaves `.` raw (RFC-3986 + * unreserved), and the WHATWG URL parser behind `fetch` (undici, Workers) + * removes the segment before the request leaves the process — `..` → `/v1/`, + * `../ttl` → `/v1/ttl`. `%2E` does not help: WHATWG treats `%2e`/`%2e%2e` + * as dot segments too, and so does the SaaS worker's own `new URL()`. + * - Route tokens `health` / `ttl` / `lock`: `/v1/cache/health` IS the health + * endpoint, and a trailing `ttl` / `lock` selects a sub-resource, so the + * key routes elsewhere or is read as an empty key. The SaaS router matches + * these case-sensitively and exactly, so only the lowercase words are reserved. * - * Python's fix encodes dots to `%2E`, which works because Python HTTP - * clients (httpx/requests) use RFC-3986 where `%2E` is opaque. In JS, - * `fetch`/undici parse URLs per the WHATWG URL Standard, which treats `%2E` - * identically to `.` for dot-segment removal — there is no percent-encoding - * of `.` that survives WHATWG normalisation in a path segment. We reject - * these keys instead: fail-fast with a clear error rather than silently - * sending an authenticated request to the wrong path. + * Case-sensitive, exact: `a:..`, `..a`, `HEALTH`, `ttls` are inert and + * transmitted unchanged. + */ +const RESERVED_SEGMENTS = new Set(['.', '..', 'health', 'ttl', 'lock']); + +/** + * Percent-encode a cache key as a single URL path segment, or throw + * `ConfigurationError` if it is a reserved segment (see RESERVED_SEGMENTS). + * Every other key is exactly `encodeURIComponent(key)`, so wire bytes match + * cachekit-rs (`encode_key`) and decode-equivalently match cachekit-py. */ export function encodeKey(key: string): string { const encoded = encodeURIComponent(key); - if (encoded === '.' || encoded === '..') { + if (RESERVED_SEGMENTS.has(encoded)) { throw new ConfigurationError( - `Cache key "${key}" is a bare dot-segment and cannot be used as a URL path segment — ` + - `the WHATWG URL parser (used by fetch) would collapse it, sending the request outside ` + - `/v1/cache/ (CWE-22). Use a namespaced key instead.` + `Cache key "${key}" is a reserved path segment (one of . .. health ttl lock) and cannot ` + + `be addressed at /v1/cache/{key}: the URL parser or the SaaS router would route it ` + + `elsewhere (CWE-22). Use a namespaced key instead.` ); } return encoded; @@ -104,9 +114,10 @@ export class CachekitIOCore implements Backend { async get(key: string): Promise { this.ensureNotClosed(); + const url = this.cacheUrl(key); try { - const response = await this.request('GET', this.cacheUrl(key)); + const response = await this.request('GET', url); if (response.status === 404) { return null; @@ -134,9 +145,10 @@ export class CachekitIOCore implements Backend { const effectiveTtl = validateTtl(ttl ?? this.defaultTtl); const headers: Record = { 'X-CacheKit-TTL': String(effectiveTtl) }; + const url = this.cacheUrl(key); try { - const response = await this.request('PUT', this.cacheUrl(key), { + const response = await this.request('PUT', url, { body: value, headers, }); @@ -152,9 +164,10 @@ export class CachekitIOCore implements Backend { async delete(key: string): Promise { this.ensureNotClosed(); + const url = this.cacheUrl(key); try { - const response = await this.request('DELETE', this.cacheUrl(key)); + const response = await this.request('DELETE', url); if (response.status === 404) { return false; @@ -173,9 +186,10 @@ export class CachekitIOCore implements Backend { async exists(key: string): Promise { this.ensureNotClosed(); + const url = this.cacheUrl(key); try { - const response = await this.request('HEAD', this.cacheUrl(key)); + const response = await this.request('HEAD', url); if (response.status === 404) { return false; @@ -226,6 +240,8 @@ export class CachekitIOCore implements Backend { // ── Internal ────────────────────────────────────────────── + /** Call before the network `try`: encodeKey's ConfigurationError must reach + * the caller as-is, not wrapped by the catch as a BackendError. */ private cacheUrl(key: string): string { return `${this.apiUrl}/v1/cache/${encodeKey(key)}`; } diff --git a/packages/cachekit/test/protocol/fixtures/path-encoding.json b/packages/cachekit/test/protocol/fixtures/path-encoding.json new file mode 100644 index 0000000..3837a80 --- /dev/null +++ b/packages/cachekit/test/protocol/fixtures/path-encoding.json @@ -0,0 +1,105 @@ +{ + "version": "1.0.0", + "generator": "urllib.parse.quote(key, safe=\"\") — cachekit-py v0.18.0 CachekitIOBackend._encode_key (f000ba3) minus its `.`/`..` → %2E rewrite, which the server's WHATWG URL parse collapses; `.` and `..` are reject rows", + "spec": "spec/saas-api.md § Cache-Key Path Encoding", + "ci_verification": "tools/path-encoding-verify.py (stdlib only; runs in this repo's verify.yml; mutation self-test first)", + "contract": "`encoded` is the single `{key}` path segment in the reference form (spec rule 1); `decoded` is the key the server sees after its single percent-decode and equals `key` in every transmittable row (spec rules 3-4). Rows with `reject: true` are the reserved segments of spec rule 2 (`.`, `..`, `health`, `ttl`, `lock`): a conformant client raises before building the URL, so `encoded` and `decoded` are null. `encoded_alternates` lists the other conformant wire form where `encodeURIComponent` differs (`! * ' ( )` raw); assert `encoded in [encoded] + encoded_alternates`.", + "vectors": [ + { + "key": "ns:test:func:__main__.get_user:args:3870b2ea5735ae639ded9450ef117768db676f037bec636503796c5b81095153:1s", + "encoded": "ns%3Atest%3Afunc%3A__main__.get_user%3Aargs%3A3870b2ea5735ae639ded9450ef117768db676f037bec636503796c5b81095153%3A1s", + "decoded": "ns:test:func:__main__.get_user:args:3870b2ea5735ae639ded9450ef117768db676f037bec636503796c5b81095153:1s", + "note": "Canonical 7-segment auto-mode key (test-vectors/cache-keys.json `single_integer`). Only `:` is encoded; identical bytes from all three reference encoders." + }, + { + "key": "default:../../admin", + "encoded": "default%3A..%2F..%2Fadmin", + "decoded": "default:../../admin", + "note": "Embedded traversal: every `/` is `%2F`, so no `../` boundary exists for a URL parser to collapse. Server decodes once, then rejects (`/` outside charset; `..` substring)." + }, + { + "key": "x/../../health", + "encoded": "x%2F..%2F..%2Fhealth", + "decoded": "x/../../health", + "note": "Traversal aimed at the `/v1/cache/health` route token; inert once `/` is `%2F`. Server rejects the decoded key (charset)." + }, + { + "key": "k?x=1#f", + "encoded": "k%3Fx%3D1%23f", + "decoded": "k?x=1#f", + "note": "Query/fragment injection: `?` and `#` MUST be encoded or the client's URL parser truncates the key and emits a query string. Server rejects the decoded key (charset)." + }, + { + "key": "a b", + "encoded": "a%20b", + "decoded": "a b", + "note": "Space is `%20` in a path segment, never `+` (form-encoding, which the server does not decode). Server rejects the decoded key (charset)." + }, + { + "key": "100%", + "encoded": "100%25", + "decoded": "100%", + "note": "A literal `%` is encoded exactly once; the server decodes once and sees `100%`, then rejects it (charset). `%2525` would decode to `100%25`, a different key." + }, + { + "key": ".", + "encoded": null, + "decoded": null, + "reject": true, + "note": "Dot segment: the plain encoding `.` is removed by every URL parser, and `%2E` is collapsed by the server's WHATWG parse (`/v1/cache/%2E` → `/v1/cache/`). No wire form reaches the validator." + }, + { + "key": "..", + "encoded": null, + "decoded": null, + "reject": true, + "note": "Dot segment: unencoded, `/v1/cache/..` collapses to `/v1/` and `/v1/cache/../ttl` to `/v1/ttl`; `%2E%2E` is collapsed the same way by the server (`GET /v1/cache/%2E%2E/health` returns the `/v1/health` response). No wire form reaches the validator." + }, + { + "key": "health", + "encoded": null, + "decoded": null, + "reject": true, + "note": "Route token: `/v1/cache/health` is the health endpoint, so a GET for this key would return the health payload as a cache hit." + }, + { + "key": "ttl", + "encoded": null, + "decoded": null, + "reject": true, + "note": "Route token: a final `ttl` segment selects the TTL sub-resource, so `/v1/cache/ttl` is read as an empty key plus `/ttl`." + }, + { + "key": "lock", + "encoded": null, + "decoded": null, + "reject": true, + "note": "Route token: a final `lock` segment selects the lock sub-resource, so `/v1/cache/lock` is read as an empty key plus `/lock`." + }, + { + "key": "a:..", + "encoded": "a%3A..", + "decoded": "a:..", + "note": "Trailing dots but NOT an all-dot segment: not a dot segment under either parsing model, so the dots stay raw. Server rejects the decoded `..` substring." + }, + { + "key": "..a", + "encoded": "..a", + "decoded": "..a", + "note": "Leading dots, not an all-dot segment: sent verbatim (all characters unreserved). Server rejects the decoded `..` substring." + }, + { + "key": "ns:key", + "encoded": "ns%3Akey", + "decoded": "ns:key", + "note": "`:` → `%3A`, decoded once server-side. The server then rejects it: an `ns:` key must be `ns:{namespace}:{rest}` (spec rule 3), and this one has no `{rest}`." + }, + { + "key": "f(x)!*'", + "encoded": "f%28x%29%21%2A%27", + "decoded": "f(x)!*'", + "encoded_alternates": ["f(x)!*'"], + "note": "Encoder-variance row (spec rule 4): `quote(safe=\"\")` and `urlencoding::encode` produce `encoded`; `encodeURIComponent` produces the alternate. Both decode to `key`. Server rejects the decoded key (charset)." + } + ] +} diff --git a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts new file mode 100644 index 0000000..87381a5 --- /dev/null +++ b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts @@ -0,0 +1,161 @@ +/** + * Cache-Key Path Encoding Protocol Tests (CWE-22) + * + * Verifies `encodeKey` and every CachekitIO request builder against + * protocol/test-vectors/path-encoding.json (vendored in ./fixtures/ — re-copy + * on spec change). Spec: protocol/spec/saas-api.md § Cache-Key Path Encoding. + * + * Every wire assertion reads the URL the real class handed to `fetch` and + * parses it with `new URL()` — the post-WHATWG-normalisation path, which is + * what leaves the process. A template-string comparison would pass while the + * traversal ships (spec rule 2). + */ + +import { readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { CachekitIOCore, encodeKey } from '../../src/backends/cachekitio.js'; +import { TTLCachekitIO } from '../../src/backends/cachekitio-ttl.js'; +import { LockableCachekitIO } from '../../src/backends/cachekitio-lockable.js'; +import { BackendError, ConfigurationError } from '../../src/errors.js'; + +interface Vector { + key: string; + encoded: string | null; + decoded: string | null; + reject?: boolean; + encoded_alternates?: string[]; + note: string; +} + +const here = dirname(fileURLToPath(import.meta.url)); +const { vectors } = JSON.parse( + readFileSync(join(here, 'fixtures', 'path-encoding.json'), 'utf8') +) as { vectors: Vector[] }; +const reserved = vectors.filter((v) => v.reject); +const transmittable = vectors.filter((v) => !v.reject); + +const BASE = 'https://api.cachekit.io'; +const PREFIX = '/v1/cache/'; + +/** Real backend classes over a fetch spy that answers every call 200 with a JSON body. */ +function harness() { + const fetchSpy = vi.fn( + async () => + new Response(JSON.stringify({ ttl: 60, lock_id: 'lock-1' }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }) + ); + vi.stubGlobal('fetch', fetchSpy); + const core = new CachekitIOCore({ apiKey: 'ck_test_fake-not-a-secret', apiUrl: BASE }); // pragma: allowlist secret + return { fetchSpy, core, ttl: new TTLCachekitIO(core), lock: new LockableCachekitIO(core) }; +} +type Harness = ReturnType; + +/** Every operation that places `{key}` in the request path, with its route suffix. */ +const OPERATIONS: Record< + string, + { run: (h: Harness, key: string) => Promise; suffix: string } +> = { + get: { run: (h, k) => h.core.get(k), suffix: '' }, + set: { run: (h, k) => h.core.set(k, new Uint8Array([1])), suffix: '' }, + delete: { run: (h, k) => h.core.delete(k), suffix: '' }, + exists: { run: (h, k) => h.core.exists(k), suffix: '' }, + getTTL: { run: (h, k) => h.ttl.getTTL(k), suffix: '/ttl' }, + refreshTTL: { run: (h, k) => h.ttl.refreshTTL(k, 60), suffix: '/ttl' }, + acquireLock: { run: (h, k) => h.lock.acquireLock(k, 1000), suffix: '/lock' }, + releaseLock: { run: (h, k) => h.lock.releaseLock(k, 'lock-1'), suffix: '/lock' }, +}; + +/** The path `fetch` was handed, as the WHATWG parser resolves it. */ +function sentPathname(h: Harness): string { + expect(h.fetchSpy).toHaveBeenCalledTimes(1); + const [url] = h.fetchSpy.mock.calls[0] as unknown as [string]; + return new URL(url).pathname; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('AC-0 repro — raw encodeURIComponent lets a dot-segment key escape /v1/cache/', () => { + it('literal dots collapse client-side', () => { + expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('.')}`).pathname).toBe(PREFIX); + expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('..')}`).pathname).toBe('/v1/'); + expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('..')}/ttl`).pathname).toBe('/v1/ttl'); + expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('..')}/lock`).pathname).toBe('/v1/lock'); + }); + + it('%2E does not help — WHATWG treats it as a dot segment (URL Standard §4.1)', () => { + expect(new URL(`${BASE}${PREFIX}%2E`).pathname).toBe(PREFIX); + expect(new URL(`${BASE}${PREFIX}%2E%2E`).pathname).toBe('/v1/'); + expect(new URL(`${BASE}${PREFIX}%2E%2E/ttl`).pathname).toBe('/v1/ttl'); + expect(new URL(`${BASE}${PREFIX}%2e%2e/lock`).pathname).toBe('/v1/lock'); + }); +}); + +describe('rule 2 — reserved segments are rejected before the URL is built', () => { + it('the vendored fixture reserves exactly the five spec tokens', () => { + expect(reserved.map((v) => v.key).sort()).toEqual(['.', '..', 'health', 'lock', 'ttl']); + }); + + it.each(reserved)('encodeKey($key) throws ConfigurationError', ({ key }) => { + expect(() => encodeKey(key)).toThrow(ConfigurationError); + expect(() => encodeKey(key)).toThrow(/CWE-22/); + }); + + // Case-sensitive, exact match — mirrors the SaaS router (`=== 'health'`, `=== 'ttl' || 'lock'`). + it.each(['...', 'HEALTH', 'Health', 'ttls', 'unlock', 'health:x', '.health', 'a:..', '..a'])( + 'near-miss %j is transmittable and unchanged', + (key) => { + expect(encodeKey(key)).toBe(encodeURIComponent(key)); + } + ); + + describe.each(Object.entries(OPERATIONS))('%s', (_name, op) => { + it.each(reserved)( + 'rejects $key with ConfigurationError and never calls fetch', + async ({ key }) => { + const h = harness(); + const err = await op.run(h, key).catch((e: unknown) => e); + expect(err).toBeInstanceOf(ConfigurationError); + expect(err).not.toBeInstanceOf(BackendError); + expect(h.fetchSpy).not.toHaveBeenCalled(); + } + ); + }); +}); + +describe('rules 1, 3, 4 — transmittable keys travel as one segment and decode once to the key', () => { + it.each(transmittable)( + 'encodeKey($key) is a conformant wire form', + ({ key, encoded, encoded_alternates }) => { + expect([encoded, ...(encoded_alternates ?? [])]).toContain(encodeKey(key)); + expect(encodeKey(key)).toBe(encodeURIComponent(key)); + } + ); + + it.each(transmittable)('decodeURIComponent(encodeKey($key)) round-trips', ({ key, decoded }) => { + expect(decodeURIComponent(encodeKey(key))).toBe(decoded); + expect(decoded).toBe(key); + }); + + describe.each(Object.entries(OPERATIONS))('%s', (_name, op) => { + it.each(transmittable)( + 'sends $key inside /v1/cache/ as one segment', + async ({ key, decoded }) => { + const h = harness(); + await op.run(h, key); + const pathname = sentPathname(h); + expect(pathname.startsWith(PREFIX)).toBe(true); + expect(pathname.endsWith(op.suffix)).toBe(true); + const segment = pathname.slice(PREFIX.length, pathname.length - op.suffix.length); + expect(segment).toBe(encodeKey(key)); + expect(segment).not.toContain('/'); + expect(decodeURIComponent(segment)).toBe(decoded); + } + ); + }); +}); From 48b9279f29b565a134e33493c36727233af6371c Mon Sep 17 00:00:00 2001 From: Winston Date: Mon, 7 Sep 2026 09:44:25 +1000 Subject: [PATCH 4/6] fix(cachekitio): pre-flight reserved keys before the reliability executor (expert panel, LAB-2877) Panel findings applied (high stakes, post-spec): - Backend.validateKey capability, mirroring validateTtl: CachekitIOCore implements it via encodeKey, the TTL/Lockable/combined wrappers forward it, and CacheImpl calls it synchronously in get/set/delete/exists before run(). Without it the ConfigurationError fired inside the executor on the public createCache path: retried maxAttempts times, counted by the circuit breaker (five reserved keys in 60s opened it and blackholed legitimate keys), then swallowed by degradation into a silent miss / a set() that never stored. - encodeKey wraps encodeURIComponent: a lone surrogate threw a raw URIError, which the URL hoist had moved outside the catch that used to wrap it. Every SDK error stays a CachekitError. - Doc fix: ts is decode-equivalent to cachekit-rs, not byte-identical (urlencoding::encode escapes !*'() that encodeURIComponent leaves raw). - Error message derives the token list from RESERVED_SEGMENTS; SECURITY.md no longer says a percent-encoded key is "sent unchanged". - Tests: dropped the tautological not-BackendError assertion, the standalone round-trip block (the decode is asserted on the real wire path x8 ops), the message-regex pin and the over-long near-miss list; the wire matrix asserts the exact pathname. Added validateKey coverage on all three backend classes, the lone-surrogate case, and a cache.test.ts regression beside LAB-239's. --- SECURITY.md | 2 +- .../src/backends/cachekitio-factory.ts | 3 ++ .../src/backends/cachekitio-lockable.ts | 3 ++ .../cachekit/src/backends/cachekitio-ttl.ts | 3 ++ packages/cachekit/src/backends/cachekitio.ts | 51 ++++++++++--------- packages/cachekit/src/backends/types.ts | 14 +++++ packages/cachekit/src/cache-core.ts | 9 +++- packages/cachekit/src/cache.test.ts | 47 +++++++++++++++++ .../protocol/path-encoding.protocol.test.ts | 37 ++++++++------ 9 files changed, 129 insertions(+), 40 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 8ff7577..7298562 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -16,7 +16,7 @@ Instead, use [GitHub's private vulnerability reporting](https://github.com/cache ## Cache-Key Path Encoding (CWE-22) -The CachekitIO backend transmits cache keys as a single URL path segment (`/v1/cache/{key}`, `…/{key}/ttl`, `…/{key}/lock`). Keys are percent-encoded with `encodeURIComponent`; a key that is exactly `.`, `..`, `health`, `ttl` or `lock` is rejected with a `ConfigurationError` before any request is built ([protocol `spec/saas-api.md` § Cache-Key Path Encoding](https://github.com/cachekit-io/protocol/blob/main/spec/saas-api.md#cache-key-path-encoding), rule 2): the WHATWG URL parser behind `fetch` removes literal and percent-encoded (`%2E`) dot segments before the request reaches the wire, and the other three words are live route tokens at that path level. Every other key — including `a:..` and every canonical `ns:…` key — is sent unchanged and decodes once server-side to the original key. +The CachekitIO backend transmits cache keys as a single URL path segment (`/v1/cache/{key}`, `…/{key}/ttl`, `…/{key}/lock`). Keys are percent-encoded with `encodeURIComponent`; a key that is exactly `.`, `..`, `health`, `ttl` or `lock` is rejected with a `ConfigurationError` before any request is built ([protocol `spec/saas-api.md` § Cache-Key Path Encoding](https://github.com/cachekit-io/protocol/blob/main/spec/saas-api.md#cache-key-path-encoding), rule 2): the WHATWG URL parser behind `fetch` removes literal and percent-encoded (`%2E`) dot segments before the request reaches the wire, and the other three words are live route tokens at that path level. Every other key — including `a:..` and every canonical `ns:…` key — is sent percent-encoded as a single path segment and decodes once server-side to the original key. ## Scope diff --git a/packages/cachekit/src/backends/cachekitio-factory.ts b/packages/cachekit/src/backends/cachekitio-factory.ts index 6daa143..0b66a3b 100644 --- a/packages/cachekit/src/backends/cachekitio-factory.ts +++ b/packages/cachekit/src/backends/cachekitio-factory.ts @@ -43,6 +43,9 @@ class CachekitIO implements LockableBackend, TTLBackend { validateTtl(ttl: number) { this.lockable.validateTtl(ttl); } + validateKey(key: string) { + this.lockable.validateKey(key); + } delete(key: string) { return this.lockable.delete(key); } diff --git a/packages/cachekit/src/backends/cachekitio-lockable.ts b/packages/cachekit/src/backends/cachekitio-lockable.ts index f87a5ab..b1262ad 100644 --- a/packages/cachekit/src/backends/cachekitio-lockable.ts +++ b/packages/cachekit/src/backends/cachekitio-lockable.ts @@ -33,6 +33,9 @@ export class LockableCachekitIO implements LockableBackend { validateTtl(ttl: number) { this.inner.validateTtl(ttl); } + validateKey(key: string) { + this.inner.validateKey(key); + } delete(key: string) { return this.inner.delete(key); } diff --git a/packages/cachekit/src/backends/cachekitio-ttl.ts b/packages/cachekit/src/backends/cachekitio-ttl.ts index 38084c3..7304536 100644 --- a/packages/cachekit/src/backends/cachekitio-ttl.ts +++ b/packages/cachekit/src/backends/cachekitio-ttl.ts @@ -25,6 +25,9 @@ export class TTLCachekitIO implements TTLBackend { validateTtl(ttl: number) { this.inner.validateTtl(ttl); } + validateKey(key: string) { + this.inner.validateKey(key); + } delete(key: string) { return this.inner.delete(key); } diff --git a/packages/cachekit/src/backends/cachekitio.ts b/packages/cachekit/src/backends/cachekitio.ts index 2c4ecc7..eb14253 100644 --- a/packages/cachekit/src/backends/cachekitio.ts +++ b/packages/cachekit/src/backends/cachekitio.ts @@ -7,38 +7,37 @@ import { classifyHttpError, classifyNetworkError } from './error-classifier.js'; import { validateCachekitUrl } from './url-validator.js'; /** - * Reserved `{key}` path segments (protocol spec/saas-api.md § Cache-Key Path - * Encoding, rule 2). Two hazards, one CWE-22 class — both land the bearer - * token on a route the SaaS key validator never sees: - * - * - Dot segments `.` / `..`: `encodeURIComponent` leaves `.` raw (RFC-3986 - * unreserved), and the WHATWG URL parser behind `fetch` (undici, Workers) - * removes the segment before the request leaves the process — `..` → `/v1/`, - * `../ttl` → `/v1/ttl`. `%2E` does not help: WHATWG treats `%2e`/`%2e%2e` - * as dot segments too, and so does the SaaS worker's own `new URL()`. - * - Route tokens `health` / `ttl` / `lock`: `/v1/cache/health` IS the health - * endpoint, and a trailing `ttl` / `lock` selects a sub-resource, so the - * key routes elsewhere or is read as an empty key. The SaaS router matches - * these case-sensitively and exactly, so only the lowercase words are reserved. - * - * Case-sensitive, exact: `a:..`, `..a`, `HEALTH`, `ttls` are inert and - * transmitted unchanged. + * Path segments a key may never encode to (protocol spec/saas-api.md + * § Cache-Key Path Encoding, rule 2). `.` / `..` are dot segments that every + * WHATWG parser — fetch's and the SaaS worker's own — removes before routing, + * `%2E` forms included, so no encoding keeps them inside /v1/cache/ (CWE-22). + * `health` / `ttl` / `lock` are live route tokens at that level. The SaaS + * router matches all five exactly and case-sensitively, so only these + * lowercase words are reserved: `a:..`, `..a`, `HEALTH`, `ttls` transmit. */ const RESERVED_SEGMENTS = new Set(['.', '..', 'health', 'ttl', 'lock']); /** * Percent-encode a cache key as a single URL path segment, or throw - * `ConfigurationError` if it is a reserved segment (see RESERVED_SEGMENTS). - * Every other key is exactly `encodeURIComponent(key)`, so wire bytes match - * cachekit-rs (`encode_key`) and decode-equivalently match cachekit-py. + * `ConfigurationError` if it is a reserved segment (RESERVED_SEGMENTS) or + * malformed UTF-16. Every other key is exactly `encodeURIComponent(key)`: + * decode-equivalent to cachekit-py and cachekit-rs, which additionally + * encode `! * ' ( )` (spec rule 4; fixture `encoded_alternates`). */ export function encodeKey(key: string): string { - const encoded = encodeURIComponent(key); + let encoded: string; + try { + encoded = encodeURIComponent(key); + } catch (error) { + // Lone surrogate: encodeURIComponent throws a raw URIError. Every SDK error is a CachekitError. + throw new ConfigurationError('Cache key is not well-formed UTF-16 (lone surrogate)', { + cause: error, + }); + } if (RESERVED_SEGMENTS.has(encoded)) { throw new ConfigurationError( - `Cache key "${key}" is a reserved path segment (one of . .. health ttl lock) and cannot ` + - `be addressed at /v1/cache/{key}: the URL parser or the SaaS router would route it ` + - `elsewhere (CWE-22). Use a namespaced key instead.` + `Cache key "${key}" is a reserved path segment (one of ${[...RESERVED_SEGMENTS].join(' ')}) ` + + `and cannot be addressed at /v1/cache/{key} (CWE-22). Use a namespaced key instead.` ); } return encoded; @@ -140,6 +139,12 @@ export class CachekitIOCore implements Backend { validateTtl(ttl); } + /** Backend.validateKey capability — rejects a reserved path segment + * synchronously, before the reliability executor can swallow it. */ + validateKey(key: string): void { + encodeKey(key); + } + async set(key: string, value: Uint8Array, ttl?: number): Promise { this.ensureNotClosed(); diff --git a/packages/cachekit/src/backends/types.ts b/packages/cachekit/src/backends/types.ts index c82d183..a624bef 100644 --- a/packages/cachekit/src/backends/types.ts +++ b/packages/cachekit/src/backends/types.ts @@ -77,6 +77,20 @@ export interface Backend { */ validateTtl?(ttl: number): void; + /** + * Reject a key this backend cannot address, synchronously and before the + * reliability executor runs — CachekitIO refuses the reserved path + * segments `.` `..` `health` `ttl` `lock` (protocol spec/saas-api.md + * § Cache-Key Path Encoding). Same contract as validateTtl: inside `run`, + * degradation would swallow the deterministic caller error and + * retry/circuit-breaker would count it as backend failures. + * + * Delegating wrappers MUST forward the inner backend's implementation. + * + * @throws {ConfigurationError} when the key is rejected by this backend + */ + validateKey?(key: string): void; + /** * Delete a key from the cache. * diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index 81f7200..67fd916 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -665,6 +665,9 @@ export class CacheImpl implements SecureCache { } } + // Reserved-key pre-flight — see Backend.validateKey. + this.backend.validateKey?.(key); + // Fetch from L2 (backend) return this.run('get', null, async (): Promise => { // When L1 will be re-populated, prefer the TTL-carrying read (same @@ -754,11 +757,13 @@ export class CacheImpl implements SecureCache { } // A backend with hard TTL bounds (CachekitIO: reject 0 / > 30 days per - // protocol) rejects here, synchronously — for the same reason as the + // protocol) or a key it cannot address (CachekitIO's reserved path + // segments) rejects here, synchronously — for the same reason as the // interop rejection below: inside `run`, degradation would swallow the // deterministic caller error (set() would silently never store) and // retry/circuit-breaker would count it as backend failures. this.backend.validateTtl?.(ttl); + this.backend.validateKey?.(key); const namespace = options?.namespace ?? extractNamespace(key); const useEnvelope = this.useEnvelope(interop); @@ -844,6 +849,7 @@ export class CacheImpl implements SecureCache { async delete(key: string): Promise { this.ensureNotClosed(); + this.backend.validateKey?.(key); // see Backend.validateKey return this.run('delete', false, async (): Promise => { // Delete from backend @@ -861,6 +867,7 @@ export class CacheImpl implements SecureCache { async exists(key: string): Promise { this.ensureNotClosed(); + this.backend.validateKey?.(key); // see Backend.validateKey // Check L1 first. Presence alone is not an answer for a secure cache: L1 // holds ciphertext, and after a key rotation every resident entry is diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 5941353..84081ee 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -272,6 +272,53 @@ describe('Cache Integration', () => { expect(setCalls).toBe(1); await boundedCache.close(); }); + + // LAB-2877 regression: a backend's key rejection (Backend.validateKey — + // CachekitIO's reserved path segments) is the same kind of deterministic + // caller error: inside the executor it would be retried, counted by the + // circuit breaker, and swallowed by degradation into a silent miss or a + // set() that never stores. + it('surfaces a backend key rejection despite default-on degradation', async () => { + const inner = new InMemoryBackend(); + const calls: string[] = []; + const guardedBackend: Backend = { + async get(key) { + calls.push('get'); + return inner.get(key); + }, + async set(key, value, ttl) { + calls.push('set'); + return inner.set(key, value, ttl!); + }, + async delete(key) { + calls.push('delete'); + return inner.delete(key); + }, + async exists(key) { + calls.push('exists'); + return inner.exists(key); + }, + close: () => inner.close(), + validateKey(key) { + if (key === '..') throw new ConfigurationError('reserved path segment'); + }, + }; + + const guardedCache = createCache({ + backend: guardedBackend, + compression: false, + l1: { enabled: false }, + }); + await expect(guardedCache.get('..')).rejects.toThrow(ConfigurationError); + await expect(guardedCache.set('..', 'value')).rejects.toThrow(ConfigurationError); + await expect(guardedCache.delete('..')).rejects.toThrow(ConfigurationError); + await expect(guardedCache.exists('..')).rejects.toThrow(ConfigurationError); + expect(calls).toEqual([]); // rejected before the reliability executor ran + + await guardedCache.set('test:key-ok', 'value'); + expect(calls).toEqual(['set']); + await guardedCache.close(); + }); }); describe('Compression (ByteStorage)', () => { diff --git a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts index 87381a5..d164e56 100644 --- a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts +++ b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts @@ -18,7 +18,7 @@ import { describe, it, expect, afterEach, vi } from 'vitest'; import { CachekitIOCore, encodeKey } from '../../src/backends/cachekitio.js'; import { TTLCachekitIO } from '../../src/backends/cachekitio-ttl.js'; import { LockableCachekitIO } from '../../src/backends/cachekitio-lockable.js'; -import { BackendError, ConfigurationError } from '../../src/errors.js'; +import { ConfigurationError } from '../../src/errors.js'; interface Vector { key: string; @@ -80,6 +80,8 @@ afterEach(() => { vi.unstubAllGlobals(); }); +// Pins the platform premise the reject-not-encode design rests on. If a runtime +// ever stops collapsing these, the decision needs revisiting — not silently. describe('AC-0 repro — raw encodeURIComponent lets a dot-segment key escape /v1/cache/', () => { it('literal dots collapse client-side', () => { expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('.')}`).pathname).toBe(PREFIX); @@ -103,25 +105,38 @@ describe('rule 2 — reserved segments are rejected before the URL is built', () it.each(reserved)('encodeKey($key) throws ConfigurationError', ({ key }) => { expect(() => encodeKey(key)).toThrow(ConfigurationError); - expect(() => encodeKey(key)).toThrow(/CWE-22/); }); // Case-sensitive, exact match — mirrors the SaaS router (`=== 'health'`, `=== 'ttl' || 'lock'`). - it.each(['...', 'HEALTH', 'Health', 'ttls', 'unlock', 'health:x', '.health', 'a:..', '..a'])( + it.each(['...', 'HEALTH', 'ttls', 'unlock'])( 'near-miss %j is transmittable and unchanged', (key) => { expect(encodeKey(key)).toBe(encodeURIComponent(key)); } ); + it('a lone surrogate is a ConfigurationError, not a raw URIError', async () => { + expect(() => encodeKey('a\uD800')).toThrow(ConfigurationError); + const h = harness(); + await expect(h.core.get('a\uD800')).rejects.toBeInstanceOf(ConfigurationError); + expect(h.fetchSpy).not.toHaveBeenCalled(); + }); + + // Backend.validateKey lets CacheImpl reject synchronously, before the + // reliability executor could retry, count, and swallow the rejection. + it.each(reserved)('validateKey($key) throws on the core and both wrappers', ({ key }) => { + const h = harness(); + for (const backend of [h.core, h.ttl, h.lock]) { + expect(() => backend.validateKey(key)).toThrow(ConfigurationError); + } + }); + describe.each(Object.entries(OPERATIONS))('%s', (_name, op) => { it.each(reserved)( 'rejects $key with ConfigurationError and never calls fetch', async ({ key }) => { const h = harness(); - const err = await op.run(h, key).catch((e: unknown) => e); - expect(err).toBeInstanceOf(ConfigurationError); - expect(err).not.toBeInstanceOf(BackendError); + await expect(op.run(h, key)).rejects.toBeInstanceOf(ConfigurationError); expect(h.fetchSpy).not.toHaveBeenCalled(); } ); @@ -137,11 +152,6 @@ describe('rules 1, 3, 4 — transmittable keys travel as one segment and decode } ); - it.each(transmittable)('decodeURIComponent(encodeKey($key)) round-trips', ({ key, decoded }) => { - expect(decodeURIComponent(encodeKey(key))).toBe(decoded); - expect(decoded).toBe(key); - }); - describe.each(Object.entries(OPERATIONS))('%s', (_name, op) => { it.each(transmittable)( 'sends $key inside /v1/cache/ as one segment', @@ -149,11 +159,8 @@ describe('rules 1, 3, 4 — transmittable keys travel as one segment and decode const h = harness(); await op.run(h, key); const pathname = sentPathname(h); - expect(pathname.startsWith(PREFIX)).toBe(true); - expect(pathname.endsWith(op.suffix)).toBe(true); + expect(pathname).toBe(`${PREFIX}${encodeKey(key)}${op.suffix}`); const segment = pathname.slice(PREFIX.length, pathname.length - op.suffix.length); - expect(segment).toBe(encodeKey(key)); - expect(segment).not.toContain('/'); expect(decodeURIComponent(segment)).toBe(decoded); } ); From e3fb6a60d646b7ae292185233f70f1272428cdac Mon Sep 17 00:00:00 2001 From: Mark S Date: Mon, 7 Sep 2026 10:09:35 +1000 Subject: [PATCH 5/6] fix(cachekitio): reject empty cache key before URL construction (CWE-22, LAB-2877) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit encodeKey('') returned '' (not a reserved segment), so cacheUrl('') built /v1/cache/ — the collection path, not a keyed resource — the same dot-segment escape class RESERVED_SEGMENTS guards, reached without hitting it. Guard the empty key up front in encodeKey, the single chokepoint both validateKey and cacheUrl route through, so every operation rejects it synchronously with a ConfigurationError and never calls fetch. Addresses CodeRabbit finding on cachekitio.ts validateKey. --- packages/cachekit/src/backends/cachekitio.ts | 9 ++++++ .../protocol/path-encoding.protocol.test.ts | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/packages/cachekit/src/backends/cachekitio.ts b/packages/cachekit/src/backends/cachekitio.ts index eb14253..eabca05 100644 --- a/packages/cachekit/src/backends/cachekitio.ts +++ b/packages/cachekit/src/backends/cachekitio.ts @@ -25,6 +25,15 @@ const RESERVED_SEGMENTS = new Set(['.', '..', 'health', 'ttl', 'lock']); * encode `! * ' ( )` (spec rule 4; fixture `encoded_alternates`). */ export function encodeKey(key: string): string { + // An empty key encodes to an empty segment, so `/v1/cache/${''}` collapses to + // the `/v1/cache/` collection path — the same CWE-22 escape as a dot segment, + // reached without ever hitting RESERVED_SEGMENTS. Reject it up front. + if (key === '') { + throw new ConfigurationError( + 'Cache key must not be empty: an empty key addresses the /v1/cache/ collection path, ' + + 'not a keyed resource (CWE-22). Use a non-empty, namespaced key.' + ); + } let encoded: string; try { encoded = encodeURIComponent(key); diff --git a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts index d164e56..6b9b1c6 100644 --- a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts +++ b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts @@ -143,6 +143,34 @@ describe('rule 2 — reserved segments are rejected before the URL is built', () }); }); +describe('rule 2 — the empty key is rejected before the URL is built', () => { + // An empty key is not a fixture vector (it has no wire form): `/v1/cache/${''}` + // collapses to the `/v1/cache/` collection path, the same escape class as `.`, + // reached without hitting RESERVED_SEGMENTS. Guard it here. + it('the platform premise: an empty segment collapses to the collection path', () => { + expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('')}`).pathname).toBe(PREFIX); + }); + + it('encodeKey("") throws ConfigurationError', () => { + expect(() => encodeKey('')).toThrow(ConfigurationError); + }); + + it('validateKey("") throws on the core and both wrappers', () => { + const h = harness(); + for (const backend of [h.core, h.ttl, h.lock]) { + expect(() => backend.validateKey('')).toThrow(ConfigurationError); + } + }); + + describe.each(Object.entries(OPERATIONS))('%s', (_name, op) => { + it('rejects "" with ConfigurationError and never calls fetch', async () => { + const h = harness(); + await expect(op.run(h, '')).rejects.toBeInstanceOf(ConfigurationError); + expect(h.fetchSpy).not.toHaveBeenCalled(); + }); + }); +}); + describe('rules 1, 3, 4 — transmittable keys travel as one segment and decode once to the key', () => { it.each(transmittable)( 'encodeKey($key) is a conformant wire form', From 3b768ac160b46702d3f254355a8a40f43cb057e2 Mon Sep 17 00:00:00 2001 From: Mark S Date: Mon, 7 Sep 2026 10:14:34 +1000 Subject: [PATCH 6/6] test(cachekitio): name empty-key check a precondition guard, not a spec rule-2 vector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expert-panel [MIN]: the block title claimed spec rule-2 provenance while its own comment disclaimed the empty key as a fixture vector — an internal contradiction. Retitle to name it a local precondition guard and note the cross-SDK fixture parity (empty-key reject row in protocol/test-vectors/path-encoding.json) as tracked follow-up. No behaviour change. --- .../test/protocol/path-encoding.protocol.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts index 6b9b1c6..6ac4ba1 100644 --- a/packages/cachekit/test/protocol/path-encoding.protocol.test.ts +++ b/packages/cachekit/test/protocol/path-encoding.protocol.test.ts @@ -143,10 +143,13 @@ describe('rule 2 — reserved segments are rejected before the URL is built', () }); }); -describe('rule 2 — the empty key is rejected before the URL is built', () => { - // An empty key is not a fixture vector (it has no wire form): `/v1/cache/${''}` - // collapses to the `/v1/cache/` collection path, the same escape class as `.`, - // reached without hitting RESERVED_SEGMENTS. Guard it here. +describe('empty-key precondition — rejected before the URL is built (no shared fixture vector yet)', () => { + // The empty key is the same CWE-22 escape class as the `.`/`..` reject rows — + // `/v1/cache/${''}` collapses to the `/v1/cache/` collection path — but it is + // not yet a row in the vendored cross-SDK fixture, so it is enforced here as a + // local precondition guard rather than claimed as a spec rule-2 vector. Adding + // the empty-key reject row to protocol/test-vectors/path-encoding.json (and + // re-vendoring) is tracked as cross-SDK parity follow-up. it('the platform premise: an empty segment collapses to the collection path', () => { expect(new URL(`${BASE}${PREFIX}${encodeURIComponent('')}`).pathname).toBe(PREFIX); });