Address review: fail-closed cache freshness and wrong-shape document guards

- A NaN expiresAt passed both freshness comparisons and was served
  forever; the gate is now the positive check the doc already states.
- read() served decodable-but-non-object documents verbatim; they now
  route through the same report + drop + miss path as parse failures.
- _decodeListTools validated only the container, so a null element threw
  in the index builders before memoization, re-firing per call; the guard
  now requires object elements.

Author: felixweinberger

packages/client/src/client/responseCache.ts

@@ -553,16 +553,21 @@ export class ClientResponseCache {
      * `entry.expiresAt > now()` (a missing `expiresAt` is never fresh),
      * checked BEFORE decoding so stale entries cost no parse. Every hit is
      * freshly parsed, so the caller owns the value outright. An entry whose
-     * document does not parse (corrupted external store) is reported,
+     * document does not parse or is not an object (corrupted external
+     * store) is reported,
      * deleted, and treated as a miss — deleted because a fresh-but-corrupt
      * entry would otherwise re-parse and re-report on every read until its
      * `expiresAt` passes.
      */
     async read(method: string, params?: string): Promise<{ value: unknown } | undefined> {
         const entry = await this._probe(method, params);
-        if (entry?.expiresAt === undefined || entry.expiresAt <= this.now()) return undefined;
+        if (entry?.expiresAt === undefined || !(entry.expiresAt > this.now())) return undefined;
         try {
-            return { value: JSON.parse(entry.value) };
+            const parsed: unknown = JSON.parse(entry.value);
+            if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) {
+                throw new TypeError('cached document is not an object');
+            }
+            return { value: parsed };
         } catch (error) {
             this._reportError(error);
             await this._deleteBoth(method, params ?? '');
@@ -657,15 +662,17 @@ export class ClientResponseCache {
     }
 
     /** Parse a held `tools/list` document for the index builders; a document
-     * that does not parse OR parses to something without a `tools` array
+     * that does not parse OR whose `tools` is not an array of objects
      * (both mean a corrupted external store) is reported and treated as if
      * nothing were held. Callers memoize the outcome against the entry's
      * stamp, so a corrupt document costs one parse + report per stamp, not
      * per lookup. */
     private _decodeListTools(entry: CacheEntry): ListToolsResult | undefined {
         try {
             const parsed = JSON.parse(entry.value) as ListToolsResult | null;
-            if (!Array.isArray(parsed?.tools)) throw new TypeError('cached tools/list document has no tools array');
+            if (!Array.isArray(parsed?.tools) || !parsed.tools.every(t => t !== null && typeof t === 'object')) {
+                throw new TypeError('cached tools/list document has a malformed tools array');
+            }
             return parsed;
         } catch (error) {
             this._reportError(error);

packages/client/test/client/responseCacheCodec.test.ts

@@ -209,7 +209,59 @@ describe('response cache document codec', () => {
             // Memoized against the unchanged stamp: one report for the tool
             // index, one for the validator index — not one per lookup.
             expect(reported).toHaveLength(2);
-            expect(String(reported[0])).toMatch(/no tools array/);
+            expect(String(reported[0])).toMatch(/tools array/);
         }
     });
+
+    test('a NaN expiresAt is never fresh: the entry is not served at any clock value', async () => {
+        const store: ResponseCacheStore = {
+            get: () => ({ value: '{"tools":[]}', stamp: 1, expiresAt: NaN, scope: 'private' as const }),
+            set: () => 1,
+            delete: () => {},
+            evict: () => {},
+            clear: () => {}
+        };
+        const cache = new ClientResponseCache(store, true);
+        expect(await cache.read('tools/list')).toBeUndefined();
+    });
+
+    test('a fresh decodable-but-non-object document is reported, dropped, and read as a miss', async () => {
+        for (const document of ['null', '"str"', '[]']) {
+            const reported: unknown[] = [];
+            const deletes: CacheKey[] = [];
+            let entry: CacheEntry | undefined = { value: document, stamp: 1, expiresAt: Date.now() + 60_000, scope: 'private' };
+            const store: ResponseCacheStore = {
+                get: () => entry,
+                set: () => 1,
+                delete: key => {
+                    deletes.push(key);
+                    entry = undefined;
+                },
+                evict: () => {},
+                clear: () => {}
+            };
+            const cache = new ClientResponseCache(store, true, error => reported.push(error));
+            expect(await cache.read('tools/list')).toBeUndefined();
+            expect(reported).toHaveLength(1);
+            expect(String(reported[0])).toMatch(/not an object/);
+            expect(deletes.length).toBeGreaterThan(0);
+        }
+    });
+
+    test('a tools/list document with non-object elements is reported once per stamp, not thrown per lookup', async () => {
+        const reported: unknown[] = [];
+        const store: ResponseCacheStore = {
+            get: () => ({ value: '{"tools":[null]}', stamp: 9, scope: 'private' as const }),
+            set: () => 1,
+            delete: () => {},
+            evict: () => {},
+            clear: () => {}
+        };
+        const cache = new ClientResponseCache(store, true, error => reported.push(error));
+        await expect(cache.toolDefinition('a')).resolves.toBeUndefined();
+        await expect(cache.toolDefinition('a')).resolves.toBeUndefined();
+        await expect(cache.outputValidator('a', () => undefined)).resolves.toBeUndefined();
+        expect(reported).toHaveLength(2);
+        expect(String(reported[0])).toMatch(/malformed tools array/);
+    });
 });