From d8150f195e1e8b34f2c6a3ec782d06bb5b13cca4 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:06:43 -0400 Subject: [PATCH] refactor(@angular/build): prevent key collisions in cache namespaces Previously, Cache formatted keys by joining the namespace and key with a single colon delimiter (${namespace}:${key}). This allowed potential key collisions if a namespace contained colons (such as 'a' with key 'b:c' versus 'a:b' with key 'c'). While this was not an issue in existing usages because all current namespaces are fixed, colon-free identifiers and keys are hash digests, it presented an architectural risk as caching usages expand. Namespacing is now encapsulated in a dedicated NamespacedCacheStore wrapper that frames keys using length-prefix encoding (::). The length prefix eliminates delimiter ambiguity regardless of what characters appear in the namespace or key. Additionally, Cache has been decoupled from namespace management, simplifying MemoryCache and internal request tracking. --- .../angular/build/src/tools/esbuild/cache.ts | 113 ++++++++++-------- .../build/src/tools/esbuild/cache_spec.ts | 108 ++++++++++++++++- .../src/tools/esbuild/lmdb-cache-store.ts | 4 +- .../src/tools/esbuild/sqlite-cache-store.ts | 4 +- 4 files changed, 174 insertions(+), 55 deletions(-) diff --git a/packages/angular/build/src/tools/esbuild/cache.ts b/packages/angular/build/src/tools/esbuild/cache.ts index 5f1cd0d2927b..40c6787abee3 100644 --- a/packages/angular/build/src/tools/esbuild/cache.ts +++ b/packages/angular/build/src/tools/esbuild/cache.ts @@ -51,6 +51,39 @@ export interface PersistentCacheStore extends CacheStore { close(): void | Promise; } +/** + * A backing data store wrapper that namespaces all keys using length-prefix framing. + * Prevents key collisions between namespaces regardless of characters (such as colons) + * in the namespace or key. + */ +export class NamespacedCacheStore implements CacheStore { + readonly #prefix: string; + + constructor( + private readonly store: CacheStore, + readonly namespace: string, + ) { + this.#prefix = `${namespace.length}:${namespace}:`; + } + + get(key: string): V | undefined | Promise { + return this.store.get(this.#prefix + key); + } + + has(key: string): boolean | Promise { + return this.store.has(this.#prefix + key); + } + + set(key: string, value: V): this | Promise { + const result = this.store.set(this.#prefix + key, value); + if (result instanceof Promise) { + return result.then(() => this); + } + + return this; + } +} + /** * A cache object that allows accessing and storing key/value pairs in * an underlying CacheStore. This class is the primary method for consumers @@ -64,10 +97,7 @@ export class Cache = CacheStore> { // Count the number of active, pending getOrCreate operations per key to avoid memory leaks. readonly #pendingGets = new Map(); - constructor( - protected readonly store: S, - readonly namespace?: string, - ) {} + constructor(protected readonly store: S) {} #incrementWrite(key: string) { // Only track write counts if there is a pending getOrCreate operation active for the key. @@ -77,19 +107,6 @@ export class Cache = CacheStore> { } } - /** - * Prefixes a key with the cache namespace if present. - * @param key A key string to prefix. - * @returns A prefixed key if a namespace is present. Otherwise the provided key. - */ - protected withNamespace(key: string): string { - if (this.namespace) { - return `${this.namespace}:${key}`; - } - - return key; - } - /** * Gets the value associated with a provided key if available. * Otherwise, creates a value using the factory creator function, puts the value @@ -99,27 +116,25 @@ export class Cache = CacheStore> { * @returns A value associated with the provided key. */ async getOrCreate(key: string, creator: () => V | Promise): Promise { - const namespacedKey = this.withNamespace(key); - // 1. If another call is already running the creator for this key, share its promise. - let activeRequest = this.#requests.get(namespacedKey); + let activeRequest = this.#requests.get(key); if (activeRequest !== undefined) { return activeRequest; } // Increment pending gets count to enable write-tracking for this key. - const currentPending = this.#pendingGets.get(namespacedKey) || 0; - this.#pendingGets.set(namespacedKey, currentPending + 1); + const currentPending = this.#pendingGets.get(key) || 0; + this.#pendingGets.set(key, currentPending + 1); try { - const startWriteCount = this.#writeCounts.get(namespacedKey) || 0; + const startWriteCount = this.#writeCounts.get(key) || 0; // 2. Query the backing store. Since store.get can be async, we yield to the event loop. - const value = await this.store.get(namespacedKey); + const value = await this.store.get(key); // If a write (e.g. put) occurred during the store.get await gap, we must abort // the current execution and restart to ensure we return the newly written value. - if ((this.#writeCounts.get(namespacedKey) || 0) !== startWriteCount) { + if ((this.#writeCounts.get(key) || 0) !== startWriteCount) { return this.getOrCreate(key, creator); } @@ -129,7 +144,7 @@ export class Cache = CacheStore> { // 3. Recheck active request after the await gap in case another concurrent call // initiated a creator during the store.get wait. - activeRequest = this.#requests.get(namespacedKey); + activeRequest = this.#requests.get(key); if (activeRequest !== undefined) { return activeRequest; } @@ -139,34 +154,34 @@ export class Cache = CacheStore> { async (newValue) => { // Ensure this request is still the active one before writing back to the store // (prevents overwriting newer data if put() was called before resolution). - if (this.#requests.get(namespacedKey) === activeRequest) { - this.#incrementWrite(namespacedKey); - await this.store.set(namespacedKey, newValue); - this.#requests.delete(namespacedKey); + if (this.#requests.get(key) === activeRequest) { + this.#incrementWrite(key); + await this.store.set(key, newValue); + this.#requests.delete(key); } return newValue; }, (error) => { // Clean up the active request if the creator fails. - if (this.#requests.get(namespacedKey) === activeRequest) { - this.#requests.delete(namespacedKey); + if (this.#requests.get(key) === activeRequest) { + this.#requests.delete(key); } throw error; }, ); - this.#requests.set(namespacedKey, activeRequest); + this.#requests.set(key, activeRequest); return activeRequest; } finally { // Clean up write counts and pending gets once all concurrent gets for this key finish. - const current = this.#pendingGets.get(namespacedKey) || 0; + const current = this.#pendingGets.get(key) || 0; if (current <= 1) { - this.#pendingGets.delete(namespacedKey); - this.#writeCounts.delete(namespacedKey); + this.#pendingGets.delete(key); + this.#writeCounts.delete(key); } else { - this.#pendingGets.set(namespacedKey, current - 1); + this.#pendingGets.set(key, current - 1); } } } @@ -177,7 +192,7 @@ export class Cache = CacheStore> { * @returns A value associated with the provided key if present. Otherwise, `undefined`. */ async get(key: string): Promise { - const value = await this.store.get(this.withNamespace(key)); + const value = await this.store.get(key); return value; } @@ -189,19 +204,18 @@ export class Cache = CacheStore> { * @param value A value to put in the cache. */ async put(key: string, value: V): Promise { - const namespacedKey = this.withNamespace(key); - this.#requests.delete(namespacedKey); - this.#incrementWrite(namespacedKey); - await this.store.set(namespacedKey, value); + this.#requests.delete(key); + this.#incrementWrite(key); + await this.store.set(key, value); } /** - * Clears internal state for a specific namespaced key (requests, write counts, and pending gets). + * Clears internal state for a specific key (requests, write counts, and pending gets). */ - protected deleteInternal(namespacedKey: string): void { - this.#requests.delete(namespacedKey); - this.#writeCounts.delete(namespacedKey); - this.#pendingGets.delete(namespacedKey); + protected deleteInternal(key: string): void { + this.#requests.delete(key); + this.#writeCounts.delete(key); + this.#pendingGets.delete(key); } /** @@ -228,10 +242,9 @@ export class MemoryCache extends Cache> { * @returns True if an element in the Map existed and has been removed, or false if the element does not exist. */ delete(key: string): boolean { - const namespacedKey = this.withNamespace(key); - this.deleteInternal(namespacedKey); + this.deleteInternal(key); - return this.store.delete(namespacedKey); + return this.store.delete(key); } /** diff --git a/packages/angular/build/src/tools/esbuild/cache_spec.ts b/packages/angular/build/src/tools/esbuild/cache_spec.ts index a81bfbd25acd..c5c0b18f6a49 100644 --- a/packages/angular/build/src/tools/esbuild/cache_spec.ts +++ b/packages/angular/build/src/tools/esbuild/cache_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.dev/license */ -import { MemoryCache } from './cache'; +import { Cache, CacheStore, MemoryCache, NamespacedCacheStore } from './cache'; describe('MemoryCache', () => { let cache: MemoryCache; @@ -158,4 +158,110 @@ describe('MemoryCache', () => { it('should return false when deleting a non-existent key', () => { expect(cache.delete('non-existent')).toBeFalse(); }); + + it('should return unencoded keys in entries() and allow deletion', async () => { + await cache.put('component/style.scss', 'content'); + + const entries = Array.from(cache.entries()); + expect(entries).toEqual([['component/style.scss', 'content']]); + + expect(cache.delete(entries[0][0])).toBeTrue(); + expect(await cache.get('component/style.scss')).toBeUndefined(); + }); +}); + +describe('NamespacedCacheStore', () => { + class TestStore implements CacheStore { + readonly map = new Map(); + + get(key: string): string | undefined { + return this.map.get(key); + } + + has(key: string): boolean { + return this.map.has(key); + } + + set(key: string, value: string): this { + this.map.set(key, value); + + return this; + } + } + + let store: TestStore; + + beforeEach(() => { + store = new TestStore(); + }); + + it('should encode namespaced keys with ::', async () => { + const namespacedStore = new NamespacedCacheStore(store, 'test-ns'); + const cache = new Cache(namespacedStore); + await cache.put('my-key', 'my-val'); + + expect(store.map.has('7:test-ns:my-key')).toBeTrue(); + expect(await cache.get('my-key')).toBe('my-val'); + }); + + it('should prevent collisions between namespaces containing colons', async () => { + const cacheA = new Cache(new NamespacedCacheStore(store, 'a')); + const cacheB = new Cache(new NamespacedCacheStore(store, 'a:b')); + + await cacheA.put('b:c', 'val-a'); + await cacheB.put('c', 'val-b'); + + expect(await cacheA.get('b:c')).toBe('val-a'); + expect(await cacheB.get('c')).toBe('val-b'); + expect(store.map.get('1:a:b:c')).toBe('val-a'); + expect(store.map.get('3:a:b:c')).toBe('val-b'); + }); + + it('should encode empty string namespace as 0::', async () => { + const namespacedStore = new NamespacedCacheStore(store, ''); + const cache = new Cache(namespacedStore); + await cache.put('key', 'val'); + + expect(store.map.has('0::key')).toBeTrue(); + expect(await cache.get('key')).toBe('val'); + }); + + it('should forward get, has, and set calls with the namespaced prefix', async () => { + const namespacedStore = new NamespacedCacheStore(store, 'custom'); + await namespacedStore.set('hello', 'world'); + + expect(store.map.has('6:custom:hello')).toBeTrue(); + expect(await namespacedStore.has('hello')).toBeTrue(); + expect(await namespacedStore.get('hello')).toBe('world'); + }); + + it('should return this when the underlying store set is asynchronous', async () => { + class AsyncStore implements CacheStore { + readonly map = new Map(); + + get(key: string): Promise { + return Promise.resolve(this.map.get(key)); + } + + has(key: string): Promise { + return Promise.resolve(this.map.has(key)); + } + + async set(key: string, value: string): Promise { + this.map.set(key, value); + + return this; + } + } + + const asyncStore = new AsyncStore(); + const namespacedStore = new NamespacedCacheStore(asyncStore, 'async-ns'); + const setPromise = namespacedStore.set('foo', 'bar'); + + expect(setPromise instanceof Promise).toBeTrue(); + expect(await setPromise).toBe(namespacedStore); + expect(asyncStore.map.get('8:async-ns:foo')).toBe('bar'); + expect(await namespacedStore.get('foo')).toBe('bar'); + expect(await namespacedStore.has('foo')).toBeTrue(); + }); }); diff --git a/packages/angular/build/src/tools/esbuild/lmdb-cache-store.ts b/packages/angular/build/src/tools/esbuild/lmdb-cache-store.ts index d8f95b8de9da..d7ac3fed49db 100644 --- a/packages/angular/build/src/tools/esbuild/lmdb-cache-store.ts +++ b/packages/angular/build/src/tools/esbuild/lmdb-cache-store.ts @@ -7,7 +7,7 @@ */ import { RootDatabase, open } from 'lmdb'; -import { Cache, PersistentCacheStore } from './cache'; +import { Cache, NamespacedCacheStore, PersistentCacheStore } from './cache'; export class LmdbCacheStore implements PersistentCacheStore { readonly #cacheFileUrl; @@ -46,7 +46,7 @@ export class LmdbCacheStore implements PersistentCacheStore { } createCache(namespace: string): Cache { - return new Cache(this, namespace); + return new Cache(new NamespacedCacheStore(this, namespace)); } async close() { diff --git a/packages/angular/build/src/tools/esbuild/sqlite-cache-store.ts b/packages/angular/build/src/tools/esbuild/sqlite-cache-store.ts index c59fda17b43e..d9273f5816e7 100644 --- a/packages/angular/build/src/tools/esbuild/sqlite-cache-store.ts +++ b/packages/angular/build/src/tools/esbuild/sqlite-cache-store.ts @@ -12,7 +12,7 @@ import { DatabaseSync, StatementSync } from 'node:sqlite'; import { promisify } from 'node:util'; import { deserialize, serialize } from 'node:v8'; import { deflateRaw, inflateRawSync } from 'node:zlib'; -import { Cache, PersistentCacheStore } from './cache'; +import { Cache, NamespacedCacheStore, PersistentCacheStore } from './cache'; const deflateRawAsync = promisify(deflateRaw); @@ -315,7 +315,7 @@ export class SqliteCacheStore implements PersistentCacheStore { } createCache(namespace: string): Cache { - return new Cache(this, namespace); + return new Cache(new NamespacedCacheStore(this, namespace)); } close(): void {